[PATCH v5 2/3] drm/plane: modify create_in_formats to accommodate async

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Borah, Chaitanya Kumar
> Sent: Thursday, February 13, 2025 3:26 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; dri-
> devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org
> Cc: Syrjala, Ville <ville.syrjala at intel.com>
> Subject: RE: [PATCH v5 2/3] drm/plane: modify create_in_formats to
> accommodate async
> 
> > -----Original Message-----
> > From: Murthy, Arun R <arun.r.murthy at intel.com>
> > Sent: Wednesday, February 12, 2025 9:48 PM
> > To: dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
> > intel- xe at lists.freedesktop.org
> > Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; Syrjala,
> > Ville <ville.syrjala at intel.com>; Murthy, Arun R
> > <arun.r.murthy at intel.com>
> > Subject: [PATCH v5 2/3] drm/plane: modify create_in_formats to
> > accommodate async
> >
> > create_in_formats creates the list of supported format/modifiers for
> > synchronous flips, modify the same function so as to take the
> > format_mod_supported as argument and create list of format/modifier
> > for async as well.
> >
> > v5: create_in_formats can return -ve value in failure case, correct
> > the if condition to check the creation of blob <Chaitanya> Dont add
> > the modifier for which none of the formats is not supported.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy at intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 46
> +++++++++++++++++++++++++++++++++--
> > ----------
> >  1 file changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index
> >
> c9d8871417722186d2b6f87197c9e15a70924b4f..01f67f1f5f29e37b8d0e0793c
> > 58bbe1bba337eb2 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -190,9 +190,12 @@ 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)
> > +static int create_in_format_blob(struct drm_device *dev, struct
> > +drm_plane
> > *plane,
> > +				 bool (*format_mod_supported)
> > +						(struct drm_plane *plane,
> > +						 uint32_t format,
> > +						 uint64_t modifier))
> >  {
> > -	const struct drm_mode_config *config = &dev->mode_config;
> >  	struct drm_property_blob *blob;
> >  	struct drm_format_modifier *mod;
> >  	size_t blob_size, formats_size, modifiers_size; @@ -234,24 +237,26
> > @@ static int create_in_format_blob(struct drm_device *dev, struct
> > drm_plane *plane
> >  	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 ||
> > -			    plane->funcs->format_mod_supported(plane,
> > -							       plane-
> > >format_types[j],
> > -							       plane-
> > >modifiers[i])) {
> > +			if (!format_mod_supported ||
> > format_mod_supported
> > +							(plane,
> > +							 plane-
> > >format_types[j],
> > +							 plane->modifiers[i]))
> > {
> >  				mod->formats |= 1ULL << j;
> >  			}
> >  		}
> >
> > +		if (!mod->formats) {
> > +			mod->modifier = 0;
> > +			blob_data->count_modifiers--;
> > +			continue;
> > +		}
> 
> I don't think change works. 0 represents DRM_FORMAT_MOD_LINEAR.

Relooked into the code, this change will prevent  adding modifiers which do not have any formats supported into the modifier list.
So it is correct in that. Smart idea!
However, the line mod->modifier = 0 is redundant. It will just get overwritten by the next valid modifier.

Regards

Chaitanya

> 
> >  		mod->modifier = plane->modifiers[i];
> >  		mod->offset = 0;
> >  		mod->pad = 0;
> >  		mod++;
> >  	}
> >
> > -	drm_object_attach_property(&plane->base, config-
> > >modifiers_property,
> > -				   blob->base.id);
> > -
> > -	return 0;
> > +	return blob->base.id;
> >  }
> >
> >  /**
> > @@ -368,6 +373,7 @@ static int __drm_universal_plane_init(struct
> > drm_device *dev,
> >  	};
> >  	unsigned int format_modifier_count = 0;
> >  	int ret;
> > +	int blob_id;
> >
> >  	/* plane index is used with 32bit bitmasks */
> >  	if (WARN_ON(config->num_total_plane >= 32)) @@ -474,8 +480,24
> @@
> > static int __drm_universal_plane_init(struct drm_device *dev,
> >  		drm_plane_create_hotspot_properties(plane);
> >  	}
> >
> > -	if (format_modifier_count)
> > -		create_in_format_blob(dev, plane);
> > +	if (format_modifier_count) {
> > +		blob_id = create_in_format_blob(dev, plane,
> > +						plane->funcs-
> > >format_mod_supported);
> > +		if (blob_id > 0)
> > +			drm_object_attach_property(&plane->base,
> > +						   config->modifiers_property,
> > +						   blob_id);
> > +	}
> > +
> > +	if (plane->funcs->format_mod_supported_async) {
> > +		blob_id = create_in_format_blob(dev, plane,
> > +						plane->funcs-
> > >format_mod_supported_async);
> > +		if (blob_id > 0)
> > +			drm_object_attach_property(&plane->base,
> > +						   config-
> > >async_modifiers_property,
> > +						   blob_id);
> > +	}
> > +
> >
> >  	return 0;
> >  }
> >
> > --
> > 2.25.1