devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 0/2] Add QoS support for cpuidle system.
@ 2020-09-22 19:08 Joakim Zhang
  2020-09-22 19:08 ` [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property Joakim Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Joakim Zhang @ 2020-09-22 19:08 UTC (permalink / raw)
  To: mchehab, robh+dt, sean; +Cc: linux-media, devicetree, linux-kernel, linux-imx

Add QoS support for cpuidle system.

Joakim Zhang (2):
  bindings: media: gpio-ir-receiver: add linux,autosuspend-period
    property
  media: rc: gpio-ir-recv: add QoS support for cpuidle system

 .../bindings/media/gpio-ir-receiver.txt       |  3 ++
 drivers/media/rc/gpio-ir-recv.c               | 53 +++++++++++++++++++
 2 files changed, 56 insertions(+)

-- 
2.17.1


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

* [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property
  2020-09-22 19:08 [PATCH V3 0/2] Add QoS support for cpuidle system Joakim Zhang
@ 2020-09-22 19:08 ` Joakim Zhang
  2020-09-29 15:52   ` Rob Herring
  2020-09-22 19:08 ` [PATCH V3 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system Joakim Zhang
  2020-09-23  7:06 ` [PATCH V3 0/2] Add " Sean Young
  2 siblings, 1 reply; 8+ messages in thread
From: Joakim Zhang @ 2020-09-22 19:08 UTC (permalink / raw)
  To: mchehab, robh+dt, sean; +Cc: linux-media, devicetree, linux-kernel, linux-imx

Add linux,autosuspend-period property for gpio ir receiver. Some cpuidle
systems wake from idle may take a bit long time, for such case, need
disable cpuidle temporarily.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLogs:
V1->V2:
	* New add.
V2->V3:
	* linux,autosuspend-period = 125; -> linux,autosuspend-period = <125>;
---
 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
index 58261fb7b408..e1447c9b0e26 100644
--- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -7,6 +7,8 @@ Required properties:
 Optional properties:
 	- linux,rc-map-name: see rc.txt file in the same
 	  directory.
+        - linux,autosuspend-period: autosuspend delay time,
+          the unit is milisecond.
 
 Example node:
 
@@ -14,4 +16,5 @@ Example node:
 		compatible = "gpio-ir-receiver";
 		gpios = <&gpio0 19 1>;
 		linux,rc-map-name = "rc-rc6-mce";
+		linux,autosuspend-period = <125>;
 	};
-- 
2.17.1


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

* [PATCH V3 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system
  2020-09-22 19:08 [PATCH V3 0/2] Add QoS support for cpuidle system Joakim Zhang
  2020-09-22 19:08 ` [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property Joakim Zhang
@ 2020-09-22 19:08 ` Joakim Zhang
  2020-09-23  7:06 ` [PATCH V3 0/2] Add " Sean Young
  2 siblings, 0 replies; 8+ messages in thread
From: Joakim Zhang @ 2020-09-22 19:08 UTC (permalink / raw)
  To: mchehab, robh+dt, sean; +Cc: linux-media, devicetree, linux-kernel, linux-imx

GPIO IR receive is much rely on interrupt response, uneven interrupt
latency will lead to incorrect timing, so the decoder fails to decode
it. The issue is particularly acute on some systems which support
cpuidle, not all, dynamically disable and enable cpuidle can solve this
problem to a great extent.

However, there is a downside to this approach, the measurement of header
on the first frame may incorrect. Test on i.MX8M serials, when enable
cpuidle, interrupt latency could be about 500us.

With this patch:
1. has no side effect on non-cpuidle system, even runtime pm api won't
be invoked to avoid a bunch of pm busy work for devices that do not need
it, including spinlocks, ktime, etc.
2. latency is still much longer for the first gpio interrupt on cpuidle
system, so the first frame may not be decoded. Generally, RC would transmit
multiple frames at once press, we can sacrifice the first frame.
3. add "linux,autosuspend-period" property in device tree if you also
suffer this cpuidle issue.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
ChangeLogs:
V1->V2:
	* set autosuspend delay time via device tree.
V2->V3:
	* let runtime pm api conditionally.
---
 drivers/media/rc/gpio-ir-recv.c | 53 +++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index a20413008c3c..22e524b69806 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -11,6 +11,8 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_qos.h>
 #include <linux/irq.h>
 #include <media/rc-core.h>
 
@@ -20,17 +22,38 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	struct gpio_desc *gpiod;
 	int irq;
+	struct device *pmdev;
+	struct pm_qos_request qos;
 };
 
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
 	int val;
 	struct gpio_rc_dev *gpio_dev = dev_id;
+	struct device *pmdev = gpio_dev->pmdev;
+
+	/*
+	 * For some cpuidle systems, not all:
+	 * Respond to interrupt taking more latency when cpu in idle.
+	 * Invoke asynchronous pm runtime get from interrupt context,
+	 * this may introduce a millisecond delay to call resume callback,
+	 * where to disable cpuilde.
+	 *
+	 * Two issues lead to fail to decode first frame, one is latency to
+	 * respond to interrupt, another is delay introduced by async api.
+	 */
+	if (pmdev)
+		pm_runtime_get(pmdev);
 
 	val = gpiod_get_value(gpio_dev->gpiod);
 	if (val >= 0)
 		ir_raw_event_store_edge(gpio_dev->rcdev, val == 1);
 
+	if (pmdev) {
+		pm_runtime_mark_last_busy(pmdev);
+		pm_runtime_put_autosuspend(pmdev);
+	}
+
 	return IRQ_HANDLED;
 }
 
@@ -40,6 +63,7 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct gpio_rc_dev *gpio_dev;
 	struct rc_dev *rcdev;
+	u32 period = 0;
 	int rc;
 
 	if (!np)
@@ -90,6 +114,15 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	of_property_read_u32(np, "linux,autosuspend-period", &period);
+	if (period) {
+		gpio_dev->pmdev = dev;
+		pm_runtime_set_autosuspend_delay(dev, period);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
 	platform_set_drvdata(pdev, gpio_dev);
 
 	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
@@ -122,9 +155,29 @@ static int gpio_ir_recv_resume(struct device *dev)
 	return 0;
 }
 
+static int gpio_ir_recv_runtime_suspend(struct device *dev)
+{
+	struct gpio_rc_dev *gpio_dev = dev_get_drvdata(dev);
+
+	cpu_latency_qos_remove_request(&gpio_dev->qos);
+
+	return 0;
+}
+
+static int gpio_ir_recv_runtime_resume(struct device *dev)
+{
+	struct gpio_rc_dev *gpio_dev = dev_get_drvdata(dev);
+
+	cpu_latency_qos_add_request(&gpio_dev->qos, 0);
+
+	return 0;
+}
+
 static const struct dev_pm_ops gpio_ir_recv_pm_ops = {
 	.suspend        = gpio_ir_recv_suspend,
 	.resume         = gpio_ir_recv_resume,
+	.runtime_suspend = gpio_ir_recv_runtime_suspend,
+	.runtime_resume  = gpio_ir_recv_runtime_resume,
 };
 #endif
 
-- 
2.17.1


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

* Re: [PATCH V3 0/2] Add QoS support for cpuidle system.
  2020-09-22 19:08 [PATCH V3 0/2] Add QoS support for cpuidle system Joakim Zhang
  2020-09-22 19:08 ` [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property Joakim Zhang
  2020-09-22 19:08 ` [PATCH V3 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system Joakim Zhang
@ 2020-09-23  7:06 ` Sean Young
  2 siblings, 0 replies; 8+ messages in thread
From: Sean Young @ 2020-09-23  7:06 UTC (permalink / raw)
  To: Joakim Zhang
  Cc: mchehab, robh+dt, linux-media, devicetree, linux-kernel, linux-imx

On Wed, Sep 23, 2020 at 03:08:05AM +0800, Joakim Zhang wrote:
> Add QoS support for cpuidle system.

Reviewed-by: Sean Young <sean@mess.org>

Rob, would you be able to review the device tree bindings please?

Thanks,

Sean


> 
> Joakim Zhang (2):
>   bindings: media: gpio-ir-receiver: add linux,autosuspend-period
>     property
>   media: rc: gpio-ir-recv: add QoS support for cpuidle system
> 
>  .../bindings/media/gpio-ir-receiver.txt       |  3 ++
>  drivers/media/rc/gpio-ir-recv.c               | 53 +++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> -- 
> 2.17.1

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

* Re: [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property
  2020-09-22 19:08 ` [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property Joakim Zhang
@ 2020-09-29 15:52   ` Rob Herring
  2020-10-03  8:46     ` Sean Young
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2020-09-29 15:52 UTC (permalink / raw)
  To: Joakim Zhang
  Cc: mchehab, sean, linux-media, devicetree, linux-kernel, linux-imx

On Wed, Sep 23, 2020 at 03:08:06AM +0800, Joakim Zhang wrote:
> Add linux,autosuspend-period property for gpio ir receiver. Some cpuidle
> systems wake from idle may take a bit long time, for such case, need
> disable cpuidle temporarily.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> ChangeLogs:
> V1->V2:
> 	* New add.
> V2->V3:
> 	* linux,autosuspend-period = 125; -> linux,autosuspend-period = <125>;
> ---
>  Documentation/devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> index 58261fb7b408..e1447c9b0e26 100644
> --- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> @@ -7,6 +7,8 @@ Required properties:
>  Optional properties:
>  	- linux,rc-map-name: see rc.txt file in the same
>  	  directory.
> +        - linux,autosuspend-period: autosuspend delay time,
> +          the unit is milisecond.

What makes this linux specific?

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

* Re: [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property
  2020-09-29 15:52   ` Rob Herring
@ 2020-10-03  8:46     ` Sean Young
  2020-10-05 13:36       ` Rob Herring
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Young @ 2020-10-03  8:46 UTC (permalink / raw)
  To: Rob Herring
  Cc: Joakim Zhang, mchehab, linux-media, devicetree, linux-kernel, linux-imx

On Tue, Sep 29, 2020 at 10:52:01AM -0500, Rob Herring wrote:
> On Wed, Sep 23, 2020 at 03:08:06AM +0800, Joakim Zhang wrote:
> > Add linux,autosuspend-period property for gpio ir receiver. Some cpuidle
> > systems wake from idle may take a bit long time, for such case, need
> > disable cpuidle temporarily.
> > 
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > ChangeLogs:
> > V1->V2:
> > 	* New add.
> > V2->V3:
> > 	* linux,autosuspend-period = 125; -> linux,autosuspend-period = <125>;
> > ---
> >  Documentation/devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > index 58261fb7b408..e1447c9b0e26 100644
> > --- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > @@ -7,6 +7,8 @@ Required properties:
> >  Optional properties:
> >  	- linux,rc-map-name: see rc.txt file in the same
> >  	  directory.
> > +        - linux,autosuspend-period: autosuspend delay time,
> > +          the unit is milisecond.
> 
> What makes this linux specific?

Good point. "linux,autosuspend-period" does not say what is being
suspended either. How about "cpuidle-suspend-period" instead?


Sean

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

* Re: [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property
  2020-10-03  8:46     ` Sean Young
@ 2020-10-05 13:36       ` Rob Herring
  2020-10-05 14:09         ` Sean Young
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2020-10-05 13:36 UTC (permalink / raw)
  To: Sean Young
  Cc: Joakim Zhang, Mauro Carvalho Chehab, Linux Media Mailing List,
	devicetree, linux-kernel, NXP Linux Team

On Sat, Oct 3, 2020 at 3:46 AM Sean Young <sean@mess.org> wrote:
>
> On Tue, Sep 29, 2020 at 10:52:01AM -0500, Rob Herring wrote:
> > On Wed, Sep 23, 2020 at 03:08:06AM +0800, Joakim Zhang wrote:
> > > Add linux,autosuspend-period property for gpio ir receiver. Some cpuidle
> > > systems wake from idle may take a bit long time, for such case, need
> > > disable cpuidle temporarily.
> > >
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > > ChangeLogs:
> > > V1->V2:
> > >     * New add.
> > > V2->V3:
> > >     * linux,autosuspend-period = 125; -> linux,autosuspend-period = <125>;
> > > ---
> > >  Documentation/devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > index 58261fb7b408..e1447c9b0e26 100644
> > > --- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > @@ -7,6 +7,8 @@ Required properties:
> > >  Optional properties:
> > >     - linux,rc-map-name: see rc.txt file in the same
> > >       directory.
> > > +        - linux,autosuspend-period: autosuspend delay time,
> > > +          the unit is milisecond.
> >
> > What makes this linux specific?
>
> Good point. "linux,autosuspend-period" does not say what is being
> suspended either. How about "cpuidle-suspend-period" instead?

'cpuidle' is a Linuxism. And you also need a unit suffix.

I'm not clear on how autosuspend which is generally how long a
peripheral is idle before runtime suspending it relates to this which
seems to be concerned with cpu wakeup latency. I'm assuming you need
to wake up within a certain time period to capture GPIO edges. Don't
you know what this time would be based on IR data rates and can
provide that constraint to cpuidle?

Also, we can set autosuspend times from sysfs. Why do you need to do
this from DT?

Rob

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

* Re: [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property
  2020-10-05 13:36       ` Rob Herring
@ 2020-10-05 14:09         ` Sean Young
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Young @ 2020-10-05 14:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: Joakim Zhang, Mauro Carvalho Chehab, Linux Media Mailing List,
	devicetree, linux-kernel, NXP Linux Team

On Mon, Oct 05, 2020 at 08:36:39AM -0500, Rob Herring wrote:
> On Sat, Oct 3, 2020 at 3:46 AM Sean Young <sean@mess.org> wrote:
> >
> > On Tue, Sep 29, 2020 at 10:52:01AM -0500, Rob Herring wrote:
> > > On Wed, Sep 23, 2020 at 03:08:06AM +0800, Joakim Zhang wrote:
> > > > Add linux,autosuspend-period property for gpio ir receiver. Some cpuidle
> > > > systems wake from idle may take a bit long time, for such case, need
> > > > disable cpuidle temporarily.
> > > >
> > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > ---
> > > > ChangeLogs:
> > > > V1->V2:
> > > >     * New add.
> > > > V2->V3:
> > > >     * linux,autosuspend-period = 125; -> linux,autosuspend-period = <125>;
> > > > ---
> > > >  Documentation/devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > > index 58261fb7b408..e1447c9b0e26 100644
> > > > --- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > > +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> > > > @@ -7,6 +7,8 @@ Required properties:
> > > >  Optional properties:
> > > >     - linux,rc-map-name: see rc.txt file in the same
> > > >       directory.
> > > > +        - linux,autosuspend-period: autosuspend delay time,
> > > > +          the unit is milisecond.
> > >
> > > What makes this linux specific?
> >
> > Good point. "linux,autosuspend-period" does not say what is being
> > suspended either. How about "cpuidle-suspend-period" instead?
> 
> 'cpuidle' is a Linuxism. And you also need a unit suffix.
> 
> I'm not clear on how autosuspend which is generally how long a
> peripheral is idle before runtime suspending it relates to this which
> seems to be concerned with cpu wakeup latency. I'm assuming you need
> to wake up within a certain time period to capture GPIO edges. Don't
> you know what this time would be based on IR data rates and can
> provide that constraint to cpuidle?
> 
> Also, we can set autosuspend times from sysfs. Why do you need to do
> this from DT?

This driver collects the timings of the edges on the gpio line, and sends
them to the IR decoders. Waking up the cpu out of cpuidle causes random
delays and therefore timing are off, on some hardware. This setting turns
off cpuidle for a period to prevent these problems.

Note that on this soc cpuidle is causing huge delays, 500 microseconds or so.


Sean

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

end of thread, other threads:[~2020-10-05 14:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 19:08 [PATCH V3 0/2] Add QoS support for cpuidle system Joakim Zhang
2020-09-22 19:08 ` [PATCH V3 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property Joakim Zhang
2020-09-29 15:52   ` Rob Herring
2020-10-03  8:46     ` Sean Young
2020-10-05 13:36       ` Rob Herring
2020-10-05 14:09         ` Sean Young
2020-09-22 19:08 ` [PATCH V3 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system Joakim Zhang
2020-09-23  7:06 ` [PATCH V3 0/2] Add " Sean Young

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