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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
> ---
> 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
