Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 29/10/2022 01:59, Jessica Zhang wrote:
> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> drm_plane. In addition, add support for setting and getting the values
> of these properties.
> 
> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> represents the format of the color fill. Userspace can set enable solid
> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> framebuffer to NULL.

I suppose that COLOR_FILL should override framebuffer rather than 
requiring that FB is set to NULL. In other words, if color_filL_format 
is non-zero, it would make sense to ignore the FB. Then one can use the 
color_fill_format property to quickly switch between filled plane and 
FB-backed one.

> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c |  8 +++++++
>   drivers/gpu/drm/drm_blend.c       | 38 +++++++++++++++++++++++++++++++
>   include/drm/drm_blend.h           |  2 ++
>   include/drm/drm_plane.h           | 28 +++++++++++++++++++++++
>   4 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 79730fa1dd8e..e1664463fca4 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -544,6 +544,10 @@ 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->color_fill_format_property) {
> +		state->color_fill_format = val;
> +	} else if (property == plane->color_fill_property) {
> +		state->color_fill = val;
>   	} else if (property == plane->alpha_property) {
>   		state->alpha = val;
>   	} else if (property == plane->blend_mode_property) {
> @@ -616,6 +620,10 @@ 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->color_fill_format_property) {
> +		*val = state->color_fill_format;
> +	} else if (property == plane->color_fill_property) {
> +		*val = state->color_fill;
>   	} 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 b4c8cab7158c..b8c2b263fa51 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -616,3 +616,41 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +int drm_plane_create_color_fill_property(struct drm_plane *plane)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "color_fill",
> +					 0, 0xffffffff);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +	plane->color_fill_property = prop;
> +
> +	if (plane->state)
> +		plane->state->color_fill = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_fill_property);
> +
> +int drm_plane_create_color_fill_format_property(struct drm_plane *plane)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "color_fill_format",
> +					 0, 0xffffffff);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +	plane->color_fill_format_property = prop;
> +
> +	if (plane->state)
> +		plane->state->color_fill_format = 0;

Don't you also need to reset these properties in 
__drm_atomic_helper_plane_state_reset() ?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_fill_format_property);
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 88bdfec3bd88..3e96f5e83cce 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -58,4 +58,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   			      struct drm_atomic_state *state);
>   int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>   					 unsigned int supported_modes);
> +int drm_plane_create_color_fill_property(struct drm_plane *plane);
> +int drm_plane_create_color_fill_format_property(struct drm_plane *plane);
>   #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..dcbfdb0e1f71 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -116,6 +116,20 @@ struct drm_plane_state {
>   	/** @src_h: height of visible portion of plane (in 16.16) */
>   	uint32_t src_h, src_w;
>   
> +	/**
> +	 * @color_fill_format:
> +	 * Format of the color fill value.
> +	 */
> +	uint32_t color_fill_format;
> +
> +	/**
> +	 * @color_fill:
> +	 * Fill color of the plane with 0 as black and 0xffffffff as white.
> +	 * Can be set by user by setting the COLOR_FILL property. See
> +	 * drm_plane_create_color_fill_property() for more details.
> +	 */
> +	uint32_t color_fill;
> +
>   	/**
>   	 * @alpha:
>   	 * Opacity of the plane with 0 as completely transparent and 0xffff as
> @@ -699,6 +713,20 @@ struct drm_plane {
>   	 */
>   	struct drm_plane_state *state;
>   
> +	/*
> +	 * @color_fill_format_property:
> +	 * Optional color fill format property for this plane. See
> +	 * drm_plane_create_color_fill_format_property().
> +	 */
> +	struct drm_property *color_fill_format_property;
> +
> +	/*
> +	 * @color_fill_property:
> +	 * Optional color fill property for this plane. See
> +	 * drm_plane_create_color_fill_property().
> +	 */
> +	struct drm_property *color_fill_property;
> +
>   	/**
>   	 * @alpha_property:
>   	 * Optional alpha property for this plane. See

-- 
With best wishes
Dmitry