Re: [PATCH] media: rzv2h-ivc: Wait for frame end in stop_streaming

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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

Hello again Nicolas

On Wed, May 13, 2026 at 09:54:44AM +0200, Jacopo Mondi wrote:
> Hi Nicolas
>
>    thanks for looking into this
>
> On Tue, May 12, 2026 at 01:46:31PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 01 avril 2026 à 17:35 +0200, Jacopo Mondi a écrit :
> > > From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > >
> > > The rzv2h-ivc driver fails to handle back-2-back streaming sessions that
> > > do not go through a peripheral reset. As the driver uses an autosuspend
> > > delay of 2 seconds, it is quite possible that two consecutive streaming
> > > sessions won't go through a suspend/resume sequence.
> > >
> > > If the peripheral is not reset the second streaming session hangs and no
> > > frames are delivered to the ISP.
> > >
> > > This is because the stop_streaming() procedure implemented in the driver
> > > doesn't match what's prescribed by the chip datasheet:
> > >
> > > 1) The chip manual suggests to poll the RZV2H_IVC_FM_INT_STAT_STPEND bit
> > >    of RZV2H_IVC_REG_FM_INT_STA instead of polling on RZV2H_IVC_REG_FM_STOP
> > >    and prescribes to clear the bit after polling has completed
> > >
> > > 2) More importantly: the RZV2H_IVC_REG_FM_STOP_FSTOP bit has to be set
> > >    on RZV2H_IVC_REG_FM_STOP -only- if a frame transfer to the ISP is in
> > >    progress. Setting the RZV2H_IVC_REG_FM_STOP_FSTOP bit when no frame is
> > >    being transferred causes the polling routine to timeout and the next
> > >    streaming session fails to start
> > >
> > > As a frame transfer of an image in 1920x1080@10bi takes 5 milliseconds
> > > at most, it is quite possible that the frame transfer completion interrupt
> > > races with the stop procedure.
> > >
> > > Instead of forcing a frame transfer abort, simply wait for the
> > > in-progress transfer to complete by polling the ivc->vvalid_ifp status
> > > variable in an hand-rolled loop that allows to inspect the variable
> > > while holding the spinlock, to allow the irq handler to complete the
> > > current buffer.
> > >
> > > With this change, streaming back-2-back without suspending the
> > > peripheral works successfully.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: f0b3984d821b ("media: platform: Add Renesas Input Video Control block
> > > driver")
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > > ---
> > > As detailed in the commit message, re-starting a streaming session
> > > without going through a peripheral reset doesn't currently work.
> > >
> > > I initially thought this is because the stop_streaming() procedure
> > > implemented in the rzv2h-ivc driver does not comply with what is
> > > prescribed by the chip manual.
> > >
> > > So I went and modified it according to the manual.
> > >
> > > Unfortunately, even by following the suggested procedure, once
> > > RZV2H_IVC_REG_FM_STOP is set and a forceful frame transfer abort is
> > > started, the RZV2H_IVC_FM_INT_STAT_STPEND bit takes a long time to
> > > clear, during which is most often times the case the current in-progress
> > > transfer completes by itself. If this happen, then a peripheral
> > > reset is required to restart streaming regardless if I forcefully clear
> > > the RZV2H_IVC_REG_FM_STOP_FSTOP and RZV2H_IVC_FM_INT_STAT_STPEND bits.
> > >
> > > I have tried several strategies to properly forcefully stop an
> > > in-progress transfer and handle the potential race betwee the
> > > transfer-complete irq and the polling the RZV2H_IVC_REG_FM_INT_STA
> > > register (which could potentially sleep), but it's still quite easy to
> > > get races between frame completion and the forced stop procedure unless
> > > I hold on to the ivc->spinlock preventing the irq handler to run.
> > >
> > > Once I timed the transfer time for a 1920x1080@10bit frame to 5 milli-seconds
> > > at most I decided to simply wait for the current in-progress transfer to
> > > complete, as this seems the most reliable way to be able to re-start
> > > streaming without resetting the peripheral.
> > > ---
> > >  .../platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c   | 31 ++++++++++++++++++---
> > > -
> > >  1 file changed, 26 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> > > b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> > > index b167f1bab7ef..932fed38cf3f 100644
> > > --- a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> > > +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> > > @@ -297,12 +297,33 @@ static int rzv2h_ivc_start_streaming(struct vb2_queue
> > > *q, unsigned int count)
> > >  static void rzv2h_ivc_stop_streaming(struct vb2_queue *q)
> > >  {
> > >  	struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
> > > -	u32 val = 0;
> > > +	unsigned int loop = 5;
> > >  
> > > -	rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_STOP,
> > > RZV2H_IVC_REG_FM_STOP_FSTOP);
> > > -	readl_poll_timeout(ivc->base + RZV2H_IVC_REG_FM_STOP,
> > > -			   val, !(val & RZV2H_IVC_REG_FM_STOP_FSTOP),
> > > -			   10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
> > > +	/*
> > > +	 * If no frame transfer is in progress, we're done, otherwise, wait
> > > for
> > > +	 * the transfer to complete.
> > > +	 *
> > > +	 * Transferring a 1920x1080@10bit frame to the ISP takes less than 5
> > > +	 * msec so sleep for 2.5 msec (+- 25%) and give up after 5 attempts.
> > > +	 */
> > > +	for (; loop > 0; loop--) {
> > > +		unsigned int vvalid_ifp;
> > > +
> > > +		/*
> > > +		 * Inspect the ivc->vvalid_ifp variable holding the spinlock
> > > not
> > > +		 * to the race with the rzv2h_ivc_buffer_done() call in the
> > > irq
> > > +		 * handler.
> > > +		 */
> > > +		scoped_guard(spinlock_irq, &ivc->spinlock) {
> > > +			vvalid_ifp = ivc->vvalid_ifp;
> > > +		}
> > > +		if (vvalid_ifp < 2)
> > > +			break;
> > > +
> > > +		fsleep(2500);
> > > +	}
> > > +	if (!loop)
> > > +		dev_err(ivc->dev, "Failed to stop streaming\n");
> >
> > Would simply using vb2_wait_for_all_buffers() worked for your use case ? Or does
>
> Good suggestion. Let me explain why I think vb2_wait_for_all_buffers()
> doesn't really apply here, but as I realized while writing this email
> maybe I'm getting requirements wrong, so my below recollection might
> not be accurate.
>
> vb2_wait_for_all_buffers() waits until the driver doesn't call
> vb2_buffer_done() all the buffers it has been given. Not unusually,
> the IVC keeps a list of queued buffers and processes them one after
> the other.
>
> Now, assume stop_streaming() is called while a buffer transfer is in
> progress. What we can do here is
>
> 1) forcefully stop the transfer and return the current buffer in ERROR
> state and then complete in ERROR state all the buffers queued to the
> driver
> 2) wait until the transfer competes, return the current buffer in DONE
> state and then complete in ERROR state all the buffers queued to the
> driver
>
> For reasons explained above, below the commit message, I ditched
> option 1).
>
> So now, we have to wait for the current transfer in progress, return the
> current buffer and then complete all the queued ones. Only at this
> point we could call vb2_wait_for_all_buffers(), but that would be
> useless as we have completed all buffers.
>
> Now, all of this is under the assumption that we aim to complete
> buffers in the same order as they have been queued. So the currently
> in-progress buffer has to be competed first, then we can return all
> the other ones. Surprisingly I didn't find this requirement mentioned
> in the VIDIOC_STREAMOFF documentation, nor I clearly found this
> requirement being enforced in the v4l2-compliance tests.
>
> Iff we can complete buffers out of order then yes, I can complete all
> queued buffers in error state first, then let the current in-progress
> one complete and just calling vb2_wait_for_all_buffers() here.
>
> I'll check with Hans about the ordering requirement.
>

I asked Hans for clarification about the completion ordering
requirements.

My understanding is that if we could forcefully stop the current
in-progress transfer we then should return the buffer in ERROR state,
and buffers returned in ERROR state can be completed in any order.

If instead the current in-progress transfer has to be completed (as
this patch does) then the buffer has to be returned in DONE state, and
we should return it first and then complete all other buffers in ERROR
state.

So I would keep the patch as it if that's fine with you.


> > RZV2H_IVC_REG_FM_STOP mask off IRQ causing buffers to never be signalled ?
>
> This patch removes usage of RZV2H_IVC_REG_FM_STOP.
>
> Thanks
>   j
>
> >
> > >  
> > >  	rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_ERROR);
> > >  	video_device_pipeline_stop(&ivc->vdev.dev);
> > >
> > > ---
> > > base-commit: 4fbeef21f5387234111b5d52924e77757626faa5
> > > change-id: 20260331-ivc-stop-streaming-2c992277b050
> > >
> > > Best regards,
>
>



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]