Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch events
tmp = I915_READ(GEN8_GT_IIR(0));
if (tmp) {
ret = IRQ_HANDLED;
+
rcs = tmp GEN8_RCS_IRQ_SHIFT;
- bcs = tmp GEN8_BCS_IRQ_SHIFT;
+ ring = dev_priv-ring[RCS];
if (rcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[RCS]);
+ notify_ring(dev, ring);
+ if (rcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
+ gen8_handle_context_events(ring);
Handling the context events here can generate a new execlist submission,
which if a small enough workload, can finish and generate a new context
event
interrupt before we ack this interrupt.
When we ack this interrupt, we clear the new one too, loosing an interrupt.
Moving the
I915_WRITE(GEN8_GT_IIR(0), tmp);
to just inside the if (tmp) { conditional (or anywhere before this call)
fixes this
issue. There is no harm in acking the interrupt immediately as we have the
read stored in tmp.
-Original Message-
From: Daniel, Thomas
Sent: Monday, April 28, 2014 10:58 AM
To: Beckett, Robert; Mateo Lozano, Oscar; Barbalho, Rafael; Ewins, Jon
Subject: RE: Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch
events
Hi Bob,
Looks like a good catch, and a sensible fix.
Thomas.
I agree with Thomas. Will add to the next revision of the series.
Thanks!
Oscar
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch events
On 27/03/2014 18:00, oscar.ma...@intel.com wrote:
From: Thomas Daniel thomas.dan...@intel.com
Handle all context status events in the context status buffer on every
context switch interrupt. We only remove work from the execlist queue
after a context status buffer reports that it has completed and we only
attempt to schedule new contexts on interrupt when a previously submitted
context completes (unless no contexts are queued, which means the GPU is
free).
Signed-off-by: Thomas Daniel thomas.dan...@intel.com
v2: Unreferencing the context when we are freeing the request might free
the backing bo, which requires the struct_mutex to be grabbed, so defer
unreferencing and freeing to a bottom half.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +
drivers/gpu/drm/i915/i915_irq.c | 28 ++---
drivers/gpu/drm/i915/i915_lrc.c | 101 +++-
drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
5 files changed, 123 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2607664..4c8cf52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1679,6 +1679,8 @@ struct drm_i915_gem_request {
/** execlist queue entry for this request */
struct list_head execlist_link;
+ /** Struct to handle this request in the bottom half of an interrupt */
+ struct work_struct work;
};
struct drm_i915_file_private {
@@ -2344,6 +2346,7 @@ void gen8_gem_context_free(struct i915_hw_context *ctx);
int gen8_switch_context_queue(struct intel_engine *ring,
struct i915_hw_context *to,
u32 tail);
+void gen8_handle_context_events(struct intel_engine *ring);
/* i915_gem_evict.c */
int __must_check i915_gem_evict_something(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 56657b5..6e0f456 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1334,6 +1334,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device
*dev,
struct drm_i915_private *dev_priv,
u32 master_ctl)
{
+ struct intel_engine *ring;
u32 rcs, bcs, vcs, vecs;
uint32_t tmp = 0;
irqreturn_t ret = IRQ_NONE;
@@ -1342,14 +1343,21 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
tmp = I915_READ(GEN8_GT_IIR(0));
if (tmp) {
ret = IRQ_HANDLED;
+
rcs = tmp GEN8_RCS_IRQ_SHIFT;
- bcs = tmp GEN8_BCS_IRQ_SHIFT;
+ ring = dev_priv-ring[RCS];
if (rcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[RCS]);
+ notify_ring(dev, ring);
+ if (rcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
+ gen8_handle_context_events(ring);
Handling the context events here can generate a new execlist submission,
which if a small enough workload, can finish and generate a new context
event interrupt before we ack this interrupt.
When we ack this interrupt, we clear the new one too, loosing an interrupt.
Moving the
I915_WRITE(GEN8_GT_IIR(0), tmp);
to just inside the if (tmp) { conditional (or anywhere before this call)
fixes this issue. There is no harm in acking the interrupt immediately
as we have the read stored in tmp.
+
+ bcs = tmp GEN8_BCS_IRQ_SHIFT;
+ ring = dev_priv-ring[BCS];
if (bcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[BCS]);
- if ((rcs | bcs) GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
-DRM_DEBUG_DRIVER(TODO: Context switch\n);
+ notify_ring(dev, ring);
+ if (bcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
+ gen8_handle_context_events(ring);
+
I915_WRITE(GEN8_GT_IIR(0), tmp);
} else
DRM_ERROR(The master control interrupt lied (GT0)!\n);
@@ -1360,10 +1368,11 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
if (tmp) {
ret = IRQ_HANDLED;
vcs = tmp GEN8_VCS1_IRQ_SHIFT;
+ ring = dev_priv-ring[VCS];
if (vcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[VCS]);
+ notify_ring(dev, ring);
if (vcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
-
Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch events
It seems to be completely managed by SW, for SW (or, at least, it does not seem
to have any visible effect in the HW). But you are right, it is probably worth
updating.
-- Oscar
-Original Message-
From: Lespiau, Damien
Sent: Thursday, April 03, 2014 3:25 PM
To: Mateo Lozano, Oscar
Cc: intel-gfx@lists.freedesktop.org; Daniel, Thomas
Subject: Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch
events
On Thu, Mar 27, 2014 at 06:00:12PM +, oscar.ma...@intel.com wrote:
+void gen8_handle_context_events(struct intel_engine *ring) {
+ struct drm_i915_private *dev_priv = ring-dev-dev_private;
+ u32 status_pointer;
+ u8 read_pointer;
+ u8 write_pointer;
+ u32 status;
+ u32 status_id;
+ u32 submit_contexts = 0;
+
+ status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring));
+
+ read_pointer = ring-next_context_status_buffer;
+ write_pointer = status_pointer 0x07;
+ if (read_pointer write_pointer)
+ write_pointer += 6;
+
+ spin_lock(ring-execlist_lock);
+
+ while (read_pointer write_pointer) {
+ read_pointer++;
+ status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
+ (read_pointer % 6) * 8);
+ status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
+ (read_pointer % 6) * 8 + 4);
+
+ if (status GEN8_CTX_STATUS_ELEMENT_SWITCH) {
+ if (check_remove_request(ring, status_id))
+ submit_contexts++;
+ } else if (status GEN8_CTX_STATUS_COMPLETE) {
+ if (check_remove_request(ring, status_id))
+ submit_contexts++;
+ }
+ }
+
+ if (submit_contexts != 0)
+ gen8_switch_context_unqueue(ring);
+
+ spin_unlock(ring-execlist_lock);
+
+ WARN(submit_contexts 2, More than two context complete
events?\n);
+ ring-next_context_status_buffer = write_pointer % 6; }
I'm a bit suprised that we never update the read pointer in the
CONTEXT_STATUS_PTR when we consume entries from
CONTEXT_STATUS_BUF.
Are we sure this field isn't used by hw at all to figure out if the circular
buffer
has some free space?
--
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch events
On Thu, Mar 27, 2014 at 06:00:12PM +, oscar.ma...@intel.com wrote:
+void gen8_handle_context_events(struct intel_engine *ring)
+{
+ struct drm_i915_private *dev_priv = ring-dev-dev_private;
+ u32 status_pointer;
+ u8 read_pointer;
+ u8 write_pointer;
+ u32 status;
+ u32 status_id;
+ u32 submit_contexts = 0;
+
+ status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring));
+
+ read_pointer = ring-next_context_status_buffer;
+ write_pointer = status_pointer 0x07;
+ if (read_pointer write_pointer)
+ write_pointer += 6;
+
+ spin_lock(ring-execlist_lock);
+
+ while (read_pointer write_pointer) {
+ read_pointer++;
+ status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
+ (read_pointer % 6) * 8);
+ status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
+ (read_pointer % 6) * 8 + 4);
+
+ if (status GEN8_CTX_STATUS_ELEMENT_SWITCH) {
+ if (check_remove_request(ring, status_id))
+ submit_contexts++;
+ } else if (status GEN8_CTX_STATUS_COMPLETE) {
+ if (check_remove_request(ring, status_id))
+ submit_contexts++;
+ }
+ }
+
+ if (submit_contexts != 0)
+ gen8_switch_context_unqueue(ring);
+
+ spin_unlock(ring-execlist_lock);
+
+ WARN(submit_contexts 2, More than two context complete events?\n);
+ ring-next_context_status_buffer = write_pointer % 6;
+}
I'm a bit suprised that we never update the read pointer in the
CONTEXT_STATUS_PTR when we consume entries from CONTEXT_STATUS_BUF.
Are we sure this field isn't used by hw at all to figure out if the
circular buffer has some free space?
--
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 43/49] drm/i915/bdw: Handle context switch events
From: Thomas Daniel thomas.dan...@intel.com
Handle all context status events in the context status buffer on every
context switch interrupt. We only remove work from the execlist queue
after a context status buffer reports that it has completed and we only
attempt to schedule new contexts on interrupt when a previously submitted
context completes (unless no contexts are queued, which means the GPU is
free).
Signed-off-by: Thomas Daniel thomas.dan...@intel.com
v2: Unreferencing the context when we are freeing the request might free
the backing bo, which requires the struct_mutex to be grabbed, so defer
unreferencing and freeing to a bottom half.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +
drivers/gpu/drm/i915/i915_irq.c | 28 ++---
drivers/gpu/drm/i915/i915_lrc.c | 101 +++-
drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
5 files changed, 123 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2607664..4c8cf52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1679,6 +1679,8 @@ struct drm_i915_gem_request {
/** execlist queue entry for this request */
struct list_head execlist_link;
+ /** Struct to handle this request in the bottom half of an interrupt */
+ struct work_struct work;
};
struct drm_i915_file_private {
@@ -2344,6 +2346,7 @@ void gen8_gem_context_free(struct i915_hw_context *ctx);
int gen8_switch_context_queue(struct intel_engine *ring,
struct i915_hw_context *to,
u32 tail);
+void gen8_handle_context_events(struct intel_engine *ring);
/* i915_gem_evict.c */
int __must_check i915_gem_evict_something(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 56657b5..6e0f456 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1334,6 +1334,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device
*dev,
struct drm_i915_private *dev_priv,
u32 master_ctl)
{
+ struct intel_engine *ring;
u32 rcs, bcs, vcs, vecs;
uint32_t tmp = 0;
irqreturn_t ret = IRQ_NONE;
@@ -1342,14 +1343,21 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
tmp = I915_READ(GEN8_GT_IIR(0));
if (tmp) {
ret = IRQ_HANDLED;
+
rcs = tmp GEN8_RCS_IRQ_SHIFT;
- bcs = tmp GEN8_BCS_IRQ_SHIFT;
+ ring = dev_priv-ring[RCS];
if (rcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[RCS]);
+ notify_ring(dev, ring);
+ if (rcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
+ gen8_handle_context_events(ring);
+
+ bcs = tmp GEN8_BCS_IRQ_SHIFT;
+ ring = dev_priv-ring[BCS];
if (bcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[BCS]);
- if ((rcs | bcs) GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
-DRM_DEBUG_DRIVER(TODO: Context switch\n);
+ notify_ring(dev, ring);
+ if (bcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
+ gen8_handle_context_events(ring);
+
I915_WRITE(GEN8_GT_IIR(0), tmp);
} else
DRM_ERROR(The master control interrupt lied (GT0)!\n);
@@ -1360,10 +1368,11 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
if (tmp) {
ret = IRQ_HANDLED;
vcs = tmp GEN8_VCS1_IRQ_SHIFT;
+ ring = dev_priv-ring[VCS];
if (vcs GT_RENDER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[VCS]);
+ notify_ring(dev, ring);
if (vcs GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
-DRM_DEBUG_DRIVER(TODO: Context switch\n);
+ gen8_handle_context_events(ring);
I915_WRITE(GEN8_GT_IIR(1), tmp);
} else
DRM_ERROR(The master control interrupt lied (GT1)!\n);
@@ -1374,10 +1383,11 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
if (tmp) {
ret = IRQ_HANDLED;
vecs = tmp GEN8_VECS_IRQ_SHIFT;
+ ring = dev_priv-ring[VECS];
if (vecs
