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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 11, 2014 at 01:53:49PM +0530, Sagar Arun Kamble wrote:
> This patch can be marked as "abandoned".
> Have verified locally that pci driver is taking care of D0ix transitions
> and state save/restore.

That still leaves the question why you originally thought this is
required. What did blow up here in your testing?

Thanks, Daniel
> 
> Thanks Imre for the clarification.
> 
> On Tue, 2014-06-10 at 20:51 +0300, Imre Deak wrote:
> > On Tue, 2014-06-10 at 23:05 +0530, Sagar Arun Kamble wrote:
> > > On Tue, 2014-06-10 at 15:43 +0300, Imre Deak wrote:
> > > > 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(). 
> > > 
> > > Where exactly it calls pci_pm_set_powerstate to set D3_hot ?
> > 
> > It's
> > pci_pm_runtime_suspend()->pci_finish_runtime_suspend()->pci_set_power_state() .
> > 
> > > 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);
> > >         }
> > 
> > I don't think this is a bug, but something the PCI framework requires
> > from drivers. That is if they set the power state to something else than
> > PCI_D0 or PCI_UNKNOWN they also have to save the state themselves.
> > 
> > I can't check this right now, but I haven't ever seen the above WARN and
> > by the look of it we are doing the right thing in the driver. That is
> > leave the power state in D0 and let the PCI framework handle the state
> > saving/power state transitioning.
> > 
> > --Imre
> > 
> > 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch