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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
