[Nouveau] [PATCH] drm/nouveau: Accept 'legacy' format modifiers

[Daniel Vetter]

On Sat, Jul 18, 2020 at 5:43 AM James Jones <jajones at nvidia.com> wrote:
>
> On 7/17/20 12:47 PM, Daniel Vetter wrote:
> > On Fri, Jul 17, 2020 at 11:57:57AM -0700, James Jones wrote:
> >> Accept the DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK()
> >> family of modifiers to handle broken userspace
> >> Xorg modesetting and Mesa drivers.
> >>
> >> Tested with Xorg 1.20 modesetting driver,
> >> weston at c46c70dac84a4b3030cd05b380f9f410536690fc,
> >> gnome & KDE wayland desktops from Ubuntu 18.04,
> >> and sway 1.5
> >
> > Just bikeshed, but maybe a few more words on what exactly is broken and
> > how this works around it. Specifically why we only accept these, but don't
> > advertise them.
>
> Added quite a few words.
>
> >>
> >> Signed-off-by: James Jones <jajones at nvidia.com>
> >
> > Needs Fixes: line here. Also nice to mention the bug reporter/link.
>
> Done in v2.
>
> >> ---
> >>   drivers/gpu/drm/nouveau/nouveau_display.c | 26 +++++++++++++++++++++--
> >>   1 file changed, 24 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> >> index 496c4621cc78..31543086254b 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> >> @@ -191,8 +191,14 @@ nouveau_decode_mod(struct nouveau_drm *drm,
> >>                 uint32_t *tile_mode,
> >>                 uint8_t *kind)
> >>   {
> >> +    struct nouveau_display *disp = nouveau_display(drm->dev);
> >>      BUG_ON(!tile_mode || !kind);
> >>
> >> +    if ((modifier & (0xffull << 12)) == 0ull) {
> >> +            /* Legacy modifier.  Translate to this device's 'kind.' */
> >> +            modifier |= disp->format_modifiers[0] & (0xffull << 12);
> >> +    }
> >
> > Hm I tried to understand what this magic does by looking at drm_fourcc.h,
> > but the drm_fourcc_canonicalize_nvidia_format_mod() in there implements
> > something else. Is that function wrong, or should we use it here instead?
> >
>  > Or is there something else going on entirely?
>
> This may be slightly clearer with the expanded change description:
>
> Canonicalize assumes the old modifiers are only used by certain Tegra
> revisions, because the Mesa patches were supposed to land and obliterate
> all uses beyond that.  That assumption means it can assume the specific
> page kind (0xfe) used by the display-engine-compatible layout on those
> specific devices.  There is no way to generally canonicalize a legacy
> modifier without referencing a specific device type, as is indirectly
> done here.
>
> This code does a limited device-specific canonicalization: It
> substitutes the display-appropriate page kind used by this specific
> device, ensuring we derive this correct page kind later in the function.
>   I iterated on the best way to accomplish this a few times, and this
> was the least-invasive thing I came up with, but it does require a
> pretty thorough understanding of the NVIDIA modifier macros.
>
> Thanks for the quick review.

Ah yes this makes a ton more sense with your explanation of what's all
going on. Thanks for all the explaining, but probably better if
someone with real nouveau clues takes a look too. Fwiw (i.e. not much)

Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Cheers, Daniel

>
> -James
>
> >
> > Cheers, Daniel
> >
> >> +
> >>      if (modifier == DRM_FORMAT_MOD_LINEAR) {
> >>              /* tile_mode will not be used in this case */
> >>              *tile_mode = 0;
> >> @@ -227,6 +233,16 @@ nouveau_framebuffer_get_layout(struct drm_framebuffer *fb,
> >>      }
> >>   }
> >>
> >> +static const u64 legacy_modifiers[] = {
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(0),
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(1),
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(2),
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(3),
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(4),
> >> +    DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(5),
> >> +    DRM_FORMAT_MOD_INVALID
> >> +};
> >> +
> >>   static int
> >>   nouveau_validate_decode_mod(struct nouveau_drm *drm,
> >>                          uint64_t modifier,
> >> @@ -247,8 +263,14 @@ nouveau_validate_decode_mod(struct nouveau_drm *drm,
> >>           (disp->format_modifiers[mod] != modifier);
> >>           mod++);
> >>
> >> -    if (disp->format_modifiers[mod] == DRM_FORMAT_MOD_INVALID)
> >> -            return -EINVAL;
> >> +    if (disp->format_modifiers[mod] == DRM_FORMAT_MOD_INVALID) {
> >> +            for (mod = 0;
> >> +                 (legacy_modifiers[mod] != DRM_FORMAT_MOD_INVALID) &&
> >> +                 (legacy_modifiers[mod] != modifier);
> >> +                 mod++);
> >> +            if (legacy_modifiers[mod] == DRM_FORMAT_MOD_INVALID)
> >> +                    return -EINVAL;
> >> +    }
> >>
> >>      nouveau_decode_mod(drm, modifier, tile_mode, kind);
> >>
> >> --
> >> 2.17.1
> >>
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch