Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The PRE resource allocation in `ipu_planes_assign_pre()` does not track global state, allowing resource overcommit across multiple CRTCs and leading to silent hardware configuration failures. -- --- Patch [1]: [PATCH v2] drm/imx: Fix ipu_plane_duplicate_state() OOM handling --- commit e059dd54b800a823912e85c46d6931922398d1c7 Author: Osama Abdelkader <[email protected]> drm/imx: Fix ipu_plane_duplicate_state() OOM handling This patch fixes a potential NULL pointer dereference in ipu_plane_duplicate_state(). When kmalloc_obj() fails, the function now returns NULL properly instead of unconditionally returning &state->base. > diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c > b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c > index c7ec09e557c19..67f2da7f2b65e 100644 > --- a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c > +++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c > @@ -323,9 +323,10 @@ ipu_plane_duplicate_state(struct drm_plane *plane) > return NULL; > > state = kmalloc_obj(*state); > - if (state) > - __drm_atomic_helper_plane_duplicate_state(plane, &state->base); > + if (!state) > + return NULL; > > + __drm_atomic_helper_plane_duplicate_state(plane, &state->base); > return &state->base; > } [Severity: High] This isn't a bug introduced by this patch, but while reviewing this file I noticed a pre-existing issue with PRE resource allocation. In ipu_planes_assign_pre(), the available_pres counter is reset to the system maximum on every atomic commit: drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c:ipu_planes_assign_pre() { ... int available_pres = ipu_prg_max_active_channels(); ... for_each_new_plane_in_state(state, plane, plane_state, i) { ... ipu_state->use_pre = true; available_pres--; ... } Does this allow resource overcommit across multiple CRTCs if they are configured independently via separate atomic commits, since it ignores globally active PREs from other CRTCs? Additionally, in ipu_plane_atomic_update(), the return value of ipu_prg_channel_configure() is ignored: drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c:ipu_plane_atomic_update() { ... axi_id = ipu_chan_assign_axi_id(ipu_plane->dma); ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width, height, fb->pitches[0], fb->format->format, fb->modifier, &eba); ... } If a PRE acquisition fails, will this silently drop the error and program the hardware with invalid state? Could this cause the IPU to read raw, potentially tiled memory directly if the expected SRAM buffer address is not mapped? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
