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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/13/26 16:41, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Truncation of RC buffer thresholds due to incorrect bitwise masking instead of right-shifting.
> - [High] Hardcoded and conflicting slice count assumptions without panel validation.
> - [High] Component discovery loop skips DSI component, preventing DSC initialization.
> - [High] DSI timing calculation ignores compression and overwrites D-PHY clock lane register.
> - [Medium] State retention bug where `dsc_config_done` is never reset to false.
> --
> 
> --- Patch [2]: [PATCH v4 02/10] drm/mediatek: Implement Display Stream Compression support ---
> commit 94686398e3eedc1a9e39476763b6747a4bcfb4c9
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Subject: drm/mediatek: Implement Display Stream Compression support
> 
> This commit introduces a new driver for the Display Stream Compression (DSC)
> Display Controller IP, implementing support for DSC v1.1 to v1.2. It adds
> DSC handling into the DSI driver and integrates the bypass setup via CPU
> writes instead of CMDQ.
> 
>> 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];
>> +
> 
> [Severity: High]
> Does this loop terminate one iteration early, causing the DSI component
> to be skipped?
> 
> Because the loop condition is i < mtk_crtc->ddp_comp_nr - 1, and the DSI
> interface is typically the final component in the display pipeline, it
> appears comp_dsi will remain NULL here. This would cause the subsequent
> DSC initialization check to silently fail and bypass the DSC setup entirely.
> 
>> 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..bed6b77bf9a9e
>> --- /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 |= (rbt[buf_index] & DSC_P8_RC_BUF_THR_X) << data_shift;
> 
> [Severity: High]
> Does this code truncate the RC buffer thresholds by masking with 0xFF
> instead of shifting right?
> 

This is not truncating anything, and it is the equivalent of

		dsc->dsc_cfg.rc_buf_thresh[idx+3] << 24 |
		dsc->dsc_cfg.rc_buf_thresh[idx+2] << 16 |
		dsc->dsc_cfg.rc_buf_thresh[idx+1] << 8 |
		dsc->dsc_cfg.rc_buf_thresh[idx+0]);

So this is fine.

> The VESA DSC specification requires programming the Picture Parameter Set
> registers with RC buffer thresholds divided by 64. Using a bitwise AND mask
> instead of shifting right by 6 appears to write truncated values to the PPS
> registers, which could break rate control.
> 
> [ ... ]
>> +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg)
>> +{
> [ ... ]
>> +	pic_height_ext_num = dsc_cfg->pic_height + dsc_cfg->slice_height - 1;
>> +	pic_group_width = dsc_cfg->slice_width * 4;
>> +	pic_group_width /= 3;
> 
> [Severity: High]
> Does this calculation hardcode the picture group width to assume exactly
> four slices per line?
> 

No, that makes the hardware process 1+1/4 pic width per iteration.

> If a panel requests a different slice count, this could conflict with the
> payload size assumptions made elsewhere in the pipeline and potentially
> cause display corruption.
> 
> [ ... ]
>> +	if (dsc_en_bit)
>> +		dsc_con |= DSC_EN;
>> +
>> +	writel(dsc_con, disp_dsc->reg + DISP_REG_DSC_CON);
>> +
>> +	disp_dsc->dsc_config_done = true;
>> +}
> 
> [Severity: Medium]
> Can this state retention cause issues on subsequent modesets?
> 
> Since the dsc_config_done flag is never reset to false on teardown, will a
> subsequent non-DSC modeset bypass the check below in mtk_dsc_start() and
> leave the DSC IP active with stale parameters?
> 

The dynamic DSC -> non-DSC modesets can be supported only after the mediatek-drm
restructuring. As of now, only all-compressed or all-uncompressed modes displays
are supported.

> [ ... ]
>> +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) {
> 
> [ ... ]
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index 3f3f56eed3f92..8ab5c3431dbbc 100644
>> --- 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 to assume exactly two slices per line?
> 
> This appears to conflict with the assumption of four slices per line inside
> mtk_dsc_setup(). Is it possible to validate the panel's requested slice
> count against these assumptions during the mode_valid check?
> 

No, that's another thing entirely, it's the per-iteration DSI word count and
the dsc_setup is not hardcoding four slices per line.

>> +
>> +	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]
> Will writing the compressed payload size here overwrite the correctly
> calculated D-PHY High-Speed Clock Lane Word Count?
> 

What!??! No!!!

> [ ... ]
>> -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>> +static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>>   {
>>   	struct videomode *vm = &dsi->vm;
>> +	int ret;
>>   
>>   	writel(vm->vsync_len, dsi->regs + DSI_VSA_NL);
>>   	writel(vm->vback_porch, dsi->regs + DSI_VBP_NL);
>>   	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) {
>> +		ret = mtk_dsi_set_dsc_params(dsi);
>> +		if (ret)
>> +			return ret;
>> +
>> +		mtk_dsi_ps_control(dsi, true);
> 
> [Severity: High]
> Do the DSI lane timings calculated earlier in
> mtk_dsi_config_vdo_timing_per_frame_lp() incorrectly use the uncompressed
> pixel count?
> 
> By passing config_vact = true into mtk_dsi_ps_control() for DSC streams, it
> looks like the raw compressed payload size will subsequently overwrite the
> timing synchronized register DSI_HSTX_CKL_WC.
> 

It's done on purpose.