Re: [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 04:30:29PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 06, 2014 at 03:08:25PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 06, 2014 at 02:49:58PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We should update the last in drm_update_vblank_count() to avoid applying
> > > the diff more than once. This could occur eg. if drm_vblank_off() gets
> > > called multiple times for the crtc.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we update ->last when disabling the vblank and use it when
> > re-enabling it. Those calls should be symmetric, except for driver bugs.
> > Imo would be better to tighten up the checks for that.
> > 
> > Or do I completely misunderstand what's going on here?
> 
> The issue is that we want to call drm_vblank_off() in
> intel_sanitize_crtc() for already disabled crtcs in order to
> to bump the refcount and set inmodeset=1 so that
> drm_vblank_get() won't try to enable vblank interrupts if someone calls
> drm_vblank_get() on it. So during suspend+resume we can get two
> drm_vblank_off() calls for the same crtc w/o a drm_vblank_on() in
> between.
> 
> Although this patch doesn't actually fix the problem really since
> vblank counter query during sanitize will just return 0 because the pipe
> isn't active, so there's a later patch to just override .last=0 in
> intel_sanitize_crtc(). Another approach might be to set .last=0 in
> drm_vblank_off() itself (after vblank_disable_and_save()), mainly
> just to hide the details from the caller.

Resetting ->last (if it works, need to be careful with drivers calling
_off multiple times perhaps) sounds like the much simpler option, and
probably does what everyone expects anyway.
-Daniel

> 
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_irq.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 0523f5b..67507a4 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -109,6 +109,8 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> > >  	if (diff == 0)
> > >  		return;
> > >  
> > > +	vblank->last = cur_vblank;
> > > +
> > >  	/* Reinitialize corresponding vblank timestamp if high-precision query
> > >  	 * available. Skip this step if query unsupported or failed. Will
> > >  	 * reinitialize delayed at next vblank interrupt in that case.
> > > -- 
> > > 1.8.5.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch