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 Tue, Mar 07, 2023 at 02:16:48PM +0200, Jani Nikula wrote:
> On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > 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);
> 
> Side note, do we ensure somewhere that intel_crtc_needs_modeset() &&
> intel_crtc_needs_fastset() is never true?

commit 7de5b6b54630 ("drm/i915: Don't flag both full modeset and fastset
at the same time")

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks

> 
> > +
> >  	local_irq_enable();
> >  
> >  	if (intel_vgpu_active(dev_priv))
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel