On Fri, Aug 22, 2025 at 1:03 PM James Jones <jajo...@nvidia.com> wrote:
>
> 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.
Okay. I'm convinced.
Reviewed-by: Faith Ekstrand <faith.ekstr...@collabora.com>
> 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
> >
>
>
