[PATCH v2 0/8] drm/edid: overhaul displayid iterator

[PATCH v2 0/8] drm/edid: overhaul displayid iterator

[Jani Nikula]

v2 of [1], addressing Ville's review comments, and adding a couple of
extra patches on top.

BR,
Jani.

[1] https://patchwork.freedesktop.org/series/87802/


Jani Nikula (8):
  drm/edid: make a number of functions, parameters and variables const
  drm/displayid: add separate drm_displayid.c
  drm/displayid: add new displayid section/block iterators
  drm/edid: use the new displayid iterator for detailed modes
  drm/edid: use the new displayid iterator for finding CEA extension
  drm/edid: use the new displayid iterator for tile info
  drm/displayid: allow data blocks with 0 payload length
  drm/displayid: rename displayid_hdr to displayid_header

 drivers/gpu/drm/Makefile        |   2 +-
 drivers/gpu/drm/drm_displayid.c | 132 ++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c      | 171 +++++++-------------------------
 include/drm/drm_displayid.h     |  30 ++++--
 include/drm/drm_edid.h          |   3 +
 5 files changed, 196 insertions(+), 142 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_displayid.c

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/8] drm/edid: make a number of functions, parameters and variables const

[Jani Nikula]

If there's no need to change it, it should be const. There's more to be
done, but start off with changes that make follow-up work easier. No
functional changes.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 58 ++++++++++++++++++-------------------
 include/drm/drm_displayid.h |  4 +--
 2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c2bbe7bee7b6..d510b827a1f8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1585,7 +1585,7 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
 		 "Minimum number of valid EDID header bytes (0-8, default 6)");
 
-static int validate_displayid(u8 *displayid, int length, int idx);
+static int validate_displayid(const u8 *displayid, int length, int idx);
 
 static int drm_edid_block_checksum(const u8 *raw_edid)
 {
@@ -3241,10 +3241,10 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 /*
  * Search EDID for CEA extension block.
  */
-static u8 *drm_find_edid_extension(const struct edid *edid,
-				   int ext_id, int *ext_index)
+static const u8 *drm_find_edid_extension(const struct edid *edid,
+					 int ext_id, int *ext_index)
 {
-	u8 *edid_ext = NULL;
+	const u8 *edid_ext = NULL;
 	int i;
 
 	/* No EDID or EDID extensions */
@@ -3253,7 +3253,7 @@ static u8 *drm_find_edid_extension(const struct edid *edid,
 
 	/* Find CEA extension */
 	for (i = *ext_index; i < edid->extensions; i++) {
-		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
+		edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1);
 		if (edid_ext[0] == ext_id)
 			break;
 	}
@@ -3267,12 +3267,12 @@ static u8 *drm_find_edid_extension(const struct edid *edid,
 }
 
 
-static u8 *drm_find_displayid_extension(const struct edid *edid,
-					int *length, int *idx,
-					int *ext_index)
+static const u8 *drm_find_displayid_extension(const struct edid *edid,
+					      int *length, int *idx,
+					      int *ext_index)
 {
-	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
-	struct displayid_hdr *base;
+	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
+	const struct displayid_hdr *base;
 	int ret;
 
 	if (!displayid)
@@ -3286,18 +3286,18 @@ static u8 *drm_find_displayid_extension(const struct edid *edid,
 	if (ret)
 		return NULL;
 
-	base = (struct displayid_hdr *)&displayid[*idx];
+	base = (const struct displayid_hdr *)&displayid[*idx];
 	*length = *idx + sizeof(*base) + base->bytes;
 
 	return displayid;
 }
 
-static u8 *drm_find_cea_extension(const struct edid *edid)
+static const u8 *drm_find_cea_extension(const struct edid *edid)
 {
 	int length, idx;
-	struct displayid_block *block;
-	u8 *cea;
-	u8 *displayid;
+	const struct displayid_block *block;
+	const u8 *cea;
+	const u8 *displayid;
 	int ext_index;
 
 	/* Look for a top level CEA extension block */
@@ -3318,7 +3318,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
 		idx += sizeof(struct displayid_hdr);
 		for_each_displayid_db(displayid, block, idx, length) {
 			if (block->tag == DATA_BLOCK_CTA)
-				return (u8 *)block;
+				return (const u8 *)block;
 		}
 	}
 
@@ -4503,8 +4503,8 @@ static void clear_eld(struct drm_connector *connector)
 static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 {
 	uint8_t *eld = connector->eld;
-	u8 *cea;
-	u8 *db;
+	const u8 *cea;
+	const u8 *db;
 	int total_sad_count = 0;
 	int mnl;
 	int dbl;
@@ -4600,7 +4600,7 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
 {
 	int count = 0;
 	int i, start, end, dbl;
-	u8 *cea;
+	const u8 *cea;
 
 	cea = drm_find_cea_extension(edid);
 	if (!cea) {
@@ -4619,7 +4619,7 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
 	}
 
 	for_each_cea_db(cea, i, start, end) {
-		u8 *db = &cea[i];
+		const u8 *db = &cea[i];
 
 		if (cea_db_tag(db) == AUDIO_BLOCK) {
 			int j;
@@ -4631,7 +4631,7 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
 			if (!*sads)
 				return -ENOMEM;
 			for (j = 0; j < count; j++) {
-				u8 *sad = &db[1 + j * 3];
+				const u8 *sad = &db[1 + j * 3];
 
 				(*sads)[j].format = (sad[0] & 0x78) >> 3;
 				(*sads)[j].channels = sad[0] & 0x7;
@@ -4755,7 +4755,7 @@ EXPORT_SYMBOL(drm_av_sync_delay);
  */
 bool drm_detect_hdmi_monitor(struct edid *edid)
 {
-	u8 *edid_ext;
+	const u8 *edid_ext;
 	int i;
 	int start_offset, end_offset;
 
@@ -4793,7 +4793,7 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
  */
 bool drm_detect_monitor_audio(struct edid *edid)
 {
-	u8 *edid_ext;
+	const u8 *edid_ext;
 	int i, j;
 	bool has_audio = false;
 	int start_offset, end_offset;
@@ -5287,13 +5287,13 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 	return quirks;
 }
 
-static int validate_displayid(u8 *displayid, int length, int idx)
+static int validate_displayid(const u8 *displayid, int length, int idx)
 {
 	int i, dispid_length;
 	u8 csum = 0;
-	struct displayid_hdr *base;
+	const struct displayid_hdr *base;
 
-	base = (struct displayid_hdr *)&displayid[idx];
+	base = (const struct displayid_hdr *)&displayid[idx];
 
 	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
 		      base->rev, base->bytes, base->prod_id, base->ext_count);
@@ -5359,7 +5359,7 @@ static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *d
 }
 
 static int add_displayid_detailed_1_modes(struct drm_connector *connector,
-					  struct displayid_block *block)
+					  const struct displayid_block *block)
 {
 	struct displayid_detailed_timing_block *det = (struct displayid_detailed_timing_block *)block;
 	int i;
@@ -5387,9 +5387,9 @@ static int add_displayid_detailed_1_modes(struct drm_connector *connector,
 static int add_displayid_detailed_modes(struct drm_connector *connector,
 					struct edid *edid)
 {
-	u8 *displayid;
+	const u8 *displayid;
 	int length, idx;
-	struct displayid_block *block;
+	const struct displayid_block *block;
 	int num_modes = 0;
 	int ext_index = 0;
 
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 77941efb5426..f141c0eff083 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -93,11 +93,11 @@ struct displayid_detailed_timing_block {
 };
 
 #define for_each_displayid_db(displayid, block, idx, length) \
-	for ((block) = (struct displayid_block *)&(displayid)[idx]; \
+	for ((block) = (const struct displayid_block *)&(displayid)[idx]; \
 	     (idx) + sizeof(struct displayid_block) <= (length) && \
 	     (idx) + sizeof(struct displayid_block) + (block)->num_bytes <= (length) && \
 	     (block)->num_bytes > 0; \
 	     (idx) += sizeof(struct displayid_block) + (block)->num_bytes, \
-	     (block) = (struct displayid_block *)&(displayid)[idx])
+	     (block) = (const struct displayid_block *)&(displayid)[idx])
 
 #endif
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/8] drm/displayid: add separate drm_displayid.c

[Jani Nikula]

We'll be adding more DisplayID specific functions going forward, so
start off by splitting out a few functions to a separate file.

We don't bother with exporting the functions; at least for now they
should be needed solely within drm.ko.

No functional changes.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/Makefile        |  2 +-
 drivers/gpu/drm/drm_displayid.c | 59 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c      | 58 ++------------------------------
 include/drm/drm_displayid.h     |  8 +++++
 include/drm/drm_edid.h          |  3 ++
 5 files changed, 73 insertions(+), 57 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_displayid.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e932730a1706..5279db4392df 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -7,7 +7,7 @@ drm-y       :=	drm_auth.o drm_cache.o \
 		drm_file.o drm_gem.o drm_ioctl.o drm_irq.o \
 		drm_drv.o \
 		drm_sysfs.o drm_hashtab.o drm_mm.o \
-		drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o \
+		drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o drm_displayid.o \
 		drm_encoder_slave.o \
 		drm_trace_points.o drm_prime.o \
 		drm_rect.o drm_vma_manager.o drm_flip_work.o \
diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
new file mode 100644
index 000000000000..908bbe6feb61
--- /dev/null
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include <drm/drm_displayid.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_print.h>
+
+static int validate_displayid(const u8 *displayid, int length, int idx)
+{
+	int i, dispid_length;
+	u8 csum = 0;
+	const struct displayid_hdr *base;
+
+	base = (const struct displayid_hdr *)&displayid[idx];
+
+	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
+		      base->rev, base->bytes, base->prod_id, base->ext_count);
+
+	/* +1 for DispID checksum */
+	dispid_length = sizeof(*base) + base->bytes + 1;
+	if (dispid_length > length - idx)
+		return -EINVAL;
+
+	for (i = 0; i < dispid_length; i++)
+		csum += displayid[idx + i];
+	if (csum) {
+		DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+const u8 *drm_find_displayid_extension(const struct edid *edid,
+				       int *length, int *idx,
+				       int *ext_index)
+{
+	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
+	const struct displayid_hdr *base;
+	int ret;
+
+	if (!displayid)
+		return NULL;
+
+	/* EDID extensions block checksum isn't for us */
+	*length = EDID_LENGTH - 1;
+	*idx = 1;
+
+	ret = validate_displayid(displayid, *length, *idx);
+	if (ret)
+		return NULL;
+
+	base = (const struct displayid_hdr *)&displayid[*idx];
+	*length = *idx + sizeof(*base) + base->bytes;
+
+	return displayid;
+}
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d510b827a1f8..58e61f792bc7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1585,8 +1585,6 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
 		 "Minimum number of valid EDID header bytes (0-8, default 6)");
 
-static int validate_displayid(const u8 *displayid, int length, int idx);
-
 static int drm_edid_block_checksum(const u8 *raw_edid)
 {
 	int i;
@@ -3241,8 +3239,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 /*
  * Search EDID for CEA extension block.
  */
-static const u8 *drm_find_edid_extension(const struct edid *edid,
-					 int ext_id, int *ext_index)
+const u8 *drm_find_edid_extension(const struct edid *edid,
+				  int ext_id, int *ext_index)
 {
 	const u8 *edid_ext = NULL;
 	int i;
@@ -3266,32 +3264,6 @@ static const u8 *drm_find_edid_extension(const struct edid *edid,
 	return edid_ext;
 }
 
-
-static const u8 *drm_find_displayid_extension(const struct edid *edid,
-					      int *length, int *idx,
-					      int *ext_index)
-{
-	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
-	const struct displayid_hdr *base;
-	int ret;
-
-	if (!displayid)
-		return NULL;
-
-	/* EDID extensions block checksum isn't for us */
-	*length = EDID_LENGTH - 1;
-	*idx = 1;
-
-	ret = validate_displayid(displayid, *length, *idx);
-	if (ret)
-		return NULL;
-
-	base = (const struct displayid_hdr *)&displayid[*idx];
-	*length = *idx + sizeof(*base) + base->bytes;
-
-	return displayid;
-}
-
 static const u8 *drm_find_cea_extension(const struct edid *edid)
 {
 	int length, idx;
@@ -5287,32 +5259,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 	return quirks;
 }
 
-static int validate_displayid(const u8 *displayid, int length, int idx)
-{
-	int i, dispid_length;
-	u8 csum = 0;
-	const struct displayid_hdr *base;
-
-	base = (const struct displayid_hdr *)&displayid[idx];
-
-	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
-		      base->rev, base->bytes, base->prod_id, base->ext_count);
-
-	/* +1 for DispID checksum */
-	dispid_length = sizeof(*base) + base->bytes + 1;
-	if (dispid_length > length - idx)
-		return -EINVAL;
-
-	for (i = 0; i < dispid_length; i++)
-		csum += displayid[idx + i];
-	if (csum) {
-		DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,
 							    struct displayid_detailed_timings_1 *timings)
 {
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index f141c0eff083..3c6db22a518a 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -22,6 +22,10 @@
 #ifndef DRM_DISPLAYID_H
 #define DRM_DISPLAYID_H
 
+#include <linux/types.h>
+
+struct edid;
+
 #define DATA_BLOCK_PRODUCT_ID 0x00
 #define DATA_BLOCK_DISPLAY_PARAMETERS 0x01
 #define DATA_BLOCK_COLOR_CHARACTERISTICS 0x02
@@ -100,4 +104,8 @@ struct displayid_detailed_timing_block {
 	     (idx) += sizeof(struct displayid_block) + (block)->num_bytes, \
 	     (block) = (const struct displayid_block *)&(displayid)[idx])
 
+const u8 *drm_find_displayid_extension(const struct edid *edid,
+				       int *length, int *idx,
+				       int *ext_index);
+
 #endif
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index a158f585f658..759328a5eeb2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -543,5 +543,8 @@ struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 struct drm_display_mode *
 drm_display_mode_from_cea_vic(struct drm_device *dev,
 			      u8 video_code);
+const u8 *drm_find_edid_extension(const struct edid *edid,
+				  int ext_id, int *ext_index);
+
 
 #endif /* __DRM_EDID_H__ */
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/8] drm/displayid: add new displayid section/block iterators

[Jani Nikula]

Iterating DisplayID blocks across sections (in EDID extensions) is
unnecessarily complicated for the caller. Implement DisplayID iterators
to go through all blocks in all sections.

Usage example:

	const struct displayid_block *block;
	struct displayid_iter iter;

	displayid_iter_edid_begin(edid, &iter);
	displayid_iter_for_each(block, &iter) {
		/* operate on block */
	}
	displayid_iter_end(&iter);

When DisplayID is stored in EDID extensions, the DisplayID sections map
to extensions as described in VESA DisplayID v1.3 Appendix B: DisplayID
as an EDID Extension. This is implemented here.

When DisplayID is stored in its dedicated DDC device 0xA4, according to
VESA E-DDC v1.3, different rules apply for the structure. This is not
implemented here, as we don't currently use it, but the idea is you'd
have a different call for beginning the iteration, for example simply:

	displayid_iter_begin(displayid, &iter);

instead of displayid_iter_edid_begin(), and everything else would be
hidden away in the iterator functions.

v2:
- sizeof(struct displayid_block) -> sizeof(*block) (Ville)
- remove __ prefix from displayid_iter_block

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_displayid.c | 74 +++++++++++++++++++++++++++++++++
 include/drm/drm_displayid.h     | 18 ++++++++
 2 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 908bbe6feb61..5b809bb9fecc 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -57,3 +57,77 @@ const u8 *drm_find_displayid_extension(const struct edid *edid,
 
 	return displayid;
 }
+
+void displayid_iter_edid_begin(const struct edid *edid,
+			       struct displayid_iter *iter)
+{
+	memset(iter, 0, sizeof(*iter));
+
+	iter->edid = edid;
+}
+
+static const struct displayid_block *
+displayid_iter_block(const struct displayid_iter *iter)
+{
+	const struct displayid_block *block;
+
+	if (!iter->section)
+		return NULL;
+
+	block = (const struct displayid_block *)&iter->section[iter->idx];
+
+	if (iter->idx + sizeof(*block) <= iter->length &&
+	    iter->idx + sizeof(*block) + block->num_bytes <= iter->length &&
+	    block->num_bytes > 0)
+		return block;
+
+	return NULL;
+}
+
+const struct displayid_block *
+__displayid_iter_next(struct displayid_iter *iter)
+{
+	const struct displayid_block *block;
+
+	if (!iter->edid)
+		return NULL;
+
+	if (iter->section) {
+		/* current block should always be valid */
+		block = displayid_iter_block(iter);
+		if (WARN_ON(!block)) {
+			iter->section = NULL;
+			iter->edid = NULL;
+			return NULL;
+		}
+
+		/* next block in section */
+		iter->idx += sizeof(*block) + block->num_bytes;
+
+		block = displayid_iter_block(iter);
+		if (block)
+			return block;
+	}
+
+	for (;;) {
+		iter->section = drm_find_displayid_extension(iter->edid,
+							     &iter->length,
+							     &iter->idx,
+							     &iter->ext_index);
+		if (!iter->section) {
+			iter->edid = NULL;
+			return NULL;
+		}
+
+		iter->idx += sizeof(struct displayid_hdr);
+
+		block = displayid_iter_block(iter);
+		if (block)
+			return block;
+	}
+}
+
+void displayid_iter_end(struct displayid_iter *iter)
+{
+	memset(iter, 0, sizeof(*iter));
+}
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 3c6db22a518a..27e06c98db17 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -108,4 +108,22 @@ const u8 *drm_find_displayid_extension(const struct edid *edid,
 				       int *length, int *idx,
 				       int *ext_index);
 
+/* DisplayID iteration */
+struct displayid_iter {
+	const struct edid *edid;
+
+	const u8 *section;
+	int length;
+	int idx;
+	int ext_index;
+};
+
+void displayid_iter_edid_begin(const struct edid *edid,
+			       struct displayid_iter *iter);
+const struct displayid_block *
+__displayid_iter_next(struct displayid_iter *iter);
+#define displayid_iter_for_each(__block, __iter) \
+	while (((__block) = __displayid_iter_next(__iter)))
+void displayid_iter_end(struct displayid_iter *iter);
+
 #endif
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/8] drm/edid: use the new displayid iterator for detailed modes

[Jani Nikula]

Neatly reduce displayid boilerplate in code. No functional changes.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 58e61f792bc7..fbaa7d679cb2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5333,27 +5333,16 @@ static int add_displayid_detailed_1_modes(struct drm_connector *connector,
 static int add_displayid_detailed_modes(struct drm_connector *connector,
 					struct edid *edid)
 {
-	const u8 *displayid;
-	int length, idx;
 	const struct displayid_block *block;
+	struct displayid_iter iter;
 	int num_modes = 0;
-	int ext_index = 0;
-
-	for (;;) {
-		displayid = drm_find_displayid_extension(edid, &length, &idx,
-							 &ext_index);
-		if (!displayid)
-			break;
 
-		idx += sizeof(struct displayid_hdr);
-		for_each_displayid_db(displayid, block, idx, length) {
-			switch (block->tag) {
-			case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
-				num_modes += add_displayid_detailed_1_modes(connector, block);
-				break;
-			}
-		}
+	displayid_iter_edid_begin(edid, &iter);
+	displayid_iter_for_each(block, &iter) {
+		if (block->tag == DATA_BLOCK_TYPE_1_DETAILED_TIMING)
+			num_modes += add_displayid_detailed_1_modes(connector, block);
 	}
+	displayid_iter_end(&iter);
 
 	return num_modes;
 }
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/8] drm/edid: use the new displayid iterator for finding CEA extension

[Jani Nikula]

Neatly reduce displayid boilerplate in code. No functional changes.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fbaa7d679cb2..4526e2557dca 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3266,35 +3266,28 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
 
 static const u8 *drm_find_cea_extension(const struct edid *edid)
 {
-	int length, idx;
 	const struct displayid_block *block;
+	struct displayid_iter iter;
 	const u8 *cea;
-	const u8 *displayid;
-	int ext_index;
+	int ext_index = 0;
 
 	/* Look for a top level CEA extension block */
 	/* FIXME: make callers iterate through multiple CEA ext blocks? */
-	ext_index = 0;
 	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
 	if (cea)
 		return cea;
 
 	/* CEA blocks can also be found embedded in a DisplayID block */
-	ext_index = 0;
-	for (;;) {
-		displayid = drm_find_displayid_extension(edid, &length, &idx,
-							 &ext_index);
-		if (!displayid)
-			return NULL;
-
-		idx += sizeof(struct displayid_hdr);
-		for_each_displayid_db(displayid, block, idx, length) {
-			if (block->tag == DATA_BLOCK_CTA)
-				return (const u8 *)block;
+	displayid_iter_edid_begin(edid, &iter);
+	displayid_iter_for_each(block, &iter) {
+		if (block->tag == DATA_BLOCK_CTA) {
+			cea = (const u8 *)block;
+			break;
 		}
 	}
+	displayid_iter_end(&iter);
 
-	return NULL;
+	return cea;
 }
 
 static __always_inline const struct drm_display_mode *cea_mode_for_vic(u8 vic)
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/8] drm/edid: use the new displayid iterator for tile info

[Jani Nikula]

Neatly reduce displayid boilerplate in code. Remove excessive debug
logging while at it, no other functional changes.

The old displayid iterator becomes unused; remove it as well as make
drm_find_displayid_extension() static.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_displayid.c |  6 +++---
 drivers/gpu/drm/drm_edid.c      | 37 +++++++--------------------------
 include/drm/drm_displayid.h     | 12 -----------
 3 files changed, 10 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 5b809bb9fecc..902ff6114b68 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -33,9 +33,9 @@ static int validate_displayid(const u8 *displayid, int length, int idx)
 	return 0;
 }
 
-const u8 *drm_find_displayid_extension(const struct edid *edid,
-				       int *length, int *idx,
-				       int *ext_index)
+static const u8 *drm_find_displayid_extension(const struct edid *edid,
+					      int *length, int *idx,
+					      int *ext_index)
 {
 	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
 	const struct displayid_hdr *base;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 4526e2557dca..81d5f2524246 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5969,43 +5969,20 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
 	}
 }
 
-static void drm_displayid_parse_tiled(struct drm_connector *connector,
-				      const u8 *displayid, int length, int idx)
-{
-	const struct displayid_block *block;
-
-	idx += sizeof(struct displayid_hdr);
-	for_each_displayid_db(displayid, block, idx, length) {
-		DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
-			      block->tag, block->rev, block->num_bytes);
-
-		switch (block->tag) {
-		case DATA_BLOCK_TILED_DISPLAY:
-			drm_parse_tiled_block(connector, block);
-			break;
-		default:
-			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
-			break;
-		}
-	}
-}
-
 void drm_update_tile_info(struct drm_connector *connector,
 			  const struct edid *edid)
 {
-	const void *displayid = NULL;
-	int ext_index = 0;
-	int length, idx;
+	const struct displayid_block *block;
+	struct displayid_iter iter;
 
 	connector->has_tile = false;
-	for (;;) {
-		displayid = drm_find_displayid_extension(edid, &length, &idx,
-							 &ext_index);
-		if (!displayid)
-			break;
 
-		drm_displayid_parse_tiled(connector, displayid, length, idx);
+	displayid_iter_edid_begin(edid, &iter);
+	displayid_iter_for_each(block, &iter) {
+		if (block->tag == DATA_BLOCK_TILED_DISPLAY)
+			drm_parse_tiled_block(connector, block);
 	}
+	displayid_iter_end(&iter);
 
 	if (!connector->has_tile && connector->tile_group) {
 		drm_mode_put_tile_group(connector->dev, connector->tile_group);
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 27e06c98db17..10ee863f1734 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -96,18 +96,6 @@ struct displayid_detailed_timing_block {
 	struct displayid_detailed_timings_1 timings[];
 };
 
-#define for_each_displayid_db(displayid, block, idx, length) \
-	for ((block) = (const struct displayid_block *)&(displayid)[idx]; \
-	     (idx) + sizeof(struct displayid_block) <= (length) && \
-	     (idx) + sizeof(struct displayid_block) + (block)->num_bytes <= (length) && \
-	     (block)->num_bytes > 0; \
-	     (idx) += sizeof(struct displayid_block) + (block)->num_bytes, \
-	     (block) = (const struct displayid_block *)&(displayid)[idx])
-
-const u8 *drm_find_displayid_extension(const struct edid *edid,
-				       int *length, int *idx,
-				       int *ext_index);
-
 /* DisplayID iteration */
 struct displayid_iter {
 	const struct edid *edid;
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 7/8] drm/displayid: allow data blocks with 0 payload length

[Jani Nikula]

The DisplayID specifications explicitly call out 0 as a valid payload
length for data blocks. The mere presence of a data block, or the
information coded in the block specific data (bits 7:3 in offset 1), may
be enough to convey the necessary information.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_displayid.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 902ff6114b68..e0b9e58a9dc8 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -77,8 +77,7 @@ displayid_iter_block(const struct displayid_iter *iter)
 	block = (const struct displayid_block *)&iter->section[iter->idx];
 
 	if (iter->idx + sizeof(*block) <= iter->length &&
-	    iter->idx + sizeof(*block) + block->num_bytes <= iter->length &&
-	    block->num_bytes > 0)
+	    iter->idx + sizeof(*block) + block->num_bytes <= iter->length)
 		return block;
 
 	return NULL;
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 8/8] drm/displayid: rename displayid_hdr to displayid_header

[Jani Nikula]

Avoid any confusion with High Dynamic Range. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_displayid.c | 10 +++++-----
 include/drm/drm_displayid.h     |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index e0b9e58a9dc8..32da557b960f 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -11,9 +11,9 @@ static int validate_displayid(const u8 *displayid, int length, int idx)
 {
 	int i, dispid_length;
 	u8 csum = 0;
-	const struct displayid_hdr *base;
+	const struct displayid_header *base;
 
-	base = (const struct displayid_hdr *)&displayid[idx];
+	base = (const struct displayid_header *)&displayid[idx];
 
 	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
 		      base->rev, base->bytes, base->prod_id, base->ext_count);
@@ -38,7 +38,7 @@ static const u8 *drm_find_displayid_extension(const struct edid *edid,
 					      int *ext_index)
 {
 	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
-	const struct displayid_hdr *base;
+	const struct displayid_header *base;
 	int ret;
 
 	if (!displayid)
@@ -52,7 +52,7 @@ static const u8 *drm_find_displayid_extension(const struct edid *edid,
 	if (ret)
 		return NULL;
 
-	base = (const struct displayid_hdr *)&displayid[*idx];
+	base = (const struct displayid_header *)&displayid[*idx];
 	*length = *idx + sizeof(*base) + base->bytes;
 
 	return displayid;
@@ -118,7 +118,7 @@ __displayid_iter_next(struct displayid_iter *iter)
 			return NULL;
 		}
 
-		iter->idx += sizeof(struct displayid_hdr);
+		iter->idx += sizeof(struct displayid_header);
 
 		block = displayid_iter_block(iter);
 		if (block)
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 10ee863f1734..ec64d141f578 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -56,7 +56,7 @@ struct edid;
 #define PRODUCT_TYPE_REPEATER 5
 #define PRODUCT_TYPE_DIRECT_DRIVE 6
 
-struct displayid_hdr {
+struct displayid_header {
 	u8 rev;
 	u8 bytes;
 	u8 prod_id;
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/8] drm/displayid: allow data blocks with 0 payload length

[Ville Syrjälä]

On Mon, Mar 29, 2021 at 04:37:21PM +0300, Jani Nikula wrote:
> The DisplayID specifications explicitly call out 0 as a valid payload
> length for data blocks. The mere presence of a data block, or the
> information coded in the block specific data (bits 7:3 in offset 1), may
> be enough to convey the necessary information.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Though after looking at the current users it looks to me like
we're missing some block length checks. In particular
drm_parse_tiled_block() looks suspect. Judging by what I wrote
in cea_db_offsets() I think I once convinced myself that the
CEA ext block stuff is safe. And add_displayid_detailed_1_modes()
looks OK as well.

> ---
>  drivers/gpu/drm/drm_displayid.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
> index 902ff6114b68..e0b9e58a9dc8 100644
> --- a/drivers/gpu/drm/drm_displayid.c
> +++ b/drivers/gpu/drm/drm_displayid.c
> @@ -77,8 +77,7 @@ displayid_iter_block(const struct displayid_iter *iter)
>  	block = (const struct displayid_block *)&iter->section[iter->idx];
>  
>  	if (iter->idx + sizeof(*block) <= iter->length &&
> -	    iter->idx + sizeof(*block) + block->num_bytes <= iter->length &&
> -	    block->num_bytes > 0)
> +	    iter->idx + sizeof(*block) + block->num_bytes <= iter->length)
>  		return block;
>  
>  	return NULL;
> -- 
> 2.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 8/8] drm/displayid: rename displayid_hdr to displayid_header

[Ville Syrjälä]

On Mon, Mar 29, 2021 at 04:37:22PM +0300, Jani Nikula wrote:
> Avoid any confusion with High Dynamic Range. No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_displayid.c | 10 +++++-----
>  include/drm/drm_displayid.h     |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
> index e0b9e58a9dc8..32da557b960f 100644
> --- a/drivers/gpu/drm/drm_displayid.c
> +++ b/drivers/gpu/drm/drm_displayid.c
> @@ -11,9 +11,9 @@ static int validate_displayid(const u8 *displayid, int length, int idx)
>  {
>  	int i, dispid_length;
>  	u8 csum = 0;
> -	const struct displayid_hdr *base;
> +	const struct displayid_header *base;
>  
> -	base = (const struct displayid_hdr *)&displayid[idx];
> +	base = (const struct displayid_header *)&displayid[idx];
>  
>  	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
>  		      base->rev, base->bytes, base->prod_id, base->ext_count);
> @@ -38,7 +38,7 @@ static const u8 *drm_find_displayid_extension(const struct edid *edid,
>  					      int *ext_index)
>  {
>  	const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
> -	const struct displayid_hdr *base;
> +	const struct displayid_header *base;
>  	int ret;
>  
>  	if (!displayid)
> @@ -52,7 +52,7 @@ static const u8 *drm_find_displayid_extension(const struct edid *edid,
>  	if (ret)
>  		return NULL;
>  
> -	base = (const struct displayid_hdr *)&displayid[*idx];
> +	base = (const struct displayid_header *)&displayid[*idx];
>  	*length = *idx + sizeof(*base) + base->bytes;
>  
>  	return displayid;
> @@ -118,7 +118,7 @@ __displayid_iter_next(struct displayid_iter *iter)
>  			return NULL;
>  		}
>  
> -		iter->idx += sizeof(struct displayid_hdr);
> +		iter->idx += sizeof(struct displayid_header);
>  
>  		block = displayid_iter_block(iter);
>  		if (block)
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 10ee863f1734..ec64d141f578 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -56,7 +56,7 @@ struct edid;
>  #define PRODUCT_TYPE_REPEATER 5
>  #define PRODUCT_TYPE_DIRECT_DRIVE 6
>  
> -struct displayid_hdr {
> +struct displayid_header {
>  	u8 rev;
>  	u8 bytes;
>  	u8 prod_id;
> -- 
> 2.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/8] drm/edid: overhaul displayid iterator

[Jani Nikula]

On Mon, 29 Mar 2021, Jani Nikula <jani.nikula@intel.com> wrote:
> v2 of [1], addressing Ville's review comments, and adding a couple of
> extra patches on top.
>
> BR,
> Jani.
>
> [1] https://patchwork.freedesktop.org/series/87802/

Pushed to drm-misc-next, with Ville's review and Maxime's IRC nod of
approval. Thanks. :)

BR,
Jani.

>
>
> Jani Nikula (8):
>   drm/edid: make a number of functions, parameters and variables const
>   drm/displayid: add separate drm_displayid.c
>   drm/displayid: add new displayid section/block iterators
>   drm/edid: use the new displayid iterator for detailed modes
>   drm/edid: use the new displayid iterator for finding CEA extension
>   drm/edid: use the new displayid iterator for tile info
>   drm/displayid: allow data blocks with 0 payload length
>   drm/displayid: rename displayid_hdr to displayid_header
>
>  drivers/gpu/drm/Makefile        |   2 +-
>  drivers/gpu/drm/drm_displayid.c | 132 ++++++++++++++++++++++++
>  drivers/gpu/drm/drm_edid.c      | 171 +++++++-------------------------
>  include/drm/drm_displayid.h     |  30 ++++--
>  include/drm/drm_edid.h          |   3 +
>  5 files changed, 196 insertions(+), 142 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_displayid.c

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel