[Intel-gfx] [PATCH v7 00/22] drm/edid: info & modes parsing and drm_edid refactors

[Intel-gfx] [PATCH v7 00/22] drm/edid: info & modes parsing and drm_edid refactors

[Jani Nikula]

Another step deeper into the EDID rabbit hole.

v7 of [1], with a bunch of stuff added regarding display info and modes
parsing. Primarily separating them to two distinct steps. To do that
cleanly, we need a bunch of refactors. This should clean up any
inconsistent states with add modes modifying the display info. And
generally make the code neater.

There are also a couple of bug fixes first.

BR,
Jani.


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


Jani Nikula (22):
  drm/edid: fix AVI infoframe aspect ratio handling
  drm/edid: fix parsing of 3D modes from HDMI VSDB
  drm/edid: parse VICs from CTA VDB early
  drm/edid: Use the pre-parsed VICs
  drm/edid: use VIC in AVI infoframe if sink lists it in CTA VDB
  drm/edid: rename struct drm_display_info *display to *info
  drm/edid: refactor CTA Y420CMDB parsing
  drm/edid: split CTA Y420VDB info and mode parsing
  drm/edid: fix and clarify HDMI VSDB audio latency parsing
  drm/edid: add helper for HDMI VSDB audio latency field length
  drm/edid: split HDMI VSDB info and mode parsing
  drm/edid: store quirks in display info
  drm/edid: stop passing quirks around
  drm/edid: merge ELD handling to update_display_info()
  drm/edid: move EDID BPC quirk application to update_display_info()
  drm/edid: refactor _drm_edid_connector_update() and rename
  drm/edid: add separate drm_edid_connector_add_modes()
  drm/edid: remove redundant _drm_connector_update_edid_property()
  drm/i915/edid: convert DP, HDMI and LVDS to drm_edid
  drm/i915/bios: convert intel_bios_init_panel() to drm_edid
  drm/i915/opregion: convert intel_opregion_get_edid() to struct
    drm_edid
  drm/i915/panel: move panel fixed EDID to struct intel_panel

 drivers/gpu/drm/drm_connector.c               |   1 +
 drivers/gpu/drm/drm_edid.c                    | 554 ++++++++++--------
 drivers/gpu/drm/drm_probe_helper.c            |   4 +-
 drivers/gpu/drm/i915/display/icl_dsi.c        |   2 +-
 drivers/gpu/drm/i915/display/intel_bios.c     |  23 +-
 drivers/gpu/drm/i915/display/intel_bios.h     |   4 +-
 .../gpu/drm/i915/display/intel_connector.c    |   5 +-
 .../drm/i915/display/intel_display_types.h    |   8 +-
 drivers/gpu/drm/i915/display/intel_dp.c       |  91 +--
 drivers/gpu/drm/i915/display/intel_dvo.c      |   2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  28 +-
 drivers/gpu/drm/i915/display/intel_lvds.c     |  51 +-
 drivers/gpu/drm/i915/display/intel_opregion.c |  29 +-
 drivers/gpu/drm/i915/display/intel_opregion.h |   4 +-
 drivers/gpu/drm/i915/display/intel_panel.c    |  10 +-
 drivers/gpu/drm/i915/display/intel_panel.h    |   4 +-
 drivers/gpu/drm/i915/display/intel_sdvo.c     |   2 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c        |   2 +-
 include/drm/drm_connector.h                   |  18 +-
 include/drm/drm_edid.h                        |   2 +
 20 files changed, 488 insertions(+), 356 deletions(-)

-- 
2.34.1

[Intel-gfx] [PATCH v7 01/22] drm/edid: fix AVI infoframe aspect ratio handling

[Jani Nikula]

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 <william.tseng@intel.com>
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6153
References: https://lore.kernel.org/r/20220920062316.43162-1-william.tseng@intel.com
Cc: <stable@vger.kernel.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3841aba17abd..23f7146e6a9b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 01/22] drm/edid: fix AVI infoframe aspect ratio handling

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:16PM +0200, Jani Nikula wrote:
> 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 <william.tseng@intel.com>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6153
> References: https://lore.kernel.org/r/20220920062316.43162-1-william.tseng@intel.com
> Cc: <stable@vger.kernel.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3841aba17abd..23f7146e6a9b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 02/22] drm/edid: fix parsing of 3D modes from HDMI VSDB

[Jani Nikula]

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: <stable@vger.kernel.org> # v6.0+
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.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 23f7146e6a9b..b94adb9bbefb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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);
 	}
 	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;
 }
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 02/22] drm/edid: fix parsing of 3D modes from HDMI VSDB

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:17PM +0200, Jani Nikula wrote:
> 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: <stable@vger.kernel.org> # v6.0+
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.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 23f7146e6a9b..b94adb9bbefb 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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);
>  	}
>  	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);

I wonder if there are any EDIDs with multiple copies
of either data block... But the original code couldn't
deal with that either so not really a concern for this
patch.

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

> +
>  	return modes;
>  }
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 03/22] drm/edid: parse VICs from CTA VDB early

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 8d92777e57dd..21b3df75ab8c 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -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;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b94adb9bbefb..90846dc69061 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9037f1317aee..d97ed06d5837 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -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,
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 03/22] drm/edid: parse VICs from CTA VDB early

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:18PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  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(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8d92777e57dd..21b3df75ab8c 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -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;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index b94adb9bbefb..90846dc69061 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9037f1317aee..d97ed06d5837 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -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,
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 04/22] drm/edid: Use the pre-parsed VICs

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 92 +++++++++++++-------------------------
 1 file changed, 32 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 90846dc69061..7f0386175230 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
 }
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 04/22] drm/edid: Use the pre-parsed VICs

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:19PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Cool. Takes care of my previously stated concern of
multiple CTA/HDMI data blocks.

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

> ---
>  drivers/gpu/drm/drm_edid.c | 92 +++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 90846dc69061..7f0386175230 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
>  }
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 05/22] drm/edid: use VIC in AVI infoframe if sink lists it in CTA VDB

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Cc: William Tseng <william.tseng@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7f0386175230..3dfcd6450f10 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 05/22] drm/edid: use VIC in AVI infoframe if sink lists it in CTA VDB

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:20PM +0200, Jani Nikula wrote:
> 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.

Not 100% this is a real issue or not. IIRC we decided to 
play it safe regardless.

However later CTA specs do clarify the handling of VIC >=128 which
apparently are known to have issues, and we're even supposed to
use the presence of such SVDs to determine which version of the
AVI infoframe to transmit. Currently we don't handle that stuff
correctly, but looks like it'll be much easier to remedy now.

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

> 
> 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ä <ville.syrjala@linux.intel.com>
> Cc: William Tseng <william.tseng@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7f0386175230..3dfcd6450f10 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 06/22] drm/edid: rename struct drm_display_info *display to *info

[Jani Nikula]

Rename the local variable to info for consistency.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3dfcd6450f10..4e9108e9fc96 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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) {
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 06/22] drm/edid: rename struct drm_display_info *display to *info

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:21PM +0200, Jani Nikula wrote:
> Rename the local variable to info for consistency.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3dfcd6450f10..4e9108e9fc96 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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) {
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 07/22] drm/edid: refactor CTA Y420CMDB parsing

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 86 ++++++++++++++++++-------------------
 include/drm/drm_connector.h |  3 --
 2 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 4e9108e9fc96..ead7a4ce0422 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index d97ed06d5837..1c26c4e72c62 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -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;
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 07/22] drm/edid: refactor CTA Y420CMDB parsing

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:22PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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  | 86 ++++++++++++++++++-------------------
>  include/drm/drm_connector.h |  3 --
>  2 files changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 4e9108e9fc96..ead7a4ce0422 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index d97ed06d5837..1c26c4e72c62 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -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;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 08/22] drm/edid: split CTA Y420VDB info and mode parsing

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ead7a4ce0422..076ba125c38d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
 }
 
@@ -5876,6 +5871,26 @@ static bool cta_vdb_has_vic(const struct drm_connector *connector, u8 vic)
 	return false;
 }
 
+/* CTA-861-H YCbCr 4:2:0 Video Data Block (CTA Y420VDB) */
+static void parse_cta_y420vdb(struct drm_connector *connector,
+			      const struct cea_db *db)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+	const u8 *svds = cea_db_data(db) + 1;
+	int i;
+
+	for (i = 0; i < cea_db_payload_len(db) - 1; i++) {
+		u8 vic = svd_to_vic(svds[i]);
+
+		if (!drm_valid_cea_vic(vic))
+			continue;
+
+		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
+		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))
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 08/22] drm/edid: split CTA Y420VDB info and mode parsing

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:23PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ead7a4ce0422..076ba125c38d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
>  }
>  
> @@ -5876,6 +5871,26 @@ static bool cta_vdb_has_vic(const struct drm_connector *connector, u8 vic)
>  	return false;
>  }
>  
> +/* CTA-861-H YCbCr 4:2:0 Video Data Block (CTA Y420VDB) */
> +static void parse_cta_y420vdb(struct drm_connector *connector,
> +			      const struct cea_db *db)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> +	const u8 *svds = cea_db_data(db) + 1;

Sidenote: I wonder if we should abstract the payload handling
better for blocks using extended tag codes...

> +	int i;
> +
> +	for (i = 0; i < cea_db_payload_len(db) - 1; i++) {
> +		u8 vic = svd_to_vic(svds[i]);
> +
> +		if (!drm_valid_cea_vic(vic))
> +			continue;
> +
> +		bitmap_set(hdmi->y420_vdb_modes, vic, 1);

I'm thinking we should probably also add these to the vic list.
But I suppose we can take care of that with a separate patch.

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

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

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 09/22] drm/edid: fix and clarify HDMI VSDB audio latency parsing

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 076ba125c38d..847076b29594 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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",
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 09/22] drm/edid: fix and clarify HDMI VSDB audio latency parsing

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:24PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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 | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 076ba125c38d..847076b29594 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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",
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 10/22] drm/edid: add helper for HDMI VSDB audio latency field length

[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.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 847076b29594..93067b8dd9f6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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 */
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 10/22] drm/edid: add helper for HDMI VSDB audio latency field length

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:25PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 847076b29594..93067b8dd9f6 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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 */
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v7 10/22] drm/edid: add helper for HDMI VSDB audio latency field length

[Jani Nikula]

On Wed, 18 Jan 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Jan 04, 2023 at 12:05:25PM +0200, Jani Nikula wrote:
>> 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ä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the reviews so far, pushed up to and including this patch to
drm-misc-next.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 847076b29594..93067b8dd9f6 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -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 */
>> -- 
>> 2.34.1

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] [PATCH v7 11/22] drm/edid: split HDMI VSDB info and mode parsing

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 39 +++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 93067b8dd9f6..5cb1d36ce48a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
+
+	if (offset < size)
+		offset += hdmi_vsdb_latency_length(db);
+
+	/* 3D_present */
+	if (offset < size && db[offset++] & BIT(7))
+		return true;
+
+	/* HDMI_VIC_LEN and HDMI_3D_LEN */
+	if (offset < size && db[offset])
+		return true;
+
+	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);
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 11/22] drm/edid: split HDMI VSDB info and mode parsing

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:26PM +0200, Jani Nikula wrote:
> 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.

I do wonder how much point there is in this difference between
the payload handling for HDMI vs. CTA specified blocks. CTA
specifies the bytes using 1-based indexing so the indices won't
directly match the spec there either. Perhaps we should just use
the same approach for all data blocks regardless of where they're
specified. Anyways, just food for thought in the future.

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 39 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 93067b8dd9f6..5cb1d36ce48a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
> +
> +	if (offset < size)
> +		offset += hdmi_vsdb_latency_length(db);
> +
> +	/* 3D_present */
> +	if (offset < size && db[offset++] & BIT(7))
> +		return true;
> +
> +	/* HDMI_VIC_LEN and HDMI_3D_LEN */
> +	if (offset < size && db[offset])
> +		return true;

I'm thinking it should be enough to just check the HDMI_Video_present
bit. Granted it would be a slight change in behaviour if there are 
EDIDs with said bit set but with no actual 3D/HDMI modes included. But
such sinks would still conform to spec version 1.4 and so should have
no problems with the infoframe.

> +
> +	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);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH] drm/edid: split HDMI VSDB info and mode parsing

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e8b67f3f5c91..ee453e39562a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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);
-- 
2.34.1

Re: [Intel-gfx] [PATCH] drm/edid: split HDMI VSDB info and mode parsing

[Ville Syrjälä]

On Thu, Jan 19, 2023 at 05:46:28PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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 | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e8b67f3f5c91..ee453e39562a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 12/22] drm/edid: store quirks in display info

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 42 ++++++++++++++++++-------------------
 include/drm/drm_connector.h |  5 +++++
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5cb1d36ce48a..fd8d056e38c1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1c26c4e72c62..7b5048516185 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -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,
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 12/22] drm/edid: store quirks in display info

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:27PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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  | 42 ++++++++++++++++++-------------------
>  include/drm/drm_connector.h |  5 +++++
>  2 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 5cb1d36ce48a..fd8d056e38c1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1c26c4e72c62..7b5048516185 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -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,
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 13/22] drm/edid: stop passing quirks around

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fd8d056e38c1..6bc0432046c8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 13/22] drm/edid: stop passing quirks around

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:28PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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 | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index fd8d056e38c1..6bc0432046c8 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 14/22] drm/edid: merge ELD handling to update_display_info()

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6bc0432046c8..810a5fca398a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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);
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 14/22] drm/edid: merge ELD handling to update_display_info()

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:29PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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 | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6bc0432046c8..810a5fca398a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 15/22] drm/edid: move EDID BPC quirk application to update_display_info()

[Jani Nikula]

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

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 810a5fca398a..5881df5bddb9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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;
 }
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 15/22] drm/edid: move EDID BPC quirk application to update_display_info()

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:30PM +0200, Jani Nikula wrote:
> The BPC quirks are closer to home in update_display_info().
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 810a5fca398a..5881df5bddb9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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;
>  }
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v7 15/22] drm/edid: move EDID BPC quirk application to update_display_info()

[Jani Nikula]

On Wed, 18 Jan 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Jan 04, 2023 at 12:05:30PM +0200, Jani Nikula wrote:
>> The BPC quirks are closer to home in update_display_info().
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, also pushed patches 12-15 because they don't depend on patch 11.

BR,
Jani.

>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 810a5fca398a..5881df5bddb9 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -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;
>>  }
>>  
>> -- 
>> 2.34.1

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] [PATCH v7 16/22] drm/edid: refactor _drm_edid_connector_update() and rename

[Jani Nikula]

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ä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5881df5bddb9..95c383220afc 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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);
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 16/22] drm/edid: refactor _drm_edid_connector_update() and rename

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:31PM +0200, Jani Nikula wrote:
> 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ä <ville.syrjala@linux.intel.com>
> 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, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 5881df5bddb9..95c383220afc 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 17/22] drm/edid: add separate drm_edid_connector_add_modes()

[Jani Nikula]

The original goal with drm_edid_connector_update() was to have a single
call for updating the connector and adding probed modes, in this order,
but that turned out to be problematic. Drivers that need to update the
connector in the .detect() callback would end up updating the probed
modes as well. Turns out the callback may be called so many times that
the probed mode list fills up without bounds, and this is amplified by
add_alternate_cea_modes() duplicating the CEA modes on every call,
actually running out of memory on some machines.

Kudos to Imre Deak <imre.deak@intel.com> for explaining this to me.

Go back to having separate drm_edid_connector_update() and
drm_edid_connector_add_modes() calls. The former may be called from
.detect(), .force(), or .get_modes(), but the latter only from
.get_modes().

Unlike drm_add_edid_modes(), have drm_edid_connector_add_modes() update
the probed modes from the EDID property instead of the passed in
EDID. This is mainly to enforce two things:

1) drm_edid_connector_update() must be called before
   drm_edid_connector_add_modes().

   Display info and quirks are needed for parsing the modes, and we
   don't want to call update_display_info() again to ensure the info is
   available, like drm_add_edid_modes() does.

2) The same EDID is used for both updating the connector and adding the
   probed modes.

Fortunately, the change is easy, because no driver has actually adopted
drm_edid_connector_update(). Not even i915, and that's mainly because of
the problem described above.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c         | 44 +++++++++++++++++++++++-------
 drivers/gpu/drm/drm_probe_helper.c |  4 ++-
 include/drm/drm_edid.h             |  2 ++
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 95c383220afc..a64c0807e97f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6761,30 +6761,54 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
  * @connector: Connector
  * @drm_edid: EDID
  *
- * Update the connector mode list, display info, ELD, HDR metadata, relevant
- * properties, etc. from the passed in EDID.
+ * Update the connector display info, ELD, HDR metadata, relevant properties,
+ * etc. from the passed in EDID.
  *
  * If EDID is NULL, reset the information.
  *
- * Return: The number of modes added or 0 if we couldn't find any.
+ * Must be called before calling drm_edid_connector_add_modes().
+ *
+ * Return: 0 on success, negative error on errors.
  */
 int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *drm_edid)
 {
+	update_display_info(connector, drm_edid);
+
+	_drm_update_tile_info(connector, drm_edid);
+
+	return _drm_edid_connector_property_update(connector, drm_edid);
+}
+EXPORT_SYMBOL(drm_edid_connector_update);
+
+/**
+ * drm_edid_connector_add_modes - Update probed modes from the EDID property
+ * @connector: Connector
+ *
+ * Add the modes from the previously updated EDID property to the connector
+ * probed modes list.
+ *
+ * drm_edid_connector_update() must have been called before this to update the
+ * EDID property.
+ *
+ * Return: The number of modes added, or 0 if we couldn't find any.
+ */
+int drm_edid_connector_add_modes(struct drm_connector *connector)
+{
+	const struct drm_edid *drm_edid = NULL;
 	int count;
 
-	update_display_info(connector, drm_edid);
+	if (connector->edid_blob_ptr)
+		drm_edid = drm_edid_alloc(connector->edid_blob_ptr->data,
+					  connector->edid_blob_ptr->length);
 
 	count = _drm_edid_connector_add_modes(connector, drm_edid);
 
-	_drm_update_tile_info(connector, drm_edid);
-
-	/* Note: Ignore errors for now. */
-	_drm_edid_connector_property_update(connector, drm_edid);
+	drm_edid_free(drm_edid);
 
 	return count;
 }
-EXPORT_SYMBOL(drm_edid_connector_update);
+EXPORT_SYMBOL(drm_edid_connector_add_modes);
 
 static int _drm_connector_update_edid_property(struct drm_connector *connector,
 					       const struct drm_edid *drm_edid)
@@ -6839,7 +6863,7 @@ EXPORT_SYMBOL(drm_connector_update_edid_property);
  * &drm_display_info structure and ELD in @connector with any information which
  * can be derived from the edid.
  *
- * This function is deprecated. Use drm_edid_connector_update() instead.
+ * This function is deprecated. Use drm_edid_connector_add_modes() instead.
  *
  * Return: The number of modes added or 0 if we couldn't find any.
  */
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 1ea053cef557..26844befc6f5 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1139,7 +1139,9 @@ int drm_connector_helper_get_modes(struct drm_connector *connector)
 	 * EDID. Otherwise, if the EDID is NULL, clear the connector
 	 * information.
 	 */
-	count = drm_edid_connector_update(connector, drm_edid);
+	drm_edid_connector_update(connector, drm_edid);
+
+	count = drm_edid_connector_add_modes(connector);
 
 	drm_edid_free(drm_edid);
 
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 372963600f1d..70ae6c290bdc 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -609,6 +609,8 @@ const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
 					    void *context);
 int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *edid);
+int drm_edid_connector_add_modes(struct drm_connector *connector);
+
 const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index);
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 17/22] drm/edid: add separate drm_edid_connector_add_modes()

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:32PM +0200, Jani Nikula wrote:
> The original goal with drm_edid_connector_update() was to have a single
> call for updating the connector and adding probed modes, in this order,
> but that turned out to be problematic. Drivers that need to update the
> connector in the .detect() callback would end up updating the probed
> modes as well. Turns out the callback may be called so many times that
> the probed mode list fills up without bounds, and this is amplified by
> add_alternate_cea_modes() duplicating the CEA modes on every call,
> actually running out of memory on some machines.
> 
> Kudos to Imre Deak <imre.deak@intel.com> for explaining this to me.
> 
> Go back to having separate drm_edid_connector_update() and
> drm_edid_connector_add_modes() calls. The former may be called from
> .detect(), .force(), or .get_modes(), but the latter only from
> .get_modes().
> 
> Unlike drm_add_edid_modes(), have drm_edid_connector_add_modes() update
> the probed modes from the EDID property instead of the passed in
> EDID. This is mainly to enforce two things:
> 
> 1) drm_edid_connector_update() must be called before
>    drm_edid_connector_add_modes().
> 
>    Display info and quirks are needed for parsing the modes, and we
>    don't want to call update_display_info() again to ensure the info is
>    available, like drm_add_edid_modes() does.
> 
> 2) The same EDID is used for both updating the connector and adding the
>    probed modes.
> 
> Fortunately, the change is easy, because no driver has actually adopted
> drm_edid_connector_update(). Not even i915, and that's mainly because of
> the problem described above.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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         | 44 +++++++++++++++++++++++-------
>  drivers/gpu/drm/drm_probe_helper.c |  4 ++-
>  include/drm/drm_edid.h             |  2 ++
>  3 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 95c383220afc..a64c0807e97f 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6761,30 +6761,54 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>   * @connector: Connector
>   * @drm_edid: EDID
>   *
> - * Update the connector mode list, display info, ELD, HDR metadata, relevant
> - * properties, etc. from the passed in EDID.
> + * Update the connector display info, ELD, HDR metadata, relevant properties,
> + * etc. from the passed in EDID.
>   *
>   * If EDID is NULL, reset the information.
>   *
> - * Return: The number of modes added or 0 if we couldn't find any.
> + * Must be called before calling drm_edid_connector_add_modes().
> + *
> + * Return: 0 on success, negative error on errors.
>   */
>  int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *drm_edid)
>  {
> +	update_display_info(connector, drm_edid);
> +
> +	_drm_update_tile_info(connector, drm_edid);
> +
> +	return _drm_edid_connector_property_update(connector, drm_edid);
> +}
> +EXPORT_SYMBOL(drm_edid_connector_update);
> +
> +/**
> + * drm_edid_connector_add_modes - Update probed modes from the EDID property
> + * @connector: Connector
> + *
> + * Add the modes from the previously updated EDID property to the connector
> + * probed modes list.
> + *
> + * drm_edid_connector_update() must have been called before this to update the
> + * EDID property.
> + *
> + * Return: The number of modes added, or 0 if we couldn't find any.
> + */
> +int drm_edid_connector_add_modes(struct drm_connector *connector)
> +{
> +	const struct drm_edid *drm_edid = NULL;
>  	int count;
>  
> -	update_display_info(connector, drm_edid);
> +	if (connector->edid_blob_ptr)
> +		drm_edid = drm_edid_alloc(connector->edid_blob_ptr->data,
> +					  connector->edid_blob_ptr->length);
>  
>  	count = _drm_edid_connector_add_modes(connector, drm_edid);
>  
> -	_drm_update_tile_info(connector, drm_edid);
> -
> -	/* Note: Ignore errors for now. */
> -	_drm_edid_connector_property_update(connector, drm_edid);
> +	drm_edid_free(drm_edid);
>  
>  	return count;
>  }
> -EXPORT_SYMBOL(drm_edid_connector_update);
> +EXPORT_SYMBOL(drm_edid_connector_add_modes);
>  
>  static int _drm_connector_update_edid_property(struct drm_connector *connector,
>  					       const struct drm_edid *drm_edid)
> @@ -6839,7 +6863,7 @@ EXPORT_SYMBOL(drm_connector_update_edid_property);
>   * &drm_display_info structure and ELD in @connector with any information which
>   * can be derived from the edid.
>   *
> - * This function is deprecated. Use drm_edid_connector_update() instead.
> + * This function is deprecated. Use drm_edid_connector_add_modes() instead.
>   *
>   * Return: The number of modes added or 0 if we couldn't find any.
>   */
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1ea053cef557..26844befc6f5 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1139,7 +1139,9 @@ int drm_connector_helper_get_modes(struct drm_connector *connector)
>  	 * EDID. Otherwise, if the EDID is NULL, clear the connector
>  	 * information.
>  	 */
> -	count = drm_edid_connector_update(connector, drm_edid);
> +	drm_edid_connector_update(connector, drm_edid);
> +
> +	count = drm_edid_connector_add_modes(connector);
>  
>  	drm_edid_free(drm_edid);
>  
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 372963600f1d..70ae6c290bdc 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -609,6 +609,8 @@ const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
>  					    void *context);
>  int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *edid);
> +int drm_edid_connector_add_modes(struct drm_connector *connector);
> +
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 18/22] drm/edid: remove redundant _drm_connector_update_edid_property()

[Jani Nikula]

Realize that drm_edid_connector_update() and
_drm_connector_update_edid_property() are now the same thing. Drop the
latter.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a64c0807e97f..ae50f533fea3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6810,24 +6810,6 @@ int drm_edid_connector_add_modes(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_edid_connector_add_modes);
 
-static int _drm_connector_update_edid_property(struct drm_connector *connector,
-					       const struct drm_edid *drm_edid)
-{
-	/*
-	 * Set the display info, using edid if available, otherwise resetting
-	 * the values to defaults. This duplicates the work done in
-	 * drm_add_edid_modes, but that function is not consistently called
-	 * before this one in all drivers and the computation is cheap enough
-	 * that it seems better to duplicate it rather than attempt to ensure
-	 * some arbitrary ordering of calls.
-	 */
-	update_display_info(connector, drm_edid);
-
-	_drm_update_tile_info(connector, drm_edid);
-
-	return _drm_edid_connector_property_update(connector, drm_edid);
-}
-
 /**
  * drm_connector_update_edid_property - update the edid property of a connector
  * @connector: drm connector
@@ -6849,8 +6831,7 @@ int drm_connector_update_edid_property(struct drm_connector *connector,
 {
 	struct drm_edid drm_edid;
 
-	return _drm_connector_update_edid_property(connector,
-						   drm_edid_legacy_init(&drm_edid, edid));
+	return drm_edid_connector_update(connector, drm_edid_legacy_init(&drm_edid, edid));
 }
 EXPORT_SYMBOL(drm_connector_update_edid_property);
 
-- 
2.34.1

Re: [Intel-gfx] [PATCH v7 18/22] drm/edid: remove redundant _drm_connector_update_edid_property()

[Ville Syrjälä]

On Wed, Jan 04, 2023 at 12:05:33PM +0200, Jani Nikula wrote:
> Realize that drm_edid_connector_update() and
> _drm_connector_update_edid_property() are now the same thing. Drop the
> latter.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a64c0807e97f..ae50f533fea3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6810,24 +6810,6 @@ int drm_edid_connector_add_modes(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_edid_connector_add_modes);
>  
> -static int _drm_connector_update_edid_property(struct drm_connector *connector,
> -					       const struct drm_edid *drm_edid)
> -{
> -	/*
> -	 * Set the display info, using edid if available, otherwise resetting
> -	 * the values to defaults. This duplicates the work done in
> -	 * drm_add_edid_modes, but that function is not consistently called
> -	 * before this one in all drivers and the computation is cheap enough
> -	 * that it seems better to duplicate it rather than attempt to ensure
> -	 * some arbitrary ordering of calls.
> -	 */
> -	update_display_info(connector, drm_edid);
> -
> -	_drm_update_tile_info(connector, drm_edid);
> -
> -	return _drm_edid_connector_property_update(connector, drm_edid);
> -}
> -
>  /**
>   * drm_connector_update_edid_property - update the edid property of a connector
>   * @connector: drm connector
> @@ -6849,8 +6831,7 @@ int drm_connector_update_edid_property(struct drm_connector *connector,
>  {
>  	struct drm_edid drm_edid;
>  
> -	return _drm_connector_update_edid_property(connector,
> -						   drm_edid_legacy_init(&drm_edid, edid));
> +	return drm_edid_connector_update(connector, drm_edid_legacy_init(&drm_edid, edid));
>  }
>  EXPORT_SYMBOL(drm_connector_update_edid_property);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH v7 19/22] drm/i915/edid: convert DP, HDMI and LVDS to drm_edid

[Jani Nikula]

Convert all the connectors that use cached connector edid and
detect_edid to drm_edid.

Since drm_get_edid() calls drm_connector_update_edid_property() while
drm_edid_read*() do not, we need to call drm_edid_connector_update()
separately, in part due to the EDID caching behaviour in HDMI and
DP. Especially DP depends on the details parsed from EDID. (The big
behavioural change conflating EDID reading with parsing and property
update was done in commit 5186421cbfe2 ("drm: Introduce epoch counter to
drm_connector"))

v6: Rebase on drm_edid_connector_add_modes()

v5: Fix potential uninitialized var use (kernel test robot <lkp@intel.com>)

v4: Call drm_edid_connector_update() after reading HDMI/DP EDID

v3: Don't leak vga switcheroo EDID in LVDS init (Ville)

v2: Don't leak opregion fallback EDID (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 .../gpu/drm/i915/display/intel_connector.c    |  4 +-
 .../drm/i915/display/intel_display_types.h    |  4 +-
 drivers/gpu/drm/i915/display/intel_dp.c       | 83 +++++++++++--------
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 28 ++++---
 drivers/gpu/drm/i915/display/intel_lvds.c     | 46 ++++++----
 5 files changed, 96 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 562da3b741e2..4814d4e2f7f9 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -95,12 +95,12 @@ void intel_connector_destroy(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
-	kfree(intel_connector->detect_edid);
+	drm_edid_free(intel_connector->detect_edid);
 
 	intel_hdcp_cleanup(intel_connector);
 
 	if (!IS_ERR_OR_NULL(intel_connector->edid))
-		kfree(intel_connector->edid);
+		drm_edid_free(intel_connector->edid);
 
 	intel_panel_fini(intel_connector);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 32e8b2fc3cc6..34dc850340b8 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -592,8 +592,8 @@ struct intel_connector {
 	struct intel_panel panel;
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
-	struct edid *edid;
-	struct edid *detect_edid;
+	const struct drm_edid *edid;
+	const struct drm_edid *detect_edid;
 
 	/* Number of times hotplug detection was tried after an HPD interrupt */
 	int hotplug_retries;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fd..6caa290ef6e6 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3651,12 +3651,11 @@ static u8 intel_dp_autotest_edid(struct intel_dp *intel_dp)
 				    intel_dp->aux.i2c_defer_count);
 		intel_dp->compliance.test_data.edid = INTEL_DP_RESOLUTION_FAILSAFE;
 	} else {
-		struct edid *block = intel_connector->detect_edid;
+		/* FIXME: Get rid of drm_edid_raw() */
+		const struct edid *block = drm_edid_raw(intel_connector->detect_edid);
 
-		/* We have to write the checksum
-		 * of the last block read
-		 */
-		block += intel_connector->detect_edid->extensions;
+		/* We have to write the checksum of the last block read */
+		block += block->extensions;
 
 		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_EDID_CHECKSUM,
 				       block->checksum) <= 0)
@@ -4478,7 +4477,7 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
 	return is_connected;
 }
 
-static struct edid *
+static const struct drm_edid *
 intel_dp_get_edid(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
@@ -4489,18 +4488,22 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
 		if (IS_ERR(intel_connector->edid))
 			return NULL;
 
-		return drm_edid_duplicate(intel_connector->edid);
+		return drm_edid_dup(intel_connector->edid);
 	} else
-		return drm_get_edid(&intel_connector->base,
-				    &intel_dp->aux.ddc);
+		return drm_edid_read_ddc(&intel_connector->base,
+					 &intel_dp->aux.ddc);
 }
 
 static void
 intel_dp_update_dfp(struct intel_dp *intel_dp,
-		    const struct edid *edid)
+		    const struct drm_edid *drm_edid)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_connector *connector = intel_dp->attached_connector;
+	const struct edid *edid;
+
+	/* FIXME: Get rid of drm_edid_raw() */
+	edid = drm_edid_raw(drm_edid);
 
 	intel_dp->dfp.max_bpc =
 		drm_dp_downstream_max_bpc(intel_dp->dpcd,
@@ -4600,21 +4603,27 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_connector *connector = intel_dp->attached_connector;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 	bool vrr_capable;
 
 	intel_dp_unset_edid(intel_dp);
-	edid = intel_dp_get_edid(intel_dp);
-	connector->detect_edid = edid;
+	drm_edid = intel_dp_get_edid(intel_dp);
+	connector->detect_edid = drm_edid;
+
+	/* Below we depend on display info having been updated */
+	drm_edid_connector_update(&connector->base, drm_edid);
 
 	vrr_capable = intel_vrr_is_capable(connector);
 	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
 		    connector->base.base.id, connector->base.name, str_yes_no(vrr_capable));
 	drm_connector_set_vrr_capable_property(&connector->base, vrr_capable);
 
-	intel_dp_update_dfp(intel_dp, edid);
+	intel_dp_update_dfp(intel_dp, drm_edid);
 	intel_dp_update_420(intel_dp);
 
+	/* FIXME: Get rid of drm_edid_raw() */
+	edid = drm_edid_raw(drm_edid);
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
 		intel_dp->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
 		intel_dp->has_audio = drm_detect_monitor_audio(edid);
@@ -4629,7 +4638,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	struct intel_connector *connector = intel_dp->attached_connector;
 
 	drm_dp_cec_unset_edid(&intel_dp->aux);
-	kfree(connector->detect_edid);
+	drm_edid_free(connector->detect_edid);
 	connector->detect_edid = NULL;
 
 	intel_dp->has_hdmi_sink = false;
@@ -4793,12 +4802,10 @@ intel_dp_force(struct drm_connector *connector)
 static int intel_dp_get_modes(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct edid *edid;
-	int num_modes = 0;
+	int num_modes;
 
-	edid = intel_connector->detect_edid;
-	if (edid)
-		num_modes = intel_connector_update_modes(connector, edid);
+	/* drm_edid_connector_update() done in ->detect() or ->force() */
+	num_modes = drm_edid_connector_add_modes(connector);
 
 	/* Also add fixed mode, which may or may not be present in EDID */
 	if (intel_dp_is_edp(intel_attached_dp(intel_connector)))
@@ -4807,7 +4814,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	if (num_modes)
 		return num_modes;
 
-	if (!edid) {
+	if (!intel_connector->detect_edid) {
 		struct intel_dp *intel_dp = intel_attached_dp(intel_connector);
 		struct drm_display_mode *mode;
 
@@ -5243,7 +5250,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct drm_display_mode *fixed_mode;
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	bool has_dpcd;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 
 	if (!intel_dp_is_edp(intel_dp))
 		return true;
@@ -5290,29 +5297,35 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 
 	mutex_lock(&dev_priv->drm.mode_config.mutex);
-	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
-	if (!edid) {
+	drm_edid = drm_edid_read_ddc(connector, &intel_dp->aux.ddc);
+	if (!drm_edid) {
+		const struct edid *edid;
+
 		/* Fallback to EDID from ACPI OpRegion, if any */
+		/* FIXME: Make intel_opregion_get_edid() return drm_edid */
 		edid = intel_opregion_get_edid(intel_connector);
-		if (edid)
+		if (edid) {
+			drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
 			drm_dbg_kms(&dev_priv->drm,
 				    "[CONNECTOR:%d:%s] Using OpRegion EDID\n",
 				    connector->base.id, connector->name);
-	}
-	if (edid) {
-		if (drm_add_edid_modes(connector, edid)) {
-			drm_connector_update_edid_property(connector, edid);
-		} else {
 			kfree(edid);
-			edid = ERR_PTR(-EINVAL);
+		}
+	}
+	if (drm_edid) {
+		if (drm_edid_connector_update(connector, drm_edid) ||
+		    !drm_edid_connector_add_modes(connector)) {
+			drm_edid_connector_update(connector, NULL);
+			drm_edid_free(drm_edid);
+			drm_edid = ERR_PTR(-EINVAL);
 		}
 	} else {
-		edid = ERR_PTR(-ENOENT);
+		drm_edid = ERR_PTR(-ENOENT);
 	}
-	intel_connector->edid = edid;
+	intel_connector->edid = drm_edid;
 
-	intel_bios_init_panel_late(dev_priv, &intel_connector->panel,
-				   encoder->devdata, IS_ERR(edid) ? NULL : edid);
+	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, encoder->devdata,
+				   IS_ERR_OR_NULL(drm_edid) ? NULL : drm_edid_raw(drm_edid));
 
 	intel_panel_add_edid_fixed_modes(intel_connector, true);
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index efa2da080f62..e69b14704494 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2359,7 +2359,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->dp_dual_mode.type = DRM_DP_DUAL_MODE_NONE;
 	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
 
-	kfree(to_intel_connector(connector)->detect_edid);
+	drm_edid_free(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
 }
 
@@ -2420,7 +2420,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(to_intel_connector(connector));
 	intel_wakeref_t wakeref;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 	bool connected = false;
 	struct i2c_adapter *i2c;
 
@@ -2428,17 +2429,23 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 
 	i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
 
-	edid = drm_get_edid(connector, i2c);
+	drm_edid = drm_edid_read_ddc(connector, i2c);
 
-	if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
+	if (!drm_edid && !intel_gmbus_is_forced_bit(i2c)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n");
 		intel_gmbus_force_bit(i2c, true);
-		edid = drm_get_edid(connector, i2c);
+		drm_edid = drm_edid_read_ddc(connector, i2c);
 		intel_gmbus_force_bit(i2c, false);
 	}
 
-	to_intel_connector(connector)->detect_edid = edid;
+	/* Below we depend on display info having been updated */
+	drm_edid_connector_update(connector, drm_edid);
+
+	to_intel_connector(connector)->detect_edid = drm_edid;
+
+	/* FIXME: Get rid of drm_edid_raw() */
+	edid = drm_edid_raw(drm_edid);
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
 		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 		intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
@@ -2514,13 +2521,8 @@ intel_hdmi_force(struct drm_connector *connector)
 
 static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
-	struct edid *edid;
-
-	edid = to_intel_connector(connector)->detect_edid;
-	if (edid == NULL)
-		return 0;
-
-	return intel_connector_update_modes(connector, edid);
+	/* drm_edid_connector_update() done in ->detect() or ->force() */
+	return drm_edid_connector_add_modes(connector);
 }
 
 static struct i2c_adapter *
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index aecec992cd0d..6a98787edf48 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -479,8 +479,11 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
 	/* use cached edid if we have one */
-	if (!IS_ERR_OR_NULL(intel_connector->edid))
-		return drm_add_edid_modes(connector, intel_connector->edid);
+	if (!IS_ERR_OR_NULL(intel_connector->edid)) {
+		drm_edid_connector_update(connector, intel_connector->edid);
+
+		return drm_edid_connector_add_modes(connector);
+	}
 
 	return intel_panel_get_modes(intel_connector);
 }
@@ -834,7 +837,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 	i915_reg_t lvds_reg;
 	u32 lvds;
 	u8 pin;
@@ -945,27 +948,36 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	 * preferred mode is the right one.
 	 */
 	mutex_lock(&dev_priv->drm.mode_config.mutex);
-	if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)
+	if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC) {
+		const struct edid *edid;
+
+		/* FIXME: Make drm_get_edid_switcheroo() return drm_edid */
 		edid = drm_get_edid_switcheroo(connector,
-				    intel_gmbus_get_adapter(dev_priv, pin));
-	else
-		edid = drm_get_edid(connector,
-				    intel_gmbus_get_adapter(dev_priv, pin));
-	if (edid) {
-		if (drm_add_edid_modes(connector, edid)) {
-			drm_connector_update_edid_property(connector,
-								edid);
-		} else {
+					       intel_gmbus_get_adapter(dev_priv, pin));
+		if (edid) {
+			drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
 			kfree(edid);
-			edid = ERR_PTR(-EINVAL);
+		} else {
+			drm_edid = NULL;
+		}
+	} else {
+		drm_edid = drm_edid_read_ddc(connector,
+					     intel_gmbus_get_adapter(dev_priv, pin));
+	}
+	if (drm_edid) {
+		if (drm_edid_connector_update(connector, drm_edid) ||
+		    !drm_edid_connector_add_modes(connector)) {
+			drm_edid_connector_update(connector, NULL);
+			drm_edid_free(drm_edid);
+			drm_edid = ERR_PTR(-EINVAL);
 		}
 	} else {
-		edid = ERR_PTR(-ENOENT);
+		drm_edid = ERR_PTR(-ENOENT);
 	}
-	intel_connector->edid = edid;
+	intel_connector->edid = drm_edid;
 
 	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, NULL,
-				   IS_ERR(edid) ? NULL : edid);
+				   IS_ERR_OR_NULL(drm_edid) ? NULL : drm_edid_raw(drm_edid));
 
 	/* Try EDID first */
 	intel_panel_add_edid_fixed_modes(intel_connector, true);
-- 
2.34.1

[Intel-gfx] [PATCH v7 20/22] drm/i915/bios: convert intel_bios_init_panel() to drm_edid

[Jani Nikula]

Try to use struct drm_edid where possible, even if having to fall back
to looking into struct edid down low via drm_edid_raw().

v2: Rebase

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 23 ++++++++++++-----------
 drivers/gpu/drm/i915/display/intel_bios.h |  4 ++--
 drivers/gpu/drm/i915/display/intel_dp.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_lvds.c |  2 +-
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 55544d484318..9badd77d656a 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -620,14 +620,14 @@ static void dump_pnp_id(struct drm_i915_private *i915,
 
 static int opregion_get_panel_type(struct drm_i915_private *i915,
 				   const struct intel_bios_encoder_data *devdata,
-				   const struct edid *edid, bool use_fallback)
+				   const struct drm_edid *drm_edid, bool use_fallback)
 {
 	return intel_opregion_get_panel_type(i915);
 }
 
 static int vbt_get_panel_type(struct drm_i915_private *i915,
 			      const struct intel_bios_encoder_data *devdata,
-			      const struct edid *edid, bool use_fallback)
+			      const struct drm_edid *drm_edid, bool use_fallback)
 {
 	const struct bdb_lvds_options *lvds_options;
 
@@ -652,12 +652,13 @@ static int vbt_get_panel_type(struct drm_i915_private *i915,
 
 static int pnpid_get_panel_type(struct drm_i915_private *i915,
 				const struct intel_bios_encoder_data *devdata,
-				const struct edid *edid, bool use_fallback)
+				const struct drm_edid *drm_edid, bool use_fallback)
 {
 	const struct bdb_lvds_lfp_data *data;
 	const struct bdb_lvds_lfp_data_ptrs *ptrs;
 	const struct lvds_pnp_id *edid_id;
 	struct lvds_pnp_id edid_id_nodate;
+	const struct edid *edid = drm_edid_raw(drm_edid); /* FIXME */
 	int i, best = -1;
 
 	if (!edid)
@@ -701,7 +702,7 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
 
 static int fallback_get_panel_type(struct drm_i915_private *i915,
 				   const struct intel_bios_encoder_data *devdata,
-				   const struct edid *edid, bool use_fallback)
+				   const struct drm_edid *drm_edid, bool use_fallback)
 {
 	return use_fallback ? 0 : -1;
 }
@@ -715,13 +716,13 @@ enum panel_type {
 
 static int get_panel_type(struct drm_i915_private *i915,
 			  const struct intel_bios_encoder_data *devdata,
-			  const struct edid *edid, bool use_fallback)
+			  const struct drm_edid *drm_edid, bool use_fallback)
 {
 	struct {
 		const char *name;
 		int (*get_panel_type)(struct drm_i915_private *i915,
 				      const struct intel_bios_encoder_data *devdata,
-				      const struct edid *edid, bool use_fallback);
+				      const struct drm_edid *drm_edid, bool use_fallback);
 		int panel_type;
 	} panel_types[] = {
 		[PANEL_TYPE_OPREGION] = {
@@ -745,7 +746,7 @@ static int get_panel_type(struct drm_i915_private *i915,
 
 	for (i = 0; i < ARRAY_SIZE(panel_types); i++) {
 		panel_types[i].panel_type = panel_types[i].get_panel_type(i915, devdata,
-									  edid, use_fallback);
+									  drm_edid, use_fallback);
 
 		drm_WARN_ON(&i915->drm, panel_types[i].panel_type > 0xf &&
 			    panel_types[i].panel_type != 0xff);
@@ -3187,7 +3188,7 @@ void intel_bios_init(struct drm_i915_private *i915)
 static void intel_bios_init_panel(struct drm_i915_private *i915,
 				  struct intel_panel *panel,
 				  const struct intel_bios_encoder_data *devdata,
-				  const struct edid *edid,
+				  const struct drm_edid *drm_edid,
 				  bool use_fallback)
 {
 	/* already have it? */
@@ -3197,7 +3198,7 @@ static void intel_bios_init_panel(struct drm_i915_private *i915,
 	}
 
 	panel->vbt.panel_type = get_panel_type(i915, devdata,
-					       edid, use_fallback);
+					       drm_edid, use_fallback);
 	if (panel->vbt.panel_type < 0) {
 		drm_WARN_ON(&i915->drm, use_fallback);
 		return;
@@ -3228,9 +3229,9 @@ void intel_bios_init_panel_early(struct drm_i915_private *i915,
 void intel_bios_init_panel_late(struct drm_i915_private *i915,
 				struct intel_panel *panel,
 				const struct intel_bios_encoder_data *devdata,
-				const struct edid *edid)
+				const struct drm_edid *drm_edid)
 {
-	intel_bios_init_panel(i915, panel, devdata, edid, true);
+	intel_bios_init_panel(i915, panel, devdata, drm_edid, true);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
index ff1fdd2e0c1c..d221f784aa88 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -32,8 +32,8 @@
 
 #include <linux/types.h>
 
+struct drm_edid;
 struct drm_i915_private;
-struct edid;
 struct intel_bios_encoder_data;
 struct intel_crtc_state;
 struct intel_encoder;
@@ -238,7 +238,7 @@ void intel_bios_init_panel_early(struct drm_i915_private *dev_priv,
 void intel_bios_init_panel_late(struct drm_i915_private *dev_priv,
 				struct intel_panel *panel,
 				const struct intel_bios_encoder_data *devdata,
-				const struct edid *edid);
+				const struct drm_edid *drm_edid);
 void intel_bios_fini_panel(struct intel_panel *panel);
 void intel_bios_driver_remove(struct drm_i915_private *dev_priv);
 bool intel_bios_is_valid_vbt(const void *buf, size_t size);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 6caa290ef6e6..a64e97808813 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5325,7 +5325,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	intel_connector->edid = drm_edid;
 
 	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, encoder->devdata,
-				   IS_ERR_OR_NULL(drm_edid) ? NULL : drm_edid_raw(drm_edid));
+				   IS_ERR(drm_edid) ? NULL : drm_edid);
 
 	intel_panel_add_edid_fixed_modes(intel_connector, true);
 
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index 6a98787edf48..9f6910bba2e9 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -977,7 +977,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	intel_connector->edid = drm_edid;
 
 	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, NULL,
-				   IS_ERR_OR_NULL(drm_edid) ? NULL : drm_edid_raw(drm_edid));
+				   IS_ERR(drm_edid) ? NULL : drm_edid);
 
 	/* Try EDID first */
 	intel_panel_add_edid_fixed_modes(intel_connector, true);
-- 
2.34.1

[Intel-gfx] [PATCH v7 21/22] drm/i915/opregion: convert intel_opregion_get_edid() to struct drm_edid

[Jani Nikula]

Simplify validation and use by converting to drm_edid.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c       | 10 ++-----
 drivers/gpu/drm/i915/display/intel_opregion.c | 29 +++++++------------
 drivers/gpu/drm/i915/display/intel_opregion.h |  4 +--
 3 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a64e97808813..67f2cb048ac1 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5299,18 +5299,12 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	mutex_lock(&dev_priv->drm.mode_config.mutex);
 	drm_edid = drm_edid_read_ddc(connector, &intel_dp->aux.ddc);
 	if (!drm_edid) {
-		const struct edid *edid;
-
 		/* Fallback to EDID from ACPI OpRegion, if any */
-		/* FIXME: Make intel_opregion_get_edid() return drm_edid */
-		edid = intel_opregion_get_edid(intel_connector);
-		if (edid) {
-			drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
+		drm_edid = intel_opregion_get_edid(intel_connector);
+		if (drm_edid)
 			drm_dbg_kms(&dev_priv->drm,
 				    "[CONNECTOR:%d:%s] Using OpRegion EDID\n",
 				    connector->base.id, connector->name);
-			kfree(edid);
-		}
 	}
 	if (drm_edid) {
 		if (drm_edid_connector_update(connector, drm_edid) ||
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index e0184745632c..b8dce0576512 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -1101,41 +1101,34 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv)
  * The EDID in the OpRegion, or NULL if there is none or it's invalid.
  *
  */
-struct edid *intel_opregion_get_edid(struct intel_connector *intel_connector)
+const struct drm_edid *intel_opregion_get_edid(struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct drm_i915_private *i915 = to_i915(connector->dev);
 	struct intel_opregion *opregion = &i915->display.opregion;
-	const void *in_edid;
-	const struct edid *edid;
-	struct edid *new_edid;
+	const struct drm_edid *drm_edid;
+	const void *edid;
 	int len;
 
 	if (!opregion->asle_ext)
 		return NULL;
 
-	in_edid = opregion->asle_ext->bddc;
+	edid = opregion->asle_ext->bddc;
 
 	/* Validity corresponds to number of 128-byte blocks */
 	len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128;
-	if (!len || !memchr_inv(in_edid, 0, len))
+	if (!len || !memchr_inv(edid, 0, len))
 		return NULL;
 
-	edid = in_edid;
+	drm_edid = drm_edid_alloc(edid, len);
 
-	if (len < EDID_LENGTH * (1 + edid->extensions)) {
-		drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5): too short\n");
-		return NULL;
-	}
-	new_edid = drm_edid_duplicate(edid);
-	if (!new_edid)
-		return NULL;
-	if (!drm_edid_is_valid(new_edid)) {
-		kfree(new_edid);
+	if (!drm_edid_valid(drm_edid)) {
 		drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n");
-		return NULL;
+		drm_edid_free(drm_edid);
+		drm_edid = NULL;
 	}
-	return new_edid;
+
+	return drm_edid;
 }
 
 bool intel_opregion_headless_sku(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
index 2f261f985400..d02e6696a050 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.h
+++ b/drivers/gpu/drm/i915/display/intel_opregion.h
@@ -74,7 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
 				  pci_power_t state);
 int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv);
-struct edid *intel_opregion_get_edid(struct intel_connector *connector);
+const struct drm_edid *intel_opregion_get_edid(struct intel_connector *connector);
 
 bool intel_opregion_headless_sku(struct drm_i915_private *i915);
 
@@ -123,7 +123,7 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev)
 	return -ENODEV;
 }
 
-static inline struct edid *
+static inline const struct drm_edid *
 intel_opregion_get_edid(struct intel_connector *connector)
 {
 	return NULL;
-- 
2.34.1

[Intel-gfx] [PATCH v7 22/22] drm/i915/panel: move panel fixed EDID to struct intel_panel

[Jani Nikula]

It's a bit confusing to have two cached EDIDs in struct intel_connector
with slightly different purposes. Make the distinction a bit clearer by
moving the EDID cached for eDP and LVDS panels at connector init time to
struct intel_panel, and name it fixed_edid. That's what it is, a fixed
EDID for the panels.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/icl_dsi.c        |  2 +-
 .../gpu/drm/i915/display/intel_connector.c    |  3 ---
 .../drm/i915/display/intel_display_types.h    |  6 ++++--
 drivers/gpu/drm/i915/display/intel_dp.c       | 20 +++++++++----------
 drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
 drivers/gpu/drm/i915/display/intel_lvds.c     | 11 +++++-----
 drivers/gpu/drm/i915/display/intel_panel.c    | 10 +++++++++-
 drivers/gpu/drm/i915/display/intel_panel.h    |  4 +++-
 drivers/gpu/drm/i915/display/intel_sdvo.c     |  2 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c        |  2 +-
 10 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index ae14c794c4bc..d56d01f07bb7 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -2054,7 +2054,7 @@ void icl_dsi_init(struct drm_i915_private *dev_priv)
 		goto err;
 	}
 
-	intel_panel_init(intel_connector);
+	intel_panel_init(intel_connector, NULL);
 
 	intel_backlight_setup(intel_connector, INVALID_PIPE);
 
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 4814d4e2f7f9..257afac34839 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -99,9 +99,6 @@ void intel_connector_destroy(struct drm_connector *connector)
 
 	intel_hdcp_cleanup(intel_connector);
 
-	if (!IS_ERR_OR_NULL(intel_connector->edid))
-		drm_edid_free(intel_connector->edid);
-
 	intel_panel_fini(intel_connector);
 
 	drm_connector_cleanup(connector);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 34dc850340b8..6feb232bb1c2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -351,6 +351,9 @@ struct intel_vbt_panel_data {
 };
 
 struct intel_panel {
+	/* Fixed EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
+	const struct drm_edid *fixed_edid;
+
 	struct list_head fixed_modes;
 
 	/* backlight */
@@ -591,8 +594,7 @@ struct intel_connector {
 	/* Panel info for eDP and LVDS */
 	struct intel_panel panel;
 
-	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
-	const struct drm_edid *edid;
+	/* Cached EDID for detect. */
 	const struct drm_edid *detect_edid;
 
 	/* Number of times hotplug detection was tried after an HPD interrupt */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 67f2cb048ac1..3f4396b5f029 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4480,18 +4480,19 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
 static const struct drm_edid *
 intel_dp_get_edid(struct intel_dp *intel_dp)
 {
-	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct intel_connector *connector = intel_dp->attached_connector;
+	const struct drm_edid *fixed_edid = connector->panel.fixed_edid;
 
-	/* use cached edid if we have one */
-	if (intel_connector->edid) {
+	/* Use panel fixed edid if we have one */
+	if (fixed_edid) {
 		/* invalid edid */
-		if (IS_ERR(intel_connector->edid))
+		if (IS_ERR(fixed_edid))
 			return NULL;
 
-		return drm_edid_dup(intel_connector->edid);
-	} else
-		return drm_edid_read_ddc(&intel_connector->base,
-					 &intel_dp->aux.ddc);
+		return drm_edid_dup(fixed_edid);
+	}
+
+	return drm_edid_read_ddc(&connector->base, &intel_dp->aux.ddc);
 }
 
 static void
@@ -5316,7 +5317,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	} else {
 		drm_edid = ERR_PTR(-ENOENT);
 	}
-	intel_connector->edid = drm_edid;
 
 	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, encoder->devdata,
 				   IS_ERR(drm_edid) ? NULL : drm_edid);
@@ -5343,7 +5343,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		goto out_vdd_off;
 	}
 
-	intel_panel_init(intel_connector);
+	intel_panel_init(intel_connector, drm_edid);
 
 	intel_edp_backlight_setup(intel_dp, intel_connector);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
index 4aeae0f3ac91..0be8105cb18a 100644
--- a/drivers/gpu/drm/i915/display/intel_dvo.c
+++ b/drivers/gpu/drm/i915/display/intel_dvo.c
@@ -554,6 +554,6 @@ void intel_dvo_init(struct drm_i915_private *i915)
 		 */
 		intel_panel_add_encoder_fixed_mode(connector, encoder);
 
-		intel_panel_init(connector);
+		intel_panel_init(connector, NULL);
 	}
 }
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index 9f6910bba2e9..a1557d84ce0a 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -477,10 +477,11 @@ static int intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 static int intel_lvds_get_modes(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
+	const struct drm_edid *fixed_edid = intel_connector->panel.fixed_edid;
 
-	/* use cached edid if we have one */
-	if (!IS_ERR_OR_NULL(intel_connector->edid)) {
-		drm_edid_connector_update(connector, intel_connector->edid);
+	/* Use panel fixed edid if we have one */
+	if (!IS_ERR_OR_NULL(fixed_edid)) {
+		drm_edid_connector_update(connector, fixed_edid);
 
 		return drm_edid_connector_add_modes(connector);
 	}
@@ -974,8 +975,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	} else {
 		drm_edid = ERR_PTR(-ENOENT);
 	}
-	intel_connector->edid = drm_edid;
-
 	intel_bios_init_panel_late(dev_priv, &intel_connector->panel, NULL,
 				   IS_ERR(drm_edid) ? NULL : drm_edid);
 
@@ -1000,7 +999,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	if (!intel_panel_preferred_fixed_mode(intel_connector))
 		goto failed;
 
-	intel_panel_init(intel_connector);
+	intel_panel_init(intel_connector, drm_edid);
 
 	intel_backlight_setup(intel_connector, INVALID_PIPE);
 
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 3b1004b019a8..42aa04bac261 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -31,6 +31,8 @@
 #include <linux/kernel.h>
 #include <linux/pwm.h>
 
+#include <drm/drm_edid.h>
+
 #include "i915_reg.h"
 #include "intel_backlight.h"
 #include "intel_connector.h"
@@ -670,10 +672,13 @@ void intel_panel_init_alloc(struct intel_connector *connector)
 	INIT_LIST_HEAD(&panel->fixed_modes);
 }
 
-int intel_panel_init(struct intel_connector *connector)
+int intel_panel_init(struct intel_connector *connector,
+		     const struct drm_edid *fixed_edid)
 {
 	struct intel_panel *panel = &connector->panel;
 
+	panel->fixed_edid = fixed_edid;
+
 	intel_backlight_init_funcs(panel);
 
 	if (!has_drrs_modes(connector))
@@ -692,6 +697,9 @@ void intel_panel_fini(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	struct drm_display_mode *fixed_mode, *next;
 
+	if (!IS_ERR_OR_NULL(panel->fixed_edid))
+		drm_edid_free(panel->fixed_edid);
+
 	intel_backlight_destroy(panel);
 
 	intel_bios_fini_panel(panel);
diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
index 4b51e1c51da6..15a8c897b33f 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.h
+++ b/drivers/gpu/drm/i915/display/intel_panel.h
@@ -13,13 +13,15 @@ enum drrs_type;
 struct drm_connector;
 struct drm_connector_state;
 struct drm_display_mode;
+struct drm_edid;
 struct drm_i915_private;
 struct intel_connector;
 struct intel_crtc_state;
 struct intel_encoder;
 
 void intel_panel_init_alloc(struct intel_connector *connector);
-int intel_panel_init(struct intel_connector *connector);
+int intel_panel_init(struct intel_connector *connector,
+		     const struct drm_edid *fixed_edid);
 void intel_panel_fini(struct intel_connector *connector);
 enum drm_connector_status
 intel_panel_detect(struct drm_connector *connector, bool force);
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 21805c15d5eb..c58e5cfa8e88 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2903,7 +2903,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, u16 type)
 		mutex_unlock(&i915->drm.mode_config.mutex);
 	}
 
-	intel_panel_init(intel_connector);
+	intel_panel_init(intel_connector, NULL);
 
 	if (!intel_panel_preferred_fixed_mode(intel_connector))
 		goto err;
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
index 662bdb656aa3..2289f6b1b4eb 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1983,7 +1983,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
 		goto err_cleanup_connector;
 	}
 
-	intel_panel_init(intel_connector);
+	intel_panel_init(intel_connector, NULL);
 
 	intel_backlight_setup(intel_connector, INVALID_PIPE);
 
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/edid: info & modes parsing and drm_edid refactors

[Patchwork]

== Series Details ==

Series: drm/edid: info & modes parsing and drm_edid refactors
URL   : https://patchwork.freedesktop.org/series/112392/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/edid: info & modes parsing and drm_edid refactors

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 10387 bytes --]

== Series Details ==

Series: drm/edid: info & modes parsing and drm_edid refactors
URL   : https://patchwork.freedesktop.org/series/112392/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12541 -> Patchwork_112392v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_112392v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_112392v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/index.html

Participating hosts (42 -> 42)
------------------------------

  Additional (1): fi-rkl-11600 
  Missing    (1): bat-dg2-oem1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_112392v1:

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-icl-u2:          [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/fi-icl-u2/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-icl-u2/igt@debugfs_test@read_all_entries.html

  
Known issues
------------

  Here are the changes found in Patchwork_112392v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-rkl-11600:       NOTRUN -> [SKIP][3] ([i915#7456])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@debugfs_test@basic-hwmon.html

  * igt@gem_huc_copy@huc-copy:
    - fi-rkl-11600:       NOTRUN -> [SKIP][4] ([i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][5] ([i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@gem_lmem_swapping@basic.html

  * igt@gem_tiled_pread_basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][6] ([i915#3282])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-rkl-11600:       NOTRUN -> [SKIP][7] ([i915#7561])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_selftest@live@migrate:
    - bat-adlp-4:         [PASS][8] -> [DMESG-FAIL][9] ([i915#7699])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/bat-adlp-4/igt@i915_selftest@live@migrate.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-adlp-4/igt@i915_selftest@live@migrate.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][10] ([i915#4817])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - bat-dg1-6:          NOTRUN -> [SKIP][11] ([fdo#111827])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-dg1-6/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-rkl-11600:       NOTRUN -> [SKIP][12] ([fdo#111827]) +7 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@kms_chamelium@hdmi-edid-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-rkl-11600:       NOTRUN -> [SKIP][13] ([i915#4103])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-rkl-11600:       NOTRUN -> [SKIP][14] ([fdo#109285] / [i915#4098])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@primary_page_flip:
    - fi-rkl-11600:       NOTRUN -> [SKIP][15] ([i915#1072]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@kms_psr@primary_page_flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-rkl-11600:       NOTRUN -> [SKIP][16] ([i915#3555] / [i915#4098])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-read:
    - fi-rkl-11600:       NOTRUN -> [SKIP][17] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - fi-rkl-11600:       NOTRUN -> [SKIP][18] ([fdo#109295] / [i915#3301] / [i915#3708])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-rkl-11600/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-icl-u2:          NOTRUN -> [FAIL][19] ([i915#4312])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/fi-icl-u2/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-6:          [INCOMPLETE][20] -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/bat-dg1-6/igt@i915_selftest@live@hangcheck.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-dg1-6/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@migrate:
    - {bat-atsm-1}:       [DMESG-FAIL][22] ([i915#7699]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-atsm-1/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@slpc:
    - bat-adlp-4:         [DMESG-FAIL][24] ([i915#6367]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/bat-adlp-4/igt@i915_selftest@live@slpc.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-adlp-4/igt@i915_selftest@live@slpc.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1:
    - {bat-adlp-9}:       [DMESG-WARN][26] ([i915#2867]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12541/bat-adlp-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/bat-adlp-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#6121]: https://gitlab.freedesktop.org/drm/intel/issues/6121
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#7336]: https://gitlab.freedesktop.org/drm/intel/issues/7336
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699


Build changes
-------------

  * Linux: CI_DRM_12541 -> Patchwork_112392v1

  CI-20190529: 20190529
  CI_DRM_12541: b832866fa6063614b3637598aca19aee3bc3039f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7106: 8cce332bdc50d2b20d553d7a0221737f4399d031 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112392v1: b832866fa6063614b3637598aca19aee3bc3039f @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

ac4bdeb3f106 drm/i915/panel: move panel fixed EDID to struct intel_panel
10ecf1164578 drm/i915/opregion: convert intel_opregion_get_edid() to struct drm_edid
b84a844e65eb drm/i915/bios: convert intel_bios_init_panel() to drm_edid
b3f2cde47d19 drm/i915/edid: convert DP, HDMI and LVDS to drm_edid
84a61d7e61d3 drm/edid: remove redundant _drm_connector_update_edid_property()
6ddd809e7a8f drm/edid: add separate drm_edid_connector_add_modes()
259138be9dca drm/edid: refactor _drm_edid_connector_update() and rename
e52f1d95bd97 drm/edid: move EDID BPC quirk application to update_display_info()
9cb8092ab481 drm/edid: merge ELD handling to update_display_info()
2c32dee70c6c drm/edid: stop passing quirks around
66b4f7f8be61 drm/edid: store quirks in display info
cda5b75852ce drm/edid: split HDMI VSDB info and mode parsing
4730e20a625e drm/edid: add helper for HDMI VSDB audio latency field length
f59471cabdf4 drm/edid: fix and clarify HDMI VSDB audio latency parsing
e121e84dc41e drm/edid: split CTA Y420VDB info and mode parsing
ed4b1decf57d drm/edid: refactor CTA Y420CMDB parsing
17c49a4797c6 drm/edid: rename struct drm_display_info *display to *info
a63af6eea43b drm/edid: use VIC in AVI infoframe if sink lists it in CTA VDB
6dd820256090 drm/edid: Use the pre-parsed VICs
d814b7b51a76 drm/edid: parse VICs from CTA VDB early
cbe30b22621a drm/edid: fix parsing of 3D modes from HDMI VSDB
012e05cb0531 drm/edid: fix AVI infoframe aspect ratio handling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112392v1/index.html

[-- Attachment #2: Type: text/html, Size: 11875 bytes --]

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/edid: info & modes parsing and drm_edid refactors (rev2)

[Patchwork]

== Series Details ==

Series: drm/edid: info & modes parsing and drm_edid refactors (rev2)
URL   : https://patchwork.freedesktop.org/series/112392/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/112392/revisions/2/mbox/ not applied
Applying: drm/edid: fix AVI infoframe aspect ratio handling
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/drm_edid.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/drm_edid.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_edid.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/edid: fix AVI infoframe aspect ratio handling
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".