Re: [PATCH v2 7/8] 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 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 <ze.huang@oss.qualcomm.com>
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-1133a8fc3785@oss.qualcomm.com?part=7