[RFC,v5,02/10] drm: Introduce solid fill DRM plane property

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 <quic_jesszhan@quicinc.com>
---
 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(+)

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 <quic_jesszhan@quicinc.com>
> ---
>   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.

> +
> +		/* 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,
> +				       blob_version);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	drm_property_blob_put(state->solid_fill_blob);
> +
> +	if (blob)
> +		state->solid_fill_blob = drm_property_blob_get(blob);
> +	else
> +		state->solid_fill_blob = NULL;
> +
> +	return 0;
> +}
> +
>   static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>   				   struct drm_crtc *crtc, s32 __user *fence_ptr)
>   {
> @@ -546,6 +591,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->src_h = val;
>   	} else if (property == plane->pixel_source_property) {
>   		state->pixel_source = 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;

Bonus point: dropping version from SOLID_FILL would allow us to use 
drm_atomic_replace_property_blob_from_id() here.

>   	} else if (property == plane->alpha_property) {
>   		state->alpha = val;
>   	} else if (property == plane->blend_mode_property) {
> @@ -620,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   		*val = state->src_h;
>   	} else if (property == plane->pixel_source_property) {
>   		*val = state->pixel_source;
> +	} 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 c500310a3d09..c632dfcd8a9b 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,10 @@
>    *	"FB":
>    *		Framebuffer source set by the "FB_ID" property.
>    *
> + * 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.
> + *
>    * 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).
> @@ -700,3 +704,29 @@ int drm_plane_create_pixel_source_property(struct drm_plane *plane,
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_plane_create_pixel_source_property);
> +
> +/**
> + * drm_plane_create_solid_fill_property - create a new solid_fill property
> + * @plane: drm plane
> + *
> + * This creates a new property blob that holds pixel data for solid fill planes.
> + * The property is exposed to userspace as a property blob called "solid_fill".
> + *
> + * For information on what the blob contains, see `drm_mode_solid_fill`.
> + */
> +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 122bbfbaae33..e7158fbee389 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -60,4 +60,5 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>   					 unsigned int supported_modes);
>   int drm_plane_create_pixel_source_property(struct drm_plane *plane,
>   					   unsigned long extra_sources);
> +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 89508b4dea4a..abf1458fa099 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -46,6 +46,17 @@ enum drm_plane_pixel_source {
>   	DRM_PLANE_PIXEL_SOURCE_MAX
>   };
>   
> +/**
> + * 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
>    *
> @@ -130,6 +141,23 @@ struct drm_plane_state {
>   	 */
>   	enum drm_plane_pixel_source pixel_source;
>   
> +	/**
> +	 * @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
> @@ -720,6 +748,13 @@ struct drm_plane {
>   	 */
>   	struct drm_property *pixel_source_property;
>   
> +	/**
> +	 * @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
> 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;
> +};
> +
> +
>   struct drm_mode_card_res {
>   	__u64 fb_id_ptr;
>   	__u64 crtc_id_ptr;
>

On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> ---
>  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.

On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com>
> > ---
> >   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,
> > +                                    blob_version);
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     drm_property_blob_put(state->solid_fill_blob);
> > +
> > +     if (blob)
> > +             state->solid_fill_blob = drm_property_blob_get(blob);
> > +     else
> > +             state->solid_fill_blob = NULL;
> > +
> > +     return 0;
> > +}
> > +
> >   static void set_out_fence_for_crtc(struct drm_atomic_state *state,
> >                                  struct drm_crtc *crtc, s32 __user *fence_ptr)
> >   {
> > @@ -546,6 +591,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >               state->src_h = val;
> >       } else if (property == plane->pixel_source_property) {
> >               state->pixel_source = 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;
>
> Bonus point: dropping version from SOLID_FILL would allow us to use
> drm_atomic_replace_property_blob_from_id() here.
>
> >       } else if (property == plane->alpha_property) {
> >               state->alpha = val;
> >       } else if (property == plane->blend_mode_property) {
> > @@ -620,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >               *val = state->src_h;
> >       } else if (property == plane->pixel_source_property) {
> >               *val = state->pixel_source;
> > +     } 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 c500310a3d09..c632dfcd8a9b 100644
> > --- a/drivers/gpu/drm/drm_blend.c
> > +++ b/drivers/gpu/drm/drm_blend.c
> > @@ -200,6 +200,10 @@
> >    *  "FB":
> >    *          Framebuffer source set by the "FB_ID" property.
> >    *
> > + * 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.
> > + *
> >    * 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).
> > @@ -700,3 +704,29 @@ int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> >       return 0;
> >   }
> >   EXPORT_SYMBOL(drm_plane_create_pixel_source_property);
> > +
> > +/**
> > + * drm_plane_create_solid_fill_property - create a new solid_fill property
> > + * @plane: drm plane
> > + *
> > + * This creates a new property blob that holds pixel data for solid fill planes.
> > + * The property is exposed to userspace as a property blob called "solid_fill".
> > + *
> > + * For information on what the blob contains, see `drm_mode_solid_fill`.
> > + */
> > +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 122bbfbaae33..e7158fbee389 100644
> > --- a/include/drm/drm_blend.h
> > +++ b/include/drm/drm_blend.h
> > @@ -60,4 +60,5 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >                                        unsigned int supported_modes);
> >   int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> >                                          unsigned long extra_sources);
> > +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 89508b4dea4a..abf1458fa099 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -46,6 +46,17 @@ enum drm_plane_pixel_source {
> >       DRM_PLANE_PIXEL_SOURCE_MAX
> >   };
> >
> > +/**
> > + * 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
> >    *
> > @@ -130,6 +141,23 @@ struct drm_plane_state {
> >        */
> >       enum drm_plane_pixel_source pixel_source;
> >
> > +     /**
> > +      * @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
> > @@ -720,6 +748,13 @@ struct drm_plane {
> >        */
> >       struct drm_property *pixel_source_property;
> >
> > +     /**
> > +      * @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
> > 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;
> > +};
> > +
> > +
> >   struct drm_mode_card_res {
> >       __u64 fb_id_ptr;
> >       __u64 crtc_id_ptr;
> >
>
> --
> With best wishes
> Dmitry
>

On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> > ---
> >  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
>

On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:
>
> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> > > ---
> > >  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.

On 8/4/2023 6:40 AM, Sebastian Wick wrote:
> On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com>
>>> ---
>>>    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.

Acked.

Thanks,

Jessica Zhang

> 
>>
>>> +
>>> +             /* 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,
>>> +                                    blob_version);
>>> +                     return -EINVAL;
>>> +             }
>>> +     }
>>> +
>>> +     drm_property_blob_put(state->solid_fill_blob);
>>> +
>>> +     if (blob)
>>> +             state->solid_fill_blob = drm_property_blob_get(blob);
>>> +     else
>>> +             state->solid_fill_blob = NULL;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>    static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>>>                                   struct drm_crtc *crtc, s32 __user *fence_ptr)
>>>    {
>>> @@ -546,6 +591,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>                state->src_h = val;
>>>        } else if (property == plane->pixel_source_property) {
>>>                state->pixel_source = 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;
>>
>> Bonus point: dropping version from SOLID_FILL would allow us to use
>> drm_atomic_replace_property_blob_from_id() here.
>>
>>>        } else if (property == plane->alpha_property) {
>>>                state->alpha = val;
>>>        } else if (property == plane->blend_mode_property) {
>>> @@ -620,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>                *val = state->src_h;
>>>        } else if (property == plane->pixel_source_property) {
>>>                *val = state->pixel_source;
>>> +     } 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 c500310a3d09..c632dfcd8a9b 100644
>>> --- a/drivers/gpu/drm/drm_blend.c
>>> +++ b/drivers/gpu/drm/drm_blend.c
>>> @@ -200,6 +200,10 @@
>>>     *  "FB":
>>>     *          Framebuffer source set by the "FB_ID" property.
>>>     *
>>> + * 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.
>>> + *
>>>     * 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).
>>> @@ -700,3 +704,29 @@ int drm_plane_create_pixel_source_property(struct drm_plane *plane,
>>>        return 0;
>>>    }
>>>    EXPORT_SYMBOL(drm_plane_create_pixel_source_property);
>>> +
>>> +/**
>>> + * drm_plane_create_solid_fill_property - create a new solid_fill property
>>> + * @plane: drm plane
>>> + *
>>> + * This creates a new property blob that holds pixel data for solid fill planes.
>>> + * The property is exposed to userspace as a property blob called "solid_fill".
>>> + *
>>> + * For information on what the blob contains, see `drm_mode_solid_fill`.
>>> + */
>>> +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 122bbfbaae33..e7158fbee389 100644
>>> --- a/include/drm/drm_blend.h
>>> +++ b/include/drm/drm_blend.h
>>> @@ -60,4 +60,5 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>>>                                         unsigned int supported_modes);
>>>    int drm_plane_create_pixel_source_property(struct drm_plane *plane,
>>>                                           unsigned long extra_sources);
>>> +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 89508b4dea4a..abf1458fa099 100644
>>> --- a/include/drm/drm_plane.h
>>> +++ b/include/drm/drm_plane.h
>>> @@ -46,6 +46,17 @@ enum drm_plane_pixel_source {
>>>        DRM_PLANE_PIXEL_SOURCE_MAX
>>>    };
>>>
>>> +/**
>>> + * 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
>>>     *
>>> @@ -130,6 +141,23 @@ struct drm_plane_state {
>>>         */
>>>        enum drm_plane_pixel_source pixel_source;
>>>
>>> +     /**
>>> +      * @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
>>> @@ -720,6 +748,13 @@ struct drm_plane {
>>>         */
>>>        struct drm_property *pixel_source_property;
>>>
>>> +     /**
>>> +      * @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
>>> 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;
>>> +};
>>> +
>>> +
>>>    struct drm_mode_card_res {
>>>        __u64 fb_id_ptr;
>>>        __u64 crtc_id_ptr;
>>>
>>
>> --
>> With best wishes
>> Dmitry
>>
>

On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
>> ---
>>   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.

Hi Dmitry,

I don't really agree that adding a solid fill alpha would be a good 
idea. Since the intent behind solid fill is to have a single color for 
the entire plane, I think it makes more sense to have solid fill rely on 
the global plane alpha.

As stated in earlier discussions, I think having both a solid_fill.alpha 
and a plane_state.alpha would be redundant and serve to confuse the user 
as to which one to set.

Thanks,

Jessica Zhang

> 
> -- 
> With best wishes
> Dmitry

On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
>>> ---
>>>   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.
>
>Hi Dmitry,
>
>I don't really agree that adding a solid fill alpha would be a good idea. Since the intent behind solid fill is to have a single color for the entire plane, I think it makes more sense to have solid fill rely on the global plane alpha.
>
>As stated in earlier discussions, I think having both a solid_fill.alpha and a plane_state.alpha would be redundant and serve to confuse the user as to which one to set.

That depends on the blending mode: in Coverage mode one has independent plane and contents alpha values. And I consider alpha value to be a part of the colour in the rgba/bgra modes.


>
>Thanks,
>
>Jessica Zhang
>
>> 
>> -- 
>> With best wishes
>> Dmitry

On 8/7/2023 6:07 PM, Dmitry Baryshkov wrote:
> 
> 
> On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>> On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    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.
>>
>> Hi Dmitry,
>>
>> I don't really agree that adding a solid fill alpha would be a good idea. Since the intent behind solid fill is to have a single color for the entire plane, I think it makes more sense to have solid fill rely on the global plane alpha.
>>
>> As stated in earlier discussions, I think having both a solid_fill.alpha and a plane_state.alpha would be redundant and serve to confuse the user as to which one to set.
> 
> That depends on the blending mode: in Coverage mode one has independent plane and contents alpha values. And I consider alpha value to be a part of the colour in the rgba/bgra modes.

Acked -- taking Sebastian's concern into consideration, I think I'll 
have "PIXEL_SOURCE_SOLID_FILL_RGB" and add a separate 
"PIXEL_SOURCE_SOLID_FILL_RGBA".

Thanks,

Jessica Zhang

> 
> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
> 
> -- 
> With best wishes
> Dmitry

On Fri, 4 Aug 2023 16:59:00 +0300
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:
> >
> > On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:  
> > >
> > > On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> > > > ---
> > > >  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.

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?


Thanks,
pq

On 18/08/2023 13:51, Pekka Paalanen wrote:
> On Fri, 4 Aug 2023 16:59:00 +0300
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
>> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:
>>>
>>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
>>>>> ---
>>>>>   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.
> 
> 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).

On Fri, 18 Aug 2023 14:03:14 +0300
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On 18/08/2023 13:51, Pekka Paalanen wrote:
> > On Fri, 4 Aug 2023 16:59:00 +0300
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >   
> >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:  
> >>>
> >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> >>> <dmitry.baryshkov@linaro.org> wrote:  
> >>>>
> >>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> >>>>> ---
> >>>>>   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?


Thanks,
pq

On Fri, 18 Aug 2023 at 16:55, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Fri, 18 Aug 2023 14:03:14 +0300
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On 18/08/2023 13:51, Pekka Paalanen wrote:
> > > On Fri, 4 Aug 2023 16:59:00 +0300
> > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > >
> > >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > >>>
> > >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > >>> <dmitry.baryshkov@linaro.org> wrote:
> > >>>>
> > >>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> > >>>>> ---
> > >>>>>   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.



--
With best wishes
Dmitry

On Mon, 21 Aug 2023 17:30:21 +0300
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Fri, 18 Aug 2023 at 16:55, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Fri, 18 Aug 2023 14:03:14 +0300
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >  
> > > On 18/08/2023 13:51, Pekka Paalanen wrote:  
> > > > On Fri, 4 Aug 2023 16:59:00 +0300
> > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > > >  
> > > >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick <sebastian.wick@redhat.com> wrote:  
> > > >>>
> > > >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > > >>> <dmitry.baryshkov@linaro.org> wrote:  
> > > >>>>
> > > >>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> > > >>>>> ---
> > > >>>>>   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 programming the
hardware. If there is no plane alpha, but there is fill color alpha, it
would be really awkward to expose a fake plane alpha because it would
only work with fill color.

Assuming that all those combinations exist in hardware, then separate
fill colors without and with alpha make sense, advertised independently.


Thanks,
pq

On 8/8/2023 3:57 PM, Jessica Zhang wrote:
> 
> 
> On 8/7/2023 6:07 PM, Dmitry Baryshkov wrote:
>>
>>
>> On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang 
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>> On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
>>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang 
>>>> <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
>>>>> ---
>>>>>    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.
>>>
>>> Hi Dmitry,
>>>
>>> I don't really agree that adding a solid fill alpha would be a good 
>>> idea. Since the intent behind solid fill is to have a single color 
>>> for the entire plane, I think it makes more sense to have solid fill 
>>> rely on the global plane alpha.
>>>
>>> As stated in earlier discussions, I think having both a 
>>> solid_fill.alpha and a plane_state.alpha would be redundant and serve 
>>> to confuse the user as to which one to set.
>>
>> That depends on the blending mode: in Coverage mode one has 
>> independent plane and contents alpha values. And I consider alpha 
>> value to be a part of the colour in the rgba/bgra modes.
> 
> Acked -- taking Sebastian's concern into consideration, I think I'll 
> have "PIXEL_SOURCE_SOLID_FILL_RGB" and add a separate 
> "PIXEL_SOURCE_SOLID_FILL_RGBA".

Hi Dmitry,

Since it looks like there's still some ongoing discussion with Pekka 
about whether to support an RGBA solid fill source, I'll just leave a 
note to add an RGBA source in the future.

Thanks,

Jessica Zhang

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>
>> -- 
>> With best wishes
>> Dmitry

On Tue, 29 Aug 2023 at 02:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 8/8/2023 3:57 PM, Jessica Zhang wrote:
> >
> >
> > On 8/7/2023 6:07 PM, Dmitry Baryshkov wrote:
> >>
> >>
> >> On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang
> >> <quic_jesszhan@quicinc.com> wrote:
> >>>
> >>>
> >>> On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
> >>>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang
> >>>> <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com>
> >>>>> ---
> >>>>>    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.
> >>>
> >>> Hi Dmitry,
> >>>
> >>> I don't really agree that adding a solid fill alpha would be a good
> >>> idea. Since the intent behind solid fill is to have a single color
> >>> for the entire plane, I think it makes more sense to have solid fill
> >>> rely on the global plane alpha.
> >>>
> >>> As stated in earlier discussions, I think having both a
> >>> solid_fill.alpha and a plane_state.alpha would be redundant and serve
> >>> to confuse the user as to which one to set.
> >>
> >> That depends on the blending mode: in Coverage mode one has
> >> independent plane and contents alpha values. And I consider alpha
> >> value to be a part of the colour in the rgba/bgra modes.
> >
> > Acked -- taking Sebastian's concern into consideration, I think I'll
> > have "PIXEL_SOURCE_SOLID_FILL_RGB" and add a separate
> > "PIXEL_SOURCE_SOLID_FILL_RGBA".
>
> Hi Dmitry,
>
> Since it looks like there's still some ongoing discussion with Pekka
> about whether to support an RGBA solid fill source, I'll just leave a
> note to add an RGBA source in the future.

Sure! Let's get the basic framework landed. After that we can extend
it with RGBA, if that seems sensible.

>
> Thanks,
>
> Jessica Zhang
>
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> >>
> >>
> >>>
> >>> Thanks,
> >>>
> >>> Jessica Zhang
> >>>
> >>>>
> >>>> --
> >>>> With best wishes
> >>>> Dmitry
> >>
> >> --
> >> With best wishes
> >> Dmitry