On 27/07/16 11:00, 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);
Using the vfunc directly is consistent with elsewhere.
-Chris
I don't mind the naked vfunc if the call looks simple enough, as the
latter does. It's when the vfunc parameters aren't sufficiently simple
and obvious that it needs wrapping up.
So let's go with the second option :)
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
