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

From: Helen Koike <helen.koike@collabora.com>
To: Hans Verkuil <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-kernel@vger.kernel.org
Cc: jgebben@codeaurora.org, mchehab@osg.samsung.com,
	Sakari Ailus <sakari.ailus@iki.fi>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe
Date: Mon, 12 Jun 2017 16:24:06 -0300	[thread overview]
Message-ID: <cc25b952-efd1-1cf8-b7f5-b2f4a4ab1571@collabora.com> (raw)
In-Reply-To: <d5425795-a581-65f6-ea19-f9dd3e07867f@xs4all.nl>

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