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

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

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/ ?


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