[Intel-gfx] [PATCH 2/2] drm/i915/vlv: Set D3_hot for vlv during runtime_suspend

[Sagar Arun Kamble]

On Tue, 2014-06-10 at 15:43 +0300, Imre Deak wrote:
> On Tue, 2014-06-10 at 00:27 +0530, sagar.a.kamble at intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble at 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 at ffwll.ch> (supporter:INTEL DRM DRIVERS...)
> > Cc: Jani Nikula <jani.nikula at linux.intel.com> (supporter:INTEL DRM DRIVERS...)
> > Signed-off-by: Sagar Kamble <sagar.a.kamble at 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(). 

Where exactly it calls pci_pm_set_powerstate to set D3_hot ?
There seems to be bug in the pci driver in the suspend path. it is not
saving the pci configuration space although code for same exists.
pci driver checks if client driver has saved if not it will exit from
suspend path.
check the code snippet below:

        if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
            && pci_dev->current_state != PCI_UNKNOWN) {
                WARN_ONCE(pci_dev->current_state != prev,
                        "PCI PM: State of device not saved by %pF\n",
                        pm->runtime_suspend);
                return 0;
        }

        if (!pci_dev->state_saved) {
                pci_save_state(pci_dev);
                pci_finish_runtime_suspend(pci_dev);
        }

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