On Sat, Jul 18, 2020 at 5:43 AM James Jones <jajones@xxxxxxxxxx> 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@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@xxxxxxxxxx>
> >
> > 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@xxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
