On 11/12/15 11:33, 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 <chris@xxxxxxxxxxxxxxxxxx>
---
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(-)
This looks worthwhile, and most of it is obviously correct, just being a
simple transformation. But it looks like pc_render_add_request()
actually changes functionality somewhat:
@@ -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;
/* For Ironlake, MI_USER_INTERRUPT was deprecated and apparently
@@ -1455,11 +1451,12 @@ pc_render_add_request(struct drm_i915_gem_request *req)
if (ret)
return ret;
- 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);
+ intel_ring_emit(ring, addr | PIPE_CONTROL_GLOBAL_GTT);
+ intel_ring_emit(ring, req->seqno);
intel_ring_emit(ring, 0);
PIPE_CONTROL_FLUSH(ring, scratch_addr);
scratch_addr += 2 * CACHELINE_BYTES; /* write to separate cachelines */
@@ -1473,12 +1470,12 @@ pc_render_add_request(struct drm_i915_gem_request *req)
scratch_addr += 2 * CACHELINE_BYTES;
PIPE_CONTROL_FLUSH(ring, scratch_addr);
- intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) | PIPE_CONTROL_QW_WRITE |
+ intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) |
+ PIPE_CONTROL_QW_WRITE |
PIPE_CONTROL_WRITE_FLUSH |
- PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
PIPE_CONTROL_NOTIFY);
- 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, addr | PIPE_CONTROL_GLOBAL_GTT);
+ intel_ring_emit(ring, req->seqno);
intel_ring_emit(ring, 0);
__intel_ring_advance(ring);
So previously, we wrote the seqno and other dummy data into the SCRATCH
page, whereas now it's going to the HWSP. Does that make the scratch
page redundant? Or are there other bits of code that write other stuff
into it?
Then, the new code not only writes in dword I915_GEM_HWS_INDEX of the
HWSP, but also at multiple cache lines above this address. Do those
write have to be to the same page as the seqno, or could they still go
to the scratch page?
If the latter, I think we'd need to flag this in a big comment above the
definition of I915_GEM_HWS_INDEX, because it's not just defining a
single word as it appears, but rather a large block that will be
scribbled on.
(Note: TDR and the preemption code each require some extra locations in
the HWSP; they mustn't be scribbled over with dummy data).
Finally, we seem to have lost the PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE
bits; was that deliberate? And should it have been in a separate patch?
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
