[PATCH 0/5] drm/edid: Convert cea_ext parsers to use struct cea_db *

[PATCH 0/5] drm/edid: Convert cea_ext parsers to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
	FIXME: convert parsers to use struct cea_db
in the drm_edid cea_ext parsers.
1) drm_parse_hdmi_vsdb_video
2) drm_parse_hdmi_forum_scds
3) drm_parse_microsoft_vsdb
4) drm_parse_vcdb
5) drm_parse_hdr_metadata_block

These patches are pre-requisite to address another FIXME
/* FIXME: Transition to passing struct cea_db * everywhere. */
based on feedback from
https://lore.kernel.org/all/20241011152929.10145-1-vamsikrishna.brahmajosyula@gmail.com/
which will be sent in subsequent patch series.

In all the patches in this series,
db[n] has changed to data[n-1] since db={u8 len, u8 *data}.

Vamsi Krishna Brahmajosyula (5):
  drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *
  drm/edid: convert drm_parse_hdmi_forum_scds to use struct cea_db *
  drm/edid: convert drm_parse_microsoft_vsdb to use struct cea_db *
  drm/edid: convert drm_parse_vcdb to use struct cea_db *
  drm/edid: convert drm_parse_hdr_metadata_block to use struct cea_db *

 drivers/gpu/drm/drm_edid.c | 126 +++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 60 deletions(-)


base-commit: 850925a8133c73c4a2453c360b2c3beb3bab67c9
-- 
2.47.0

[PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
	FIXME: convert parsers to use struct cea_db
in the parser drm_parse_hdmi_vsdb_video

cea_db contains len and then data. Appropriately change the indices
when referring to individual elements (db[n] becomes data[n-1]).

Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 855beafb76ff..e07e39c0caa0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6259,32 +6259,33 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 }
 
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
-					   const u8 *hdmi)
+					   const struct cea_db *db)
 {
 	struct drm_display_info *info = &connector->display_info;
 	unsigned int dc_bpc = 0;
+	const u8 *hdmi = cea_db_data(db);
 
 	/* HDMI supports at least 8 bpc */
 	info->bpc = 8;
 
-	if (cea_db_payload_len(hdmi) < 6)
+	if (cea_db_payload_len(db) < 6)
 		return;
 
-	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
+	if (hdmi[5] & DRM_EDID_HDMI_DC_30) {
 		dc_bpc = 10;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30;
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 30.\n",
 			    connector->base.id, connector->name);
 	}
 
-	if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
+	if (hdmi[5] & DRM_EDID_HDMI_DC_36) {
 		dc_bpc = 12;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36;
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 36.\n",
 			    connector->base.id, connector->name);
 	}
 
-	if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
+	if (hdmi[5] & DRM_EDID_HDMI_DC_48) {
 		dc_bpc = 16;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48;
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 48.\n",
@@ -6302,7 +6303,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	info->bpc = dc_bpc;
 
 	/* YCRCB444 is optional according to spec. */
-	if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
+	if (hdmi[5] & DRM_EDID_HDMI_DC_Y444) {
 		info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes;
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does YCRCB444 in deep color.\n",
 			    connector->base.id, connector->name);
@@ -6312,7 +6313,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	 * Spec says that if any deep color mode is supported at all,
 	 * then deep color 36 bit must be supported.
 	 */
-	if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
+	if (!(hdmi[5] & DRM_EDID_HDMI_DC_36)) {
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink should do DC_36, but does not!\n",
 			    connector->base.id, connector->name);
 	}
@@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 
 /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
 static void
-drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
+drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
 {
 	struct drm_display_info *info = &connector->display_info;
 	u8 len = cea_db_payload_len(db);
+	const u8 *data = cea_db_data(db);
 
 	info->is_hdmi = true;
 
-	info->source_physical_address = (db[4] << 8) | db[5];
+	info->source_physical_address = (data[3] << 8) | data[4];
 
 	if (len >= 6)
-		info->dvi_dual = db[6] & 1;
+		info->dvi_dual = data[5] & 1;
 	if (len >= 7)
-		info->max_tmds_clock = db[7] * 5000;
+		info->max_tmds_clock = data[6] * 5000;
 
 	/*
 	 * Try to infer whether the sink supports HDMI infoframes.
@@ -6340,7 +6342,7 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	 * HDMI infoframe support was first added in HDMI 1.4. Assume the sink
 	 * supports infoframes if HDMI_Video_present is set.
 	 */
-	if (len >= 8 && db[8] & BIT(5))
+	if (len >= 8 && data[7] & BIT(5))
 		info->has_hdmi_infoframe = true;
 
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n",
@@ -6412,7 +6414,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 		const u8 *data = (const u8 *)db;
 
 		if (cea_db_is_hdmi_vsdb(db))
-			drm_parse_hdmi_vsdb_video(connector, data);
+			drm_parse_hdmi_vsdb_video(connector, db);
 		else if (cea_db_is_hdmi_forum_vsdb(db) ||
 			 cea_db_is_hdmi_forum_scdb(db))
 			drm_parse_hdmi_forum_scds(connector, data);
-- 
2.47.0

[PATCH 2/5] drm/edid: convert drm_parse_hdmi_forum_scds to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
	FIXME: convert parsers to use struct cea_db
in the parser drm_parse_hdmi_forum_scds and related methods

cea_db contains len and then data. Appropriately change the indices
when referring to individual elements (db[n] becomes data[n-1]).

Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 56 ++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e07e39c0caa0..f79d8fbdb62b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6112,45 +6112,48 @@ void drm_get_max_frl_rate(int max_frl_rate, u8 *max_lanes, u8 *max_rate_per_lane
 }
 
 static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
-					       const u8 *db)
+					       const struct cea_db *db)
 {
 	u8 dc_mask;
 	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+	const u8 *data = cea_db_data(db);
 
-	dc_mask = db[7] & DRM_EDID_YCBCR420_DC_MASK;
+	dc_mask = data[6] & DRM_EDID_YCBCR420_DC_MASK;
 	hdmi->y420_dc_modes = dc_mask;
 }
 
 static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
-			       const u8 *hf_scds)
+			       const struct cea_db *db)
 {
-	hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
+	const u8 *hf_scds = cea_db_data(db);
+
+	hdmi_dsc->v_1p2 = hf_scds[10] & DRM_EDID_DSC_1P2;
 
 	if (!hdmi_dsc->v_1p2)
 		return;
 
-	hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
-	hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
+	hdmi_dsc->native_420 = hf_scds[10] & DRM_EDID_DSC_NATIVE_420;
+	hdmi_dsc->all_bpp = hf_scds[10] & DRM_EDID_DSC_ALL_BPP;
 
-	if (hf_scds[11] & DRM_EDID_DSC_16BPC)
+	if (hf_scds[10] & DRM_EDID_DSC_16BPC)
 		hdmi_dsc->bpc_supported = 16;
-	else if (hf_scds[11] & DRM_EDID_DSC_12BPC)
+	else if (hf_scds[10] & DRM_EDID_DSC_12BPC)
 		hdmi_dsc->bpc_supported = 12;
-	else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
+	else if (hf_scds[10] & DRM_EDID_DSC_10BPC)
 		hdmi_dsc->bpc_supported = 10;
 	else
 		/* Supports min 8 BPC if DSC 1.2 is supported*/
 		hdmi_dsc->bpc_supported = 8;
 
-	if (cea_db_payload_len(hf_scds) >= 12 && hf_scds[12]) {
+	if (cea_db_payload_len(db) >= 12 && hf_scds[11]) {
 		u8 dsc_max_slices;
 		u8 dsc_max_frl_rate;
 
-		dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
+		dsc_max_frl_rate = (hf_scds[11] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
 				     &hdmi_dsc->max_frl_rate_per_lane);
 
-		dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
+		dsc_max_slices = hf_scds[11] & DRM_EDID_DSC_MAX_SLICES;
 
 		switch (dsc_max_slices) {
 		case 1:
@@ -6188,26 +6191,27 @@ static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
 		}
 	}
 
-	if (cea_db_payload_len(hf_scds) >= 13 && hf_scds[13])
-		hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
+	if (cea_db_payload_len(db) >= 13 && hf_scds[12])
+		hdmi_dsc->total_chunk_kbytes = hf_scds[12] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
 }
 
 /* Sink Capability Data Structure */
 static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
-				      const u8 *hf_scds)
+				      const struct cea_db *db)
 {
 	struct drm_display_info *info = &connector->display_info;
 	struct drm_hdmi_info *hdmi = &info->hdmi;
 	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
+	const u8 *hf_scds = cea_db_data(db);
 	int max_tmds_clock = 0;
 	u8 max_frl_rate = 0;
 	bool dsc_support = false;
 
 	info->has_hdmi_infoframe = true;
 
-	if (hf_scds[6] & 0x80) {
+	if (hf_scds[5] & 0x80) {
 		hdmi->scdc.supported = true;
-		if (hf_scds[6] & 0x40)
+		if (hf_scds[5] & 0x40)
 			hdmi->scdc.read_request = true;
 	}
 
@@ -6220,11 +6224,11 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	 * Lets check it out.
 	 */
 
-	if (hf_scds[5]) {
+	if (hf_scds[4]) {
 		struct drm_scdc *scdc = &hdmi->scdc;
 
 		/* max clock is 5000 KHz times block value */
-		max_tmds_clock = hf_scds[5] * 5000;
+		max_tmds_clock = hf_scds[4] * 5000;
 
 		if (max_tmds_clock > 340000) {
 			info->max_tmds_clock = max_tmds_clock;
@@ -6234,21 +6238,21 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 			scdc->scrambling.supported = true;
 
 			/* Few sinks support scrambling for clocks < 340M */
-			if ((hf_scds[6] & 0x8))
+			if ((hf_scds[5] & 0x8))
 				scdc->scrambling.low_rates = true;
 		}
 	}
 
-	if (hf_scds[7]) {
-		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
+	if (hf_scds[6]) {
+		max_frl_rate = (hf_scds[6] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
 				     &hdmi->max_frl_rate_per_lane);
 	}
 
-	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
+	drm_parse_ycbcr420_deep_color_info(connector, db);
 
-	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
-		drm_parse_dsc_info(hdmi_dsc, hf_scds);
+	if (cea_db_payload_len(db) >= 11 && hf_scds[10]) {
+		drm_parse_dsc_info(hdmi_dsc, db);
 		dsc_support = true;
 	}
 
@@ -6417,7 +6421,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_hdmi_vsdb_video(connector, db);
 		else if (cea_db_is_hdmi_forum_vsdb(db) ||
 			 cea_db_is_hdmi_forum_scdb(db))
-			drm_parse_hdmi_forum_scds(connector, data);
+			drm_parse_hdmi_forum_scds(connector, db);
 		else if (cea_db_is_microsoft_vsdb(db))
 			drm_parse_microsoft_vsdb(connector, data);
 		else if (cea_db_is_y420cmdb(db))
-- 
2.47.0

[PATCH 3/5] drm/edid: convert drm_parse_microsoft_vsdb to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
	FIXME: convert parsers to use struct cea_db
in the parser drm_parse_microsoft_vsdb

cea_db contains len and then data. Appropriately change the indices
when referring to individual elements (db[n] becomes data[n-1]).

Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f79d8fbdb62b..e2ef07d00aaf 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6361,11 +6361,12 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *
  * https://docs.microsoft.com/en-us/windows-hardware/drivers/display/specialized-monitors-edid-extension
  */
 static void drm_parse_microsoft_vsdb(struct drm_connector *connector,
-				     const u8 *db)
+				     const struct cea_db *db)
 {
 	struct drm_display_info *info = &connector->display_info;
-	u8 version = db[4];
-	bool desktop_usage = db[5] & BIT(6);
+	const u8 *data = cea_db_data(db);
+	u8 version = data[3];
+	bool desktop_usage = data[4] & BIT(6);
 
 	/* Version 1 and 2 for HMDs, version 3 flags desktop usage explicitly */
 	if (version == 1 || version == 2 || (version == 3 && !desktop_usage))
@@ -6373,7 +6374,7 @@ static void drm_parse_microsoft_vsdb(struct drm_connector *connector,
 
 	drm_dbg_kms(connector->dev,
 		    "[CONNECTOR:%d:%s] HMD or specialized display VSDB version %u: 0x%02x\n",
-		    connector->base.id, connector->name, version, db[5]);
+		    connector->base.id, connector->name, version, data[4]);
 }
 
 static void drm_parse_cea_ext(struct drm_connector *connector,
@@ -6423,7 +6424,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			 cea_db_is_hdmi_forum_scdb(db))
 			drm_parse_hdmi_forum_scds(connector, db);
 		else if (cea_db_is_microsoft_vsdb(db))
-			drm_parse_microsoft_vsdb(connector, data);
+			drm_parse_microsoft_vsdb(connector, db);
 		else if (cea_db_is_y420cmdb(db))
 			parse_cta_y420cmdb(connector, db, &y420cmdb_map);
 		else if (cea_db_is_y420vdb(db))
-- 
2.47.0

[PATCH 4/5] drm/edid: convert drm_parse_vcdb to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
        FIXME: convert parsers to use struct cea_db
in the parser drm_parse_vcdb

cea_db contains len and then data. Appropriately change the indices
when referring to individual elements (db[n] becomes data[n-1]).

Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e2ef07d00aaf..d9fa994a3acc 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6065,14 +6065,15 @@ static void parse_cta_y420vdb(struct drm_connector *connector,
 	}
 }
 
-static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
+static void drm_parse_vcdb(struct drm_connector *connector, const struct cea_db *db)
 {
 	struct drm_display_info *info = &connector->display_info;
+	const u8 *data = cea_db_data(db);
 
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] CEA VCDB 0x%02x\n",
-		    connector->base.id, connector->name, db[2]);
+		    connector->base.id, connector->name, data[1]);
 
-	if (db[2] & EDID_CEA_VCDB_QS)
+	if (data[1] & EDID_CEA_VCDB_QS)
 		info->rgb_quant_range_selectable = true;
 }
 
@@ -6430,7 +6431,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 		else if (cea_db_is_y420vdb(db))
 			parse_cta_y420vdb(connector, db);
 		else if (cea_db_is_vcdb(db))
-			drm_parse_vcdb(connector, data);
+			drm_parse_vcdb(connector, db);
 		else if (cea_db_is_hdmi_hdr_metadata_block(db))
 			drm_parse_hdr_metadata_block(connector, data);
 		else if (cea_db_tag(db) == CTA_DB_VIDEO)
-- 
2.47.0

[PATCH 5/5] drm/edid: convert drm_parse_hdr_metadata_block to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Address the following
	FIXME: convert parsers to use struct cea_db
in the parser drm_parse_hdr_metadata_block

cea_db contains len and then data. Appropriately change the indices
when referring to individual elements (db[n] becomes data[n-1]).

Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d9fa994a3acc..8b2b75885027 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5418,7 +5418,7 @@ static void drm_calculate_luminance_range(struct drm_connector *connector)
 
 static uint8_t eotf_supported(const u8 *edid_ext)
 {
-	return edid_ext[2] &
+	return edid_ext[1] &
 		(BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) |
 		 BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) |
 		 BIT(HDMI_EOTF_SMPTE_ST2084) |
@@ -5427,28 +5427,29 @@ static uint8_t eotf_supported(const u8 *edid_ext)
 
 static uint8_t hdr_metadata_type(const u8 *edid_ext)
 {
-	return edid_ext[3] &
+	return edid_ext[2] &
 		BIT(HDMI_STATIC_METADATA_TYPE1);
 }
 
 static void
-drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
+drm_parse_hdr_metadata_block(struct drm_connector *connector, const struct cea_db *db)
 {
 	u16 len;
 
 	len = cea_db_payload_len(db);
+	const u8 *data = cea_db_data(db);
 
 	connector->hdr_sink_metadata.hdmi_type1.eotf =
-						eotf_supported(db);
+						eotf_supported(data);
 	connector->hdr_sink_metadata.hdmi_type1.metadata_type =
-						hdr_metadata_type(db);
+						hdr_metadata_type(data);
 
 	if (len >= 4)
-		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
+		connector->hdr_sink_metadata.hdmi_type1.max_cll = data[3];
 	if (len >= 5)
-		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
+		connector->hdr_sink_metadata.hdmi_type1.max_fall = data[4];
 	if (len >= 6) {
-		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
+		connector->hdr_sink_metadata.hdmi_type1.min_cll = data[5];
 
 		/* Calculate only when all values are available */
 		drm_calculate_luminance_range(connector);
@@ -6416,9 +6417,6 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 
 	cea_db_iter_edid_begin(drm_edid, &iter);
 	cea_db_iter_for_each(db, &iter) {
-		/* FIXME: convert parsers to use struct cea_db */
-		const u8 *data = (const u8 *)db;
-
 		if (cea_db_is_hdmi_vsdb(db))
 			drm_parse_hdmi_vsdb_video(connector, db);
 		else if (cea_db_is_hdmi_forum_vsdb(db) ||
@@ -6433,7 +6431,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 		else if (cea_db_is_vcdb(db))
 			drm_parse_vcdb(connector, db);
 		else if (cea_db_is_hdmi_hdr_metadata_block(db))
-			drm_parse_hdr_metadata_block(connector, data);
+			drm_parse_hdr_metadata_block(connector, db);
 		else if (cea_db_tag(db) == CTA_DB_VIDEO)
 			parse_cta_vdb(connector, db);
 		else if (cea_db_tag(db) == CTA_DB_AUDIO)
-- 
2.47.0

Re: [PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *

[Jani Nikula]

On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com> wrote:
> @@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  
>  /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
>  static void
> -drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
> +drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  	u8 len = cea_db_payload_len(db);
> +	const u8 *data = cea_db_data(db);
>  
>  	info->is_hdmi = true;
>  
> -	info->source_physical_address = (db[4] << 8) | db[5];
> +	info->source_physical_address = (data[3] << 8) | data[4];
>  
>  	if (len >= 6)
> -		info->dvi_dual = db[6] & 1;
> +		info->dvi_dual = data[5] & 1;

Just commenting on one hunk, because it's a good example of the whole
series I think.

The above is nice, because it improves the offset vs. length
comparisons. Many of the old checks like above look like off-by-ones,
when indexing from the beginning of the data block, not from the
beginning of payload, and cea_db_payload_len() excludes the first byte.

The main problem is that the specs are written with indexing from the
beginning of the data block. For example, HDMI 1.4 table 8-16 defining
the HDMI VSDB says source physical address is at byte offsets 4 and 5,
and dvi dual flag at byte offset 6. That will no longer be the case in
code. It gets tricky to review when you have to keep adjusting the
offsets in your head. (I don't remember if there are specs that specify
the offsets starting from the "actual" payload after all the meta stuff
has been removed.)

Now, if we accept having to do that mental acrobatics, why stop there?
You also have extended tags (first payload byte is the tag), as well as
vendor tags (first three payload bytes are the OUI). It begs the
question whether there should be higher level data and length helpers
that identify and remove the tags (including extended tags and OUI
stuff). For example, the actual data for HDMI VSDB starts at payload
offset 3, as the first three bytes are the HDMI OUI.

What to do? Ville, thoughts?


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *

[Ville Syrjälä]

On Mon, Oct 28, 2024 at 03:45:07PM +0200, Jani Nikula wrote:
> On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com> wrote:
> > @@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> >  
> >  /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
> >  static void
> > -drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
> > +drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
> >  {
> >  	struct drm_display_info *info = &connector->display_info;
> >  	u8 len = cea_db_payload_len(db);
> > +	const u8 *data = cea_db_data(db);
> >  
> >  	info->is_hdmi = true;
> >  
> > -	info->source_physical_address = (db[4] << 8) | db[5];
> > +	info->source_physical_address = (data[3] << 8) | data[4];
> >  
> >  	if (len >= 6)
> > -		info->dvi_dual = db[6] & 1;
> > +		info->dvi_dual = data[5] & 1;
> 
> Just commenting on one hunk, because it's a good example of the whole
> series I think.
> 
> The above is nice, because it improves the offset vs. length
> comparisons. Many of the old checks like above look like off-by-ones,
> when indexing from the beginning of the data block, not from the
> beginning of payload, and cea_db_payload_len() excludes the first byte.
> 
> The main problem is that the specs are written with indexing from the
> beginning of the data block. For example, HDMI 1.4 table 8-16 defining
> the HDMI VSDB says source physical address is at byte offsets 4 and 5,
> and dvi dual flag at byte offset 6. That will no longer be the case in
> code. It gets tricky to review when you have to keep adjusting the
> offsets in your head. (I don't remember if there are specs that specify
> the offsets starting from the "actual" payload after all the meta stuff
> has been removed.)

IIRC there was some off-by-one indexing difference between
some of the specs. But I don't remember which ones use what.

> 
> Now, if we accept having to do that mental acrobatics, why stop there?
> You also have extended tags (first payload byte is the tag), as well as
> vendor tags (first three payload bytes are the OUI). It begs the
> question whether there should be higher level data and length helpers
> that identify and remove the tags (including extended tags and OUI
> stuff). For example, the actual data for HDMI VSDB starts at payload
> offset 3, as the first three bytes are the HDMI OUI.
> 
> What to do? Ville, thoughts?

So just different *_{data,len}() for the different indexing variants
(as defined by the relevant spec)? That seems like a reasonable
apporach as then the len vs. index checks might actually make sense.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Thanks for the feedback.

On Mon, Oct 28, 2024 at 8:09 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Oct 28, 2024 at 03:45:07PM +0200, Jani Nikula wrote:
> > On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com> wrote:
> > > @@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> > >
> > >  /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
> > >  static void
> > > -drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
> > > +drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
> > >  {
> > >     struct drm_display_info *info = &connector->display_info;
> > >     u8 len = cea_db_payload_len(db);
> > > +   const u8 *data = cea_db_data(db);
> > >
> > >     info->is_hdmi = true;
> > >
> > > -   info->source_physical_address = (db[4] << 8) | db[5];
> > > +   info->source_physical_address = (data[3] << 8) | data[4];
> > >
> > >     if (len >= 6)
> > > -           info->dvi_dual = db[6] & 1;
> > > +           info->dvi_dual = data[5] & 1;
> >
> > Just commenting on one hunk, because it's a good example of the whole
> > series I think.
> >
> > The above is nice, because it improves the offset vs. length
> > comparisons. Many of the old checks like above look like off-by-ones,
> > when indexing from the beginning of the data block, not from the
> > beginning of payload, and cea_db_payload_len() excludes the first byte.
> >
> > The main problem is that the specs are written with indexing from the
> > beginning of the data block. For example, HDMI 1.4 table 8-16 defining
> > the HDMI VSDB says source physical address is at byte offsets 4 and 5,
> > and dvi dual flag at byte offset 6. That will no longer be the case in
> > code. It gets tricky to review when you have to keep adjusting the
> > offsets in your head. (I don't remember if there are specs that specify
> > the offsets starting from the "actual" payload after all the meta stuff
> > has been removed.)
>
> IIRC there was some off-by-one indexing difference between
> some of the specs. But I don't remember which ones use what.
>
> >
> > Now, if we accept having to do that mental acrobatics, why stop there?
> > You also have extended tags (first payload byte is the tag), as well as
> > vendor tags (first three payload bytes are the OUI). It begs the
> > question whether there should be higher level data and length helpers
> > that identify and remove the tags (including extended tags and OUI
> > stuff). For example, the actual data for HDMI VSDB starts at payload
> > offset 3, as the first three bytes are the HDMI OUI.
> >
> > What to do? Ville, thoughts?
>
> So just different *_{data,len}() for the different indexing variants
> (as defined by the relevant spec)? That seems like a reasonable
> apporach as then the len vs. index checks might actually make sense.
>
Please let me know if the below snippet matches the feedback.

const u8 *vsdb_data = cea_db_to_vsdb_data(db); // skips indexes for HDMI OUI
...
info->source_physical_address = (vsdb_data[0] << 8) | vsdb_data[1];
...
info->dvi_dual = vsdb_data[2] & 1;
> --
> Ville Syrjälä
> Intel

Thanks,
Vamsi