Re: [PATCH v4 13/14] drm/rockchip: dw_hdmi_qp: Enable YUV420 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] Hardware PHY is programmed and driver software state is mutated during the DRM atomic_check phase.
--

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

drm/rockchip: dw_hdmi_qp: Enable YUV420 output format

Both RK3576 and RK3588 SoCs are capable of driving the YUV420 output
color format, and the required bus-format handling and VOP2 support are
already in place.  Advertise it via the platform supported formats so
the HDMI core can select it.

Tested-by: Igor Paunovic <royalnet026@gmail.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 78473f9a0fce3..1af29066a0a31 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -611,7 +611,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  
>  	plat_data.supported_formats = BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) |
>  				      BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) |
> -				      BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422);
> +				      BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) |
> +				      BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how the
atomic state machine is handled.

In dw_hdmi_qp_rockchip_encoder_atomic_check():

	ret = phy_configure(hdmi->phy, &phy_cfg);
	if (!ret) {
		hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
		s->output_type = DRM_MODE_CONNECTOR_HDMIA;
		s->output_bpc = conn_state->hdmi.output_bpc;
		s->bus_format = ingest_fmt;

Does this actively program the hardware PHY and mutate global driver state
(hdmi->tmds_char_rate) during the DRM atomic_check phase?

If userspace issues a DRM atomic test-only commit, it appears this will
overwrite hdmi->tmds_char_rate. Later, when the actual atomic commit is
performed with the same configuration, the early-return condition at the
beginning of the function will evaluate to true:

	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;

Will this cause the driver to incorrectly skip configuring the PHY during the
actual commit phase?

Also, could calling phy_configure() outside of the enable paths bypass
expected runtime PM references, risking bus faults if the block is suspended?

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