2023-09-26 17:26:36

by Harry Wentland

[permalink] [raw]
Subject: Re: [PATCH RFC v6 00/10] Support for Solid Fill Planes



On 2023-08-28 20:05, Jessica Zhang wrote:
> 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.
>

I didn't have a detailed review of this patchset but at a high-level this
change makes sense to me.

Feel free to add my
Acked-by: Harry Wentland <[email protected]>
to patches 1-5.

Harry

> ---
> Changes in v6:
> - Have _dpu_plane_color_fill() take in a single ABGR8888 color instead
> of having separate alpha and BGR color parameters (Dmitry)
> - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check
> in SetPlane ioctl (Dmitry)
> - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian)
> - Dropped versioning from solid fill property blob (Dmitry)
> - Use DRM_ENUM_NAME_FN (Dmitry)
> - Use drm_atomic_replace_property_blob_from_id() (Dmitry)
> - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry)
> - Group redundant NULL FB checks (Dmitry)
> - Squashed drm_plane_needs_disable() implementation with
> DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian)
> - Add comment to support RGBA solid fill color in the future (Dmitry)
> - Link to v5: https://lore.kernel.org/r/[email protected]
>
> 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/[email protected]
>
> 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/[email protected]
>
> 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 | 36 ++++----
> drivers/gpu/drm/drm_atomic_state_helper.c | 10 ++
> drivers/gpu/drm/drm_atomic_uapi.c | 30 ++++++
> drivers/gpu/drm/drm_blend.c | 129 ++++++++++++++++++++++++++
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_plane.c | 27 +++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 80 ++++++++++------
> include/drm/drm_atomic_helper.h | 4 +-
> include/drm/drm_blend.h | 3 +
> include/drm/drm_plane.h | 90 ++++++++++++++++++
> include/uapi/drm/drm_mode.h | 24 +++++
> 13 files changed, 478 insertions(+), 112 deletions(-)
> ---
> base-commit: 00ee72279c963989ab435b0bc90b5dc05a9aab79
> change-id: 20230404-solid-fill-05016175db36
>
> Best regards,