linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] i2c: Enable async resume for i2c devices
@ 2021-10-22  2:28 Rajat Jain
  2021-10-22  2:28 ` [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices Rajat Jain
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Rajat Jain @ 2021-10-22  2:28 UTC (permalink / raw)
  To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: Rajat Jain, rajatxjain, dbasehore

PM Core allows buses and drivers to specify if they'd like their devices
to suspend/resume synchronously or asynchronously. When resuming:

1) SYNCHRONOUS DEVICES:
 - All synchronous devices (system wide!) are resumed in a single thread,
   serially i.e. one after the other. So their resume latencies add up,
   and also, this results in unnecessary and unnatural waiting order.

   In my current system (total resume time ~895ms) and this is the trend
   on almost all chromebooks in the past 3-4 years (we carry patch3 in
   our tree already, without which it would be even more worse):
   https://rajatxjain.github.io/public_shared/resume_before_patches.html
   As you can see I2C devices do not even begin to resume until 450ms,
   waiting unnaturally for another device i915 to finish resuming: 

   I2C touchscreen device (resume latency = 374 ms) - asynchronous
   -> (waiting on) I2C adapter resume (synchronous)
     -> (waiting on) Designware resume (synchronous)
       -> (waiting on) intel_backlight resume (synchronous)
         -> (waiting on) its PARENT i915 resume (asynchronous resume
                                                       time = 376ms)
   As you can see the two biggest resume routines are both run serially
   after one another (even though they don't have any real dependency)
   thus increasing the system critical resume path. If we can run them
   concurrently, we can cut down the system resume time considerably. 
 
2) ASYNCHRONOUS DEVICES: 
- On the other hand, all asynchronous devices's resume routines are
  scheduled so they can run in parallel with other asynchronous
  routines. PM core still ensures for both async/sync devices that:
   - All parent child relations are honored.
   - Any device dependencies are honored. Device dependencies between
     any 2 unrelated devices can be specified using device_link_add().
   - Async resume devices are sychnronized at the end of each
     suspend/resume phase, before moving onto next.

   With these patches in place, the I2C devices can resume in parallel
   with i915: 
   https://rajatxjain.github.io/public_shared/resume_after_patch.html

As far as I understand, the only reason we might not want a device to be
marked for asynchronous resume is if we suspect it cannot handle
concurrent resume with other devices, which does not look to be the
case. 
    
This patchset marks the designware, the I2c adapters, and the i2c 
clients for asynchronous suspend/resume. In case it helps to gain any
confidence, the patch 3 (for i2c clients) has been included and shipping
on all our chromebooks for the past 3+ years, and has not shown any
issues. The designware and i2c adapters should be easier.

Derek Basehore (1):
  i2c: enable async suspend/resume on i2c client devices

Rajat Jain (2):
  i2c: designware: Enable async suspend / resume of designware devices
  i2c: enable async suspend/resume for i2c adapters

 drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
 drivers/i2c/i2c-core-base.c                 | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.33.0.1079.g6e70778dc9-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices
  2021-10-22  2:28 [PATCH 0/3] i2c: Enable async resume for i2c devices Rajat Jain
@ 2021-10-22  2:28 ` Rajat Jain
  2021-10-22 11:37   ` Jarkko Nikula
  2021-10-22  2:28 ` [PATCH 2/3] i2c: enable async suspend/resume for i2c adapters Rajat Jain
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Rajat Jain @ 2021-10-22  2:28 UTC (permalink / raw)
  To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: Rajat Jain, rajatxjain, dbasehore

Mark the designware devices for asynchronous suspend. With this, the
resume for designware devices does not get stuck behind other unrelated
devices (e.g. intel_backlight that takes hundreds of ms to resume,
waiting for its parent devices).

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 21113665ddea..2bd81abc86f6 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -293,6 +293,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 					DPM_FLAG_MAY_SKIP_RESUME);
 	}
 
+	device_enable_async_suspend(&pdev->dev);
+
 	/* The code below assumes runtime PM to be disabled. */
 	WARN_ON(pm_runtime_enabled(&pdev->dev));
 
-- 
2.33.0.1079.g6e70778dc9-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] i2c: enable async suspend/resume for i2c adapters
  2021-10-22  2:28 [PATCH 0/3] i2c: Enable async resume for i2c devices Rajat Jain
  2021-10-22  2:28 ` [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices Rajat Jain
@ 2021-10-22  2:28 ` Rajat Jain
  2021-10-22  2:28 ` [PATCH 3/3] i2c: enable async suspend/resume on i2c client devices Rajat Jain
  2021-10-22 11:50 ` [PATCH 0/3] i2c: Enable async resume for i2c devices Jarkko Nikula
  3 siblings, 0 replies; 7+ messages in thread
From: Rajat Jain @ 2021-10-22  2:28 UTC (permalink / raw)
  To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: Rajat Jain, rajatxjain, dbasehore

Enable async suspend/resume of i2c adapters. It enormously helps with
reducing the resume time of systems (as much as 20%-40%) where I2C devices
can take significant time (100s of ms) to resume.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/i2c/i2c-core-base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 54964fbe3f03..8d4f2be54e17 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1576,6 +1576,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
+	device_enable_async_suspend(&adap->dev);
 	pm_runtime_no_callbacks(&adap->dev);
 	pm_suspend_ignore_children(&adap->dev, true);
 	pm_runtime_enable(&adap->dev);
-- 
2.33.0.1079.g6e70778dc9-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] i2c: enable async suspend/resume on i2c client devices
  2021-10-22  2:28 [PATCH 0/3] i2c: Enable async resume for i2c devices Rajat Jain
  2021-10-22  2:28 ` [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices Rajat Jain
  2021-10-22  2:28 ` [PATCH 2/3] i2c: enable async suspend/resume for i2c adapters Rajat Jain
@ 2021-10-22  2:28 ` Rajat Jain
  2021-10-22 11:50 ` [PATCH 0/3] i2c: Enable async resume for i2c devices Jarkko Nikula
  3 siblings, 0 replies; 7+ messages in thread
From: Rajat Jain @ 2021-10-22  2:28 UTC (permalink / raw)
  To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: Derek Basehore, rajatxjain, Rajat Jain

From: Derek Basehore <dbasehore@chromium.org>

This enables the async suspend for i2c client devices. This reduces
the suspend/resume time considerably on platforms where i2c devices
can take a lot of time (hundreds of ms) to resume.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/i2c/i2c-core-base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 8d4f2be54e17..70d32efb68ef 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1047,6 +1047,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	client->dev.of_node = of_node_get(info->of_node);
 	client->dev.fwnode = info->fwnode;
 
+	device_enable_async_suspend(&client->dev);
 	i2c_dev_set_name(adap, client, info);
 
 	if (info->swnode) {
-- 
2.33.0.1079.g6e70778dc9-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices
  2021-10-22  2:28 ` [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices Rajat Jain
@ 2021-10-22 11:37   ` Jarkko Nikula
  2021-10-22 16:53     ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Nikula @ 2021-10-22 11:37 UTC (permalink / raw)
  To: Rajat Jain, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: rajatxjain, dbasehore

On 10/22/21 5:28 AM, Rajat Jain wrote:
> Mark the designware devices for asynchronous suspend. With this, the
> resume for designware devices does not get stuck behind other unrelated
> devices (e.g. intel_backlight that takes hundreds of ms to resume,
> waiting for its parent devices).
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 21113665ddea..2bd81abc86f6 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -293,6 +293,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   					DPM_FLAG_MAY_SKIP_RESUME);
>   	}
>   
> +	device_enable_async_suspend(&pdev->dev);
> +
>   	/* The code below assumes runtime PM to be disabled. */
>   	WARN_ON(pm_runtime_enabled(&pdev->dev));
>   
I guess same can be done to i2c_dw_pci_probe() too. I don't have any 
strong opinion should it be done in this patch or somewhere in the future.

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] i2c: Enable async resume for i2c devices
  2021-10-22  2:28 [PATCH 0/3] i2c: Enable async resume for i2c devices Rajat Jain
                   ` (2 preceding siblings ...)
  2021-10-22  2:28 ` [PATCH 3/3] i2c: enable async suspend/resume on i2c client devices Rajat Jain
@ 2021-10-22 11:50 ` Jarkko Nikula
  3 siblings, 0 replies; 7+ messages in thread
From: Jarkko Nikula @ 2021-10-22 11:50 UTC (permalink / raw)
  To: Rajat Jain, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, dtor
  Cc: rajatxjain, dbasehore

On 10/22/21 5:28 AM, Rajat Jain wrote:
> PM Core allows buses and drivers to specify if they'd like their devices
> to suspend/resume synchronously or asynchronously. When resuming:
> 
> 1) SYNCHRONOUS DEVICES:
>   - All synchronous devices (system wide!) are resumed in a single thread,
>     serially i.e. one after the other. So their resume latencies add up,
>     and also, this results in unnecessary and unnatural waiting order.
> 
>     In my current system (total resume time ~895ms) and this is the trend
>     on almost all chromebooks in the past 3-4 years (we carry patch3 in
>     our tree already, without which it would be even more worse):
>     https://rajatxjain.github.io/public_shared/resume_before_patches.html
>     As you can see I2C devices do not even begin to resume until 450ms,
>     waiting unnaturally for another device i915 to finish resuming:
> 
>     I2C touchscreen device (resume latency = 374 ms) - asynchronous
>     -> (waiting on) I2C adapter resume (synchronous)
>       -> (waiting on) Designware resume (synchronous)
>         -> (waiting on) intel_backlight resume (synchronous)
>           -> (waiting on) its PARENT i915 resume (asynchronous resume
>                                                         time = 376ms)
>     As you can see the two biggest resume routines are both run serially
>     after one another (even though they don't have any real dependency)
>     thus increasing the system critical resume path. If we can run them
>     concurrently, we can cut down the system resume time considerably.
>   
> 2) ASYNCHRONOUS DEVICES:
> - On the other hand, all asynchronous devices's resume routines are
>    scheduled so they can run in parallel with other asynchronous
>    routines. PM core still ensures for both async/sync devices that:
>     - All parent child relations are honored.
>     - Any device dependencies are honored. Device dependencies between
>       any 2 unrelated devices can be specified using device_link_add().
>     - Async resume devices are sychnronized at the end of each
>       suspend/resume phase, before moving onto next.
> 
>     With these patches in place, the I2C devices can resume in parallel
>     with i915:
>     https://rajatxjain.github.io/public_shared/resume_after_patch.html
> 
> As far as I understand, the only reason we might not want a device to be
> marked for asynchronous resume is if we suspect it cannot handle
> concurrent resume with other devices, which does not look to be the
> case.
>      
> This patchset marks the designware, the I2c adapters, and the i2c
> clients for asynchronous suspend/resume. In case it helps to gain any
> confidence, the patch 3 (for i2c clients) has been included and shipping
> on all our chromebooks for the past 3+ years, and has not shown any
> issues. The designware and i2c adapters should be easier.
> 
> Derek Basehore (1):
>    i2c: enable async suspend/resume on i2c client devices
> 
> Rajat Jain (2):
>    i2c: designware: Enable async suspend / resume of designware devices
>    i2c: enable async suspend/resume for i2c adapters
> 
I did a quick test to these three patches on one system. It has i2c-hid 
compatible touchpanel and touchpad on separate I2C buses. Nothing 
exploded but I don't know does it qualify as comprehensive test for 
Tested-by. Perhaps it's good idea to try do changes here expose any 
regressions on some use cases and systems? At least it brings good 
improvements to resume time according to your measurements.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices
  2021-10-22 11:37   ` Jarkko Nikula
@ 2021-10-22 16:53     ` Dmitry Torokhov
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2021-10-22 16:53 UTC (permalink / raw)
  To: Jarkko Nikula
  Cc: Rajat Jain, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel, linux-pm, rajatxjain, dbasehore

Hi Jarkko,

On Fri, Oct 22, 2021 at 4:37 AM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> On 10/22/21 5:28 AM, Rajat Jain wrote:
> > Mark the designware devices for asynchronous suspend. With this, the
> > resume for designware devices does not get stuck behind other unrelated
> > devices (e.g. intel_backlight that takes hundreds of ms to resume,
> > waiting for its parent devices).
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 21113665ddea..2bd81abc86f6 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -293,6 +293,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >                                       DPM_FLAG_MAY_SKIP_RESUME);
> >       }
> >
> > +     device_enable_async_suspend(&pdev->dev);
> > +
> >       /* The code below assumes runtime PM to be disabled. */
> >       WARN_ON(pm_runtime_enabled(&pdev->dev));
> >
> I guess same can be done to i2c_dw_pci_probe() too. I don't have any
> strong opinion should it be done in this patch or somewhere in the future.

All PCI devices are marked for asynchronous suspend/resume by default.
See drivers/pci/pci.c::pci_pm_init().

Thanks,
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-22 16:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22  2:28 [PATCH 0/3] i2c: Enable async resume for i2c devices Rajat Jain
2021-10-22  2:28 ` [PATCH 1/3] i2c: designware: Enable async suspend / resume of designware devices Rajat Jain
2021-10-22 11:37   ` Jarkko Nikula
2021-10-22 16:53     ` Dmitry Torokhov
2021-10-22  2:28 ` [PATCH 2/3] i2c: enable async suspend/resume for i2c adapters Rajat Jain
2021-10-22  2:28 ` [PATCH 3/3] i2c: enable async suspend/resume on i2c client devices Rajat Jain
2021-10-22 11:50 ` [PATCH 0/3] i2c: Enable async resume for i2c devices Jarkko Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).