On 8/22/25 08:48, Faith Ekstrand wrote:

On Mon, Aug 11, 2025 at 5:57 PM James Jones <jajo...@nvidia.com <mailto:jajo...@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 <jajo...@nvidia.com
    <mailto:jajo...@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



Reply via email to