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

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

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...

> 
> >  		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