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