Re: [PATCH v5 02/11] 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] Hardware Initialization Bypass due to skipped array element in DDP component loop.
- [High] DSC configuration is written after the component is started, causing pipeline corruption.
- [High] State leak of `dsc_config_done` breaks non-DSC display modes.
- [High] DSI clock lane timing register `DSI_HSTX_CKL_WC` is destroyed by being overwritten with the pixel payload word count.
- [High] Hardcoded 2x multiplier for DSC slice count results in incorrect DSI payload word count.
- [High] Bandwidth calculations use uncompressed bits-per-pixel (bpp), falsely rejecting valid DSC modes and misconfiguring PHY clocks.
- [Medium] Uninitialized variable `slice_chunk_size` is used for hardware configuration during DSI power-on.
--

--- Patch [2]: [PATCH v5 02/11] drm/mediatek: Implement Display Stream Compression support ---
commit e5bc691dc168426274a3882d1ba1c028fccf77c4
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: drm/mediatek: Implement Display Stream Compression support

This commit introduces Display Stream Compression (DSC) support for the
MediaTek display controller IP. It extracts DSC IP bypass setup from
mtk_ddp_comp into a new mtk_disp_dsc driver and adds DSC handling to the
mtk_dsi driver.

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> --- 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];

[Severity: High]
Does this loop skip checking the DSI component?

Looking at the loop definition in mtk_crtc_ddp_hw_init(), it iterates up to
ddp_comp_nr - 1. Since DSI is typically the sink and sits at the very end of
the pipeline, it is at index ddp_comp_nr - 1. This means the loop body
checking for DSI components never executes for it, leaving comp_dsi as NULL.
Consequently, the subsequent mtk_ddp_comp_dsc_setup() call is silently
bypassed.

> +
> +		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];
>  	}
[ ... ]
> @@ -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]
Is mtk_ddp_comp_dsc_setup() called too late here?

The preceding loop calls mtk_ddp_comp_start(comp) for all components, which
starts the DSC in bypass mode. Then mtk_dsc_setup() disables the active DSC
engine:

writel(dsc_con & ~DSC_EN, disp_dsc->reg + DISP_REG_DSC_CON);

It then reconfigures and re-enables the engine. Could disabling and
reconfiguring the active component while the pipeline is already streaming
video data cause hardware hangs or stream corruption?

> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
[ ... ]
> +void mtk_dsc_start(struct device *dev)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +
> +	/* If no DSC or config not done, set bypass mode */
> +	if (!disp_dsc->dsc_config_done) {
> +		mtk_ddp_write_mask(NULL, DSC_BYPASS, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_BYPASS);

[Severity: High]
Does disp_dsc->dsc_config_done need to be cleared when the DSC is stopped?

When switching from a DSC mode to a non-DSC mode, mtk_dsc_stop() zeros
DISP_REG_DSC_CON but leaves dsc_config_done set to true. Later,
mtk_dsc_start() evaluates !disp_dsc->dsc_config_done as false and skips
writing the DSC_BYPASS bit.

Since mtk_dsc_stop() previously zeroed the register, DSC_BYPASS is 0,
leaving compression enabled on a pipeline that now expects uncompressed
video.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
> +static void mtk_dsi_ps_control_dsc(struct mtk_dsi *dsi, bool config_vact)
> +{
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_main = dsi->driver_data->reg_main;
> +	const short dsi_buf_bpp = 3;
> +	u32 ps_wc;
> +
> +	/* Word count */
> +	ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size);

[Severity: High]
Is it safe to hardcode the slice multiplier to 2?

If a DSC panel uses a different slice count, such as 1 or 4, this calculates
an incorrect payload word count. Should this dynamically multiply by
dsi->dsc->slice_count instead?

> +
> +	if (config_vact) {
> +		writel(FIELD_PREP(VACT_NL, dsi->vm.vactive),
> +		       dsi->regs + reg_main[DSI_VACT_NL]);
> +		writel(ps_wc, dsi->regs + reg_main[DSI_HSTX_CKL_WC]);

[Severity: High]
Does this overwrite the clock lane timing configuration?

In mtk_dsi_config_vdo_timing(), the code calculates and writes intricate
clock timing to DSI_HSTX_CKL_WC via mtk_dsi_config_vdo_timing_per_frame_lp().
Immediately after, it calls mtk_dsi_ps_control(dsi, true), which executes
this writel.

Could this destroy the previously calculated clock timing by completely
overwriting it with just the pixel payload word count?

[ ... ]
> @@ -741,12 +828,19 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  
>  	mtk_dsi_ps_control(dsi, true);

[Severity: Medium]
Is dsi->dsc->slice_chunk_size fully initialized before this call?

Here, mtk_dsi_ps_control(dsi, true) routes to mtk_dsi_ps_control_dsc(),
which reads dsi->dsc->slice_chunk_size.

However, the chunk size is only calculated later when
mtk_dsi_config_vdo_timing() calls drm_dsc_compute_rc_parameters(dsc). Will
this result in uninitialized values being temporarily written to DSI hardware
registers during power-on?

[ ... ]
> @@ -883,6 +977,28 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (mode->clock * bpp / dsi->lanes > 1500000)
>  		return MODE_CLOCK_HIGH;

[Severity: High]
Does this bandwidth calculation falsely reject valid DSC modes?

The bpp value is calculated earlier using mipi_dsi_pixel_format_to_bpp(),
which returns the uncompressed bits-per-pixel. Using the uncompressed bpp
for DSC modes calculates a much higher required bandwidth, which can exceed
hardware limits and reject legitimate modes.

Similarly, in mtk_dsi_poweron(), using the uncompressed bpp calculates
a data_rate that misconfigures the PHY clock. Should these calculations
use the compressed bpp (dsi->dsc->bits_per_pixel >> 4) when DSC is enabled?

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