[Intel-gfx] [PATCH 17/31] drm/i915: Remove obsolete engine->gpu_caches_dirty

[Chris Wilson]

On Wed, Jul 27, 2016 at 10:49:46AM +0100, Dave Gordon wrote:
> On 25/07/16 08:44, Chris Wilson wrote:
> >Space for flushing the GPU cache prior to completing the request is
> >preallocated and so cannot fail.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_gem_context.c    |  2 +-
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c |  9 +---
> > drivers/gpu/drm/i915/i915_gem_gtt.c        | 11 +++--
> > drivers/gpu/drm/i915/i915_gem_request.c    |  7 ++-
> > drivers/gpu/drm/i915/intel_lrc.c           | 47 +++----------------
> > drivers/gpu/drm/i915/intel_lrc.h           |  2 -
> > drivers/gpu/drm/i915/intel_ringbuffer.c    | 72 +++++++-----------------------
> > drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 ---
> > 8 files changed, 37 insertions(+), 120 deletions(-)
> 
> [snip]
> 
> >-static int logical_ring_invalidate_all_caches(struct drm_i915_gem_request *req)
> >-{
> >-	struct intel_engine_cs *engine = req->engine;
> >-	uint32_t flush_domains;
> >-	int ret;
> >-
> >-	flush_domains = 0;
> >-	if (engine->gpu_caches_dirty)
> >-		flush_domains = I915_GEM_GPU_DOMAINS;
> >-
> >-	ret = engine->emit_flush(req, I915_GEM_GPU_DOMAINS, flush_domains);
> >-	if (ret)
> >-		return ret;
> >-
> >-	engine->gpu_caches_dirty = false;
> >-	return 0;
> >-}
> >-
> > static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
> > 				 struct list_head *vmas)
> > {
> >@@ -690,7 +672,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
> > 	/* Unconditionally invalidate gpu caches and ensure that we do flush
> > 	 * any residual writes from the previous batch.
> > 	 */
> >-	return logical_ring_invalidate_all_caches(req);
> >+	return req->engine->emit_flush(req, I915_GEM_GPU_DOMAINS, 0);
> > }
> 
> I don't think the direct call to the vfunc is as clear as to what
> we're trying to achieve here. I'd like some flavour of
> flush_caches() and invalidate_caches() reinstated, even if they're
> just trivial wrappers round the ->emit_flush().

> While we're here, could we simplify the parameters? AFAICT we need
> only three permutations: FLUSH (only), INVALIDATE (only) or FLUSH
> and INVALIDATE; and in each case each parameter is either
> GEM_GPU_DOMAINS or 0.

Yes, a couple of years ago I sent patches to reduce it down to a single
parameter, (INVALIDATE, FLUSH, BARRIER).

The choice now is which would you prefer

i915_gem_request_emit_flush() {
	req->engine->emit_flush(req, 0, I915_GEM_GPU_DOMAINS);
}
i915_gem_request_emit_invalidate() {
	req->engine->emit_flush(req, I915_GEM_GPU_DOMAINS, 0);
}

or

	engine->emit_flush(req, INVALIDATE);
	engine->emit_flush(req, FLUSH);

Using the vfunc directly is consistent with elsewhere.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre