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

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10556 bytes --]

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.

> 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,


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx