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 <[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); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
