Re: [PATCH RFC 14/20] drm: msm: mdp5: Add MDP5 configuration for MSM8952

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing designated initializers in the lm.base array maps the LM3 base address to the wrong index, causing register corruption.
- [Critical] Defining `.pipe_cursor` with a Layer Mixer offset (`0x440DC`) causes the driver to incorrectly program LM cursor registers as if they were a standard SSPP pipe, trashing the configuration.
- [High] Inconsistent MDP_PIPE_CAP_SW_PIX_EXT capability between pipe_cursor and main pipes.
--

commit fb5220e24e57b56379d9ec75eb6d29f391529a7b
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

drm: msm: mdp5: Add MDP5 configuration for MSM8952

Add the mdp5_cfg_hw entry for MDP5 version v1.12 found on MSM8952.

> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 7c91fc1915f39..d34c94d6f7e2e 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -744,6 +744,86 @@ static const struct mdp5_cfg_hw msm8x76_config = {

[ ... ]

> +static const struct mdp5_cfg_hw msm8952_config = {
> +	.name = "msm8952",

[ ... ]

> +	.pipe_cursor = {
> +		.count = 1,
> +		.base = { 0x440DC },

[Severity: Critical]
This isn't a bug introduced by this patch, but copying this configuration
defines a Layer Mixer offset (0x440DC) for an SSPP pipe.

Does this cause the driver to incorrectly program LM cursor registers as if
they were a standard SSPP pipe?

During cursor plane updates via mdp5_hwpipe_mode_set(), standard SSPP macros
like REG_MDP5_PIPE_* will treat 0x440DC as the SSPP base.
REG_MDP5_PIPE_OUT_XY (offset 0x10) will write to 0x440DC + 0x10 = 0x440EC
(LM_CURSOR_FORMAT), and REG_MDP5_PIPE_SRC_SIZE (offset 0x0) overwrites
LM_CURSOR_STRIDE. This misalignment destroys the LM cursor configurations
and causes hardware faults.

> +		.caps = MDP_PIPE_CAP_HFLIP	|
> +			MDP_PIPE_CAP_VFLIP	|
> +			MDP_PIPE_CAP_SW_PIX_EXT	|
> +			MDP_PIPE_CAP_CURSOR	|
> +			0,
> +	},

[Severity: High]
This isn't a bug introduced by this patch, but copying this configuration
leaves an inconsistent MDP_PIPE_CAP_SW_PIX_EXT capability between
pipe_cursor and the main pipes.

Are we exposing Software Pixel Extension capabilities on the cursor pipe
while main pipes lack it?

If the SoC does not support Software Pixel Extension,
mdp5_plane_atomic_update() will call mdp5_write_pixel_ext() and write to
non-existent SW_PIX_EXT registers when programming the cursor pipe,
triggering a Synchronous External Abort. If it does support it, the main
planes are underutilizing the hardware.

> +	.lm = {
> +		.count = 2,
> +		.base = { 0x44000, 0x47000 },
> +		.instances = {
> +				{ .id = 0, .pp = 0, .dspp = 0,
> +				  .caps = MDP_LM_CAP_DISPLAY, },
> +				{ .id = 3, .pp = -1, .dspp = -1,
> +				  .caps = MDP_LM_CAP_WB },
> +			     },

[Severity: Critical]
This isn't a bug introduced by this patch, but copying this configuration
leaves the lm.base array initialized sequentially without designated
initializers.

Will this map the LM3 base address to the wrong index and cause register
corruption?

Because lm.base is initialized sequentially as { 0x44000, 0x47000 },
base[0] becomes 0x44000 and base[1] becomes 0x47000. The remaining indices,
including base[3], are implicitly initialized to 0.

When the driver creates a mixer for LM3 (since its .id is 3), atomic commits
will cause REG_MDP5_LM_OUT_SIZE(3) to resolve to __offset_LM(3). This
returns mdp5_cfg->lm.base[3], which evaluates to 0x0. Writes to LM3
registers will thus write to SOC offset 0x0, corrupting the global MDP
configuration registers.

Should this use designated initializers for the base array to ensure base[3]
is mapped correctly?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-msm8952-initial-support-v1-0-0e742578b524@mainlining.org?part=14