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://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@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://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
