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@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
