Re: [PATCH 3/5] media: verisilicon: Reset the Rockchip encoder cores on timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] media: verisilicon: uncleared IRQ disable bit in
  hantro_h1_jpeg_enc_run() [1]
--

commit 433d2270fcbefc689a4dc88d1c36fa283eb8f9a5
Author: Arun Iyengar <aiyengar1990@gmail.com>

media: verisilicon: Reset the Rockchip encoder cores on timeout

This commit pulses the optional hardware reset lines in the Rockchip
VEPU1 and VEPU2 reset callbacks to recover encoder cores that become
stuck on the bus after an IOMMU fault and job timeout.

> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
> index 02673be9878e1..0dced75158334 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c

[ ... ]

> @@ -468,6 +469,33 @@ static void rk3066_vpu_dec_reset(struct hantro_ctx *ctx)
>  	vdpu_write(vpu, G1_REG_CONFIG_DEC_CLK_GATE_E, G1_REG_CONFIG);
>  }
>  
> +/*
> + * Stopping the encoder through its registers isn't enough when it is stuck
> + * on the bus, for instance after a translation fault in its IOMMU: it keeps
> + * AXI transactions outstanding and never becomes idle, which also prevents
> + * the IOMMU from stalling. Pulse the reset lines of the core, if the device
> + * tree describes them, to return it to its idle state.
> + */
> +static void rockchip_vpu_enc_hw_reset(struct hantro_dev *vpu)
> +{
> +	int ret;
> +
> +	if (!vpu->resets)
> +		return;
> +
> +	ret = reset_control_assert(vpu->resets);
> +	if (ret) {
> +		dev_warn(vpu->dev, "Failed to assert resets: %d\n", ret);
> +		return;
> +	}
> +
> +	udelay(5);
> +
> +	ret = reset_control_deassert(vpu->resets);
> +	if (ret)
> +		dev_err(vpu->dev, "Failed to deassert resets: %d\n", ret);
> +}
> +
>  static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
>  {
>  	struct hantro_dev *vpu = ctx->dev;
> @@ -475,6 +503,7 @@ static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
>  	vepu_write(vpu, H1_REG_INTERRUPT_DIS_BIT, H1_REG_INTERRUPT);
>  	vepu_write(vpu, 0, H1_REG_ENC_CTRL);
>  	vepu_write(vpu, 0, H1_REG_AXI_CTRL);
> +	rockchip_vpu_enc_hw_reset(vpu);
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but when vpu->resets is not
present in the device tree, does rockchip_vpu1_enc_reset() leave
H1_REG_INTERRUPT_DIS_BIT set in H1_REG_INTERRUPT [1] across later
runs?

hantro_h1_jpeg_enc_run() starts the encoder without writing to
H1_REG_INTERRUPT to clear H1_REG_INTERRUPT_DIS_BIT, so after a watchdog
reset all subsequent H1 JPEG encoding jobs have completion interrupts
disabled and time out as well.

[1] https://sashiko.dev/#/bug/linux-22957774-1c15-4f2d-87b4-d6b2e043ac2a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-hantro-robust-v1-0-6b6269c05bfa@gmail.com?part=3