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@xxxxxxxx>
> Sent: 2020年9月22日 17:42
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: mchehab@xxxxxxxxxx; robh+dt@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> 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@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.

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