Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Medium] Removing software clipping for the cursor plane exposes a preexisting register programming bug, completely breaking cursor clipping. - [Low] Dangling prototype for `vs_fb_get_dma_addr()`. Pre-existing issues: - [High] NULL pointer dereference in plane atomic_disable functions when a plane is newly assigned but invisible. - [High] `vs_primary_plane_atomic_disable` incorrectly enables the plane instead of disabling it. -- --- Patch [1]: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses --- commit d8f5b888e2f48aaf1e83d6bb5aee579fe194f0ab Author: Chen-Yu Tsai <[email protected]> drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses The verisilicon driver has a custom framebuffer address calculating helper that the common drm_fb_dma_get_addr() can substitute. > diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > index fa4f601dd0c87..59778433ae843 100644 > --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c [ ... ] > @@ -176,7 +177,8 @@ static void vs_cursor_plane_atomic_update(struct > drm_plane *plane, > break; > } > > - dma_addr = vs_fb_get_dma_addr(fb, &state->src); > + /* hardware handles clipping as seen below */ > + dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0); [Severity: Medium] Since we are removing the software clipping workaround, does the hardware clipping actually work correctly here? Looking further down in vs_cursor_plane_atomic_update(), the hardware offsets are programmed without using the shift macros: regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), VSDC_CURSOR_CONFIG_X_OFF_MASK, -state->crtc_x); regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), VSDC_CURSOR_CONFIG_Y_OFF_MASK, -state->crtc_y); Because the unshifted small negative integer falls outside the masks, won't this evaluate to 0 during the bitwise AND, permanently programming the offsets to 0 and completely breaking cursor clipping? [Severity: High] This is a pre-existing issue, but I noticed a potential NULL pointer dereference in the disable path that gets called just above this snippet. If userspace binds a previously unbound plane where old_plane_state->crtc is NULL, but places it entirely off-screen, the DRM core sets new_plane_state->visible = false and calls vs_cursor_plane_atomic_update(). The code then calls vs_cursor_plane_atomic_disable(): if (!state->visible) { vs_cursor_plane_atomic_disable(plane, atomic_state); return; } Inside vs_cursor_plane_atomic_disable(), and identically in vs_primary_plane_atomic_disable(), it fetches the CRTC from the old state, which is NULL, and directly dereferences it: struct drm_crtc *crtc = state->crtc; struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); unsigned int output = vcrtc->id; Could this trigger a kernel panic if a malicious or buggy userspace submits this type of atomic modeset? [Severity: High] This is also a pre-existing issue, but looking at vs_primary_plane_atomic_disable(), the hardware appears to be enabled instead of disabled: regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); Because regmap_set_bits() turns the plane on, won't the primary plane remain actively rendering when the DRM core disables it, causing the hardware state to diverge from the kernel state? > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c > b/drivers/gpu/drm/verisilicon/vs_plane.c > index d81f7b8f4c650..38b8b536eccbd 100644 > --- a/drivers/gpu/drm/verisilicon/vs_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_plane.c [ ... ] > @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format, struct > vs_format *vs_format) > return 0; > } > > -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb, > - const struct drm_rect *src_rect) > -{ > - struct drm_gem_dma_object *gem; > - dma_addr_t dma_addr; [Severity: Low] Is the prototype for vs_fb_get_dma_addr() still declared in vs_plane.h? Should we remove it from drivers/gpu/drm/verisilicon/vs_plane.h as well to avoid leaving a dangling prototype? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
