On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@xxxxxxxxx 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@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Goel, Akash <akash.goel@xxxxxxxxx>
> 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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
