Nouveau - Jul 2025 - [RFC 0/4] Add Format Modifiers for NVIDIA Blackwell chipsets

On Thu, Jul 3, 2025 at 6:34?PM James Jones <jajones at nvidia.com> wrote:
> The layout of bits within the individual tiles (referred to as
> sectors in the DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() macro)
> changed for some formats starting in Blackwell 2 GPUs. New format
> modifiers are needed to denote the difference and prevent direct
> sharing of these incompatible buffers with older GPUs.
>
> This patch series proposes first adding some helper macros and
> inline functions to drm_fourcc.h to make the NVIDIA block-linear
> format modifiers easier to work with given the proposed solution
> makes them harder to parse, then extending the existing sector type
> field in the parametric format modifier macro
> DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() by another bit to
> accommodate the new layout type.
>
> There are a few ways the parameteric format modifier definition
> could have been altered to handle the new layouts:
>
> -The GOB Height and Page Kind field has a reserved value that could
>  have been used. However, the GOB height and page kind enums did
>  not change relative to prior chips, so this is sort of a lie.
>  However, this is the least-invasive change.
>
> -An entirely new field could have been added. This seems
>  inappropriate given the presence of an existing appropriate field.
>  The advantage here is it avoids splitting the sector layout field
>  across two bitfields.
>
> The proposed approach is the logically consistent one, but has the
> downside of being the most complex, and that it causes the
> DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() macro to evaluate its
> 's' parameter twice. However, I believe the added helper functions
> and macros address the complexity, and I have audited the relevant
> code and do not believe the double evaluation should cause any
> problems in practice.
>
I think we'll converge pretty quickly on the last patch. I'm less sure
about the first 3. While I like the idea of having static inlines for
modifiers that are shared by everybody, we can't actually use them from NVK
because our image layout code is in rust and bindgen can't generate
bindings for inlines so we're going to end up re-typing that all anyway.

Also, I'm not seeing a patch to fix KMS to advertise the correct modifiers.
Were you planning to type that or should I ask Lyude or Ben?

~Faith

> James Jones (4):
>   drm: macros defining fields of NVIDIA modifiers
>   drm: add inline helper funcs for NVIDIA modifiers
>   drm/nouveau: use format modifier helper funcs
>   drm: define NVIDIA DRM format modifiers for GB20x
>
>  drivers/gpu/drm/nouveau/nouveau_display.c |  12 ++-
>  include/uapi/drm/drm_fourcc.h             | 100 ++++++++++++++++++----
>  2 files changed, 92 insertions(+), 20 deletions(-)
>
> --
> 2.49.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20250704/559af368/attachment.htm>

On 7/4/25 07:45, Faith Ekstrand wrote:
> On Thu, Jul 3, 2025 at 6:34?PM James Jones <jajones at nvidia.com 
> <mailto:jajones at nvidia.com>> wrote:
> 
>     The layout of bits within the individual tiles (referred to as
>     sectors in the DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() macro)
>     changed for some formats starting in Blackwell 2 GPUs. New format
>     modifiers are needed to denote the difference and prevent direct
>     sharing of these incompatible buffers with older GPUs.
> 
>     This patch series proposes first adding some helper macros and
>     inline functions to drm_fourcc.h to make the NVIDIA block-linear
>     format modifiers easier to work with given the proposed solution
>     makes them harder to parse, then extending the existing sector type
>     field in the parametric format modifier macro
>     DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() by another bit to
>     accommodate the new layout type.
> 
>     There are a few ways the parameteric format modifier definition
>     could have been altered to handle the new layouts:
> 
>     -The GOB Height and Page Kind field has a reserved value that could
>      ?have been used. However, the GOB height and page kind enums did
>      ?not change relative to prior chips, so this is sort of a lie.
>      ?However, this is the least-invasive change.
> 
>     -An entirely new field could have been added. This seems
>      ?inappropriate given the presence of an existing appropriate field.
>      ?The advantage here is it avoids splitting the sector layout field
>      ?across two bitfields.
> 
>     The proposed approach is the logically consistent one, but has the
>     downside of being the most complex, and that it causes the
>     DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D() macro to evaluate its
>     's' parameter twice. However, I believe the added helper
functions
>     and macros address the complexity, and I have audited the relevant
>     code and do not believe the double evaluation should cause any
>     problems in practice.
> 
> 
> I think we'll converge pretty quickly on the last patch. I'm less
sure
> about the first 3. While I like the idea of having static inlines for 
> modifiers that are shared by everybody, we can't actually use them from
> NVK because our image layout code is in rust and bindgen can't generate
> bindings for inlines so we're going to end up re-typing that all
anyway.
(Sorry for the long delay. Back from a series of vacations now)

Oh, that's too bad. Is there some better way to express this that can be 
consumed by Rust or a Rust generator script? Maybe just some defines for 
the bitfield offsets and sizes? I'm not sure how to clearly encapsulate 
the split sector field that way though. It might be useful in the 
Nouveau kernel code below, but would probably have the same problem 
moving to Nova. Might just have to type that part N times in each client 
codebase. I'll give it some more thought.

The main purpose of including all the inlines before the actual format 
modifier update was to illustrate that although a split bitfield can 
sound nasty (Or did to me initially anyway), it can be encapsulated well 
enough to make it a non-issue. I'm not wedded to the actual 
implementation of the helper code. If we're in general agreement on the 
modifier layout, I'll send that out stand-alone and we can iterate on 
the helpers as needed given they're much less urgent.
> Also, I'm not seeing a patch to fix KMS to advertise the correct 
> modifiers. Were you planning to type that or should I ask Lyude or Ben?
This was just an RFC, so I didn't want to type everything out given it'd
take a lot more time to test it (I lightly tested the RFC patches with 
some hacky one-off test code). I'll take a look at what's needed in 
Nouveau to advertise the right display modifiers and see whether it's 
something I can realistically sign up to do.

Thanks,
-James
> ~Faith
> 
>     James Jones (4):
>      ? drm: macros defining fields of NVIDIA modifiers
>      ? drm: add inline helper funcs for NVIDIA modifiers
>      ? drm/nouveau: use format modifier helper funcs
>      ? drm: define NVIDIA DRM format modifiers for GB20x
> 
>      ?drivers/gpu/drm/nouveau/nouveau_display.c |? 12 ++-
>      ?include/uapi/drm/drm_fourcc.h? ? ? ? ? ? ?| 100
++++++++++++++++++----
>      ?2 files changed, 92 insertions(+), 20 deletions(-)
> 
>     -- 
>     2.49.0
>