Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 2 Oct 2023 09:20:20 +0100
From:   Sean Young <sean@mess.org>
To:     Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org
Subject: Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer
 interrupt context
Message-ID: <ZRp9RE2jOZdL0+1/@gofer.mess.org>
References: <cover.1696156485.git.sean@mess.org>
 <7efe4229514001b835fa70d51973cd3306dc0b04.1696156485.git.sean@mess.org>
 <5982681d-4fb5-0271-fdc5-712d6c8512e3@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5982681d-4fb5-0271-fdc5-712d6c8512e3@gmail.com>
Precedence: bulk

Hi,

On Mon, Oct 02, 2023 at 08:49:47AM +0300, Ivaylo Dimitrov wrote:
> On 1.10.23 г. 13:40 ч., Sean Young wrote:
> > 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 <sean@mess.org>
> > Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > ---
> >   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> >   1 file changed, 63 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> > index c5f37c03af9c..557725a07a67 100644
> > --- a/drivers/media/rc/pwm-ir-tx.c
> > +++ b/drivers/media/rc/pwm-ir-tx.c
> > @@ -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;
> >   	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.

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

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).

> > -		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.

> > -	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;
> > +		}
> > +
> > +		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).

> > +		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");
> > +		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

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