Re: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[sashiko-bot@kernel.org]

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 <wenst@chromium.org>

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/20260807114654.1373305-1-wenst@chromium.org?part=1