Re: [v7 1/5] drm/panel: add basic DP AUX backlight support
Hi,
On 23-06-2021 00:03, Doug Anderson wrote:
Hi,
On Mon, Jun 21, 2021 at 11:38 AM Sam Ravnborg 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, >info,
brightness);
bl->enabled = true;
return 0;
}
ret = drm_edp_backlight_set_level(bl->aux, >info,
brightness);
} else {
if (bl->enabled) {
+ pr_err("%s: disabling backlight\n", __func__);
drm_edp_backlight_disable(bl->aux, >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,
Re: [v7 1/5] drm/panel: add basic DP AUX backlight support
Hi, On Mon, Jun 21, 2021 at 11:38 AM Sam Ravnborg 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". -Doug
Re: [v7 1/5] drm/panel: add basic DP AUX backlight support
Hi Rajeev,
On Mon, Jun 21, 2021 at 02:08:17PM +0530, rajee...@codeaurora.org wrote:
> Hi Sam,
>
> On 20-06-2021 15:01, Sam Ravnborg wrote:
> > Hi Rajeev
> >
> > On Sat, Jun 19, 2021 at 04:10:26PM +0530, Rajeev Nandan wrote:
> > > Some panels support backlight control over DP AUX channel using
> > > VESA's standard backlight control interface.
> > > Using new DRM eDP backlight helpers, add support to create and
> > > register a backlight for those panels in drm_panel to simplify
> > > the panel drivers.
> > >
> > > The panel driver with access to "struct drm_dp_aux" can create and
> > > register a backlight device using following code snippet in its
> > > probe() function:
> > >
> > > err = drm_panel_dp_aux_backlight(panel, aux);
> > > if (err)
> > > return err;
> >
> > IT very good to have this supported by drm_panel, so we avoid
> > bolierplate in various drivers.
> >
> > >
> > > Then drm_panel will handle backlight_(enable|disable) calls
> > > similar to the case when drm_panel_of_backlight() is used.
> > >
> > > Currently, we are not supporting one feature where the source
> > > device can combine the backlight brightness levels set through
> > > DP AUX and the BL_PWM_DIM eDP connector pin. Since it's not
> > > required for the basic backlight controls, it can be added later.
> > >
> > > Signed-off-by: Rajeev Nandan
> > > Reviewed-by: Douglas Anderson
> > > Reviewed-by: Lyude Paul
> > > ---
> > >
> > > (no changes since v6)
> > >
> > > Changes in v5:
> > > - New
> > >
> > > Changes in v6:
> > > - Fixed ordering of memory allocation (Douglas)
> > > - Updated word wrapping in a comment (Douglas)
> > >
> > > drivers/gpu/drm/drm_panel.c | 108
> > >
> > > include/drm/drm_panel.h | 15 --
> > > 2 files changed, 119 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > index f634371..9e65342 100644
> > > --- a/drivers/gpu/drm/drm_panel.c
> > > +++ b/drivers/gpu/drm/drm_panel.c
> > > @@ -26,12 +26,20 @@
> > > #include
> > >
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > static DEFINE_MUTEX(panel_lock);
> > > static LIST_HEAD(panel_list);
> > >
> > > +struct dp_aux_backlight {
> > > + struct backlight_device *base;
> > > + struct drm_dp_aux *aux;
> > > + struct drm_edp_backlight_info info;
> > > + bool enabled;
> > > +};
> > > +
> > > /**
> > > * DOC: drm panel
> > > *
> > > @@ -342,6 +350,106 @@ int drm_panel_of_backlight(struct drm_panel
> > > *panel)
> > > return 0;
> > > }
> > > EXPORT_SYMBOL(drm_panel_of_backlight);
> > > +
> > > +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);
> > backlight_get_brightness() returns an int, so using u16 seems wrong.
> > But then drm_edp_backlight_enable() uses u16 for level - so I guess it
> > is OK.
> > We use unsigned long, int, u16 for brightness. Looks like something one
> > could look at one day, but today is not that day.
> >
> > > + int ret = 0;
> > > +
> > > + if (brightness > 0) {
> > Use backlight_is_blank(bd) here, as this is really what you test for.
>
> The backlight_get_brightness() used above has the backlight_is_blank() check
> and returns brightness 0 when the backlight_is_blank(bd) is true.
> So, instead of calling backlight_is_blank(bd), we are checking brightness
> value here.
> I took the reference from pwm_backlight_update_status() of the PWM backlight
> driver (drivers/video/backlight/pwm_bl.c)
>
> Yes, we can change this _if_ condition to use backlight_is_blank(bd), as
> this is an inline function, and is more meaningful.
> With this, there would be one change in the behavior of
> _backlight_update_status function in the following case:
>
> - Setting brightness=0 when the backlight is not blank:
> In the current case setting brightness=0 is disabling the backlight.
> In the new case, setting brightness=0 will set the brightness to 0 and will
> do nothing to backlight disable.
>
> I think that should not be a problem?
Reading "ABI/stable/sysfs-class-backlight" does not say anything about
any special bahaviour with brightness == 0. Some panels may not dim back
to dark on brightness == 0, just barely readable. So in such cases doing
something special on the brightness == 0 case is wrong.
I recall that some backlight drivets do not get this so it is easy to
make it wrong. Unless one of the backlight people tell otherwise the
safe choice is to avoid the specail handling of brightness here.
>
> >
> > 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
Re: [v7 1/5] drm/panel: add basic DP AUX backlight support
Hi Sam,
On 20-06-2021 15:01, Sam Ravnborg wrote:
Hi Rajeev
On Sat, Jun 19, 2021 at 04:10:26PM +0530, Rajeev Nandan wrote:
Some panels support backlight control over DP AUX channel using
VESA's standard backlight control interface.
Using new DRM eDP backlight helpers, add support to create and
register a backlight for those panels in drm_panel to simplify
the panel drivers.
The panel driver with access to "struct drm_dp_aux" can create and
register a backlight device using following code snippet in its
probe() function:
err = drm_panel_dp_aux_backlight(panel, aux);
if (err)
return err;
IT very good to have this supported by drm_panel, so we avoid
bolierplate in various drivers.
Then drm_panel will handle backlight_(enable|disable) calls
similar to the case when drm_panel_of_backlight() is used.
Currently, we are not supporting one feature where the source
device can combine the backlight brightness levels set through
DP AUX and the BL_PWM_DIM eDP connector pin. Since it's not
required for the basic backlight controls, it can be added later.
Signed-off-by: Rajeev Nandan
Reviewed-by: Douglas Anderson
Reviewed-by: Lyude Paul
---
(no changes since v6)
Changes in v5:
- New
Changes in v6:
- Fixed ordering of memory allocation (Douglas)
- Updated word wrapping in a comment (Douglas)
drivers/gpu/drm/drm_panel.c | 108
include/drm/drm_panel.h | 15 --
2 files changed, 119 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371..9e65342 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -26,12 +26,20 @@
#include
#include
+#include
#include
#include
static DEFINE_MUTEX(panel_lock);
static LIST_HEAD(panel_list);
+struct dp_aux_backlight {
+ struct backlight_device *base;
+ struct drm_dp_aux *aux;
+ struct drm_edp_backlight_info info;
+ bool enabled;
+};
+
/**
* DOC: drm panel
*
@@ -342,6 +350,106 @@ int drm_panel_of_backlight(struct drm_panel
*panel)
return 0;
}
EXPORT_SYMBOL(drm_panel_of_backlight);
+
+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);
backlight_get_brightness() returns an int, so using u16 seems wrong.
But then drm_edp_backlight_enable() uses u16 for level - so I guess it
is OK.
We use unsigned long, int, u16 for brightness. Looks like something one
could look at one day, but today is not that day.
+ int ret = 0;
+
+ if (brightness > 0) {
Use backlight_is_blank(bd) here, as this is really what you test for.
The backlight_get_brightness() used above has the backlight_is_blank()
check and returns brightness 0 when the backlight_is_blank(bd) is true.
So, instead of calling backlight_is_blank(bd), we are checking
brightness value here.
I took the reference from pwm_backlight_update_status() of the PWM
backlight driver (drivers/video/backlight/pwm_bl.c)
Yes, we can change this _if_ condition to use backlight_is_blank(bd), as
this is an inline function, and is more meaningful.
With this, there would be one change in the behavior of
_backlight_update_status function in the following case:
- Setting brightness=0 when the backlight is not blank:
In the current case setting brightness=0 is disabling the backlight.
In the new case, setting brightness=0 will set the brightness to 0 and
will do nothing to backlight disable.
I think that should not be a problem?
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.
+ if (!bl->enabled) {
+ drm_edp_backlight_enable(bl->aux, >info,
brightness);
+ bl->enabled = true;
+ return 0;
+ }
+ ret = drm_edp_backlight_set_level(bl->aux, >info,
brightness);
+ } else {
+ if (bl->enabled) {
+ drm_edp_backlight_disable(bl->aux, >info);
+ bl->enabled = false;
+ }
+ }
+
+ return ret;
+}
Sam
Thanks,
Rajeev
Re: [v7 1/5] drm/panel: add basic DP AUX backlight support
Hi Rajeev
On Sat, Jun 19, 2021 at 04:10:26PM +0530, Rajeev Nandan wrote:
> Some panels support backlight control over DP AUX channel using
> VESA's standard backlight control interface.
> Using new DRM eDP backlight helpers, add support to create and
> register a backlight for those panels in drm_panel to simplify
> the panel drivers.
>
> The panel driver with access to "struct drm_dp_aux" can create and
> register a backlight device using following code snippet in its
> probe() function:
>
> err = drm_panel_dp_aux_backlight(panel, aux);
> if (err)
> return err;
IT very good to have this supported by drm_panel, so we avoid
bolierplate in various drivers.
>
> Then drm_panel will handle backlight_(enable|disable) calls
> similar to the case when drm_panel_of_backlight() is used.
>
> Currently, we are not supporting one feature where the source
> device can combine the backlight brightness levels set through
> DP AUX and the BL_PWM_DIM eDP connector pin. Since it's not
> required for the basic backlight controls, it can be added later.
>
> Signed-off-by: Rajeev Nandan
> Reviewed-by: Douglas Anderson
> Reviewed-by: Lyude Paul
> ---
>
> (no changes since v6)
>
> Changes in v5:
> - New
>
> Changes in v6:
> - Fixed ordering of memory allocation (Douglas)
> - Updated word wrapping in a comment (Douglas)
>
> drivers/gpu/drm/drm_panel.c | 108
>
> include/drm/drm_panel.h | 15 --
> 2 files changed, 119 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f634371..9e65342 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -26,12 +26,20 @@
> #include
>
> #include
> +#include
> #include
> #include
>
> static DEFINE_MUTEX(panel_lock);
> static LIST_HEAD(panel_list);
>
> +struct dp_aux_backlight {
> + struct backlight_device *base;
> + struct drm_dp_aux *aux;
> + struct drm_edp_backlight_info info;
> + bool enabled;
> +};
> +
> /**
> * DOC: drm panel
> *
> @@ -342,6 +350,106 @@ int drm_panel_of_backlight(struct drm_panel *panel)
> return 0;
> }
> EXPORT_SYMBOL(drm_panel_of_backlight);
> +
> +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);
backlight_get_brightness() returns an int, so using u16 seems wrong.
But then drm_edp_backlight_enable() uses u16 for level - so I guess it
is OK.
We use unsigned long, int, u16 for brightness. Looks like something one
could look at one day, but today is not that day.
> + int ret = 0;
> +
> + if (brightness > 0) {
Use backlight_is_blank(bd) here, as this is really what you test for.
I cannot see why you need the extra check on ->enabled?
Would it be sufficient to check backlight_is_blank() only?
> + if (!bl->enabled) {
> + drm_edp_backlight_enable(bl->aux, >info,
> brightness);
> + bl->enabled = true;
> + return 0;
> + }
> + ret = drm_edp_backlight_set_level(bl->aux, >info,
> brightness);
> + } else {
> + if (bl->enabled) {
> + drm_edp_backlight_disable(bl->aux, >info);
> + bl->enabled = false;
> + }
> + }
> +
> + return ret;
> +}
Sam
[v7 1/5] drm/panel: add basic DP AUX backlight support
Some panels support backlight control over DP AUX channel using
VESA's standard backlight control interface.
Using new DRM eDP backlight helpers, add support to create and
register a backlight for those panels in drm_panel to simplify
the panel drivers.
The panel driver with access to "struct drm_dp_aux" can create and
register a backlight device using following code snippet in its
probe() function:
err = drm_panel_dp_aux_backlight(panel, aux);
if (err)
return err;
Then drm_panel will handle backlight_(enable|disable) calls
similar to the case when drm_panel_of_backlight() is used.
Currently, we are not supporting one feature where the source
device can combine the backlight brightness levels set through
DP AUX and the BL_PWM_DIM eDP connector pin. Since it's not
required for the basic backlight controls, it can be added later.
Signed-off-by: Rajeev Nandan
Reviewed-by: Douglas Anderson
Reviewed-by: Lyude Paul
---
(no changes since v6)
Changes in v5:
- New
Changes in v6:
- Fixed ordering of memory allocation (Douglas)
- Updated word wrapping in a comment (Douglas)
drivers/gpu/drm/drm_panel.c | 108
include/drm/drm_panel.h | 15 --
2 files changed, 119 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371..9e65342 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -26,12 +26,20 @@
#include
#include
+#include
#include
#include
static DEFINE_MUTEX(panel_lock);
static LIST_HEAD(panel_list);
+struct dp_aux_backlight {
+ struct backlight_device *base;
+ struct drm_dp_aux *aux;
+ struct drm_edp_backlight_info info;
+ bool enabled;
+};
+
/**
* DOC: drm panel
*
@@ -342,6 +350,106 @@ int drm_panel_of_backlight(struct drm_panel *panel)
return 0;
}
EXPORT_SYMBOL(drm_panel_of_backlight);
+
+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;
+
+ if (brightness > 0) {
+ if (!bl->enabled) {
+ drm_edp_backlight_enable(bl->aux, >info,
brightness);
+ bl->enabled = true;
+ return 0;
+ }
+ ret = drm_edp_backlight_set_level(bl->aux, >info,
brightness);
+ } else {
+ if (bl->enabled) {
+ drm_edp_backlight_disable(bl->aux, >info);
+ bl->enabled = false;
+ }
+ }
+
+ return ret;
+}
+
+static const struct backlight_ops dp_aux_bl_ops = {
+ .update_status = dp_aux_backlight_update_status,
+};
+
+/**
+ * drm_panel_dp_aux_backlight - create and use DP AUX backlight
+ * @panel: DRM panel
+ * @aux: The DP AUX channel to use
+ *
+ * Use this function to create and handle backlight if your panel
+ * supports backlight control over DP AUX channel using DPCD
+ * registers as per VESA's standard backlight control interface.
+ *
+ * When the panel is enabled backlight will be enabled after a
+ * successful call to _panel_funcs.enable()
+ *
+ * When the panel is disabled backlight will be disabled before the
+ * call to _panel_funcs.disable().
+ *
+ * A typical implementation for a panel driver supporting backlight
+ * control over DP AUX will call this function at probe time.
+ * Backlight will then be handled transparently without requiring
+ * any intervention from the driver.
+ *
+ * drm_panel_dp_aux_backlight() must be called after the call to
drm_panel_init().
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
+{
+ struct dp_aux_backlight *bl;
+ struct backlight_properties props = { 0 };
+ u16 current_level;
+ u8 current_mode;
+ u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
+ int ret;
+
+ if (!panel || !panel->dev || !aux)
+ return -EINVAL;
+
+ ret = drm_dp_dpcd_read(aux, DP_EDP_DPCD_REV, edp_dpcd,
+ EDP_DISPLAY_CTL_CAP_SIZE);
+ if (ret < 0)
+ return ret;
+
+ if (!drm_edp_backlight_supported(edp_dpcd)) {
+ DRM_DEV_INFO(panel->dev, "DP AUX backlight is not supported\n");
+ return 0;
+ }
+
+ bl = devm_kzalloc(panel->dev, sizeof(*bl), GFP_KERNEL);
+ if (!bl)
+ return -ENOMEM;
+
+ bl->aux = aux;
+
+ ret = drm_edp_backlight_init(aux, >info, 0, edp_dpcd,
+_level, _mode);
+ if (ret < 0)
+ return ret;
+
+ props.type = BACKLIGHT_RAW;
+ props.brightness = current_level;
+ props.max_brightness = bl->info.max;
+
+ bl->base = devm_backlight_device_register(panel->dev,
"dp_aux_backlight",
+
