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

[Joonas KylmÃlÃ]

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