Re: [Intel-gfx] [PATCH] drm/drm_edid: Refactor HFVSDB parsing for DSC1.2

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 22 Jul 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. It also corrects min DSC BPC to 8, and minor refactoring for
> better readability, and proper log messages.

I think this patch tries to do too much at once. Please split it up. One
thing per patch.

I think the logging is excessive, and what logging remains should use
drm_dbg_kms() instead of DRM_DEBUG_KMS().

Further comments inline.

>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 124 +++++++++++++++++++++++--------------
>  1 file changed, 77 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index bbc25e3b7220..f683a8d5fd31 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5703,12 +5703,58 @@ static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,
>  	hdmi->y420_dc_modes = dc_mask;
>  }
>  
> +static void drm_parse_dsc_slice_info(u8 dsc_max_slices,
> +				     struct drm_hdmi_dsc_cap *hdmi_dsc)

Arguments should always be in the order: context, destination, source.

> +{
> +	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)
>  {
>  	struct drm_display_info *display = &connector->display_info;
>  	struct drm_hdmi_info *hdmi = &display->hdmi;
> +	u8 db_length = hf_scds[0] & 0x1F;

There's cea_db_payload_len() for this, and you can use that directly
instead of caching the value to a local variable.

> +	u8 dsc_max_frl_rate;
> +	u8 dsc_max_slices;

These two are local to a tiny if block and should be declared there.

> +	struct drm_hdmi_dsc_cap *hdmi_dsc = &hdmi->dsc_cap;
> +
> +	if (db_length < 7 || db_length > 31)
> +		return;

Both cea_db_is_hdmi_forum_vsdb() and cea_db_is_hdmi_forum_scdb() check
the payload is >= 7 bytes before this one even gets called.

There's no reason to not parse the first 31 bytes if the length is > 31
bytes. That condition just breaks future compatibility for no reason.

>  
>  	display->has_hdmi_infoframe = true;
>  
> @@ -5749,17 +5795,25 @@ 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;
> +		if (max_frl_rate)
> +			DRM_DEBUG_KMS("HDMI2.1 FRL support detected\n");
> +
>  		drm_get_max_frl_rate(max_frl_rate, &hdmi->max_lanes,
>  				     &hdmi->max_frl_rate_per_lane);
> +
> +		drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
> +	}
> +
> +	if (db_length < 11)
> +		return;
> +
> +	if (hf_scds[11]) {

Matter of taste, but I'd probably make these

	if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11])

and drop the early returns, and add a single (or very few) debug logging
call at the end.

>  		hdmi_dsc->v_1p2 = hf_scds[11] & DRM_EDID_DSC_1P2;
>  
>  		if (hdmi_dsc->v_1p2) {
> +			DRM_DEBUG_KMS("HDMI DSC1.2 support detected\n");
>  			hdmi_dsc->native_420 = hf_scds[11] & DRM_EDID_DSC_NATIVE_420;
>  			hdmi_dsc->all_bpp = hf_scds[11] & DRM_EDID_DSC_ALL_BPP;
>  
> @@ -5770,52 +5824,28 @@ 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;
> -
> -			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;
> -			}

Splitting this to a separate function should be a non-functional prep
patch.

BR,
Jani.

> +				/* Supports min 8 BPC if DSC1.2 is supported*/
> +				hdmi_dsc->bpc_supported = 8;
>  		}
>  	}
>  
> -	drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
> +	if (db_length < 12)
> +		return;
> +
> +	if (hdmi_dsc->v_1p2 && hf_scds[12]) {
> +		dsc_max_slices = hf_scds[12] & DRM_EDID_DSC_MAX_SLICES;
> +		drm_parse_dsc_slice_info(dsc_max_slices, hdmi_dsc);
> +
> +		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);
> +	}
> +
> +	if (db_length < 13)
> +		return;
> +
> +	if (hdmi_dsc->v_1p2 && hf_scds[13])
> +		hdmi_dsc->total_chunk_kbytes = hf_scds[13] & DRM_EDID_DSC_TOTAL_CHUNK_KBYTES;
>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,

-- 
Jani Nikula, Intel Open Source Graphics Center