mbox series

[RFC,v5,00/10] Support for Solid Fill Planes

Message ID 20230728-solid-fill-v5-0-053dbefa909c@quicinc.com
Headers show
Series Support for Solid Fill Planes | expand

Message

Jessica Zhang July 28, 2023, 5:02 p.m. UTC
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.

In order to expose this capability to userspace, this series will:

- Introduce solid_fill and pixel_source properties to allow userspace to
  toggle between FB and solid fill sources
- Loosen NULL FB checks within the DRM atomic commit callstack to allow
  for NULL FB when solid fill is enabled.
- Add NULL FB checks in methods where FB was previously assumed to be
  non-NULL
- Have MSM DPU driver use drm_plane_state.solid_fill instead of
  dpu_plane_state.color_fill

Note: The solid fill planes feature depends on both the solid_fill *and*
pixel_source properties.

To use this feature, userspace can set the solid_fill property to a blob
containing the appropriate version number and solid fill color (in
RGB323232 format) and and setting the pixel_source property to
DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the
resulting plane will display the color specified by the solid_fill blob.

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.

This 2 property approach was chosen because passing in a special 1x1 FB
with the necessary color information would have unecessary overhead that
does not reflect the behavior of the solid fill feature. In addition,
assigning the solid fill blob to FB_ID would require loosening some core
drm_property checks that might cause unwanted side effects elsewhere.

---
Changes in v5:
- Added support for PIXEL_SOURCE_NONE (Sebastian)
- Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't
  set (Dmitry)
- Added debugfs support for both properties (Dmitry)
- Corrected u32 to u8 conversion (Pekka)
- Moved drm_solid_fill_info struct and related documentation to
  include/uapi (Pekka)
- Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka)
- Added more detailed UAPI and kernel documentation (Pekka)
- Reordered patch series so that the pixel_source property is introduced
  before solid_fill (Dmitry)
- Fixed inconsistent ABGR8888/RGBA8888 format declaration (Pekka)
- Reset pixel_source to FB in drm_mode_setplane() (Dmitry)
- Rename supported_sources to extra_sources (Dmitry)
- Only destroy old solid_fill blob state if new state is valid (Pekka)
- Link to v4: https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa742d@quicinc.com

Changes in v4:
- Rebased onto latest kernel
- Reworded cover letter for clarity (Dmitry)
- Reworded commit messages for clarity
- Split existing changes into smaller commits
- Added pixel_source enum property (Dmitry, Pekka, Ville)
- Updated drm-kms comment docs with pixel_source and solid_fill
  properties (Dmitry)
- Inlined drm_atomic_convert_solid_fill_info() (Dmitry)
- Passed in plane state alpha value to _dpu_plane_color_fill_pipe()
- Link to v3: https://lore.kernel.org/r/20230104234036.636-1-quic_jesszhan@quicinc.com

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)
- Fixed typo in drm_solid_fill struct documentation
- 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)

Changes in v2:
- Dropped SOLID_FILL_FORMAT property (Simon)
- Switched to implementing solid_fill property as a blob (Simon, Dmitry)
- Added drm_solid_fill and drm_solid_fill_info structs (Simon)
- 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)
- Changed to checks for if solid_fill_blob is set (Dmitry)
- Abstracted (plane_state && !solid_fill_blob) checks to helper method
  (Dmitry)
- Fixed dropped 'const' warning
- Added helper to convert color fill to BGR888 (Rob)
- Fixed indentation issue (Dmitry)
- Added support for solid fill on planes of varying sizes

---
Jessica Zhang (10):
      drm: Introduce pixel_source DRM plane property
      drm: Introduce solid fill DRM plane property
      drm: Add solid fill pixel source
      drm/atomic: Add pixel source to plane state dump
      drm/atomic: Add solid fill data to plane state dump
      drm/atomic: Move framebuffer checks to helper
      drm/atomic: Loosen FB atomic checks
      drm/msm/dpu: Allow NULL FBs in atomic commit
      drm/msm/dpu: Use DRM solid_fill property
      drm/msm/dpu: Add solid fill and pixel source properties

 drivers/gpu/drm/drm_atomic.c              | 147 +++++++++++++++++-------------
 drivers/gpu/drm/drm_atomic_helper.c       |  34 ++++---
 drivers/gpu/drm/drm_atomic_state_helper.c |  10 ++
 drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++++
 drivers/gpu/drm/drm_blend.c               | 123 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h       |   2 +
 drivers/gpu/drm/drm_plane.c               |  41 ++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  67 +++++++++-----
 include/drm/drm_atomic_helper.h           |   4 +-
 include/drm/drm_blend.h                   |   3 +
 include/drm/drm_plane.h                   |  89 ++++++++++++++++++
 include/uapi/drm/drm_mode.h               |  24 +++++
 13 files changed, 508 insertions(+), 104 deletions(-)
---
base-commit: eab616ad7f56cafc8af85e9774816f0901e1efa2
change-id: 20230404-solid-fill-05016175db36

Best regards,

Comments

Dmitry Baryshkov July 29, 2023, 12:04 a.m. UTC | #1
On 28/07/2023 20:02, Jessica Zhang wrote:
> Add pixel source to the atomic plane state dump
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/drm_atomic.c        |  1 +
>   drivers/gpu/drm/drm_crtc_internal.h |  2 ++
>   drivers/gpu/drm/drm_plane.c         | 12 ++++++++++++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b4c6ffc438da..c38014abc590 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -713,6 +713,7 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>   
>   	drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name);
>   	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
> +	drm_printf(p, "\tpixel-source=%s\n", drm_plane_get_pixel_source_name(state->pixel_source));
>   	drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
>   	if (state->fb)
>   		drm_framebuffer_print_info(p, 2, state->fb);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 501a10edd0e1..75b59ec9f1be 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -38,6 +38,7 @@ enum drm_color_encoding;
>   enum drm_color_range;
>   enum drm_connector_force;
>   enum drm_mode_status;
> +enum drm_plane_pixel_source;
>   
>   struct drm_atomic_state;
>   struct drm_bridge;
> @@ -267,6 +268,7 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
>   				 u32 format, u64 modifier);
>   struct drm_mode_rect *
>   __drm_plane_get_damage_clips(const struct drm_plane_state *state);
> +const char *drm_plane_get_pixel_source_name(enum drm_plane_pixel_source pixel_source);
>   
>   /* drm_bridge.c */
>   void drm_bridge_detach(struct drm_bridge *bridge);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index f342cf15412b..4188b3491625 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1487,6 +1487,18 @@ __drm_plane_get_damage_clips(const struct drm_plane_state *state)
>   					state->fb_damage_clips->data : NULL);
>   }
>   
> +const char *drm_plane_get_pixel_source_name(enum drm_plane_pixel_source pixel_source)
> +{
> +	switch(pixel_source) {
> +	case DRM_PLANE_PIXEL_SOURCE_NONE:
> +		return "NONE";
> +	case DRM_PLANE_PIXEL_SOURCE_FB:
> +		return "fb";
> +	default:
> +		return "";
> +	}
> +}

Please use DRM_ENUM_NAME_FN instead.

> +
>   /**
>    * drm_plane_get_damage_clips - Returns damage clips.
>    * @state: Plane state.
>
Dmitry Baryshkov July 31, 2023, 4:02 a.m. UTC | #2
On 28/07/2023 20:02, Jessica Zhang wrote:
> Add "SOLID_FILL" as a valid pixel source. If the pixel_source property is
> set to "SOLID_FILL", it will display data from the drm_plane "solid_fill"
> blob property.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/drm_blend.c | 10 +++++++++-
>   include/drm/drm_plane.h     |  1 +
>   2 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index c632dfcd8a9b..34b1fd3e2310 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,9 @@
>    *	"FB":
>    *		Framebuffer source set by the "FB_ID" property.
>    *
> + *	"SOLID_FILL":
> + *		Solid fill color source set by the "solid_fill" property.
> + *
>    * solid_fill:
>    *	solid_fill is set up with drm_plane_create_solid_fill_property(). It
>    *	contains pixel data that drivers can use to fill a plane.
> @@ -657,6 +660,9 @@ EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
>    * "FB":
>    *	Framebuffer pixel source
>    *
> + * "SOLID_FILL":
> + * 	Solid fill color pixel source
> + *
>    * Returns:
>    * Zero on success, negative errno on failure.
>    */
> @@ -668,8 +674,10 @@ int drm_plane_create_pixel_source_property(struct drm_plane *plane,
>   	static const struct drm_prop_enum_list enum_list[] = {
>   		{ DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
>   		{ DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
> +		{ DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
>   	};
> -	static const unsigned int valid_source_mask = BIT(DRM_PLANE_PIXEL_SOURCE_FB);
> +	static const unsigned int valid_source_mask = BIT(DRM_PLANE_PIXEL_SOURCE_FB) |
> +						      BIT(DRM_PLANE_PIXEL_SOURCE_SOLID_FILL);
>   	int i;
>   
>   	/* FB is supported by default */
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index abf1458fa099..234fee3d5a95 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -43,6 +43,7 @@ enum drm_scaling_filter {
>   enum drm_plane_pixel_source {
>   	DRM_PLANE_PIXEL_SOURCE_NONE,
>   	DRM_PLANE_PIXEL_SOURCE_FB,
> +	DRM_PLANE_PIXEL_SOURCE_SOLID_FILL,
>   	DRM_PLANE_PIXEL_SOURCE_MAX
>   };
>   
>
Jessica Zhang Aug. 7, 2023, 4:39 p.m. UTC | #3
On 7/28/2023 5:04 PM, Dmitry Baryshkov wrote:
> On 28/07/2023 20:02, Jessica Zhang wrote:
>> Add pixel source to the atomic plane state dump
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c        |  1 +
>>   drivers/gpu/drm/drm_crtc_internal.h |  2 ++
>>   drivers/gpu/drm/drm_plane.c         | 12 ++++++++++++
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index b4c6ffc438da..c38014abc590 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -713,6 +713,7 @@ static void drm_atomic_plane_print_state(struct 
>> drm_printer *p,
>>       drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name);
>>       drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : 
>> "(null)");
>> +    drm_printf(p, "\tpixel-source=%s\n", 
>> drm_plane_get_pixel_source_name(state->pixel_source));
>>       drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
>>       if (state->fb)
>>           drm_framebuffer_print_info(p, 2, state->fb);
>> diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
>> b/drivers/gpu/drm/drm_crtc_internal.h
>> index 501a10edd0e1..75b59ec9f1be 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -38,6 +38,7 @@ enum drm_color_encoding;
>>   enum drm_color_range;
>>   enum drm_connector_force;
>>   enum drm_mode_status;
>> +enum drm_plane_pixel_source;
>>   struct drm_atomic_state;
>>   struct drm_bridge;
>> @@ -267,6 +268,7 @@ int drm_plane_check_pixel_format(struct drm_plane 
>> *plane,
>>                    u32 format, u64 modifier);
>>   struct drm_mode_rect *
>>   __drm_plane_get_damage_clips(const struct drm_plane_state *state);
>> +const char *drm_plane_get_pixel_source_name(enum 
>> drm_plane_pixel_source pixel_source);
>>   /* drm_bridge.c */
>>   void drm_bridge_detach(struct drm_bridge *bridge);
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index f342cf15412b..4188b3491625 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -1487,6 +1487,18 @@ __drm_plane_get_damage_clips(const struct 
>> drm_plane_state *state)
>>                       state->fb_damage_clips->data : NULL);
>>   }
>> +const char *drm_plane_get_pixel_source_name(enum 
>> drm_plane_pixel_source pixel_source)
>> +{
>> +    switch(pixel_source) {
>> +    case DRM_PLANE_PIXEL_SOURCE_NONE:
>> +        return "NONE";
>> +    case DRM_PLANE_PIXEL_SOURCE_FB:
>> +        return "fb";
>> +    default:
>> +        return "";
>> +    }
>> +}
> 
> Please use DRM_ENUM_NAME_FN instead.

Hi Dmitry,

Acked.

Thanks,

Jessica Zhang

> 
>> +
>>   /**
>>    * drm_plane_get_damage_clips - Returns damage clips.
>>    * @state: Plane state.
>>
> 
> -- 
> With best wishes
> Dmitry
>