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@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
Some comments below.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
