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

[lyude@redhat.com]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2026-08-20 at 20:49 +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 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.
> The mode list then tops out at the CTA VICs.
> 
> 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.
> 
> A failed RM read now takes the same "no EDID" path as an empty DDC
> read
> which stops it from leaking the runtime-PM reference detect() holds.
> The
> property, display_info, and mode list go through
> drm_edid_connector_update()/_add_modes() and every EDID source is
> funneled through nouveau_connector_set_edid() so all of them reach
> the
> property that drm_edid_connector_add_modes() reads from.
> 
> nouveau_acpi_edid() now returns a drm_edid sized from the EDID's
> extension count but never past the length _DDC actually returned, so
> padded buffers pass and truncated ones are rejected, and every
> firmware-provided EDID (OF, ACPI, VBIOS-embedded) is validated with
> drm_edid_valid(). An invalid one is treated as no EDID. get_modes()
> re-syncs the property from the connector's copy when the probe helper
> has cleared it for a forced-off connector, before adding modes.
> 
> The few raw EDID readers are converted along with it, so no raw
> struct
> edid pointer is kept. Ownership and freeing move to the drm_edid. MST
> connectors keep their separate legacy path.
> 
> Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
> ---
>  drivers/gpu/drm/nouveau/dispnv04/dfp.c      |   5 +-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c     |   4 +-
>  drivers/gpu/drm/nouveau/nouveau_acpi.c      |  21 ++-
>  drivers/gpu/drm/nouveau/nouveau_acpi.h      |  10 +-
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 147 +++++++++++++++---
> --
>  drivers/gpu/drm/nouveau/nouveau_connector.h |  12 +-
>  6 files changed, 151 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/dfp.c
> b/drivers/gpu/drm/nouveau/dispnv04/dfp.c
> index c9f96ec8455d..00eb2aacbe93 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/dfp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/dfp.c
> @@ -344,9 +344,8 @@ static void nv04_dfp_mode_set(struct drm_encoder
> *encoder,
>  		regp->fp_control |= (2 << 24);
>  	if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS) {
>  		bool duallink = false, dummy;
> -		if (nv_connector->edid &&
> -		    nv_connector->type == DCB_CONNECTOR_LVDS_SPWG) {
> -			duallink = (((u8 *)nv_connector->edid)[121]
> == 2);
> +		if (nv_connector->spwg_links) {
> +			duallink = nv_connector->spwg_links == 2;
>  		} else {
>  			nouveau_bios_parse_lvds_table(dev,
> output_mode->clock,
>  						      &duallink,
> &dummy);
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 2c66e480b511..add19d479ebe 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1803,8 +1803,8 @@ nv50_sor_atomic_enable(struct drm_encoder
> *encoder, struct drm_atomic_commit *st
>  			lvds_dual = bios->fp.dual_link;
>  			lvds_8bpc = bios->fp.if_is_24bit;
>  		} else {
> -			if (nv_connector->type ==
> DCB_CONNECTOR_LVDS_SPWG) {
> -				if (((u8 *)nv_connector->edid)[121]
> == 2)
> +			if (nv_connector->spwg_links) {
> +				if (nv_connector->spwg_links == 2)
>  					lvds_dual = true;
>  			} else
>  			if (mode->clock >= bios-
> >fp.duallink_transition_clk) {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> index 21b56cc7605c..ab2868edd42b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> @@ -360,9 +360,10 @@ void nouveau_unregister_dsm_handler(void) {}
>  void nouveau_switcheroo_optimus_dsm(void) {}
>  #endif
>  
> -void *
> +const struct drm_edid *
>  nouveau_acpi_edid(struct drm_device *dev, struct drm_connector
> *connector)
>  {
> +	const struct drm_edid *drm_edid;
>  	struct acpi_device *acpidev;
>  	int type, ret;
>  	void *edid;
> @@ -384,7 +385,23 @@ nouveau_acpi_edid(struct drm_device *dev, struct
> drm_connector *connector)
>  	if (ret < 0)
>  		return NULL;
>  
> -	return edid;
> +	/* Never let the EDID's own extension count reach past what
> _DDC
> +	 * actually returned. Drop the padding some firmware appends
> so the
> +	 * container is exactly the EDID, then validate it like the
> DDC
> +	 * readers would.
> +	 */
> +	if (ret >= EDID_LENGTH)
> +		ret = min_t(int, ret, EDID_LENGTH *
> +				      (1 + ((const struct edid
> *)edid)->extensions));
> +	drm_edid = drm_edid_alloc(edid, ret);
> +	kfree(edid);
> +
> +	if (drm_edid && !drm_edid_valid(drm_edid)) {
> +		drm_dbg_kms(dev, "Invalid EDID from ACPI _DDC\n");
> +		drm_edid_free(drm_edid);
> +		drm_edid = NULL;
> +	}
> +	return drm_edid;
>  }
>  
>  bool nouveau_acpi_video_backlight_use_native(void)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h
> b/drivers/gpu/drm/nouveau/nouveau_acpi.h
> index e39dd8b94b8b..bc3ccca7b338 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h
> @@ -4,13 +4,15 @@
>  
>  #define ROM_BIOS_PAGE 4096
>  
> +struct drm_edid;
> +
>  #if defined(CONFIG_ACPI) && defined(CONFIG_X86)
>  bool nouveau_is_optimus(void);
>  bool nouveau_is_v1_dsm(void);
>  void nouveau_register_dsm_handler(void);
>  void nouveau_unregister_dsm_handler(void);
>  void nouveau_switcheroo_optimus_dsm(void);
> -void *nouveau_acpi_edid(struct drm_device *, struct drm_connector
> *);
> +const struct drm_edid *nouveau_acpi_edid(struct drm_device *, struct
> drm_connector *);
>  bool nouveau_acpi_video_backlight_use_native(void);
>  void nouveau_acpi_video_register_backlight(void);
>  #else
> @@ -19,7 +21,11 @@ static inline bool nouveau_is_v1_dsm(void) {
> return false; };
>  static inline void nouveau_register_dsm_handler(void) {}
>  static inline void nouveau_unregister_dsm_handler(void) {}
>  static inline void nouveau_switcheroo_optimus_dsm(void) {}
> -static inline void *nouveau_acpi_edid(struct drm_device *dev, struct
> drm_connector *connector) { return NULL; }
> +static inline const struct drm_edid *
> +nouveau_acpi_edid(struct drm_device *dev, struct drm_connector
> *connector)
> +{
> +	return NULL;
> +}
>  static inline bool nouveau_acpi_video_backlight_use_native(void) {
> return true; }
>  static inline void nouveau_acpi_video_register_backlight(void) {}
>  #endif
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index b0b0ad9a0c24..55912da83cd0 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)
> @@ -469,6 +469,36 @@ nouveau_connector_ddc_detect(struct
> drm_connector *connector)
>  	return found;
>  }
>  
> +static void
> +nouveau_connector_set_edid(struct nouveau_connector *nv_connector,
> +			   const struct drm_edid *drm_edid)
> +{
> +	if (nv_connector->drm_edid == drm_edid)
> +		return;
> +
> +	/* 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
> the
> +	 * high-refresh timings.
> +	 */
> +	drm_edid_connector_update(&nv_connector->base, drm_edid);
> +
> +	drm_edid_free(nv_connector->drm_edid);
> +	nv_connector->drm_edid = drm_edid;
> +
> +	/* The SPWG link-count byte lives in a vendor descriptor drm
> has no
> +	 * accessor for. Peek at it once here so nothing else needs
> the raw
> +	 * EDID.
> +	 */
> +	nv_connector->spwg_links = 0;
> +	if (nv_connector->type == DCB_CONNECTOR_LVDS_SPWG) {
> +		const u8 *raw = (const u8 *)drm_edid_raw(drm_edid);
> +
> +		if (raw)
> +			nv_connector->spwg_links = raw[121] == 2 ? 2
> : 1;
> +	}
> +}
> +
>  static struct nouveau_encoder *
>  nouveau_connector_of_detect(struct drm_connector *connector)
>  {
> @@ -490,8 +520,17 @@ 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) {
> -			nv_connector->edid =
> -				kmemdup(edid, EDID_LENGTH,
> GFP_KERNEL);
> +			const struct drm_edid *drm_edid =
> +				drm_edid_alloc(edid, EDID_LENGTH);
> +
> +			/* Firmware-provided, so validate it like
> the DDC
> +			 * readers would.
> +			 */
> +			if (drm_edid && !drm_edid_valid(drm_edid)) {
> +				drm_edid_free(drm_edid);
> +				drm_edid = NULL;
> +			}
> +			nouveau_connector_set_edid(nv_connector,
> drm_edid);
>  			return nv_encoder;
>  		}
>  	}
> @@ -546,17 +585,23 @@ nouveau_connector_set_encoder(struct
> drm_connector *connector,
>  	}
>  }
>  
> -static void
> -nouveau_connector_set_edid(struct nouveau_connector *nv_connector,
> -			   struct edid *edid)
> +struct nouveau_rm_edid {
> +	u8 *data;
> +	size_t size;
> +};
> +
> +static int
> +nouveau_connector_rm_edid_block(void *context, u8 *buf, unsigned int
> block,
> +				size_t len)
>  {
> -	if (nv_connector->edid != edid) {
> -		struct edid *old_edid = nv_connector->edid;
> +	struct nouveau_rm_edid *rm = context;
> +	size_t offset = (size_t)block * EDID_LENGTH;
>  
> -		drm_connector_update_edid_property(&nv_connector-
> >base, edid);
> -		kfree(old_edid);
> -		nv_connector->edid = edid;
> -	}
> +	if (offset + len > rm->size)
> +		return -EINVAL;
> +
> +	memcpy(buf, rm->data + offset, len);
> +	return 0;
>  }
>  
>  static enum drm_connector_status
> @@ -590,22 +635,37 @@ 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);
> -			if (ret < 0)
> -				return
> connector_status_disconnected;
> +			struct nouveau_rm_edid rm = {};
> +
> +			/* RM (which owns the DDC pads on GSP
> boards) reads the
> +			 * EDID whole and returns its true size,
> which for an
> +			 * HF-EEODB EDID exceeds what byte 126
> admits. Serve it
> +			 * through drm's block reader so EEODB
> sizing, block
> +			 * validation, and the debugfs EDID override
> all apply.
> +			 * A failed read is treated like an empty
> DDC read,
> +			 * which releases the runtime-PM reference.
> +			 */
> +			ret = nvif_outp_edid_get(&nv_encoder->outp,
> &rm.data);
> +			if (ret >= 0) {
> +				rm.size = ret;
> +				new_edid =
> drm_edid_read_custom(connector,
> +								nouv
> eau_connector_rm_edid_block,
> +								&rm)
> ;
> +				kfree(rm.data);
> +			}
>  		}
>  
>  		nouveau_connector_set_edid(nv_connector, new_edid);
> -		if (!nv_connector->edid) {
> +		if (!nv_connector->drm_edid) {
>  			NV_ERROR(drm, "DDC responded, but no EDID
> for %s\n",
>  				 connector->name);
>  			goto detect_analog;
> @@ -626,7 +686,7 @@ nouveau_connector_detect(struct drm_connector
> *connector, bool force)
>  				    nv_partner->dcb->type ==
> DCB_OUTPUT_TMDS) ||
>  				   (nv_encoder->dcb->type ==
> DCB_OUTPUT_TMDS &&
>  				    nv_partner->dcb->type ==
> DCB_OUTPUT_ANALOG))) {
> -			if (nv_connector->edid->input &
> DRM_EDID_INPUT_DIGITAL)
> +			if (drm_edid_is_digital(nv_connector-
> >drm_edid))
>  				type = DCB_OUTPUT_TMDS;
>  			else
>  				type = DCB_OUTPUT_ANALOG;
> @@ -638,7 +698,8 @@ nouveau_connector_detect(struct drm_connector
> *connector, bool force)
>  		conn_status = connector_status_connected;
>  
>  		if (nv_encoder->dcb->type == DCB_OUTPUT_DP)
> -			drm_dp_cec_set_edid(&nv_connector->aux,
> nv_connector->edid);
> +			drm_dp_cec_attach(&nv_connector->aux,
> +					  connector-
> >display_info.source_physical_address);
>  
>  		goto out;
>  	} else {
> @@ -670,7 +731,7 @@ nouveau_connector_detect(struct drm_connector
> *connector, bool force)
>  	}
>  
>   out:
> -	if (!nv_connector->edid)
> +	if (!nv_connector->drm_edid)
>  		drm_dp_cec_unset_edid(&nv_connector->aux);
>  
>  	pm_runtime_mark_last_busy(dev->dev);
> @@ -686,7 +747,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 +758,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;
>  		}
>  	}
> @@ -733,9 +794,17 @@ 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);
> +			/* Firmware-provided, so validate it like
> the DDC
> +			 * readers would.
> +			 */
> +			if (edid && !drm_edid_valid(edid)) {
> +				drm_edid_free(edid);
> +				edid = NULL;
> +			}
>  			if (edid)
>  				status = connector_status_connected;
>  		}
> @@ -886,7 +955,7 @@ nouveau_connector_detect_depth(struct
> drm_connector *connector)
>  	bool duallink;
>  
>  	/* if the edid is feeling nice enough to provide this info,
> use it */
> -	if (nv_connector->edid && connector->display_info.bpc)
> +	if (nv_connector->drm_edid && connector->display_info.bpc)
>  		return;
>  
>  	/* EDID 1.4 is *supposed* to be supported on eDP, but,
> Apple... */
> @@ -913,9 +982,8 @@ nouveau_connector_detect_depth(struct
> drm_connector *connector)
>  	/* LVDS: DDC panel, need to first determine the number of
> links to
>  	 * know which if_is_24bit flag to check...
>  	 */
> -	if (nv_connector->edid &&
> -	    nv_connector->type == DCB_CONNECTOR_LVDS_SPWG)
> -		duallink = ((u8 *)nv_connector->edid)[121] == 2;
> +	if (nv_connector->spwg_links)
> +		duallink = nv_connector->spwg_links == 2;
>  	else
>  		duallink = mode->clock >= bios-
> >fp.duallink_transition_clk;
>  
> @@ -973,12 +1041,17 @@ 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);
> -	else
> -	if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
> -	    (nv_encoder->dcb->lvdsconf.use_straps_for_mode ||
> -	     drm->vbios.fp_no_ddc) && nouveau_bios_fp_mode(dev,
> NULL)) {
> +	if (nv_connector->drm_edid) {
> +		/* The probe helper clears the property and
> display_info for
> +		 * a forced-off connector without calling detect().
> Re-sync
> +		 * from our copy then, since add_modes() reads the
> property.
> +		 */
> +		if (!connector->edid_blob_ptr)
> +			drm_edid_connector_update(connector,
> 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 ||
> +		    drm->vbios.fp_no_ddc) &&
> nouveau_bios_fp_mode(dev, NULL)) {
>  		struct drm_display_mode mode;
>  
>  		nouveau_bios_fp_mode(dev, &mode);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h
> b/drivers/gpu/drm/nouveau/nouveau_connector.h
> index 0608cabed058..eb292d2ba4bc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
> @@ -43,7 +43,7 @@
>  
>  struct nvkm_i2c_port;
>  struct dcb_output;
> -struct edid;
> +struct drm_edid;
>  
>  #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
>  struct nouveau_backlight {
> @@ -121,6 +121,13 @@ struct nouveau_connector {
>  	struct drm_connector base;
>  	enum dcb_connector_type type;
>  	u8 index;
> +	/* LVDS_SPWG panels state their link count in EDID
> descriptor 4 (SPWG
> +	 * byte 0x79), cached by nouveau_connector_set_edid() so
> nothing else
> +	 * needs the raw EDID. 0 = unknown (not an SPWG panel, or no
> EDID) and
> +	 * callers fall back to their transition-clock/VBIOS
> heuristics;
> +	 * 1 = single link; 2 = dual link.
> +	 */
> +	u8 spwg_links;
>  
>  	struct nvif_conn conn;
>  	u64 hpd_pending;
> @@ -137,7 +144,8 @@ struct nouveau_connector {
>  	int scaling_mode;
>  
>  	struct nouveau_encoder *detected_encoder;
> -	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;