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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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. --- drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate;
if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) { - max = LPG_RESOLUTION_15BIT - 1; + max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1; clk_rate = lpg_clk_rates_hi_res[chan->clk_sel]; } else { max = LPG_RESOLUTION_9BIT - 1;
--- base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Hello Abel,
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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?
I'm asking because the max value is only used for capping, so with this patch the maximum brightness will be reached at around 80% duty cycle (i.e. when the wrap over happens without this patch).
Locally I'm currently remapping the duty cycle range to the PWM value range, which means the display brightness increases step-by-step until reaching 100% "duty cycle":
val = (duty * 255) / chan->period; chan->pwm_value = min(val, 255);
But for the backlight control the absolute numbers do not really matter and I have zero knowledge about the chip. So it might be that the controller really can only go up to ~80% duty cycle at these settings?
Greetings,
-- Sebastian
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Abel Vesa abel.vesa@linaro.org
On 25-02-21 00:35:08, Sebastian Reichel wrote:
Hello Abel,
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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.
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.
Something like this:
step = period / (1 << resolution)
So:
5000000 / ((1 << 8) - 1) = 19607
and then:
pwm_value = duty / step;
Hope this makes sense.
Will try this out and respin the patch.
I'm asking because the max value is only used for capping, so with this patch the maximum brightness will be reached at around 80% duty cycle (i.e. when the wrap over happens without this patch).
Locally I'm currently remapping the duty cycle range to the PWM value range, which means the display brightness increases step-by-step until reaching 100% "duty cycle":
val = (duty * 255) / chan->period; chan->pwm_value = min(val, 255);But for the backlight control the absolute numbers do not really matter and I have zero knowledge about the chip. So it might be that the controller really can only go up to ~80% duty cycle at these settings?
Greetings,
-- Sebastian
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Abel Vesa abel.vesa@linaro.org
Hi,
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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 * refclkprediv_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 * refclkpwm_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%
Then the question is: Why is the PWM backlight driver not aware of the reduced period? It runs pwm_get_state(), so leds-qcom-lpg can actually report back that it is using 4.2 MHz instead of 5 MHz.
I guess that also means the bug could be avoided by requesting a period of 4266666 in DT in the first place. Might be an option to unblock the T14s upstreaming.
Greetings,
-- Sebastian
Something like this:
step = period / (1 << resolution)
So:
5000000 / ((1 << 8) - 1) = 19607
and then:
pwm_value = duty / step;
Hope this makes sense.
Will try this out and respin the patch.
I'm asking because the max value is only used for capping, so with this patch the maximum brightness will be reached at around 80% duty cycle (i.e. when the wrap over happens without this patch).
Locally I'm currently remapping the duty cycle range to the PWM value range, which means the display brightness increases step-by-step until reaching 100% "duty cycle":
val = (duty * 255) / chan->period; chan->pwm_value = min(val, 255);But for the backlight control the absolute numbers do not really matter and I have zero knowledge about the chip. So it might be that the controller really can only go up to ~80% duty cycle at these settings?
Greetings,
-- Sebastian
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Abel Vesa abel.vesa@linaro.org
On 25-02-25 01:09:00, Sebastian Reichel wrote:
Hi,
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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 * refclkprediv_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 * refclkpwm_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:
- PWM backlight driver requests PWM with 5 MHz period
- leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
- PWM backlight driver is unaware and requests a duty cycle expecting the period to be 5 MHz, so the duty cycle can exceed 100%
Yes, exactly.
Then the question is: Why is the PWM backlight driver not aware of the reduced period? It runs pwm_get_state(), so leds-qcom-lpg can actually report back that it is using 4.2 MHz instead of 5 MHz.
That's a good point. Will try to do that instead.
I guess that also means the bug could be avoided by requesting a period of 4266666 in DT in the first place. Might be an option to unblock the T14s upstreaming.
Haven't tried yet. But yes, it should work. Will try soon.
Greetings,
-- Sebastian
Something like this:
step = period / (1 << resolution)
So:
5000000 / ((1 << 8) - 1) = 19607
and then:
pwm_value = duty / step;
As for this, it's all wrong, because if the user will expect an exact duty cycle, this will not give that. And I think it's obvious why.
So your suggestion of reporting the "actual" period should be the way to go.
Hope this makes sense.
Will try this out and respin the patch.
I'm asking because the max value is only used for capping, so with this patch the maximum brightness will be reached at around 80% duty cycle (i.e. when the wrap over happens without this patch).
Locally I'm currently remapping the duty cycle range to the PWM value range, which means the display brightness increases step-by-step until reaching 100% "duty cycle":
val = (duty * 255) / chan->period; chan->pwm_value = min(val, 255);But for the backlight control the absolute numbers do not really matter and I have zero knowledge about the chip. So it might be that the controller really can only go up to ~80% duty cycle at these settings?
Greetings,
-- Sebastian
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Abel Vesa abel.vesa@linaro.org
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-v...
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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 * refclkprediv_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 * refclkpwm_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:
- PWM backlight driver requests PWM with 5 MHz period
- leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
- 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
.
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
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-v...
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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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 * refclkprediv_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 * refclkpwm_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:
- PWM backlight driver requests PWM with 5 MHz period
- leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
- 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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- 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
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
Best regards Uwe
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow. So this is what is wrong. And this is what actually happens.
Best regards Uwe
On 25-02-27 17:44:35, Abel Vesa wrote:
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG.
But still, the first pwm_apply() requests duty cycle of 5MHz:
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
So since the period is 4.26MHz, due to the knobs selected by the provider, this duty cycle will result in a PWM value that is above the selected resolution, as I already mentioned.
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow. So this is what is wrong. And this is what actually happens.
Best regards Uwe
On 25-02-27 18:32:41, Abel Vesa wrote:
On 25-02-27 17:44:35, Abel Vesa wrote:
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG.
But still, the first pwm_apply() requests duty cycle of 5MHz:
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
So since the period is 4.26MHz, due to the knobs selected by the provider, this duty cycle will result in a PWM value that is above the selected resolution, as I already mentioned.
On top of that, the duty cycle in debugfs is also reported as 5000000ns when in fact it is 4266666ns, as the trace shows.
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow. So this is what is wrong. And this is what actually happens.
Best regards Uwe
Hello,
On Thu, Feb 27, 2025 at 07:05:14PM +0200, Abel Vesa wrote:
On 25-02-27 18:32:41, Abel Vesa wrote:
On 25-02-27 17:44:35, Abel Vesa wrote:
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote:
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):
PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
- leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG.
ack. This is done just for the tests implemented in CONFIG_PWM_DEBUG, as are the two pwm_get()s.
But still, the first pwm_apply() requests duty cycle of 5MHz:
5 ms, yes. But it cannot give you 5 ms and so you get 4.266 ns.
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
So since the period is 4.26MHz, due to the knobs selected by the provider, this duty cycle will result in a PWM value that is above the selected resolution, as I already mentioned.
"above the selected resolution"? Do you mean you don't get the exact value that you requested?
On top of that, the duty cycle in debugfs is also reported as 5000000ns when in fact it is 4266666ns, as the trace shows.
Yes. Consider that a relict from the times when there was no pwm_get_state_hw(). Both values are interesting in different situations. So just telling the real parameters isn't the optimal way forward either.
Something like the patch I showed in https://lore.kernel.org/all/7bcnckef23w6g47ll5l3bktygedrcfvr7fk3qjuq2swtoffh... would make you a bit luckier I guess. Feel free to polish that one a bit (e.g. by checking the return value of pwm_get_state_hw() and acting sensible in reply to it) and send a proper patch. (A Suggested-by for me is enough for such a patch, grab authorship yourself.)
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow.
No it will not. The .duty_cycle value (also 5000000 ns) will reach the lowlevel PWM driver together with .period = 5000000 ns. Both are rounded down to 4266666ns. I see no overflow.
Best regards Uwe
On 25-02-27 19:09:39, Uwe Kleine-König wrote:
Hello,
On Thu, Feb 27, 2025 at 07:05:14PM +0200, Abel Vesa wrote:
On 25-02-27 18:32:41, Abel Vesa wrote:
On 25-02-27 17:44:35, Abel Vesa wrote:
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote:
On 25-02-27 10:58:47, Uwe Kleine-König wrote: > 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.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG.
ack. This is done just for the tests implemented in CONFIG_PWM_DEBUG, as are the two pwm_get()s.
But still, the first pwm_apply() requests duty cycle of 5MHz:
5 ms, yes. But it cannot give you 5 ms and so you get 4.266 ns.
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
So since the period is 4.26MHz, due to the knobs selected by the provider, this duty cycle will result in a PWM value that is above the selected resolution, as I already mentioned.
"above the selected resolution"? Do you mean you don't get the exact value that you requested?
I think I understand your point now.
You expectation is that the provider would remap the entire range of the period to whatever the HW can do.
So in this case, when 5ms is requested as duty cycle from consumer, the provider will select the max value.
What the current implementation of the leds-qcom-lpg does is that will expect a duty cycle request of up to 4.26ms. And according to you, even if the consumer requests 5ms, the leds-qcom-lpg driver should write the value of 255 (which is what the selected resolution allows (1 << 8) ) and not compute a higher value.
I think this is wrong though. The fact that the pwm generic framework reports 5ms when it is actually 4.26ms should be considered wrong. For cases where the exact value of the duty cycle matters, this would not even make sense.
Correct me if I'm wrong, but the pwm API should behave more like: The consumer should ask for the closest period the HW can actually do and then use that closest period from there on for every duty cycle request. This way, if the consumer initially wants 5ms but the provider can do only 4.26ms instead, at least the consumer would be able to correct its duty cycle requests based on what the HW says it can do.
On top of that, the duty cycle in debugfs is also reported as 5000000ns when in fact it is 4266666ns, as the trace shows.
Yes. Consider that a relict from the times when there was no pwm_get_state_hw(). Both values are interesting in different situations. So just telling the real parameters isn't the optimal way forward either.
Something like the patch I showed in https://lore.kernel.org/all/7bcnckef23w6g47ll5l3bktygedrcfvr7fk3qjuq2swtoffh...
And this patchset only adds the info of actual value that the HW is actually doing. So basically, the already existing state in this case will represent the "desired" state.
would make you a bit luckier I guess. Feel free to polish that one a bit (e.g. by checking the return value of pwm_get_state_hw() and acting sensible in reply to it) and send a proper patch. (A Suggested-by for me is enough for such a patch, grab authorship yourself.)
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow.
No it will not. The .duty_cycle value (also 5000000 ns) will reach the lowlevel PWM driver together with .period = 5000000 ns. Both are rounded down to 4266666ns. I see no overflow.
Again, the consumer is being lied to. It expects 5ms and gets 4.26ms instead.
Imagine a device that is controlled via PWM and needs exact duty cycle values in ms, what would the consumer driver do in this case?
And to make things worse, when the consumer is asking for duty cycle of 4ms while the period requested is 5ms (which would be 80%), the period the provider will do is actually 4.26ms while the duty cycle would be ~3.41ms, which if the pwm step (reg value) doesn't allow, it will probably result in an actual value that is even further than what the consumer is expecting.
So I'm thinking maybe the pwm should probably even ask the provider for what duty cycle it will provide based on provider's provided period and then decide if the resulting duty cycle is what it really wants.
IIRC, this is more in line with what the CCF (common clocks framework) currently does.
Best regards Uwe
Hello Abel,
On Fri, Feb 28, 2025 at 10:59:09AM +0200, Abel Vesa wrote:
On 25-02-27 19:09:39, Uwe Kleine-König wrote:
On Thu, Feb 27, 2025 at 07:05:14PM +0200, Abel Vesa wrote:
On 25-02-27 18:32:41, Abel Vesa wrote:
On 25-02-27 17:44:35, Abel Vesa wrote:
On 25-02-27 16:25:06, Uwe Kleine-König wrote:
Hello Abel,
On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote: > On 25-02-27 10:58:47, Uwe Kleine-König wrote: > > 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.
The trace looks better than what I expected. 2. is fine here because it seems when Sebastian wrote "driver requests PWM with 5 MHz period" that meant period = 5000000 ns. That was then rounded down to 4266537 ns. And the request for period = 5000000 ns + duty_cycle = 5000000 ns was serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. So that's a 100 % relative duty configuration as intended.
So just from the traces I don't spot a problem. Do these logs not match what actually happens on the signal?
What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() calls each time ?
OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG.
ack. This is done just for the tests implemented in CONFIG_PWM_DEBUG, as are the two pwm_get()s.
But still, the first pwm_apply() requests duty cycle of 5MHz:
5 ms, yes. But it cannot give you 5 ms and so you get 4.266 ns.
systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0
So since the period is 4.26MHz, due to the knobs selected by the provider, this duty cycle will result in a PWM value that is above the selected resolution, as I already mentioned.
"above the selected resolution"? Do you mean you don't get the exact value that you requested?
I think I understand your point now.
You expectation is that the provider would remap the entire range of the period to whatever the HW can do.
If I understand you correctly, that's right. For a given hardware there is a set of possible periods P. .apply() should pick max{ p ∈ P | p ≤ state->period }.
And similar for duty_cycle: After choosing a possible period p ∈ P, there is a set D(p) of duty_cycles that the hardware can implement in combination to period p. .apply() should pick max{ d ∈ D(p) | d ≤ state->duty_cycle }.
So in this case, when 5ms is requested as duty cycle from consumer, the provider will select the max value.
Yes.
What the current implementation of the leds-qcom-lpg does is that will expect a duty cycle request of up to 4.26ms. And according to you, even if the consumer requests 5ms, the leds-qcom-lpg driver should write the value of 255 (which is what the selected resolution allows (1 << 8) ) and not compute a higher value.
If the period is 4.26 ms, duty_cycle cannot be bigger than 4.26 ms. So yes, that's what the driver should do.
I think this is wrong though. The fact that the pwm generic framework reports 5ms when it is actually 4.26ms should be considered wrong.
After pwm_apply_might_sleep(mypwm, { .period = 5000000, .duty_cycle = 5000000, .enabled = true }), pwm_get_state() gives you 5000000 and pwm_get_state_hw() gives you 4266537. You could argue that the functions's names and semantic are not optimal. Changing that is hard, see my failed attempt in 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state") + 40a6b9a00930 ("Revert "pwm: Let pwm_get_state() return the last implemented state"")
So I don't see how the PWM framework is wrong here. Depending on what value you want to get, pick pwm_get_state() or pwm_get_state_hw().
For cases where the exact value of the duty cycle matters, this would not even make sense.
What is "this"? pwm_get_state() returning the last requested value? If you're interested in the last requested value, it does make sense.
Correct me if I'm wrong, but the pwm API should behave more like: The consumer should ask for the closest period the HW can actually do and then use that closest period from there on for every duty cycle request.
You can do that today using pwm_round_waveform_might_sleep() (however that needs some glue in the leds-qcom-lpg driver).
And note that most in-kernel users don't care about exactness a lot. So the fire-and-forget approach is fine and it shouldn't be made more complicated for those.
This way, if the consumer initially wants 5ms but the provider can do only 4.26ms instead, at least the consumer would be able to correct its duty cycle requests based on what the HW says it can do.
I agree that the consumer should be able to make an informed choice, and that was my focus when designing the waveform API. But I intend to not force that on (e.g.) the leds-pwm driver if that doesn't care about getting 4.26 ms or 5 ms.
On top of that, the duty cycle in debugfs is also reported as 5000000ns when in fact it is 4266666ns, as the trace shows.
Yes. Consider that a relict from the times when there was no pwm_get_state_hw(). Both values are interesting in different situations. So just telling the real parameters isn't the optimal way forward either.
Something like the patch I showed in https://lore.kernel.org/all/7bcnckef23w6g47ll5l3bktygedrcfvr7fk3qjuq2swtoffh...
And this patchset only adds the info of actual value that the HW is actually doing.
"only"? Yes, that's the intention of that patch. What should it do more?
So basically, the already existing state in this case will represent the "desired" state.
Yes, pwm->state tracks the state that was last passed to pwm_apply_might_sleep() (most of the time).
would make you a bit luckier I guess. Feel free to polish that one a bit (e.g. by checking the return value of pwm_get_state_hw() and acting sensible in reply to it) and send a proper patch. (A Suggested-by for me is enough for such a patch, grab authorship yourself.)
Need to dig a bit further.
But meanwhile, if the first pwm_apply() call goes all the way to the provider, then the duty cycle value, when translated to the actual PWM value that gets written to reg, will overflow.
No it will not. The .duty_cycle value (also 5000000 ns) will reach the lowlevel PWM driver together with .period = 5000000 ns. Both are rounded down to 4266666ns. I see no overflow.
Again, the consumer is being lied to. It expects 5ms and gets 4.26ms instead.
I see what you mean, but I don't agree. The semantic of pwm_apply_might_sleep() is: "Configure the state that is nearest to the passed state" (for some metric that defines "nearest"). The function returning 0 means: The hardware now has this nearest state.
The semantic of pwm_get_state() is approximately: "What state was requested before?" So it will give you .period = 5000000 ns and .duty_cycle = 5000000 ns.
The semantic of pwm_get_state_hs() is: "What state is the hardware in?" So it will give you .period = 4266666 ns and .duty_cycle = 4266666 ns.
So there are no lies, just wrong expectations about the semantic of these functions.
And if you think that pwm_apply_might_sleep() should fail when 5000000 ns is requested and it can only do 4266537 ns: Where should the line drawn that decides between "4977777 ns is still ok" and "4977777 ns is too far from 5000000 ns"?
Imagine a device that is controlled via PWM and needs exact duty cycle values in ms, what would the consumer driver do in this case?
Traditionally it would need some hardware specific extra information. Today it could work out the needed details with the waveform API functions (though this is hard because there are only two supported lowlevel drivers and no helper functions yet).
And to make things worse, when the consumer is asking for duty cycle of 4ms while the period requested is 5ms (which would be 80%), the period the provider will do is actually 4.26ms while the duty cycle would be ~3.41ms, which if the pwm step (reg value) doesn't allow, it will probably result in an actual value that is even further than what the consumer is expecting.
Where does ~3.41 ms come from? (I guess that's 0.8 * 4.26 ms.) Note that if you request .period = 5 ms and .duty_cycle = 4 ms, you get .period = 4.26 ms and the biggest duty_cycle not bigger than 4 ms that is possible with .period = 4.26 ms. So most likely not a 80% relative duty_cycle.
So I'm thinking maybe the pwm should probably even ask the provider for what duty cycle it will provide based on provider's provided period and then decide if the resulting duty cycle is what it really wants.
Look into the waveform functions. The basic building blocks for what you want should be there.
IIRC, this is more in line with what the CCF (common clocks framework) currently does.
It does? There is clk_round_rate() but that is really hard to use because there are virtually no promises in that function. Consider you want a clock to run at 666666 Hz and clk_round_rate(yourclk, 666666) gives you 500000 Hz. What would you do? Even: What is the rate above 666666 Hz that is as good as 500000 Hz for your usecase? Is it 833332 Hz or 888888 Hz? And do you want 666666 Hz or 666666.666666667 Hz and how does that influence your search for the right clkrate? And it has the same problem as the pwm waveform functions: Just because clk_round_rate(yourclk, 666666) returned 500000 200 ms ago, it doesn't mean that if I ask for 666666 now the world didn't change.
Best regards Uwe
On 20.02.2025 11:31 AM, 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 * refclkprediv_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 * refclkpwm_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@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org
Maybe Anjelique would know better, but the computer tells me PMK8550 has a 1*4*(not 15)-bit PWM controller.. I don't know if it's related, but something to keep in mind
Konrad
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,
Is this problem really limited to the high resolution channels?
the implementation loops over each one of these and tries to find the closest (higher) period based on the following formula:
period * refclkprediv_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 * refclkpwm_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).
I think you should be able to write this more succinct.
The important part of the problem description is that the requested period will be rounded down to a possible hardware configuration, while the duty cycle isn't. The calculated PWM_VALUE might therefor exceed the calculated resolution, but the value is only capped to 15 bits for high resolution channels.
Unless I'm misunderstanding Uwe's comment, this is how the API is expected to work (although I was under the impression that we rounded the period up, rather than down...)
Worth pointing out then is that, as there's a finite number of possible periods that the hardware can operate at, you might want to tweak the period given in the Devicetree to best facilitate the expected brightness range.
And the same would go for my choice of NSEC_PER_MSEC for the non-PWM code paths... I can't explain that choice...
Cc: stable@vger.kernel.org # 6.4
v6.4 is when b00d2ed37617 was introduced, so that's a given...
Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.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.
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
This looks correct!
Regards, Bjorn
clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];} else { max = LPG_RESOLUTION_9BIT - 1;
base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24 change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Abel Vesa abel.vesa@linaro.org
linux-stable-mirror@lists.linaro.org
-
Abel Vesa -
Bjorn Andersson -
Konrad Dybcio -
Sebastian Reichel -
Uwe Kleine-König