Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Directly accessing `priv->plane.state` in the CRTC atomic enable callback introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition with subsequent non-blocking commits. - [High] Missing `.cleanup_fb` callback in `tve200_plane_helper_funcs` leads to memory and resource leaks of pinned GEM objects and DMA-buf mappings. Pre-existing issues: - [High] The CRTC atomic enable and disable callbacks call functions that can sleep (`clk_prepare_enable`, `usleep_range`), violating DRM atomic context guidelines. -- --- Patch [7]: [PATCH v2 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers --- commit 30e007b0736d27ee72ba4a5f39e203c3808e470e Author: Ze Huang <[email protected]> drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers Convert TVE200 to explicit plane, CRTC and encoder objects. Keep generic plane-state validation before TVE200-specific mode, alignment, pitch and format-change checks, and install GEM framebuffer prepare helper explicitly. > diff --git a/drivers/gpu/drm/tve200/tve200_display.c > b/drivers/gpu/drm/tve200/tve200_display.c > --- a/drivers/gpu/drm/tve200/tve200_display.c > +++ b/drivers/gpu/drm/tve200/tve200_display.c [ ... ] > @@ -119,16 +140,72 @@ static int tve200_display_check(struct > drm_simple_display_pipe *pipe, [ ... ] > +static const struct drm_plane_helper_funcs tve200_plane_helper_funcs = { > + .prepare_fb = drm_gem_plane_helper_prepare_fb, > + .atomic_check = tve200_plane_helper_atomic_check, > + .atomic_update = tve200_plane_helper_atomic_update, > +}; [Severity: High] Since .prepare_fb is assigned to drm_gem_plane_helper_prepare_fb, will the lack of a corresponding .cleanup_fb callback lead to memory and resource leaks of pinned GEM objects and DMA-buf mappings when the framebuffer is swapped out? [ ... ] > -static void tve200_display_enable(struct drm_simple_display_pipe *pipe, > - struct drm_crtc_state *cstate, > - struct drm_plane_state *plane_state) > +static void tve200_crtc_helper_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_commit *commit) > { > - struct drm_crtc *crtc = &pipe->crtc; > - struct drm_plane *plane = &pipe->plane; > struct drm_device *drm = crtc->dev; > struct tve200_drm_dev_private *priv = drm->dev_private; > + struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, > crtc); > const struct drm_display_mode *mode = &cstate->mode; > - struct drm_framebuffer *fb = plane->state->fb; > + struct drm_plane_state *plane_state = priv->plane.state; > + struct drm_framebuffer *fb = plane_state->fb; [Severity: High] Could this direct access to priv->plane.state introduce a race condition with subsequent non-blocking commits? A fast sequence of commits might swap the global state before a previous commit's worker executes this function. Would it be safer to extract the current commit's plane state using drm_atomic_get_new_plane_state(commit, &priv->plane) instead to guarantee it reads the correct snapshot? [Severity: High] This is a pre-existing issue, but does tve200_crtc_helper_atomic_enable() call sleeping functions in an atomic context? Looking further down in this function, it calls clk_prepare_enable() and usleep_range(). Similarly, tve200_crtc_helper_atomic_disable() calls clk_disable_unprepare(). Since atomic commit callbacks can run in an atomic context where sleeping is forbidden, could this cause system instability or deadlocks? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3...@oss.qualcomm.com?part=7
