Nouveau - Jul 2020 - [PATCH v2] drm/nouveau: Accept 'legacy' format modifiers

On 7/23/20 9:06 PM, Ben Skeggs wrote:
> On Sat, 18 Jul 2020 at 13:34, James Jones <jajones at nvidia.com>
wrote:
>>
>> Accept the DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK()
>> family of modifiers to handle broken userspace
>> Xorg modesetting and Mesa drivers. Existing Mesa
>> drivers are still aware of only these older
>> format modifiers which do not differentiate
>> between different variations of the block linear
>> layout. When the format modifier support flag was
>> flipped in the nouveau kernel driver, the X.org
>> modesetting driver began attempting to use its
>> format modifier-enabled framebuffer path. Because
>> the set of format modifiers advertised by the
>> kernel prior to this change do not intersect with
>> the set of format modifiers advertised by Mesa,
>> allocating GBM buffers using format modifiers
>> fails and the modesetting driver falls back to
>> non-modifier allocation. However, it still later
>> queries the modifier of the GBM buffer when
>> creating its DRM-KMS framebuffer object, receives
>> the old-format modifier from Mesa, and attempts
>> to create a framebuffer with it. Since the kernel
>> is still not aware of these formats, this fails.
>>
>> Userspace should not be attempting to query format
>> modifiers of GBM buffers allocated with a non-
>> format-modifier-aware allocation path, but to
>> avoid breaking existing userspace behavior, this
>> change accepts the old-style format modifiers when
>> creating framebuffers and applying them to planes
>> by translating them to the equivalent new-style
>> modifier. To accomplish this, some layout
>> parameters must be assumed to match properties of
>> the device targeted by the relevant ioctls. To
>> avoid perpetuating misuse of the old-style
>> modifiers, this change does not advertise support
>> for them. Doing so would imply compatibility
>> between devices with incompatible memory layouts.
>>
>> Tested with Xorg 1.20 modesetting driver,
>> weston at c46c70dac84a4b3030cd05b380f9f410536690fc,
>> gnome & KDE wayland desktops from Ubuntu 18.04,
>> and sway 1.5
>>
>> Reported-by: Kirill A. Shutemov <kirill at shutemov.name>
>> Fixes: fa4f4c213f5f ("drm/nouveau/kms: Support NVIDIA format
modifiers")
>> Link: https://lkml.org/lkml/2020/6/30/1251
>> Signed-off-by: James Jones <jajones at nvidia.com>
>> ---
>>   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);
>> +       }
> I believe this should be moved into the != MOD_LINEAR case.
Yes, of course, thanks.  I need to re-evaluate my testing yet again to 
make sure I hit that case too.  Preparing a v3...

Thanks,
-James
>> +
>>          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
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau

On Tue, 28 Jul 2020 at 04:51, James Jones <jajones at nvidia.com>
wrote:
>
> On 7/23/20 9:06 PM, Ben Skeggs wrote:
> > On Sat, 18 Jul 2020 at 13:34, James Jones <jajones at
nvidia.com> wrote:
> >>
> >> Accept the DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK()
> >> family of modifiers to handle broken userspace
> >> Xorg modesetting and Mesa drivers. Existing Mesa
> >> drivers are still aware of only these older
> >> format modifiers which do not differentiate
> >> between different variations of the block linear
> >> layout. When the format modifier support flag was
> >> flipped in the nouveau kernel driver, the X.org
> >> modesetting driver began attempting to use its
> >> format modifier-enabled framebuffer path. Because
> >> the set of format modifiers advertised by the
> >> kernel prior to this change do not intersect with
> >> the set of format modifiers advertised by Mesa,
> >> allocating GBM buffers using format modifiers
> >> fails and the modesetting driver falls back to
> >> non-modifier allocation. However, it still later
> >> queries the modifier of the GBM buffer when
> >> creating its DRM-KMS framebuffer object, receives
> >> the old-format modifier from Mesa, and attempts
> >> to create a framebuffer with it. Since the kernel
> >> is still not aware of these formats, this fails.
> >>
> >> Userspace should not be attempting to query format
> >> modifiers of GBM buffers allocated with a non-
> >> format-modifier-aware allocation path, but to
> >> avoid breaking existing userspace behavior, this
> >> change accepts the old-style format modifiers when
> >> creating framebuffers and applying them to planes
> >> by translating them to the equivalent new-style
> >> modifier. To accomplish this, some layout
> >> parameters must be assumed to match properties of
> >> the device targeted by the relevant ioctls. To
> >> avoid perpetuating misuse of the old-style
> >> modifiers, this change does not advertise support
> >> for them. Doing so would imply compatibility
> >> between devices with incompatible memory layouts.
> >>
> >> Tested with Xorg 1.20 modesetting driver,
> >> weston at c46c70dac84a4b3030cd05b380f9f410536690fc,
> >> gnome & KDE wayland desktops from Ubuntu 18.04,
> >> and sway 1.5
> >>
> >> Reported-by: Kirill A. Shutemov <kirill at shutemov.name>
> >> Fixes: fa4f4c213f5f ("drm/nouveau/kms: Support NVIDIA format
modifiers")
> >> Link: https://lkml.org/lkml/2020/6/30/1251
> >> Signed-off-by: James Jones <jajones at nvidia.com>
> >> ---
> >>   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);
> >> +       }
> > I believe this should be moved into the != MOD_LINEAR case.
>
> Yes, of course, thanks.  I need to re-evaluate my testing yet again to
> make sure I hit that case too.  Preparing a v3...
Going to need something here in the next day, two max.

Linus may wait for another week, but it's not guaranteed.

Dave.

On Wed, 29 Jul 2020 at 12:48, Dave Airlie <airlied at gmail.com>
wrote:
>
> On Tue, 28 Jul 2020 at 04:51, James Jones <jajones at nvidia.com>
wrote:
> >
> > On 7/23/20 9:06 PM, Ben Skeggs wrote:
> > > On Sat, 18 Jul 2020 at 13:34, James Jones <jajones at
nvidia.com> wrote:
> > >>
> > >> Accept the DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK()
> > >> family of modifiers to handle broken userspace
> > >> Xorg modesetting and Mesa drivers. Existing Mesa
> > >> drivers are still aware of only these older
> > >> format modifiers which do not differentiate
> > >> between different variations of the block linear
> > >> layout. When the format modifier support flag was
> > >> flipped in the nouveau kernel driver, the X.org
> > >> modesetting driver began attempting to use its
> > >> format modifier-enabled framebuffer path. Because
> > >> the set of format modifiers advertised by the
> > >> kernel prior to this change do not intersect with
> > >> the set of format modifiers advertised by Mesa,
> > >> allocating GBM buffers using format modifiers
> > >> fails and the modesetting driver falls back to
> > >> non-modifier allocation. However, it still later
> > >> queries the modifier of the GBM buffer when
> > >> creating its DRM-KMS framebuffer object, receives
> > >> the old-format modifier from Mesa, and attempts
> > >> to create a framebuffer with it. Since the kernel
> > >> is still not aware of these formats, this fails.
> > >>
> > >> Userspace should not be attempting to query format
> > >> modifiers of GBM buffers allocated with a non-
> > >> format-modifier-aware allocation path, but to
> > >> avoid breaking existing userspace behavior, this
> > >> change accepts the old-style format modifiers when
> > >> creating framebuffers and applying them to planes
> > >> by translating them to the equivalent new-style
> > >> modifier. To accomplish this, some layout
> > >> parameters must be assumed to match properties of
> > >> the device targeted by the relevant ioctls. To
> > >> avoid perpetuating misuse of the old-style
> > >> modifiers, this change does not advertise support
> > >> for them. Doing so would imply compatibility
> > >> between devices with incompatible memory layouts.
> > >>
> > >> Tested with Xorg 1.20 modesetting driver,
> > >> weston at c46c70dac84a4b3030cd05b380f9f410536690fc,
> > >> gnome & KDE wayland desktops from Ubuntu 18.04,
> > >> and sway 1.5
> > >>
> > >> Reported-by: Kirill A. Shutemov <kirill at
shutemov.name>
> > >> Fixes: fa4f4c213f5f ("drm/nouveau/kms: Support NVIDIA
format modifiers")
> > >> Link: https://lkml.org/lkml/2020/6/30/1251
> > >> Signed-off-by: James Jones <jajones at nvidia.com>
> > >> ---
> > >>   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);
> > >> +       }
> > > I believe this should be moved into the != MOD_LINEAR case.
> >
> > Yes, of course, thanks.  I need to re-evaluate my testing yet again to
> > make sure I hit that case too.  Preparing a v3...
>
> Going to need something here in the next day, two max.
>
> Linus may wait for another week, but it's not guaranteed.
I tested a whole bunch of GPUs before sending nouveau's -next tree,
and with the change I suggested to this patch + the other stuff I sent
through -fixes already, things seemed to be in OK shape.

Ben.
>
> Dave.