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

[Dave Gordon]

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 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

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.