Message-ID: <5urcec625u3jva5kyf7bztehqijfhztmo4op25joqyt7gl6kjt@5tn2fbot3fri>
Date: Wed, 30 Apr 2025 14:33:42 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <danielt@...nel.org>, 
	Jingoo Han <jingoohan1@...il.com>, Helge Deller <deller@....de>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Johan Hovold <johan@...nel.org>, Sebastian Reichel <sre@...nel.org>, linux-pwm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, linux-fbdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider

Hello Abel,

On Thu, Feb 27, 2025 at 06:50:38PM +0200, Abel Vesa wrote:
> On 25-02-27 16:51:15, Uwe Kleine-König wrote:
> > On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> > > On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > > > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > > > The current implementation assumes that the PWM provider will be able to
> > > > > meet the requested period, but that is not always the case. Some PWM
> > > > > providers have limited HW configuration capabilities and can only
> > > > > provide a period that is somewhat close to the requested one. This
> > > > > simply means that the duty cycle requested might either be above the
> > > > > PWM's maximum value or the 100% duty cycle is never reached.
> > > > 
> > > > If you request a state with 100% relative duty cycle you should get 100%
> > > > unless the hardware cannot do that. Which PWM hardware are you using?
> > > > Which requests are you actually doing that don't match your expectation?
> > > 
> > > The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> > > controlled is described in the following comment found in lpg_calc_freq
> > > of the leds-qcom-lpg driver:
> > > 
> > > /*
> > >  * The PWM period is determined by:
> > >  *
> > >  *          resolution * pre_div * 2^M
> > >  * period = --------------------------
> > >  *                   refclk
> > >  *
> > >  * Resolution = 2^9 bits for PWM or
> > >  *              2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> > >  * pre_div = {1, 3, 5, 6} and
> > >  * M = [0..7].
> > >  *
> > >  * This allows for periods between 27uS and 384s for PWM channels and periods between
> > >  * 3uS and 24576s for high resolution PWMs.
> > >  * The PWM framework wants a period of equal or lower length than requested,
> > >  * reject anything below minimum period.
> > >  */
> > > 
> > > So if we request a period of 5MHz, that will not ever be reached no matter what config
> > > is used. Instead, the 4.26 MHz is selected as closest possible.
> > 
> > The trace in the other mail thread suggest that you asked for a period
> > of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
> 
> OK. So unit is ms. Got it.
> 
> > 
> > > Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> > > go above 4.26MHz.
> > 
> > It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
> > according to the trace this results in both values becoming 4.26 ms in
> > real life. Seems fine to me.
> 
> Right, but as I keep trying to explain is that, the consumer keeps
> asking for duty cycles that go over the 4.26ms, which is the period that
> the provider decided it can do instead of 5ms.

I see no problem here. Yes, requests are not implemented exactly in
general, but there is no way around that. For some use-cases the picked
configuration is sensible, for others less so. There is also no way
around that.

> > > > > This could be easily fixed if the pwm_apply*() API family would allow
> > > > > overriding the period within the PWM state that's used for providing the
> > > > > duty cycle. But that is currently not the case.
> > > > 
> > > > I don't understand what you mean here.
> > > 
> > > What I was trying to say is that the PWM generic framework currently doesn't
> > > allow overriding the PWM state's period with one provided by the consumer,
> > > when calling pwm_apply_might_sleep().
> > 
> > Either I still don't understand what you want, or that is impossible or
> > useless. If you target .period = 5 ms and the hardware can only do 4.26
> > ms, why would you want to override period to 5 ms?
> 
> Meaning the consumer should become aware of the period the provider can
> do before asking for a duty cycle. 

There is pwm_round_waveform_might_sleep() that you could use. Downside
is that is currently only works for two lowlevel drivers.

> If you look at the other mail thread, the trace there shows the
> following sequence for every new backlight update request:
> 
> 1. pwm_apply with consumer's period (5ms)
> 2. pwm_get reads the provider's period (4.25ms) 
>    - which is what the provider is able to do instead of 5ms
> 3. pwm_apply (due to debug) which uses the state from 2.
> 4. pwm_get reads back exactly as 2.
> 
> So we can ignore 3 and 4 for now as they are there due to debug,
> but the step 1 still requests a value over the 4.26ms (5ms),
> which in the provider will translate to a pwm value higher than allowed
> by the selected configuration.

The lowlevel driver sees the request e.g.

	.period = 5000
	.duty_cycle = 4600

and is supposed to implement (I guess)

	.period = 4260
	.duty_cycle = 4260

. I don't see a technical problem here (apart from you being unlucky
about the choice made?).

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists