[PATCH 2/2] drm: panel: add support for Samsung S6E8AA5X01 panel controller

[Thomas Zimmermann]

Hi

Am 13.06.25 um 13:03 schrieb Kaustabh Chakraborty:
> On 2025-06-13 09:39, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.06.25 um 16:52 schrieb Kaustabh Chakraborty:
>>> Samsung S6E8AA5X01 is an AMOLED MIPI DSI panel controller. Implement
>>> a basic panel driver for such panels.
>>>
>>> The driver also initializes a backlight device, which works by changing
>>> the panel's gamma values and aid brightness levels appropriately, with
>>> the help of look-up tables acquired from downstream kernel sources.
>>>
>>> Signed-off-by: Kaustabh Chakraborty <kauschluss at disroot.org>
> [...]
>
>>> +
>>> +static void s6e8aa5x01_mcs_protect(struct mipi_dsi_multi_context *dsi,
>>> +				   struct s6e8aa5x01_ctx *ctx, bool protect)
>> I found this interface confusing. Rather split it up into .  It also does two different things AFAICT.
>>
>> - The mcs_mutex protects against concurrent access from update_status and enable
> mcs_mutex is meant to prevent any early access protection of the MCS commands.
> Suppose there are two functions, A and B, accessing MCS.
>
> ENTRY: A()
> (access protection disabled)
> ...
>
> ENTRY: B()
> (access protection disabled)
> ...
> (access protection enabled)
> EXIT: B()
>
> [!] cannot access MCS commands here anymore
> (access protection enabled)
> EXIT: A()
>
> And to avoid such errors a mutex is provided.

This mutex protects a lot more than just the access flags. It prevents 
backlight and enable code to concurrently set gamma on the device. Even 
if you move the MCS_ACCESSPROT to enable/disable helpers, you'll likely 
need the mutex around the gamma updates.

But there's maybe an easy fix. See that the panel code already calls the 
backlight helpers in its enable/disable [1][2] functions. They will 
invoke ->update_status with the proper locking. [3] This means that you 
shouldn't program gamma in the ->enable callback. Leave everything in 
->update_status and let your panel helpers deal with it. No need for 
mcs_mutex at all.

[1] 
https://elixir.bootlin.com/linux/v6.15.1/source/drivers/gpu/drm/drm_panel.c#L235
[2] 
https://elixir.bootlin.com/linux/v6.15.1/source/drivers/gpu/drm/drm_panel.c#L275
[3] 
https://elixir.bootlin.com/linux/v6.15.1/source/include/linux/backlight.h#L318

>
>> - MSC_ACCESSPROT enable access to hardware state.
>>
>> Maybe try this:
>>
>> - Move msc_mutex into the callers, so that ->update_status and ->enable acquire and release the lock.
>>
>> - Move MCS_ACCESSPROT into ->enable and ->disable and leave it accessible, if the hardware allows that.
> Yeah this is a good idea, I'll try it.
>
>>> +{
>>> +	if (protect) {
>>> +		mipi_dsi_dcs_write_seq_multi(dsi, MCS_ACCESSPROT, 0xa5, 0xa5);
>>> +		mutex_unlock(&ctx->mcs_mutex);
>>> +	} else {
>>> +		mutex_lock(&ctx->mcs_mutex);
>>> +		mipi_dsi_dcs_write_seq_multi(dsi, MCS_ACCESSPROT, 0x5a, 0x5a);
>>> +	}
>>> +}
>>> +
>>> +static int s6e8aa5x01_update_brightness(struct backlight_device *backlight)#
>> Maybe call this function s6e8aa5x01_update_status() to match the callback.
>>
>>> +{
>>> +	struct mipi_dsi_multi_context dsi = { .dsi = bl_get_data(backlight) };
>>> +	struct s6e8aa5x01_ctx *ctx = mipi_dsi_get_drvdata(dsi.dsi);
>>> +	u16 lvl = backlight->props.brightness;
>> backlight_get_brightness() here ?
>>
>>
>> I think you should also check panel->enabled and return if false. AFAIU there will be no gamma changes on disabled hardware anyway.
>>
> The enable function is never executed when the panel is disabled. This is
> because flag checking is done by drm_panel anyway. See drm_panel_enable()
> in drivers/gpu/drm/drm_panel.c [1]

What I mean is: the drm_panel.enabled flag is set at [4] and cleared at 
[5]. It tells you that the panel is running. If someone tries to update 
the backlight brightness while the panel is not enabled, you likely what 
to return here without touching hardware.

[4] 
https://elixir.bootlin.com/linux/v6.15.1/source/drivers/gpu/drm/drm_panel.c#L285
[5] 
https://elixir.bootlin.com/linux/v6.15.1/source/drivers/gpu/drm/drm_panel.c#L233

>
>>> +
>>> +static int s6e8aa5x01_probe(struct mipi_dsi_device *dsi)
>>> +{
>>> +	struct device *dev = &dsi->dev;
>>> +	struct s6e8aa5x01_ctx *ctx;
>>> +	int ret;
>>> +
>>> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> You're possibly using the instance after the hardware device has been removed. Alloc with drmm_kzalloc() or you might end up with UAF errors.
> Hmm, none of the panel drivers are using drmm_kzalloc(), or even any
> drmm_*(). Are you sure I must use it?

Then leave it as it is. Maybe one of the panel maintainers can confirm.

I still don't trust it to not possibly blow up. devm_ is released when 
the hardware device goes away.

Best regards
Thomas

>
>>> +	ret = devm_mutex_init(dev, &ctx->mcs_mutex);
>> You're taking this mutex in DRM code, so rather use drmm_mutex_init() here.
> (The comment by me above applies here too)
>
>> Best regards
>> Thomas
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/gpu/drm/drm_panel.c#n209

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)