[PATCH 0/4] Fix HFVSDB parsing

[PATCH 0/4] Fix HFVSDB parsing

[Ankit Nautiyal]

Fix issues in HFVSDB parsing for DSC support.
Also minor refactoring in Logging.

Split from original patch into a new series.
https://patchwork.freedesktop.org/patch/495193/

Ankit Nautiyal (4):
  drm/edid: Fix minimum bpc supported with DSC1.2 for HDMI sink
  drm/edid: Split DSC parsing into separate function
  drm/edid: Refactor HFVSDB parsing for DSC1.2
  drm/edid: Avoid multiple log lines for HFVSDB parsing

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

-- 
2.25.1

[PATCH 1/4] drm/edid: Fix minimum bpc supported with DSC1.2 for HDMI sink

[Ankit Nautiyal]

HF-VSDB/SCDB has bits to advertise support for 16, 12 and 10 bpc.
If none of the bits are set, the minimum bpc supported with DSC is 8.

This patch corrects the min bpc supported to be 8, instead of 0.

Fixes: 76ee7b905678 ("drm/edid: Parse DSC1.2 cap fields from HFVSDB block")
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bbc25e3b7220..cdf10279e1bd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5770,7 +5770,8 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 			else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
 				hdmi_dsc->bpc_supported = 10;
 			else
-				hdmi_dsc->bpc_supported = 0;
+				/* Supports min 8 BPC if DSC1.2 is supported*/
+				hdmi_dsc->bpc_supported = 8;
 
 			dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
 			drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
-- 
2.25.1

[PATCH 2/4] drm/edid: Split DSC parsing into separate function

[Ankit Nautiyal]

Move the DSC parsing logic into separate function.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 128 ++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index cdf10279e1bd..ffff1d08b3a4 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5703,6 +5703,73 @@ static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
 	hdmi->y420_dc_modes = dc_mask;
 }
 
+static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
+			       const u8 *hf_scds)
+{
+	u8 dsc_max_slices;
+	u8 dsc_max_frl_rate;
+
+	hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
+
+	if (!hdmi_dsc->v_1p2)
+		return;
+
+	hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
+	hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
+
+	if (hf_scds[11] & DRM_EDID_DSC_16BPC)
+		hdmi_dsc->bpc_supported = 16;
+	else if (hf_scds[11] & DRM_EDID_DSC_12BPC)
+		hdmi_dsc->bpc_supported = 12;
+	else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
+		hdmi_dsc->bpc_supported = 10;
+	else
+		/* Supports min 8 BPC if DSC1.2 is supported*/
+		hdmi_dsc->bpc_supported = 8;
+
+	dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
+	drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
+			     &hdmi_dsc->max_frl_rate_per_lane);
+	hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
+
+	dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
+
+	switch (dsc_max_slices) {
+	case 1:
+		hdmi_dsc->max_slices = 1;
+		hdmi_dsc->clk_per_slice = 340;
+		break;
+	case 2:
+		hdmi_dsc->max_slices = 2;
+		hdmi_dsc->clk_per_slice = 340;
+		break;
+	case 3:
+		hdmi_dsc->max_slices = 4;
+		hdmi_dsc->clk_per_slice = 340;
+		break;
+	case 4:
+		hdmi_dsc->max_slices = 8;
+		hdmi_dsc->clk_per_slice = 340;
+		break;
+	case 5:
+		hdmi_dsc->max_slices = 8;
+		hdmi_dsc->clk_per_slice = 400;
+		break;
+	case 6:
+		hdmi_dsc->max_slices = 12;
+		hdmi_dsc->clk_per_slice = 400;
+		break;
+	case 7:
+		hdmi_dsc->max_slices = 16;
+		hdmi_dsc->clk_per_slice = 400;
+		break;
+	case 0:
+	default:
+		hdmi_dsc->max_slices = 0;
+		hdmi_dsc->clk_per_slice = 0;
+	}
+}
+
 /* Sink Capability Data Structure */
 static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 				      const u8 *hf_scds)
@@ -5749,71 +5816,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 
 	if (hf_scds[7]) {
 		u8 max_frl_rate;
-		u8 dsc_max_frl_rate;
-		u8 dsc_max_slices;
 		struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
 
 		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
 		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
 				     &hdmi->max_frl_rate_per_lane);
-		hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
-
-		if (hdmi_dsc->v_1p2) {
-			hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
-			hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
-
-			if (hf_scds[11] & DRM_EDID_DSC_16BPC)
-				hdmi_dsc->bpc_supported = 16;
-			else if (hf_scds[11] & DRM_EDID_DSC_12BPC)
-				hdmi_dsc->bpc_supported = 12;
-			else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
-				hdmi_dsc->bpc_supported = 10;
-			else
-				/* Supports min 8 BPC if DSC1.2 is supported*/
-				hdmi_dsc->bpc_supported = 8;
-
-			dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
-			drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
-					     &hdmi_dsc->max_frl_rate_per_lane);
-			hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
-
-			dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
-			switch (dsc_max_slices) {
-			case 1:
-				hdmi_dsc->max_slices = 1;
-				hdmi_dsc->clk_per_slice = 340;
-				break;
-			case 2:
-				hdmi_dsc->max_slices = 2;
-				hdmi_dsc->clk_per_slice = 340;
-				break;
-			case 3:
-				hdmi_dsc->max_slices = 4;
-				hdmi_dsc->clk_per_slice = 340;
-				break;
-			case 4:
-				hdmi_dsc->max_slices = 8;
-				hdmi_dsc->clk_per_slice = 340;
-				break;
-			case 5:
-				hdmi_dsc->max_slices = 8;
-				hdmi_dsc->clk_per_slice = 400;
-				break;
-			case 6:
-				hdmi_dsc->max_slices = 12;
-				hdmi_dsc->clk_per_slice = 400;
-				break;
-			case 7:
-				hdmi_dsc->max_slices = 16;
-				hdmi_dsc->clk_per_slice = 400;
-				break;
-			case 0:
-			default:
-				hdmi_dsc->max_slices = 0;
-				hdmi_dsc->clk_per_slice = 0;
-			}
-		}
+
+		drm_parse_dsc_info(hdmi_dsc, hf_scds);
 	}
 
 	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
-- 
2.25.1

[PATCH 3/4] drm/edid: Refactor HFVSDB parsing for DSC1.2

[Ankit Nautiyal]

DSC capabilities are given in bytes 11-13 of VSDB (i.e. bytes 8-10 of
SCDS). Since minimum length of Data block is 7, all bytes greater than 7
must be read only after checking the length of the data block.

This patch adds check for data block length before reading relavant DSC
bytes.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 93 ++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ffff1d08b3a4..c9c3a9c8fa26 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5706,9 +5706,6 @@ static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
 static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
 			       const u8 *hf_scds)
 {
-	u8 dsc_max_slices;
-	u8 dsc_max_frl_rate;
-
 	hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
 
 	if (!hdmi_dsc->v_1p2)
@@ -5727,47 +5724,54 @@ static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
 		/* Supports min 8 BPC if DSC1.2 is supported*/
 		hdmi_dsc->bpc_supported = 8;
 
-	dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
-	drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
-			     &hdmi_dsc->max_frl_rate_per_lane);
-	hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
+	if (cea_db_payload_len(hf_scds) >= 12 && hf_scds[12]) {
+		u8 dsc_max_slices;
+		u8 dsc_max_frl_rate;
 
-	dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
+		dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
+		drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
+				     &hdmi_dsc->max_frl_rate_per_lane);
 
-	switch (dsc_max_slices) {
-	case 1:
-		hdmi_dsc->max_slices = 1;
-		hdmi_dsc->clk_per_slice = 340;
-		break;
-	case 2:
-		hdmi_dsc->max_slices = 2;
-		hdmi_dsc->clk_per_slice = 340;
-		break;
-	case 3:
-		hdmi_dsc->max_slices = 4;
-		hdmi_dsc->clk_per_slice = 340;
-		break;
-	case 4:
-		hdmi_dsc->max_slices = 8;
-		hdmi_dsc->clk_per_slice = 340;
-		break;
-	case 5:
-		hdmi_dsc->max_slices = 8;
-		hdmi_dsc->clk_per_slice = 400;
-		break;
-	case 6:
-		hdmi_dsc->max_slices = 12;
-		hdmi_dsc->clk_per_slice = 400;
-		break;
-	case 7:
-		hdmi_dsc->max_slices = 16;
-		hdmi_dsc->clk_per_slice = 400;
-		break;
-	case 0:
-	default:
-		hdmi_dsc->max_slices = 0;
-		hdmi_dsc->clk_per_slice = 0;
+		dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
+
+		switch (dsc_max_slices) {
+		case 1:
+			hdmi_dsc->max_slices = 1;
+			hdmi_dsc->clk_per_slice = 340;
+			break;
+		case 2:
+			hdmi_dsc->max_slices = 2;
+			hdmi_dsc->clk_per_slice = 340;
+			break;
+		case 3:
+			hdmi_dsc->max_slices = 4;
+			hdmi_dsc->clk_per_slice = 340;
+			break;
+		case 4:
+			hdmi_dsc->max_slices = 8;
+			hdmi_dsc->clk_per_slice = 340;
+			break;
+		case 5:
+			hdmi_dsc->max_slices = 8;
+			hdmi_dsc->clk_per_slice = 400;
+			break;
+		case 6:
+			hdmi_dsc->max_slices = 12;
+			hdmi_dsc->clk_per_slice = 400;
+			break;
+		case 7:
+			hdmi_dsc->max_slices = 16;
+			hdmi_dsc->clk_per_slice = 400;
+			break;
+		case 0:
+		default:
+			hdmi_dsc->max_slices = 0;
+			hdmi_dsc->clk_per_slice = 0;
+		}
 	}
+
+	if (cea_db_payload_len(hf_scds) >= 13 && hf_scds[13])
+		hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
 }
 
 /* Sink Capability Data Structure */
@@ -5776,6 +5780,7 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 {
 	struct drm_display_info *display = &connector->display_info;
 	struct drm_hdmi_info *hdmi = &display->hdmi;
+	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
 
 	display->has_hdmi_infoframe = true;
 
@@ -5816,17 +5821,17 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 
 	if (hf_scds[7]) {
 		u8 max_frl_rate;
-		struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
 
 		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
 		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
 				     &hdmi->max_frl_rate_per_lane);
-
-		drm_parse_dsc_info(hdmi_dsc, hf_scds);
 	}
 
 	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
+
+	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
+		drm_parse_dsc_info(hdmi_dsc, hf_scds);
 }
 
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
-- 
2.25.1

[PATCH 4/4] drm/edid: Avoid multiple log lines for HFVSDB parsing

[Ankit Nautiyal]

Replace multiple log lines with a single log line at the end of
parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
add log for DSC1.2 support.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c9c3a9c8fa26..7a319d570297 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5781,6 +5781,9 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	struct drm_display_info *display = &connector->display_info;
 	struct drm_hdmi_info *hdmi = &display->hdmi;
 	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
+	u32 max_tmds_clock = 0;
+	u8 max_frl_rate = 0;
+	bool dsc_support = false;
 
 	display->has_hdmi_infoframe = true;
 
@@ -5800,14 +5803,13 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	 */
 
 	if (hf_scds[5]) {
-		/* max clock is 5000 KHz times block value */
-		u32 max_tmds_clock = hf_scds[5] * 5000;
 		struct drm_scdc *scdc = &hdmi->scdc;
 
+		/* max clock is 5000 KHz times block value */
+		max_tmds_clock = hf_scds[5] * 5000;
+
 		if (max_tmds_clock > 340000) {
 			display->max_tmds_clock = max_tmds_clock;
-			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
-				display->max_tmds_clock);
 		}
 
 		if (scdc->supported) {
@@ -5820,9 +5822,6 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	}
 
 	if (hf_scds[7]) {
-		u8 max_frl_rate;
-
-		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
 		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
 		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
 				     &hdmi->max_frl_rate_per_lane);
@@ -5830,8 +5829,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 
 	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
 
-	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
+	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
 		drm_parse_dsc_info(hdmi_dsc, hf_scds);
+		dsc_support = true;
+	}
+
+	drm_dbg_kms(connector->dev,
+		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",
+		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");
 }
 
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
-- 
2.25.1

Re: [PATCH 4/4] drm/edid: Avoid multiple log lines for HFVSDB parsing

[Jani Nikula]

On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Replace multiple log lines with a single log line at the end of
> parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
> add log for DSC1.2 support.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c9c3a9c8fa26..7a319d570297 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5781,6 +5781,9 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	struct drm_display_info *display = &connector->display_info;
>  	struct drm_hdmi_info *hdmi = &display->hdmi;
>  	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
> +	u32 max_tmds_clock = 0;

This should be int because display->max_tmds_clock is int. Yes, it's a
change from the current local var, but logging u32 would require %u
instead of %d in the format string anyway, so better just use the right
type.

> +	u8 max_frl_rate = 0;
> +	bool dsc_support = false;
>  
>  	display->has_hdmi_infoframe = true;
>  
> @@ -5800,14 +5803,13 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	 */
>  
>  	if (hf_scds[5]) {
> -		/* max clock is 5000 KHz times block value */
> -		u32 max_tmds_clock = hf_scds[5] * 5000;
>  		struct drm_scdc *scdc = &hdmi->scdc;
>  
> +		/* max clock is 5000 KHz times block value */
> +		max_tmds_clock = hf_scds[5] * 5000;
> +
>  		if (max_tmds_clock > 340000) {
>  			display->max_tmds_clock = max_tmds_clock;
> -			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> -				display->max_tmds_clock);

Hmm, the logic for what is logged gets changed.

>  		}
>  
>  		if (scdc->supported) {
> @@ -5820,9 +5822,6 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  	}
>  
>  	if (hf_scds[7]) {
> -		u8 max_frl_rate;
> -
> -		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>  		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>  		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>  				     &hdmi->max_frl_rate_per_lane);
> @@ -5830,8 +5829,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  
>  	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
>  
> -	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
> +	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
>  		drm_parse_dsc_info(hdmi_dsc, hf_scds);
> +		dsc_support = true;
> +	}
> +
> +	drm_dbg_kms(connector->dev,
> +		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",

Nitpicks, %d needs int instead of u32, "kHz" not "Khz", "HDMI 2.1" and
"DSC 1.2" with spaces, would prefer a space after ":".

> +		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");

See str_yes_no().

BR,
Jani.

>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 2/4] drm/edid: Split DSC parsing into separate function

[Jani Nikula]

On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Move the DSC parsing logic into separate function.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 128 ++++++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index cdf10279e1bd..ffff1d08b3a4 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5703,6 +5703,73 @@ static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
>  	hdmi->y420_dc_modes = dc_mask;
>  }
>  
> +static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
> +			       const u8 *hf_scds)
> +{
> +	u8 dsc_max_slices;
> +	u8 dsc_max_frl_rate;
> +
> +	hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
> +
> +	if (!hdmi_dsc->v_1p2)
> +		return;
> +
> +	hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
> +	hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
> +
> +	if (hf_scds[11] & DRM_EDID_DSC_16BPC)
> +		hdmi_dsc->bpc_supported = 16;
> +	else if (hf_scds[11] & DRM_EDID_DSC_12BPC)
> +		hdmi_dsc->bpc_supported = 12;
> +	else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
> +		hdmi_dsc->bpc_supported = 10;
> +	else
> +		/* Supports min 8 BPC if DSC1.2 is supported*/
> +		hdmi_dsc->bpc_supported = 8;
> +
> +	dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
> +	drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
> +			     &hdmi_dsc->max_frl_rate_per_lane);
> +	hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
> +
> +	dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
> +
> +	switch (dsc_max_slices) {
> +	case 1:
> +		hdmi_dsc->max_slices = 1;
> +		hdmi_dsc->clk_per_slice = 340;
> +		break;
> +	case 2:
> +		hdmi_dsc->max_slices = 2;
> +		hdmi_dsc->clk_per_slice = 340;
> +		break;
> +	case 3:
> +		hdmi_dsc->max_slices = 4;
> +		hdmi_dsc->clk_per_slice = 340;
> +		break;
> +	case 4:
> +		hdmi_dsc->max_slices = 8;
> +		hdmi_dsc->clk_per_slice = 340;
> +		break;
> +	case 5:
> +		hdmi_dsc->max_slices = 8;
> +		hdmi_dsc->clk_per_slice = 400;
> +		break;
> +	case 6:
> +		hdmi_dsc->max_slices = 12;
> +		hdmi_dsc->clk_per_slice = 400;
> +		break;
> +	case 7:
> +		hdmi_dsc->max_slices = 16;
> +		hdmi_dsc->clk_per_slice = 400;
> +		break;
> +	case 0:
> +	default:
> +		hdmi_dsc->max_slices = 0;
> +		hdmi_dsc->clk_per_slice = 0;
> +	}
> +}
> +
>  /* Sink Capability Data Structure */
>  static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  				      const u8 *hf_scds)
> @@ -5749,71 +5816,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  
>  	if (hf_scds[7]) {
>  		u8 max_frl_rate;
> -		u8 dsc_max_frl_rate;
> -		u8 dsc_max_slices;
>  		struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
>  
>  		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>  		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>  		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>  				     &hdmi->max_frl_rate_per_lane);
> -		hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
> -
> -		if (hdmi_dsc->v_1p2) {
> -			hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
> -			hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
> -
> -			if (hf_scds[11] & DRM_EDID_DSC_16BPC)
> -				hdmi_dsc->bpc_supported = 16;
> -			else if (hf_scds[11] & DRM_EDID_DSC_12BPC)
> -				hdmi_dsc->bpc_supported = 12;
> -			else if (hf_scds[11] & DRM_EDID_DSC_10BPC)
> -				hdmi_dsc->bpc_supported = 10;
> -			else
> -				/* Supports min 8 BPC if DSC1.2 is supported*/
> -				hdmi_dsc->bpc_supported = 8;
> -
> -			dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
> -			drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
> -					     &hdmi_dsc->max_frl_rate_per_lane);
> -			hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
> -
> -			dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
> -			switch (dsc_max_slices) {
> -			case 1:
> -				hdmi_dsc->max_slices = 1;
> -				hdmi_dsc->clk_per_slice = 340;
> -				break;
> -			case 2:
> -				hdmi_dsc->max_slices = 2;
> -				hdmi_dsc->clk_per_slice = 340;
> -				break;
> -			case 3:
> -				hdmi_dsc->max_slices = 4;
> -				hdmi_dsc->clk_per_slice = 340;
> -				break;
> -			case 4:
> -				hdmi_dsc->max_slices = 8;
> -				hdmi_dsc->clk_per_slice = 340;
> -				break;
> -			case 5:
> -				hdmi_dsc->max_slices = 8;
> -				hdmi_dsc->clk_per_slice = 400;
> -				break;
> -			case 6:
> -				hdmi_dsc->max_slices = 12;
> -				hdmi_dsc->clk_per_slice = 400;
> -				break;
> -			case 7:
> -				hdmi_dsc->max_slices = 16;
> -				hdmi_dsc->clk_per_slice = 400;
> -				break;
> -			case 0:
> -			default:
> -				hdmi_dsc->max_slices = 0;
> -				hdmi_dsc->clk_per_slice = 0;
> -			}
> -		}
> +
> +		drm_parse_dsc_info(hdmi_dsc, hf_scds);
>  	}
>  
>  	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 3/4] drm/edid: Refactor HFVSDB parsing for DSC1.2

[Jani Nikula]

On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> DSC capabilities are given in bytes 11-13 of VSDB (i.e. bytes 8-10 of
> SCDS). Since minimum length of Data block is 7, all bytes greater than 7
> must be read only after checking the length of the data block.
>
> This patch adds check for data block length before reading relavant DSC
> bytes.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 93 ++++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ffff1d08b3a4..c9c3a9c8fa26 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5706,9 +5706,6 @@ static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
>  static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
>  			       const u8 *hf_scds)
>  {
> -	u8 dsc_max_slices;
> -	u8 dsc_max_frl_rate;
> -
>  	hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
>  
>  	if (!hdmi_dsc->v_1p2)
> @@ -5727,47 +5724,54 @@ static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
>  		/* Supports min 8 BPC if DSC1.2 is supported*/
>  		hdmi_dsc->bpc_supported = 8;
>  
> -	dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
> -	drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
> -			     &hdmi_dsc->max_frl_rate_per_lane);
> -	hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
> +	if (cea_db_payload_len(hf_scds) >= 12 && hf_scds[12]) {
> +		u8 dsc_max_slices;
> +		u8 dsc_max_frl_rate;
>  
> -	dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
> +		dsc_max_frl_rate = (hf_scds[12] & DRM_EDID_DSC_MAX_FRL_RATE_MASK) >> 4;
> +		drm_get_max_frl_rate(dsc_max_frl_rate, &hdmi_dsc->max_lanes,
> +				     &hdmi_dsc->max_frl_rate_per_lane);
>  
> -	switch (dsc_max_slices) {
> -	case 1:
> -		hdmi_dsc->max_slices = 1;
> -		hdmi_dsc->clk_per_slice = 340;
> -		break;
> -	case 2:
> -		hdmi_dsc->max_slices = 2;
> -		hdmi_dsc->clk_per_slice = 340;
> -		break;
> -	case 3:
> -		hdmi_dsc->max_slices = 4;
> -		hdmi_dsc->clk_per_slice = 340;
> -		break;
> -	case 4:
> -		hdmi_dsc->max_slices = 8;
> -		hdmi_dsc->clk_per_slice = 340;
> -		break;
> -	case 5:
> -		hdmi_dsc->max_slices = 8;
> -		hdmi_dsc->clk_per_slice = 400;
> -		break;
> -	case 6:
> -		hdmi_dsc->max_slices = 12;
> -		hdmi_dsc->clk_per_slice = 400;
> -		break;
> -	case 7:
> -		hdmi_dsc->max_slices = 16;
> -		hdmi_dsc->clk_per_slice = 400;
> -		break;
> -	case 0:
> -	default:
> -		hdmi_dsc->max_slices = 0;
> -		hdmi_dsc->clk_per_slice = 0;
> +		dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
> +
> +		switch (dsc_max_slices) {
> +		case 1:
> +			hdmi_dsc->max_slices = 1;
> +			hdmi_dsc->clk_per_slice = 340;
> +			break;
> +		case 2:
> +			hdmi_dsc->max_slices = 2;
> +			hdmi_dsc->clk_per_slice = 340;
> +			break;
> +		case 3:
> +			hdmi_dsc->max_slices = 4;
> +			hdmi_dsc->clk_per_slice = 340;
> +			break;
> +		case 4:
> +			hdmi_dsc->max_slices = 8;
> +			hdmi_dsc->clk_per_slice = 340;
> +			break;
> +		case 5:
> +			hdmi_dsc->max_slices = 8;
> +			hdmi_dsc->clk_per_slice = 400;
> +			break;
> +		case 6:
> +			hdmi_dsc->max_slices = 12;
> +			hdmi_dsc->clk_per_slice = 400;
> +			break;
> +		case 7:
> +			hdmi_dsc->max_slices = 16;
> +			hdmi_dsc->clk_per_slice = 400;
> +			break;
> +		case 0:
> +		default:
> +			hdmi_dsc->max_slices = 0;
> +			hdmi_dsc->clk_per_slice = 0;
> +		}
>  	}
> +
> +	if (cea_db_payload_len(hf_scds) >= 13 && hf_scds[13])
> +		hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
>  }
>  
>  /* Sink Capability Data Structure */
> @@ -5776,6 +5780,7 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  {
>  	struct drm_display_info *display = &connector->display_info;
>  	struct drm_hdmi_info *hdmi = &display->hdmi;
> +	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
>  
>  	display->has_hdmi_infoframe = true;
>  
> @@ -5816,17 +5821,17 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  
>  	if (hf_scds[7]) {
>  		u8 max_frl_rate;
> -		struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
>  
>  		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>  		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>  		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>  				     &hdmi->max_frl_rate_per_lane);
> -
> -		drm_parse_dsc_info(hdmi_dsc, hf_scds);
>  	}
>  
>  	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
> +
> +	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
> +		drm_parse_dsc_info(hdmi_dsc, hf_scds);
>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 4/4] drm/edid: Avoid multiple log lines for HFVSDB parsing

[Nautiyal, Ankit K]

Thanks Jani for the review and suggestions.

I agree with the suggestions and will make changes in next version.

Please find my response inline:

On 9/13/2022 7:24 PM, Jani Nikula wrote:
> On Thu, 11 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Replace multiple log lines with a single log line at the end of
>> parsing HF-VSDB. Also use drm_dbg_kms instead of DRM_DBG_KMS, and
>> add log for DSC1.2 support.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index c9c3a9c8fa26..7a319d570297 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -5781,6 +5781,9 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	struct drm_display_info *display = &connector->display_info;
>>   	struct drm_hdmi_info *hdmi = &display->hdmi;
>>   	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
>> +	u32 max_tmds_clock = 0;
> This should be int because display->max_tmds_clock is int. Yes, it's a
> change from the current local var, but logging u32 would require %u
> instead of %d in the format string anyway, so better just use the right
> type.
Alright, makes sense.
>> +	u8 max_frl_rate = 0;
>> +	bool dsc_support = false;
>>   
>>   	display->has_hdmi_infoframe = true;
>>   
>> @@ -5800,14 +5803,13 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	 */
>>   
>>   	if (hf_scds[5]) {
>> -		/* max clock is 5000 KHz times block value */
>> -		u32 max_tmds_clock = hf_scds[5] * 5000;
>>   		struct drm_scdc *scdc = &hdmi->scdc;
>>   
>> +		/* max clock is 5000 KHz times block value */
>> +		max_tmds_clock = hf_scds[5] * 5000;
>> +
>>   		if (max_tmds_clock > 340000) {
>>   			display->max_tmds_clock = max_tmds_clock;
>> -			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> -				display->max_tmds_clock);
> Hmm, the logic for what is logged gets changed.

You are right, we are now logging this always.

Should we log this only for rate > 340MHz? The logging line at last will 
require some jugglery.

>>   		}
>>   
>>   		if (scdc->supported) {
>> @@ -5820,9 +5822,6 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   	}
>>   
>>   	if (hf_scds[7]) {
>> -		u8 max_frl_rate;
>> -
>> -		DRM_DEBUG_KMS("hdmi_21 sink detected. parsing edid\n");
>>   		max_frl_rate = (hf_scds[7] & DRM_EDID_MAX_FRL_RATE_MASK) >> 4;
>>   		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>>   				     &hdmi->max_frl_rate_per_lane);
>> @@ -5830,8 +5829,14 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>>   
>>   	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
>>   
>> -	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])
>> +	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
>>   		drm_parse_dsc_info(hdmi_dsc, hf_scds);
>> +		dsc_support = true;
>> +	}
>> +
>> +	drm_dbg_kms(connector->dev,
>> +		    "HF-VSDB: max TMDS clock:%d Khz, HDMI2.1 support:%s, DSC1.2 support:%s\n",
> Nitpicks, %d needs int instead of u32, "kHz" not "Khz", "HDMI 2.1" and
> "DSC 1.2" with spaces, would prefer a space after ":".
Noted, Will fix this.
>
>> +		    max_tmds_clock, max_frl_rate ? "yes" : "no", dsc_support ? "yes" : "no");
> See str_yes_no().

Right, should have used str_yes_no().


Thanks & Regards,

Ankit

>
> BR,
> Jani.
>
>>   }
>>   
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,