Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Incorrect usage of `drm_atomic_get_new_*_state` causes valid partial atomic commits to be rejected with `-EINVAL` and leads to potential hardware misconfiguration. Pre-existing issues: - [High] Sleeping clock functions are called from CRTC atomic enable/disable paths, violating atomic context constraints. - [High] Hardware DMA use-after-free on page flip due to lack of hardware VBLANK synchronization. -- commit 56c718e0d1deb02575937fad72869dc5fbfabbaa Author: Davide Bonatto <[email protected]> drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers The driver is being migrated from the deprecated drm_simple_display_pipe helper to explicitly managed plane, CRTC, and encoder objects using standard atomic helpers. > diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c > index c93d61ac0bb75..b66b9562df10d 100644 > --- a/drivers/gpu/drm/tiny/arcpgu.c > +++ b/drivers/gpu/drm/tiny/arcpgu.c [ ... ] > -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe, > - struct drm_crtc_state *crtc_state, > - struct drm_plane_state *plane_state) > +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe); > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > + struct drm_plane_state *plane_state = > + drm_atomic_get_new_plane_state(state, &arcpgu->plane); > + const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL; [Severity: High] Does this skip essential pixel format programming during a CRTC enable if the plane isn't part of the commit state? If a userspace compositor requests a CRTC-only DPMS state change, drm_atomic_get_new_plane_state() will return NULL for the plane state, leading to a NULL fb and skipping the pixel format programming entirely. > - arc_pgu_mode_set(arcpgu); > + arc_pgu_mode_set(arcpgu, fb); > > clk_prepare_enable(arcpgu->clk); [Severity: High] This is a pre-existing issue, but does this execute a sleeping clock framework function inside an atomic context? According to DRM subsystem guidelines, CRTC atomic enable callbacks execute in an atomic context where sleeping functions are forbidden. Calling clk_prepare_enable() here can cause kernel warnings and potential deadlocks. > arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, > arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) | > - ARCPGU_CTRL_ENABLE_MASK); > + ARCPGU_CTRL_ENABLE_MASK); > } > > -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe) > +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe); > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > clk_disable_unprepare(arcpgu->clk); [Severity: High] This is a pre-existing issue, but does this execute a sleeping clock framework function inside an atomic context? Similar to the enable path, arc_pgu_crtc_atomic_disable() executes in an atomic context. Calling clk_disable_unprepare() here acquires a mutex and sleeps, which is not permitted. [ ... ] > -static void arc_pgu_update(struct drm_simple_display_pipe *pipe, > - struct drm_plane_state *state) > +static void arc_pgu_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu; > + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane); > + struct drm_plane_state *new_plane_state = > drm_atomic_get_new_plane_state(state, plane); > struct drm_gem_dma_object *gem; > > - if (!pipe->plane.state->fb) > + if (!new_plane_state->fb) > return; > > - arcpgu = pipe_to_arcpgu_priv(pipe); > - gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0); > + gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0); > arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr); [Severity: High] This is a pre-existing issue inherited from the simple display pipe setup, but can this lead to a hardware DMA use-after-free on page flip? Because the driver lacks VBLANK interrupts, the new DMA address is committed to hardware here without a mechanism to delay freeing the previous buffer. The DRM core simulates a VBLANK and immediately unpins/frees the old memory, but the physical hardware won't latch this new address until its next real VBLANK. The hardware could continue scanning from the now-freed memory buffer. [ ... ] > +static int arc_pgu_plane_atomic_check(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ > + struct drm_plane_state *new_plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_crtc_state *crtc_state; > + > + if (!new_plane_state->crtc) > + return 0; > + > + crtc_state = drm_atomic_get_new_crtc_state(state, > new_plane_state->crtc); > + if (!crtc_state) > + return -EINVAL; [Severity: High] Does this incorrectly reject valid plane-only updates? If a userspace compositor requests a non-modesetting atomic update (such as only updating the plane's FB), the CRTC state is not natively included in the atomic commit state. drm_atomic_get_new_crtc_state() will return NULL, causing this function to fail with -EINVAL and breaking userspace display servers. [ ... ] > +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct drm_crtc_state *crct_state = > drm_atomic_get_new_crtc_state(state, crtc); > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > + struct drm_plane_state *plane_state; > + > + if (!crct_state->enable) > + return 0; > + > + plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane); > + if (!plane_state || !plane_state->fb) > + return -EINVAL; [Severity: High] Will this fail valid CRTC-only updates? Similar to the plane check, if a userspace compositor requests an update to a CRTC property (e.g., active state) without including the plane in the commit request, plane_state will evaluate to NULL and cause the driver to incorrectly reject the partial atomic commit. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
