Re: [PATCH 8/9] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers

["Ze Huang" <ze.huang@oss.qualcomm.com>]

On Sun Jul 5, 2026 at 2:46 AM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Vblank event is leaked on CRTC disable and may be delivered prematurely on CRTC enable.
> - [Medium] Calling `drm_connector_attach_encoder()` on an uninitialized connector.
>
> Pre-existing issues:
> - [High] Calling `usleep_range()` from a CRTC `atomic_enable` callback.
> - [High] Direct, lockless access to global state pointers `plane->state` and `crtc->state` in atomic commit callbacks.
> --
>
> Note: The format of this review has been altered to a free-form summary due
> to mailing list quoting restrictions.
>
> Commit: 2c5e6508a64461d7a207075c4328681142076aef
> Author: Ze Huang <ze.huang@oss.qualcomm.com>
> Subject: drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers
> Summary: This commit converts the TVE200 driver to use explicit plane, CRTC,
> and encoder objects instead of the simple display pipe helpers, while keeping
> generic plane-state validation and GEM framebuffer preparation.
>
> [Severity: High]
> In drivers/gpu/drm/tve200/tve200_display.c,
> tve200_plane_helper_atomic_update() checks if the CRTC is NULL and returns
> early. Does this cause the pending vblank event to be leaked during a CRTC
> disable, since the plane is also disabled (making crtc NULL) and the event
> is never consumed?

Will fix it

>
> Additionally, since commit_planes() executes before commit_modeset_enables(),
> is the event processed and delivered prematurely on CRTC enable? Could the
> event consumption be moved to CRTC callbacks like atomic_enable,
> atomic_disable, or atomic_flush instead of handling it in the plane callback?
>

Yes, will move them to atomic_flush

> [Severity: Medium]
> In drivers/gpu/drm/tve200/tve200_drv.c, tve200_modeset_init() gets the
> connector using drm_panel_bridge_connector() and immediately passes it to
> drm_connector_attach_encoder(). Since drm_bridge_attach() has not run yet,
> is this modifying an uninitialized DRM object structure?
>
> Would it be better to rely on panel_bridge_attach(), which already
> automatically handles attaching the connector to the encoder, making this
> manual call redundant?

Will move drm_bridge_attach() before drm_panel_bridge_connector() and drop
drm_connector_attach_encoder() like:

    priv->panel = panel;
    priv->bridge = bridge;

    ret = drm_bridge_attach(&priv->encoder, bridge, NULL, 0);
    if (ret) {
    dev_err(dev->dev, "failed to attach bridge\n");
    goto out_bridge;
    }

    priv->connector = drm_panel_bridge_connector(bridge);
    if (!priv->connector) {
    ret = -ENODEV;
    goto out_bridge;
    }

>
> [Severity: High]
> This isn't a bug introduced by this patch, but in
> drivers/gpu/drm/tve200/tve200_display.c, tve200_crtc_helper_atomic_enable()
> contains a hardware polling loop that calls usleep_range(30000, 50000).
> Since CRTC atomic enable callbacks run in atomic context during non-blocking
> commits where sleeping is strictly forbidden, can this cause kernel warnings
> or deadlocks? Should this be replaced with non-sleeping delay functions like
> udelay or ndelay?
>
> [Severity: High]
> This is a pre-existing issue, but it appears to be expanded here. In
> drivers/gpu/drm/tve200/tve200_display.c, tve200_plane_helper_atomic_update()
> and tve200_crtc_helper_atomic_enable() directly access the global state
> pointers plane->state and priv->plane.state.
>
> In non-blocking commits, these global pointers can be swapped if userspace
> submits a concurrent commit. Does this create a TOCTOU race where a
> concurrent commit setting the framebuffer to NULL could lead to a NULL
> pointer dereference when accessing fb->format->format? Should this code use
> the proper drm_atomic_get_new_plane_state() (or equivalent) accessors on the
> passed state object instead?