Re: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 10, 2023 at 07:01:18PM +0000, Golani, Mitulkumar Ajitkumar wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: 06 March 2023 20:59
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff
> > on seamless M/N change
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When we change the M/N values seamlessly during a fastset we should also
> > update the vblank timestamping stuff to make sure the vblank timestamp
> > corrections/guesstimations come out exact.
> > 
> > Note that only crtc_clock and framedur_ns can actually end up changing here
> > during fastsets. Everything else we touch can only change during full
> > modesets.
> > 
> > Technically we should try to do this exactly at the start of vblank, but that
> > would require some kind of double buffering scheme. Let's skip that for now
> > and just update things right after the commit has been submitted to the
> > hardware. This means the information will be properly up to date when the
> > vblank irq handler goes to work. Only if someone ends up querying some
> > vblanky stuff in between the commit and start of vblank may we see a slight
> > discrepancy.
> > 
> > Also this same problem really exists for the DRRS downclocking stuff. But as
> > that is supposed to be more or less transparent to the user, and it only drops
> > to low gear after a long delay
> > (1 sec currently) we probably don't have to worry about it.
> > Any time something is actively submitting updates DRRS will remain in high
> > gear and so the timestamping constants will match the hardware state.
> > 
> > Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index b79a8834559f..41d381bbb57a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state
> > *new_crtc_state)
> >  	 */
> >  	intel_vrr_send_push(new_crtc_state);
> > 
> > +	/*
> > +	 * Seamless M/N update may need to update frame timings.
> > +	 *
> > +	 * FIXME Should be synchronized with the start of vblank somehow...
> > +	 */
> > +	if (new_crtc_state->seamless_m_n &&
> > intel_crtc_needs_fastset(new_crtc_state))
> > +		intel_crtc_update_active_timings(new_crtc_state);
> > +
> Hi Ville,
> 
> changes LGTM. Although have a question, are we suspecting any timing param changes after Push bit is sent ?

Push is only used with VRR, at which points the real timings
can change of course as the hw terminates the vblank early.
But our notion of active timings will not change (they've already
been correctly set up for VRR when we enabled VRR).

> 
> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> 
> Thanks
> >  	local_irq_enable();
> > 
> >  	if (intel_vgpu_active(dev_priv))
> > --
> > 2.39.2
> 

-- 
Ville Syrjälä
Intel