Re: [PATCH v4 2/4] drm/panel: Add driver for Novatek NT37705 panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/panel: missing error cleanup in nt37705_prepare() leaks regulators
- [High] drm/panel: nt37705: data race and DSI timeout in backlight sysfs
- [Medium] drm/panel: dsi->mode_flags mutated and not restored on error in nt37705_bl_update_status()
- [Medium] drm/panel: invalid payload for 0-parameter DCS commands
- [Medium] drm/panel: nt37705: missing panel.prepared check in backlight update_status
--

commit a437b7eca971b2adefff7488ccaf50028c00b068
Author: Luca Weiss <luca.weiss@fairphone.com>
drm/panel: Add driver for Novatek NT37705 panel

This commit adds support for the 2484x1116 AMOLED panel from BOE
(BJ631JHM-T71-D900) bundled with a NT37705 driver IC, as found on the
Fairphone 6 smartphone.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37705.c b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
> @@ -0,0 +1,396 @@
[ ... ]
> +static int nt37705_on(struct nt37705_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
[ ... ]
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_TEAR_ON);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +				     0x20);
> +	mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0x0000, 0x045b);
> +	mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0x0000, 0x09b3);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_GAMMA_CURVE, 0x00);
> +	mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0xbb0d);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x6f, 0x04);
> +	mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0xfe0f);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x81, 0x01, 0x19);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x03, 0x01);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x90, 0x03, 0x03);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x91,
> +				     0x89, 0x28, 0x00, 0x0c, 0xd2, 0x00, 0x02,
> +				     0x2f, 0x01, 0x18, 0x00, 0x07, 0x09, 0x75,
> +				     0x08, 0x34, 0x10, 0xf0);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x2f, 0x02);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x5a, 0x01);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x2f, 0x30);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x6d, 0x00);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_DISPLAY_ON, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 22);

[Severity: Medium]
The driver issues several MIPI DCS commands that require zero parameters,
such as MIPI_DCS_EXIT_SLEEP_MODE (0x11) and MIPI_DCS_SET_DISPLAY_ON (0x29).
However, passing 0x00 as an argument to mipi_dsi_dcs_write_seq_multi()
creates a 2-byte array ({ cmd, 0x00 }). This triggers a DCS Short Write
with 1 parameter (Data Type 0x15) instead of a 0-parameter write (Data Type
0x05). 

Similarly, earlier in this sequence, MIPI_DCS_SET_TEAR_ON takes 1 parameter 
but is called with no parameters.

Could this cause strict DSI sinks to reject these malformed packets and fail
panel initialization?

> +	return dsi_ctx.accum_err;
> +}
> +
> +static int nt37705_off(struct nt37705_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_DISPLAY_OFF, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 20);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_ENTER_SLEEP_MODE, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 120);

[Severity: Medium]
As mentioned above, MIPI_DCS_SET_DISPLAY_OFF and MIPI_DCS_ENTER_SLEEP_MODE
are also 0-parameter commands being given a 0x00 payload, creating an
incorrect DCS short write length.

> +	return dsi_ctx.accum_err;
> +}
> +
> +static int nt37705_prepare(struct drm_panel *panel)
> +{
> +	struct nt37705_panel *ctx = to_nt37705_panel(panel);
> +	struct device *dev = &ctx->dsi->dev;
> +	struct drm_dsc_picture_parameter_set pps;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(nt37705_supplies), ctx->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	nt37705_reset(ctx);
> +
> +	ret = nt37705_on(ctx);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(nt37705_supplies), ctx->supplies);
> +		return ret;
> +	}
> +
> +	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +
> +	ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_compression_mode(ctx->dsi, true);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable compression mode: %d\n", ret);
> +		return ret;
> +	}

[Severity: High]
If mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode() fails
here, the function returns immediately. Earlier in this function, the
regulators are enabled and the reset GPIO is toggled. Should these error
paths also assert the reset GPIO and call regulator_bulk_disable() like
the error path for nt37705_on() does?

Since the prepare callback failed, the DRM core will not call
drm_panel_unprepare(), which leaves a permanent regulator imbalance and the
panel powered on in an inconsistent state.

> +	msleep(28); /* TODO: Is this panel-dependent? */
> +
> +	return 0;
> +}
[ ... ]
> +static int nt37705_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}

[Severity: High]
This function modifies dsi->mode_flags using non-atomic bitwise operations
without taking a lock. Since userspace can write to the sysfs brightness
node at any time, could this race with a DRM modeset where nt37705_on() or
nt37705_off() also modify the panel state? Concurrent execution could
corrupt dsi->mode_flags and lose flags like MIPI_DSI_MODE_NO_EOT_PACKET set
during probe.

[Severity: Medium]
When the panel is disabled and unprepared, its regulators are turned off.
If a write occurs to the sysfs backlight node while the panel is unprepared,
is it safe to unconditionally issue a DSI transmission via
mipi_dsi_dcs_set_display_brightness_large()?

Sending DSI DCS commands to a powered-off panel usually causes the DSI bus
to hang or timeout. Should this function check if ctx->panel.prepared is
true before attempting to communicate with the panel?

[Severity: Medium]
If mipi_dsi_dcs_set_display_brightness_large() fails, the function
returns early. Doesn't this leave the MIPI_DSI_MODE_LPM flag permanently
cleared? 

Also, on success, it unconditionally forces the MIPI_DSI_MODE_LPM flag on,
even if it wasn't originally set. This might corrupt the panel's mode flags
and cause subsequent commands to incorrectly use or omit Low Power Mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-fp6-panel-v4-0-7c3819664778@fairphone.com?part=2