Re: [PATCH v3 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Sleeping functions and blocking memory allocations are called from within CRTC and plane atomic callbacks, violating atomic context constraints.
--

--- Patch [6]: [PATCH v3 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit c95d4b930f0cb70a4eb89a2c19785d09216e66c1
Author: Ze Huang <[email protected]>

drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers

This commit converts the repaper driver to use explicit primary plane, CRTC,
and encoder objects instead of the simple display pipe helpers. It retains
the shadow-plane helpers, framebuffer access handling, and no-scaling
plane-state validation from the simple-KMS path.

> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index c8270591afc7f..f225e38fd4045 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c

[ ... ]

> -static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
> -				struct drm_crtc_state *crtc_state,
> -				struct drm_plane_state *plane_state)
> +static void repaper_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					      struct drm_atomic_commit *commit)
>  {
> -	struct repaper_epd *epd = drm_to_epd(pipe->crtc.dev);
> +	struct repaper_epd *epd = drm_to_epd(crtc->dev);
>  	struct spi_device *spi = epd->spi;
>  	struct device *dev = &spi->dev;
>  	bool dc_ok = false;
>  	int i, ret, idx;
>  
> -	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +	if (!drm_dev_enter(crtc->dev, &idx))
>  		return;

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

CRTC atomic enable callbacks execute in atomic context where sleeping is
forbidden. However, looking at the code, this function makes direct calls to
sleeping functions:

    gpiod_set_value_cansleep(epd->reset, 0);
    ...
    usleep_range(5000, 10000);

Could this cause system instability or kernel panics during display state
updates?

[ ... ]

> -static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void repaper_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					       struct drm_atomic_commit *commit)
>  {
> -	struct repaper_epd *epd = drm_to_epd(pipe->crtc.dev);
> +	struct repaper_epd *epd = drm_to_epd(crtc->dev);
>  	struct spi_device *spi = epd->spi;
>  	unsigned int line;

[Severity: High]
This is a pre-existing issue, but similar to the enable callback, is it safe
for this atomic disable callback to sleep?

It calls functions that can sleep:

    msleep(25);
    gpiod_set_value_cansleep(epd->border, 0);
    msleep(200);
    ...
    power_off(epd);

[ ... ]

> -static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
> -				struct drm_plane_state *old_state)
> +static void repaper_plane_helper_atomic_update(struct drm_plane *plane,
> +					       struct drm_atomic_commit *commit)
>  {
> -	struct drm_plane_state *state = pipe->plane.state;
> -	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
> +	struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(commit, plane);
> +	struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(pstate);
> +	struct drm_crtc_state *cstate = NULL;
>  	struct drm_rect rect;
>  
> -	if (!pipe->crtc.state->active)
> +	if (pstate->crtc)
> +		cstate = drm_atomic_get_new_crtc_state(commit, pstate->crtc);
> +
> +	if (!cstate || !cstate->active)
>  		return;
>  
> -	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> -		repaper_fb_dirty(state->fb, shadow_plane_state->data,
> +	if (drm_atomic_helper_damage_merged(old_pstate, pstate, &rect))
> +		repaper_fb_dirty(pstate->fb, shadow_plane_state->data,
>  				 &shadow_plane_state->fmtcnv_state);
>  }

[Severity: High]
This is a pre-existing issue, but does repaper_fb_dirty() perform blocking
memory allocations in atomic context?

The plane atomic update callback must not sleep, but repaper_fb_dirty()
uses GFP_KERNEL for memory allocation:

    buf = kmalloc(fb->width * fb->height / 8, GFP_KERNEL);

Should this allocation be moved outside of the atomic update path or use a
non-blocking flag?

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