| Message ID | 1505929104-28823-5-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 20, 2017 at 11:08:19PM +0530, Sagar Arun Kamble wrote: > With this patch we disable GuC submission in i915_drm_suspend. This will > destroy the client which will be setup back again. We also reuse the > complete sanitization done via intel_uc_runtime_suspend in this path. > Post drm resume this state is recreated by intel_uc_init_hw hence we need > not have similar reuse for intel_uc_resume. > This also fixes issue where intel_uc_fini_hw was being called after GPU > reset happening in i915_gem_suspend in i915_driver_unload. > > v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex > protection for i915_guc_submission_disable. > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index fa698db..0c7e45c7 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > if (!i915.enable_guc_loading) > return; > > - if (i915.enable_guc_submission) > - i915_guc_submission_disable(dev_priv); > - > guc_disable_communication(&dev_priv->guc); > > if (i915.enable_guc_submission) { > @@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) > > int intel_uc_suspend(struct drm_i915_private *dev_priv) > { > - return intel_guc_enter_sleep(&dev_priv->guc); > + struct drm_device *dev = &dev_priv->drm; > + int ret; Drop the locals. I'd drop the dev, and just go through dev_priv in this case. > + > + if (i915.enable_guc_submission) { > + mutex_lock(&dev->struct_mutex); > + i915_guc_submission_disable(dev_priv); > + mutex_unlock(&dev->struct_mutex); > + } Since we're starting to use i915_guc_submission_disable from different places, some of which without struct_mutex already held (like this one), it would be a good idea to add a lockdep assert documenting this requirement inside both i915_guc_submission_enable and i915_guc_submission_disable. It could be even squeezed in with this patch IMHO. > + > + ret = intel_uc_runtime_suspend(dev_priv); > + if (ret) > + return ret; return intel_guc_runtime_suspend(dev_priv); With that: Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> -Michał > + > + return 0; > } > > int intel_uc_resume(struct drm_i915_private *dev_priv) > -- > 1.9.1 >
On 9/21/2017 10:16 PM, Michał Winiarski wrote: > On Wed, Sep 20, 2017 at 11:08:19PM +0530, Sagar Arun Kamble wrote: >> With this patch we disable GuC submission in i915_drm_suspend. This will >> destroy the client which will be setup back again. We also reuse the >> complete sanitization done via intel_uc_runtime_suspend in this path. >> Post drm resume this state is recreated by intel_uc_init_hw hence we need >> not have similar reuse for intel_uc_resume. >> This also fixes issue where intel_uc_fini_hw was being called after GPU >> reset happening in i915_gem_suspend in i915_driver_unload. >> >> v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex >> protection for i915_guc_submission_disable. >> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c >> index fa698db..0c7e45c7 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) >> if (!i915.enable_guc_loading) >> return; >> >> - if (i915.enable_guc_submission) >> - i915_guc_submission_disable(dev_priv); >> - >> guc_disable_communication(&dev_priv->guc); >> >> if (i915.enable_guc_submission) { >> @@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) >> >> int intel_uc_suspend(struct drm_i915_private *dev_priv) >> { >> - return intel_guc_enter_sleep(&dev_priv->guc); >> + struct drm_device *dev = &dev_priv->drm; >> + int ret; > Drop the locals. > I'd drop the dev, and just go through dev_priv in this case. Sure. Will update. > >> + >> + if (i915.enable_guc_submission) { >> + mutex_lock(&dev->struct_mutex); >> + i915_guc_submission_disable(dev_priv); >> + mutex_unlock(&dev->struct_mutex); >> + } > Since we're starting to use i915_guc_submission_disable from different places, > some of which without struct_mutex already held (like this one), it would be a > good idea to add a lockdep assert documenting this requirement inside both > i915_guc_submission_enable and i915_guc_submission_disable. > It could be even squeezed in with this patch IMHO. Sure. Will update. > >> + >> + ret = intel_uc_runtime_suspend(dev_priv); >> + if (ret) >> + return ret; > return intel_guc_runtime_suspend(dev_priv); Did you really mean intel_*guc*_runtime_suspend here or was typo? > > With that: > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > -Michał > >> + >> + return 0; >> } >> >> int intel_uc_resume(struct drm_i915_private *dev_priv) >> -- >> 1.9.1 >>
On Thu, Sep 21, 2017 at 10:25:50PM +0530, Sagar Arun Kamble wrote: [SNIP] > > > + ret = intel_uc_runtime_suspend(dev_priv); > > > + if (ret) > > > + return ret; > > return intel_guc_runtime_suspend(dev_priv); > Did you really mean intel_*guc*_runtime_suspend here or was typo? Typo - sorry about that. -Michał > > > > With that: > > > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > > > -Michał > > > > > + > > > + return 0; > > > } > > > int intel_uc_resume(struct drm_i915_private *dev_priv) > > > -- > > > 1.9.1 > > > >
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index fa698db..0c7e45c7 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) if (!i915.enable_guc_loading) return; - if (i915.enable_guc_submission) - i915_guc_submission_disable(dev_priv); - guc_disable_communication(&dev_priv->guc); if (i915.enable_guc_submission) { @@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) int intel_uc_suspend(struct drm_i915_private *dev_priv) { - return intel_guc_enter_sleep(&dev_priv->guc); + struct drm_device *dev = &dev_priv->drm; + int ret; + + if (i915.enable_guc_submission) { + mutex_lock(&dev->struct_mutex); + i915_guc_submission_disable(dev_priv); + mutex_unlock(&dev->struct_mutex); + } + + ret = intel_uc_runtime_suspend(dev_priv); + if (ret) + return ret; + + return 0; } int intel_uc_resume(struct drm_i915_private *dev_priv)
With this patch we disable GuC submission in i915_drm_suspend. This will destroy the client which will be setup back again. We also reuse the complete sanitization done via intel_uc_runtime_suspend in this path. Post drm resume this state is recreated by intel_uc_init_hw hence we need not have similar reuse for intel_uc_resume. This also fixes issue where intel_uc_fini_hw was being called after GPU reset happening in i915_gem_suspend in i915_driver_unload. v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex protection for i915_guc_submission_disable. Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)