Re: [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS

From: Daniel Vetter <daniel@ffwll.ch>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>,
	David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Maxime Ripard <maxime@cerno.tech>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
Date: Tue, 27 Apr 2021 14:22:07 +0200	[thread overview]
Message-ID: <YIgB76WmQijHCJeV@phenom.ffwll.local> (raw)
In-Reply-To: <CACvgo51rQJmHc1K-MSq-WLZkwVt34MY73csgEyxorrYsKPwQiA@mail.gmail.com>

On Tue, Apr 27, 2021 at 12:32:19PM +0100, Emil Velikov wrote:
> Hi Daniel,
> 
> On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > @@ -360,6 +373,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.
> > + *
> The comment says "must", yet we have an "if (format_modifiers)" in the codebase.
> Shouldn't we add a WARN_ON() + return -EINVAL (or similar) so people
> can see and fix their drivers?

This is a must only for drivers supporting modifiers, not all drivers.
Hence the check in the if. I did add WARN_ON for the combos that get stuff
wrong though (like only supply one side of the modifier info, not both).

> As a follow-up one could even go a step further, by erroring out when
> the driver hasn't provided valid modifier(s) and even removing
> config::allow_fb_modifiers all together.

Well that currently only exists to avoid walking the plane list (which we
need to do for validation that all planes are the same). It's quite tricky
code for tiny benefit, so I don't think it's worth it trying to remove
allow_fb_modifiers completely.

> Although for stable - this series + WARN_ON (no return since it might
> break buggy drivers) sounds good.
> 
> > @@ -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:
> >          *
> The new note and the existing IMPORTANT are in a weird mix.
> Quoting the latter since it doesn't show in the diff.
> 
> If this is set the driver must fill out the full implicit modifier
> information in their &drm_mode_config_funcs.fb_create hook for legacy
> userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> broken for modifier aware userspace.
> 
> In particular:
> As the new note says "don't set it" and the existing note one says "if
> it's set". Yet no drivers do "if (config->allow_fb_modifiers)".
> 
> Sadly, nothing comes to mind atm wrt alternative wording.

Yeah it's a bit disappointing.

> With the WARN_ON() added or s/must/should/ in the documentation, the series is:

With my clarification, can you please recheck whether as-is it's not
correct?

Thanks, Daniel

> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> HTH
> -Emil

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