| Message ID | 20191210020559.170594-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Support V4L2_FIELD_SEQ_{TB,BT} | expand |
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 >
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 > >
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;
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(-)