[PATCH] drm/doc: Document that modifiers are always required for fb

[Daniel Vetter]

On Mon, Sep 7, 2020 at 11:07 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Mon, 7 Sep 2020 10:41:37 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
>
> > On Mon, Sep 07, 2020 at 08:37:31AM +0000, Simon Ser wrote:
> > > On Monday, September 7, 2020 10:31 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > > On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
> > > >
> > > > > On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter daniel.vetter at ffwll.ch wrote:
> > > > >
> > > > > > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact at emersion.fr wrote:
> > > > > >
> > > > > > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter at ffwll.ch wrote:
> > > > > > >
> > > > > > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > > > > > something already documented?
> > > > > > > >
> > > > > > > > I don't think that happens, but it has come up in discussions. It's
> > > > > > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > > > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > > > > > a legit reason for mixing modifier-aware userspace with
> > > > > > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > > > > > like that everything works with modifiers consistently, including
> > > > > > > > getfb2. But gbm is just within a single process, and that should
> > > > > > > > either run all with modifiers, or not at all, since these worlds just
> > > > > > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > > > > > being a confused mess nonwithstanding :-)
> > > > > > >
> > > > > > > Well… There's also the case where some legacy Wayland client talks to a
> > > > > > > modifier-aware compositor. gbm_bo_import would be called without a
> > > > > > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > > > > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > > > > > the driver pick "safe" modifiers in case passing the full list of
> > > > > > > modifiers results in a black screen. Later on wlroots will call
> > > > > > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> > > > > >
> > > > > > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > > > > > should figure out the right modifiers (and I think it does that, at
> > > > > > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > > > > > require that.
> > > > >
> > > > > I guess the compositor will just forward the value returned by
> > > > > gbm_bo_get_modifier in any case, so returning INVALID would be fine
> > > > > too (to mean "implicit modifier").
> > > > > In both the create and import cases, other metadata like pitches and
> > > > > offsets should be correctly set I think?
> > > >
> > > > Well if you have a modifier format underneath, the non-modifiered offsets
> > > > and pitches might be pure fiction. Also, they might not be sufficient, if
> > > > the modifier adds more planes.
> > >
> > > In this case (gbm_bo_create without modifiers), we're discussing
> > > whether we require gbm_bo_get_modifier to return a valid modifier, or
> > > if INVALID is fine.
> >
> > Hm then I missed the use-case for a gbm_bo_create without modifiers, where
> > afterwards userspace wants the modifiers. That sounds like a bug (and yes
> > -modesetting is buggy that way).
>
> I'm guessing that use case might be related to
> https://gitlab.freedesktop.org/wayland/weston/-/issues/429
>
> The weston issue is about
> gbm_surface_create/gbm_surface_create_with_modifiers, but that's not
> too different from gbm_bo_create/gbm_bo_create_with_modifiers?
>
> Weston happens to have this code:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/drm-gbm.c#L209-230
> and then it unconditionally calls gbm_bo_get_modifier(). However,
> DRM_FORMAT_MOD_INVALID is handled specially:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/fb.c#L80-97

Hm yeah that feels a bit like an interim hack instead of more
modifiers fallback logic. I guess shouldn't be too tricky for mesa to
support that, since internally modifier aware drivers work only with
modifiers anyway (or at least should, that's what we're requiring on
the kms side with this patch at least).

Up to mesa people I'd say.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch