Re: [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote:
> On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble at intel.com>
> 
> The change is substantial enough that you should add a commit message
> explaining what it fixes and how. There are further useful guidelines on
> this topic in Documentation/SubmittingPatches.
> 
> In the future, you would make the review (and a possible bisection)
> easier if you split this patch into a refactoring patch without
> functional change and one that fixes the issue.
Thanks for the review Imre, Daniel.
Agree that I need to split this patch and give more description for the
changes. Will fix other reviewed code as well.
> 
> > v1:
> > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> > PM suspend and resume path similar to runtime suspend and resume.
> > 
> > v2:
> > 1. Keeping GT access, wake, gunit save/restore related helpers static.
> > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> > 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> > 
> > v3:
> > 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from
> > D0i3.
> > 2. Changed commit header.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Goel, Akash <akash.goel@intel.com>
> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------------
> >  1 file changed, 85 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ec96f9a..4440722 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +
> > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
> > +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend);
> 
> Nitpick: Something like intel_suspend_complete, intel_resume_prepare
> would be more descriptive names.
> 
> > +
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
> >  static int i915_drm_thaw_early(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> 
> The initialization here is redundant and could suppress complier
> warnings. There are also a couple more instances below.
>  
> > -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -		hsw_disable_pc8(dev_priv);
> > +	/* Restore any platform specific registers/clk state */
> > +	ret = post_hw_resume_init(dev_priv, false);
> 
> You could print an error message here, noting that we continue resuming
> despite the error.
> 
> >  	intel_uncore_early_sanitize(dev, true);
> >  	intel_uncore_sanitize(dev);
> >  	intel_power_domains_init_hw(dev_priv);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * We have a suspedn ordering issue with the snd-hda driver also
> > @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> >  
> > -	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
> > -		hsw_enable_pc8(dev_priv);
> > +	/* Save any platform specific registers/clk state needed post resume */
> > +	ret = pre_hw_suspend_deinit(dev_priv);
> >  
> >  	pci_disable_device(pdev);
> >  	pci_set_power_state(pdev, PCI_D3hot);
> 
> In case of error the suspend will be canceled and the resume handler
> will be called, so we shouldn't disable the device.
> 
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int i915_pm_resume_early(struct device *dev)
> > @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int hsw_suspend(struct drm_i915_private *dev_priv)
> 
> Based on the above nitpick something like hsw_suspend_prepare would be
> better. The same goes for the other platform handlers.
> 
> >  {
> >  	hsw_enable_pc8(dev_priv);
> >  
> >  	return 0;
> >  }
> >  
> > -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int snb_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> 
> Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more
> compact.
> 
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  
> > -	intel_init_pch_refclk(dev);
> > +	if (resume_from_rpm_suspend)
> > +		intel_init_pch_refclk(dev);
> >  
> >  	return 0;
> >  }
> >  
> > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int hsw_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> >  {
> >  	hsw_disable_pc8(dev_priv);
> >  
> > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> >  }
> >  
> > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int vlv_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 mask;
> > -	int err;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * Bspec defines the following GT well on flags as debug only, so
> > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	err = vlv_force_gfx_clock(dev_priv, true);
> > -	if (err)
> > +	ret = vlv_force_gfx_clock(dev_priv, true);
> > +	if (ret)
> >  		goto err1;
> >  
> > -	err = vlv_allow_gt_wake(dev_priv, false);
> > -	if (err)
> > +	ret = vlv_allow_gt_wake(dev_priv, false);
> > +	if (ret)
> >  		goto err2;
> >  	vlv_save_gunit_s0ix_state(dev_priv);
> >  
> > -	err = vlv_force_gfx_clock(dev_priv, false);
> > -	if (err)
> > +	ret = vlv_force_gfx_clock(dev_priv, false);
> > +	if (ret)
> >  		goto err2;
> > -
> > -	return 0;
> > +	return ret;
> >  
> >  err2:
> >  	/* For safety always re-enable waking and disable gfx clock forcing */
> > @@ -1332,14 +1341,15 @@ err2:
> >  err1:
> >  	vlv_force_gfx_clock(dev_priv, false);
> >  
> > -	return err;
> > +	return ret;
> >  }
> 
> In the above function I can't see any change besides renaming err to
> ret. There isn't much point in that imo.
> 
> >  
> > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int vlv_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	int err;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * If any of the steps fail just try to continue, that's the best we
> > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	intel_init_clock_gating(dev);
> > -	i915_gem_restore_fences(dev);
> > +	if (resume_from_rpm_suspend) {
> > +		intel_init_clock_gating(dev);
> > +		i915_gem_restore_fences(dev);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device)
> >  	cancel_work_sync(&dev_priv->rps.work);
> >  	intel_runtime_pm_disable_interrupts(dev);
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = 0;
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_suspend(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_suspend(dev_priv);
> > -	} else {
> > -		ret = -ENODEV;
> > -		WARN_ON(1);
> > -	}
> > +	/* Save any platform specific registers/clk state needed post resume */
> > +	ret = pre_hw_suspend_deinit(dev_priv);
> >  
> >  	if (ret) {
> >  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device)
> >  	intel_opregion_notify_adapter(dev, PCI_D0);
> >  	dev_priv->pm.suspended = false;
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = snb_runtime_resume(dev_priv);
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_resume(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_resume(dev_priv);
> > -	} else {
> > -		WARN_ON(1);
> > -		ret = -ENODEV;
> > -	}
> > +	/* Restore any platform specific registers/clk state */
> > +	ret = post_hw_resume_init(dev_priv, true);
> >  
> >  	/*
> >  	 * No point of rolling back things in case of an error, as the best
> > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device)
> >  	return ret;
> >  }
> >  
> > +/* This handler is used in system/runtime suspend path to reuse
> > + * Gfx clock, Wake control, Gunit state save related functionaility for VLV.
> > + */
> 
> The above comment is not precise, the function is used for all
> platforms. I think it'd be enough to mention that it's the common part
> of the runtime and system suspend sequence.
> 
> > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret = 0;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = 0;
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_suspend(dev_priv);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_suspend(dev_priv);
> > +	} else {
> > +		ret = -ENODEV;
> > +		WARN_ON(1);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* This handler is used in system/runtime resume path to reuse
> > + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV.
> > + */
> > +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret = 0;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = snb_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else {
> > +		WARN_ON(1);
> > +		ret = -ENODEV;
> > +	}
> > +	return ret;
> > +}
> > +
> >  static const struct dev_pm_ops i915_pm_ops = {
> >  	.suspend = i915_pm_suspend,
> >  	.suspend_late = i915_pm_suspend_late,
>