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

Helen Koike
2017-06-06T14:11:26Z

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 <helen.koike@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);
> + 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;
>
>

Hi,

There is a bug in this patch as I didn't consider we could have more
then one link enabled for a single sink pad. Please, ignore this patch
in this series, I'll re-work on it and send it in a different patch
series latter.
I'll will re-send this series without this patch after your comments in
the other patches.

Thanks
LN Koike