> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Arun R Murthy
> > Sent: Wednesday, January 8, 2025 11:09 AM
> > To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > intel- xe@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > Subject: [PATCH v3 1/5] drm/plane: Add new plane property
> > IN_FORMATS_ASYNC
> >
> > There exists a property IN_FORMATS which exposes the plane supported
> > modifiers/formats to the user. In some platforms when asynchronous
> > flips are used all of modifiers/formats mentioned in IN_FORMATS are not
> supported.
> > This patch adds a new plane property IN_FORMATS_ASYNC to expose the
> > async flips supported modifiers/formats so that user can use this
> > information ahead and done flips with unsupported formats/modifiers.
> > This will save flips failures.
>
> s/done/do
> s/unsupported/supported
> s/flips/flip
>
Done!
> > Add a new function pointer similar to format_mod_supported
> > specifically for asynchronous flips.
> >
> > v2: Remove async variable from drm_plane (Ville)
> > v3: Add new function pointer for async (Ville)
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 7 +++++++
> > drivers/gpu/drm/drm_plane.c | 6 ++++++
> > include/drm/drm_mode_config.h | 6 ++++++
> > include/drm/drm_plane.h | 20 ++++++++++++++++++++
> > 4 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > index
> >
> 8642a2fb25a90116dab975aa0ab6b51deafb4b96..dbbef20753f834a85ae9ded
> > 748cff2b3f0e85043 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -388,6 +388,13 @@ static int
> > drm_mode_create_standard_properties(struct drm_device *dev)
> > return -ENOMEM;
> > dev->mode_config.size_hints_property = prop;
> >
> > + prop = drm_property_create(dev,
> > + DRM_MODE_PROP_IMMUTABLE |
> > DRM_MODE_PROP_BLOB,
> > + "IN_FORMATS_ASYNC", 0);
> > + if (!prop)
> > + return -ENOMEM;
> > + dev->mode_config.async_modifiers_property = prop;
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index
> >
> a28b22fdd7a41aca82d097d42237851da9a0a79b..416818e54ccffcf3d3aada27
> > 23e96ce8ccf1dd97 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -141,6 +141,12 @@
> > * various bugs in this area with inconsistencies between the capability
> > * flag and per-plane properties.
> > *
> > + * IN_FORMATS_ASYNC:
> > + * Blob property which contains the set of buffer format and modifier
> > + * pairs supported by this plane for asynchronous flips. The blob is a struct
> > + * drm_format_modifier_blob. Without this property the plane doesn't
> > + * support buffers with modifiers. Userspace cannot change this property.
> > + *
>
> I think we should mention here that the absence of this property would mean
> that all format modifier pair in IN_FORMATS are also supported for async flips.
> That way the uAPI remains backward compatible.
>
I think this is clear with the above documentation. Anyway will
reframe to be more specific.
> > * SIZE_HINTS:
> > * Blob property which contains the set of recommended plane size
> > * which can used for simple "cursor like" use cases (eg. no scaling).
> > diff --git a/include/drm/drm_mode_config.h
> > b/include/drm/drm_mode_config.h index
> >
> 271765e2e9f2da62aaf0d258828ef4196e14822e..0c116d6dfd277262b1a4c0f0
> > 97fce2d719f43844 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -936,6 +936,12 @@ struct drm_mode_config {
> > */
> > struct drm_property *modifiers_property;
> >
> > + /**
> > + * @async_modifiers_property: Plane property to list support
> > modifier/format
> > + * combination for asynchronous flips.
> > + */
> > + struct drm_property *async_modifiers_property;
> > +
> > /**
> > * @size_hints_property: Plane SIZE_HINTS property.
> > */
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> >
> dd718c62ac31bf16606f3ee9f025a5b171cd1e67..e8749e6fc3bc0acfc73bbd840
> > 1f85c3126e86759 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -549,6 +549,26 @@ struct drm_plane_funcs {
> > */
> > bool (*format_mod_supported)(struct drm_plane *plane, uint32_t
> > format,
> > uint64_t modifier);
> > + /**
> > + * @format_mod_supported_async:
> > + *
> > + * This optional hook is used for the DRM to determine if for
> > + * asychronous flip the given format/modifier combination is valid for
> > + * the plane. This allows the DRM to generate the correct format
> > + * bitmask (which formats apply to which modifier), and to validate
> > + * modifiers at atomic_check time.
> > + *
> > + * If not present, then any modifier in the plane's modifier
> > + * list is allowed with any of the plane's formats.
> > + *
>
> We still have to honor the IN_FORMATS property, right?
Sorry didn’t get it, this new property is in parallel to the existing
IN_FORMATS. We have retained it as is and added new property for async.
Anyway will reframe to be more clear.
>
> > + * Returns:
> > + *
> > + * True if the given modifier is valid for that format on the plane.
> > + * False otherwise.
> > + */
> > + bool (*format_mod_supported_async)(struct drm_plane *plane,
> > + uint32_t format, uint64_t modifier);
> > +
> > };
> >
>
> Some thoughts after going through all the patches. There is a bit of ambiguity
> how a driver can handle the IN_FORMATS_ASYNC property in cases where
> there are no specific restrictions in format modifier pairs for async.
>
> Two approaches here.
>
> 1. The driver should not expose the property at all.
> 2. The driver should fill up IN_FORMATS_ASYNC blob that indicates that all
> format modifier pair supported for sync are also supported for async flips.
>
> Approach 1 seems to be better for backward compatibility. If we follow
> approach 2, we do not need to expose the function create_in_formats_blob()
> and it can be handled through the format_mod_supported_async() function
> internally during plane initialization.
>
> Either way the documentation should be made clear what we expect from the
> userspace.
>
Documentation added says this is an optional property to convey the user with the list for formats/modifiers supported by the plane for async flips.
Compatibility wise this is a new property which is in parallel to IN_FORMATS and hence should not break anything.
Anyway will reframe and be more specific.
Thanks and Regards,
Arun R Murthy
--------------------
