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

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

On Thu Jul 16, 2026 at 5:21 PM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Missing CRTC active check in the plane's atomic_update callback causes the driver to send SPI framebuffer updates to a powered-off display panel.
>
> Pre-existing issues:
> - [High] The CRTC atomic enable/disable and plane atomic update callbacks execute sleeping functions and allocate memory with GFP_KERNEL, which is forbidden in atomic context.
> --
>
> --- Patch [6]: [PATCH v2 6/8] drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers ---
> commit 3d9748839d37276a557c720301a5a848296e54b7
> Author: Ze Huang <ze.huang@oss.qualcomm.com>
>
> drm/repaper: replace struct drm_simple_display_pipe with regular atomic helpers
>     
> Convert repaper to explicit primary plane, CRTC and encoder objects.
> Keep shadow-plane helpers, framebuffer access handling and no-scaling
> plane-state validation from simple-KMS path.
>
>> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
>> index c8270591afc7f..7057075161c4d 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);
>
> [Severity: High]
> This is a pre-existing issue, but do the CRTC atomic enable/disable and plane
> atomic update callbacks execute sleeping functions in atomic context?
>
> The DRM subsystem explicitly forbids sleeping in CRTC enable/disable and plane
> update callbacks. However:
> - repaper_crtc_helper_atomic_enable() calls usleep_range()
> - repaper_crtc_helper_atomic_disable() calls msleep()
> - repaper_plane_helper_atomic_update() calls repaper_fb_dirty(), which
>   allocates memory using GFP_KERNEL
>
> Since these operations are now wired directly to the standard atomic helpers
> which run in atomic context (for example, during non-blocking commits), can
> this cause system instability or deadlocks?
>

I think it is a false positive.

>
> [ ... ]
>
>> -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_rect rect;
>>  
>> -	if (!pipe->crtc.state->active)
>> +	if (!pstate->fb)
>>  		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]
> Can this cause the driver to send SPI framebuffer updates to a powered-off
> display panel?
>
> The previous code checked !pipe->crtc.state->active here to ensure we
> do not update inactive displays. During a DPMS OFF request,
> drm_atomic_helper_commit_modeset_disables() runs first and disables the
> panel by calling repaper_crtc_helper_atomic_disable(). 
>
> Next, drm_atomic_helper_commit_planes() will still call this update
> function because active_only is not set by default and pstate->visible
> can remain true.
>
> Without the crtc_state->active check, doesn't this result in writing data
> to dead hardware via repaper_fb_dirty(), potentially causing SPI timeouts
> or driver hangs?
>

OK, I'll restore crtc state check here.

>>  }
>
> [ ... ]