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 <helen.ko...@collabora.com>

---

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 <linux/freezer.h>
+#include <linux/kthread.h>
  #include <media/v4l2-ioctl.h>
+#include <media/v4l2-tpg.h>
  #include <media/videobuf2-core.h>
  #include <media/videobuf2-vmalloc.h>
  @@ -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(&vcap->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(&vcap->qlock);
+
+    /* Get the first entry of the list */
+    vimc_buf = list_first_entry_or_null(&vcap->buf_list,
+                        typeof(*vimc_buf), list);
+    if (!vimc_buf) {
+        spin_unlock(&vcap->qlock);
+        return;
+    }
+
+    /* Remove this entry from the list */
+    list_del(&vimc_buf->list);
+
+    spin_unlock(&vcap->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(&vimc_buf->vb2.vb2_buf, 0);
+
+    if (sink)
+        memcpy(vbuf, frame, vcap->format.sizeimage);
+    else
+        tpg_fill_plane_buffer(&vcap->tpg, V4L2_STD_PAL, 0, vbuf);
+
+    /* Set it as ready */
+    vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
+                  vcap->format.sizeimage);
+    vb2_buffer_done(&vimc_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(&vcap->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(&vcap->tpg, vcap->format.width,
vcap->format.height,
+             vcap->format.field);
+    tpg_s_bytesperline(&vcap->tpg, 0, vcap->format.width * vpix->bpp);
+    tpg_s_buf_height(&vcap->tpg, vcap->format.height);
+    tpg_s_fourcc(&vcap->tpg, vpix->pixelformat);
+    /*
+     * TODO: check why the tpg_s_field need this third argument if
+     * it is already receiving the field
+     */
+    tpg_s_field(&vcap->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(&vcap->tpg, vcap->format.colorspace);
+    tpg_s_ycbcr_enc(&vcap->tpg, vcap->format.ycbcr_enc);
+    tpg_s_quantization(&vcap->tpg, vcap->format.quantization);
+    tpg_s_xfer_func(&vcap->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, &vcap->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(&vcap->vdev.entity, 1);
-    if (ret) {
-        media_pipeline_stop(entity);
-        vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
-        return ret;
+    if (ret < 0)
+        goto err_mpipe_stop;
+
+    if (ret == VIMC_PIPE_OPT) {
+        tpg_init(&vcap->tpg, vcap->format.width, vcap->format.height);
+        ret = tpg_alloc(&vcap->tpg, VIMC_FRAME_MAX_WIDTH);
+        if (ret)
+            /* We don't need to call vimc_pipeline_s_stream(e, 0) */
+            goto err_mpipe_stop;
+
+        vimc_cap_tpg_s_format(vcap);
+        vcap->kthread_cap = kthread_run(vimc_cap_tpg_thread, vcap,
+                    "%s-cap", vcap->vdev.v4l2_dev->name);
+        if (IS_ERR(vcap->kthread_cap)) {
+            dev_err(vcap->vdev.v4l2_dev->dev,
+                "%s: kernel_thread() failed\n",
+                vcap->vdev.name);
+            ret = PTR_ERR(vcap->kthread_cap);
+            goto err_tpg_free;
+        }
      }
        return 0;
+
+err_tpg_free:
+    tpg_free(&vcap->tpg);
+err_mpipe_stop:
+    media_pipeline_stop(entity);
+err_ret_all_buffs:
+    vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
+
+    return ret;
  }
    /*
@@ -280,8 +393,15 @@ static void vimc_cap_stop_streaming(struct
vb2_queue *vq)
  {
      struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
  -    /* Disable streaming from the pipe */
-    vimc_pipeline_s_stream(&vcap->vdev.entity, 0);
+    if (vcap->kthread_cap) {
+        /* Stop image generator */
+        kthread_stop(vcap->kthread_cap);
+        vcap->kthread_cap = NULL;
+        tpg_free(&vcap->tpg);
+    } else {
+        /* Disable streaming from the pipe */
+        vimc_pipeline_s_stream(&vcap->vdev.entity, 0);
+    }
        /* Stop the media pipeline */
      media_pipeline_stop(&vcap->vdev.entity);
@@ -361,44 +481,6 @@ static void vimc_cap_destroy(struct
vimc_ent_device *ved)
      kfree(vcap);
  }
  -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(&vcap->qlock);
-
-    /* Get the first entry of the list */
-    vimc_buf = list_first_entry_or_null(&vcap->buf_list,
-                        typeof(*vimc_buf), list);
-    if (!vimc_buf) {
-        spin_unlock(&vcap->qlock);
-        return;
-    }
-
-    /* Remove this entry from the list */
-    list_del(&vimc_buf->list);
-
-    spin_unlock(&vcap->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(&vimc_buf->vb2.vb2_buf, 0);
-
-    memcpy(vbuf, frame, vcap->format.sizeimage);
-
-    /* Set it as ready */
-    vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
-                  vcap->format.sizeimage);
-    vb2_buffer_done(&vimc_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
-}
-
  struct vimc_ent_device *vimc_cap_create(struct v4l2_device *v4l2_dev,
                      const char *const name,
                      u16 num_pads,
diff --git a/drivers/media/platform/vimc/vimc-common.h
b/drivers/media/platform/vimc/vimc-common.h
index 2189fd6..5b2691e 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -27,6 +27,8 @@
  #define VIMC_FRAME_MIN_WIDTH 16
  #define VIMC_FRAME_MIN_HEIGHT 16
  +#define VIMC_PIPE_OPT 1
+
  /**
   * struct vimc_pix_map - maps media bus code with v4l2 pixel format
   *
diff --git a/drivers/media/platform/vimc/vimc-sensor.c
b/drivers/media/platform/vimc/vimc-sensor.c
index 90c41c6..df89ee7 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -15,6 +15,7 @@
   *
   */
  +#include <linux/module.h>
  #include <linux/freezer.h>
  #include <linux/kthread.h>
  #include <linux/v4l2-mediabus.h>
@@ -24,6 +25,13 @@
    #include "vimc-sensor.h"
  +static bool vsen_tpg;
+module_param(vsen_tpg, bool, 0000);
+MODULE_PARM_DESC(vsen_tpg, " generate image from sensor node\n"
+    "If set to false, then the pipe will be optimized and image will
be "
+    "generated directly in the capture node instead of going through "
+    "the whole pipe");
+
  struct vimc_sen_device {
      struct vimc_ent_device ved;
      struct v4l2_subdev sd;
@@ -239,6 +247,13 @@ static int vimc_sen_s_stream(struct v4l2_subdev
*sd, int enable)
                  container_of(sd, struct vimc_sen_device, sd);
      int ret;
  +    if (!vsen_tpg)
+        /*
+         * If we are not generating the frames, then inform the caller
+         * to generate the frame in shallow level of the pipe
+         */
+        return VIMC_PIPE_OPT;
+
      if (enable) {
          const struct vimc_pix_map *vpix;
          unsigned int frame_size;
@@ -306,7 +321,8 @@ static void vimc_sen_destroy(struct
vimc_ent_device *ved)
                  container_of(ved, struct vimc_sen_device, ved);
        vimc_ent_sd_unregister(ved, &vsen->sd);
-    tpg_free(&vsen->tpg);
+    if (vsen_tpg)
+        tpg_free(&vsen->tpg);
      kfree(vsen);
  }
  @@ -344,11 +360,13 @@ struct vimc_ent_device *vimc_sen_create(struct
v4l2_device *v4l2_dev,
      vsen->mbus_format = fmt_default;
        /* Initialize the test pattern generator */
-    tpg_init(&vsen->tpg, vsen->mbus_format.width,
-         vsen->mbus_format.height);
-    ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
-    if (ret)
-        goto err_unregister_ent_sd;
+    if (vsen_tpg) {
+        tpg_init(&vsen->tpg, vsen->mbus_format.width,
+             vsen->mbus_format.height);
+        ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
+        if (ret)
+            goto err_unregister_ent_sd;
+    }
        return &vsen->ved;


Regards,

    Hans

Thanks,

Helen

Reply via email to