On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote:
> 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.
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@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,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
