[PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT}

[PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT}

[Niklas Söderlund]

Hi,

This series add support for sequential filed formats to rcar-vin. The
series is based on the media-tree and tested on both R-Car Gen2 and Gen3
boards without regressions.

Patch 1/2 prepares for the new filed formats by reworking and renaming
an existing struct member while 2/2 adds support for the two new field
formats.

Niklas Söderlund (2):
  rcar-vin: Move hardware buffer tracking to own struct
  rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT}

 drivers/media/platform/rcar-vin/rcar-dma.c  | 91 ++++++++++++++++-----
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  5 ++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 28 ++++++-
 3 files changed, 100 insertions(+), 24 deletions(-)

-- 
2.24.0

[PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct

[Niklas Söderlund]

To support SEQ_TB/BT not all buffers given to the hardware will be
equal, the driver needs to keep track of different buffer types. Move
the tracking of buffers given to hardware into a struct so additional
tracking fields can be associated with each buffer.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index cf9029efeb0450cb..cd1778977b2ba56e 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -844,20 +844,20 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
 	dma_addr_t phys_addr;
 
 	/* A already populated slot shall never be overwritten. */
-	if (WARN_ON(vin->queue_buf[slot] != NULL))
+	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
 		return;
 
 	vin_dbg(vin, "Filling HW slot: %d\n", slot);
 
 	if (list_empty(&vin->buf_list)) {
-		vin->queue_buf[slot] = NULL;
+		vin->buf_hw[slot].buffer = 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;
+		vin->buf_hw[slot].buffer = vbuf;
 
 		/* Setup DMA */
 		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
@@ -953,13 +953,14 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	}
 
 	/* Capture frame */
-	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,
+	if (vin->buf_hw[slot].buffer) {
+		vin->buf_hw[slot].buffer->field =
+			rvin_get_active_field(vin, vnms);
+		vin->buf_hw[slot].buffer->sequence = vin->sequence;
+		vin->buf_hw[slot].buffer->vb2_buf.timestamp = ktime_get_ns();
+		vb2_buffer_done(&vin->buf_hw[slot].buffer->vb2_buf,
 				VB2_BUF_STATE_DONE);
-		vin->queue_buf[slot] = NULL;
+		vin->buf_hw[slot].buffer = NULL;
 	} else {
 		/* Scratch buffer was used, dropping frame. */
 		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
@@ -983,10 +984,10 @@ static void return_all_buffers(struct rvin_dev *vin,
 	int i;
 
 	for (i = 0; i < HW_BUFFER_NUM; i++) {
-		if (vin->queue_buf[i]) {
-			vb2_buffer_done(&vin->queue_buf[i]->vb2_buf,
+		if (vin->buf_hw[i].buffer) {
+			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
 					state);
-			vin->queue_buf[i] = NULL;
+			vin->buf_hw[i].buffer = NULL;
 		}
 	}
 
@@ -1291,7 +1292,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
 	vin->state = STOPPED;
 
 	for (i = 0; i < HW_BUFFER_NUM; i++)
-		vin->queue_buf[i] = NULL;
+		vin->buf_hw[i].buffer = NULL;
 
 	/* buffer queue */
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index a36b0824f81d171d..0aa904a4af5b0a97 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -164,9 +164,8 @@ struct rvin_info {
  * @scratch:		cpu address for scratch buffer
  * @scratch_phys:	physical address of the scratch buffer
  *
- * @qlock:		protects @queue_buf, @buf_list, @sequence
- *			@state
- * @queue_buf:		Keeps track of buffers given to HW slot
+ * @qlock:		protects @buf_hw, @buf_list, @sequence and @state
+ * @buf_hw:		Keeps track of buffers given to HW slot
  * @buf_list:		list of queued buffers
  * @sequence:		V4L2 buffers sequence number
  * @state:		keeps track of operation state
@@ -205,7 +204,9 @@ struct rvin_dev {
 	dma_addr_t scratch_phys;
 
 	spinlock_t qlock;
-	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
+	struct {
+		struct vb2_v4l2_buffer *buffer;
+	} buf_hw[HW_BUFFER_NUM];
 	struct list_head buf_list;
 	unsigned int sequence;
 	enum rvin_dma_state state;
-- 
2.24.0

[PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT}

[Niklas Söderlund]

The hardware does not support capturing the field types
V4L2_FIELD_SEQ_TB and V4L2_FIELD_SEQ_BT. To capture in these formats the
driver needs to adjust the offset of the capture buffer and capture
twice to each vb2 buffer.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 68 ++++++++++++++++++---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  5 ++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 19 ++++++
 3 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index cd1778977b2ba56e..f50b615eda36c107 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -535,7 +535,7 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 
 	/* Set scaling coefficient */
 	crop_height = vin->crop.height;
-	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
+	if (V4L2_FIELD_HAS_BOTH(vin->format.field))
 		crop_height *= 2;
 
 	ys = 0;
@@ -564,7 +564,7 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 	rvin_write(vin, 0, VNSLPOC_REG);
 	rvin_write(vin, vin->format.width - 1, VNEPPOC_REG);
 
-	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
+	if (V4L2_FIELD_HAS_BOTH(vin->format.field))
 		rvin_write(vin, vin->format.height / 2 - 1, VNELPOC_REG);
 	else
 		rvin_write(vin, vin->format.height - 1, VNELPOC_REG);
@@ -626,6 +626,8 @@ static int rvin_setup(struct rvin_dev *vin)
 	case V4L2_FIELD_INTERLACED_BT:
 		vnmc = VNMC_IM_FULL | VNMC_FOC;
 		break;
+	case V4L2_FIELD_SEQ_TB:
+	case V4L2_FIELD_SEQ_BT:
 	case V4L2_FIELD_NONE:
 		vnmc = VNMC_IM_ODD_EVEN;
 		progressive = true;
@@ -842,15 +844,32 @@ 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;
+	int prev;
 
 	/* A already populated slot shall never be overwritten. */
 	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
 		return;
 
-	vin_dbg(vin, "Filling HW slot: %d\n", slot);
+	prev = (slot == 0 ? HW_BUFFER_NUM : slot) - 1;
 
-	if (list_empty(&vin->buf_list)) {
+	if (vin->buf_hw[prev].type == HALF_TOP) {
+		vbuf = vin->buf_hw[prev].buffer;
+		vin->buf_hw[slot].buffer = vbuf;
+		vin->buf_hw[slot].type = HALF_BOTTOM;
+		switch (vin->format.pixelformat) {
+		case V4L2_PIX_FMT_NV12:
+		case V4L2_PIX_FMT_NV16:
+			phys_addr = vin->buf_hw[prev].phys +
+				vin->format.sizeimage / 4;
+			break;
+		default:
+			phys_addr = vin->buf_hw[prev].phys +
+				vin->format.sizeimage / 2;
+			break;
+		}
+	} else if (list_empty(&vin->buf_list)) {
 		vin->buf_hw[slot].buffer = NULL;
+		vin->buf_hw[slot].type = FULL;
 		phys_addr = vin->scratch_phys;
 	} else {
 		/* Keep track of buffer we give to HW */
@@ -859,10 +878,18 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
 		list_del_init(to_buf_list(vbuf));
 		vin->buf_hw[slot].buffer = vbuf;
 
+		vin->buf_hw[slot].type =
+			V4L2_FIELD_IS_SEQUENTIAL(vin->format.field) ?
+			HALF_TOP : FULL;
+
 		/* Setup DMA */
 		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
 	}
 
+	vin_dbg(vin, "Filling HW slot: %d type: %d buffer: %p\n",
+		slot, vin->buf_hw[slot].type, vin->buf_hw[slot].buffer);
+
+	vin->buf_hw[slot].phys = phys_addr;
 	rvin_set_slot_addr(vin, slot, phys_addr);
 }
 
@@ -870,6 +897,11 @@ static int rvin_capture_start(struct rvin_dev *vin)
 {
 	int slot, ret;
 
+	for (slot = 0; slot < HW_BUFFER_NUM; slot++) {
+		vin->buf_hw[slot].buffer = NULL;
+		vin->buf_hw[slot].type = FULL;
+	}
+
 	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
 		rvin_fill_hw_slot(vin, slot);
 
@@ -954,6 +986,16 @@ static irqreturn_t rvin_irq(int irq, void *data)
 
 	/* Capture frame */
 	if (vin->buf_hw[slot].buffer) {
+		/*
+		 * Nothing to do but refill the hardware slot if
+		 * capture only filled first half of vb2 buffer.
+		 */
+		if (vin->buf_hw[slot].type == HALF_TOP) {
+			vin->buf_hw[slot].buffer = NULL;
+			rvin_fill_hw_slot(vin, slot);
+			goto done;
+		}
+
 		vin->buf_hw[slot].buffer->field =
 			rvin_get_active_field(vin, vnms);
 		vin->buf_hw[slot].buffer->sequence = vin->sequence;
@@ -981,14 +1023,22 @@ static void return_all_buffers(struct rvin_dev *vin,
 			       enum vb2_buffer_state state)
 {
 	struct rvin_buffer *buf, *node;
-	int i;
+	struct vb2_v4l2_buffer *freed[HW_BUFFER_NUM];
+	unsigned int i, n;
 
 	for (i = 0; i < HW_BUFFER_NUM; i++) {
-		if (vin->buf_hw[i].buffer) {
-			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
-					state);
-			vin->buf_hw[i].buffer = NULL;
+		freed[i] = vin->buf_hw[i].buffer;
+		vin->buf_hw[i].buffer = NULL;
+
+		for (n = 0; n < i; n++) {
+			if (freed[i] == freed[n]) {
+				freed[i] = NULL;
+				break;
+			}
 		}
+
+		if (freed[i])
+			vb2_buffer_done(&freed[i]->vb2_buf, state);
 	}
 
 	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 9e2e63ffcc47acad..7ab938ae93826675 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -107,6 +107,9 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
 		break;
 	}
 
+	if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
+		align = 0x80;
+
 	return ALIGN(pix->width, align) * fmt->bpp;
 }
 
@@ -137,6 +140,8 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
 	case V4L2_FIELD_INTERLACED_BT:
 	case V4L2_FIELD_INTERLACED:
 	case V4L2_FIELD_ALTERNATE:
+	case V4L2_FIELD_SEQ_TB:
+	case V4L2_FIELD_SEQ_BT:
 		break;
 	default:
 		pix->field = RVIN_DEFAULT_FIELD;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 0aa904a4af5b0a97..c19d077ce1cb4f4b 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -60,6 +60,23 @@ enum rvin_dma_state {
 	STOPPING,
 };
 
+/**
+ * enum rvin_buffer_type
+ *
+ * Describes how a buffer is given to the hardware. To be able
+ * to capture SEQ_TB/BT it's needed to capture to the same vb2
+ * buffer twice so the type of buffer needs to be kept.
+ *
+ * FULL - One capture fills the whole vb2 buffer
+ * HALF_TOP - One capture fills the top half of the vb2 buffer
+ * HALF_BOTTOM - One capture fills the bottom half of the vb2 buffer
+ */
+enum rvin_buffer_type {
+	FULL,
+	HALF_TOP,
+	HALF_BOTTOM,
+};
+
 /**
  * struct rvin_video_format - Data format stored in memory
  * @fourcc:	Pixelformat
@@ -206,6 +223,8 @@ struct rvin_dev {
 	spinlock_t qlock;
 	struct {
 		struct vb2_v4l2_buffer *buffer;
+		enum rvin_buffer_type type;
+		dma_addr_t phys;
 	} buf_hw[HW_BUFFER_NUM];
 	struct list_head buf_list;
 	unsigned int sequence;
-- 
2.24.0

Re: [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct

[Jacopo Mondi]

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

Hi Niklas,

On Tue, Dec 10, 2019 at 03:05:58AM +0100, Niklas Söderlund wrote:
> To support SEQ_TB/BT not all buffers given to the hardware will be
> equal, the driver needs to keep track of different buffer types. Move
> the tracking of buffers given to hardware into a struct so additional
> tracking fields can be associated with each buffer.
>

This change alone does not make sense by itself. I cannot judge if
it's a good idea or not if not looking at 2/2. Why have you kept it
separate ?

Thanks
   j

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 27 +++++++++++-----------
>  drivers/media/platform/rcar-vin/rcar-vin.h |  9 ++++----
>  2 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index cf9029efeb0450cb..cd1778977b2ba56e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -844,20 +844,20 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  	dma_addr_t phys_addr;
>
>  	/* A already populated slot shall never be overwritten. */
> -	if (WARN_ON(vin->queue_buf[slot] != NULL))
> +	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
>  		return;
>
>  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
>
>  	if (list_empty(&vin->buf_list)) {
> -		vin->queue_buf[slot] = NULL;
> +		vin->buf_hw[slot].buffer = 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;
> +		vin->buf_hw[slot].buffer = vbuf;
>
>  		/* Setup DMA */
>  		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> @@ -953,13 +953,14 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	}
>
>  	/* Capture frame */
> -	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,
> +	if (vin->buf_hw[slot].buffer) {
> +		vin->buf_hw[slot].buffer->field =
> +			rvin_get_active_field(vin, vnms);
> +		vin->buf_hw[slot].buffer->sequence = vin->sequence;
> +		vin->buf_hw[slot].buffer->vb2_buf.timestamp = ktime_get_ns();
> +		vb2_buffer_done(&vin->buf_hw[slot].buffer->vb2_buf,
>  				VB2_BUF_STATE_DONE);
> -		vin->queue_buf[slot] = NULL;
> +		vin->buf_hw[slot].buffer = NULL;
>  	} else {
>  		/* Scratch buffer was used, dropping frame. */
>  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> @@ -983,10 +984,10 @@ static void return_all_buffers(struct rvin_dev *vin,
>  	int i;
>
>  	for (i = 0; i < HW_BUFFER_NUM; i++) {
> -		if (vin->queue_buf[i]) {
> -			vb2_buffer_done(&vin->queue_buf[i]->vb2_buf,
> +		if (vin->buf_hw[i].buffer) {
> +			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
>  					state);
> -			vin->queue_buf[i] = NULL;
> +			vin->buf_hw[i].buffer = NULL;
>  		}
>  	}
>
> @@ -1291,7 +1292,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>  	vin->state = STOPPED;
>
>  	for (i = 0; i < HW_BUFFER_NUM; i++)
> -		vin->queue_buf[i] = NULL;
> +		vin->buf_hw[i].buffer = NULL;
>
>  	/* buffer queue */
>  	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index a36b0824f81d171d..0aa904a4af5b0a97 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -164,9 +164,8 @@ struct rvin_info {
>   * @scratch:		cpu address for scratch buffer
>   * @scratch_phys:	physical address of the scratch buffer
>   *
> - * @qlock:		protects @queue_buf, @buf_list, @sequence
> - *			@state
> - * @queue_buf:		Keeps track of buffers given to HW slot
> + * @qlock:		protects @buf_hw, @buf_list, @sequence and @state
> + * @buf_hw:		Keeps track of buffers given to HW slot
>   * @buf_list:		list of queued buffers
>   * @sequence:		V4L2 buffers sequence number
>   * @state:		keeps track of operation state
> @@ -205,7 +204,9 @@ struct rvin_dev {
>  	dma_addr_t scratch_phys;
>
>  	spinlock_t qlock;
> -	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> +	struct {
> +		struct vb2_v4l2_buffer *buffer;
> +	} buf_hw[HW_BUFFER_NUM];
>  	struct list_head buf_list;
>  	unsigned int sequence;
>  	enum rvin_dma_state state;
> --
> 2.24.0
>

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

Re: [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT}

[Jacopo Mondi]

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

Hi Niklas,

On Tue, Dec 10, 2019 at 03:05:57AM +0100, Niklas Söderlund wrote:
> Hi,
>
> This series add support for sequential filed formats to rcar-vin. The
> series is based on the media-tree and tested on both R-Car Gen2 and Gen3
> boards without regressions.
>
> Patch 1/2 prepares for the new filed formats by reworking and renaming
> an existing struct member while 2/2 adds support for the two new field
> formats.

Please try to add a changelog, otherwise I have to read my comments in v2
and manually check what has been addressed or not

>
> Niklas Söderlund (2):
>   rcar-vin: Move hardware buffer tracking to own struct
>   rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT}
>
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 91 ++++++++++++++++-----
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  5 ++
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 28 ++++++-
>  3 files changed, 100 insertions(+), 24 deletions(-)
>
> --
> 2.24.0
>

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

Re: [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT}

[Jacopo Mondi]

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

Hi Niklas,

On Tue, Dec 10, 2019 at 03:05:59AM +0100, Niklas Söderlund wrote:
> The hardware does not support capturing the field types
> V4L2_FIELD_SEQ_TB and V4L2_FIELD_SEQ_BT. To capture in these formats the
> driver needs to adjust the offset of the capture buffer and capture
> twice to each vb2 buffer.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 68 ++++++++++++++++++---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  5 ++
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 19 ++++++
>  3 files changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index cd1778977b2ba56e..f50b615eda36c107 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -535,7 +535,7 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>
>  	/* Set scaling coefficient */
>  	crop_height = vin->crop.height;
> -	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
> +	if (V4L2_FIELD_HAS_BOTH(vin->format.field))
>  		crop_height *= 2;
>
>  	ys = 0;
> @@ -564,7 +564,7 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  	rvin_write(vin, 0, VNSLPOC_REG);
>  	rvin_write(vin, vin->format.width - 1, VNEPPOC_REG);
>
> -	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
> +	if (V4L2_FIELD_HAS_BOTH(vin->format.field))
>  		rvin_write(vin, vin->format.height / 2 - 1, VNELPOC_REG);
>  	else
>  		rvin_write(vin, vin->format.height - 1, VNELPOC_REG);
> @@ -626,6 +626,8 @@ static int rvin_setup(struct rvin_dev *vin)
>  	case V4L2_FIELD_INTERLACED_BT:
>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>  		break;
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
>  	case V4L2_FIELD_NONE:
>  		vnmc = VNMC_IM_ODD_EVEN;
>  		progressive = true;
> @@ -842,15 +844,32 @@ 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;
> +	int prev;
>
>  	/* A already populated slot shall never be overwritten. */
>  	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
>  		return;
>
> -	vin_dbg(vin, "Filling HW slot: %d\n", slot);
> +	prev = (slot == 0 ? HW_BUFFER_NUM : slot) - 1;
>
> -	if (list_empty(&vin->buf_list)) {
> +	if (vin->buf_hw[prev].type == HALF_TOP) {
> +		vbuf = vin->buf_hw[prev].buffer;
> +		vin->buf_hw[slot].buffer = vbuf;
> +		vin->buf_hw[slot].type = HALF_BOTTOM;
> +		switch (vin->format.pixelformat) {
> +		case V4L2_PIX_FMT_NV12:
> +		case V4L2_PIX_FMT_NV16:
> +			phys_addr = vin->buf_hw[prev].phys +
> +				vin->format.sizeimage / 4;

Thanks for your reply on v2, but I'm still a bit puzzled, but it
might just be my poor understanding of the issue.

From your reply I get that you want to end up with:

start ->+---------+   /
        |  Y odd  |   |
irq <-  +---------+   | v
        |  Y even |   | b
irq <-  +---------+   | u
        |CbCr  odd|   | f
irq <-  +---------+   |
        |CbCr even|   |
irq <-  +---------+   /

and to do so you advance phys addrs by 1/4 every time.
Did I get you right ?

I would have expected multiplanar formats when sent as TB/BT

       +---------+   /
       |  Y odd  |   |
       +---------+   | HALF_TOP
       |CbCr odd |   |
       +---------+   /
       |  Y even |   |
       +---------+   | HALF_BOTTOM
       |CbCr even|   |
       +---------+   /

And the final buffer, after 2 irqs to look like

start ->+---------+   /
        |  Y odd  |   |
        +---------+   | v
        |CbCr odd |   | b
irq <-  +---------+   | u
        |  Y even |   | f
        +---------+   |
        |CbCr even|   |
irq <-  +---------+   /

Have you tested NV12/NV16 ? what am I missing ?

> +			break;
> +		default:
> +			phys_addr = vin->buf_hw[prev].phys +
> +				vin->format.sizeimage / 2;

Nit: please align the + signe below the =, or align vin->format with
vin->buf... As you like it the most, as there are no other occurences
in the diver of multi-line operations like this one.


> +			break;
> +		}
> +	} else if (list_empty(&vin->buf_list)) {
>  		vin->buf_hw[slot].buffer = NULL;
> +		vin->buf_hw[slot].type = FULL;
>  		phys_addr = vin->scratch_phys;
>  	} else {
>  		/* Keep track of buffer we give to HW */
> @@ -859,10 +878,18 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  		list_del_init(to_buf_list(vbuf));
>  		vin->buf_hw[slot].buffer = vbuf;
>
> +		vin->buf_hw[slot].type =
> +			V4L2_FIELD_IS_SEQUENTIAL(vin->format.field) ?
> +			HALF_TOP : FULL;
> +

This seems sane, but do you think it would be more readable if the
whole
        if (HALF_TOP) {

        } else if (list_empty(&vin->buf_list)) {

        } else { /* which catches HALF_BOTTOM and FULL */

        }

could be re-structured as:

	if (list_empty(&vin->buf_list)) {
		vin->buf_hw[slot].buffer = NULL;
		vin->buf_hw[slot].type = FULL;
		vin->buf_hw[slot].phys = vin->scratch_phys;
		rvin_set_slot_addr(vin, slot, phys_addr);

		return;
	}

	switch (vin_buf_hw[prev].type) {
	case HALF_TOP:

		break;
	case HALF_BOTTOM:
	case FULL:

		break;
	}

>  		/* Setup DMA */

Even if you want to keep the current code structure, could you drop
these comments, as the same happens in the if (HALF_TOP) branch, but
it's only commented here. It feels a bit weird looking at the end
result

>  		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
>  	}
>
> +	vin_dbg(vin, "Filling HW slot: %d type: %d buffer: %p\n",
> +		slot, vin->buf_hw[slot].type, vin->buf_hw[slot].buffer);
> +
> +	vin->buf_hw[slot].phys = phys_addr;
>  	rvin_set_slot_addr(vin, slot, phys_addr);
>  }
>
> @@ -870,6 +897,11 @@ static int rvin_capture_start(struct rvin_dev *vin)
>  {
>  	int slot, ret;

slot is an unsigned int

Actually rvin_fill_hw_slot() takes an int. Why so ?

>
> +	for (slot = 0; slot < HW_BUFFER_NUM; slot++) {
> +		vin->buf_hw[slot].buffer = NULL;
> +		vin->buf_hw[slot].type = FULL;
> +	}
> +
>  	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
>  		rvin_fill_hw_slot(vin, slot);
>
> @@ -954,6 +986,16 @@ static irqreturn_t rvin_irq(int irq, void *data)
>
>  	/* Capture frame */
>  	if (vin->buf_hw[slot].buffer) {
> +		/*
> +		 * Nothing to do but refill the hardware slot if
> +		 * capture only filled first half of vb2 buffer.
> +		 */
> +		if (vin->buf_hw[slot].type == HALF_TOP) {
> +			vin->buf_hw[slot].buffer = NULL;
> +			rvin_fill_hw_slot(vin, slot);

Have you considered jumping to the below rvin_fill_hw_slot() which is
just one line above the 'done' label you jump to here ?
Could you
                        goto fill_hw_slot;

?

Bonus point if you can avoid to duplicate
			vin->buf_hw[slot].buffer = NULL;

How would something like this look like ?

	if (vin->buf_hw[slot].buffer) {
  		struct vb2_v4l2_buffer *done = vin->buf_hw[slot].buffer;
                vin->buf_hw[slot].buffer = NULL;

		/*
		 * Nothing to do but refill the hardware slot if
		 * capture only filled first half of vb2 buffer.
		 */
		if (vin->buf_hw[slot].type == HALF_TOP)
                        goto fill_hw_slot;

		done->field = rvin_get_active_field(vin, vnms);
		done->sequence = vin->sequence;
		done->vb2_buf.timestamp = ktime_get_ns();
		vb2_buffer_done(&done->vb2_buf, VB2_BUF_STATE_DONE);
       }

> +			goto done;
> +		}
> +
>  		vin->buf_hw[slot].buffer->field =
>  			rvin_get_active_field(vin, vnms);
>  		vin->buf_hw[slot].buffer->sequence = vin->sequence;
> @@ -981,14 +1023,22 @@ static void return_all_buffers(struct rvin_dev *vin,
>  			       enum vb2_buffer_state state)
>  {
>  	struct rvin_buffer *buf, *node;
> -	int i;
> +	struct vb2_v4l2_buffer *freed[HW_BUFFER_NUM];
> +	unsigned int i, n;

nit: you could keep n inside the for loop if I'm not mistaken

>
>  	for (i = 0; i < HW_BUFFER_NUM; i++) {
> -		if (vin->buf_hw[i].buffer) {
> -			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
> -					state);
> -			vin->buf_hw[i].buffer = NULL;
> +		freed[i] = vin->buf_hw[i].buffer;
> +		vin->buf_hw[i].buffer = NULL;
> +
> +		for (n = 0; n < i; n++) {
> +			if (freed[i] == freed[n]) {
> +				freed[i] = NULL;
> +				break;
> +			}
>  		}
> +
> +		if (freed[i])
> +			vb2_buffer_done(&freed[i]->vb2_buf, state);
>  	}
>
>  	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 9e2e63ffcc47acad..7ab938ae93826675 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -107,6 +107,9 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
>  		break;
>  	}
>
> +	if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> +		align = 0x80;
> +
>  	return ALIGN(pix->width, align) * fmt->bpp;
>  }
>
> @@ -137,6 +140,8 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
>  	case V4L2_FIELD_INTERLACED_BT:
>  	case V4L2_FIELD_INTERLACED:
>  	case V4L2_FIELD_ALTERNATE:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
>  		break;
>  	default:
>  		pix->field = RVIN_DEFAULT_FIELD;
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 0aa904a4af5b0a97..c19d077ce1cb4f4b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -60,6 +60,23 @@ enum rvin_dma_state {
>  	STOPPING,
>  };
>
> +/**
> + * enum rvin_buffer_type
> + *
> + * Describes how a buffer is given to the hardware. To be able

I would say that it describes how the frame's fields are expected to
be received and stored into memory, with FULL describing progressive
capture operations where one pass fills the whole memory buffer and
HALF_TOP/HALF_BOTTOM describing sequential interlaced capture
operations where to capture a full frame the same vb2 buffer has to be
filled by two consective passes.

> + * to capture SEQ_TB/BT it's needed to capture to the same vb2
> + * buffer twice so the type of buffer needs to be kept.
> + *
> + * FULL - One capture fills the whole vb2 buffer
> + * HALF_TOP - One capture fills the top half of the vb2 buffer
> + * HALF_BOTTOM - One capture fills the bottom half of the vb2 buffer
> + */
> +enum rvin_buffer_type {
> +	FULL,
> +	HALF_TOP,
> +	HALF_BOTTOM,
> +};
> +

Even if the documentation for the VIN interface is not generated, I
would use kerneldoc already.

With the NV12/16 thing clarified and a confirmation that it works, all
I have here are style suggestions you're free to take in or ignore, so
please add
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>  /**
>   * struct rvin_video_format - Data format stored in memory
>   * @fourcc:	Pixelformat
> @@ -206,6 +223,8 @@ struct rvin_dev {
>  	spinlock_t qlock;
>  	struct {
>  		struct vb2_v4l2_buffer *buffer;
> +		enum rvin_buffer_type type;
> +		dma_addr_t phys;
>  	} buf_hw[HW_BUFFER_NUM];
>  	struct list_head buf_list;
>  	unsigned int sequence;
> --
> 2.24.0
>

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

Re: [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-12-11 10:44:11 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Dec 10, 2019 at 03:05:58AM +0100, Niklas Söderlund wrote:
> > To support SEQ_TB/BT not all buffers given to the hardware will be
> > equal, the driver needs to keep track of different buffer types. Move
> > the tracking of buffers given to hardware into a struct so additional
> > tracking fields can be associated with each buffer.
> >
> 
> This change alone does not make sense by itself. I cannot judge if
> it's a good idea or not if not looking at 2/2. Why have you kept it
> separate ?

That's why they are grouped in a series and not sent as two separate 
patches. I split things as I would like to review them. If there is a 
rename of a variable or other no functional change that takes up a lot 
of screen space I will split it out into it's own patch and make it 
clear that's all that's in there and then follow up with the series real 
change. I find this allows for a better review of the real change in the 
series as the preparation step is quiet uninteresting.

> 
> Thanks
>    j
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 27 +++++++++++-----------
> >  drivers/media/platform/rcar-vin/rcar-vin.h |  9 ++++----
> >  2 files changed, 19 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index cf9029efeb0450cb..cd1778977b2ba56e 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -844,20 +844,20 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> >  	dma_addr_t phys_addr;
> >
> >  	/* A already populated slot shall never be overwritten. */
> > -	if (WARN_ON(vin->queue_buf[slot] != NULL))
> > +	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
> >  		return;
> >
> >  	vin_dbg(vin, "Filling HW slot: %d\n", slot);
> >
> >  	if (list_empty(&vin->buf_list)) {
> > -		vin->queue_buf[slot] = NULL;
> > +		vin->buf_hw[slot].buffer = 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;
> > +		vin->buf_hw[slot].buffer = vbuf;
> >
> >  		/* Setup DMA */
> >  		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> > @@ -953,13 +953,14 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	}
> >
> >  	/* Capture frame */
> > -	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,
> > +	if (vin->buf_hw[slot].buffer) {
> > +		vin->buf_hw[slot].buffer->field =
> > +			rvin_get_active_field(vin, vnms);
> > +		vin->buf_hw[slot].buffer->sequence = vin->sequence;
> > +		vin->buf_hw[slot].buffer->vb2_buf.timestamp = ktime_get_ns();
> > +		vb2_buffer_done(&vin->buf_hw[slot].buffer->vb2_buf,
> >  				VB2_BUF_STATE_DONE);
> > -		vin->queue_buf[slot] = NULL;
> > +		vin->buf_hw[slot].buffer = NULL;
> >  	} else {
> >  		/* Scratch buffer was used, dropping frame. */
> >  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> > @@ -983,10 +984,10 @@ static void return_all_buffers(struct rvin_dev *vin,
> >  	int i;
> >
> >  	for (i = 0; i < HW_BUFFER_NUM; i++) {
> > -		if (vin->queue_buf[i]) {
> > -			vb2_buffer_done(&vin->queue_buf[i]->vb2_buf,
> > +		if (vin->buf_hw[i].buffer) {
> > +			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
> >  					state);
> > -			vin->queue_buf[i] = NULL;
> > +			vin->buf_hw[i].buffer = NULL;
> >  		}
> >  	}
> >
> > @@ -1291,7 +1292,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >  	vin->state = STOPPED;
> >
> >  	for (i = 0; i < HW_BUFFER_NUM; i++)
> > -		vin->queue_buf[i] = NULL;
> > +		vin->buf_hw[i].buffer = NULL;
> >
> >  	/* buffer queue */
> >  	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index a36b0824f81d171d..0aa904a4af5b0a97 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -164,9 +164,8 @@ struct rvin_info {
> >   * @scratch:		cpu address for scratch buffer
> >   * @scratch_phys:	physical address of the scratch buffer
> >   *
> > - * @qlock:		protects @queue_buf, @buf_list, @sequence
> > - *			@state
> > - * @queue_buf:		Keeps track of buffers given to HW slot
> > + * @qlock:		protects @buf_hw, @buf_list, @sequence and @state
> > + * @buf_hw:		Keeps track of buffers given to HW slot
> >   * @buf_list:		list of queued buffers
> >   * @sequence:		V4L2 buffers sequence number
> >   * @state:		keeps track of operation state
> > @@ -205,7 +204,9 @@ struct rvin_dev {
> >  	dma_addr_t scratch_phys;
> >
> >  	spinlock_t qlock;
> > -	struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> > +	struct {
> > +		struct vb2_v4l2_buffer *buffer;
> > +	} buf_hw[HW_BUFFER_NUM];
> >  	struct list_head buf_list;
> >  	unsigned int sequence;
> >  	enum rvin_dma_state state;
> > --
> > 2.24.0
> >



-- 
Regards,
Niklas Söderlund