[Intel-gfx] [PATCH 18/32] drm/i915: Use HWS for seqno tracking everywhere

[Dave Gordon]

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 at chris-wilson.co.uk>
> ---
>   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.