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 <[email protected]>
[...]
+
+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)
