Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The return value of `drm_plane_create_blend_mode_property` is ignored. Pre-existing issues: - [Critical] Dereferencing a NULL `crtc` pointer in `vs_primary_plane_atomic_disable` during offscreen plane transitions. - [High] `vs_primary_plane_atomic_disable` incorrectly sets the enable bit instead of clearing it. -- commit 9aa67f5c63b091d89b8b4a7a3066a896f35cf108 Author: Icenowy Zheng <[email protected]> drm/verisilicon: set blend mode for the primary plane Blend modes are now required to expose pixel formats w/ alpha. As it's the primary plane and blending is explicitly disabled, just expose PIXEL_NONE blend mode. > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > index 1f2be41ae496c..8d58682d88ef8 100644 > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > @@ -7,6 +7,7 @@ > > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_blend.h> > #include <drm/drm_crtc.h> > #include <drm/drm_fourcc.h> > #include <drm/drm_framebuffer.h> [ ... ] [Severity: Critical] This is a pre-existing issue, but can this dereference a NULL pointer during offscreen plane transitions? If userspace commits an atomic state where a plane transitions from fully disabled (old_state->crtc == NULL) to assigned but offscreen (new_state->crtc != NULL but new_state->visible == false), the DRM core calls vs_primary_plane_atomic_update(). This function sees !state->visible and delegates to the disable function: vs_primary_plane_atomic_update() { ... if (!state->visible) { vs_primary_plane_atomic_disable(plane, atomic_state); return; } ... } The disable function then fetches old_state->crtc, which is NULL, and dereferences it: vs_primary_plane_atomic_disable() { ... struct drm_plane_state *state = drm_atomic_get_old_plane_state(atomic_state, plane); struct drm_crtc *crtc = state->crtc; struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); unsigned int output = vcrtc->id; ... } [Severity: High] This isn't a bug introduced by this patch, but does vs_primary_plane_atomic_disable() incorrectly set the enable bit instead of clearing it? vs_primary_plane_atomic_disable() { ... regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); vs_primary_plane_commit(dc, output); } When the plane needs to be disabled, the core calls this function, which uses regmap_set_bits() with the VSDC_FB_CONFIG_EX_FB_EN bit. Should this use regmap_clear_bits() instead? The hardware plane is never disabled, which can cause visual corruption, unexpected display state, and potential memory access faults if the buffer memory is subsequently freed while the hardware is still scanning it out. > @@ -179,5 +180,7 @@ struct drm_plane *vs_primary_plane_init(struct drm_device > *drm_dev, struct vs_dc > > drm_plane_helper_add(plane, &vs_primary_plane_helper_funcs); > > + drm_plane_create_blend_mode_property(plane, > + BIT(DRM_MODE_BLEND_PIXEL_NONE)); [Severity: Medium] Is it intentional to ignore the return value of drm_plane_create_blend_mode_property()? If property creation fails (e.g. due to memory exhaustion), the plane is initialized without the requested blend mode property, breaking userspace expectations for alpha formats. Should this function check for an error code and handle the failure? > return plane; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
