Re: [PATCH v3 07/10] drm/mediatek: mtk_dsi: Add support for MT8196

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unit mismatch between DSI data rate (Hz) and pixel clock rate (MHz) leads to integer underflow and incorrect QoS hardware buffer configuration.
- [High] 64-bit division by a non-constant variable causes a build error on 32-bit platforms.
--

--- Patch [7]: [PATCH v3 07/10] drm/mediatek: mtk_dsi: Add support for MT8196 ---
commit 92e543d518b805c7b9129880068f5a6955059779
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: drm/mediatek: mtk_dsi: Add support for MT8196

This commit adds support for the new DSI IP in the MT8196 SoC. It
implements hardware buffer configuration and initializes QoS parameters
required by this newer hardware.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index dffb6befc749e..573299f0e63b6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -776,6 +872,91 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
>  	return drm_dsc_compute_rc_parameters(dsc);
>  }
>  
> +static void mtk_dsi_config_hw_buffers(struct mtk_dsi *dsi)
> +{
[ ... ]
> +	tmp = (u64)dsi->data_rate * dsi->lanes;
> +	data_rate_per_buf = tmp / 8 / buffer_unit;

[Severity: High]
Does this 64-bit division break builds on 32-bit platforms?

Since tmp is a u64 and buffer_unit is a non-constant u32, this will likely
cause an undefined reference to __udivdi3 or __aeabi_uldivmod when compiling
for 32-bit architectures. Should this use div_u64() instead?

> +
> +	/* Calculate valid threshold and avoid exceeding FIFO size */
> +	out_valid_thresh = MTK_DSI_DEFAULT_QOS_VALID_FIFO_US * data_rate_per_buf;

[Severity: High]
Are there unit mismatches in these rate calculations?

Since dsi->data_rate is in Hz, data_rate_per_buf is calculated
as buffers per second (e.g., ~15,000,000). Multiplying it by
MTK_DSI_DEFAULT_QOS_VALID_FIFO_US (25) will result in a huge value
that will always over-saturate the FIFO limit.

> +	out_valid_thresh = min(out_valid_thresh, MTK_DSI_MAX_FIFO_BYTES - 1);
> +	mtk_dsi_mask(dsi, reg_qos[DSI_QOS_BUF_CON1], BUF_OUT_VALID_THRESH, out_valid_thresh);
> +
> +	/* Enable ULTRA signal trigger between SOF and VACT */
> +	mtk_dsi_mask(dsi, data->reg_adv[DSI_VDE], VDE_BLOCK_ULTRA, 0);
> +
> +	/* Calculate fill rate with line counter mode for DSI Video Mode */
> +	if (dsi->format == MIPI_DSI_FMT_RGB565)
> +		dsi_buf_bpp = 2;
> +	else
> +		dsi_buf_bpp = 3;
> +
> +	pclk_mhz = dsi->vm.pixelclock / HZ_PER_MHZ;
> +	fill_rate = pclk_mhz * data->pixels_per_iter * dsi_buf_bpp / buffer_unit;
> +
> +	/* Calculate QoS Anti-Latency parameters */
> +	sodi_hi = MTK_DSI_MAX_FIFO_BYTES * num_hw_buffers;
> +	sodi_hi -= (fill_rate - data_rate_per_buf) * 12 / 10;

Similarly, here fill_rate is computed using pclk_mhz and results in
buffers per microsecond (e.g., ~40). Subtracting the much larger
buffers-per-second value of data_rate_per_buf will cause an integer
underflow, resulting in incorrect hardware buffer configurations.

Could data_rate_per_buf be converted to a per-microsecond rate to align
with fill_rate and the microseconds constant?

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