Re: [PATCH v5 07/11] drm/mediatek: mtk_dsi: Add support for MT8189

[sashiko-bot@kernel.org]

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

New issues:
- [High] Bandwidth check and PHY clock calculations do not account for DSC compression, incorrectly rejecting high-resolution modes and over-clocking the PHY.

Pre-existing issues:
- [High] Error path in `mtk_dsi_poweron` fails to decrement `refcount`, permanently leaving the DSI driver in a corrupted state.
- [High] Signal interruptions during IRQ waits are incorrectly treated as successful operations.
--

--- Patch [7]: [PATCH v5 07/11] drm/mediatek: mtk_dsi: Add support for MT8189 ---
commit b1ac2564850b4d8fbc42fa9bf9284c320bec103e
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
drm/mediatek: mtk_dsi: Add support for MT8189

This commit adds support for the DSI IP in the MT8189 SoC, which supports a
higher link rate of 2.5Gbps. It introduces a max_link_rate_mbps member to
handle SoC-specific link rates dynamically instead of using a hardcoded
value, and casts calculations to u64 to prevent overflows.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index c5db63b93ff91..3f58683be70fa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -926,7 +928,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	}
>  	bit_per_pixel = ret;
>  

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the
reference count?

In mtk_dsi_poweron(), dsi->refcount is incremented at the start of the
function. If mipi_dsi_pixel_format_to_bpp() fails, the function returns
directly instead of jumping to the err_refcount label:

static int mtk_dsi_poweron(struct mtk_dsi *dsi)
{
        ...
        if (++dsi->refcount != 1)
                return 0;

        ret = mipi_dsi_pixel_format_to_bpp(dsi->format);
        if (ret < 0) {
                dev_err(dev, "Unknown MIPI DSI format %d\n", dsi->format);
                return ret;
        }

Would this leave the DSI driver in a state where subsequent calls incorrectly
assume the hardware is already powered on?

> -	dsi->data_rate = DIV_ROUND_UP_ULL(dsi->vm.pixelclock * bit_per_pixel,
> +	dsi->data_rate = DIV_ROUND_UP_ULL((u64)dsi->vm.pixelclock * bit_per_pixel,
>  					  dsi->lanes);

[Severity: High]
Does this PHY clock calculation need to account for DSC compression?

By using the uncompressed bit_per_pixel here, could this clock the PHY at
the uncompressed data rate instead of the compressed DSC rate, potentially
over-clocking the PHY and breaking the DSC feature?

>  
>  	ret = clk_set_rate(dsi->hs_clk, dsi->data_rate);
[ ... ]
> @@ -1101,13 +1103,21 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  			  const struct drm_display_mode *mode)
>  {
>  	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	u64 wanted_link_rate, max_link_rate;
>  	int bpp;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0)
>  		return MODE_ERROR;
>  
> -	if (mode->clock * bpp / dsi->lanes > 1500000)
> +	wanted_link_rate = mode->clock;
> +	wanted_link_rate *= bpp;

[Severity: High]
Similarly, does the wanted_link_rate calculation fail to account for the
DSC bit rate?

If bpp remains the uncompressed value (e.g., 24 for RGB888) instead of
the compressed rate from dsi->dsc->bits_per_pixel, will this incorrectly
reject valid high-resolution DSC modes by returning MODE_CLOCK_HIGH?

> +	max_link_rate = data->max_link_rate_mbps;
> +	max_link_rate *= dsi->lanes;
> +	max_link_rate *= KILO;
> +
> +	if (wanted_link_rate > max_link_rate)
>  		return MODE_CLOCK_HIGH;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but are signal interruptions
incorrectly treated as successes during IRQ waits?

In mtk_dsi_wait_for_irq_done(), wait_event_interruptible_timeout() can
return -ERESTARTSYS if interrupted by a signal.

However, both mtk_dsi_host_send_cmd() and mtk_dsi_switch_to_cmd_mode()
evaluate this negative error code with a logical NOT:

drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_host_send_cmd()
{
        ...
        if (!mtk_dsi_wait_for_irq_done(dsi, flag, 2000))
                return -ETIME;
        else
                return 0;
}

Since -ERESTARTSYS is non-zero, the condition evaluates to false, taking
the else branch and returning 0 (success). Will this cause the driver
to silently ignore interrupted hardware operations and incorrectly assume
state changes have completed?

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