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

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.

On Wed, Jul 29, 2020 at 01:40:13PM +1000, Ben Skeggs
wrote:
> 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.
JFYI, the adjusted (moved into != MOD_LINEAR case) patch works fine for me
on top of drm-fixes-2020-07-29.

-- 
 Kirill A. Shutemov

On 7/29/20 7:47 AM, Kirill A. Shutemov wrote:
> On Wed, Jul 29, 2020 at 01:40:13PM +1000, Ben Skeggs wrote:
>> 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.
> 
> JFYI, the adjusted (moved into != MOD_LINEAR case) patch works fine for me
> on top of drm-fixes-2020-07-29.
Sorry again for the delays (life is terrible lately), but the signed-off 
version with Ben's suggestion went out this morning, and I specifically 
tested linear modifiers in addition to retesting all the other test 
cases mentioned in the patch.

Thanks,
-James