Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Hans Verkuil]

On 03/09/2018 04:09 PM, 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

continues -> continuous

> 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.

ditto

> 
> 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(>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 = >vb;
> - list_del_init(to_buf_list(vbuf));
> - vin->queue_buf[slot] = vbuf;
> -
> - /* Setup DMA */
> - phys_addr_top = vb2_dma_contig_plane_dma_addr(>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(>buf_list)) {
> + vin->queue_buf[slot] = NULL;
> + phys_addr = vin->scratch_phys;
> + } else {
> + /* Keep track of buffer we give to HW */
> + buf = list_entry(vin->buf_list.next, struct 

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-03-12 15:38:12 +0100, Jacopo Mondi wrote:
> 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(>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 = >vb;
> > -   list_del_init(to_buf_list(vbuf));
> > -   vin->queue_buf[slot] = vbuf;
> > -
> > -   /* Setup DMA */
> > -   phys_addr_top = vb2_dma_contig_plane_dma_addr(>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 

Re: [PATCH 3/3] rcar-vin: use scratch buffer and always run in continuous mode

[jacopo mondi]

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(>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 = >vb;
> - list_del_init(to_buf_list(vbuf));
> - vin->queue_buf[slot] = vbuf;
> -
> - /* Setup DMA */
> - phys_addr_top = vb2_dma_contig_plane_dma_addr(>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(>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 {
> +