[Intel-gfx] [PATCH] drm/drm_edid: Refactor HFVSDB parsing for DSC1.2
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Wed Aug 10 14:45:59 UTC 2022
On 8/2/2022 8:19 PM, Jani Nikula wrote:
> On Fri, 22 Jul 2022, Ankit Nautiyal <ankit.k.nautiyal at 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.
Hi Jani,
Thanks for the comments. I do agree, it makes more sense to have a
separate patches with incremental changes.
Will send another series with the comments addressed.
Please find the response inline:
>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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.
Noted. Will take care in the next patch.
>
>> +{
>> + 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.
Right, will use the function here.
>
>> + u8 dsc_max_frl_rate;
>> + u8 dsc_max_slices;
> These two are local to a tiny if block and should be declared there.
Agreed. Will fix in the next patchset.
>
>> + 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.
Makes sense, will drop these checks.
>
>>
>> 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.
Hmm. We can get rid of early return.
Will have a separate patch to have logging call at the end with
drm_dbg_kms as suggested.
>
>> 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.
Right makes sense. Will have this change as a separate patch.
Regards,
Ankit
>
> 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,
More information about the Intel-gfx
mailing list