Hi,
On 23-06-2021 00:03, Doug Anderson wrote:
Hi,
On Mon, Jun 21, 2021 at 11:38 AM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
> > I cannot see why you need the extra check on ->enabled?
> > Would it be sufficient to check backlight_is_blank() only?
>
> This extra check on bl->enabled flag is added to avoid enabling/disabling
> backlight again if it is already enabled/disabled.
> Using this flag way can know the transition between backlight blank and
> un-blank, and decide when to enable/disable the backlight.
My point is that this should really not be needed, as it would cover
up
for some other bug whaere we try to do something twice that is not
needed. But I am less certain here so if you think it is needed, keep
it as is.
I haven't tested this myself, but I believe that it is needed. I don't
think the backlight update_status() function is like an enable/disable
function. I believe it can be called more than one time even while the
backlight is disabled. For instance, you can see that
backlight_update_status() just blindly calls through to update the
status. That function can be called for a number of reasons. Perhaps
Rajeev can put some printouts to confirm but I think that if the
backlight is "blanked" for whatever reason and you write to sysfs and
change the backlight level you'll still get called again even though
the backlight is still "disabled".
Yes, sysfs write will always try to update the backlight even though the
backlight is "blanked".
The "bl->enabled" check is also required to prevent unnecessary calls to
drm_edp_backlight_enable() during every backlight level change.
To confirm this, I have added few prints in
dp_aux_backlight_update_status() function and collected the logs.
(Copying the code here to make the review easy)
static int dp_aux_backlight_update_status(struct backlight_device *bd)
{
struct dp_aux_backlight *bl = bl_get_data(bd);
u16 brightness = backlight_get_brightness(bd);
int ret = 0;
+ pr_err("%s: brightness %d, _is_blank %d, bl->enabled %d\n",
__func__,
+ brightness, backlight_is_blank(bd), bl->enabled);
if (!backlight_is_blank(bd)) {
if (!bl->enabled) {
+ pr_err("%s: enabling backlight\n", __func__);
drm_edp_backlight_enable(bl->aux, &bl->info,
brightness);
bl->enabled = true;
return 0;
}
ret = drm_edp_backlight_set_level(bl->aux, &bl->info,
brightness);
} else {
if (bl->enabled) {
+ pr_err("%s: disabling backlight\n", __func__);
drm_edp_backlight_disable(bl->aux, &bl->info);
bl->enabled = false;
}
}
return ret;
}
LOGS
====
During boot
-----------
[ 4.752188] dp_aux_backlight_update_status: brightness 102, _is_blank
0, bl->enabled 0
[ 4.760447] dp_aux_backlight_update_status: enabling backlight
[ 5.503866] dp_aux_backlight_update_status: brightness 102, _is_blank
0, bl->enabled 1
[ 6.897355] dp_aux_backlight_update_status: brightness 103, _is_blank
0, bl->enabled 1
[ 6.938617] dp_aux_backlight_update_status: brightness 104, _is_blank
0, bl->enabled 1
[ 6.980634] dp_aux_backlight_update_status: brightness 105, _is_blank
0, bl->enabled 1
Turning Panel OFF
-----------------
localhost ~ # set_power_policy --ac_screen_dim_delay=5
--ac_screen_off_delay=10
localhost ~ #
[ 106.555140] dp_aux_backlight_update_status: brightness 145, _is_blank
0, bl->enabled 1
...
...
[ 111.679407] dp_aux_backlight_update_status: brightness 7, _is_blank
0, bl->enabled 1
[ 111.700302] dp_aux_backlight_update_status: brightness 4, _is_blank
0, bl->enabled 1
[ 111.720805] dp_aux_backlight_update_status: brightness 2, _is_blank
0, bl->enabled 1
[ 111.747486] dp_aux_backlight_update_status: brightness 0, _is_blank
1, bl->enabled 1
[ 111.755580] dp_aux_backlight_update_status: disabling backlight
[ 111.792344] dp_aux_backlight_update_status: brightness 0, _is_blank
1, bl->enabled 0
Changing brightness from sysfs while panel is off
--------------------------------------------------
(it will do nothing)
localhost ~ # echo 100 >
/sys/class/backlight/dp_aux_backlight/brightness
[ 352.754963] dp_aux_backlight_update_status: brightness 0, _is_blank
1, bl->enabled 0
localhost ~ # echo 200 >
/sys/class/backlight/dp_aux_backlight/brightness
[ 364.708048] dp_aux_backlight_update_status: brightness 0, _is_blank
1, bl->enabled 0
localhost ~ # echo 0 > /sys/class/backlight/dp_aux_backlight/brightness
[ 378.850978] dp_aux_backlight_update_status: brightness 0, _is_blank
1, bl->enabled 0
Turning Panel ON
----------------
[ 553.381745] dp_aux_backlight_update_status: brightness 0, _is_blank
0, bl->enabled 0
[ 553.418133] dp_aux_backlight_update_status: enabling backlight
[ 553.426397] dp_aux_backlight_update_status: brightness 159, _is_blank
0, bl->enabled 1
====
Thanks,
Rajeev
