> -----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
> 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.
> * 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?
> + * 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.
Regards
Chaitanya
> /**
>
> --
> 2.25.1
