Re: [PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

Thomas Zimmermann <tzimmermann@xxxxxxx> · Fri, 5 Apr 2024 09:06:50 +0200

Hi

Am 05.04.24 um 08:38 schrieb Jani Nikula:
On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
EDID read functions do not validate their return data. So they might
return the required number of bytes of probing, but with invalid
data. Therefore test for the presence of an EDID header similar to
the code in edid_block_check().
I don't think the point of drm_probe_ddc() ever was to validate
anything. It reads one byte to see if there's any response. That's all
there is to it.

All EDID validation happens in the _drm_do_get_edid() when actually
reading the EDID.

I don't think I like duplicating this behaviour in both the probe and
the EDID read. And I'm not sure why we're giving drivers the option to
pass a parameter whether to validate or not. Just why?

Udl doesn't use DDC, but a custom USB protocol. When queried for the 
EDID, the udl adapter sends data even if there's no monitor connected. 
All bytes are zero in this case. So the driver needs to do some sort of 
EDID validation on the received bytes to ensure that there's a monitor 
present.

drm_edid.c has all code necessary to validate the header. And there's 
the edid_fixup parameter, which I think should be respected by any 
validation helper. I'd preferably not duplicate this in udl.

Can we instead implement drm_probe_ddc() separately from 
drm_edid_probe_custom()? The former would remain as it is. The latter 
would validate unconditionally.

Best regards
Thomas


BR,
Jani.


Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
  drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
  include/drm/drm_edid.h     |  2 +-
  2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a3e9333f9177a..4e12d4b83a720 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
  	memcpy(edid, edid_header, sizeof(edid_header));
  }
  
+static int edid_header_score(const u8 *header)
+{
+	int i, score = 0;
+
+	for (i = 0; i < sizeof(edid_header); i++) {
+		if (header[i] == edid_header[i])
+			score++;
+	}
+
+	return score;
+}
+
  /**
   * drm_edid_header_is_valid - sanity check the header of the base EDID block
   * @_edid: pointer to raw base EDID block
@@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
  int drm_edid_header_is_valid(const void *_edid)
  {
  	const struct edid *edid = _edid;
-	int i, score = 0;
  
-	for (i = 0; i < sizeof(edid_header); i++) {
-		if (edid->header[i] == edid_header[i])
-			score++;
-	}
-
-	return score;
+	return edid_header_score(edid->header);
  }
  EXPORT_SYMBOL(drm_edid_header_is_valid);
  
@@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
   * drm_edid_probe_custom() - probe DDC presence
   * @read_block: EDID block read function
   * @context: Private data passed to the block read function
+ * @validate: True to validate the EDID header
   *
   * Probes for EDID data. Only reads enough data to detect the presence
- * of the EDID channel.
+ * of the EDID channel. Some EDID block read functions do not fail,
+ * but return invalid data if no EDID data is available. If @validate
+ * has been specified, drm_edid_probe_custom() validates the retrieved
+ * EDID header before signalling success.
   *
   * Return: True on success, false on failure.
   */
-bool drm_edid_probe_custom(read_block_fn read_block, void *context)
+bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
  {
-	unsigned char out;
+	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
+	int ret;
+	size_t len = 1;
+
+	if (validate)
+		len = sizeof(header); // read full header
+
+	ret = read_block(context, header, 0, len);
+	if (ret)
+		return false;
+
+	if (validate) {
+		int score = edid_header_score(header);
+
+		if (score < clamp(edid_fixup, 0, 8))
+			return false;
+	}
  
-	return (read_block(context, &out, 0, 1) == 0);
+	return true;
  }
  EXPORT_SYMBOL(drm_edid_probe_custom);
  
@@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
  bool
  drm_probe_ddc(struct i2c_adapter *adapter)
  {
-	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
+	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
  }
  EXPORT_SYMBOL(drm_probe_ddc);
  
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 4d1797ade5f1d..299278c7ee1c2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
  
  bool
  drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
-		      void *context);
+		      void *context, bool validate);
  bool drm_probe_ddc(struct i2c_adapter *adapter);
  struct edid *drm_do_get_edid(struct drm_connector *connector,
  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)







