Re: [PATCH v4 12/14] drm/rockchip: dw_hdmi_qp: Support 10-bit YUV422 output format

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Modifying hardware state and global driver state during the atomic_check phase.
- [High] drm_atomic_get_bridge_state() error code masking breaks wait/wound mutex backoff.
--

commit d0dd6a8692e00076c21b4fe6dcb51ded1a0bb7d0
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: dw_hdmi_qp: Support 10-bit YUV422 output format

This commit adds support for the 10-bit YUV422 output format to the Rockchip
DW HDMI QP encoder. It also configures the HDMI PHY with 8 bpc when YUV422
is in use to keep its output clock aligned with the TMDS character rate.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 9c4cf68d79d50..78473f9a0fce3 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -226,7 +227,14 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
>  	}
>  
>  	phy_cfg.hdmi.tmds_char_rate = conn_state->hdmi.tmds_char_rate;
> -	phy_cfg.hdmi.bpc = conn_state->hdmi.output_bpc;
> +	/*
> +	 * YUV422 always transmits two 12-bit components per clock cycle,
> +	 * regardless of the color depth, which from a rate perspective is
> +	 * equivalent to three 8-bit RGB components.  Force 8 bpc here to
> +	 * keep the PHY PLL output aligned with the TMDS character rate.
> +	 */
> +	phy_cfg.hdmi.bpc = (s->output_mode == ROCKCHIP_OUT_MODE_YUV422 ?
> +				8 : conn_state->hdmi.output_bpc);
>  
>  	ret = phy_configure(hdmi->phy, &phy_cfg);
>  	if (!ret) {

[Severity: High]
This isn't a bug introduced by this patch, but does calling phy_configure()
and modifying hdmi->tmds_char_rate inside atomic_check illegally commit
changes to global hardware tracking state?

In the DRM atomic KMS framework, atomic_check callbacks must be strictly
stateless and free of side-effects because they are frequently executed during
test-only commits intended purely to validate configurations. By calling
phy_configure() here, the driver incorrectly modifies physical hardware
registers during a test phase, which can cause display glitches, sync loss,
or hardware lockups.

Additionally, upon success, the function updates the global state:

	if (!ret) {
		hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
		s->output_type = DRM_MODE_CONNECTOR_HDMIA;

This permanently corrupts the driver's cached state. A subsequent real commit
may skip hardware configuration entirely because hdmi->tmds_char_rate matches
the requested rate, leaving the device out of sync and breaking display
output. This fundamentally breaks compositors that rely on test-only commits.


There is also another pre-existing issue earlier in the same function:

dw_hdmi_qp_rockchip_encoder_atomic_check() {
	...
	ingest_fmt = dw_hdmi_qp_rockchip_get_vop_format(encoder, conn_state);
	if (ingest_fmt < 0)
		return -EINVAL;
	...
}

[Severity: High]
This is a pre-existing issue, but does swallowing the error code here break
the wait/wound mutex backoff mechanism?

dw_hdmi_qp_rockchip_get_vop_format() calls drm_atomic_get_bridge_state(),
which uses wait/wound mutexes to lock bridge states and can return -EDEADLK
if the lock is contended. Returning -EDEADLK signals the DRM core to back
off, drop all locks, and retry the commit.

By checking if (ingest_fmt < 0) and returning a hardcoded -EINVAL, the
-EDEADLK error is swallowed. This breaks the DRM atomic commit WW mutex
backoff mechanism, causing valid userspace commits to fail spuriously with
-EINVAL under lock contention when multiple threads or processes attempt
atomic commits concurrently.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-dw-hdmi-qp-yuv-v4-0-fb45bf4147eb@collabora.com?part=12