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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
