Re: [PATCH 1/4] drm/rockchip: lvds: propagate bus_flags to the CRTC state

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly introduced bus_flags assignment bypasses DRM bridge state negotiation by reading flags directly from the final connector instead of the adjacent bridge.

Pre-existing issues:
- [High] The driver unconditionally overwrites the function pointers and private data of external bridge objects, causing type confusion and completely breaking external bridge support.
--

--- Patch [1]: [PATCH 1/4] drm/rockchip: lvds: propagate bus_flags to the CRTC state ---
commit e03a59004874598201254d64c0f6e0e88de3ff1a
Author: Rok Markovic <rok@kanardia.eu>

drm/rockchip: lvds: propagate bus_flags to the CRTC state

This commit updates the Rockchip LVDS encoder's atomic_check function to
propagate bus_flags from the connector's display_info to the CRTC state.
This ensures that the VOP can correctly set the pixel clock polarity based
on the panel's requirements.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 7a0c4fa29f2f0..150299c12298a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -129,7 +129,15 @@ rockchip_lvds_encoder_atomic_check(struct drm_encoder *encoder,
>  				   struct drm_connector_state *conn_state)
>  {
>  	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> -
> +	struct drm_connector *connector = conn_state->connector;
> +
> +	/*
> +	 * The VOP derives the pixel clock polarity from this. Without it a
> +	 * panel that declares pixelclk-active = <0> is clocked on the wrong
> +	 * edge. panel-lvds fills the connector's bus_flags in from the DT
> +	 * display timing.
> +	 */
> +	s->bus_flags = connector->display_info.bus_flags;

[Severity: Medium]
Does this bypass DRM bridge state negotiation by reading flags directly from
the final connector instead of the adjacent bridge?

By reading from connector->display_info.bus_flags in
rockchip_lvds_encoder_atomic_check(), the VOP pixel clock polarity is
configured based on the final sink (e.g., an HDMI monitor) rather than the
LVDS bridge's input requirements.

This causes incorrect pixel clock polarity for external bridge setups, as it
violates DRM bridge chaining contracts which expect the state to be retrieved
via drm_atomic_get_new_bridge_state().

>  	s->output_mode = ROCKCHIP_OUT_MODE_P888;
>  	s->output_type = DRM_MODE_CONNECTOR_LVDS;

[Severity: High]
This isn't a bug introduced by this patch, but does the driver unconditionally
overwrite the function pointers and private data of external bridge objects in
rockchip_lvds_bind()?

When a valid external bridge is present, the initialization path overwrites
the bridge data:

rockchip_lvds_bind() {
    ...
    if (lvds->bridge) {
        lvds->bridge->driver_private = lvds;
        lvds->bridge->ops = DRM_BRIDGE_OP_MODES;
        lvds->bridge->funcs = &rockchip_lvds_bridge_funcs;
    ...
}

Does this hijack the external bridge by replacing its callbacks and private
data with LVDS-specific values?

This causes type confusion and prevents external bridges from ever being
enabled, since their callbacks are replaced by the LVDS functions. It also
seems likely to cause panics if the external bridge driver attempts to access
its original driver_private data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717120005.2087386-1-rok@kanardia.eu?part=1