Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The `atomic_check` callback unconditionally mutates global driver state and configures hardware PHY, breaking DRM atomic design principles and corrupting state during `TEST_ONLY` commits. -- --- Patch [12]: [PATCH v3 12/14] drm/rockchip: dw_hdmi_qp: Support 10-bit YUV422 output format --- commit 4acf186e6f48344c73efb358bbf5b17c6a065aa5 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: dw_hdmi_qp: Support 10-bit YUV422 output format This commit adds support for 10-bit YUV422 output via the MEDIA_BUS_FMT_UYVY10_1X20 format. It also forces 8 bpc for the HDMI PHY configuration when YUV 4:2:2 is in use to keep the PHY PLL output 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 > --- 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) { > hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate; [Severity: High] This is a pre-existing issue, but does dw_hdmi_qp_rockchip_encoder_atomic_check mutate active hardware state and global driver state during TEST_ONLY commits? Because atomic_check callbacks are executed when userspace issues a DRM_MODE_ATOMIC_TEST_ONLY ioctl to probe display configurations, calling phy_configure() here could actively alter the PHY hardware under an active display. Additionally, modifying hdmi->tmds_char_rate mutates global driver state even if the test commit is subsequently discarded by the kernel, potentially corrupting the active configuration. Should these side effects be deferred to an atomic_enable or atomic_update callback instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
