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

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

On Thu Jul 16, 2026 at 5:25 PM CST, sashiko-bot wrote:
> 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?

The documentation for drm_gem_plane_helper_prepare_fb() [1] explicitly says
that simple GEM framebuffer drivers do not need a matching cleanup_fb hook.

[1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_gem_atomic_helper.c#L107

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

Will do

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

It is a false positive.