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

[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]. 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.

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.

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