[PATCH V2 0/2] add QoS support for cpuidle system

[PATCH V2 0/2] add QoS support for cpuidle system

[Joakim Zhang]

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               | 50 +++++++++++++++++++
 2 files changed, 53 insertions(+)

-- 
2.17.1

[PATCH V2 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property

[Joakim Zhang]

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

RE: [PATCH V2 1/2] bindings: media: gpio-ir-receiver: add linux,autosuspend-period property

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2020年9月19日 2:17
> To: mchehab@kernel.org; robh+dt@kernel.org; sean@mess.org
> Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH V2 1/2] bindings: media: gpio-ir-receiver: add
> linux,autosuspend-period property
> 
> 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.
> ---
>  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..ef802d5fd1f6 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;

Sorry for this, should be linux,autosuspend-period = <125>;
Please review the driver part first, will fix it in next version.

Best Regards,
Joakim Zhang
>  	};
> --
> 2.17.1

[PATCH V2 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Joakim Zhang]

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.
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.
---
 drivers/media/rc/gpio-ir-recv.c | 50 +++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index a20413008c3c..63cf8290eb19 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,35 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	struct gpio_desc *gpiod;
 	int irq;
+	struct device *dev;
+	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 *dev = gpio_dev->dev;
+
+	/*
+	 * 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.
+	 */
+	pm_runtime_get(dev);
 
 	val = gpiod_get_value(gpio_dev->gpiod);
 	if (val >= 0)
 		ir_raw_event_store_edge(gpio_dev->rcdev, val == 1);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return IRQ_HANDLED;
 }
 
@@ -40,6 +60,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)
@@ -83,6 +104,7 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 		rcdev->map_name = RC_MAP_EMPTY;
 
 	gpio_dev->rcdev = rcdev;
+	gpio_dev->dev = dev;
 
 	rc = devm_rc_register_device(dev, rcdev);
 	if (rc < 0) {
@@ -90,6 +112,14 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	of_property_read_u32(np, "linux,autosuspend-period", &period);
+	if (period) {
+		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 +152,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

Re: [PATCH V2 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Sean Young]

Hi Joakim,

Thanks for your updated patch.

On Sat, Sep 19, 2020 at 02:17:17AM +0800, Joakim Zhang wrote:
> 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.
> 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.
> ---
>  drivers/media/rc/gpio-ir-recv.c | 50 +++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> index a20413008c3c..63cf8290eb19 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,35 @@ struct gpio_rc_dev {
>  	struct rc_dev *rcdev;
>  	struct gpio_desc *gpiod;
>  	int irq;
> +	struct device *dev;
> +	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 *dev = gpio_dev->dev;
> +
> +	/*
> +	 * 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.
> +	 */
> +	pm_runtime_get(dev);
>  
>  	val = gpiod_get_value(gpio_dev->gpiod);
>  	if (val >= 0)
>  		ir_raw_event_store_edge(gpio_dev->rcdev, val == 1);
>  
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +

I've dusted off my rpi and tried to see if it fails with cpuidle enabled.
As far as I can see, it always works fine. So, my only concern is that
there is a bunch of pm busy work done here for devices that do not need it,
including spinlocks, ktime. etc.

You could rename the dev field of gpio_rc_dev to pmdev and only do this
work if pm_dev != NULL. Or some other mechanism of doing this work
conditionally.

Having said that I'm not entirely sure this will make much of a difference.

>  	return IRQ_HANDLED;
>  }
>  
> @@ -40,6 +60,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)
> @@ -83,6 +104,7 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>  		rcdev->map_name = RC_MAP_EMPTY;
>  
>  	gpio_dev->rcdev = rcdev;
> +	gpio_dev->dev = dev;
>  
>  	rc = devm_rc_register_device(dev, rcdev);
>  	if (rc < 0) {
> @@ -90,6 +112,14 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>  		return rc;
>  	}
>  
> +	of_property_read_u32(np, "linux,autosuspend-period", &period);
> +	if (period) {
> +		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 +152,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

RE: [PATCH V2 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Joakim Zhang]

Hi Sean,

Thanks for you kindly review.

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月22日 17:42
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; robh+dt@kernel.org; linux-media@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/2] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> Hi Joakim,
> 
> Thanks for your updated patch.
> 
> On Sat, Sep 19, 2020 at 02:17:17AM +0800, Joakim Zhang wrote:
> > 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.
> > 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.
> > ---
> >  drivers/media/rc/gpio-ir-recv.c | 50
> > +++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/media/rc/gpio-ir-recv.c
> > b/drivers/media/rc/gpio-ir-recv.c index a20413008c3c..63cf8290eb19
> > 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,35 @@ struct gpio_rc_dev {
> >  	struct rc_dev *rcdev;
> >  	struct gpio_desc *gpiod;
> >  	int irq;
> > +	struct device *dev;
> > +	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 *dev = gpio_dev->dev;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	pm_runtime_get(dev);
> >
> >  	val = gpiod_get_value(gpio_dev->gpiod);
> >  	if (val >= 0)
> >  		ir_raw_event_store_edge(gpio_dev->rcdev, val == 1);
> >
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +
> 
> I've dusted off my rpi and tried to see if it fails with cpuidle enabled.
> As far as I can see, it always works fine. So, my only concern is that there is a
> bunch of pm busy work done here for devices that do not need it, including
> spinlocks, ktime. etc.
> 
> You could rename the dev field of gpio_rc_dev to pmdev and only do this work if
> pm_dev != NULL. Or some other mechanism of doing this work conditionally.
> 
> Having said that I'm not entirely sure this will make much of a difference.

Per my understanding, this should not make much difference. But, GPIO IR recv is sensitive to time, so we need try our best not introduce extra work.

So, I will improve it then send a V3.

Best Regards,
Joakim Zhang
> >  	return IRQ_HANDLED;
> >  }
> >
> > @@ -40,6 +60,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)
> > @@ -83,6 +104,7 @@ static int gpio_ir_recv_probe(struct platform_device
> *pdev)
> >  		rcdev->map_name = RC_MAP_EMPTY;
> >
> >  	gpio_dev->rcdev = rcdev;
> > +	gpio_dev->dev = dev;
> >
> >  	rc = devm_rc_register_device(dev, rcdev);
> >  	if (rc < 0) {
> > @@ -90,6 +112,14 @@ static int gpio_ir_recv_probe(struct platform_device
> *pdev)
> >  		return rc;
> >  	}
> >
> > +	of_property_read_u32(np, "linux,autosuspend-period", &period);
> > +	if (period) {
> > +		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 +152,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

RE: [PATCH V2 0/2] add QoS support for cpuidle system

[Joakim Zhang]

Hi Sean,

Any comments?

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2020年9月19日 2:17
> To: mchehab@kernel.org; robh+dt@kernel.org; sean@mess.org
> Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH V2 0/2] add QoS support for cpuidle system
> 
> 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               | 50 +++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> --
> 2.17.1