Re: [PATCH RFC 03/25] drm/atomic-helper: Set mode_changed on vrr_enabled change

[Nicolas Frattaroli]

On Monday, 21 September 2026 23:59:11 Central European Summer Time Leo Li wrote:
> Hi Nicholas,
> 
> Thanks for sending this out. I'm looking into an amdgpu implementation
> and had some comments along the way. Will reply inline on respective
> patches.
> 
> On 2026-09-21 11:51, Nicolas Frattaroli wrote:
> > A change in whether VRR is enabled or not on a CRTC will be of interest
> > to any VRR implementation, as they may need to recalculate certain
> > internal state in response. So far, this was handled ad-hoc in the
> > drivers that implement VRR.
> > 
> > Handle this in the common DRM atomic core by setting the mode_changed
> > flag when this occurs. The rationale for using mode_changed is that VRR
> > is deeply intertwined with the current mode with regards to its timing.
> > Consequently, any component that cares about a mode change is likely
> > also going to care about VRR being enabled/disabled.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@xxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9d006f98413a..0664d5779282 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -694,6 +694,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  			new_crtc_state->active_changed = true;
> >  		}
> >  
> > +		if (old_crtc_state->vrr_enabled != new_crtc_state->vrr_enabled) {
> > +			drm_dbg_atomic(dev, "[CRTC:%d:%s] VRR changed\n",
> > +				       crtc->base.id, crtc->name);
> > +			new_crtc_state->mode_changed = true;
> > +		}
> > +
> 
> I don't think this is the case for all hardware, at least not for amdgpu.
> The parts in HW that adjust vtotal can be programmed without requiring
> bandwidth recalculations or reprogramming of pipes.

Thanks for pointing this out. I was already unsure of this patch before,
and I don't think Rockchip requires these recalculations either. Originally
I added this when I looked at the i915 code iirc and saw them setting it.

I think in the commit message I also conflate needing to know about VRR
being changed with needing to do a full modeset (which is what mode_changed
basically means).

I'll drop this patch on the next revision.

Kind regards,
Nicolas Frattaroli

> 
> - Leo
> 
> >  		if (new_crtc_state->enable != has_connectors) {
> >  			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch (%d/%d)\n",
> >  				       crtc->base.id, crtc->name,
> > 
> 
>