[RFC PATCH v3 0/3] Support for Solid Fill Planes

[RFC PATCH v3 0/3] Support for Solid Fill Planes

[Jessica Zhang]

Introduce and add support for a solid_fill property. When the solid_fill
property is set, and the framebuffer is set to NULL, memory fetch will be
disabled.

In addition, loosen the NULL FB checks within the atomic commit callstack
to allow a NULL FB when the solid_fill property is set and add FB checks
in methods where the FB was previously assumed to be non-NULL.

Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
callstack to account for a NULL FB in cases where solid_fill is set.

Some drivers support hardware that have optimizations for solid fill
planes. This series aims to expose these capabilities to userspace as
some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
hardware composer HAL) that can be set by apps like the Android Gears
app.

Userspace can set the solid_fill property to a blob containing the
appropriate version number and solid fill color (in RGB323232 format) and
setting the framebuffer to NULL.

Note: Currently, there's only one version of the solid_fill blob property.
However if other drivers want to support a similar feature, but require
more than just the solid fill color, they can extend this feature by
creating additional versions of the drm_solid_fill struct.

Changes in V2:
- Dropped SOLID_FILL_FORMAT property (Simon)
- Switched to implementing solid_fill property as a blob (Simon, Dmitry)
- Changed to checks for if solid_fill_blob is set (Dmitry)
- Abstracted (plane_state && !solid_fill_blob) checks to helper method
  (Dmitry)
- Removed DPU_PLANE_COLOR_FILL_FLAG
- Fixed whitespace and indentation issues (Dmitry)

Changes in V3:
- Fixed some logic errors in atomic checks (Dmitry)
- Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
  methods (Dmitry)

Jessica Zhang (3):
  drm: Introduce solid fill property for drm plane
  drm: Adjust atomic checks for solid fill color
  drm/msm/dpu: Use color_fill property for DPU planes

 drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
 drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
 drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
 drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
 drivers/gpu/drm/drm_blend.c               |  17 +++
 drivers/gpu/drm/drm_plane.c               |   8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
 include/drm/drm_atomic_helper.h           |   5 +-
 include/drm/drm_blend.h                   |   1 +
 include/drm/drm_plane.h                   |  62 ++++++++++
 11 files changed, 302 insertions(+), 103 deletions(-)

-- 
2.38.1

[RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Jessica Zhang]

Add support for solid_fill property to drm_plane. In addition, add
support for setting and getting the values for solid_fill.

solid_fill holds data for supporting solid fill planes. The property
accepts an RGB323232 value and the driver data is formatted as such:

struct drm_solid_fill {
	u32 r;
	u32 g;
	u32 b;
};

To enable solid fill planes, userspace must assigned solid_fill to a
property blob containing the following information:

struct drm_solid_fill_info {
	u8 version;
	u32 r, g, b;
};

Changes in V2:
- Changed solid_fill property to a property blob (Simon, Dmitry)
- Added drm_solid_fill struct (Simon)
- Added drm_solid_fill_info struct (Simon)

Changes in V3:
- Corrected typo in drm_solid_fill struct documentation

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         | 59 +++++++++++++++++++++++
 drivers/gpu/drm/drm_blend.c               | 17 +++++++
 include/drm/drm_blend.h                   |  1 +
 include/drm/drm_plane.h                   | 43 +++++++++++++++++
 5 files changed, 129 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index dfb57217253b..c96fd1f2ad99 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
 	plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
 	plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
 
+	if (plane_state->solid_fill_blob) {
+		drm_property_blob_put(plane_state->solid_fill_blob);
+		plane_state->solid_fill_blob = NULL;
+	}
+
 	if (plane->color_encoding_property) {
 		if (!drm_object_property_get_default_value(&plane->base,
 							   plane->color_encoding_property,
@@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 
+	if (state->solid_fill_blob)
+		drm_property_blob_get(state->solid_fill_blob);
+
 	state->fence = NULL;
 	state->commit = NULL;
 	state->fb_damage_clips = NULL;
@@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 		drm_crtc_commit_put(state->commit);
 
 	drm_property_blob_put(state->fb_damage_clips);
+	drm_property_blob_put(state->solid_fill_blob);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index c06d0639d552..8a1d2fb7a757 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 }
 EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
 
+static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
+		struct drm_solid_fill_info *in)
+{
+	out->r = in->r;
+	out->g = in->g;
+	out->b = in->b;
+}
+
+static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
+		struct drm_property_blob *blob)
+{
+	int ret = 0;
+	int blob_version;
+
+	if (blob == state->solid_fill_blob)
+		return 0;
+
+	drm_property_blob_put(state->solid_fill_blob);
+	state->solid_fill_blob = NULL;
+
+	memset(&state->solid_fill, 0, sizeof(state->solid_fill));
+
+	if (blob) {
+		if (blob->length != sizeof(struct drm_solid_fill_info)) {
+			drm_dbg_atomic(state->plane->dev,
+					"[PLANE:%d:%s] bad solid fill blob length: %zu\n",
+					state->plane->base.id, state->plane->name,
+					blob->length);
+			return -EINVAL;
+		}
+
+		blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
+
+		/* Append with more versions if necessary */
+		if (blob_version == 1) {
+			drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
+		} else {
+			drm_dbg_atomic(state->plane->dev,
+					"[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
+					state->plane->base.id, state->plane->name,
+					ret);
+			return -EINVAL;
+		}
+		state->solid_fill_blob = drm_property_blob_get(blob);
+	}
+
+	return ret;
+}
+
 static void set_out_fence_for_crtc(struct drm_atomic_state *state,
 				   struct drm_crtc *crtc, s32 __user *fence_ptr)
 {
@@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->src_w = val;
 	} else if (property == config->prop_src_h) {
 		state->src_h = val;
+	} else if (property == plane->solid_fill_property) {
+		struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
+
+		ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
+		drm_property_blob_put(solid_fill);
+
+		return ret;
 	} else if (property == plane->alpha_property) {
 		state->alpha = val;
 	} else if (property == plane->blend_mode_property) {
@@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 		*val = state->src_w;
 	} else if (property == config->prop_src_h) {
 		*val = state->src_h;
+	} else if (property == plane->solid_fill_property) {
+		*val = state->solid_fill_blob ?
+			state->solid_fill_blob->base.id : 0;
 	} else if (property == plane->alpha_property) {
 		*val = state->alpha;
 	} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index b4c8cab7158c..17ab645c8309 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create(plane->dev,
+			DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
+			"solid_fill", 0);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&plane->base, prop, 0);
+	plane->solid_fill_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..0338a860b9c8 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
 			      struct drm_atomic_state *state);
 int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 					 unsigned int supported_modes);
+int drm_plane_create_solid_fill_property(struct drm_plane *plane);
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 447e664e49d5..3b9da06f358b 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -40,6 +40,25 @@ enum drm_scaling_filter {
 	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
 };
 
+/**
+ * struct drm_solid_fill_info - User info for solid fill planes
+ */
+struct drm_solid_fill_info {
+	__u8 version;
+	__u32 r, g, b;
+};
+
+/**
+ * struct solid_fill_property - RGB values for solid fill plane
+ *
+ * Note: This is the V1 for this feature
+ */
+struct drm_solid_fill {
+	uint32_t r;
+	uint32_t g;
+	uint32_t b;
+};
+
 /**
  * struct drm_plane_state - mutable plane state
  *
@@ -116,6 +135,23 @@ struct drm_plane_state {
 	/** @src_h: height of visible portion of plane (in 16.16) */
 	uint32_t src_h, src_w;
 
+	/**
+	 * @solid_fill_blob:
+	 *
+	 * Blob containing relevant information for a solid fill plane
+	 * including pixel format and data. See
+	 * drm_plane_create_solid_fill_property() for more details.
+	 */
+	struct drm_property_blob *solid_fill_blob;
+
+	/**
+	 * @solid_fill:
+	 *
+	 * Pixel data for solid fill planes. See
+	 * drm_plane_create_solid_fill_property() for more details.
+	 */
+	struct drm_solid_fill solid_fill;
+
 	/**
 	 * @alpha:
 	 * Opacity of the plane with 0 as completely transparent and 0xffff as
@@ -699,6 +735,13 @@ struct drm_plane {
 	 */
 	struct drm_plane_state *state;
 
+	/*
+	 * @solid_fill_property:
+	 * Optional solid_fill property for this plane. See
+	 * drm_plane_create_solid_fill_property().
+	 */
+	struct drm_property *solid_fill_property;
+
 	/**
 	 * @alpha_property:
 	 * Optional alpha property for this plane. See
-- 
2.38.1

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Dmitry Baryshkov]

On 05/01/2023 01:40, Jessica Zhang wrote:
> Add support for solid_fill property to drm_plane. In addition, add
> support for setting and getting the values for solid_fill.
> 
> solid_fill holds data for supporting solid fill planes. The property
> accepts an RGB323232 value and the driver data is formatted as such:
> 
> struct drm_solid_fill {
> 	u32 r;
> 	u32 g;
> 	u32 b;
> };

This description is unnecessary (and confusing), since you describe 
drm_solid_fill_info below.

> 
> To enable solid fill planes, userspace must assigned solid_fill to a

must assign. Also the phrasing seems strange. The blob is assigned to 
the 'solid_fill' property, not the other way around.

> property blob containing the following information:

This should clearly describe if solid_fill overrides FB or if FB 
overrides solid_fill. Also please extend properties documentation in 
Documentation/gpu/drm-kms.rst.

> 
> struct drm_solid_fill_info {
> 	u8 version;
> 	u32 r, g, b;
> };
> 
> Changes in V2:
> - Changed solid_fill property to a property blob (Simon, Dmitry)
> - Added drm_solid_fill struct (Simon)
> - Added drm_solid_fill_info struct (Simon)
> 
> Changes in V3:
> - Corrected typo in drm_solid_fill struct documentation
> 
> 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         | 59 +++++++++++++++++++++++
>   drivers/gpu/drm/drm_blend.c               | 17 +++++++
>   include/drm/drm_blend.h                   |  1 +
>   include/drm/drm_plane.h                   | 43 +++++++++++++++++
>   5 files changed, 129 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index dfb57217253b..c96fd1f2ad99 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>   	plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>   	plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   
> +	if (plane_state->solid_fill_blob) {
> +		drm_property_blob_put(plane_state->solid_fill_blob);
> +		plane_state->solid_fill_blob = NULL;
> +	}
> +
>   	if (plane->color_encoding_property) {
>   		if (!drm_object_property_get_default_value(&plane->base,
>   							   plane->color_encoding_property,
> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>   	if (state->fb)
>   		drm_framebuffer_get(state->fb);
>   
> +	if (state->solid_fill_blob)
> +		drm_property_blob_get(state->solid_fill_blob);
> +
>   	state->fence = NULL;
>   	state->commit = NULL;
>   	state->fb_damage_clips = NULL;
> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>   		drm_crtc_commit_put(state->commit);
>   
>   	drm_property_blob_put(state->fb_damage_clips);
> +	drm_property_blob_put(state->solid_fill_blob);
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>   
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c06d0639d552..8a1d2fb7a757 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>   }
>   EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>   
> +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
> +		struct drm_solid_fill_info *in)

No need for a separate function, you can inline this.

> +{
> +	out->r = in->r;
> +	out->g = in->g;
> +	out->b = in->b;
> +}
> +
> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
> +		struct drm_property_blob *blob)
> +{
> +	int ret = 0;
> +	int blob_version;
> +
> +	if (blob == state->solid_fill_blob)
> +		return 0;
> +
> +	drm_property_blob_put(state->solid_fill_blob);
> +	state->solid_fill_blob = NULL;
> +
> +	memset(&state->solid_fill, 0, sizeof(state->solid_fill));
> +
> +	if (blob) {
> +		if (blob->length != sizeof(struct drm_solid_fill_info)) {
> +			drm_dbg_atomic(state->plane->dev,
> +					"[PLANE:%d:%s] bad solid fill blob length: %zu\n",
> +					state->plane->base.id, state->plane->name,
> +					blob->length);
> +			return -EINVAL;
> +		}
> +
> +		blob_version = ((struct drm_solid_fill_info *)blob->data)->version;

Please assign blob->data to temporary var.

> +
> +		/* Append with more versions if necessary */

s/Append/Add/

> +		if (blob_version == 1) {
> +			drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
> +		} else {
> +			drm_dbg_atomic(state->plane->dev,
> +					"[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
> +					state->plane->base.id, state->plane->name,
> +					ret);
> +			return -EINVAL;
> +		}
> +		state->solid_fill_blob = drm_property_blob_get(blob);
> +	}
> +
> +	return ret;
> +}
> +
>   static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>   				   struct drm_crtc *crtc, s32 __user *fence_ptr)
>   {
> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->src_w = val;
>   	} else if (property == config->prop_src_h) {
>   		state->src_h = val;
> +	} else if (property == plane->solid_fill_property) {
> +		struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
> +
> +		ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
> +		drm_property_blob_put(solid_fill);
> +
> +		return ret;
>   	} else if (property == plane->alpha_property) {
>   		state->alpha = val;
>   	} else if (property == plane->blend_mode_property) {
> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   		*val = state->src_w;
>   	} else if (property == config->prop_src_h) {
>   		*val = state->src_h;
> +	} else if (property == plane->solid_fill_property) {
> +		*val = state->solid_fill_blob ?
> +			state->solid_fill_blob->base.id : 0;
>   	} else if (property == plane->alpha_property) {
>   		*val = state->alpha;
>   	} else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index b4c8cab7158c..17ab645c8309 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create(plane->dev,
> +			DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
> +			"solid_fill", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +	plane->solid_fill_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 88bdfec3bd88..0338a860b9c8 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   			      struct drm_atomic_state *state);
>   int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>   					 unsigned int supported_modes);
> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>   #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 447e664e49d5..3b9da06f358b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>   	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>   };
>   
> +/**
> + * struct drm_solid_fill_info - User info for solid fill planes
> + */
> +struct drm_solid_fill_info {
> +	__u8 version;
> +	__u32 r, g, b;
> +};
> +
> +/**
> + * struct solid_fill_property - RGB values for solid fill plane
> + *
> + * Note: This is the V1 for this feature
> + */
> +struct drm_solid_fill {
> +	uint32_t r;
> +	uint32_t g;
> +	uint32_t b;
> +};
> +
>   /**
>    * struct drm_plane_state - mutable plane state
>    *
> @@ -116,6 +135,23 @@ struct drm_plane_state {
>   	/** @src_h: height of visible portion of plane (in 16.16) */
>   	uint32_t src_h, src_w;
>   
> +	/**
> +	 * @solid_fill_blob:
> +	 *
> +	 * Blob containing relevant information for a solid fill plane
> +	 * including pixel format and data. See
> +	 * drm_plane_create_solid_fill_property() for more details.
> +	 */
> +	struct drm_property_blob *solid_fill_blob;
> +
> +	/**
> +	 * @solid_fill:
> +	 *
> +	 * Pixel data for solid fill planes. See
> +	 * drm_plane_create_solid_fill_property() for more details.
> +	 */
> +	struct drm_solid_fill solid_fill;
> +
>   	/**
>   	 * @alpha:
>   	 * Opacity of the plane with 0 as completely transparent and 0xffff as
> @@ -699,6 +735,13 @@ struct drm_plane {
>   	 */
>   	struct drm_plane_state *state;
>   
> +	/*
> +	 * @solid_fill_property:
> +	 * Optional solid_fill property for this plane. See
> +	 * drm_plane_create_solid_fill_property().
> +	 */
> +	struct drm_property *solid_fill_property;
> +
>   	/**
>   	 * @alpha_property:
>   	 * Optional alpha property for this plane. See

-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Dmitry Baryshkov]

On 05/01/2023 01:40, Jessica Zhang wrote:
> Add support for solid_fill property to drm_plane. In addition, add
> support for setting and getting the values for solid_fill.
> 
> solid_fill holds data for supporting solid fill planes. The property
> accepts an RGB323232 value and the driver data is formatted as such:
> 
> struct drm_solid_fill {
> 	u32 r;
> 	u32 g;
> 	u32 b;
> };
> 
> To enable solid fill planes, userspace must assigned solid_fill to a
> property blob containing the following information:
> 
> struct drm_solid_fill_info {
> 	u8 version;
> 	u32 r, g, b;

BTW: should we add support for alpha too? DPU hardware supports using 
RGBA as a fill colour format, doesn't it?

But then we face the obvious question, how do we communicate to 
userspace if the hardware support RGB or RGBA?

> };
> 
> Changes in V2:
> - Changed solid_fill property to a property blob (Simon, Dmitry)
> - Added drm_solid_fill struct (Simon)
> - Added drm_solid_fill_info struct (Simon)
> 
> Changes in V3:
> - Corrected typo in drm_solid_fill struct documentation
> 
> 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         | 59 +++++++++++++++++++++++
>   drivers/gpu/drm/drm_blend.c               | 17 +++++++
>   include/drm/drm_blend.h                   |  1 +
>   include/drm/drm_plane.h                   | 43 +++++++++++++++++
>   5 files changed, 129 insertions(+)

-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Harry Wentland]

On 1/4/23 18:40, Jessica Zhang wrote:
> Add support for solid_fill property to drm_plane. In addition, add
> support for setting and getting the values for solid_fill.
> 
> solid_fill holds data for supporting solid fill planes. The property
> accepts an RGB323232 value and the driver data is formatted as such:
> 
> struct drm_solid_fill {
> 	u32 r;
> 	u32 g;
> 	u32 b;
> };

Rather than special-casing this would it make sense to define this
as a single pixel of a FOURCC property?

I.e., something like this:

struct drm_solid_fill_info {
	u32 format; /* FOURCC value */
	u64 value; /* FOURCC pixel value */
}

That removes some ambiguity how the value should be interpreted, i.e.,
it can be interpreted like a single pixel of the specified FOURCC format.

It might also make sense to let drivers advertise the supported
FOURCC formats for solid_fill planes.

Is there an implementation for this in a corresponding canonical
upstream userspace project, to satisfy [1]? If not, what is the plan
for this? If so, please point to the corresponding patches.

[1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Harry

> 
> To enable solid fill planes, userspace must assigned solid_fill to a
> property blob containing the following information:
> 
> struct drm_solid_fill_info {
> 	u8 version;
> 	u32 r, g, b;
> };
> 
> Changes in V2:
> - Changed solid_fill property to a property blob (Simon, Dmitry)
> - Added drm_solid_fill struct (Simon)
> - Added drm_solid_fill_info struct (Simon)
> 
> Changes in V3:
> - Corrected typo in drm_solid_fill struct documentation
> 
> 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         | 59 +++++++++++++++++++++++
>  drivers/gpu/drm/drm_blend.c               | 17 +++++++
>  include/drm/drm_blend.h                   |  1 +
>  include/drm/drm_plane.h                   | 43 +++++++++++++++++
>  5 files changed, 129 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index dfb57217253b..c96fd1f2ad99 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>  	plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>  	plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>  
> +	if (plane_state->solid_fill_blob) {
> +		drm_property_blob_put(plane_state->solid_fill_blob);
> +		plane_state->solid_fill_blob = NULL;
> +	}
> +
>  	if (plane->color_encoding_property) {
>  		if (!drm_object_property_get_default_value(&plane->base,
>  							   plane->color_encoding_property,
> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  	if (state->fb)
>  		drm_framebuffer_get(state->fb);
>  
> +	if (state->solid_fill_blob)
> +		drm_property_blob_get(state->solid_fill_blob);
> +
>  	state->fence = NULL;
>  	state->commit = NULL;
>  	state->fb_damage_clips = NULL;
> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  		drm_crtc_commit_put(state->commit);
>  
>  	drm_property_blob_put(state->fb_damage_clips);
> +	drm_property_blob_put(state->solid_fill_blob);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c06d0639d552..8a1d2fb7a757 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  }
>  EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>  
> +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
> +		struct drm_solid_fill_info *in)
> +{
> +	out->r = in->r;
> +	out->g = in->g;
> +	out->b = in->b;
> +}
> +
> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
> +		struct drm_property_blob *blob)
> +{
> +	int ret = 0;
> +	int blob_version;
> +
> +	if (blob == state->solid_fill_blob)
> +		return 0;
> +
> +	drm_property_blob_put(state->solid_fill_blob);
> +	state->solid_fill_blob = NULL;
> +
> +	memset(&state->solid_fill, 0, sizeof(state->solid_fill));
> +
> +	if (blob) {
> +		if (blob->length != sizeof(struct drm_solid_fill_info)) {
> +			drm_dbg_atomic(state->plane->dev,
> +					"[PLANE:%d:%s] bad solid fill blob length: %zu\n",
> +					state->plane->base.id, state->plane->name,
> +					blob->length);
> +			return -EINVAL;
> +		}
> +
> +		blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
> +
> +		/* Append with more versions if necessary */
> +		if (blob_version == 1) {
> +			drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
> +		} else {
> +			drm_dbg_atomic(state->plane->dev,
> +					"[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
> +					state->plane->base.id, state->plane->name,
> +					ret);
> +			return -EINVAL;
> +		}
> +		state->solid_fill_blob = drm_property_blob_get(blob);
> +	}
> +
> +	return ret;
> +}
> +
>  static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>  				   struct drm_crtc *crtc, s32 __user *fence_ptr)
>  {
> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->src_w = val;
>  	} else if (property == config->prop_src_h) {
>  		state->src_h = val;
> +	} else if (property == plane->solid_fill_property) {
> +		struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
> +
> +		ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
> +		drm_property_blob_put(solid_fill);
> +
> +		return ret;
>  	} else if (property == plane->alpha_property) {
>  		state->alpha = val;
>  	} else if (property == plane->blend_mode_property) {
> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->src_w;
>  	} else if (property == config->prop_src_h) {
>  		*val = state->src_h;
> +	} else if (property == plane->solid_fill_property) {
> +		*val = state->solid_fill_blob ?
> +			state->solid_fill_blob->base.id : 0;
>  	} else if (property == plane->alpha_property) {
>  		*val = state->alpha;
>  	} else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index b4c8cab7158c..17ab645c8309 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create(plane->dev,
> +			DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
> +			"solid_fill", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +	plane->solid_fill_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 88bdfec3bd88..0338a860b9c8 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>  			      struct drm_atomic_state *state);
>  int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>  					 unsigned int supported_modes);
> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 447e664e49d5..3b9da06f358b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>  	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>  };
>  
> +/**
> + * struct drm_solid_fill_info - User info for solid fill planes
> + */
> +struct drm_solid_fill_info {
> +	__u8 version;
> +	__u32 r, g, b;
> +};
> +
> +/**
> + * struct solid_fill_property - RGB values for solid fill plane
> + *
> + * Note: This is the V1 for this feature
> + */
> +struct drm_solid_fill {
> +	uint32_t r;
> +	uint32_t g;
> +	uint32_t b;
> +};
> +
>  /**
>   * struct drm_plane_state - mutable plane state
>   *
> @@ -116,6 +135,23 @@ struct drm_plane_state {
>  	/** @src_h: height of visible portion of plane (in 16.16) */
>  	uint32_t src_h, src_w;
>  
> +	/**
> +	 * @solid_fill_blob:
> +	 *
> +	 * Blob containing relevant information for a solid fill plane
> +	 * including pixel format and data. See
> +	 * drm_plane_create_solid_fill_property() for more details.
> +	 */
> +	struct drm_property_blob *solid_fill_blob;
> +
> +	/**
> +	 * @solid_fill:
> +	 *
> +	 * Pixel data for solid fill planes. See
> +	 * drm_plane_create_solid_fill_property() for more details.
> +	 */
> +	struct drm_solid_fill solid_fill;
> +
>  	/**
>  	 * @alpha:
>  	 * Opacity of the plane with 0 as completely transparent and 0xffff as
> @@ -699,6 +735,13 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
>  
> +	/*
> +	 * @solid_fill_property:
> +	 * Optional solid_fill property for this plane. See
> +	 * drm_plane_create_solid_fill_property().
> +	 */
> +	struct drm_property *solid_fill_property;
> +
>  	/**
>  	 * @alpha_property:
>  	 * Optional alpha property for this plane. See

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Jessica Zhang]

On 1/18/2023 10:57 AM, Harry Wentland wrote:
> On 1/4/23 18:40, Jessica Zhang wrote:
>> Add support for solid_fill property to drm_plane. In addition, add
>> support for setting and getting the values for solid_fill.
>>
>> solid_fill holds data for supporting solid fill planes. The property
>> accepts an RGB323232 value and the driver data is formatted as such:
>>
>> struct drm_solid_fill {
>> 	u32 r;
>> 	u32 g;
>> 	u32 b;
>> };
> 
> Rather than special-casing this would it make sense to define this
> as a single pixel of a FOURCC property?
> 
> I.e., something like this:
> 
> struct drm_solid_fill_info {
> 	u32 format; /* FOURCC value */
> 	u64 value; /* FOURCC pixel value */
> }
> 
> That removes some ambiguity how the value should be interpreted, i.e.,
> it can be interpreted like a single pixel of the specified FOURCC format.
> 
> It might also make sense to let drivers advertise the supported
> FOURCC formats for solid_fill planes.

Hi Harry,

The initial v1 of this RFC had support for taking in a format and such, 
but it was decided that just supporting RGB323232 would work too.

Here's the original thread discussing it [1], but to summarize, the work 
needed to convert the solid fill color to RGB is trivial (as it's just a 
single pixel of data). In addition, supporting various formats for 
solid_fill would add complexity as we'd have to communicate which 
formats are supported.

[1] 
https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html

> 
> Is there an implementation for this in a corresponding canonical
> upstream userspace project, to satisfy [1]? If not, what is the plan
> for this? If so, please point to the corresponding patches.

The use case we're trying to support here is the Android HWC SOLID_FILL 
hint [1], though it can also be used to address the Wayland single pixel 
FB protocol [2]. I'm also planning to add an IGT test to show an example 
of end to end usage.

[1] 
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52

[2] 
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104

Thanks,

Jessica Zhang

> 
> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 
> Harry
> 
>>
>> To enable solid fill planes, userspace must assigned solid_fill to a
>> property blob containing the following information:
>>
>> struct drm_solid_fill_info {
>> 	u8 version;
>> 	u32 r, g, b;
>> };
>>
>> Changes in V2:
>> - Changed solid_fill property to a property blob (Simon, Dmitry)
>> - Added drm_solid_fill struct (Simon)
>> - Added drm_solid_fill_info struct (Simon)
>>
>> Changes in V3:
>> - Corrected typo in drm_solid_fill struct documentation
>>
>> 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         | 59 +++++++++++++++++++++++
>>   drivers/gpu/drm/drm_blend.c               | 17 +++++++
>>   include/drm/drm_blend.h                   |  1 +
>>   include/drm/drm_plane.h                   | 43 +++++++++++++++++
>>   5 files changed, 129 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>> index dfb57217253b..c96fd1f2ad99 100644
>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>>   	plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>   	plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>   
>> +	if (plane_state->solid_fill_blob) {
>> +		drm_property_blob_put(plane_state->solid_fill_blob);
>> +		plane_state->solid_fill_blob = NULL;
>> +	}
>> +
>>   	if (plane->color_encoding_property) {
>>   		if (!drm_object_property_get_default_value(&plane->base,
>>   							   plane->color_encoding_property,
>> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>   	if (state->fb)
>>   		drm_framebuffer_get(state->fb);
>>   
>> +	if (state->solid_fill_blob)
>> +		drm_property_blob_get(state->solid_fill_blob);
>> +
>>   	state->fence = NULL;
>>   	state->commit = NULL;
>>   	state->fb_damage_clips = NULL;
>> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>   		drm_crtc_commit_put(state->commit);
>>   
>>   	drm_property_blob_put(state->fb_damage_clips);
>> +	drm_property_blob_put(state->solid_fill_blob);
>>   }
>>   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>   
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index c06d0639d552..8a1d2fb7a757 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>   }
>>   EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>   
>> +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
>> +		struct drm_solid_fill_info *in)
>> +{
>> +	out->r = in->r;
>> +	out->g = in->g;
>> +	out->b = in->b;
>> +}
>> +
>> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
>> +		struct drm_property_blob *blob)
>> +{
>> +	int ret = 0;
>> +	int blob_version;
>> +
>> +	if (blob == state->solid_fill_blob)
>> +		return 0;
>> +
>> +	drm_property_blob_put(state->solid_fill_blob);
>> +	state->solid_fill_blob = NULL;
>> +
>> +	memset(&state->solid_fill, 0, sizeof(state->solid_fill));
>> +
>> +	if (blob) {
>> +		if (blob->length != sizeof(struct drm_solid_fill_info)) {
>> +			drm_dbg_atomic(state->plane->dev,
>> +					"[PLANE:%d:%s] bad solid fill blob length: %zu\n",
>> +					state->plane->base.id, state->plane->name,
>> +					blob->length);
>> +			return -EINVAL;
>> +		}
>> +
>> +		blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
>> +
>> +		/* Append with more versions if necessary */
>> +		if (blob_version == 1) {
>> +			drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
>> +		} else {
>> +			drm_dbg_atomic(state->plane->dev,
>> +					"[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
>> +					state->plane->base.id, state->plane->name,
>> +					ret);
>> +			return -EINVAL;
>> +		}
>> +		state->solid_fill_blob = drm_property_blob_get(blob);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>>   				   struct drm_crtc *crtc, s32 __user *fence_ptr)
>>   {
>> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   		state->src_w = val;
>>   	} else if (property == config->prop_src_h) {
>>   		state->src_h = val;
>> +	} else if (property == plane->solid_fill_property) {
>> +		struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
>> +
>> +		ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
>> +		drm_property_blob_put(solid_fill);
>> +
>> +		return ret;
>>   	} else if (property == plane->alpha_property) {
>>   		state->alpha = val;
>>   	} else if (property == plane->blend_mode_property) {
>> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>   		*val = state->src_w;
>>   	} else if (property == config->prop_src_h) {
>>   		*val = state->src_h;
>> +	} else if (property == plane->solid_fill_property) {
>> +		*val = state->solid_fill_blob ?
>> +			state->solid_fill_blob->base.id : 0;
>>   	} else if (property == plane->alpha_property) {
>>   		*val = state->alpha;
>>   	} else if (property == plane->blend_mode_property) {
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index b4c8cab7158c..17ab645c8309 100644
>> --- a/drivers/gpu/drm/drm_blend.c
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
>> +{
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create(plane->dev,
>> +			DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
>> +			"solid_fill", 0);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	drm_object_attach_property(&plane->base, prop, 0);
>> +	plane->solid_fill_property = prop;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>> index 88bdfec3bd88..0338a860b9c8 100644
>> --- a/include/drm/drm_blend.h
>> +++ b/include/drm/drm_blend.h
>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>   			      struct drm_atomic_state *state);
>>   int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>>   					 unsigned int supported_modes);
>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>>   #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 447e664e49d5..3b9da06f358b 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>>   	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>>   };
>>   
>> +/**
>> + * struct drm_solid_fill_info - User info for solid fill planes
>> + */
>> +struct drm_solid_fill_info {
>> +	__u8 version;
>> +	__u32 r, g, b;
>> +};
>> +
>> +/**
>> + * struct solid_fill_property - RGB values for solid fill plane
>> + *
>> + * Note: This is the V1 for this feature
>> + */
>> +struct drm_solid_fill {
>> +	uint32_t r;
>> +	uint32_t g;
>> +	uint32_t b;
>> +};
>> +
>>   /**
>>    * struct drm_plane_state - mutable plane state
>>    *
>> @@ -116,6 +135,23 @@ struct drm_plane_state {
>>   	/** @src_h: height of visible portion of plane (in 16.16) */
>>   	uint32_t src_h, src_w;
>>   
>> +	/**
>> +	 * @solid_fill_blob:
>> +	 *
>> +	 * Blob containing relevant information for a solid fill plane
>> +	 * including pixel format and data. See
>> +	 * drm_plane_create_solid_fill_property() for more details.
>> +	 */
>> +	struct drm_property_blob *solid_fill_blob;
>> +
>> +	/**
>> +	 * @solid_fill:
>> +	 *
>> +	 * Pixel data for solid fill planes. See
>> +	 * drm_plane_create_solid_fill_property() for more details.
>> +	 */
>> +	struct drm_solid_fill solid_fill;
>> +
>>   	/**
>>   	 * @alpha:
>>   	 * Opacity of the plane with 0 as completely transparent and 0xffff as
>> @@ -699,6 +735,13 @@ struct drm_plane {
>>   	 */
>>   	struct drm_plane_state *state;
>>   
>> +	/*
>> +	 * @solid_fill_property:
>> +	 * Optional solid_fill property for this plane. See
>> +	 * drm_plane_create_solid_fill_property().
>> +	 */
>> +	struct drm_property *solid_fill_property;
>> +
>>   	/**
>>   	 * @alpha_property:
>>   	 * Optional alpha property for this plane. See
> 

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Harry Wentland]

On 1/18/23 17:53, Jessica Zhang wrote:
> 
> 
> On 1/18/2023 10:57 AM, Harry Wentland wrote:
>> On 1/4/23 18:40, Jessica Zhang wrote:
>>> Add support for solid_fill property to drm_plane. In addition, add
>>> support for setting and getting the values for solid_fill.
>>>
>>> solid_fill holds data for supporting solid fill planes. The property
>>> accepts an RGB323232 value and the driver data is formatted as such:
>>>
>>> struct drm_solid_fill {
>>>     u32 r;
>>>     u32 g;
>>>     u32 b;
>>> };
>>
>> Rather than special-casing this would it make sense to define this
>> as a single pixel of a FOURCC property?
>>
>> I.e., something like this:
>>
>> struct drm_solid_fill_info {
>>     u32 format; /* FOURCC value */
>>     u64 value; /* FOURCC pixel value */
>> }
>>
>> That removes some ambiguity how the value should be interpreted, i.e.,
>> it can be interpreted like a single pixel of the specified FOURCC format.
>>
>> It might also make sense to let drivers advertise the supported
>> FOURCC formats for solid_fill planes.
> 
> Hi Harry,
> 
> The initial v1 of this RFC had support for taking in a format and such, but it was decided that just supporting RGB323232 would work too.
> 
> Here's the original thread discussing it [1], but to summarize, the work needed to convert the solid fill color to RGB is trivial (as it's just a single pixel of data). In addition, supporting various formats for solid_fill would add complexity as we'd have to communicate which formats are supported.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
> 

Make sense, and thanks for summarizing.

The only comment I would have left then, is that maybe it'd be good to add
an alpha value. I think it was suggested by someone else as well.

>>
>> Is there an implementation for this in a corresponding canonical
>> upstream userspace project, to satisfy [1]? If not, what is the plan
>> for this? If so, please point to the corresponding patches.
> 
> The use case we're trying to support here is the Android HWC SOLID_FILL hint [1], though it can also be used to address the Wayland single pixel FB protocol [2]. I'm also planning to add an IGT test to show an example of end to end usage.
> 
> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
> 
> [2] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
> 

Makes sense.

Harry

> Thanks,
> 
> Jessica Zhang
> 
>>
>> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>
>> Harry
>>
>>>
>>> To enable solid fill planes, userspace must assigned solid_fill to a
>>> property blob containing the following information:
>>>
>>> struct drm_solid_fill_info {
>>>     u8 version;
>>>     u32 r, g, b;
>>> };
>>>
>>> Changes in V2:
>>> - Changed solid_fill property to a property blob (Simon, Dmitry)
>>> - Added drm_solid_fill struct (Simon)
>>> - Added drm_solid_fill_info struct (Simon)
>>>
>>> Changes in V3:
>>> - Corrected typo in drm_solid_fill struct documentation
>>>
>>> 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         | 59 +++++++++++++++++++++++
>>>   drivers/gpu/drm/drm_blend.c               | 17 +++++++
>>>   include/drm/drm_blend.h                   |  1 +
>>>   include/drm/drm_plane.h                   | 43 +++++++++++++++++
>>>   5 files changed, 129 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>> index dfb57217253b..c96fd1f2ad99 100644
>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>>>       plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>>       plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>>   +    if (plane_state->solid_fill_blob) {
>>> +        drm_property_blob_put(plane_state->solid_fill_blob);
>>> +        plane_state->solid_fill_blob = NULL;
>>> +    }
>>> +
>>>       if (plane->color_encoding_property) {
>>>           if (!drm_object_property_get_default_value(&plane->base,
>>>                                  plane->color_encoding_property,
>>> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>       if (state->fb)
>>>           drm_framebuffer_get(state->fb);
>>>   +    if (state->solid_fill_blob)
>>> +        drm_property_blob_get(state->solid_fill_blob);
>>> +
>>>       state->fence = NULL;
>>>       state->commit = NULL;
>>>       state->fb_damage_clips = NULL;
>>> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>           drm_crtc_commit_put(state->commit);
>>>         drm_property_blob_put(state->fb_damage_clips);
>>> +    drm_property_blob_put(state->solid_fill_blob);
>>>   }
>>>   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>>   diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>> index c06d0639d552..8a1d2fb7a757 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>>   }
>>>   EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>>   +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
>>> +        struct drm_solid_fill_info *in)
>>> +{
>>> +    out->r = in->r;
>>> +    out->g = in->g;
>>> +    out->b = in->b;
>>> +}
>>> +
>>> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
>>> +        struct drm_property_blob *blob)
>>> +{
>>> +    int ret = 0;
>>> +    int blob_version;
>>> +
>>> +    if (blob == state->solid_fill_blob)
>>> +        return 0;
>>> +
>>> +    drm_property_blob_put(state->solid_fill_blob);
>>> +    state->solid_fill_blob = NULL;
>>> +
>>> +    memset(&state->solid_fill, 0, sizeof(state->solid_fill));
>>> +
>>> +    if (blob) {
>>> +        if (blob->length != sizeof(struct drm_solid_fill_info)) {
>>> +            drm_dbg_atomic(state->plane->dev,
>>> +                    "[PLANE:%d:%s] bad solid fill blob length: %zu\n",
>>> +                    state->plane->base.id, state->plane->name,
>>> +                    blob->length);
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
>>> +
>>> +        /* Append with more versions if necessary */
>>> +        if (blob_version == 1) {
>>> +            drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
>>> +        } else {
>>> +            drm_dbg_atomic(state->plane->dev,
>>> +                    "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
>>> +                    state->plane->base.id, state->plane->name,
>>> +                    ret);
>>> +            return -EINVAL;
>>> +        }
>>> +        state->solid_fill_blob = drm_property_blob_get(blob);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>>>                      struct drm_crtc *crtc, s32 __user *fence_ptr)
>>>   {
>>> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>           state->src_w = val;
>>>       } else if (property == config->prop_src_h) {
>>>           state->src_h = val;
>>> +    } else if (property == plane->solid_fill_property) {
>>> +        struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
>>> +
>>> +        ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
>>> +        drm_property_blob_put(solid_fill);
>>> +
>>> +        return ret;
>>>       } else if (property == plane->alpha_property) {
>>>           state->alpha = val;
>>>       } else if (property == plane->blend_mode_property) {
>>> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>           *val = state->src_w;
>>>       } else if (property == config->prop_src_h) {
>>>           *val = state->src_h;
>>> +    } else if (property == plane->solid_fill_property) {
>>> +        *val = state->solid_fill_blob ?
>>> +            state->solid_fill_blob->base.id : 0;
>>>       } else if (property == plane->alpha_property) {
>>>           *val = state->alpha;
>>>       } else if (property == plane->blend_mode_property) {
>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>> index b4c8cab7158c..17ab645c8309 100644
>>> --- a/drivers/gpu/drm/drm_blend.c
>>> +++ b/drivers/gpu/drm/drm_blend.c
>>> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
>>> +{
>>> +    struct drm_property *prop;
>>> +
>>> +    prop = drm_property_create(plane->dev,
>>> +            DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
>>> +            "solid_fill", 0);
>>> +    if (!prop)
>>> +        return -ENOMEM;
>>> +
>>> +    drm_object_attach_property(&plane->base, prop, 0);
>>> +    plane->solid_fill_property = prop;
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>>> index 88bdfec3bd88..0338a860b9c8 100644
>>> --- a/include/drm/drm_blend.h
>>> +++ b/include/drm/drm_blend.h
>>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>>                     struct drm_atomic_state *state);
>>>   int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>>>                        unsigned int supported_modes);
>>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>>>   #endif
>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>> index 447e664e49d5..3b9da06f358b 100644
>>> --- a/include/drm/drm_plane.h
>>> +++ b/include/drm/drm_plane.h
>>> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>>>       DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>>>   };
>>>   +/**
>>> + * struct drm_solid_fill_info - User info for solid fill planes
>>> + */
>>> +struct drm_solid_fill_info {
>>> +    __u8 version;
>>> +    __u32 r, g, b;
>>> +};
>>> +
>>> +/**
>>> + * struct solid_fill_property - RGB values for solid fill plane
>>> + *
>>> + * Note: This is the V1 for this feature
>>> + */
>>> +struct drm_solid_fill {
>>> +    uint32_t r;
>>> +    uint32_t g;
>>> +    uint32_t b;
>>> +};
>>> +
>>>   /**
>>>    * struct drm_plane_state - mutable plane state
>>>    *
>>> @@ -116,6 +135,23 @@ struct drm_plane_state {
>>>       /** @src_h: height of visible portion of plane (in 16.16) */
>>>       uint32_t src_h, src_w;
>>>   +    /**
>>> +     * @solid_fill_blob:
>>> +     *
>>> +     * Blob containing relevant information for a solid fill plane
>>> +     * including pixel format and data. See
>>> +     * drm_plane_create_solid_fill_property() for more details.
>>> +     */
>>> +    struct drm_property_blob *solid_fill_blob;
>>> +
>>> +    /**
>>> +     * @solid_fill:
>>> +     *
>>> +     * Pixel data for solid fill planes. See
>>> +     * drm_plane_create_solid_fill_property() for more details.
>>> +     */
>>> +    struct drm_solid_fill solid_fill;
>>> +
>>>       /**
>>>        * @alpha:
>>>        * Opacity of the plane with 0 as completely transparent and 0xffff as
>>> @@ -699,6 +735,13 @@ struct drm_plane {
>>>        */
>>>       struct drm_plane_state *state;
>>>   +    /*
>>> +     * @solid_fill_property:
>>> +     * Optional solid_fill property for this plane. See
>>> +     * drm_plane_create_solid_fill_property().
>>> +     */
>>> +    struct drm_property *solid_fill_property;
>>> +
>>>       /**
>>>        * @alpha_property:
>>>        * Optional alpha property for this plane. See
>>

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Jessica Zhang]

On 1/19/2023 7:57 AM, Harry Wentland wrote:
> 
> 
> On 1/18/23 17:53, Jessica Zhang wrote:
>>
>>
>> On 1/18/2023 10:57 AM, Harry Wentland wrote:
>>> On 1/4/23 18:40, Jessica Zhang wrote:
>>>> Add support for solid_fill property to drm_plane. In addition, add
>>>> support for setting and getting the values for solid_fill.
>>>>
>>>> solid_fill holds data for supporting solid fill planes. The property
>>>> accepts an RGB323232 value and the driver data is formatted as such:
>>>>
>>>> struct drm_solid_fill {
>>>>      u32 r;
>>>>      u32 g;
>>>>      u32 b;
>>>> };
>>>
>>> Rather than special-casing this would it make sense to define this
>>> as a single pixel of a FOURCC property?
>>>
>>> I.e., something like this:
>>>
>>> struct drm_solid_fill_info {
>>>      u32 format; /* FOURCC value */
>>>      u64 value; /* FOURCC pixel value */
>>> }
>>>
>>> That removes some ambiguity how the value should be interpreted, i.e.,
>>> it can be interpreted like a single pixel of the specified FOURCC format.
>>>
>>> It might also make sense to let drivers advertise the supported
>>> FOURCC formats for solid_fill planes.
>>
>> Hi Harry,
>>
>> The initial v1 of this RFC had support for taking in a format and such, but it was decided that just supporting RGB323232 would work too.
>>
>> Here's the original thread discussing it [1], but to summarize, the work needed to convert the solid fill color to RGB is trivial (as it's just a single pixel of data). In addition, supporting various formats for solid_fill would add complexity as we'd have to communicate which formats are supported.
>>
>> [1] https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
>>
> 
> Make sense, and thanks for summarizing.
> 
> The only comment I would have left then, is that maybe it'd be good to add
> an alpha value. I think it was suggested by someone else as well.

Yep it was mentioned in the v1 discussion, but we came to the conclusion 
that user can set the alpha through the separate alpha plane property.

Thanks,

Jessica Zhang

> 
>>>
>>> Is there an implementation for this in a corresponding canonical
>>> upstream userspace project, to satisfy [1]? If not, what is the plan
>>> for this? If so, please point to the corresponding patches.
>>
>> The use case we're trying to support here is the Android HWC SOLID_FILL hint [1], though it can also be used to address the Wayland single pixel FB protocol [2]. I'm also planning to add an IGT test to show an example of end to end usage.
>>
>> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>>
>> [2] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>>
> 
> Makes sense.
> 
> Harry
> 
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>>
>>> Harry
>>>
>>>>
>>>> To enable solid fill planes, userspace must assigned solid_fill to a
>>>> property blob containing the following information:
>>>>
>>>> struct drm_solid_fill_info {
>>>>      u8 version;
>>>>      u32 r, g, b;
>>>> };
>>>>
>>>> Changes in V2:
>>>> - Changed solid_fill property to a property blob (Simon, Dmitry)
>>>> - Added drm_solid_fill struct (Simon)
>>>> - Added drm_solid_fill_info struct (Simon)
>>>>
>>>> Changes in V3:
>>>> - Corrected typo in drm_solid_fill struct documentation
>>>>
>>>> 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         | 59 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/drm_blend.c               | 17 +++++++
>>>>    include/drm/drm_blend.h                   |  1 +
>>>>    include/drm/drm_plane.h                   | 43 +++++++++++++++++
>>>>    5 files changed, 129 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> index dfb57217253b..c96fd1f2ad99 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>>>>        plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>>>        plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>>>    +    if (plane_state->solid_fill_blob) {
>>>> +        drm_property_blob_put(plane_state->solid_fill_blob);
>>>> +        plane_state->solid_fill_blob = NULL;
>>>> +    }
>>>> +
>>>>        if (plane->color_encoding_property) {
>>>>            if (!drm_object_property_get_default_value(&plane->base,
>>>>                                   plane->color_encoding_property,
>>>> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>>        if (state->fb)
>>>>            drm_framebuffer_get(state->fb);
>>>>    +    if (state->solid_fill_blob)
>>>> +        drm_property_blob_get(state->solid_fill_blob);
>>>> +
>>>>        state->fence = NULL;
>>>>        state->commit = NULL;
>>>>        state->fb_damage_clips = NULL;
>>>> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>>            drm_crtc_commit_put(state->commit);
>>>>          drm_property_blob_put(state->fb_damage_clips);
>>>> +    drm_property_blob_put(state->solid_fill_blob);
>>>>    }
>>>>    EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>>>    diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index c06d0639d552..8a1d2fb7a757 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>>>    }
>>>>    EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>>>    +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
>>>> +        struct drm_solid_fill_info *in)
>>>> +{
>>>> +    out->r = in->r;
>>>> +    out->g = in->g;
>>>> +    out->b = in->b;
>>>> +}
>>>> +
>>>> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
>>>> +        struct drm_property_blob *blob)
>>>> +{
>>>> +    int ret = 0;
>>>> +    int blob_version;
>>>> +
>>>> +    if (blob == state->solid_fill_blob)
>>>> +        return 0;
>>>> +
>>>> +    drm_property_blob_put(state->solid_fill_blob);
>>>> +    state->solid_fill_blob = NULL;
>>>> +
>>>> +    memset(&state->solid_fill, 0, sizeof(state->solid_fill));
>>>> +
>>>> +    if (blob) {
>>>> +        if (blob->length != sizeof(struct drm_solid_fill_info)) {
>>>> +            drm_dbg_atomic(state->plane->dev,
>>>> +                    "[PLANE:%d:%s] bad solid fill blob length: %zu\n",
>>>> +                    state->plane->base.id, state->plane->name,
>>>> +                    blob->length);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +
>>>> +        blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
>>>> +
>>>> +        /* Append with more versions if necessary */
>>>> +        if (blob_version == 1) {
>>>> +            drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
>>>> +        } else {
>>>> +            drm_dbg_atomic(state->plane->dev,
>>>> +                    "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
>>>> +                    state->plane->base.id, state->plane->name,
>>>> +                    ret);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +        state->solid_fill_blob = drm_property_blob_get(blob);
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>                       struct drm_crtc *crtc, s32 __user *fence_ptr)
>>>>    {
>>>> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>            state->src_w = val;
>>>>        } else if (property == config->prop_src_h) {
>>>>            state->src_h = val;
>>>> +    } else if (property == plane->solid_fill_property) {
>>>> +        struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
>>>> +
>>>> +        ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
>>>> +        drm_property_blob_put(solid_fill);
>>>> +
>>>> +        return ret;
>>>>        } else if (property == plane->alpha_property) {
>>>>            state->alpha = val;
>>>>        } else if (property == plane->blend_mode_property) {
>>>> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>            *val = state->src_w;
>>>>        } else if (property == config->prop_src_h) {
>>>>            *val = state->src_h;
>>>> +    } else if (property == plane->solid_fill_property) {
>>>> +        *val = state->solid_fill_blob ?
>>>> +            state->solid_fill_blob->base.id : 0;
>>>>        } else if (property == plane->alpha_property) {
>>>>            *val = state->alpha;
>>>>        } else if (property == plane->blend_mode_property) {
>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>> index b4c8cab7158c..17ab645c8309 100644
>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
>>>> +{
>>>> +    struct drm_property *prop;
>>>> +
>>>> +    prop = drm_property_create(plane->dev,
>>>> +            DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
>>>> +            "solid_fill", 0);
>>>> +    if (!prop)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    drm_object_attach_property(&plane->base, prop, 0);
>>>> +    plane->solid_fill_property = prop;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>>>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>>>> index 88bdfec3bd88..0338a860b9c8 100644
>>>> --- a/include/drm/drm_blend.h
>>>> +++ b/include/drm/drm_blend.h
>>>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>>>                      struct drm_atomic_state *state);
>>>>    int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>>>>                         unsigned int supported_modes);
>>>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>>>>    #endif
>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>> index 447e664e49d5..3b9da06f358b 100644
>>>> --- a/include/drm/drm_plane.h
>>>> +++ b/include/drm/drm_plane.h
>>>> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>>>>        DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>>>>    };
>>>>    +/**
>>>> + * struct drm_solid_fill_info - User info for solid fill planes
>>>> + */
>>>> +struct drm_solid_fill_info {
>>>> +    __u8 version;
>>>> +    __u32 r, g, b;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct solid_fill_property - RGB values for solid fill plane
>>>> + *
>>>> + * Note: This is the V1 for this feature
>>>> + */
>>>> +struct drm_solid_fill {
>>>> +    uint32_t r;
>>>> +    uint32_t g;
>>>> +    uint32_t b;
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct drm_plane_state - mutable plane state
>>>>     *
>>>> @@ -116,6 +135,23 @@ struct drm_plane_state {
>>>>        /** @src_h: height of visible portion of plane (in 16.16) */
>>>>        uint32_t src_h, src_w;
>>>>    +    /**
>>>> +     * @solid_fill_blob:
>>>> +     *
>>>> +     * Blob containing relevant information for a solid fill plane
>>>> +     * including pixel format and data. See
>>>> +     * drm_plane_create_solid_fill_property() for more details.
>>>> +     */
>>>> +    struct drm_property_blob *solid_fill_blob;
>>>> +
>>>> +    /**
>>>> +     * @solid_fill:
>>>> +     *
>>>> +     * Pixel data for solid fill planes. See
>>>> +     * drm_plane_create_solid_fill_property() for more details.
>>>> +     */
>>>> +    struct drm_solid_fill solid_fill;
>>>> +
>>>>        /**
>>>>         * @alpha:
>>>>         * Opacity of the plane with 0 as completely transparent and 0xffff as
>>>> @@ -699,6 +735,13 @@ struct drm_plane {
>>>>         */
>>>>        struct drm_plane_state *state;
>>>>    +    /*
>>>> +     * @solid_fill_property:
>>>> +     * Optional solid_fill property for this plane. See
>>>> +     * drm_plane_create_solid_fill_property().
>>>> +     */
>>>> +    struct drm_property *solid_fill_property;
>>>> +
>>>>        /**
>>>>         * @alpha_property:
>>>>         * Optional alpha property for this plane. See
>>>
> 

Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane

[Harry Wentland]

On 1/19/23 11:24, Jessica Zhang wrote:
> 
> 
> On 1/19/2023 7:57 AM, Harry Wentland wrote:
>>
>>
>> On 1/18/23 17:53, Jessica Zhang wrote:
>>>
>>>
>>> On 1/18/2023 10:57 AM, Harry Wentland wrote:
>>>> On 1/4/23 18:40, Jessica Zhang wrote:
>>>>> Add support for solid_fill property to drm_plane. In addition, add
>>>>> support for setting and getting the values for solid_fill.
>>>>>
>>>>> solid_fill holds data for supporting solid fill planes. The property
>>>>> accepts an RGB323232 value and the driver data is formatted as such:
>>>>>
>>>>> struct drm_solid_fill {
>>>>>      u32 r;
>>>>>      u32 g;
>>>>>      u32 b;
>>>>> };
>>>>
>>>> Rather than special-casing this would it make sense to define this
>>>> as a single pixel of a FOURCC property?
>>>>
>>>> I.e., something like this:
>>>>
>>>> struct drm_solid_fill_info {
>>>>      u32 format; /* FOURCC value */
>>>>      u64 value; /* FOURCC pixel value */
>>>> }
>>>>
>>>> That removes some ambiguity how the value should be interpreted, i.e.,
>>>> it can be interpreted like a single pixel of the specified FOURCC format.
>>>>
>>>> It might also make sense to let drivers advertise the supported
>>>> FOURCC formats for solid_fill planes.
>>>
>>> Hi Harry,
>>>
>>> The initial v1 of this RFC had support for taking in a format and such, but it was decided that just supporting RGB323232 would work too.
>>>
>>> Here's the original thread discussing it [1], but to summarize, the work needed to convert the solid fill color to RGB is trivial (as it's just a single pixel of data). In addition, supporting various formats for solid_fill would add complexity as we'd have to communicate which formats are supported.
>>>
>>> [1] https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
>>>
>>
>> Make sense, and thanks for summarizing.
>>
>> The only comment I would have left then, is that maybe it'd be good to add
>> an alpha value. I think it was suggested by someone else as well.
> 
> Yep it was mentioned in the v1 discussion, but we came to the conclusion that user can set the alpha through the separate alpha plane property.
> 

Got it.

Harry

> Thanks,
> 
> Jessica Zhang
> 
>>
>>>>
>>>> Is there an implementation for this in a corresponding canonical
>>>> upstream userspace project, to satisfy [1]? If not, what is the plan
>>>> for this? If so, please point to the corresponding patches.
>>>
>>> The use case we're trying to support here is the Android HWC SOLID_FILL hint [1], though it can also be used to address the Wayland single pixel FB protocol [2]. I'm also planning to add an IGT test to show an example of end to end usage.
>>>
>>> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>>>
>>> [2] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>>>
>>
>> Makes sense.
>>
>> Harry
>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>>>
>>>> Harry
>>>>
>>>>>
>>>>> To enable solid fill planes, userspace must assigned solid_fill to a
>>>>> property blob containing the following information:
>>>>>
>>>>> struct drm_solid_fill_info {
>>>>>      u8 version;
>>>>>      u32 r, g, b;
>>>>> };
>>>>>
>>>>> Changes in V2:
>>>>> - Changed solid_fill property to a property blob (Simon, Dmitry)
>>>>> - Added drm_solid_fill struct (Simon)
>>>>> - Added drm_solid_fill_info struct (Simon)
>>>>>
>>>>> Changes in V3:
>>>>> - Corrected typo in drm_solid_fill struct documentation
>>>>>
>>>>> 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         | 59 +++++++++++++++++++++++
>>>>>    drivers/gpu/drm/drm_blend.c               | 17 +++++++
>>>>>    include/drm/drm_blend.h                   |  1 +
>>>>>    include/drm/drm_plane.h                   | 43 +++++++++++++++++
>>>>>    5 files changed, 129 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>> index dfb57217253b..c96fd1f2ad99 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>>>>>        plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>>>>        plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>>>>    +    if (plane_state->solid_fill_blob) {
>>>>> +        drm_property_blob_put(plane_state->solid_fill_blob);
>>>>> +        plane_state->solid_fill_blob = NULL;
>>>>> +    }
>>>>> +
>>>>>        if (plane->color_encoding_property) {
>>>>>            if (!drm_object_property_get_default_value(&plane->base,
>>>>>                                   plane->color_encoding_property,
>>>>> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>>>        if (state->fb)
>>>>>            drm_framebuffer_get(state->fb);
>>>>>    +    if (state->solid_fill_blob)
>>>>> +        drm_property_blob_get(state->solid_fill_blob);
>>>>> +
>>>>>        state->fence = NULL;
>>>>>        state->commit = NULL;
>>>>>        state->fb_damage_clips = NULL;
>>>>> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>>>            drm_crtc_commit_put(state->commit);
>>>>>          drm_property_blob_put(state->fb_damage_clips);
>>>>> +    drm_property_blob_put(state->solid_fill_blob);
>>>>>    }
>>>>>    EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>>>>    diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> index c06d0639d552..8a1d2fb7a757 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> @@ -316,6 +316,55 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>>>>    }
>>>>>    EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>>>>    +static void drm_atomic_convert_solid_fill_info(struct drm_solid_fill *out,
>>>>> +        struct drm_solid_fill_info *in)
>>>>> +{
>>>>> +    out->r = in->r;
>>>>> +    out->g = in->g;
>>>>> +    out->b = in->b;
>>>>> +}
>>>>> +
>>>>> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
>>>>> +        struct drm_property_blob *blob)
>>>>> +{
>>>>> +    int ret = 0;
>>>>> +    int blob_version;
>>>>> +
>>>>> +    if (blob == state->solid_fill_blob)
>>>>> +        return 0;
>>>>> +
>>>>> +    drm_property_blob_put(state->solid_fill_blob);
>>>>> +    state->solid_fill_blob = NULL;
>>>>> +
>>>>> +    memset(&state->solid_fill, 0, sizeof(state->solid_fill));
>>>>> +
>>>>> +    if (blob) {
>>>>> +        if (blob->length != sizeof(struct drm_solid_fill_info)) {
>>>>> +            drm_dbg_atomic(state->plane->dev,
>>>>> +                    "[PLANE:%d:%s] bad solid fill blob length: %zu\n",
>>>>> +                    state->plane->base.id, state->plane->name,
>>>>> +                    blob->length);
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>> +
>>>>> +        blob_version = ((struct drm_solid_fill_info *)blob->data)->version;
>>>>> +
>>>>> +        /* Append with more versions if necessary */
>>>>> +        if (blob_version == 1) {
>>>>> +            drm_atomic_convert_solid_fill_info(&state->solid_fill, blob->data);
>>>>> +        } else {
>>>>> +            drm_dbg_atomic(state->plane->dev,
>>>>> +                    "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n",
>>>>> +                    state->plane->base.id, state->plane->name,
>>>>> +                    ret);
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>> +        state->solid_fill_blob = drm_property_blob_get(blob);
>>>>> +    }
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>    static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>>                       struct drm_crtc *crtc, s32 __user *fence_ptr)
>>>>>    {
>>>>> @@ -544,6 +593,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>>            state->src_w = val;
>>>>>        } else if (property == config->prop_src_h) {
>>>>>            state->src_h = val;
>>>>> +    } else if (property == plane->solid_fill_property) {
>>>>> +        struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val);
>>>>> +
>>>>> +        ret = drm_atomic_set_solid_fill_prop(state, solid_fill);
>>>>> +        drm_property_blob_put(solid_fill);
>>>>> +
>>>>> +        return ret;
>>>>>        } else if (property == plane->alpha_property) {
>>>>>            state->alpha = val;
>>>>>        } else if (property == plane->blend_mode_property) {
>>>>> @@ -616,6 +672,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>>            *val = state->src_w;
>>>>>        } else if (property == config->prop_src_h) {
>>>>>            *val = state->src_h;
>>>>> +    } else if (property == plane->solid_fill_property) {
>>>>> +        *val = state->solid_fill_blob ?
>>>>> +            state->solid_fill_blob->base.id : 0;
>>>>>        } else if (property == plane->alpha_property) {
>>>>>            *val = state->alpha;
>>>>>        } else if (property == plane->blend_mode_property) {
>>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>>> index b4c8cab7158c..17ab645c8309 100644
>>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>>> @@ -616,3 +616,20 @@ 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_solid_fill_property(struct drm_plane *plane)
>>>>> +{
>>>>> +    struct drm_property *prop;
>>>>> +
>>>>> +    prop = drm_property_create(plane->dev,
>>>>> +            DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
>>>>> +            "solid_fill", 0);
>>>>> +    if (!prop)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    drm_object_attach_property(&plane->base, prop, 0);
>>>>> +    plane->solid_fill_property = prop;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>>>>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>>>>> index 88bdfec3bd88..0338a860b9c8 100644
>>>>> --- a/include/drm/drm_blend.h
>>>>> +++ b/include/drm/drm_blend.h
>>>>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>>>>                      struct drm_atomic_state *state);
>>>>>    int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>>>>>                         unsigned int supported_modes);
>>>>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
>>>>>    #endif
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 447e664e49d5..3b9da06f358b 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
>>>>>        DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
>>>>>    };
>>>>>    +/**
>>>>> + * struct drm_solid_fill_info - User info for solid fill planes
>>>>> + */
>>>>> +struct drm_solid_fill_info {
>>>>> +    __u8 version;
>>>>> +    __u32 r, g, b;
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct solid_fill_property - RGB values for solid fill plane
>>>>> + *
>>>>> + * Note: This is the V1 for this feature
>>>>> + */
>>>>> +struct drm_solid_fill {
>>>>> +    uint32_t r;
>>>>> +    uint32_t g;
>>>>> +    uint32_t b;
>>>>> +};
>>>>> +
>>>>>    /**
>>>>>     * struct drm_plane_state - mutable plane state
>>>>>     *
>>>>> @@ -116,6 +135,23 @@ struct drm_plane_state {
>>>>>        /** @src_h: height of visible portion of plane (in 16.16) */
>>>>>        uint32_t src_h, src_w;
>>>>>    +    /**
>>>>> +     * @solid_fill_blob:
>>>>> +     *
>>>>> +     * Blob containing relevant information for a solid fill plane
>>>>> +     * including pixel format and data. See
>>>>> +     * drm_plane_create_solid_fill_property() for more details.
>>>>> +     */
>>>>> +    struct drm_property_blob *solid_fill_blob;
>>>>> +
>>>>> +    /**
>>>>> +     * @solid_fill:
>>>>> +     *
>>>>> +     * Pixel data for solid fill planes. See
>>>>> +     * drm_plane_create_solid_fill_property() for more details.
>>>>> +     */
>>>>> +    struct drm_solid_fill solid_fill;
>>>>> +
>>>>>        /**
>>>>>         * @alpha:
>>>>>         * Opacity of the plane with 0 as completely transparent and 0xffff as
>>>>> @@ -699,6 +735,13 @@ struct drm_plane {
>>>>>         */
>>>>>        struct drm_plane_state *state;
>>>>>    +    /*
>>>>> +     * @solid_fill_property:
>>>>> +     * Optional solid_fill property for this plane. See
>>>>> +     * drm_plane_create_solid_fill_property().
>>>>> +     */
>>>>> +    struct drm_property *solid_fill_property;
>>>>> +
>>>>>        /**
>>>>>         * @alpha_property:
>>>>>         * Optional alpha property for this plane. See
>>>>
>>

[RFC PATCH v3 2/3] drm: Adjust atomic checks for solid fill color

[Jessica Zhang]

Loosen the requirements for atomic and legacy commit so that, in cases
where solid fill planes is enabled (and FB_ID is NULL), the commit can
still go through.

In addition, add framebuffer NULL checks in other areas to account for
FB being NULL when solid fill is enabled.

Changes in V2:
- Changed to checks for if solid_fill_blob is set (Dmitry)
- Abstracted (plane_state && !solid_fill_blob) checks to helper method
  (Dmitry)
- Fixed indentation issue (Dmitry)

Changes in V3:
- Created drm_plane_has_visible_data() helper and corrected CRTC and FB
  NULL-check logic (Dmitry)
- Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted
  them into helper method (Dmitry)
- Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry)
- Fixed indentation (Dmitry)

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/drm_atomic.c        | 136 ++++++++++++++++------------
 drivers/gpu/drm/drm_atomic_helper.c |  34 ++++---
 drivers/gpu/drm/drm_plane.c         |   8 +-
 include/drm/drm_atomic_helper.h     |   5 +-
 include/drm/drm_plane.h             |  19 ++++
 5 files changed, 124 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..63f34b430479 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state *old_plane_state,
 	return true;
 }
 
+static int drm_atomic_check_fb(const struct drm_plane_state *state)
+{
+	struct drm_plane *plane = state->plane;
+	const struct drm_framebuffer *fb = state->fb;
+	struct drm_mode_rect *clips;
+
+	uint32_t num_clips;
+	unsigned int fb_width, fb_height;
+	int ret;
+
+	/* Check whether this plane supports the fb pixel format. */
+	ret = drm_plane_check_pixel_format(plane, fb->format->format,
+					   fb->modifier);
+
+	if (ret) {
+		drm_dbg_atomic(plane->dev,
+			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
+			       plane->base.id, plane->name,
+			       &fb->format->format, fb->modifier);
+		return ret;
+	}
+
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
+
+	/* Make sure source coordinates are inside the fb. */
+	if (state->src_w > fb_width ||
+	    state->src_x > fb_width - state->src_w ||
+	    state->src_h > fb_height ||
+	    state->src_y > fb_height - state->src_h) {
+		drm_dbg_atomic(plane->dev,
+			       "[PLANE:%d:%s] invalid source coordinates "
+			       "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
+			       plane->base.id, plane->name,
+			       state->src_w >> 16,
+			       ((state->src_w & 0xffff) * 15625) >> 10,
+			       state->src_h >> 16,
+			       ((state->src_h & 0xffff) * 15625) >> 10,
+			       state->src_x >> 16,
+			       ((state->src_x & 0xffff) * 15625) >> 10,
+			       state->src_y >> 16,
+			       ((state->src_y & 0xffff) * 15625) >> 10,
+			       fb->width, fb->height);
+		return -ENOSPC;
+	}
+
+	clips = __drm_plane_get_damage_clips(state);
+	num_clips = drm_plane_get_damage_clips_count(state);
+
+	/* Make sure damage clips are valid and inside the fb. */
+	while (num_clips > 0) {
+		if (clips->x1 >= clips->x2 ||
+		    clips->y1 >= clips->y2 ||
+		    clips->x1 < 0 ||
+		    clips->y1 < 0 ||
+		    clips->x2 > fb_width ||
+		    clips->y2 > fb_height) {
+			drm_dbg_atomic(plane->dev,
+				       "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n",
+				       plane->base.id, plane->name, clips->x1,
+				       clips->y1, clips->x2, clips->y2);
+			return -EINVAL;
+		}
+		clips++;
+		num_clips--;
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_plane_check - check plane state
  * @old_plane_state: old plane state to check
@@ -596,13 +666,12 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 	struct drm_plane *plane = new_plane_state->plane;
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	const struct drm_framebuffer *fb = new_plane_state->fb;
-	unsigned int fb_width, fb_height;
-	struct drm_mode_rect *clips;
-	uint32_t num_clips;
 	int ret;
 
-	/* either *both* CRTC and FB must be set, or neither */
-	if (crtc && !fb) {
+	/* When solid_fill is disabled,
+	 * either *both* CRTC and FB must be set, or neither
+	 */
+	if (crtc && !drm_atomic_has_visible_data(new_plane_state)) {
 		drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
 			       plane->base.id, plane->name);
 		return -EINVAL;
@@ -625,17 +694,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 		return -EINVAL;
 	}
 
-	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, fb->format->format,
-					   fb->modifier);
-	if (ret) {
-		drm_dbg_atomic(plane->dev,
-			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
-			       plane->base.id, plane->name,
-			       &fb->format->format, fb->modifier);
-		return ret;
-	}
-
 	/* Give drivers some help against integer overflows */
 	if (new_plane_state->crtc_w > INT_MAX ||
 	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||
@@ -649,49 +707,11 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 		return -ERANGE;
 	}
 
-	fb_width = fb->width << 16;
-	fb_height = fb->height << 16;
 
-	/* Make sure source coordinates are inside the fb. */
-	if (new_plane_state->src_w > fb_width ||
-	    new_plane_state->src_x > fb_width - new_plane_state->src_w ||
-	    new_plane_state->src_h > fb_height ||
-	    new_plane_state->src_y > fb_height - new_plane_state->src_h) {
-		drm_dbg_atomic(plane->dev,
-			       "[PLANE:%d:%s] invalid source coordinates "
-			       "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
-			       plane->base.id, plane->name,
-			       new_plane_state->src_w >> 16,
-			       ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
-			       new_plane_state->src_h >> 16,
-			       ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
-			       new_plane_state->src_x >> 16,
-			       ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
-			       new_plane_state->src_y >> 16,
-			       ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
-			       fb->width, fb->height);
-		return -ENOSPC;
-	}
-
-	clips = __drm_plane_get_damage_clips(new_plane_state);
-	num_clips = drm_plane_get_damage_clips_count(new_plane_state);
-
-	/* Make sure damage clips are valid and inside the fb. */
-	while (num_clips > 0) {
-		if (clips->x1 >= clips->x2 ||
-		    clips->y1 >= clips->y2 ||
-		    clips->x1 < 0 ||
-		    clips->y1 < 0 ||
-		    clips->x2 > fb_width ||
-		    clips->y2 > fb_height) {
-			drm_dbg_atomic(plane->dev,
-				       "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n",
-				       plane->base.id, plane->name, clips->x1,
-				       clips->y1, clips->x2, clips->y2);
-			return -EINVAL;
-		}
-		clips++;
-		num_clips--;
+	if (fb) {
+		ret = drm_atomic_check_fb(new_plane_state);
+		if (ret)
+			return ret;
 	}
 
 	if (plane_switching_crtc(old_plane_state, new_plane_state)) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1a586b3c454b..804ae107ae59 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 	*src = drm_plane_state_src(plane_state);
 	*dst = drm_plane_state_dest(plane_state);
 
-	if (!fb) {
+	if (!fb && !drm_plane_solid_fill_enabled(plane_state)) {
 		plane_state->visible = false;
 		return 0;
 	}
@@ -881,25 +881,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 		return -EINVAL;
 	}
 
-	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
+	if (fb) {
+		drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
 
-	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
-	if (hscale < 0 || vscale < 0) {
-		drm_dbg_kms(plane_state->plane->dev,
-			    "Invalid scaling of plane\n");
-		drm_rect_debug_print("src: ", &plane_state->src, true);
-		drm_rect_debug_print("dst: ", &plane_state->dst, false);
-		return -ERANGE;
+		/* Check scaling */
+		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
+
+		if (hscale < 0 || vscale < 0) {
+			drm_dbg_kms(plane_state->plane->dev,
+					"Invalid scaling of plane\n");
+			drm_rect_debug_print("src: ", &plane_state->src, true);
+			drm_rect_debug_print("dst: ", &plane_state->dst, false);
+			return -ERANGE;
+		}
 	}
 
 	if (crtc_state->enable)
 		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
 
-	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
-
-	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
+	if (fb) {
+		plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
+		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
+	} else if (drm_plane_solid_fill_enabled(plane_state)) {
+		plane_state->visible = true;
+	}
 
 	if (!plane_state->visible)
 		/*
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 33357629a7f5..bdce2acbef6a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -856,8 +856,8 @@ static int __setplane_internal(struct drm_plane *plane,
 
 	WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
 
-	/* No fb means shut it down */
-	if (!fb) {
+	/* No fb and no color fill means shut it down */
+	if (!fb && !drm_plane_solid_fill_enabled(plane->state)) {
 		plane->old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane, ctx);
 		if (!ret) {
@@ -908,8 +908,8 @@ static int __setplane_atomic(struct drm_plane *plane,
 
 	WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
 
-	/* No fb means shut it down */
-	if (!fb)
+	/* No fb and no color fill means shut it down */
+	if (!fb && !drm_plane_solid_fill_enabled(plane->state))
 		return plane->funcs->disable_plane(plane, ctx);
 
 	/*
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 33f982cd1a27..a87997b3e0b5 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -230,8 +230,9 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
 	 * Anything else should be considered a bug in the atomic core, so we
 	 * gently warn about it.
 	 */
-	WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
-		(new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
+	WARN_ON(((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
+		(new_plane_state->crtc != NULL &&
+		 !drm_atomic_has_visible_data(new_plane_state))));
 
 	return old_plane_state->crtc && !new_plane_state->crtc;
 }
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3b9da06f358b..3bc6b8d73e8a 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -977,6 +977,25 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 #define drm_for_each_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
 
+/**
+ * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane
+ * @state: plane state
+ *
+ * Returns:
+ * Whether the plane has been assigned a solid_fill_blob
+ */
+static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state)
+{
+	return state && state->solid_fill_blob;
+}
+
+static inline bool
+drm_atomic_has_visible_data(const struct drm_plane_state *state)
+{
+	return state->fb || state->solid_fill_blob;
+}
+
+
 bool drm_any_plane_has_format(struct drm_device *dev,
 			      u32 format, u64 modifier);
 
-- 
2.38.1

Re: [RFC PATCH v3 2/3] drm: Adjust atomic checks for solid fill color

[Dmitry Baryshkov]

On 05/01/2023 01:40, Jessica Zhang wrote:
> Loosen the requirements for atomic and legacy commit so that, in cases
> where solid fill planes is enabled (and FB_ID is NULL), the commit can
> still go through.
> 
> In addition, add framebuffer NULL checks in other areas to account for
> FB being NULL when solid fill is enabled.
> 
> Changes in V2:
> - Changed to checks for if solid_fill_blob is set (Dmitry)
> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>    (Dmitry)
> - Fixed indentation issue (Dmitry)
> 
> Changes in V3:
> - Created drm_plane_has_visible_data() helper and corrected CRTC and FB
>    NULL-check logic (Dmitry)
> - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted
>    them into helper method (Dmitry)
> - Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry)
> - Fixed indentation (Dmitry)
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/drm_atomic.c        | 136 ++++++++++++++++------------
>   drivers/gpu/drm/drm_atomic_helper.c |  34 ++++---
>   drivers/gpu/drm/drm_plane.c         |   8 +-
>   include/drm/drm_atomic_helper.h     |   5 +-
>   include/drm/drm_plane.h             |  19 ++++
>   5 files changed, 124 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f197f59f6d99..63f34b430479 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state *old_plane_state,
>   	return true;
>   }
>   
> +static int drm_atomic_check_fb(const struct drm_plane_state *state)

This change should go to a separate patch. Please don't mix refactoring 
(moving of the code) with the actual functionality changes.

> +{
> +	struct drm_plane *plane = state->plane;
> +	const struct drm_framebuffer *fb = state->fb;
> +	struct drm_mode_rect *clips;
> +
> +	uint32_t num_clips;
> +	unsigned int fb_width, fb_height;
> +	int ret;
> +
> +	/* Check whether this plane supports the fb pixel format. */
> +	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> +					   fb->modifier);
> +
> +	if (ret) {
> +		drm_dbg_atomic(plane->dev,
> +			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
> +			       plane->base.id, plane->name,
> +			       &fb->format->format, fb->modifier);
> +		return ret;
> +	}
> +
> +	fb_width = fb->width << 16;
> +	fb_height = fb->height << 16;
> +
> +	/* Make sure source coordinates are inside the fb. */
> +	if (state->src_w > fb_width ||
> +	    state->src_x > fb_width - state->src_w ||
> +	    state->src_h > fb_height ||
> +	    state->src_y > fb_height - state->src_h) {
> +		drm_dbg_atomic(plane->dev,
> +			       "[PLANE:%d:%s] invalid source coordinates "
> +			       "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> +			       plane->base.id, plane->name,
> +			       state->src_w >> 16,
> +			       ((state->src_w & 0xffff) * 15625) >> 10,
> +			       state->src_h >> 16,
> +			       ((state->src_h & 0xffff) * 15625) >> 10,
> +			       state->src_x >> 16,
> +			       ((state->src_x & 0xffff) * 15625) >> 10,
> +			       state->src_y >> 16,
> +			       ((state->src_y & 0xffff) * 15625) >> 10,
> +			       fb->width, fb->height);
> +		return -ENOSPC;
> +	}
> +
> +	clips = __drm_plane_get_damage_clips(state);
> +	num_clips = drm_plane_get_damage_clips_count(state);
> +
> +	/* Make sure damage clips are valid and inside the fb. */
> +	while (num_clips > 0) {
> +		if (clips->x1 >= clips->x2 ||
> +		    clips->y1 >= clips->y2 ||
> +		    clips->x1 < 0 ||
> +		    clips->y1 < 0 ||
> +		    clips->x2 > fb_width ||
> +		    clips->y2 > fb_height) {
> +			drm_dbg_atomic(plane->dev,
> +				       "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n",
> +				       plane->base.id, plane->name, clips->x1,
> +				       clips->y1, clips->x2, clips->y2);
> +			return -EINVAL;
> +		}
> +		clips++;
> +		num_clips--;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_atomic_plane_check - check plane state
>    * @old_plane_state: old plane state to check
> @@ -596,13 +666,12 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>   	struct drm_plane *plane = new_plane_state->plane;
>   	struct drm_crtc *crtc = new_plane_state->crtc;
>   	const struct drm_framebuffer *fb = new_plane_state->fb;
> -	unsigned int fb_width, fb_height;
> -	struct drm_mode_rect *clips;
> -	uint32_t num_clips;
>   	int ret;
>   
> -	/* either *both* CRTC and FB must be set, or neither */
> -	if (crtc && !fb) {
> +	/* When solid_fill is disabled,
> +	 * either *both* CRTC and FB must be set, or neither
> +	 */
> +	if (crtc && !drm_atomic_has_visible_data(new_plane_state)) {
>   		drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
>   			       plane->base.id, plane->name);
>   		return -EINVAL;
> @@ -625,17 +694,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>   		return -EINVAL;
>   	}
>   
> -	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> -					   fb->modifier);
> -	if (ret) {
> -		drm_dbg_atomic(plane->dev,
> -			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
> -			       plane->base.id, plane->name,
> -			       &fb->format->format, fb->modifier);
> -		return ret;
> -	}
> -
>   	/* Give drivers some help against integer overflows */
>   	if (new_plane_state->crtc_w > INT_MAX ||
>   	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||
> @@ -649,49 +707,11 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>   		return -ERANGE;
>   	}
>   
> -	fb_width = fb->width << 16;
> -	fb_height = fb->height << 16;
>   
> -	/* Make sure source coordinates are inside the fb. */
> -	if (new_plane_state->src_w > fb_width ||
> -	    new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> -	    new_plane_state->src_h > fb_height ||
> -	    new_plane_state->src_y > fb_height - new_plane_state->src_h) {
> -		drm_dbg_atomic(plane->dev,
> -			       "[PLANE:%d:%s] invalid source coordinates "
> -			       "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> -			       plane->base.id, plane->name,
> -			       new_plane_state->src_w >> 16,
> -			       ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
> -			       new_plane_state->src_h >> 16,
> -			       ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
> -			       new_plane_state->src_x >> 16,
> -			       ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
> -			       new_plane_state->src_y >> 16,
> -			       ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
> -			       fb->width, fb->height);
> -		return -ENOSPC;
> -	}
> -
> -	clips = __drm_plane_get_damage_clips(new_plane_state);
> -	num_clips = drm_plane_get_damage_clips_count(new_plane_state);
> -
> -	/* Make sure damage clips are valid and inside the fb. */
> -	while (num_clips > 0) {
> -		if (clips->x1 >= clips->x2 ||
> -		    clips->y1 >= clips->y2 ||
> -		    clips->x1 < 0 ||
> -		    clips->y1 < 0 ||
> -		    clips->x2 > fb_width ||
> -		    clips->y2 > fb_height) {
> -			drm_dbg_atomic(plane->dev,
> -				       "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n",
> -				       plane->base.id, plane->name, clips->x1,
> -				       clips->y1, clips->x2, clips->y2);
> -			return -EINVAL;
> -		}
> -		clips++;
> -		num_clips--;
> +	if (fb) {
> +		ret = drm_atomic_check_fb(new_plane_state);
> +		if (ret)
> +			return ret;
>   	}
>   
>   	if (plane_switching_crtc(old_plane_state, new_plane_state)) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1a586b3c454b..804ae107ae59 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>   	*src = drm_plane_state_src(plane_state);
>   	*dst = drm_plane_state_dest(plane_state);
>   
> -	if (!fb) {
> +	if (!fb && !drm_plane_solid_fill_enabled(plane_state)) {

You have the helper for this check.

>   		plane_state->visible = false;
>   		return 0;
>   	}
> @@ -881,25 +881,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>   		return -EINVAL;
>   	}
>   
> -	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
> +	if (fb) {
> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>   
> -	/* Check scaling */
> -	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> -	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> -	if (hscale < 0 || vscale < 0) {
> -		drm_dbg_kms(plane_state->plane->dev,
> -			    "Invalid scaling of plane\n");
> -		drm_rect_debug_print("src: ", &plane_state->src, true);
> -		drm_rect_debug_print("dst: ", &plane_state->dst, false);
> -		return -ERANGE;
> +		/* Check scaling */
> +		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> +		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> +
> +		if (hscale < 0 || vscale < 0) {
> +			drm_dbg_kms(plane_state->plane->dev,
> +					"Invalid scaling of plane\n");
> +			drm_rect_debug_print("src: ", &plane_state->src, true);
> +			drm_rect_debug_print("dst: ", &plane_state->dst, false);
> +			return -ERANGE;
> +		}
>   	}
>   
>   	if (crtc_state->enable)
>   		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>   
> -	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
> -
> -	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
> +	if (fb) {
> +		plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
> +		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
> +	} else if (drm_plane_solid_fill_enabled(plane_state)) {
> +		plane_state->visible = true;
> +	}
>   
>   	if (!plane_state->visible)
>   		/*
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 33357629a7f5..bdce2acbef6a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -856,8 +856,8 @@ static int __setplane_internal(struct drm_plane *plane,
>   
>   	WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>   
> -	/* No fb means shut it down */
> -	if (!fb) {
> +	/* No fb and no color fill means shut it down */
> +	if (!fb && !drm_plane_solid_fill_enabled(plane->state)) {

And here. And below.

>   		plane->old_fb = plane->fb;
>   		ret = plane->funcs->disable_plane(plane, ctx);
>   		if (!ret) {
> @@ -908,8 +908,8 @@ static int __setplane_atomic(struct drm_plane *plane,
>   
>   	WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
>   
> -	/* No fb means shut it down */
> -	if (!fb)
> +	/* No fb and no color fill means shut it down */
> +	if (!fb && !drm_plane_solid_fill_enabled(plane->state))
>   		return plane->funcs->disable_plane(plane, ctx);
>   
>   	/*
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 33f982cd1a27..a87997b3e0b5 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -230,8 +230,9 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
>   	 * Anything else should be considered a bug in the atomic core, so we
>   	 * gently warn about it.
>   	 */
> -	WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
> -		(new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
> +	WARN_ON(((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||

This condition also needs to be adjusted.

> +		(new_plane_state->crtc != NULL &&
> +		 !drm_atomic_has_visible_data(new_plane_state))));
>   
>   	return old_plane_state->crtc && !new_plane_state->crtc;
>   }
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 3b9da06f358b..3bc6b8d73e8a 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -977,6 +977,25 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
>   #define drm_for_each_plane(plane, dev) \
>   	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>   
> +/**
> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane
> + * @state: plane state
> + *
> + * Returns:
> + * Whether the plane has been assigned a solid_fill_blob
> + */
> +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state *state)
> +{
> +	return state && state->solid_fill_blob;
> +}
> +
> +static inline bool
> +drm_atomic_has_visible_data(const struct drm_plane_state *state)
> +{
> +	return state->fb || state->solid_fill_blob;
> +}
> +
> +
>   bool drm_any_plane_has_format(struct drm_device *dev,
>   			      u32 format, u64 modifier);
>   

-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 2/3] drm: Adjust atomic checks for solid fill color

[Jessica Zhang]

On 1/4/2023 5:57 PM, Dmitry Baryshkov wrote:
> On 05/01/2023 01:40, Jessica Zhang wrote:
>> Loosen the requirements for atomic and legacy commit so that, in cases
>> where solid fill planes is enabled (and FB_ID is NULL), the commit can
>> still go through.
>>
>> In addition, add framebuffer NULL checks in other areas to account for
>> FB being NULL when solid fill is enabled.
>>
>> Changes in V2:
>> - Changed to checks for if solid_fill_blob is set (Dmitry)
>> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>>    (Dmitry)
>> - Fixed indentation issue (Dmitry)
>>
>> Changes in V3:
>> - Created drm_plane_has_visible_data() helper and corrected CRTC and FB
>>    NULL-check logic (Dmitry)
>> - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted
>>    them into helper method (Dmitry)
>> - Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry)
>> - Fixed indentation (Dmitry)
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c        | 136 ++++++++++++++++------------
>>   drivers/gpu/drm/drm_atomic_helper.c |  34 ++++---
>>   drivers/gpu/drm/drm_plane.c         |   8 +-
>>   include/drm/drm_atomic_helper.h     |   5 +-
>>   include/drm/drm_plane.h             |  19 ++++
>>   5 files changed, 124 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f197f59f6d99..63f34b430479 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state 
>> *old_plane_state,
>>       return true;
>>   }
>> +static int drm_atomic_check_fb(const struct drm_plane_state *state)
> 
> This change should go to a separate patch. Please don't mix refactoring 
> (moving of the code) with the actual functionality changes.

Hi Dmitry,

Acked.

> 
>> +{
>> +    struct drm_plane *plane = state->plane;
>> +    const struct drm_framebuffer *fb = state->fb;
>> +    struct drm_mode_rect *clips;
>> +
>> +    uint32_t num_clips;
>> +    unsigned int fb_width, fb_height;
>> +    int ret;
>> +
>> +    /* Check whether this plane supports the fb pixel format. */
>> +    ret = drm_plane_check_pixel_format(plane, fb->format->format,
>> +                       fb->modifier);
>> +
>> +    if (ret) {
>> +        drm_dbg_atomic(plane->dev,
>> +                   "[PLANE:%d:%s] invalid pixel format %p4cc, 
>> modifier 0x%llx\n",
>> +                   plane->base.id, plane->name,
>> +                   &fb->format->format, fb->modifier);
>> +        return ret;
>> +    }
>> +
>> +    fb_width = fb->width << 16;
>> +    fb_height = fb->height << 16;
>> +
>> +    /* Make sure source coordinates are inside the fb. */
>> +    if (state->src_w > fb_width ||
>> +        state->src_x > fb_width - state->src_w ||
>> +        state->src_h > fb_height ||
>> +        state->src_y > fb_height - state->src_h) {
>> +        drm_dbg_atomic(plane->dev,
>> +                   "[PLANE:%d:%s] invalid source coordinates "
>> +                   "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
>> +                   plane->base.id, plane->name,
>> +                   state->src_w >> 16,
>> +                   ((state->src_w & 0xffff) * 15625) >> 10,
>> +                   state->src_h >> 16,
>> +                   ((state->src_h & 0xffff) * 15625) >> 10,
>> +                   state->src_x >> 16,
>> +                   ((state->src_x & 0xffff) * 15625) >> 10,
>> +                   state->src_y >> 16,
>> +                   ((state->src_y & 0xffff) * 15625) >> 10,
>> +                   fb->width, fb->height);
>> +        return -ENOSPC;
>> +    }
>> +
>> +    clips = __drm_plane_get_damage_clips(state);
>> +    num_clips = drm_plane_get_damage_clips_count(state);
>> +
>> +    /* Make sure damage clips are valid and inside the fb. */
>> +    while (num_clips > 0) {
>> +        if (clips->x1 >= clips->x2 ||
>> +            clips->y1 >= clips->y2 ||
>> +            clips->x1 < 0 ||
>> +            clips->y1 < 0 ||
>> +            clips->x2 > fb_width ||
>> +            clips->y2 > fb_height) {
>> +            drm_dbg_atomic(plane->dev,
>> +                       "[PLANE:%d:%s] invalid damage clip %d %d %d 
>> %d\n",
>> +                       plane->base.id, plane->name, clips->x1,
>> +                       clips->y1, clips->x2, clips->y2);
>> +            return -EINVAL;
>> +        }
>> +        clips++;
>> +        num_clips--;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * drm_atomic_plane_check - check plane state
>>    * @old_plane_state: old plane state to check
>> @@ -596,13 +666,12 @@ static int drm_atomic_plane_check(const struct 
>> drm_plane_state *old_plane_state,
>>       struct drm_plane *plane = new_plane_state->plane;
>>       struct drm_crtc *crtc = new_plane_state->crtc;
>>       const struct drm_framebuffer *fb = new_plane_state->fb;
>> -    unsigned int fb_width, fb_height;
>> -    struct drm_mode_rect *clips;
>> -    uint32_t num_clips;
>>       int ret;
>> -    /* either *both* CRTC and FB must be set, or neither */
>> -    if (crtc && !fb) {
>> +    /* When solid_fill is disabled,
>> +     * either *both* CRTC and FB must be set, or neither
>> +     */
>> +    if (crtc && !drm_atomic_has_visible_data(new_plane_state)) {
>>           drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no 
>> FB\n",
>>                      plane->base.id, plane->name);
>>           return -EINVAL;
>> @@ -625,17 +694,6 @@ static int drm_atomic_plane_check(const struct 
>> drm_plane_state *old_plane_state,
>>           return -EINVAL;
>>       }
>> -    /* Check whether this plane supports the fb pixel format. */
>> -    ret = drm_plane_check_pixel_format(plane, fb->format->format,
>> -                       fb->modifier);
>> -    if (ret) {
>> -        drm_dbg_atomic(plane->dev,
>> -                   "[PLANE:%d:%s] invalid pixel format %p4cc, 
>> modifier 0x%llx\n",
>> -                   plane->base.id, plane->name,
>> -                   &fb->format->format, fb->modifier);
>> -        return ret;
>> -    }
>> -
>>       /* Give drivers some help against integer overflows */
>>       if (new_plane_state->crtc_w > INT_MAX ||
>>           new_plane_state->crtc_x > INT_MAX - (int32_t) 
>> new_plane_state->crtc_w ||
>> @@ -649,49 +707,11 @@ static int drm_atomic_plane_check(const struct 
>> drm_plane_state *old_plane_state,
>>           return -ERANGE;
>>       }
>> -    fb_width = fb->width << 16;
>> -    fb_height = fb->height << 16;
>> -    /* Make sure source coordinates are inside the fb. */
>> -    if (new_plane_state->src_w > fb_width ||
>> -        new_plane_state->src_x > fb_width - new_plane_state->src_w ||
>> -        new_plane_state->src_h > fb_height ||
>> -        new_plane_state->src_y > fb_height - new_plane_state->src_h) {
>> -        drm_dbg_atomic(plane->dev,
>> -                   "[PLANE:%d:%s] invalid source coordinates "
>> -                   "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
>> -                   plane->base.id, plane->name,
>> -                   new_plane_state->src_w >> 16,
>> -                   ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
>> -                   new_plane_state->src_h >> 16,
>> -                   ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
>> -                   new_plane_state->src_x >> 16,
>> -                   ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
>> -                   new_plane_state->src_y >> 16,
>> -                   ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
>> -                   fb->width, fb->height);
>> -        return -ENOSPC;
>> -    }
>> -
>> -    clips = __drm_plane_get_damage_clips(new_plane_state);
>> -    num_clips = drm_plane_get_damage_clips_count(new_plane_state);
>> -
>> -    /* Make sure damage clips are valid and inside the fb. */
>> -    while (num_clips > 0) {
>> -        if (clips->x1 >= clips->x2 ||
>> -            clips->y1 >= clips->y2 ||
>> -            clips->x1 < 0 ||
>> -            clips->y1 < 0 ||
>> -            clips->x2 > fb_width ||
>> -            clips->y2 > fb_height) {
>> -            drm_dbg_atomic(plane->dev,
>> -                       "[PLANE:%d:%s] invalid damage clip %d %d %d 
>> %d\n",
>> -                       plane->base.id, plane->name, clips->x1,
>> -                       clips->y1, clips->x2, clips->y2);
>> -            return -EINVAL;
>> -        }
>> -        clips++;
>> -        num_clips--;
>> +    if (fb) {
>> +        ret = drm_atomic_check_fb(new_plane_state);
>> +        if (ret)
>> +            return ret;
>>       }
>>       if (plane_switching_crtc(old_plane_state, new_plane_state)) {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 1a586b3c454b..804ae107ae59 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct 
>> drm_plane_state *plane_state,
>>       *src = drm_plane_state_src(plane_state);
>>       *dst = drm_plane_state_dest(plane_state);
>> -    if (!fb) {
>> +    if (!fb && !drm_plane_solid_fill_enabled(plane_state)) {
> 
> You have the helper for this check.

Noted.

Thanks,

Jessica Zhang

> 
>>           plane_state->visible = false;
>>           return 0;
>>       }
>> @@ -881,25 +881,31 @@ int drm_atomic_helper_check_plane_state(struct 
>> drm_plane_state *plane_state,
>>           return -EINVAL;
>>       }
>> -    drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>> +    if (fb) {
>> +        drm_rect_rotate(src, fb->width << 16, fb->height << 16, 
>> rotation);
>> -    /* Check scaling */
>> -    hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>> -    vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>> -    if (hscale < 0 || vscale < 0) {
>> -        drm_dbg_kms(plane_state->plane->dev,
>> -                "Invalid scaling of plane\n");
>> -        drm_rect_debug_print("src: ", &plane_state->src, true);
>> -        drm_rect_debug_print("dst: ", &plane_state->dst, false);
>> -        return -ERANGE;
>> +        /* Check scaling */
>> +        hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>> +        vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>> +
>> +        if (hscale < 0 || vscale < 0) {
>> +            drm_dbg_kms(plane_state->plane->dev,
>> +                    "Invalid scaling of plane\n");
>> +            drm_rect_debug_print("src: ", &plane_state->src, true);
>> +            drm_rect_debug_print("dst: ", &plane_state->dst, false);
>> +            return -ERANGE;
>> +        }
>>       }
>>       if (crtc_state->enable)
>>           drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>> -    plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
>> -
>> -    drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, 
>> rotation);
>> +    if (fb) {
>> +        plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
>> +        drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, 
>> rotation);
>> +    } else if (drm_plane_solid_fill_enabled(plane_state)) {
>> +        plane_state->visible = true;
>> +    }
>>       if (!plane_state->visible)
>>           /*
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 33357629a7f5..bdce2acbef6a 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -856,8 +856,8 @@ static int __setplane_internal(struct drm_plane 
>> *plane,
>>       WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>> -    /* No fb means shut it down */
>> -    if (!fb) {
>> +    /* No fb and no color fill means shut it down */
>> +    if (!fb && !drm_plane_solid_fill_enabled(plane->state)) {
> 
> And here. And below.
> 
>>           plane->old_fb = plane->fb;
>>           ret = plane->funcs->disable_plane(plane, ctx);
>>           if (!ret) {
>> @@ -908,8 +908,8 @@ static int __setplane_atomic(struct drm_plane *plane,
>>       WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
>> -    /* No fb means shut it down */
>> -    if (!fb)
>> +    /* No fb and no color fill means shut it down */
>> +    if (!fb && !drm_plane_solid_fill_enabled(plane->state))
>>           return plane->funcs->disable_plane(plane, ctx);
>>       /*
>> diff --git a/include/drm/drm_atomic_helper.h 
>> b/include/drm/drm_atomic_helper.h
>> index 33f982cd1a27..a87997b3e0b5 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -230,8 +230,9 @@ drm_atomic_plane_disabling(struct drm_plane_state 
>> *old_plane_state,
>>        * Anything else should be considered a bug in the atomic core, 
>> so we
>>        * gently warn about it.
>>        */
>> -    WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != 
>> NULL) ||
>> -        (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
>> +    WARN_ON(((new_plane_state->crtc == NULL && new_plane_state->fb != 
>> NULL) ||
> 
> This condition also needs to be adjusted.
> 
>> +        (new_plane_state->crtc != NULL &&
>> +         !drm_atomic_has_visible_data(new_plane_state))));
>>       return old_plane_state->crtc && !new_plane_state->crtc;
>>   }
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 3b9da06f358b..3bc6b8d73e8a 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -977,6 +977,25 @@ static inline struct drm_plane 
>> *drm_plane_find(struct drm_device *dev,
>>   #define drm_for_each_plane(plane, dev) \
>>       list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>> +/**
>> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on 
>> plane
>> + * @state: plane state
>> + *
>> + * Returns:
>> + * Whether the plane has been assigned a solid_fill_blob
>> + */
>> +static inline bool drm_plane_solid_fill_enabled(struct 
>> drm_plane_state *state)
>> +{
>> +    return state && state->solid_fill_blob;
>> +}
>> +
>> +static inline bool
>> +drm_atomic_has_visible_data(const struct drm_plane_state *state)
>> +{
>> +    return state->fb || state->solid_fill_blob;
>> +}
>> +
>> +
>>   bool drm_any_plane_has_format(struct drm_device *dev,
>>                     u32 format, u64 modifier);
> 
> -- 
> With best wishes
> Dmitry
> 

[RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes

[Jessica Zhang]

Initialize and use the color_fill properties for planes in DPU driver. In
addition, relax framebuffer requirements within atomic commit path and
add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
as it's unused.

Changes since V2:
- Fixed dropped 'const' warning
- Dropped use of solid_fill_format
- Switched to using drm_plane_solid_fill_enabled helper method
- Added helper to convert color fill to BGR888 (Rob)
- Added support for solid fill on planes of varying sizes
- Removed DPU_PLANE_COLOR_FILL_FLAG

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  9 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++++++++++++++---------
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..0695b70ea1b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
 	struct drm_plane_state *state;
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
 	struct dpu_plane_state *pstate = NULL;
+	const struct msm_format *fmt;
 	struct dpu_format *format;
 	struct dpu_hw_ctl *ctl = mixer->lm_ctl;
 
@@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
 				sspp_idx - SSPP_VIG0,
 				state->fb ? state->fb->base.id : -1);
 
-		format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+		if (pstate->base.fb)
+			fmt = msm_framebuffer_format(pstate->base.fb);
+		else
+			fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
+					DRM_FORMAT_ABGR8888, 0);
+
+		format = to_dpu_format(fmt);
 
 		if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
 			bg_alpha_enable = true;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 86719020afe2..51a7507373f7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -44,7 +44,6 @@
 
 #define DPU_NAME_SIZE  12
 
-#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
 #define DPU_ZPOS_MAX 255
 
 /* multirect rect index */
@@ -105,7 +104,6 @@ struct dpu_plane {
 	enum dpu_sspp pipe;
 
 	struct dpu_hw_pipe *pipe_hw;
-	uint32_t color_fill;
 	bool is_error;
 	bool is_rt_pipe;
 	const struct dpu_mdss_cfg *catalog;
@@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
 				&scaler3_cfg);
 }
 
+static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
+{
+	uint32_t ret = 0;
+
+	ret |= ((uint8_t) solid_fill.b) << 16;
+	ret |= ((uint8_t) solid_fill.g) << 8;
+	ret |= ((uint8_t) solid_fill.r);
+
+	return ret;
+}
+
 /**
  * _dpu_plane_color_fill - enables color fill on plane
  * @pdpu:   Pointer to DPU plane object
@@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
 
 	dst = drm_plane_state_dest(new_plane_state);
 
-	fb_rect.x2 = new_plane_state->fb->width;
-	fb_rect.y2 = new_plane_state->fb->height;
+	if (new_plane_state->fb) {
+		fb_rect.x2 = new_plane_state->fb->width;
+		fb_rect.y2 = new_plane_state->fb->height;
+	}
 
 	max_linewidth = pdpu->catalog->caps->max_linewidth;
 
-	fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+	if (new_plane_state->fb)
+		fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+	else
+		fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
 
 	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
 
@@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 
 	/* check src bounds */
-	} else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
+	} else if (new_plane_state->fb && !dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
 		DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
 				DRM_RECT_ARG(&src));
 		return -E2BIG;
@@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
 	if (pdpu->is_error)
 		/* force white frame with 100% alpha pipe output on error */
 		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
-	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
+	else if (!(plane->state->fb) && drm_plane_solid_fill_enabled(plane->state))
 		/* force 100% alpha */
-		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
+				0xFF);
 	else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
 		const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
 		const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
@@ -1127,23 +1142,30 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 	struct drm_crtc *crtc = state->crtc;
 	struct drm_framebuffer *fb = state->fb;
 	bool is_rt_pipe, update_qos_remap;
-	const struct dpu_format *fmt =
-		to_dpu_format(msm_framebuffer_format(fb));
+	const struct dpu_format *fmt;
 	struct dpu_hw_pipe_cfg pipe_cfg;
 
-	memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
-
-	_dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
-
 	pstate->pending = true;
 
 	is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
 	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
 
-	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
-			", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
-			crtc->base.id, DRM_RECT_ARG(&state->dst),
-			(char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
+	/* override for color fill */
+	if (!fb && drm_plane_solid_fill_enabled(plane->state)) {
+		/* skip remaining processing on color fill */
+		return;
+	}
+
+	memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
+
+	fmt = to_dpu_format(msm_framebuffer_format(fb));
+	_dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
+
+	if (fb)
+		DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
+				", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
+				crtc->base.id, DRM_RECT_ARG(&state->dst),
+				(char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
 
 	pipe_cfg.src_rect = state->src;
 
@@ -1155,12 +1177,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 
 	pipe_cfg.dst_rect = state->dst;
 
-	/* override for color fill */
-	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
-		/* skip remaining processing on color fill */
-		return;
-	}
-
 	if (pdpu->pipe_hw->ops.setup_rects) {
 		pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
 				&pipe_cfg,
@@ -1511,6 +1527,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
 		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
 
 	drm_plane_create_alpha_property(plane);
+	drm_plane_create_solid_fill_property(plane);
 	drm_plane_create_blend_mode_property(plane,
 			BIT(DRM_MODE_BLEND_PIXEL_NONE) |
 			BIT(DRM_MODE_BLEND_PREMULTI) |
-- 
2.38.1

Re: [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes

[Dmitry Baryshkov]

On 05/01/2023 01:40, Jessica Zhang wrote:
> Initialize and use the color_fill properties for planes in DPU driver. In
> addition, relax framebuffer requirements within atomic commit path and
> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
> as it's unused.
> 
> Changes since V2:
> - Fixed dropped 'const' warning
> - Dropped use of solid_fill_format
> - Switched to using drm_plane_solid_fill_enabled helper method
> - Added helper to convert color fill to BGR888 (Rob)
> - Added support for solid fill on planes of varying sizes
> - Removed DPU_PLANE_COLOR_FILL_FLAG
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  9 +++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++++++++++++++---------
>   2 files changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 13ce321283ff..0695b70ea1b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   	struct drm_plane_state *state;
>   	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>   	struct dpu_plane_state *pstate = NULL;
> +	const struct msm_format *fmt;
>   	struct dpu_format *format;
>   	struct dpu_hw_ctl *ctl = mixer->lm_ctl;
>   
> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   				sspp_idx - SSPP_VIG0,
>   				state->fb ? state->fb->base.id : -1);
>   
> -		format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> +		if (pstate->base.fb)
> +			fmt = msm_framebuffer_format(pstate->base.fb);
> +		else
> +			fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> +					DRM_FORMAT_ABGR8888, 0);
> +
> +		format = to_dpu_format(fmt);
>   
>   		if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
>   			bg_alpha_enable = true;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 86719020afe2..51a7507373f7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -44,7 +44,6 @@
>   
>   #define DPU_NAME_SIZE  12
>   
> -#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
>   #define DPU_ZPOS_MAX 255
>   
>   /* multirect rect index */
> @@ -105,7 +104,6 @@ struct dpu_plane {
>   	enum dpu_sspp pipe;
>   
>   	struct dpu_hw_pipe *pipe_hw;
> -	uint32_t color_fill;
>   	bool is_error;
>   	bool is_rt_pipe;
>   	const struct dpu_mdss_cfg *catalog;
> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
>   				&scaler3_cfg);
>   }
>   
> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
> +{
> +	uint32_t ret = 0;
> +
> +	ret |= ((uint8_t) solid_fill.b) << 16;
> +	ret |= ((uint8_t) solid_fill.g) << 8;
> +	ret |= ((uint8_t) solid_fill.r);
> +
> +	return ret;
> +}
> +
>   /**
>    * _dpu_plane_color_fill - enables color fill on plane
>    * @pdpu:   Pointer to DPU plane object
> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   
>   	dst = drm_plane_state_dest(new_plane_state);
>   
> -	fb_rect.x2 = new_plane_state->fb->width;
> -	fb_rect.y2 = new_plane_state->fb->height;
> +	if (new_plane_state->fb) {
> +		fb_rect.x2 = new_plane_state->fb->width;
> +		fb_rect.y2 = new_plane_state->fb->height;
> +	}
>   
>   	max_linewidth = pdpu->catalog->caps->max_linewidth;
>   
> -	fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> +	if (new_plane_state->fb)
> +		fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> +	else
> +		fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);

I think this should be more explicit:

if (solid_fill)
    fmt = dpu_get_dpu_format(...)
else
    fmt = to_dpu_format(msm_framebuffer_format(...).

And in the _dpu_crtc_blend_setup_mixer() too.

Maybe the code can be extracted to a helper.

>   
>   	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>   
> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   		return -EINVAL;
>   
>   	/* check src bounds */
> -	} else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
> +	} else if (new_plane_state->fb && !dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
>   		DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>   				DRM_RECT_ARG(&src));
>   		return -E2BIG;
> @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
>   	if (pdpu->is_error)
>   		/* force white frame with 100% alpha pipe output on error */
>   		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> -	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> +	else if (!(plane->state->fb) && drm_plane_solid_fill_enabled(plane->state))
>   		/* force 100% alpha */
> -		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> +		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
> +				0xFF);

I'd push alpha into _dpu_plane_get_fill_color(). Then adding alpha 
support would be more transparent.

>   	else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
>   		const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
>   		const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
> @@ -1127,23 +1142,30 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   	struct drm_crtc *crtc = state->crtc;
>   	struct drm_framebuffer *fb = state->fb;
>   	bool is_rt_pipe, update_qos_remap;
> -	const struct dpu_format *fmt =
> -		to_dpu_format(msm_framebuffer_format(fb));
> +	const struct dpu_format *fmt;
>   	struct dpu_hw_pipe_cfg pipe_cfg;
>   
> -	memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
> -
> -	_dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
> -
>   	pstate->pending = true;
>   
>   	is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>   	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
>   
> -	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
> -			", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
> -			crtc->base.id, DRM_RECT_ARG(&state->dst),
> -			(char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
> +	/* override for color fill */
> +	if (!fb && drm_plane_solid_fill_enabled(plane->state)) {
> +		/* skip remaining processing on color fill */
> +		return;
> +	}
> +
> +	memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
> +
> +	fmt = to_dpu_format(msm_framebuffer_format(fb));
> +	_dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);

This change deserves a comment somewhere (in the commit message?). 
Beforehand the driver tried to set the scanout/layout for the COLOR_FILL 
case. You have changed this.

> +
> +	if (fb)
> +		DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
> +				", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
> +				crtc->base.id, DRM_RECT_ARG(&state->dst),
> +				(char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
>   
>   	pipe_cfg.src_rect = state->src;
>   
> @@ -1155,12 +1177,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   
>   	pipe_cfg.dst_rect = state->dst;
>   
> -	/* override for color fill */
> -	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
> -		/* skip remaining processing on color fill */
> -		return;
> -	}
> -
>   	if (pdpu->pipe_hw->ops.setup_rects) {
>   		pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
>   				&pipe_cfg,
> @@ -1511,6 +1527,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>   		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>   
>   	drm_plane_create_alpha_property(plane);
> +	drm_plane_create_solid_fill_property(plane);
>   	drm_plane_create_blend_mode_property(plane,
>   			BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>   			BIT(DRM_MODE_BLEND_PREMULTI) |

-- 
With best wishes
Dmitry

Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes

[Jessica Zhang]

On 1/4/2023 6:16 PM, Dmitry Baryshkov wrote:
> On 05/01/2023 01:40, Jessica Zhang wrote:
>> Initialize and use the color_fill properties for planes in DPU driver. In
>> addition, relax framebuffer requirements within atomic commit path and
>> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
>> as it's unused.
>>
>> Changes since V2:
>> - Fixed dropped 'const' warning
>> - Dropped use of solid_fill_format
>> - Switched to using drm_plane_solid_fill_enabled helper method
>> - Added helper to convert color fill to BGR888 (Rob)
>> - Added support for solid fill on planes of varying sizes
>> - Removed DPU_PLANE_COLOR_FILL_FLAG
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  9 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++++++++++++++---------
>>   2 files changed, 49 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 13ce321283ff..0695b70ea1b7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct 
>> drm_crtc *crtc,
>>       struct drm_plane_state *state;
>>       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>>       struct dpu_plane_state *pstate = NULL;
>> +    const struct msm_format *fmt;
>>       struct dpu_format *format;
>>       struct dpu_hw_ctl *ctl = mixer->lm_ctl;
>> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct 
>> drm_crtc *crtc,
>>                   sspp_idx - SSPP_VIG0,
>>                   state->fb ? state->fb->base.id : -1);
>> -        format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
>> +        if (pstate->base.fb)
>> +            fmt = msm_framebuffer_format(pstate->base.fb);
>> +        else
>> +            fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
>> +                    DRM_FORMAT_ABGR8888, 0);
>> +
>> +        format = to_dpu_format(fmt);
>>           if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
>>               bg_alpha_enable = true;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index 86719020afe2..51a7507373f7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -44,7 +44,6 @@
>>   #define DPU_NAME_SIZE  12
>> -#define DPU_PLANE_COLOR_FILL_FLAG    BIT(31)
>>   #define DPU_ZPOS_MAX 255
>>   /* multirect rect index */
>> @@ -105,7 +104,6 @@ struct dpu_plane {
>>       enum dpu_sspp pipe;
>>       struct dpu_hw_pipe *pipe_hw;
>> -    uint32_t color_fill;
>>       bool is_error;
>>       bool is_rt_pipe;
>>       const struct dpu_mdss_cfg *catalog;
>> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct 
>> dpu_plane *pdpu,
>>                   &scaler3_cfg);
>>   }
>> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill 
>> solid_fill)
>> +{
>> +    uint32_t ret = 0;
>> +
>> +    ret |= ((uint8_t) solid_fill.b) << 16;
>> +    ret |= ((uint8_t) solid_fill.g) << 8;
>> +    ret |= ((uint8_t) solid_fill.r);
>> +
>> +    return ret;
>> +}
>> +
>>   /**
>>    * _dpu_plane_color_fill - enables color fill on plane
>>    * @pdpu:   Pointer to DPU plane object
>> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct 
>> drm_plane *plane,
>>       dst = drm_plane_state_dest(new_plane_state);
>> -    fb_rect.x2 = new_plane_state->fb->width;
>> -    fb_rect.y2 = new_plane_state->fb->height;
>> +    if (new_plane_state->fb) {
>> +        fb_rect.x2 = new_plane_state->fb->width;
>> +        fb_rect.y2 = new_plane_state->fb->height;
>> +    }
>>       max_linewidth = pdpu->catalog->caps->max_linewidth;
>> -    fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>> +    if (new_plane_state->fb)
>> +        fmt = 
>> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>> +    else
>> +        fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
> 
> I think this should be more explicit:
> 
> if (solid_fill)
>     fmt = dpu_get_dpu_format(...)
> else
>     fmt = to_dpu_format(msm_framebuffer_format(...).
> 
> And in the _dpu_crtc_blend_setup_mixer() too.

Hi Dmitry,

Noted.

> 
> Maybe the code can be extracted to a helper.
> 
>>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct 
>> drm_plane *plane,
>>           return -EINVAL;
>>       /* check src bounds */
>> -    } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
>> +    } else if (new_plane_state->fb && !dpu_plane_validate_src(&src, 
>> &fb_rect, min_src_size)) {
>>           DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>>                   DRM_RECT_ARG(&src));
>>           return -E2BIG;
>> @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
>>       if (pdpu->is_error)
>>           /* force white frame with 100% alpha pipe output on error */
>>           _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
>> -    else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>> +    else if (!(plane->state->fb) && 
>> drm_plane_solid_fill_enabled(plane->state))
>>           /* force 100% alpha */
>> -        _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>> +        _dpu_plane_color_fill(pdpu, 
>> _dpu_plane_get_fill_color(plane->state->solid_fill),
>> +                0xFF);
> 
> I'd push alpha into _dpu_plane_get_fill_color(). Then adding alpha 
> support would be more transparent.

Acked.

> 
>>       else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
>>           const struct dpu_format *fmt = 
>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>>           const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, 
>> fmt);
>> @@ -1127,23 +1142,30 @@ static void 
>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>       struct drm_crtc *crtc = state->crtc;
>>       struct drm_framebuffer *fb = state->fb;
>>       bool is_rt_pipe, update_qos_remap;
>> -    const struct dpu_format *fmt =
>> -        to_dpu_format(msm_framebuffer_format(fb));
>> +    const struct dpu_format *fmt;
>>       struct dpu_hw_pipe_cfg pipe_cfg;
>> -    memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
>> -
>> -    _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
>> -
>>       pstate->pending = true;
>>       is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>>       _dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
>> -    DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " 
>> DRM_RECT_FMT
>> -            ", %4.4s ubwc %d\n", fb->base.id, 
>> DRM_RECT_FP_ARG(&state->src),
>> -            crtc->base.id, DRM_RECT_ARG(&state->dst),
>> -            (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
>> +    /* override for color fill */
>> +    if (!fb && drm_plane_solid_fill_enabled(plane->state)) {
>> +        /* skip remaining processing on color fill */
>> +        return;
>> +    }
>> +
>> +    memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
>> +
>> +    fmt = to_dpu_format(msm_framebuffer_format(fb));
>> +    _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
> 
> This change deserves a comment somewhere (in the commit message?). 
> Beforehand the driver tried to set the scanout/layout for the COLOR_FILL 
> case. You have changed this.

Sounds good.

Thanks,

Jessica Zhang

> 
>> +
>> +    if (fb)
>> +        DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " 
>> DRM_RECT_FMT
>> +                ", %4.4s ubwc %d\n", fb->base.id, 
>> DRM_RECT_FP_ARG(&state->src),
>> +                crtc->base.id, DRM_RECT_ARG(&state->dst),
>> +                (char *)&fmt->base.pixel_format, 
>> DPU_FORMAT_IS_UBWC(fmt));
>>       pipe_cfg.src_rect = state->src;
>> @@ -1155,12 +1177,6 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>>       pipe_cfg.dst_rect = state->dst;
>> -    /* override for color fill */
>> -    if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
>> -        /* skip remaining processing on color fill */
>> -        return;
>> -    }
>> -
>>       if (pdpu->pipe_hw->ops.setup_rects) {
>>           pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
>>                   &pipe_cfg,
>> @@ -1511,6 +1527,7 @@ struct drm_plane *dpu_plane_init(struct 
>> drm_device *dev,
>>           DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>>       drm_plane_create_alpha_property(plane);
>> +    drm_plane_create_solid_fill_property(plane);
>>       drm_plane_create_blend_mode_property(plane,
>>               BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>>               BIT(DRM_MODE_BLEND_PREMULTI) |
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes

[Dmitry Baryshkov]

On Fri, 6 Jan 2023 at 22:57, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> On 1/4/2023 6:16 PM, Dmitry Baryshkov wrote:
> > On 05/01/2023 01:40, Jessica Zhang wrote:
> >> Initialize and use the color_fill properties for planes in DPU driver. In
> >> addition, relax framebuffer requirements within atomic commit path and
> >> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
> >> as it's unused.
> >>
> >> Changes since V2:
> >> - Fixed dropped 'const' warning
> >> - Dropped use of solid_fill_format
> >> - Switched to using drm_plane_solid_fill_enabled helper method
> >> - Added helper to convert color fill to BGR888 (Rob)
> >> - Added support for solid fill on planes of varying sizes
> >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  9 +++-
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++++++++++++++---------
> >>   2 files changed, 49 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index 13ce321283ff..0695b70ea1b7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >>       struct drm_plane_state *state;
> >>       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> >>       struct dpu_plane_state *pstate = NULL;
> >> +    const struct msm_format *fmt;
> >>       struct dpu_format *format;
> >>       struct dpu_hw_ctl *ctl = mixer->lm_ctl;
> >> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >>                   sspp_idx - SSPP_VIG0,
> >>                   state->fb ? state->fb->base.id : -1);
> >> -        format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> >> +        if (pstate->base.fb)
> >> +            fmt = msm_framebuffer_format(pstate->base.fb);
> >> +        else
> >> +            fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> >> +                    DRM_FORMAT_ABGR8888, 0);
> >> +
> >> +        format = to_dpu_format(fmt);
> >>           if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> >>               bg_alpha_enable = true;
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> index 86719020afe2..51a7507373f7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -44,7 +44,6 @@
> >>   #define DPU_NAME_SIZE  12
> >> -#define DPU_PLANE_COLOR_FILL_FLAG    BIT(31)
> >>   #define DPU_ZPOS_MAX 255
> >>   /* multirect rect index */
> >> @@ -105,7 +104,6 @@ struct dpu_plane {
> >>       enum dpu_sspp pipe;
> >>       struct dpu_hw_pipe *pipe_hw;
> >> -    uint32_t color_fill;
> >>       bool is_error;
> >>       bool is_rt_pipe;
> >>       const struct dpu_mdss_cfg *catalog;
> >> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct
> >> dpu_plane *pdpu,
> >>                   &scaler3_cfg);
> >>   }
> >> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill
> >> solid_fill)
> >> +{
> >> +    uint32_t ret = 0;
> >> +
> >> +    ret |= ((uint8_t) solid_fill.b) << 16;
> >> +    ret |= ((uint8_t) solid_fill.g) << 8;
> >> +    ret |= ((uint8_t) solid_fill.r);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>   /**
> >>    * _dpu_plane_color_fill - enables color fill on plane
> >>    * @pdpu:   Pointer to DPU plane object
> >> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >>       dst = drm_plane_state_dest(new_plane_state);
> >> -    fb_rect.x2 = new_plane_state->fb->width;
> >> -    fb_rect.y2 = new_plane_state->fb->height;
> >> +    if (new_plane_state->fb) {
> >> +        fb_rect.x2 = new_plane_state->fb->width;
> >> +        fb_rect.y2 = new_plane_state->fb->height;
> >> +    }
> >>       max_linewidth = pdpu->catalog->caps->max_linewidth;
> >> -    fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +    if (new_plane_state->fb)
> >> +        fmt =
> >> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +    else
> >> +        fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
> >
> > I think this should be more explicit:
> >
> > if (solid_fill)
> >     fmt = dpu_get_dpu_format(...)
> > else
> >     fmt = to_dpu_format(msm_framebuffer_format(...).
> >
> > And in the _dpu_crtc_blend_setup_mixer() too.
>
> Hi Dmitry,
>
> Noted.
>
> >
> > Maybe the code can be extracted to a helper.
> >
> >>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >>           return -EINVAL;
> >>       /* check src bounds */
> >> -    } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
> >> +    } else if (new_plane_state->fb && !dpu_plane_validate_src(&src,
> >> &fb_rect, min_src_size)) {
> >>           DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> >>                   DRM_RECT_ARG(&src));
> >>           return -E2BIG;
> >> @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
> >>       if (pdpu->is_error)
> >>           /* force white frame with 100% alpha pipe output on error */
> >>           _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> >> -    else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> >> +    else if (!(plane->state->fb) &&
> >> drm_plane_solid_fill_enabled(plane->state))
> >>           /* force 100% alpha */
> >> -        _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> >> +        _dpu_plane_color_fill(pdpu,
> >> _dpu_plane_get_fill_color(plane->state->solid_fill),
> >> +                0xFF);
> >
> > I'd push alpha into _dpu_plane_get_fill_color(). Then adding alpha
> > support would be more transparent.
>
> Acked.

Actually after our discussion I wanted to discuss this with you. We
pass the plane's alpha value and blending mode using LM_BLEND*
registers. Does that integrate correctly with the alpha part of
SSPP_SRC_CONSTANT_COLOR?


-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Daniel Vetter]

On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> Introduce and add support for a solid_fill property. When the solid_fill
> property is set, and the framebuffer is set to NULL, memory fetch will be
> disabled.
> 
> In addition, loosen the NULL FB checks within the atomic commit callstack
> to allow a NULL FB when the solid_fill property is set and add FB checks
> in methods where the FB was previously assumed to be non-NULL.
> 
> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> callstack to account for a NULL FB in cases where solid_fill is set.
> 
> Some drivers support hardware that have optimizations for solid fill
> planes. This series aims to expose these capabilities to userspace as
> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> hardware composer HAL) that can be set by apps like the Android Gears
> app.
> 
> Userspace can set the solid_fill property to a blob containing the
> appropriate version number and solid fill color (in RGB323232 format) and
> setting the framebuffer to NULL.
> 
> Note: Currently, there's only one version of the solid_fill blob property.
> However if other drivers want to support a similar feature, but require
> more than just the solid fill color, they can extend this feature by
> creating additional versions of the drm_solid_fill struct.
> 
> Changes in V2:
> - Dropped SOLID_FILL_FORMAT property (Simon)
> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> - Changed to checks for if solid_fill_blob is set (Dmitry)
> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>   (Dmitry)
> - Removed DPU_PLANE_COLOR_FILL_FLAG
> - Fixed whitespace and indentation issues (Dmitry)

Now that this is a blob, I do wonder again whether it's not cleaner to set
the blob as the FB pointer. Or create some kind other kind of special data
source objects (because solid fill is by far not the only such thing).

We'd still end up in special cases like when userspace that doesn't
understand solid fill tries to read out such a framebuffer, but these
cases already exist anyway for lack of priviledges.

So I still think that feels like the more consistent way to integrate this
feature. Which doesn't mean it has to happen like that, but the
patches/cover letter should at least explain why we don't do it like this.
-Daniel

> 
> Changes in V3:
> - Fixed some logic errors in atomic checks (Dmitry)
> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
>   methods (Dmitry)
> 
> Jessica Zhang (3):
>   drm: Introduce solid fill property for drm plane
>   drm: Adjust atomic checks for solid fill color
>   drm/msm/dpu: Use color_fill property for DPU planes
> 
>  drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
>  drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
>  drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
>  drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
>  drivers/gpu/drm/drm_blend.c               |  17 +++
>  drivers/gpu/drm/drm_plane.c               |   8 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
>  include/drm/drm_atomic_helper.h           |   5 +-
>  include/drm/drm_blend.h                   |   1 +
>  include/drm/drm_plane.h                   |  62 ++++++++++
>  11 files changed, 302 insertions(+), 103 deletions(-)
> 
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Jessica Zhang]

On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
>> Introduce and add support for a solid_fill property. When the solid_fill
>> property is set, and the framebuffer is set to NULL, memory fetch will be
>> disabled.
>>
>> In addition, loosen the NULL FB checks within the atomic commit callstack
>> to allow a NULL FB when the solid_fill property is set and add FB checks
>> in methods where the FB was previously assumed to be non-NULL.
>>
>> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
>> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
>> callstack to account for a NULL FB in cases where solid_fill is set.
>>
>> Some drivers support hardware that have optimizations for solid fill
>> planes. This series aims to expose these capabilities to userspace as
>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>> hardware composer HAL) that can be set by apps like the Android Gears
>> app.
>>
>> Userspace can set the solid_fill property to a blob containing the
>> appropriate version number and solid fill color (in RGB323232 format) and
>> setting the framebuffer to NULL.
>>
>> Note: Currently, there's only one version of the solid_fill blob property.
>> However if other drivers want to support a similar feature, but require
>> more than just the solid fill color, they can extend this feature by
>> creating additional versions of the drm_solid_fill struct.
>>
>> Changes in V2:
>> - Dropped SOLID_FILL_FORMAT property (Simon)
>> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
>> - Changed to checks for if solid_fill_blob is set (Dmitry)
>> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>>    (Dmitry)
>> - Removed DPU_PLANE_COLOR_FILL_FLAG
>> - Fixed whitespace and indentation issues (Dmitry)
> 
> Now that this is a blob, I do wonder again whether it's not cleaner to set
> the blob as the FB pointer. Or create some kind other kind of special data
> source objects (because solid fill is by far not the only such thing).
> 
> We'd still end up in special cases like when userspace that doesn't
> understand solid fill tries to read out such a framebuffer, but these
> cases already exist anyway for lack of priviledges.
> 
> So I still think that feels like the more consistent way to integrate this
> feature. Which doesn't mean it has to happen like that, but the
> patches/cover letter should at least explain why we don't do it like this.

Hi Daniel,

IIRC we were facing some issues with this check [1] when trying to set 
FB to a PROP_BLOB instead. Which is why we went with making it a 
separate property instead. Will mention this in the cover letter.

[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71

Thanks,

Jessica Zhang

> -Daniel
> 
>>
>> Changes in V3:
>> - Fixed some logic errors in atomic checks (Dmitry)
>> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
>>    methods (Dmitry)
>>
>> Jessica Zhang (3):
>>    drm: Introduce solid fill property for drm plane
>>    drm: Adjust atomic checks for solid fill color
>>    drm/msm/dpu: Use color_fill property for DPU planes
>>
>>   drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
>>   drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
>>   drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
>>   drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
>>   drivers/gpu/drm/drm_blend.c               |  17 +++
>>   drivers/gpu/drm/drm_plane.c               |   8 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
>>   include/drm/drm_atomic_helper.h           |   5 +-
>>   include/drm/drm_blend.h                   |   1 +
>>   include/drm/drm_plane.h                   |  62 ++++++++++
>>   11 files changed, 302 insertions(+), 103 deletions(-)
>>
>> -- 
>> 2.38.1
>>
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Dmitry Baryshkov]

On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> >> Introduce and add support for a solid_fill property. When the solid_fill
> >> property is set, and the framebuffer is set to NULL, memory fetch will be
> >> disabled.
> >>
> >> In addition, loosen the NULL FB checks within the atomic commit callstack
> >> to allow a NULL FB when the solid_fill property is set and add FB checks
> >> in methods where the FB was previously assumed to be non-NULL.
> >>
> >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> >> callstack to account for a NULL FB in cases where solid_fill is set.
> >>
> >> Some drivers support hardware that have optimizations for solid fill
> >> planes. This series aims to expose these capabilities to userspace as
> >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> >> hardware composer HAL) that can be set by apps like the Android Gears
> >> app.
> >>
> >> Userspace can set the solid_fill property to a blob containing the
> >> appropriate version number and solid fill color (in RGB323232 format) and
> >> setting the framebuffer to NULL.
> >>
> >> Note: Currently, there's only one version of the solid_fill blob property.
> >> However if other drivers want to support a similar feature, but require
> >> more than just the solid fill color, they can extend this feature by
> >> creating additional versions of the drm_solid_fill struct.
> >>
> >> Changes in V2:
> >> - Dropped SOLID_FILL_FORMAT property (Simon)
> >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> >> - Changed to checks for if solid_fill_blob is set (Dmitry)
> >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> >>    (Dmitry)
> >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> >> - Fixed whitespace and indentation issues (Dmitry)
> >
> > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > the blob as the FB pointer. Or create some kind other kind of special data
> > source objects (because solid fill is by far not the only such thing).
> >
> > We'd still end up in special cases like when userspace that doesn't
> > understand solid fill tries to read out such a framebuffer, but these
> > cases already exist anyway for lack of priviledges.
> >
> > So I still think that feels like the more consistent way to integrate this
> > feature. Which doesn't mean it has to happen like that, but the
> > patches/cover letter should at least explain why we don't do it like this.
>
> Hi Daniel,
>
> IIRC we were facing some issues with this check [1] when trying to set
> FB to a PROP_BLOB instead. Which is why we went with making it a
> separate property instead. Will mention this in the cover letter.

What kind of issues? Could you please describe them?

>
> [1]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
>
> Thanks,
>
> Jessica Zhang
>
> > -Daniel
> >
> >>
> >> Changes in V3:
> >> - Fixed some logic errors in atomic checks (Dmitry)
> >> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
> >>    methods (Dmitry)
> >>
> >> Jessica Zhang (3):
> >>    drm: Introduce solid fill property for drm plane
> >>    drm: Adjust atomic checks for solid fill color
> >>    drm/msm/dpu: Use color_fill property for DPU planes
> >>
> >>   drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
> >>   drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
> >>   drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
> >>   drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
> >>   drivers/gpu/drm/drm_blend.c               |  17 +++
> >>   drivers/gpu/drm/drm_plane.c               |   8 +-
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
> >>   include/drm/drm_atomic_helper.h           |   5 +-
> >>   include/drm/drm_blend.h                   |   1 +
> >>   include/drm/drm_plane.h                   |  62 ++++++++++
> >>   11 files changed, 302 insertions(+), 103 deletions(-)
> >>
> >> --
> >> 2.38.1
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Daniel Vetter]

On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
> On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >
> >
> >
> > On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> > >> Introduce and add support for a solid_fill property. When the solid_fill
> > >> property is set, and the framebuffer is set to NULL, memory fetch will be
> > >> disabled.
> > >>
> > >> In addition, loosen the NULL FB checks within the atomic commit callstack
> > >> to allow a NULL FB when the solid_fill property is set and add FB checks
> > >> in methods where the FB was previously assumed to be non-NULL.
> > >>
> > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > >> callstack to account for a NULL FB in cases where solid_fill is set.
> > >>
> > >> Some drivers support hardware that have optimizations for solid fill
> > >> planes. This series aims to expose these capabilities to userspace as
> > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > >> hardware composer HAL) that can be set by apps like the Android Gears
> > >> app.
> > >>
> > >> Userspace can set the solid_fill property to a blob containing the
> > >> appropriate version number and solid fill color (in RGB323232 format) and
> > >> setting the framebuffer to NULL.
> > >>
> > >> Note: Currently, there's only one version of the solid_fill blob property.
> > >> However if other drivers want to support a similar feature, but require
> > >> more than just the solid fill color, they can extend this feature by
> > >> creating additional versions of the drm_solid_fill struct.
> > >>
> > >> Changes in V2:
> > >> - Dropped SOLID_FILL_FORMAT property (Simon)
> > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > >> - Changed to checks for if solid_fill_blob is set (Dmitry)
> > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > >>    (Dmitry)
> > >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> > >> - Fixed whitespace and indentation issues (Dmitry)
> > >
> > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > the blob as the FB pointer. Or create some kind other kind of special data
> > > source objects (because solid fill is by far not the only such thing).
> > >
> > > We'd still end up in special cases like when userspace that doesn't
> > > understand solid fill tries to read out such a framebuffer, but these
> > > cases already exist anyway for lack of priviledges.
> > >
> > > So I still think that feels like the more consistent way to integrate this
> > > feature. Which doesn't mean it has to happen like that, but the
> > > patches/cover letter should at least explain why we don't do it like this.
> >
> > Hi Daniel,
> >
> > IIRC we were facing some issues with this check [1] when trying to set
> > FB to a PROP_BLOB instead. Which is why we went with making it a
> > separate property instead. Will mention this in the cover letter.
> 
> What kind of issues? Could you please describe them?

We switched from bitmask to enum style for prop types, which means it's
not possible to express with the current uapi a property which accepts
both an object or a blob.

Which yeah sucks a bit ...

But!

blob properties are kms objects (like framebuffers), so it should be
possible to stuff a blob into an object property as-is. Of course you need
to update the validation code to make sure we accept either an fb or a
blob for the internal representation. But that kind of split internally is
required no matter what I think.
-Daniel

> 
> >
> > [1]
> > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> > > -Daniel
> > >
> > >>
> > >> Changes in V3:
> > >> - Fixed some logic errors in atomic checks (Dmitry)
> > >> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
> > >>    methods (Dmitry)
> > >>
> > >> Jessica Zhang (3):
> > >>    drm: Introduce solid fill property for drm plane
> > >>    drm: Adjust atomic checks for solid fill color
> > >>    drm/msm/dpu: Use color_fill property for DPU planes
> > >>
> > >>   drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
> > >>   drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
> > >>   drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
> > >>   drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
> > >>   drivers/gpu/drm/drm_blend.c               |  17 +++
> > >>   drivers/gpu/drm/drm_plane.c               |   8 +-
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
> > >>   include/drm/drm_atomic_helper.h           |   5 +-
> > >>   include/drm/drm_blend.h                   |   1 +
> > >>   include/drm/drm_plane.h                   |  62 ++++++++++
> > >>   11 files changed, 302 insertions(+), 103 deletions(-)
> > >>
> > >> --
> > >> 2.38.1
> > >>
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Dmitry Baryshkov]

On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
> > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > >
> > >
> > >
> > > On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> > > >> Introduce and add support for a solid_fill property. When the solid_fill
> > > >> property is set, and the framebuffer is set to NULL, memory fetch will be
> > > >> disabled.
> > > >>
> > > >> In addition, loosen the NULL FB checks within the atomic commit callstack
> > > >> to allow a NULL FB when the solid_fill property is set and add FB checks
> > > >> in methods where the FB was previously assumed to be non-NULL.
> > > >>
> > > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > > >> callstack to account for a NULL FB in cases where solid_fill is set.
> > > >>
> > > >> Some drivers support hardware that have optimizations for solid fill
> > > >> planes. This series aims to expose these capabilities to userspace as
> > > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > > >> hardware composer HAL) that can be set by apps like the Android Gears
> > > >> app.
> > > >>
> > > >> Userspace can set the solid_fill property to a blob containing the
> > > >> appropriate version number and solid fill color (in RGB323232 format) and
> > > >> setting the framebuffer to NULL.
> > > >>
> > > >> Note: Currently, there's only one version of the solid_fill blob property.
> > > >> However if other drivers want to support a similar feature, but require
> > > >> more than just the solid fill color, they can extend this feature by
> > > >> creating additional versions of the drm_solid_fill struct.
> > > >>
> > > >> Changes in V2:
> > > >> - Dropped SOLID_FILL_FORMAT property (Simon)
> > > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > > >> - Changed to checks for if solid_fill_blob is set (Dmitry)
> > > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > > >>    (Dmitry)
> > > >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> > > >> - Fixed whitespace and indentation issues (Dmitry)
> > > >
> > > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > > the blob as the FB pointer. Or create some kind other kind of special data
> > > > source objects (because solid fill is by far not the only such thing).
> > > >
> > > > We'd still end up in special cases like when userspace that doesn't
> > > > understand solid fill tries to read out such a framebuffer, but these
> > > > cases already exist anyway for lack of priviledges.
> > > >
> > > > So I still think that feels like the more consistent way to integrate this
> > > > feature. Which doesn't mean it has to happen like that, but the
> > > > patches/cover letter should at least explain why we don't do it like this.
> > >
> > > Hi Daniel,
> > >
> > > IIRC we were facing some issues with this check [1] when trying to set
> > > FB to a PROP_BLOB instead. Which is why we went with making it a
> > > separate property instead. Will mention this in the cover letter.
> >
> > What kind of issues? Could you please describe them?
>
> We switched from bitmask to enum style for prop types, which means it's
> not possible to express with the current uapi a property which accepts
> both an object or a blob.
>
> Which yeah sucks a bit ...
>
> But!
>
> blob properties are kms objects (like framebuffers), so it should be
> possible to stuff a blob into an object property as-is. Of course you need
> to update the validation code to make sure we accept either an fb or a
> blob for the internal representation. But that kind of split internally is
> required no matter what I think.

I checked your idea and notes from Jessica. So while we can pass blobs
to property objects, the prop_fb_id is created as an object property
with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
fail a check in drm_property_change_valid_get() ->
__drm_mode_object_find(). And I don't think that we should break the
existing validation code for this special case.

If you insist on using FB_ID for passing solid_fill information, I'd
ask you to reconsider using a 1x1 framebuffer. It would be fully
compatible with the existing userspace, which can then treat it
seamlessly.

> -Daniel
>
> >
> > >
> > > [1]
> > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71

-- 
With best wishes
Dmitry

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Abhinav Kumar]

Hi Daniel

Thanks for looking into this series.

On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote:
> On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
>>> On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/5/2023 3:33 AM, Daniel Vetter wrote:
>>>>> On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
>>>>>> Introduce and add support for a solid_fill property. When the solid_fill
>>>>>> property is set, and the framebuffer is set to NULL, memory fetch will be
>>>>>> disabled.
>>>>>>
>>>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>>>> to allow a NULL FB when the solid_fill property is set and add FB checks
>>>>>> in methods where the FB was previously assumed to be non-NULL.
>>>>>>
>>>>>> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
>>>>>> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
>>>>>> callstack to account for a NULL FB in cases where solid_fill is set.
>>>>>>
>>>>>> Some drivers support hardware that have optimizations for solid fill
>>>>>> planes. This series aims to expose these capabilities to userspace as
>>>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>>>> app.
>>>>>>
>>>>>> Userspace can set the solid_fill property to a blob containing the
>>>>>> appropriate version number and solid fill color (in RGB323232 format) and
>>>>>> setting the framebuffer to NULL.
>>>>>>
>>>>>> Note: Currently, there's only one version of the solid_fill blob property.
>>>>>> However if other drivers want to support a similar feature, but require
>>>>>> more than just the solid fill color, they can extend this feature by
>>>>>> creating additional versions of the drm_solid_fill struct.
>>>>>>
>>>>>> Changes in V2:
>>>>>> - Dropped SOLID_FILL_FORMAT property (Simon)
>>>>>> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
>>>>>> - Changed to checks for if solid_fill_blob is set (Dmitry)
>>>>>> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>>>>>>     (Dmitry)
>>>>>> - Removed DPU_PLANE_COLOR_FILL_FLAG
>>>>>> - Fixed whitespace and indentation issues (Dmitry)
>>>>>
>>>>> Now that this is a blob, I do wonder again whether it's not cleaner to set
>>>>> the blob as the FB pointer. Or create some kind other kind of special data
>>>>> source objects (because solid fill is by far not the only such thing).
>>>>>
>>>>> We'd still end up in special cases like when userspace that doesn't
>>>>> understand solid fill tries to read out such a framebuffer, but these
>>>>> cases already exist anyway for lack of priviledges.
>>>>>
>>>>> So I still think that feels like the more consistent way to integrate this
>>>>> feature. Which doesn't mean it has to happen like that, but the
>>>>> patches/cover letter should at least explain why we don't do it like this.
>>>>
>>>> Hi Daniel,
>>>>
>>>> IIRC we were facing some issues with this check [1] when trying to set
>>>> FB to a PROP_BLOB instead. Which is why we went with making it a
>>>> separate property instead. Will mention this in the cover letter.
>>>
>>> What kind of issues? Could you please describe them?
>>
>> We switched from bitmask to enum style for prop types, which means it's
>> not possible to express with the current uapi a property which accepts
>> both an object or a blob.
>>
>> Which yeah sucks a bit ...
>>
>> But!
>>
>> blob properties are kms objects (like framebuffers), so it should be
>> possible to stuff a blob into an object property as-is. Of course you need
>> to update the validation code to make sure we accept either an fb or a
>> blob for the internal representation. But that kind of split internally is
>> required no matter what I think.
> 
> I checked your idea and notes from Jessica. So while we can pass blobs
> to property objects, the prop_fb_id is created as an object property
> with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> fail a check in drm_property_change_valid_get() ->
> __drm_mode_object_find(). And I don't think that we should break the
> existing validation code for this special case.
> 

Like Jessica wrote, re-using the FB_ID property to pass solid fill 
information will need modification of existing checks shown in [1] OR 
the property creation itself would fail.

We just went with this approach, as it was less intrusive and would not 
affect the existing FB_ID path.

Since both approaches need modifications of validation checks, adding a 
new property is less intrusive and safer than the already convoluted 
checks in drm_property_flags_valid().

Let us know if its a strong preference on your side to re-use FB_ID and 
if so why.

Thanks

Abhinav

> If you insist on using FB_ID for passing solid_fill information, I'd
> ask you to reconsider using a 1x1 framebuffer. It would be fully
> compatible with the existing userspace, which can then treat it
> seamlessly.
> 
>> -Daniel
>>
>>>
>>>>
>>>> [1]
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
> 

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Daniel Vetter]

On Fri, Jan 06, 2023 at 04:33:04PM -0800, Abhinav Kumar wrote:
> Hi Daniel
> 
> Thanks for looking into this series.
> 
> On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote:
> > On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > > > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> > > > > > > Introduce and add support for a solid_fill property. When the solid_fill
> > > > > > > property is set, and the framebuffer is set to NULL, memory fetch will be
> > > > > > > disabled.
> > > > > > > 
> > > > > > > In addition, loosen the NULL FB checks within the atomic commit callstack
> > > > > > > to allow a NULL FB when the solid_fill property is set and add FB checks
> > > > > > > in methods where the FB was previously assumed to be non-NULL.
> > > > > > > 
> > > > > > > Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > > > > > > dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > > > > > > callstack to account for a NULL FB in cases where solid_fill is set.
> > > > > > > 
> > > > > > > Some drivers support hardware that have optimizations for solid fill
> > > > > > > planes. This series aims to expose these capabilities to userspace as
> > > > > > > some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > > > > > > hardware composer HAL) that can be set by apps like the Android Gears
> > > > > > > app.
> > > > > > > 
> > > > > > > Userspace can set the solid_fill property to a blob containing the
> > > > > > > appropriate version number and solid fill color (in RGB323232 format) and
> > > > > > > setting the framebuffer to NULL.
> > > > > > > 
> > > > > > > Note: Currently, there's only one version of the solid_fill blob property.
> > > > > > > However if other drivers want to support a similar feature, but require
> > > > > > > more than just the solid fill color, they can extend this feature by
> > > > > > > creating additional versions of the drm_solid_fill struct.
> > > > > > > 
> > > > > > > Changes in V2:
> > > > > > > - Dropped SOLID_FILL_FORMAT property (Simon)
> > > > > > > - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > > > > > > - Changed to checks for if solid_fill_blob is set (Dmitry)
> > > > > > > - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > > > > > >     (Dmitry)
> > > > > > > - Removed DPU_PLANE_COLOR_FILL_FLAG
> > > > > > > - Fixed whitespace and indentation issues (Dmitry)
> > > > > > 
> > > > > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > > > > the blob as the FB pointer. Or create some kind other kind of special data
> > > > > > source objects (because solid fill is by far not the only such thing).
> > > > > > 
> > > > > > We'd still end up in special cases like when userspace that doesn't
> > > > > > understand solid fill tries to read out such a framebuffer, but these
> > > > > > cases already exist anyway for lack of priviledges.
> > > > > > 
> > > > > > So I still think that feels like the more consistent way to integrate this
> > > > > > feature. Which doesn't mean it has to happen like that, but the
> > > > > > patches/cover letter should at least explain why we don't do it like this.
> > > > > 
> > > > > Hi Daniel,
> > > > > 
> > > > > IIRC we were facing some issues with this check [1] when trying to set
> > > > > FB to a PROP_BLOB instead. Which is why we went with making it a
> > > > > separate property instead. Will mention this in the cover letter.
> > > > 
> > > > What kind of issues? Could you please describe them?
> > > 
> > > We switched from bitmask to enum style for prop types, which means it's
> > > not possible to express with the current uapi a property which accepts
> > > both an object or a blob.
> > > 
> > > Which yeah sucks a bit ...
> > > 
> > > But!
> > > 
> > > blob properties are kms objects (like framebuffers), so it should be
> > > possible to stuff a blob into an object property as-is. Of course you need
> > > to update the validation code to make sure we accept either an fb or a
> > > blob for the internal representation. But that kind of split internally is
> > > required no matter what I think.
> > 
> > I checked your idea and notes from Jessica. So while we can pass blobs
> > to property objects, the prop_fb_id is created as an object property
> > with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> > fail a check in drm_property_change_valid_get() ->
> > __drm_mode_object_find(). And I don't think that we should break the
> > existing validation code for this special case.
> > 
> 
> Like Jessica wrote, re-using the FB_ID property to pass solid fill
> information will need modification of existing checks shown in [1] OR the
> property creation itself would fail.

Yeah modifications there are needed. Or well, we need to extend that code
to allow bitmask for objects iff the driver allows that. What I meant is
that from an uapi pov it's doable, which is the part that matters because
that is the part that we wont ever be able to change once it's baked in.

> We just went with this approach, as it was less intrusive and would not
> affect the existing FB_ID path.
> 
> Since both approaches need modifications of validation checks, adding a new
> property is less intrusive and safer than the already convoluted checks in
> drm_property_flags_valid().

It's easier, but it's also the part that we cannot ever change (because
it's uapi). If the NULL fb on an active plane makes some compositor die on
vt switch, we then have a pretty serious problem.

I'm honestly not sure which is the approach that blows up less, just that
justifying uapi with the implementation work on the kernel side is not
really the right approach :-)

> Let us know if its a strong preference on your side to re-use FB_ID and if
> so why.

I just want to make sure we're picking the right uapi option, and that
we're not just excluding them because it looks like we can't implement
them in the kernel. Uapi is forever, the implemenation is not.
-Daniel

> Thanks
> 
> Abhinav
> 
> > If you insist on using FB_ID for passing solid_fill information, I'd
> > ask you to reconsider using a 1x1 framebuffer. It would be fully
> > compatible with the existing userspace, which can then treat it
> > seamlessly.
> > 
> > > -Daniel
> > > 
> > > > 
> > > > > 
> > > > > [1]
> > > > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Simon Ser]

On Wednesday, January 11th, 2023 at 23:29, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Jan 06, 2023 at 04:33:04PM -0800, Abhinav Kumar wrote:
> > Hi Daniel
> >
> > Thanks for looking into this series.
> >
> > On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote:
> > > On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
> > > > > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > > > > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> > > > > > > > Introduce and add support for a solid_fill property. When the solid_fill
> > > > > > > > property is set, and the framebuffer is set to NULL, memory fetch will be
> > > > > > > > disabled.
> > > > > > > >
> > > > > > > > In addition, loosen the NULL FB checks within the atomic commit callstack
> > > > > > > > to allow a NULL FB when the solid_fill property is set and add FB checks
> > > > > > > > in methods where the FB was previously assumed to be non-NULL.
> > > > > > > >
> > > > > > > > Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > > > > > > > dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > > > > > > > callstack to account for a NULL FB in cases where solid_fill is set.
> > > > > > > >
> > > > > > > > Some drivers support hardware that have optimizations for solid fill
> > > > > > > > planes. This series aims to expose these capabilities to userspace as
> > > > > > > > some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > > > > > > > hardware composer HAL) that can be set by apps like the Android Gears
> > > > > > > > app.
> > > > > > > >
> > > > > > > > Userspace can set the solid_fill property to a blob containing the
> > > > > > > > appropriate version number and solid fill color (in RGB323232 format) and
> > > > > > > > setting the framebuffer to NULL.
> > > > > > > >
> > > > > > > > Note: Currently, there's only one version of the solid_fill blob property.
> > > > > > > > However if other drivers want to support a similar feature, but require
> > > > > > > > more than just the solid fill color, they can extend this feature by
> > > > > > > > creating additional versions of the drm_solid_fill struct.
> > > > > > > >
> > > > > > > > Changes in V2:
> > > > > > > > - Dropped SOLID_FILL_FORMAT property (Simon)
> > > > > > > > - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > > > > > > > - Changed to checks for if solid_fill_blob is set (Dmitry)
> > > > > > > > - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > > > > > > >     (Dmitry)
> > > > > > > > - Removed DPU_PLANE_COLOR_FILL_FLAG
> > > > > > > > - Fixed whitespace and indentation issues (Dmitry)
> > > > > > >
> > > > > > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > > > > > the blob as the FB pointer. Or create some kind other kind of special data
> > > > > > > source objects (because solid fill is by far not the only such thing).
> > > > > > >
> > > > > > > We'd still end up in special cases like when userspace that doesn't
> > > > > > > understand solid fill tries to read out such a framebuffer, but these
> > > > > > > cases already exist anyway for lack of priviledges.
> > > > > > >
> > > > > > > So I still think that feels like the more consistent way to integrate this
> > > > > > > feature. Which doesn't mean it has to happen like that, but the
> > > > > > > patches/cover letter should at least explain why we don't do it like this.
> > > > > >
> > > > > > Hi Daniel,
> > > > > >
> > > > > > IIRC we were facing some issues with this check [1] when trying to set
> > > > > > FB to a PROP_BLOB instead. Which is why we went with making it a
> > > > > > separate property instead. Will mention this in the cover letter.
> > > > >
> > > > > What kind of issues? Could you please describe them?
> > > >
> > > > We switched from bitmask to enum style for prop types, which means it's
> > > > not possible to express with the current uapi a property which accepts
> > > > both an object or a blob.
> > > >
> > > > Which yeah sucks a bit ...
> > > >
> > > > But!
> > > >
> > > > blob properties are kms objects (like framebuffers), so it should be
> > > > possible to stuff a blob into an object property as-is. Of course you need
> > > > to update the validation code to make sure we accept either an fb or a
> > > > blob for the internal representation. But that kind of split internally is
> > > > required no matter what I think.
> > >
> > > I checked your idea and notes from Jessica. So while we can pass blobs
> > > to property objects, the prop_fb_id is created as an object property
> > > with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> > > fail a check in drm_property_change_valid_get() ->
> > > __drm_mode_object_find(). And I don't think that we should break the
> > > existing validation code for this special case.
> > >
> >
> > Like Jessica wrote, re-using the FB_ID property to pass solid fill
> > information will need modification of existing checks shown in [1] OR the
> > property creation itself would fail.
> 
> Yeah modifications there are needed. Or well, we need to extend that code
> to allow bitmask for objects iff the driver allows that. What I meant is
> that from an uapi pov it's doable, which is the part that matters because
> that is the part that we wont ever be able to change once it's baked in.
> 
> > We just went with this approach, as it was less intrusive and would not
> > affect the existing FB_ID path.
> >
> > Since both approaches need modifications of validation checks, adding a new
> > property is less intrusive and safer than the already convoluted checks in
> > drm_property_flags_valid().
> 
> It's easier, but it's also the part that we cannot ever change (because
> it's uapi). If the NULL fb on an active plane makes some compositor die on
> vt switch, we then have a pretty serious problem.

The new compositor might try to GETFB on the blob which isn't actually an FB,
and gets confused even more...

> I'm honestly not sure which is the approach that blows up less, just that
> justifying uapi with the implementation work on the kernel side is not
> really the right approach :-)
> 
> > Let us know if its a strong preference on your side to re-use FB_ID and if
> > so why.
> 
> I just want to make sure we're picking the right uapi option, and that
> we're not just excluding them because it looks like we can't implement
> them in the kernel. Uapi is forever, the implemenation is not.

IMHO re-using FB_ID for this is not a good idea. FB_ID is described by the
kernel as a property which accepts FB objects (DRM_MODE_OBJECT_FB). Mixing up
something else will just result in more confusion.

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5522 bytes --]

On Fri, 6 Jan 2023 23:49:34 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:  
> > > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:  
> > > >
> > > >
> > > >
> > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote:  
> > > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:  
> > > > >> Introduce and add support for a solid_fill property. When the solid_fill
> > > > >> property is set, and the framebuffer is set to NULL, memory fetch will be
> > > > >> disabled.
> > > > >>
> > > > >> In addition, loosen the NULL FB checks within the atomic commit callstack
> > > > >> to allow a NULL FB when the solid_fill property is set and add FB checks
> > > > >> in methods where the FB was previously assumed to be non-NULL.
> > > > >>
> > > > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > > > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > > > >> callstack to account for a NULL FB in cases where solid_fill is set.
> > > > >>
> > > > >> Some drivers support hardware that have optimizations for solid fill
> > > > >> planes. This series aims to expose these capabilities to userspace as
> > > > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > > > >> hardware composer HAL) that can be set by apps like the Android Gears
> > > > >> app.
> > > > >>
> > > > >> Userspace can set the solid_fill property to a blob containing the
> > > > >> appropriate version number and solid fill color (in RGB323232 format) and
> > > > >> setting the framebuffer to NULL.
> > > > >>
> > > > >> Note: Currently, there's only one version of the solid_fill blob property.
> > > > >> However if other drivers want to support a similar feature, but require
> > > > >> more than just the solid fill color, they can extend this feature by
> > > > >> creating additional versions of the drm_solid_fill struct.
> > > > >>
> > > > >> Changes in V2:
> > > > >> - Dropped SOLID_FILL_FORMAT property (Simon)
> > > > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > > > >> - Changed to checks for if solid_fill_blob is set (Dmitry)
> > > > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > > > >>    (Dmitry)
> > > > >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> > > > >> - Fixed whitespace and indentation issues (Dmitry)  
> > > > >
> > > > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > > > the blob as the FB pointer. Or create some kind other kind of special data
> > > > > source objects (because solid fill is by far not the only such thing).
> > > > >
> > > > > We'd still end up in special cases like when userspace that doesn't
> > > > > understand solid fill tries to read out such a framebuffer, but these
> > > > > cases already exist anyway for lack of priviledges.
> > > > >
> > > > > So I still think that feels like the more consistent way to integrate this
> > > > > feature. Which doesn't mean it has to happen like that, but the
> > > > > patches/cover letter should at least explain why we don't do it like this.  
> > > >
> > > > Hi Daniel,
> > > >
> > > > IIRC we were facing some issues with this check [1] when trying to set
> > > > FB to a PROP_BLOB instead. Which is why we went with making it a
> > > > separate property instead. Will mention this in the cover letter.  
> > >
> > > What kind of issues? Could you please describe them?  
> >
> > We switched from bitmask to enum style for prop types, which means it's
> > not possible to express with the current uapi a property which accepts
> > both an object or a blob.
> >
> > Which yeah sucks a bit ...
> >
> > But!
> >
> > blob properties are kms objects (like framebuffers), so it should be
> > possible to stuff a blob into an object property as-is. Of course you need
> > to update the validation code to make sure we accept either an fb or a
> > blob for the internal representation. But that kind of split internally is
> > required no matter what I think.  
> 
> I checked your idea and notes from Jessica. So while we can pass blobs
> to property objects, the prop_fb_id is created as an object property
> with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> fail a check in drm_property_change_valid_get() ->
> __drm_mode_object_find(). And I don't think that we should break the
> existing validation code for this special case.
> 
> If you insist on using FB_ID for passing solid_fill information, I'd
> ask you to reconsider using a 1x1 framebuffer. It would be fully
> compatible with the existing userspace, which can then treat it
> seamlessly.

Hi,

indeed, what about simply using a 1x1 framebuffer for real? Why was that
approach rejected?

Is there some problem with drivers just special-casing 1x1 framebuffers
and hitting the solid-fill hardware path instead of
framebuffer-with-scaling hardware path?

If needed, the KMS plane could have a property that tells userspace
that if you set a 1x1 RGB FB here without any color ops, I am able to
scale that to *any* size without any limits very efficiently.


Thanks,
pq

> > > > [1]
> > > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Simon Ser]

On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> indeed, what about simply using a 1x1 framebuffer for real? Why was that
> approach rejected?

Ideally we don't want to allocate any GPU memory for the solid-fill
stuff. And if we special-case 1x1 FB creation to not be backed by real
GPU memory then we hit several situations where user-space expects a
real FB but there isn't: for instance, GETFB2 converts from FB object
ID to GEM handles. Even if we make GETFB2 fail and accept that this
breaks user-space, then there is no way for user-space to recover the
FB color for flicker-free transitions and such.

This is all purely from a uAPI PoV, completely ignoring the potential
issues with the internal kernel abstractions which might not be suitable
for this either.

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

On Tue, 31 Jan 2023 10:06:39 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > indeed, what about simply using a 1x1 framebuffer for real? Why was that
> > approach rejected?  
> 
> Ideally we don't want to allocate any GPU memory for the solid-fill
> stuff. And if we special-case 1x1 FB creation to not be backed by real
> GPU memory then we hit several situations where user-space expects a
> real FB but there isn't: for instance, GETFB2 converts from FB object
> ID to GEM handles. Even if we make GETFB2 fail and accept that this
> breaks user-space, then there is no way for user-space to recover the
> FB color for flicker-free transitions and such.
> 
> This is all purely from a uAPI PoV, completely ignoring the potential
> issues with the internal kernel abstractions which might not be suitable
> for this either.

I mean a real 1x1 buffer: a dumb buffer.

It would be absolutely compatible with anything existing, because it is
a real FB. As a dumb buffer it would be trivial to write into and read
out. As 1x1 it would be tiny (one page?). Even if something needs to
raw-access uncached memory over 33 MHz PCI bus or whatever the worst
case is, it's just one pixel, so it's fast enough, right? And it only
needs to be read once when set, like USB display drivers do. The driver
does not need to manually apply any color operations, because none are
supported in this special case.

One can put all these limitations and even pixel format in the plane
property that tells userspace that a 1x1 FB works here.

To recap, the other alternatives under discussion I see right now are:

- this proposal of dedicated fill color property
- stuffing something new into FB_ID property

There is also the question of other kinds of plane content sources like
live camera feeds where userspace won't be shovelling each frame
individually like we do now.

1x1 dumb buffer is not as small and lean as a dedicated fill color
property, but the UAPI design questions seem to be much less. What's
the best trade-off and for whom?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Simon Ser]

On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Tue, 31 Jan 2023 10:06:39 +0000
> Simon Ser <contact@emersion.fr> wrote:
> 
> > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > 
> > > indeed, what about simply using a 1x1 framebuffer for real? Why was that
> > > approach rejected?  
> > 
> > Ideally we don't want to allocate any GPU memory for the solid-fill
> > stuff. And if we special-case 1x1 FB creation to not be backed by real
> > GPU memory then we hit several situations where user-space expects a
> > real FB but there isn't: for instance, GETFB2 converts from FB object
> > ID to GEM handles. Even if we make GETFB2 fail and accept that this
> > breaks user-space, then there is no way for user-space to recover the
> > FB color for flicker-free transitions and such.
> > 
> > This is all purely from a uAPI PoV, completely ignoring the potential
> > issues with the internal kernel abstractions which might not be suitable
> > for this either.
> 
> I mean a real 1x1 buffer: a dumb buffer.
> 
> It would be absolutely compatible with anything existing, because it is
> a real FB. As a dumb buffer it would be trivial to write into and read
> out. As 1x1 it would be tiny (one page?). Even if something needs to
> raw-access uncached memory over 33 MHz PCI bus or whatever the worst
> case is, it's just one pixel, so it's fast enough, right? And it only
> needs to be read once when set, like USB display drivers do. The driver
> does not need to manually apply any color operations, because none are
> supported in this special case.
> 
> One can put all these limitations and even pixel format in the plane
> property that tells userspace that a 1x1 FB works here.
> 
> To recap, the other alternatives under discussion I see right now are:
> 
> - this proposal of dedicated fill color property
> - stuffing something new into FB_ID property
> 
> There is also the question of other kinds of plane content sources like
> live camera feeds where userspace won't be shovelling each frame
> individually like we do now.
> 
> 1x1 dumb buffer is not as small and lean as a dedicated fill color
> property, but the UAPI design questions seem to be much less. What's
> the best trade-off and for whom?

By "real memory" yes I mean the 1 page.

Using a real buffer also brings back other discussions, e.g. the one about
which pixel formats to accept.

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

On Tue, 31 Jan 2023 11:21:18 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > On Tue, 31 Jan 2023 10:06:39 +0000
> > Simon Ser <contact@emersion.fr> wrote:
> >   
> > > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >   
> > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that
> > > > approach rejected?    
> > > 
> > > Ideally we don't want to allocate any GPU memory for the solid-fill
> > > stuff. And if we special-case 1x1 FB creation to not be backed by real
> > > GPU memory then we hit several situations where user-space expects a
> > > real FB but there isn't: for instance, GETFB2 converts from FB object
> > > ID to GEM handles. Even if we make GETFB2 fail and accept that this
> > > breaks user-space, then there is no way for user-space to recover the
> > > FB color for flicker-free transitions and such.
> > > 
> > > This is all purely from a uAPI PoV, completely ignoring the potential
> > > issues with the internal kernel abstractions which might not be suitable
> > > for this either.  
> > 
> > I mean a real 1x1 buffer: a dumb buffer.
> > 
> > It would be absolutely compatible with anything existing, because it is
> > a real FB. As a dumb buffer it would be trivial to write into and read
> > out. As 1x1 it would be tiny (one page?). Even if something needs to
> > raw-access uncached memory over 33 MHz PCI bus or whatever the worst
> > case is, it's just one pixel, so it's fast enough, right? And it only
> > needs to be read once when set, like USB display drivers do. The driver
> > does not need to manually apply any color operations, because none are
> > supported in this special case.
> > 
> > One can put all these limitations and even pixel format in the plane
> > property that tells userspace that a 1x1 FB works here.
> > 
> > To recap, the other alternatives under discussion I see right now are:
> > 
> > - this proposal of dedicated fill color property
> > - stuffing something new into FB_ID property
> > 
> > There is also the question of other kinds of plane content sources like
> > live camera feeds where userspace won't be shovelling each frame
> > individually like we do now.
> > 
> > 1x1 dumb buffer is not as small and lean as a dedicated fill color
> > property, but the UAPI design questions seem to be much less. What's
> > the best trade-off and for whom?  
> 
> By "real memory" yes I mean the 1 page.
> 
> Using a real buffer also brings back other discussions, e.g. the one about
> which pixel formats to accept.

Yeah, which is why I wrote: "One can put all these limitations and even
pixel format in the plane property". It doesn't even need to be a
variable in the UAPI, it can be hardcoded in the UAPI doc.

Please, do not understand this as me strongly advocating for the real FB
approach! I just don't want that option to be misunderstood.

I don't really care which design is chosen, but I do care about
documenting why other designs were rejected. If the rejection reasons
were false, they should be revised, even if the decision does not
change.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Jessica Zhang]

On 1/31/2023 4:49 AM, Pekka Paalanen wrote:
> On Tue, 31 Jan 2023 11:21:18 +0000
> Simon Ser <contact@emersion.fr> wrote:
> 
>> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>
>>> On Tue, 31 Jan 2023 10:06:39 +0000
>>> Simon Ser <contact@emersion.fr> wrote:
>>>    
>>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>    
>>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that
>>>>> approach rejected?
>>>>
>>>> Ideally we don't want to allocate any GPU memory for the solid-fill
>>>> stuff. And if we special-case 1x1 FB creation to not be backed by real
>>>> GPU memory then we hit several situations where user-space expects a
>>>> real FB but there isn't: for instance, GETFB2 converts from FB object
>>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this
>>>> breaks user-space, then there is no way for user-space to recover the
>>>> FB color for flicker-free transitions and such.
>>>>
>>>> This is all purely from a uAPI PoV, completely ignoring the potential
>>>> issues with the internal kernel abstractions which might not be suitable
>>>> for this either.
>>>
>>> I mean a real 1x1 buffer: a dumb buffer.
>>>
>>> It would be absolutely compatible with anything existing, because it is
>>> a real FB. As a dumb buffer it would be trivial to write into and read
>>> out. As 1x1 it would be tiny (one page?). Even if something needs to
>>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst
>>> case is, it's just one pixel, so it's fast enough, right? And it only
>>> needs to be read once when set, like USB display drivers do. The driver
>>> does not need to manually apply any color operations, because none are
>>> supported in this special case.
>>>
>>> One can put all these limitations and even pixel format in the plane
>>> property that tells userspace that a 1x1 FB works here.
>>>
>>> To recap, the other alternatives under discussion I see right now are:
>>>
>>> - this proposal of dedicated fill color property
>>> - stuffing something new into FB_ID property
>>>
>>> There is also the question of other kinds of plane content sources like
>>> live camera feeds where userspace won't be shovelling each frame
>>> individually like we do now.
>>>
>>> 1x1 dumb buffer is not as small and lean as a dedicated fill color
>>> property, but the UAPI design questions seem to be much less. What's
>>> the best trade-off and for whom?
>>
>> By "real memory" yes I mean the 1 page.
>>
>> Using a real buffer also brings back other discussions, e.g. the one about
>> which pixel formats to accept.
> 
> Yeah, which is why I wrote: "One can put all these limitations and even
> pixel format in the plane property". It doesn't even need to be a
> variable in the UAPI, it can be hardcoded in the UAPI doc.
> 
> Please, do not understand this as me strongly advocating for the real FB
> approach! I just don't want that option to be misunderstood.
> 
> I don't really care which design is chosen, but I do care about
> documenting why other designs were rejected. If the rejection reasons
> were false, they should be revised, even if the decision does not
> change.

Hi Pekka/Daniel,

Looks like the general sentiment is to keep solid fill as a separate 
property, so I will stick with that implementation for v4.

I can document the reason why we chose this approach over 1x1 FB in the 
cover letter, but to summarize here:

Allocating an FB for solid_fill brings in unnecessary overhead (ex. 
having to allocate memory for the FB). In addition, since memory fetch 
is disabled when solid fill is enabled, having a separate property that 
doesn't do any memory allocation for solid fill better reflects the 
behavior of this feature within driver.

We also wanted to avoid having FB_ID accept a property blob as it would 
involve loosening some drm_property checks, which could cause issues 
with other property ioctls.



Also, re: other plane sources -- FWIW, I have tried implementing a 
source enum as Ville suggested, but ultimately dropped the change as it 
would require userspace to set properties in a specific order (i.e. to 
enable solid_fill, userspace would have to first set FB_ID to NULL then 
set SOLID_FILL).

I'm not sure how much of a can of worms that would be for userspace, but 
if you're fine with having that as a requirement the I can re-add the code.

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5499 bytes --]

On Wed, 1 Feb 2023 18:06:41 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> On 1/31/2023 4:49 AM, Pekka Paalanen wrote:
> > On Tue, 31 Jan 2023 11:21:18 +0000
> > Simon Ser <contact@emersion.fr> wrote:
> >   
> >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >>  
> >>> On Tue, 31 Jan 2023 10:06:39 +0000
> >>> Simon Ser <contact@emersion.fr> wrote:
> >>>      
> >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >>>>      
> >>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that
> >>>>> approach rejected?  
> >>>>
> >>>> Ideally we don't want to allocate any GPU memory for the solid-fill
> >>>> stuff. And if we special-case 1x1 FB creation to not be backed by real
> >>>> GPU memory then we hit several situations where user-space expects a
> >>>> real FB but there isn't: for instance, GETFB2 converts from FB object
> >>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this
> >>>> breaks user-space, then there is no way for user-space to recover the
> >>>> FB color for flicker-free transitions and such.
> >>>>
> >>>> This is all purely from a uAPI PoV, completely ignoring the potential
> >>>> issues with the internal kernel abstractions which might not be suitable
> >>>> for this either.  
> >>>
> >>> I mean a real 1x1 buffer: a dumb buffer.
> >>>
> >>> It would be absolutely compatible with anything existing, because it is
> >>> a real FB. As a dumb buffer it would be trivial to write into and read
> >>> out. As 1x1 it would be tiny (one page?). Even if something needs to
> >>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst
> >>> case is, it's just one pixel, so it's fast enough, right? And it only
> >>> needs to be read once when set, like USB display drivers do. The driver
> >>> does not need to manually apply any color operations, because none are
> >>> supported in this special case.
> >>>
> >>> One can put all these limitations and even pixel format in the plane
> >>> property that tells userspace that a 1x1 FB works here.
> >>>
> >>> To recap, the other alternatives under discussion I see right now are:
> >>>
> >>> - this proposal of dedicated fill color property
> >>> - stuffing something new into FB_ID property
> >>>
> >>> There is also the question of other kinds of plane content sources like
> >>> live camera feeds where userspace won't be shovelling each frame
> >>> individually like we do now.
> >>>
> >>> 1x1 dumb buffer is not as small and lean as a dedicated fill color
> >>> property, but the UAPI design questions seem to be much less. What's
> >>> the best trade-off and for whom?  
> >>
> >> By "real memory" yes I mean the 1 page.
> >>
> >> Using a real buffer also brings back other discussions, e.g. the one about
> >> which pixel formats to accept.  
> > 
> > Yeah, which is why I wrote: "One can put all these limitations and even
> > pixel format in the plane property". It doesn't even need to be a
> > variable in the UAPI, it can be hardcoded in the UAPI doc.
> > 
> > Please, do not understand this as me strongly advocating for the real FB
> > approach! I just don't want that option to be misunderstood.
> > 
> > I don't really care which design is chosen, but I do care about
> > documenting why other designs were rejected. If the rejection reasons
> > were false, they should be revised, even if the decision does not
> > change.  
> 
> Hi Pekka/Daniel,
> 
> Looks like the general sentiment is to keep solid fill as a separate 
> property, so I will stick with that implementation for v4.
> 
> I can document the reason why we chose this approach over 1x1 FB in the 
> cover letter, but to summarize here:
> 
> Allocating an FB for solid_fill brings in unnecessary overhead (ex. 
> having to allocate memory for the FB). In addition, since memory fetch 
> is disabled when solid fill is enabled, having a separate property that 
> doesn't do any memory allocation for solid fill better reflects the 
> behavior of this feature within driver.
> 
> We also wanted to avoid having FB_ID accept a property blob as it would 
> involve loosening some drm_property checks, which could cause issues 
> with other property ioctls.
> 

That's fine by me, thanks!

> Also, re: other plane sources -- FWIW, I have tried implementing a 
> source enum as Ville suggested, but ultimately dropped the change as it 
> would require userspace to set properties in a specific order (i.e. to 
> enable solid_fill, userspace would have to first set FB_ID to NULL then 
> set SOLID_FILL).
> 
> I'm not sure how much of a can of worms that would be for userspace, but 
> if you're fine with having that as a requirement the I can re-add the code.

There is no ordering between properties set in a single atomic commit,
they all apply at the same time. Therefore the kernel code needs to
consider the whole new state set as a single entity.

If userspace splits changing those two properties into different atomic
commits, that's a userspace bug. It would not work with atomic
properties already today, where you need to set half a dozen properties
to update one KMS plane.

The only complication I can see is the legacy KMS UAPI, non-atomic.
They will change FB_ID, but they cannot touch the solid fill property.
I guess that needs to be special-cased somehow.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Jessica Zhang]

On 1/5/2023 7:43 PM, Dmitry Baryshkov wrote:
> On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 1/5/2023 3:33 AM, Daniel Vetter wrote:
>>> On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
>>>> Introduce and add support for a solid_fill property. When the solid_fill
>>>> property is set, and the framebuffer is set to NULL, memory fetch will be
>>>> disabled.
>>>>
>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>> to allow a NULL FB when the solid_fill property is set and add FB checks
>>>> in methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
>>>> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
>>>> callstack to account for a NULL FB in cases where solid_fill is set.
>>>>
>>>> Some drivers support hardware that have optimizations for solid fill
>>>> planes. This series aims to expose these capabilities to userspace as
>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>> app.
>>>>
>>>> Userspace can set the solid_fill property to a blob containing the
>>>> appropriate version number and solid fill color (in RGB323232 format) and
>>>> setting the framebuffer to NULL.
>>>>
>>>> Note: Currently, there's only one version of the solid_fill blob property.
>>>> However if other drivers want to support a similar feature, but require
>>>> more than just the solid fill color, they can extend this feature by
>>>> creating additional versions of the drm_solid_fill struct.
>>>>
>>>> Changes in V2:
>>>> - Dropped SOLID_FILL_FORMAT property (Simon)
>>>> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
>>>> - Changed to checks for if solid_fill_blob is set (Dmitry)
>>>> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
>>>>     (Dmitry)
>>>> - Removed DPU_PLANE_COLOR_FILL_FLAG
>>>> - Fixed whitespace and indentation issues (Dmitry)
>>>
>>> Now that this is a blob, I do wonder again whether it's not cleaner to set
>>> the blob as the FB pointer. Or create some kind other kind of special data
>>> source objects (because solid fill is by far not the only such thing).
>>>
>>> We'd still end up in special cases like when userspace that doesn't
>>> understand solid fill tries to read out such a framebuffer, but these
>>> cases already exist anyway for lack of priviledges.
>>>
>>> So I still think that feels like the more consistent way to integrate this
>>> feature. Which doesn't mean it has to happen like that, but the
>>> patches/cover letter should at least explain why we don't do it like this.
>>
>> Hi Daniel,
>>
>> IIRC we were facing some issues with this check [1] when trying to set
>> FB to a PROP_BLOB instead. Which is why we went with making it a
>> separate property instead. Will mention this in the cover letter.
> 
> What kind of issues? Could you please describe them?

Hi Dmitry,

PROP_BLOB is defined as a legacy type here [1], but FB_ID is a 
PROP_OBJECT which is defined as an extended type [2]. So, setting a 
property blob as the FB would fail drm_property_flags_valid() due to 
this check [3].

[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L523

[2] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/include/uapi/drm/drm_mode.h#L534

[3] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71

Thanks,

Jessica Zhang

> 
>>
>> [1]
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>> -Daniel
>>>
>>>>
>>>> Changes in V3:
>>>> - Fixed some logic errors in atomic checks (Dmitry)
>>>> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
>>>>     methods (Dmitry)
>>>>
>>>> Jessica Zhang (3):
>>>>     drm: Introduce solid fill property for drm plane
>>>>     drm: Adjust atomic checks for solid fill color
>>>>     drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>>    drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
>>>>    drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
>>>>    drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
>>>>    drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
>>>>    drivers/gpu/drm/drm_blend.c               |  17 +++
>>>>    drivers/gpu/drm/drm_plane.c               |   8 +-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
>>>>    include/drm/drm_atomic_helper.h           |   5 +-
>>>>    include/drm/drm_blend.h                   |   1 +
>>>>    include/drm/drm_plane.h                   |  62 ++++++++++
>>>>    11 files changed, 302 insertions(+), 103 deletions(-)
>>>>
>>>> --
>>>> 2.38.1
>>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
> 
> 
> 
> -- 
> With best wishes
> Dmitry