On Fri, 22 Jul 2022, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
