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@xxxxxxx>
> ---
> 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
