Re: [PATCH 10/19] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use

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

On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote:
> The code to switch_mm() is already handled by i915_switch_context(), the
> only difference required to setup the aliasing ppgtt is that we need to
> emit te switch_mm() on the first context, i.e. when transitioning from
> engine->last_context == NULL. This allows us to defer the
> initialisation of the GPU from early device initialisation to first use,
> which should marginally speed up both. The caveat is that we then defer
> the context initialisation until first use - i.e. we cannot assume that
> the GPU engines are initialised. For example, this means that power
> contexts for rc6 (Ironlake) need to explicitly loaded, as they are.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

Some comments below.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index aafcb4942acf..14c9b29294c5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4910,34 +4910,6 @@ i915_gem_init_hw(struct drm_device *dev)
>  	if (ret)
>  		goto out;

This whole if (ret) goto out can be removed as out is right after the
removed code block.

>  
> -	/* Now it is safe to go back round and do everything else: */
> -	for_each_engine(engine, dev_priv) {
> -		struct drm_i915_gem_request *req;
> -
> -		req = i915_gem_request_alloc(engine, NULL);
> -		if (IS_ERR(req)) {
> -			ret = PTR_ERR(req);
> -			break;
> -		}
> -
> -		ret = i915_ppgtt_init_ring(req);
> -		if (ret)
> -			goto err_request;
> -
> -		ret = i915_gem_context_enable(req);
> -		if (ret)
> -			goto err_request;
> -
> -err_request:
> -		i915_add_request_no_flush(req);
> -		if (ret) {
> -			DRM_ERROR("Failed to enable %s, error=%d\n",
> -				  engine->name, ret);
> -			i915_gem_cleanup_engines(dev);
> -			break;
> -		}
> -	}
> -
>  out:
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e5b3d74f8222..4d376f984a8c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -431,27 +431,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	dev_priv->kernel_context = NULL;
>  }
> 
<SNIP>
>  
>  static bool
> -needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to)
> +needs_pd_load_pre(struct i915_hw_ppgtt *ppgtt,
> +		  struct intel_engine_cs *engine,
> +		  struct intel_context *to)
>  {
> -	if (!to->ppgtt)
> +	if (ppgtt == NULL)

Code checker will scream and ask for !ppgtt. And I'm pretty sure we
should not keep flip-flopping between two styles. Also, you're not
doing != NULL either to check if pointer is not NULL, but just if
(foo), so I do not see why to avoid if (!foo).

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