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