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.
>
> Other than that, patches 1-8, 10, and 11 are
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
