On 17. 10. 19 10:10, Uwe Kleine-König wrote:
A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
pwm_get_state() return the last implemented state")) changed the
semantic of pwm_get_state() and disclosed an (as it seems) common
problem in lowlevel PWM drivers. By not relying on the period and duty
cycle being retrievable from a disabled PWM this type of problem is
worked around.
Apart from this issue only calling the pwm_get_state/pwm_apply_state
combo once is also more effective.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
---
Hello,
There are now two reports about 01ccf903edd6 breaking a backlight. As
far as I understand the problem this is a combination of the backend pwm
driver yielding surprising results and the pwm-bl driver doing things
more complicated than necessary.
So I guess this patch works around these problems. Still it would be
interesting to find out the details in the imx driver that triggers the
problem. So Adam, can you please instrument the pwm-imx27 driver to
print *state at the beginning of pwm_imx27_apply() and the end of
pwm_imx27_get_state() and provide the results?
Note I only compile tested this change.
Hi Uwe,
I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+"
thread that I have a similar problem when you submitted this patch.
So here are my few cents:
My setup is as follows:
- imx6dl-yapp4-draco with i.MX6Solo
- backlight is controlled with inverted PWM signal
- max brightness level = 32, default brightness level set to 32 in DT.
1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let
pwm_get_state() return the last implemented state):
- System boots to userspace and backlight is enabled all the time from
power up.
$ dmesg | grep state
[ 1.763381] get state end: -1811360608, enabled: 0
[ 4.939658] apply state start: -1797997308, enabled: 0
[ 4.946437] apply state start: -1797997436, enabled: 0
[ 4.952936] apply state start: -1797997420, enabled: 1
- $ cat brightness
32
- $ echo 32 > brightness # nothing happens, max. brightness
- $ echo 1 > brightness # backlight goes down to lowest level
[ 59.876561] apply state start: -1711276636, enabled: 1
- $ echo 0 > brightness # backlight goes up to max. level, this is
# problem of the inverted PWM on i.MX we attempted
# to solve some time ago.
[ 102.456582] apply state start: -1711276700, enabled: 0
2. Backlight behavior on v5.4-rc3:
- System boots to userspace and backlight is enabled all the time from
power up.
- $ dmesg | grep state
[ 1.773056] get state end: -1811319520, enabled: 0
[ 4.994003] apply state start: -1797948160, enabled: 0
[ 5.000617] get state end: -1811319520, enabled: 0
[ 5.006950] apply state start: -1797948292, enabled: 0
[ 5.013446] get state end: -1811319520, enabled: 0
[ 5.019602] apply state start: -1797948276, enabled: 1
[ 5.027536] get state end: -1811319520, enabled: 1
- $ cat brightness
32
- $ echo 32 > brightness # backlight goes down
[ 189.956756] apply state start: -1711923804, enabled: 1
[ 189.965907] get state end: -1811319520, enabled: 1
- $ echo 1 > brightness # backlight goes up to high level
[ 464.136633] apply state start: -1711923804, enabled: 1
[ 464.145625] get state end: -1811319520, enabled: 1
- $ echo 0 > brightness # backlight goes up to highest level
[ 527.136640] apply state start: -1711923868, enabled: 0
[ 527.145538] get state end: -1811319520, enabled: 0
So the behavior is clearly inverted to how it worked prior to 01ccf903edd6
with the weird exception that the initial brightness level 32 is
not applied.
3. Backlight behavior on v5.4-rc3 + this patch:
- System boots with backlight enabled. In the middle of kernel boot
backlight is disabled.
- $ dmesg | grep state
[ 1.773233] get state end: -1811319520, enabled: 0
[ 5.002753] apply state start: -1797948160, enabled: 0
[ 5.009397] get state end: -1811319520, enabled: 0
[ 5.015652] apply state start: -1797948276, enabled: 1
[ 5.026682] get state end: -1811319520, enabled: 1
- $ cat brightness
32
- $ echo 32 > brightness # nothing happens, backlight is down
- $ echo 1 > brightness # backlight goes to high level
[ 305.377274] apply state start: -1711915596, enabled: 1
[ 305.386374] get state end: -1811319520, enabled: 1
- $ echo 0 > brightness # backlight goes to max brightness
[ 362.637297] apply state start: -1711915596, enabled: 0
[ 362.646325] get state end: -1811319520, enabled: 0
Same behavior as (2) but the default state from DT is apparently applied.
I only did this experiments. I did not delve into the code to track what is
going on in there yet.
Hopefully this helps you a bit and feel free to request other experiments,
Michal
drivers/video/backlight/pwm_bl.c | 34 +++++++++++---------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 746eebc411df..ddebd62b3978 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -42,10 +42,8 @@ struct pwm_bl_data {
static void pwm_backlight_power_on(struct pwm_bl_data *pb)
{
- struct pwm_state state;
int err;
- pwm_get_state(pb->pwm, &state);
if (pb->enabled)
return;
@@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
if (err < 0)
dev_err(pb->dev, "failed to enable power supply\n");
- state.enabled = true;
- pwm_apply_state(pb->pwm, &state);
-
if (pb->post_pwm_on_delay)
msleep(pb->post_pwm_on_delay);
@@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
static void pwm_backlight_power_off(struct pwm_bl_data *pb)
{
- struct pwm_state state;
-
- pwm_get_state(pb->pwm, &state);
- if (!pb->enabled)
- return;
-
if (pb->enable_gpio)
gpiod_set_value_cansleep(pb->enable_gpio, 0);
if (pb->pwm_off_delay)
msleep(pb->pwm_off_delay);
- state.enabled = false;
- state.duty_cycle = 0;
- pwm_apply_state(pb->pwm, &state);
-
regulator_disable(pb->power_supply);
pb->enabled = false;
}
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state)
{
unsigned int lth = pb->lth_brightness;
- struct pwm_state state;
u64 duty_cycle;
- pwm_get_state(pb->pwm, &state);
-
if (pb->levels)
duty_cycle = pb->levels[brightness];
else
duty_cycle = brightness;
- duty_cycle *= state.period - lth;
+ duty_cycle *= state->period - lth;
do_div(duty_cycle, pb->scale);
return duty_cycle + lth;
@@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness > 0) {
pwm_get_state(pb->pwm, &state);
- state.duty_cycle = compute_duty_cycle(pb, brightness);
+ state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
+ state.enabled = true;
pwm_apply_state(pb->pwm, &state);
+
pwm_backlight_power_on(pb);
- } else
+ } else {
pwm_backlight_power_off(pb);
+ pwm_get_state(pb->pwm, &state);
+ state.enabled = false;
+ state.duty_cycle = 0;
+ pwm_apply_state(pb->pwm, &state);
+ }
+
if (pb->notify_after)
pb->notify_after(pb->dev, brightness);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
