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

[sashiko-bot@kernel.org]

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?

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?

[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?

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=8