Re: [PATCH 7/7] nouveau: honor HF-EEODB EDIDs by converting to struct drm_edid

[email protected]
Newsgroups gmane.comp.freedesktop.xorg.nouveau,gmane.linux.kernel,gmane.comp.video.dri.devel
Message-ID <[email protected]>
Ah, how rust has changed me. Some comments below:

On Sat, 2026-08-15 at 03:57 +0400, Mohamed Ahmed wrote:
> HDMI 2.1 sinks ship HF-EEODB EDIDs. Byte 126 deliberately claims one
> extension block for legacy sources, and the true count lives in the
> CTA
> block's first data block. Their high-refresh timings (e.g., 4K240,
> 6K165,
> etc) sit in DisplayID extension blocks 2+.
> 
> The kernel already reads these EDIDs whole, and on GSP boards RM
> returns
> the full EDID with its true size. However, nouveau then hands the
> buffer
> to the legacy API which sizes the EDID from byte 126 so the DisplayID
> blocks are not parsed/exposed and the sysfs blob truncates to 256
> bytes.
> 
> Convert to the struct drm_edid API, which carries the real size.
> Read via drm_edid_read_ddc()/drm_edid_read_switcheroo() where an
> adapter exists, and serve RM's buffer through drm_edid_read_custom()
> with a block-reading callback where not, which also routes the GSP
> path through drm's block validation and the debugfs EDID override
> (previously dead on GSP boards, since nothing consulted it outside
> the i2c readers).
> 
> The connector keeps a borrowed const drm_edid_raw() view in ->edid,
> same
> as i915's detect_edid so the raw-EDID consumers (SAD parsing, deep-
> color
> flags, the DVI dual-link byte, DP CEC) are untouched. Ownership and
> freeing move to the drm_edid. The OF path additionally now frees any
> previous EDID itself rather than relying on the callers. MST
> connectors
> keep their separate legacy path.
> 
> Signed-off-by: Mohamed Ahmed <[email protected]>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c     |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 95 ++++++++++++++++---
> --
>  drivers/gpu/drm/nouveau/nouveau_connector.h |  7 +-
>  3 files changed, 80 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 364227f5456f..c3b7fd29c077 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1804,7 +1804,7 @@ nv50_sor_atomic_enable(struct drm_encoder
> *encoder, struct drm_atomic_commit *st
>  			lvds_8bpc = bios->fp.if_is_24bit;
>  		} else {
>  			if (nv_connector->type ==
> DCB_CONNECTOR_LVDS_SPWG) {
> -				if (((u8 *)nv_connector->edid)[121]
> == 2)
> +				if (((const u8 *)nv_connector-
> >edid)[121] == 2)
>  					lvds_dual = true;
>  			} else
>  			if (mode->clock >= bios-
> >fp.duallink_transition_clk) {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index b0b0ad9a0c24..4105555151c9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -397,7 +397,7 @@ nouveau_connector_destroy(struct drm_connector
> *connector)
>  	struct nouveau_connector *nv_connector =
> nouveau_connector(connector);
>  	nvif_event_dtor(&nv_connector->irq);
>  	nvif_event_dtor(&nv_connector->hpd);
> -	kfree(nv_connector->edid);
> +	drm_edid_free(nv_connector->drm_edid);
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  	if (nv_connector->aux.transfer)
> @@ -490,8 +490,11 @@ nouveau_connector_of_detect(struct drm_connector
> *connector)
>  		int idx = name ? name[strlen(name) - 1] - 'A' : 0;
>  
>  		if (nv_encoder->dcb->i2c_index == idx && edid) {
> +			drm_edid_free(nv_connector->drm_edid);
> +			nv_connector->drm_edid =
> +				drm_edid_alloc(edid, EDID_LENGTH);
>  			nv_connector->edid =
> -				kmemdup(edid, EDID_LENGTH,
> GFP_KERNEL);
> +				drm_edid_raw(nv_connector-
> >drm_edid);
>  			return nv_encoder;
>  		}
>  	}
> @@ -546,17 +549,42 @@ nouveau_connector_set_encoder(struct
> drm_connector *connector,
>  	}
>  }
>  
> +struct nouveau_rm_edid {
> +	u8 *data;
> +	int size;
> +};
> +
> +static int
> +nouveau_connector_rm_edid_block(void *context, u8 *buf, unsigned int
> block,
> +				size_t len)
> +{
> +	struct nouveau_rm_edid *rm = context;
> +	size_t offset = (size_t)block * EDID_LENGTH;
> +
> +	if (offset + len > rm->size)
> +		return -EINVAL;
> +
> +	memcpy(buf, rm->data + offset, len);
> +	return 0;
> +}
> +
>  static void
>  nouveau_connector_set_edid(struct nouveau_connector *nv_connector,
> -			   struct edid *edid)
> +			   const struct drm_edid *drm_edid)
>  {
> -	if (nv_connector->edid != edid) {
> -		struct edid *old_edid = nv_connector->edid;
> +	if (nv_connector->drm_edid == drm_edid)
> +		return;
>  
> -		drm_connector_update_edid_property(&nv_connector-
> >base, edid);
> -		kfree(old_edid);
> -		nv_connector->edid = edid;
> -	}
> +	/* Updates the EDID property and display_info with HF-EEODB-
> aware
> +	 * sizing. The legacy helpers truncate both to what EDID
> byte 126
> +	 * admits, hiding the DisplayID extension blocks that carry
> every
> +	 * timing a DTD can't express (>655.35 MHz).
> +	 */
> +	drm_edid_connector_update(&nv_connector->base, drm_edid);
> +
> +	drm_edid_free(nv_connector->drm_edid);
> +	nv_connector->drm_edid = drm_edid;
> +	nv_connector->edid = drm_edid_raw(drm_edid);
>  }
>  
>  static enum drm_connector_status
> @@ -590,20 +618,36 @@ nouveau_connector_detect(struct drm_connector
> *connector, bool force)
>  
>  	nv_encoder = nouveau_connector_ddc_detect(connector);
>  	if (nv_encoder) {
> -		struct edid *new_edid = NULL;
> +		const struct drm_edid *new_edid = NULL;
>  
>  		if (nv_encoder->i2c) {
>  			if ((vga_switcheroo_handler_flags() &
> VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
>  			    nv_connector->type ==
> DCB_CONNECTOR_LVDS)
> -				new_edid =
> drm_get_edid_switcheroo(connector, nv_encoder->i2c);
> +				new_edid =
> drm_edid_read_switcheroo(connector, nv_encoder->i2c);
>  			else
> -				new_edid = drm_get_edid(connector,
> nv_encoder->i2c);
> +				new_edid =
> drm_edid_read_ddc(connector, nv_encoder->i2c);
>  		} else {
> -			ret = nvif_outp_edid_get(&nv_encoder->outp,
> (u8 **)&new_edid);
> +			struct nouveau_rm_edid rm = {};
> +
> +			ret = nvif_outp_edid_get(&nv_encoder->outp,
> &rm.data);
>  			if (ret < 0)
>  				return
> connector_status_disconnected;
> +
> +			/* ret is RM's true buffer size: an HF-EEODB
> EDID is
> +			 * larger than its byte 126 admits, and RM
> (which
> +			 * owns the DDC pads on GSP boards) reads it
> whole.
> +			 * Serve it through drm's block reader so
> EEODB
> +			 * sizing, block validation, and the debugfs
> EDID
> +			 * override all apply.
> +			 */
> +			rm.size = ret;
> +			new_edid = drm_edid_read_custom(connector,
> +							nouveau_conn
> ector_rm_edid_block,
> +							&rm);
> +			kfree(rm.data);
>  		}
>  
> +
>  		nouveau_connector_set_edid(nv_connector, new_edid);
>  		if (!nv_connector->edid) {
>  			NV_ERROR(drm, "DDC responded, but no EDID
> for %s\n",
> @@ -686,7 +730,7 @@ nouveau_connector_detect_lvds(struct
> drm_connector *connector, bool force)
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nouveau_connector *nv_connector =
> nouveau_connector(connector);
>  	struct nouveau_encoder *nv_encoder = NULL;
> -	struct edid *edid = NULL;
> +	const struct drm_edid *edid = NULL;
>  	enum drm_connector_status status =
> connector_status_disconnected;
>  
>  	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
> @@ -697,7 +741,7 @@ nouveau_connector_detect_lvds(struct
> drm_connector *connector, bool force)
>  	if (!drm->vbios.fp_no_ddc) {
>  		status = nouveau_connector_detect(connector, force);
>  		if (status == connector_status_connected) {
> -			edid = nv_connector->edid;
> +			edid = nv_connector->drm_edid;
>  			goto out;
>  		}
>  	}
> @@ -712,7 +756,13 @@ nouveau_connector_detect_lvds(struct
> drm_connector *connector, bool force)
>  	 * valid - it's not (rh#613284)
>  	 */
>  	if (nv_encoder->dcb->lvdsconf.use_acpi_for_edid) {
> -		edid = nouveau_acpi_edid(dev, connector);
> +		struct edid *raw = nouveau_acpi_edid(dev,
> connector);
> +
> +		if (raw) {
> +			edid = drm_edid_alloc(raw,
> +					      EDID_LENGTH * (1 +
> raw->extensions));
> +			kfree(raw);

Maybe check drm_edid_alloc() after calling it so we can print something
if it fails? Otherwise, an allocation failure might mask itself as some
other unrelated connector probing error

> +		}
>  		if (edid) {
>  			status = connector_status_connected;
>  			goto out;
> @@ -733,9 +783,10 @@ nouveau_connector_detect_lvds(struct
> drm_connector *connector, bool force)
>  	 * stored for the panel stored in them.
>  	 */
>  	if (!drm->vbios.fp_no_ddc) {
> -		edid = (struct edid
> *)nouveau_bios_embedded_edid(dev);
> -		if (edid) {
> -			edid = kmemdup(edid, EDID_LENGTH,
> GFP_KERNEL);
> +		const void *embedded =
> nouveau_bios_embedded_edid(dev);
> +
> +		if (embedded) {
> +			edid = drm_edid_alloc(embedded,
> EDID_LENGTH);
>  			if (edid)
>  				status = connector_status_connected;
>  		}
> @@ -915,7 +966,7 @@ nouveau_connector_detect_depth(struct
> drm_connector *connector)
>  	 */
>  	if (nv_connector->edid &&
>  	    nv_connector->type == DCB_CONNECTOR_LVDS_SPWG)
> -		duallink = ((u8 *)nv_connector->edid)[121] == 2;
> +		duallink = ((const u8 *)nv_connector->edid)[121] ==
> 2;
>  	else
>  		duallink = mode->clock >= bios-
> >fp.duallink_transition_clk;
>  
> @@ -973,8 +1024,8 @@ nouveau_connector_get_modes(struct drm_connector
> *connector)
>  		nv_connector->native_mode = NULL;
>  	}
>  
> -	if (nv_connector->edid)
> -		ret = drm_add_edid_modes(connector, nv_connector-
> >edid);
> +	if (nv_connector->drm_edid)
> +		ret = drm_edid_connector_add_modes(connector);
>  	else
>  	if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
>  	    (nv_encoder->dcb->lvdsconf.use_straps_for_mode ||
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h
> b/drivers/gpu/drm/nouveau/nouveau_connector.h
> index 0608cabed058..ca8f310b3680 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
> @@ -137,7 +137,12 @@ struct nouveau_connector {
>  	int scaling_mode;
>  
>  	struct nouveau_encoder *detected_encoder;
> -	struct edid *edid;
> +	/* Borrowed decoded view of ->drm_edid for the raw-EDID
> consumers
> +	 * (SADs, deep-colour flags, dual-link byte); do not free.
> +	 */
> +	const struct edid *edid;
> +	/* Owner of the sink's EDID, HF-EEODB-complete. */
> +	const struct drm_edid *drm_edid;
>  	struct drm_display_mode *native_mode;
>  #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
>  	struct nouveau_backlight *backlight;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.