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

Commit Message

Arun R Murthy Jan. 8, 2025, 5:39 a.m. UTC

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 <[e-mail]>
---
 drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++++++++++---------------
 include/drm/drm_plane.h     |  4 ++++
 2 files changed, 33 insertions(+), 15 deletions(-)

Comments

Borah, Chaitanya Kumar Jan. 12, 2025, 8 a.m. UTC | #1

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Arun
> R Murthy
> Sent: Wednesday, January 8, 2025 11:09 AM
> To: [e-mail]; [e-mail]; intel-
> [e-mail]
> Cc: Murthy, Arun R <[e-mail]>
> 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 <[e-mail]>
> ---
>  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

Arun R Murthy Jan. 13, 2025, 8:22 a.m. UTC | #2

> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of
> > Arun R Murthy
> > Sent: Wednesday, January 8, 2025 11:09 AM
> > To: [e-mail]; [e-mail];
> > intel- [e-mail]
> > Cc: Murthy, Arun R <[e-mail]>
> > 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 <[e-mail]>
> > ---
> >  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
--------------------

Borah, Chaitanya Kumar Jan. 13, 2025, 6:44 p.m. UTC | #3

> -----Original Message-----
> From: Murthy, Arun R <[e-mail]>
> Sent: Monday, January 13, 2025 1:52 PM
> To: Borah, Chaitanya Kumar <[e-mail]>; dri-
> [e-mail]; [e-mail]; intel-
> [e-mail]
> Subject: RE: [PATCH v3 2/5] drm/plane: Expose function to create
> format/modifier blob
> 
> > > -----Original Message-----
> > > From: Intel-gfx <[e-mail]> On Behalf
> > > Of Arun R Murthy
> > > Sent: Wednesday, January 8, 2025 11:09 AM
> > > To: [e-mail];
> > > [e-mail];
> > > intel- [e-mail]
> > > Cc: Murthy, Arun R <[e-mail]>
> > > 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 <[e-mail]>
> > > ---
> > >  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
> --------------------

Arun R Murthy Jan. 16, 2025, 9:54 a.m. UTC | #4

> > > > 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
-------------------

Ville Syrjälä Jan. 20, 2025, 8:42 p.m. UTC | #5

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 <[e-mail]>
> ---
>  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

Arun R Murthy Jan. 22, 2025, 9:27 a.m. UTC | #6

> 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 <[e-mail]>
> > ---
> >  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
--------------------

Arun R Murthy Jan. 23, 2025, 7:47 a.m. UTC | #7

> > 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 <[e-mail]>
> > > ---
> > >  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
--------------------

Ville Syrjälä Jan. 24, 2025, 11:25 a.m. UTC | #8

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 <[e-mail]>
> > > > ---
> > > >  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
> --------------------

Arun R Murthy Jan. 25, 2025, 6:55 a.m. UTC | #9

> 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 <[e-mail]>
> > > > > ---
> > > > >  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

Borah, Chaitanya Kumar Jan. 27, 2025, 7:25 a.m. UTC | #10

Hello Ville,

> -----Original Message-----
> From: Intel-xe <[e-mail]> On Behalf Of Murthy,
> Arun R
> Sent: Saturday, January 25, 2025 12:25 PM
> To: Ville Syrjälä <[e-mail]>
> Cc: [e-mail]; [e-mail]; intel-
> [e-mail]
> 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 <[e-mail]>
> > > > > > ---
> > > > > >  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/[e-mail]/

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

Ville Syrjälä Jan. 27, 2025, 7:13 p.m. UTC | #11

On Mon, Jan 27, 2025 at 07:25:31AM +0000, Borah, Chaitanya Kumar wrote:
> Hello Ville,
> 
> > -----Original Message-----
> > From: Intel-xe <[e-mail]> On Behalf Of Murthy,
> > Arun R
> > Sent: Saturday, January 25, 2025 12:25 PM
> > To: Ville Syrjälä <[e-mail]>
> > Cc: [e-mail]; [e-mail]; intel-
> > [e-mail]
> > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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.

Borah, Chaitanya Kumar Jan. 28, 2025, 5:32 a.m. UTC | #12

> -----Original Message-----
> From: Ville Syrjälä <[e-mail]>
> Sent: Tuesday, January 28, 2025 12:44 AM
> To: Borah, Chaitanya Kumar <[e-mail]>
> Cc: Murthy, Arun R <[e-mail]>; dri-
> [e-mail]; [e-mail]; intel-
> [e-mail]
> 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 <[e-mail]> On Behalf Of
> > > Murthy, Arun R
> > > Sent: Saturday, January 25, 2025 12:25 PM
> > > To: Ville Syrjälä <[e-mail]>
> > > Cc: [e-mail];
> > > [e-mail]; intel- [e-mail]
> > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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

Patch

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