Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Mon, 21 Aug 2023 17:30:21 +0300
Dmitry Baryshkov wrote:
> On Fri, 18 Aug 2023 at 16:55, Pekka Paalanen wrote:
> >
> > On Fri, 18 Aug 2023 14:03:14 +0300
> > Dmitry Baryshkov wrote:
> >
> > > On 18/08/2023 13:51, Pekka Paalanen wrote:
> > > > On Fri, 4 Aug 2023 16:59:00 +0300
> > > > Dmitry Baryshkov wrote:
> > > >
> > > >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick
> > > >> wrote:
> > > >>>
> > > >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > > >>> wrote:
> > >
> > > On Fri, 28 Jul 2023 at 20:03, Jessica Zhang
> > > wrote:
> > > >
> > > > Document and add support for solid_fill property to drm_plane. In
> > > > addition, add support for setting and getting the values for
> > > > solid_fill.
> > > >
> > > > To enable solid fill planes, userspace must assign a property blob
> > > > to
> > > > the "solid_fill" plane property containing the following
> > > > information:
> > > >
> > > > struct drm_mode_solid_fill {
> > > > u32 version;
> > > > u32 r, g, b;
> > > > };
> > > >
> > > > Signed-off-by: Jessica Zhang
> > > > ---
> > > > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> > > > drivers/gpu/drm/drm_atomic_uapi.c | 55
> > > > +++
> > > > drivers/gpu/drm/drm_blend.c | 30 +
> > > > include/drm/drm_blend.h | 1 +
> > > > include/drm/drm_plane.h | 35
> > > >
> > > > include/uapi/drm/drm_mode.h | 24 ++
> > > > 6 files changed, 154 insertions(+)
> > > >
> > >
> > > [skipped most of the patch]
> >
> > ...
> >
> > > >>> Maybe another COLOR_FILL enum value
> > > >>> with alpha might be better? Maybe just doing the alpha via the alpha
> > > >>> property is good enough.
> > > >>
> > > >> One of our customers has a use case for setting the opaque solid fill,
> > > >> while keeping the plane's alpha intact.
> > > >
> > > > Could you explain more about why they must keep plane alpha intact
> > > > instead of reprogramming everything with atomic? Is there some
> > > > combination that just cannot reach the same end result via userspace
> > > > manipulation of the solid fill values with plane alpha?
> > > >
> > > > Or is it a matter of userspace architecture where you have independent
> > > > components responsible for different KMS property values?
> >
> > > The latter one. The goal is to be able to switch between pixel sources
> > > without touching any additional properties (including plane's alpha
> > > value).
> >
> > Sorry, but that does not seem like a good justification for KMS UAPI
> > design.
> >
> > It is even in conflict with how atomic KMS UAPI was designed to work:
> > collect all your changes into a single commit, and push it at once.
> > Here we are talking about separate components changing the different
> > properties of the same KMS plane even. If you want to change both plane
> > opacity and contents, does it mean you need two refresh cycles, one at
> > a time? Could the two components be even racing with each other,
> > stalling each other randomly?
>
> Most likely I was not verbose enough.
>
> We want to setup the blending scene, including the FB and the solid
> fill properties for the plane. FB is set up in the RGBA format, each
> pixel having its own alpha value in addition to the plane's alpha
> value. Then under certain circumstances, the plane gets hidden by the
> solid fill (think of a curtain). We do not want to touch the global
> scene setup (including plane alpha value), just switch the curtain on
> and off.
> I think this plays good enough with the defined plane blending rules,
> where one can use pre-multiplied blending mode or use coverage
> blending mode.
Right, that's what I understood. But this does complicate the KMS UAPI
for something that is well possible and feasible without the added
complication as well.
Is there a hardware or driver reason to avoid touching the global scene
setup? Does something in the hardware or driver work more optimally
that way?
Personally I'd favour simpler UAPI with more complex userspace for
maintainability and testing reasons. I'd also favour UAPI that exposes
common hardware features instead of design driven by userspace
process-internal architecture. There does not seem to be any
functionality or performance reasons to justify adding alpha channel to
the solid fill color.
OTOH, do we know of hardware that does not have separate alpha for the
fill color?
Do we know of hardware that can only do opaque solid fills, meaning no
alpha in the fill color nor for the plane?
What about hardware that has no plane alpha, but does have fill color
alpha?
If the plane has an alpha property, then drivers could implement fill
color alpha by combining the two alpha values before program
Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
On Tue, Aug 15, 2023 at 03:57:09PM -0300, André Almeida wrote:
> Given that prop changes may lead to modesetting, which would defeat the
> fast path of the async flip, refuse any atomic prop change for async
> flips in atomic API. The only exceptions are the framebuffer ID to flip
> to and the mode ID, that could be referring to an identical mode.
FYI, the solid fill series adds an enum drm_plane_pixel_source and and a
new solid fill pixel source. Changing the solid fill color would be
effectively the same as changing the FB_ID. On the other hand, changing
the FB_ID no longer necessarily results in an update when the pixel
source is set to solid fill.
> Signed-off-by: André Almeida
> ---
> v5: no changes
> v4: new patch
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
> drivers/gpu/drm/drm_crtc_internal.h | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 4 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 292e38eb6218..b34e3104afd1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>
> if (!drm_mode_equal(&old_crtc_state->mode,
> &new_crtc_state->mode)) {
> + if (new_crtc_state->async_flip) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
> changes allowed during async flip\n",
> +crtc->base.id, crtc->name);
> + return -EINVAL;
> + }
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a15121e75a0a..6c423a7e8c7b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1006,13 +1006,28 @@ int drm_atomic_connector_commit_dpms(struct
> drm_atomic_state *state,
> return ret;
> }
>
> +static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t
> prop_value,
> + struct drm_property *prop)
> +{
> + if (ret != 0 || old_val != prop_value) {
> + drm_dbg_atomic(prop->dev,
> +"[PROP:%d:%s] No prop can be changed during
> async flip\n",
> +prop->base.id, prop->name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> int drm_atomic_set_property(struct drm_atomic_state *state,
> struct drm_file *file_priv,
> struct drm_mode_object *obj,
> struct drm_property *prop,
> - uint64_t prop_value)
> + uint64_t prop_value,
> + bool async_flip)
> {
> struct drm_mode_object *ref;
> + uint64_t old_val;
> int ret;
>
> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> @@ -1029,6 +1044,13 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + if (async_flip) {
> + ret = drm_atomic_connector_get_property(connector,
> connector_state,
> + prop, &old_val);
> + ret = drm_atomic_check_prop_changes(ret, old_val,
> prop_value, prop);
> + break;
> + }
> +
> ret = drm_atomic_connector_set_property(connector,
> connector_state, file_priv,
> prop, prop_value);
> @@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> case DRM_MODE_OBJECT_CRTC: {
> struct drm_crtc *crtc = obj_to_crtc(obj);
> struct drm_crtc_state *crtc_state;
> + struct drm_mode_config *config = &crtc->dev->mode_config;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state)) {
> @@ -1044,6 +1067,18 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + /*
> + * We allow mode_id changes here for async flips, because we
> + * check later on drm_atomic_helper_check_modeset() callers if
> + * there are modeset changes or they are equal
> + */
> + if (async_flip && prop != config->prop_mode_id) {
> + ret = drm_atomic_crtc_get_property(crtc, crtc_state,
> +
Re: [PATCH v6 6/6] drm/doc: Define KMS atomic state set
On 8/21/23 22:02, André Almeida wrote: > Em 17/08/2023 07:37, Michel Dänzer escreveu: >> On 8/15/23 20:57, André Almeida wrote: >>> From: Pekka Paalanen >>> >>> Specify how the atomic state is maintained between userspace and >>> kernel, plus the special case for async flips. >>> >>> Signed-off-by: Pekka Paalanen >>> Signed-off-by: André Almeida >> >> [...] >> >>> +An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is allowed to >>> +effectively change only the FB_ID property on any planes. No-operation >>> changes >>> +are ignored as always. [...] >> >> During the hackfest in Brno, it was mentioned that a commit which re-sets >> the same FB_ID could actually have an effect with VRR: It could trigger >> scanout of the next frame before vertical blank has reached its maximum >> duration. Some kind of mechanism is required for this in order to allow user >> space to perform low frame rate compensation. >> > > I believe the documentation already addresses that sending redundant > information may not lead to the desired behavior during an async flip. Do you > think adding a note about using the same FB_ID would be helpful? Maybe not. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
