Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support

[Sam Ravnborg]

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