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

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

Hi Dan,

On Mon, Sep 29, 2025 at 09:19:39PM +0100, Dan Scally wrote:
> Hi Tommaso
> 
> On 29/09/2025 17:08, Tommaso Merciai wrote:
> > 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.
> 
> No problem :)
> 
> > 
> > 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.
> 
> Ooh. I don't think you're missing anything...does it happen every time? Let
> me see if I can work out what would trigger that - thanks for testing!

You are welcome! :)

Sorry for the delay.
Yes this happens every time using:

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

Thanks & Regard,
Tommaso

> 
> Dan
> 
> > 
> > Thanks & Regards,
> > Tommaso
> > 
> > 
> > 
> > > 
> 
>