Re: [PATCH 12/16] media: rockchip: rga: handle error interrupt

[Nicolas Dufresne <nicolas@ndufresne.ca>]

[-- Attachment #1: Type: text/plain, Size: 5216 bytes --]

Le mardi 07 octobre 2025 à 10:32 +0200, Sven Püschel a écrit :
> Handle the error interrupt status in preparation of the RGA3 addition.
> This allows the buffer to be marked as done, as it would otherwise
> be stuck in the queue.
> 
> The RGA3 needs a soft reset to properly work after an error occurred,
> as it would otherwise cease to deliver new interrupts. Also the soft
> reset avoids additional error interrupts to be triggered, which are
> currently not supported by the rga_isr function.
> As it is unknown how the RGA2 behaves in the error case, no
> error interrupt was enabled and handled.
> 
> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
> ---
>  drivers/media/platform/rockchip/rga/rga-hw.c |  6 ++++--
>  drivers/media/platform/rockchip/rga/rga.c    | 32 +++++++++++++++++----------
> -
>  drivers/media/platform/rockchip/rga/rga.h    |  8 ++++++-
>  3 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c
> b/drivers/media/platform/rockchip/rga/rga-hw.c
> index
> d54183d224b3e9c42d5503acf172257f2e736f7b..93822b5b8b15e76862bd022759eaa5cb9552
> dd76 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -459,7 +459,7 @@ static void rga_hw_start(struct rockchip_rga *rga,
>  	rga_write(rga, RGA_CMD_CTRL, 0x1);
>  }
>  
> -static bool rga_handle_irq(struct rockchip_rga *rga)
> +static enum rga_irq_result rga_handle_irq(struct rockchip_rga *rga)
>  {
>  	int intr;
>  
> @@ -467,7 +467,9 @@ static bool rga_handle_irq(struct rockchip_rga *rga)
>  
>  	rga_mod(rga, RGA_INT, intr << 4, 0xf << 4);
>  
> -	return intr & 0x04;
> +	if (intr & 0x04)
> +		return RGA_IRQ_DONE;

Since you reuse an old driver, would be nice to create proper defines for
RGA_INT bit 3 (current command finish interrupt flag).

> +	return RGA_IRQ_IGNORE;
>  }
>  
>  static void rga_get_version(struct rockchip_rga *rga)
> diff --git a/drivers/media/platform/rockchip/rga/rga.c
> b/drivers/media/platform/rockchip/rga/rga.c
> index
> 0a725841b0cfa41bbc5b861b8f5ceac2452fc2b5..3b5d2eb8e109f44af76dd2240a239b1fa8a7
> 8cee 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -56,30 +56,38 @@ static void device_run(void *prv)
>  static irqreturn_t rga_isr(int irq, void *prv)
>  {
>  	struct rockchip_rga *rga = prv;
> +	struct vb2_v4l2_buffer *src, *dst;
> +	struct rga_ctx *ctx = rga->curr;
> +	enum rga_irq_result result;
>  
> -	if (rga->hw->handle_irq(rga)) {
> -		struct vb2_v4l2_buffer *src, *dst;
> -		struct rga_ctx *ctx = rga->curr;
> +	result = rga->hw->handle_irq(rga);
> +	if (result == RGA_IRQ_IGNORE)
> +		return IRQ_HANDLED;
>  
> -		WARN_ON(!ctx);
> +	WARN_ON(!ctx);
>  
> -		rga->curr = NULL;
> +	rga->curr = NULL;
>  
> -		src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -		dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>  
> -		WARN_ON(!src);
> -		WARN_ON(!dst);
> +	WARN_ON(!src);
> +	WARN_ON(!dst);
>  
> -		v4l2_m2m_buf_copy_metadata(src, dst, true);
> +	v4l2_m2m_buf_copy_metadata(src, dst, true);
>  
> -		dst->sequence = ctx->csequence++;
> +	dst->sequence = ctx->csequence++;
>  
> +	if (result == RGA_IRQ_DONE) {
>  		v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
>  		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
> -		v4l2_m2m_job_finish(rga->m2m_dev, ctx->fh.m2m_ctx);
> +	} else {
> +		v4l2_m2m_buf_done(src, VB2_BUF_STATE_ERROR);
> +		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_ERROR)

I'm not fan of assumption that its an error on else case. If often lead to
multiple calls. Please use an explicit error return.

> ;
>  	}
>  
> +	v4l2_m2m_job_finish(rga->m2m_dev, ctx->fh.m2m_ctx);

What if you get an IRQ and none of the flags are raised ? I did see that in the
past, and that least to bad things happening.

> +
>  	return IRQ_HANDLED;
>  }
>  
> diff --git a/drivers/media/platform/rockchip/rga/rga.h
> b/drivers/media/platform/rockchip/rga/rga.h
> index
> e19c4c82aca5ae2056f52d525138093fbbb81af8..dc4bb85707d12f5378c4891098cd7ea4a4d7
> 5e2d 100644
> --- a/drivers/media/platform/rockchip/rga/rga.h
> +++ b/drivers/media/platform/rockchip/rga/rga.h
> @@ -143,6 +143,12 @@ static inline void rga_mod(struct rockchip_rga *rga, u32
> reg, u32 val, u32 mask)
>  	rga_write(rga, reg, temp);
>  };
>  
> +enum rga_irq_result {
> +	RGA_IRQ_IGNORE,
> +	RGA_IRQ_DONE,
> +	RGA_IRQ_ERROR,
> +};
> +
>  struct rga_hw {
>  	const char *card_type;
>  	bool has_internal_iommu;
> @@ -152,7 +158,7 @@ struct rga_hw {
>  
>  	void (*start)(struct rockchip_rga *rga,
>  		      struct rga_vb_buffer *src, struct rga_vb_buffer *dst);
> -	bool (*handle_irq)(struct rockchip_rga *rga);
> +	enum rga_irq_result (*handle_irq)(struct rockchip_rga *rga);
>  	void (*get_version)(struct rockchip_rga *rga);
>  	void *(*try_format)(u32 *fourcc, bool is_output);
>  	int (*enum_format)(struct v4l2_fmtdesc *f);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]