[PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[Jesse Barnes]

According to the PCI docs and Rafael, we need to be using
pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
teardown routines, rather than using a direct enable/disable pair (and
we didn't even have the enable side, so never autosuspended after an
unload).

This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
still debugging why the device never autosuspends.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 85c35fd..1addb8a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
 
 	/* Make sure we're not suspended first. */
 	pm_runtime_get_sync(device);
-	pm_runtime_disable(device);
+	pm_runtime_get_noresume(device);
 }
 
 /**
@@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
-	pm_runtime_set_active(device);
-
 	/*
 	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
 	 * requirement.
@@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	pm_runtime_use_autosuspend(device);
 
 	pm_runtime_put_autosuspend(device);
+	pm_runtime_put_noidle(device);
 }
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[Paulo Zanoni]

2015-09-17 18:23 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> According to the PCI docs and Rafael, we need to be using
> pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
> teardown routines, rather than using a direct enable/disable pair (and
> we didn't even have the enable side, so never autosuspended after an
> unload).
>
> This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
> still debugging why the device never autosuspends.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 85c35fd..1addb8a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>
>         /* Make sure we're not suspended first. */

So is the comment above still valid?

As far as I remember, we explicitly wake up the hardware after unload
because our driver is (was) not prepared to be loaded on a hardware
that is suspended. Did you try module reloading after this change?

Also, basic-pci-d3-state should not be unloading/reloading the driver,
so it's not clear to me how this change helps passing that test.

>         pm_runtime_get_sync(device);
> -       pm_runtime_disable(device);
> +       pm_runtime_get_noresume(device);
>  }
>
>  /**
> @@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>         if (!HAS_RUNTIME_PM(dev))
>                 return;
>
> -       pm_runtime_set_active(device);
> -
>         /*
>          * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>          * requirement.
> @@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>         pm_runtime_use_autosuspend(device);
>
>         pm_runtime_put_autosuspend(device);
> +       pm_runtime_put_noidle(device);
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[Jesse Barnes]

On 09/17/2015 02:40 PM, Paulo Zanoni wrote:
> 2015-09-17 18:23 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> According to the PCI docs and Rafael, we need to be using
>> pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
>> teardown routines, rather than using a direct enable/disable pair (and
>> we didn't even have the enable side, so never autosuspended after an
>> unload).
>>
>> This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
>> still debugging why the device never autosuspends.
>>
>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 85c35fd..1addb8a 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>>
>>         /* Make sure we're not suspended first. */
> 
> So is the comment above still valid?
> 
> As far as I remember, we explicitly wake up the hardware after unload
> because our driver is (was) not prepared to be loaded on a hardware
> that is suspended. Did you try module reloading after this change?

I'm still debugging it; in fact I think this change may be wrong.

> Also, basic-pci-d3-state should not be unloading/reloading the driver,
> so it's not clear to me how this change helps passing that test.

The BAT tests just happen to run the module reload test first.  That's how I caught this in the first place...

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

Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[Rafael J. Wysocki]

On Thursday, September 17, 2015 02:23:32 PM Jesse Barnes wrote:
> According to the PCI docs and Rafael, we need to be using
> pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
> teardown routines, rather than using a direct enable/disable pair (and
> we didn't even have the enable side, so never autosuspended after an
> unload).
> 
> This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
> still debugging why the device never autosuspends.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 85c35fd..1addb8a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>  
>  	/* Make sure we're not suspended first. */
>  	pm_runtime_get_sync(device);
> -	pm_runtime_disable(device);

That is correct IMO.  If you ensure that the usage counter is always above 0
and the device is "on", you don't need to disable runtime PM in addition to that.

> +	pm_runtime_get_noresume(device);

I'm not sure that this is needed.  You've already bumped up the usage counter.

Are you going to drop it going forward?

>  }
>  
>  /**
> @@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	if (!HAS_RUNTIME_PM(dev))
>  		return;
>  
> -	pm_runtime_set_active(device);
> -
>  	/*
>  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>  	 * requirement.
> @@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	pm_runtime_use_autosuspend(device);
>  
>  	pm_runtime_put_autosuspend(device);
> +	pm_runtime_put_noidle(device);

That shouldn't be necessary.  The "put" is already done in pm_runtime_put_autosuspend().

>  }
>  
> 

The rule is that .probe() should do one extra decrementation of the usage
counter and .remove() should do one extra incrementation of it.  Enable/disable
should never be necessary.

Thanks,
Rafael

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

Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

[Rafael J. Wysocki]

On Friday, September 18, 2015 03:01:50 AM Rafael J. Wysocki wrote:
> On Thursday, September 17, 2015 02:23:32 PM Jesse Barnes wrote:
> > According to the PCI docs and Rafael, we need to be using
> > pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
> > teardown routines, rather than using a direct enable/disable pair (and
> > we didn't even have the enable side, so never autosuspended after an
> > unload).
> > 
> > This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
> > still debugging why the device never autosuspends.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 85c35fd..1addb8a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
> >  
> >  	/* Make sure we're not suspended first. */
> >  	pm_runtime_get_sync(device);
> > -	pm_runtime_disable(device);
> 
> That is correct IMO.  If you ensure that the usage counter is always above 0
> and the device is "on", you don't need to disable runtime PM in addition to that.
> 
> > +	pm_runtime_get_noresume(device);
> 
> I'm not sure that this is needed.  You've already bumped up the usage counter.
> 
> Are you going to drop it going forward?
>
> 
> >  }
> >  
> >  /**
> > @@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> >  	if (!HAS_RUNTIME_PM(dev))
> >  		return;
> >  
> > -	pm_runtime_set_active(device);
> > -

This change looks OK to me.

You're called from local_pci_probe() that does pm_runtime_get_sync(), so the
device should be active at this point if I'm not missing anything.

The only kind of gray area is that the device may be physically off at probe
time (after boot) and we're thinking it is on. Is that possible even?

> >  	/*
> >  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
> >  	 * requirement.
> > @@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> >  	pm_runtime_use_autosuspend(device);
> >  
> >  	pm_runtime_put_autosuspend(device);
> > +	pm_runtime_put_noidle(device);
> 
> That shouldn't be necessary.  The "put" is already done in pm_runtime_put_autosuspend().
> 
> >  }
> >  
> > 

Thanks,
Rafael

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

[PATCH] drm/i915: fixup runtime PM handling v2

[Jesse Barnes]

According to the PCI docs and Rafael, we don't need to be doing explicit
enables and disables in our init and teardown routines, as they're taken
care of by the PCI core.  So drop the pm_runtime_disable() at teardown
and pm_runtime_set_active() at init.

This fixes one failure of the basic-pci-d3-state test on my BYT.

v2: drop extra get_noresume() and put_noidle() (Rafael)

Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 85c35fd..824e7b0 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1822,7 +1822,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
 
 	/* Make sure we're not suspended first. */
 	pm_runtime_get_sync(device);
-	pm_runtime_disable(device);
 }
 
 /**
@@ -2114,8 +2113,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
-	pm_runtime_set_active(device);
-
 	/*
 	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
 	 * requirement.
-- 
1.9.1

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

Re: [PATCH] drm/i915: fixup runtime PM handling v2

[Jesse Barnes]

Forgot to cc Rafael.

On 09/23/2015 02:37 PM, Jesse Barnes wrote:
> According to the PCI docs and Rafael, we don't need to be doing explicit
> enables and disables in our init and teardown routines, as they're taken
> care of by the PCI core.  So drop the pm_runtime_disable() at teardown
> and pm_runtime_set_active() at init.
> 
> This fixes one failure of the basic-pci-d3-state test on my BYT.
> 
> v2: drop extra get_noresume() and put_noidle() (Rafael)
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 85c35fd..824e7b0 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1822,7 +1822,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>  
>  	/* Make sure we're not suspended first. */
>  	pm_runtime_get_sync(device);
> -	pm_runtime_disable(device);
>  }
>  
>  /**
> @@ -2114,8 +2113,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	if (!HAS_RUNTIME_PM(dev))
>  		return;
>  
> -	pm_runtime_set_active(device);
> -
>  	/*
>  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>  	 * requirement.
> 

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

Re: [PATCH] drm/i915: fixup runtime PM handling v2

[Rafael J. Wysocki]

On Thursday, September 24, 2015 08:40:28 AM Jesse Barnes wrote:
> Forgot to cc Rafael.
> 
> On 09/23/2015 02:37 PM, Jesse Barnes wrote:
> > According to the PCI docs and Rafael, we don't need to be doing explicit
> > enables and disables in our init and teardown routines, as they're taken
> > care of by the PCI core.  So drop the pm_runtime_disable() at teardown
> > and pm_runtime_set_active() at init.
> > 
> > This fixes one failure of the basic-pci-d3-state test on my BYT.
> > 
> > v2: drop extra get_noresume() and put_noidle() (Rafael)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 85c35fd..824e7b0 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1822,7 +1822,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
> >  
> >  	/* Make sure we're not suspended first. */
> >  	pm_runtime_get_sync(device);
> > -	pm_runtime_disable(device);
> >  }
> >  
> >  /**
> > @@ -2114,8 +2113,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> >  	if (!HAS_RUNTIME_PM(dev))
> >  		return;
> >  
> > -	pm_runtime_set_active(device);
> > -
> >  	/*
> >  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
> >  	 * requirement.
> > 
> 

Looks good to me, thanks!

Rafael

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

Re: [PATCH] drm/i915: fixup runtime PM handling v2

[Daniel Vetter]

On Thu, Sep 24, 2015 at 09:14:15PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 24, 2015 08:40:28 AM Jesse Barnes wrote:
> > Forgot to cc Rafael.
> > 
> > On 09/23/2015 02:37 PM, Jesse Barnes wrote:
> > > According to the PCI docs and Rafael, we don't need to be doing explicit
> > > enables and disables in our init and teardown routines, as they're taken
> > > care of by the PCI core.  So drop the pm_runtime_disable() at teardown
> > > and pm_runtime_set_active() at init.
> > > 
> > > This fixes one failure of the basic-pci-d3-state test on my BYT.
> > > 
> > > v2: drop extra get_noresume() and put_noidle() (Rafael)
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 85c35fd..824e7b0 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -1822,7 +1822,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
> > >  
> > >  	/* Make sure we're not suspended first. */
> > >  	pm_runtime_get_sync(device);
> > > -	pm_runtime_disable(device);
> > >  }
> > >  
> > >  /**
> > > @@ -2114,8 +2113,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> > >  	if (!HAS_RUNTIME_PM(dev))
> > >  		return;
> > >  
> > > -	pm_runtime_set_active(device);
> > > -
> > >  	/*
> > >  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
> > >  	 * requirement.
> > > 
> > 
> 
> Looks good to me, thanks!

Queued for -next, thanks for the patch.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx