by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property

On Fri, 30 Jun 2023 03:42:28 +0300
Dmitry Baryshkov <[email protected]> wrote:

> On 30/06/2023 03:25, Jessica Zhang wrote:
> > Add support for pixel_source property to drm_plane and related
> > documentation.
> >
> > This enum property will allow user to specify a pixel source for the
> > plane. Possible pixel sources will be defined in the
> > drm_plane_pixel_source enum.
> >
> > The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> > DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
>
> I think, this should come before the solid fill property addition. First
> you tell that there is a possibility to define other pixel sources, then
> additional sources are defined.

Hi,

that would be logical indeed.

> >
> > Signed-off-by: Jessica Zhang <[email protected]>
> > ---
> > drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> > drivers/gpu/drm/drm_blend.c | 81 +++++++++++++++++++++++++++++++
> > include/drm/drm_blend.h | 2 +
> > include/drm/drm_plane.h | 21 ++++++++
> > 5 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index fe14be2bd2b2..86fb876efbe6 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
> >
> > plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> > plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> > + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >
> > if (plane_state->solid_fill_blob) {
> > drm_property_blob_put(plane_state->solid_fill_blob);
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index a28b4ee79444..6e59c21af66b 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> > drm_property_blob_put(solid_fill);
> >
> > return ret;
> > + } else if (property == plane->pixel_source_property) {
> > + state->pixel_source = val;
> > } else if (property == plane->alpha_property) {
> > state->alpha = val;
> > } else if (property == plane->blend_mode_property) {
>
> I think, it was pointed out in the discussion that drm_mode_setplane()
> (a pre-atomic IOCTL to turn the plane on and off) should also reset
> pixel_source to FB.
>
> > @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > } else if (property == plane->solid_fill_property) {
> > *val = state->solid_fill_blob ?
> > state->solid_fill_blob->base.id : 0;
> > + } else if (property == plane->pixel_source_property) {
> > + *val = state->pixel_source;
> > } else if (property == plane->alpha_property) {
> > *val = state->alpha;
> > } else if (property == plane->blend_mode_property) {
> > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> > index 38c3c5d6453a..8c100a957ee2 100644
> > --- a/drivers/gpu/drm/drm_blend.c
> > +++ b/drivers/gpu/drm/drm_blend.c
> > @@ -189,6 +189,18 @@
> > * solid_fill is set up with drm_plane_create_solid_fill_property(). It
> > * contains pixel data that drivers can use to fill a plane.
> > *
> > + * pixel_source:
> > + * pixel_source is set up with drm_plane_create_pixel_source_property().
> > + * It is used to toggle the source of pixel data for the plane.

Other sources than the selected one are ignored?

> > + *
> > + * Possible values:

Wouldn't hurt to explicitly mention here that this is an enum.

> > + *
> > + * "FB":
> > + * Framebuffer source
> > + *
> > + * "COLOR":
> > + * solid_fill source

I think these two should be more explicit. Framebuffer source is the
usual source from the property "FB_ID". Solid fill source comes from
the property "solid_fill".

Why "COLOR" and not, say, "SOLID_FILL"?

> > + *
> > * Note that all the property extensions described here apply either to the
> > * plane or the CRTC (e.g. for the background color, which currently is not
> > * exposed and assumed to be black).
> > @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct drm_plane *plane)
> > return 0;
> > }
> > EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
> > +
> > +/**
> > + * drm_plane_create_pixel_source_property - create a new pixel source property
> > + * @plane: drm plane
> > + * @supported_sources: bitmask of supported pixel_sources for the driver (NOT
> > + * including DRM_PLANE_PIXEL_SOURCE_FB, as it will be supported
> > + * by default).
>
> I'd say this is too strong. I'd suggest either renaming this to
> extra_sources (mentioning that FB is enabled for all the planes) or
> allowing any source bitmask (mentioning that FB should be enabled by the
> caller, unless there is a good reason not to do so).

Right. I don't see any problem with having planes of type OVERLAY that
support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I
would expect to always support at least FB.

Atomic userspace is prepared to have an OVERLAY plane fail for any
arbitrary reason. Legacy userspace probably should not ever see a plane
that does not support FB.

> > + *
> > + * This creates a new property describing the current source of pixel data for the
> > + * plane.
> > + *
> > + * The property is exposed to userspace as an enumeration property called
> > + * "pixel_source" and has the following enumeration values:
> > + *
> > + * "FB":
> > + * Framebuffer pixel source
> > + *
> > + * "COLOR":
> > + * Solid fill color pixel source
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> > + unsigned int supported_sources)
> > +{
> > + struct drm_device *dev = plane->dev;
> > + struct drm_property *prop;
> > + const struct drm_prop_enum_list enum_list[] = {
> > + { DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
> > + { DRM_PLANE_PIXEL_SOURCE_COLOR, "COLOR" },
> > + };
> > + unsigned int valid_source_mask = BIT(DRM_PLANE_PIXEL_SOURCE_FB) |
> > + BIT(DRM_PLANE_PIXEL_SOURCE_COLOR);
>
>
> static const?
>
> > + int i;
> > +
> > + /* FB is supported by default */
> > + supported_sources |= BIT(DRM_PLANE_PIXEL_SOURCE_FB);
> > +
> > + if (WARN_ON(supported_sources & ~valid_source_mask))
> > + return -EINVAL;
> > +
> > + prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "pixel_source",

Shouldn't this be an atomic prop?

> > + hweight32(supported_sources));
> > +
> > + if (!prop)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < ARRAY_SIZE(enum_list); i++) {
> > + int ret;
> > +
> > + if (!(BIT(enum_list[i].type) & supported_sources))
>
> test_bit?
>
> > + continue;
> > +
> > + ret = drm_property_add_enum(prop, enum_list[i].type, enum_list[i].name);
> > +
>
> No need for an empty line in such cases. Please drop it.
>
> > + if (ret) {
> > + drm_property_destroy(dev, prop);
> > +
> > + return ret;
> > + }
> > + }
> > +
> > + drm_object_attach_property(&plane->base, prop, DRM_PLANE_PIXEL_SOURCE_FB);
> > + plane->pixel_source_property = prop;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_plane_create_pixel_source_property);
> > diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> > index 0338a860b9c8..31af7cfa5b1b 100644
> > --- a/include/drm/drm_blend.h
> > +++ b/include/drm/drm_blend.h
> > @@ -59,4 +59,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
> > int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> > unsigned int supported_modes);
> > int drm_plane_create_solid_fill_property(struct drm_plane *plane);
> > +int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> > + unsigned int supported_sources);
> > #endif
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index f6ab313cb83e..73fb6cf8a5d9 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -59,6 +59,12 @@ struct drm_solid_fill {
> > uint32_t b;
> > };
> >
> > +enum drm_plane_pixel_source {
> > + DRM_PLANE_PIXEL_SOURCE_FB,
> > + DRM_PLANE_PIXEL_SOURCE_COLOR,
> > + DRM_PLANE_PIXEL_SOURCE_MAX
> > +};

Just to be very clear that I'm not confusing you with my comment about
UAPI headers in the previous patch, this enum is already in a good
place. It does not belong in a UAPI header, because userspace
recognises enum values by the name string.

Thanks,
pq

> > +
> > /**
> > * struct drm_plane_state - mutable plane state
> > *
> > @@ -152,6 +158,14 @@ struct drm_plane_state {
> > */
> > struct drm_solid_fill solid_fill;
> >
> > + /*
> > + * @pixel_source:
> > + *
> > + * Source of pixel information for the plane. See
> > + * drm_plane_create_pixel_source_property() for more details.
> > + */
> > + enum drm_plane_pixel_source pixel_source;
> > +
> > /**
> > * @alpha:
> > * Opacity of the plane with 0 as completely transparent and 0xffff as
> > @@ -742,6 +756,13 @@ struct drm_plane {
> > */
> > struct drm_property *solid_fill_property;
> >
> > + /*
> > + * @pixel_source_property:
> > + * Optional pixel_source property for this plane. See
> > + * drm_plane_create_pixel_source_property().
> > + */
> > + struct drm_property *pixel_source_property;
> > +
> > /**
> > * @alpha_property:
> > * Optional alpha property for this plane. See
> >
>

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2023-06-30 08:47:16

On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
> On Thu, 29 Jun 2023 17:25:00 -0700
> Jessica Zhang <[email protected]> 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_solid_fill_info {
>> u8 version;
>> u32 r, g, b;
>> };
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>
> Hi Jessica,
>
> I've left some general UAPI related comments here.
>
>> ---
>> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++
>> drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++
>> include/drm/drm_blend.h | 1 +
>> include/drm/drm_plane.h | 43 ++++++++++++++++++++++++
>> 5 files changed, 141 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>> index 784e63d70a42..fe14be2bd2b2 100644
>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>
>> + if (plane_state->solid_fill_blob) {
>> + drm_property_blob_put(plane_state->solid_fill_blob);
>> + plane_state->solid_fill_blob = NULL;
>> + }
>> +
>> if (plane->color_encoding_property) {
>> if (!drm_object_property_get_default_value(&plane->base,
>> plane->color_encoding_property,
>> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>> if (state->fb)
>> drm_framebuffer_get(state->fb);
>>
>> + if (state->solid_fill_blob)
>> + drm_property_blob_get(state->solid_fill_blob);
>> +
>> state->fence = NULL;
>> state->commit = NULL;
>> state->fb_damage_clips = NULL;
>> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>> drm_crtc_commit_put(state->commit);
>>
>> drm_property_blob_put(state->fb_damage_clips);
>> + drm_property_blob_put(state->solid_fill_blob);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index d867e7f9f2cd..a28b4ee79444 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>> }
>> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>
>> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
>> + struct drm_property_blob *blob)
>> +{
>> + int ret = 0;
>> + int blob_version;
>> +
>> + if (blob == state->solid_fill_blob)
>> + return 0;
>> +
>> + drm_property_blob_put(state->solid_fill_blob);
>> + state->solid_fill_blob = NULL;
>
> Is it ok to destroy old state before it is guaranteed that the new
> state is accepted?

Hi Pekka,

Good point. I'll change this behavior so that an error case will keep
the old solid_fill_blob value.

>
>> +
>> + memset(&state->solid_fill, 0, sizeof(state->solid_fill));
>> +
>> + if (blob) {
>> + struct drm_solid_fill_info *user_info = (struct drm_solid_fill_info *)blob->data;
>> +
>> + if (blob->length != sizeof(struct drm_solid_fill_info)) {
>> + drm_dbg_atomic(state->plane->dev,
>> + "[PLANE:%d:%s] bad solid fill blob length: %zu\n",
>> + state->plane->base.id, state->plane->name,
>> + blob->length);
>> + return -EINVAL;
>> + }
>> +
>> + blob_version = user_info->version;
>> +
>> + /* Add more versions if necessary */
>> + if (blob_version == 1) {
>> + state->solid_fill.r = user_info->r;
>> + state->solid_fill.g = user_info->g;
>> + state->solid_fill.b = user_info->b;
>> + } else {
>> + drm_dbg_atomic(state->plane->dev,
>> + "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
>> + state->plane->base.id, state->plane->name,
>> + ret);
>> + return -EINVAL;
>
> Btw. how does the atomic check machinery work here?
>
> I expect that a TEST_ONLY atomic commit will do all the above checks
> and return failure if anything is not right. Right?

Correct, drm_atomic_set_property() will still be called for a TEST_ONLY
commit, so these checks will still happen and return an error (or set
the property) when appropriate.

>
>> + }
>> + state->solid_fill_blob = drm_property_blob_get(blob);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>> struct drm_crtc *crtc, s32 __user *fence_ptr)
>> {
>> @@ -544,6 +589,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>> state->src_w = val;
>> } else if (property == config->prop_src_h) {
>> state->src_h = val;
>> + } else if (property == plane->solid_fill_property) {
>> + struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
>> +
>> + ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
>> + drm_property_blob_put(solid_fill);
>> +
>> + return ret;
>> } else if (property == plane->alpha_property) {
>> state->alpha = val;
>> } else if (property == plane->blend_mode_property) {
>> @@ -616,6 +668,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>> *val = state->src_w;
>> } else if (property == config->prop_src_h) {
>> *val = state->src_h;
>> + } else if (property == plane->solid_fill_property) {
>> + *val = state->solid_fill_blob ?
>> + state->solid_fill_blob->base.id : 0;
>> } else if (property == plane->alpha_property) {
>> *val = state->alpha;
>> } else if (property == plane->blend_mode_property) {
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index 6e74de833466..38c3c5d6453a 100644
>> --- a/drivers/gpu/drm/drm_blend.c
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -185,6 +185,10 @@
>> * plane does not expose the "alpha" property, then this is
>> * assumed to be 1.0
>> *
>> + * solid_fill:
>> + * solid_fill is set up with drm_plane_create_solid_fill_property(). It
>> + * contains pixel data that drivers can use to fill a plane.
>
> This is a nice start, but I feel it needs to explain much more about
> how userspace should go about making use of this.
>
> Yeah, the pixel_source property comes in the next patch, but I feel
> that it is harder to review if the doc is built over multiple patches.
> My personal approach would be to write the doc in full and referring to
> pixel_source property already, and explain in the commit message that
> the next patch will add pixel_source so people don't wonder about
> referring to a non-existing property.
>
> I mean just a reference to pixel_source, and leave the actual
> pixel_source doc for the patch adding the property like it already is.
>
> Dmitry's suggestion of swapping the patch order is good too.

Makes sense. I'll switch the patch order, but will keep this in mind.

>
>> + *
>> * Note that all the property extensions described here apply either to the
>> * plane or the CRTC (e.g. for the background color, which currently is not
>> * exposed and assumed to be black).
>> @@ -615,3 +619,32 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>> return 0;
>> }
>> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
>> +
>> +/**
>> + * drm_plane_create_solid_fill_property - create a new solid_fill property
>> + * @plane: drm plane
>> + *
>> + * This creates a new property that holds pixel data for solid fill planes. This
>> + * property is exposed to userspace as a property blob called "solid_fill".
>> + *
>> + * For information on what the blob contains, see `drm_solid_fill_info`.
>
> I think you should be more explicit here. For example: the blob must
> contain exactly one struct drm_solid_fill_info.
>
> It's better to put this content spec with the UAPI doc rather than in this
> kerner-internal function doc that userspace programmers won't know to
> look at.

Acked.

>
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane)
>> +{
>> + struct drm_property *prop;
>> +
>> + prop = drm_property_create(plane->dev,
>> + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
>> + "solid_fill", 0);
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + drm_object_attach_property(&plane->base, prop, 0);
>> + plane->solid_fill_property = prop;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>> index 88bdfec3bd88..0338a860b9c8 100644
>> --- a/include/drm/drm_blend.h
>> +++ b/include/drm/drm_blend.h
>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>> struct drm_atomic_state *state);
>> int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>> unsigned int supported_modes);
>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>> #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 51291983ea44..f6ab313cb83e 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>> DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>> };
>>
>> +/**
>> + * struct drm_solid_fill_info - User info for solid fill planes
>> + */
>> +struct drm_solid_fill_info {
>> + __u8 version;
>> + __u32 r, g, b;
>> +};
>
> Shouldn't UAPI structs be in UAPI headers?

Acked, will move this to uapi/drm_mode.h

>
> Shouldn't UAPI structs use explicit padding to not leave any gaps when
> it's unavoidable? And the kernel to check that the gaps are indeed
> zeroed?

I don't believe so... From my understanding, padding will be taken care
of by the compiler by default. Looking at the drm_mode_modeinfo UAPI
struct [1], it also doesn't seem to do explicit padding. And the
corresponding set_property() code doesn't seem check the gaps either.

That being said, it's possible that I'm missing something here, so
please let me know if that's the case.

[1]
https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242

>
> It also needs more UAPI doc, like a link to the property doc that uses
> this and an explanation of what the values mean.

Acked.

Thanks,

Jessica Zhang

>
>
> Thanks,
> pq
>
>> +
>> +/**
>> + * struct solid_fill_property - RGB values for solid fill plane
>> + *
>> + * Note: This is the V1 for this feature
>> + */
>> +struct drm_solid_fill {
>> + uint32_t r;
>> + uint32_t g;
>> + uint32_t b;
>> +};
>> +
>> /**
>> * struct drm_plane_state - mutable plane state
>> *
>> @@ -116,6 +135,23 @@ struct drm_plane_state {
>> /** @src_h: height of visible portion of plane (in 16.16) */
>> uint32_t src_h, src_w;
>>
>> + /**
>> + * @solid_fill_blob:
>> + *
>> + * Blob containing relevant information for a solid fill plane
>> + * including pixel format and data. See
>> + * drm_plane_create_solid_fill_property() for more details.
>> + */
>> + struct drm_property_blob *solid_fill_blob;
>> +
>> + /**
>> + * @solid_fill:
>> + *
>> + * Pixel data for solid fill planes. See
>> + * drm_plane_create_solid_fill_property() for more details.
>> + */
>> + struct drm_solid_fill solid_fill;
>> +
>> /**
>> * @alpha:
>> * Opacity of the plane with 0 as completely transparent and 0xffff as
>> @@ -699,6 +735,13 @@ struct drm_plane {
>> */
>> struct drm_plane_state *state;
>>
>> + /*
>> + * @solid_fill_property:
>> + * Optional solid_fill property for this plane. See
>> + * drm_plane_create_solid_fill_property().
>> + */
>> + struct drm_property *solid_fill_property;
>> +
>> /**
>> * @alpha_property:
>> * Optional alpha property for this plane. See
>>
>

2023-07-11 08:01:31

On Tue, 11 Jul 2023 15:42:28 -0700
Abhinav Kumar <[email protected]> wrote:

> On 7/11/2023 3:19 PM, Dmitry Baryshkov wrote:
> > On 12/07/2023 01:07, Jessica Zhang wrote:
> >>
> >>
> >> On 7/10/2023 1:11 PM, Dmitry Baryshkov wrote:
> >>> On 10/07/2023 22:51, Jessica Zhang wrote:
> >>>>
> >>>>
> >>>> On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
> >>>>> On Fri, 30 Jun 2023 03:42:28 +0300
> >>>>> Dmitry Baryshkov <[email protected]> wrote:
> >>>>>
> >>>>>> On 30/06/2023 03:25, Jessica Zhang wrote:
> >>>>>>> Add support for pixel_source property to drm_plane and related
> >>>>>>> documentation.
> >>>>>>>
> >>>>>>> This enum property will allow user to specify a pixel source for the
> >>>>>>> plane. Possible pixel sources will be defined in the
> >>>>>>> drm_plane_pixel_source enum.
> >>>>>>>
> >>>>>>> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> >>>>>>> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
> >>>>>>
> >>>>>> I think, this should come before the solid fill property addition.
> >>>>>> First
> >>>>>> you tell that there is a possibility to define other pixel
> >>>>>> sources, then
> >>>>>> additional sources are defined.
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> that would be logical indeed.
> >>>>
> >>>> Hi Dmitry and Pekka,
> >>>>
> >>>> Sorry for the delay in response, was out of office last week.
> >>>>
> >>>> Acked.
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
> >>>>>>> ---
> >>>>>>>    drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> >>>>>>>    drivers/gpu/drm/drm_atomic_uapi.c         | 4 ++
> >>>>>>>    drivers/gpu/drm/drm_blend.c               | 81
> >>>>>>> +++++++++++++++++++++++++++++++
> >>>>>>>    include/drm/drm_blend.h                  | 2 +
> >>>>>>>    include/drm/drm_plane.h                  | 21 ++++++++
> >>>>>>>    5 files changed, 109 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>>>>> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>>>>> index fe14be2bd2b2..86fb876efbe6 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>>>>> @@ -252,6 +252,7 @@ void
> >>>>>>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state
> >>>>>>> *plane_state,
> >>>>>>>        plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> >>>>>>>        plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >>>>>>> +    plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >>>>>>>        if (plane_state->solid_fill_blob) {
> >>>>>>>            drm_property_blob_put(plane_state->solid_fill_blob);
> >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >>>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
> >>>>>>> index a28b4ee79444..6e59c21af66b 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >>>>>>> @@ -596,6 +596,8 @@ static int
> >>>>>>> drm_atomic_plane_set_property(struct drm_plane *plane,
> >>>>>>>            drm_property_blob_put(solid_fill);
> >>>>>>>            return ret;
> >>>>>>> +    } else if (property == plane->pixel_source_property) {
> >>>>>>> +        state->pixel_source = val;
> >>>>>>>        } else if (property == plane->alpha_property) {
> >>>>>>>            state->alpha = val;
> >>>>>>>        } else if (property == plane->blend_mode_property) {
> >>>>>>
> >>>>>> I think, it was pointed out in the discussion that
> >>>>>> drm_mode_setplane()
> >>>>>> (a pre-atomic IOCTL to turn the plane on and off) should also reset
> >>>>>> pixel_source to FB.
> >>>>
> >>>> I don't remember drm_mode_setplane() being mentioned in the
> >>>> pixel_source discussion... can you share where it was mentioned?
> >>>
> >>> https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/
> >>>
> >>> Let me quote it here:
> >>> "Legacy non-atomic UAPI wrappers can do whatever they want, and program
> >>> any (new) properties they want in order to implement the legacy
> >>> expectations, so that does not seem to be a problem."
> >>>
> >>>
> >>>>
> >>>> I'd prefer to avoid having driver change the pixel_source directly
> >>>> as it could cause some unexpected side effects. In general, I would
> >>>> like userspace to assign the value of pixel_source without driver
> >>>> doing anything "under the hood".
> >>>
> >>> s/driver/drm core/
> >>>
> >>> We have to remain compatible with old userspace, especially with the
> >>> non-atomic one. If the userspace calls
> >>> ioctl(DRM_IOCTL_MODE_SETPLANE), we have to display the specified FB,
> >>> no matter what was the value of PIXEL_SOURCE before this ioctl.
> >>
> >>
> >> Got it, thanks the clarification -- I see your point.
> >>
> >> I'm already setting plane_state->pixel_source to FB in
> >> __drm_atomic_helper_plane_reset() and it seems to me that all drivers
> >> are calling that within their respective plane_funcs->reset().
> >>
> >> Since (as far as I know) reset() is being called for both the atomic
> >> and non-atomic paths, shouldn't that be enough to default pixel_source
> >> to FB for old userspace?
> >
> > No, this will not clean up the state between userspace apps. Currently
> > the rule is simple: call DRM_IOCTL_MODE_SETPLANE, get the image from FB
> > displayed. We should keep it so.
> >
>
> Ok, so you are considering a use-case where we bootup with a userspace
> (which is aware of pixel_source), that one uses the pixel_source to
> switch the property to solid_color and then we kill this userspace and
> bootup one which is unaware of this property and uses
> DRM_IOCTL_MODE_SETPLANE, then we should default back to FB.

Not even that complex. There is no need to reboot and no need to kill
anything to hit this. A simple VT-switch can switch between two
different KMS clients, one could be using atomic with solid_fill, and
the other is an old legacy UAPI user. If the atomic client leaves stuff
up in KMS state, it would be nice if the legacy app still worked.

Or, maybe it's not a VT-switch but switching from, say, a graphical
login manager to a desktop or back. The same thing, different KMS
clients. But in this case it is more likely that userspace follows
common play rules, so it's less likely to have problematic left-over
KMS state.

Or, maybe you simply quit the atomic KMS client and expect fbcon to
successfully take over. I don't know what APIs fbcon uses inside the
kernel, but it definitely should clean up any mess left over by an
atomic (or legacy) KMS client to make sure it shows itself.
Traditionally it has failed to do that though, I don't know if things
are better nowadays (GAMMA_LUT, HDR_OUTPUT_METADATA, probably more).

Switching between two KMS clients is fundamentally problematic. If both
old and new KMS client are atomic, the kernel cannot simply go
resetting any KMS state automatically, because the kernel does not
know which part of state is good to reset and which would just cause
unwanted flicker and delays. That means that it is up to the two atomic
KMS clients to agree on common play rules and not leave funny state up.
IOW, not the kernel's problem, by what I have understood from kernel
developer opinions.

However, legacy KMS clients are different. As legacy clients, they may
not even have access to the properties they might need to clean up
after a previous atomic KMS client. The legacy UAPI is expected to
be slow and glitchy, but also Just Work(TM), so the kernel can reset
everything a legacy KMS client would not have access to when a legacy
KMS client issues drmModeSetPlane or drmModeSetCrtc (pardon for using
libdrm API functions for the ioctls whose names I never memorised).

Thanks,
pq

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature