Re: [Intel-gfx] [PATCH] drm/i915/guc: Use correct context lock when callig clr_context_registered

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Dec 10, 2021 at 08:41:22AM +0000, Tvrtko Ursulin wrote:
> 
> On 09/12/2021 19:14, Daniele Ceraolo Spurio wrote:
> > 
> > 
> > On 12/9/2021 10:48 AM, Matthew Brost wrote:
> > > s/ce/cn/ when grabbing guc_state.lock before calling
> > > clr_context_registered.
> > > 
> > > Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking")
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > Cc: <stable@vger.kernel.org>
> 
> I think Cc: stable is not needed here:
> 
> $ git tag --contains 0f7976506de61
> drm-intel-fixes-2021-11-18
> drm-intel-gt-next-2021-10-08
> drm-intel-gt-next-2021-10-21
> drm-intel-gt-next-2021-11-22
> drm-intel-next-2021-10-15
> drm-intel-next-fixes-2021-11-09
> v5.16-rc1
> v5.16-rc2
> v5.16-rc3
> v5.16-rc4
> 
> So still can hit 5.16 via fixes. Rodrigo, did I get this right and you will
> be able to pick it up next week or so?
> 

Will remove.

> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > 
> > I'm assuming we didn't see any splat from the lockdep assert in
> > clr_context_registered in our CI runs because we never hit this case as
> > it requires 64k+ contexts. Maybe we can add a selftest to purposely
> > exercise this path? Not a blocker for merging this fix.
> 
> Was the bug found by inspection or reported?
>

Internal testing.
 
> Given the buggy function is called steal_guc_id, so if the implication is
> there is no testing for guc id stealing, then it indeed please add some
> coverage ASAP.
>

Will do. I'll aim to get something out next week.

Matt
 
> Regards,
> 
> Tvrtko
> 
> > 
> > Daniele
> > 
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > index 1f9d4fde421f..9b7b4f4e0d91 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc,
> > > struct intel_context *ce)
> > >           list_del_init(&cn->guc_id.link);
> > >           ce->guc_id = cn->guc_id;
> > > -        spin_lock(&ce->guc_state.lock);
> > > +        spin_lock(&cn->guc_state.lock);
> > >           clr_context_registered(cn);
> > > -        spin_unlock(&ce->guc_state.lock);
> > > +        spin_unlock(&cn->guc_state.lock);
> > >           set_context_guc_id_invalid(cn);
> >