Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] Unintentional removal of 10-bit color formats from the primary plane. Pre-existing issues: - [High] Plane `atomic_disable` crashes when called on a previously disabled plane. - [High] Primary plane `atomic_disable` mistakenly enables the plane in the hardware register. -- commit 53c18bf18dd1a86e14f16784c764ae73c2637d54 Author: Icenowy Zheng <[email protected]> drm/verisilicon: remove ARGB formats from primary plane As the blending of the primary plane is currently explicitly disabled (and it's not possible on DC8000), remove the ARGB formats from the primary plane format tables. > diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c > b/drivers/gpu/drm/verisilicon/vs_hwdb.c > index 56aa450443068..ebf6f843bc885 100644 > --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c > +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c > @@ -15,32 +15,16 @@ static const u32 vs_primary_formats_array_no_yuv444[] = { [ ... ] > DRM_FORMAT_XRGB8888, > DRM_FORMAT_XBGR8888, > DRM_FORMAT_RGBX8888, > DRM_FORMAT_BGRX8888, > - DRM_FORMAT_ARGB8888, > - DRM_FORMAT_ABGR8888, > - DRM_FORMAT_RGBA8888, > - DRM_FORMAT_BGRA8888, > - DRM_FORMAT_ARGB2101010, > - DRM_FORMAT_ABGR2101010, > - DRM_FORMAT_RGBA1010102, > - DRM_FORMAT_BGRA1010102, [Severity: Medium] Does the removal of DRM_FORMAT_ARGB2101010 and its variants drop all 10-bit color support from the primary plane? Since the opaque 10-bit counterparts (like DRM_FORMAT_XRGB2101010) are missing from vs_primary_formats_array_no_yuv444, any userspace compositor attempting to use 10-bit color will find no supported formats and fail. > /* TODO: non-RGB formats */ > }; > > @@ -49,32 +33,16 @@ static const u32 vs_primary_formats_array_with_yuv444[] = > { [ ... ] > DRM_FORMAT_XRGB8888, > DRM_FORMAT_XBGR8888, > DRM_FORMAT_RGBX8888, > DRM_FORMAT_BGRX8888, > - DRM_FORMAT_ARGB8888, > - DRM_FORMAT_ABGR8888, > - DRM_FORMAT_RGBA8888, > - DRM_FORMAT_BGRA8888, > - DRM_FORMAT_ARGB2101010, > - DRM_FORMAT_ABGR2101010, > - DRM_FORMAT_RGBA1010102, > - DRM_FORMAT_BGRA1010102, [Severity: Medium] Will this also remove 10-bit color support from the secondary array vs_primary_formats_array_with_yuv444? Just like the array above, the opaque 10-bit formats were not added, removing 10-bit color support for userspace compositors on this plane. > /* TODO: non-RGB formats */ > }; [Severity: High] This isn't a bug introduced by this patch, but can vs_primary_plane_atomic_disable() crash when called on a previously disabled plane? drivers/gpu/drm/verisilicon/vs_primary_plane.c: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; ... If a userspace process performs an atomic commit that targets a disabled plane and keeps it disabled or moves it out of bounds (!state->visible), DRM helpers may call atomic_update. The driver's atomic_update unconditionally calls atomic_disable for invisible planes. When atomic_disable retrieves old_plane_state->crtc, it is NULL since the plane was already disabled. Would this result in a NULL pointer dereference at vcrtc->id? This exact same pattern exists in the cursor plane's disable path in drivers/gpu/drm/verisilicon/vs_cursor_plane.c for vs_cursor_plane_atomic_disable(): drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_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 is a pre-existing issue, but does vs_primary_plane_atomic_disable() mistakenly enable the plane in the hardware register instead of disabling it? drivers/gpu/drm/verisilicon/vs_primary_plane.c:vs_primary_plane_atomic_disable() { ... regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); ... } Whenever the primary plane is disabled (e.g., DPMS off, mode set, or compositor exit), this code uses regmap_set_bits with VSDC_FB_CONFIG_EX_FB_EN. This leaves the hardware plane active instead of disabling it. When the DRM core subsequently frees the framebuffer memory, the display controller hardware will continue to DMA read from the freed memory. Can this cause a Use-After-Free and potential memory corruption or IOMMU faults? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
