[PATCH 0/3] rcar-vin: always run in continues mode

[PATCH 0/3] rcar-vin: always run in continues mode

[Niklas Söderlund]

Hi,

This series reworks the R-Car VIN driver to only run using its continues 
capture mode. This improves performance a lot when userspace struggles 
to keep up and queue buffers as fast as the VIN driver consumes them.  
The solution to always be able to run in continues is to introduce a 
scratch buffer inside the VIN driver which it can pad the hardware 
capture buffer ring with if it have no buffer from userspace. Using this 
scratch buffer allows the driver to not need to stop capturing when it 
run out of buffers and then restart it once it have more buffers.

Patch 1/3 removes a duplicated check in the VIN interrupt handler. Patch 
2/3 adds the allocation of the scratch buffer. And finally 3/3 drops the 
single capture mode in favor of always running in continues capture mode 
and the scratch buffer.

The series is based on top of the latest media-tree master branch and 
can be fetched from.

git://git.ragnatech.se/linux v4l2/next/vin/mode-v1

It is tested on R-Car Koelsch Gen2 board using the onboard HDMI and CVBS 
inputs. The test application v4l2-compliance pass for both inputs 
without issues or warnings. A slight adaption of these patches to the 
pending VIN Gen3 patches have been tested with great improvement in 
capture speed for buffer strained situations and no regressions in the 
vin-tests suite.

Niklas Söderlund (3):
  rcar-vin: remove duplicated check of state in irq handler
  rcar-vin: allocate a scratch buffer at stream start
  rcar-vin: use scratch buffer and always run in continuous mode

 drivers/media/platform/rcar-vin/rcar-dma.c | 212 ++++++++++-------------------
 drivers/media/platform/rcar-vin/rcar-vin.h |  10 +-
 2 files changed, 75 insertions(+), 147 deletions(-)

-- 
2.16.2

[PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

This is an error from when the driver where converted from soc-camera.
There is absolutely no gain to check the state variable two times to be
extra sure if the hardware is stopped.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 23fdff7a7370842e..b4be75d5009080f7 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	rvin_ack_interrupt(vin);
 	handled = 1;
 
-	/* Nothing to do if capture status is 'STOPPED' */
-	if (vin->state == STOPPED) {
-		vin_dbg(vin, "IRQ while state stopped\n");
-		goto done;
-	}
-
 	/* Nothing to do if capture status is 'STOPPING' */
 	if (vin->state == STOPPING) {
 		vin_dbg(vin, "IRQ while state stopping\n");
-- 
2.16.2

[PATCH 2/3] rcar-vin: allocate a scratch buffer at stream start

[Niklas Söderlund]

Before starting capturing allocate a scratch buffer which can be used by
the driver to give to the hardware if no buffers are available from
userspace. The buffer is not used in this patch but prepares for future
refactoring where the scratch buffer can be used to avoid the need to
fallback on single capture mode if userspace don't queue buffers as fast
as the VIN driver consumes them.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 19 +++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-vin.h |  4 ++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b4be75d5009080f7..8ea73cdc9a720abe 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1070,6 +1070,17 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
+	/* Allocate scratch buffer. */
+	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
+					  &vin->scratch_phys, GFP_KERNEL);
+	if (!vin->scratch) {
+		spin_lock_irqsave(&vin->qlock, flags);
+		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
+		spin_unlock_irqrestore(&vin->qlock, flags);
+		vin_err(vin, "Failed to allocate scratch buffer\n");
+		return -ENOMEM;
+	}
+
 	sd = vin_to_source(vin);
 	v4l2_subdev_call(sd, video, s_stream, 1);
 
@@ -1085,6 +1096,10 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
 
+	if (ret)
+		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
+				  vin->scratch_phys);
+
 	return ret;
 }
 
@@ -1135,6 +1150,10 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 
 	/* disable interrupts */
 	rvin_disable_interrupts(vin);
+
+	/* Free scratch buffer. */
+	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
+			  vin->scratch_phys);
 }
 
 static const struct vb2_ops rvin_qops = {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 5382078143fb3869..11a981d707c7ca47 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -102,6 +102,8 @@ struct rvin_graph_entity {
  *
  * @lock:		protects @queue
  * @queue:		vb2 buffers queue
+ * @scratch:		cpu address for scratch buffer
+ * @scratch_phys:	pysical address of the scratch buffer
  *
  * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
  *			@state
@@ -130,6 +132,8 @@ struct rvin_dev {
 
 	struct mutex lock;
 	struct vb2_queue queue;
+	void *scratch;
+	dma_addr_t scratch_phys;
 
 	spinlock_t qlock;
 	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
-- 
2.16.2

[PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Niklas Söderlund]

Instead of switching capture mode depending on how many buffers are
available use a scratch buffer and always run in continuous mode. By
using a scratch buffer the responsiveness of the capture loop is
increased as it can keep running even if there are no buffers available
from userspace.

As soon as a userspace queues a buffer it is inserted into the capture
loop and returned as soon as it is filled. This is a improvement on the
previous logic where the whole capture loop was stopped and switched to
single capture mode if userspace did not feed the VIN driver buffers at
the same time it consumed them. To make matters worse it was difficult
for the driver to reenter continues mode if it entered single mode even
if userspace started to queue buffers faster. This resulted in
suboptimal performance where if userspace where delayed for a short
period the ongoing capture would be slowed down and run in single mode
until the capturing process where restarted.

An additional effect of this change is that the capture logic can be
made much simple as we know that continues mode will always be used.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
 drivers/media/platform/rcar-vin/rcar-vin.h |   6 +-
 2 files changed, 52 insertions(+), 141 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case V4L2_FIELD_ALTERNATE:
 	case V4L2_FIELD_NONE:
-		if (vin->continuous) {
-			vnmc = VNMC_IM_ODD_EVEN;
-			progressive = true;
-		} else {
-			vnmc = VNMC_IM_ODD;
-		}
+		vnmc = VNMC_IM_ODD_EVEN;
+		progressive = true;
 		break;
 	default:
 		vnmc = VNMC_IM_ODD;
@@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
 	return rvin_read(vin, VNMS_REG) & VNMS_CA;
 }
 
-static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
-{
-	if (vin->continuous)
-		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
-
-	return 0;
-}
-
 static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
 {
 	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
@@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
 	rvin_write(vin, offset, VNMB_REG(slot));
 }
 
-/* Moves a buffer from the queue to the HW slots */
-static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
+/*
+ * 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.
+ */
+static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
 {
 	struct rvin_buffer *buf;
 	struct vb2_v4l2_buffer *vbuf;
-	dma_addr_t phys_addr_top;
-
-	if (vin->queue_buf[slot] != NULL)
-		return true;
+	dma_addr_t phys_addr;
 
-	if (list_empty(&vin->buf_list))
-		return false;
+	/* A already populated slot shall never be overwritten. */
+	if (WARN_ON(vin->queue_buf[slot] != NULL))
+		return;
 
 	vin_dbg(vin, "Filling HW slot: %d\n", slot);
 
-	/* Keep track of buffer we give to HW */
-	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
-	vbuf = &buf->vb;
-	list_del_init(to_buf_list(vbuf));
-	vin->queue_buf[slot] = vbuf;
-
-	/* Setup DMA */
-	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
-	rvin_set_slot_addr(vin, slot, phys_addr_top);
-
-	return true;
-}
-
-static bool rvin_fill_hw(struct rvin_dev *vin)
-{
-	int slot, limit;
-
-	limit = vin->continuous ? HW_BUFFER_NUM : 1;
-
-	for (slot = 0; slot < limit; slot++)
-		if (!rvin_fill_hw_slot(vin, slot))
-			return false;
-	return true;
-}
-
-static void rvin_capture_on(struct rvin_dev *vin)
-{
-	vin_dbg(vin, "Capture on in %s mode\n",
-		vin->continuous ? "continuous" : "single");
+	if (list_empty(&vin->buf_list)) {
+		vin->queue_buf[slot] = NULL;
+		phys_addr = vin->scratch_phys;
+	} else {
+		/* Keep track of buffer we give to HW */
+		buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
+		vbuf = &buf->vb;
+		list_del_init(to_buf_list(vbuf));
+		vin->queue_buf[slot] = vbuf;
+
+		/* Setup DMA */
+		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
+	}
 
-	if (vin->continuous)
-		/* Continuous Frame Capture Mode */
-		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
-	else
-		/* Single Frame Capture Mode */
-		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
+	rvin_set_slot_addr(vin, slot, phys_addr);
 }
 
 static int rvin_capture_start(struct rvin_dev *vin)
 {
-	struct rvin_buffer *buf, *node;
-	int bufs, ret;
+	int slot, ret;
 
-	/* Count number of free buffers */
-	bufs = 0;
-	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
-		bufs++;
-
-	/* Continuous capture requires more buffers then there are HW slots */
-	vin->continuous = bufs > HW_BUFFER_NUM;
-
-	if (!rvin_fill_hw(vin)) {
-		vin_err(vin, "HW not ready to start, not enough buffers available\n");
-		return -EINVAL;
-	}
+	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
+		rvin_fill_hw_slot(vin, slot);
 
 	rvin_crop_scale_comp(vin);
 
@@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
 	if (ret)
 		return ret;
 
-	rvin_capture_on(vin);
+	vin_dbg(vin, "Starting to capture\n");
+
+	/* Continuous Frame Capture Mode */
+	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
 
 	vin->state = RUNNING;
 
@@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	struct rvin_dev *vin = data;
 	u32 int_status, vnms;
 	int slot;
-	unsigned int i, sequence, handled = 0;
+	unsigned int handled = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vin->qlock, flags);
@@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
 
 	/* Prepare for capture and update state */
 	vnms = rvin_read(vin, VNMS_REG);
-	slot = rvin_get_active_slot(vin, vnms);
-	sequence = vin->sequence++;
-
-	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
-		sequence, slot,
-		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
-		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
-		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
-		!list_empty(&vin->buf_list));
-
-	/* HW have written to a slot that is not prepared we are in trouble */
-	if (WARN_ON((vin->queue_buf[slot] == NULL)))
-		goto done;
+	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
 
 	/* Capture frame */
-	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
-	vin->queue_buf[slot]->sequence = sequence;
-	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
-	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
-	vin->queue_buf[slot] = NULL;
-
-	/* Prepare for next frame */
-	if (!rvin_fill_hw(vin)) {
-
-		/*
-		 * Can't supply HW with new buffers fast enough. Halt
-		 * capture until more buffers are available.
-		 */
-		vin->state = STALLED;
-
-		/*
-		 * The continuous capturing requires an explicit stop
-		 * operation when there is no buffer to be set into
-		 * the VnMBm registers.
-		 */
-		if (vin->continuous) {
-			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);
-		}
+	if (vin->queue_buf[slot]) {
+		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
+		vin->queue_buf[slot]->sequence = vin->sequence;
+		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
+		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
+				VB2_BUF_STATE_DONE);
+		vin->queue_buf[slot] = NULL;
 	} else {
-		/*
-		 * The single capturing requires an explicit capture
-		 * operation to fetch the next frame.
-		 */
-		if (!vin->continuous)
-			rvin_capture_on(vin);
+		/* Scratch buffer was used, dropping frame. */
+		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
 	}
+
+	vin->sequence++;
+
+	/* Prepare for next frame */
+	rvin_fill_hw_slot(vin, slot);
 done:
 	spin_unlock_irqrestore(&vin->qlock, flags);
 
@@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
 
 	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
 
-	/*
-	 * If capture is stalled add buffer to HW and restart
-	 * capturing if HW is ready to continue.
-	 */
-	if (vin->state == STALLED)
-		rvin_capture_start(vin);
-
 	spin_unlock_irqrestore(&vin->qlock, flags);
 }
 
@@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
 	q->ops = &rvin_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
-	q->min_buffers_needed = 1;
+	q->min_buffers_needed = 4;
 	q->dev = vin->dev;
 
 	ret = vb2_queue_init(q);
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 11a981d707c7ca47..1adc1b809f761e71 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -38,13 +38,11 @@ enum chip_id {
 /**
  * STOPPED  - No operation in progress
  * RUNNING  - Operation in progress have buffers
- * STALLED  - No operation in progress have no buffers
  * STOPPING - Stopping operation
  */
 enum rvin_dma_state {
 	STOPPED = 0,
 	RUNNING,
-	STALLED,
 	STOPPING,
 };
 
@@ -105,11 +103,10 @@ struct rvin_graph_entity {
  * @scratch:		cpu address for scratch buffer
  * @scratch_phys:	pysical address of the scratch buffer
  *
- * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
+ * @qlock:		protects @queue_buf, @buf_list, @sequence
  *			@state
  * @queue_buf:		Keeps track of buffers given to HW slot
  * @buf_list:		list of queued buffers
- * @continuous:		tracks if active operation is continuous or single mode
  * @sequence:		V4L2 buffers sequence number
  * @state:		keeps track of operation state
  *
@@ -138,7 +135,6 @@ struct rvin_dev {
 	spinlock_t qlock;
 	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
 	struct list_head buf_list;
-	bool continuous;
 	unsigned int sequence;
 	enum rvin_dma_state state;
 
-- 
2.16.2

Re: [PATCH 2/3] rcar-vin: allocate a scratch buffer at stream start

[jacopo mondi]

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

Hi Niklas,

On Sat, Mar 10, 2018 at 01:09:52AM +0100, Niklas Söderlund wrote:
> Before starting capturing allocate a scratch buffer which can be used by
> the driver to give to the hardware if no buffers are available from
> userspace. The buffer is not used in this patch but prepares for future
> refactoring where the scratch buffer can be used to avoid the need to
> fallback on single capture mode if userspace don't queue buffers as fast
> as the VIN driver consumes them.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 19 +++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h |  4 ++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b4be75d5009080f7..8ea73cdc9a720abe 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1070,6 +1070,17 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	unsigned long flags;
>  	int ret;
>
> +	/* Allocate scratch buffer. */
> +	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> +					  &vin->scratch_phys, GFP_KERNEL);
> +	if (!vin->scratch) {
> +		spin_lock_irqsave(&vin->qlock, flags);
> +		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> +		spin_unlock_irqrestore(&vin->qlock, flags);
> +		vin_err(vin, "Failed to allocate scratch buffer\n");
> +		return -ENOMEM;
> +	}
> +
>  	sd = vin_to_source(vin);
>  	v4l2_subdev_call(sd, video, s_stream, 1);
>
> @@ -1085,6 +1096,10 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>
> +	if (ret)
> +		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> +				  vin->scratch_phys);
> +
>  	return ret;
>  }
>
> @@ -1135,6 +1150,10 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>
>  	/* disable interrupts */
>  	rvin_disable_interrupts(vin);
> +
> +	/* Free scratch buffer. */
> +	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> +			  vin->scratch_phys);
>  }
>
>  static const struct vb2_ops rvin_qops = {
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 5382078143fb3869..11a981d707c7ca47 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -102,6 +102,8 @@ struct rvin_graph_entity {
>   *
>   * @lock:		protects @queue
>   * @queue:		vb2 buffers queue
> + * @scratch:		cpu address for scratch buffer
> + * @scratch_phys:	pysical address of the scratch buffer

Nitpicking: physical

Thanks
   j

>   *
>   * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
>   *			@state
> @@ -130,6 +132,8 @@ struct rvin_dev {
>
>  	struct mutex lock;
>  	struct vb2_queue queue;
> +	void *scratch;
> +	dma_addr_t scratch_phys;
>
>  	spinlock_t qlock;
>  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> --
> 2.16.2
>

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

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[jacopo mondi]

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

Hi Niklas,
  a few comments below

On Sat, Mar 10, 2018 at 01:09:53AM +0100, Niklas Söderlund wrote:
> Instead of switching capture mode depending on how many buffers are
> available use a scratch buffer and always run in continuous mode. By
> using a scratch buffer the responsiveness of the capture loop is
> increased as it can keep running even if there are no buffers available
> from userspace.
>
> As soon as a userspace queues a buffer it is inserted into the capture
> loop and returned as soon as it is filled. This is a improvement on the
> previous logic where the whole capture loop was stopped and switched to
> single capture mode if userspace did not feed the VIN driver buffers at
> the same time it consumed them. To make matters worse it was difficult
> for the driver to reenter continues mode if it entered single mode even
> if userspace started to queue buffers faster. This resulted in
> suboptimal performance where if userspace where delayed for a short
> period the ongoing capture would be slowed down and run in single mode
> until the capturing process where restarted.
>
> An additional effect of this change is that the capture logic can be
> made much simple as we know that continues mode will always be used.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
>  drivers/media/platform/rcar-vin/rcar-vin.h |   6 +-
>  2 files changed, 52 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case V4L2_FIELD_ALTERNATE:
>  	case V4L2_FIELD_NONE:
> -		if (vin->continuous) {
> -			vnmc = VNMC_IM_ODD_EVEN;
> -			progressive = true;
> -		} else {
> -			vnmc = VNMC_IM_ODD;
> -		}
> +		vnmc = VNMC_IM_ODD_EVEN;
> +		progressive = true;
>  		break;
>  	default:
>  		vnmc = VNMC_IM_ODD;
> @@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>  }
>
> -static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> -{
> -	if (vin->continuous)
> -		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> -
> -	return 0;
> -}
> -
>  static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>  {
>  	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> @@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>  	rvin_write(vin, offset, VNMB_REG(slot));
>  }
>
> -/* Moves a buffer from the queue to the HW slots */
> -static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> +/*
> + * 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.
> + */
> +static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  {
>  	struct rvin_buffer *buf;
>  	struct vb2_v4l2_buffer *vbuf;
> -	dma_addr_t phys_addr_top;
> -
> -	if (vin->queue_buf[slot] != NULL)
> -		return true;
> +	dma_addr_t phys_addr;
>
> -	if (list_empty(&vin->buf_list))
> -		return false;
> +	/* A already populated slot shall never be overwritten. */
> +	if (WARN_ON(vin->queue_buf[slot] != NULL))
> +		return;
>
>  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
>
> -	/* Keep track of buffer we give to HW */
> -	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> -	vbuf = &buf->vb;
> -	list_del_init(to_buf_list(vbuf));
> -	vin->queue_buf[slot] = vbuf;
> -
> -	/* Setup DMA */
> -	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> -	rvin_set_slot_addr(vin, slot, phys_addr_top);
> -
> -	return true;
> -}
> -
> -static bool rvin_fill_hw(struct rvin_dev *vin)
> -{
> -	int slot, limit;
> -
> -	limit = vin->continuous ? HW_BUFFER_NUM : 1;
> -
> -	for (slot = 0; slot < limit; slot++)
> -		if (!rvin_fill_hw_slot(vin, slot))
> -			return false;
> -	return true;
> -}
> -
> -static void rvin_capture_on(struct rvin_dev *vin)
> -{
> -	vin_dbg(vin, "Capture on in %s mode\n",
> -		vin->continuous ? "continuous" : "single");
> +	if (list_empty(&vin->buf_list)) {
> +		vin->queue_buf[slot] = NULL;
> +		phys_addr = vin->scratch_phys;

I guess it is not an issue if MB1/MB2 and MB3 they all get pointed to
the same scratch buffer, right?

> +	} else {
> +		/* Keep track of buffer we give to HW */
> +		buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> +		vbuf = &buf->vb;
> +		list_del_init(to_buf_list(vbuf));
> +		vin->queue_buf[slot] = vbuf;
> +
> +		/* Setup DMA */
> +		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> +	}
>
> -	if (vin->continuous)
> -		/* Continuous Frame Capture Mode */
> -		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> -	else
> -		/* Single Frame Capture Mode */
> -		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> +	rvin_set_slot_addr(vin, slot, phys_addr);
>  }
>
>  static int rvin_capture_start(struct rvin_dev *vin)
>  {
> -	struct rvin_buffer *buf, *node;
> -	int bufs, ret;
> +	int slot, ret;
>
> -	/* Count number of free buffers */
> -	bufs = 0;
> -	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
> -		bufs++;
> -
> -	/* Continuous capture requires more buffers then there are HW slots */
> -	vin->continuous = bufs > HW_BUFFER_NUM;
> -
> -	if (!rvin_fill_hw(vin)) {
> -		vin_err(vin, "HW not ready to start, not enough buffers available\n");
> -		return -EINVAL;
> -	}
> +	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> +		rvin_fill_hw_slot(vin, slot);
>
>  	rvin_crop_scale_comp(vin);
>
> @@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
>  	if (ret)
>  		return ret;
>
> -	rvin_capture_on(vin);
> +	vin_dbg(vin, "Starting to capture\n");
> +
> +	/* Continuous Frame Capture Mode */
> +	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
>
>  	vin->state = RUNNING;
>
> @@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	struct rvin_dev *vin = data;
>  	u32 int_status, vnms;
>  	int slot;
> -	unsigned int i, sequence, handled = 0;
> +	unsigned int handled = 0;
>  	unsigned long flags;
>
>  	spin_lock_irqsave(&vin->qlock, flags);
> @@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
>
>  	/* Prepare for capture and update state */
>  	vnms = rvin_read(vin, VNMS_REG);
> -	slot = rvin_get_active_slot(vin, vnms);
> -	sequence = vin->sequence++;
> -
> -	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> -		sequence, slot,
> -		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
> -		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
> -		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
> -		!list_empty(&vin->buf_list));
> -
> -	/* HW have written to a slot that is not prepared we are in trouble */
> -	if (WARN_ON((vin->queue_buf[slot] == NULL)))
> -		goto done;
> +	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>
>  	/* Capture frame */
> -	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> -	vin->queue_buf[slot]->sequence = sequence;
> -	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> -	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> -	vin->queue_buf[slot] = NULL;
> -
> -	/* Prepare for next frame */
> -	if (!rvin_fill_hw(vin)) {
> -
> -		/*
> -		 * Can't supply HW with new buffers fast enough. Halt
> -		 * capture until more buffers are available.
> -		 */
> -		vin->state = STALLED;
> -
> -		/*
> -		 * The continuous capturing requires an explicit stop
> -		 * operation when there is no buffer to be set into
> -		 * the VnMBm registers.
> -		 */
> -		if (vin->continuous) {
> -			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);
> -		}
> +	if (vin->queue_buf[slot]) {
> +		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> +		vin->queue_buf[slot]->sequence = vin->sequence;
> +		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> +		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
> +				VB2_BUF_STATE_DONE);
> +		vin->queue_buf[slot] = NULL;
>  	} else {
> -		/*
> -		 * The single capturing requires an explicit capture
> -		 * operation to fetch the next frame.
> -		 */
> -		if (!vin->continuous)
> -			rvin_capture_on(vin);
> +		/* Scratch buffer was used, dropping frame. */
> +		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);

Nit: even if that's debug output, you're going to prin out this
message every discarded frame. Isn't this a bit too much?

>  	}
> +
> +	vin->sequence++;

Sequence counter is incremented even if frame is discarded. Is this
intended?

> +
> +	/* Prepare for next frame */
> +	rvin_fill_hw_slot(vin, slot);
>  done:
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>
> @@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
>
>  	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
>
> -	/*
> -	 * If capture is stalled add buffer to HW and restart
> -	 * capturing if HW is ready to continue.
> -	 */
> -	if (vin->state == STALLED)
> -		rvin_capture_start(vin);
> -
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  }
>
> @@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
>  	q->ops = &rvin_qops;
>  	q->mem_ops = &vb2_dma_contig_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_buffers_needed = 1;
> +	q->min_buffers_needed = 4;
>  	q->dev = vin->dev;
>
>  	ret = vb2_queue_init(q);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 11a981d707c7ca47..1adc1b809f761e71 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -38,13 +38,11 @@ enum chip_id {
>  /**
>   * STOPPED  - No operation in progress
>   * RUNNING  - Operation in progress have buffers
> - * STALLED  - No operation in progress have no buffers
>   * STOPPING - Stopping operation
>   */
>  enum rvin_dma_state {
>  	STOPPED = 0,
>  	RUNNING,
> -	STALLED,
>  	STOPPING,
>  };
>
> @@ -105,11 +103,10 @@ struct rvin_graph_entity {
>   * @scratch:		cpu address for scratch buffer
>   * @scratch_phys:	pysical address of the scratch buffer
>   *
> - * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
> + * @qlock:		protects @queue_buf, @buf_list, @sequence
>   *			@state
>   * @queue_buf:		Keeps track of buffers given to HW slot
>   * @buf_list:		list of queued buffers
> - * @continuous:		tracks if active operation is continuous or single mode
>   * @sequence:		V4L2 buffers sequence number
>   * @state:		keeps track of operation state
>   *
> @@ -138,7 +135,6 @@ struct rvin_dev {
>  	spinlock_t qlock;
>  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
>  	struct list_head buf_list;
> -	bool continuous;
>  	unsigned int sequence;
>  	enum rvin_dma_state state;

With the above clarified, for the whole series:

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>
> --
> 2.16.2
>

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

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-03-12 15:38:12 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>   a few comments below
> 
> On Sat, Mar 10, 2018 at 01:09:53AM +0100, Niklas Söderlund wrote:
> > Instead of switching capture mode depending on how many buffers are
> > available use a scratch buffer and always run in continuous mode. By
> > using a scratch buffer the responsiveness of the capture loop is
> > increased as it can keep running even if there are no buffers available
> > from userspace.
> >
> > As soon as a userspace queues a buffer it is inserted into the capture
> > loop and returned as soon as it is filled. This is a improvement on the
> > previous logic where the whole capture loop was stopped and switched to
> > single capture mode if userspace did not feed the VIN driver buffers at
> > the same time it consumed them. To make matters worse it was difficult
> > for the driver to reenter continues mode if it entered single mode even
> > if userspace started to queue buffers faster. This resulted in
> > suboptimal performance where if userspace where delayed for a short
> > period the ongoing capture would be slowed down and run in single mode
> > until the capturing process where restarted.
> >
> > An additional effect of this change is that the capture logic can be
> > made much simple as we know that continues mode will always be used.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
> >  drivers/media/platform/rcar-vin/rcar-vin.h |   6 +-
> >  2 files changed, 52 insertions(+), 141 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		break;
> >  	case V4L2_FIELD_ALTERNATE:
> >  	case V4L2_FIELD_NONE:
> > -		if (vin->continuous) {
> > -			vnmc = VNMC_IM_ODD_EVEN;
> > -			progressive = true;
> > -		} else {
> > -			vnmc = VNMC_IM_ODD;
> > -		}
> > +		vnmc = VNMC_IM_ODD_EVEN;
> > +		progressive = true;
> >  		break;
> >  	default:
> >  		vnmc = VNMC_IM_ODD;
> > @@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >  }
> >
> > -static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> > -{
> > -	if (vin->continuous)
> > -		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> > -
> > -	return 0;
> > -}
> > -
> >  static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> >  {
> >  	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > @@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >  	rvin_write(vin, offset, VNMB_REG(slot));
> >  }
> >
> > -/* Moves a buffer from the queue to the HW slots */
> > -static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> > +/*
> > + * 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.
> > + */
> > +static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> >  {
> >  	struct rvin_buffer *buf;
> >  	struct vb2_v4l2_buffer *vbuf;
> > -	dma_addr_t phys_addr_top;
> > -
> > -	if (vin->queue_buf[slot] != NULL)
> > -		return true;
> > +	dma_addr_t phys_addr;
> >
> > -	if (list_empty(&vin->buf_list))
> > -		return false;
> > +	/* A already populated slot shall never be overwritten. */
> > +	if (WARN_ON(vin->queue_buf[slot] != NULL))
> > +		return;
> >
> >  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
> >
> > -	/* Keep track of buffer we give to HW */
> > -	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> > -	vbuf = &buf->vb;
> > -	list_del_init(to_buf_list(vbuf));
> > -	vin->queue_buf[slot] = vbuf;
> > -
> > -	/* Setup DMA */
> > -	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> > -	rvin_set_slot_addr(vin, slot, phys_addr_top);
> > -
> > -	return true;
> > -}
> > -
> > -static bool rvin_fill_hw(struct rvin_dev *vin)
> > -{
> > -	int slot, limit;
> > -
> > -	limit = vin->continuous ? HW_BUFFER_NUM : 1;
> > -
> > -	for (slot = 0; slot < limit; slot++)
> > -		if (!rvin_fill_hw_slot(vin, slot))
> > -			return false;
> > -	return true;
> > -}
> > -
> > -static void rvin_capture_on(struct rvin_dev *vin)
> > -{
> > -	vin_dbg(vin, "Capture on in %s mode\n",
> > -		vin->continuous ? "continuous" : "single");
> > +	if (list_empty(&vin->buf_list)) {
> > +		vin->queue_buf[slot] = NULL;
> > +		phys_addr = vin->scratch_phys;
> 
> I guess it is not an issue if MB1/MB2 and MB3 they all get pointed to
> the same scratch buffer, right?

Correct this is no issue. The data in the scratch buffer will if it's 
used in all of the MBx registers possibly contain two partial captures 
in different stages but as this buffer is never returned to userspace 
nor used for anything in the driver this is not a problem. The only use 
of the scratch buffer is to have somewhere to write the capture if we 
have no buffers from userspace, the frame/field captured to the scratch 
buffer will just be dropped.

> 
> > +	} else {
> > +		/* Keep track of buffer we give to HW */
> > +		buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> > +		vbuf = &buf->vb;
> > +		list_del_init(to_buf_list(vbuf));
> > +		vin->queue_buf[slot] = vbuf;
> > +
> > +		/* Setup DMA */
> > +		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> > +	}
> >
> > -	if (vin->continuous)
> > -		/* Continuous Frame Capture Mode */
> > -		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> > -	else
> > -		/* Single Frame Capture Mode */
> > -		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> > +	rvin_set_slot_addr(vin, slot, phys_addr);
> >  }
> >
> >  static int rvin_capture_start(struct rvin_dev *vin)
> >  {
> > -	struct rvin_buffer *buf, *node;
> > -	int bufs, ret;
> > +	int slot, ret;
> >
> > -	/* Count number of free buffers */
> > -	bufs = 0;
> > -	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
> > -		bufs++;
> > -
> > -	/* Continuous capture requires more buffers then there are HW slots */
> > -	vin->continuous = bufs > HW_BUFFER_NUM;
> > -
> > -	if (!rvin_fill_hw(vin)) {
> > -		vin_err(vin, "HW not ready to start, not enough buffers available\n");
> > -		return -EINVAL;
> > -	}
> > +	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> > +		rvin_fill_hw_slot(vin, slot);
> >
> >  	rvin_crop_scale_comp(vin);
> >
> > @@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
> >  	if (ret)
> >  		return ret;
> >
> > -	rvin_capture_on(vin);
> > +	vin_dbg(vin, "Starting to capture\n");
> > +
> > +	/* Continuous Frame Capture Mode */
> > +	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> >
> >  	vin->state = RUNNING;
> >
> > @@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	struct rvin_dev *vin = data;
> >  	u32 int_status, vnms;
> >  	int slot;
> > -	unsigned int i, sequence, handled = 0;
> > +	unsigned int handled = 0;
> >  	unsigned long flags;
> >
> >  	spin_lock_irqsave(&vin->qlock, flags);
> > @@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >
> >  	/* Prepare for capture and update state */
> >  	vnms = rvin_read(vin, VNMS_REG);
> > -	slot = rvin_get_active_slot(vin, vnms);
> > -	sequence = vin->sequence++;
> > -
> > -	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> > -		sequence, slot,
> > -		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
> > -		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
> > -		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
> > -		!list_empty(&vin->buf_list));
> > -
> > -	/* HW have written to a slot that is not prepared we are in trouble */
> > -	if (WARN_ON((vin->queue_buf[slot] == NULL)))
> > -		goto done;
> > +	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >
> >  	/* Capture frame */
> > -	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > -	vin->queue_buf[slot]->sequence = sequence;
> > -	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > -	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > -	vin->queue_buf[slot] = NULL;
> > -
> > -	/* Prepare for next frame */
> > -	if (!rvin_fill_hw(vin)) {
> > -
> > -		/*
> > -		 * Can't supply HW with new buffers fast enough. Halt
> > -		 * capture until more buffers are available.
> > -		 */
> > -		vin->state = STALLED;
> > -
> > -		/*
> > -		 * The continuous capturing requires an explicit stop
> > -		 * operation when there is no buffer to be set into
> > -		 * the VnMBm registers.
> > -		 */
> > -		if (vin->continuous) {
> > -			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);
> > -		}
> > +	if (vin->queue_buf[slot]) {
> > +		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > +		vin->queue_buf[slot]->sequence = vin->sequence;
> > +		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > +		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
> > +				VB2_BUF_STATE_DONE);
> > +		vin->queue_buf[slot] = NULL;
> >  	} else {
> > -		/*
> > -		 * The single capturing requires an explicit capture
> > -		 * operation to fetch the next frame.
> > -		 */
> > -		if (!vin->continuous)
> > -			rvin_capture_on(vin);
> > +		/* Scratch buffer was used, dropping frame. */
> > +		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> 
> Nit: even if that's debug output, you're going to prin out this
> message every discarded frame. Isn't this a bit too much?

With DEBUG enabled the driver will also print a dev_dbg() for every 
frame it manage to capture :-) So yes it is a lot of output but I found 
it really useful when understanding the interaction with userspace when 
capturing so I would prefer to keep debug output as we already print a 
debug message for each frame we do manage to capture.

> 
> >  	}
> > +
> > +	vin->sequence++;
> 
> Sequence counter is incremented even if frame is discarded. Is this
> intended?

Yes this is intentional. This mechanism informs userspace that one or 
more frame have been dropped. If a application sees a gap in the 
sequence numbers it knows if frames have been dropped. And by looking at 
how large the diff is between the two sequence numbers it do have is it 
knows how many frames where dropped.

However I talked to Hans about this last week and he informed me that 
not all drivers manage to detect dropped frames and inform the 
application of this using sequence numbers. So in practice an 
application can not depend on this information. But I think it is worth 
it in this driver where we can detect it do to the right thing.

As a side not this helped find a issue with this patch-set as 
v4l2-compliance looks at the sequence numbers and warned me that 
something where off and it turned out I had forget to change the 
in_buffers_needed  in the struct vb2_queue to make the most of this 
patch to always run in continues mode :-)

> 
> > +
> > +	/* Prepare for next frame */
> > +	rvin_fill_hw_slot(vin, slot);
> >  done:
> >  	spin_unlock_irqrestore(&vin->qlock, flags);
> >
> > @@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
> >
> >  	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
> >
> > -	/*
> > -	 * If capture is stalled add buffer to HW and restart
> > -	 * capturing if HW is ready to continue.
> > -	 */
> > -	if (vin->state == STALLED)
> > -		rvin_capture_start(vin);
> > -
> >  	spin_unlock_irqrestore(&vin->qlock, flags);
> >  }
> >
> > @@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
> >  	q->ops = &rvin_qops;
> >  	q->mem_ops = &vb2_dma_contig_memops;
> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > -	q->min_buffers_needed = 1;
> > +	q->min_buffers_needed = 4;
> >  	q->dev = vin->dev;
> >
> >  	ret = vb2_queue_init(q);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 11a981d707c7ca47..1adc1b809f761e71 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -38,13 +38,11 @@ enum chip_id {
> >  /**
> >   * STOPPED  - No operation in progress
> >   * RUNNING  - Operation in progress have buffers
> > - * STALLED  - No operation in progress have no buffers
> >   * STOPPING - Stopping operation
> >   */
> >  enum rvin_dma_state {
> >  	STOPPED = 0,
> >  	RUNNING,
> > -	STALLED,
> >  	STOPPING,
> >  };
> >
> > @@ -105,11 +103,10 @@ struct rvin_graph_entity {
> >   * @scratch:		cpu address for scratch buffer
> >   * @scratch_phys:	pysical address of the scratch buffer
> >   *
> > - * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
> > + * @qlock:		protects @queue_buf, @buf_list, @sequence
> >   *			@state
> >   * @queue_buf:		Keeps track of buffers given to HW slot
> >   * @buf_list:		list of queued buffers
> > - * @continuous:		tracks if active operation is continuous or single mode
> >   * @sequence:		V4L2 buffers sequence number
> >   * @state:		keeps track of operation state
> >   *
> > @@ -138,7 +135,6 @@ struct rvin_dev {
> >  	spinlock_t qlock;
> >  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> >  	struct list_head buf_list;
> > -	bool continuous;
> >  	unsigned int sequence;
> >  	enum rvin_dma_state state;
> 
> With the above clarified, for the whole series:
> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thank you!

> 
> Thanks
>    j
> 
> >
> > --
> > 2.16.2
> >



-- 
Regards,
Niklas Söderlund

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-03-12 15:38:12 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>   a few comments below
> 
> On Sat, Mar 10, 2018 at 01:09:53AM +0100, Niklas S�derlund wrote:
> > Instead of switching capture mode depending on how many buffers are
> > available use a scratch buffer and always run in continuous mode. By
> > using a scratch buffer the responsiveness of the capture loop is
> > increased as it can keep running even if there are no buffers available
> > from userspace.
> >
> > As soon as a userspace queues a buffer it is inserted into the capture
> > loop and returned as soon as it is filled. This is a improvement on the
> > previous logic where the whole capture loop was stopped and switched to
> > single capture mode if userspace did not feed the VIN driver buffers at
> > the same time it consumed them. To make matters worse it was difficult
> > for the driver to reenter continues mode if it entered single mode even
> > if userspace started to queue buffers faster. This resulted in
> > suboptimal performance where if userspace where delayed for a short
> > period the ongoing capture would be slowed down and run in single mode
> > until the capturing process where restarted.
> >
> > An additional effect of this change is that the capture logic can be
> > made much simple as we know that continues mode will always be used.
> >
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
> >  drivers/media/platform/rcar-vin/rcar-vin.h |   6 +-
> >  2 files changed, 52 insertions(+), 141 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		break;
> >  	case V4L2_FIELD_ALTERNATE:
> >  	case V4L2_FIELD_NONE:
> > -		if (vin->continuous) {
> > -			vnmc = VNMC_IM_ODD_EVEN;
> > -			progressive = true;
> > -		} else {
> > -			vnmc = VNMC_IM_ODD;
> > -		}
> > +		vnmc = VNMC_IM_ODD_EVEN;
> > +		progressive = true;
> >  		break;
> >  	default:
> >  		vnmc = VNMC_IM_ODD;
> > @@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >  }
> >
> > -static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> > -{
> > -	if (vin->continuous)
> > -		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> > -
> > -	return 0;
> > -}
> > -
> >  static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> >  {
> >  	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > @@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >  	rvin_write(vin, offset, VNMB_REG(slot));
> >  }
> >
> > -/* Moves a buffer from the queue to the HW slots */
> > -static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> > +/*
> > + * 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.
> > + */
> > +static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> >  {
> >  	struct rvin_buffer *buf;
> >  	struct vb2_v4l2_buffer *vbuf;
> > -	dma_addr_t phys_addr_top;
> > -
> > -	if (vin->queue_buf[slot] != NULL)
> > -		return true;
> > +	dma_addr_t phys_addr;
> >
> > -	if (list_empty(&vin->buf_list))
> > -		return false;
> > +	/* A already populated slot shall never be overwritten. */
> > +	if (WARN_ON(vin->queue_buf[slot] != NULL))
> > +		return;
> >
> >  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
> >
> > -	/* Keep track of buffer we give to HW */
> > -	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> > -	vbuf = &buf->vb;
> > -	list_del_init(to_buf_list(vbuf));
> > -	vin->queue_buf[slot] = vbuf;
> > -
> > -	/* Setup DMA */
> > -	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> > -	rvin_set_slot_addr(vin, slot, phys_addr_top);
> > -
> > -	return true;
> > -}
> > -
> > -static bool rvin_fill_hw(struct rvin_dev *vin)
> > -{
> > -	int slot, limit;
> > -
> > -	limit = vin->continuous ? HW_BUFFER_NUM : 1;
> > -
> > -	for (slot = 0; slot < limit; slot++)
> > -		if (!rvin_fill_hw_slot(vin, slot))
> > -			return false;
> > -	return true;
> > -}
> > -
> > -static void rvin_capture_on(struct rvin_dev *vin)
> > -{
> > -	vin_dbg(vin, "Capture on in %s mode\n",
> > -		vin->continuous ? "continuous" : "single");
> > +	if (list_empty(&vin->buf_list)) {
> > +		vin->queue_buf[slot] = NULL;
> > +		phys_addr = vin->scratch_phys;
> 
> I guess it is not an issue if MB1/MB2 and MB3 they all get pointed to
> the same scratch buffer, right?

Correct this is no issue. The data in the scratch buffer will if it's 
used in all of the MBx registers possibly contain two partial captures 
in different stages but as this buffer is never returned to userspace 
nor used for anything in the driver this is not a problem. The only use 
of the scratch buffer is to have somewhere to write the capture if we 
have no buffers from userspace, the frame/field captured to the scratch 
buffer will just be dropped.

> 
> > +	} else {
> > +		/* Keep track of buffer we give to HW */
> > +		buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> > +		vbuf = &buf->vb;
> > +		list_del_init(to_buf_list(vbuf));
> > +		vin->queue_buf[slot] = vbuf;
> > +
> > +		/* Setup DMA */
> > +		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> > +	}
> >
> > -	if (vin->continuous)
> > -		/* Continuous Frame Capture Mode */
> > -		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> > -	else
> > -		/* Single Frame Capture Mode */
> > -		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> > +	rvin_set_slot_addr(vin, slot, phys_addr);
> >  }
> >
> >  static int rvin_capture_start(struct rvin_dev *vin)
> >  {
> > -	struct rvin_buffer *buf, *node;
> > -	int bufs, ret;
> > +	int slot, ret;
> >
> > -	/* Count number of free buffers */
> > -	bufs = 0;
> > -	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
> > -		bufs++;
> > -
> > -	/* Continuous capture requires more buffers then there are HW slots */
> > -	vin->continuous = bufs > HW_BUFFER_NUM;
> > -
> > -	if (!rvin_fill_hw(vin)) {
> > -		vin_err(vin, "HW not ready to start, not enough buffers available\n");
> > -		return -EINVAL;
> > -	}
> > +	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> > +		rvin_fill_hw_slot(vin, slot);
> >
> >  	rvin_crop_scale_comp(vin);
> >
> > @@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
> >  	if (ret)
> >  		return ret;
> >
> > -	rvin_capture_on(vin);
> > +	vin_dbg(vin, "Starting to capture\n");
> > +
> > +	/* Continuous Frame Capture Mode */
> > +	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> >
> >  	vin->state = RUNNING;
> >
> > @@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	struct rvin_dev *vin = data;
> >  	u32 int_status, vnms;
> >  	int slot;
> > -	unsigned int i, sequence, handled = 0;
> > +	unsigned int handled = 0;
> >  	unsigned long flags;
> >
> >  	spin_lock_irqsave(&vin->qlock, flags);
> > @@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >
> >  	/* Prepare for capture and update state */
> >  	vnms = rvin_read(vin, VNMS_REG);
> > -	slot = rvin_get_active_slot(vin, vnms);
> > -	sequence = vin->sequence++;
> > -
> > -	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> > -		sequence, slot,
> > -		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
> > -		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
> > -		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
> > -		!list_empty(&vin->buf_list));
> > -
> > -	/* HW have written to a slot that is not prepared we are in trouble */
> > -	if (WARN_ON((vin->queue_buf[slot] == NULL)))
> > -		goto done;
> > +	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >
> >  	/* Capture frame */
> > -	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > -	vin->queue_buf[slot]->sequence = sequence;
> > -	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > -	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > -	vin->queue_buf[slot] = NULL;
> > -
> > -	/* Prepare for next frame */
> > -	if (!rvin_fill_hw(vin)) {
> > -
> > -		/*
> > -		 * Can't supply HW with new buffers fast enough. Halt
> > -		 * capture until more buffers are available.
> > -		 */
> > -		vin->state = STALLED;
> > -
> > -		/*
> > -		 * The continuous capturing requires an explicit stop
> > -		 * operation when there is no buffer to be set into
> > -		 * the VnMBm registers.
> > -		 */
> > -		if (vin->continuous) {
> > -			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);
> > -		}
> > +	if (vin->queue_buf[slot]) {
> > +		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > +		vin->queue_buf[slot]->sequence = vin->sequence;
> > +		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > +		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
> > +				VB2_BUF_STATE_DONE);
> > +		vin->queue_buf[slot] = NULL;
> >  	} else {
> > -		/*
> > -		 * The single capturing requires an explicit capture
> > -		 * operation to fetch the next frame.
> > -		 */
> > -		if (!vin->continuous)
> > -			rvin_capture_on(vin);
> > +		/* Scratch buffer was used, dropping frame. */
> > +		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> 
> Nit: even if that's debug output, you're going to prin out this
> message every discarded frame. Isn't this a bit too much?

With DEBUG enabled the driver will also print a dev_dbg() for every 
frame it manage to capture :-) So yes it is a lot of output but I found 
it really useful when understanding the interaction with userspace when 
capturing so I would prefer to keep debug output as we already print a 
debug message for each frame we do manage to capture.

> 
> >  	}
> > +
> > +	vin->sequence++;
> 
> Sequence counter is incremented even if frame is discarded. Is this
> intended?

Yes this is intentional. This mechanism informs userspace that one or 
more frame have been dropped. If a application sees a gap in the 
sequence numbers it knows if frames have been dropped. And by looking at 
how large the diff is between the two sequence numbers it do have is it 
knows how many frames where dropped.

However I talked to Hans about this last week and he informed me that 
not all drivers manage to detect dropped frames and inform the 
application of this using sequence numbers. So in practice an 
application can not depend on this information. But I think it is worth 
it in this driver where we can detect it do to the right thing.

As a side not this helped find a issue with this patch-set as 
v4l2-compliance looks at the sequence numbers and warned me that 
something where off and it turned out I had forget to change the 
in_buffers_needed  in the struct vb2_queue to make the most of this 
patch to always run in continues mode :-)

> 
> > +
> > +	/* Prepare for next frame */
> > +	rvin_fill_hw_slot(vin, slot);
> >  done:
> >  	spin_unlock_irqrestore(&vin->qlock, flags);
> >
> > @@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
> >
> >  	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
> >
> > -	/*
> > -	 * If capture is stalled add buffer to HW and restart
> > -	 * capturing if HW is ready to continue.
> > -	 */
> > -	if (vin->state == STALLED)
> > -		rvin_capture_start(vin);
> > -
> >  	spin_unlock_irqrestore(&vin->qlock, flags);
> >  }
> >
> > @@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
> >  	q->ops = &rvin_qops;
> >  	q->mem_ops = &vb2_dma_contig_memops;
> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > -	q->min_buffers_needed = 1;
> > +	q->min_buffers_needed = 4;
> >  	q->dev = vin->dev;
> >
> >  	ret = vb2_queue_init(q);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 11a981d707c7ca47..1adc1b809f761e71 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -38,13 +38,11 @@ enum chip_id {
> >  /**
> >   * STOPPED  - No operation in progress
> >   * RUNNING  - Operation in progress have buffers
> > - * STALLED  - No operation in progress have no buffers
> >   * STOPPING - Stopping operation
> >   */
> >  enum rvin_dma_state {
> >  	STOPPED = 0,
> >  	RUNNING,
> > -	STALLED,
> >  	STOPPING,
> >  };
> >
> > @@ -105,11 +103,10 @@ struct rvin_graph_entity {
> >   * @scratch:		cpu address for scratch buffer
> >   * @scratch_phys:	pysical address of the scratch buffer
> >   *
> > - * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
> > + * @qlock:		protects @queue_buf, @buf_list, @sequence
> >   *			@state
> >   * @queue_buf:		Keeps track of buffers given to HW slot
> >   * @buf_list:		list of queued buffers
> > - * @continuous:		tracks if active operation is continuous or single mode
> >   * @sequence:		V4L2 buffers sequence number
> >   * @state:		keeps track of operation state
> >   *
> > @@ -138,7 +135,6 @@ struct rvin_dev {
> >  	spinlock_t qlock;
> >  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> >  	struct list_head buf_list;
> > -	bool continuous;
> >  	unsigned int sequence;
> >  	enum rvin_dma_state state;
> 
> With the above clarified, for the whole series:
> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thank you!

> 
> Thanks
>    j
> 
> >
> > --
> > 2.16.2
> >



-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Kieran Bingham]

Hi Niklas,

Thanks for the patch series :) - I've been looking forward to seeing this one !

On 10/03/18 01:09, Niklas Söderlund wrote:
> This is an error from when the driver where converted from soc-camera.
> There is absolutely no gain to check the state variable two times to be
> extra sure if the hardware is stopped.

I'll not say this isn't a redundant check ... but isn't the check against two
different states, and thus the remaining check doesn't actually catch the case
now where state == STOPPED ?

(Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
the code)

--
Kieran


> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 23fdff7a7370842e..b4be75d5009080f7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	rvin_ack_interrupt(vin);
>  	handled = 1;
>  
> -	/* Nothing to do if capture status is 'STOPPED' */
> -	if (vin->state == STOPPED) {
> -		vin_dbg(vin, "IRQ while state stopped\n");
> -		goto done;
> -	}
> -
>  	/* Nothing to do if capture status is 'STOPPING' */
>  	if (vin->state == STOPPING) {
>  		vin_dbg(vin, "IRQ while state stopping\n");
> 

Re: [PATCH 2/3] rcar-vin: allocate a scratch buffer at stream start

[Kieran Bingham]

Hi Niklas,

On 10/03/18 01:09, Niklas Söderlund wrote:
> Before starting capturing allocate a scratch buffer which can be used by

"Before starting a capture, allocate a..."
  (two 'ings' together doesn't sound right)

> the driver to give to the hardware if no buffers are available from
> userspace. The buffer is not used in this patch but prepares for future
> refactoring where the scratch buffer can be used to avoid the need to
> fallback on single capture mode if userspace don't queue buffers as fast

s/don't/doesn't/ or alternatively s/don't/can't/

> as the VIN driver consumes them.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

With minor comments attended to:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 19 +++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h |  4 ++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b4be75d5009080f7..8ea73cdc9a720abe 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1070,6 +1070,17 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	unsigned long flags;
>  	int ret;
>  
> +	/* Allocate scratch buffer. */
> +	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> +					  &vin->scratch_phys, GFP_KERNEL);
> +	if (!vin->scratch) {
> +		spin_lock_irqsave(&vin->qlock, flags);
> +		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> +		spin_unlock_irqrestore(&vin->qlock, flags);
> +		vin_err(vin, "Failed to allocate scratch buffer\n");
> +		return -ENOMEM;
> +	}
> +
>  	sd = vin_to_source(vin);
>  	v4l2_subdev_call(sd, video, s_stream, 1);
>  
> @@ -1085,6 +1096,10 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  
> +	if (ret)
> +		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> +				  vin->scratch_phys);
> +
>  	return ret;
>  }
>  
> @@ -1135,6 +1150,10 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  
>  	/* disable interrupts */
>  	rvin_disable_interrupts(vin);
> +
> +	/* Free scratch buffer. */
> +	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> +			  vin->scratch_phys);
>  }
>  
>  static const struct vb2_ops rvin_qops = {
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 5382078143fb3869..11a981d707c7ca47 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -102,6 +102,8 @@ struct rvin_graph_entity {
>   *
>   * @lock:		protects @queue
>   * @queue:		vb2 buffers queue
> + * @scratch:		cpu address for scratch buffer
> + * @scratch_phys:	pysical address of the scratch buffer

s/pysical/physical/


>   *
>   * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
>   *			@state
> @@ -130,6 +132,8 @@ struct rvin_dev {
>  
>  	struct mutex lock;
>  	struct vb2_queue queue;
> +	void *scratch;
> +	dma_addr_t scratch_phys;
>  
>  	spinlock_t qlock;
>  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> 

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> Thanks for the patch series :) - I've been looking forward to seeing this one !
> 
> On 10/03/18 01:09, Niklas Söderlund wrote:
> > This is an error from when the driver where converted from soc-camera.
> > There is absolutely no gain to check the state variable two times to be
> > extra sure if the hardware is stopped.
> 
> I'll not say this isn't a redundant check ... but isn't the check against two
> different states, and thus the remaining check doesn't actually catch the case
> now where state == STOPPED ?

Thanks for noticing this, you are correct. I think I need to refresh my 
glasses subscription after missing this :-)

> 
> (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> the code)

I will respin this in a v2 and either use state != RUNNING or at least 
combine the two checks to prevent future embarrassing mistakes like 
this.

> 
> --
> Kieran
> 
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	rvin_ack_interrupt(vin);
> >  	handled = 1;
> >  
> > -	/* Nothing to do if capture status is 'STOPPED' */
> > -	if (vin->state == STOPPED) {
> > -		vin_dbg(vin, "IRQ while state stopped\n");
> > -		goto done;
> > -	}
> > -
> >  	/* Nothing to do if capture status is 'STOPPING' */
> >  	if (vin->state == STOPPING) {
> >  		vin_dbg(vin, "IRQ while state stopping\n");
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> Thanks for the patch series :) - I've been looking forward to seeing this one !
> 
> On 10/03/18 01:09, Niklas S�derlund wrote:
> > This is an error from when the driver where converted from soc-camera.
> > There is absolutely no gain to check the state variable two times to be
> > extra sure if the hardware is stopped.
> 
> I'll not say this isn't a redundant check ... but isn't the check against two
> different states, and thus the remaining check doesn't actually catch the case
> now where state == STOPPED ?

Thanks for noticing this, you are correct. I think I need to refresh my 
glasses subscription after missing this :-)

> 
> (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> the code)

I will respin this in a v2 and either use state != RUNNING or at least 
combine the two checks to prevent future embarrassing mistakes like 
this.

> 
> --
> Kieran
> 
> 
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	rvin_ack_interrupt(vin);
> >  	handled = 1;
> >  
> > -	/* Nothing to do if capture status is 'STOPPED' */
> > -	if (vin->state == STOPPED) {
> > -		vin_dbg(vin, "IRQ while state stopped\n");
> > -		goto done;
> > -	}
> > -
> >  	/* Nothing to do if capture status is 'STOPPING' */
> >  	if (vin->state == STOPPING) {
> >  		vin_dbg(vin, "IRQ while state stopping\n");
> > 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Hans Verkuil]

On 03/09/2018 04:09 PM, Niklas Söderlund wrote:
> This is an error from when the driver where converted from soc-camera.

where -> was

> There is absolutely no gain to check the state variable two times to be
> extra sure if the hardware is stopped.

I'll wait for v2 before applying this.

Regards,

	Hans

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 23fdff7a7370842e..b4be75d5009080f7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	rvin_ack_interrupt(vin);
>  	handled = 1;
>  
> -	/* Nothing to do if capture status is 'STOPPED' */
> -	if (vin->state == STOPPED) {
> -		vin_dbg(vin, "IRQ while state stopped\n");
> -		goto done;
> -	}
> -
>  	/* Nothing to do if capture status is 'STOPPING' */
>  	if (vin->state == STOPPING) {
>  		vin_dbg(vin, "IRQ while state stopping\n");
> 

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Hans Verkuil]

On 03/09/2018 04:09 PM, Niklas Söderlund wrote:
> Instead of switching capture mode depending on how many buffers are
> available use a scratch buffer and always run in continuous mode. By
> using a scratch buffer the responsiveness of the capture loop is
> increased as it can keep running even if there are no buffers available
> from userspace.
> 
> As soon as a userspace queues a buffer it is inserted into the capture
> loop and returned as soon as it is filled. This is a improvement on the
> previous logic where the whole capture loop was stopped and switched to
> single capture mode if userspace did not feed the VIN driver buffers at
> the same time it consumed them. To make matters worse it was difficult
> for the driver to reenter continues mode if it entered single mode even

continues -> continuous

> if userspace started to queue buffers faster. This resulted in
> suboptimal performance where if userspace where delayed for a short
> period the ongoing capture would be slowed down and run in single mode
> until the capturing process where restarted.
> 
> An additional effect of this change is that the capture logic can be
> made much simple as we know that continues mode will always be used.

ditto

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
>  drivers/media/platform/rcar-vin/rcar-vin.h |   6 +-
>  2 files changed, 52 insertions(+), 141 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case V4L2_FIELD_ALTERNATE:
>  	case V4L2_FIELD_NONE:
> -		if (vin->continuous) {
> -			vnmc = VNMC_IM_ODD_EVEN;
> -			progressive = true;
> -		} else {
> -			vnmc = VNMC_IM_ODD;
> -		}
> +		vnmc = VNMC_IM_ODD_EVEN;
> +		progressive = true;
>  		break;
>  	default:
>  		vnmc = VNMC_IM_ODD;
> @@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>  }
>  
> -static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> -{
> -	if (vin->continuous)
> -		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> -
> -	return 0;
> -}
> -
>  static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>  {
>  	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> @@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>  	rvin_write(vin, offset, VNMB_REG(slot));
>  }
>  
> -/* Moves a buffer from the queue to the HW slots */
> -static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> +/*
> + * 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.
> + */
> +static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  {
>  	struct rvin_buffer *buf;
>  	struct vb2_v4l2_buffer *vbuf;
> -	dma_addr_t phys_addr_top;
> -
> -	if (vin->queue_buf[slot] != NULL)
> -		return true;
> +	dma_addr_t phys_addr;
>  
> -	if (list_empty(&vin->buf_list))
> -		return false;
> +	/* A already populated slot shall never be overwritten. */
> +	if (WARN_ON(vin->queue_buf[slot] != NULL))
> +		return;
>  
>  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
>  
> -	/* Keep track of buffer we give to HW */
> -	buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> -	vbuf = &buf->vb;
> -	list_del_init(to_buf_list(vbuf));
> -	vin->queue_buf[slot] = vbuf;
> -
> -	/* Setup DMA */
> -	phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> -	rvin_set_slot_addr(vin, slot, phys_addr_top);
> -
> -	return true;
> -}
> -
> -static bool rvin_fill_hw(struct rvin_dev *vin)
> -{
> -	int slot, limit;
> -
> -	limit = vin->continuous ? HW_BUFFER_NUM : 1;
> -
> -	for (slot = 0; slot < limit; slot++)
> -		if (!rvin_fill_hw_slot(vin, slot))
> -			return false;
> -	return true;
> -}
> -
> -static void rvin_capture_on(struct rvin_dev *vin)
> -{
> -	vin_dbg(vin, "Capture on in %s mode\n",
> -		vin->continuous ? "continuous" : "single");
> +	if (list_empty(&vin->buf_list)) {
> +		vin->queue_buf[slot] = NULL;
> +		phys_addr = vin->scratch_phys;
> +	} else {
> +		/* Keep track of buffer we give to HW */
> +		buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> +		vbuf = &buf->vb;
> +		list_del_init(to_buf_list(vbuf));
> +		vin->queue_buf[slot] = vbuf;
> +
> +		/* Setup DMA */
> +		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> +	}
>  
> -	if (vin->continuous)
> -		/* Continuous Frame Capture Mode */
> -		rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> -	else
> -		/* Single Frame Capture Mode */
> -		rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> +	rvin_set_slot_addr(vin, slot, phys_addr);
>  }
>  
>  static int rvin_capture_start(struct rvin_dev *vin)
>  {
> -	struct rvin_buffer *buf, *node;
> -	int bufs, ret;
> +	int slot, ret;
>  
> -	/* Count number of free buffers */
> -	bufs = 0;
> -	list_for_each_entry_safe(buf, node, &vin->buf_list, list)
> -		bufs++;
> -
> -	/* Continuous capture requires more buffers then there are HW slots */
> -	vin->continuous = bufs > HW_BUFFER_NUM;
> -
> -	if (!rvin_fill_hw(vin)) {
> -		vin_err(vin, "HW not ready to start, not enough buffers available\n");
> -		return -EINVAL;
> -	}
> +	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> +		rvin_fill_hw_slot(vin, slot);
>  
>  	rvin_crop_scale_comp(vin);
>  
> @@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
>  	if (ret)
>  		return ret;
>  
> -	rvin_capture_on(vin);
> +	vin_dbg(vin, "Starting to capture\n");
> +
> +	/* Continuous Frame Capture Mode */
> +	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
>  
>  	vin->state = RUNNING;
>  
> @@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	struct rvin_dev *vin = data;
>  	u32 int_status, vnms;
>  	int slot;
> -	unsigned int i, sequence, handled = 0;
> +	unsigned int handled = 0;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&vin->qlock, flags);
> @@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  
>  	/* Prepare for capture and update state */
>  	vnms = rvin_read(vin, VNMS_REG);
> -	slot = rvin_get_active_slot(vin, vnms);
> -	sequence = vin->sequence++;
> -
> -	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> -		sequence, slot,
> -		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
> -		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
> -		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
> -		!list_empty(&vin->buf_list));
> -
> -	/* HW have written to a slot that is not prepared we are in trouble */
> -	if (WARN_ON((vin->queue_buf[slot] == NULL)))
> -		goto done;
> +	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>  
>  	/* Capture frame */
> -	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> -	vin->queue_buf[slot]->sequence = sequence;
> -	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> -	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> -	vin->queue_buf[slot] = NULL;
> -
> -	/* Prepare for next frame */
> -	if (!rvin_fill_hw(vin)) {
> -
> -		/*
> -		 * Can't supply HW with new buffers fast enough. Halt
> -		 * capture until more buffers are available.
> -		 */
> -		vin->state = STALLED;
> -
> -		/*
> -		 * The continuous capturing requires an explicit stop
> -		 * operation when there is no buffer to be set into
> -		 * the VnMBm registers.
> -		 */
> -		if (vin->continuous) {
> -			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);
> -		}
> +	if (vin->queue_buf[slot]) {
> +		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> +		vin->queue_buf[slot]->sequence = vin->sequence;
> +		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> +		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
> +				VB2_BUF_STATE_DONE);
> +		vin->queue_buf[slot] = NULL;
>  	} else {
> -		/*
> -		 * The single capturing requires an explicit capture
> -		 * operation to fetch the next frame.
> -		 */
> -		if (!vin->continuous)
> -			rvin_capture_on(vin);
> +		/* Scratch buffer was used, dropping frame. */
> +		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
>  	}
> +
> +	vin->sequence++;
> +
> +	/* Prepare for next frame */
> +	rvin_fill_hw_slot(vin, slot);
>  done:
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  
> @@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
>  
>  	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
>  
> -	/*
> -	 * If capture is stalled add buffer to HW and restart
> -	 * capturing if HW is ready to continue.
> -	 */
> -	if (vin->state == STALLED)
> -		rvin_capture_start(vin);
> -
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  }
>  
> @@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
>  	q->ops = &rvin_qops;
>  	q->mem_ops = &vb2_dma_contig_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_buffers_needed = 1;
> +	q->min_buffers_needed = 4;
>  	q->dev = vin->dev;
>  
>  	ret = vb2_queue_init(q);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 11a981d707c7ca47..1adc1b809f761e71 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -38,13 +38,11 @@ enum chip_id {
>  /**
>   * STOPPED  - No operation in progress
>   * RUNNING  - Operation in progress have buffers
> - * STALLED  - No operation in progress have no buffers
>   * STOPPING - Stopping operation
>   */
>  enum rvin_dma_state {
>  	STOPPED = 0,
>  	RUNNING,
> -	STALLED,
>  	STOPPING,
>  };
>  
> @@ -105,11 +103,10 @@ struct rvin_graph_entity {
>   * @scratch:		cpu address for scratch buffer
>   * @scratch_phys:	pysical address of the scratch buffer
>   *
> - * @qlock:		protects @queue_buf, @buf_list, @continuous, @sequence
> + * @qlock:		protects @queue_buf, @buf_list, @sequence
>   *			@state
>   * @queue_buf:		Keeps track of buffers given to HW slot
>   * @buf_list:		list of queued buffers
> - * @continuous:		tracks if active operation is continuous or single mode
>   * @sequence:		V4L2 buffers sequence number
>   * @state:		keeps track of operation state
>   *
> @@ -138,7 +135,6 @@ struct rvin_dev {
>  	spinlock_t qlock;
>  	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
>  	struct list_head buf_list;
> -	bool continuous;
>  	unsigned int sequence;
>  	enum rvin_dma_state state;
>  
> 

Looks good!

Regards,

	Hans

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[jacopo mondi]

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

Hi Niklas, Kieran,

On Tue, Mar 13, 2018 at 06:56:54PM +0100, Niklas Söderlund wrote:
> Hi Kieran,
>
> Thanks for your feedback.
>
> On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> > Hi Niklas,
> >
> > Thanks for the patch series :) - I've been looking forward to seeing this one !
> >
> > On 10/03/18 01:09, Niklas Söderlund wrote:
> > > This is an error from when the driver where converted from soc-camera.
> > > There is absolutely no gain to check the state variable two times to be
> > > extra sure if the hardware is stopped.
> >
> > I'll not say this isn't a redundant check ... but isn't the check against two
> > different states, and thus the remaining check doesn't actually catch the case
> > now where state == STOPPED ?
>
> Thanks for noticing this, you are correct. I think I need to refresh my
> glasses subscription after missing this :-)
>
> >
> > (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> > the code)
>
> I will respin this in a v2 and either use state != RUNNING or at least
> combine the two checks to prevent future embarrassing mistakes like
> this.

I am sorry I have missed this comment, but I think your patch has some
merits. Ofc no need to hold on v2 of this series for this, but still I
think you can re-propose this later (and I didn't get from
your commit message you were confusing STOPPED/STOPPING).

In rvin_stop_streaming(), you enter STOPPING state, disable the
interface cleaning ME bit in VnMC and single/continuous capture mode
in VnFC, and then poll on CA bit of VnMS until the VIN peripheral has
not been stopped, at this  point you set interface state to STOPPED.

As you loop you can still receive interrupts, as you are releasing the
spinlock when sleeping before testing the ME bit again, so it's fine
checking for STOPPING state in irq handler.
It seems to me though, that once you enter STOPPED state, you are sure the
peripheral has stopped and you should not receive any more interrupt, spurious
ones apart or when the peripheral fails to stop at all, but things went
south already at that point.

Again no need to have this part of this series, but you may want to
take into consideration this for the future, as with this change you can
remove the STOPPED state at all from the driver.

Thanks
   j

>
> >
> > --
> > Kieran
> >
> >
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > >  	rvin_ack_interrupt(vin);
> > >  	handled = 1;
> > >
> > > -	/* Nothing to do if capture status is 'STOPPED' */
> > > -	if (vin->state == STOPPED) {
> > > -		vin_dbg(vin, "IRQ while state stopped\n");
> > > -		goto done;
> > > -	}
> > > -
> > >  	/* Nothing to do if capture status is 'STOPPING' */
> > >  	if (vin->state == STOPPING) {
> > >  		vin_dbg(vin, "IRQ while state stopping\n");
> > >
>
> --
> Regards,
> Niklas Söderlund

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

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

Hi Jacopo,

On 2018-03-14 16:17:33 +0100, Jacopo Mondi wrote:
> Hi Niklas, Kieran,
> 
> On Tue, Mar 13, 2018 at 06:56:54PM +0100, Niklas Söderlund wrote:
> > Hi Kieran,
> >
> > Thanks for your feedback.
> >
> > On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> > > Hi Niklas,
> > >
> > > Thanks for the patch series :) - I've been looking forward to seeing this one !
> > >
> > > On 10/03/18 01:09, Niklas Söderlund wrote:
> > > > This is an error from when the driver where converted from soc-camera.
> > > > There is absolutely no gain to check the state variable two times to be
> > > > extra sure if the hardware is stopped.
> > >
> > > I'll not say this isn't a redundant check ... but isn't the check against two
> > > different states, and thus the remaining check doesn't actually catch the case
> > > now where state == STOPPED ?
> >
> > Thanks for noticing this, you are correct. I think I need to refresh my
> > glasses subscription after missing this :-)
> >
> > >
> > > (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> > > the code)
> >
> > I will respin this in a v2 and either use state != RUNNING or at least
> > combine the two checks to prevent future embarrassing mistakes like
> > this.
> 
> I am sorry I have missed this comment, but I think your patch has some
> merits. Ofc no need to hold on v2 of this series for this, but still I
> think you can re-propose this later (and I didn't get from
> your commit message you were confusing STOPPED/STOPPING).
> 
> In rvin_stop_streaming(), you enter STOPPING state, disable the
> interface cleaning ME bit in VnMC and single/continuous capture mode
> in VnFC, and then poll on CA bit of VnMS until the VIN peripheral has
> not been stopped, at this  point you set interface state to STOPPED.
> 
> As you loop you can still receive interrupts, as you are releasing the
> spinlock when sleeping before testing the ME bit again, so it's fine
> checking for STOPPING state in irq handler.
> It seems to me though, that once you enter STOPPED state, you are sure the
> peripheral has stopped and you should not receive any more interrupt, spurious
> ones apart or when the peripheral fails to stop at all, but things went
> south already at that point.
> 
> Again no need to have this part of this series, but you may want to
> take into consideration this for the future, as with this change you can
> remove the STOPPED state at all from the driver.

You are correct.

This patch was extracted from another series I plan to post after the 
VIN Gen3 beast is done. The aim of that series is to add SEQ_TB/BT 
support to VIN and to do so another state STARTING is needed to handle 
the first few fields. But to avoid growing that series too large I 
thought I could get away with adding this cleanup to this series which 
cleans up the interrupt handler. So this patch will comeback in some 
form when I post that series :-)

But for now I'm happy to drop it as the performance gain with this 
patch-set applied are so nice when running into buffer starved 
situations.

> 
> Thanks
>    j
> 
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> > > >  1 file changed, 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > > >  	rvin_ack_interrupt(vin);
> > > >  	handled = 1;
> > > >
> > > > -	/* Nothing to do if capture status is 'STOPPED' */
> > > > -	if (vin->state == STOPPED) {
> > > > -		vin_dbg(vin, "IRQ while state stopped\n");
> > > > -		goto done;
> > > > -	}
> > > > -
> > > >  	/* Nothing to do if capture status is 'STOPPING' */
> > > >  	if (vin->state == STOPPING) {
> > > >  		vin_dbg(vin, "IRQ while state stopping\n");
> > > >
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[Niklas Söderlund]

Hi Jacopo,

On 2018-03-14 16:17:33 +0100, Jacopo Mondi wrote:
> Hi Niklas, Kieran,
> 
> On Tue, Mar 13, 2018 at 06:56:54PM +0100, Niklas S�derlund wrote:
> > Hi Kieran,
> >
> > Thanks for your feedback.
> >
> > On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> > > Hi Niklas,
> > >
> > > Thanks for the patch series :) - I've been looking forward to seeing this one !
> > >
> > > On 10/03/18 01:09, Niklas S�derlund wrote:
> > > > This is an error from when the driver where converted from soc-camera.
> > > > There is absolutely no gain to check the state variable two times to be
> > > > extra sure if the hardware is stopped.
> > >
> > > I'll not say this isn't a redundant check ... but isn't the check against two
> > > different states, and thus the remaining check doesn't actually catch the case
> > > now where state == STOPPED ?
> >
> > Thanks for noticing this, you are correct. I think I need to refresh my
> > glasses subscription after missing this :-)
> >
> > >
> > > (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> > > the code)
> >
> > I will respin this in a v2 and either use state != RUNNING or at least
> > combine the two checks to prevent future embarrassing mistakes like
> > this.
> 
> I am sorry I have missed this comment, but I think your patch has some
> merits. Ofc no need to hold on v2 of this series for this, but still I
> think you can re-propose this later (and I didn't get from
> your commit message you were confusing STOPPED/STOPPING).
> 
> In rvin_stop_streaming(), you enter STOPPING state, disable the
> interface cleaning ME bit in VnMC and single/continuous capture mode
> in VnFC, and then poll on CA bit of VnMS until the VIN peripheral has
> not been stopped, at this  point you set interface state to STOPPED.
> 
> As you loop you can still receive interrupts, as you are releasing the
> spinlock when sleeping before testing the ME bit again, so it's fine
> checking for STOPPING state in irq handler.
> It seems to me though, that once you enter STOPPED state, you are sure the
> peripheral has stopped and you should not receive any more interrupt, spurious
> ones apart or when the peripheral fails to stop at all, but things went
> south already at that point.
> 
> Again no need to have this part of this series, but you may want to
> take into consideration this for the future, as with this change you can
> remove the STOPPED state at all from the driver.

You are correct.

This patch was extracted from another series I plan to post after the 
VIN Gen3 beast is done. The aim of that series is to add SEQ_TB/BT 
support to VIN and to do so another state STARTING is needed to handle 
the first few fields. But to avoid growing that series too large I 
thought I could get away with adding this cleanup to this series which 
cleans up the interrupt handler. So this patch will comeback in some 
form when I post that series :-)

But for now I'm happy to drop it as the performance gain with this 
patch-set applied are so nice when running into buffer starved 
situations.

> 
> Thanks
>    j
> 
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> > > >  1 file changed, 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > > >  	rvin_ack_interrupt(vin);
> > > >  	handled = 1;
> > > >
> > > > -	/* Nothing to do if capture status is 'STOPPED' */
> > > > -	if (vin->state == STOPPED) {
> > > > -		vin_dbg(vin, "IRQ while state stopped\n");
> > > > -		goto done;
> > > > -	}
> > > > -
> > > >  	/* Nothing to do if capture status is 'STOPPING' */
> > > >  	if (vin->state == STOPPING) {
> > > >  		vin_dbg(vin, "IRQ while state stopping\n");
> > > >
> >
> > --
> > Regards,
> > Niklas S�derlund



-- 
Regards,
Niklas S�derlund