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

[Murthy, Arun R]

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

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

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

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

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

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