[PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out

[PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out

[Pengpeng Hou]

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. Before returning, put the channel back into
bypass mode and release the PRE allocated for this configuration so the
software state stays consistent with the failed setup.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/gpu/ipu-v3/ipu-prg.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
index d38d3ba54..ff32f4ca5 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);
+	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);
+		ipu_prg_put_pre(prg, prg_chan);
+		return ret;
+	}
 
 	pm_runtime_put(prg->dev);
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] gpu: ipu-v3: fail PRG channel setup when buffer fill times out

[sashiko-bot]

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