Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752598AbdFLTYV (ORCPT ); Mon, 12 Jun 2017 15:24:21 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:60500 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752076AbdFLTYT (ORCPT ); Mon, 12 Jun 2017 15:24:19 -0400 Subject: Re: [RFC PATCH v3 08/11] [media] vimc: Optimize frame generation through the pipe To: Hans Verkuil , 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: jgebben@codeaurora.org, mchehab@osg.samsung.com, Sakari Ailus , Laurent Pinchart From: Helen Koike Message-ID: Date: Mon, 12 Jun 2017 16:24:06 -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: Content-Type: text/plain; charset=utf-8; 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: 12580 Lines: 365 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 >> >> --- >> >> 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); > > 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 >> #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; >> > > Regards, > > Hans Thanks, Helen