Messages in this thread | | | Date | Sun, 5 Apr 2020 16:27:15 +0200 | From | Sam Ravnborg <> | Subject | Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support |
| |
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
| |