This makes the backlight brightness controllable from the
userspace.
Signed-off-by: Joonas Kylmälä <[email protected]>
---
drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 82 ++++++++++++++++++++-------
1 file changed, 60 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
index dbced6501204..aa75934f5bed 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
@@ -10,8 +10,12 @@
* Eunchul Kim <[email protected]>
* Tomasz Figa <[email protected]>
* Andrzej Hajda <[email protected]>
+ *
+ * Derived from panel-samsung-s6e63m0.c:
+ * Copyright (C) 2019 Paweł Chmiel <[email protected]>
*/
+#include <linux/backlight.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/module.h>
@@ -85,6 +89,8 @@
#define AID_2 (0x6)
#define AID_3 (0x7)
+#define MAX_BRIGHTNESS (GAMMA_LEVEL_NUM - 1)
+
typedef u8 s6e8aa0_gamma_table[GAMMA_TABLE_LEN];
struct s6e8aa0_variant {
@@ -95,6 +101,7 @@ struct s6e8aa0_variant {
struct s6e8aa0 {
struct device *dev;
struct drm_panel panel;
+ struct backlight_device *bl_dev;
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
@@ -110,7 +117,6 @@ struct s6e8aa0 {
u8 version;
u8 id;
const struct s6e8aa0_variant *variant;
- int brightness;
/* This field is tested by functions directly accessing DSI bus before
* transfer, transfer is skipped if it is set. In case of transfer
@@ -321,9 +327,10 @@ static void s6e8aa0_etc_elvss_control(struct s6e8aa0 *ctx)
static void s6e8aa0_elvss_nvm_set_v142(struct s6e8aa0 *ctx)
{
+ struct backlight_device *bd = ctx->bl_dev;
u8 br;
- switch (ctx->brightness) {
+ switch (bd->props.brightness) {
case 0 ... 6: /* 30cd ~ 100cd */
br = 0xdf;
break;
@@ -762,24 +769,6 @@ static const struct s6e8aa0_variant s6e8aa0_variants[] = {
}
};
-static void s6e8aa0_brightness_set(struct s6e8aa0 *ctx)
-{
- const u8 *gamma;
-
- if (ctx->error)
- return;
-
- gamma = ctx->variant->gamma_tables[ctx->brightness];
-
- if (ctx->version >= 142)
- s6e8aa0_elvss_nvm_set(ctx);
-
- s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
-
- /* update gamma table. */
- s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
-}
-
static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
{
s6e8aa0_apply_level_1_key(ctx);
@@ -791,7 +780,7 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
s6e8aa0_panel_cond_set(ctx);
s6e8aa0_display_condition_set(ctx);
- s6e8aa0_brightness_set(ctx);
+ backlight_enable(ctx->bl_dev);
s6e8aa0_etc_source_control(ctx);
s6e8aa0_etc_pentile_control(ctx);
s6e8aa0_elvss_nvm_set(ctx);
@@ -974,6 +963,53 @@ static int s6e8aa0_parse_dt(struct s6e8aa0 *ctx)
return 0;
}
+static int s6e8aa0_set_brightness(struct backlight_device *bd)
+{
+ struct s6e8aa0 *ctx = bl_get_data(bd);
+ const u8 *gamma;
+
+ if (ctx->error)
+ return;
+
+ gamma = ctx->variant->gamma_tables[bd->props.brightness];
+
+ if (ctx->version >= 142)
+ s6e8aa0_elvss_nvm_set(ctx);
+
+ s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
+
+ /* update gamma table. */
+ s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
+
+ return s6e8aa0_clear_error(ctx);
+}
+
+static const struct backlight_ops s6e8aa0_backlight_ops = {
+ .update_status = s6e8aa0_set_brightness,
+};
+
+static int s6e8aa0_backlight_register(struct s6e8aa0 *ctx)
+{
+ struct backlight_properties props = {
+ .type = BACKLIGHT_RAW,
+ .brightness = MAX_BRIGHTNESS,
+ .max_brightness = MAX_BRIGHTNESS
+ };
+ struct device *dev = ctx->dev;
+ int ret = 0;
+
+ ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
+ &s6e8aa0_backlight_ops,
+ &props);
+ if (IS_ERR(ctx->bl_dev)) {
+ ret = PTR_ERR(ctx->bl_dev);
+ DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
+ ret);
+ }
+
+ return ret;
+}
+
static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
{
struct device *dev = &dsi->dev;
@@ -1015,7 +1051,9 @@ static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
return PTR_ERR(ctx->reset_gpio);
}
- ctx->brightness = GAMMA_LEVEL_NUM - 1;
+ ret = s6e8aa0_backlight_register(ctx);
+ if (ret < 0)
+ return ret;
drm_panel_init(&ctx->panel, dev, &s6e8aa0_drm_funcs,
DRM_MODE_CONNECTOR_DSI);
--
2.11.0
Hi Joonas,
On 21.09.2019 14:48, Joonas Kylmälä wrote:
> This makes the backlight brightness controllable from the
> userspace.
>
> Signed-off-by: Joonas Kylmälä <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 82 ++++++++++++++++++++-------
> 1 file changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> index dbced6501204..aa75934f5bed 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> @@ -10,8 +10,12 @@
> * Eunchul Kim <[email protected]>
> * Tomasz Figa <[email protected]>
> * Andrzej Hajda <[email protected]>
> + *
> + * Derived from panel-samsung-s6e63m0.c:
> + * Copyright (C) 2019 Paweł Chmiel <[email protected]>
> */
>
> +#include <linux/backlight.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/module.h>
> @@ -85,6 +89,8 @@
> #define AID_2 (0x6)
> #define AID_3 (0x7)
>
> +#define MAX_BRIGHTNESS (GAMMA_LEVEL_NUM - 1)
> +
> typedef u8 s6e8aa0_gamma_table[GAMMA_TABLE_LEN];
>
> struct s6e8aa0_variant {
> @@ -95,6 +101,7 @@ struct s6e8aa0_variant {
> struct s6e8aa0 {
> struct device *dev;
> struct drm_panel panel;
> + struct backlight_device *bl_dev;
>
> struct regulator_bulk_data supplies[2];
> struct gpio_desc *reset_gpio;
> @@ -110,7 +117,6 @@ struct s6e8aa0 {
> u8 version;
> u8 id;
> const struct s6e8aa0_variant *variant;
> - int brightness;
>
> /* This field is tested by functions directly accessing DSI bus before
> * transfer, transfer is skipped if it is set. In case of transfer
> @@ -321,9 +327,10 @@ static void s6e8aa0_etc_elvss_control(struct s6e8aa0 *ctx)
>
> static void s6e8aa0_elvss_nvm_set_v142(struct s6e8aa0 *ctx)
> {
> + struct backlight_device *bd = ctx->bl_dev;
> u8 br;
>
> - switch (ctx->brightness) {
> + switch (bd->props.brightness) {
> case 0 ... 6: /* 30cd ~ 100cd */
> br = 0xdf;
> break;
> @@ -762,24 +769,6 @@ static const struct s6e8aa0_variant s6e8aa0_variants[] = {
> }
> };
>
> -static void s6e8aa0_brightness_set(struct s6e8aa0 *ctx)
> -{
> - const u8 *gamma;
> -
> - if (ctx->error)
> - return;
> -
> - gamma = ctx->variant->gamma_tables[ctx->brightness];
> -
> - if (ctx->version >= 142)
> - s6e8aa0_elvss_nvm_set(ctx);
> -
> - s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> -
> - /* update gamma table. */
> - s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> -}
> -
> static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
> {
> s6e8aa0_apply_level_1_key(ctx);
> @@ -791,7 +780,7 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
>
> s6e8aa0_panel_cond_set(ctx);
> s6e8aa0_display_condition_set(ctx);
> - s6e8aa0_brightness_set(ctx);
> + backlight_enable(ctx->bl_dev);
> s6e8aa0_etc_source_control(ctx);
> s6e8aa0_etc_pentile_control(ctx);
> s6e8aa0_elvss_nvm_set(ctx);
> @@ -974,6 +963,53 @@ static int s6e8aa0_parse_dt(struct s6e8aa0 *ctx)
> return 0;
> }
>
> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
> +{
> + struct s6e8aa0 *ctx = bl_get_data(bd);
> + const u8 *gamma;
> +
> + if (ctx->error)
> + return;
> +
> + gamma = ctx->variant->gamma_tables[bd->props.brightness];
> +
> + if (ctx->version >= 142)
> + s6e8aa0_elvss_nvm_set(ctx);
> +
> + s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> +
> + /* update gamma table. */
> + s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> +
> + return s6e8aa0_clear_error(ctx);
> +}
> +
> +static const struct backlight_ops s6e8aa0_backlight_ops = {
> + .update_status = s6e8aa0_set_brightness,
This is racy, update_status can be called in any time between probe and
remove, particularly:
a) before panel enable,
b) during panel enable,
c) when panel is enabled,
d) during panel disable,
e) after panel disable,
b and d are racy for sure - backlight and drm callbacks are async.
IMO the best solution would be to register backlight after attaching
panel to drm, but for this drm_panel_funcs should have attach/detach
callbacks (like drm_bridge_funcs),
then update_status callback should take some drm_connector lock to
synchronize with drm, and write to hw only when pipe is enabled.
Regards
Andrzej
> +};
> +
> +static int s6e8aa0_backlight_register(struct s6e8aa0 *ctx)
> +{
> + struct backlight_properties props = {
> + .type = BACKLIGHT_RAW,
> + .brightness = MAX_BRIGHTNESS,
> + .max_brightness = MAX_BRIGHTNESS
> + };
> + struct device *dev = ctx->dev;
> + int ret = 0;
> +
> + ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
> + &s6e8aa0_backlight_ops,
> + &props);
> + if (IS_ERR(ctx->bl_dev)) {
> + ret = PTR_ERR(ctx->bl_dev);
> + DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
> + ret);
> + }
> +
> + return ret;
> +}
> +
> static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
> {
> struct device *dev = &dsi->dev;
> @@ -1015,7 +1051,9 @@ static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
> return PTR_ERR(ctx->reset_gpio);
> }
>
> - ctx->brightness = GAMMA_LEVEL_NUM - 1;
> + ret = s6e8aa0_backlight_register(ctx);
> + if (ret < 0)
> + return ret;
>
> drm_panel_init(&ctx->panel, dev, &s6e8aa0_drm_funcs,
> DRM_MODE_CONNECTOR_DSI);
Hi,
thanks a lot for the review, Andrzej!
Andrzej Hajda:
>> +static const struct backlight_ops s6e8aa0_backlight_ops = {
>> + .update_status = s6e8aa0_set_brightness,
>
>
> This is racy, update_status can be called in any time between probe and
> remove, particularly:
>
> a) before panel enable,
>
> b) during panel enable,
>
> c) when panel is enabled,
>
> d) during panel disable,
>
> e) after panel disable,
>
>
> b and d are racy for sure - backlight and drm callbacks are async.
>
> IMO the best solution would be to register backlight after attaching
> panel to drm, but for this drm_panel_funcs should have attach/detach
> callbacks (like drm_bridge_funcs),
>
> then update_status callback should take some drm_connector lock to
> synchronize with drm, and write to hw only when pipe is enabled.
I will start reading the kernel DRM KMS documentation in order to learn
how the attach callback would fit in the picture and do what you suggest
but it might take a few weeks or months.
Also, as a lot of this code was mimicked from
drivers/gpu/drm/panel/panel-samsung-s6e63m0.c it looks like that is also
having this race condition. Unfortunately, I don't think I have the
s6e63m0 panel so for that I probably cannot fix this issue.
Joonas
Hi,
addressing this email to you all since there might be widespread race
condition issue in the DRM panel drivers that are using MIPI DSI. See
below for my message.
Andrzej Hajda:
>> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
>> +{
>> + struct s6e8aa0 *ctx = bl_get_data(bd);
>> + const u8 *gamma;
>> +
>> + if (ctx->error)
>> + return;
>> +
>> + gamma = ctx->variant->gamma_tables[bd->props.brightness];
>> +
>> + if (ctx->version >= 142)
>> + s6e8aa0_elvss_nvm_set(ctx);
>> +
>> + s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
>> +
>> + /* update gamma table. */
>> + s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
>> +
>> + return s6e8aa0_clear_error(ctx);
>> +}
>> +
>> +static const struct backlight_ops s6e8aa0_backlight_ops = {
>> + .update_status = s6e8aa0_set_brightness,
>
>
> This is racy, update_status can be called in any time between probe and
> remove, particularly:
>
> a) before panel enable,
>
> b) during panel enable,
>
> c) when panel is enabled,
>
> d) during panel disable,
>
> e) after panel disable,
>
>
> b and d are racy for sure - backlight and drm callbacks are async.
>
> IMO the best solution would be to register backlight after attaching
> panel to drm, but for this drm_panel_funcs should have attach/detach
> callbacks (like drm_bridge_funcs),
>
> then update_status callback should take some drm_connector lock to
> synchronize with drm, and write to hw only when pipe is enabled.
I have done now research and if I understand right one issue here might
be with setting the backlight brightness if the DSI device is not
attached before calling update_status() since calling it would call
subsequently s6e8aa0_set_brightness() -> s6e8aa0_dcs_write() ->
mipi_dsi_dcs_write_buffer(), which then requires DSI to be attached.
But now to the part that affects many of the panel drivers using MIPI
DSI, like panel-samsung-s6e63j0x03.c, panel-simple.c, etc.:
mipi_dsi_attach(dsi) seems to be always called only after the DRM panel
helper drm_panel_add(). Now I think this is problematic since
drm_panel_add() makes the panel available for use in userspace but if
the user tries to actually do something with the panel before
mipi_dsi_attach(dsi) is called it would not work.
So for some reason the mipi_dsi_attach() is called in all those drivers
after drm_panel_add() and at least to the problem I pointed out above
moving the call there before drm_panel_add() would fix the issue but
then I don't know if it causes some other issue.
Also I don't know if Andrzej had some other issues in mind that could be
caused by this race condition, so if there are multiple instead of just
that one issue with DSI not being attached then we might want to have
all these issues fixed by for example the solution Andrzej proposed
where we have attach/detach callbacks in drm_bridge_funcs.
Please let me know if anything I write above doesn't make sense, I'm
still trying to understand the DRM subsystem better.
Joonas
On Sat, Apr 4, 2020 at 3:27 PM Joonas Kylmälä <[email protected]> wrote:
>
> Hi,
>
> addressing this email to you all since there might be widespread race
> condition issue in the DRM panel drivers that are using MIPI DSI. See
> below for my message.
>
> Andrzej Hajda:
> >> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
> >> +{
> >> + struct s6e8aa0 *ctx = bl_get_data(bd);
> >> + const u8 *gamma;
> >> +
> >> + if (ctx->error)
> >> + return;
> >> +
> >> + gamma = ctx->variant->gamma_tables[bd->props.brightness];
> >> +
> >> + if (ctx->version >= 142)
> >> + s6e8aa0_elvss_nvm_set(ctx);
> >> +
> >> + s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> >> +
> >> + /* update gamma table. */
> >> + s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> >> +
> >> + return s6e8aa0_clear_error(ctx);
> >> +}
> >> +
> >> +static const struct backlight_ops s6e8aa0_backlight_ops = {
> >> + .update_status = s6e8aa0_set_brightness,
> >
> >
> > This is racy, update_status can be called in any time between probe and
> > remove, particularly:
> >
> > a) before panel enable,
> >
> > b) during panel enable,
> >
> > c) when panel is enabled,
> >
> > d) during panel disable,
> >
> > e) after panel disable,
> >
> >
> > b and d are racy for sure - backlight and drm callbacks are async.
> >
> > IMO the best solution would be to register backlight after attaching
> > panel to drm, but for this drm_panel_funcs should have attach/detach
> > callbacks (like drm_bridge_funcs),
> >
> > then update_status callback should take some drm_connector lock to
> > synchronize with drm, and write to hw only when pipe is enabled.
>
> I have done now research and if I understand right one issue here might
> be with setting the backlight brightness if the DSI device is not
> attached before calling update_status() since calling it would call
> subsequently s6e8aa0_set_brightness() -> s6e8aa0_dcs_write() ->
> mipi_dsi_dcs_write_buffer(), which then requires DSI to be attached.
>
> But now to the part that affects many of the panel drivers using MIPI
> DSI, like panel-samsung-s6e63j0x03.c, panel-simple.c, etc.:
> mipi_dsi_attach(dsi) seems to be always called only after the DRM panel
> helper drm_panel_add(). Now I think this is problematic since
> drm_panel_add() makes the panel available for use in userspace but if
> the user tries to actually do something with the panel before
> mipi_dsi_attach(dsi) is called it would not work.
>
> So for some reason the mipi_dsi_attach() is called in all those drivers
> after drm_panel_add() and at least to the problem I pointed out above
> moving the call there before drm_panel_add() would fix the issue but
> then I don't know if it causes some other issue.
Nope, drm_panel_add only makes the panel visible to other drm drivers,
not yet to userspace. That only happens once the overall drm driver
calls drm_dev_register. At that point the mipi_dsi_attach should have
happened I think.
That in turn calls a drm_connector_funcs->late_register hook, which is
meant to be use to register stuff like backlight interfaces. If you
set up your backlight before that, yes I expect some good fireworks.
Now as you noticed, we're not wiring that through to panels, so maybe
the best solution would be if drm_panel gets a backlight pointer with
an initialized, but not yet registered backlight. And then we can
drive this in the bridge or connector wrapper for panels.
Or I'm totally not understanding the issue even, which is also possible :-)
Cheers, Daniel
> Also I don't know if Andrzej had some other issues in mind that could be
> caused by this race condition, so if there are multiple instead of just
> that one issue with DSI not being attached then we might want to have
> all these issues fixed by for example the solution Andrzej proposed
> where we have attach/detach callbacks in drm_bridge_funcs.
>
> Please let me know if anything I write above doesn't make sense, I'm
> still trying to understand the DRM subsystem better.
>
> Joonas
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Joonas.
On Sat, Apr 04, 2020 at 04:27:02PM +0300, Joonas Kylm?l? wrote:
> Hi,
>
> addressing this email to you all since there might be widespread race
> condition issue in the DRM panel drivers that are using MIPI DSI. See
> below for my message.
>
> Andrzej Hajda:
> >> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
> >> +{
> >> + struct s6e8aa0 *ctx = bl_get_data(bd);
> >> + const u8 *gamma;
> >> +
> >> + if (ctx->error)
> >> + return;
> >> +
> >> + gamma = ctx->variant->gamma_tables[bd->props.brightness];
> >> +
> >> + if (ctx->version >= 142)
> >> + s6e8aa0_elvss_nvm_set(ctx);
> >> +
> >> + s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> >> +
> >> + /* update gamma table. */
> >> + s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> >> +
> >> + return s6e8aa0_clear_error(ctx);
> >> +}
> >> +
> >> +static const struct backlight_ops s6e8aa0_backlight_ops = {
> >> + .update_status = s6e8aa0_set_brightness,
> >
> >
> > This is racy, update_status can be called in any time between probe and
> > remove, particularly:
> >
> > a) before panel enable,
> >
> > b) during panel enable,
> >
> > c) when panel is enabled,
> >
> > d) during panel disable,
> >
> > e) after panel disable,
> >
> >
> > b and d are racy for sure - backlight and drm callbacks are async.
> >
> > IMO the best solution would be to register backlight after attaching
> > panel to drm, but for this drm_panel_funcs should have attach/detach
> > callbacks (like drm_bridge_funcs),
> >
> > then update_status callback should take some drm_connector lock to
> > synchronize with drm, and write to hw only when pipe is enabled.
>
> I have done now research and if I understand right one issue here might
> be with setting the backlight brightness if the DSI device is not
> attached before calling update_status() since calling it would call
> subsequently s6e8aa0_set_brightness() -> s6e8aa0_dcs_write() ->
> mipi_dsi_dcs_write_buffer(), which then requires DSI to be attached.
Not directly related to your comments above.
But I have looked at the backlight support for the various
samsung panels.
None of them are good examples to follow.
Please have a look at for example panel-novatek-nt35510.c
which is a good example how to have a local backligth
and tie it into the general way it is used by drm_panel.
I have typed patches to fix all three samsung panels, will
post patches later when I get more time.
If we are concerned with set_brightness() being called
while not ready, this can be checked in the
set_brightness() function and return error if not OK.
As the the race concerns see Daniel's reply.
Sam