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

2014-07-29 Thread Ville Syrjälä 
On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote:
 On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrj...@linux.intel.com wrote:
  From: Ville Syrjälä ville.syrj...@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.syrj...@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.ro...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-07-29 Thread Daniel Vetter 
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.syrj...@linux.intel.com 
  wrote:
   From: Ville Syrjälä ville.syrj...@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.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-06-19 Thread Matt Roper 
On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrj...@linux.intel.com wrote:
 From: Ville Syrjälä ville.syrj...@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.syrj...@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'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.

Other than that, patches 1-8, 10, and 11 are
Reviewed-by: Matt Roper matthew.d.ro...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-05-26 Thread ville . syrjala 
From: Ville Syrjälä ville.syrj...@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.syrj...@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);
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