Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm: false positive VRR capability for HDMI monitors with generic EDID range limits - [Medium] drm: off-by-one payload length checks skip valid VRR limits in HF-VSDB --
commit e42644d1c43977633d9a0e950d2749308a75d1b1 Author: Derek Foreman <[email protected]> drm/edid: Add a query for vrr range This commit adds parsing for minimum and maximum VRR vertical rates from the EDID. > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 3148038747b6e..a1494028e0f5e 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c [ ... ] > @@ -6309,6 +6309,25 @@ static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap > *hdmi_dsc, > hdmi_dsc->total_chunk_kbytes = hf_scds[13] & > DRM_EDID_DSC_TOTAL_CHUNK_KBYTES; > } > > +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 < 10) > + return; [Severity: Medium] Does this overly restrictive check skip parsing the VRR minimum frequency when the payload is exactly 9 bytes? If the payload length is 9, which is sufficient to contain the VRR minimum at index 9, the condition evaluates to true and triggers an early return. The monitor could lose HDMI VRR support or fall back to incorrect limits. > + > + if (!range->min_vfreq) > + range->min_vfreq = FIELD_GET(DRM_EDID_VRR_MIN_MASK, hf_scds[9]); [Severity: High] Does this zero-check prevent the driver from overriding generic VESA limits with explicit HDMI VRR limits? Since generic VESA limits are parsed first and populate the monitor_range structure, this check might actively prevent the function from applying the explicit HDMI VRR limits, sealing the false positive capability check below. > + > + if (pld_len < 11) > + return; [Severity: Medium] Does this similarly skip parsing the VRR maximum frequency when the payload is exactly 10 bytes? If the payload length is 10, which contains the VRR maximum at index 10, this check causes an early return and skips reading the data. > + > + if (!range->max_vfreq) > + range->max_vfreq = (FIELD_GET(DRM_EDID_VRR_MAX_UPPER_MASK, > hf_scds[9]) << 8) | > + hf_scds[10]; > +} > + > /* Sink Capability Data Structure */ > static void drm_parse_hdmi_forum_scds(struct drm_connector *connector, > const u8 *hf_scds) [ ... ] > @@ -6371,6 +6390,13 @@ static void drm_parse_hdmi_forum_scds(struct > drm_connector *connector, > dsc_support = true; > } > > + drm_parse_vrr_info(info, hf_scds); > + > + if (info->monitor_range.min_vfreq > 0) > + hdmi->vrr_capable = true; > + else > + hdmi->vrr_capable = false; [Severity: High] Does this unconditionally assign HDMI VRR capability based on a field that might be populated by generic VESA monitor range limits? If a monitor provides generic VESA continuous frequency range limits but does not support explicit HDMI VRR in the HF-VSDB, it might still have a non-zero min_vfreq. This could falsely mark the monitor as HDMI VRR capable, causing the driver to attempt VRR signaling on unsupported hardware and leading to blank screens, flickering, or modeset failures. > + > drm_dbg_kms(connector->dev, > "[CONNECTOR:%d:%s] HF-VSDB: max TMDS clock: %d KHz, HDMI > 2.1 support: %s, DSC 1.2 support: %s\n", > connector->base.id, connector->name, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
