* Re: [PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out
2026-06-24 14:37 [PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out Pengpeng Hou
@ 2026-06-24 14:52 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-24 14:52 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Critical] A failed channel configuration sets `chan->used_pre = -1`, leading to an out-of-bounds array access in the VBLANK interrupt handler.
- [High] The error path fails to restore the out-parameter `*eba`, causing the caller to program the hardware with a disabled SRAM address.
Pre-existing issues:
- [High] Sleeping functions are called from within a DRM plane atomic update callback.
- [High] The return value of pm_runtime_get_sync() is ignored, risking a synchronous external abort.
--
commit 026bb13a574603516c5e8a7270cb349020d87261
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
gpu: ipu-v3: fail PRG channel setup when buffer fill times out
ipu_prg_channel_configure() waits for both PRG double buffers to become
ready, but ignores readl_poll_timeout() failures. A timeout is therefore
reported as a successful configuration and chan->enabled is set even
though the hardware did not report ready buffers. Propagate the timeout
error.
> diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
> index d38d3ba54d722..ff32f4ca57cb4 100644
> --- a/drivers/gpu/ipu-v3/ipu-prg.c
> +++ b/drivers/gpu/ipu-v3/ipu-prg.c
> @@ -327,10 +327,22 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
> writel(val, prg->regs + IPU_PRG_REG_UPDATE);
>
> /* wait for both double buffers to be filled */
> - readl_poll_timeout(prg->regs + IPU_PRG_STATUS, val,
> - (val & IPU_PRG_STATUS_BUFFER0_READY(prg_chan)) &&
> - (val & IPU_PRG_STATUS_BUFFER1_READY(prg_chan)),
> - 5, 1000);
> + ret = readl_poll_timeout(prg->regs + IPU_PRG_STATUS, val,
> + (val & IPU_PRG_STATUS_BUFFER0_READY(prg_chan)) &&
> + (val & IPU_PRG_STATUS_BUFFER1_READY(prg_chan)),
> + 5, 1000);
[Severity: High]
This is a pre-existing issue, but does using readl_poll_timeout() here
cause us to sleep in an atomic context?
The macro internally uses usleep_range(), but ipu_prg_channel_configure()
is called from the DRM plane atomic update callback ipu_plane_atomic_update()
which runs in an atomic context where sleeping is forbidden.
Additionally, the pm_runtime_get_sync() call earlier in this function
can also sleep. Can we safely sleep here, or should this be avoided?
> + if (ret) {
> + val = readl(prg->regs + IPU_PRG_CTL);
> + val |= IPU_PRG_CTL_BYPASS(prg_chan);
> + writel(val, prg->regs + IPU_PRG_CTL);
> +
> + val = IPU_PRG_REG_UPDATE_REG_UPDATE;
> + writel(val, prg->regs + IPU_PRG_REG_UPDATE);
> +
> + pm_runtime_put(prg->dev);
[Severity: High]
This isn't a bug introduced by this patch, but earlier in
ipu_prg_channel_configure() we call pm_runtime_get_sync(prg->dev)
without checking its return value.
If the device fails to power on and we proceed to access unpowered PRG
MMIO registers like IPU_PRG_STRIDE, will it trigger a synchronous
external abort?
> + ipu_prg_put_pre(prg, prg_chan);
[Severity: Critical]
Could this regression cause the call to ipu_prg_put_pre() to lead to an
out-of-bounds array access later?
When this fails, ipu_prg_put_pre() correctly releases the PRE but sets
chan->used_pre to -1. However, because the caller ipu_plane_atomic_update()
in ipuv3-plane.c ignores the error, ipu_state->use_pre remains true.
On the next VBLANK interrupt, ipu_plane_atomic_update_pending() blindly
delegates to the PRG:
ipu_plane_atomic_update_pending(...) {
if (ipu_state->use_pre)
return ipu_prg_channel_configure_pending(ipu_plane->ipu_ch);
}
Which then executes:
ipu_prg_channel_configure_pending(...) {
...
return ipu_pre_update_pending(prg->pres[chan->used_pre]);
}
Will accessing prg->pres[-1] panic the kernel?
> + return ret;
[Severity: High]
If we return on this error path, does this regression leave the
out-parameter *eba pointing to the now-disabled PRE SRAM?
In the caller ipu_plane_atomic_update() in ipuv3-plane.c, the return
value is ignored:
ipu_plane_atomic_update(...) {
...
ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
height, fb->pitches[0],
fb->format->format, fb->modifier,
&eba);
...
ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
}
Will this result in the IPU IDMAC fetching pixel data from a disabled
PRE SRAM buffer and potentially causing a bus hang?
> + }
>
> pm_runtime_put(prg->dev);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624143752.53943-1-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread