Improve pwm-ir-tx precision

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link:    https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
config: i386-buildonly-randconfig-004-20231001 (https://download.01.org/0day-ci/archive/20231001/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from arch/x86/include/asm/percpu.h:27:0,

                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from include/linux/backlight.h:12,
                    from drivers/video/fbdev/ssd1307fb.c:8:
   include/linux/pwm.h: In function 'pwm_apply_state':
   include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'gpiod_cansleep'? [-Werror=implicit-function-declaration]
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~

In file included from drivers/video/fbdev/ssd1307fb.c:16:0:
include/linux/pwm.h: At top level:

Quoted 1 line

Quoted 1 line

include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'

static inline bool pwm_can_sleep(struct pwm_device *pwm)
^~~~~~~~~~~~~
In file included from arch/x86/include/asm/percpu.h:27:0,

                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from include/linux/backlight.h:12,
                    from drivers/video/fbdev/ssd1307fb.c:8:
   include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   cc1: some warnings being treated as errors
--
   In file included from arch/x86/include/asm/percpu.h:27:0,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/mfd/intel_soc_pmic_crc.c:11:
   include/linux/pwm.h: In function 'pwm_apply_state':
   include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~

In file included from drivers/mfd/intel_soc_pmic_crc.c:18:0:
include/linux/pwm.h: At top level:

Quoted 1 line

Quoted 1 line

include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'

static inline bool pwm_can_sleep(struct pwm_device *pwm)
^~~~~~~~~~~~~
In file included from arch/x86/include/asm/percpu.h:27:0,

                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/mfd/intel_soc_pmic_crc.c:11:
   include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   cc1: some warnings being treated as errors

vim +/pwm_can_sleep +455 include/linux/pwm.h

   454	

Quoted 1 line

455 static inline bool pwm_can_sleep(struct pwm_device *pwm)

   456	{
   457		return true;
   458	}
   459	

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Quoted 11 lines · Sean Young

drivers/pwm/core.c +1 -1

index dc66e3405bf5..d9679ae5b2be 100644
@@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	 * is a bad idea. So make it explicit that calling this function might
 	 * sleep.
 	 */
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 

drivers/pwm/core.c int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) · line 511

Quoted 2 lines

drivers/pwm/core.c +0 -0

 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)

I'd like to have a mechanism to catch drivers that missed to set .can_sleep. The best idea I currently have for that is to disable preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while .apply() is running.

Quoted 6 lines

drivers/pwm/pwm-fsl-ftm.c +0 -0

index b7c6045c5d08..b8b9392844e9 100644
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 

drivers/pwm/pwm-fsl-ftm.c static int fsl_pwm_probe(struct platform_device *pdev) · line 408

Quoted 3 lines

drivers/pwm/pwm-fsl-ftm.c +1 -0

 	fpc->soc = of_device_get_match_data(&pdev->dev);
 	fpc->chip.dev = &pdev->dev;
+	fpc->chip.can_sleep = true;

As .apply() being callable in non-sleepable context only depends on .apply() I think a better place for this property is in struct pwm_ops.

Also I wonder if the distinction between atomic and sleeping pwm_state_apply() should be more explicit. For GPIOs you have a sleeping variant gpiod_set_value_cansleep() that allows to immediately determine the intended context in the caller. This would allow that programming a PWM stays a preemption point (if possible/desired) even if the underlying hardware/driver is atomic. To not have to touch all consumer drivers, maybe the pair for pwm should better be

pwm_apply_state()
pwm_apply_state_atomic()

instead of a "cansleep" suffix for the sleeping variant? Or maybe it's better to accept touching all consumer drivers to get semantics similar to gpio? I couldn't decide quickly what I really like better here, so that's your chance to comment and influence the outcome :-)

Quoted 12 lines · Uwe Kleine-König

Quoted 11 lines · Sean Young

drivers/pwm/core.c +1 -1

index dc66e3405bf5..d9679ae5b2be 100644
@@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	 * is a bad idea. So make it explicit that calling this function might
 	 * sleep.
 	 */
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 

Quoted 7 lines

drivers/pwm/core.c int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) · line 511

Quoted 2 lines

drivers/pwm/core.c +0 -0

 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)

I'd like to have a mechanism to catch drivers that missed to set .can_sleep. The best idea I currently have for that is to disable preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while .apply() is running.

If we have pwm_apply_state_atomic(), then CONFIG_DEBUG_ATOMIC_SLEEP will catch them, but only in that code path of course.

How about using non_block_start() and non_block_end() if can_sleep is not set?

Quoted 18 lines

drivers/pwm/pwm-fsl-ftm.c static int fsl_pwm_probe(struct platform_device *pdev) · line 408

Quoted 9 lines

drivers/pwm/pwm-fsl-ftm.c +1 -0

index b7c6045c5d08..b8b9392844e9 100644
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 
 	fpc->soc = of_device_get_match_data(&pdev->dev);
 	fpc->chip.dev = &pdev->dev;
+	fpc->chip.can_sleep = true;

Also I wonder if the distinction between atomic and sleeping pwm_state_apply() should be more explicit. For GPIOs you have a sleeping variant gpiod_set_value_cansleep() that allows to immediately determine the intended context in the caller. This would allow that programming a PWM stays a preemption point (if possible/desired) even if the underlying hardware/driver is atomic. To not have to touch all consumer drivers, maybe the pair for pwm should better be

Quoted 2 lines

pwm_apply_state()
pwm_apply_state_atomic()

Do we need pwm_config_atomic(), pwm_enable_atomic(), and pwm_disable_atomic() too? These are just convenience functions, so we can probably do without them.

Quoted 4 lines

instead of a "cansleep" suffix for the sleeping variant? Or maybe it's better to accept touching all consumer drivers to get semantics similar to gpio? I couldn't decide quickly what I really like better here, so that's your chance to comment and influence the outcome :-)

If you expect to have more parameters for pwm_apply_state() then a flags argument makes sense.

TBH I like the pwm_apply_state_atomic() option.

Hello Sean,

drivers/pwm/core.c int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) · line 511

Quoted 13 lines · Sean Young

drivers/pwm/core.c +1 -1

index dc66e3405bf5..d9679ae5b2be 100644
@@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	 * is a bad idea. So make it explicit that calling this function might
 	 * sleep.
 	 */
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 
 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)

I'd like to have a mechanism to catch drivers that missed to set .can_sleep. The best idea I currently have for that is to disable preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while .apply() is running.

Quoted 6 lines

drivers/pwm/pwm-fsl-ftm.c +0 -0

index b7c6045c5d08..b8b9392844e9 100644
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 

drivers/pwm/pwm-fsl-ftm.c static int fsl_pwm_probe(struct platform_device *pdev) · line 408

Quoted 3 lines

drivers/pwm/pwm-fsl-ftm.c +1 -0

 	fpc->soc = of_device_get_match_data(&pdev->dev);
 	fpc->chip.dev = &pdev->dev;
+	fpc->chip.can_sleep = true;

As .apply() being callable in non-sleepable context only depends on .apply() I think a better place for this property is in struct pwm_ops.

Quoted 7 lines · Uwe Kleine-König

Quoted 6 lines · Sean Young

drivers/pwm/pwm-fsl-ftm.c +0 -0

index b7c6045c5d08..b8b9392844e9 100644
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 

Quoted 6 lines

drivers/pwm/pwm-fsl-ftm.c static int fsl_pwm_probe(struct platform_device *pdev) · line 408

Quoted 3 lines

drivers/pwm/pwm-fsl-ftm.c +1 -0

 	fpc->soc = of_device_get_match_data(&pdev->dev);
 	fpc->chip.dev = &pdev->dev;
+	fpc->chip.can_sleep = true;

As .apply() being callable in non-sleepable context only depends on .apply() I think a better place for this property is in struct pwm_ops.

What about drivers for devices that can be either sleeping or not? There are things like regmap which can abstract those differences away, so you could have a driver that works on both types of devices, so setting this in ops wouldn't be correct all the time. I think can_sleep needs to be per-chip rather than per-driver.

include/linux/pwm.h +1 -0

index d2f9f690a9c1..c94894ffa4c4 100644
@@ -287,6 +287,7 @@ struct pwm_ops {
  * @ops: callbacks for this PWM controller
  * @base: number of first PWM controlled by this chip
  * @npwm: number of PWMs controlled by this chip
+ * @can_sleep: can the driver sleep in pwm_apply_state
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
  * @list: list node for internal use

include/linux/pwm.h +0 -0

 	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
 					const struct of_phandle_args *args);

@@ -380,6 +382,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
 	pwm_apply_state(pwm, &state);

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
  drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
  1 file changed, 63 insertions(+), 16 deletions(-)

drivers/media/rc/pwm-ir-tx.c +9 -2

index c5f37c03af9c..557725a07a67 100644
@@ -10,6 +10,8 @@
  #include <linux/slab.h>
  #include <linux/of.h>
  #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
  #include <media/rc-core.h>
 
  #define DRIVER_NAME	"pwm-ir-tx"

@@ -17,8 +19,13 @@
 
  struct pwm_ir {
  	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
  };
 
  static const struct of_device_id pwm_ir_of_match[] = {

drivers/media/rc/pwm-ir-tx.c +1 -0

  	pwm_init_state(pwm, &state);
 
  	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
  	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+	state.enabled = false;
 

drivers/media/rc/pwm-ir-tx.c +17 -2

-	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	wait_for_completion(&pwm_ir->completion);
 
  	return count;
  }
 
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			/* Stop TX here */
+			pwm_disable(pwm_ir->pwm);
+

drivers/media/rc/pwm-ir-tx.c +11 -0

+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
  static int pwm_ir_probe(struct platform_device *pdev)
  {
  	struct pwm_ir *pwm_ir;

Quoted 41 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
  drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
  1 file changed, 63 insertions(+), 16 deletions(-)

drivers/media/rc/pwm-ir-tx.c +9 -2

index c5f37c03af9c..557725a07a67 100644
@@ -10,6 +10,8 @@
  #include <linux/slab.h>
  #include <linux/of.h>
  #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
  #include <media/rc-core.h>
  #define DRIVER_NAME	"pwm-ir-tx"

@@ -17,8 +19,13 @@
  struct pwm_ir {
  	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
  };
  static const struct of_device_id pwm_ir_of_match[] = {

@@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
  	struct pwm_ir *pwm_ir = dev->priv;
  	struct pwm_device *pwm = pwm_ir->pwm;

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 66

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +2 -3

  	struct pwm_state state;
-	int i;
-	ktime_t edge;
-	long delta;
+
+	reinit_completion(&pwm_ir->completion);

You should not need that.

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +2 -1

  	pwm_init_state(pwm, &state);
  	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
  	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+	state.enabled = false;
-	edge = ktime_get();
+	pwm_ir->txbuf = txbuf;

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 78

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +3 -3

+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
-	for (i = 0; i < count; i++) {
-		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm, &state);

ditto, first pwm control should be in the timer function

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 80

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +1 -5

-		edge = ktime_add_us(edge, txbuf[i]);
-		delta = ktime_us_delta(edge, ktime_get());
-		if (delta > 0)
-			usleep_range(delta, delta + 10);
-	}
+	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);

why not just call it with 0 time?

Quoted 26 lines

drivers/media/rc/pwm-ir-tx.c +22 -2

-	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	wait_for_completion(&pwm_ir->completion);
  	return count;
  }
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			/* Stop TX here */
+			pwm_disable(pwm_ir->pwm);
+
+			complete(&pwm_ir->completion);
+
+			return HRTIMER_NORESTART;
+		}
+

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 112

Quoted 5 lines

drivers/media/rc/pwm-ir-tx.c +5 -0

+		if (pwm_ir->txbuf_index % 2)
+			pwm_disable(pwm_ir->pwm);
+		else
+			pwm_enable(pwm_ir->pwm);
+

pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2); pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

Quoted 19 lines

drivers/media/rc/pwm-ir-tx.c +13 -0

+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
  static int pwm_ir_probe(struct platform_device *pdev)
  {
  	struct pwm_ir *pwm_ir;

@@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
  	if (IS_ERR(pwm_ir->pwm))
  		return PTR_ERR(pwm_ir->pwm);
+	if (pwm_can_sleep(pwm_ir->pwm)) {
+		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 142

Quoted 3 lines

drivers/media/rc/pwm-ir-tx.c +3 -0

+		return -ENODEV;
+	}
+

I think we shall not limit, but use high priority thread to support those drivers. I have that working on n900 with current (sleeping) pwm, see my reply on the other mail. Maybe we can combine both patches in a way to support both atomic and sleeping pwm drivers.

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev)

Quoted 9 lines

drivers/media/rc/pwm-ir-tx.c +3 -0

  	pwm_ir->carrier = 38000;
  	pwm_ir->duty_cycle = 50;
+	init_completion(&pwm_ir->completion);
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer;
  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
  	if (!rcdev)
 

Regards,
Ivo

Having said that, the extra call to pwm_apply_state() may have benefits, see this comment in the pwm-sifive driver:

* - When changing both duty cycle and period, we cannot prevent in
* software that the output might produce a period with mixed
* settings (new period length and old duty cycle).

Hi,

Quoted 42 lines · Ivaylo Dimitrov

Quoted 41 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
   1 file changed, 63 insertions(+), 16 deletions(-)

drivers/media/rc/pwm-ir-tx.c +9 -2

index c5f37c03af9c..557725a07a67 100644
@@ -10,6 +10,8 @@
   #include <linux/slab.h>
   #include <linux/of.h>
   #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
   #include <media/rc-core.h>
   #define DRIVER_NAME	"pwm-ir-tx"

@@ -17,8 +19,13 @@
   struct pwm_ir {
   	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
   };
   static const struct of_device_id pwm_ir_of_match[] = {

@@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
   	struct pwm_ir *pwm_ir = dev->priv;
   	struct pwm_device *pwm = pwm_ir->pwm;

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 66

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +2 -3

   	struct pwm_state state;
-	int i;
-	ktime_t edge;
-	long delta;
+
+	reinit_completion(&pwm_ir->completion);

You should not need that.

It does not work without it - the process doing the 2nd tx hangs indefinitely.

Quoted 6 lines

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +2 -1

   	pwm_init_state(pwm, &state);
   	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
   	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+	state.enabled = false;
-	edge = ktime_get();
+	pwm_ir->txbuf = txbuf;

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 78

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +3 -3

+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
-	for (i = 0; i < count; i++) {
-		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm, &state);

ditto, first pwm control should be in the timer function

This requires keeping a copy of pwm_state in pwm_ir but does avoid the extra call to pwm_apply_state() here.

Having said that, the extra call to pwm_apply_state() may have benefits, see this comment in the pwm-sifive driver:

* - When changing both duty cycle and period, we cannot prevent in
* software that the output might produce a period with mixed
* settings (new period length and old duty cycle).

So setting the duty cycle and period once with enabled = false prevents a first period with mixed settings (i.e. bogus).

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 80

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +1 -5

-		edge = ktime_add_us(edge, txbuf[i]);
-		delta = ktime_us_delta(edge, ktime_get());
-		if (delta > 0)
-			usleep_range(delta, delta + 10);
-	}
+	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);

why not just call it with 0 time?

Otherwise the timings are a little off for the first edge - hrtimer setup time, I think. I can experiment again.

Quoted 26 lines

Quoted 26 lines

drivers/media/rc/pwm-ir-tx.c +22 -2

-	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	wait_for_completion(&pwm_ir->completion);
   	return count;
   }
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			/* Stop TX here */
+			pwm_disable(pwm_ir->pwm);
+
+			complete(&pwm_ir->completion);
+
+			return HRTIMER_NORESTART;
+		}
+

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 112

Quoted 5 lines

drivers/media/rc/pwm-ir-tx.c +5 -0

+		if (pwm_ir->txbuf_index % 2)
+			pwm_disable(pwm_ir->pwm);
+		else
+			pwm_enable(pwm_ir->pwm);
+

pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2); pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28 bytes vs keeping it around).

Quoted 20 lines

Quoted 20 lines

drivers/media/rc/pwm-ir-tx.c +13 -0

+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
   static int pwm_ir_probe(struct platform_device *pdev)
   {
   	struct pwm_ir *pwm_ir;

@@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
   	if (IS_ERR(pwm_ir->pwm))
   		return PTR_ERR(pwm_ir->pwm);
+	if (pwm_can_sleep(pwm_ir->pwm)) {
+		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 142

Quoted 3 lines

drivers/media/rc/pwm-ir-tx.c +3 -0

+		return -ENODEV;
+	}
+

I think we shall not limit, but use high priority thread to support those drivers. I have that working on n900 with current (sleeping) pwm, see my reply on the other mail. Maybe we can combine both patches in a way to support both atomic and sleeping pwm drivers.

If the ir-rx51 driver uses a sleeping pwm then that's broken and only works by accident - the current driver is broken then.

Spinning for longer periods (e.g. 100us) does not play well with RT. Would make more sense to fix the pwm driver to non-sleeping when a pwm driver is used for pwm-ir-tx?

Thanks

Sean

Quoted 12 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev)

Quoted 9 lines

drivers/media/rc/pwm-ir-tx.c +3 -0

   	pwm_ir->carrier = 38000;
   	pwm_ir->duty_cycle = 50;
+	init_completion(&pwm_ir->completion);
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer;
   	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
   	if (!rcdev)
 

Regards,
Ivo

Quoted 52 lines · Sean Young

Quoted 50 lines · Ivaylo Dimitrov

Quoted 39 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
   1 file changed, 63 insertions(+), 16 deletions(-)

drivers/media/rc/pwm-ir-tx.c +9 -2

index c5f37c03af9c..557725a07a67 100644
@@ -10,6 +10,8 @@
   #include <linux/slab.h>
   #include <linux/of.h>
   #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
   #include <media/rc-core.h>
   #define DRIVER_NAME	"pwm-ir-tx"

@@ -17,8 +19,13 @@
   struct pwm_ir {
   	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
   };
   static const struct of_device_id pwm_ir_of_match[] = {

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 66

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c +2 -3

@@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
   	struct pwm_ir *pwm_ir = dev->priv;
   	struct pwm_device *pwm = pwm_ir->pwm;
   	struct pwm_state state;
-	int i;
-	ktime_t edge;
-	long delta;
+
+	reinit_completion(&pwm_ir->completion);

You should not need that.

Quoted 2 lines

It does not work without it - the process doing the 2nd tx hangs indefinitely.

that means your calls to wait_for_completion() / complete() do not match. I think you should check why.

Quoted 15 lines

Quoted 14 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 78

Quoted 12 lines

drivers/media/rc/pwm-ir-tx.c +5 -4

   	pwm_init_state(pwm, &state);
   	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
   	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+	state.enabled = false;
-	edge = ktime_get();
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
-	for (i = 0; i < count; i++) {
-		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm, &state);

ditto, first pwm control should be in the timer function

Quoted 3 lines

This requires keeping a copy of pwm_state in pwm_ir but does avoid the extra call to pwm_apply_state() here.

not really, you can have pwm_state * pwm_ir member and pass pointer to the stack variable.

Quoted 7 lines

Having said that, the extra call to pwm_apply_state() may have benefits, see this comment in the pwm-sifive driver:

* - When changing both duty cycle and period, we cannot prevent in
* software that the output might produce a period with mixed
* settings (new period length and old duty cycle).

Quoted 3 lines

So setting the duty cycle and period once with enabled = false prevents a first period with mixed settings (i.e. bogus).

Who will enable pwm if not in tx? Like, doesn't the driver have exclusive ownership of the pwm? Also, every transmission ends up wit pwm disabled, so disabling it once again does not make sense to me.

Quoted 9 lines

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 80

Quoted 6 lines

drivers/media/rc/pwm-ir-tx.c +1 -5

-		edge = ktime_add_us(edge, txbuf[i]);
-		delta = ktime_us_delta(edge, ktime_get());
-		if (delta > 0)
-			usleep_range(delta, delta + 10);
-	}
+	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);

why not just call it with 0 time?

Quoted 3 lines

Otherwise the timings are a little off for the first edge - hrtimer setup time, I think. I can experiment again.

Why is that? Edge start is controlled by the calls in timer function, it should not matter when it is called for the first time.

Quoted 35 lines

Quoted 34 lines

Quoted 21 lines

drivers/media/rc/pwm-ir-tx.c +17 -2

-	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	wait_for_completion(&pwm_ir->completion);
   	return count;
   }
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			/* Stop TX here */
+			pwm_disable(pwm_ir->pwm);
+

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, · line 112

Quoted 10 lines

drivers/media/rc/pwm-ir-tx.c +10 -0

+			complete(&pwm_ir->completion);
+
+			return HRTIMER_NORESTART;
+		}
+
+		if (pwm_ir->txbuf_index % 2)
+			pwm_disable(pwm_ir->pwm);
+		else
+			pwm_enable(pwm_ir->pwm);
+

pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2); pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

Quoted 2 lines

Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28 bytes vs keeping it around).

see my previous comment re struct var. Also, look at the overhead:
https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/pwm.h#L349 - you call pwm_get_state() for every edge.

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

drivers/media/rc/pwm-ir-tx.c +13 -0

+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
   static int pwm_ir_probe(struct platform_device *pdev)
   {
   	struct pwm_ir *pwm_ir;

@@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
   	if (IS_ERR(pwm_ir->pwm))
   		return PTR_ERR(pwm_ir->pwm);
+	if (pwm_can_sleep(pwm_ir->pwm)) {
+		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");

Quoted 8 lines

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 142

Quoted 3 lines

drivers/media/rc/pwm-ir-tx.c +3 -0

+		return -ENODEV;
+	}
+

I think we shall not limit, but use high priority thread to support those drivers. I have that working on n900 with current (sleeping) pwm, see my reply on the other mail. Maybe we can combine both patches in a way to support both atomic and sleeping pwm drivers.

Quoted 3 lines

If the ir-rx51 driver uses a sleeping pwm then that's broken and only works by accident - the current driver is broken then.

Yes, and I stated that couple of times in my previous emails :)

Quoted 4 lines

Spinning for longer periods (e.g. 100us) does not play well with RT. Would make more sense to fix the pwm driver to non-sleeping when a pwm driver is used for pwm-ir-tx?

Sure, and I have a patch for n900 that does this, however, for your i2c case there is no solution. Also, we may play smart and dynamically decrease sleep time (by adjusting edge by lets say 5-10 us every pulse until we have some sane value) if we see it is too long. No strong preferences here, it is just that I have code that works.

Quoted 7 lines · Ivaylo Dimitrov

Quoted 2 lines · Sean Young

Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28 bytes vs keeping it around).

see my previous comment re struct var. Also, look at the overhead:
https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/pwm.h#L349 - you call pwm_get_state() for every edge.

That's the 28 bytes copy I was talking about.

However keeping a pointer in struct pwm_ir is a good compromise and makes the rest of the code cleaner.

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
  drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
  1 file changed, 63 insertions(+), 16 deletions(-)

Quoted 4 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

Quoted 8 lines

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
  drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
  1 file changed, 63 insertions(+), 16 deletions(-)

what about the following patch(not a proper one, just RFC)? It achieves the same (if not better) precision (on n900 at least) without using atomic pwm. What it does is: create a fifo thread in which we swicth pwm on/off, start hrtimer that is used to signal thread when to switch pwm.
As signal comes earlier than needed(because hrtimer runs async to the thread), we do a busy loop wait for the precise time to switch the pwm. At least on n900, this busy loop is less than 200 us per switch(worst case, usually it is less than 100 us). That way, even if we have some latency spike, it is covered by not doing busy loop for that particular pwm switch and we keep the precise timing.

Maybe we shall merge both patches so fifo thread to be used for sleeping pwms and hrtimer for atomic. I can do that and test it here if you think that approach makes sense.

Regards,
Ivo

PS: I have a patch that converts timer-ti-dm to non-sleeping one, will send it when it comes to it.

drivers/media/rc/pwm-ir-tx.c +13 -2

index 105a9c24f1e3..e8b620f53056 100644
@@ -4,6 +4,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/pwm.h>
 #include <linux/delay.h>

@@ -17,8 +18,16 @@
 
 struct pwm_ir {
 	struct pwm_device *pwm;
+	struct hrtimer timer;
+	struct task_struct *tx_thread;
+	wait_queue_head_t tx_wq;
+	struct completion tx_done;
+	struct completion edge;
 	unsigned int carrier;
 	unsigned int duty_cycle;
+	unsigned int *txbuf;
+	unsigned int count;
+	unsigned int index;
 };
 
 static const struct of_device_id pwm_ir_of_match[] = {

@@ -48,35 +57,103 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32
 carrier)
 	return 0;
 }
 
-static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
-		     unsigned int count)
+static enum hrtimer_restart pwm_ir_timer_cb(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;

drivers/media/rc/pwm-ir-tx.c +64 -8

+
+		complete(&pwm_ir->edge);
+
+		edge = US_TO_NS(pwm_ir->txbuf[pwm_ir->index]);
+		hrtimer_add_expires_ns(timer, edge);
+
+		pwm_ir->index++;
+
+		now = timer->base->get_time();
+
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+out:
+	complete(&pwm_ir->edge);
+
+	return HRTIMER_NORESTART;
+}
+
+static void _pwm_ir_tx(struct pwm_ir *pwm_ir)
 {
-	struct pwm_ir *pwm_ir = dev->priv;
-	struct pwm_device *pwm = pwm_ir->pwm;
 	struct pwm_state state;
 	int i;
 	ktime_t edge;
 	long delta;
 
-	pwm_init_state(pwm, &state);
+	pwm_init_state(pwm_ir->pwm, &state);
 
 	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
 	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
 
+	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
+	wait_for_completion(&pwm_ir->edge);
 	edge = ktime_get();
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < pwm_ir->count; i++) {
 		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+		pwm_apply_state(pwm_ir->pwm, &state);
+
+		edge = ktime_add_us(edge, pwm_ir->txbuf[i]);
+		wait_for_completion(&pwm_ir->edge);
 
-		edge = ktime_add_us(edge, txbuf[i]);
 		delta = ktime_us_delta(edge, ktime_get());
+
 		if (delta > 0)
-			usleep_range(delta, delta + 10);
+			udelay(delta);
 	}
 
 	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm_ir->pwm, &state);
+
+	pwm_ir->count = 0;
+}
+
+static int pwm_ir_thread(void *data)
+{
+	struct pwm_ir *pwm_ir = data;
+
+	for (;;) {
+		wait_event_idle(pwm_ir->tx_wq,
+				kthread_should_stop() || pwm_ir->count);
+
+		if (kthread_should_stop())
+			break;
+
+		_pwm_ir_tx(pwm_ir);
+		complete(&pwm_ir->tx_done);
+	}
+
+	return 0;
+}
+
+static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
+		     unsigned int count)
+{
+	struct pwm_ir *pwm_ir = dev->priv;
+
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->count = count;
+	pwm_ir->index = 0;
+
+	wake_up(&pwm_ir->tx_wq);
+	wait_for_completion(&pwm_ir->tx_done);
 
 	return count;

}

drivers/media/rc/pwm-ir-tx.c +13 -1

@@ -91,12 +168,24 @@ static int pwm_ir_probe(struct platform_device *pdev)
 	if (!pwm_ir)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, pwm_ir);
+
+	pwm_ir->count = 0;
+
 	pwm_ir->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pwm_ir->pwm))
 		return PTR_ERR(pwm_ir->pwm);
 
-	pwm_ir->carrier = 38000;
+	init_waitqueue_head(&pwm_ir->tx_wq);
+	init_completion(&pwm_ir->edge);
+	init_completion(&pwm_ir->tx_done);
+
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer_cb;
+
 	pwm_ir->duty_cycle = 50;
+	pwm_ir->carrier = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm_ir->pwm),
+						NSEC_PER_SEC);
 
 	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
 	if (!rcdev)

drivers/media/rc/pwm-ir-tx.c +17 -1

+
 	rc = devm_rc_register_device(&pdev->dev, rcdev);
-	if (rc < 0)
+	if (rc < 0) {
+		kthread_stop(pwm_ir->tx_thread);
 		dev_err(&pdev->dev, "failed to register rc device\n");
+	}
 
 	return rc;
 }
 
+static int pwm_ir_remove(struct platform_device *pdev)
+{
+	struct pwm_ir *pwm_ir = platform_get_drvdata(pdev);
+
+	if (pwm_ir->tx_thread) {
+		kthread_stop(pwm_ir->tx_thread);
+		pwm_ir->tx_thread = NULL;
+	}
+
+	return 0;
+}
+
 static struct platform_driver pwm_ir_driver = {
 	.probe = pwm_ir_probe,
+	.remove = pwm_ir_remove,
 	.driver = {
 		.name	= DRIVER_NAME,
 		.of_match_table = of_match_ptr(pwm_ir_of_match),

Hi,

Quoted 14 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

Quoted 8 lines

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
   1 file changed, 63 insertions(+), 16 deletions(-)

Quoted 10 lines

what about the following patch(not a proper one, just RFC)? It achieves the same (if not better) precision (on n900 at least) without using atomic pwm. What it does is: create a fifo thread in which we swicth pwm on/off, start hrtimer that is used to signal thread when to switch pwm.
As signal comes earlier than needed(because hrtimer runs async to the thread), we do a busy loop wait for the precise time to switch the pwm. At least on n900, this busy loop is less than 200 us per switch(worst case, usually it is less than 100 us). That way, even if we have some latency spike, it is covered by not doing busy loop for that particular pwm switch and we keep the precise timing.

I think this is a good idea.

Quoted 3 lines

Maybe we shall merge both patches so fifo thread to be used for sleeping pwms and hrtimer for atomic. I can do that and test it here if you think that approach makes sense.

Let's try and merge this patch for the next merge window, and worry about the atomic version after that. We've already queued the ir-rx51 removal patches to media_stage so it would be nice to have to revert these patches, and improve pwm-ir-tx for the next kernel release.

I'll test your patch, in the mean time would you mind posting this patch as a proper patch (with review comments below addressed)?

Thanks,

Sean

Quoted 52 lines

Regards,
Ivo

PS: I have a patch that converts timer-ti-dm to non-sleeping one, will send it when it comes to it.

drivers/media/rc/pwm-ir-tx.c +18 -2

index 105a9c24f1e3..e8b620f53056 100644
@@ -4,6 +4,7 @@
   */
 
  #include <linux/kernel.h>
+#include <linux/kthread.h>
  #include <linux/module.h>
  #include <linux/pwm.h>
  #include <linux/delay.h>

@@ -17,8 +18,16 @@
 
  struct pwm_ir {
  	struct pwm_device *pwm;
+	struct hrtimer timer;
+	struct task_struct *tx_thread;
+	wait_queue_head_t tx_wq;
+	struct completion tx_done;
+	struct completion edge;
  	unsigned int carrier;
  	unsigned int duty_cycle;
+	unsigned int *txbuf;
+	unsigned int count;
+	unsigned int index;
  };
 
  static const struct of_device_id pwm_ir_of_match[] = {

@@ -48,35 +57,103 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32
 carrier)
  	return 0;
  }
 
-static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
-		     unsigned int count)
+static enum hrtimer_restart pwm_ir_timer_cb(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_set_carrier(struct rc_dev *dev, u32 carrier) · line 73

Quoted 5 lines

drivers/media/rc/pwm-ir-tx.c +5 -0

+	do {
+		u64 edge;
+
+		if (pwm_ir->index >= pwm_ir->count)
+			goto out;

Might as well avoid the goto and put the complete and return in the body of the if.

Quoted 124 lines

drivers/media/rc/pwm-ir-tx.c +64 -8

+
+		complete(&pwm_ir->edge);
+
+		edge = US_TO_NS(pwm_ir->txbuf[pwm_ir->index]);
+		hrtimer_add_expires_ns(timer, edge);
+
+		pwm_ir->index++;
+
+		now = timer->base->get_time();
+
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+out:
+	complete(&pwm_ir->edge);
+
+	return HRTIMER_NORESTART;
+}
+
+static void _pwm_ir_tx(struct pwm_ir *pwm_ir)
  {
-	struct pwm_ir *pwm_ir = dev->priv;
-	struct pwm_device *pwm = pwm_ir->pwm;
  	struct pwm_state state;
  	int i;
  	ktime_t edge;
  	long delta;
 
-	pwm_init_state(pwm, &state);
+	pwm_init_state(pwm_ir->pwm, &state);
 
  	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
  	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
 
+	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
+	wait_for_completion(&pwm_ir->edge);
  	edge = ktime_get();
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < pwm_ir->count; i++) {
  		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+		pwm_apply_state(pwm_ir->pwm, &state);
+
+		edge = ktime_add_us(edge, pwm_ir->txbuf[i]);
+		wait_for_completion(&pwm_ir->edge);
 
-		edge = ktime_add_us(edge, txbuf[i]);
  		delta = ktime_us_delta(edge, ktime_get());
+
  		if (delta > 0)
-			usleep_range(delta, delta + 10);
+			udelay(delta);
  	}
 
  	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm_ir->pwm, &state);
+
+	pwm_ir->count = 0;
+}
+
+static int pwm_ir_thread(void *data)
+{
+	struct pwm_ir *pwm_ir = data;
+
+	for (;;) {
+		wait_event_idle(pwm_ir->tx_wq,
+				kthread_should_stop() || pwm_ir->count);
+
+		if (kthread_should_stop())
+			break;
+
+		_pwm_ir_tx(pwm_ir);
+		complete(&pwm_ir->tx_done);
+	}
+
+	return 0;
+}
+
+static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
+		     unsigned int count)
+{
+	struct pwm_ir *pwm_ir = dev->priv;
+
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->count = count;
+	pwm_ir->index = 0;
+
+	wake_up(&pwm_ir->tx_wq);
+	wait_for_completion(&pwm_ir->tx_done);
 
  	return count;

  }

drivers/media/rc/pwm-ir-tx.c +13 -1

@@ -91,12 +168,24 @@ static int pwm_ir_probe(struct platform_device *pdev)
  	if (!pwm_ir)
  		return -ENOMEM;
 
+	platform_set_drvdata(pdev, pwm_ir);
+
+	pwm_ir->count = 0;
+
  	pwm_ir->pwm = devm_pwm_get(&pdev->dev, NULL);
  	if (IS_ERR(pwm_ir->pwm))
  		return PTR_ERR(pwm_ir->pwm);
 
-	pwm_ir->carrier = 38000;
+	init_waitqueue_head(&pwm_ir->tx_wq);
+	init_completion(&pwm_ir->edge);
+	init_completion(&pwm_ir->tx_done);
+
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer_cb;
+
  	pwm_ir->duty_cycle = 50;
+	pwm_ir->carrier = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm_ir->pwm),
+						NSEC_PER_SEC);
 
  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
  	if (!rcdev)

@@ -109,15 +198,38 @@ static int pwm_ir_probe(struct platform_device *pdev)
  	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
  	rcdev->s_tx_carrier = pwm_ir_set_carrier;
 

drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 207

Quoted 7 lines

drivers/media/rc/pwm-ir-tx.c +7 -0

+	pwm_ir->tx_thread = kthread_create(pwm_ir_thread, pwm_ir, "%s/tx",
+					   dev_name(&pdev->dev));
+	if (IS_ERR(pwm_ir->tx_thread))
+		return PTR_ERR(pwm_ir->tx_thread);
+
+	sched_set_fifo(pwm_ir->tx_thread);
+	wake_up_process(pwm_ir->tx_thread);

This means the thread is always around. How about creating the thread per-tx?

Quoted 29 lines

drivers/media/rc/pwm-ir-tx.c +17 -1

+
  	rc = devm_rc_register_device(&pdev->dev, rcdev);
-	if (rc < 0)
+	if (rc < 0) {
+		kthread_stop(pwm_ir->tx_thread);
  		dev_err(&pdev->dev, "failed to register rc device\n");
+	}
 
  	return rc;
  }
 
+static int pwm_ir_remove(struct platform_device *pdev)
+{
+	struct pwm_ir *pwm_ir = platform_get_drvdata(pdev);
+
+	if (pwm_ir->tx_thread) {
+		kthread_stop(pwm_ir->tx_thread);
+		pwm_ir->tx_thread = NULL;
+	}
+
+	return 0;
+}
+
  static struct platform_driver pwm_ir_driver = {
  	.probe = pwm_ir_probe,
+	.remove = pwm_ir_remove,
  	.driver = {
  		.name	= DRIVER_NAME,
  		.of_match_table = of_match_ptr(pwm_ir_of_match),

Quoted 28 lines · Sean Young

Quoted 14 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.

Quoted 8 lines

This makes the driver much more precise.

Signed-off-by Sean Young <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

---
   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
   1 file changed, 63 insertions(+), 16 deletions(-)

Quoted 10 lines

what about the following patch(not a proper one, just RFC)? It achieves the same (if not better) precision (on n900 at least) without using atomic pwm. What it does is: create a fifo thread in which we swicth pwm on/off, start hrtimer that is used to signal thread when to switch pwm.
As signal comes earlier than needed(because hrtimer runs async to the thread), we do a busy loop wait for the precise time to switch the pwm. At least on n900, this busy loop is less than 200 us per switch(worst case, usually it is less than 100 us). That way, even if we have some latency spike, it is covered by not doing busy loop for that particular pwm switch and we keep the precise timing.

I think this is a good idea.

Quoted 9 lines

Quoted 3 lines

Maybe we shall merge both patches so fifo thread to be used for sleeping pwms and hrtimer for atomic. I can do that and test it here if you think that approach makes sense.

Let's try and merge this patch for the next merge window, and worry about the atomic version after that. We've already queued the ir-rx51 removal patches to media_stage so it would be nice to have to revert these patches, and improve pwm-ir-tx for the next kernel release.

ir-rx51 is broken without
https://www.spinics.net/lists/kernel/msg4953300.html, it is also missing a call to init_waitqueue_head() in the probe() function. So I have no strong opinion on what shall be done with it.

Quoted 3 lines

This means the thread is always around. How about creating the thread per-tx?

Yes, that can be done, just not sure what the overhead would be.

Also, I think we shall reconsider the way the driver works:

Imagine we have to pretend we are TV remote that supports NEC protocol (for example), especially the "REPEAT CODES" part. Currently, no matter what we do, there is no way to get the timings even remotely right, as we have no idea what the "warmup" and "complete" delays are. Like, starting thread (if needed), hrtimer setup time, completions waiting, contexts switching, etc.

So, I think the correct thing to do is to copy txbuf (as a list of txbufs) into pwm_ir in tx function, start pulses generation and return from pwm_ir_tx() *immediately*, without waiting for tx to finish. If userspace requests submission of another set of pulses while we are still sending the current one, well, we accept it, add it to the list and delay the sending until the current one is finished. When there is nothing more to send (the list is empty), stop the hrtimer (and perhaps the thread)

I think that way userspace will be able to append as many "repeat" pulses with proper timings as it wants (with some sane limits ofc).

Unless we somehow have API restriction that we shall not return until tx is finished.

Does that make any sense to you?