Re: [PATCH 2/2] drm/i915/vlv: Set D3_hot for vlv during runtime_suspend

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

[-- Attachment #1.1: Type: text/plain, Size: 2119 bytes --]

On Tue, 2014-06-10 at 00:27 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> To do a platform wide S0i3 transition, Gfx is required to go
> to D3_hot state. pci_save_state and pci_restore_state needed to avoid ring
> hangs across D3_hot transitions.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> (supporter:INTEL DRM DRIVERS...)
> Cc: Jani Nikula <jani.nikula@linux.intel.com> (supporter:INTEL DRM DRIVERS...)
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5a08c86..70bb456 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1412,6 +1412,11 @@ static int intel_runtime_suspend(struct device *device)
>  	 * via the suspend path.
>  	 */
>  	intel_opregion_notify_adapter(dev, PCI_D1);
> +	if (IS_VALLEYVIEW(dev)) {
> +		pci_save_state(pdev);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, PCI_D3hot);
> +	}
>  
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
> @@ -1428,6 +1433,12 @@ static int intel_runtime_resume(struct device *device)
>  
>  	DRM_DEBUG_KMS("Resuming device\n");
>  
> +	if (IS_VALLEYVIEW(dev)) {
> +		pci_set_power_state(pdev, PCI_D0);
> +		pci_restore_state(pdev);
> +		pci_enable_device(pdev);
> +	}

Setting the proper Dx state and saving/restoring the PCI config space is
already done for us by the PCI runtime PM framework, see
pci_pm_runtime_suspend/resume(). They don't disable/enable the PCI
device, but I'm not sure if that's really needed. Based on the docs I
found so far the requirement for S0ix is that we put the device into D3
state and that's already the case w/o disabling the device.

So could you explain/confirm if we need that particular step?

Also if it's really needed it should be done for all platforms.

--Imre 


> +
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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