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 > --- > 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 Thanks j > > -- > 2.16.2 >