| Message ID | 20190921124843.6967-1-joonas.kylmala@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: samsung: s6e8aa0: Add backlight control support | expand |
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ä <joonas.kylmala@iki.fi> > --- > 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 <chulspro.kim@samsung.com> > * Tomasz Figa <t.figa@samsung.com> > * Andrzej Hajda <a.hajda@samsung.com> > + * > + * Derived from panel-samsung-s6e63m0.c: > + * Copyright (C) 2019 Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> > */ > > +#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ä <joonas.kylmala@iki.fi> 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
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
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 <chulspro.kim@samsung.com> * Tomasz Figa <t.figa@samsung.com> * Andrzej Hajda <a.hajda@samsung.com> + * + * Derived from panel-samsung-s6e63m0.c: + * Copyright (C) 2019 Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> */ +#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);
This makes the backlight brightness controllable from the userspace. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> --- drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 82 ++++++++++++++++++++------- 1 file changed, 60 insertions(+), 22 deletions(-)