Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751474AbdFFOMy (ORCPT ); Tue, 6 Jun 2017 10:12:54 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:37247 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751406AbdFFOL0 (ORCPT ); Tue, 6 Jun 2017 10:11:26 -0400 Subject: Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe To: linux-media@vger.kernel.org, Mauro Carvalho Chehab , linux-kernel@vger.kernel.org References: <1491604632-23544-1-git-send-email-helen.koike@collabora.com> <1496458714-16834-1-git-send-email-helen.koike@collabora.com> <1496458714-16834-9-git-send-email-helen.koike@collabora.com> Cc: Hans Verkuil , jgebben@codeaurora.org, mchehab@osg.samsung.com, Sakari Ailus , Laurent Pinchart From: Helen Koike Message-ID: <413f83bb-4b98-8623-f02e-058641402739@collabora.com> Date: Tue, 6 Jun 2017 11:11:16 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1496458714-16834-9-git-send-email-helen.koike@collabora.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10801 Lines: 354 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(&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 > #include > #include > #include > @@ -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