Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

On Tuesday, May 02, 2017 12:05:38 PM Imre Deak wrote:
> On Mon, May 01, 2017 at 10:36:13PM +0200, Rafael J. Wysocki wrote:
> > On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote:
> > > On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote:
> > > > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> > > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > > complete optimization due to differences in their runtime and system
> > > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > > calling the driver's system suspend handlers which effectively 
> > > > > > disables
> > > > > > the optimization.
> > > > > > 
> > > > > > Needed by the next patch fixing suspend/resume on i915.
> > > > > > 
> > > > > > Suggested by Rafael.
> > > > > > 
> > > > > > Cc: Rafael J. Wysocki 
> > > > > > Cc: Bjorn Helgaas 
> > > > > > Cc: linux-...@vger.kernel.org
> > > > > > Cc: sta...@vger.kernel.org
> > > > > > Signed-off-by: Imre Deak 
> > > > > 
> > > > > Acked-by: Rafael J. Wysocki 
> > > > > 
> > > > > The reason why the opt-out flag was not added on day one was because 
> > > > > we were
> > > > > not sure whether or not it would be necessary at all.
> > > > > 
> > > > > Quite evidently, it is needed.
> > > > 
> > > > But that said, it actually can be implemented as a flag in dev_flags 
> > > > too, say
> > > > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's
> > > > already there.
> > > > 
> > > > The struct size would not need to grow then which I guess would be 
> > > > better?
> > > 
> > > Hm, both the bit field and the flag would need to increase if running
> > > out of bits, so what's the difference? (Atm, the struct size wouldn't
> > > change either way.)
> > 
> > In the bit field case this depends on what the compiler thinks is better to 
> > be
> > entirely precise, so they are not 100% equivalent.
> > 
> > Plus, since there already are things related to PM in dev_flags, why to 
> > depart
> > from that pattern?
> 
> There are a few PM related flags in the bit field too.
> 
> The need for moving it is still not clear to me, but I don't see any
> problem with it either, so will move it there.

Well, we are not too consistent in that respect overall.

Either way works, so I guess it's Bjorn's call at this point. :-)

Thanks,
Rafael

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

On Mon, May 01, 2017 at 10:36:13PM +0200, Rafael J. Wysocki wrote:
> On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote:
> > On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote:
> > > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote:
> > > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > complete optimization due to differences in their runtime and system
> > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > calling the driver's system suspend handlers which effectively 
> > > > > disables
> > > > > the optimization.
> > > > > 
> > > > > Needed by the next patch fixing suspend/resume on i915.
> > > > > 
> > > > > Suggested by Rafael.
> > > > > 
> > > > > Cc: Rafael J. Wysocki 
> > > > > Cc: Bjorn Helgaas 
> > > > > Cc: linux-...@vger.kernel.org
> > > > > Cc: sta...@vger.kernel.org
> > > > > Signed-off-by: Imre Deak 
> > > > 
> > > > Acked-by: Rafael J. Wysocki 
> > > > 
> > > > The reason why the opt-out flag was not added on day one was because we 
> > > > were
> > > > not sure whether or not it would be necessary at all.
> > > > 
> > > > Quite evidently, it is needed.
> > > 
> > > But that said, it actually can be implemented as a flag in dev_flags too, 
> > > say
> > > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's
> > > already there.
> > > 
> > > The struct size would not need to grow then which I guess would be better?
> > 
> > Hm, both the bit field and the flag would need to increase if running
> > out of bits, so what's the difference? (Atm, the struct size wouldn't
> > change either way.)
> 
> In the bit field case this depends on what the compiler thinks is better to be
> entirely precise, so they are not 100% equivalent.
> 
> Plus, since there already are things related to PM in dev_flags, why to depart
> from that pattern?

There are a few PM related flags in the bit field too.

The need for moving it is still not clear to me, but I don't see any
problem with it either, so will move it there.

--Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote:
> On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote:
> > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote:
> > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> > > > Some drivers - like i915 - may not support the system suspend direct
> > > > complete optimization due to differences in their runtime and system
> > > > suspend sequence. Add a flag that when set resumes the device before
> > > > calling the driver's system suspend handlers which effectively disables
> > > > the optimization.
> > > > 
> > > > Needed by the next patch fixing suspend/resume on i915.
> > > > 
> > > > Suggested by Rafael.
> > > > 
> > > > Cc: Rafael J. Wysocki 
> > > > Cc: Bjorn Helgaas 
> > > > Cc: linux-...@vger.kernel.org
> > > > Cc: sta...@vger.kernel.org
> > > > Signed-off-by: Imre Deak 
> > > 
> > > Acked-by: Rafael J. Wysocki 
> > > 
> > > The reason why the opt-out flag was not added on day one was because we 
> > > were
> > > not sure whether or not it would be necessary at all.
> > > 
> > > Quite evidently, it is needed.
> > 
> > But that said, it actually can be implemented as a flag in dev_flags too, 
> > say
> > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's
> > already there.
> > 
> > The struct size would not need to grow then which I guess would be better?
> 
> Hm, both the bit field and the flag would need to increase if running
> out of bits, so what's the difference? (Atm, the struct size wouldn't
> change either way.)

In the bit field case this depends on what the compiler thinks is better to be
entirely precise, so they are not 100% equivalent.

Plus, since there already are things related to PM in dev_flags, why to depart
from that pattern?

Thanks,
Rafael

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote:
> On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote:
> > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> > > Some drivers - like i915 - may not support the system suspend direct
> > > complete optimization due to differences in their runtime and system
> > > suspend sequence. Add a flag that when set resumes the device before
> > > calling the driver's system suspend handlers which effectively disables
> > > the optimization.
> > > 
> > > Needed by the next patch fixing suspend/resume on i915.
> > > 
> > > Suggested by Rafael.
> > > 
> > > Cc: Rafael J. Wysocki 
> > > Cc: Bjorn Helgaas 
> > > Cc: linux-...@vger.kernel.org
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Imre Deak 
> > 
> > Acked-by: Rafael J. Wysocki 
> > 
> > The reason why the opt-out flag was not added on day one was because we were
> > not sure whether or not it would be necessary at all.
> > 
> > Quite evidently, it is needed.
> 
> But that said, it actually can be implemented as a flag in dev_flags too, say
> PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's
> already there.
> 
> The struct size would not need to grow then which I guess would be better?

Hm, both the bit field and the flag would need to increase if running
out of bits, so what's the difference? (Atm, the struct size wouldn't
change either way.)

--Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote:
> On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> > Some drivers - like i915 - may not support the system suspend direct
> > complete optimization due to differences in their runtime and system
> > suspend sequence. Add a flag that when set resumes the device before
> > calling the driver's system suspend handlers which effectively disables
> > the optimization.
> > 
> > Needed by the next patch fixing suspend/resume on i915.
> > 
> > Suggested by Rafael.
> > 
> > Cc: Rafael J. Wysocki 
> > Cc: Bjorn Helgaas 
> > Cc: linux-...@vger.kernel.org
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Imre Deak 
> 
> Acked-by: Rafael J. Wysocki 
> 
> The reason why the opt-out flag was not added on day one was because we were
> not sure whether or not it would be necessary at all.
> 
> Quite evidently, it is needed.

But that said, it actually can be implemented as a flag in dev_flags too, say
PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's
already there.

The struct size would not need to grow then which I guess would be better?

Thanks,
Rafael

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote:
> Some drivers - like i915 - may not support the system suspend direct
> complete optimization due to differences in their runtime and system
> suspend sequence. Add a flag that when set resumes the device before
> calling the driver's system suspend handlers which effectively disables
> the optimization.
> 
> Needed by the next patch fixing suspend/resume on i915.
> 
> Suggested by Rafael.
> 
> Cc: Rafael J. Wysocki 
> Cc: Bjorn Helgaas 
> Cc: linux-...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Signed-off-by: Imre Deak 

Acked-by: Rafael J. Wysocki 

The reason why the opt-out flag was not added on day one was because we were
not sure whether or not it would be necessary at all.

Quite evidently, it is needed.

> ---
> 
> [ After discussing with Jani, I'm going to apply this and the next patch
>   for now to the intel-gfx specific CI branch to unblock our testing. ]
> 
>  drivers/pci/pci.c   | 3 ++-
>  include/linux/pci.h | 7 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02..020f02d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  
>   if (!pm_runtime_suspended(dev)
>   || pci_target_state(pci_dev) != pci_dev->current_state
> - || platform_pci_need_resume(pci_dev))
> + || platform_pci_need_resume(pci_dev)
> + || pci_dev->needs_resume)
>   return false;
>  
>   /*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5948cfd..2f012f8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -316,6 +316,9 @@ struct pci_dev {
>   unsigned inthotplug_user_indicators:1; /* SlotCtl indicators
> controlled exclusively by
> user sysfs */
> + unsigned intneeds_resume:1; /* Resume before calling the driver's
> +system suspend hooks, disabling the
> +direct_complete optimization. */
>   unsigned intd3_delay;   /* D3->D0 transition time in ms */
>   unsigned intd3cold_delay;   /* D3cold->D0 transition time in ms */
>  
> @@ -1110,6 +1113,10 @@ bool pci_check_pme_status(struct pci_dev *dev);
>  void pci_pme_wakeup_bus(struct pci_bus *bus);
>  void pci_d3cold_enable(struct pci_dev *dev);
>  void pci_d3cold_disable(struct pci_dev *dev);
> +static inline void pci_resume_before_suspend(struct pci_dev *dev, bool 
> enable)
> +{
> + dev->needs_resume = enable;
> +}
>  
>  static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
> bool enable)
> 

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

Some drivers - like i915 - may not support the system suspend direct
complete optimization due to differences in their runtime and system
suspend sequence. Add a flag that when set resumes the device before
calling the driver's system suspend handlers which effectively disables
the optimization.

Needed by the next patch fixing suspend/resume on i915.

Suggested by Rafael.

Cc: Rafael J. Wysocki 
Cc: Bjorn Helgaas 
Cc: linux-...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Imre Deak 
---

[ After discussing with Jani, I'm going to apply this and the next patch
  for now to the intel-gfx specific CI branch to unblock our testing. ]

 drivers/pci/pci.c   | 3 ++-
 include/linux/pci.h | 7 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7904d02..020f02d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
 
if (!pm_runtime_suspended(dev)
|| pci_target_state(pci_dev) != pci_dev->current_state
-   || platform_pci_need_resume(pci_dev))
+   || platform_pci_need_resume(pci_dev)
+   || pci_dev->needs_resume)
return false;
 
/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5948cfd..2f012f8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -316,6 +316,9 @@ struct pci_dev {
unsigned inthotplug_user_indicators:1; /* SlotCtl indicators
  controlled exclusively by
  user sysfs */
+   unsigned intneeds_resume:1; /* Resume before calling the driver's
+  system suspend hooks, disabling the
+  direct_complete optimization. */
unsigned intd3_delay;   /* D3->D0 transition time in ms */
unsigned intd3cold_delay;   /* D3cold->D0 transition time in ms */
 
@@ -1110,6 +1113,10 @@ bool pci_check_pme_status(struct pci_dev *dev);
 void pci_pme_wakeup_bus(struct pci_bus *bus);
 void pci_d3cold_enable(struct pci_dev *dev);
 void pci_d3cold_disable(struct pci_dev *dev);
+static inline void pci_resume_before_suspend(struct pci_dev *dev, bool enable)
+{
+   dev->needs_resume = enable;
+}
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
  bool enable)
-- 
2.5.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx