| Message ID | 20210726190800.26762-8-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc/slpc: Enable GuC based power management features | expand |
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? 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 >
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. 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 >>
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 > > >
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 */ }
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. 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(-)