Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrj�l� wrote:
> On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > 
> > > On i965gm we need to adjust max_vblank_count dynamically
> > > depending on whether the TV encoder is used or not. To
> > > that end add a per-crtc max_vblank_count that takes
> > > precedence over its device wide counterpart. The driver
> > > can now call drm_crtc_set_max_vblank_count() to configure
> > > the per-crtc value before calling drm_vblank_on().
> > > 
> > > Also looks like there was some discussion about exynos needing
> > > similar treatment.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Inki Dae <inki.dae@samsung.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > >  include/drm/drm_vblank.h     |  8 ++++++++
> > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 98e091175921..c3abbdca8aba 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > >  	write_sequnlock(&vblank->seqlock);
> > >  }
> > >  
> > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > +{
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > +
> > > +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > +}
> > > +
> > >  /*
> > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > >   * if there is no useable hardware frame counter available.
> > >   */
> > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > >  {
> > > -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  	ktime_t t_vblank;
> > >  	int count = DRM_TIMESTAMP_MAXRETRIES;
> > >  	int framedur_ns = vblank->framedur_ns;
> > > +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > >  
> > >  	/*
> > >  	 * Interrupts were disabled prior to this call, so deal with counter
> > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > >  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > >  
> > > -	if (dev->max_vblank_count != 0) {
> > > +	if (max_vblank_count) {
> > >  		/* trust the hw counter when it's around */
> > > -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > +		diff = (cur_vblank - vblank->last) & max_vblank_count;
> > >  	} else if (rc && framedur_ns) {
> > >  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > >  
> > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> > >  
> > >  	if (diff == 0) {
> > > -		WARN_ON_ONCE(cur_vblank != vblank->last);
> > > +		WARN_ON_ONCE(max_vblank_count &&
> > > +			     cur_vblank != vblank->last);
> > 
> > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > I'm missing something.
> 
> Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> does work until the exact point when we enable TV encoder. Thus we
> will get non-zero values up to that point, and since the TV encoder
> isn't yet throttling the pipe it presumably runs at the oversample
> clock so our timestamp based estimates can give us a diff==0 even
> though the pipe did indeed pass a vblank already. I forgot to
> note this in the commit message.
> 
> I think we can handle this three ways:
> 1. do what I do here and just let the mismatch slip through
> 2. force i915_get_vblank_counter() to return 0 always when the
>    TV encoder is going to be used
> 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
>    and instead delay it until just before we enable the TV encoder
> 
> I think option 3 is overly complicated to consider seriously. So
> option 1 or option 2 is what I think we should do. For whatever
> reason I went with option 1 here, but maybe option 2 is better
> since it would be all contained within i915...

Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
semantically clean solution to me, instead of hacking around in core code
when drivers leak garbage out ...

-Daniel


> > 
> > >  		return;
> > >  	}
> > >  
> > > @@ -1204,6 +1213,28 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
> > >  }
> > >  EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > >  
> > > +/**
> > > + * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
> > > + * @crtc: CRTC in question
> > > + * @max_vblank_count: max hardware vblank counter value
> > > + *
> > > + * Update the maximum hardware vblank counter value for @crtc. Useful
> > > + * for hardware where the operation of the hardware vblank counter
> > > + * depends on the active display configuration.
> > > + *
> > > + * If used, must be called before drm_vblank_on().
> > 
> > I think we should check this at runtime with a WARN_ON. Plus make the
> > comment here a bit clearer that this is indeed for runtime adjusting of
> > the max_vblank_count, in cases where that depends upon the connected
> > outputs.
> 
> Sure. I'll try to pimp up the docs a bit.
> 
> > 
> > > + */
> > > +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> > > +				   u32 max_vblank_count)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	unsigned int pipe = drm_crtc_index(crtc);
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > +
> > > +	vblank->max_vblank_count = max_vblank_count;
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
> > > +
> > >  /**
> > >   * drm_crtc_vblank_on - enable vblank events on a CRTC
> > >   * @crtc: CRTC in question
> > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > index 6ad9630d4f48..ecb2cf9913e2 100644
> > > --- a/include/drm/drm_vblank.h
> > > +++ b/include/drm/drm_vblank.h
> > > @@ -128,6 +128,12 @@ struct drm_vblank_crtc {
> > >  	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
> > >  	 */
> > >  	u32 last;
> > > +	/**
> > > +	 * @max_vblank_count: Maximum value of the hardware vblank counter.
> > > +	 * If non-zero this takes precedence over &drm_device.max_vblank_count
> > > +	 * for this crtc. Otherwise &drm_device.max_vblank_count is used.
> > > +	 */
> > 
> > I'd add "This should be set by calling drm_crtc_set_max_vblank_count()."
> > 
> > And please also add a note to the kerneldoc of drm_driver.max_vblank_count
> > pointing at &drm_vblank_crtc.max_vblank_count for per-crtc limits.
> 
> Ack.
> 
> > 
> > Aside from the nits lgtm. I think I'll skip looking at the TV out stuff
> > though ...
> 
> :)
> 
> -- 
> Ville Syrj�l�
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch