Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/edid: redundant VRR parsing has off-by-one length checks skipping valid EDID data --
commit 5e1483c916355c112a6de67649f48244b2260d1e Author: Nicolas Frattaroli <[email protected]> drm/edid: Parse QMS flag from HDMI SCDS This commit adds support for parsing the Quick Media Switching (QMS) flag from the HDMI Forum EDID extension. It adds the definition to the EDID header file and parses it into the qms_capable member of drm_hdmi_info. > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index cc6f2ba8c9f4d..5a7d3b7e65daa 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c [ ... ] > @@ -6318,6 +6318,7 @@ static void drm_parse_vrr_info(struct drm_display_info > *info, const u8 *hf_scds) > if (pld_len < 9) > return; > > info->hdmi.cinema_vrr = !!(hf_scds[8] & DRM_EDID_CINEMA_VRR); > + info->hdmi.qms_capable = !!(hf_scds[8] & DRM_EDID_QMS); [Severity: Medium] Does this off-by-one bounds check skip valid EDID data? Looking at drm_parse_vrr_info(), it returns early if the payload length is less than 9. This means it silently skips parsing the valid QMS and CinemaVRR flags at hf_scds[8] for sinks whose payloads are exactly 8 bytes long. For comparison, the pre-existing drm_parse_hdmi_gaming_info() function correctly parses this exact same payload byte using: if (cea_db_payload_len(db) < 8) return; Could we update the pld_len check in drm_parse_vrr_info() to < 8 so that it matches the bounds logic in drm_parse_hdmi_gaming_info() and doesn't break QMS/VRR limit detection for those sinks? > > if (pld_len < 10) > return; [ ... ] > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index b147b8474f1a9..c97cf0625fda5 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -386,6 +386,9 @@ struct drm_hdmi_info { > > /** @cinema_vrr: The sink supports "CinemaVRR" */ > bool cinema_vrr; > + > + /** @qms_capable: The sink supports Quick Media Switching */ > + bool qms_capable; [Severity: Medium] Does the cinema_vrr field duplicate the existing vrr_cap.cinema_vrr field? This isn't a bug introduced by this patch, but having a duplicate field can create desynchronized state when parsed. Would it be better to consolidate this redundant VRR parsing altogether? > }; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
