Re: [PATCH v4 1/2] phy: rockchip: inno-hdmi: Add configure() and validate() ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] inno_hdmi_phy_validate() performs an incomplete validation that allows unsupported (pixclock, tmdsclock) combinations to pass DRM's atomic_check, causing fatal failures during atomic_enable.
--

commit 09a1c2d66dc86a8ff3ab2a20274166576f00f813
Author: Jonas Karlman <jonas@kwiboo.se>

phy: rockchip: inno-hdmi: Add configure() and validate() ops

This commit introduces phy_configure() and phy_validate() operations for
the Inno HDMI PHY driver. It uses the TMDS character rate to validate and
configure the PHY, falling back to bus width if needed.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> --- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c

[ ... ]

> @@ -608,6 +608,57 @@ static irqreturn_t inno_hdmi_phy_rk3328_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * phy_validate() is expected to be called from encoder atomic_check(), before
> + * the hdmiphy pixel clock is known. Without knowing the actual pixel clock, we
> + * cannot do full validation of the configuration. Instead, we do a simple check
> + * that the pre-pll table contains an entry for the requested TMDS char rate.
> + */
> +static int inno_hdmi_phy_validate(struct phy *phy, enum phy_mode mode,
> +				  int submode, union phy_configure_opts *opts)
> +{
> +	const struct pre_pll_config *cfg = pre_pll_cfg_table;
> +	unsigned long tmdsclock;
> +
> +	if (!(mode == PHY_MODE_HDMI && submode == PHY_HDMI_MODE_TMDS))
> +		return -EINVAL;
> +
> +	if (!opts)
> +		return -EINVAL;
> +
> +	if (!opts->hdmi.tmds_char_rate || opts->hdmi.tmds_char_rate > 594000000)
> +		return -EINVAL;
> +
> +	tmdsclock = opts->hdmi.tmds_char_rate;
> +	for (; cfg->pixclock != 0; cfg++)
> +		if (cfg->tmdsclock == tmdsclock)
> +			return 0;
> +
> +	return -EINVAL;
> +}

Could this incomplete validation allow unsupported pixclock and tmdsclock
combinations to pass DRM's atomic_check, leading to fatal failures later
during atomic_enable?

The DRM atomic API requires hardware constraints to be verified during
atomic_check. If pixclock is validated independently elsewhere (such as via
clk_round_rate), a mismatched pair might pass both checks.

For example, if a mode requests a pixclock of 54MHz with a 16-bit color depth
(which yields a tmdsclock of 108MHz), atomic_check would pass because 54MHz
exists in the pixclock column and 108MHz exists in the tmdsclock column.

However, later during atomic_enable, clk_set_rate() calls
inno_hdmi_phy_get_pre_pll_cfg() which searches for the exact pair:

inno_hdmi_phy_get_pre_pll_cfg() {
...
	for (; cfg->pixclock != 0; cfg++)
		if (cfg->pixclock == rate && cfg->tmdsclock == tmdsclock)
			break;

	if (cfg->pixclock == 0)
		return ERR_PTR(-EINVAL);
...
}

Since the specific pair 54MHz and 108MHz does not exist in the table together,
it returns -EINVAL, causing the modeset to abort midway and leaving the
display pipeline broken.

Can the exact pixclock and tmdsclock combination be verified here instead,
perhaps by deriving pixclock from opts->hdmi.tmds_char_rate and the color
depth?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518180722.2480799-1-jonas@kwiboo.se?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy