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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2826 bytes --]

On Tue, Sep 22, 2026 at 02:53:36PM +0200, Nicolas Frattaroli wrote:
> 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@collabora.com>
> > > ---
> > >  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.

Alternatively, and since we only have a single driver enabling it (for
now), I don't think having some driver specific code for now to handle
all this is that bad.

Helpers only really make sense when we start sharing code between
drivers.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]