Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/edid: redundant cinema_vrr field introduces logic duplication - [Medium] drm/edid: off-by-one payload length check in drm_parse_vrr_info() --
commit 2696b9f8c9224d0c1b191489f857db21f792e625 Author: Nicolas Frattaroli <[email protected]> drm/edid: Parse CinemaVRR flag from HDMI SCDS HDMI's extension to EDID contains a flag which is used by sinks to signal that they support CinemaVRR. When CinemaVRR is enabled, some VRR is possible even if the minimum VRR range is set to 0. > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a1494028e0f5e..cc6f2ba8c9f4d 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -6314,6 +6314,11 @@ static void drm_parse_vrr_info(struct drm_display_info > *info, const u8 *hf_scds) > struct drm_monitor_range_info *range = &info->monitor_range; > const int pld_len = cea_db_payload_len(hf_scds); > > + if (pld_len < 9) > + return; [Severity: Medium] Does this length check prematurely abort parsing for valid HDMI Forum VSDBs that provide exactly an 8-byte payload? Since cea_db_payload_len() returns the number of payload bytes following the tag, and payload bytes are indexed starting from 1 up to pld_len, accessing hf_scds[8] requires pld_len to be at least 8. When pld_len == 8, it seems the valid 8th payload byte containing the CinemaVRR flag would be incorrectly skipped. > + > + info->hdmi.cinema_vrr = !!(hf_scds[8] & DRM_EDID_CINEMA_VRR); > + > if (pld_len < 10) > return; [ ... ] > @@ -6392,7 +6397,7 @@ static void drm_parse_hdmi_forum_scds(struct > drm_connector *connector, > > drm_parse_vrr_info(info, hf_scds); > > - if (info->monitor_range.min_vfreq > 0) > + if (info->monitor_range.min_vfreq > 0 || hdmi->cinema_vrr) [Severity: Medium] Are we consuming a newly added redundant field here instead of the existing vrr_cap.cinema_vrr? Since the driver already correctly parses the CinemaVRR capability from the exact same EDID bit in drm_parse_hdmi_gaming_info() into hdmi->vrr_cap.cinema_vrr, could using this new duplicate field cause driver state inconsistencies if the two parsing logics diverge? > hdmi->vrr_capable = true; > else > hdmi->vrr_capable = false; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index b18684d039d5e..b147b8474f1a9 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -383,6 +383,9 @@ struct drm_hdmi_info { > > /** @vrr_capable: The sink supports variable refresh rate */ > bool vrr_capable; > + > + /** @cinema_vrr: The sink supports "CinemaVRR" */ > + bool cinema_vrr; [Severity: Medium] Could this introduce a duplicate field when vrr_cap.cinema_vrr already exists in the same structure? > }; > > /** -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
