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

[Emil Velikov]

Hi Daniel,

Thanks for the extra clarification.

On Tue, 27 Apr 2021 at 13:22, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> 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 at 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).
>
Hmm you're spot on - the arm/malidp patch threw me off for a minute.

> > 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.
>
Pardon if I'm saying something painfully silly - it's been a while
since I've looked closely at KMS.

>From some grepping around, removing ::allow_fb_modifiers would be OK
although it's a secondary goal. It feels like the bigger win will be
simpler modifier handling in DRM.

In particular, one could always "inject" the linear modifier within
drm_universal_plane_init() and always expose DRM_CAP_ADDFB2_MODIFIERS.
Some drivers mxsfb, mgag200, stm and likely others already advertise
the CAP, even though they seemingly lack any modifiers.

The linear/invalid cargo-cult to drm_universal_plane_init() seems
strong and this series adds even more.

Another plus of always exposing the CAP, is that one could mandate (or
nuke) optional .format_mod_supported that you/Ville discussed
earlier[1].
Currently things are weird, since it's required to create IN_FORMAT
blob, yet drivers lack it while simultaneously exposing the CAP to
userspace.

One such example is exynos... Although recently it recently dropped
`allow_fb_modifiers = true` and there are no modifiers passed to
drm_universal_plane_init(), so the CAP is no longer supported.
Inki you might want to check, if that broke your userspace.


Tl:Dr: There _might_ be value in simplifying the modifier handling
_after_ these fixes land.


[1] https://lore.kernel.org/dri-devel/CAKMK7uGNP5us8KFffnPwq7g4b0-B2q-m7deqz_rPHtCrh_qUTw@mail.gmail.com/

> > 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?
>
Indeed - with the series as-is my RB stands.

Thanks
-Emil