Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/gm12u320: fb offset dropped due to redundant drm_gem_fb_vmap call - [High] drm/gm12u320: sleeping in atomic context via gm12u320_stop_fb_update - [Medium] drm/gm12u320: redundant logic for FB swaps improperly executes for invisible planes - [Medium] drm/tiny: commit message claims to fix EDID extension count but no code changes exist --
commit f388ed102067844063cbe52109b60321de9bfe2f Author: Hui Peng <[email protected]> drm/tiny: fix VRAM/EDID/transfer bounds in bochs, gm12u320, pixpaper, and sharp-memory This commit aims to fix framebuffer size, EDID extension count, and transfer buffer bounds checks across several tiny DRM drivers. > Fix framebuffer size, EDID extension count, and transfer buffer bounds > checks across drivers/gpu/drm/tiny/ (bochs.c, gm12u320.c, pixpaper.c, > and sharp-memory.c). [Severity: Medium] Is the commit message missing some context, or were the EDID extension count changes left out of this patch? The diff modifies framebuffer bounds and buffer maps, but it doesn't appear to contain any changes related to EDID parsing or EDID extension counts. > diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > index 4ad074337af07..9dfbfc6bb1b64 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c > @@ -268,12 +268,18 @@ static void gm12u320_copy_fb_to_blocks(struct > gm12u320_device *gm12u320) > x2 = gm12u320->fb_update.rect.x2; > y1 = gm12u320->fb_update.rect.y1; > y2 = gm12u320->fb_update.rect.y2; > - vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping > abstraction properly */ > + > + ret = drm_gem_fb_vmap(fb, &gm12u320->fb_update.src_map, NULL); [Severity: High] Does this redundant mapping overwrite the correct mapping provided by the shadow plane helper and drop the framebuffer offset? The shadow plane helpers already map the framebuffer and apply the offset into the shadow plane state data, which gets passed to gm12u320_fb_mark_dirty() and saved in gm12u320->fb_update.src_map. By calling drm_gem_fb_vmap() here in gm12u320_copy_fb_to_blocks() with a NULL data parameter, doesn't it bypass the offset calculation, causing the driver to read from the base GEM address instead of the correct offset address? [ ... ] > @@ -583,8 +592,17 @@ 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 (!state->fb) { > + gm12u320_stop_fb_update(to_gm12u320(pipe->crtc.dev)); > + return; > + } [Severity: High] Can this sequence cause us to sleep in an atomic context? gm12u320_pipe_update() implements the plane's atomic_update callback, which runs in atomic context where sleeping is forbidden. However, this patch introduces a call to gm12u320_stop_fb_update(), which directly calls cancel_delayed_work_sync() and mutex_lock() - both of which can sleep. > + > + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) { > gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], > &rect); > + } else if (old_state->fb != state->fb) { > + drm_rect_init(&rect, 0, 0, state->fb->width, state->fb->height); > + gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], > &rect); > + } > } [Severity: Medium] Will this fallback logic bypass visibility checks and incorrectly trigger redundant full updates? For visible planes, drm_atomic_helper_damage_merged() detects FB swaps and returns true, making this else-if block dead code. But if the plane transitions to invisible (state->visible == false), the helper returns false. Won't this else-if block then incorrectly trigger and queue a full frame transfer for an invisible plane? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
