Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 04/15/2015 03:03 AM, Mario Kleiner wrote:
On 04/02/2015 01:34 PM, Chris Wilson wrote:
On vblank instant-off systems, we can get into a situation where the cost
of enabling and disabling the vblank IRQ around a drmWaitVblank query
dominates. However, we know that if the user wants the current vblank
counter, they are also very likely to immediately queue a vblank wait
and so we can keep the interrupt around and only turn it off if we have
no further vblank requests in the interrupt interval.
After vblank event delivery there is a shadow of one vblank where the
interrupt is kept alive for the user to query and queue another vblank
event. Similarly, if the user is using blocking drmWaitVblanks, the
interrupt will be disabled on the IRQ following the wait completion.
However, if the user is simply querying the current vblank counter and
timestamp, the interrupt will be disabled after every IRQ and the user
will enabled it again on the first query following the IRQ.
Testcase: igt/kms_vblank
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Cc: Michel Dänzer mic...@daenzer.net
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com,
Cc: Mario Kleiner mario.kleiner...@gmail.com
---
drivers/gpu/drm/drm_irq.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6f5dc18779e2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int
crtc)
if (atomic_dec_and_test(vblank-refcount)) {
if (drm_vblank_offdelay == 0)
return;
-else if (dev-vblank_disable_immediate || drm_vblank_offdelay
0)
+else if (drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
-else
+else if (!dev-vblank_disable_immediate)
mod_timer(vblank-disable_timer,
jiffies + ((drm_vblank_offdelay * HZ)/1000));
}
@@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev,
int crtc)
spin_lock_irqsave(dev-event_lock, irqflags);
You could move the code before the spin_lock_irqsave(dev-event_lock,
irqflags); i think it doesn't need that lock?
+if (dev-vblank_disable_immediate
!atomic_read(vblank-refcount)) {
Also check for (drm_vblank_offdelay 0) to make sure we have a way out
of instant disable here, and the same meaning of of drm_vblank_offdelay
like we have in the current implementation.
This hunk ...
+unsigned long vbl_lock_irqflags;
+
+spin_lock_irqsave(dev-vbl_lock, vbl_lock_irqflags);
+if (atomic_read(vblank-refcount) == 0 vblank-enabled) {
+DRM_DEBUG(disabling vblank on crtc %d\n, crtc);
+vblank_disable_and_save(dev, crtc);
+}
+spin_unlock_irqrestore(dev-vbl_lock, vbl_lock_irqflags);
... is the same as a call to vblank_disable_fn((unsigned long) vblank);
Maybe replace by that call?
You could also return here already, as the code below will just take a
lock, realize vblanks are now disabled and then release the locks and exit.
+}
+
/* Need timestamp lock to prevent concurrent execution with
* vblank enable/disable, as this would cause inconsistent
* or corrupted timestamps and vblank counts.
I think the logic itself is fine and at least basic testing of the patch
on a Intel HD Ironlake didn't show problems, so with the above taken
into account it would have my slightly uneasy reviewed-by.
One thing that worries me a little bit about the disable inside vblank
irq are the potential races between the disable code and the display
engine which could cause really bad off-by-one errors for clients on a
imperfect driver. These races can only happen if vblank enable or
disable happens close to or inside the vblank. This approach lets the
instant disable happen exactly inside vblank when there is the highest
chance of triggering that condition.
This doesn't seem to be a problem for intel kms, but other drivers don't
have instant disable yet, so we don't know how well we could do it
there. Additionally things like dynamic power management tend to operate
inside vblank, sometimes with funny side effects to other stuff, e.g.,
dpm on AMD, as i remember from some long debug session with Michel and
Alex last summer where dpm played a role. Therefore it seems more safe
to me to avoid actions inside vblank that could be done outside. E.g.,
instead of doing the disable inside the vblank irq one could maybe just
schedule an exact timer to do the disable a few milliseconds later in
the middle of active scanout to avoid these potential issues?
-mario
After testing this, one more thing that would make sense is to move the
disable block at the end of drm_handle_vblank() instead of at the top.
Turns out
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 04/02/2015 01:34 PM, Chris Wilson wrote:
On vblank instant-off systems, we can get into a situation where the cost
of enabling and disabling the vblank IRQ around a drmWaitVblank query
dominates. However, we know that if the user wants the current vblank
counter, they are also very likely to immediately queue a vblank wait
and so we can keep the interrupt around and only turn it off if we have
no further vblank requests in the interrupt interval.
After vblank event delivery there is a shadow of one vblank where the
interrupt is kept alive for the user to query and queue another vblank
event. Similarly, if the user is using blocking drmWaitVblanks, the
interrupt will be disabled on the IRQ following the wait completion.
However, if the user is simply querying the current vblank counter and
timestamp, the interrupt will be disabled after every IRQ and the user
will enabled it again on the first query following the IRQ.
Testcase: igt/kms_vblank
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Cc: Michel Dänzer mic...@daenzer.net
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com,
Cc: Mario Kleiner mario.kleiner...@gmail.com
---
drivers/gpu/drm/drm_irq.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6f5dc18779e2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
if (atomic_dec_and_test(vblank-refcount)) {
if (drm_vblank_offdelay == 0)
return;
- else if (dev-vblank_disable_immediate || drm_vblank_offdelay
0)
+ else if (drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
- else
+ else if (!dev-vblank_disable_immediate)
mod_timer(vblank-disable_timer,
jiffies + ((drm_vblank_offdelay * HZ)/1000));
}
@@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
spin_lock_irqsave(dev-event_lock, irqflags);
You could move the code before the spin_lock_irqsave(dev-event_lock,
irqflags); i think it doesn't need that lock?
+ if (dev-vblank_disable_immediate !atomic_read(vblank-refcount)) {
Also check for (drm_vblank_offdelay 0) to make sure we have a way out
of instant disable here, and the same meaning of of drm_vblank_offdelay
like we have in the current implementation.
This hunk ...
+ unsigned long vbl_lock_irqflags;
+
+ spin_lock_irqsave(dev-vbl_lock, vbl_lock_irqflags);
+ if (atomic_read(vblank-refcount) == 0 vblank-enabled) {
+ DRM_DEBUG(disabling vblank on crtc %d\n, crtc);
+ vblank_disable_and_save(dev, crtc);
+ }
+ spin_unlock_irqrestore(dev-vbl_lock, vbl_lock_irqflags);
... is the same as a call to vblank_disable_fn((unsigned long) vblank);
Maybe replace by that call?
You could also return here already, as the code below will just take a
lock, realize vblanks are now disabled and then release the locks and exit.
+ }
+
/* Need timestamp lock to prevent concurrent execution with
* vblank enable/disable, as this would cause inconsistent
* or corrupted timestamps and vblank counts.
I think the logic itself is fine and at least basic testing of the patch
on a Intel HD Ironlake didn't show problems, so with the above taken
into account it would have my slightly uneasy reviewed-by.
One thing that worries me a little bit about the disable inside vblank
irq are the potential races between the disable code and the display
engine which could cause really bad off-by-one errors for clients on a
imperfect driver. These races can only happen if vblank enable or
disable happens close to or inside the vblank. This approach lets the
instant disable happen exactly inside vblank when there is the highest
chance of triggering that condition.
This doesn't seem to be a problem for intel kms, but other drivers don't
have instant disable yet, so we don't know how well we could do it
there. Additionally things like dynamic power management tend to operate
inside vblank, sometimes with funny side effects to other stuff, e.g.,
dpm on AMD, as i remember from some long debug session with Michel and
Alex last summer where dpm played a role. Therefore it seems more safe
to me to avoid actions inside vblank that could be done outside. E.g.,
instead of doing the disable inside the vblank irq one could maybe just
schedule an exact timer to do the disable a few milliseconds later in
the middle of active scanout to avoid these potential issues?
-mario
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On Fri, Apr 03, 2015 at 11:20:20AM +0900, Michel Dänzer wrote: On 02.04.2015 20:34, Chris Wilson wrote: On vblank instant-off systems, we can get into a situation where the cost of enabling and disabling the vblank IRQ around a drmWaitVblank query dominates. However, we know that if the user wants the current vblank counter, they are also very likely to immediately queue a vblank wait and so we can keep the interrupt around and only turn it off if we have no further vblank requests in the interrupt interval. After vblank event delivery there is a shadow of one vblank where the interrupt is kept alive for the user to query and queue another vblank event. Similarly, if the user is using blocking drmWaitVblanks, the interrupt will be disabled on the IRQ following the wait completion. However, if the user is simply querying the current vblank counter and timestamp, the interrupt will be disabled after every IRQ and the user will enabled it again on the first query following the IRQ. As I mentioned before, it might not be too hard to make querying the current counter work without enabling the interrupt. But this looks like a step in the right direction. I honestly chickened out in case I broke something! Hindsight says both are useful as currently with instant-off we will disable the vblank interrupt inside the IRQ handler delivering the event, whereas we can save quite a bit of pain by waiting for the next IRQ before doing the disable (culmulatively saving a lot more CPU cycles over the course of swap chain than the extra IRQ will cost). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 02.04.2015 20:34, Chris Wilson wrote: On vblank instant-off systems, we can get into a situation where the cost of enabling and disabling the vblank IRQ around a drmWaitVblank query dominates. However, we know that if the user wants the current vblank counter, they are also very likely to immediately queue a vblank wait and so we can keep the interrupt around and only turn it off if we have no further vblank requests in the interrupt interval. After vblank event delivery there is a shadow of one vblank where the interrupt is kept alive for the user to query and queue another vblank event. Similarly, if the user is using blocking drmWaitVblanks, the interrupt will be disabled on the IRQ following the wait completion. However, if the user is simply querying the current vblank counter and timestamp, the interrupt will be disabled after every IRQ and the user will enabled it again on the first query following the IRQ. As I mentioned before, it might not be too hard to make querying the current counter work without enabling the interrupt. But this looks like a step in the right direction. Acked-by: Michel Dänzer michel.daen...@amd.com -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On vblank instant-off systems, we can get into a situation where the cost
of enabling and disabling the vblank IRQ around a drmWaitVblank query
dominates. However, we know that if the user wants the current vblank
counter, they are also very likely to immediately queue a vblank wait
and so we can keep the interrupt around and only turn it off if we have
no further vblank requests in the interrupt interval.
After vblank event delivery there is a shadow of one vblank where the
interrupt is kept alive for the user to query and queue another vblank
event. Similarly, if the user is using blocking drmWaitVblanks, the
interrupt will be disabled on the IRQ following the wait completion.
However, if the user is simply querying the current vblank counter and
timestamp, the interrupt will be disabled after every IRQ and the user
will enabled it again on the first query following the IRQ.
Testcase: igt/kms_vblank
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Cc: Michel Dänzer mic...@daenzer.net
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com,
Cc: Mario Kleiner mario.kleiner...@gmail.com
---
drivers/gpu/drm/drm_irq.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6f5dc18779e2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
if (atomic_dec_and_test(vblank-refcount)) {
if (drm_vblank_offdelay == 0)
return;
- else if (dev-vblank_disable_immediate || drm_vblank_offdelay
0)
+ else if (drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
- else
+ else if (!dev-vblank_disable_immediate)
mod_timer(vblank-disable_timer,
jiffies + ((drm_vblank_offdelay * HZ)/1000));
}
@@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
spin_lock_irqsave(dev-event_lock, irqflags);
+ if (dev-vblank_disable_immediate !atomic_read(vblank-refcount)) {
+ unsigned long vbl_lock_irqflags;
+
+ spin_lock_irqsave(dev-vbl_lock, vbl_lock_irqflags);
+ if (atomic_read(vblank-refcount) == 0 vblank-enabled) {
+ DRM_DEBUG(disabling vblank on crtc %d\n, crtc);
+ vblank_disable_and_save(dev, crtc);
+ }
+ spin_unlock_irqrestore(dev-vbl_lock, vbl_lock_irqflags);
+ }
+
/* Need timestamp lock to prevent concurrent execution with
* vblank enable/disable, as this would cause inconsistent
* or corrupted timestamps and vblank counts.
--
2.1.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6120 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 272/272 272/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW -1 361/361 360/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(12) FAIL(1)PASS(1) *HSW igt@gem_storedw_loop_blt PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
