On 8/2/2022 8:19 PM, Jani Nikula wrote:
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.
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@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.
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,
