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

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

[Jacopo Mondi]

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");
 
 	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,
-- 
Jacopo Mondi <jacopo.mondi@ideasonboard.com>

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

[Dan Scally]

Hi Jacopo

On 01/04/2026 16:35, Jacopo Mondi wrote:
> 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>
> ---

Sorry for missing this before. Kudos on the detailed investigation, and thanks for the fix...seems 
like a reasonable approach to me.

Reviewed-by: Daniel Scally <dan.scally@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");
>   
>   	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,

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

[Nicolas Dufresne]

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

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
RZV2H_IVC_REG_FM_STOP mask off IRQ causing buffers to never be signalled ?

>  
>  	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: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]