Re: [PATCH 12/16] rcar-vin: allow switch between capturing modes when stalling

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:53 Niklas Söderlund wrote:
> If userspace can't feed the driver with buffers as fast as the driver
> consumes them the driver will stop video capturing and wait for more
> buffers from userspace, the driver is stalled. Once it have been feed
> one or more free buffers it will recover from the stall and resume
> capturing.
> 
> Instead of of continue to capture using the same capture mode as before

s/of of continue/of continuing/

> the stall allow the driver to choose between single and continuous mode
> base on free buffer availability. Do this by stopping capturing when the

s/base/based/

> driver becomes stalled and restart capturing once it continues. By doing
> this the capture mode will be evaluated each time the driver is
> recovering from a stall.
> 
> This behavior is needed to fix a bug where continuous capturing mode is
> used, userspace is about to stop the stream and is waiting for the last
> buffers to be returned from the driver and is not queuing any new
> buffers. In this case the driver becomes stalled when there are only 3
> buffers remaining streaming will never resume since the driver is
> waiting for userspace to feed it more buffers before it can continue
> streaming.  With this fix the driver will then switch to single capture
> mode for the last 3 buffers and a deadlock is avoided. The issue can be
> demonstrated using yavta.
> 
> $ yavta -f RGB565 -s 640x480 -n 4 --capture=10  /dev/video22
> Device /dev/video22 opened.
> Device `R_Car_VIN' on `platform:e6ef1000.video' (driver 'rcar_vin') supports
> video, capture, without mplanes. Video format set: RGB565 (50424752)
> 640x480 (stride 1280) field interlaced buffer size 614400 Video format:
> RGB565 (50424752) 640x480 (stride 1280) field interlaced buffer size 614400
> 4 buffers requested.
> length: 614400 offset: 0 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb6cc7000.
> length: 614400 offset: 614400 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xb6c31000.
> length: 614400 offset: 1228800 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xb6b9b000.
> length: 614400 offset: 1843200 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xb6b05000.
> 0 (0) [-] interlaced 0 614400 B 38.240285 38.240303 12.421 fps ts mono/EoF
> 1 (1) [-] interlaced 1 614400 B 38.282329 38.282346 23.785 fps ts mono/EoF
> 2 (2) [-] interlaced 2 614400 B 38.322324 38.322338 25.003 fps ts mono/EoF
> 3 (3) [-] interlaced 3 614400 B 38.362318 38.362333 25.004 fps ts mono/EoF
> 4 (0) [-] interlaced 4 614400 B 38.402313 38.402328 25.003 fps ts mono/EoF
> 5 (1) [-] interlaced 5 614400 B 38.442307 38.442321 25.004 fps ts mono/EoF
> 6 (2) [-] interlaced 6 614400 B 38.482301 38.482316 25.004 fps ts mono/EoF
> 7 (3) [-] interlaced 7 614400 B 38.522295 38.522312 25.004 fps ts mono/EoF
> 8 (0) [-] interlaced 8 614400 B 38.562290 38.562306 25.003 fps ts mono/EoF
> <blocks forever, waiting for the last buffer>
> 
> This fix also allow the driver to switch to single capture mode if
> userspace don't feed it buffers fast enough. Or the other way around, if

s/don't/doesn't/

> userspace suddenly feeds the driver buffers faster it can switch to
> continues capturing mode.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I have a feeling that the streaming code is a bit fragile, but it doesn't seem 
that this patch is making it worse, so we can rework it later.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> f7776592b9a13d41..bd1ccb70ae2bc47e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -428,6 +428,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
> 
>  	rvin_capture_on(vin);
> 
> +	vin->state = RUNNING;
> +
>  	return 0;
>  }
> 
> @@ -906,7 +908,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	struct rvin_dev *vin = data;
>  	u32 int_status, vnms;
>  	int slot;
> -	unsigned int sequence, handled = 0;
> +	unsigned int i, sequence, handled = 0;
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&vin->qlock, flags);
> @@ -968,8 +970,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		 * the VnMBm registers.
>  		 */
>  		if (vin->continuous) {
> -			rvin_capture_off(vin);
> +			rvin_capture_stop(vin);
>  			vin_dbg(vin, "IRQ %02d: hw not ready stop\n", 
sequence);
> +
> +			/* Maybe we can continue in single capture mode */
> +			for (i = 0; i < HW_BUFFER_NUM; i++) {
> +				if (vin->queue_buf[i]) {
> +					list_add(to_buf_list(vin-
>queue_buf[i]),
> +						 &vin->buf_list);
> +					vin->queue_buf[i] = NULL;
> +				}
> +			}
> +
> +			if (!list_empty(&vin->buf_list))
> +				rvin_capture_start(vin);
>  		}
>  	} else {
>  		/*
> @@ -1054,8 +1068,7 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
>  	 * capturing if HW is ready to continue.
>  	 */
>  	if (vin->state == STALLED)
> -		if (rvin_fill_hw(vin))
> -			rvin_capture_on(vin);
> +		rvin_capture_start(vin);
> 
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  }
> @@ -1072,7 +1085,6 @@ static int rvin_start_streaming(struct vb2_queue *vq,
> unsigned int count)
> 
>  	spin_lock_irqsave(&vin->qlock, flags);
> 
> -	vin->state = RUNNING;
>  	vin->sequence = 0;
> 
>  	ret = rvin_capture_start(vin);

-- 
Regards,

Laurent Pinchart