| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cc25b952-efd1-1cf8-b7f5-b2f4a4ab1571@collabora.com>
Date: Mon, 12 Jun 2017 16:24:06 -0300
From: Helen Koike <helen.koike@...labora.com>
To: Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-kernel@...r.kernel.org
Cc: jgebben@...eaurora.org, mchehab@....samsung.com,
Sakari Ailus <sakari.ailus@....fi>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation
through the pipe
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@...labora.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
Powered by blists - more mailing lists