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@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);
+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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
