[PATCH 0/2] Enabling D0i3 transition for Valleyview

[PATCH 0/2] Enabling D0i3 transition for Valleyview

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

With these patches runtime_suspend is triggered by updating runtime pm
usage count when display well is power gated and pci state is set to D3 hot
in runtime_suspend and set to D0 in runtime_resume.

Sagar Kamble (2):
  drm/i915: do runtime_get/put during display well power gate/ungate
  drm/i915/vlv: Set D3_hot for vlv during runtime_suspend

 drivers/gpu/drm/i915/i915_drv.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_pm.c |  6 ++++++
 2 files changed, 17 insertions(+)

-- 
1.8.5

[PATCH 1/2] drm/i915: do runtime_get/put during display well power gate/ungate

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

Display power island is on during boot, we have one count for it
once this power gates, we do a put making sure runtime_suspend is
called

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/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f83d1ff..b333aae 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6017,6 +6017,12 @@ void __vlv_set_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
 
+	if (PUNIT_POWER_WELL_DISP2D == power_well_id) {
+		if (enable)
+			intel_runtime_pm_get(dev_priv);
+		else
+			intel_runtime_pm_put(dev_priv);
+	}
 #undef COND
 
 out:
-- 
1.8.5

Re: [PATCH 1/2] drm/i915: do runtime_get/put during display well power gate/ungate

[Imre Deak]

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

Hi Sagar,

On Tue, 2014-06-10 at 00:27 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Display power island is on during boot, we have one count for it
> once this power gates, we do a put making sure runtime_suspend is
> called
> 
> 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/intel_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f83d1ff..b333aae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6017,6 +6017,12 @@ void __vlv_set_power_well(struct drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>  
> +	if (PUNIT_POWER_WELL_DISP2D == power_well_id) {
> +		if (enable)
> +			intel_runtime_pm_get(dev_priv);
> +		else
> +			intel_runtime_pm_put(dev_priv);
> +	}

The RPM refcount should already be get/put properly in
intel_display_power_get/put(), so the above doesn't seem correct to me.

With current -nightly after blanking the screen the RPM refcount does
drop to 0 for me, so I'm not sure what you're missing. One possibility
is:

# echo auto > /sys/bus/pci/devices/0000:00:02.0/power/control

--Imre


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

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

[sagar.a.kamble]

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);
+	}
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-- 
1.8.5

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

[Imre Deak]

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

Re: [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@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 ?
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;
> >  
> 

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

[Imre Deak]

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

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

[Sagar Arun Kamble]

This patch can be marked as "abandoned".
Have verified locally that pci driver is taking care of D0ix transitions
and state save/restore.

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

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

[Daniel Vetter]

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

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

[Sagar Arun Kamble]

On Wed, 2014-06-11 at 12:17 +0200, Daniel Vetter wrote:
> 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?
Took reference from suspend path in our previous repo where driver was
setting and saving the state. For S0iX, Gfx is supposed to go into
D3_hot, so we explicitly set that state assuming its gfx driver
responsibility.
We should have done more testing with current driver to ensure it is
indeed going to D3_hot.
> 
> 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
> > > 
> > > 
> > 
> > 
>