Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Hardware State Mutation in Atomic Check -- commit 88f697e7fc21ab15ca1c009f0f830ab90e6ec4e7 Author: Cristian Ciocaltea <[email protected]> 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. > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > index 3a1c027aa90bb..e29522afc6f05 100644 > --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > @@ -612,7 +612,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 that adding new formats might exercise the atomic check path more frequently. Looking at dw_hdmi_qp_rockchip_encoder_atomic_check() in this same file, it appears to mutate hardware state during the atomic validation phase: drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c: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; } else { ... } And in the PHY driver, phy_configure() mutates global state: drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c:rk_hdptx_phy_configure() { ... ret = rk_hdptx_phy_verify_hdmi_config(hdptx, &opts->hdmi, &hdptx->hdmi_cfg); if (ret) { dev_err(hdptx->dev, "invalid hdmi params for phy configure\n"); } else { hdptx->restrict_rate_change = true; ... } Does this sequence cause problems when a userspace application (like a display compositor) submits a test-only commit using the DRM_MODE_ATOMIC_TEST_ONLY flag? Since the atomic check runs for test-only commits, won't this corrupt the active driver state and potentially break subsequent real commits if they skip necessary hardware configurations due to the cached state already matching? > > encoder = &hdmi->encoder.encoder; > encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
