Re: [PATCH v3] drm/tiny: arcpgu: Replace 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] Incorrect usage of `drm_atomic_get_new_*_state` causes valid partial atomic commits to be rejected with `-EINVAL` and leads to potential hardware misconfiguration.

Pre-existing issues:
- [High] Sleeping clock functions are called from CRTC atomic enable/disable paths, violating atomic context constraints.
- [High] Hardware DMA use-after-free on page flip due to lack of hardware VBLANK synchronization.
--

commit 56c718e0d1deb02575937fad72869dc5fbfabbaa
Author: Davide Bonatto <bonatto.davide@gmail.com>

drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers

The driver is being migrated from the deprecated drm_simple_display_pipe
helper to explicitly managed plane, CRTC, and encoder objects using standard
atomic helpers.

> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..b66b9562df10d 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
> -			   struct drm_crtc_state *crtc_state,
> -			   struct drm_plane_state *plane_state)
> +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
> +				       struct drm_atomic_commit *state)
>  {
> -	struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> +	struct drm_plane_state *plane_state =
> +		drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> +	const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;

[Severity: High]
Does this skip essential pixel format programming during a CRTC enable if the
plane isn't part of the commit state?

If a userspace compositor requests a CRTC-only DPMS state change,
drm_atomic_get_new_plane_state() will return NULL for the plane state, leading
to a NULL fb and skipping the pixel format programming entirely.

> -	arc_pgu_mode_set(arcpgu);
> +	arc_pgu_mode_set(arcpgu, fb);
>  
>  	clk_prepare_enable(arcpgu->clk);

[Severity: High]
This is a pre-existing issue, but does this execute a sleeping clock framework
function inside an atomic context?

According to DRM subsystem guidelines, CRTC atomic enable callbacks execute in
an atomic context where sleeping functions are forbidden. Calling
clk_prepare_enable() here can cause kernel warnings and potential deadlocks.

>  	arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
>  		      arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
> -		      ARCPGU_CTRL_ENABLE_MASK);
> +		ARCPGU_CTRL_ENABLE_MASK);
>  }
>  
> -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
> +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
> +					struct drm_atomic_commit *state)
>  {
> -	struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>  
>  	clk_disable_unprepare(arcpgu->clk);

[Severity: High]
This is a pre-existing issue, but does this execute a sleeping clock framework
function inside an atomic context?

Similar to the enable path, arc_pgu_crtc_atomic_disable() executes in an
atomic context. Calling clk_disable_unprepare() here acquires a mutex and
sleeps, which is not permitted.

[ ... ]
> -static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
> -			   struct drm_plane_state *state)
> +static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
> +					struct drm_atomic_commit *state)
>  {
> -	struct arcpgu_drm_private *arcpgu;
> +	struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>  	struct drm_gem_dma_object *gem;
>  
> -	if (!pipe->plane.state->fb)
> +	if (!new_plane_state->fb)
>  		return;
>  
> -	arcpgu = pipe_to_arcpgu_priv(pipe);
> -	gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
> +	gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
>  	arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);

[Severity: High]
This is a pre-existing issue inherited from the simple display pipe setup,
but can this lead to a hardware DMA use-after-free on page flip?

Because the driver lacks VBLANK interrupts, the new DMA address is committed
to hardware here without a mechanism to delay freeing the previous buffer. The
DRM core simulates a VBLANK and immediately unpins/frees the old memory, but
the physical hardware won't latch this new address until its next real VBLANK.
The hardware could continue scanning from the now-freed memory buffer.

[ ... ]
> +static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
> +				      struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (!new_plane_state->crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> +	if (!crtc_state)
> +		return -EINVAL;

[Severity: High]
Does this incorrectly reject valid plane-only updates?

If a userspace compositor requests a non-modesetting atomic update (such as
only updating the plane's FB), the CRTC state is not natively included in the
atomic commit state. drm_atomic_get_new_crtc_state() will return NULL, causing
this function to fail with -EINVAL and breaking userspace display servers.

[ ... ]
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> +	struct drm_plane_state *plane_state;
> +
> +	if (!crct_state->enable)
> +		return 0;
> +
> +	plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> +	if (!plane_state || !plane_state->fb)
> +		return -EINVAL;

[Severity: High]
Will this fail valid CRTC-only updates?

Similar to the plane check, if a userspace compositor requests an update to
a CRTC property (e.g., active state) without including the plane in the
commit request, plane_state will evaluate to NULL and cause the driver to
incorrectly reject the partial atomic commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909104332.160612-1-bonatto.davide@gmail.com?part=1