[PATCH v3 0/5] Expose modifiers/formats supported by async flips

[PATCH v3 0/5] Expose modifiers/formats supported by async flips

[Arun R Murthy]

All of the formats/modifiers supported by the plane during synchronous
flips are nor supported by asynchronous flips. The formats/modifiers
exposed to user by IN_FORMATS exposes all formats/modifiers supported by
plane and this list varies for async flips. If the async flip supported
formats/modifiers are exposed to the user, user based on this list can
take decision to proceed or not and avoid flip failures during async
flips.
Discussion around this can be located @
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29618#note_2487123
Userspace implementation for IN_FORMARTS_ASYNC under review @
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4063

TODO: Upon merge of the patch related to async flip
https://patchwork.freedesktop.org/patch/626849/?series=139807&rev=6
the patch 5 in this series will have to make use of the new function
pointer can_async_flip().

v3: Add new plane->funcs format_mod_supported_async (Ville)

Arun R Murthy (3):
  drm/plane: Add new plane property IN_FORMATS_ASYNC
  drm/plane: Expose function to create format/modifier blob
  drm/i915/display: Populate list of async supported formats/modifiers

 drivers/gpu/drm/drm_mode_config.c             |  7 +++
 drivers/gpu/drm/drm_plane.c                   | 50 ++++++++++++------
 .../drm/i915/display/skl_universal_plane.c    | 51 +++++++++++++++++++
 include/drm/drm_mode_config.h                 |  6 +++
 include/drm/drm_plane.h                       |  4 ++
 5 files changed, 103 insertions(+), 15 deletions(-)

--
2.25.1

---
Arun R Murthy (5):
      drm/plane: Add new plane property IN_FORMATS_ASYNC
      drm/plane: Expose function to create format/modifier blob
      drm/plane: Function to check async supported modifier/format
      drm/i915/display: Populate list of async supported formats/modifiers
      drm/i915/display: Add function for format_mod_supported_async

 drivers/gpu/drm/drm_mode_config.c                  |   7 ++
 drivers/gpu/drm/drm_plane.c                        |  72 +++++++++----
 drivers/gpu/drm/i915/display/skl_universal_plane.c | 113 ++++++++++++++++++---
 include/drm/drm_mode_config.h                      |   6 ++
 include/drm/drm_plane.h                            |  24 +++++
 5 files changed, 188 insertions(+), 34 deletions(-)
---
base-commit: 08bd590935a5258ffd79355c59adffd72fb2c642
change-id: 20250102-asyn-bf76730501cc

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

[PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Arun R Murthy]

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.
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@intel.com>
---
 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..dbbef20753f834a85ae9ded748cff2b3f0e85043 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..416818e54ccffcf3d3aada2723e96ce8ccf1dd97 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.
+ *
  * 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..0c116d6dfd277262b1a4c0f097fce2d719f43844 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..e8749e6fc3bc0acfc73bbd8401f85c3126e86759 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.
+	 *
+	 * 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);
+
 };
 
 /**

-- 
2.25.1

[PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Arun R Murthy]

Expose drm plane function to create formats/modifiers blob. This
function can be used to expose list of supported formats/modifiers for
sync/async flips.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++++++++++---------------
 include/drm/drm_plane.h     |  4 ++++
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a9068b31c0563a4c0 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
 	return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
 }
 
-static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
+int drm_plane_create_format_blob(struct drm_device *dev,
+				 struct drm_plane *plane, u64 *modifiers,
+				 unsigned int modifier_count, u32 *formats,
+				 unsigned int format_count, bool is_async)
 {
 	const struct drm_mode_config *config = &dev->mode_config;
 	struct drm_property_blob *blob;
@@ -200,14 +203,14 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 	struct drm_format_modifier_blob *blob_data;
 	unsigned int i, j;
 
-	formats_size = sizeof(__u32) * plane->format_count;
+	formats_size = sizeof(__u32) * format_count;
 	if (WARN_ON(!formats_size)) {
 		/* 0 formats are never expected */
 		return 0;
 	}
 
 	modifiers_size =
-		sizeof(struct drm_format_modifier) * plane->modifier_count;
+		sizeof(struct drm_format_modifier) * modifier_count;
 
 	blob_size = sizeof(struct drm_format_modifier_blob);
 	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
@@ -223,37 +226,45 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 
 	blob_data = blob->data;
 	blob_data->version = FORMAT_BLOB_CURRENT;
-	blob_data->count_formats = plane->format_count;
+	blob_data->count_formats = format_count;
 	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
-	blob_data->count_modifiers = plane->modifier_count;
+	blob_data->count_modifiers = modifier_count;
 
 	blob_data->modifiers_offset =
 		ALIGN(blob_data->formats_offset + formats_size, 8);
 
-	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
+	memcpy(formats_ptr(blob_data), formats, formats_size);
 
 	mod = modifiers_ptr(blob_data);
-	for (i = 0; i < plane->modifier_count; i++) {
-		for (j = 0; j < plane->format_count; j++) {
-			if (!plane->funcs->format_mod_supported ||
+	for (i = 0; i < modifier_count; i++) {
+		for (j = 0; j < format_count; j++) {
+			if (is_async ||
+			    !plane->funcs->format_mod_supported ||
 			    plane->funcs->format_mod_supported(plane,
-							       plane->format_types[j],
-							       plane->modifiers[i])) {
+							       formats[j],
+							       modifiers[i])) {
 				mod->formats |= 1ULL << j;
 			}
 		}
 
-		mod->modifier = plane->modifiers[i];
+		mod->modifier = modifiers[i];
 		mod->offset = 0;
 		mod->pad = 0;
 		mod++;
 	}
 
-	drm_object_attach_property(&plane->base, config->modifiers_property,
-				   blob->base.id);
+	if (is_async)
+		drm_object_attach_property(&plane->base,
+					   config->async_modifiers_property,
+					   blob->base.id);
+	else
+		drm_object_attach_property(&plane->base,
+					   config->modifiers_property,
+					   blob->base.id);
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_plane_create_format_blob);
 
 /**
  * DOC: hotspot properties
@@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 	}
 
 	if (format_modifier_count)
-		create_in_format_blob(dev, plane);
+		drm_plane_create_format_blob(dev, plane, plane->modifiers,
+					     format_modifier_count,
+					     plane->format_types, format_count,
+					     false);
 
 	return 0;
 }
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf369894a5657cd45 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -1008,5 +1008,9 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
 int drm_plane_add_size_hints_property(struct drm_plane *plane,
 				      const struct drm_plane_size_hint *hints,
 				      int num_hints);
+int drm_plane_create_format_blob(struct drm_device *dev,
+				 struct drm_plane *plane, u64 *modifiers,
+				 unsigned int modifier_count, u32 *formats,
+				 unsigned int format_count, bool is_async);
 
 #endif

-- 
2.25.1

[PATCH v3 3/5] drm/plane: Function to check async supported modifier/format

[Arun R Murthy]

Seperate function for async flips is to be called in order to check the
provided format/modifier support.
At present the flag for async flip is stored in crtc_state as async flip
is supported on only one plane for a given crtc. The same is being
used over here to decide the async function pointer.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_plane.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 4f35eec2b7770fcc90c3e07a9068b31c0563a4c0..9e08ba4318cf0c07fa0701023659986855e0e98a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -238,12 +238,21 @@ int drm_plane_create_format_blob(struct drm_device *dev,
 	mod = modifiers_ptr(blob_data);
 	for (i = 0; i < modifier_count; i++) {
 		for (j = 0; j < format_count; j++) {
-			if (is_async ||
-			    !plane->funcs->format_mod_supported ||
-			    plane->funcs->format_mod_supported(plane,
-							       formats[j],
-							       modifiers[i])) {
-				mod->formats |= 1ULL << j;
+			if (is_async) {
+				if (!plane->funcs->format_mod_supported_async ||
+				    plane->funcs->format_mod_supported_async(plane,
+									     formats[j],
+									     modifiers[i])) {
+					mod->formats |= 1ULL << j;
+				}
+
+			} else {
+				if (!plane->funcs->format_mod_supported ||
+				    plane->funcs->format_mod_supported(plane,
+								       formats[j],
+								       modifiers[i])) {
+					mod->formats |= 1ULL << j;
+				}
 			}
 		}
 
@@ -910,6 +919,7 @@ bool drm_plane_has_format(struct drm_plane *plane,
 			  u32 format, u64 modifier)
 {
 	unsigned int i;
+	bool is_async = plane->crtc->state->async_flip;
 
 	for (i = 0; i < plane->format_count; i++) {
 		if (format == plane->format_types[i])
@@ -918,8 +928,12 @@ bool drm_plane_has_format(struct drm_plane *plane,
 	if (i == plane->format_count)
 		return false;
 
-	if (plane->funcs->format_mod_supported) {
-		if (!plane->funcs->format_mod_supported(plane, format, modifier))
+	if (is_async ? plane->funcs->format_mod_supported_async :
+			plane->funcs->format_mod_supported) {
+		if (!(is_async ? plane->funcs->format_mod_supported_async(
+						plane, format, modifier) :
+					plane->funcs->format_mod_supported(
+						plane, format, modifier)))
 			return false;
 	} else {
 		if (!plane->modifier_count)

-- 
2.25.1

[PATCH v3 4/5] drm/i915/display: Populate list of async supported formats/modifiers

[Arun R Murthy]

Populate the list of formats/modifiers supported by async flip. Register
a async property and expose the same to user through blob.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/skl_universal_plane.c | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index ff9764cac1e71959e56283f61b5192ea261cec7a..e5e47f2219dae62e76cbde2efb40266b047ab2b2 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -170,6 +170,44 @@ static const u32 icl_hdr_plane_formats[] = {
 	DRM_FORMAT_XVYU16161616,
 };
 
+static u64 tgl_asyn_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	I915_FORMAT_MOD_X_TILED,
+	I915_FORMAT_MOD_Y_TILED,
+	I915_FORMAT_MOD_4_TILED,
+	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
+	I915_FORMAT_MOD_4_TILED_MTL_RC_CCS,
+	I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
+	I915_FORMAT_MOD_4_TILED_BMG_CCS,
+	I915_FORMAT_MOD_4_TILED_LNL_CCS,
+};
+
+static u64 icl_async_modifiers[] = {
+	I915_FORMAT_MOD_X_TILED,
+	I915_FORMAT_MOD_Y_TILED,
+	I915_FORMAT_MOD_Yf_TILED,
+	I915_FORMAT_MOD_Y_TILED_CCS,
+	I915_FORMAT_MOD_Yf_TILED_CCS,
+};
+
+static u64 skl_async_modifiers[] = {
+	I915_FORMAT_MOD_X_TILED,
+	I915_FORMAT_MOD_Y_TILED,
+	I915_FORMAT_MOD_Yf_TILED,
+};
+
+static u32 intel_async_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_XRGB16161616F,
+	DRM_FORMAT_XBGR16161616F,
+};
+
 int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
 {
 	switch (format) {
@@ -2613,6 +2651,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	unsigned int supported_rotations;
 	unsigned int supported_csc;
 	const u64 *modifiers;
+	u64 *async_modifiers;
 	const u32 *formats;
 	int num_formats;
 	int ret;
@@ -2715,6 +2754,18 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	if (ret)
 		goto fail;
 
+	if (DISPLAY_VER(dev_priv) >= 12)
+		async_modifiers = tgl_asyn_modifiers;
+	else if (DISPLAY_VER(dev_priv) == 11)
+		async_modifiers = icl_async_modifiers;
+	else
+		async_modifiers = skl_async_modifiers;
+
+	drm_plane_create_format_blob(&dev_priv->drm, &plane->base,
+				     async_modifiers, sizeof(async_modifiers),
+				     intel_async_formats,
+				     sizeof(intel_async_formats), true);
+
 	if (DISPLAY_VER(dev_priv) >= 13)
 		supported_rotations = DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180;
 	else

-- 
2.25.1

[PATCH v3 5/5] drm/i915/display: Add function for format_mod_supported_async

[Arun R Murthy]

Add driver specific function definition for the plane->funcs
format_mod_supported_async to check if the provided format/modifier is
supported for asynchronous flip.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/skl_universal_plane.c | 62 ++++++++++++++++------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index e5e47f2219dae62e76cbde2efb40266b047ab2b2..00aa254a3b4e992268c9159bc15687e54718dc43 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -2526,30 +2526,62 @@ static bool tgl_plane_format_mod_supported(struct drm_plane *_plane,
 	}
 }
 
+static bool intel_plane_format_mod_supported_async(struct drm_plane *_plane,
+						   u32 format, u64 modifier)
+{
+	struct intel_plane *plane = to_intel_plane(_plane);
+	struct intel_display *display = to_intel_display(plane);
+	int i, found = false;
+	u64 *async_modifiers;
+
+	if (plane->id != 1)
+		return false;
+
+	if (DISPLAY_VER(display) >= 12)
+		async_modifiers = tgl_asyn_modifiers;
+	else if (DISPLAY_VER(display) == 11)
+		async_modifiers = icl_async_modifiers;
+	else
+		async_modifiers = skl_async_modifiers;
+
+	for (i = 0; i < sizeof(async_modifiers); i++) {
+		if (modifier == async_modifiers[i])
+			found = true;
+	}
+	if (!found)
+		return false;
+
+	/* Async flip supported only on RGB formats */
+	for (i = 0; i < sizeof(intel_async_formats); i++) {
+		if (format == intel_async_formats[i])
+			return true;
+	}
+	return false;
+}
+
+#define INTEL_PLANE_FUNCS \
+	.update_plane = drm_atomic_helper_update_plane, \
+	.disable_plane = drm_atomic_helper_disable_plane, \
+	.destroy = intel_plane_destroy, \
+	.atomic_duplicate_state = intel_plane_duplicate_state, \
+	.atomic_destroy_state = intel_plane_destroy_state, \
+	.format_mod_supported_async = intel_plane_format_mod_supported_async
+
 static const struct drm_plane_funcs skl_plane_funcs = {
-	.update_plane = drm_atomic_helper_update_plane,
-	.disable_plane = drm_atomic_helper_disable_plane,
-	.destroy = intel_plane_destroy,
-	.atomic_duplicate_state = intel_plane_duplicate_state,
-	.atomic_destroy_state = intel_plane_destroy_state,
+	INTEL_PLANE_FUNCS,
+
 	.format_mod_supported = skl_plane_format_mod_supported,
 };
 
 static const struct drm_plane_funcs icl_plane_funcs = {
-	.update_plane = drm_atomic_helper_update_plane,
-	.disable_plane = drm_atomic_helper_disable_plane,
-	.destroy = intel_plane_destroy,
-	.atomic_duplicate_state = intel_plane_duplicate_state,
-	.atomic_destroy_state = intel_plane_destroy_state,
+	INTEL_PLANE_FUNCS,
+
 	.format_mod_supported = icl_plane_format_mod_supported,
 };
 
 static const struct drm_plane_funcs tgl_plane_funcs = {
-	.update_plane = drm_atomic_helper_update_plane,
-	.disable_plane = drm_atomic_helper_disable_plane,
-	.destroy = intel_plane_destroy,
-	.atomic_duplicate_state = intel_plane_duplicate_state,
-	.atomic_destroy_state = intel_plane_destroy_state,
+	INTEL_PLANE_FUNCS,
+
 	.format_mod_supported = tgl_plane_format_mod_supported,
 };
 

-- 
2.25.1

✗ i915.CI.BAT: failure for Expose modifiers/formats supported by async flips (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 14416 bytes --]

== Series Details ==

Series: Expose modifiers/formats supported by async flips (rev2)
URL   : https://patchwork.freedesktop.org/series/140935/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15920 -> Patchwork_140935v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_140935v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_140935v2, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/index.html

Participating hosts (38 -> 37)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_140935v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-ilk-650:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-ilk-650/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-ilk-650/igt@i915_module_load@load.html
    - bat-jsl-1:          [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-jsl-1/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-jsl-1/igt@i915_module_load@load.html
    - fi-blb-e6850:       [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-blb-e6850/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-blb-e6850/igt@i915_module_load@load.html
    - bat-arlh-2:         [PASS][7] -> [DMESG-WARN][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-arlh-2/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-arlh-2/igt@i915_module_load@load.html
    - fi-rkl-11600:       [PASS][9] -> [ABORT][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-rkl-11600/igt@i915_module_load@load.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-rkl-11600/igt@i915_module_load@load.html
    - bat-arlh-3:         [PASS][11] -> [ABORT][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-arlh-3/igt@i915_module_load@load.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-arlh-3/igt@i915_module_load@load.html
    - fi-pnv-d510:        [PASS][13] -> [ABORT][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-pnv-d510/igt@i915_module_load@load.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-pnv-d510/igt@i915_module_load@load.html
    - bat-dg1-7:          [PASS][15] -> [ABORT][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg1-7/igt@i915_module_load@load.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg1-7/igt@i915_module_load@load.html
    - bat-dg2-13:         [PASS][17] -> [ABORT][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg2-13/igt@i915_module_load@load.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg2-13/igt@i915_module_load@load.html
    - bat-jsl-3:          [PASS][19] -> [ABORT][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-jsl-3/igt@i915_module_load@load.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-jsl-3/igt@i915_module_load@load.html
    - bat-adlp-9:         [PASS][21] -> [ABORT][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-adlp-9/igt@i915_module_load@load.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-adlp-9/igt@i915_module_load@load.html
    - bat-twl-2:          [PASS][23] -> [ABORT][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-twl-2/igt@i915_module_load@load.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-twl-2/igt@i915_module_load@load.html
    - bat-dg2-11:         [PASS][25] -> [ABORT][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg2-11/igt@i915_module_load@load.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg2-11/igt@i915_module_load@load.html
    - bat-rpls-4:         [PASS][27] -> [ABORT][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-rpls-4/igt@i915_module_load@load.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-rpls-4/igt@i915_module_load@load.html
    - fi-kbl-7567u:       [PASS][29] -> [ABORT][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-7567u/igt@i915_module_load@load.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-7567u/igt@i915_module_load@load.html
    - fi-cfl-8700k:       [PASS][31] -> [ABORT][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-cfl-8700k/igt@i915_module_load@load.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-cfl-8700k/igt@i915_module_load@load.html
    - fi-kbl-8809g:       [PASS][33] -> [DMESG-WARN][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-8809g/igt@i915_module_load@load.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-8809g/igt@i915_module_load@load.html
    - bat-dg2-14:         [PASS][35] -> [ABORT][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg2-14/igt@i915_module_load@load.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg2-14/igt@i915_module_load@load.html
    - fi-elk-e7500:       [PASS][37] -> [ABORT][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-elk-e7500/igt@i915_module_load@load.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-elk-e7500/igt@i915_module_load@load.html
    - bat-kbl-2:          [PASS][39] -> [DMESG-WARN][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-kbl-2/igt@i915_module_load@load.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-kbl-2/igt@i915_module_load@load.html
    - bat-arls-5:         [PASS][41] -> [ABORT][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-arls-5/igt@i915_module_load@load.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-arls-5/igt@i915_module_load@load.html
    - bat-rplp-1:         [PASS][43] -> [ABORT][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-rplp-1/igt@i915_module_load@load.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-rplp-1/igt@i915_module_load@load.html
    - fi-tgl-1115g4:      [PASS][45] -> [ABORT][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-tgl-1115g4/igt@i915_module_load@load.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-tgl-1115g4/igt@i915_module_load@load.html
    - fi-cfl-guc:         [PASS][47] -> [ABORT][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-cfl-guc/igt@i915_module_load@load.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-cfl-guc/igt@i915_module_load@load.html
    - bat-mtlp-6:         [PASS][49] -> [DMESG-WARN][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-mtlp-6/igt@i915_module_load@load.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-mtlp-6/igt@i915_module_load@load.html
    - fi-kbl-x1275:       [PASS][51] -> [DMESG-WARN][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-x1275/igt@i915_module_load@load.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-x1275/igt@i915_module_load@load.html
    - bat-adlp-11:        [PASS][53] -> [DMESG-WARN][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-adlp-11/igt@i915_module_load@load.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-adlp-11/igt@i915_module_load@load.html
    - fi-hsw-4770:        [PASS][55] -> [ABORT][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-hsw-4770/igt@i915_module_load@load.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-hsw-4770/igt@i915_module_load@load.html
    - fi-ivb-3770:        [PASS][57] -> [ABORT][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-ivb-3770/igt@i915_module_load@load.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-ivb-3770/igt@i915_module_load@load.html
    - bat-mtlp-8:         [PASS][59] -> [ABORT][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-mtlp-8/igt@i915_module_load@load.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-mtlp-8/igt@i915_module_load@load.html
    - bat-dg1-6:          [PASS][61] -> [DMESG-WARN][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg1-6/igt@i915_module_load@load.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg1-6/igt@i915_module_load@load.html
    - bat-dg2-8:          [PASS][63] -> [ABORT][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg2-8/igt@i915_module_load@load.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg2-8/igt@i915_module_load@load.html
    - fi-kbl-guc:         [PASS][65] -> [DMESG-WARN][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-guc/igt@i915_module_load@load.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-guc/igt@i915_module_load@load.html
    - bat-adls-6:         [PASS][67] -> [ABORT][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-adls-6/igt@i915_module_load@load.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-adls-6/igt@i915_module_load@load.html

  * igt@kms_addfb_basic@addfb25-4-tiled:
    - fi-kbl-x1275:       [PASS][69] -> [ABORT][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-x1275/igt@kms_addfb_basic@addfb25-4-tiled.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-x1275/igt@kms_addfb_basic@addfb25-4-tiled.html
    - fi-kbl-8809g:       [PASS][71] -> [ABORT][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-8809g/igt@kms_addfb_basic@addfb25-4-tiled.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-8809g/igt@kms_addfb_basic@addfb25-4-tiled.html
    - fi-kbl-guc:         [PASS][73] -> [ABORT][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-kbl-guc/igt@kms_addfb_basic@addfb25-4-tiled.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-kbl-guc/igt@kms_addfb_basic@addfb25-4-tiled.html
    - bat-mtlp-6:         [PASS][75] -> [ABORT][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-mtlp-6/igt@kms_addfb_basic@addfb25-4-tiled.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-mtlp-6/igt@kms_addfb_basic@addfb25-4-tiled.html
    - bat-adlp-11:        [PASS][77] -> [ABORT][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-adlp-11/igt@kms_addfb_basic@addfb25-4-tiled.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-adlp-11/igt@kms_addfb_basic@addfb25-4-tiled.html
    - bat-dg1-6:          [PASS][79] -> [ABORT][80]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-dg1-6/igt@kms_addfb_basic@addfb25-4-tiled.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-dg1-6/igt@kms_addfb_basic@addfb25-4-tiled.html
    - bat-kbl-2:          [PASS][81] -> [ABORT][82]
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-kbl-2/igt@kms_addfb_basic@addfb25-4-tiled.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-kbl-2/igt@kms_addfb_basic@addfb25-4-tiled.html
    - bat-arlh-2:         [PASS][83] -> [ABORT][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-arlh-2/igt@kms_addfb_basic@addfb25-4-tiled.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-arlh-2/igt@kms_addfb_basic@addfb25-4-tiled.html

  
#### Warnings ####

  * igt@i915_module_load@load:
    - fi-cfl-8109u:       [DMESG-WARN][85] ([i915#11621]) -> [ABORT][86]
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/fi-cfl-8109u/igt@i915_module_load@load.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/fi-cfl-8109u/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-mtlp-9}:       [PASS][87] -> [ABORT][88]
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-mtlp-9/igt@i915_module_load@load.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-mtlp-9/igt@i915_module_load@load.html
    - {bat-arls-6}:       [PASS][89] -> [ABORT][90]
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15920/bat-arls-6/igt@i915_module_load@load.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/bat-arls-6/igt@i915_module_load@load.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#11621]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11621


Build changes
-------------

  * Linux: CI_DRM_15920 -> Patchwork_140935v2

  CI-20190529: 20190529
  CI_DRM_15920: 8f14ac27d7b0ff9ff973fa44a67cc9a0ff3acad5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8179: 183b33f81365dd4a57fe3100a13d3fb13788d158 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_140935v2: 8f14ac27d7b0ff9ff973fa44a67cc9a0ff3acad5 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140935v2/index.html

[-- Attachment #2: Type: text/html, Size: 15299 bytes --]

RE: [PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Arun R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> 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@intel.com>
> ---
>  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

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier
> blob
> 
> Expose drm plane function to create formats/modifiers blob. This function
> can be used to expose list of supported formats/modifiers for sync/async
> flips.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++++++++++--------
> -------
>  include/drm/drm_plane.h     |  4 ++++
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> 68b31c0563a4c0 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> *blob)
>  	return (struct drm_format_modifier *)(((char *)blob) + blob-
> >modifiers_offset);  }
> 
> -static int create_in_format_blob(struct drm_device *dev, struct drm_plane
> *plane)
> +int drm_plane_create_format_blob(struct drm_device *dev,
> +				 struct drm_plane *plane, u64 *modifiers,
> +				 unsigned int modifier_count, u32 *formats,
> +				 unsigned int format_count, bool is_async)

We can retain the original arguments (dev, plane) here. As I understand, plane->formats[] and plane->modifiers[] are populated with all the formats and modifiers supported by the platform, respectively.

The goal is to establish a mapping between the two using the callbacks format_mod_supported() or format_mod_supported_async().
Since these arrays represent a superset of all the formats and modifiers the platform supports, we have sufficient information within drm_plane to decide how to pair them here.

>  {
>  	const struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_property_blob *blob;
> @@ -200,14 +203,14 @@ static int create_in_format_blob(struct drm_device
> *dev, struct drm_plane *plane
>  	struct drm_format_modifier_blob *blob_data;
>  	unsigned int i, j;
> 
> -	formats_size = sizeof(__u32) * plane->format_count;
> +	formats_size = sizeof(__u32) * format_count;
>  	if (WARN_ON(!formats_size)) {
>  		/* 0 formats are never expected */
>  		return 0;
>  	}
> 
>  	modifiers_size =
> -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> +		sizeof(struct drm_format_modifier) * modifier_count;
> 
>  	blob_size = sizeof(struct drm_format_modifier_blob);
>  	/* Modifiers offset is a pointer to a struct with a 64 bit field so it @@ -
> 223,37 +226,45 @@ static int create_in_format_blob(struct drm_device *dev,
> struct drm_plane *plane
> 
>  	blob_data = blob->data;
>  	blob_data->version = FORMAT_BLOB_CURRENT;
> -	blob_data->count_formats = plane->format_count;
> +	blob_data->count_formats = format_count;
>  	blob_data->formats_offset = sizeof(struct
> drm_format_modifier_blob);
> -	blob_data->count_modifiers = plane->modifier_count;
> +	blob_data->count_modifiers = modifier_count;
> 
>  	blob_data->modifiers_offset =
>  		ALIGN(blob_data->formats_offset + formats_size, 8);
> 
> -	memcpy(formats_ptr(blob_data), plane->format_types,
> formats_size);
> +	memcpy(formats_ptr(blob_data), formats, formats_size);
> 
>  	mod = modifiers_ptr(blob_data);
> -	for (i = 0; i < plane->modifier_count; i++) {
> -		for (j = 0; j < plane->format_count; j++) {
> -			if (!plane->funcs->format_mod_supported ||
> +	for (i = 0; i < modifier_count; i++) {
> +		for (j = 0; j < format_count; j++) {
> +			if (is_async ||

This patch looks incomplete because the implementation for async is split between this and [1]. It might be a good idea to have both the patch squashed.


[1] https://patchwork.freedesktop.org/patch/631261/?series=140935&rev=2

Regards

Chaitanya

> +			    !plane->funcs->format_mod_supported ||
>  			    plane->funcs->format_mod_supported(plane,
> -							       plane-
> >format_types[j],
> -							       plane-
> >modifiers[i])) {
> +							       formats[j],
> +							       modifiers[i])) {
>  				mod->formats |= 1ULL << j;
>  			}
>  		}
> 
> -		mod->modifier = plane->modifiers[i];
> +		mod->modifier = modifiers[i];
>  		mod->offset = 0;
>  		mod->pad = 0;
>  		mod++;
>  	}
> 
> -	drm_object_attach_property(&plane->base, config-
> >modifiers_property,
> -				   blob->base.id);
> +	if (is_async)
> +		drm_object_attach_property(&plane->base,
> +					   config->async_modifiers_property,
> +					   blob->base.id);
> +	else
> +		drm_object_attach_property(&plane->base,
> +					   config->modifiers_property,
> +					   blob->base.id);
> 
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_plane_create_format_blob);
> 
>  /**
>   * DOC: hotspot properties
> @@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct
> drm_device *dev,
>  	}
> 
>  	if (format_modifier_count)
> -		create_in_format_blob(dev, plane);
> +		drm_plane_create_format_blob(dev, plane, plane->modifiers,
> +					     format_modifier_count,
> +					     plane->format_types,
> format_count,
> +					     false);
> 
>  	return 0;
>  }
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> 369894a5657cd45 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -1008,5 +1008,9 @@ int
> drm_plane_create_scaling_filter_property(struct drm_plane *plane,  int
> drm_plane_add_size_hints_property(struct drm_plane *plane,
>  				      const struct drm_plane_size_hint *hints,
>  				      int num_hints);
> +int drm_plane_create_format_blob(struct drm_device *dev,
> +				 struct drm_plane *plane, u64 *modifiers,
> +				 unsigned int modifier_count, u32 *formats,
> +				 unsigned int format_count, bool is_async);
> 
>  #endif
> 
> --
> 2.25.1

RE: [PATCH v3 3/5] drm/plane: Function to check async supported modifier/format

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 3/5] drm/plane: Function to check async supported
> modifier/format
> 
> Seperate function for async flips is to be called in order to check the provided
> format/modifier support.
> At present the flag for async flip is stored in crtc_state as async flip is
> supported on only one plane for a given crtc. The same is being used over
> here to decide the async function pointer.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index
> 4f35eec2b7770fcc90c3e07a9068b31c0563a4c0..9e08ba4318cf0c07fa0701023
> 659986855e0e98a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -238,12 +238,21 @@ int drm_plane_create_format_blob(struct
> drm_device *dev,
>  	mod = modifiers_ptr(blob_data);
>  	for (i = 0; i < modifier_count; i++) {
>  		for (j = 0; j < format_count; j++) {
> -			if (is_async ||
> -			    !plane->funcs->format_mod_supported ||
> -			    plane->funcs->format_mod_supported(plane,
> -							       formats[j],
> -							       modifiers[i])) {
> -				mod->formats |= 1ULL << j;
> +			if (is_async) {
> +				if (!plane->funcs-
> >format_mod_supported_async ||
> +				    plane->funcs-
> >format_mod_supported_async(plane,
> +
> formats[j],
> +
> modifiers[i])) {
> +					mod->formats |= 1ULL << j;
> +				}
> +
> +			} else {
> +				if (!plane->funcs->format_mod_supported ||
> +				    plane->funcs-
> >format_mod_supported(plane,
> +								       formats[j],
> +
> modifiers[i])) {
> +					mod->formats |= 1ULL << j;
> +				}
>  			}
>  		}
> 
> @@ -910,6 +919,7 @@ bool drm_plane_has_format(struct drm_plane
> *plane,
>  			  u32 format, u64 modifier)
>  {
>  	unsigned int i;
> +	bool is_async = plane->crtc->state->async_flip;
> 
>  	for (i = 0; i < plane->format_count; i++) {
>  		if (format == plane->format_types[i]) @@ -918,8 +928,12 @@
> bool drm_plane_has_format(struct drm_plane *plane,
>  	if (i == plane->format_count)
>  		return false;
> 
> -	if (plane->funcs->format_mod_supported) {
> -		if (!plane->funcs->format_mod_supported(plane, format,
> modifier))
> +	if (is_async ? plane->funcs->format_mod_supported_async :
> +			plane->funcs->format_mod_supported) {
> +		if (!(is_async ? plane->funcs->format_mod_supported_async(
> +						plane, format, modifier) :
> +					plane->funcs-
> >format_mod_supported(
> +						plane, format, modifier)))

Since we are bringing this check here. Should the check be removed from the i915 code?
We also need to account for the fact that some drivers will not have the format_mod_supported_async() implemented.

Regards

Chaitanya

>  			return false;
>  	} else {
>  		if (!plane->modifier_count)
> 
> --
> 2.25.1

RE: [PATCH v3 4/5] drm/i915/display: Populate list of async supported formats/modifiers

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 4/5] drm/i915/display: Populate list of async supported
> formats/modifiers
> 
> Populate the list of formats/modifiers supported by async flip. Register a
> async property and expose the same to user through blob.

s/a/an

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 51
> ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index
> ff9764cac1e71959e56283f61b5192ea261cec7a..e5e47f2219dae62e76cbde2e
> fb40266b047ab2b2 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -170,6 +170,44 @@ static const u32 icl_hdr_plane_formats[] = {
>  	DRM_FORMAT_XVYU16161616,
>  };
> 
> +static u64 tgl_asyn_modifiers[] = {

/s/asn/async

> +	DRM_FORMAT_MOD_LINEAR,
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_4_TILED,
> +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_MTL_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_BMG_CCS,
> +	I915_FORMAT_MOD_4_TILED_LNL_CCS,
> +};
> +
> +static u64 icl_async_modifiers[] = {
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_Yf_TILED,
> +	I915_FORMAT_MOD_Y_TILED_CCS,
> +	I915_FORMAT_MOD_Yf_TILED_CCS,
> +};
> +
> +static u64 skl_async_modifiers[] = {
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_Yf_TILED,
> +};
> +
> +static u32 intel_async_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_XRGB16161616F,
> +	DRM_FORMAT_XBGR16161616F,
> +};

There are platforms which also support packed YUV formats for async flip. We should consider them as well.
However, we need not maintain these arrays like this. See below and comments on [2]

> +
>  int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)  {
>  	switch (format) {
> @@ -2613,6 +2651,7 @@ skl_universal_plane_create(struct
> drm_i915_private *dev_priv,
>  	unsigned int supported_rotations;
>  	unsigned int supported_csc;
>  	const u64 *modifiers;
> +	u64 *async_modifiers;
>  	const u32 *formats;
>  	int num_formats;
>  	int ret;
> @@ -2715,6 +2754,18 @@ skl_universal_plane_create(struct
> drm_i915_private *dev_priv,
>  	if (ret)
>  		goto fail;
> 
> +	if (DISPLAY_VER(dev_priv) >= 12)
> +		async_modifiers = tgl_asyn_modifiers;
> +	else if (DISPLAY_VER(dev_priv) == 11)
> +		async_modifiers = icl_async_modifiers;
> +	else
> +		async_modifiers = skl_async_modifiers;
> +
> +	drm_plane_create_format_blob(&dev_priv->drm, &plane->base,
> +				     async_modifiers, sizeof(async_modifiers),
> +				     intel_async_formats,
> +				     sizeof(intel_async_formats), true);

As discussed in [1], we need not pass a filtered array to drm_plane_create_format_blob(). The function should filter out what format modifier pair is supported from the superset of all formats and modifiers which actually is being done in [2]

[1] https://patchwork.freedesktop.org/patch/631260/?series=140935&rev=2
[2] https://patchwork.freedesktop.org/patch/631267/?series=140935&rev=2

Regards

Chaitanya

> +
>  	if (DISPLAY_VER(dev_priv) >= 13)
>  		supported_rotations = DRM_MODE_ROTATE_0 |
> DRM_MODE_ROTATE_180;
>  	else
> 
> --
> 2.25.1

RE: [PATCH v3 5/5] drm/i915/display: Add function for format_mod_supported_async

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Arun R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 5/5] drm/i915/display: Add function for
> format_mod_supported_async
> 
> Add driver specific function definition for the plane->funcs
> format_mod_supported_async to check if the provided format/modifier is
> supported for asynchronous flip.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 62 ++++++++++++++++-
> -----
>  1 file changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index
> e5e47f2219dae62e76cbde2efb40266b047ab2b2..00aa254a3b4e992268c9159
> bc15687e54718dc43 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2526,30 +2526,62 @@ static bool
> tgl_plane_format_mod_supported(struct drm_plane *_plane,
>  	}
>  }
> 
> +static bool intel_plane_format_mod_supported_async(struct drm_plane
> *_plane,
> +						   u32 format, u64 modifier)
> +{
> +	struct intel_plane *plane = to_intel_plane(_plane);
> +	struct intel_display *display = to_intel_display(plane);
> +	int i, found = false;
> +	u64 *async_modifiers;
> +
> +	if (plane->id != 1)
> +		return false;
> +
> +	if (DISPLAY_VER(display) >= 12)
> +		async_modifiers = tgl_asyn_modifiers;
> +	else if (DISPLAY_VER(display) == 11)
> +		async_modifiers = icl_async_modifiers;
> +	else
> +		async_modifiers = skl_async_modifiers;
> +
> +	for (i = 0; i < sizeof(async_modifiers); i++) {

Array size check is incorrect.

> +		if (modifier == async_modifiers[i])
> +			found = true;
> +	}
> +	if (!found)
> +		return false;
> +
> +	/* Async flip supported only on RGB formats */
> +	for (i = 0; i < sizeof(intel_async_formats); i++) {

Array size check is incorrect.

> +		if (format == intel_async_formats[i])
> +			return true;
> +	}
> +	return false;
> +}
> +

It is better to implement this function in the switch case format that XXX_plane_format_mod_supported() functions are implemented.
Imagine a scenario where a modifier is supported for some formats but not others. This function won't be able to make the distinction.

Regards

Chaitanya

> +#define INTEL_PLANE_FUNCS \
> +	.update_plane = drm_atomic_helper_update_plane, \
> +	.disable_plane = drm_atomic_helper_disable_plane, \
> +	.destroy = intel_plane_destroy, \
> +	.atomic_duplicate_state = intel_plane_duplicate_state, \
> +	.atomic_destroy_state = intel_plane_destroy_state, \
> +	.format_mod_supported_async =
> intel_plane_format_mod_supported_async
> +
>  static const struct drm_plane_funcs skl_plane_funcs = {
> -	.update_plane = drm_atomic_helper_update_plane,
> -	.disable_plane = drm_atomic_helper_disable_plane,
> -	.destroy = intel_plane_destroy,
> -	.atomic_duplicate_state = intel_plane_duplicate_state,
> -	.atomic_destroy_state = intel_plane_destroy_state,
> +	INTEL_PLANE_FUNCS,
> +
>  	.format_mod_supported = skl_plane_format_mod_supported,  };
> 
>  static const struct drm_plane_funcs icl_plane_funcs = {
> -	.update_plane = drm_atomic_helper_update_plane,
> -	.disable_plane = drm_atomic_helper_disable_plane,
> -	.destroy = intel_plane_destroy,
> -	.atomic_duplicate_state = intel_plane_duplicate_state,
> -	.atomic_destroy_state = intel_plane_destroy_state,
> +	INTEL_PLANE_FUNCS,
> +
>  	.format_mod_supported = icl_plane_format_mod_supported,  };
> 
>  static const struct drm_plane_funcs tgl_plane_funcs = {
> -	.update_plane = drm_atomic_helper_update_plane,
> -	.disable_plane = drm_atomic_helper_disable_plane,
> -	.destroy = intel_plane_destroy,
> -	.atomic_duplicate_state = intel_plane_duplicate_state,
> -	.atomic_destroy_state = intel_plane_destroy_state,
> +	INTEL_PLANE_FUNCS,
> +
>  	.format_mod_supported = tgl_plane_format_mod_supported,  };
> 
> 
> --
> 2.25.1

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Murthy, Arun R]

> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Wednesday, January 8, 2025 11:09 AM
> > To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> > intel- xe@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > Subject: [PATCH v3 2/5] drm/plane: Expose function to create
> > format/modifier blob
> >
> > Expose drm plane function to create formats/modifiers blob. This
> > function can be used to expose list of supported formats/modifiers for
> > sync/async flips.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 44
> > +++++++++++++++++++++++++++++--------
> > -------
> >  include/drm/drm_plane.h     |  4 ++++
> >  2 files changed, 33 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index
> >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > 68b31c0563a4c0 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> > *blob)
> >  	return (struct drm_format_modifier *)(((char *)blob) + blob-
> > >modifiers_offset);  }
> >
> > -static int create_in_format_blob(struct drm_device *dev, struct
> > drm_plane
> > *plane)
> > +int drm_plane_create_format_blob(struct drm_device *dev,
> > +				 struct drm_plane *plane, u64 *modifiers,
> > +				 unsigned int modifier_count, u32 *formats,
> > +				 unsigned int format_count, bool is_async)
> 
> We can retain the original arguments (dev, plane) here. As I understand, plane-
> >formats[] and plane->modifiers[] are populated with all the formats and
> modifiers supported by the platform, respectively.
> 
> The goal is to establish a mapping between the two using the callbacks
> format_mod_supported() or format_mod_supported_async().
> Since these arrays represent a superset of all the formats and modifiers the
> platform supports, we have sufficient information within drm_plane to decide
> how to pair them here.
> 
Plane->format_types and plane->modifiers is the list of formats and modifiers supported by the plane.  (This is true for sync flips only.) For each modifier all the formats listed in plane->format_types may not be supported but all of the formats mentioned in plane->format_types is supported by sync flips. In order to get the list of formats supported for each modifier a bit mask is created by the func pointer format_mod_supported.
The element format_offset in struct drm_format_modifier_blob is plane->format_types. So this is list of formats supported by this plane and holds good for only sync flips and may not support async flips. We need to get the subset of formats for async from the superset of formats from sync flips. For this we need a separate list of formats supported by async flip.

Please let me know if my understanding is wrong!

So with this understand we need pass 2 separate list of formats/modifiers for sync and async hence making it a parameter to this function.

> >  {
> >  	const struct drm_mode_config *config = &dev->mode_config;
> >  	struct drm_property_blob *blob;
> > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct
> > drm_device *dev, struct drm_plane *plane
> >  	struct drm_format_modifier_blob *blob_data;
> >  	unsigned int i, j;
> >
> > -	formats_size = sizeof(__u32) * plane->format_count;
> > +	formats_size = sizeof(__u32) * format_count;
> >  	if (WARN_ON(!formats_size)) {
> >  		/* 0 formats are never expected */
> >  		return 0;
> >  	}
> >
> >  	modifiers_size =
> > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > +		sizeof(struct drm_format_modifier) * modifier_count;
> >
> >  	blob_size = sizeof(struct drm_format_modifier_blob);
> >  	/* Modifiers offset is a pointer to a struct with a 64 bit field so
> > it @@ -
> > 223,37 +226,45 @@ static int create_in_format_blob(struct drm_device
> > *dev, struct drm_plane *plane
> >
> >  	blob_data = blob->data;
> >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > -	blob_data->count_formats = plane->format_count;
> > +	blob_data->count_formats = format_count;
> >  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> > -	blob_data->count_modifiers = plane->modifier_count;
> > +	blob_data->count_modifiers = modifier_count;
> >
> >  	blob_data->modifiers_offset =
> >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> >
> > -	memcpy(formats_ptr(blob_data), plane->format_types,
> > formats_size);
> > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> >
> >  	mod = modifiers_ptr(blob_data);
> > -	for (i = 0; i < plane->modifier_count; i++) {
> > -		for (j = 0; j < plane->format_count; j++) {
> > -			if (!plane->funcs->format_mod_supported ||
> > +	for (i = 0; i < modifier_count; i++) {
> > +		for (j = 0; j < format_count; j++) {
> > +			if (is_async ||
> 
> This patch looks incomplete because the implementation for async is split
> between this and [1]. It might be a good idea to have both the patch squashed.
> 
> 
> [1] https://patchwork.freedesktop.org/patch/631261/?series=140935&rev=2
> 
In the 1st patch property is created and this 2nd patch trying to fetch the data to be exposed by the property created in 1st patch. I can squash both the patch in my next version if it makes sense to have both of these in a single patch.

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

RE: [PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Murthy, Arun R]

> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Wednesday, January 8, 2025 11:09 AM
> > To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> > intel- xe@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > 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@intel.com>
> > ---
> >  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
--------------------

RE: [PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@intel.com>
> Sent: Monday, January 13, 2025 1:52 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Subject: RE: [PATCH v3 1/5] drm/plane: Add new plane property
> IN_FORMATS_ASYNC
> 
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf
> > > Of Arun R Murthy
> > > Sent: Wednesday, January 8, 2025 11:09 AM
> > > To: dri-devel@lists.freedesktop.org;
> > > intel-gfx@lists.freedesktop.org;
> > > intel- xe@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > > 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@intel.com>
> > > ---
> > >  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
> --------------------

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@intel.com>
> Sent: Monday, January 13, 2025 1:52 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Subject: RE: [PATCH v3 2/5] drm/plane: Expose function to create
> format/modifier blob
> 
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Arun R Murthy
> > > Sent: Wednesday, January 8, 2025 11:09 AM
> > > To: dri-devel@lists.freedesktop.org;
> > > intel-gfx@lists.freedesktop.org;
> > > intel- xe@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > > Subject: [PATCH v3 2/5] drm/plane: Expose function to create
> > > format/modifier blob
> > >
> > > Expose drm plane function to create formats/modifiers blob. This
> > > function can be used to expose list of supported formats/modifiers
> > > for sync/async flips.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 44
> > > +++++++++++++++++++++++++++++--------
> > > -------
> > >  include/drm/drm_plane.h     |  4 ++++
> > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > b/drivers/gpu/drm/drm_plane.c index
> > >
> >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > > 68b31c0563a4c0 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> > > *blob)
> > >  	return (struct drm_format_modifier *)(((char *)blob) + blob-
> > > >modifiers_offset);  }
> > >
> > > -static int create_in_format_blob(struct drm_device *dev, struct
> > > drm_plane
> > > *plane)
> > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > +				 struct drm_plane *plane, u64 *modifiers,
> > > +				 unsigned int modifier_count, u32 *formats,
> > > +				 unsigned int format_count, bool is_async)
> >
> > We can retain the original arguments (dev, plane) here. As I
> > understand, plane-
> > >formats[] and plane->modifiers[] are populated with all the formats
> > >and
> > modifiers supported by the platform, respectively.
> >
> > The goal is to establish a mapping between the two using the callbacks
> > format_mod_supported() or format_mod_supported_async().
> > Since these arrays represent a superset of all the formats and
> > modifiers the platform supports, we have sufficient information within
> > drm_plane to decide how to pair them here.
> >
> Plane->format_types and plane->modifiers is the list of formats and modifiers
> supported by the plane.  (This is true for sync flips only.) For each modifier all
> the formats listed in plane->format_types may not be supported but all of the
> formats mentioned in plane->format_types is supported by sync flips. In
> order to get the list of formats supported for each modifier a bit mask is
> created by the func pointer format_mod_supported.
> The element format_offset in struct drm_format_modifier_blob is plane-
> >format_types. So this is list of formats supported by this plane and holds
> good for only sync flips and may not support async flips. We need to get the
> subset of formats for async from the superset of formats from sync flips. For
> this we need a separate list of formats supported by async flip.
> 
> Please let me know if my understanding is wrong!
> 
> So with this understand we need pass 2 separate list of formats/modifiers for
> sync and async hence making it a parameter to this function.

Your understanding is correct however from pure coding perspective it will still work even if we don't pass different parameters for async and sync.
I am making an assumption here (please correct me if I am wrong) that all the formats and modifiers supported for async are a subset of
all the formats and modifiers supported for sync flips.

In cases where none of the formats are supported for a modifier (or a format is not supported at all)
the bit field will end up with all "Zero"s

				if (!plane->funcs->format_mod_supported_async ||
				    plane->funcs->format_mod_supported_async(plane,
									     formats[j],
									     modifiers[i])) {
					mod->formats |= 1ULL << j;
				}

And if you look in the proposed mutter code[1] for example, it will lead to a NULL entry in the hash table for that particular format indicating that it is not supported.

However, I do see merits in your implementation because it will avoid creation of unnecessary entries both in the blob and the hash table in user space.

On the flip side, it creates more static arrays that we need to maintain for different platforms. (Though we can perhaps avoid it by some smart re-use of intel_modifiers[] which IRC you had implemented in a previous patch).

So as far exposing the arguments I leave it to you. You can weigh in both the options.

[1] https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4063/diffs?commit_id=c21a47175d7ec0f48414335cd2de010ae9670626#0806c68597f9e25c110c6597ae0c9e69d9c0c5a8_378_448

> 
> > >  {
> > >  	const struct drm_mode_config *config = &dev->mode_config;
> > >  	struct drm_property_blob *blob;
> > > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct
> > > drm_device *dev, struct drm_plane *plane
> > >  	struct drm_format_modifier_blob *blob_data;
> > >  	unsigned int i, j;
> > >
> > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > +	formats_size = sizeof(__u32) * format_count;
> > >  	if (WARN_ON(!formats_size)) {
> > >  		/* 0 formats are never expected */
> > >  		return 0;
> > >  	}
> > >
> > >  	modifiers_size =
> > > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > >
> > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > >  	/* Modifiers offset is a pointer to a struct with a 64 bit field
> > > so it @@ -
> > > 223,37 +226,45 @@ static int create_in_format_blob(struct drm_device
> > > *dev, struct drm_plane *plane
> > >
> > >  	blob_data = blob->data;
> > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > -	blob_data->count_formats = plane->format_count;
> > > +	blob_data->count_formats = format_count;
> > >  	blob_data->formats_offset = sizeof(struct
> drm_format_modifier_blob);
> > > -	blob_data->count_modifiers = plane->modifier_count;
> > > +	blob_data->count_modifiers = modifier_count;
> > >
> > >  	blob_data->modifiers_offset =
> > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > >
> > > -	memcpy(formats_ptr(blob_data), plane->format_types,
> > > formats_size);
> > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > >
> > >  	mod = modifiers_ptr(blob_data);
> > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > -		for (j = 0; j < plane->format_count; j++) {
> > > -			if (!plane->funcs->format_mod_supported ||
> > > +	for (i = 0; i < modifier_count; i++) {
> > > +		for (j = 0; j < format_count; j++) {
> > > +			if (is_async ||
> >
> > This patch looks incomplete because the implementation for async is
> > split between this and [1]. It might be a good idea to have both the patch
> squashed.
> >
> >
> > [1]
> > https://patchwork.freedesktop.org/patch/631261/?series=140935&rev=2
> >
> In the 1st patch property is created and this 2nd patch trying to fetch the data
> to be exposed by the property created in 1st patch. I can squash both the
> patch in my next version if it makes sense to have both of these in a single
> patch.
> 

If at all it should be made other way around. First fill up the data and then expose the property.
Also the patch has some remnants of the previous version which do not fit well with the current version.
For example the part,

+			if (is_async ||
+			    !plane->funcs->format_mod_supported ||

Making it one patch should be good enough.

Regards

Chaitanya

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

RE: [PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Murthy, Arun R]

> > > > -----Original Message-----
> > > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On
> > > > Behalf Of Arun R Murthy
> > > > Sent: Wednesday, January 8, 2025 11:09 AM
> > > > To: dri-devel@lists.freedesktop.org;
> > > > intel-gfx@lists.freedesktop.org;
> > > > intel- xe@lists.freedesktop.org
> > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > > > 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@intel.com>
> > > > ---
> > > >  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.
> 
As documented this an optional property with this property user will have the
feasibility to check for supported one and if not might end up with failure commit
due to unsupported formats.
Will reframe this to avoid confusion!

> > > >   * 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.
> 
Ok sure will be more specific.

> 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.
> 
That cannot be the case, as this is an optional and not that all the drivers will
have this property. This is a good to have to avoid failure due to unsupported
format/modifier.

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

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Murthy, Arun R]

> > > > 68b31c0563a4c0 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> > > > *blob)
> > > >  	return (struct drm_format_modifier *)(((char *)blob) + blob-
> > > > >modifiers_offset);  }
> > > >
> > > > -static int create_in_format_blob(struct drm_device *dev, struct
> > > > drm_plane
> > > > *plane)
> > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > +				 struct drm_plane *plane, u64 *modifiers,
> > > > +				 unsigned int modifier_count, u32 *formats,
> > > > +				 unsigned int format_count, bool is_async)
> > >
> > > We can retain the original arguments (dev, plane) here. As I
> > > understand, plane-
> > > >formats[] and plane->modifiers[] are populated with all the formats
> > > >and
> > > modifiers supported by the platform, respectively.
> > >
> > > The goal is to establish a mapping between the two using the
> > > callbacks
> > > format_mod_supported() or format_mod_supported_async().
> > > Since these arrays represent a superset of all the formats and
> > > modifiers the platform supports, we have sufficient information
> > > within drm_plane to decide how to pair them here.
> > >
> > Plane->format_types and plane->modifiers is the list of formats and
> > Plane->modifiers
> > supported by the plane.  (This is true for sync flips only.) For each
> > modifier all the formats listed in plane->format_types may not be
> > supported but all of the formats mentioned in plane->format_types is
> > supported by sync flips. In order to get the list of formats supported
> > for each modifier a bit mask is created by the func pointer
> format_mod_supported.
> > The element format_offset in struct drm_format_modifier_blob is plane-
> > >format_types. So this is list of formats supported by this plane and
> > >holds
> > good for only sync flips and may not support async flips. We need to
> > get the subset of formats for async from the superset of formats from
> > sync flips. For this we need a separate list of formats supported by async flip.
> >
> > Please let me know if my understanding is wrong!
> >
> > So with this understand we need pass 2 separate list of
> > formats/modifiers for sync and async hence making it a parameter to this
> function.
> 
> Your understanding is correct however from pure coding perspective it will still
> work even if we don't pass different parameters for async and sync.
> I am making an assumption here (please correct me if I am wrong) that all the
> formats and modifiers supported for async are a subset of all the formats and
> modifiers supported for sync flips.
> 
Yes this assumption is correct.

> In cases where none of the formats are supported for a modifier (or a format is
> not supported at all) the bit field will end up with all "Zero"s
> 
Yes, that’s right.

> 				if (!plane->funcs-
> >format_mod_supported_async ||
> 				    plane->funcs-
> >format_mod_supported_async(plane,
> 
> formats[j],
> 
> modifiers[i])) {
> 					mod->formats |= 1ULL << j;
> 				}
> 
> And if you look in the proposed mutter code[1] for example, it will lead to a
> NULL entry in the hash table for that particular format indicating that it is not
> supported.
> 
In the structure that is exposed to the user, struct drm_format_modifier_blob the 
element formats_offset holds the list of formats that hardware supports.
In order to get this list we will have to maintain the supported list between sync
and async separately.

> However, I do see merits in your implementation because it will avoid creation
> of unnecessary entries both in the blob and the hash table in user space.
> 
> On the flip side, it creates more static arrays that we need to maintain for
> different platforms. (Though we can perhaps avoid it by some smart re-use of
> intel_modifiers[] which IRC you had implemented in a previous patch).
> 
Yes somewhere in the driver we will have to have this statically either in a list
or in the format_mod_supported_async().

> So as far exposing the arguments I leave it to you. You can weigh in both the
> options.
> 
> [1] https://gitlab.gnome.org/GNOME/mutter/-
> /merge_requests/4063/diffs?commit_id=c21a47175d7ec0f48414335cd2de010
> ae9670626#0806c68597f9e25c110c6597ae0c9e69d9c0c5a8_378_448
> 
> >
> > > >  {
> > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > >  	struct drm_property_blob *blob;
> > > > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct
> > > > drm_device *dev, struct drm_plane *plane
> > > >  	struct drm_format_modifier_blob *blob_data;
> > > >  	unsigned int i, j;
> > > >
> > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > +	formats_size = sizeof(__u32) * format_count;
> > > >  	if (WARN_ON(!formats_size)) {
> > > >  		/* 0 formats are never expected */
> > > >  		return 0;
> > > >  	}
> > > >
> > > >  	modifiers_size =
> > > > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > >
> > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > >  	/* Modifiers offset is a pointer to a struct with a 64 bit field
> > > > so it @@ -
> > > > 223,37 +226,45 @@ static int create_in_format_blob(struct
> > > > drm_device *dev, struct drm_plane *plane
> > > >
> > > >  	blob_data = blob->data;
> > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > -	blob_data->count_formats = plane->format_count;
> > > > +	blob_data->count_formats = format_count;
> > > >  	blob_data->formats_offset = sizeof(struct
> > drm_format_modifier_blob);
> > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > +	blob_data->count_modifiers = modifier_count;
> > > >
> > > >  	blob_data->modifiers_offset =
> > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > >
> > > > -	memcpy(formats_ptr(blob_data), plane->format_types,
> > > > formats_size);
> > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > >
> > > >  	mod = modifiers_ptr(blob_data);
> > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > -			if (!plane->funcs->format_mod_supported ||
> > > > +	for (i = 0; i < modifier_count; i++) {
> > > > +		for (j = 0; j < format_count; j++) {
> > > > +			if (is_async ||
> > >
> > > This patch looks incomplete because the implementation for async is
> > > split between this and [1]. It might be a good idea to have both the
> > > patch
> > squashed.
> > >
> > >
> > > [1]
> > > https://patchwork.freedesktop.org/patch/631261/?series=140935&rev=2
> > >
> > In the 1st patch property is created and this 2nd patch trying to
> > fetch the data to be exposed by the property created in 1st patch. I
> > can squash both the patch in my next version if it makes sense to have
> > both of these in a single patch.
> >
> 
> If at all it should be made other way around. First fill up the data and then
> expose the property.
> Also the patch has some remnants of the previous version which do not fit well
> with the current version.
> For example the part,
> 
> +			if (is_async ||
> +			    !plane->funcs->format_mod_supported ||
> 
> Making it one patch should be good enough.
Ok, Done!

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

Re: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Ville Syrjälä]

On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> Expose drm plane function to create formats/modifiers blob. This
> function can be used to expose list of supported formats/modifiers for
> sync/async flips.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++++++++++---------------
>  include/drm/drm_plane.h     |  4 ++++
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a9068b31c0563a4c0 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
>  	return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
>  }
>  
> -static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> +int drm_plane_create_format_blob(struct drm_device *dev,
> +				 struct drm_plane *plane, u64 *modifiers,
> +				 unsigned int modifier_count, u32 *formats,
> +				 unsigned int format_count, bool is_async)
>  {
>  	const struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_property_blob *blob;
> @@ -200,14 +203,14 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
>  	struct drm_format_modifier_blob *blob_data;
>  	unsigned int i, j;
>  
> -	formats_size = sizeof(__u32) * plane->format_count;
> +	formats_size = sizeof(__u32) * format_count;
>  	if (WARN_ON(!formats_size)) {
>  		/* 0 formats are never expected */
>  		return 0;
>  	}
>  
>  	modifiers_size =
> -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> +		sizeof(struct drm_format_modifier) * modifier_count;
>  
>  	blob_size = sizeof(struct drm_format_modifier_blob);
>  	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
> @@ -223,37 +226,45 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
>  
>  	blob_data = blob->data;
>  	blob_data->version = FORMAT_BLOB_CURRENT;
> -	blob_data->count_formats = plane->format_count;
> +	blob_data->count_formats = format_count;
>  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> -	blob_data->count_modifiers = plane->modifier_count;
> +	blob_data->count_modifiers = modifier_count;
>  
>  	blob_data->modifiers_offset =
>  		ALIGN(blob_data->formats_offset + formats_size, 8);
>  
> -	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> +	memcpy(formats_ptr(blob_data), formats, formats_size);
>  
>  	mod = modifiers_ptr(blob_data);
> -	for (i = 0; i < plane->modifier_count; i++) {
> -		for (j = 0; j < plane->format_count; j++) {
> -			if (!plane->funcs->format_mod_supported ||
> +	for (i = 0; i < modifier_count; i++) {
> +		for (j = 0; j < format_count; j++) {
> +			if (is_async ||
> +			    !plane->funcs->format_mod_supported ||
>  			    plane->funcs->format_mod_supported(plane,
> -							       plane->format_types[j],
> -							       plane->modifiers[i])) {
> +							       formats[j],
> +							       modifiers[i])) {
>  				mod->formats |= 1ULL << j;
>  			}
>  		}
>  
> -		mod->modifier = plane->modifiers[i];
> +		mod->modifier = modifiers[i];
>  		mod->offset = 0;
>  		mod->pad = 0;
>  		mod++;
>  	}
>  
> -	drm_object_attach_property(&plane->base, config->modifiers_property,
> -				   blob->base.id);
> +	if (is_async)
> +		drm_object_attach_property(&plane->base,
> +					   config->async_modifiers_property,
> +					   blob->base.id);
> +	else
> +		drm_object_attach_property(&plane->base,
> +					   config->modifiers_property,
> +					   blob->base.id);

IMO the function should only create the blob. Leave it to the caller to
attach it.

The 'is_async' parameter could also be replaced with with a function
pointer to the appropriate format_mod_supported() function. That
makes the implementation entirely generic.

>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_plane_create_format_blob);
>  
>  /**
>   * DOC: hotspot properties
> @@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>  	}
>  
>  	if (format_modifier_count)
> -		create_in_format_blob(dev, plane);
> +		drm_plane_create_format_blob(dev, plane, plane->modifiers,
> +					     format_modifier_count,
> +					     plane->format_types, format_count,
> +					     false);
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf369894a5657cd45 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -1008,5 +1008,9 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>  int drm_plane_add_size_hints_property(struct drm_plane *plane,
>  				      const struct drm_plane_size_hint *hints,
>  				      int num_hints);
> +int drm_plane_create_format_blob(struct drm_device *dev,
> +				 struct drm_plane *plane, u64 *modifiers,
> +				 unsigned int modifier_count, u32 *formats,
> +				 unsigned int format_count, bool is_async);

I don't think this needs to be exposed to anyone.
__drm_universal_plane_init() should just populate both
the normal blob, and and the async blob (iff 
.format_mod_supported_async() is provided).

>  
>  #endif
> 
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 4/5] drm/i915/display: Populate list of async supported formats/modifiers

[Ville Syrjälä]

On Wed, Jan 08, 2025 at 11:09:02AM +0530, Arun R Murthy wrote:
> Populate the list of formats/modifiers supported by async flip. Register
> a async property and expose the same to user through blob.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index ff9764cac1e71959e56283f61b5192ea261cec7a..e5e47f2219dae62e76cbde2efb40266b047ab2b2 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -170,6 +170,44 @@ static const u32 icl_hdr_plane_formats[] = {
>  	DRM_FORMAT_XVYU16161616,
>  };
>  
> +static u64 tgl_asyn_modifiers[] = {
> +	DRM_FORMAT_MOD_LINEAR,
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_4_TILED,
> +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_MTL_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> +	I915_FORMAT_MOD_4_TILED_BMG_CCS,
> +	I915_FORMAT_MOD_4_TILED_LNL_CCS,
> +};
> +
> +static u64 icl_async_modifiers[] = {
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_Yf_TILED,
> +	I915_FORMAT_MOD_Y_TILED_CCS,
> +	I915_FORMAT_MOD_Yf_TILED_CCS,
> +};
> +
> +static u64 skl_async_modifiers[] = {
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_Yf_TILED,
> +};
> +
> +static u32 intel_async_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_XRGB16161616F,
> +	DRM_FORMAT_XBGR16161616F,
> +};

I've just pushed my .can_async_flip() thing. I'm thinking with
that all this can just disappear and we can have a completely
generic implementation. Eg something like:

intel_plane_format_mod_supported_async()
{
	// some generic checks here (eg. reject planar formats)

	return plane->format_mod_supported() &&
		plane->can_async_flip();
}

> +
>  int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
>  {
>  	switch (format) {
> @@ -2613,6 +2651,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	unsigned int supported_rotations;
>  	unsigned int supported_csc;
>  	const u64 *modifiers;
> +	u64 *async_modifiers;
>  	const u32 *formats;
>  	int num_formats;
>  	int ret;
> @@ -2715,6 +2754,18 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	if (ret)
>  		goto fail;
>  
> +	if (DISPLAY_VER(dev_priv) >= 12)
> +		async_modifiers = tgl_asyn_modifiers;
> +	else if (DISPLAY_VER(dev_priv) == 11)
> +		async_modifiers = icl_async_modifiers;
> +	else
> +		async_modifiers = skl_async_modifiers;
> +
> +	drm_plane_create_format_blob(&dev_priv->drm, &plane->base,
> +				     async_modifiers, sizeof(async_modifiers),
> +				     intel_async_formats,
> +				     sizeof(intel_async_formats), true);
> +
>  	if (DISPLAY_VER(dev_priv) >= 13)
>  		supported_rotations = DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180;
>  	else
> 
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

RE: [PATCH v3 4/5] drm/i915/display: Populate list of async supported formats/modifiers

[Murthy, Arun R]

> On Wed, Jan 08, 2025 at 11:09:02AM +0530, Arun R Murthy wrote:
> > Populate the list of formats/modifiers supported by async flip.
> > Register a async property and expose the same to user through blob.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_universal_plane.c | 51
> > ++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index
> >
> ff9764cac1e71959e56283f61b5192ea261cec7a..e5e47f2219dae62e76cbde2e
> fb40
> > 266b047ab2b2 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -170,6 +170,44 @@ static const u32 icl_hdr_plane_formats[] = {
> >  	DRM_FORMAT_XVYU16161616,
> >  };
> >
> > +static u64 tgl_asyn_modifiers[] = {
> > +	DRM_FORMAT_MOD_LINEAR,
> > +	I915_FORMAT_MOD_X_TILED,
> > +	I915_FORMAT_MOD_Y_TILED,
> > +	I915_FORMAT_MOD_4_TILED,
> > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > +	I915_FORMAT_MOD_4_TILED_MTL_RC_CCS,
> > +	I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> > +	I915_FORMAT_MOD_4_TILED_BMG_CCS,
> > +	I915_FORMAT_MOD_4_TILED_LNL_CCS,
> > +};
> > +
> > +static u64 icl_async_modifiers[] = {
> > +	I915_FORMAT_MOD_X_TILED,
> > +	I915_FORMAT_MOD_Y_TILED,
> > +	I915_FORMAT_MOD_Yf_TILED,
> > +	I915_FORMAT_MOD_Y_TILED_CCS,
> > +	I915_FORMAT_MOD_Yf_TILED_CCS,
> > +};
> > +
> > +static u64 skl_async_modifiers[] = {
> > +	I915_FORMAT_MOD_X_TILED,
> > +	I915_FORMAT_MOD_Y_TILED,
> > +	I915_FORMAT_MOD_Yf_TILED,
> > +};
> > +
> > +static u32 intel_async_formats[] = {
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_XRGB2101010,
> > +	DRM_FORMAT_XBGR2101010,
> > +	DRM_FORMAT_XRGB16161616F,
> > +	DRM_FORMAT_XBGR16161616F,
> > +};
> 
> I've just pushed my .can_async_flip() thing. I'm thinking with that all this can
> just disappear and we can have a completely generic implementation. Eg
> something like:
> 
Thanks, will rebase and push!

Thanks and Regards,
Arun R Murthy
--------------------
> intel_plane_format_mod_supported_async()
> {
> 	// some generic checks here (eg. reject planar formats)
> 
> 	return plane->format_mod_supported() &&
> 		plane->can_async_flip();
> }
> 
> > +
> >  int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)  {
> >  	switch (format) {
> > @@ -2613,6 +2651,7 @@ skl_universal_plane_create(struct
> drm_i915_private *dev_priv,
> >  	unsigned int supported_rotations;
> >  	unsigned int supported_csc;
> >  	const u64 *modifiers;
> > +	u64 *async_modifiers;
> >  	const u32 *formats;
> >  	int num_formats;
> >  	int ret;
> > @@ -2715,6 +2754,18 @@ skl_universal_plane_create(struct
> drm_i915_private *dev_priv,
> >  	if (ret)
> >  		goto fail;
> >
> > +	if (DISPLAY_VER(dev_priv) >= 12)
> > +		async_modifiers = tgl_asyn_modifiers;
> > +	else if (DISPLAY_VER(dev_priv) == 11)
> > +		async_modifiers = icl_async_modifiers;
> > +	else
> > +		async_modifiers = skl_async_modifiers;
> > +
> > +	drm_plane_create_format_blob(&dev_priv->drm, &plane->base,
> > +				     async_modifiers, sizeof(async_modifiers),
> > +				     intel_async_formats,
> > +				     sizeof(intel_async_formats), true);
> > +
> >  	if (DISPLAY_VER(dev_priv) >= 13)
> >  		supported_rotations = DRM_MODE_ROTATE_0 |
> DRM_MODE_ROTATE_180;
> >  	else
> >
> > --
> > 2.25.1
> 
> --
> Ville Syrjälä
> Intel

Re: [PATCH v3 4/5] drm/i915/display: Populate list of async supported formats/modifiers

[Ville Syrjälä]

On Tue, Jan 21, 2025 at 03:34:20AM +0000, Murthy, Arun R wrote:
> > On Wed, Jan 08, 2025 at 11:09:02AM +0530, Arun R Murthy wrote:
> > > Populate the list of formats/modifiers supported by async flip.
> > > Register a async property and expose the same to user through blob.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/skl_universal_plane.c | 51
> > > ++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index
> > >
> > ff9764cac1e71959e56283f61b5192ea261cec7a..e5e47f2219dae62e76cbde2e
> > fb40
> > > 266b047ab2b2 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -170,6 +170,44 @@ static const u32 icl_hdr_plane_formats[] = {
> > >  	DRM_FORMAT_XVYU16161616,
> > >  };
> > >
> > > +static u64 tgl_asyn_modifiers[] = {
> > > +	DRM_FORMAT_MOD_LINEAR,
> > > +	I915_FORMAT_MOD_X_TILED,
> > > +	I915_FORMAT_MOD_Y_TILED,
> > > +	I915_FORMAT_MOD_4_TILED,
> > > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > +	I915_FORMAT_MOD_4_TILED_MTL_RC_CCS,
> > > +	I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> > > +	I915_FORMAT_MOD_4_TILED_BMG_CCS,
> > > +	I915_FORMAT_MOD_4_TILED_LNL_CCS,
> > > +};
> > > +
> > > +static u64 icl_async_modifiers[] = {
> > > +	I915_FORMAT_MOD_X_TILED,
> > > +	I915_FORMAT_MOD_Y_TILED,
> > > +	I915_FORMAT_MOD_Yf_TILED,
> > > +	I915_FORMAT_MOD_Y_TILED_CCS,
> > > +	I915_FORMAT_MOD_Yf_TILED_CCS,
> > > +};
> > > +
> > > +static u64 skl_async_modifiers[] = {
> > > +	I915_FORMAT_MOD_X_TILED,
> > > +	I915_FORMAT_MOD_Y_TILED,
> > > +	I915_FORMAT_MOD_Yf_TILED,
> > > +};
> > > +
> > > +static u32 intel_async_formats[] = {
> > > +	DRM_FORMAT_RGB565,
> > > +	DRM_FORMAT_XRGB8888,
> > > +	DRM_FORMAT_XBGR8888,
> > > +	DRM_FORMAT_ARGB8888,
> > > +	DRM_FORMAT_ABGR8888,
> > > +	DRM_FORMAT_XRGB2101010,
> > > +	DRM_FORMAT_XBGR2101010,
> > > +	DRM_FORMAT_XRGB16161616F,
> > > +	DRM_FORMAT_XBGR16161616F,
> > > +};
> > 
> > I've just pushed my .can_async_flip() thing. I'm thinking with that all this can
> > just disappear and we can have a completely generic implementation. Eg
> > something like:
> > 
> Thanks, will rebase and push!
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > intel_plane_format_mod_supported_async()
> > {
> > 	// some generic checks here (eg. reject planar formats)
> > 
> > 	return plane->format_mod_supported() &&
> > 		plane->can_async_flip();
> > }

Actually for this one I think it'd probably make sense to call the 
normal format_mod_supported() before doing any other checks, just 
in case we ever get situations where the parameters come directly
from userspace. It's better to filter out completely bogus values
as early as possible, and the normal format_mod_supported() already
has to be prepared for garbage values.

> > 
> > > +
> > >  int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)  {
> > >  	switch (format) {
> > > @@ -2613,6 +2651,7 @@ skl_universal_plane_create(struct
> > drm_i915_private *dev_priv,
> > >  	unsigned int supported_rotations;
> > >  	unsigned int supported_csc;
> > >  	const u64 *modifiers;
> > > +	u64 *async_modifiers;
> > >  	const u32 *formats;
> > >  	int num_formats;
> > >  	int ret;
> > > @@ -2715,6 +2754,18 @@ skl_universal_plane_create(struct
> > drm_i915_private *dev_priv,
> > >  	if (ret)
> > >  		goto fail;
> > >
> > > +	if (DISPLAY_VER(dev_priv) >= 12)
> > > +		async_modifiers = tgl_asyn_modifiers;
> > > +	else if (DISPLAY_VER(dev_priv) == 11)
> > > +		async_modifiers = icl_async_modifiers;
> > > +	else
> > > +		async_modifiers = skl_async_modifiers;
> > > +
> > > +	drm_plane_create_format_blob(&dev_priv->drm, &plane->base,
> > > +				     async_modifiers, sizeof(async_modifiers),
> > > +				     intel_async_formats,
> > > +				     sizeof(intel_async_formats), true);
> > > +
> > >  	if (DISPLAY_VER(dev_priv) >= 13)
> > >  		supported_rotations = DRM_MODE_ROTATE_0 |
> > DRM_MODE_ROTATE_180;
> > >  	else
> > >
> > > --
> > > 2.25.1
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Murthy, Arun R]

> On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > Expose drm plane function to create formats/modifiers blob. This
> > function can be used to expose list of supported formats/modifiers for
> > sync/async flips.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++++++++++-------
> --------
> >  include/drm/drm_plane.h     |  4 ++++
> >  2 files changed, 33 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index
> >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a906
> 8
> > b31c0563a4c0 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> *blob)
> >  	return (struct drm_format_modifier *)(((char *)blob) +
> > blob->modifiers_offset);  }
> >
> > -static int create_in_format_blob(struct drm_device *dev, struct
> > drm_plane *plane)
> > +int drm_plane_create_format_blob(struct drm_device *dev,
> > +				 struct drm_plane *plane, u64 *modifiers,
> > +				 unsigned int modifier_count, u32 *formats,
> > +				 unsigned int format_count, bool is_async)
> >  {
> >  	const struct drm_mode_config *config = &dev->mode_config;
> >  	struct drm_property_blob *blob;
> > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct drm_device
> *dev, struct drm_plane *plane
> >  	struct drm_format_modifier_blob *blob_data;
> >  	unsigned int i, j;
> >
> > -	formats_size = sizeof(__u32) * plane->format_count;
> > +	formats_size = sizeof(__u32) * format_count;
> >  	if (WARN_ON(!formats_size)) {
> >  		/* 0 formats are never expected */
> >  		return 0;
> >  	}
> >
> >  	modifiers_size =
> > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > +		sizeof(struct drm_format_modifier) * modifier_count;
> >
> >  	blob_size = sizeof(struct drm_format_modifier_blob);
> >  	/* Modifiers offset is a pointer to a struct with a 64 bit field so
> > it @@ -223,37 +226,45 @@ static int create_in_format_blob(struct
> > drm_device *dev, struct drm_plane *plane
> >
> >  	blob_data = blob->data;
> >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > -	blob_data->count_formats = plane->format_count;
> > +	blob_data->count_formats = format_count;
> >  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> > -	blob_data->count_modifiers = plane->modifier_count;
> > +	blob_data->count_modifiers = modifier_count;
> >
> >  	blob_data->modifiers_offset =
> >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> >
> > -	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> >
> >  	mod = modifiers_ptr(blob_data);
> > -	for (i = 0; i < plane->modifier_count; i++) {
> > -		for (j = 0; j < plane->format_count; j++) {
> > -			if (!plane->funcs->format_mod_supported ||
> > +	for (i = 0; i < modifier_count; i++) {
> > +		for (j = 0; j < format_count; j++) {
> > +			if (is_async ||
> > +			    !plane->funcs->format_mod_supported ||
> >  			    plane->funcs->format_mod_supported(plane,
> > -							       plane-
> >format_types[j],
> > -							       plane-
> >modifiers[i])) {
> > +							       formats[j],
> > +							       modifiers[i])) {
> >  				mod->formats |= 1ULL << j;
> >  			}
> >  		}
> >
> > -		mod->modifier = plane->modifiers[i];
> > +		mod->modifier = modifiers[i];
> >  		mod->offset = 0;
> >  		mod->pad = 0;
> >  		mod++;
> >  	}
> >
> > -	drm_object_attach_property(&plane->base, config-
> >modifiers_property,
> > -				   blob->base.id);
> > +	if (is_async)
> > +		drm_object_attach_property(&plane->base,
> > +					   config->async_modifiers_property,
> > +					   blob->base.id);
> > +	else
> > +		drm_object_attach_property(&plane->base,
> > +					   config->modifiers_property,
> > +					   blob->base.id);
> 
> IMO the function should only create the blob. Leave it to the caller to attach it.
> 
Prior to this change for sync formats/modifiers the property attach was also done in the same function. So retained it as it.

> The 'is_async' parameter could also be replaced with with a function pointer to
> the appropriate format_mod_supported() function. That makes the
> implementation entirely generic.
> 
If the list of formats/modifiers for sync and async is passed to this function, then based on the list the corresponding function pointer can be called.
This was done in the earlier patchset.

> >
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> >
> >  /**
> >   * DOC: hotspot properties
> > @@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct
> drm_device *dev,
> >  	}
> >
> >  	if (format_modifier_count)
> > -		create_in_format_blob(dev, plane);
> > +		drm_plane_create_format_blob(dev, plane, plane->modifiers,
> > +					     format_modifier_count,
> > +					     plane->format_types,
> format_count,
> > +					     false);
> >
> >  	return 0;
> >  }
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> >
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> 369
> > 894a5657cd45 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -1008,5 +1008,9 @@ int
> > drm_plane_create_scaling_filter_property(struct drm_plane *plane,  int
> drm_plane_add_size_hints_property(struct drm_plane *plane,
> >  				      const struct drm_plane_size_hint *hints,
> >  				      int num_hints);
> > +int drm_plane_create_format_blob(struct drm_device *dev,
> > +				 struct drm_plane *plane, u64 *modifiers,
> > +				 unsigned int modifier_count, u32 *formats,
> > +				 unsigned int format_count, bool is_async);
> 
> I don't think this needs to be exposed to anyone.
> __drm_universal_plane_init() should just populate both the normal blob, and
> and the async blob (iff
> .format_mod_supported_async() is provided).
> 
Ok!

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

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Murthy, Arun R]

> > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > Expose drm plane function to create formats/modifiers blob. This
> > > function can be used to expose list of supported formats/modifiers
> > > for sync/async flips.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 44
> > > +++++++++++++++++++++++++++++-------
> > --------
> > >  include/drm/drm_plane.h     |  4 ++++
> > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > b/drivers/gpu/drm/drm_plane.c index
> > >
> >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a906
> > 8
> > > b31c0563a4c0 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> > *blob)
> > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > blob->modifiers_offset);  }
> > >
> > > -static int create_in_format_blob(struct drm_device *dev, struct
> > > drm_plane *plane)
> > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > +				 struct drm_plane *plane, u64 *modifiers,
> > > +				 unsigned int modifier_count, u32 *formats,
> > > +				 unsigned int format_count, bool is_async)
> > >  {
> > >  	const struct drm_mode_config *config = &dev->mode_config;
> > >  	struct drm_property_blob *blob;
> > > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct
> > > drm_device
> > *dev, struct drm_plane *plane
> > >  	struct drm_format_modifier_blob *blob_data;
> > >  	unsigned int i, j;
> > >
> > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > +	formats_size = sizeof(__u32) * format_count;
> > >  	if (WARN_ON(!formats_size)) {
> > >  		/* 0 formats are never expected */
> > >  		return 0;
> > >  	}
> > >
> > >  	modifiers_size =
> > > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > >
> > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > >  	/* Modifiers offset is a pointer to a struct with a 64 bit field
> > > so it @@ -223,37 +226,45 @@ static int create_in_format_blob(struct
> > > drm_device *dev, struct drm_plane *plane
> > >
> > >  	blob_data = blob->data;
> > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > -	blob_data->count_formats = plane->format_count;
> > > +	blob_data->count_formats = format_count;
> > >  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> > > -	blob_data->count_modifiers = plane->modifier_count;
> > > +	blob_data->count_modifiers = modifier_count;
> > >
> > >  	blob_data->modifiers_offset =
> > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > >
> > > -	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > >
> > >  	mod = modifiers_ptr(blob_data);
> > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > -		for (j = 0; j < plane->format_count; j++) {
> > > -			if (!plane->funcs->format_mod_supported ||
> > > +	for (i = 0; i < modifier_count; i++) {
> > > +		for (j = 0; j < format_count; j++) {
> > > +			if (is_async ||
> > > +			    !plane->funcs->format_mod_supported ||
> > >  			    plane->funcs->format_mod_supported(plane,
> > > -							       plane-
> > >format_types[j],
> > > -							       plane-
> > >modifiers[i])) {
> > > +							       formats[j],
> > > +							       modifiers[i])) {
> > >  				mod->formats |= 1ULL << j;
> > >  			}
> > >  		}
> > >
> > > -		mod->modifier = plane->modifiers[i];
> > > +		mod->modifier = modifiers[i];
> > >  		mod->offset = 0;
> > >  		mod->pad = 0;
> > >  		mod++;
> > >  	}
> > >
> > > -	drm_object_attach_property(&plane->base, config-
> > >modifiers_property,
> > > -				   blob->base.id);
> > > +	if (is_async)
> > > +		drm_object_attach_property(&plane->base,
> > > +					   config->async_modifiers_property,
> > > +					   blob->base.id);
> > > +	else
> > > +		drm_object_attach_property(&plane->base,
> > > +					   config->modifiers_property,
> > > +					   blob->base.id);
> >
> > IMO the function should only create the blob. Leave it to the caller to attach
> it.
> >
> Prior to this change for sync formats/modifiers the property attach was also
> done in the same function. So retained it as it.
> 
> > The 'is_async' parameter could also be replaced with with a function
> > pointer to the appropriate format_mod_supported() function. That makes
> > the implementation entirely generic.
> >
> If the list of formats/modifiers for sync and async is passed to this function, then
> based on the list the corresponding function pointer can be called.
> This was done in the earlier patchset.
> 
> > >
> > >  	return 0;
> > >  }
> > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > >
> > >  /**
> > >   * DOC: hotspot properties
> > > @@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct
> > drm_device *dev,
> > >  	}
> > >
> > >  	if (format_modifier_count)
> > > -		create_in_format_blob(dev, plane);
> > > +		drm_plane_create_format_blob(dev, plane, plane->modifiers,
> > > +					     format_modifier_count,
> > > +					     plane->format_types,
> > format_count,
> > > +					     false);
> > >
> > >  	return 0;
> > >  }
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> > >
> >
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > 369
> > > 894a5657cd45 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -1008,5 +1008,9 @@ int
> > > drm_plane_create_scaling_filter_property(struct drm_plane *plane,
> > > int
> > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > >  				      const struct drm_plane_size_hint *hints,
> > >  				      int num_hints);
> > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > +				 struct drm_plane *plane, u64 *modifiers,
> > > +				 unsigned int modifier_count, u32 *formats,
> > > +				 unsigned int format_count, bool is_async);
> >
> > I don't think this needs to be exposed to anyone.
> > __drm_universal_plane_init() should just populate both the normal
> > blob, and and the async blob (iff
> > .format_mod_supported_async() is provided).
> >
> Ok!
> 
For __drm_universal_plane_init() to have both the sync/async format/modifiers list we will have to add new elements to struct drm_plane to hold the async formats/modifiers.
Then upon adding new elements in struct drm_plane we may not need to pass a function pointer instead of is_async as commented above as well as this new elements added in the struct drm_plane helps out over here.

New elements to be added to the struct drm_plane 
Struct drm_plane {
	U32 * formats_async;
	U32 format_async_count;
	U64 *async_modifiers,
	U32 async_modifier_count
}

Then the functionwith below changes it will be generic and no exporting of the func would be required
 create_format_blob()
{
	If(plane->format_count)
		/* Create blob for sync and call the format_mod_supported()  */

	If(plane->format_async_count)
		/* Create blob for async and call the format_mod_async_supported()  */
}

Is my understanding correct?

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

Re: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Ville Syrjälä]

On Thu, Jan 23, 2025 at 07:47:14AM +0000, Murthy, Arun R wrote:
> > > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > > Expose drm plane function to create formats/modifiers blob. This
> > > > function can be used to expose list of supported formats/modifiers
> > > > for sync/async flips.
> > > >
> > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 44
> > > > +++++++++++++++++++++++++++++-------
> > > --------
> > > >  include/drm/drm_plane.h     |  4 ++++
> > > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > b/drivers/gpu/drm/drm_plane.c index
> > > >
> > >
> > 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a906
> > > 8
> > > > b31c0563a4c0 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct drm_format_modifier_blob
> > > *blob)
> > > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > > blob->modifiers_offset);  }
> > > >
> > > > -static int create_in_format_blob(struct drm_device *dev, struct
> > > > drm_plane *plane)
> > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > +				 struct drm_plane *plane, u64 *modifiers,
> > > > +				 unsigned int modifier_count, u32 *formats,
> > > > +				 unsigned int format_count, bool is_async)
> > > >  {
> > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > >  	struct drm_property_blob *blob;
> > > > @@ -200,14 +203,14 @@ static int create_in_format_blob(struct
> > > > drm_device
> > > *dev, struct drm_plane *plane
> > > >  	struct drm_format_modifier_blob *blob_data;
> > > >  	unsigned int i, j;
> > > >
> > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > +	formats_size = sizeof(__u32) * format_count;
> > > >  	if (WARN_ON(!formats_size)) {
> > > >  		/* 0 formats are never expected */
> > > >  		return 0;
> > > >  	}
> > > >
> > > >  	modifiers_size =
> > > > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > >
> > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > >  	/* Modifiers offset is a pointer to a struct with a 64 bit field
> > > > so it @@ -223,37 +226,45 @@ static int create_in_format_blob(struct
> > > > drm_device *dev, struct drm_plane *plane
> > > >
> > > >  	blob_data = blob->data;
> > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > -	blob_data->count_formats = plane->format_count;
> > > > +	blob_data->count_formats = format_count;
> > > >  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > +	blob_data->count_modifiers = modifier_count;
> > > >
> > > >  	blob_data->modifiers_offset =
> > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > >
> > > > -	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > >
> > > >  	mod = modifiers_ptr(blob_data);
> > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > -			if (!plane->funcs->format_mod_supported ||
> > > > +	for (i = 0; i < modifier_count; i++) {
> > > > +		for (j = 0; j < format_count; j++) {
> > > > +			if (is_async ||
> > > > +			    !plane->funcs->format_mod_supported ||
> > > >  			    plane->funcs->format_mod_supported(plane,
> > > > -							       plane-
> > > >format_types[j],
> > > > -							       plane-
> > > >modifiers[i])) {
> > > > +							       formats[j],
> > > > +							       modifiers[i])) {
> > > >  				mod->formats |= 1ULL << j;
> > > >  			}
> > > >  		}
> > > >
> > > > -		mod->modifier = plane->modifiers[i];
> > > > +		mod->modifier = modifiers[i];
> > > >  		mod->offset = 0;
> > > >  		mod->pad = 0;
> > > >  		mod++;
> > > >  	}
> > > >
> > > > -	drm_object_attach_property(&plane->base, config-
> > > >modifiers_property,
> > > > -				   blob->base.id);
> > > > +	if (is_async)
> > > > +		drm_object_attach_property(&plane->base,
> > > > +					   config->async_modifiers_property,
> > > > +					   blob->base.id);
> > > > +	else
> > > > +		drm_object_attach_property(&plane->base,
> > > > +					   config->modifiers_property,
> > > > +					   blob->base.id);
> > >
> > > IMO the function should only create the blob. Leave it to the caller to attach
> > it.
> > >
> > Prior to this change for sync formats/modifiers the property attach was also
> > done in the same function. So retained it as it.
> > 
> > > The 'is_async' parameter could also be replaced with with a function
> > > pointer to the appropriate format_mod_supported() function. That makes
> > > the implementation entirely generic.
> > >
> > If the list of formats/modifiers for sync and async is passed to this function, then
> > based on the list the corresponding function pointer can be called.
> > This was done in the earlier patchset.
> > 
> > > >
> > > >  	return 0;
> > > >  }
> > > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > > >
> > > >  /**
> > > >   * DOC: hotspot properties
> > > > @@ -476,7 +487,10 @@ static int __drm_universal_plane_init(struct
> > > drm_device *dev,
> > > >  	}
> > > >
> > > >  	if (format_modifier_count)
> > > > -		create_in_format_blob(dev, plane);
> > > > +		drm_plane_create_format_blob(dev, plane, plane->modifiers,
> > > > +					     format_modifier_count,
> > > > +					     plane->format_types,
> > > format_count,
> > > > +					     false);
> > > >
> > > >  	return 0;
> > > >  }
> > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> > > >
> > >
> > e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > > 369
> > > > 894a5657cd45 100644
> > > > --- a/include/drm/drm_plane.h
> > > > +++ b/include/drm/drm_plane.h
> > > > @@ -1008,5 +1008,9 @@ int
> > > > drm_plane_create_scaling_filter_property(struct drm_plane *plane,
> > > > int
> > > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > > >  				      const struct drm_plane_size_hint *hints,
> > > >  				      int num_hints);
> > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > +				 struct drm_plane *plane, u64 *modifiers,
> > > > +				 unsigned int modifier_count, u32 *formats,
> > > > +				 unsigned int format_count, bool is_async);
> > >
> > > I don't think this needs to be exposed to anyone.
> > > __drm_universal_plane_init() should just populate both the normal
> > > blob, and and the async blob (iff
> > > .format_mod_supported_async() is provided).
> > >
> > Ok!
> > 
> For __drm_universal_plane_init() to have both the sync/async format/modifiers list we will have to add new elements to struct drm_plane to hold the async formats/modifiers.

No, you can just use the already existing format/modifier lists.
.format_mod_supported_async() will filter out what's not wanted.

> Then upon adding new elements in struct drm_plane we may not need to pass a function pointer instead of is_async as commented above as well as this new elements added in the struct drm_plane helps out over here.
> 
> New elements to be added to the struct drm_plane 
> Struct drm_plane {
> 	U32 * formats_async;
> 	U32 format_async_count;
> 	U64 *async_modifiers,
> 	U32 async_modifier_count
> }
> 
> Then the functionwith below changes it will be generic and no exporting of the func would be required
>  create_format_blob()
> {
> 	If(plane->format_count)
> 		/* Create blob for sync and call the format_mod_supported()  */
> 
> 	If(plane->format_async_count)
> 		/* Create blob for async and call the format_mod_async_supported()  */
> }
> 
> Is my understanding correct?
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------

-- 
Ville Syrjälä
Intel

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Murthy, Arun R]

> On Thu, Jan 23, 2025 at 07:47:14AM +0000, Murthy, Arun R wrote:
> > > > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > > > Expose drm plane function to create formats/modifiers blob. This
> > > > > function can be used to expose list of supported
> > > > > formats/modifiers for sync/async flips.
> > > > >
> > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 44
> > > > > +++++++++++++++++++++++++++++-------
> > > > --------
> > > > >  include/drm/drm_plane.h     |  4 ++++
> > > > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > b/drivers/gpu/drm/drm_plane.c index
> > > > >
> > > >
> > >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > > 6
> > > > 8
> > > > > b31c0563a4c0 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct
> > > > > drm_format_modifier_blob
> > > > *blob)
> > > > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > > > blob->modifiers_offset);  }
> > > > >
> > > > > -static int create_in_format_blob(struct drm_device *dev, struct
> > > > > drm_plane *plane)
> > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > +				 struct drm_plane *plane, u64
> *modifiers,
> > > > > +				 unsigned int modifier_count, u32
> *formats,
> > > > > +				 unsigned int format_count, bool
> is_async)
> > > > >  {
> > > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > > >  	struct drm_property_blob *blob; @@ -200,14 +203,14 @@ static
> > > > > int create_in_format_blob(struct drm_device
> > > > *dev, struct drm_plane *plane
> > > > >  	struct drm_format_modifier_blob *blob_data;
> > > > >  	unsigned int i, j;
> > > > >
> > > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > > +	formats_size = sizeof(__u32) * format_count;
> > > > >  	if (WARN_ON(!formats_size)) {
> > > > >  		/* 0 formats are never expected */
> > > > >  		return 0;
> > > > >  	}
> > > > >
> > > > >  	modifiers_size =
> > > > > -		sizeof(struct drm_format_modifier) * plane->modifier_count;
> > > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > > >
> > > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > > >  	/* Modifiers offset is a pointer to a struct with a 64 bit
> > > > > field so it @@ -223,37 +226,45 @@ static int
> > > > > create_in_format_blob(struct drm_device *dev, struct drm_plane
> > > > > *plane
> > > > >
> > > > >  	blob_data = blob->data;
> > > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > > -	blob_data->count_formats = plane->format_count;
> > > > > +	blob_data->count_formats = format_count;
> > > > >  	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> > > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > > +	blob_data->count_modifiers = modifier_count;
> > > > >
> > > > >  	blob_data->modifiers_offset =
> > > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > > >
> > > > > -	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> > > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > > >
> > > > >  	mod = modifiers_ptr(blob_data);
> > > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > > -			if (!plane->funcs->format_mod_supported ||
> > > > > +	for (i = 0; i < modifier_count; i++) {
> > > > > +		for (j = 0; j < format_count; j++) {
> > > > > +			if (is_async ||
> > > > > +			    !plane->funcs->format_mod_supported ||
> > > > >  			    plane->funcs->format_mod_supported(plane,
> > > > > -							       plane-
> > > > >format_types[j],
> > > > > -							       plane-
> > > > >modifiers[i])) {
> > > > > +							       formats[j],
> > > > > +
> modifiers[i])) {
> > > > >  				mod->formats |= 1ULL << j;
> > > > >  			}
> > > > >  		}
> > > > >
> > > > > -		mod->modifier = plane->modifiers[i];
> > > > > +		mod->modifier = modifiers[i];
> > > > >  		mod->offset = 0;
> > > > >  		mod->pad = 0;
> > > > >  		mod++;
> > > > >  	}
> > > > >
> > > > > -	drm_object_attach_property(&plane->base, config-
> > > > >modifiers_property,
> > > > > -				   blob->base.id);
> > > > > +	if (is_async)
> > > > > +		drm_object_attach_property(&plane->base,
> > > > > +					   config-
> >async_modifiers_property,
> > > > > +					   blob->base.id);
> > > > > +	else
> > > > > +		drm_object_attach_property(&plane->base,
> > > > > +					   config->modifiers_property,
> > > > > +					   blob->base.id);
> > > >
> > > > IMO the function should only create the blob. Leave it to the
> > > > caller to attach
> > > it.
> > > >
> > > Prior to this change for sync formats/modifiers the property attach
> > > was also done in the same function. So retained it as it.
> > >
> > > > The 'is_async' parameter could also be replaced with with a
> > > > function pointer to the appropriate format_mod_supported()
> > > > function. That makes the implementation entirely generic.
> > > >
> > > If the list of formats/modifiers for sync and async is passed to
> > > this function, then based on the list the corresponding function pointer can
> be called.
> > > This was done in the earlier patchset.
> > >
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > > > >
> > > > >  /**
> > > > >   * DOC: hotspot properties
> > > > > @@ -476,7 +487,10 @@ static int
> > > > > __drm_universal_plane_init(struct
> > > > drm_device *dev,
> > > > >  	}
> > > > >
> > > > >  	if (format_modifier_count)
> > > > > -		create_in_format_blob(dev, plane);
> > > > > +		drm_plane_create_format_blob(dev, plane, plane-
> >modifiers,
> > > > > +					     format_modifier_count,
> > > > > +					     plane->format_types,
> > > > format_count,
> > > > > +					     false);
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > > index
> > > > >
> > > >
> > >
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > > > 369
> > > > > 894a5657cd45 100644
> > > > > --- a/include/drm/drm_plane.h
> > > > > +++ b/include/drm/drm_plane.h
> > > > > @@ -1008,5 +1008,9 @@ int
> > > > > drm_plane_create_scaling_filter_property(struct drm_plane
> > > > > *plane, int
> > > > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > > > >  				      const struct drm_plane_size_hint *hints,
> > > > >  				      int num_hints);
> > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > +				 struct drm_plane *plane, u64
> *modifiers,
> > > > > +				 unsigned int modifier_count, u32
> *formats,
> > > > > +				 unsigned int format_count, bool
> is_async);
> > > >
> > > > I don't think this needs to be exposed to anyone.
> > > > __drm_universal_plane_init() should just populate both the normal
> > > > blob, and and the async blob (iff
> > > > .format_mod_supported_async() is provided).
> > > >
> > > Ok!
> > >
> > For __drm_universal_plane_init() to have both the sync/async
> format/modifiers list we will have to add new elements to struct drm_plane to
> hold the async formats/modifiers.
> 
> No, you can just use the already existing format/modifier lists.
> .format_mod_supported_async() will filter out what's not wanted.
> 
Agree, to populate the struct drm_format_modifier .format_mod_supported_async along with the existing formats/modifier list should be sufficient.
In case of async for the struct drm_format_modifier_blob the elements format_offset includes the list of formats supported by async only. For this we will need to have the static list. So can passing this list be done by adding new elements in drm_plane specifically for async.

Thanks and Regards,
Arun R Murthy
-------------------
> > Then upon adding new elements in struct drm_plane we may not need to
> pass a function pointer instead of is_async as commented above as well as this
> new elements added in the struct drm_plane helps out over here.
> >
> > New elements to be added to the struct drm_plane Struct drm_plane {
> > 	U32 * formats_async;
> > 	U32 format_async_count;
> > 	U64 *async_modifiers,
> > 	U32 async_modifier_count
> > }
> >
> > Then the functionwith below changes it will be generic and no
> > exporting of the func would be required
> >  create_format_blob()
> > {
> > 	If(plane->format_count)
> > 		/* Create blob for sync and call the format_mod_supported()
> */
> >
> > 	If(plane->format_async_count)
> > 		/* Create blob for async and call the
> format_mod_async_supported()
> > */ }
> >
> > Is my understanding correct?
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> 
> --
> Ville Syrjälä
> Intel

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Borah, Chaitanya Kumar]

Hello Ville,

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Murthy,
> Arun R
> Sent: Saturday, January 25, 2025 12:25 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Subject: RE: [PATCH v3 2/5] drm/plane: Expose function to create
> format/modifier blob
> 
> > On Thu, Jan 23, 2025 at 07:47:14AM +0000, Murthy, Arun R wrote:
> > > > > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > > > > Expose drm plane function to create formats/modifiers blob.
> > > > > > This function can be used to expose list of supported
> > > > > > formats/modifiers for sync/async flips.
> > > > > >
> > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_plane.c | 44
> > > > > > +++++++++++++++++++++++++++++-------
> > > > > --------
> > > > > >  include/drm/drm_plane.h     |  4 ++++
> > > > > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > > b/drivers/gpu/drm/drm_plane.c index
> > > > > >
> > > > >
> > > >
> >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > > > 6
> > > > > 8
> > > > > > b31c0563a4c0 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct
> > > > > > drm_format_modifier_blob
> > > > > *blob)
> > > > > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > > > > blob->modifiers_offset);  }
> > > > > >
> > > > > > -static int create_in_format_blob(struct drm_device *dev,
> > > > > > struct drm_plane *plane)
> > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > +				 struct drm_plane *plane, u64
> > *modifiers,
> > > > > > +				 unsigned int modifier_count, u32
> > *formats,
> > > > > > +				 unsigned int format_count, bool
> > is_async)
> > > > > >  {
> > > > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > > > >  	struct drm_property_blob *blob; @@ -200,14 +203,14 @@
> static
> > > > > > int create_in_format_blob(struct drm_device
> > > > > *dev, struct drm_plane *plane
> > > > > >  	struct drm_format_modifier_blob *blob_data;
> > > > > >  	unsigned int i, j;
> > > > > >
> > > > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > > > +	formats_size = sizeof(__u32) * format_count;
> > > > > >  	if (WARN_ON(!formats_size)) {
> > > > > >  		/* 0 formats are never expected */
> > > > > >  		return 0;
> > > > > >  	}
> > > > > >
> > > > > >  	modifiers_size =
> > > > > > -		sizeof(struct drm_format_modifier) * plane-
> >modifier_count;
> > > > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > > > >
> > > > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > > > >  	/* Modifiers offset is a pointer to a struct with a 64 bit
> > > > > > field so it @@ -223,37 +226,45 @@ static int
> > > > > > create_in_format_blob(struct drm_device *dev, struct drm_plane
> > > > > > *plane
> > > > > >
> > > > > >  	blob_data = blob->data;
> > > > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > > > -	blob_data->count_formats = plane->format_count;
> > > > > > +	blob_data->count_formats = format_count;
> > > > > >  	blob_data->formats_offset = sizeof(struct
> drm_format_modifier_blob);
> > > > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > > > +	blob_data->count_modifiers = modifier_count;
> > > > > >
> > > > > >  	blob_data->modifiers_offset =
> > > > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > > > >
> > > > > > -	memcpy(formats_ptr(blob_data), plane->format_types,
> formats_size);
> > > > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > > > >
> > > > > >  	mod = modifiers_ptr(blob_data);
> > > > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > > > -			if (!plane->funcs->format_mod_supported ||
> > > > > > +	for (i = 0; i < modifier_count; i++) {
> > > > > > +		for (j = 0; j < format_count; j++) {
> > > > > > +			if (is_async ||
> > > > > > +			    !plane->funcs->format_mod_supported ||
> > > > > >  			    plane->funcs-
> >format_mod_supported(plane,
> > > > > > -							       plane-
> > > > > >format_types[j],
> > > > > > -							       plane-
> > > > > >modifiers[i])) {
> > > > > > +							       formats[j],
> > > > > > +
> > modifiers[i])) {
> > > > > >  				mod->formats |= 1ULL << j;
> > > > > >  			}
> > > > > >  		}
> > > > > >
> > > > > > -		mod->modifier = plane->modifiers[i];
> > > > > > +		mod->modifier = modifiers[i];
> > > > > >  		mod->offset = 0;
> > > > > >  		mod->pad = 0;
> > > > > >  		mod++;
> > > > > >  	}
> > > > > >
> > > > > > -	drm_object_attach_property(&plane->base, config-
> > > > > >modifiers_property,
> > > > > > -				   blob->base.id);
> > > > > > +	if (is_async)
> > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > +					   config-
> > >async_modifiers_property,
> > > > > > +					   blob->base.id);
> > > > > > +	else
> > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > +					   config->modifiers_property,
> > > > > > +					   blob->base.id);
> > > > >
> > > > > IMO the function should only create the blob. Leave it to the
> > > > > caller to attach
> > > > it.
> > > > >
> > > > Prior to this change for sync formats/modifiers the property
> > > > attach was also done in the same function. So retained it as it.
> > > >
> > > > > The 'is_async' parameter could also be replaced with with a
> > > > > function pointer to the appropriate format_mod_supported()
> > > > > function. That makes the implementation entirely generic.
> > > > >
> > > > If the list of formats/modifiers for sync and async is passed to
> > > > this function, then based on the list the corresponding function
> > > > pointer can
> > be called.
> > > > This was done in the earlier patchset.
> > > >
> > > > > >
> > > > > >  	return 0;
> > > > > >  }
> > > > > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > > > > >
> > > > > >  /**
> > > > > >   * DOC: hotspot properties
> > > > > > @@ -476,7 +487,10 @@ static int
> > > > > > __drm_universal_plane_init(struct
> > > > > drm_device *dev,
> > > > > >  	}
> > > > > >
> > > > > >  	if (format_modifier_count)
> > > > > > -		create_in_format_blob(dev, plane);
> > > > > > +		drm_plane_create_format_blob(dev, plane, plane-
> > >modifiers,
> > > > > > +					     format_modifier_count,
> > > > > > +					     plane->format_types,
> > > > > format_count,
> > > > > > +					     false);
> > > > > >
> > > > > >  	return 0;
> > > > > >  }
> > > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > > > index
> > > > > >
> > > > >
> > > >
> >
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > > > > 369
> > > > > > 894a5657cd45 100644
> > > > > > --- a/include/drm/drm_plane.h
> > > > > > +++ b/include/drm/drm_plane.h
> > > > > > @@ -1008,5 +1008,9 @@ int
> > > > > > drm_plane_create_scaling_filter_property(struct drm_plane
> > > > > > *plane, int
> > > > > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > > > > >  				      const struct drm_plane_size_hint
> *hints,
> > > > > >  				      int num_hints);
> > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > +				 struct drm_plane *plane, u64
> > *modifiers,
> > > > > > +				 unsigned int modifier_count, u32
> > *formats,
> > > > > > +				 unsigned int format_count, bool
> > is_async);
> > > > >
> > > > > I don't think this needs to be exposed to anyone.
> > > > > __drm_universal_plane_init() should just populate both the
> > > > > normal blob, and and the async blob (iff
> > > > > .format_mod_supported_async() is provided).
> > > > >
> > > > Ok!
> > > >
> > > For __drm_universal_plane_init() to have both the sync/async
> > format/modifiers list we will have to add new elements to struct
> > drm_plane to hold the async formats/modifiers.
> >
> > No, you can just use the already existing format/modifier lists.
> > .format_mod_supported_async() will filter out what's not wanted.
> >
> Agree, to populate the struct drm_format_modifier
> .format_mod_supported_async along with the existing formats/modifier list
> should be sufficient.
> In case of async for the struct drm_format_modifier_blob the elements
> format_offset includes the list of formats supported by async only. For this we
> will need to have the static list. So can passing this list be done by adding new
> elements in drm_plane specifically for async.

Just to add to Arun's point. We had this discussion on thread [1].

If we populate struct drm_format_modifier_blob for async using just the existing format/modifier lists in struct drm_plane,
We will be mis-representing the following members of the structure to the user space

struct drm_format_modifier_blob {
        /* Number of fourcc formats supported */
        __u32 count_formats;

       /* Number of drm_format_modifiers */
        __u32 count_modifiers;
};

However, as you correctly point out, it should still work because of the format-modifier bit mask.
But it leaves the overall blob unnecessarily bloated (for example, with unnecessary entries in the format list).

We could however change the function in such a way that the loop

for (i = 0; i < modifier_count; i++) {
                for (j = 0; j < format_count; j++)

runs before filling up these members.

I am not sure how much juggling that would take but it could be a solution.

Anything you would suggest?

[1] https://lore.kernel.org/intel-gfx/SJ1PR11MB6129F7B51AD33A31D6A07B95B91F2@SJ1PR11MB6129.namprd11.prod.outlook.com/

Regards

Chaitanya

> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> > > Then upon adding new elements in struct drm_plane we may not need to
> > pass a function pointer instead of is_async as commented above as well
> > as this new elements added in the struct drm_plane helps out over here.
> > >
> > > New elements to be added to the struct drm_plane Struct drm_plane {
> > > 	U32 * formats_async;
> > > 	U32 format_async_count;
> > > 	U64 *async_modifiers,
> > > 	U32 async_modifier_count
> > > }
> > >
> > > Then the functionwith below changes it will be generic and no
> > > exporting of the func would be required
> > >  create_format_blob()
> > > {
> > > 	If(plane->format_count)
> > > 		/* Create blob for sync and call the format_mod_supported()
> > */
> > >
> > > 	If(plane->format_async_count)
> > > 		/* Create blob for async and call the
> > format_mod_async_supported()
> > > */ }
> > >
> > > Is my understanding correct?
> > >
> > > Thanks and Regards,
> > > Arun R Murthy
> > > --------------------
> >
> > --
> > Ville Syrjälä
> > Intel

Re: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Ville Syrjälä]

On Mon, Jan 27, 2025 at 07:25:31AM +0000, Borah, Chaitanya Kumar wrote:
> Hello Ville,
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Murthy,
> > Arun R
> > Sent: Saturday, January 25, 2025 12:25 PM
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> > xe@lists.freedesktop.org
> > Subject: RE: [PATCH v3 2/5] drm/plane: Expose function to create
> > format/modifier blob
> > 
> > > On Thu, Jan 23, 2025 at 07:47:14AM +0000, Murthy, Arun R wrote:
> > > > > > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > > > > > Expose drm plane function to create formats/modifiers blob.
> > > > > > > This function can be used to expose list of supported
> > > > > > > formats/modifiers for sync/async flips.
> > > > > > >
> > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_plane.c | 44
> > > > > > > +++++++++++++++++++++++++++++-------
> > > > > > --------
> > > > > > >  include/drm/drm_plane.h     |  4 ++++
> > > > > > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > > > b/drivers/gpu/drm/drm_plane.c index
> > > > > > >
> > > > > >
> > > > >
> > >
> > 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > > > > 6
> > > > > > 8
> > > > > > > b31c0563a4c0 100644
> > > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct
> > > > > > > drm_format_modifier_blob
> > > > > > *blob)
> > > > > > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > > > > > blob->modifiers_offset);  }
> > > > > > >
> > > > > > > -static int create_in_format_blob(struct drm_device *dev,
> > > > > > > struct drm_plane *plane)
> > > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > > +				 struct drm_plane *plane, u64
> > > *modifiers,
> > > > > > > +				 unsigned int modifier_count, u32
> > > *formats,
> > > > > > > +				 unsigned int format_count, bool
> > > is_async)
> > > > > > >  {
> > > > > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > > > > >  	struct drm_property_blob *blob; @@ -200,14 +203,14 @@
> > static
> > > > > > > int create_in_format_blob(struct drm_device
> > > > > > *dev, struct drm_plane *plane
> > > > > > >  	struct drm_format_modifier_blob *blob_data;
> > > > > > >  	unsigned int i, j;
> > > > > > >
> > > > > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > > > > +	formats_size = sizeof(__u32) * format_count;
> > > > > > >  	if (WARN_ON(!formats_size)) {
> > > > > > >  		/* 0 formats are never expected */
> > > > > > >  		return 0;
> > > > > > >  	}
> > > > > > >
> > > > > > >  	modifiers_size =
> > > > > > > -		sizeof(struct drm_format_modifier) * plane-
> > >modifier_count;
> > > > > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > > > > >
> > > > > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > > > > >  	/* Modifiers offset is a pointer to a struct with a 64 bit
> > > > > > > field so it @@ -223,37 +226,45 @@ static int
> > > > > > > create_in_format_blob(struct drm_device *dev, struct drm_plane
> > > > > > > *plane
> > > > > > >
> > > > > > >  	blob_data = blob->data;
> > > > > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > > > > -	blob_data->count_formats = plane->format_count;
> > > > > > > +	blob_data->count_formats = format_count;
> > > > > > >  	blob_data->formats_offset = sizeof(struct
> > drm_format_modifier_blob);
> > > > > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > > > > +	blob_data->count_modifiers = modifier_count;
> > > > > > >
> > > > > > >  	blob_data->modifiers_offset =
> > > > > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > > > > >
> > > > > > > -	memcpy(formats_ptr(blob_data), plane->format_types,
> > formats_size);
> > > > > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > > > > >
> > > > > > >  	mod = modifiers_ptr(blob_data);
> > > > > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > > > > -			if (!plane->funcs->format_mod_supported ||
> > > > > > > +	for (i = 0; i < modifier_count; i++) {
> > > > > > > +		for (j = 0; j < format_count; j++) {
> > > > > > > +			if (is_async ||
> > > > > > > +			    !plane->funcs->format_mod_supported ||
> > > > > > >  			    plane->funcs-
> > >format_mod_supported(plane,
> > > > > > > -							       plane-
> > > > > > >format_types[j],
> > > > > > > -							       plane-
> > > > > > >modifiers[i])) {
> > > > > > > +							       formats[j],
> > > > > > > +
> > > modifiers[i])) {
> > > > > > >  				mod->formats |= 1ULL << j;
> > > > > > >  			}
> > > > > > >  		}
> > > > > > >
> > > > > > > -		mod->modifier = plane->modifiers[i];
> > > > > > > +		mod->modifier = modifiers[i];
> > > > > > >  		mod->offset = 0;
> > > > > > >  		mod->pad = 0;
> > > > > > >  		mod++;
> > > > > > >  	}
> > > > > > >
> > > > > > > -	drm_object_attach_property(&plane->base, config-
> > > > > > >modifiers_property,
> > > > > > > -				   blob->base.id);
> > > > > > > +	if (is_async)
> > > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > > +					   config-
> > > >async_modifiers_property,
> > > > > > > +					   blob->base.id);
> > > > > > > +	else
> > > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > > +					   config->modifiers_property,
> > > > > > > +					   blob->base.id);
> > > > > >
> > > > > > IMO the function should only create the blob. Leave it to the
> > > > > > caller to attach
> > > > > it.
> > > > > >
> > > > > Prior to this change for sync formats/modifiers the property
> > > > > attach was also done in the same function. So retained it as it.
> > > > >
> > > > > > The 'is_async' parameter could also be replaced with with a
> > > > > > function pointer to the appropriate format_mod_supported()
> > > > > > function. That makes the implementation entirely generic.
> > > > > >
> > > > > If the list of formats/modifiers for sync and async is passed to
> > > > > this function, then based on the list the corresponding function
> > > > > pointer can
> > > be called.
> > > > > This was done in the earlier patchset.
> > > > >
> > > > > > >
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > > > > > >
> > > > > > >  /**
> > > > > > >   * DOC: hotspot properties
> > > > > > > @@ -476,7 +487,10 @@ static int
> > > > > > > __drm_universal_plane_init(struct
> > > > > > drm_device *dev,
> > > > > > >  	}
> > > > > > >
> > > > > > >  	if (format_modifier_count)
> > > > > > > -		create_in_format_blob(dev, plane);
> > > > > > > +		drm_plane_create_format_blob(dev, plane, plane-
> > > >modifiers,
> > > > > > > +					     format_modifier_count,
> > > > > > > +					     plane->format_types,
> > > > > > format_count,
> > > > > > > +					     false);
> > > > > > >
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > > > > index
> > > > > > >
> > > > > >
> > > > >
> > >
> > e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > > > > > 369
> > > > > > > 894a5657cd45 100644
> > > > > > > --- a/include/drm/drm_plane.h
> > > > > > > +++ b/include/drm/drm_plane.h
> > > > > > > @@ -1008,5 +1008,9 @@ int
> > > > > > > drm_plane_create_scaling_filter_property(struct drm_plane
> > > > > > > *plane, int
> > > > > > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > > > > > >  				      const struct drm_plane_size_hint
> > *hints,
> > > > > > >  				      int num_hints);
> > > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > > +				 struct drm_plane *plane, u64
> > > *modifiers,
> > > > > > > +				 unsigned int modifier_count, u32
> > > *formats,
> > > > > > > +				 unsigned int format_count, bool
> > > is_async);
> > > > > >
> > > > > > I don't think this needs to be exposed to anyone.
> > > > > > __drm_universal_plane_init() should just populate both the
> > > > > > normal blob, and and the async blob (iff
> > > > > > .format_mod_supported_async() is provided).
> > > > > >
> > > > > Ok!
> > > > >
> > > > For __drm_universal_plane_init() to have both the sync/async
> > > format/modifiers list we will have to add new elements to struct
> > > drm_plane to hold the async formats/modifiers.
> > >
> > > No, you can just use the already existing format/modifier lists.
> > > .format_mod_supported_async() will filter out what's not wanted.
> > >
> > Agree, to populate the struct drm_format_modifier
> > .format_mod_supported_async along with the existing formats/modifier list
> > should be sufficient.
> > In case of async for the struct drm_format_modifier_blob the elements
> > format_offset includes the list of formats supported by async only. For this we
> > will need to have the static list. So can passing this list be done by adding new
> > elements in drm_plane specifically for async.
> 
> Just to add to Arun's point. We had this discussion on thread [1].
> 
> If we populate struct drm_format_modifier_blob for async using just the existing format/modifier lists in struct drm_plane,
> We will be mis-representing the following members of the structure to the user space
> 
> struct drm_format_modifier_blob {
>         /* Number of fourcc formats supported */
>         __u32 count_formats;
> 
>        /* Number of drm_format_modifiers */
>         __u32 count_modifiers;
> };

Nothing is misrepresentd. Those things just tell you what the bits in
the bimask mean.

> 
> However, as you correctly point out, it should still work because of the format-modifier bit mask.
> But it leaves the overall blob unnecessarily bloated (for example, with unnecessary entries in the format list).
> 
> We could however change the function in such a way that the loop
> 
> for (i = 0; i < modifier_count; i++) {
>                 for (j = 0; j < format_count; j++)
> 
> runs before filling up these members.
> 
> I am not sure how much juggling that would take but it could be a solution.
> 
> Anything you would suggest?

You're complicating things needlessly. The slightly bloated blob
should make no practical difference anywhere.

-- 
Ville Syrjälä
Intel

RE: [PATCH v3 2/5] drm/plane: Expose function to create format/modifier blob

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, January 28, 2025 12:44 AM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Subject: Re: [PATCH v3 2/5] drm/plane: Expose function to create
> format/modifier blob
> 
> On Mon, Jan 27, 2025 at 07:25:31AM +0000, Borah, Chaitanya Kumar wrote:
> > Hello Ville,
> >
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Murthy, Arun R
> > > Sent: Saturday, January 25, 2025 12:25 PM
> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: dri-devel@lists.freedesktop.org;
> > > intel-gfx@lists.freedesktop.org; intel- xe@lists.freedesktop.org
> > > Subject: RE: [PATCH v3 2/5] drm/plane: Expose function to create
> > > format/modifier blob
> > >
> > > > On Thu, Jan 23, 2025 at 07:47:14AM +0000, Murthy, Arun R wrote:
> > > > > > > On Wed, Jan 08, 2025 at 11:09:00AM +0530, Arun R Murthy wrote:
> > > > > > > > Expose drm plane function to create formats/modifiers blob.
> > > > > > > > This function can be used to expose list of supported
> > > > > > > > formats/modifiers for sync/async flips.
> > > > > > > >
> > > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/drm_plane.c | 44
> > > > > > > > +++++++++++++++++++++++++++++-------
> > > > > > > --------
> > > > > > > >  include/drm/drm_plane.h     |  4 ++++
> > > > > > > >  2 files changed, 33 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > > > > b/drivers/gpu/drm/drm_plane.c index
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> 416818e54ccffcf3d3aada2723e96ce8ccf1dd97..4f35eec2b7770fcc90c3e07a90
> > > > > > 6
> > > > > > > 8
> > > > > > > > b31c0563a4c0 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > > > @@ -191,7 +191,10 @@ modifiers_ptr(struct
> > > > > > > > drm_format_modifier_blob
> > > > > > > *blob)
> > > > > > > >  	return (struct drm_format_modifier *)(((char *)blob) +
> > > > > > > > blob->modifiers_offset);  }
> > > > > > > >
> > > > > > > > -static int create_in_format_blob(struct drm_device *dev,
> > > > > > > > struct drm_plane *plane)
> > > > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > > > +				 struct drm_plane *plane, u64
> > > > *modifiers,
> > > > > > > > +				 unsigned int modifier_count, u32
> > > > *formats,
> > > > > > > > +				 unsigned int format_count, bool
> > > > is_async)
> > > > > > > >  {
> > > > > > > >  	const struct drm_mode_config *config = &dev->mode_config;
> > > > > > > >  	struct drm_property_blob *blob; @@ -200,14 +203,14 @@
> > > static
> > > > > > > > int create_in_format_blob(struct drm_device
> > > > > > > *dev, struct drm_plane *plane
> > > > > > > >  	struct drm_format_modifier_blob *blob_data;
> > > > > > > >  	unsigned int i, j;
> > > > > > > >
> > > > > > > > -	formats_size = sizeof(__u32) * plane->format_count;
> > > > > > > > +	formats_size = sizeof(__u32) * format_count;
> > > > > > > >  	if (WARN_ON(!formats_size)) {
> > > > > > > >  		/* 0 formats are never expected */
> > > > > > > >  		return 0;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > >  	modifiers_size =
> > > > > > > > -		sizeof(struct drm_format_modifier) * plane-
> > > >modifier_count;
> > > > > > > > +		sizeof(struct drm_format_modifier) * modifier_count;
> > > > > > > >
> > > > > > > >  	blob_size = sizeof(struct drm_format_modifier_blob);
> > > > > > > >  	/* Modifiers offset is a pointer to a struct with a 64
> > > > > > > > bit field so it @@ -223,37 +226,45 @@ static int
> > > > > > > > create_in_format_blob(struct drm_device *dev, struct
> > > > > > > > drm_plane *plane
> > > > > > > >
> > > > > > > >  	blob_data = blob->data;
> > > > > > > >  	blob_data->version = FORMAT_BLOB_CURRENT;
> > > > > > > > -	blob_data->count_formats = plane->format_count;
> > > > > > > > +	blob_data->count_formats = format_count;
> > > > > > > >  	blob_data->formats_offset = sizeof(struct
> > > drm_format_modifier_blob);
> > > > > > > > -	blob_data->count_modifiers = plane->modifier_count;
> > > > > > > > +	blob_data->count_modifiers = modifier_count;
> > > > > > > >
> > > > > > > >  	blob_data->modifiers_offset =
> > > > > > > >  		ALIGN(blob_data->formats_offset + formats_size, 8);
> > > > > > > >
> > > > > > > > -	memcpy(formats_ptr(blob_data), plane->format_types,
> > > formats_size);
> > > > > > > > +	memcpy(formats_ptr(blob_data), formats, formats_size);
> > > > > > > >
> > > > > > > >  	mod = modifiers_ptr(blob_data);
> > > > > > > > -	for (i = 0; i < plane->modifier_count; i++) {
> > > > > > > > -		for (j = 0; j < plane->format_count; j++) {
> > > > > > > > -			if (!plane->funcs->format_mod_supported ||
> > > > > > > > +	for (i = 0; i < modifier_count; i++) {
> > > > > > > > +		for (j = 0; j < format_count; j++) {
> > > > > > > > +			if (is_async ||
> > > > > > > > +			    !plane->funcs->format_mod_supported ||
> > > > > > > >  			    plane->funcs-
> > > >format_mod_supported(plane,
> > > > > > > > -							       plane-
> > > > > > > >format_types[j],
> > > > > > > > -							       plane-
> > > > > > > >modifiers[i])) {
> > > > > > > > +							       formats[j],
> > > > > > > > +
> > > > modifiers[i])) {
> > > > > > > >  				mod->formats |= 1ULL << j;
> > > > > > > >  			}
> > > > > > > >  		}
> > > > > > > >
> > > > > > > > -		mod->modifier = plane->modifiers[i];
> > > > > > > > +		mod->modifier = modifiers[i];
> > > > > > > >  		mod->offset = 0;
> > > > > > > >  		mod->pad = 0;
> > > > > > > >  		mod++;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > -	drm_object_attach_property(&plane->base, config-
> > > > > > > >modifiers_property,
> > > > > > > > -				   blob->base.id);
> > > > > > > > +	if (is_async)
> > > > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > > > +					   config-
> > > > >async_modifiers_property,
> > > > > > > > +					   blob->base.id);
> > > > > > > > +	else
> > > > > > > > +		drm_object_attach_property(&plane->base,
> > > > > > > > +					   config->modifiers_property,
> > > > > > > > +					   blob->base.id);
> > > > > > >
> > > > > > > IMO the function should only create the blob. Leave it to
> > > > > > > the caller to attach
> > > > > > it.
> > > > > > >
> > > > > > Prior to this change for sync formats/modifiers the property
> > > > > > attach was also done in the same function. So retained it as it.
> > > > > >
> > > > > > > The 'is_async' parameter could also be replaced with with a
> > > > > > > function pointer to the appropriate format_mod_supported()
> > > > > > > function. That makes the implementation entirely generic.
> > > > > > >
> > > > > > If the list of formats/modifiers for sync and async is passed
> > > > > > to this function, then based on the list the corresponding
> > > > > > function pointer can
> > > > be called.
> > > > > > This was done in the earlier patchset.
> > > > > >
> > > > > > > >
> > > > > > > >  	return 0;
> > > > > > > >  }
> > > > > > > > +EXPORT_SYMBOL(drm_plane_create_format_blob);
> > > > > > > >
> > > > > > > >  /**
> > > > > > > >   * DOC: hotspot properties @@ -476,7 +487,10 @@ static
> > > > > > > > int __drm_universal_plane_init(struct
> > > > > > > drm_device *dev,
> > > > > > > >  	}
> > > > > > > >
> > > > > > > >  	if (format_modifier_count)
> > > > > > > > -		create_in_format_blob(dev, plane);
> > > > > > > > +		drm_plane_create_format_blob(dev, plane, plane-
> > > > >modifiers,
> > > > > > > > +					     format_modifier_count,
> > > > > > > > +					     plane->format_types,
> > > > > > > format_count,
> > > > > > > > +					     false);
> > > > > > > >
> > > > > > > >  	return 0;
> > > > > > > >  }
> > > > > > > > diff --git a/include/drm/drm_plane.h
> > > > > > > > b/include/drm/drm_plane.h index
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> e8749e6fc3bc0acfc73bbd8401f85c3126e86759..eb84830fbb723e39436bfbadf
> > > > > > > 369
> > > > > > > > 894a5657cd45 100644
> > > > > > > > --- a/include/drm/drm_plane.h
> > > > > > > > +++ b/include/drm/drm_plane.h
> > > > > > > > @@ -1008,5 +1008,9 @@ int
> > > > > > > > drm_plane_create_scaling_filter_property(struct drm_plane
> > > > > > > > *plane, int
> > > > > > > drm_plane_add_size_hints_property(struct drm_plane *plane,
> > > > > > > >  				      const struct drm_plane_size_hint
> > > *hints,
> > > > > > > >  				      int num_hints);
> > > > > > > > +int drm_plane_create_format_blob(struct drm_device *dev,
> > > > > > > > +				 struct drm_plane *plane, u64
> > > > *modifiers,
> > > > > > > > +				 unsigned int modifier_count, u32
> > > > *formats,
> > > > > > > > +				 unsigned int format_count, bool
> > > > is_async);
> > > > > > >
> > > > > > > I don't think this needs to be exposed to anyone.
> > > > > > > __drm_universal_plane_init() should just populate both the
> > > > > > > normal blob, and and the async blob (iff
> > > > > > > .format_mod_supported_async() is provided).
> > > > > > >
> > > > > > Ok!
> > > > > >
> > > > > For __drm_universal_plane_init() to have both the sync/async
> > > > format/modifiers list we will have to add new elements to struct
> > > > drm_plane to hold the async formats/modifiers.
> > > >
> > > > No, you can just use the already existing format/modifier lists.
> > > > .format_mod_supported_async() will filter out what's not wanted.
> > > >
> > > Agree, to populate the struct drm_format_modifier
> > > .format_mod_supported_async along with the existing formats/modifier
> > > list should be sufficient.
> > > In case of async for the struct drm_format_modifier_blob the
> > > elements format_offset includes the list of formats supported by
> > > async only. For this we will need to have the static list. So can
> > > passing this list be done by adding new elements in drm_plane specifically
> for async.
> >
> > Just to add to Arun's point. We had this discussion on thread [1].
> >
> > If we populate struct drm_format_modifier_blob for async using just
> > the existing format/modifier lists in struct drm_plane, We will be
> > mis-representing the following members of the structure to the user
> > space
> >
> > struct drm_format_modifier_blob {
> >         /* Number of fourcc formats supported */
> >         __u32 count_formats;
> >
> >        /* Number of drm_format_modifiers */
> >         __u32 count_modifiers;
> > };
> 
> Nothing is misrepresentd. Those things just tell you what the bits in the
> bimask mean.
> 

At the very least, it makes the comments inaccurate. But perhaps something we can live with.

> >
> > However, as you correctly point out, it should still work because of the
> format-modifier bit mask.
> > But it leaves the overall blob unnecessarily bloated (for example, with
> unnecessary entries in the format list).
> >
> > We could however change the function in such a way that the loop
> >
> > for (i = 0; i < modifier_count; i++) {
> >                 for (j = 0; j < format_count; j++)
> >
> > runs before filling up these members.
> >
> > I am not sure how much juggling that would take but it could be a solution.
> >
> > Anything you would suggest?
> 
> You're complicating things needlessly. The slightly bloated blob should make
> no practical difference anywhere.
> 

I agree, if bloated blob is not a concern then we can re-use the existing struct drm_plane members and keep the function more or less the same.

Regards

Chaitanya

> --
> Ville Syrjälä
> Intel

RE: [PATCH v3 1/5] drm/plane: Add new plane property IN_FORMATS_ASYNC

[Kumar, Naveen1]

Hi,

I have tested revision 4 of the patch series on my DG2 setup with drm_info & Mutter MR https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4063. The patches apply cleanly, and the new uapi IN_FORMATS_ASYNC works fine. Though I had some observation whether the unsupported Async modifiers should even be exposed to userspace or not? So far, it does not populate the list of supported formats for these modifiers and Mutter is able to handle this scenario as it ignores these modifiers as there are no formats exposed. Apart from this, it works as expected and LGTM.  

Tested-by: Naveen Kumar <naveen1.kumar@intel.com>

Best regards,
Naveen Kumar

>-----Original Message-----
>From: Murthy, Arun R <arun.r.murthy@intel.com>
>Sent: Thursday, January 16, 2025 3:24 PM
>To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; dri-
>devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
>xe@lists.freedesktop.org
>Cc: Kumar, Naveen1 <naveen1.kumar@intel.com>
>Subject: RE: [PATCH v3 1/5] drm/plane: Add new plane property
>IN_FORMATS_ASYNC
>
>> > > > -----Original Message-----
>> > > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On
>> > > > Behalf Of Arun R Murthy
>> > > > Sent: Wednesday, January 8, 2025 11:09 AM
>> > > > To: dri-devel@lists.freedesktop.org;
>> > > > intel-gfx@lists.freedesktop.org;
>> > > > intel- xe@lists.freedesktop.org
>> > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
>> > > > 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@intel.com>
>> > > > ---
>> > > >  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..dbbef20753f834a85ae9d
>ed
>> > > > 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..416818e54ccffcf3d3aada
>27
>> > > > 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.
>>
>As documented this an optional property with this property user will have the
>feasibility to check for supported one and if not might end up with failure
>commit due to unsupported formats.
>Will reframe this to avoid confusion!
>
>> > > >   * 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..0c116d6dfd277262b1a4c
>0f0
>> > > > 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..e8749e6fc3bc0acfc73bbd8
>40
>> > > > 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.
>>
>Ok sure will be more specific.
>
>> 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.
>>
>That cannot be the case, as this is an optional and not that all the drivers will
>have this property. This is a good to have to avoid failure due to unsupported
>format/modifier.
>
>Thanks and Regards,
>Arun R Murthy
>-------------------