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

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

On ma, 2016-07-25 at 08:44 +0100, Chris Wilson wrote:
> Space for flushing the GPU cache prior to completing the request is
> preallocated and so cannot fail.

Patch title and commit message have some disconnect. Could you explain
in a bit more detail what made gpu_caches_dirty obsolete?

Also, worth mentioning that after this change legacy/execlist flushing
code is unified (could be split patch too? With your "Now that ..."
reasoning).

Somebody not reviewing the series linearly might feel lost.

> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ca1d4f573832..048050176ff9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -998,10 +998,8 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
>  	if (flush_domains & I915_GEM_DOMAIN_GTT)
>  		wmb();
>  
> -	/* Unconditionally invalidate gpu caches and ensure that we do flush
> -	 * any residual writes from the previous batch.
> -	 */
> -	return intel_engine_invalidate_all_caches(req);
> +	/* Unconditionally invalidate gpu caches and TLBs. */

A nitpick, but maybe s/gpu/GPU/


> +	return req->engine->emit_flush(req, I915_GEM_GPU_DOMAINS, 0);
>  }
>  
>  static bool

<SNIP>

> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 942b5b1f1602..7b772d914e23 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -451,10 +451,9 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	 * what.
>  	 */
>  	if (flush_caches) {
> -		if (i915.enable_execlists)
> -			ret = logical_ring_flush_all_caches(request);
> -		else
> -			ret = intel_engine_flush_all_caches(request);
> +		ret = request->engine->emit_flush(request,
> +						  0, I915_GEM_GPU_DOMAINS);
> +
>  		/* Not allowed to fail! */
>  		WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);

Fix this message too.

Regards, Joonas
-- 
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