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

[Murthy, Arun R]

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

Please let me know if my understanding is wrong!

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

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

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