On Tue, Apr 13, 2021 at 01:54:00PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.04.2021 um 11:49 +0200 schrieb Daniel Vetter:
> > It's very confusing for userspace to have to deal with inconsistencies
> > here, and some drivers screwed this up a bit. Most just ommitted the
> > format list when they meant to say that only linear modifier is
> > allowed, but some also meant that only implied modifiers are
> > acceptable (because actually none of the planes registered supported
> > modifiers).
>
> For a lot of the embedded display drivers that never had any out-of-
> band tiling meta shared with the GPU part, the implied modifier is
> actually DRM_FORMAT_MOD_LINEAR, so maybe that's where some of the
> confusion about needing to specify the modifier list comes from.
Yeah that entire discussion last few days is why I wanted to audit all the
drivers and make sure that going forward we're more explicit&consistent
with all this. There's huge confusion around implied modifier vs "no
IN_FORMATS blob property" and what that exactly means.
-Daniel
> > Now that this is all done consistently across all drivers, document
> > the rules and enforce it in the drm core.
>
> This clarification looks good to me.
>
> Reviewed-by: Lucas Stach <l.st...@pengutronix.de>
>
> > Cc: Pekka Paalanen <pekka.paala...@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Cc: Maxime Ripard <mrip...@kernel.org>
> > Cc: Thomas Zimmermann <tzimmerm...@suse.de>
> > Cc: David Airlie <airl...@linux.ie>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > ---
> > drivers/gpu/drm/drm_plane.c | 16 +++++++++++++++-
> > include/drm/drm_mode_config.h | 2 ++
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 0dd43882fe7c..16a7e3e57f7f 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -128,6 +128,11 @@
> > * pairs supported by this plane. The blob is a struct
> > * drm_format_modifier_blob. Without this property the plane doesn't
> > * support buffers with modifiers. Userspace cannot change this
> > property.
> > + *
> > + * Note that userspace can check the DRM_CAP_ADDFB2_MODIFIERS driver
> > + * capability for general modifier support. If this flag is set then
> > every
> > + * plane will have the IN_FORMATS property, even when it only supports
> > + * DRM_FORMAT_MOD_LINEAR.
> > */
> >
> >
> >
> >
> >
> >
> >
> >
> > static unsigned int drm_num_planes(struct drm_device *dev)
> > @@ -277,8 +282,14 @@ static int __drm_universal_plane_init(struct
> > drm_device *dev,
> > format_modifier_count++;
> > }
> >
> >
> >
> >
> >
> >
> >
> >
> > - if (format_modifier_count)
> > + /* autoset the cap and check for consistency across all planes */
> > + if (format_modifier_count) {
> > + WARN_ON(!config->allow_fb_modifiers &&
> > + !list_empty(&config->plane_list));
> > config->allow_fb_modifiers = true;
> > + } else {
> > + WARN_ON(config->allow_fb_modifiers);
> > + }
> >
> >
> >
> >
> >
> >
> >
> >
> > plane->modifier_count = format_modifier_count;
> > plane->modifiers = kmalloc_array(format_modifier_count,
> > @@ -360,6 +371,9 @@ static int __drm_universal_plane_init(struct drm_device
> > *dev,
> > * drm_universal_plane_init() to let the DRM managed resource
> > infrastructure
> > * take care of cleanup and deallocation.
> > *
> > + * Drivers supporting modifiers must set @format_modifiers on all their
> > planes,
> > + * even those that only support DRM_FORMAT_MOD_LINEAR.
> > + *
> > * Returns:
> > * Zero on success, error code on failure.
> > */
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index ab424ddd7665..1ddf7783fdf7 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -909,6 +909,8 @@ struct drm_mode_config {
> > * @allow_fb_modifiers:
> > *
> > * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > + * Note that drivers should not set this directly, it is automatically
> > + * set in drm_universal_plane_init().
> > *
> > * IMPORTANT:
> > *
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
