Hi Niklas,
a few comments below
On Sat, Mar 10, 2018 at 01:09:53AM +0100, Niklas Söderlund wrote:
> Instead of switching capture mode depending on how many buffers are
> available use a scratch buffer and always run in continuous mode. By
> using a scratch buffer the responsiveness of the capture loop is
> increased as it can keep running even if there are no buffers available
> from userspace.
>
> As soon as a userspace queues a buffer it is inserted into the capture
> loop and returned as soon as it is filled. This is a improvement on the
> previous logic where the whole capture loop was stopped and switched to
> single capture mode if userspace did not feed the VIN driver buffers at
> the same time it consumed them. To make matters worse it was difficult
> for the driver to reenter continues mode if it entered single mode even
> if userspace started to queue buffers faster. This resulted in
> suboptimal performance where if userspace where delayed for a short
> period the ongoing capture would be slowed down and run in single mode
> until the capturing process where restarted.
>
> An additional effect of this change is that the capture logic can be
> made much simple as we know that continues mode will always be used.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 187 ++++++++---------------------
> drivers/media/platform/rcar-vin/rcar-vin.h | 6 +-
> 2 files changed, 52 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 8ea73cdc9a720abe..208cf8a0ea77002d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -168,12 +168,8 @@ static int rvin_setup(struct rvin_dev *vin)
> break;
> case V4L2_FIELD_ALTERNATE:
> case V4L2_FIELD_NONE:
> - if (vin->continuous) {
> - vnmc = VNMC_IM_ODD_EVEN;
> - progressive = true;
> - } else {
> - vnmc = VNMC_IM_ODD;
> - }
> + vnmc = VNMC_IM_ODD_EVEN;
> + progressive = true;
> break;
> default:
> vnmc = VNMC_IM_ODD;
> @@ -298,14 +294,6 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> return rvin_read(vin, VNMS_REG) & VNMS_CA;
> }
>
> -static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> -{
> - if (vin->continuous)
> - return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> -
> - return 0;
> -}
> -
> static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> {
> if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> @@ -344,76 +332,47 @@ static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> rvin_write(vin, offset, VNMB_REG(slot));
> }
>
> -/* Moves a buffer from the queue to the HW slots */
> -static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> +/*
> + * Moves a buffer from the queue to the HW slot. If no buffer is
> + * available use the scratch buffer. The scratch buffer is never
> + * returned to userspace, its only function is to enable the capture
> + * loop to keep running.
> + */
> +static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> {
> struct rvin_buffer *buf;
> struct vb2_v4l2_buffer *vbuf;
> - dma_addr_t phys_addr_top;
> -
> - if (vin->queue_buf[slot] != NULL)
> - return true;
> + dma_addr_t phys_addr;
>
> - if (list_empty(&vin->buf_list))
> - return false;
> + /* A already populated slot shall never be overwritten. */
> + if (WARN_ON(vin->queue_buf[slot] != NULL))
> + return;
>
> vin_dbg(vin, "Filling HW slot: %d\n", slot);
>
> - /* Keep track of buffer we give to HW */
> - buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> - vbuf = &buf->vb;
> - list_del_init(to_buf_list(vbuf));
> - vin->queue_buf[slot] = vbuf;
> -
> - /* Setup DMA */
> - phys_addr_top = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> - rvin_set_slot_addr(vin, slot, phys_addr_top);
> -
> - return true;
> -}
> -
> -static bool rvin_fill_hw(struct rvin_dev *vin)
> -{
> - int slot, limit;
> -
> - limit = vin->continuous ? HW_BUFFER_NUM : 1;
> -
> - for (slot = 0; slot < limit; slot++)
> - if (!rvin_fill_hw_slot(vin, slot))
> - return false;
> - return true;
> -}
> -
> -static void rvin_capture_on(struct rvin_dev *vin)
> -{
> - vin_dbg(vin, "Capture on in %s mode\n",
> - vin->continuous ? "continuous" : "single");
> + if (list_empty(&vin->buf_list)) {
> + vin->queue_buf[slot] = NULL;
> + phys_addr = vin->scratch_phys;
I guess it is not an issue if MB1/MB2 and MB3 they all get pointed to
the same scratch buffer, right?
> + } else {
> + /* Keep track of buffer we give to HW */
> + buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> + vbuf = &buf->vb;
> + list_del_init(to_buf_list(vbuf));
> + vin->queue_buf[slot] = vbuf;
> +
> + /* Setup DMA */
> + phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> + }
>
> - if (vin->continuous)
> - /* Continuous Frame Capture Mode */
> - rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> - else
> - /* Single Frame Capture Mode */
> - rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> + rvin_set_slot_addr(vin, slot, phys_addr);
> }
>
> static int rvin_capture_start(struct rvin_dev *vin)
> {
> - struct rvin_buffer *buf, *node;
> - int bufs, ret;
> + int slot, ret;
>
> - /* Count number of free buffers */
> - bufs = 0;
> - list_for_each_entry_safe(buf, node, &vin->buf_list, list)
> - bufs++;
> -
> - /* Continuous capture requires more buffers then there are HW slots */
> - vin->continuous = bufs > HW_BUFFER_NUM;
> -
> - if (!rvin_fill_hw(vin)) {
> - vin_err(vin, "HW not ready to start, not enough buffers available\n");
> - return -EINVAL;
> - }
> + for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> + rvin_fill_hw_slot(vin, slot);
>
> rvin_crop_scale_comp(vin);
>
> @@ -421,7 +380,10 @@ static int rvin_capture_start(struct rvin_dev *vin)
> if (ret)
> return ret;
>
> - rvin_capture_on(vin);
> + vin_dbg(vin, "Starting to capture\n");
> +
> + /* Continuous Frame Capture Mode */
> + rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
>
> vin->state = RUNNING;
>
> @@ -904,7 +866,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> struct rvin_dev *vin = data;
> u32 int_status, vnms;
> int slot;
> - unsigned int i, sequence, handled = 0;
> + unsigned int handled = 0;
> unsigned long flags;
>
> spin_lock_irqsave(&vin->qlock, flags);
> @@ -924,65 +886,25 @@ static irqreturn_t rvin_irq(int irq, void *data)
>
> /* Prepare for capture and update state */
> vnms = rvin_read(vin, VNMS_REG);
> - slot = rvin_get_active_slot(vin, vnms);
> - sequence = vin->sequence++;
> -
> - vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> - sequence, slot,
> - slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
> - slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
> - slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
> - !list_empty(&vin->buf_list));
> -
> - /* HW have written to a slot that is not prepared we are in trouble */
> - if (WARN_ON((vin->queue_buf[slot] == NULL)))
> - goto done;
> + slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>
> /* Capture frame */
> - vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> - vin->queue_buf[slot]->sequence = sequence;
> - vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> - vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> - vin->queue_buf[slot] = NULL;
> -
> - /* Prepare for next frame */
> - if (!rvin_fill_hw(vin)) {
> -
> - /*
> - * Can't supply HW with new buffers fast enough. Halt
> - * capture until more buffers are available.
> - */
> - vin->state = STALLED;
> -
> - /*
> - * The continuous capturing requires an explicit stop
> - * operation when there is no buffer to be set into
> - * the VnMBm registers.
> - */
> - if (vin->continuous) {
> - rvin_capture_stop(vin);
> - vin_dbg(vin, "IRQ %02d: hw not ready stop\n", sequence);
> -
> - /* Maybe we can continue in single capture mode */
> - for (i = 0; i < HW_BUFFER_NUM; i++) {
> - if (vin->queue_buf[i]) {
> - list_add(to_buf_list(vin->queue_buf[i]),
> - &vin->buf_list);
> - vin->queue_buf[i] = NULL;
> - }
> - }
> -
> - if (!list_empty(&vin->buf_list))
> - rvin_capture_start(vin);
> - }
> + if (vin->queue_buf[slot]) {
> + vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> + vin->queue_buf[slot]->sequence = vin->sequence;
> + vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> + vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
> + VB2_BUF_STATE_DONE);
> + vin->queue_buf[slot] = NULL;
> } else {
> - /*
> - * The single capturing requires an explicit capture
> - * operation to fetch the next frame.
> - */
> - if (!vin->continuous)
> - rvin_capture_on(vin);
> + /* Scratch buffer was used, dropping frame. */
> + vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
Nit: even if that's debug output, you're going to prin out this
message every discarded frame. Isn't this a bit too much?
> }
> +
> + vin->sequence++;
Sequence counter is incremented even if frame is discarded. Is this
intended?
> +
> + /* Prepare for next frame */
> + rvin_fill_hw_slot(vin, slot);
> done:
> spin_unlock_irqrestore(&vin->qlock, flags);
>
> @@ -1053,13 +975,6 @@ static void rvin_buffer_queue(struct vb2_buffer *vb)
>
> list_add_tail(to_buf_list(vbuf), &vin->buf_list);
>
> - /*
> - * If capture is stalled add buffer to HW and restart
> - * capturing if HW is ready to continue.
> - */
> - if (vin->state == STALLED)
> - rvin_capture_start(vin);
> -
> spin_unlock_irqrestore(&vin->qlock, flags);
> }
>
> @@ -1202,7 +1117,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
> q->ops = &rvin_qops;
> q->mem_ops = &vb2_dma_contig_memops;
> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> - q->min_buffers_needed = 1;
> + q->min_buffers_needed = 4;
> q->dev = vin->dev;
>
> ret = vb2_queue_init(q);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 11a981d707c7ca47..1adc1b809f761e71 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -38,13 +38,11 @@ enum chip_id {
> /**
> * STOPPED - No operation in progress
> * RUNNING - Operation in progress have buffers
> - * STALLED - No operation in progress have no buffers
> * STOPPING - Stopping operation
> */
> enum rvin_dma_state {
> STOPPED = 0,
> RUNNING,
> - STALLED,
> STOPPING,
> };
>
> @@ -105,11 +103,10 @@ struct rvin_graph_entity {
> * @scratch: cpu address for scratch buffer
> * @scratch_phys: pysical address of the scratch buffer
> *
> - * @qlock: protects @queue_buf, @buf_list, @continuous, @sequence
> + * @qlock: protects @queue_buf, @buf_list, @sequence
> * @state
> * @queue_buf: Keeps track of buffers given to HW slot
> * @buf_list: list of queued buffers
> - * @continuous: tracks if active operation is continuous or single mode
> * @sequence: V4L2 buffers sequence number
> * @state: keeps track of operation state
> *
> @@ -138,7 +135,6 @@ struct rvin_dev {
> spinlock_t qlock;
> struct vb2_v4l2_buffer *queue_buf[HW_BUFFER_NUM];
> struct list_head buf_list;
> - bool continuous;
> unsigned int sequence;
> enum rvin_dma_state state;
With the above clarified, for the whole series:
Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
Thanks
j
>
> --
> 2.16.2
>
Attachment:
signature.asc
Description: PGP signature
