Re: [PATCH v2 11/13] drm/rockchip: dw_hdmi_qp: Support 10-bit YUV422 output format

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `atomic_check` callback incorrectly modifies driver private hardware state (`hdmi->tmds_char_rate`) and invokes PHY configuration functions (`phy_configure`), violating the stateless nature of atomic checks.
--

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

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

This commit adds support for 10-bit YUV 4:2:2 output to the DW HDMI QP
encoder by configuring the PHY with 8 bpc when YUV 4:2:2 is in use. This
keeps the 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
> @@ -215,6 +215,7 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
>  		s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
>  		break;
>  	case MEDIA_BUS_FMT_UYVY8_1X16:
> +	case MEDIA_BUS_FMT_UYVY10_1X20:
>  		s->output_mode = ROCKCHIP_OUT_MODE_YUV422;
>  		break;
>  	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> @@ -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) {
>  		hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;

[Severity: High]
This isn't a bug introduced by this patch, but does this violate the DRM
atomic API contract?

Modifying physical PHY hardware state and persistent driver state
(hdmi->tmds_char_rate) during the stateless atomic check phase could cause
problems if userspace issues a test-only atomic commit.

If a compositor issues DRM_MODE_ATOMIC_TEST_ONLY, phy_configure() will program
the PHY hardware and hdmi->tmds_char_rate will be mutated. When the test-only
commit is discarded, the PHY is left in an unexpected state.

A subsequent real commit might then hit the fast-path check earlier in this
function:

    if (hdmi->tmds_char_rate == conn_state->hdmi.tmds_char_rate &&
        s->output_bpc == conn_state->hdmi.output_bpc &&
        s->bus_format == ingest_fmt)
            return 0;

and skip PHY configuration entirely, leaving the hardware incorrectly
configured.

Could this lead to hardware misconfiguration, visual glitches, or black
screens?

>  		s->output_type = DRM_MODE_CONNECTOR_HDMIA;
>  		s->output_bpc = conn_state->hdmi.output_bpc;
>  		s->bus_format = ingest_fmt;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-dw-hdmi-qp-yuv-v2-0-b91beb33dedf@collabora.com?part=11