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

[PATCH] 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 systems which supports
cpuidle, 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.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/media/rc/gpio-ir-recv.c | 49 ++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index a20413008c3c..42c942ce98cd 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,36 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	struct gpio_desc *gpiod;
 	int irq;
+	struct pm_qos_request qos;
 };
 
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
-	int val;
+	int ret, val;
 	struct gpio_rc_dev *gpio_dev = dev_id;
+	struct device *dev = gpio_dev->rcdev->dev.parent;
+
+	/*
+	 * For cpuidle system:
+	 * 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 interupt, another is delay introduced by async api.
+	 */
+	ret = pm_runtime_get(dev);
+	if (ret < 0)
+		return IRQ_NONE;
 
 	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;
 }
 
@@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, gpio_dev);
 
+
+	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 / 1000));
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_enable(dev);
+
 	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
 				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 				"gpio-ir-recv-irq", gpio_dev);
@@ -122,9 +149,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] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Joakim Zhang]

Hi Sean,

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月18日 16:24
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> Hi Joakim,
> 
> On Fri, Sep 18, 2020 at 01:42:15AM +0000, Joakim Zhang wrote:
> > > -----Original Message-----
> > > From: Sean Young <sean@mess.org>
> > > Sent: 2020年9月18日 4:44
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for
> > > cpuidle system
> 
> -snip-
> 
> > > > Autosuspend delay should be fixed value, should be set to gpio
> > > > device timeout
> > > value, which is 125ms.
> > >
> > > So the idea was that cpuidle is only enabled while IR frames are
> > > being received, that's why I suggested it.
> >
> > May be a typo, "cpuidle is only DISABLED while IR frames are being receive,",
> this is not I want to implement, experiments have also shown poor results.
> 
> Sorry, yes I got this wrong. You are right.
> 
> > > If you set the autosuspend delay to 125ms, then the cpuidle will not
> > > be enabled between IR frames. Maybe this is what you want, but it
> > > does mean cpuidle is totally suspended while anyone is pressing buttons on
> a remote.
> >
> > Yes, this is what I want, cpuidle is totally disabled while pressing buttons,
> disable cpuidle at the first frame then keep disabled until there is no activity for
> a while.
> > So that we only can not decode the first frame, such as, if transmitting 4
> frames once, we can correctly decode 3 times.
> >
> > I also try your suggestion, set autosuspend delay time to protocol's
> > timeout value, but the result is terrible. If transmitting 4 frames once, we
> can't correctly decode 3 times, even can't decode it sometime. The sequence is,
> cpu in idle state when the first frame coming, then disable cpu idle until
> protocols' timeout, cpu in idle state again, the first frame can't be decoded.
> > The second frame coming, it will repeat the behavior of the first frame, may
> cause the second frame can't be decode......
> >
> > Can you take account of I have done in the first version, autosuspend delay
> time is set to 125ms?
> 
> Yes, in retrospect you are right. Trying to shorten the cpuidle suspended period
> will not work. I am sorry about this.
> 
> How about setting the autosuspend period in devicetree, and 0 will turn this
> feature off completely?

Of course, we can do this. Such as add a linux,delaytime property:
For those systems that don't suffer this issue, need not add this property, instead of check the value is 0 as you suggested.
For those systems that would suffer this issue, need add this property and then give a appropriate value

So that users can change the autosuspend delay time via device tree, it is more flexible for different systems. If you agree with it, I will send a V2.

Best Regards,
Joakim Zhang
> Thanks,
> 
> Sean

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

[Sean Young]

Hi Joakim,

On Fri, Sep 18, 2020 at 01:42:15AM +0000, Joakim Zhang wrote:
> > -----Original Message-----
> > From: Sean Young <sean@mess.org>
> > Sent: 2020年9月18日 4:44
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> > system

-snip-

> > > Autosuspend delay should be fixed value, should be set to gpio device timeout
> > value, which is 125ms.
> > 
> > So the idea was that cpuidle is only enabled while IR frames are being received,
> > that's why I suggested it.
> 
> May be a typo, "cpuidle is only DISABLED while IR frames are being receive,", this is not I want to implement, experiments have also shown poor results.

Sorry, yes I got this wrong. You are right.

> > If you set the autosuspend delay to 125ms, then the cpuidle will not be enabled
> > between IR frames. Maybe this is what you want, but it does mean cpuidle is
> > totally suspended while anyone is pressing buttons on a remote.
> 
> Yes, this is what I want, cpuidle is totally disabled while pressing buttons, disable cpuidle at the first frame then keep disabled until there is no activity for a while.
> So that we only can not decode the first frame, such as, if transmitting 4 frames once, we can correctly decode 3 times.
> 
> I also try your suggestion, set autosuspend delay time to protocol's timeout value, but the result is terrible. If transmitting 4 frames once, we can't correctly decode 3 times,
> even can't decode it sometime. The sequence is, cpu in idle state when the first frame coming, then disable cpu idle until protocols' timeout, cpu in idle state again, the first frame can't be decoded.
> The second frame coming, it will repeat the behavior of the first frame, may cause the second frame can't be decode......
> 
> Can you take account of I have done in the first version, autosuspend delay time is set to 125ms?

Yes, in retrospect you are right. Trying to shorten the cpuidle suspended
period will not work. I am sorry about this.

How about setting the autosuspend period in devicetree, and 0 will turn
this feature off completely?

Thanks,

Sean

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

[Joakim Zhang]

Hi Sean,

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月18日 4:44
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> Hi Joakim,
> 
> On Thu, Sep 17, 2020 at 09:12:32AM +0000, Joakim Zhang wrote:
> >
> > > -----Original Message-----
> > > From: Sean Young <sean@mess.org>
> > > Sent: 2020年9月15日 17:34
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for
> > > cpuidle system
> > >
> > >
> >
> > [...]
> > > > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > > > platform_device *pdev)
> > > >
> > > >  	platform_set_drvdata(pdev, gpio_dev);
> > > >
> > > > +
> > > > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > > > +1000));
> > >
> > > rcdev->timeout is in microseconds (since very recently), so this is wrong.
> > > Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT
> > > ioctl (using ir-ctl -t in userspace). The autosuspend delay should
> > > be updated when this happens. This can be done by implementing the
> s_timeout rcdev function.
> >
> > Hi Sean,
> >
> > I come across a problem when implementing this feature.
> >
> > At probe stage, devm_rc_register_device -> change_protocol, then timeout
> set to 125ms.
> >
> > When echo sony or nec to protocols, will call change_protocol changing the
> timeout value, that timeout would change to handler->min_timeout + 10ms.
> For sony is 16000000ns, for 15625000ns.
> 
> The sony protocol decoder wants a 6ms space after the last bit. So, the timeout
> for the IR receiver can be set to 6ms (plus 10ms margin). This has the
> advantage that decoding is happens very quickly. Before this change, there as a
> 125ms delay before the first and last IR frame was decoded. This causes
> decoding to feel laggy and keys also a bit sticky.

Yes, I can understand this, each IR protocols have their own timeout, this is reasonable.


> > This is not the way I want to take before, this would frequently disable/enable
> cpuidle. So is it necessary to provide s_timeout, this callback should be used to
> change protocols' timeout?
> > If implement s_timeout, users need change the timeout value from
> userspace, this is a mandatory operation and unfriendly. And it will affect
> protocol's timeout.
> >
> > Autosuspend delay should be fixed value, should be set to gpio device timeout
> value, which is 125ms.
> 
> So the idea was that cpuidle is only enabled while IR frames are being received,
> that's why I suggested it.

May be a typo, "cpuidle is only DISABLED while IR frames are being receive,", this is not I want to implement, experiments have also shown poor results.

> If you set the autosuspend delay to 125ms, then the cpuidle will not be enabled
> between IR frames. Maybe this is what you want, but it does mean cpuidle is
> totally suspended while anyone is pressing buttons on a remote.

Yes, this is what I want, cpuidle is totally disabled while pressing buttons, disable cpuidle at the first frame then keep disabled until there is no activity for a while.
So that we only can not decode the first frame, such as, if transmitting 4 frames once, we can correctly decode 3 times.

I also try your suggestion, set autosuspend delay time to protocol's timeout value, but the result is terrible. If transmitting 4 frames once, we can't correctly decode 3 times,
even can't decode it sometime. The sequence is, cpu in idle state when the first frame coming, then disable cpu idle until protocols' timeout, cpu in idle state again, the first frame can't be decoded.
The second frame coming, it will repeat the behavior of the first frame, may cause the second frame can't be decode......

Can you take account of I have done in the first version, autosuspend delay time is set to 125ms?

> Thanks
> Sean
> 
> >
> > Best Regards,
> > Joakim Zhang
> > > > +	pm_runtime_use_autosuspend(dev);
> > > > +	pm_runtime_set_suspended(dev);
> > > > +	pm_runtime_enable(dev);
> > > > +
> > > >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> > > >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > > >  				"gpio-ir-recv-irq", gpio_dev);

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

[Sean Young]

Hi Joakim,

On Thu, Sep 17, 2020 at 09:12:32AM +0000, Joakim Zhang wrote:
> 
> > -----Original Message-----
> > From: Sean Young <sean@mess.org>
> > Sent: 2020年9月15日 17:34
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> > system
> > 
> > 
> 
> [...]
> > > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > > platform_device *pdev)
> > >
> > >  	platform_set_drvdata(pdev, gpio_dev);
> > >
> > > +
> > > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > > +1000));
> > 
> > rcdev->timeout is in microseconds (since very recently), so this is wrong.
> > Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT ioctl
> > (using ir-ctl -t in userspace). The autosuspend delay should be updated when
> > this happens. This can be done by implementing the s_timeout rcdev function.
> 
> Hi Sean,
> 
> I come across a problem when implementing this feature.
> 
> At probe stage, devm_rc_register_device -> change_protocol, then timeout set to 125ms.
> 
> When echo sony or nec to protocols, will call change_protocol changing the timeout value, that timeout would change to handler->min_timeout + 10ms. For sony is 16000000ns, for 15625000ns.

The sony protocol decoder wants a 6ms space after the last bit. So, the timeout
for the IR receiver can be set to 6ms (plus 10ms margin). This has the
advantage that decoding is happens very quickly. Before this change, there
as a 125ms delay before the first and last IR frame was decoded. This
causes decoding to feel laggy and keys also a bit sticky.

> This is not the way I want to take before, this would frequently disable/enable cpuidle. So is it necessary to provide s_timeout, this callback should be used to change protocols' timeout?
> If implement s_timeout, users need change the timeout value from userspace, this is a mandatory operation and unfriendly. And it will affect protocol's timeout.
> 
> Autosuspend delay should be fixed value, should be set to gpio device timeout value, which is 125ms.

So the idea was that cpuidle is only enabled while IR frames are being
received, that's why I suggested it.

If you set the autosuspend delay to 125ms, then the cpuidle will not be enabled
between IR frames. Maybe this is what you want, but it does mean cpuidle
is totally suspended while anyone is pressing buttons on a remote.

Thanks
Sean

> 
> Best Regards,
> Joakim Zhang
> > > +	pm_runtime_use_autosuspend(dev);
> > > +	pm_runtime_set_suspended(dev);
> > > +	pm_runtime_enable(dev);
> > > +
> > >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> > >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > >  				"gpio-ir-recv-irq", gpio_dev);

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

[Joakim Zhang]

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月15日 17:34
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> 

[...]
> > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, gpio_dev);
> >
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > +1000));
> 
> rcdev->timeout is in microseconds (since very recently), so this is wrong.
> Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT ioctl
> (using ir-ctl -t in userspace). The autosuspend delay should be updated when
> this happens. This can be done by implementing the s_timeout rcdev function.

Hi Sean,

I come across a problem when implementing this feature.

At probe stage, devm_rc_register_device -> change_protocol, then timeout set to 125ms.

When echo sony or nec to protocols, will call change_protocol changing the timeout value, that timeout would change to handler->min_timeout + 10ms. For sony is 16000000ns, for 15625000ns.
This is not the way I want to take before, this would frequently disable/enable cpuidle. So is it necessary to provide s_timeout, this callback should be used to change protocols' timeout?
If implement s_timeout, users need change the timeout value from userspace, this is a mandatory operation and unfriendly. And it will affect protocol's timeout.

Autosuspend delay should be fixed value, should be set to gpio device timeout value, which is 125ms.

Best Regards,
Joakim Zhang
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_suspended(dev);
> > +	pm_runtime_enable(dev);
> > +
> >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> >  				"gpio-ir-recv-irq", gpio_dev);

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

[Sean Young]

Hi Joakim,

On Wed, Sep 16, 2020 at 10:22:11AM +0000, Joakim Zhang wrote:
> 
> Hi Sean,
> 
> Thanks for your hint, I will send a V2 soon according to your suggestions.
> 
> We also have a concern, since you are a IR expert, may you can give us answers. With this patch, the first frame once press could not been decoded.
> AFAIK, IR protocols have not specify how may frames transmitting once press, is there ang criterion to decide this?
> 
> Is it possible that single frame transmitting once pressing? Per my understanding, it will transmit more than one frame.

So remotes send IR signals while a button is being pressed down. For the
remotes I've seen, when pressing a button a short amount of time will repeat
the IR message at least three times. This is a few times when I've tried
this, but by no means exhaustive of all remotes or protocols.

I think the question you are trying to answer is, if we miss the first
message, will we at least have another chance if the message is repeated?

So I think the message will be repeated, but the repeat message is not
enough for the nec protocol. The nec repeat is a shorter message which
does not carry any information apart from "last key still pressed":

https://www.sbprojects.net/knowledge/ir/nec.php

Thanks,

Sean

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

[Joakim Zhang]

Hi Sean,

Thanks for your hint, I will send a V2 soon according to your suggestions.

We also have a concern, since you are a IR expert, may you can give us answers. With this patch, the first frame once press could not been decoded.
AFAIK, IR protocols have not specify how may frames transmitting once press, is there ang criterion to decide this?

Is it possible that single frame transmitting once pressing? Per my understanding, it will transmit more than one frame.

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月16日 4:20
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> Hi Joakim,
> 
> On Tue, Sep 15, 2020 at 10:55:17AM +0000, Joakim Zhang wrote:
> >
> > Hi Sean,
> >
> > Thanks a lot for your review.
> >
> > > -----Original Message-----
> > > From: Sean Young <sean@mess.org>
> > > Sent: 2020年9月15日 17:34
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for
> > > cpuidle system
> > >
> > >
> > > Hi Joakim,
> > >
> > > Thanks for your patch, I think it looks good in principle but needs
> > > a few small fixes.
> > >
> > > On Tue, Sep 15, 2020 at 11:02:02PM +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 systems
> > > > which supports cpuidle, dynamically disable and enable cpuidle can
> > > > solve this problem to a great extent.
> > >
> > > This is the first soc to be affected by this problem, and
> > > gpio-ir-recv has been used on my systems. For example, the raspberry
> > > pi has cpu idle enabled and does not suffer from this problem. There
> > > are many more; this driver has been used on many arm devices, which will
> have cpuidle enabled.
> > I have not noticed raspberry pi enabled cpu idle in Linux, could you point me
> where it is? Then I can have a look, whether it implements multiple idle or not,
> to see why it makes difference.
> 
> I just noticed that it not enabled on raspbian kernel, but it is enabled on fedora:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsrc.fedo
> raproject.org%2Frpms%2Fkernel%2Fblob%2Fmaster%2Ff%2Fkernel-armv7hl-fe
> dora.config&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.com%7C80f939
> 09a074443de17e08d859b4b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C1%7C637357979905274906&amp;sdata=TSZHAwWip6s5LJ1TGzyaGD38Z
> vJ2QRJiZyV9TH9%2F%2F3w%3D&amp;reserved=0
> 
> When I use gpio-ir-recv with my own kernel and fedora kernel, there no
> problems with gpio-ir-recv on this kernel.
> 
> > > > 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.
> > > >
> > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > ---
> > > >  drivers/media/rc/gpio-ir-recv.c | 49
> > > > ++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/rc/gpio-ir-recv.c
> > > > b/drivers/media/rc/gpio-ir-recv.c index a20413008c3c..42c942ce98cd
> > > > 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,36 @@ struct gpio_rc_dev {
> > > >  	struct rc_dev *rcdev;
> > > >  	struct gpio_desc *gpiod;
> > > >  	int irq;
> > > > +	struct pm_qos_request qos;
> > > >  };
> > > >
> > > >  static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)  {
> > > > -	int val;
> > > > +	int ret, val;
> > > >  	struct gpio_rc_dev *gpio_dev = dev_id;
> > > > +	struct device *dev = gpio_dev->rcdev->dev.parent;
> > > > +
> > > > +	/*
> > > > +	 * For cpuidle system:
> > >
> > > For some cpuidle systems, not all. This is why this feature needs a
> > > device tree option for enabling. Otherwise, it will negatively affect power
> usage on e.g.
> > > raspberry pi.
> > Yes, you are right. As you said, some cpu idle systems may not suffer such
> case, I will add a device tree property.
> >
> >
> > > > +	 * 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 interupt, another is delay introduced by async api.
> > > > +	 */
> > > > +	ret = pm_runtime_get(dev);
> > > > +	if (ret < 0)
> > > > +		return IRQ_NONE;
> > >
> > > If we end up here, we also abandon sending the IR to rc-core
> > > (below). I don't think it should do that. It should call
> > > ir_raw_event_store_edge() always even if it can't do the pm things.
> > Make sense, I will remove this error check.
> >
> >
> > > >
> > > >  	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;
> > > >  }
> > > >
> > > > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > > > platform_device *pdev)
> > > >
> > > >  	platform_set_drvdata(pdev, gpio_dev);
> > > >
> > > > +
> > > > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > > > +1000));
> > >
> > > rcdev->timeout is in microseconds (since very recently), so this is wrong.
> > What this meaning, is that rcdev->timeout woud change the unit, to be
> microseconds, not nanoseconds any more ?
> 
> See:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.linux
> tv.org%2Fmedia_tree.git%2Fcommit%2F%3Fid%3D528222d853f9283110f0118
> dd71d9f0ad686d586&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.com%7
> C80f93909a074443de17e08d859b4b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C1%7C637357979905274906&amp;sdata=0M2Mzz5zCwavK8mHT
> %2B8UO%2BhDM2OvbY3sy%2Br516SG0nY%3D&amp;reserved=0
> 
> > > Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT
> > > ioctl (using ir-ctl -t in userspace). The autosuspend delay should
> > > be updated when this happens. This can be done by implementing the
> s_timeout rcdev function.
> > Make sense, could you point me where s_timeout funcition has implemented?
> So that I can refer to it, I am not familiar with this.
> 
> See:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.linux
> tv.org%2Fmedia_tree.git%2Ftree%2Fdrivers%2Fmedia%2Frc%2Fredrat3.c%3Fi
> d%3D528222d853f9283110f0118dd71d9f0ad686d586%23n952&amp;data=02
> %7C01%7Cqiangqing.zhang%40nxp.com%7C80f93909a074443de17e08d859b4
> b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C6373579799052
> 74906&amp;sdata=gPWKtedeWMzWFsC4ebCPGq8HTkz251a3OVRLQlHC7Oc%
> 3D&amp;reserved=0
> 
> 
> >
> >
> > BRs,
> > Joakim
> > > > +	pm_runtime_use_autosuspend(dev);
> > > > +	pm_runtime_set_suspended(dev);
> > > > +	pm_runtime_enable(dev);
> > > > +
> > > >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> > > >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > > >  				"gpio-ir-recv-irq", gpio_dev); @@ -122,9 +149,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] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Sean Young]

Hi Joakim,

On Tue, Sep 15, 2020 at 10:55:17AM +0000, Joakim Zhang wrote:
> 
> Hi Sean,
> 
> Thanks a lot for your review.
> 
> > -----Original Message-----
> > From: Sean Young <sean@mess.org>
> > Sent: 2020年9月15日 17:34
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> > system
> > 
> > 
> > Hi Joakim,
> > 
> > Thanks for your patch, I think it looks good in principle but needs a few small
> > fixes.
> > 
> > On Tue, Sep 15, 2020 at 11:02:02PM +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 systems which supports cpuidle,
> > > dynamically disable and enable cpuidle can solve this problem to a
> > > great extent.
> > 
> > This is the first soc to be affected by this problem, and gpio-ir-recv has been
> > used on my systems. For example, the raspberry pi has cpu idle enabled and
> > does not suffer from this problem. There are many more; this driver has been
> > used on many arm devices, which will have cpuidle enabled.
> I have not noticed raspberry pi enabled cpu idle in Linux, could you point me where it is? Then I can have a look, whether it implements multiple idle or not, to see why it makes difference.

I just noticed that it not enabled on raspbian kernel, but it is enabled
on fedora:

https://src.fedoraproject.org/rpms/kernel/blob/master/f/kernel-armv7hl-fedora.config

When I use gpio-ir-recv with my own kernel and fedora kernel, there no
problems with gpio-ir-recv on this kernel.

> > > 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.
> > >
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > >  drivers/media/rc/gpio-ir-recv.c | 49
> > > ++++++++++++++++++++++++++++++++-
> > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/rc/gpio-ir-recv.c
> > > b/drivers/media/rc/gpio-ir-recv.c index a20413008c3c..42c942ce98cd
> > > 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,36 @@ struct gpio_rc_dev {
> > >  	struct rc_dev *rcdev;
> > >  	struct gpio_desc *gpiod;
> > >  	int irq;
> > > +	struct pm_qos_request qos;
> > >  };
> > >
> > >  static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)  {
> > > -	int val;
> > > +	int ret, val;
> > >  	struct gpio_rc_dev *gpio_dev = dev_id;
> > > +	struct device *dev = gpio_dev->rcdev->dev.parent;
> > > +
> > > +	/*
> > > +	 * For cpuidle system:
> > 
> > For some cpuidle systems, not all. This is why this feature needs a device tree
> > option for enabling. Otherwise, it will negatively affect power usage on e.g.
> > raspberry pi.
> Yes, you are right. As you said, some cpu idle systems may not suffer such case, I will add a device tree property.
> 
> 
> > > +	 * 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 interupt, another is delay introduced by async api.
> > > +	 */
> > > +	ret = pm_runtime_get(dev);
> > > +	if (ret < 0)
> > > +		return IRQ_NONE;
> > 
> > If we end up here, we also abandon sending the IR to rc-core (below). I don't
> > think it should do that. It should call ir_raw_event_store_edge() always even if
> > it can't do the pm things.
> Make sense, I will remove this error check.
> 
> 
> > >
> > >  	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;
> > >  }
> > >
> > > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > > platform_device *pdev)
> > >
> > >  	platform_set_drvdata(pdev, gpio_dev);
> > >
> > > +
> > > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > > +1000));
> > 
> > rcdev->timeout is in microseconds (since very recently), so this is wrong.
> What this meaning, is that rcdev->timeout woud change the unit, to be microseconds, not nanoseconds any more ?

See:

https://git.linuxtv.org/media_tree.git/commit/?id=528222d853f9283110f0118dd71d9f0ad686d586

> > Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT ioctl
> > (using ir-ctl -t in userspace). The autosuspend delay should be updated when
> > this happens. This can be done by implementing the s_timeout rcdev function.
> Make sense, could you point me where s_timeout funcition has implemented? So that I can refer to it, I am not familiar with this.

See:

https://git.linuxtv.org/media_tree.git/tree/drivers/media/rc/redrat3.c?id=528222d853f9283110f0118dd71d9f0ad686d586#n952


> 
> 
> BRs,
> Joakim
> > > +	pm_runtime_use_autosuspend(dev);
> > > +	pm_runtime_set_suspended(dev);
> > > +	pm_runtime_enable(dev);
> > > +
> > >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> > >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > >  				"gpio-ir-recv-irq", gpio_dev);
> > > @@ -122,9 +149,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

[PATCH] 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 systems which supports
cpuidle, 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.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/media/rc/gpio-ir-recv.c | 49 ++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index a20413008c3c..42c942ce98cd 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,36 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	struct gpio_desc *gpiod;
 	int irq;
+	struct pm_qos_request qos;
 };
 
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
-	int val;
+	int ret, val;
 	struct gpio_rc_dev *gpio_dev = dev_id;
+	struct device *dev = gpio_dev->rcdev->dev.parent;
+
+	/*
+	 * For cpuidle system:
+	 * 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 interupt, another is delay introduced by async api.
+	 */
+	ret = pm_runtime_get(dev);
+	if (ret < 0)
+		return IRQ_NONE;
 
 	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;
 }
 
@@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, gpio_dev);
 
+
+	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 / 1000));
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_enable(dev);
+
 	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
 				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 				"gpio-ir-recv-irq", gpio_dev);
@@ -122,9 +149,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] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Joakim Zhang]

Hi Sean,

Thanks a lot for your review.

> -----Original Message-----
> From: Sean Young <sean@mess.org>
> Sent: 2020年9月15日 17:34
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mchehab@kernel.org; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle
> system
> 
> 
> Hi Joakim,
> 
> Thanks for your patch, I think it looks good in principle but needs a few small
> fixes.
> 
> On Tue, Sep 15, 2020 at 11:02:02PM +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 systems which supports cpuidle,
> > dynamically disable and enable cpuidle can solve this problem to a
> > great extent.
> 
> This is the first soc to be affected by this problem, and gpio-ir-recv has been
> used on my systems. For example, the raspberry pi has cpu idle enabled and
> does not suffer from this problem. There are many more; this driver has been
> used on many arm devices, which will have cpuidle enabled.
I have not noticed raspberry pi enabled cpu idle in Linux, could you point me where it is? Then I can have a look, whether it implements multiple idle or not, to see why it makes difference.


> >
> > 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.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/media/rc/gpio-ir-recv.c | 49
> > ++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/rc/gpio-ir-recv.c
> > b/drivers/media/rc/gpio-ir-recv.c index a20413008c3c..42c942ce98cd
> > 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,36 @@ struct gpio_rc_dev {
> >  	struct rc_dev *rcdev;
> >  	struct gpio_desc *gpiod;
> >  	int irq;
> > +	struct pm_qos_request qos;
> >  };
> >
> >  static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)  {
> > -	int val;
> > +	int ret, val;
> >  	struct gpio_rc_dev *gpio_dev = dev_id;
> > +	struct device *dev = gpio_dev->rcdev->dev.parent;
> > +
> > +	/*
> > +	 * For cpuidle system:
> 
> For some cpuidle systems, not all. This is why this feature needs a device tree
> option for enabling. Otherwise, it will negatively affect power usage on e.g.
> raspberry pi.
Yes, you are right. As you said, some cpu idle systems may not suffer such case, I will add a device tree property.


> > +	 * 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 interupt, another is delay introduced by async api.
> > +	 */
> > +	ret = pm_runtime_get(dev);
> > +	if (ret < 0)
> > +		return IRQ_NONE;
> 
> If we end up here, we also abandon sending the IR to rc-core (below). I don't
> think it should do that. It should call ir_raw_event_store_edge() always even if
> it can't do the pm things.
Make sense, I will remove this error check.


> >
> >  	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;
> >  }
> >
> > @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct
> > platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, gpio_dev);
> >
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 /
> > +1000));
> 
> rcdev->timeout is in microseconds (since very recently), so this is wrong.
What this meaning, is that rcdev->timeout woud change the unit, to be microseconds, not nanoseconds any more ?


> Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT ioctl
> (using ir-ctl -t in userspace). The autosuspend delay should be updated when
> this happens. This can be done by implementing the s_timeout rcdev function.
Make sense, could you point me where s_timeout funcition has implemented? So that I can refer to it, I am not familiar with this.


BRs,
Joakim
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_suspended(dev);
> > +	pm_runtime_enable(dev);
> > +
> >  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
> >  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> >  				"gpio-ir-recv-irq", gpio_dev);
> > @@ -122,9 +149,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] media: rc: gpio-ir-recv: add QoS support for cpuidle system

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 11:06 AM Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: 2020年9月15日 15:18
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: sean@mess.org; mchehab@kernel.org; linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle system
> On Tuesday, September 15, 2020, Joakim Zhang <qiangqing.zhang@nxp.com> wrote:




> +       ret = pm_runtime_get(dev);
> +       if (ret < 0)
>
>
>
> Here is reference counter leak.
>
>
>
> Thanks Andy for your kindly review.
>
> pm_runtime_get increase the usage_count even it fails, so need call pm_runtime_put_noidle here to balance the usage_count. Am I right?

Yes.

> +               return IRQ_NONE;


-- 
With Best Regards,
Andy Shevchenko

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

[Sean Young]

Hi Joakim,

Thanks for your patch, I think it looks good in principle but needs a
few small fixes.

On Tue, Sep 15, 2020 at 11:02:02PM +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 systems which supports
> cpuidle, dynamically disable and enable cpuidle can solve this problem
> to a great extent.

This is the first soc to be affected by this problem, and gpio-ir-recv
has been used on my systems. For example, the raspberry pi has cpu idle
enabled and does not suffer from this problem. There are many more; this
driver has been used on many arm devices, which will have cpuidle enabled.

> 
> 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.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/media/rc/gpio-ir-recv.c | 49 ++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> index a20413008c3c..42c942ce98cd 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,36 @@ struct gpio_rc_dev {
>  	struct rc_dev *rcdev;
>  	struct gpio_desc *gpiod;
>  	int irq;
> +	struct pm_qos_request qos;
>  };
>  
>  static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>  {
> -	int val;
> +	int ret, val;
>  	struct gpio_rc_dev *gpio_dev = dev_id;
> +	struct device *dev = gpio_dev->rcdev->dev.parent;
> +
> +	/*
> +	 * For cpuidle system:

For some cpuidle systems, not all. This is why this feature needs a
device tree option for enabling. Otherwise, it will negatively affect
power usage on e.g. raspberry pi.

> +	 * 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 interupt, another is delay introduced by async api.
> +	 */
> +	ret = pm_runtime_get(dev);
> +	if (ret < 0)
> +		return IRQ_NONE;

If we end up here, we also abandon sending the IR to rc-core (below). I
don't think it should do that. It should call ir_raw_event_store_edge()
always even if it can't do the pm things.

>  
>  	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;
>  }
>  
> @@ -92,6 +113,12 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, gpio_dev);
>  
> +
> +	pm_runtime_set_autosuspend_delay(dev, (rcdev->timeout / 1000 / 1000));

rcdev->timeout is in microseconds (since very recently), so this is wrong.
Also, the timeout can be changed using the LIRC_SET_REC_TIMEOUT ioctl
(using ir-ctl -t in userspace). The autosuspend delay should be updated
when this happens. This can be done by implementing the s_timeout rcdev
function.

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_enable(dev);
> +
>  	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
>  				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>  				"gpio-ir-recv-irq", gpio_dev);
> @@ -122,9 +149,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