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 <arun.r.murthy@xxxxxxxxx>
> > Sent: Tuesday, July 9, 2024 1:17 PM
> > To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > 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 <arun.r.murthy@xxxxxxxxx>
> > ---
> > 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
