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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:

New issues:
- [High] DSC setup overwrites the DSC control register, disabling the IP after it was started.
- [High] Uninitialized `slice_chunk_size` is used to configure hardware registers.
- [High] Integer truncation on `range_data` zeroes out the second set of RC range parameters.
- [High] Off-by-one loop boundary misses PPS 19 and the final RC range parameter.
- [High] Incorrect bitmask validation erroneously rejects valid integer BPP configurations.
- [High] State leakage of `dsc_config_done` causes bypass to be skipped when switching out of DSC modes.
- [High] Loop skips the final pipeline component, causing DSI and DSC to not be found and DSC configuration to be ignored.
- [High] Integer truncation on `dsc_pre_pad_sz` zeroes out the `DSC_PIC_PREPAD_WIDTH` configuration.
- [High] The DSI pixel stream word count assumes exactly 2 DSC slices per line, violating panel variability.
- [High] DSI link timings use uncompressed byte counts instead of compressed payloads when DSC is active.
- [High] Dangling pointer to `drm_dsc_config` leads to Use-After-Free if the DSI panel is unbound.
- [High] Unscaled RC buffer thresholds are packed into PPS registers, causing bitwise overflows.
- [Medium] Missing hardware cleanup in `mtk_dsi_poweron()` error path leaves DSI enabled but unclocked.
- [Medium] `COMPRESSED_PS_DSC` stream type is conditionally set only when the format is `MIPI_DSI_FMT_RGB888`.
- [Medium] Missing zero validation for `slice_width` and `slice_height` risks division by zero panics.

Pre-existing issues:
- [High] Caller ignores `mtk_dsi_poweron()` errors, causing unprotected writes to unclocked DSI MMIO registers.
--

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

Add a real driver for the Display Stream Compression (DSC) Display
Controller IP, implementing support for DSC v1.1 to v1.2.

> 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 skip the final pipeline component? Because it iterates only to
mtk_crtc->ddp_comp_nr - 1, the final component (which is often DSI) won't be
checked, leaving comp_dsi as NULL and silently bypassing DSC configuration.

>  	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);
> @@ -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]
Can calling mtk_ddp_comp_dsc_setup() after mtk_ddp_comp_start() disable the
DSC IP? Looking at mtk_dsc_setup(), it performs a direct writel() to
DISP_REG_DSC_CON which clears the DSC_EN bit that was just set during the
start phase.

>  	/* Initially configure all planes */
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
> 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..79b66bd7e5f3c
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
[ ... ]
> +static void mtk_dsc_pps_setup(struct mtk_dsc *disp_dsc, struct drm_dsc_config *dsc_cfg)
> +{
[ ... ]
> +	/* PPS 8..11 - Each register holds 4 RC buffer thresholds (PPS 11 has two) */
> +	for (i = 0; i < 4; i++) {
> +		u8 block_num = i * 4;
> +		data = 0;
> +
> +		for (j = 0; j < 4; j++) {
> +			u8 buf_index = block_num + j;
> +			u8 data_shift = j * 8;
> +
> +			/* rc_buf_thresh holds 14 elements in total */
> +			if (buf_index > 13)
> +				break;
> +
> +			data |= dsc_cfg->rc_buf_thresh[buf_index] << data_shift;

[Severity: High]
Are the RC buffer thresholds missing scaling here? The rc_buf_thresh values
can be up to 14 bits wide, but they are packed into 8-bit fields without being
right-shifted by 6, which could cause bitwise overflows into adjacent fields.

> +		}
> +		writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(8 + i));
> +	}
> +
> +	/* PPS 12..19 - Each register holds two sets of RC range parameters (19 has one) */
> +	for (i = 0; i < 7; i++) {

[Severity: High]
Does this loop terminate before configuring PPS 19? The condition i < 7
means it iterates through i = 6, which covers up to PPS 18 (12 + 6). The 15th
RC range parameter in PPS 19 appears to be skipped.

> +		struct drm_dsc_rc_range_parameters *rcrp = dsc_cfg->rc_range_params;
> +		u8 block_num = i * 2;
> +		data = 0;
> +
> +		for (j = 0; j < 2; j++) {
> +			u8 buf_index = block_num + j;
> +			u8 data_shift = j * 16;
> +			u16 range_data;
> +
> +			range_data = FIELD_PREP(DSC_P12_RC_RANGE_MIN_QP,
> +						 rcrp[buf_index].range_min_qp);
> +			range_data |= FIELD_PREP(DSC_P12_RC_RANGE_MAX_QP,
> +						 rcrp[buf_index].range_max_qp);
> +			range_data |= FIELD_PREP(DSC_P12_RC_RANGE_BPG_OFFSET,
> +						 rcrp[buf_index].range_bpg_offset);
> +
> +			range_data <<= data_shift;

[Severity: High]
Could this shift cause integer truncation? The range_data variable is declared
as u16. When j=1, data_shift is 16, shifting all bits out of the u16 and
truncating it to 0, which would lose the configuration for the odd-indexed RC
range parameters.

> +			data |= range_data;
> +
> +			/* rc_range_params holds 15 elements in total */
> +			if (buf_index == 14)
> +				break;
> +		}
> +		writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(12 + i));
> +	}
> +}
> +
> +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +	u32 dsc_slice_w, dsc_slice_h, dsc_mode, dsc_cfg_rval, dsc_shadow;
> +	u32 dsc_dbg_con, dsc_con, dsc_enc_width, dsc_pic_w, dsc_pic_h;
> +	u16 pic_group_width, pic_height_ext_num, slice_group_width;
> +	u16 chunk_size, dsc_pad_num, dsc_pre_pad_sz;
[ ... ]
> +	dsc_slice_w = FIELD_PREP(DSC_SLICE_GROUP_WIDTH_M1, slice_group_width - 1);
> +	dsc_slice_w |= FIELD_PREP(DSC_SLICE_WIDTH, dsc_cfg->slice_width);
> +	dsc_slice_h = FIELD_PREP(DSC_SLICE_WIDTH_MOD3, dsc_cfg->slice_width % 3);
> +	dsc_slice_h |= FIELD_PREP(DSC_SLICE_NUM_M1,
> +				  (pic_height_ext_num / dsc_cfg->slice_height) - 1);

[Severity: Medium]
Is it possible for dsc_cfg->slice_height to be zero here, leading to a
division by zero panic? A validation check for zero slice height might be
needed.

> +	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);
> +
> +	dsc_pre_pad_sz = FIELD_PREP(DSC_PIC_PREPAD_HEIGHT, dsc_cfg->pic_height);
> +	dsc_pre_pad_sz |= FIELD_PREP(DSC_PIC_PREPAD_WIDTH, dsc_cfg->pic_width);

[Severity: High]
Will this truncate the DSC_PIC_PREPAD_WIDTH value? The variable dsc_pre_pad_sz
is declared as a u16, but DSC_PIC_PREPAD_WIDTH is defined as GENMASK(31, 16),
which means the OR operation will overflow the u16 and lose the width data.

> +
> +	dsc_mode = FIELD_PREP(DSC_INIT_DELAY_HEIGHT, 4);
[ ... ]
> +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) {

[Severity: High]
Does this handle transitioning from DSC to non-DSC modes correctly? Because
disp_dsc->dsc_config_done is never cleared after being set in mtk_dsc_setup(),
it looks like setting bypass mode will be skipped on subsequent starts in
non-DSC configurations.

> +		mtk_ddp_write_mask(NULL, DSC_BYPASS, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_BYPASS);
> +		mtk_ddp_write_mask(NULL, DSC_UFOE_SEL, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_UFOE_SEL);
> +		mtk_ddp_write_mask(NULL, DSC_DUAL_INOUT, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_DUAL_INOUT);
> +	}
> +
> +	mtk_ddp_write_mask(NULL, DSC_EN, &disp_dsc->cmdq_reg,
> +			   disp_dsc->reg, DISP_REG_DSC_CON, DSC_EN);
> +}
> +
> +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);
> +}
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 3f3f56eed3f92..3e22553ca990f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -395,7 +399,7 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>  
>  static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>  {
> -	u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl;
> +	u32 dsi_buf_bpp, ps_val, ps_wc, size_val, vact_nl;
>  
>  	if (dsi->format == MIPI_DSI_FMT_RGB565)
>  		dsi_buf_bpp = 2;
>  	else
>  		dsi_buf_bpp = 3;
>  
>  	/* Word count */
> -	ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp);
> +	if (dsi->dsc)
> +		ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size);

[Severity: High]
Is it safe to assume exactly 2 DSC slices per line here? If a panel requires
1, 4, or another slice count, hardcoding the multiplier to 2 could result in
improperly sized MIPI DSI packets.

> +	else
> +		ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp);
>  	ps_val = ps_wc;
>  
>  	/* Pixel Stream type */
> @@ -411,7 +418,10 @@ static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>  	default:
>  		fallthrough;
>  	case MIPI_DSI_FMT_RGB888:
> -		ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888);
> +		if (dsi->dsc)
> +			ps_val |= FIELD_PREP(DSI_PS_SEL, COMPRESSED_PS_DSC);
> +		else
> +			ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888);
>  		break;

[Severity: Medium]
Will this prevent compressed streams for formats other than RGB888? Since the
COMPRESSED_PS_DSC assignment is fully nested inside the MIPI_DSI_FMT_RGB888
case, using DSC with RGB666 would mistakenly signal PACKED_PS_18BIT_RGB666
over the bus.

>  	case MIPI_DSI_FMT_RGB666:
>  		ps_val |= FIELD_PREP(DSI_PS_SEL, LOOSELY_PS_24BIT_RGB666);
> @@ -565,7 +587,43 @@ static void mtk_dsi_config_vdo_timing_per_line_lp(struct mtk_dsi *dsi)
>  	writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>  }
>  
> -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> +static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
> +{
> +	struct drm_dsc_config *dsc = dsi->dsc;
> +	struct device *dev = dsi->host.dev;
> +	int ret;
> +
> +	if (dsc->bits_per_pixel & GENMASK(7, 0)) {
> +		dev_err(dev, "Fractional bits_per_pixel not supported\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Could this bitmask erroneously reject valid integer bits-per-pixel values? The
bits_per_pixel value is shifted by 4 (to allow 4 fractional bits).
GENMASK(7, 0) covers the lower 8 bits, which includes 4 of the integer bits.
This would fail for a valid 8 BPP configuration.

> +
> +	if (dsc->bits_per_component != 8) {
> +		dev_err(dev, "%u bits per component is not supported\n",
> +			dsc->bits_per_component);
[ ... ]
> +static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>  {
>  	struct videomode *vm = &dsi->vm;
>  
> @@ -574,17 +632,17 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>  	writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL);
>  	writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>  
> -	if (dsi->driver_data->has_size_ctl)
> -		writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
> -			FIELD_PREP(DSI_WIDTH, vm->hactive),
> -			dsi->regs + DSI_SIZE_CON);
> -
>  	if (dsi->driver_data->support_per_frame_lp)
>  		mtk_dsi_config_vdo_timing_per_frame_lp(dsi);
>  	else
>  		mtk_dsi_config_vdo_timing_per_line_lp(dsi);
>  
>  	mtk_dsi_ps_control(dsi, false);
> +
> +	if (dsi->dsc)
> +		return mtk_dsi_set_dsc_params(dsi);
> +
> +	return 0;
>  }

[Severity: High]
Is there an ordering issue here with uninitialized parameters? 
mtk_dsi_ps_control() reads dsi->dsc->slice_chunk_size, but it hasn't been
computed yet because mtk_dsi_set_dsc_params() is called afterwards.

Additionally, do the DSI timing calculations use uncompressed byte counts when
DSC is active? The functions mtk_dsi_config_vdo_timing_per_frame_lp() and
per_line_lp() calculate ps_wc using vm->hactive * dsi_tmp_buf_bpp, which
ignores the reduced compressed payload size, potentially miscomputing the
front/back porch and roundup metrics.

> @@ -741,12 +799,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);
> +	if (ret)
> +		goto err_disable_digital_clk;
> +
>  	mtk_dsi_set_interrupt_enable(dsi);
>  	mtk_dsi_lane_ready(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
>  	return 0;
> +err_disable_digital_clk:
> +	clk_disable_unprepare(dsi->digital_clk);
>  err_disable_engine_clk:
>  	clk_disable_unprepare(dsi->engine_clk);
>  err_phy_power_off:

[Severity: Medium]
Does this error path miss cleaning up the DSI hardware state? If
mtk_dsi_config_vdo_timing() fails, jumping to err_disable_digital_clk skips
calling mtk_dsi_disable(dsi), which was enabled earlier in mtk_dsi_poweron(),
leaving it enabled but unclocked.

[Severity: High]
This is a pre-existing issue, but does mtk_dsi_ddp_start() ignore
mtk_dsi_poweron() errors? Since mtk_dsi_poweron() can now return errors (like
when DSC parameter setup fails), mtk_dsi_ddp_start() might proceed to call
mtk_dsi_start() on unclocked hardware.

> @@ -883,6 +946,21 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (mode->clock * bpp / dsi->lanes > 1500000)
>  		return MODE_CLOCK_HIGH;
>  
> +	if (dsi->dsc) {
> +		if (mode->hdisplay % dsi->dsc->slice_width) {

[Severity: Medium]
Is there a risk of a division by zero panic here? If an external panel driver
supplies a zero slice_width, this modulo operation will crash.

> +			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;
> +		}
> +	}
> +
>  	return MODE_OK;
>  }
[ ... ]
> @@ -1006,6 +1091,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;
> +

[Severity: High]
Does this create a dangling pointer if the DSI panel is unbound? A pointer to
device->dsc is captured here, but there appears to be no corresponding code in
mtk_dsi_host_detach() to set it back to NULL, which could lead to a
use-after-free.

>  	drm_bridge_add(&dsi->bridge);
>  
>  	ret = component_add(host->dev, &mtk_dsi_component_ops);

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