Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm: integer overflow and implicit sign conversion in VRR vsync pacing - [Medium] drm: missing initialization of VRR accumulator cur_err on rate transitions - [Medium] drm: fixed VRR target rates bypass sink's max_vfreq capability check - [Low] drm: redundant check_mul_overflow and unused limit_d in VRR limits validation --
commit d061573186c289ab886bdae3285afbec3f7304ae Author: Nicolas Frattaroli <[email protected]> drm: Implement VRR rate limiting Currently, VRR is allowed to use the entire range advertised by the sink's EDID. Userspace may find it desirable, however, to limit this range, or set a fixed target rate entirely. The latter is mainly useful for multimedia playback applications, where judder can be avoided by matching the content's frame rate precisely. > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > index 33d0c94916430..7dbd678cf0612 100644 > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c [ ... ] > @@ -1046,6 +1047,39 @@ static int hdmi_calculate_vtotal(const struct > drm_display_mode *mode, > return 0; > } > > +static int hdmi_validate_vrr_limits(struct drm_crtc_state *crtc_state, u16 > vrr_min) > +{ > + struct drm_crtc_vrr_state *vrr_state = &crtc_state->vrr_state; > + const struct drm_display_mode *mode = &crtc_state->mode; > + u32 limit_d; > + int ret; > + > + /* No limiter, don't need to validate, but reset the error */ > + if (!drm_crtc_helper_vrr_is_fixed_rate(vrr_state)) { > + vrr_state->per_frame_err = 0; > + return 0; > + } > + > + /* Don't allow rates equal to or below the minimum VRR rate / 1.001 */ > + if (cmp_fraction(vrr_state->vrr_min_n, vrr_state->vrr_min_d, > + vrr_min * 1000, 1001) < 0) > + return -EINVAL; > + > + if (check_mul_overflow(mode->crtc_htotal, vrr_state->vrr_min_n, > &limit_d)) > + return -ERANGE; [Severity: Low] Is the limit_d variable ever used after being assigned in this overflow check? Also, doesn't hdmi_calculate_vtotal() duplicate the exact same mode->crtc_htotal * rate_n check immediately below, making this check redundant? > + > + ret = hdmi_calculate_vtotal(&crtc_state->mode, vrr_state->vrr_min_n, > + vrr_state->vrr_min_d, > &vrr_state->base_vtotal, > + &vrr_state->denom, > &vrr_state->per_frame_err); [Severity: Medium] When userspace changes the fixed VRR target rate or switches from dynamic to fixed VRR, does this state transition leave the old cur_err accumulator value intact? If the new denom is significantly smaller than the old one, could a disproportionately large carried-over cur_err take thousands of frames to drain, causing a persistent frame rate artifact (stuck at base_vtotal + 1) for several seconds? > + if (ret) > + return ret; > + > + if (vrr_state->base_vtotal < mode->crtc_vtotal) > + return -EINVAL; > + > + return 0; > +} [ ... ] > @@ -1112,25 +1151,64 @@ static int hdmi_validate_vrr(struct drm_connector > *connector, > > vrr_state->vic = drm_match_cea_mode(mode); > > - if (cmp_fraction(min_vfreq_n, min_vfreq_d, max_vfreq_n, max_vfreq_d) > > 0) { > - drm_dbg_kms(dev, "Target max (%u/%u) > target min (%u/%u)\n", > - max_vfreq_n, max_vfreq_d, min_vfreq_n, min_vfreq_d); > - return -EINVAL; > - } > - > - ret = hdmi_calculate_vtotal(mode, max_vfreq_n, max_vfreq_d, > - &vrr_state->base_vtotal, NULL, NULL); > + ret = hdmi_validate_vrr_limits(new_crtc_state, min_vfreq_n); > if (ret) { > - drm_dbg_kms(dev, "Couldn't calculate base_vtotal: %pe\n", > ERR_PTR(ret)); > + drm_dbg_kms(dev, "Invalid VRR fixed target rate: %pe\n", > ERR_PTR(ret)); > return ret; > } > > - /* > - * Even at its fastest, it can't go faster than the mode, so > - * clamp to avoid imprecisely rounded mode_refresh values we > - * can't do anything about from ruining our day. > - */ > - vrr_state->base_vtotal = max(vrr_state->base_vtotal, mode->crtc_vtotal); > + if (!is_fixed) { [Severity: Medium] If a userspace DRM client requests a fixed VRR target rate, does wrapping the capability checks in if (!is_fixed) cause the driver to silently bypass validating against the monitor's max_vfreq? If the monitor's EDID advertises a max_vfreq strictly lower than the mode's refresh rate, couldn't userspace improperly exceed the sink's hardware capabilities? > + vrr_state->dynamic = true; > + > + if (vrr_state->vrr_min_n) { > + if (!vrr_state->vrr_min_d) > + return -EINVAL; [ ... ] > diff --git a/drivers/gpu/drm/drm_crtc_helper.c > b/drivers/gpu/drm/drm_crtc_helper.c > index 4ffb3b0b78780..c46de778df309 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c [ ... ] > @@ -889,10 +908,21 @@ bool drm_crtc_helper_vrr_vsync(struct drm_crtc_state > *crtc_state) > struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state; > u16 old_vtotal = vrr->cur_vtotal; > > - if (!crtc_state->vrr_enabled) > + if (!crtc_state->vrr_enabled) { > vrr->cur_vtotal = 0; > - else > + vrr->cur_err = 0; > + } else if (drm_crtc_helper_vrr_is_fixed_rate(vrr)) { > + if (vrr->cur_err * 2 >= vrr->denom) { [Severity: High] Since vrr->cur_err is an int and vrr->denom is a u32, can this cause an implicit sign conversion bug? When userspace sets a fixed VRR rate where per_frame_err < denom / 2, cur_err becomes negative after vrr->cur_err -= vrr->denom executes. Because vrr->denom is a u32, won't C integer promotion rules implicitly cast the negative int to a massive u32, causing this check to be unconditionally true every frame thereafter? > + vrr->cur_err -= vrr->denom; > + vrr->cur_vtotal = vrr->base_vtotal + 1; > + } else { > + vrr->cur_vtotal = vrr->base_vtotal; > + } > + > + vrr->cur_err += vrr->per_frame_err; > + } else { > vrr->cur_vtotal = vrr->max_vtotal; > + } [ ... ] > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index df37f4ebb58ed..00e9df02f3d5e 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -71,6 +71,9 @@ struct drm_crtc_vrr_state { > u32 vrr_min_d; > u32 vrr_max_n; > u32 vrr_max_d; > + u32 denom; > + u32 per_frame_err; > + int cur_err; [Severity: High] Does declaring cur_err as an int while denom is a u32 cause the signed/unsigned comparison flaw in drm_crtc_helper_vrr_vsync()? > u16 base_vtotal; > bool dynamic; > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
