Re: [PATCH v8 02/13] 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] mtk_crtc: DSC hardware enabled before being configured
- [High] drm/mediatek: Hardcoded DSC slice geometry breaks configurable topology
- [High] mtk_crtc: Component loop excludes terminal DSI component, skipping DSC init
- [High] mtk_dsi: Hardware state updated using uninitialized slice_chunk_size
- [High] mtk_dsi: Data rate and mode validation ignore DSC compression benefits
- [Medium] mtk_disp_dsc: State flag dsc_config_done not cleared on hardware stop
- [Low] mtk_dsi: Reversed string formatting arguments in debug messages
--

commit 842ba0cd3cf93b49cd15a2061c815f58c2c063f1
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: Implement Display Stream Compression support

This commit adds a new driver for the Display Stream Compression (DSC)
Display Controller IP, implementing support for DSC v1.1. It introduces DSC
handling in the mtk_dsi driver and removes basic bypass setup from
mtk_ddp_comp, migrating it to the new mtk_disp_dsc 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
> @@ -396,6 +399,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]
Is the terminal DSI component missed by the bounds condition in this loop?

This logic is evaluated inside a loop defined as for (i = 0; i <
mtk_crtc->ddp_comp_nr - 1; i++). If the DSI component is the last element in
the display pipeline, it is never evaluated by the loop because of the bound
check. As a result, comp_dsi would remain NULL, and DSC initialization would
be silently skipped entirely.

[ ... ]
> @@ -411,6 +425,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]
Could this execution order lead to pipeline synchronization glitches or data
corruption?

The pipeline starts transmitting data when the loop above calls
mtk_ddp_comp_start() for all components. The DSC block is not configured
until mtk_ddp_comp_dsc_setup() runs after the loop completes. This momentary
transmission in an unconfigured or bypass state, followed by programming and
re-enabling on the fly, might lock up the hardware FIFOs.

> 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_setup(struct device *dev, struct drm_dsc_config *dsc_cfg)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
[ ... ]
> +	pic_height_ext_num = dsc_cfg->pic_height + dsc_cfg->slice_height - 1;
> +	pic_group_width = (dsc_cfg->slice_width * 4) / 3;

[Severity: High]
Is the picture group width hardcoded to exactly 4 slices per line here?

This seems to mathematically conflict with the word count calculation in
mtk_dsi_ps_control_dsc() which assumes 2 slices per line. Hardcoded,
conflicting slice geometry assumptions rather than utilizing the slice_count
from the configuration might guarantee data corruption on panels that do not
match these exact assumptions.

[ ... ]
> +void mtk_dsc_start(struct device *dev)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	val = readl(disp_dsc->reg + DISP_REG_DSC_CON);
> +
> +	/* If no DSC or config not done, stop the HW temporarily and set bypass mode */
> +	if (!disp_dsc->dsc_config_done) {
> +		val &= ~DSC_EN;
> +		val |= DSC_BYPASS | DSC_UFOE_SEL | DSC_DUAL_INOUT;
> +		writel(val, disp_dsc->reg + DISP_REG_DSC_CON);
> +	}
[ ... ]
> +void mtk_dsc_stop(struct device *dev)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +
> +	writel(0, disp_dsc->reg + DISP_REG_DSC_CON);
> +}

[Severity: Medium]
Does the config flag remain true on subsequent modesets after being stopped?

Since mtk_dsc_stop() resets the hardware register but doesn't clear the
disp_dsc->dsc_config_done flag, a subsequent modeset enable might see this
flag as true. This would skip the bypass mode logic and instead re-enable
the IP with zeroed configuration bits, momentarily corrupting the stream
until setup executes.

> 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_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]
Does this hardcode the DSI payload word count to exactly 2 slices per line?

This conflicts with the 4 slice assumption in mtk_dsc_setup() and breaks
configurable topologies that rely on dsi->dsc->slice_count.

[ ... ]
> @@ -741,10 +828,17 @@ 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);

[Severity: High]
Is it possible for this hardware state to be updated using an uninitialized
slice_chunk_size?

The mtk_dsi_ps_control() call consumes dsi->dsc->slice_chunk_size to program
active DSI hardware, but this variable is not calculated until
mtk_dsi_config_vdo_timing() runs on the subsequent line. This could write
uninitialized memory into critical hardware registers.

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

[Severity: High]
Are we validating the mode and calculating the data rate using the
uncompressed BPP?

In mtk_dsi_bridge_mode_valid() (and similarly in mtk_dsi_poweron()), the bpp
is retrieved using mipi_dsi_pixel_format_to_bpp() before checking if DSC is
enabled. Applying the uncompressed BPP to clock limits and data rate
calculations ignores DSC bandwidth savings. This forces the DSI PHY to be
clocked much faster than necessary and artificially rejects valid
high-bandwidth modes.

> +		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_dbg(dsi->host.dev,
> +				"DSC: hdisplay %u is not a multiple of slice width %u\n",
> +				dsi->dsc->slice_width, mode->hdisplay);

[Severity: Low]
Have the string formatting arguments been reversed here?

The slice_width variable is passed to the first format specifier which
expects hdisplay, and mode->hdisplay is passed to the second which expects
the slice width.

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