[Intel-gfx] [PATCH 1/2] drm/i915: kill intel_resume_prepare()
Paulo Zanoni
przanoni at gmail.com
Tue Oct 28 14:43:29 CET 2014
2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak at intel.com>:
> On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at 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. 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...
>
> 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.
>
> --Imre
>
> [1]
> http://lists.freedesktop.org/archives/intel-gfx/2014-August/050036.html
>
>>
>> v2: - Rebase.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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,
>
>
--
Paulo Zanoni
More information about the Intel-gfx
mailing list