[PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Sean Young]

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
handled entirely by the generic pwm-ir-tx driver, and in fact the
pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Note that the suspend code in the ir-rx51 driver is unnecessary, since
during transmit, the process is not in interruptable sleep. The process
is not put to sleep until the transmit completes.

Cc: Timo Kokkonen <timo.t.kokkonen@iki.fi>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Tested-by: Sicelo A. Mhlongo <absicsz@gmail.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 arch/arm/configs/omap2plus_defconfig |   1 -
 drivers/media/rc/Kconfig             |  10 -
 drivers/media/rc/Makefile            |   1 -
 drivers/media/rc/ir-rx51.c           | 285 ---------------------------
 drivers/media/rc/pwm-ir-tx.c         |   1 +
 5 files changed, 1 insertion(+), 297 deletions(-)
 delete mode 100644 drivers/media/rc/ir-rx51.c

diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig
index b685018dcf54..ef39ab57b75a 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -484,7 +484,6 @@ CONFIG_LIRC=y
 CONFIG_RC_DEVICES=y
 CONFIG_IR_GPIO_TX=m
 CONFIG_IR_PWM_TX=m
-CONFIG_IR_RX51=m
 CONFIG_IR_SPI=m
 CONFIG_MEDIA_SUPPORT=m
 CONFIG_V4L_PLATFORM_DRIVERS=y
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 922c790b577e..d1b98dd5bee6 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -337,16 +337,6 @@ config IR_REDRAT3
 	   To compile this driver as a module, choose M here: the
 	   module will be called redrat3.
 
-config IR_RX51
-	tristate "Nokia N900 IR transmitter diode"
-	depends on (OMAP_DM_TIMER && PWM_OMAP_DMTIMER && ARCH_OMAP2PLUS || COMPILE_TEST) && RC_CORE
-	help
-	   Say Y or M here if you want to enable support for the IR
-	   transmitter diode built in the Nokia N900 (RX51) device.
-
-	   The driver uses omap DM timers for generating the carrier
-	   wave and pulses.
-
 config IR_SERIAL
 	tristate "Homebrew Serial Port Receiver"
 	depends on HAS_IOPORT
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index a9285266e944..2bca6f7f07bc 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -43,7 +43,6 @@ obj-$(CONFIG_IR_MTK) += mtk-cir.o
 obj-$(CONFIG_IR_NUVOTON) += nuvoton-cir.o
 obj-$(CONFIG_IR_PWM_TX) += pwm-ir-tx.o
 obj-$(CONFIG_IR_REDRAT3) += redrat3.o
-obj-$(CONFIG_IR_RX51) += ir-rx51.o
 obj-$(CONFIG_IR_SERIAL) += serial_ir.o
 obj-$(CONFIG_IR_SPI) += ir-spi.o
 obj-$(CONFIG_IR_STREAMZAP) += streamzap.o
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
deleted file mode 100644
index adbbe639a261..000000000000
--- a/drivers/media/rc/ir-rx51.c
+++ /dev/null
@@ -1,285 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- *  Copyright (C) 2008 Nokia Corporation
- *
- *  Based on lirc_serial.c
- */
-#include <linux/clk.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/wait.h>
-#include <linux/pwm.h>
-#include <linux/of.h>
-#include <linux/hrtimer.h>
-
-#include <media/rc-core.h>
-
-#define WBUF_LEN 256
-
-struct ir_rx51 {
-	struct rc_dev *rcdev;
-	struct pwm_device *pwm;
-	struct pwm_state state;
-	struct hrtimer timer;
-	struct device	     *dev;
-	wait_queue_head_t     wqueue;
-
-	unsigned int	freq;		/* carrier frequency */
-	unsigned int	duty_cycle;	/* carrier duty cycle */
-	int		wbuf[WBUF_LEN];
-	int		wbuf_index;
-	unsigned long	device_is_open;
-};
-
-static inline void ir_rx51_on(struct ir_rx51 *ir_rx51)
-{
-	ir_rx51->state.enabled = true;
-	pwm_apply_state(ir_rx51->pwm, &ir_rx51->state);
-}
-
-static inline void ir_rx51_off(struct ir_rx51 *ir_rx51)
-{
-	ir_rx51->state.enabled = false;
-	pwm_apply_state(ir_rx51->pwm, &ir_rx51->state);
-}
-
-static int init_timing_params(struct ir_rx51 *ir_rx51)
-{
-	ir_rx51->state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq);
-	pwm_set_relative_duty_cycle(&ir_rx51->state, ir_rx51->duty_cycle, 100);
-
-	return 0;
-}
-
-static enum hrtimer_restart ir_rx51_timer_cb(struct hrtimer *timer)
-{
-	struct ir_rx51 *ir_rx51 = container_of(timer, struct ir_rx51, timer);
-	ktime_t now;
-
-	if (ir_rx51->wbuf_index < 0) {
-		dev_err_ratelimited(ir_rx51->dev,
-				    "BUG wbuf_index has value of %i\n",
-				    ir_rx51->wbuf_index);
-		goto end;
-	}
-
-	/*
-	 * If we happen to hit an odd latency spike, loop through the
-	 * pulses until we catch up.
-	 */
-	do {
-		u64 ns;
-
-		if (ir_rx51->wbuf_index >= WBUF_LEN)
-			goto end;
-		if (ir_rx51->wbuf[ir_rx51->wbuf_index] == -1)
-			goto end;
-
-		if (ir_rx51->wbuf_index % 2)
-			ir_rx51_off(ir_rx51);
-		else
-			ir_rx51_on(ir_rx51);
-
-		ns = US_TO_NS(ir_rx51->wbuf[ir_rx51->wbuf_index]);
-		hrtimer_add_expires_ns(timer, ns);
-
-		ir_rx51->wbuf_index++;
-
-		now = timer->base->get_time();
-
-	} while (hrtimer_get_expires_tv64(timer) < now);
-
-	return HRTIMER_RESTART;
-end:
-	/* Stop TX here */
-	ir_rx51_off(ir_rx51);
-	ir_rx51->wbuf_index = -1;
-
-	wake_up_interruptible(&ir_rx51->wqueue);
-
-	return HRTIMER_NORESTART;
-}
-
-static int ir_rx51_tx(struct rc_dev *dev, unsigned int *buffer,
-		      unsigned int count)
-{
-	struct ir_rx51 *ir_rx51 = dev->priv;
-
-	if (count > WBUF_LEN)
-		return -EINVAL;
-
-	memcpy(ir_rx51->wbuf, buffer, count * sizeof(unsigned int));
-
-	/* Wait any pending transfers to finish */
-	wait_event_interruptible(ir_rx51->wqueue, ir_rx51->wbuf_index < 0);
-
-	init_timing_params(ir_rx51);
-	if (count < WBUF_LEN)
-		ir_rx51->wbuf[count] = -1; /* Insert termination mark */
-
-	/*
-	 * REVISIT: Adjust latency requirements so the device doesn't go in too
-	 * deep sleep states with pm_qos_add_request().
-	 */
-
-	ir_rx51_on(ir_rx51);
-	ir_rx51->wbuf_index = 1;
-	hrtimer_start(&ir_rx51->timer,
-		      ns_to_ktime(US_TO_NS(ir_rx51->wbuf[0])),
-		      HRTIMER_MODE_REL);
-	/*
-	 * Don't return back to the userspace until the transfer has
-	 * finished
-	 */
-	wait_event_interruptible(ir_rx51->wqueue, ir_rx51->wbuf_index < 0);
-
-	/* REVISIT: Remove pm_qos constraint, we can sleep again */
-
-	return count;
-}
-
-static int ir_rx51_open(struct rc_dev *dev)
-{
-	struct ir_rx51 *ir_rx51 = dev->priv;
-
-	if (test_and_set_bit(1, &ir_rx51->device_is_open))
-		return -EBUSY;
-
-	ir_rx51->pwm = pwm_get(ir_rx51->dev, NULL);
-	if (IS_ERR(ir_rx51->pwm)) {
-		int res = PTR_ERR(ir_rx51->pwm);
-
-		dev_err(ir_rx51->dev, "pwm_get failed: %d\n", res);
-		return res;
-	}
-
-	return 0;
-}
-
-static void ir_rx51_release(struct rc_dev *dev)
-{
-	struct ir_rx51 *ir_rx51 = dev->priv;
-
-	hrtimer_cancel(&ir_rx51->timer);
-	ir_rx51_off(ir_rx51);
-	pwm_put(ir_rx51->pwm);
-
-	clear_bit(1, &ir_rx51->device_is_open);
-}
-
-static struct ir_rx51 ir_rx51 = {
-	.duty_cycle	= 50,
-	.wbuf_index	= -1,
-};
-
-static int ir_rx51_set_duty_cycle(struct rc_dev *dev, u32 duty)
-{
-	struct ir_rx51 *ir_rx51 = dev->priv;
-
-	ir_rx51->duty_cycle = duty;
-
-	return 0;
-}
-
-static int ir_rx51_set_tx_carrier(struct rc_dev *dev, u32 carrier)
-{
-	struct ir_rx51 *ir_rx51 = dev->priv;
-
-	if (carrier > 500000 || carrier < 20000)
-		return -EINVAL;
-
-	ir_rx51->freq = carrier;
-
-	return 0;
-}
-
-#ifdef CONFIG_PM
-
-static int ir_rx51_suspend(struct platform_device *dev, pm_message_t state)
-{
-	/*
-	 * In case the device is still open, do not suspend. Normally
-	 * this should not be a problem as lircd only keeps the device
-	 * open only for short periods of time. We also don't want to
-	 * get involved with race conditions that might happen if we
-	 * were in a middle of a transmit. Thus, we defer any suspend
-	 * actions until transmit has completed.
-	 */
-	if (test_and_set_bit(1, &ir_rx51.device_is_open))
-		return -EAGAIN;
-
-	clear_bit(1, &ir_rx51.device_is_open);
-
-	return 0;
-}
-
-static int ir_rx51_resume(struct platform_device *dev)
-{
-	return 0;
-}
-
-#else
-
-#define ir_rx51_suspend	NULL
-#define ir_rx51_resume	NULL
-
-#endif /* CONFIG_PM */
-
-static int ir_rx51_probe(struct platform_device *dev)
-{
-	struct pwm_device *pwm;
-	struct rc_dev *rcdev;
-
-	pwm = pwm_get(&dev->dev, NULL);
-	if (IS_ERR(pwm))
-		return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed\n");
-
-	/* Use default, in case userspace does not set the carrier */
-	ir_rx51.freq = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm), NSEC_PER_SEC);
-	pwm_init_state(pwm, &ir_rx51.state);
-	pwm_put(pwm);
-
-	hrtimer_init(&ir_rx51.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ir_rx51.timer.function = ir_rx51_timer_cb;
-
-	ir_rx51.dev = &dev->dev;
-
-	rcdev = devm_rc_allocate_device(&dev->dev, RC_DRIVER_IR_RAW_TX);
-	if (!rcdev)
-		return -ENOMEM;
-
-	rcdev->priv = &ir_rx51;
-	rcdev->open = ir_rx51_open;
-	rcdev->close = ir_rx51_release;
-	rcdev->tx_ir = ir_rx51_tx;
-	rcdev->s_tx_duty_cycle = ir_rx51_set_duty_cycle;
-	rcdev->s_tx_carrier = ir_rx51_set_tx_carrier;
-	rcdev->driver_name = KBUILD_MODNAME;
-
-	ir_rx51.rcdev = rcdev;
-
-	return devm_rc_register_device(&dev->dev, ir_rx51.rcdev);
-}
-
-static const struct of_device_id ir_rx51_match[] = {
-	{
-		.compatible = "nokia,n900-ir",
-	},
-	{},
-};
-MODULE_DEVICE_TABLE(of, ir_rx51_match);
-
-static struct platform_driver ir_rx51_platform_driver = {
-	.probe		= ir_rx51_probe,
-	.suspend	= ir_rx51_suspend,
-	.resume		= ir_rx51_resume,
-	.driver		= {
-		.name	= KBUILD_MODNAME,
-		.of_match_table = of_match_ptr(ir_rx51_match),
-	},
-};
-module_platform_driver(ir_rx51_platform_driver);
-
-MODULE_DESCRIPTION("IR TX driver for Nokia RX51");
-MODULE_AUTHOR("Nokia Corporation");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index 7732054c4621..c5f37c03af9c 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -23,6 +23,7 @@ struct pwm_ir {
 
 static const struct of_device_id pwm_ir_of_match[] = {
 	{ .compatible = "pwm-ir-tx", },
+	{ .compatible = "nokia,n900-ir" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, pwm_ir_of_match);
-- 
2.42.0

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Hello,

On Fri, Sep 01, 2023 at 03:18:56PM +0100, Sean Young wrote:
> The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
> handled entirely by the generic pwm-ir-tx driver, and in fact the
> pwm-ir-tx driver has been compatible with ir-rx51 from the start.
> 
> Note that the suspend code in the ir-rx51 driver is unnecessary, since
> during transmit, the process is not in interruptable sleep. The process
> is not put to sleep until the transmit completes.
> 
> Cc: Timo Kokkonen <timo.t.kokkonen@iki.fi>
> Cc: Pali Rohár <pali.rohar@gmail.com>
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Tested-by: Sicelo A. Mhlongo <absicsz@gmail.com>
> Signed-off-by: Sean Young <sean@mess.org>

I just commented on v3 of this patch. It was an

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

which I forward hereby to this v5. As with the previous revisions I'll
mark this as handled-elsewhere in the pwm patchwork in the assumption
that the pwm tree isn't involved to get this into the mainline.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Ivaylo Dimitrov]

On 1.09.23 г. 17:18 ч., Sean Young wrote:
> The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
> handled entirely by the generic pwm-ir-tx driver, and in fact the
> pwm-ir-tx driver has been compatible with ir-rx51 from the start.
> 

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows 
that for some reason usleep_range() sleeps for at least 300-400 us more 
than what interval it is requested to sleep. I played with cyclictest 
from rt-tests package and it gives similar results - increasing the 
priority helps, but I was not able to make it sleep for less that 300 us 
in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it 
made no difference.

I get similar results on motorola droid4 (OMAP4), albeit there average 
sleep is in 200-300 us range, which makes me believe that either OMAPs 
have issues with hrtimers or the config we use has some issue which 
leads to scheduler latency. Or, something else...

In either case help is appreciated to dig further trying to find the 
reason for such a big delay.

Regards,
Ivo

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Sean Young]

On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
> On 1.09.23 г. 17:18 ч., Sean Young wrote:
> > The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
> > handled entirely by the generic pwm-ir-tx driver, and in fact the
> > pwm-ir-tx driver has been compatible with ir-rx51 from the start.
> > 
> 
> Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
> for some reason usleep_range() sleeps for at least 300-400 us more than what
> interval it is requested to sleep. I played with cyclictest from rt-tests
> package and it gives similar results - increasing the priority helps, but I
> was not able to make it sleep for less that 300 us in average. I tried
> cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
> 
> I get similar results on motorola droid4 (OMAP4), albeit there average sleep
> is in 200-300 us range, which makes me believe that either OMAPs have issues
> with hrtimers or the config we use has some issue which leads to scheduler
> latency. Or, something else...

The pwm-ir-tx driver does suffer from this problem, but I was under the
impression that the ir-rx51 has the same problem.

> In either case help is appreciated to dig further trying to find the reason
> for such a big delay.

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
usleep_range() uses hrtimers; however if you're not seeing the same delay
on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

I don't have a n900 to test on, unfortunately.

Thanks
Sean

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Tony Lindgren]

* Sean Young <sean@mess.org> [230926 07:16]:
> On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
> > On 1.09.23 г. 17:18 ч., Sean Young wrote:
> > > The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
> > > handled entirely by the generic pwm-ir-tx driver, and in fact the
> > > pwm-ir-tx driver has been compatible with ir-rx51 from the start.
> > > 
> > 
> > Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
> > for some reason usleep_range() sleeps for at least 300-400 us more than what
> > interval it is requested to sleep. I played with cyclictest from rt-tests
> > package and it gives similar results - increasing the priority helps, but I
> > was not able to make it sleep for less that 300 us in average. I tried
> > cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
> > 
> > I get similar results on motorola droid4 (OMAP4), albeit there average sleep
> > is in 200-300 us range, which makes me believe that either OMAPs have issues
> > with hrtimers or the config we use has some issue which leads to scheduler
> > latency. Or, something else...
> 
> The pwm-ir-tx driver does suffer from this problem, but I was under the
> impression that the ir-rx51 has the same problem.
> 
> > In either case help is appreciated to dig further trying to find the reason
> > for such a big delay.
> 
> pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
> usleep_range() uses hrtimers; however if you're not seeing the same delay
> on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

Maybe using fsleep() fixes this issue? See commit c6af13d33475 ("timer: add
fsleep for flexible sleeping"), and Documentation/timers/timers-howto.rst.

The long wake-up time for an idle state could explain the values. I think
Ivaylo already tested with most cpuidle states disabled via sysfs though.

> I don't have a n900 to test on, unfortunately.

If you want one for development, the maemo folks cc:ed here likely have
some available devices.

Regards,

Tony

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Ivaylo Dimitrov]

On 26.09.23 г. 10:37 ч., Tony Lindgren wrote:
> * Sean Young <sean@mess.org> [230926 07:16]:
>> On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
>>> On 1.09.23 г. 17:18 ч., Sean Young wrote:
>>>> The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
>>>> handled entirely by the generic pwm-ir-tx driver, and in fact the
>>>> pwm-ir-tx driver has been compatible with ir-rx51 from the start.
>>>>
>>>
>>> Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
>>> for some reason usleep_range() sleeps for at least 300-400 us more than what
>>> interval it is requested to sleep. I played with cyclictest from rt-tests
>>> package and it gives similar results - increasing the priority helps, but I
>>> was not able to make it sleep for less that 300 us in average. I tried
>>> cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
>>>
>>> I get similar results on motorola droid4 (OMAP4), albeit there average sleep
>>> is in 200-300 us range, which makes me believe that either OMAPs have issues
>>> with hrtimers or the config we use has some issue which leads to scheduler
>>> latency. Or, something else...
>>
>> The pwm-ir-tx driver does suffer from this problem, but I was under the
>> impression that the ir-rx51 has the same problem.
>>
>>> In either case help is appreciated to dig further trying to find the reason
>>> for such a big delay.
>>
>> pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
>> usleep_range() uses hrtimers; however if you're not seeing the same delay
>> on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.
> 
> Maybe using fsleep() fixes this issue? See commit c6af13d33475 ("timer: add
> fsleep for flexible sleeping"), and Documentation/timers/timers-howto.rst.
> 

I doubt, time intervals we are talking about are > 500 us, which means 
fsleep will always use usleep_range() (or even worse, msleep()), see 
https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/delay.h#L82

> The long wake-up time for an idle state could explain the values. I think
> Ivaylo already tested with most cpuidle states disabled via sysfs though.
> 

Yes, I disabled all idle states on both n900 and droid4 (when doing 
cyclictest experiments), with no difference. I also locked frequency on 
n900 to 500MHz, which improved the things a bit, by some 20-50 us 
(IIRC), which makes sense, but also makes me think frequency scaling is 
not the one to blame either.

>> I don't have a n900 to test on, unfortunately.
> 
> If you want one for development, the maemo folks cc:ed here likely have
> some available devices.
> 

I think we can arrange one, yes, but my gut feeling tells me the issue 
is not n900 specific, it is just a bit worse there as the device is 
relatively slow already. I have no sane explanation why one would see 
similar latencies on droid4, given that it is times faster than n900.

Regards,

Ivo

> Regards,
> 
> Tony
> 

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Ivaylo Dimitrov]

On 26.09.23 г. 10:16 ч., Sean Young wrote:
> On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
>> On 1.09.23 г. 17:18 ч., Sean Young wrote:
>>> The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
>>> handled entirely by the generic pwm-ir-tx driver, and in fact the
>>> pwm-ir-tx driver has been compatible with ir-rx51 from the start.
>>>
>>
>> Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
>> for some reason usleep_range() sleeps for at least 300-400 us more than what
>> interval it is requested to sleep. I played with cyclictest from rt-tests
>> package and it gives similar results - increasing the priority helps, but I
>> was not able to make it sleep for less that 300 us in average. I tried
>> cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
>>
>> I get similar results on motorola droid4 (OMAP4), albeit there average sleep
>> is in 200-300 us range, which makes me believe that either OMAPs have issues
>> with hrtimers or the config we use has some issue which leads to scheduler
>> latency. Or, something else...
> 
> The pwm-ir-tx driver does suffer from this problem, but I was under the
> impression that the ir-rx51 has the same problem.
> 

Could you elaborate on the "pwm-ir-tx driver does suffer from this 
problem"? Where do you see that?

ir-rx51 does not suffer from the same problem (albeit it has its own 
one, see bellow)

>> In either case help is appreciated to dig further trying to find the reason
>> for such a big delay.
> 
> pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
> usleep_range() uses hrtimers; however if you're not seeing the same delay
> on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.
> 

usleep_range() is backed by hrtimers already, however the difference 
comes from how hrtimer is used in ir-rx51: it uses timer callback 
function that gets called in softirq context, while usleep_range() puts 
the task in TASK_UNINTERRUPTIBLE state and then calls 
schedule_hrtimeout_range(). For some reason it takes at least 200-400 us 
(on average) even on OMAP4 to switch back to TASK_RUNNING state.

The issue with ir-rx51 and the way it uses hrtimers is that it calls 
pwm_apply_state() from hrtimer function, which is not ok, per the 
comment here 
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/pwm/core.c#L502

I can make pwm-ir-tx switch to hrtimers, that's not an issue, but I am 
afraid that there is some general scheduler or timers (or something 
else) issue that manifests itself with usleep_range() misbehaving.

> I don't have a n900 to test on, unfortunately.
> 

I have and once I have an idea what's going on will port pwm-ir-tx to 
hrtimers, if needed. Don't want to do it now as I am afraid the 
completion I will have to use will have the same latency problems as 
usleep_range()

Thanks,
Ivo

> Thanks
> Sean
> 

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Sean Young]

On Tue, Sep 26, 2023 at 03:43:18PM +0300, Ivaylo Dimitrov wrote:
> On 26.09.23 г. 10:16 ч., Sean Young wrote:
> > On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
> > > On 1.09.23 г. 17:18 ч., Sean Young wrote:
> > > > The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
> > > > handled entirely by the generic pwm-ir-tx driver, and in fact the
> > > > pwm-ir-tx driver has been compatible with ir-rx51 from the start.
> > > > 
> > > 
> > > Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
> > > for some reason usleep_range() sleeps for at least 300-400 us more than what
> > > interval it is requested to sleep. I played with cyclictest from rt-tests
> > > package and it gives similar results - increasing the priority helps, but I
> > > was not able to make it sleep for less that 300 us in average. I tried
> > > cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
> > > 
> > > I get similar results on motorola droid4 (OMAP4), albeit there average sleep
> > > is in 200-300 us range, which makes me believe that either OMAPs have issues
> > > with hrtimers or the config we use has some issue which leads to scheduler
> > > latency. Or, something else...
> > 
> > The pwm-ir-tx driver does suffer from this problem, but I was under the
> > impression that the ir-rx51 has the same problem.
> > 
> 
> Could you elaborate on the "pwm-ir-tx driver does suffer from this problem"?
> Where do you see that?

So on a raspberry pi (model 3b), if I use the pwm-ir-tx driver, I get random
delays of up to 100us. It's a bit random and certainly depends on the load.

I'm measuring using a logic analyzer.

There have been reports by others on different machines with random delays
and/or transmit failures (as in the receiver occassionally fails to decode
the IR). I usually suggest they use the gpio-ir-tx driver, which does work
as far as I know (the signal looks perfect with a logic analyzer).

So far I've taken the view that the driver works ok for most situations,
since IR is usually fine with upto 100us missing here or there.

The gpio-ir-tx driver works much better because it does the entire send 
under spinlock - obviously that has its own problems, because an IR transmit
can be 10s or even 100s of milliseconds.

I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
directly improves the situation even a bit, then that would be great.

> ir-rx51 does not suffer from the same problem (albeit it has its own one,
> see bellow)
> 
> > > In either case help is appreciated to dig further trying to find the reason
> > > for such a big delay.
> > 
> > pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
> > usleep_range() uses hrtimers; however if you're not seeing the same delay
> > on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.
> > 
> 
> usleep_range() is backed by hrtimers already, however the difference comes
> from how hrtimer is used in ir-rx51: it uses timer callback function that
> gets called in softirq context, while usleep_range() puts the task in
> TASK_UNINTERRUPTIBLE state and then calls schedule_hrtimeout_range(). For
> some reason it takes at least 200-400 us (on average) even on OMAP4 to
> switch back to TASK_RUNNING state.
> 
> The issue with ir-rx51 and the way it uses hrtimers is that it calls
> pwm_apply_state() from hrtimer function, which is not ok, per the comment
> here
> https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/pwm/core.c#L502
> 
> I can make pwm-ir-tx switch to hrtimers, that's not an issue, but I am
> afraid that there is some general scheduler or timers (or something else)
> issue that manifests itself with usleep_range() misbehaving.

If we can switch pwm-ir-tx to hrtimers, that would be great.

The ir-rx51 removal patches have already been queued to media_staging;
we may have to remove them from there if we can't solve this problem.

> > I don't have a n900 to test on, unfortunately.
> > 
> 
> I have and once I have an idea what's going on will port pwm-ir-tx to
> hrtimers, if needed. Don't want to do it now as I am afraid the completion I
> will have to use will have the same latency problems as usleep_range()

That would be fantastic. Please do keep us up to date with how you are
getting on. Like I said, it would be nice to this resolved before the next
merge window.

Thanks,
Sean

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Ivaylo Dimitrov]

Hi,

On 26.09.23 г. 23:18 ч., Sean Young wrote:
> On Tue, Sep 26, 2023 at 03:43:18PM +0300, Ivaylo Dimitrov wrote:
>> On 26.09.23 г. 10:16 ч., Sean Young wrote:
>>> On Mon, Sep 25, 2023 at 07:06:44PM +0300, Ivaylo Dimitrov wrote:
>>>> On 1.09.23 г. 17:18 ч., Sean Young wrote:
>>>>> The ir-rx51 is a pwm-based TX driver specific to the N900. This can be
>>>>> handled entirely by the generic pwm-ir-tx driver, and in fact the
>>>>> pwm-ir-tx driver has been compatible with ir-rx51 from the start.
>>>>>
>>>>
>>>> Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that
>>>> for some reason usleep_range() sleeps for at least 300-400 us more than what
>>>> interval it is requested to sleep. I played with cyclictest from rt-tests
>>>> package and it gives similar results - increasing the priority helps, but I
>>>> was not able to make it sleep for less that 300 us in average. I tried
>>>> cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.
>>>>
>>>> I get similar results on motorola droid4 (OMAP4), albeit there average sleep
>>>> is in 200-300 us range, which makes me believe that either OMAPs have issues
>>>> with hrtimers or the config we use has some issue which leads to scheduler
>>>> latency. Or, something else...
>>>
>>> The pwm-ir-tx driver does suffer from this problem, but I was under the
>>> impression that the ir-rx51 has the same problem.
>>>
>>
>> Could you elaborate on the "pwm-ir-tx driver does suffer from this problem"?
>> Where do you see that?
> 
> So on a raspberry pi (model 3b), if I use the pwm-ir-tx driver, I get random
> delays of up to 100us. It's a bit random and certainly depends on the load.
> 
> I'm measuring using a logic analyzer.
> 
> There have been reports by others on different machines with random delays
> and/or transmit failures (as in the receiver occassionally fails to decode
> the IR). I usually suggest they use the gpio-ir-tx driver, which does work
> as far as I know (the signal looks perfect with a logic analyzer).
> 
> So far I've taken the view that the driver works ok for most situations,
> since IR is usually fine with upto 100us missing here or there.
> 
> The gpio-ir-tx driver works much better because it does the entire send
> under spinlock - obviously that has its own problems, because an IR transmit
> can be 10s or even 100s of milliseconds.
> 
> I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
> directly improves the situation even a bit, then that would be great.
> 

The issue with hrtimers is that we cannot use them directly, as 
pwm_apply_state() may sleep, but hrtimer function is called in atomic 
context.

>> ir-rx51 does not suffer from the same problem (albeit it has its own one,
>> see bellow)
>>
>>>> In either case help is appreciated to dig further trying to find the reason
>>>> for such a big delay.
>>>
>>> pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that
>>> usleep_range() uses hrtimers; however if you're not seeing the same delay
>>> on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.
>>>
>>
>> usleep_range() is backed by hrtimers already, however the difference comes
>> from how hrtimer is used in ir-rx51: it uses timer callback function that
>> gets called in softirq context, while usleep_range() puts the task in
>> TASK_UNINTERRUPTIBLE state and then calls schedule_hrtimeout_range(). For
>> some reason it takes at least 200-400 us (on average) even on OMAP4 to
>> switch back to TASK_RUNNING state.
>>
>> The issue with ir-rx51 and the way it uses hrtimers is that it calls
>> pwm_apply_state() from hrtimer function, which is not ok, per the comment
>> here
>> https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/pwm/core.c#L502
>>
>> I can make pwm-ir-tx switch to hrtimers, that's not an issue, but I am
>> afraid that there is some general scheduler or timers (or something else)
>> issue that manifests itself with usleep_range() misbehaving.
> 
> If we can switch pwm-ir-tx to hrtimers, that would be great.
> 

I made some POC here, but unfortunately it failed more or less. The idea 
of POC is: setup hrtimer, start it in pwm_ir_tx() and do 
wait_for_completion() in a loop while calling complete() for the timer 
function. While it improves things a bit, I wouldn't say it makes the 
driver working ok on n900 - my TV registers one of let's say 5-10 pulse 
packages.

We have couple of issues:

- scheduler seems to use 32kHz timer, which means that we can never have 
precise pulse width, with error up to ~30 us, no matter what we do, IIUC.

- wait_for_completion() suffers from the same latency issue that 
usleep_range() has - it exits after 300-400 us after complete() has been 
called in the timer function.

- turning pwm off needs ~300us, because of either omap_dm_timer_stop() 
calling clk_get_rate() or __omap_dm_timer_stop() waiting for fclk period 
* 3.5 (see 
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/clocksource/timer-ti-dm.c#L269)

- in order to achieve some sane latency distribution, I have to 
set_user_nice(current, MIN_NICE); in pwm_ir_tx()


> The ir-rx51 removal patches have already been queued to media_staging;
> we may have to remove them from there if we can't solve this problem.
> 

ir-rx51 has conceptual problem of calling function that might sleep from 
atomic context, however, we can fix omap_dm_timer_stop() to not call 
clk_get_rate() and that would make it working. So yeah, if we can't fix 
pwm-ir-tx then patches removal along with fixing dmtimer and fixing a 
couple of code issues ir-rx51 has, seems the only option to have working 
IRTX on n900. Maybe we can rename it to pwm-ir-tx-hrtimer as there is 
nothing n900 specific in it.

>>> I don't have a n900 to test on, unfortunately.
>>>
>>
>> I have and once I have an idea what's going on will port pwm-ir-tx to
>> hrtimers, if needed. Don't want to do it now as I am afraid the completion I
>> will have to use will have the same latency problems as usleep_range()
> 
> That would be fantastic. Please do keep us up to date with how you are
> getting on. Like I said, it would be nice to this resolved before the next
> merge window.
> 

The only thing I didn't try yet is to start another thread and to set 
that thread to use FIFO scheduler. I will report once I have tried that.

Regards,
Ivo

> Thanks,
> Sean
> 

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Sean Young]

Hello Uwe,

Just wanted to run an idea by you.

On Fri, Sep 29, 2023 at 11:49:52AM +0300, Ivaylo Dimitrov wrote:
> On 26.09.23 г. 23:18 ч., Sean Young wrote:
> > I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
> > directly improves the situation even a bit, then that would be great.
> 
> The issue with hrtimers is that we cannot use them directly, as
> pwm_apply_state() may sleep, but hrtimer function is called in atomic
> context.

I've also been looking at this problem and came to same conclusion: the
fact that pwm_apply_state() sleeps is a huge problem.

1) The vast majority of pwm drivers don't sleep, or could even be converted
   to spinlocks (e.g pwm-sifive.c could use spinlocks, as far as I can see).

2) Sure, some pwm devices are on i2c busses, so the driver needs to sleep.
   Those devices aren't great for what we're trying to do here, since the
   sleeping may cause delays and affect the generated signal.

What would be ideal here is to have pwm-ir-tx work in atomic context if
a non-sleeping pwm device is used, and another (non-optimal) code path
for sleeping pwm drivers. We could even just refuse to run on sleeping pwm
drivers.

Uwe what do you think of this idea? The pwm api could have a
bool pwm_may_sleep(struct pwm *pwm) function, and pwm_apply_state() does 
not contain might_sleep() - only the driver-specific apply calls might_sleep().

It would be nice if this could all be done at compile time through e.g. a
device tree attribute.

Thanks,

Sean

Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

Hello Sean,

On Fri, Sep 29, 2023 at 05:20:09PM +0100, Sean Young wrote:
> On Fri, Sep 29, 2023 at 11:49:52AM +0300, Ivaylo Dimitrov wrote:
> > On 26.09.23 г. 23:18 ч., Sean Young wrote:
> > > I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
> > > directly improves the situation even a bit, then that would be great.
> > 
> > The issue with hrtimers is that we cannot use them directly, as
> > pwm_apply_state() may sleep, but hrtimer function is called in atomic
> > context.
> 
> I've also been looking at this problem and came to same conclusion: the
> fact that pwm_apply_state() sleeps is a huge problem.
> 
> 1) The vast majority of pwm drivers don't sleep, or could even be converted
>    to spinlocks (e.g pwm-sifive.c could use spinlocks, as far as I can see).
> 
> 2) Sure, some pwm devices are on i2c busses, so the driver needs to sleep.
>    Those devices aren't great for what we're trying to do here, since the
>    sleeping may cause delays and affect the generated signal.
> 
> What would be ideal here is to have pwm-ir-tx work in atomic context if
> a non-sleeping pwm device is used, and another (non-optimal) code path
> for sleeping pwm drivers. We could even just refuse to run on sleeping pwm
> drivers.
> 
> Uwe what do you think of this idea? The pwm api could have a
> bool pwm_may_sleep(struct pwm *pwm) function,

It's certainly possible. The idea of introducing the might_sleep() was
to catch atomic users and if some appear to be able to evaluate if
something needs to be done. See commit 4ad91a227817 ("pwm: Make it
explicit that pwm_apply_state() might sleep").

It complicates things concerning my last bigger pwm series, see
https://lore.kernel.org/linux-pwm/20230808171931.944154-102-u.kleine-koenig@pengutronix.de/
which introduces a mutex_lock() in pwm_apply_state(). Hmm.

> and pwm_apply_state() does 
> not contain might_sleep() - only the driver-specific apply calls might_sleep().

I'd replace the might_sleep() by something like
might_sleep_if(pwm_may_sleep(pwm)); but that's an implementation detail.

> It would be nice if this could all be done at compile time through e.g. a
> device tree attribute.

I wouldn't have something like "linux,slow-pwm" or similar in the device
tree, and I'd expect the dt maintainers to shoot down something like
that, too. What is the problem with a pwm_can_sleep() function only?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]