| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z8B2Bl/9uD3jPvQi@linaro.org>
Date: Thu, 27 Feb 2025 16:26:14 +0200
From: Abel Vesa <abel.vesa@...aro.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
Pavel Machek <pavel@...nel.org>,
Anjelique Melendez <quic_amelende@...cinc.com>,
Kamal Wadhwa <quic_kamalw@...cinc.com>,
Jishnu Prakash <jishnu.prakash@....qualcomm.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Johan Hovold <johan@...nel.org>, Pavel Machek <pavel@....cz>,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [PATCH] leds: rgb: leds-qcom-lpg: Fix pwm resolution for Hi-Res
PWMs
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
> Hello,
>
> I was dragged into the discussion by the patch that Abel Vesa created in
> reply to this mail, i.e.
> https://lore.kernel.org/linux-pwm/20250226-pwm-bl-read-back-period-from-hw-v1-1-ccd1df656b23@linaro.org/
>
> On Tue, Feb 25, 2025 at 10:09:57AM +0200, Abel Vesa wrote:
> > On 25-02-25 01:09:00, Sebastian Reichel wrote:
> > > On Mon, Feb 24, 2025 at 10:24:33PM +0200, Abel Vesa wrote:
> > > > On 25-02-21 00:35:08, Sebastian Reichel wrote:
> > > > > On Thu, Feb 20, 2025 at 12:31:00PM +0200, Abel Vesa wrote:
> > > > > > Currently, for the high resolution PWMs, the resolution, clock,
> > > > > > pre-divider and exponent are being selected based on period. Basically,
> > > > > > the implementation loops over each one of these and tries to find the
> > > > > > closest (higher) period based on the following formula:
> > > > > >
> > > > > > period * refclk
> > > > > > prediv_exp = log2 -------------------------------------
> > > > > > NSEC_PER_SEC * pre_div * resolution
> > > > > >
> > > > > > Since the resolution is power of 2, the actual period resulting is
> > > > > > usually higher than what the resolution allows. That's why the duty
> > > > > > cycle requested needs to be capped to the maximum value allowed by the
> > > > > > resolution (known as PWM size).
> > > > > >
> > > > > > Here is an example of how this can happen:
> > > > > >
> > > > > > For a requested period of 5000000, the best clock is 19.2MHz, the best
> > > > > > prediv is 5, the best exponent is 6 and the best resolution is 256.
> > > > > >
> > > > > > Then, the pwm value is determined based on requested period and duty
> > > > > > cycle, best prediv, best exponent and best clock, using the following
> > > > > > formula:
> > > > > >
> > > > > > duty * refclk
> > > > > > pwm_value = ----------------------------------------------
> > > > > > NSEC_PER_SEC * prediv * (1 << prediv_exp)
> > > > > >
> > > > > > So in this specific scenario:
> > > > > >
> > > > > > (5000000 * 19200000) / (1000000000 * 5 * (1 << 64)) = 300
> > > > > >
> > > > > > With a resolution of 8 bits, this pwm value obviously goes over.
> > > > > >
> > > > > > Therefore, the max pwm value allowed needs to be 255.
> > > > > >
> > > > > > If not, the PMIC internal logic will only value that is under the set PWM
> > > > > > size, resulting in a wrapped around PWM value.
> > > > > >
> > > > > > This has been observed on Lenovo Thinkpad T14s Gen6 (LCD panel version)
> > > > > > which uses one of the PMK8550 to control the LCD backlight.
> > > > > >
> > > > > > Fix the value of the PWM by capping to a max based on the chosen
> > > > > > resolution (PWM size).
> > > > > >
> > > > > > Cc: stable@...r.kernel.org # 6.4
> > > > > > Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> > > > > > Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> > > > > > ---
> > > > > > Note: This fix is blocking backlight support on Lenovo Thinkpad T14s
> > > > > > Gen6 (LCD version), for which I have patches ready to send once this
> > > > > > patch is agreed on (review) and merged.
> > > > > > ---
> > > > >
> > > > > Do you know if the pwm duty cycle to pwm value calculation is
> > > > > correct otherwise?
> > > >
> > > > Sorry for the late reply.
> > >
> > > No worries, I understand this takes time.
> > >
> > > > Here is my understanding of the calculation of the pwm value currently
> > > > implemented.
> > > >
> > > > First, find the best pre_div, refclk, resolution and prediv_exp by looping
> > > > through all refclk, resolution and prediv possible values, for the
> > > > following formula:
> > > >
> > > > period * refclk
> > > > prediv_exp = log2 -------------------------------------
> > > > NSEC_PER_SEC * pre_div * (1 << resolution)
> > > >
> > > >
> > > > So in DT we set the period to 50000000. For this, as I mentioned in the
> > > > commit message the best refclk is 19.2MHz, the best prediv is 5, the best
> > > > exponent is 6 and the best resolution is 255.
> > > >
> > > > So if you use these to compute the period following this formula:
> > > >
> > > >
> > > > NSEC_PER_SEC * prediv * (1 << resolution)
> > > > best_period = -------------------------------------------
> > > > refclk
> > > >
> > > > So in our case:
> > > >
> > > > (1000000000 * 5 * (1 << 8) * (1 << 6)) / 19200000 = 4266666.6666...
> > > >
> > > > So here is where the things go wrong. Bjorn helped me figure this out today
> > > > (off-list). Basically, the pwm framework will allow values up to 5000000,
> > > > as specified in the DT, but for then pwm value will go over 255
> > > > when computing the actual pwm value by the following formula:
> > > >
> > > > duty * refclk
> > > > pwm_value = ----------------------------------------------
> > > > NSEC_PER_SEC * prediv * (1 << prediv_exp)
> > > >
> > > >
> > > > So here is how the value 300 is reached (I messed up this next formula in
> > > > the commit message):
> > > >
> > > > (5000000 * 19200000) / (1000000000 * 5 * (1 << 8)) = 300
> > > >
> > > > But if we were to use the best_period determined:
> > > >
> > > > (4266666 * 19200000) / (1000000000 * 5 * (1 << 8)) = 255
> > > >
> > > > So I guess the process of determining the best parameters is correct.
> > > > What I think is missing is we need to divide the requested period (5000000)
> > > > to the resolution (255) and make sure the duty cycle is a multiple of the
> > > > result.
> > >
> > > Let me try to summarize that:
> > >
> > > 1. PWM backlight driver requests PWM with 5 MHz period
> > > 2. leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
> > > 3. PWM backlight driver is unaware and requests a duty cycle
> > > expecting the period to be 5 MHz, so the duty cycle can
> > > exceed 100%
>
> Can you please enable CONFIG_PWM_DEBUG, enable pwm tracing (
>
> echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
>
> ) then reproduce the problem and provide the output of
>
> cat /sys/kernel/debug/tracing/trace
>
> .
$ cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 13/13 #P:12
#
# _-----=> irqs-off/BH-disabled
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / _-=> migrate-disable
# |||| / delay
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
modprobe-203 [000] ..... 0.938668: pwm_get: pwmchip0.0: period=1066407 duty_cycle=533334 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.938775: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=0 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.938821: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.938936: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.938982: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.939274: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=921458 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.939320: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.939434: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0
modprobe-203 [000] ..... 0.939480: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
systemd-backlig-724 [006] ..... 9.079585: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0
systemd-backlig-724 [006] ..... 9.079698: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0
systemd-backlig-724 [006] ..... 9.079750: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0
$
>
> I didn't take a deeper dive in this driver combination, but here is a
> description about what *should* happen:
>
> You're talking about period in MHz, the PWM abstraction uses
> nanoseconds. So your summary translated to the PWM wording is (to the
> best of my understanding):
>
> 1. PWM backlight driver requests PWM with .period = 200 ns and
> .duty_cycle = 200 ns.
>
> 2. leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period =
> 1000000000 / 4.26666 MHz = 234.375 ns
>
> 3. leds-qcom-lpg then determines setting for requested .duty_cycle
> based on .period = 200 ns which then ends up with something bogus.
>
> Right?
>
> There is a problem in 2. already: The PWM hardware driver is supposed to
> pick the highest period (in ns) not bigger than the requested value. So
> it must not pick 234.375 ns. (Enabling CONFIG_PWM_DEBUG on that is
> supposed to wail about that.) It should instead pick (say) 187 ns. In
> the next step the hw driver should pick the highest duty_cycle (again in
> ns) not exceeding the requested value (and physics). That will be (I
> guess) also 187 ns in the constructed example. So you should get your
> requested 100 % relative duty cycle at least.
>
> So the problem about now knowing the resulting PWM waveform is somewhat
> real. I think if leds-qcom-lpg behaved as expected from a PWM driver, it
> would be a tad better than your report suggests. I might miss something
> though.
>
> Best regards
> Uwe
Powered by blists - more mailing lists