> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Sent: Monday, January 13, 2025 1:52 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-
> xe@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v3 1/5] drm/plane: Add new plane property
> IN_FORMATS_ASYNC
>
> > > -----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.
>
The line "Without this property the plane doesn't support buffers with modifiers " indicates that if this property is not present then no modifiers are supported for async.
This can't be true because all drivers currently do not have this property but they surely support modifiers in async flip. More on this below.
> > > * 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.
>
Yes, they are different properties however we have to clearly define the policy for the property. For example, with the current
implementation the policy should be something like this.
1. Userspace checks if IN_FORMATS_ASYNC property is present. If present check if current format and modifier pair is supported.
2. If property the IN_FORMATS_ASYNC is *not* present, we have two options
a. The user space decides that no modifier is supported. We *cannot* go by this because IN_FORMAT_ASYNC is an optional property
and many drivers might not implement it. Because all the format modifier pair supported for sync are also supported for async.
b. The user space will check if the frame modifier pair is present in IN_FORMAT_SYNC. This should be explicitly called out in the documentation.
If no format modifier pair is found after these two steps then we can conclude that no modifier is supported.
It is important to call it out because if you see the code proposed by Naveen in comments in the MR[1]
if (meta_onscreen_native_is_tearing_enabled (onscreen_native))
crtc_mods = meta_kms_plane_get_tearing_modifiers_for_format (plane, format);
else
crtc_mods = meta_kms_plane_get_modifiers_for_format (plane, format);
This makes the property IN_FORMAT_ASYNC non-optional. That is the absence of the property will lead to no format modifier
pair supported even though the driver did not intend it that way.
[1] https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4063/diffs?commit_id=c21a47175d7ec0f48414335cd2de010ae9670626#f4e112542f79a68679333911002f2f66d032fbf6
Regards
Chaitanya
> Thanks and Regards,
> Arun R Murthy
> --------------------
