Re: [PATCH v3 02/10] drm/mediatek: Implement Display Stream Compression support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing DSC setup due to DSI component being skipped in pipeline component search loop.
- [High] DSC is permanently forced into bypass mode due to inverted initialization order and missing bit clear.
- [High] mtk_dsi_poweron() programs DSI_HSTX_CKL_WC with 0 due to using slice_chunk_size before it is computed.
- [High] An operator precedence bug in the DSC padding calculation yields 3 instead of 0 when no padding is required.
- [High] The mtk_disp_dsc driver will fail to probe if the device tree lacks the optional 'mediatek,gce-client-reg' property.
- [High] Dangling pointer to `drm_dsc_config` if `component_add()` fails during DSI host attach.
- [Low] The mode_valid callback triggers kernel log spam by using dev_err() for unsupported modes.
--

--- Patch [2]: [PATCH v3 02/10] drm/mediatek: Implement Display Stream Compression support ---
commit 5dbf4bdae96138e01b5a5d8e367604bb50ec6d84
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: Implement Display Stream Compression support

This commit introduces a dedicated driver for the Display Stream Compression
(DSC) controller. It extracts DSC bypass configuration from mtk_ddp_comp to
the new driver and updates the DSI driver to handle DSC panel configurations.
It also adds DSC parameter computation and mode validation to the DSI
component.

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 8e552cdc3b53b..d4be40b2574a1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
[ ... ]
> @@ -398,6 +401,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc)
>  		if (!mtk_ddp_comp_add(mtk_crtc->ddp_comp[i], mtk_crtc->mutex))
>  			mtk_mutex_add_comp(mtk_crtc->mutex,
>  					   mtk_crtc->ddp_comp[i]->id);
> +
> +		/* For now, only single DSI is supported */
> +		if (mtk_crtc->ddp_comp[i]->id >= DDP_COMPONENT_DSI0 &&
> +		    mtk_crtc->ddp_comp[i]->id <= DDP_COMPONENT_DSI3)
> +			if (!comp_dsi)
> +				comp_dsi = mtk_crtc->ddp_comp[i];
> +
> +		if (mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC0 ||
> +		    mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC1)
> +			if (!comp_dsc)
> +				comp_dsc = mtk_crtc->ddp_comp[i];
>  	}

[Severity: High]
Does this loop miss the DSI component when it is the final component in the
display pipeline?

The enclosing loop in mtk_crtc_ddp_hw_init() has the condition
i < mtk_crtc->ddp_comp_nr - 1. Because the DSI component is almost always the
final block in the display pipeline, the loop terminates before evaluating it,
leaving comp_dsi as NULL.

Consequently, the later check if (comp_dsc && comp_dsi) evaluates to false,
and mtk_ddp_comp_dsc_setup() is entirely skipped, causing the display to
fail.

[ ... ]
> @@ -413,6 +427,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc)
>  		mtk_ddp_comp_start(comp);
>  	}
>  
> +	/* Setup the DSC if present, with the config coming from DSI */
> +	if (comp_dsc && comp_dsi) {
> +		dsc_cfg = mtk_dsi_get_dsc_config(comp_dsi->dev);
> +		if (dsc_cfg)
> +			mtk_ddp_comp_dsc_setup(comp_dsc, dsc_cfg);
> +	}
> +

[Severity: High]
Are we permanently forcing the DSC into bypass mode by invoking start before
setup here?

Because mtk_ddp_comp_start() is called on all components right before this
block, mtk_dsc_start() executes while dsc_config_done is still false.
This explicitly sets the DSC_BYPASS bit in DISP_REG_DSC_CON.

When mtk_ddp_comp_dsc_setup() runs immediately after, mtk_dsc_setup()
reads the register (which now has bypass set) and fails to clear it before
writing it back. This sequence leaves the DSC IP in bypass mode, sending
uncompressed data to a panel expecting compressed data.

> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> new file mode 100644
> index 0000000000000..2b3da91e75b1f
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
[ ... ]
> +	dsc_slice_h |= FIELD_PREP(DSC_SLICE_NUM_M1,
> +				  (pic_height_ext_num / dsc_cfg->slice_height) - 1);
> +	dsc_slice_h |= FIELD_PREP(DSC_SLICE_HEIGHT_M1, dsc_cfg->slice_height - 1);
> +
> +	dsc_pad_num = 3 - ((chunk_size * 2) % 3) % 3;
> +	dsc_pad_num = FIELD_PREP(DSC_PAD_NUMBER, dsc_pad_num);

[Severity: High]
Could this padding calculation evaluate to 3 instead of 0 when no padding is
required?

Because the modulo % operator binds tighter than subtraction -, when
chunk_size * 2 is a multiple of 3 (meaning 0 padding bytes are needed),
it evaluates as 3 - (0 % 3), resulting in 3. This writes an invalid
padding value to the hardware which corrupts the DSC chunk output.

Should this include parentheses for the intended wrap-around logic, like
(3 - ((chunk_size * 2) % 3)) % 3?

[ ... ]
> +static int mtk_dsc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get gce client reg\n");
> +#endif

[Severity: High]
Will the mtk_disp_dsc driver fail to probe on older device trees lacking the
optional mediatek,gce-client-reg property?

In the previous setup code for DSC in mtk_ddp_comp_init(),
cmdq_dev_get_client_reg() failures were treated as a debug warning and
execution continued. Treating this as a fatal error here prevents probe
entirely on older configurations.

Since the CMDQ register does not appear to be used in this driver (which
uses CPU writes), could this fatal exit be removed to prevent breaking older
device trees?

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index eb325e68aa592..ec5b94f7fa68e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -741,12 +826,19 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  
>  	mtk_dsi_ps_control(dsi, true);
>  	mtk_dsi_set_vm_cmd(dsi);
> -	mtk_dsi_config_vdo_timing(dsi);
> +	ret = mtk_dsi_config_vdo_timing(dsi);
> +	if (ret)
> +		goto err_disable_dsi_and_digital_clk;
> +

[Severity: High]
Does mtk_dsi_poweron() program DSI_HSTX_CKL_WC with 0 because
slice_chunk_size hasn't been computed yet?

In mtk_dsi_poweron(), mtk_dsi_ps_control(dsi, true) is called before
mtk_dsi_config_vdo_timing(dsi). mtk_dsi_ps_control_dsc() calculates the
pixel stream word count using dsi->dsc->slice_chunk_size and writes it to
DSI_HSTX_CKL_WC because config_vact is true.

However, the DSC RC parameters (including slice_chunk_size) are not computed
until drm_dsc_compute_rc_parameters() executes inside the subsequent call to
mtk_dsi_config_vdo_timing(). Consequently, slice_chunk_size is initially
0, which writes a 0 word count and breaks the DSI pixel stream.

When mtk_dsi_config_vdo_timing() later completes and calls
mtk_dsi_ps_control(dsi, false), the block that writes DSI_HSTX_CKL_WC is
skipped since config_vact is false, leaving the hardware incorrectly
programmed.

[ ... ]
> @@ -883,6 +975,28 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (mode->clock * bpp / dsi->lanes > 1500000)
>  		return MODE_CLOCK_HIGH;
>  
> +	if (dsi->dsc) {
> +		if (dsi->dsc->slice_width == 0 || dsi->dsc->slice_height == 0) {
> +			dev_err(dsi->host.dev,
> +				"DSC: Slice width %u height %u not valid!\n",
> +				dsi->dsc->slice_width, dsi->dsc->slice_height);
> +			return MODE_BAD;
> +		}
> +
> +		if (mode->hdisplay % dsi->dsc->slice_width) {
> +			dev_err(dsi->host.dev,
> +				"DSC: hdisplay %u is not a multiple of slice width %u\n",
> +				dsi->dsc->slice_width, mode->hdisplay);
> +			return MODE_H_ILLEGAL;
> +		}
> +		if (mode->vdisplay % dsi->dsc->slice_height) {
> +			dev_err(dsi->host.dev,
> +				"DSC: vdisplay %u is not a multiple of slice height %u\n",
> +				dsi->dsc->slice_height, mode->vdisplay);
> +			return MODE_V_ILLEGAL;
> +		}
> +	}

[Severity: Low]
Will this trigger kernel log spam by using dev_err() during mode validation?

Because mode_valid is invoked frequently by userspace (e.g., during EDID
enumeration and mode probing), printing an error using dev_err() for
incompatible panel modes can flood the kernel log. Mode rejection should
typically use drm_dbg_kms() or fail silently.

[ ... ]
> @@ -1006,6 +1127,9 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
>  			return PTR_ERR(dsi->next_bridge);
>  	}
>  
> +	if (device->dsc)
> +		dsi->dsc = device->dsc;
> +
>  	drm_bridge_add(&dsi->bridge);
>  
>  	ret = component_add(host->dev, &mtk_dsi_component_ops);

[Severity: High]
Does this leave a dangling pointer to drm_dsc_config if component_add()
fails?

If component_add() fails, mtk_dsi_host_attach() returns an error, which
prompts the DSI panel driver to tear down and free device->dsc. However,
dsi->dsc is never cleared on this error path.

If a different panel (without DSC) later attaches, device->dsc is NULL, so
the conditional if (device->dsc) does not update dsi->dsc, leaving it as
a dangling pointer. When the display pipeline later starts, this freed
pointer will be dereferenced, leading to a Use-After-Free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130103.34654-1-angelogioacchino.delregno@collabora.com?part=2