Re: [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property
On Fri, Jul 28, 2023 at 7:03 PM Jessica Zhang wrote:
>
> Add support for pixel_source property to drm_plane and related
> documentation. In addition, force pixel_source to
> DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
> legacy userspace.
>
> 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_NONE and
> DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_blend.c | 85
> +++
> drivers/gpu/drm/drm_plane.c | 3 ++
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 21
> 6 files changed, 116 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..01638c51ce0a 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->color_encoding_property) {
> if (!drm_object_property_get_default_value(&plane->base,
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..454f980e16c9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -544,6 +544,8 @@ 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->pixel_source_property) {
> + state->pixel_source = val;
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -616,6 +618,8 @@ 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->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 6e74de833466..c500310a3d09 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -185,6 +185,21 @@
> * plane does not expose the "alpha" property, then this is
> * assumed to be 1.0
> *
> + * pixel_source:
> + * pixel_source is set up with drm_plane_create_pixel_source_property().
> + * It is used to toggle the active source of pixel data for the plane.
> + * The plane will only display data from the set pixel_source -- any
> + * data from other sources will be ignored.
> + *
> + * Possible values:
> + *
> + * "NONE":
> + * No active pixel source.
> + * Committing with a NONE pixel source will disable the plane.
> + *
> + * "FB":
> + * Framebuffer source set by the "FB_ID" property.
> + *
> * 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 +630,73 @@ int drm_plane_create_blend_mode_property(struct
> drm_plane *plane,
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +/**
> + * drm_plane_create_pixel_source_property - create a new pixel source
> property
> + * @plane: DRM plane
> + * @extra_sources: Bitmask of additional supported pixel_sources for the
> driver.
> + *DRM_PLANE_PIXEL_SOURCE_FB always be enabled as a supported
> + *source.
> + *
> + * This creates a new property describing the current source of pixel data
> for the
> + * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB
> by default.
> + *
> + * Drivers can set a custom default source by overriding the pixel_source
> value in
> + * drm_plane_funcs.reset()
> + *
> + * The property is exposed to userspace as an enumeration property called
> + * "pixel_source" and has the following enumeration values:
> + *
> + * "NONE":
> + * No active pixel source
> + *
> + * "FB":
> + * Frameb
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
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]
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 43691058d28f..53c8efa5ad7f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> char name[DRM_DISPLAY_MODE_LEN];
> };
>
> +/**
> + * struct drm_mode_solid_fill - User info for solid fill planes
> + *
> + * This is the userspace API solid fill information structure.
> + *
> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> + * property to a blob containing a single drm_mode_solid_fill struct
> populated with an RGB323232
> + * color and setting the pixel source to "SOLID_FILL".
> + *
> + * For information on the plane property, see
> drm_plane_create_solid_fill_property()
> + *
> + * @version: Version of the blob. Currently, there is only support for
> version == 1
> + * @r: Red color value of single pixel
> + * @g: Green color value of single pixel
> + * @b: Blue color value of single pixel
> + */
> +struct drm_mode_solid_fill {
> + __u32 version;
> + __u32 r;
> + __u32 g;
> + __u32 b;
Another thought about the drm_mode_solid_fill uABI. I still think we
should add alpha here. The reason is the following:
It is true that we have drm_plane_state::alpha and the plane's
"alpha" property. However it is documented as "the plane-wide opacity
[...] It can be combined with pixel alpha. The pixel values in the
framebuffers are expected to not be pre-multiplied by the global alpha
associated to the plane.".
I can imagine a use case, when a user might want to enable plane-wide
opacity, set "pixel blend mode" to "Coverage" and then switch between
partially opaque framebuffer and partially opaque solid-fill without
touching the plane's alpha value.
--
With best wishes
Dmitry
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
wrote:
>
> On 28/07/2023 20:02, 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(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> > b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 01638c51ce0a..86fb876efbe6 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> > drm_plane_state *plane_state,
> > 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);
> > + plane_state->solid_fill_blob = NULL;
> > + }
> > +
> > if (plane->color_encoding_property) {
> > if (!drm_object_property_get_default_value(&plane->base,
> >
> > plane->color_encoding_property,
> > @@ -336,6 +341,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;
> > @@ -385,6 +393,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 454f980e16c9..039686c78c2a 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 blob_version;
> > +
> > + if (blob == state->solid_fill_blob)
> > + return 0;
> > +
> > + if (blob) {
> > + struct drm_mode_solid_fill *user_info = (struct
> > drm_mode_solid_fill *)blob->data;
> > +
> > + if (blob->length != sizeof(struct drm_mode_solid_fill)) {
> > + 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;
>
> I remember that we had versioning for quite some time. However it stroke
> me while reviewing that we do not a way to negotiate supported
> SOLID_FILL versions. However as we now have support for different
> pixel_source properties, maybe we can drop version completely. If at
> some point a driver needs to support different kind of SOLID_FILL
> property (consider v2), we can add new pixel source to the enum.
Agreed. Let's drop the versioning.
>
> > +
> > + /* 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] unsupported solid fill
> > version (version=%d)\n",
> > +state->plane->base.id,
> > state->plane->name,
> > +
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
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]
>
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 43691058d28f..53c8efa5ad7f 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> > char name[DRM_DISPLAY_MODE_LEN];
> > };
> >
> > +/**
> > + * struct drm_mode_solid_fill - User info for solid fill planes
> > + *
> > + * This is the userspace API solid fill information structure.
> > + *
> > + * Userspace can enable solid fill planes by assigning the plane
> > "solid_fill"
> > + * property to a blob containing a single drm_mode_solid_fill struct
> > populated with an RGB323232
> > + * color and setting the pixel source to "SOLID_FILL".
> > + *
> > + * For information on the plane property, see
> > drm_plane_create_solid_fill_property()
> > + *
> > + * @version: Version of the blob. Currently, there is only support for
> > version == 1
> > + * @r: Red color value of single pixel
> > + * @g: Green color value of single pixel
> > + * @b: Blue color value of single pixel
> > + */
> > +struct drm_mode_solid_fill {
> > + __u32 version;
> > + __u32 r;
> > + __u32 g;
> > + __u32 b;
>
> Another thought about the drm_mode_solid_fill uABI. I still think we
> should add alpha here. The reason is the following:
>
> It is true that we have drm_plane_state::alpha and the plane's
> "alpha" property. However it is documented as "the plane-wide opacity
> [...] It can be combined with pixel alpha. The pixel values in the
> framebuffers are expected to not be pre-multiplied by the global alpha
> associated to the plane.".
>
> I can imagine a use case, when a user might want to enable plane-wide
> opacity, set "pixel blend mode" to "Coverage" and then switch between
> partially opaque framebuffer and partially opaque solid-fill without
> touching the plane's alpha value.
The only reason I see against this is that there might be some
hardware which supports only RGB but not alpha on planes and they
could then not use this property. Maybe another COLOR_FILL enum value
with alpha might be better? Maybe just doing the alpha via the alpha
property is good enough.
>
> --
> With best wishes
> Dmitry
>
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
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]
> >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 43691058d28f..53c8efa5ad7f 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> > > char name[DRM_DISPLAY_MODE_LEN];
> > > };
> > >
> > > +/**
> > > + * struct drm_mode_solid_fill - User info for solid fill planes
> > > + *
> > > + * This is the userspace API solid fill information structure.
> > > + *
> > > + * Userspace can enable solid fill planes by assigning the plane
> > > "solid_fill"
> > > + * property to a blob containing a single drm_mode_solid_fill struct
> > > populated with an RGB323232
> > > + * color and setting the pixel source to "SOLID_FILL".
> > > + *
> > > + * For information on the plane property, see
> > > drm_plane_create_solid_fill_property()
> > > + *
> > > + * @version: Version of the blob. Currently, there is only support for
> > > version == 1
> > > + * @r: Red color value of single pixel
> > > + * @g: Green color value of single pixel
> > > + * @b: Blue color value of single pixel
> > > + */
> > > +struct drm_mode_solid_fill {
> > > + __u32 version;
> > > + __u32 r;
> > > + __u32 g;
> > > + __u32 b;
> >
> > Another thought about the drm_mode_solid_fill uABI. I still think we
> > should add alpha here. The reason is the following:
> >
> > It is true that we have drm_plane_state::alpha and the plane's
> > "alpha" property. However it is documented as "the plane-wide opacity
> > [...] It can be combined with pixel alpha. The pixel values in the
> > framebuffers are expected to not be pre-multiplied by the global alpha
> > associated to the plane.".
> >
> > I can imagine a use case, when a user might want to enable plane-wide
> > opacity, set "pixel blend mode" to "Coverage" and then switch between
> > partially opaque framebuffer and partially opaque solid-fill without
> > touching the plane's alpha value.
>
> The only reason I see against this is that there might be some
> hardware which supports only RGB but not alpha on planes and they
> could then not use this property.
Fair enough.
> 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.
--
With best wishes
Dmitry
