Re: [PATCH 1/2] drm/i915: kill intel_resume_prepare()

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

On Tue, 2014-10-28 at 11:43 -0200, Paulo Zanoni wrote:
> 2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Because, really, the abstraction is not working for us. It is nice for
> >> VLV, but doesn't add anything useful on SNB/HSW/BDW. We want to change
> >> this code due to a recently-discovered bug, but we can't seem to find
> >> a nice solution that repects the current abstraction. So let's kill
> >> intel_resume_prepare() and its friends, and add an equivalent
> >> implementation to both its callers.
> >>
> >> Also, look at the diffstat!
> >
> > The reason for intel_resume_prepare() and intel_suspend_complete() was
> > to contain platform dependent code in those and to share parts between
> > the system and runtime suspend code, see the discussion at [1].
> 
> Well, IMHO we are just pretending to "share" the code paths between S3
> and RPM because we still have the "bool rpm_resume" argument on the
> current code. IMHO this is one of the biggest reasons why the code
> became so complex: take a look at the second version of my patch, it
> added even more "if (rpm_resume)" checks.

Yes, but that's one more thing we could try to remove at one point. For
example intel_init_pch_refclk() in snb_resume_prepare() could be just as
well called for both paths.

> I know that adding more
> IS_VLV checks is not cool, but the S3 and RPM code paths are very
> different here, and the VLV code even requires a really-weird
> ordering. I _really_ think the code becomes much easier to
> read/understand/modify by just killing intel_resume_prepare().
> 
> > I still
> > think this is a good idea, but I admit we need to work on it more, by
> > sharing more between the two paths. So for example instead of doing this
> > revert now I would consider calling intel_uncore_early_sanitize() for
> > both system and runtime resume.
> 
> That still wouldn't solve the ordering problem we have on S3. My goal
> is just to fix a very simple bug with our function ordering...

I think it would by having in intel_resume_prepare():

if (IS_VALLEYVIEW(dev_priv))
	ret = vlv_resume_prepare(dev_priv, rpm_resume);
intel_uncore_early_sanitize(dev,true);
if (IS_HASWELL(dev_priv) ||IS_BROADWELL(dev_priv))
	hsw_disable_pc8(dev_priv);

and call it for both system and runtime resume.

> > But if that's not feasible and you want to go ahead with the removal
> > then please also remove intel_suspend_complete(), leaving it in would be
> > confusing imo.
> 
> I don't think so. It doesn't have all the "bool rpm_resume" confusion,
> there's no function ordering issues with it, and both the S3 and RPM
> versions are actually identical. I don't think killing it is relevant
> to the bug we're trying to fix here, and I also don't think that
> removing t would be an improvement to the code base.

It's confusing to call vlv_resume_prepare(), hsw_disable_pc8() from
i915_drm_resume_early() directly and have to jump over some wrappers to
find the corresponding vlv_suspend_complete(), hsw_enable_pc8() in
i915_drm_suspend_complete(). But this is somewhat bikeshedding, so
either way feel free to add on both patches:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> >
> > --Imre
> >
> > [1]
> > http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.html
> >
> >>
> >> v2: - Rebase.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++-------------------------------
> >>  1 file changed, 15 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 035ec94..33b6fc4 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -551,8 +551,8 @@ static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> >>  }
> >>
> >>  static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> >> -static int intel_resume_prepare(struct drm_i915_private *dev_priv,
> >> -                             bool rpm_resume);
> >> +static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
> >> +                           bool rpm_resume);
> >>
> >>  static int i915_drm_suspend(struct drm_device *dev)
> >>  {
> >> @@ -744,7 +744,7 @@ static int i915_drm_resume(struct drm_device *dev)
> >>  static int i915_drm_resume_early(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >> -     int ret;
> >> +     int ret = 0;
> >>
> >>       /*
> >>        * We have a resume ordering issue with the snd-hda driver also
> >> @@ -760,7 +760,10 @@ static int i915_drm_resume_early(struct drm_device *dev)
> >>
> >>       pci_set_master(dev->pdev);
> >>
> >> -     ret = intel_resume_prepare(dev_priv, false);
> >> +     if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> +             hsw_disable_pc8(dev_priv);
> >> +     else if (IS_VALLEYVIEW(dev_priv))
> >> +             ret = vlv_resume_prepare(dev_priv, false);
> >>       if (ret)
> >>               DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
> >>
> >> @@ -986,25 +989,6 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
> >>       return 0;
> >>  }
> >>
> >> -static int snb_resume_prepare(struct drm_i915_private *dev_priv,
> >> -                             bool rpm_resume)
> >> -{
> >> -     struct drm_device *dev = dev_priv->dev;
> >> -
> >> -     if (rpm_resume)
> >> -             intel_init_pch_refclk(dev);
> >> -
> >> -     return 0;
> >> -}
> >> -
> >> -static int hsw_resume_prepare(struct drm_i915_private *dev_priv,
> >> -                             bool rpm_resume)
> >> -{
> >> -     hsw_disable_pc8(dev_priv);
> >> -
> >> -     return 0;
> >> -}
> >> -
> >>  /*
> >>   * Save all Gunit registers that may be lost after a D3 and a subsequent
> >>   * S0i[R123] transition. The list of registers needing a save/restore is
> >> @@ -1462,7 +1446,7 @@ static int intel_runtime_resume(struct device *device)
> >>       struct pci_dev *pdev = to_pci_dev(device);
> >>       struct drm_device *dev = pci_get_drvdata(pdev);
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >> -     int ret;
> >> +     int ret = 0;
> >>
> >>       if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))
> >>               return -ENODEV;
> >> @@ -1472,7 +1456,13 @@ static int intel_runtime_resume(struct device *device)
> >>       intel_opregion_notify_adapter(dev, PCI_D0);
> >>       dev_priv->pm.suspended = false;
> >>
> >> -     ret = intel_resume_prepare(dev_priv, true);
> >> +     if (IS_GEN6(dev_priv))
> >> +             intel_init_pch_refclk(dev);
> >> +     else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> +             hsw_disable_pc8(dev_priv);
> >> +     else if (IS_VALLEYVIEW(dev_priv))
> >> +             ret = vlv_resume_prepare(dev_priv, true);
> >> +
> >>       /*
> >>        * No point of rolling back things in case of an error, as the best
> >>        * we can do is to hope that things will still work (and disable RPM).
> >> @@ -1510,29 +1500,6 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> >>       return ret;
> >>  }
> >>
> >> -/*
> >> - * This function implements common functionality of runtime and system
> >> - * resume sequence. Variable rpm_resume used for implementing different
> >> - * code paths.
> >> - */
> >> -static int intel_resume_prepare(struct drm_i915_private *dev_priv,
> >> -                             bool rpm_resume)
> >> -{
> >> -     struct drm_device *dev = dev_priv->dev;
> >> -     int ret;
> >> -
> >> -     if (IS_GEN6(dev))
> >> -             ret = snb_resume_prepare(dev_priv, rpm_resume);
> >> -     else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >> -             ret = hsw_resume_prepare(dev_priv, rpm_resume);
> >> -     else if (IS_VALLEYVIEW(dev))
> >> -             ret = vlv_resume_prepare(dev_priv, rpm_resume);
> >> -     else
> >> -             ret = 0;
> >> -
> >> -     return ret;
> >> -}
> >> -
> >>  static const struct dev_pm_ops i915_pm_ops = {
> >>       /*
> >>        * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
> >
> >
> 
> 
> 


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