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

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);
+	}
+
 	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

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.
> +
>         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 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