Re: [PATCH] media: rzg2l-cru: rework rzg2l_cru_fill_hw_slot()

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Daniel,
Thank you for your patch!

On Thu, Sep 18, 2025 at 01:08:55PM +0100, Daniel Scally wrote:
> The current implementation of rzg2l_cru_fill_hw_slot() results in the
> artificial loss of frames. At present whenever a frame-complete IRQ
> is received the driver fills the hardware slot that was just written
> to with the address of the next buffer in the driver's queue. If the
> queue is empty, that hardware slot's address is set to the address of
> the scratch buffer to enable the capture loop to keep running. There
> is a minimum of a two-frame delay before that slot will be written to
> however, and in the intervening period userspace may queue more
> buffers which could be used.
> 
> To resolve the issue rework rzg2l_cru_fill_hw_slot() so that it
> iteratively fills all slots from the queue which currently do not
> have a buffer assigned, until the queue is empty. The scratch
> buffer is only resorted to in the event that the queue is empty and
> the next slot that will be written to does not already have a buffer
> assigned.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 63 +++++++++++-----------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 941badc90ff55c5225644f88de1d70239eb3a247..9ffafb0239a7388104219b2b72eec9051db82078 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -192,45 +192,47 @@ static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
>  }
>  
>  /*
> - * Moves a buffer from the queue to the HW slot. If no buffer is
> - * available use the scratch buffer. The scratch buffer is never
> - * returned to userspace, its only function is to enable the capture
> - * loop to keep running.
> + * Move as many buffers as possible from the queue to HW slots. If no buffer is
> + * available and the next slot currently lacks one then use the scratch buffer.
> + * The scratch buffer is never returned to userspace, its only function is to
> + * enable the capture loop to keep running.
>   */
> -static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
> +static void rzg2l_cru_fill_hw_slots(struct rzg2l_cru_dev *cru, int slot)
>  {
> -	struct vb2_v4l2_buffer *vbuf;
> +	unsigned int from_slot = slot;
>  	struct rzg2l_cru_buffer *buf;
> +	struct vb2_v4l2_buffer *vbuf;
>  	dma_addr_t phys_addr;
>  
> -	/* A already populated slot shall never be overwritten. */
> -	if (WARN_ON(cru->queue_buf[slot]))
> -		return;
> -
> -	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
> +	do {
> +		if (cru->queue_buf[slot]) {
> +			slot = (slot + 1) % cru->num_buf;
> +			continue;
> +		}
>  
> -	if (list_empty(&cru->buf_list)) {
> -		cru->queue_buf[slot] = NULL;
> -		phys_addr = cru->scratch_phys;
> -	} else {
> -		/* Keep track of buffer we give to HW */
> -		buf = list_entry(cru->buf_list.next,
> -				 struct rzg2l_cru_buffer, list);
> -		vbuf = &buf->vb;
> -		list_del_init(to_buf_list(vbuf));
> -		cru->queue_buf[slot] = vbuf;
> -
> -		/* Setup DMA */
> -		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> -	}
> +		if (list_empty(&cru->buf_list)) {
> +			if (slot == from_slot)
> +				phys_addr = cru->scratch_phys;
> +			else
> +				return;
> +		} else {
> +			buf = list_first_entry(&cru->buf_list,
> +					       struct rzg2l_cru_buffer, list);
> +			vbuf = &buf->vb;
> +			list_del_init(&buf->list);
> +			cru->queue_buf[slot] = vbuf;
> +			phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> +		}
>  
> -	rzg2l_cru_set_slot_addr(cru, slot, phys_addr);
> +		dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
> +		rzg2l_cru_set_slot_addr(cru, slot, phys_addr);
> +		slot = (slot + 1) % cru->num_buf;
> +	} while (slot != from_slot);
>  }
>  
>  static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  {
>  	const struct rzg2l_cru_info *info = cru->info;
> -	unsigned int slot;
>  	u32 amnaxiattr;
>  
>  	/*
> @@ -239,8 +241,7 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  	 */
>  	rzg2l_cru_write(cru, AMnMBVALID, AMnMBVALID_MBVALID(cru->num_buf - 1));
>  
> -	for (slot = 0; slot < cru->num_buf; slot++)
> -		rzg2l_cru_fill_hw_slot(cru, slot);
> +	rzg2l_cru_fill_hw_slots(cru, 0);
>  
>  	if (info->has_stride) {
>  		u32 stride = cru->format.bytesperline;
> @@ -652,7 +653,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>  	cru->sequence++;
>  
>  	/* Prepare for next frame */
> -	rzg2l_cru_fill_hw_slot(cru, slot);
> +	rzg2l_cru_fill_hw_slots(cru, (slot + 1) % cru->num_buf);
>  
>  done:
>  	spin_unlock_irqrestore(&cru->qlock, flags);
> @@ -752,7 +753,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>  		cru->sequence++;
>  
>  		/* Prepare for next frame */
> -		rzg2l_cru_fill_hw_slot(cru, slot);
> +		rzg2l_cru_fill_hw_slots(cru, (slot + 1) % cru->num_buf);
>  	}
>  
>  	return IRQ_HANDLED;
> 
> ---
> base-commit: a75b8d198c55e9eb5feb6f6e155496305caba2dc
> change-id: 20250918-rzg2l-cru-0554a4352a70
> 
> Best regards,
> -- 
> Daniel Scally <dan.scally@ideasonboard.com>

Not reviewed yet, sorry.

But testing on RZ/G3E I'm getting the following:

[  288.873715] rzg2l-cru 16000000.video: Invalid MB address 0xeacc3e00 (out of range)
[  288.884665] rzg2l-cru 16000000.video: Invalid MB address 0xeae57e00 (out of range)
[  288.967963] rzg2l-cru 16000000.video: Invalid MB address 0xe9957e00 (out of range)

Tested using:

media-ctl -d /dev/media0 --set-v4l2 '"ov5645 0-003c":0[fmt:UYVY8_2X8/1280x960@1/60 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"csi-16000400.csi2":0[fmt:UYVY8_2X8/1280x960]'
media-ctl -d /dev/media0 --set-v4l2 '"cru-ip-16000000.video":0[fmt:UYVY8_2X8/1280x960]'

gst-launch-1.0 v4l2src device=/dev/video0 io-mode=dmabuf ! video/x-raw,format=UYVY,width=1280,height=960 !  videoconvert ! queue ! glimagesink sync=false

Let me gently know if I'm missing somenthing.

Thanks & Regards,
Tommaso



>