Re: [PATCH 3/3] drm/nouveau: Advertise correct modifiers on GB20x

[James Jones <jajones@nvidia.com>]

On 8/22/25 08:48, Faith Ekstrand wrote:
> 
> On Mon, Aug 11, 2025 at 5:57 PM James Jones <jajones@nvidia.com 
> <mailto:jajones@nvidia.com>> wrote:
> 
>     8 and 16 bit formats use a different layout on
>     GB20x than they did on prior chips. Add the
>     corresponding DRM format modifiers to the list of
>     modifiers supported by the display engine on such
>     chips, and filter the supported modifiers for each
>     format based on its bytes per pixel in
>     nv50_plane_format_mod_supported().
> 
>     Note this logic will need to be updated when GB10
>     support is added, since it is a GB20x chip that
>     uses the pre-GB20x sector layout for all formats.
> 
>     Signed-off-by: James Jones <jajones@nvidia.com
>     <mailto:jajones@nvidia.com>>
>     ---
>       drivers/gpu/drm/nouveau/dispnv50/disp.c     |  4 ++-
>       drivers/gpu/drm/nouveau/dispnv50/disp.h     |  1 +
>       drivers/gpu/drm/nouveau/dispnv50/wndw.c     | 24 +++++++++++++--
>       drivers/gpu/drm/nouveau/dispnv50/wndwca7e.c | 33 +++++++++++++++++++++
>       4 files changed, 59 insertions(+), 3 deletions(-)
> 
>     diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/
>     drm/nouveau/dispnv50/disp.c
>     index e97e39abf3a2..12b1dba8e05d 100644
>     --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>     +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>     @@ -2867,7 +2867,9 @@ nv50_display_create(struct drm_device *dev)
>              }
> 
>              /* Assign the correct format modifiers */
>     -       if (disp->disp->object.oclass >= TU102_DISP)
>     +       if (disp->disp->object.oclass >= GB202_DISP)
>     +               nouveau_display(dev)->format_modifiers =
>     wndwca7e_modifiers;
>     +       else if (disp->disp->object.oclass >= TU102_DISP)
>                      nouveau_display(dev)->format_modifiers =
>     wndwc57e_modifiers;
>              else
>              if (drm->client.device.info <http://
>     client.device.info>.family >= NV_DEVICE_INFO_V0_FERMI)
>     diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.h b/drivers/gpu/
>     drm/nouveau/dispnv50/disp.h
>     index 15f9242b72ac..5d998f0319dc 100644
>     --- a/drivers/gpu/drm/nouveau/dispnv50/disp.h
>     +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.h
>     @@ -104,4 +104,5 @@ struct nouveau_encoder *nv50_real_outp(struct
>     drm_encoder *encoder);
>       extern const u64 disp50xx_modifiers[];
>       extern const u64 disp90xx_modifiers[];
>       extern const u64 wndwc57e_modifiers[];
>     +extern const u64 wndwca7e_modifiers[];
>       #endif
>     diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/
>     drm/nouveau/dispnv50/wndw.c
>     index e2c55f4b9c5a..ef9e410babbf 100644
>     --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>     +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>     @@ -786,13 +786,14 @@ nv50_wndw_destroy(struct drm_plane *plane)
>       }
> 
>       /* This function assumes the format has already been validated
>     against the plane
>     - * and the modifier was validated against the device-wides modifier
>     list at FB
>     + * and the modifier was validated against the device-wide modifier
>     list at FB
>        * creation time.
>        */
>       static bool nv50_plane_format_mod_supported(struct drm_plane *plane,
>                                                  u32 format, u64 modifier)
>       {
>              struct nouveau_drm *drm = nouveau_drm(plane->dev);
>     +       const struct drm_format_info *info = drm_format_info(format);
>              uint8_t i;
> 
>              /* All chipsets can display all formats in linear layout */
>     @@ -800,13 +801,32 @@ static bool
>     nv50_plane_format_mod_supported(struct drm_plane *plane,
>                      return true;
> 
>              if (drm->client.device.info <http://
>     client.device.info>.chipset < 0xc0) {
>     -               const struct drm_format_info *info =
>     drm_format_info(format);
>                      const uint8_t kind = (modifier >> 12) & 0xff;
> 
>                      if (!format) return false;
> 
>                      for (i = 0; i < info->num_planes; i++)
>                              if ((info->cpp[i] != 4) && kind != 0x70)
>     return false;
>     +       } else if (drm->client.device.info <http://
>     client.device.info>.chipset >= 0x1b2) {
>     +               const uint8_t slayout = ((modifier >> 22) & 0x1) |
>     +                       ((modifier >> 25) & 0x6);
>     +
>     +               if (!format)
>     +                       return false;
>     +
>     +               /*
>     +                * Note in practice this implies only formats where
>     cpp is equal
>     +                * for each plane, or >= 4 for all planes, are
>     supported.
>     +                */
>     +               for (i = 0; i < info->num_planes; i++) {
>     +                       if (((info->cpp[i] == 2) && slayout != 3) ||
>     +                           ((info->cpp[i] == 1) && slayout != 2) ||
>     +                           ((info->cpp[i] >= 4) && slayout != 1))
>     +                               return false;
>     +
>     +                       /* 24-bit not supported. It has yet another
>     layout */
>     +                       WARN_ON(info->cpp[i] == 3);
> 
> 
> Should this really be a WARN_ON()? A DRM log message, maybe, but 
> WARN_ON() implies something went funky inside the kernel, not that 
> userspace asked for something it shouldn't.

This is indeed a something funky in the kernel case. See the comment 
above: "This function assumes the format has already been validated 
against the plane and the modifier was validated against the device-wide 
modifier list at FB creation time." This isn't technically true at 
format blob query time, but in that case this function is just used as a 
filter against those same lists. Hence, the implication is someone 
modified the kernel to report 24-bit formats for some display plane on 
this device, and the WARN_ON is meant to guard against that/validate the 
assumption from the comment.

I went through the DRM code again to verify the "format has already been
validated" assumption still holds up, and I see these callers of this 
function:

drm_plane_has_format() - Validates the format against the plane's format 
list right before calling this function. This is the path a format from 
userspace would go through, indirectly as a drm_framebuffer property, 
when validating it against a plane during a commit operation.

create_in_format_blob() - As mentioned, simply iterates through the 
plane's format list.

Thanks,
-James



>     +               }
>              }
> 
>              return true;
>     diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndwca7e.c b/drivers/
>     gpu/drm/nouveau/dispnv50/wndwca7e.c
>     index 0d8e9a9d1a57..2cec8cfbd546 100644
>     --- a/drivers/gpu/drm/nouveau/dispnv50/wndwca7e.c
>     +++ b/drivers/gpu/drm/nouveau/dispnv50/wndwca7e.c
>     @@ -179,6 +179,39 @@ wndwca7e_ntfy_set(struct nv50_wndw *wndw,
>     struct nv50_wndw_atom *asyw)
>              return 0;
>       }
> 
>     +/****************************************************************
>     + *            Log2(block height) ----------------------------+  *
>     + *            Page Kind ----------------------------------+  |  *
>     + *            Gob Height/Page Kind Generation ------+     |  |  *
>     + *                          Sector layout -------+  |     |  |  *
>     + *                          Compression ------+  |  |     |  |  */
>     +const u64 wndwca7e_modifiers[] = { /*         |  |  |     |  |  */
>     +       /* 4cpp+ modifiers */
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 0),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 1),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 2),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 3),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 4),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 1, 2, 0x06, 5),
>     +       /* 1cpp/8bpp modifiers */
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 0),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 1),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 2),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 3),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 4),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 2, 2, 0x06, 5),
>     +       /* 2cpp/16bpp modifiers */
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 0),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 1),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 2),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 3),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 4),
>     +       DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D(0, 3, 2, 0x06, 5),
>     +       /* All formats support linear */
>     +       DRM_FORMAT_MOD_LINEAR,
>     +       DRM_FORMAT_MOD_INVALID
>     +};
>     +
>       static const struct nv50_wndw_func
>       wndwca7e = {
>              .acquire = wndwc37e_acquire,
>     -- 
>     2.50.1
>