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 <[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. Tested-by: Igor Paunovic <[email protected]> Signed-off-by: Cristian Ciocaltea <[email protected]> > 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/[email protected]?part=13
