Re: [PATCH v3 07/10] drm/damage-helper: Remove old state from drm_atomic_helper_damage_merged()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the plane atomic update callback `gm12u320_pipe_update` executes a path that acquires a mutex. Plane atomic update callbacks run in atomic context during non-blocking commits, making the use of sleeping locks unsafe.
- [High] This problem wasn't introduced by this patch, but the plane atomic update callback `repaper_pipe_update` executes a path that performs a memory allocation with `GFP_KERNEL`. Plane atomic update callbacks run in atomic context, where allocations that may sleep are strictly prohibited.
--

commit da86f07c371224a54756c1f750c01c663a2a7a86
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/damage-helper: Remove old state from drm_atomic_helper_damage_merged()

This patch removes the unused old_state parameter from 
drm_atomic_helper_damage_merged() and updates all callers. It also cleans up
the old_plane_state variable in caller functions where it is no longer 
needed.

[ ... ]

> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -582,7 +582,7 @@ static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>  	struct drm_rect rect;
>  
> -	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> +	if (drm_atomic_helper_damage_merged(state, &rect))

[Severity: High]
This is a pre-existing issue, but does gm12u320_pipe_update() safely execute
in an atomic context?

This function acts as the simple display pipe's plane atomic update callback,
which runs in atomic context during non-blocking commits. However, it calls
gm12u320_fb_mark_dirty(), which acquires a mutex:

drivers/gpu/drm/tiny/gm12u320.c:gm12u320_fb_mark_dirty() {
    ...
    mutex_lock(&gm12u320->fb_update.lock);
    ...
}

Since mutex_lock() is a sleeping function, could this lead to deadlocks or
kernel warnings when called from an atomic context?

>  		gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], &rect);
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -837,7 +837,7 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
>  	if (!pipe->crtc.state->active)
>  		return;
>  
> -	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> +	if (drm_atomic_helper_damage_merged(state, &rect))

[Severity: High]
This isn't a bug introduced by this patch, but is it safe for
repaper_pipe_update() to trigger allocations with GFP_KERNEL?

Like the issue above, this acts as the plane atomic update callback. It calls
repaper_fb_dirty(), which performs a memory allocation that may sleep:

drivers/gpu/drm/tiny/repaper.c:repaper_fb_dirty() {
    ...
    buf = kmalloc(fb->width * fb->height / 8, GFP_KERNEL);
    ...
    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
    ...
}

Because plane atomic update callbacks run in atomic context, allocations that
may sleep (like GFP_KERNEL) and sleeping functions like
drm_gem_fb_begin_cpu_access() are strictly prohibited. Is there a plan to
shift these sleeping operations to a workqueue or use non-sleeping
allocations?

>  		repaper_fb_dirty(state->fb, shadow_plane_state->data,
>  				 &shadow_plane_state->fmtcnv_state);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527145113.241595-1-tzimmermann@suse.de?part=7