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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2016-07-27 at 11:00 +0100, Chris Wilson wrote:
> 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@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);

+1 on latter, much more informative.

Regards, Joonas

> 
> Using the vfunc directly is consistent with elsewhere.
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx