Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe

[Helen Koike]

Hi Hans,

Thank you for your review

On 2017-06-12 07:03 AM, Hans Verkuil wrote:

On 06/03/2017 04:58 AM, Helen Koike wrote:

Add a parameter called vsen_tpg, if true then vimc will work as before:
frames will be generated in the sensor nodes then propagated through the
pipe and processed by each node until a capture node is reached.
If vsen_tpg is false, then the frame is not generated by the sensor, but
directly from the capture node, thus saving intermediate memory buffers
and process time, allowing a higher frame rate.

Signed-off-by: Helen Koike 

---

Changes in v3:
[media] vimc: Optimize frame generation through the pipe
- This is a new patch in the series

Changes in v2: None


---
  drivers/media/platform/vimc/vimc-capture.c | 178
+
  drivers/media/platform/vimc/vimc-common.h  |   2 +
  drivers/media/platform/vimc/vimc-sensor.c  |  30 -
  3 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c
b/drivers/media/platform/vimc/vimc-capture.c
index e943267..b5da0ea 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -15,7 +15,10 @@
   *
   */
  +#include 
+#include 
  #include 
+#include 
  #include 
  #include 
  @@ -38,6 +41,8 @@ struct vimc_cap_device {
  struct mutex lock;
  u32 sequence;
  struct media_pipeline pipe;
+struct tpg_data tpg;
+struct task_struct *kthread_cap;
  };
static const struct v4l2_pix_format fmt_default = {
@@ -246,6 +251,91 @@ static void vimc_cap_return_all_buffers(struct
vimc_cap_device *vcap,
  spin_unlock(>qlock);
  }
  +static void vimc_cap_process_frame(struct vimc_ent_device *ved,
+   struct media_pad *sink, const void *frame)
+{
+struct vimc_cap_device *vcap = container_of(ved, struct
vimc_cap_device,
+ved);
+struct vimc_cap_buffer *vimc_buf;
+void *vbuf;
+
+spin_lock(>qlock);
+
+/* Get the first entry of the list */
+vimc_buf = list_first_entry_or_null(>buf_list,
+typeof(*vimc_buf), list);
+if (!vimc_buf) {
+spin_unlock(>qlock);
+return;
+}
+
+/* Remove this entry from the list */
+list_del(_buf->list);
+
+spin_unlock(>qlock);
+
+/* Fill the buffer */
+vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
+vimc_buf->vb2.sequence = vcap->sequence++;
+vimc_buf->vb2.field = vcap->format.field;
+
+vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
+
+if (sink)
+memcpy(vbuf, frame, vcap->format.sizeimage);
+else
+tpg_fill_plane_buffer(>tpg, V4L2_STD_PAL, 0, vbuf);
+
+/* Set it as ready */
+vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
+  vcap->format.sizeimage);
+vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+
+static int vimc_cap_tpg_thread(void *data)
+{
+struct vimc_cap_device *vcap = data;
+
+set_freezable();
+set_current_state(TASK_UNINTERRUPTIBLE);
+
+for (;;) {
+try_to_freeze();
+if (kthread_should_stop())
+break;
+
+vimc_cap_process_frame(>ved, NULL, NULL);
+
+/* 60 frames per second */
+schedule_timeout(HZ/60);
+}
+
+return 0;
+}
+
+static void vimc_cap_tpg_s_format(struct vimc_cap_device *vcap)
+{
+const struct vimc_pix_map *vpix =
+vimc_pix_map_by_pixelformat(vcap->format.pixelformat);
+
+tpg_reset_source(>tpg, vcap->format.width,
vcap->format.height,
+ vcap->format.field);
+tpg_s_bytesperline(>tpg, 0, vcap->format.width * vpix->bpp);
+tpg_s_buf_height(>tpg, vcap->format.height);
+tpg_s_fourcc(>tpg, vpix->pixelformat);
+/*
+ * TODO: check why the tpg_s_field need this third argument if
+ * it is already receiving the field
+ */
+tpg_s_field(>tpg, vcap->format.field,
+vcap->format.field == V4L2_FIELD_ALTERNATE);


I thought I explained that earlier? When in alternate field mode the
format.field
value will be TOP or BOTTOM, but never ALTERNATE. So tpg_s_field needs
to know if
it will create TOP or BOTTOM fields only, or if they will be alternating.


Yes, sorry about that. I removed from the vimc_sensor but I forgot it 
here. I am dropping this patch in the series as I found another bug when 
multiple links are enabled going to the same sink pad. I'll re-work in 
this optimization and re-send another version in the future in a 
different patch series.




Since we don't support ALTERNATE anyway, just pass false as the third
argument and
drop the comment.


+tpg_s_colorspace(>tpg, vcap->format.colorspace);
+tpg_s_ycbcr_enc(>tpg, vcap->format.ycbcr_enc);
+tpg_s_quantization(>tpg, vcap->format.quantization);
+tpg_s_xfer_func(>tpg, vcap->format.xfer_func);
+}
+
  static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned
int count)
  {
  struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
@@ 

Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe

[Hans Verkuil]

On 06/03/2017 04:58 AM, Helen Koike wrote:

Add a parameter called vsen_tpg, if true then vimc will work as before:
frames will be generated in the sensor nodes then propagated through the
pipe and processed by each node until a capture node is reached.
If vsen_tpg is false, then the frame is not generated by the sensor, but
directly from the capture node, thus saving intermediate memory buffers
and process time, allowing a higher frame rate.

Signed-off-by: Helen Koike 

---

Changes in v3:
[media] vimc: Optimize frame generation through the pipe
- This is a new patch in the series

Changes in v2: None


---
  drivers/media/platform/vimc/vimc-capture.c | 178 +
  drivers/media/platform/vimc/vimc-common.h  |   2 +
  drivers/media/platform/vimc/vimc-sensor.c  |  30 -
  3 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c 
b/drivers/media/platform/vimc/vimc-capture.c
index e943267..b5da0ea 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -15,7 +15,10 @@
   *
   */
  
+#include 

+#include 
  #include 
+#include 
  #include 
  #include 
  
@@ -38,6 +41,8 @@ struct vimc_cap_device {

struct mutex lock;
u32 sequence;
struct media_pipeline pipe;
+   struct tpg_data tpg;
+   struct task_struct *kthread_cap;
  };
  
  static const struct v4l2_pix_format fmt_default = {

@@ -246,6 +251,91 @@ static void vimc_cap_return_all_buffers(struct 
vimc_cap_device *vcap,
spin_unlock(>qlock);
  }
  
+static void vimc_cap_process_frame(struct vimc_ent_device *ved,

+  struct media_pad *sink, const void *frame)
+{
+   struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
+   ved);
+   struct vimc_cap_buffer *vimc_buf;
+   void *vbuf;
+
+   spin_lock(>qlock);
+
+   /* Get the first entry of the list */
+   vimc_buf = list_first_entry_or_null(>buf_list,
+   typeof(*vimc_buf), list);
+   if (!vimc_buf) {
+   spin_unlock(>qlock);
+   return;
+   }
+
+   /* Remove this entry from the list */
+   list_del(_buf->list);
+
+   spin_unlock(>qlock);
+
+   /* Fill the buffer */
+   vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
+   vimc_buf->vb2.sequence = vcap->sequence++;
+   vimc_buf->vb2.field = vcap->format.field;
+
+   vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
+
+   if (sink)
+   memcpy(vbuf, frame, vcap->format.sizeimage);
+   else
+   tpg_fill_plane_buffer(>tpg, V4L2_STD_PAL, 0, vbuf);
+
+   /* Set it as ready */
+   vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
+ vcap->format.sizeimage);
+   vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+
+static int vimc_cap_tpg_thread(void *data)
+{
+   struct vimc_cap_device *vcap = data;
+
+   set_freezable();
+   set_current_state(TASK_UNINTERRUPTIBLE);
+
+   for (;;) {
+   try_to_freeze();
+   if (kthread_should_stop())
+   break;
+
+   vimc_cap_process_frame(>ved, NULL, NULL);
+
+   /* 60 frames per second */
+   schedule_timeout(HZ/60);
+   }
+
+   return 0;
+}
+
+static void vimc_cap_tpg_s_format(struct vimc_cap_device *vcap)
+{
+   const struct vimc_pix_map *vpix =
+   vimc_pix_map_by_pixelformat(vcap->format.pixelformat);
+
+   tpg_reset_source(>tpg, vcap->format.width, vcap->format.height,
+vcap->format.field);
+   tpg_s_bytesperline(>tpg, 0, vcap->format.width * vpix->bpp);
+   tpg_s_buf_height(>tpg, vcap->format.height);
+   tpg_s_fourcc(>tpg, vpix->pixelformat);
+   /*
+* TODO: check why the tpg_s_field need this third argument if
+* it is already receiving the field
+*/
+   tpg_s_field(>tpg, vcap->format.field,
+   vcap->format.field == V4L2_FIELD_ALTERNATE);


I thought I explained that earlier? When in alternate field mode the 
format.field
value will be TOP or BOTTOM, but never ALTERNATE. So tpg_s_field needs to know 
if
it will create TOP or BOTTOM fields only, or if they will be alternating.

Since we don't support ALTERNATE anyway, just pass false as the third argument 
and
drop the comment.


+   tpg_s_colorspace(>tpg, vcap->format.colorspace);
+   tpg_s_ycbcr_enc(>tpg, vcap->format.ycbcr_enc);
+   tpg_s_quantization(>tpg, vcap->format.quantization);
+   tpg_s_xfer_func(>tpg, vcap->format.xfer_func);
+}
+
  static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
  {
struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
@@ -256,20 +346,43 @@ static int vimc_cap_start_streaming(struct 

Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe

[Helen Koike]

On 2017-06-02 11:58 PM, Helen Koike wrote:

Add a parameter called vsen_tpg, if true then vimc will work as before:
frames will be generated in the sensor nodes then propagated through the
pipe and processed by each node until a capture node is reached.
If vsen_tpg is false, then the frame is not generated by the sensor, but
directly from the capture node, thus saving intermediate memory buffers
and process time, allowing a higher frame rate.

Signed-off-by: Helen Koike 

---

Changes in v3:
[media] vimc: Optimize frame generation through the pipe
- This is a new patch in the series

Changes in v2: None


---
 drivers/media/platform/vimc/vimc-capture.c | 178 +
 drivers/media/platform/vimc/vimc-common.h  |   2 +
 drivers/media/platform/vimc/vimc-sensor.c  |  30 -
 3 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c 
b/drivers/media/platform/vimc/vimc-capture.c
index e943267..b5da0ea 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -15,7 +15,10 @@
  *
  */

+#include 
+#include 
 #include 
+#include 
 #include 
 #include 

@@ -38,6 +41,8 @@ struct vimc_cap_device {
struct mutex lock;
u32 sequence;
struct media_pipeline pipe;
+   struct tpg_data tpg;
+   struct task_struct *kthread_cap;
 };

 static const struct v4l2_pix_format fmt_default = {
@@ -246,6 +251,91 @@ static void vimc_cap_return_all_buffers(struct 
vimc_cap_device *vcap,
spin_unlock(>qlock);
 }

+static void vimc_cap_process_frame(struct vimc_ent_device *ved,
+  struct media_pad *sink, const void *frame)
+{
+   struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
+   ved);
+   struct vimc_cap_buffer *vimc_buf;
+   void *vbuf;
+
+   spin_lock(>qlock);
+
+   /* Get the first entry of the list */
+   vimc_buf = list_first_entry_or_null(>buf_list,
+   typeof(*vimc_buf), list);
+   if (!vimc_buf) {
+   spin_unlock(>qlock);
+   return;
+   }
+
+   /* Remove this entry from the list */
+   list_del(_buf->list);
+
+   spin_unlock(>qlock);
+
+   /* Fill the buffer */
+   vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
+   vimc_buf->vb2.sequence = vcap->sequence++;
+   vimc_buf->vb2.field = vcap->format.field;
+
+   vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
+
+   if (sink)
+   memcpy(vbuf, frame, vcap->format.sizeimage);
+   else
+   tpg_fill_plane_buffer(>tpg, V4L2_STD_PAL, 0, vbuf);
+
+   /* Set it as ready */
+   vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
+ vcap->format.sizeimage);
+   vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+
+static int vimc_cap_tpg_thread(void *data)
+{
+   struct vimc_cap_device *vcap = data;
+
+   set_freezable();
+   set_current_state(TASK_UNINTERRUPTIBLE);
+
+   for (;;) {
+   try_to_freeze();
+   if (kthread_should_stop())
+   break;
+
+   vimc_cap_process_frame(>ved, NULL, NULL);
+
+   /* 60 frames per second */
+   schedule_timeout(HZ/60);
+   }
+
+   return 0;
+}
+
+static void vimc_cap_tpg_s_format(struct vimc_cap_device *vcap)
+{
+   const struct vimc_pix_map *vpix =
+   vimc_pix_map_by_pixelformat(vcap->format.pixelformat);
+
+   tpg_reset_source(>tpg, vcap->format.width, vcap->format.height,
+vcap->format.field);
+   tpg_s_bytesperline(>tpg, 0, vcap->format.width * vpix->bpp);
+   tpg_s_buf_height(>tpg, vcap->format.height);
+   tpg_s_fourcc(>tpg, vpix->pixelformat);
+   /*
+* TODO: check why the tpg_s_field need this third argument if
+* it is already receiving the field
+*/
+   tpg_s_field(>tpg, vcap->format.field,
+   vcap->format.field == V4L2_FIELD_ALTERNATE);
+   tpg_s_colorspace(>tpg, vcap->format.colorspace);
+   tpg_s_ycbcr_enc(>tpg, vcap->format.ycbcr_enc);
+   tpg_s_quantization(>tpg, vcap->format.quantization);
+   tpg_s_xfer_func(>tpg, vcap->format.xfer_func);
+}
+
 static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
@@ -256,20 +346,43 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, 
unsigned int count)

/* Start the media pipeline */
ret = media_pipeline_start(entity, >pipe);
-   if (ret) {
-   vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
-   return ret;
-   }
+   if (ret)
+   goto err_ret_all_buffs;

/* Enable streaming from the pipe */
ret = 

[RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe

[Helen Koike]

Add a parameter called vsen_tpg, if true then vimc will work as before:
frames will be generated in the sensor nodes then propagated through the
pipe and processed by each node until a capture node is reached.
If vsen_tpg is false, then the frame is not generated by the sensor, but
directly from the capture node, thus saving intermediate memory buffers
and process time, allowing a higher frame rate.

Signed-off-by: Helen Koike 

---

Changes in v3:
[media] vimc: Optimize frame generation through the pipe
- This is a new patch in the series

Changes in v2: None


---
 drivers/media/platform/vimc/vimc-capture.c | 178 +
 drivers/media/platform/vimc/vimc-common.h  |   2 +
 drivers/media/platform/vimc/vimc-sensor.c  |  30 -
 3 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c 
b/drivers/media/platform/vimc/vimc-capture.c
index e943267..b5da0ea 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -15,7 +15,10 @@
  *
  */
 
+#include 
+#include 
 #include 
+#include 
 #include 
 #include 
 
@@ -38,6 +41,8 @@ struct vimc_cap_device {
struct mutex lock;
u32 sequence;
struct media_pipeline pipe;
+   struct tpg_data tpg;
+   struct task_struct *kthread_cap;
 };
 
 static const struct v4l2_pix_format fmt_default = {
@@ -246,6 +251,91 @@ static void vimc_cap_return_all_buffers(struct 
vimc_cap_device *vcap,
spin_unlock(>qlock);
 }
 
+static void vimc_cap_process_frame(struct vimc_ent_device *ved,
+  struct media_pad *sink, const void *frame)
+{
+   struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
+   ved);
+   struct vimc_cap_buffer *vimc_buf;
+   void *vbuf;
+
+   spin_lock(>qlock);
+
+   /* Get the first entry of the list */
+   vimc_buf = list_first_entry_or_null(>buf_list,
+   typeof(*vimc_buf), list);
+   if (!vimc_buf) {
+   spin_unlock(>qlock);
+   return;
+   }
+
+   /* Remove this entry from the list */
+   list_del(_buf->list);
+
+   spin_unlock(>qlock);
+
+   /* Fill the buffer */
+   vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
+   vimc_buf->vb2.sequence = vcap->sequence++;
+   vimc_buf->vb2.field = vcap->format.field;
+
+   vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
+
+   if (sink)
+   memcpy(vbuf, frame, vcap->format.sizeimage);
+   else
+   tpg_fill_plane_buffer(>tpg, V4L2_STD_PAL, 0, vbuf);
+
+   /* Set it as ready */
+   vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
+ vcap->format.sizeimage);
+   vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+
+static int vimc_cap_tpg_thread(void *data)
+{
+   struct vimc_cap_device *vcap = data;
+
+   set_freezable();
+   set_current_state(TASK_UNINTERRUPTIBLE);
+
+   for (;;) {
+   try_to_freeze();
+   if (kthread_should_stop())
+   break;
+
+   vimc_cap_process_frame(>ved, NULL, NULL);
+
+   /* 60 frames per second */
+   schedule_timeout(HZ/60);
+   }
+
+   return 0;
+}
+
+static void vimc_cap_tpg_s_format(struct vimc_cap_device *vcap)
+{
+   const struct vimc_pix_map *vpix =
+   vimc_pix_map_by_pixelformat(vcap->format.pixelformat);
+
+   tpg_reset_source(>tpg, vcap->format.width, vcap->format.height,
+vcap->format.field);
+   tpg_s_bytesperline(>tpg, 0, vcap->format.width * vpix->bpp);
+   tpg_s_buf_height(>tpg, vcap->format.height);
+   tpg_s_fourcc(>tpg, vpix->pixelformat);
+   /*
+* TODO: check why the tpg_s_field need this third argument if
+* it is already receiving the field
+*/
+   tpg_s_field(>tpg, vcap->format.field,
+   vcap->format.field == V4L2_FIELD_ALTERNATE);
+   tpg_s_colorspace(>tpg, vcap->format.colorspace);
+   tpg_s_ycbcr_enc(>tpg, vcap->format.ycbcr_enc);
+   tpg_s_quantization(>tpg, vcap->format.quantization);
+   tpg_s_xfer_func(>tpg, vcap->format.xfer_func);
+}
+
 static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
@@ -256,20 +346,43 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, 
unsigned int count)
 
/* Start the media pipeline */
ret = media_pipeline_start(entity, >pipe);
-   if (ret) {
-   vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
-   return ret;
-   }
+   if (ret)
+   goto err_ret_all_buffs;
 
/* Enable streaming from the pipe */
ret = vimc_pipeline_s_stream(>vdev.entity, 1);
-