Faith Ekstrand
2025-Aug-22 15:48 UTC
[PATCH 3/3] drm/nouveau: Advertise correct modifiers on GB20x
On Mon, Aug 11, 2025 at 5:57?PM James Jones <jajones at 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 at 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.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.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.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.> + } > } > > 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 > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20250822/cc74c0e5/attachment-0001.htm>
James Jones
2025-Aug-22 17:03 UTC
[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 at nvidia.com > <mailto:jajones at 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 at nvidia.com > <mailto:jajones at 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 >