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

From: Joakim Zhang
Date: Wed Sep 16 2020 - 06:47:37 EST

Next message: Codrin.Ciubotariu: "Re: [PATCH net] net: dsa: microchip: ksz8795: really set the correct number of ports"
Previous message: Mansur Alisha Shaik: "[PATCH v2 1/4] venus: core: change clk enable and disable order in resume and suspend"
Next in thread: Sean Young: "Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle system"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxx>
> Sent: 2020年9月16日 4:20
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: mchehab@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> 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@xxxxxxxx>
> > > Sent: 2020年9月15日 17:34
> > > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > > Cc: mchehab@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> > > 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&data=02%7C01%7Cqiangqing.zhang%40nxp.com%7C80f939
> 09a074443de17e08d859b4b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C1%7C637357979905274906&sdata=TSZHAwWip6s5LJ1TGzyaGD38Z
> vJ2QRJiZyV9TH9%2F%2F3w%3D&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@xxxxxxx>
> > > > ---
> > > > 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&data=02%7C01%7Cqiangqing.zhang%40nxp.com%7
> C80f93909a074443de17e08d859b4b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C1%7C637357979905274906&sdata=0M2Mzz5zCwavK8mHT
> %2B8UO%2BhDM2OvbY3sy%2Br516SG0nY%3D&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&data=02
> %7C01%7Cqiangqing.zhang%40nxp.com%7C80f93909a074443de17e08d859b4
> b1fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C6373579799052
> 74906&sdata=gPWKtedeWMzWFsC4ebCPGq8HTkz251a3OVRLQlHC7Oc%
> 3D&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

Next message: Codrin.Ciubotariu: "Re: [PATCH net] net: dsa: microchip: ksz8795: really set the correct number of ports"
Previous message: Mansur Alisha Shaik: "[PATCH v2 1/4] venus: core: change clk enable and disable order in resume and suspend"
Next in thread: Sean Young: "Re: [PATCH] media: rc: gpio-ir-recv: add QoS support for cpuidle system"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]