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

[Matthew Auld <matthew.auld@intel.com>]

On 18/11/2021 06:57, Thomas Hellström wrote:
> On Wed, 2021-11-17 at 19:49 +0100, Thomas Hellström wrote:
>>
>> 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
> 
> Oh should this be
> 
> s/union/onion/ ?

Oops, will fixup when pushing :)

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