Re: [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 29, 2014 at 08:31:55PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote:
> > On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a flag to drm_device which will cause the vblank code to bypass the
> > > disable timer and always disable the vblank interrupt immediately when
> > > the last reference is dropped.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_irq.c |  6 +++---
> > >  include/drm/drmP.h        | 10 ++++++++++
> > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 20a4855..b008803 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> > >  
> > >  	/* Last user schedules interrupt disable */
> > >  	if (atomic_dec_and_test(&vblank->refcount)) {
> > > -		if (drm_vblank_offdelay > 0)
> > > +		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
> > > +			vblank_disable_fn((unsigned long)vblank);
> > > +		else if (drm_vblank_offdelay > 0)
> > >  			mod_timer(&vblank->disable_timer,
> > >  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> > > -		else if (drm_vblank_offdelay == 0)
> > > -			vblank_disable_fn((unsigned long)vblank);
> > >  	}
> > >  }
> > >  EXPORT_SYMBOL(drm_vblank_put);
> > 
> > Would it be better if we just squashed this device flag logic back into
> > patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never
> > disable?"  I can see there being people who might already use this when
> > debugging new and potentially flaky hardware platforms who would be
> > surprised by the change in behavior. So basically something like:
> > 
> >         if (drm_vblank_offdelay == 0)
> >                 return
> >         else if (dev->vblank_disable_immediate)
> >                 vblank_disable_fn((unsigned long)vblank);
> >         else
> >                 mod_timer(...);
> 
> I'm not sure I want drm_vblank_offdelay affecting drivers that have
> vblank_disable_immediate set since it's a global variable. If there
> are two devices on the system where one has
> vblank_disable_immediate==true and the other doesn't, the user
> might want to keep vblank interrupts enabled on the crappy device
> all time by frobbing drm_vblank_offdelay.
> 
> I agree that changing the meaning of drm_vblank_offdelay=0 is a bit
> questionable, but reversing the meaning so that '==0' meas 'never disable'
> and '<0' means 'disable immediately' seemed a bit weird for my taste. But
> I suppose I could make that change if people think it's better. Or maybe
> just forget about the 'disable immediately' behaviour when
> vblank_disable_immediate==false?
> 
> > I'd also suggest adding a comment in drm_stub.c to indicate that
> > drm_vblank_offdelay gets ignored for drivers that set
> > vblank_disable_immediate.
> 
> Good idea.

Yeah, I think that's good enough. There really shouldn't be any need for
drivers which support immediate vblank disabling to ever keep it on for a
while. Presuming the immediate disable actually works ofc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch