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 {
> +   

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

[Niklas Söderlund]

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;
+   } else {
+   /* 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 =