[RFC v1 0/6] drm/edid: overhaul displayid iterator

[RFC v1 0/6] drm/edid: overhaul displayid iterator

[Jani Nikula]

Iterating DisplayID is overcomplicated as it is, and it's not getting
easier when we eventually add support for DisplayID from DDC 0xA4
instead of EDID extensions. Prepare by abstracting the complexities away
from EDID code.

Untested, let's see what our CI thinks. ;)


Jani Nikula (6):
  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

 drivers/gpu/drm/Makefile        |   2 +-
 drivers/gpu/drm/drm_displayid.c | 133 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c      | 171 +++++++-------------------------
 include/drm/drm_displayid.h     |  28 ++++--
 include/drm/drm_edid.h          |   3 +
 5 files changed, 196 insertions(+), 141 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

[RFC v1 1/6] 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.

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

[RFC v1 2/6] 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.

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 5eb5bf7c16e3..78ef2fd14f10 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

[RFC v1 3/6] 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.

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..88070267aac9 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(struct displayid_block) <= iter->length &&
+	    iter->idx + sizeof(struct displayid_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(struct displayid_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

[RFC v1 4/6] drm/edid: use the new displayid iterator for detailed modes

[Jani Nikula]

Neatly reduce displayid boilerplate in code. No functional changes.

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

[RFC v1 5/6] drm/edid: use the new displayid iterator for finding CEA extension

[Jani Nikula]

Neatly reduce displayid boilerplate in code. No functional changes.

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

[RFC v1 6/6] 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.

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 88070267aac9..b146f2d0d72a 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

Re: [Intel-gfx] [RFC v1 1/6] drm/edid: make a number of functions, parameters and variables const

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:09PM +0200, Jani Nikula wrote:
> 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.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

const is good.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [RFC v1 2/6] drm/displayid: add separate drm_displayid.c

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:10PM +0200, Jani Nikula wrote:
> 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.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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 5eb5bf7c16e3..78ef2fd14f10 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFC v1 3/6] drm/displayid: add new displayid section/block iterators

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:11PM +0200, Jani Nikula wrote:
> 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.
> 
> 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..88070267aac9 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(struct displayid_block) <= iter->length &&
> +	    iter->idx + sizeof(struct displayid_block) + block->num_bytes <= iter->length &&

sizeof(*block) perhaps

> +	    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(struct displayid_block) + block->num_bytes;

ditto

Looks like this should do the same thing the current thing does,
or at least I couldn't immediately spot nay differences.

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

> +
> +		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

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

Re: [RFC v1 4/6] drm/edid: use the new displayid iterator for detailed modes

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:12PM +0200, Jani Nikula wrote:
> Neatly reduce displayid boilerplate in code. 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_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

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

Re: [RFC v1 5/6] drm/edid: use the new displayid iterator for finding CEA extension

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:13PM +0200, Jani Nikula wrote:
> Neatly reduce displayid boilerplate in code. 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_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

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

Re: [Intel-gfx] [RFC v1 6/6] drm/edid: use the new displayid iterator for tile info

[Ville Syrjälä]

On Tue, Mar 09, 2021 at 03:54:14PM +0200, Jani Nikula wrote:
> 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.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

Hopeuflly you tested with some of those weird EDIDs with
multiple DisplayID and/or CEA blocks ;)

I was thinking we should try to coalesce an iterator for
the CEA stuff as well...

> ---
>  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 88070267aac9..b146f2d0d72a 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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