Re: [Intel-gfx] [PATCH 021/190] drm/i915: Use HWS for seqno tracking everywhere
On Tue, Jan 12, 2016 at 04:30:03PM +0200, Mika Kuoppala wrote: > Chris Wilson writes: > > > On Tue, Jan 12, 2016 at 12:05:06PM +0200, Mika Kuoppala wrote: > >> Chris Wilson writes: > >> > -intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) | > >> > PIPE_CONTROL_QW_WRITE | > >> > -PIPE_CONTROL_WRITE_FLUSH | > >> > -PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE); > >> > -intel_ring_emit(ring, ring->scratch.gtt_offset | > >> > PIPE_CONTROL_GLOBAL_GTT); > >> > -intel_ring_emit(ring, i915_gem_request_get_seqno(req)); > >> > +intel_ring_emit(ring, > >> > +GFX_OP_PIPE_CONTROL(4) | > >> > +PIPE_CONTROL_QW_WRITE | > >> > +PIPE_CONTROL_WRITE_FLUSH); > >> > >> Why no more PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE? > > > > I opened vim to add it back in and I coulnd't bring myself to commit > > that attrocity. > > I just noticed the asymmetry. Ilk doesn't need it? Here and now, we are doing 8 writes (1 seqno, 6 scratch, 1 seqno again for good luck) simply to try and flush the pipecontrol queue to ensure the seqno write lands before the notify interrupt is asserted. The TC invalidate on the first and only first write is superfluous - it imposes a top of pipe stall when we already have a bottom of pipe stall in the flush (the write will not occur until the pipeline is drained). We do the TC invalidate along with the reset of the cache invalidation before the next batch. -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 021/190] drm/i915: Use HWS for seqno tracking everywhere
Chris Wilson writes: > On Tue, Jan 12, 2016 at 12:05:06PM +0200, Mika Kuoppala wrote: >> Chris Wilson writes: >> > - intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) | PIPE_CONTROL_QW_WRITE | >> > - PIPE_CONTROL_WRITE_FLUSH | >> > - PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE); >> > - intel_ring_emit(ring, ring->scratch.gtt_offset | >> > PIPE_CONTROL_GLOBAL_GTT); >> > - intel_ring_emit(ring, i915_gem_request_get_seqno(req)); >> > + intel_ring_emit(ring, >> > + GFX_OP_PIPE_CONTROL(4) | >> > + PIPE_CONTROL_QW_WRITE | >> > + PIPE_CONTROL_WRITE_FLUSH); >> >> Why no more PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE? > > I opened vim to add it back in and I coulnd't bring myself to commit > that attrocity. I just noticed the asymmetry. Ilk doesn't need it? -Mika > -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 021/190] drm/i915: Use HWS for seqno tracking everywhere
On Tue, Jan 12, 2016 at 12:05:06PM +0200, Mika Kuoppala wrote: > Chris Wilson writes: > > - intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) | PIPE_CONTROL_QW_WRITE | > > - PIPE_CONTROL_WRITE_FLUSH | > > - PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE); > > - intel_ring_emit(ring, ring->scratch.gtt_offset | > > PIPE_CONTROL_GLOBAL_GTT); > > - intel_ring_emit(ring, i915_gem_request_get_seqno(req)); > > + intel_ring_emit(ring, > > + GFX_OP_PIPE_CONTROL(4) | > > + PIPE_CONTROL_QW_WRITE | > > + PIPE_CONTROL_WRITE_FLUSH); > > Why no more PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE? I opened vim to add it back in and I coulnd't bring myself to commit that attrocity. -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 021/190] drm/i915: Use HWS for seqno tracking everywhere
Chris Wilson writes:
> By using the same address for storing the HWS on every platform, we can
> remove the platform specific vfuncs and reduce the get-seqno routine to
> a single read of a cached memory location.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 10 ++--
> drivers/gpu/drm/i915/i915_drv.h | 4 +-
> drivers/gpu/drm/i915/i915_gpu_error.c| 2 +-
> drivers/gpu/drm/i915/i915_irq.c | 4 +-
> drivers/gpu/drm/i915/i915_trace.h| 2 +-
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 4 +-
> drivers/gpu/drm/i915/intel_lrc.c | 46 ++---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 86
>
> drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +--
> 9 files changed, 43 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d09e48455dcb..5a706c700684 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -600,7 +600,7 @@ static int i915_gem_pageflip_info(struct seq_file *m,
> void *data)
> ring->name,
>
> i915_gem_request_get_seqno(work->flip_queued_req),
> dev_priv->next_seqno,
> -ring->get_seqno(ring),
> +intel_ring_get_seqno(ring),
>
> i915_gem_request_completed(work->flip_queued_req));
> } else
> seq_printf(m, "Flip not associated with any
> ring\n");
> @@ -732,10 +732,8 @@ static void i915_ring_seqno_info(struct seq_file *m,
> {
> struct rb_node *rb;
>
> - if (ring->get_seqno) {
> - seq_printf(m, "Current sequence (%s): %x\n",
> -ring->name, ring->get_seqno(ring));
> - }
> + seq_printf(m, "Current sequence (%s): %x\n",
> +ring->name, intel_ring_get_seqno(ring));
>
> spin_lock(&ring->breadcrumbs.lock);
> for (rb = rb_first(&ring->breadcrumbs.waiters);
> @@ -1355,7 +1353,7 @@ static int i915_hangcheck_info(struct seq_file *m, void
> *unused)
>
> for_each_ring(ring, dev_priv, i) {
> acthd[i] = intel_ring_get_active_head(ring);
> - seqno[i] = ring->get_seqno(ring);
> + seqno[i] = intel_ring_get_seqno(ring);
> }
>
> i915_get_extra_instdone(dev, instdone);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 44d46018ee13..fcedcbc50834 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2971,13 +2971,13 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
>
> static inline bool i915_gem_request_started(struct drm_i915_gem_request *req)
> {
> - return i915_seqno_passed(req->ring->get_seqno(req->ring),
> + return i915_seqno_passed(intel_ring_get_seqno(req->ring),
>req->previous_seqno);
> }
>
> static inline bool i915_gem_request_completed(struct drm_i915_gem_request
> *req)
> {
> - return i915_seqno_passed(req->ring->get_seqno(req->ring),
> + return i915_seqno_passed(intel_ring_get_seqno(req->ring),
>req->seqno);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 01d0206ca4dd..3e137fc701cf 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -903,7 +903,7 @@ static void i915_record_ring_state(struct drm_device *dev,
> ering->waiting = intel_engine_has_waiter(ring);
> ering->instpm = I915_READ(RING_INSTPM(ring->mmio_base));
> ering->acthd = intel_ring_get_active_head(ring);
> - ering->seqno = ring->get_seqno(ring);
> + ering->seqno = intel_ring_get_seqno(ring);
> ering->start = I915_READ_START(ring);
> ering->head = I915_READ_HEAD(ring);
> ering->tail = I915_READ_TAIL(ring);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d73669783045..627c7fb6aa9b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2903,7 +2903,7 @@ static int semaphore_passed(struct intel_engine_cs
> *ring)
> if (signaller->hangcheck.deadlock >= I915_NUM_RINGS)
> return -1;
>
> - if (i915_seqno_passed(signaller->get_seqno(signaller), seqno))
> + if (i915_seqno_passed(intel_ring_get_seqno(signaller), seqno))
> return 1;
>
> /* cursory check for an unkickable deadlock */
> @@ -3068,7 +3068,7 @@ static void i915_hangcheck_elapsed(struct work_struct
> *work)
> semaphore_clear_deadlocks(dev_priv);
>
> acthd = intel_ring_get_active_head(ring);
> - seqno = ring->get_seqno(ring);
> +
Re: [Intel-gfx] [PATCH 021/190] drm/i915: Use HWS for seqno tracking everywhere
On 11/01/16 09:16, Chris Wilson wrote:
By using the same address for storing the HWS on every platform, we can
remove the platform specific vfuncs and reduce the get-seqno routine to
a single read of a cached memory location.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_debugfs.c | 10 ++--
drivers/gpu/drm/i915/i915_drv.h | 4 +-
drivers/gpu/drm/i915/i915_gpu_error.c| 2 +-
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/i915/i915_trace.h| 2 +-
drivers/gpu/drm/i915/intel_breadcrumbs.c | 4 +-
drivers/gpu/drm/i915/intel_lrc.c | 46 ++---
drivers/gpu/drm/i915/intel_ringbuffer.c | 86
drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +--
9 files changed, 43 insertions(+), 122 deletions(-)
Generally I like this, except the changes to pc_render_add_request() (as
previously noted) ...
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 57ec21c5b1ab..c86d0e17d785 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1216,19 +1216,17 @@ static int gen8_rcs_signal(struct drm_i915_gem_request
*signaller_req,
return ret;
for_each_ring(waiter, dev_priv, i) {
- u32 seqno;
u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
continue;
- seqno = i915_gem_request_get_seqno(signaller_req);
intel_ring_emit(signaller, GFX_OP_PIPE_CONTROL(6));
intel_ring_emit(signaller, PIPE_CONTROL_GLOBAL_GTT_IVB |
PIPE_CONTROL_QW_WRITE |
PIPE_CONTROL_FLUSH_ENABLE);
intel_ring_emit(signaller, lower_32_bits(gtt_offset));
intel_ring_emit(signaller, upper_32_bits(gtt_offset));
- intel_ring_emit(signaller, seqno);
+ intel_ring_emit(signaller, signaller_req->seqno);
intel_ring_emit(signaller, 0);
intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
MI_SEMAPHORE_TARGET(waiter->id));
@@ -1257,18 +1255,16 @@ static int gen8_xcs_signal(struct drm_i915_gem_request
*signaller_req,
return ret;
for_each_ring(waiter, dev_priv, i) {
- u32 seqno;
u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
continue;
- seqno = i915_gem_request_get_seqno(signaller_req);
intel_ring_emit(signaller, (MI_FLUSH_DW + 1) |
MI_FLUSH_DW_OP_STOREDW);
intel_ring_emit(signaller, lower_32_bits(gtt_offset) |
MI_FLUSH_DW_USE_GTT);
intel_ring_emit(signaller, upper_32_bits(gtt_offset));
- intel_ring_emit(signaller, seqno);
+ intel_ring_emit(signaller, signaller_req->seqno);
intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
MI_SEMAPHORE_TARGET(waiter->id));
intel_ring_emit(signaller, 0);
@@ -1299,11 +1295,9 @@ static int gen6_signal(struct drm_i915_gem_request
*signaller_req,
i915_reg_t mbox_reg = signaller->semaphore.mbox.signal[i];
if (i915_mmio_reg_valid(mbox_reg)) {
- u32 seqno = i915_gem_request_get_seqno(signaller_req);
-
intel_ring_emit(signaller, MI_LOAD_REGISTER_IMM(1));
intel_ring_emit_reg(signaller, mbox_reg);
- intel_ring_emit(signaller, seqno);
+ intel_ring_emit(signaller, signaller_req->seqno);
}
}
@@ -1338,7 +1332,7 @@ gen6_add_request(struct drm_i915_gem_request *req)
intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
- intel_ring_emit(ring, i915_gem_request_get_seqno(req));
+ intel_ring_emit(ring, req->seqno);
intel_ring_emit(ring, MI_USER_INTERRUPT);
__intel_ring_advance(ring);
@@ -1440,7 +1434,9 @@ static int
pc_render_add_request(struct drm_i915_gem_request *req)
{
struct intel_engine_cs *ring = req->ring;
- u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
+ u32 addr = req->ring->status_page.gfx_addr +
+ (I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
+ u32 scratch_addr = addr;
int ret;
What I don't like is that this will now splat dummy writes across the
HWSP. I'd rather the HWSP was kept for communicating /useful/ messages
from the GPU to CPU, and the scratch page be used for dummy writes (as
[Intel-gfx] [PATCH 021/190] drm/i915: Use HWS for seqno tracking everywhere
By using the same address for storing the HWS on every platform, we can
remove the platform specific vfuncs and reduce the get-seqno routine to
a single read of a cached memory location.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_debugfs.c | 10 ++--
drivers/gpu/drm/i915/i915_drv.h | 4 +-
drivers/gpu/drm/i915/i915_gpu_error.c| 2 +-
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/i915/i915_trace.h| 2 +-
drivers/gpu/drm/i915/intel_breadcrumbs.c | 4 +-
drivers/gpu/drm/i915/intel_lrc.c | 46 ++---
drivers/gpu/drm/i915/intel_ringbuffer.c | 86
drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +--
9 files changed, 43 insertions(+), 122 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index d09e48455dcb..5a706c700684 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -600,7 +600,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void
*data)
ring->name,
i915_gem_request_get_seqno(work->flip_queued_req),
dev_priv->next_seqno,
- ring->get_seqno(ring),
+ intel_ring_get_seqno(ring),
i915_gem_request_completed(work->flip_queued_req));
} else
seq_printf(m, "Flip not associated with any
ring\n");
@@ -732,10 +732,8 @@ static void i915_ring_seqno_info(struct seq_file *m,
{
struct rb_node *rb;
- if (ring->get_seqno) {
- seq_printf(m, "Current sequence (%s): %x\n",
- ring->name, ring->get_seqno(ring));
- }
+ seq_printf(m, "Current sequence (%s): %x\n",
+ ring->name, intel_ring_get_seqno(ring));
spin_lock(&ring->breadcrumbs.lock);
for (rb = rb_first(&ring->breadcrumbs.waiters);
@@ -1355,7 +1353,7 @@ static int i915_hangcheck_info(struct seq_file *m, void
*unused)
for_each_ring(ring, dev_priv, i) {
acthd[i] = intel_ring_get_active_head(ring);
- seqno[i] = ring->get_seqno(ring);
+ seqno[i] = intel_ring_get_seqno(ring);
}
i915_get_extra_instdone(dev, instdone);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44d46018ee13..fcedcbc50834 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2971,13 +2971,13 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
static inline bool i915_gem_request_started(struct drm_i915_gem_request *req)
{
- return i915_seqno_passed(req->ring->get_seqno(req->ring),
+ return i915_seqno_passed(intel_ring_get_seqno(req->ring),
req->previous_seqno);
}
static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req)
{
- return i915_seqno_passed(req->ring->get_seqno(req->ring),
+ return i915_seqno_passed(intel_ring_get_seqno(req->ring),
req->seqno);
}
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 01d0206ca4dd..3e137fc701cf 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -903,7 +903,7 @@ static void i915_record_ring_state(struct drm_device *dev,
ering->waiting = intel_engine_has_waiter(ring);
ering->instpm = I915_READ(RING_INSTPM(ring->mmio_base));
ering->acthd = intel_ring_get_active_head(ring);
- ering->seqno = ring->get_seqno(ring);
+ ering->seqno = intel_ring_get_seqno(ring);
ering->start = I915_READ_START(ring);
ering->head = I915_READ_HEAD(ring);
ering->tail = I915_READ_TAIL(ring);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d73669783045..627c7fb6aa9b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2903,7 +2903,7 @@ static int semaphore_passed(struct intel_engine_cs *ring)
if (signaller->hangcheck.deadlock >= I915_NUM_RINGS)
return -1;
- if (i915_seqno_passed(signaller->get_seqno(signaller), seqno))
+ if (i915_seqno_passed(intel_ring_get_seqno(signaller), seqno))
return 1;
/* cursory check for an unkickable deadlock */
@@ -3068,7 +3068,7 @@ static void i915_hangcheck_elapsed(struct work_struct
*work)
semaphore_clear_deadlocks(dev_priv);
acthd = intel_ring_get_active_head(ring);
- seqno = ring->get_seqno(ring);
+ seqno = intel_ring_get_seqno(ring);
if (ring->hangcheck.seqno == seqno) {
if (ring_idle(ring, seqno)) {
d
