[PATCH] RFC: drm/drm_plane: Expose the plane capability and interoperability

[Xaver Hugl]

Hi,

On Dienstag, 9. Juli 2024 09:46:56 MESZ Arun R Murthy wrote:
> Each plane has its own capability or interoperability based on the
> harware restrictions. If this is exposed to the user, then user can read
> it once on boot and store this. Later can be used as a lookup table to
> check a corresponding capability is supported by plane then only go
> ahead with the framebuffer creation/calling atomic_ioctl.
> 
> For Ex: There are few restiction as to async flip doesnt support all the
> formats/modifiers. Similar restrictions on scaling. With the
> availabililty of this info to user, failures in atomic_check can be
> avoided as these are more the hardware capabilities.
> 
> There are two options on how this can be acheived.
> Option 1:
> 
> Add a new element to struct drm_mode_get_plane that holds the addr to
> the array of a new struct. This new struct consists of the formats
> supported and the corresponding plane capabilities.
> 
> Option 2:
> 
> These can be exposed to user as a plane property so that the user can get to
> know the limitation ahead and avoid failures in atomic_check.
> 
> Usually atomic_get_property is controlled over the state struct for the
> parameters/elements that can change. But here these capabilities or the
> interoperabilities are rather hardware restrictions and wont change over
> flips. Hence having as a plane_property may not make much sense.
> On the other hand, Option 1 include changes in the uapi struct
> drm_mode_get_plane. Shouldnt have impact on backward compatibility, but
> if userspace has some implementation so as to check the size of the
> struct, then it might a challenge.

Adding fields to the struct should be okay, but adding a per-plane immutable 
property is also fine and IMO the cleaner option. We already have the same 
thing with "IN_FORMATS" and "type" that don't ever change either.

Either way, having a capability flag per format+modifier pair sounds good to me 
and should be both easy to make use of in userspace.

> Signed-off-by: Arun R Murthy <arun.r.murthy at intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  3 +++
>  include/drm/drm_plane.h           |  8 ++++++++
>  include/uapi/drm/drm_mode.h       | 20 ++++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> =============Option 2========================
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c index 22bbb2d83e30..b46177d5fc8c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -631,6 +631,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->hotspot_x;
>  	} else if (property == plane->hotspot_y_property) {
>  		*val = state->hotspot_y;
> +	} else if (property == config->prop_plane_caps) {
> +		*val = (state->plane_caps) ?
> +			state->plane_caps->base.id : 0;
>  	} else {
>  		drm_dbg_atomic(dev,
>  			       "[PLANE:%d:%s] unknown property 
[PROP:%d:%s]\n",
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index dd718c62ac31..dfe931677d0a 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -260,6 +260,14 @@ struct drm_plane_state {
>  	 * flow.
>  	 */
>  	bool color_mgmt_changed : 1;
> +
> +	/**
> +	 * @plane_caps:
> +	 *
> +	 * Blob representing plane capcabilites and interoperability.
> +	 * This element is a pointer to the array of struct 
drm_format_blob.
> +	 */
> +	struct drm_property_blob *plane_caps;
>  };
> 
>  static inline struct drm_rect
> 
> =============Option 1========================
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d390011b89b4..0b5c1b65ef63 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -312,6 +312,20 @@ struct drm_mode_set_plane {
>  	__u32 src_w;
>  };
> 
> +#define DRM_FORMAT_PLANE_CAP_LINEAR_TILE	BIT(0)
> +#define DRM_FORMAT_PLANE_CAP_X_TILE		BIT(1)
> +#define DRM_FORMAT_PLANE_CAP_Y_TILE		BIT(2)
> +#define DRM_FORMAT_PLANE_CAP_Yf_TILE		BIT(3)
> +#define DRM_FORMAT_PLANE_CAP_ASYNC_FLIP		BIT(4)
> +#define DRM_FORMAT_PLANE_CAP_FBC		BIT(5)
> +#define DRM_FORMAT_PLANE_CAP_RC			BIT(6)
> +
> +struct drm_format_blob {
> +	__u64 modifier;
> +	__u32 plane_caps;
> +
> +};
> +
>  /**
>   * struct drm_mode_get_plane - Get plane metadata.
>   *
> @@ -355,6 +369,12 @@ struct drm_mode_get_plane {
>  	 * supported by the plane. These formats do not require modifiers.
>  	 */
>  	__u64 format_type_ptr;
> +	/**
> +	 * @ format_blob_ptr: Pointer to the array of struct 
drm_format_blob.
> +	 * Specify the plane capabilites/restrictions w.r.t tiling/sync-
async
> +	 * flips etc
> +	 */
> +	__u64 format_blob_ptr;
>  };
> 
>  struct drm_mode_get_plane_res {