| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2cf6b444-33cd-4313-a624-8d1a9cd6ec9a@nvidia.com>
Date: Fri, 22 Aug 2025 10:03:12 -0700
From: James Jones <jajones@...dia.com>
To: Faith Ekstrand <faith@...strand.net>
Cc: Danilo Krummrich <dakr@...nel.org>, Lyude Paul <lyude@...hat.com>,
Faith Ekstrand <faith.ekstrand@...labora.com>,
nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Joel Fernandes <joelagnelf@...dia.com>
Subject: Re: [PATCH 3/3] drm/nouveau: Advertise correct modifiers on GB20x
On 8/22/25 08:48, Faith Ekstrand wrote:
>
> On Mon, Aug 11, 2025 at 5:57 PM James Jones <jajones@...dia.com
> <mailto:jajones@...dia.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@...dia.com
> <mailto:jajones@...dia.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
>
Powered by blists - more mailing lists