* [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT}
@ 2019-12-10 2:05 Niklas Söderlund
2019-12-10 2:05 ` [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct Niklas Söderlund
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Niklas Söderlund @ 2019-12-10 2:05 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct 2019-12-10 2:05 [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund @ 2019-12-10 2:05 ` Niklas Söderlund 2019-12-11 9:44 ` Jacopo Mondi 2019-12-10 2:05 ` [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund 2019-12-11 9:45 ` [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Jacopo Mondi 2 siblings, 1 reply; 7+ messages in thread From: Niklas Söderlund @ 2019-12-10 2:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct 2019-12-10 2:05 ` [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct Niklas Söderlund @ 2019-12-11 9:44 ` Jacopo Mondi 2019-12-11 11:28 ` Niklas Söderlund 0 siblings, 1 reply; 7+ messages in thread From: Jacopo Mondi @ 2019-12-11 9:44 UTC (permalink / raw) To: Niklas Söderlund; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct 2019-12-11 9:44 ` Jacopo Mondi @ 2019-12-11 11:28 ` Niklas Söderlund 0 siblings, 0 replies; 7+ messages in thread From: Niklas Söderlund @ 2019-12-11 11:28 UTC (permalink / raw) To: Jacopo Mondi; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} 2019-12-10 2:05 [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund 2019-12-10 2:05 ` [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct Niklas Söderlund @ 2019-12-10 2:05 ` Niklas Söderlund 2019-12-11 11:22 ` Jacopo Mondi 2019-12-11 9:45 ` [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Jacopo Mondi 2 siblings, 1 reply; 7+ messages in thread From: Niklas Söderlund @ 2019-12-10 2:05 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} 2019-12-10 2:05 ` [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund @ 2019-12-11 11:22 ` Jacopo Mondi 0 siblings, 0 replies; 7+ messages in thread From: Jacopo Mondi @ 2019-12-11 11:22 UTC (permalink / raw) To: Niklas Söderlund; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} 2019-12-10 2:05 [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund 2019-12-10 2:05 ` [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct Niklas Söderlund 2019-12-10 2:05 ` [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund @ 2019-12-11 9:45 ` Jacopo Mondi 2 siblings, 0 replies; 7+ messages in thread From: Jacopo Mondi @ 2019-12-11 9:45 UTC (permalink / raw) To: Niklas Söderlund; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-11 11:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 2:05 [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund
2019-12-10 2:05 ` [PATCH v3 1/2] rcar-vin: Move hardware buffer tracking to own struct Niklas Söderlund
2019-12-11 9:44 ` Jacopo Mondi
2019-12-11 11:28 ` Niklas Söderlund
2019-12-10 2:05 ` [PATCH v3 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT} Niklas Söderlund
2019-12-11 11:22 ` Jacopo Mondi
2019-12-11 9:45 ` [PATCH v3 0/2] rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} Jacopo Mondi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.