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

[Helen Koike <helen.koike@collabora.com>]

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