drm/edid: info & modes parsing and drm_edid refactors

We try to avoid sending VICs defined in the later specs in AVI infoframes to sinks that conform to the earlier specs, to not upset them, and use 0 for the VIC instead. However, we do this detection and conversion to 0 too early, as we'll need the actual VIC to figure out the aspect ratio.

In particular, for a mode with 64:27 aspect ratio, 0 for VIC fails the AVI infoframe generation altogether with -EINVAL.

Separate the VIC lookup from the "filtering", and postpone the filtering, to use the proper VIC for aspect ratio handling, and the 0 VIC for the infoframe video code as needed.

Reported-by William Tseng <[email protected]>

Closes https://gitlab.freedesktop.org/drm/intel/-/issues/6153

References: https://lore.kernel.org/r/[email protected]

Cc <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

drivers/gpu/drm/drm_edid.c +12 -9

index 3841aba17abd..23f7146e6a9b 100644
@@ -6885,8 +6885,6 @@ static u8 drm_mode_hdmi_vic(const struct drm_connector *connector,
 static u8 drm_mode_cea_vic(const struct drm_connector *connector,
 			   const struct drm_display_mode *mode)
 {
-	u8 vic;
-
 	/*
 	 * HDMI spec says if a mode is found in HDMI 1.4b 4K modes
 	 * we should send its VIC in vendor infoframes, else send the

@@ -6896,13 +6894,18 @@ static u8 drm_mode_cea_vic(const struct drm_connector *connector,
 	if (drm_mode_hdmi_vic(connector, mode))
 		return 0;
 
-	vic = drm_match_cea_mode(mode);
+	return drm_match_cea_mode(mode);
+}
 
-	/*
-	 * HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
-	 * HDMI 2.0 VIC range: 1 <= VIC <= 107 (CEA-861-F). So we
-	 * have to make sure we dont break HDMI 1.4 sinks.
-	 */
+/*
+ * Avoid sending VICs defined in HDMI 2.0 in AVI infoframes to sinks that
+ * conform to HDMI 1.4.
+ *
+ * HDMI 1.4 (CTA-861-D) VIC range: [1..64]
+ * HDMI 2.0 (CTA-861-F) VIC range: [1..107]
+ */
+static u8 vic_for_avi_infoframe(const struct drm_connector *connector, u8 vic)
+{
 	if (!is_hdmi2_sink(connector) && vic > 64)
 		return 0;
 

@@ -6978,7 +6981,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
 	}
 
-	frame->video_code = vic;
+	frame->video_code = vic_for_avi_infoframe(connector, vic);
 	frame->picture_aspect = picture_aspect;
 	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
 	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;

Signature · 1 line

2.34.1

Commit 537d9ed2f6c1 ("drm/edid: convert add_cea_modes() to use cea db iter") inadvertently moved the do_hdmi_vsdb_modes() call within the db iteration loop, always passing NULL as the CTA VDB to do_hdmi_vsdb_modes(), skipping a lot of stereo modes.

Move the call back outside of the loop.

This does mean only one CTA VDB and HDMI VSDB combination will be handled, but it's an unlikely scenario to have more than one of either block, and it was not accounted for before the regression either.

Fixes 537d9ed2f6c1 ("drm/edid: convert add_cea_modes() to use cea db iter")

Cc <[email protected]> # v6.0+

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

drivers/gpu/drm/drm_edid.c +2 -12

index 23f7146e6a9b..b94adb9bbefb 100644
@@ -5249,13 +5249,12 @@ static int add_cea_modes(struct drm_connector *connector,
 {
 	const struct cea_db *db;
 	struct cea_db_iter iter;
+	const u8 *hdmi = NULL, *video = NULL;
+	u8 hdmi_len = 0, video_len = 0;
 	int modes = 0;
 
 	cea_db_iter_edid_begin(drm_edid, &iter);
 	cea_db_iter_for_each(db, &iter) {
-		const u8 *hdmi = NULL, *video = NULL;
-		u8 hdmi_len = 0, video_len = 0;
-
 		if (cea_db_tag(db) == CTA_DB_VIDEO) {
 			video = cea_db_data(db);
 			video_len = cea_db_payload_len(db);

@@ -5271,18 +5270,17 @@ static int add_cea_modes(struct drm_connector *connector,
 			modes += do_y420vdb_modes(connector, vdb420,
 						  cea_db_payload_len(db) - 1);
 		}
-
-		/*
-		 * We parse the HDMI VSDB after having added the cea modes as we
-		 * will be patching their flags when the sink supports stereo
-		 * 3D.
-		 */
-		if (hdmi)
-			modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len,
-						    video, video_len);

drivers/gpu/drm/drm_edid.c +1 -0

+
 	return modes;
 }
 

Signature · 1 line

2.34.1

A number of places need access to the VICs. Just parse them early for easy access. Gracefully handle multiple CTA VDBs. It's unlikely to have more than one, but the CTA-861 references "Video Data Block(s)", so err on the safe side.

Start parsing them now, convert users in follow-up to have fewer moving parts in one go.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_connector.c |  1 +
 drivers/gpu/drm/drm_edid.c      | 36 +++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     | 10 +++++++++
 3 files changed, 47 insertions(+)

drivers/gpu/drm/drm_connector.c +1 -0

index 8d92777e57dd..21b3df75ab8c 100644
@@ -565,6 +565,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	ida_free(&dev->mode_config.connector_ida, connector->index);
 
 	kfree(connector->display_info.bus_formats);
+	kfree(connector->display_info.vics);
 	drm_mode_object_unregister(dev, &connector->base);
 	kfree(connector->name);
 	connector->name = NULL;

drivers/gpu/drm/drm_edid.c +36 -0

index b94adb9bbefb..90846dc69061 100644
@@ -5862,6 +5862,36 @@ drm_default_rgb_quant_range(const struct drm_display_mode *mode)
 }
 EXPORT_SYMBOL(drm_default_rgb_quant_range);
 
+/* CTA-861 Video Data Block (CTA VDB) */
+static void parse_cta_vdb(struct drm_connector *connector, const struct cea_db *db)
+{
+	struct drm_display_info *info = &connector->display_info;
+	int i, vic_index, len = cea_db_payload_len(db);
+	const u8 *svds = cea_db_data(db);
+	u8 *vics;
+
+	if (!len)
+		return;
+
+	/* Gracefully handle multiple VDBs, however unlikely that is */
+	vics = krealloc(info->vics, info->vics_len + len, GFP_KERNEL);
+	if (!vics)
+		return;
+
+	vic_index = info->vics_len;
+	info->vics_len += len;
+	info->vics = vics;
+
+	for (i = 0; i < len; i++) {
+		u8 vic = svd_to_vic(svds[i]);
+
+		if (!drm_valid_cea_vic(vic))
+			vic = 0;
+
+		info->vics[vic_index++] = vic;
+	}
+}
+
 static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
 {
 	struct drm_display_info *info = &connector->display_info;

@@ -6205,6 +6235,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_vcdb(connector, data);
 		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)
+			parse_cta_vdb(connector, db);
 	}
 	cea_db_iter_end(&iter);
 }

@@ -6372,6 +6404,10 @@ static void drm_reset_display_info(struct drm_connector *connector)
 	info->mso_stream_count = 0;
 	info->mso_pixel_overlap = 0;
 	info->max_dsc_bpp = 0;
+
+	kfree(info->vics);
+	info->vics = NULL;
+	info->vics_len = 0;
 }
 
 static u32 update_display_info(struct drm_connector *connector,

include/drm/drm_connector.h +10 -0

index 9037f1317aee..d97ed06d5837 100644
@@ -721,6 +721,16 @@ struct drm_display_info {
 	 * monitor's default value is used instead.
 	 */
 	u32 max_dsc_bpp;
+
+	/**
+	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
+	 */
+	u8 *vics;
+
+	/**
+	 * @vics_len: Number of elements in vics. Internal to EDID parsing.
+	 */
+	int vics_len;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,

Signature · 1 line

2.34.1

Now that we have all the VICs in info->vics, use them to simplify access based on VIC index, i.e. on the order of VICs in the EDID, and avoid passing CTA VDB pointers around.

This also fixes the highly unlikely scenarios of a) multiple HDMI VSDBs, and b) HDMI VSDB 3D modes using VIC indexes that span across multiple CTA VDBs, and the combination of the two.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

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

drivers/gpu/drm/drm_edid.c +32 -60

index 90846dc69061..7f0386175230 100644
@@ -4468,28 +4468,20 @@ static u8 svd_to_vic(u8 svd)
 	return svd;
 }
 
+/*
+ * Return a display mode for the 0-based vic_index'th VIC across all CTA VDBs in
+ * the EDID, or NULL on errors.
+ */
 static struct drm_display_mode *
-drm_display_mode_from_vic_index(struct drm_connector *connector,
-				const u8 *video_db, u8 video_len,
-				u8 video_index)
+drm_display_mode_from_vic_index(struct drm_connector *connector, int vic_index)
 {
+	const struct drm_display_info *info = &connector->display_info;
 	struct drm_device *dev = connector->dev;
-	struct drm_display_mode *newmode;
-	u8 vic;
 
-	if (video_db == NULL || video_index >= video_len)
+	if (!info->vics || vic_index >= info->vics_len || !info->vics[vic_index])
 		return NULL;
 
-	/* CEA modes are numbered 1..127 */
-	vic = svd_to_vic(video_db[video_index]);
-	if (!drm_valid_cea_vic(vic))
-		return NULL;
-
-	newmode = drm_mode_duplicate(dev, cea_mode_for_vic(vic));
-	if (!newmode)
-		return NULL;
-
-	return newmode;
+	return drm_display_mode_from_cea_vic(dev, info->vics[vic_index]);
 }
 
 /*

@@ -4538,9 +4530,8 @@ static int do_y420vdb_modes(struct drm_connector *connector,
  * Makes an entry for a videomode in the YCBCR 420 bitmap
  */
 static void
-drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
+drm_add_cmdb_modes(struct drm_connector *connector, u8 vic)
 {
-	u8 vic = svd_to_vic(svd);
 	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
 
 	if (!drm_valid_cea_vic(vic))

@@ -4577,16 +4568,20 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_display_mode_from_cea_vic);
 
-static int
-do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
+/* Add modes based on VICs parsed in parse_cta_vdb() */
+static int add_cta_vdb_modes(struct drm_connector *connector)
 {
+	const struct drm_display_info *info = &connector->display_info;
+	const struct drm_hdmi_info *hdmi = &info->hdmi;
 	int i, modes = 0;
-	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
 
-	for (i = 0; i < len; i++) {
+	if (!info->vics)
+		return 0;
+
+	for (i = 0; i < info->vics_len; i++) {
 		struct drm_display_mode *mode;
 
-		mode = drm_display_mode_from_vic_index(connector, db, len, i);
+		mode = drm_display_mode_from_vic_index(connector, i);
 		if (mode) {
 			/*
 			 * YCBCR420 capability block contains a bitmap which

@@ -4598,7 +4593,7 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
 			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
 			 */
 			if (i < 64 && hdmi->y420_cmdb_map & (1ULL << i))
-				drm_add_cmdb_modes(connector, db[i]);
+				drm_add_cmdb_modes(connector, info->vics[i]);
 
 			drm_mode_probed_add(connector, mode);
 			modes++;

@@ -4693,15 +4688,13 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
 }
 
 static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
-			       const u8 *video_db, u8 video_len, u8 video_index)
+			       int vic_index)
 {
 	struct drm_display_mode *newmode;
 	int modes = 0;
 
 	if (structure & (1 << 0)) {
-		newmode = drm_display_mode_from_vic_index(connector, video_db,
-							  video_len,
-							  video_index);
+		newmode = drm_display_mode_from_vic_index(connector, vic_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
 			drm_mode_probed_add(connector, newmode);

@@ -4709,9 +4702,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 		}
 	}
 	if (structure & (1 << 6)) {
-		newmode = drm_display_mode_from_vic_index(connector, video_db,
-							  video_len,
-							  video_index);
+		newmode = drm_display_mode_from_vic_index(connector, vic_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
 			drm_mode_probed_add(connector, newmode);

@@ -4719,9 +4710,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 		}
 	}
 	if (structure & (1 << 8)) {
-		newmode = drm_display_mode_from_vic_index(connector, video_db,
-							  video_len,
-							  video_index);
+		newmode = drm_display_mode_from_vic_index(connector, vic_index);
 		if (newmode) {
 			newmode->flags |= DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
 			drm_mode_probed_add(connector, newmode);

@@ -4742,8 +4731,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
  * also adds the stereo 3d modes when applicable.
  */
 static int
-do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
-		   const u8 *video_db, u8 video_len)
+do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 {
 	struct drm_display_info *info = &connector->display_info;
 	int modes = 0, offset = 0, i, multi_present = 0, multi_len;

@@ -4818,9 +4806,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 		for (i = 0; i < 16; i++) {
 			if (mask & (1 << i))
 				modes += add_3d_struct_modes(connector,
-						structure_all,
-						video_db,
-						video_len, i);
+							     structure_all, i);
 		}
 	}
 

@@ -4857,8 +4843,6 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 
 		if (newflag != 0) {
 			newmode = drm_display_mode_from_vic_index(connector,
-								  video_db,
-								  video_len,
 								  vic_index);
 
 			if (newmode) {

@@ -5249,20 +5233,16 @@ static int add_cea_modes(struct drm_connector *connector,
 {
 	const struct cea_db *db;
 	struct cea_db_iter iter;
-	const u8 *hdmi = NULL, *video = NULL;
-	u8 hdmi_len = 0, video_len = 0;
-	int modes = 0;
+	int modes;
+
+	/* CTA VDB block VICs parsed earlier */
+	modes = add_cta_vdb_modes(connector);
 
 	cea_db_iter_edid_begin(drm_edid, &iter);
 	cea_db_iter_for_each(db, &iter) {
-		if (cea_db_tag(db) == CTA_DB_VIDEO) {
-			video = cea_db_data(db);
-			video_len = cea_db_payload_len(db);
-			modes += do_cea_modes(connector, video, video_len);
-		} else if (cea_db_is_hdmi_vsdb(db)) {
-			/* FIXME: Switch to use cea_db_data() */
-			hdmi = (const u8 *)db;
-			hdmi_len = cea_db_payload_len(db);
+		if (cea_db_is_hdmi_vsdb(db)) {
+			modes += do_hdmi_vsdb_modes(connector, (const u8 *)db,
+						    cea_db_payload_len(db));
 		} else if (cea_db_is_y420vdb(db)) {
 			const u8 *vdb420 = cea_db_data(db) + 1;
 

@@ -5273,14 +5253,6 @@ static int add_cea_modes(struct drm_connector *connector,
 	}
 	cea_db_iter_end(&iter);
 
-	/*
-	 * We parse the HDMI VSDB after having added the cea modes as we will be
-	 * patching their flags when the sink supports stereo 3D.
-	 */
-	if (hdmi)
-		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len,
-					    video, video_len);
-
 	return modes;
 }
 

Signature · 1 line

2.34.1

Apparently there are HDMI 1.4 compatible displays out there that support VICs from specs later than CTA-861-D, i.e. VIC > 64, although HDMI 1.4 references CTA-861-D only.

We try to avoid using VICs from the later specs in the AVI infoframes to avoid upsetting sinks that conform to earlier specs.

However, it seems reasonable to do this when the sink claims it supports the VIC. With the pre-parsed list of VICs handy, this is now trivial.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/6153

Cc Ville Syrjälä <[email protected]>

Cc William Tseng <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

drivers/gpu/drm/drm_edid.c +21 -1

index 7f0386175230..3dfcd6450f10 100644
@@ -5864,6 +5864,22 @@ static void parse_cta_vdb(struct drm_connector *connector, const struct cea_db *
 	}
 }
 
+static bool cta_vdb_has_vic(const struct drm_connector *connector, u8 vic)
+{
+	const struct drm_display_info *info = &connector->display_info;
+	int i;
+
+	if (!vic || !info->vics)
+		return false;
+
+	for (i = 0; i < info->vics_len; i++) {
+		if (info->vics[i] == vic)
+			return true;
+	}
+
+	return false;
+}
+
 static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
 {
 	struct drm_display_info *info = &connector->display_info;

@@ -6909,10 +6925,14 @@ static u8 drm_mode_cea_vic(const struct drm_connector *connector,
  *
  * HDMI 1.4 (CTA-861-D) VIC range: [1..64]
  * HDMI 2.0 (CTA-861-F) VIC range: [1..107]
+ *
+ * If the sink lists the VIC in CTA VDB, assume it's fine, regardless of HDMI
+ * version.
  */
 static u8 vic_for_avi_infoframe(const struct drm_connector *connector, u8 vic)
 {
-	if (!is_hdmi2_sink(connector) && vic > 64)
+	if (!is_hdmi2_sink(connector) && vic > 64 &&
+	    !cta_vdb_has_vic(connector, vic))
 		return 0;
 
 	return vic;

Signature · 1 line

2.34.1

Rename the local variable to info for consistency.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/gpu/drm/drm_edid.c +4 -4

index 3dfcd6450f10..4e9108e9fc96 100644
@@ -6011,14 +6011,14 @@ static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
 static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 				      const u8 *hf_scds)
 {
-	struct drm_display_info *display = &connector->display_info;
-	struct drm_hdmi_info *hdmi = &display->hdmi;
+	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;
 	int max_tmds_clock = 0;
 	u8 max_frl_rate = 0;
 	bool dsc_support = false;
 
-	display->has_hdmi_infoframe = true;
+	info->has_hdmi_infoframe = true;
 
 	if (hf_scds[6] & 0x80) {
 		hdmi->scdc.supported = true;

@@ -6042,7 +6042,7 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 		max_tmds_clock = hf_scds[5] * 5000;
 
 		if (max_tmds_clock > 340000) {
-			display->max_tmds_clock = max_tmds_clock;
+			info->max_tmds_clock = max_tmds_clock;
 		}
 
 		if (scdc->supported) {

Signature · 1 line

2.34.1

Now that we have pre-parsed CTA VDB VICs stored in info->vics, leverage that to simplify CTA Y420CMDB parsing. Move updating the y420_cmdb_modes bitmap to the display info parsing stage, instead of updating it during add modes. This allows us to drop the intermediate y420_cmdb_map from display info, and replace it with a local variable.

This is prerequisite work for overall better separation of the two parsing steps (updating display info and adding modes).

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c  | 86 ++++++++++++++++++-------------------
 include/drm/drm_connector.h |  3 --
 2 files changed, 43 insertions(+), 46 deletions(-)

drivers/gpu/drm/drm_edid.c +43 -43

index 4e9108e9fc96..ead7a4ce0422 100644
@@ -4522,24 +4522,6 @@ static int do_y420vdb_modes(struct drm_connector *connector,
 	return modes;
 }
 
-/*
- * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
- * @connector: connector corresponding to the HDMI sink
- * @vic: CEA vic for the video mode to be added in the map
- *
- * Makes an entry for a videomode in the YCBCR 420 bitmap
- */
-static void
-drm_add_cmdb_modes(struct drm_connector *connector, u8 vic)
-{
-	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
-
-	if (!drm_valid_cea_vic(vic))
-		return;
-
-	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
-}
-
 /**
  * drm_display_mode_from_cea_vic() - return a mode for CEA VIC
  * @dev: DRM device

@@ -4572,7 +4554,6 @@ EXPORT_SYMBOL(drm_display_mode_from_cea_vic);
 static int add_cta_vdb_modes(struct drm_connector *connector)
 {
 	const struct drm_display_info *info = &connector->display_info;
-	const struct drm_hdmi_info *hdmi = &info->hdmi;
 	int i, modes = 0;
 
 	if (!info->vics)

@@ -4583,18 +4564,6 @@ static int add_cta_vdb_modes(struct drm_connector *connector)
 
 		mode = drm_display_mode_from_vic_index(connector, i);
 		if (mode) {
-			/*
-			 * YCBCR420 capability block contains a bitmap which
-			 * gives the index of CEA modes from CEA VDB, which
-			 * can support YCBCR 420 sampling output also (apart
-			 * from RGB/YCBCR444 etc).
-			 * For example, if the bit 0 in bitmap is set,
-			 * first mode in VDB can support YCBCR420 output too.
-			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
-			 */
-			if (i < 64 && hdmi->y420_cmdb_map & (1ULL << i))
-				drm_add_cmdb_modes(connector, info->vics[i]);
-
 			drm_mode_probed_add(connector, mode);
 			modes++;
 		}

@@ -5188,20 +5157,26 @@ static int edid_hfeeodb_extension_block_count(const struct edid *edid)
 	return cta[4 + 2];
 }
 
-static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
-				      const u8 *db)
+/*
+ * CTA-861 YCbCr 4:2:0 Capability Map Data Block (CTA Y420CMDB)
+ *
+ * Y420CMDB contains a bitmap which gives the index of CTA modes from CTA VDB,
+ * which can support YCBCR 420 sampling output also (apart from RGB/YCBCR444
+ * etc). For example, if the bit 0 in bitmap is set, first mode in VDB can
+ * support YCBCR420 output too.
+ */
+static void parse_cta_y420cmdb(struct drm_connector *connector,
+			       const struct cea_db *db, u64 *y420cmdb_map)
 {
 	struct drm_display_info *info = &connector->display_info;
-	struct drm_hdmi_info *hdmi = &info->hdmi;
-	u8 map_len = cea_db_payload_len(db) - 1;
-	u8 count;
+	int i, map_len = cea_db_payload_len(db) - 1;
+	const u8 *data = cea_db_data(db) + 1;
 	u64 map = 0;
 
 	if (map_len == 0) {
 		/* All CEA modes support ycbcr420 sampling also.*/
-		hdmi->y420_cmdb_map = U64_MAX;
-		info->color_formats |= DRM_COLOR_FORMAT_YCBCR420;
-		return;
+		map = U64_MAX;
+		goto out;
 	}
 
 	/*

@@ -5219,13 +5194,14 @@ static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
 	if (WARN_ON_ONCE(map_len > 8))
 		map_len = 8;
 
-	for (count = 0; count < map_len; count++)
-		map |= (u64)db[2 + count] << (8 * count);
+	for (i = 0; i < map_len; i++)
+		map |= (u64)data[i] << (8 * i);
 
+out:
 	if (map)
 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR420;
 
-	hdmi->y420_cmdb_map = map;
+	*y420cmdb_map = map;
 }
 
 static int add_cea_modes(struct drm_connector *connector,

@@ -5864,6 +5840,26 @@ static void parse_cta_vdb(struct drm_connector *connector, const struct cea_db *
 	}
 }
 
+/*
+ * Update y420_cmdb_modes based on previously parsed CTA VDB and Y420CMDB.
+ *
+ * Translate the y420cmdb_map based on VIC indexes to y420_cmdb_modes indexed
+ * using the VICs themselves.
+ */
+static void update_cta_y420cmdb(struct drm_connector *connector, u64 y420cmdb_map)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+	int i, len = min_t(int, info->vics_len, BITS_PER_TYPE(y420cmdb_map));
+
+	for (i = 0; i < len; i++) {
+		u8 vic = info->vics[i];
+
+		if (vic && y420cmdb_map & BIT_ULL(i))
+			bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
+	}
+}
+
 static bool cta_vdb_has_vic(const struct drm_connector *connector, u8 vic)
 {
 	const struct drm_display_info *info = &connector->display_info;

@@ -6181,6 +6177,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 	const struct cea_db *db;
 	struct cea_db_iter iter;
 	const u8 *edid_ext;
+	u64 y420cmdb_map = 0;
 
 	drm_edid_iter_begin(drm_edid, &edid_iter);
 	drm_edid_iter_for_each(edid_ext, &edid_iter) {

@@ -6218,7 +6215,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 		else if (cea_db_is_microsoft_vsdb(db))
 			drm_parse_microsoft_vsdb(connector, data);
 		else if (cea_db_is_y420cmdb(db))
-			drm_parse_y420cmdb_bitmap(connector, data);
+			parse_cta_y420cmdb(connector, db, &y420cmdb_map);
 		else if (cea_db_is_vcdb(db))
 			drm_parse_vcdb(connector, data);
 		else if (cea_db_is_hdmi_hdr_metadata_block(db))

@@ -6227,6 +6224,9 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			parse_cta_vdb(connector, db);
 	}
 	cea_db_iter_end(&iter);
+
+	if (y420cmdb_map)
+		update_cta_y420cmdb(connector, y420cmdb_map);
 }
 
 static

include/drm/drm_connector.h +0 -3

index d97ed06d5837..1c26c4e72c62 100644
@@ -304,9 +304,6 @@ struct drm_hdmi_info {
 	 */
 	unsigned long y420_cmdb_modes[BITS_TO_LONGS(256)];
 
-	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
-	u64 y420_cmdb_map;
-
 	/** @y420_dc_modes: bitmap of deep color support index */
 	u8 y420_dc_modes;
 

Signature · 1 line

2.34.1

Separate the parsing of display info and modes from the CTA Y420VDB. This is prerequisite work for overall better separation of the two parsing steps.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

drivers/gpu/drm/drm_edid.c +1 -6

index ead7a4ce0422..076ba125c38d 100644
@@ -4497,10 +4497,8 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, int vic_index)
 static int do_y420vdb_modes(struct drm_connector *connector,
 			    const u8 *svds, u8 svds_len)
 {
-	int modes = 0, i;
 	struct drm_device *dev = connector->dev;
-	struct drm_display_info *info = &connector->display_info;
-	struct drm_hdmi_info *hdmi = &info->hdmi;
+	int modes = 0, i;
 
 	for (i = 0; i < svds_len; i++) {
 		u8 vic = svd_to_vic(svds[i]);

@@ -4512,13 +4510,10 @@ static int do_y420vdb_modes(struct drm_connector *connector,
 		newmode = drm_mode_duplicate(dev, cea_mode_for_vic(vic));
 		if (!newmode)
 			break;
-		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
 		drm_mode_probed_add(connector, newmode);
 		modes++;
 	}
 
-	if (modes > 0)
-		info->color_formats |= DRM_COLOR_FORMAT_YCBCR420;
 	return modes;
 }
 

drivers/gpu/drm/drm_edid.c +6 -0

+		info->color_formats |= DRM_COLOR_FORMAT_YCBCR420;
+	}
+}
+
 static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
 {
 	struct drm_display_info *info = &connector->display_info;

@@ -6216,6 +6231,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_microsoft_vsdb(connector, data);
 		else if (cea_db_is_y420cmdb(db))
 			parse_cta_y420cmdb(connector, db, &y420cmdb_map);
+		else if (cea_db_is_y420vdb(db))
+			parse_cta_y420vdb(connector, db);
 		else if (cea_db_is_vcdb(db))
 			drm_parse_vcdb(connector, data);
 		else if (cea_db_is_hdmi_hdr_metadata_block(db))

Signature · 1 line

2.34.1

Add helpers for Latency_Fields_Present and I_Latency_Fields_Present bits, and fix the parsing:

- Respect specification regarding "I_Latency_Fields_Present shall be zero if Latency_Fields_Present is zero".

- Don't claim latency fields are present if the data block isn't big enough to hold them.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

drivers/gpu/drm/drm_edid.c +19 -8

index 076ba125c38d..847076b29594 100644
@@ -4685,6 +4685,16 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 	return modes;
 }
 
+static bool hdmi_vsdb_latency_present(const u8 *db)
+{
+	return db[8] & BIT(7);
+}
+
+static bool hdmi_vsdb_i_latency_present(const u8 *db)
+{
+	return hdmi_vsdb_latency_present(db) && db[8] & BIT(6);
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink

@@ -5357,6 +5367,7 @@ drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
 	}
 }
 
+/* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
 static void
 drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 {

@@ -5364,18 +5375,18 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 
 	if (len >= 6 && (db[6] & (1 << 7)))
 		connector->eld[DRM_ELD_SAD_COUNT_CONN_TYPE] |= DRM_ELD_SUPPORTS_AI;
-	if (len >= 8) {
-		connector->latency_present[0] = db[8] >> 7;
-		connector->latency_present[1] = (db[8] >> 6) & 1;
-	}
-	if (len >= 9)
+
+	if (len >= 10 && hdmi_vsdb_latency_present(db)) {
+		connector->latency_present[0] = true;
 		connector->video_latency[0] = db[9];
-	if (len >= 10)
 		connector->audio_latency[0] = db[10];
-	if (len >= 11)
+	}
+
+	if (len >= 12 && hdmi_vsdb_i_latency_present(db)) {
+		connector->latency_present[1] = true;
 		connector->video_latency[1] = db[11];
-	if (len >= 12)
 		connector->audio_latency[1] = db[12];
+	}
 
 	drm_dbg_kms(connector->dev,
 		    "[CONNECTOR:%d:%s] HDMI: latency present %d %d, video latency %d %d, audio latency %d %d\n",

Signature · 1 line

2.34.1

Add a helper for skipping the HDMI VSDB audio latency fields.

There's a functional change for HDMI VSDB blocks that do not respect the
spec: "I_Latency_Fields_Present shall be zero if Latency_Fields_Present is zero". We assume this to hold when skipping the latency fields, and ignore non-zero I_Latency_Fields_Present if Latency_Fields_Present is zero.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/gpu/drm/drm_edid.c +11 -7

index 847076b29594..93067b8dd9f6 100644
@@ -4695,6 +4695,16 @@ static bool hdmi_vsdb_i_latency_present(const u8 *db)
 	return hdmi_vsdb_latency_present(db) && db[8] & BIT(6);
 }
 
+static int hdmi_vsdb_latency_length(const u8 *db)
+{
+	if (hdmi_vsdb_i_latency_present(db))
+		return 4;
+	else if (hdmi_vsdb_latency_present(db))
+		return 2;
+	else
+		return 0;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink

@@ -4720,13 +4730,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	if (!(db[8] & (1 << 5)))
 		goto out;
 
-	/* Latency_Fields_Present */
-	if (db[8] & (1 << 7))
-		offset += 2;
-
-	/* I_Latency_Fields_Present */
-	if (db[8] & (1 << 6))
-		offset += 2;
+	offset += hdmi_vsdb_latency_length(db);
 
 	/* the declared length is not long enough for the 2 first bytes
 	 * of additional video format capabilities */

Signature · 1 line

2.34.1

Quoted 8 lines · Jani Nikula

Add a helper for skipping the HDMI VSDB audio latency fields.

There's a functional change for HDMI VSDB blocks that do not respect the
spec: "I_Latency_Fields_Present shall be zero if Latency_Fields_Present is zero". We assume this to hold when skipping the latency fields, and ignore non-zero I_Latency_Fields_Present if Latency_Fields_Present is zero.

Quoted 2 lines

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 43 lines

---
 drivers/gpu/drm/drm_edid.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/gpu/drm/drm_edid.c +11 -7

index 847076b29594..93067b8dd9f6 100644
@@ -4695,6 +4695,16 @@ static bool hdmi_vsdb_i_latency_present(const u8 *db)
 	return hdmi_vsdb_latency_present(db) && db[8] & BIT(6);
 }
 
+static int hdmi_vsdb_latency_length(const u8 *db)
+{
+	if (hdmi_vsdb_i_latency_present(db))
+		return 4;
+	else if (hdmi_vsdb_latency_present(db))
+		return 2;
+	else
+		return 0;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink

@@ -4720,13 +4730,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	if (!(db[8] & (1 << 5)))
 		goto out;
 
-	/* Latency_Fields_Present */
-	if (db[8] & (1 << 7))
-		offset += 2;
-
-	/* I_Latency_Fields_Present */
-	if (db[8] & (1 << 6))
-		offset += 2;
+	offset += hdmi_vsdb_latency_length(db);
 
 	/* the declared length is not long enough for the 2 first bytes
 	 * of additional video format capabilities */

Signature · 1 line

2.34.1

Separate the parsing of display info and modes from the HDMI VSDB. This is prerequisite work for overall better separation of the two parsing steps.

The info parsing is about figuring out whether the sink supports HDMI infoframes. Since they were added in HDMI 1.4, assume the sink supports HDMI infoframes if it uses HDMI 1.4 features in HDMI VSDB. For details, see commit f1781e9bb2dd ("drm/edid: Allow HDMI infoframe without VIC or S3D").

The logic is not exactly the same, but since it was somewhat heuristic to begin with, assume this is close enough.

Add cea_db_raw_size() helper to get the size of the entire data block, including tag and length. This is helpful for checking against specs that define indexes from the start of the entire block, like here.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 39 +++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_edid.c +18 -3

index 93067b8dd9f6..5cb1d36ce48a 100644
@@ -4717,7 +4717,6 @@ static int hdmi_vsdb_latency_length(const u8 *db)
 static int
 do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 {
-	struct drm_display_info *info = &connector->display_info;
 	int modes = 0, offset = 0, i, multi_present = 0, multi_len;
 	u8 vic_len, hdmi_3d_len = 0;
 	u16 mask;

@@ -4835,8 +4834,6 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	}
 
 out:
-	if (modes > 0)
-		info->has_hdmi_infoframe = true;
 	return modes;
 }
 

@@ -4893,6 +4890,7 @@ static int cea_db_tag(const struct cea_db *db)
 	return db->tag_length >> 5;
 }
 
+/* Data block payload length excluding the tag and length byte */
 static int cea_db_payload_len(const void *_db)
 {
 	/* FIXME: Transition to passing struct cea_db * everywhere. */

@@ -4901,6 +4899,12 @@ static int cea_db_payload_len(const void *_db)
 	return db->tag_length & 0x1f;
 }
 
+/* Data block raw size including the tag and length byte */
+static int cea_db_raw_size(const void *db)
+{
+	return cea_db_payload_len(db) + 1;
+}
+
 static const void *cea_db_data(const struct cea_db *db)
 {
 	return db->data;

@@ -6159,6 +6163,32 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	}
 }
 
+/*
+ * Try to infer whether the sink supports HDMI infoframes.
+ *
+ * HDMI infoframe support was first added in HDMI 1.4. Assume the sink supports
+ * infoframes if the HDMI VSDB contains HDMI 1.4 features.
+ */
+static bool hdmi_vsdb_infoframe_support(struct drm_connector *connector, const u8 *db)
+{
+	int size = cea_db_raw_size(db);
+	int offset = 8;
+

drivers/gpu/drm/drm_edid.c +8 -0

+
+	return false;
+}
+
+/* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
 static void
 drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 {

@@ -6172,6 +6202,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	if (len >= 7)
 		info->max_tmds_clock = db[7] * 5000;
 
+	if (hdmi_vsdb_infoframe_support(connector, db))
+		info->has_hdmi_infoframe = true;
+
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n",
 		    connector->base.id, connector->name,
 		    info->dvi_dual, info->max_tmds_clock);

Signature · 1 line

2.34.1

Separate the parsing of display info and modes from the HDMI VSDB. This is prerequisite work for overall better separation of the two parsing steps.

The info parsing is about figuring out whether the sink supports HDMI infoframes. Since they were added in HDMI 1.4, assume the sink supports HDMI infoframes if it has the HDMI_Video_present bit set (introduced in HDMI 1.4). For details, see commit f1781e9bb2dd ("drm/edid: Allow HDMI infoframe without VIC or S3D").

The logic is not exactly the same, but since it was somewhat heuristic to begin with, assume this is close enough.

v2:
- Simplify to only check HDMI_Video_present bit (Ville)
- Drop cea_db_raw_size() helper (Ville)

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_edid.c +10 -3

index e8b67f3f5c91..ee453e39562a 100644
@@ -4713,7 +4713,6 @@ static int hdmi_vsdb_latency_length(const u8 *db)
 static int
 do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 {
-	struct drm_display_info *info = &connector->display_info;
 	int modes = 0, offset = 0, i, multi_present = 0, multi_len;
 	u8 vic_len, hdmi_3d_len = 0;
 	u16 mask;

@@ -4831,8 +4830,6 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
 	}
 
 out:
-	if (modes > 0)
-		info->has_hdmi_infoframe = true;
 	return modes;
 }
 

@@ -6153,6 +6150,7 @@ 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)
 {

@@ -6166,6 +6164,15 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	if (len >= 7)
 		info->max_tmds_clock = db[7] * 5000;
 
+	/*
+	 * Try to infer whether the sink supports HDMI infoframes.
+	 *
+	 * 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))
+		info->has_hdmi_infoframe = true;
+
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n",
 		    connector->base.id, connector->name,
 		    info->dvi_dual, info->max_tmds_clock);

Signature · 1 line

2.34.1

Although the quirks are internal to EDID parsing, it'll be helpful to store them in display info to avoid having to pass them around.

This will also help separate adding probed modes (which needs the quirks) from updating display info.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c  | 42 ++++++++++++++++++-------------------
 include/drm/drm_connector.h |  5 +++++
 2 files changed, 26 insertions(+), 21 deletions(-)

drivers/gpu/drm/drm_edid.c +21 -21

index 5cb1d36ce48a..fd8d056e38c1 100644
@@ -6461,18 +6461,20 @@ static void drm_reset_display_info(struct drm_connector *connector)
 	kfree(info->vics);
 	info->vics = NULL;
 	info->vics_len = 0;
+
+	info->quirks = 0;
 }
 
-static u32 update_display_info(struct drm_connector *connector,
-			       const struct drm_edid *drm_edid)
+static void update_display_info(struct drm_connector *connector,
+				const struct drm_edid *drm_edid)
 {
 	struct drm_display_info *info = &connector->display_info;
 	const struct edid *edid = drm_edid->edid;
 
-	u32 quirks = edid_get_quirks(drm_edid);
-
 	drm_reset_display_info(connector);
 
+	info->quirks = edid_get_quirks(drm_edid);
+
 	info->width_mm = edid->width_cm * 10;
 	info->height_mm = edid->height_cm * 10;
 

@@ -6543,17 +6545,15 @@ static u32 update_display_info(struct drm_connector *connector,
 	drm_update_mso(connector, drm_edid);
 
 out:
-	if (quirks & EDID_QUIRK_NON_DESKTOP) {
+	if (info->quirks & EDID_QUIRK_NON_DESKTOP) {
 		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Non-desktop display%s\n",
 			    connector->base.id, connector->name,
 			    info->non_desktop ? " (redundant quirk)" : "");
 		info->non_desktop = true;
 	}
 
-	if (quirks & EDID_QUIRK_CAP_DSC_15BPP)
+	if (info->quirks & EDID_QUIRK_CAP_DSC_15BPP)
 		info->max_dsc_bpp = 15;
-
-	return quirks;
 }
 
 static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,

@@ -6651,8 +6651,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 static int _drm_edid_connector_update(struct drm_connector *connector,
 				      const struct drm_edid *drm_edid)
 {
+	struct drm_display_info *info = &connector->display_info;
 	int num_modes = 0;
-	u32 quirks;
 
 	if (!drm_edid) {
 		drm_reset_display_info(connector);

@@ -6665,7 +6665,7 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	 * To avoid multiple parsing of same block, lets parse that map
 	 * from sink info, before parsing CEA modes.
 	 */
-	quirks = update_display_info(connector, drm_edid);
+	update_display_info(connector, drm_edid);
 
 	/* Depends on info->cea_rev set by update_display_info() above */
 	drm_edid_to_eld(connector, drm_edid);

@@ -6684,7 +6684,7 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	 *
 	 * XXX order for additional mode types in extension blocks?
 	 */
-	num_modes += add_detailed_modes(connector, drm_edid, quirks);
+	num_modes += add_detailed_modes(connector, drm_edid, info->quirks);
 	num_modes += add_cvt_modes(connector, drm_edid);
 	num_modes += add_standard_modes(connector, drm_edid);
 	num_modes += add_established_modes(connector, drm_edid);

@@ -6694,20 +6694,20 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	if (drm_edid->edid->features & DRM_EDID_FEATURE_CONTINUOUS_FREQ)
 		num_modes += add_inferred_modes(connector, drm_edid);
 
-	if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
-		edid_fixup_preferred(connector, quirks);
+	if (info->quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
+		edid_fixup_preferred(connector, info->quirks);
 
-	if (quirks & EDID_QUIRK_FORCE_6BPC)
-		connector->display_info.bpc = 6;
+	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
+		info->bpc = 6;
 
-	if (quirks & EDID_QUIRK_FORCE_8BPC)
-		connector->display_info.bpc = 8;
+	if (info->quirks & EDID_QUIRK_FORCE_8BPC)
+		info->bpc = 8;
 
-	if (quirks & EDID_QUIRK_FORCE_10BPC)
-		connector->display_info.bpc = 10;
+	if (info->quirks & EDID_QUIRK_FORCE_10BPC)
+		info->bpc = 10;
 
-	if (quirks & EDID_QUIRK_FORCE_12BPC)
-		connector->display_info.bpc = 12;
+	if (info->quirks & EDID_QUIRK_FORCE_12BPC)
+		info->bpc = 12;
 
 	return num_modes;
 }

include/drm/drm_connector.h +5 -0

index 1c26c4e72c62..7b5048516185 100644
@@ -728,6 +728,11 @@ struct drm_display_info {
 	 * @vics_len: Number of elements in vics. Internal to EDID parsing.
 	 */
 	int vics_len;
+
+	/**
+	 * @quirks: EDID based quirks. Internal to EDID parsing.
+	 */
+	u32 quirks;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,

Signature · 1 line

2.34.1

Now that quirks are stored in display info, we can just look them up using the connector instead of having to pass them around.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

drivers/gpu/drm/drm_edid.c +15 -19

index fd8d056e38c1..6bc0432046c8 100644
@@ -96,7 +96,6 @@ struct detailed_mode_closure {
 	struct drm_connector *connector;
 	const struct drm_edid *drm_edid;
 	bool preferred;
-	u32 quirks;
 	int modes;
 };
 

@@ -2887,9 +2886,9 @@ static u32 edid_get_quirks(const struct drm_edid *drm_edid)
  * Walk the mode list for connector, clearing the preferred status on existing
  * modes and setting it anew for the right mode ala quirks.
  */
-static void edid_fixup_preferred(struct drm_connector *connector,
-				 u32 quirks)
+static void edid_fixup_preferred(struct drm_connector *connector)
 {
+	const struct drm_display_info *info = &connector->display_info;
 	struct drm_display_mode *t, *cur_mode, *preferred_mode;
 	int target_refresh = 0;
 	int cur_vrefresh, preferred_vrefresh;

@@ -2897,9 +2896,9 @@ static void edid_fixup_preferred(struct drm_connector *connector,
 	if (list_empty(&connector->probed_modes))
 		return;
 
-	if (quirks & EDID_QUIRK_PREFER_LARGE_60)
+	if (info->quirks & EDID_QUIRK_PREFER_LARGE_60)
 		target_refresh = 60;
-	if (quirks & EDID_QUIRK_PREFER_LARGE_75)
+	if (info->quirks & EDID_QUIRK_PREFER_LARGE_75)
 		target_refresh = 75;
 
 	preferred_mode = list_first_entry(&connector->probed_modes,

@@ -3401,9 +3400,9 @@ drm_mode_do_interlace_quirk(struct drm_display_mode *mode,
  */
 static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connector,
 						  const struct drm_edid *drm_edid,
-						  const struct detailed_timing *timing,
-						  u32 quirks)
+						  const struct detailed_timing *timing)
 {
+	const struct drm_display_info *info = &connector->display_info;
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode;
 	const struct detailed_pixel_timing *pt = &timing->data.pixel_data;

@@ -3437,7 +3436,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto
 		return NULL;
 	}
 
-	if (quirks & EDID_QUIRK_FORCE_REDUCED_BLANKING) {
+	if (info->quirks & EDID_QUIRK_FORCE_REDUCED_BLANKING) {
 		mode = drm_cvt_mode(dev, hactive, vactive, 60, true, false, false);
 		if (!mode)
 			return NULL;

@@ -3449,7 +3448,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto
 	if (!mode)
 		return NULL;
 
-	if (quirks & EDID_QUIRK_135_CLOCK_TOO_HIGH)
+	if (info->quirks & EDID_QUIRK_135_CLOCK_TOO_HIGH)
 		mode->clock = 1088 * 10;
 	else
 		mode->clock = le16_to_cpu(timing->pixel_clock) * 10;

@@ -3472,7 +3471,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto
 
 	drm_mode_do_interlace_quirk(mode, pt);
 
-	if (quirks & EDID_QUIRK_DETAILED_SYNC_PP) {
+	if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) {
 		mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC;
 	} else {
 		mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ?

@@ -3485,12 +3484,12 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto
 	mode->width_mm = pt->width_mm_lo | (pt->width_height_mm_hi & 0xf0) << 4;
 	mode->height_mm = pt->height_mm_lo | (pt->width_height_mm_hi & 0xf) << 8;
 
-	if (quirks & EDID_QUIRK_DETAILED_IN_CM) {
+	if (info->quirks & EDID_QUIRK_DETAILED_IN_CM) {
 		mode->width_mm *= 10;
 		mode->height_mm *= 10;
 	}
 
-	if (quirks & EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE) {
+	if (info->quirks & EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE) {
 		mode->width_mm = drm_edid->edid->width_cm * 10;
 		mode->height_mm = drm_edid->edid->height_cm * 10;
 	}

@@ -4003,8 +4002,7 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
 		return;
 
 	newmode = drm_mode_detailed(closure->connector,
-				    closure->drm_edid, timing,
-				    closure->quirks);
+				    closure->drm_edid, timing);
 	if (!newmode)
 		return;
 

@@ -4027,15 +4025,13 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
  * add_detailed_modes - Add modes from detailed timings
  * @connector: attached connector
  * @drm_edid: EDID block to scan
- * @quirks: quirks to apply
  */
 static int add_detailed_modes(struct drm_connector *connector,
-			      const struct drm_edid *drm_edid, u32 quirks)
+			      const struct drm_edid *drm_edid)
 {
 	struct detailed_mode_closure closure = {
 		.connector = connector,
 		.drm_edid = drm_edid,
-		.quirks = quirks,
 	};
 
 	if (drm_edid->edid->revision >= 4)

@@ -6684,7 +6680,7 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	 *
 	 * XXX order for additional mode types in extension blocks?
 	 */
-	num_modes += add_detailed_modes(connector, drm_edid, info->quirks);
+	num_modes += add_detailed_modes(connector, drm_edid);
 	num_modes += add_cvt_modes(connector, drm_edid);
 	num_modes += add_standard_modes(connector, drm_edid);
 	num_modes += add_established_modes(connector, drm_edid);

@@ -6695,7 +6691,7 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 		num_modes += add_inferred_modes(connector, drm_edid);
 
 	if (info->quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
-		edid_fixup_preferred(connector, info->quirks);
+		edid_fixup_preferred(connector);
 
 	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
 		info->bpc = 6;

Signature · 1 line

2.34.1

Simplify display info update by merging ELD handling as well as clearing of the data in update_display_info().

The connector->eld really should be moved under display_info altogether, but that's for another time.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

drivers/gpu/drm/drm_edid.c +13 -15

index 6bc0432046c8..810a5fca398a 100644
@@ -5489,8 +5489,6 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 	int total_sad_count = 0;
 	int mnl;
 
-	clear_eld(connector);
-
 	if (!drm_edid)
 		return;
 

@@ -6465,9 +6463,15 @@ static void update_display_info(struct drm_connector *connector,
 				const struct drm_edid *drm_edid)
 {
 	struct drm_display_info *info = &connector->display_info;
-	const struct edid *edid = drm_edid->edid;
+	const struct edid *edid;
 
 	drm_reset_display_info(connector);
+	clear_eld(connector);
+
+	if (!drm_edid)
+		return;
+
+	edid = drm_edid->edid;
 
 	info->quirks = edid_get_quirks(drm_edid);
 

@@ -6550,6 +6554,9 @@ static void update_display_info(struct drm_connector *connector,
 
 	if (info->quirks & EDID_QUIRK_CAP_DSC_15BPP)
 		info->max_dsc_bpp = 15;
+
+	/* Depends on info->cea_rev set by drm_parse_cea_ext() above */
+	drm_edid_to_eld(connector, drm_edid);
 }
 
 static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,

@@ -6650,12 +6657,6 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	struct drm_display_info *info = &connector->display_info;
 	int num_modes = 0;
 
-	if (!drm_edid) {
-		drm_reset_display_info(connector);
-		clear_eld(connector);
-		return 0;
-	}
-
 	/*
 	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
 	 * To avoid multiple parsing of same block, lets parse that map

@@ -6663,8 +6664,8 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	 */
 	update_display_info(connector, drm_edid);
 
-	/* Depends on info->cea_rev set by update_display_info() above */
-	drm_edid_to_eld(connector, drm_edid);
+	if (!drm_edid)
+		return 0;
 
 	/*
 	 * EDID spec says modes should be preferred in this order:

@@ -6801,10 +6802,7 @@ static int _drm_connector_update_edid_property(struct drm_connector *connector,
 	 * that it seems better to duplicate it rather than attempt to ensure
 	 * some arbitrary ordering of calls.
 	 */
-	if (drm_edid)
-		update_display_info(connector, drm_edid);
-	else
-		drm_reset_display_info(connector);
+	update_display_info(connector, drm_edid);
 
 	_drm_update_tile_info(connector, drm_edid);
 

Signature · 1 line

2.34.1

The BPC quirks are closer to home in update_display_info().

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

drivers/gpu/drm/drm_edid.c +13 -13

index 810a5fca398a..5881df5bddb9 100644
@@ -6555,6 +6555,18 @@ static void update_display_info(struct drm_connector *connector,
 	if (info->quirks & EDID_QUIRK_CAP_DSC_15BPP)
 		info->max_dsc_bpp = 15;
 
+	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
+		info->bpc = 6;
+
+	if (info->quirks & EDID_QUIRK_FORCE_8BPC)
+		info->bpc = 8;
+
+	if (info->quirks & EDID_QUIRK_FORCE_10BPC)
+		info->bpc = 10;
+
+	if (info->quirks & EDID_QUIRK_FORCE_12BPC)
+		info->bpc = 12;
+
 	/* Depends on info->cea_rev set by drm_parse_cea_ext() above */
 	drm_edid_to_eld(connector, drm_edid);
 }

@@ -6654,7 +6666,7 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 static int _drm_edid_connector_update(struct drm_connector *connector,
 				      const struct drm_edid *drm_edid)
 {
-	struct drm_display_info *info = &connector->display_info;
+	const struct drm_display_info *info = &connector->display_info;
 	int num_modes = 0;
 
 	/*

@@ -6694,18 +6706,6 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	if (info->quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
 		edid_fixup_preferred(connector);
 
-	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
-		info->bpc = 6;
-
-	if (info->quirks & EDID_QUIRK_FORCE_8BPC)
-		info->bpc = 8;
-
-	if (info->quirks & EDID_QUIRK_FORCE_10BPC)
-		info->bpc = 10;
-
-	if (info->quirks & EDID_QUIRK_FORCE_12BPC)
-		info->bpc = 12;
-
 	return num_modes;
 }
 

Signature · 1 line

2.34.1

Quoted 4 lines · Jani Nikula

The BPC quirks are closer to home in update_display_info().

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 58 lines

---
 drivers/gpu/drm/drm_edid.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

drivers/gpu/drm/drm_edid.c +13 -13

index 810a5fca398a..5881df5bddb9 100644
@@ -6555,6 +6555,18 @@ static void update_display_info(struct drm_connector *connector,
 	if (info->quirks & EDID_QUIRK_CAP_DSC_15BPP)
 		info->max_dsc_bpp = 15;
 
+	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
+		info->bpc = 6;
+
+	if (info->quirks & EDID_QUIRK_FORCE_8BPC)
+		info->bpc = 8;
+
+	if (info->quirks & EDID_QUIRK_FORCE_10BPC)
+		info->bpc = 10;
+
+	if (info->quirks & EDID_QUIRK_FORCE_12BPC)
+		info->bpc = 12;
+
 	/* Depends on info->cea_rev set by drm_parse_cea_ext() above */
 	drm_edid_to_eld(connector, drm_edid);
 }

@@ -6654,7 +6666,7 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 static int _drm_edid_connector_update(struct drm_connector *connector,
 				      const struct drm_edid *drm_edid)
 {
-	struct drm_display_info *info = &connector->display_info;
+	const struct drm_display_info *info = &connector->display_info;
 	int num_modes = 0;
 
 	/*

@@ -6694,18 +6706,6 @@ static int _drm_edid_connector_update(struct drm_connector *connector,
 	if (info->quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
 		edid_fixup_preferred(connector);
 
-	if (info->quirks & EDID_QUIRK_FORCE_6BPC)
-		info->bpc = 6;
-
-	if (info->quirks & EDID_QUIRK_FORCE_8BPC)
-		info->bpc = 8;
-
-	if (info->quirks & EDID_QUIRK_FORCE_10BPC)
-		info->bpc = 10;
-
-	if (info->quirks & EDID_QUIRK_FORCE_12BPC)
-		info->bpc = 12;
-
 	return num_modes;
 }
 

Signature · 1 line

2.34.1

By moving update_display_info() out of _drm_edid_connector_update() we make the function purely about adding modes. Rename accordingly.

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/gpu/drm/drm_edid.c +12 -13

index 5881df5bddb9..95c383220afc 100644
@@ -6663,19 +6663,12 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 	return num_modes;
 }
 
-static int _drm_edid_connector_update(struct drm_connector *connector,
-				      const struct drm_edid *drm_edid)
+static int _drm_edid_connector_add_modes(struct drm_connector *connector,
+					 const struct drm_edid *drm_edid)
 {
 	const struct drm_display_info *info = &connector->display_info;
 	int num_modes = 0;
 
-	/*
-	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
-	 * To avoid multiple parsing of same block, lets parse that map
-	 * from sink info, before parsing CEA modes.
-	 */
-	update_display_info(connector, drm_edid);
-
 	if (!drm_edid)
 		return 0;
 

@@ -6780,7 +6773,9 @@ int drm_edid_connector_update(struct drm_connector *connector,
 {
 	int count;
 
-	count = _drm_edid_connector_update(connector, drm_edid);
+	update_display_info(connector, drm_edid);
+
+	count = _drm_edid_connector_add_modes(connector, drm_edid);
 
 	_drm_update_tile_info(connector, drm_edid);
 

@@ -6850,7 +6845,8 @@ EXPORT_SYMBOL(drm_connector_update_edid_property);
  */
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 {
-	struct drm_edid drm_edid;
+	struct drm_edid _drm_edid;
+	const struct drm_edid *drm_edid;
 
 	if (edid && !drm_edid_is_valid(edid)) {
 		drm_warn(connector->dev, "[CONNECTOR:%d:%s] EDID invalid.\n",

@@ -6858,8 +6854,11 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 		edid = NULL;
 	}
 
-	return _drm_edid_connector_update(connector,
-					  drm_edid_legacy_init(&drm_edid, edid));
+	drm_edid = drm_edid_legacy_init(&_drm_edid, edid);
+
+	update_display_info(connector, drm_edid);
+
+	return _drm_edid_connector_add_modes(connector, drm_edid);
 }
 EXPORT_SYMBOL(drm_add_edid_modes);
 

Signature · 1 line

2.34.1