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

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

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.

> 
> Other than that, patches 1-8, 10, and 11 are
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I'll finish up reviewing #9 and 12-14 tomorrow when I have some more
> time.
> 
> 
> 
> Matt
> 
> 
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 979a498..0944544 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1117,6 +1117,16 @@ struct drm_device {
> >  	 */
> >  	bool vblank_disable_allowed;
> >  
> > +	/*
> > +	 * If true, vblank interrupt will be disabled immediately when the
> > +	 * refcount drops to zero, as opposed to via the vblank disable
> > +	 * timer.
> > +	 * This can be set to true it the hardware has a working vblank
> > +	 * counter and the driver uses drm_vblank_on() and drm_vblank_off()
> > +	 * appropriately.
> > +	 */
> > +	bool vblank_disable_immediate;
> > +
> >  	/* array of size num_crtcs */
> >  	struct drm_vblank_crtc *vblank;
> >  
> > -- 
> > 1.8.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC