RFC: drm/drm_plane: Expose the plane capability and interoperability

Commit Message

Murthy, Arun R July 9, 2024, 7:46 a.m. UTC

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.

Signed-off-by: Arun R Murthy <[e-mail]>
---
 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========================

Comments

Xaver Hugl July 11, 2024, 3:35 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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 {

Shankar, Uma July 18, 2024, 4:58 a.m. UTC | #2

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Xaver
> Hugl
> Sent: Thursday, July 11, 2024 9:06 PM
> To: [e-mail]
> Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> interoperability
> 
> 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.

Adding new fields to a structure part of an existing UAPI may not be a great idea.
This can have some issues of backward compatibility and multiple structure definitions
with various kernel and user space header versions. So this doesn't look very promising.
Others can chime in and share their perspective.
 
We can expose an immutable plane capability property (with interoperability info) which userspace
can read and based on the capabilities exposed plane the composition. So, option 2 looks more
viable to me.

Regards,
Uma Shankar

> > Signed-off-by: Arun R Murthy <[e-mail]>
> > ---
> >  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 {
> 
> 
>

Murthy, Arun R July 29, 2024, 4:59 a.m. UTC | #3

Gentle Reminder!
Any comments?

Thanks and Regards,
Arun R Murthy
--------------------

> -----Original Message-----
> From: Murthy, Arun R <[e-mail]>
> Sent: Tuesday, July 9, 2024 1:17 PM
> To: [e-mail]; [e-mail]
> Cc: Murthy, Arun R <[e-mail]>
> Subject: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> interoperability
> 
> 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.
> 
> Signed-off-by: Arun R Murthy <[e-mail]>
> ---
>  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 {
> --
> 2.25.1

Dmitry Baryshkov July 29, 2024, 10:51 p.m. UTC | #4

On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> Gentle Reminder!
> Any comments?

First of all, the format is underdocumented. Second, there is a usual
requirement for new uAPI: please provide a pointer to IGT patch and to
the userspace utilising the property.

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> 
> > -----Original Message-----
> > From: Murthy, Arun R <[e-mail]>
> > Sent: Tuesday, July 9, 2024 1:17 PM
> > To: [e-mail]; [e-mail]
> > Cc: Murthy, Arun R <[e-mail]>
> > Subject: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> > interoperability
> > 
> > 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.
> > 
> > Signed-off-by: Arun R Murthy <[e-mail]>
> > ---
> >  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 {
> > --
> > 2.25.1
>

Murthy, Arun R July 30, 2024, 4:07 a.m. UTC | #5

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Tuesday, July 30, 2024 4:21 AM
> To: Murthy, Arun R <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> interoperability
> 
> On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> > Gentle Reminder!
> > Any comments?
> 
> First of all, the format is underdocumented. Second, there is a usual
> requirement for new uAPI: please provide a pointer to IGT patch and to the
> userspace utilising the property.
There are some discussions on using this in UMD. https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#note_2487123

Thanks and Regards,
Arun R Murthy
--------------------
> 
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> >
> > > -----Original Message-----
> > > From: Murthy, Arun R <[e-mail]>
> > > Sent: Tuesday, July 9, 2024 1:17 PM
> > > To: [e-mail]; [e-mail]
> > > Cc: Murthy, Arun R <[e-mail]>
> > > Subject: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> > > interoperability
> > >
> > > 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.
> > >
> > > Signed-off-by: Arun R Murthy <[e-mail]>
> > > ---
> > >  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 {
> > > --
> > > 2.25.1
> >
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 31, 2024, 8:33 a.m. UTC | #6

On Tue, 30 Jul 2024 at 07:07, Murthy, Arun R <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Tuesday, July 30, 2024 4:21 AM
> > To: Murthy, Arun R <[e-mail]>
> > Cc: [e-mail]; [e-mail]
> > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> > interoperability

Please fix your email client.

> >
> > On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> > > Gentle Reminder!
> > > Any comments?
> >
> > First of all, the format is underdocumented. Second, there is a usual
> > requirement for new uAPI: please provide a pointer to IGT patch and to the
> > userspace utilising the property.
> There are some discussions on using this in UMD. https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#note_2487123

It should be a MR rather than "some discussion". And IGT patchset too, please.

Regarding the patch itself. It is completely underdocumented. There is
no way for me to understand which of these caps should e.g. be set for
the drm/msm planes.

>
> Thanks and Regards,
> Arun R Murthy
> --------------------
> >
> > >
> > > Thanks and Regards,
> > > Arun R Murthy
> > > --------------------
> > >
> > > > -----Original Message-----
> > > > From: Murthy, Arun R <[e-mail]>
> > > > Sent: Tuesday, July 9, 2024 1:17 PM
> > > > To: [e-mail]; [e-mail]
> > > > Cc: Murthy, Arun R <[e-mail]>
> > > > Subject: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> > > > interoperability
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Arun R Murthy <[e-mail]>
> > > > ---
> > > >  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 {
> > > > --
> > > > 2.25.1
> > >
> >
> > --
> > With best wishes
> > Dmitry

Murthy, Arun R July 31, 2024, 8:48 a.m. UTC | #7

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Wednesday, July 31, 2024 2:04 PM
> To: Murthy, Arun R <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> interoperability
> 
> On Tue, 30 Jul 2024 at 07:07, Murthy, Arun R <[e-mail]>
> wrote:
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Tuesday, July 30, 2024 4:21 AM
> > > To: Murthy, Arun R <[e-mail]>
> > > Cc: [e-mail]; [e-mail]
> > > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability
> > > and interoperability
> 
> Please fix your email client.
> 
Sorry for that. Sure will fix it.

> > >
> > > On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> > > > Gentle Reminder!
> > > > Any comments?
> > >
> > > First of all, the format is underdocumented. Second, there is a
> > > usual requirement for new uAPI: please provide a pointer to IGT
> > > patch and to the userspace utilising the property.
> > There are some discussions on using this in UMD.
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#note_2
> > 487123
> 
> It should be a MR rather than "some discussion". And IGT patchset too, please.
There is no IGT patch yet.
> 
> Regarding the patch itself. It is completely underdocumented. There is no way
> for me to understand which of these caps should e.g. be set for the drm/msm
> planes.

I have explained it in the patch header. There are certain plane restrictions.
For example, certain pixel formats are not supported in async flip. If this is known to the compositor ahead, then compositor sending a flip with this unsupported formats leads to a flip failure. In order to overcome this if the KMD sends the list of supported pixel formats, compositor can verify for the same and then send the flip request.
This can be achieved in two options. The options are listed below in the patch header and expected some review comments or suggestion as to which option to use!

Thanks and Regards,
Arun R Murthy
--------------------
> 
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> > >
> > > >
> > > > Thanks and Regards,
> > > > Arun R Murthy
> > > > --------------------
> > > >
> > > > > -----Original Message-----
> > > > > From: Murthy, Arun R <[e-mail]>
> > > > > Sent: Tuesday, July 9, 2024 1:17 PM
> > > > > To: [e-mail];
> > > > > [e-mail]
> > > > > Cc: Murthy, Arun R <[e-mail]>
> > > > > Subject: [PATCH] RFC: drm/drm_plane: Expose the plane capability
> > > > > and interoperability
> > > > >
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Arun R Murthy <[e-mail]>
> > > > > ---
> > > > >  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 {
> > > > > --
> > > > > 2.25.1
> > > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> 
> 
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 31, 2024, 9:20 a.m. UTC | #8

On Wed, 31 Jul 2024 at 11:48, Murthy, Arun R <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Wednesday, July 31, 2024 2:04 PM
> > To: Murthy, Arun R <[e-mail]>
> > Cc: [e-mail]; [e-mail]
> > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> > interoperability
> >
> > On Tue, 30 Jul 2024 at 07:07, Murthy, Arun R <[e-mail]>
> > wrote:
> > >
> > > > -----Original Message-----
> > > > From: Dmitry Baryshkov <[e-mail]>
> > > > Sent: Tuesday, July 30, 2024 4:21 AM
> > > > To: Murthy, Arun R <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]
> > > > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability
> > > > and interoperability
> >
> > Please fix your email client.
> >
> Sorry for that. Sure will fix it.
>
> > > >
> > > > On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> > > > > Gentle Reminder!
> > > > > Any comments?
> > > >
> > > > First of all, the format is underdocumented. Second, there is a
> > > > usual requirement for new uAPI: please provide a pointer to IGT
> > > > patch and to the userspace utilising the property.
> > > There are some discussions on using this in UMD.
> > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#note_2
> > > 487123
> >
> > It should be a MR rather than "some discussion". And IGT patchset too, please.
> There is no IGT patch yet.
> >
> > Regarding the patch itself. It is completely underdocumented. There is no way
> > for me to understand which of these caps should e.g. be set for the drm/msm
> > planes.
>
> I have explained it in the patch header. There are certain plane restrictions.
> For example, certain pixel formats are not supported in async flip. If this is known to the compositor ahead, then compositor sending a flip with this unsupported formats leads to a flip failure. In order to overcome this if the KMD sends the list of supported pixel formats, compositor can verify for the same and then send the flip request.
> This can be achieved in two options. The options are listed below in the patch header and expected some review comments or suggestion as to which option to use!

It is impossible to understand what your options / capabilities
_actually_ mean. I browsed through the patch and I still don't
understand how to select which options apply to DRM_FORMAT_MOD_QCOM_*

Murthy, Arun R Aug. 2, 2024, 5:20 a.m. UTC | #9

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Wednesday, July 31, 2024 2:51 PM
> To: Murthy, Arun R <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability and
> interoperability
> 
> On Wed, 31 Jul 2024 at 11:48, Murthy, Arun R <[e-mail]>
> wrote:
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Wednesday, July 31, 2024 2:04 PM
> > > To: Murthy, Arun R <[e-mail]>
> > > Cc: [e-mail]; [e-mail]
> > > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane capability
> > > and interoperability
> > >
> > > On Tue, 30 Jul 2024 at 07:07, Murthy, Arun R
> > > <[e-mail]>
> > > wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > Sent: Tuesday, July 30, 2024 4:21 AM
> > > > > To: Murthy, Arun R <[e-mail]>
> > > > > Cc: [e-mail];
> > > > > [e-mail]
> > > > > Subject: Re: [PATCH] RFC: drm/drm_plane: Expose the plane
> > > > > capability and interoperability
> > >
> > > Please fix your email client.
> > >
> > Sorry for that. Sure will fix it.
> >
> > > > >
> > > > > On Mon, Jul 29, 2024 at 04:59:14AM GMT, Murthy, Arun R wrote:
> > > > > > Gentle Reminder!
> > > > > > Any comments?
> > > > >
> > > > > First of all, the format is underdocumented. Second, there is a
> > > > > usual requirement for new uAPI: please provide a pointer to IGT
> > > > > patch and to the userspace utilising the property.
> > > > There are some discussions on using this in UMD.
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#no
> > > > te_2
> > > > 487123
> > >
> > > It should be a MR rather than "some discussion". And IGT patchset too,
> please.
> > There is no IGT patch yet.
> > >
> > > Regarding the patch itself. It is completely underdocumented. There
> > > is no way for me to understand which of these caps should e.g. be
> > > set for the drm/msm planes.
> >
> > I have explained it in the patch header. There are certain plane restrictions.
> > For example, certain pixel formats are not supported in async flip. If this is
> known to the compositor ahead, then compositor sending a flip with this
> unsupported formats leads to a flip failure. In order to overcome this if the KMD
> sends the list of supported pixel formats, compositor can verify for the same
> and then send the flip request.
> > This can be achieved in two options. The options are listed below in the patch
> header and expected some review comments or suggestion as to which option
> to use!
> 
> It is impossible to understand what your options / capabilities _actually_ mean.
> I browsed through the patch and I still don't understand how to select which
> options apply to DRM_FORMAT_MOD_QCOM_*
> 
Sorry for that let me put it more simple words.
Agenda: KMD should share the plane restrictions i.e capability in terms of what is
supported and what is not to the UMD.
We can see two options of achieving this
Option 1: drm_mode_getplane() -> This function exposes the plane metadata
such as formats. But these are generic formats supported by the plane. There 
can be few restrictions like some formats not supported by async flip, certain
formats not supported in 270/90 deg rotation and so on. These are the hardware
limitations and in order to expose these, this function can be reused. A new 
pointer is added to the existing drm_mode_get_plane struct to share this.

--- 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;
 };

Option 2:  Exposing this restriction/capabilities as a plane property.
The struct drm_format_blob defined in Option 1 will be used over here as well.

--- 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;
 };

Thanks and Regards,
Arun R Murthy
--------------------

Patch

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 {