Re: [PATCH v2 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] Integer overflow and time-domain unit mismatch (bps vs Mbps) in QoS HW buffer calculations corrupts DSI thresholds.
- [High] Space domain unit mismatch (Bytes vs Buffer Units) in QoS threshold calculations bypasses physical FIFO limits.
--

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

Add support for the new DSI IP found in the Kompanio Ultra MT8196 SoC.
This introduces the initialization of QoS parameters and can make use
of a DSI SRAM reserved buffer.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 2df2a9d5ec038..5af44c13bd5ac 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -753,6 +849,89 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
> +static void mtk_dsi_config_hw_buffers(struct mtk_dsi *dsi)
> +{
[ ... ]
> +	data_rate_per_buf = dsi->data_rate * dsi->lanes / 8 / buffer_unit;

[Severity: High]
Will this calculation overflow 32-bit arithmetic for typical displays?
Since dsi->data_rate is in Hz, a bandwidth over 1 Gbps combined with 4 lanes
will exceed the u32 limit before the division.

Also, is it intentional that data_rate_per_buf is calculated in units per
second instead of units per microsecond? Later in mtk_dsi_config_hw_buffers(),
it is mixed with microsecond constants and fill_rate.

> +
> +	/* Calculate valid threshold and avoid exceeding FIFO size */
> +	out_valid_thresh = MTK_DSI_DEFAULT_QOS_VALID_FIFO_US * data_rate_per_buf;
> +	out_valid_thresh = min(out_valid_thresh, MTK_DSI_MAX_FIFO_BYTES - 1);

[Severity: High]
Does this min() correctly clamp the threshold? out_valid_thresh appears to be
calculated in buffer units, but it is being clamped against a limit specified
in bytes (MTK_DSI_MAX_FIFO_BYTES - 1, which is 1553). For an MT8196 where a
buffer unit is 32 bytes, this would allow up to 1553 buffer units (49,696
bytes), bypassing the intended physical FIFO limits.

[ ... ]
> +	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;

[Severity: High]
Is this mixing bytes and buffer units? MTK_DSI_MAX_FIFO_BYTES is in bytes,
but it seems to be multiplied by the number of hardware buffers here instead
of a byte-based metric.

> +	sodi_hi -= (fill_rate - data_rate_per_buf) * 12 / 10;

[Severity: High]
Could this subtraction cause an integer underflow? fill_rate is calculated
using pclk_mhz so it is a per-microsecond rate, while data_rate_per_buf is
a per-second rate. Subtracting a large per-second rate from a small
per-microsecond rate will wrap around.

> +	sodi_lo = (23 + 5) * data_rate_per_buf;
[ ... ]

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