Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] drm/verisilicon: uninitialized dc and output in plane atomic_update() - [High] drm/verisilicon: plane visibility state mismatch with atomic_enable() --
commit 3080d00108f48782cd8c1c8ee016e9473b324e57 Author: Icenowy Zheng <[email protected]> drm/verisilicon: stop to reuse atomic_disable for invisible planes This commit factors out helper functions to disable cursor and primary planes directly from their atomic_update callbacks when state->visible is false, avoiding a NULL CRTC dereference in the old plane state when calling atomic_disable. > diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > index 1fa6b7578b27e..8dd6caadb7075 100644 > --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > @@ -99,6 +99,15 @@ static void vs_cursor_plane_commit(struct vs_dc *dc, > unsigned int output) > VSDC_CURSOR_CONFIG_IMG_UPDATE); > } > > +static void vs_cursor_plane_disable(struct vs_dc *dc, unsigned int output) > +{ > + regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), > + VSDC_CURSOR_CONFIG_FMT_MASK, > + VSDC_CURSOR_CONFIG_FMT_OFF); > + > + vs_cursor_plane_commit(dc, output); > +} > + > static void vs_cursor_plane_atomic_enable(struct drm_plane *plane, > struct drm_atomic_commit > *atomic_state) > { > @@ -126,11 +135,7 @@ static void vs_cursor_plane_atomic_disable(struct > drm_plane *plane, > unsigned int output = vcrtc->id; > struct vs_dc *dc = vcrtc->dc; > > - regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), > - VSDC_CURSOR_CONFIG_FMT_MASK, > - VSDC_CURSOR_CONFIG_FMT_OFF); > - > - vs_cursor_plane_commit(dc, output); > + vs_cursor_plane_disable(dc, output); > } > > static void vs_cursor_plane_atomic_update(struct drm_plane *plane, > @@ -146,7 +151,7 @@ static void vs_cursor_plane_atomic_update(struct > drm_plane *plane, > dma_addr_t dma_addr; > > if (!state->visible) { > - vs_cursor_plane_atomic_disable(plane, atomic_state); > + vs_cursor_plane_disable(dc, output); > return; > } [Severity: Critical] Does vs_cursor_plane_atomic_update() pass uninitialized dc and output values to vs_cursor_plane_disable() when !state->visible is true? In drivers/gpu/drm/verisilicon/vs_cursor_plane.c, vcrtc, output, and dc are only initialized from state->crtc after the !state->visible early return: drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_plane_atomic_update() { struct drm_crtc *crtc = state->crtc; struct vs_dc *dc; struct vs_crtc *vcrtc; unsigned int output; dma_addr_t dma_addr; if (!state->visible) { vs_cursor_plane_disable(dc, output); return; } vcrtc = drm_crtc_to_vs_crtc(crtc); output = vcrtc->id; dc = vcrtc->dc; } When a cursor plane is updated while invisible, vs_cursor_plane_disable() will dereference the uninitialized pointer dc->regs and index VSDC_CURSOR_CONFIG(output) with an uninitialized output variable. [Severity: High] How does disabling the plane in hardware on !state->visible interact with vs_cursor_plane_atomic_enable() and subsequent visible updates? In drm_atomic_helper_commit_planes(), funcs->atomic_update() runs before funcs->atomic_enable(). When a previously unbound plane (old_plane_state->crtc is NULL) is bound to a CRTC while invisible (new_plane_state->crtc is non-NULL and new_plane_state->visible is false): 1. vs_cursor_plane_atomic_update() calls vs_cursor_plane_disable() and returns early without programming VSDC_CURSOR_ADDRESS(output), size, or position registers. 2. drm_atomic_plane_enabling() evaluates to true because !old_plane_state->crtc && new_plane_state->crtc, so drm_atomic_helper_commit_planes() immediately calls vs_cursor_plane_atomic_enable(): drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_plane_atomic_enable() { ... regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), VSDC_CURSOR_CONFIG_FMT_MASK, VSDC_CURSOR_CONFIG_FMT_ARGB8888); vs_cursor_plane_commit(dc, output); } Won't vs_cursor_plane_atomic_enable() unconditionally re-enable the plane in hardware without a valid framebuffer address or geometry configured? Conversely, if a plane that is already bound to a CRTC moves off-screen so state->visible becomes false, vs_cursor_plane_atomic_update() sets VSDC_CURSOR_CONFIG_FMT_OFF via vs_cursor_plane_disable(). When a later commit moves the plane back on-screen while remaining on the same CRTC (old_plane_state->crtc and new_plane_state->crtc are both non-NULL, and new_plane_state->visible becomes true), drm_atomic_plane_enabling() is false, so vs_cursor_plane_atomic_enable() is not called and vs_cursor_plane_atomic_update() never restores VSDC_CURSOR_CONFIG_FMT_ARGB8888. Will the cursor plane remain disabled in hardware? > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > index c12fe54096c33..56e4c0e168871 100644 > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > @@ -60,6 +60,14 @@ static void vs_primary_plane_commit(struct vs_dc *dc, > unsigned int output) > VSDC_FB_CONFIG_EX_COMMIT); > } > > +static void vs_primary_plane_disable(struct vs_dc *dc, unsigned int output) > +{ > + regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > + VSDC_FB_CONFIG_EX_FB_EN); > + > + vs_primary_plane_commit(dc, output); > +} > + > static void vs_primary_plane_atomic_enable(struct drm_plane *plane, > struct drm_atomic_commit > *atomic_state) > { > @@ -89,10 +97,7 @@ static void vs_primary_plane_atomic_disable(struct > drm_plane *plane, > unsigned int output = vcrtc->id; > struct vs_dc *dc = vcrtc->dc; > > - regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > - VSDC_FB_CONFIG_EX_FB_EN); > - > - vs_primary_plane_commit(dc, output); > + vs_primary_plane_disable(dc, output); > } > > static void vs_primary_plane_atomic_update(struct drm_plane *plane, > @@ -109,7 +114,7 @@ static void vs_primary_plane_atomic_update(struct > drm_plane *plane, > dma_addr_t dma_addr; > > if (!state->visible) { > - vs_primary_plane_atomic_disable(plane, atomic_state); > + vs_primary_plane_disable(dc, output); > return; > } [Severity: Critical] Similarly, in drivers/gpu/drm/verisilicon/vs_primary_plane.c, doesn't vs_primary_plane_atomic_update() also call vs_primary_plane_disable(dc, output) before vcrtc, output, and dc are initialized from state->crtc, dereferencing uninitialized dc->regs and using an uninitialized output index in VSDC_FB_CONFIG_EX(output) when !state->visible? [Severity: High] Does vs_primary_plane_atomic_update() have the same state mismatch with vs_primary_plane_atomic_enable()? When old_plane_state->crtc is NULL, new_plane_state->crtc is non-NULL, and new_plane_state->visible is false, vs_primary_plane_atomic_update() returns early without programming VSDC_FB_ADDRESS(output) or geometry registers, and then vs_primary_plane_atomic_enable() sets VSDC_FB_CONFIG_EX_FB_EN and commits the plane in hardware anyway. And if an already-bound primary plane transitions from visible to invisible (clearing VSDC_FB_CONFIG_EX_FB_EN in vs_primary_plane_disable()) and then back to visible on the same CRTC, neither vs_primary_plane_atomic_enable() nor vs_primary_plane_atomic_update() sets VSDC_FB_CONFIG_EX_FB_EN again. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
