Re: [PATCH v2 1/6] drm/i915: move the pre_pin earlier

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 11/17/21 15:20, Matthew Auld wrote:
> In intel_context_do_pin_ww, when calling into the pre_pin hook(which is
> passed the ww context) it could in theory return -EDEADLK(which is very
> likely with debug kernels), once we start adding more ww locking in there,
> like in the next patch. If so then we need to be mindful of having to
> restart the do_pin at this point.
>
> If this is the kernel_context, or some other early in-kernel context
> where we have yet to setup the default_state, then we always inhibit the
> context restore, and instead rely on the delayed active_release to set
> the CONTEXT_VALID_BIT for us(if we even care), which should indicate
> that we have context switched away, and that our newly saved context
> state should now be valid. However, since we currently grab the active
> reference before the potential ww dance, we can end up setting the
> CONTEXT_VALID_BIT much too early, if we need to backoff, and then upon
> re-trying the do_pin, we could potentially cause the hardware to
> incorrectly load some garbage context state when later context switching
> to that context, but at the very least this will trigger the
> GEM_BUG_ON() in __engine_unpark. For now let's just move any ww dance
> stuff prior to arming the active reference.
>
> For normal user contexts this shouldn't be a concern, since we should
> already have the default_state ready when initialising the lrc state,
> and so there should be no concern with active_release somehow
> prematurely setting the CONTEXT_VALID_BIT.
>
> v2(Thomas):
>    - Also re-order the union unwind
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>


> ---
>   drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 5634d14052bc..4c296de1d67d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -228,17 +228,17 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
>   	if (err)
>   		return err;
>   
> -	err = i915_active_acquire(&ce->active);
> +	err = ce->ops->pre_pin(ce, ww, &vaddr);
>   	if (err)
>   		goto err_ctx_unpin;
>   
> -	err = ce->ops->pre_pin(ce, ww, &vaddr);
> +	err = i915_active_acquire(&ce->active);
>   	if (err)
> -		goto err_release;
> +		goto err_post_unpin;
>   
>   	err = mutex_lock_interruptible(&ce->pin_mutex);
>   	if (err)
> -		goto err_post_unpin;
> +		goto err_release;
>   
>   	intel_engine_pm_might_get(ce->engine);
>   
> @@ -273,11 +273,11 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
>   
>   err_unlock:
>   	mutex_unlock(&ce->pin_mutex);
> +err_release:
> +	i915_active_release(&ce->active);
>   err_post_unpin:
>   	if (!handoff)
>   		ce->ops->post_unpin(ce);
> -err_release:
> -	i915_active_release(&ce->active);
>   err_ctx_unpin:
>   	intel_context_post_unpin(ce);
>