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