Re: [PATCH 07/15] drm/i915/guc/slpc: Remove BUG_ON in guc_submission_disable

From: Matthew Brost <matthew.brost@intel.com>
To: "Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 07/15] drm/i915/guc/slpc: Remove BUG_ON in guc_submission_disable
Date: Wed, 28 Jul 2021 01:06:13 +0000	[thread overview]
Message-ID: <20210728010613.GA50866@DUT151-ICLU.fm.intel.com> (raw)
In-Reply-To: <5c3a11b6-ca8c-cea0-7a94-e70e01ee9d2c@intel.com>

On Tue, Jul 27, 2021 at 06:01:18PM -0700, Belgaumkar, Vinay wrote:
> 
> 
> On 7/27/2021 5:20 PM, Matthew Brost wrote:
> > On Mon, Jul 26, 2021 at 12:07:52PM -0700, Vinay Belgaumkar wrote:
> > > The assumption when it was added was there would be no wakerefs
> > > held. However, if we fail to enable SLPC, we will still be
> > > holding a wakeref.
> > > 
> > 
> > So this is if intel_guc_slpc_enable() fails, right? Not seeing where the
> > wakeref is taken. It also seems wrong not to drop the wakeref before
> > calling intel_guc_submission_disable, hence the GEM_BUG_ON in this
> > function.
> > 
> > Can you explain this bit more?
> 
> I should change the desc a little. The BUG_ON assumed GT would not be awake
> i.e at shutdown, and there would be 0 GT_PM references. However, this
> slpc_enable is in gt_resume path (gt_init_hw calls uc_init_hw). Here,
> gt_pm_get reference is held, so it will result in BUG_ON when
> submission_disable is called.
> 

Ok, I see the code path. With a better commit message:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Thanks,
> Vinay.
> > 
> > Matt
> > 
> > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ----
> > >   1 file changed, 4 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 b6338742a594..48cbd800ca54 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -2523,10 +2523,6 @@ void intel_guc_submission_enable(struct intel_guc *guc)
> > >   void intel_guc_submission_disable(struct intel_guc *guc)
> > >   {
> > > -	struct intel_gt *gt = guc_to_gt(guc);
> > > -
> > > -	GEM_BUG_ON(gt->awake); /* GT should be parked first */
> > > -
> > >   	/* Note: By the time we're here, GuC may have already been reset */
> > >   }
> > > -- 
> > > 2.25.0
> > > 