Am 27.05.26 um 17:10 schrieb [email protected]:
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.
Unrelated issues should be fixed separately.
--
commit da86f07c371224a54756c1f750c01c663a2a7a86
Author: Thomas Zimmermann <[email protected]>
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);
}
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
