Re: [PATCH v3 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling sleeping functions from CRTC atomic enable and disable callbacks violates atomic context constraints.
- [High] Setting `cstate->mode_changed = true` inside the plane's `atomic_check` callback bypasses downstream modeset routing, leaving bridges active during CRTC resets.
--

--- Patch [7]: [PATCH v3 7/8] drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit c9e2ab55e3b69f124e64f349a1ced762885b2aea
Author: Ze Huang <ze.huang@oss.qualcomm.com>
Subject: 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.

> diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
> index 26b6c65ef6fd0..beb3a5a63b5d5 100644
> --- a/drivers/gpu/drm/tve200/tve200_display.c
> +++ b/drivers/gpu/drm/tve200/tve200_display.c
[ ... ]
> @@ -69,13 +71,34 @@ irqreturn_t tve200_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static int tve200_display_check(struct drm_simple_display_pipe *pipe,
> -			       struct drm_plane_state *pstate,
> -			       struct drm_crtc_state *cstate)
> +static int tve200_plane_helper_atomic_check(struct drm_plane *plane,
> +					    struct drm_atomic_commit *commit)
>  {

[Severity: High]
This is a pre-existing issue, but does setting cstate->mode_changed = true
inside the plane check hook bypass downstream modeset routing?

In tve200_plane_helper_atomic_check(), the code raises a mode change if the
framebuffer format changes:

    if (old_fb && old_fb->format != fb->format)
            cstate->mode_changed = true;

In the default DRM state check sequence, plane checks are executed after the
modeset checks. Because mode_changed is raised late and the driver does not
explicitly loop back to recalculate modeset routing, the downstream
encoders and bridges may miss their disable/enable hooks. Could this leave
bridges active during a CRTC reset?

[ ... ]
> @@ -119,16 +142,72 @@ static int tve200_display_check(struct drm_simple_display_pipe *pipe,
>  	return 0;
>  }
>  
[ ... ]
> +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;

[Severity: High]
This is a pre-existing issue, but does this code sleep in atomic context?

The DRM core invokes CRTC atomic enable callbacks during display commits,
which run in atomic context. However, tve200_crtc_helper_atomic_enable()
calls sleeping functions:

    clk_prepare_enable(priv->clk);
    ...
    usleep_range(30000, 50000);

I see this logic was carried over directly from the old
drm_simple_display_pipe hooks. Since sleeping in atomic context can cause
kernel warnings and potential deadlocks, is there a safe way to handle this
enable sequence?

[ ... ]
> @@ -240,9 +319,9 @@ static void tve200_display_enable(struct drm_simple_display_pipe *pipe,
>  	drm_crtc_vblank_on(crtc);
>  }
>  
> -static void tve200_display_disable(struct drm_simple_display_pipe *pipe)
> +static void tve200_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					      struct drm_atomic_commit *commit)
>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
>  	struct drm_device *drm = crtc->dev;
>  	struct tve200_drm_dev_private *priv = drm->dev_private;

[Severity: High]
This is a pre-existing issue, but similar to the enable path above, does the
disable callback also sleep in atomic context?

tve200_crtc_helper_atomic_disable() eventually calls clk_disable_unprepare(),
which can sleep:

    clk_disable_unprepare(priv->clk);

Since this runs in the atomic disable path, could this lead to a deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-drm-simple-kms-removal-v3-0-cd5dc89858c6@oss.qualcomm.com?part=7