[Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Lucas A. M. Magalhães]

The previous code pipeline used the stack to walk on the graph and
process a frame. Basically the vimc-sensor entity starts a thread that
generates the frames and calls the propagate_process function to send
this frame to each entity linked with a sink pad. The propagate_process
will call the process_frame of the entities which will call the
propagate_frame for each one of it's sink pad. This cycle will continue
until it reaches a vimc-capture entity that will finally return and
unstack.

This solution had many problems:
  * It was a little bit slow
  * It was susceptible to a stack overflow as it made indiscriminate
    use of the stack.
  * It doesn't allow frame rate control
  * It was complex to understand
  * It doesn't allow pipeline control

This commit proposes an alternative way to control vimc streams by
having a streamer object. This object will create a linear pipeline
walking backwards on the graph. When the stream starts it will simply
loop through the pipeline calling the respective process_frame function
for each entity on the pipeline.

This solution has some premises which are true for now:
  * Two paths can never be enabled and streaming at the same time.
  * There is no entity streaming frames to two source pads at the same
    time.
  * There is no entity receiving frames from two sink pads at the same
    time.

Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>
---
Hi,

This patch introduces a streamer controller library for the vimc
driver. It's a step towards a optimized mode I've been discussing with
Helen.
I plan to pass a tpg struct through the pipeline. This tpg struct
will be configured in each entity and the capture will generate the
frames with the correct format at the end of the pipeline.

Thanks,
Lucas

 drivers/media/platform/vimc/Makefile        |   3 +-
 drivers/media/platform/vimc/vimc-capture.c  |  18 +-
 drivers/media/platform/vimc/vimc-common.c   |  50 ++----
 drivers/media/platform/vimc/vimc-common.h   |  15 +-
 drivers/media/platform/vimc/vimc-debayer.c  |  26 +--
 drivers/media/platform/vimc/vimc-scaler.c   |  28 +---
 drivers/media/platform/vimc/vimc-sensor.c   |  56 ++-----
 drivers/media/platform/vimc/vimc-streamer.c | 176 ++++++++++++++++++++
 drivers/media/platform/vimc/vimc-streamer.h |  38 +++++
 9 files changed, 268 insertions(+), 142 deletions(-)
 create mode 100644 drivers/media/platform/vimc/vimc-streamer.c
 create mode 100644 drivers/media/platform/vimc/vimc-streamer.h

diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
index 4b2e3de7856e..c4fc8e7d365a 100644
--- a/drivers/media/platform/vimc/Makefile
+++ b/drivers/media/platform/vimc/Makefile
@@ -5,6 +5,7 @@ vimc_common-objs := vimc-common.o
 vimc_debayer-objs := vimc-debayer.o
 vimc_scaler-objs := vimc-scaler.o
 vimc_sensor-objs := vimc-sensor.o
+vimc_streamer-objs := vimc-streamer.o
 
 obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc_capture.o vimc_common.o vimc-debayer.o \
-				vimc_scaler.o vimc_sensor.o
+			    vimc_scaler.o vimc_sensor.o vimc_streamer.o
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 3f7e9ed56633..80d7515ec420 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -24,6 +24,7 @@
 #include <media/videobuf2-vmalloc.h>
 
 #include "vimc-common.h"
+#include "vimc-streamer.h"
 
 #define VIMC_CAP_DRV_NAME "vimc-capture"
 
@@ -44,7 +45,7 @@ struct vimc_cap_device {
 	spinlock_t qlock;
 	struct mutex lock;
 	u32 sequence;
-	struct media_pipeline pipe;
+	struct vimc_stream stream;
 };
 
 static const struct v4l2_pix_format fmt_default = {
@@ -248,14 +249,13 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 	vcap->sequence = 0;
 
 	/* Start the media pipeline */
-	ret = media_pipeline_start(entity, &vcap->pipe);
+	ret = media_pipeline_start(entity, &vcap->stream.pipe);
 	if (ret) {
 		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
 		return ret;
 	}
 
-	/* Enable streaming from the pipe */
-	ret = vimc_pipeline_s_stream(&vcap->vdev.entity, 1);
+	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
 	if (ret) {
 		media_pipeline_stop(entity);
 		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
@@ -273,8 +273,7 @@ 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);
+	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
 
 	/* Stop the media pipeline */
 	media_pipeline_stop(&vcap->vdev.entity);
@@ -355,8 +354,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 	kfree(vcap);
 }
 
-static void vimc_cap_process_frame(struct vimc_ent_device *ved,
-				   struct media_pad *sink, const void *frame)
+static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
+				    const void *frame)
 {
 	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
 						    ved);
@@ -370,7 +369,7 @@ static void vimc_cap_process_frame(struct vimc_ent_device *ved,
 					    typeof(*vimc_buf), list);
 	if (!vimc_buf) {
 		spin_unlock(&vcap->qlock);
-		return;
+		return ERR_PTR(-EAGAIN);
 	}
 
 	/* Remove this entry from the list */
@@ -391,6 +390,7 @@ static void vimc_cap_process_frame(struct vimc_ent_device *ved,
 	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);
+	return NULL;
 }
 
 static int vimc_cap_comp_bind(struct device *comp, struct device *master,
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index dee1b9dfc4f6..8eb27ac589e2 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -207,41 +207,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat)
 }
 EXPORT_SYMBOL_GPL(vimc_pix_map_by_pixelformat);
 
-int vimc_propagate_frame(struct media_pad *src, const void *frame)
-{
-	struct media_link *link;
-
-	if (!(src->flags & MEDIA_PAD_FL_SOURCE))
-		return -EINVAL;
-
-	/* Send this frame to all sink pads that are direct linked */
-	list_for_each_entry(link, &src->entity->links, list) {
-		if (link->source == src &&
-		    (link->flags & MEDIA_LNK_FL_ENABLED)) {
-			struct vimc_ent_device *ved = NULL;
-			struct media_entity *entity = link->sink->entity;
-
-			if (is_media_entity_v4l2_subdev(entity)) {
-				struct v4l2_subdev *sd =
-					container_of(entity, struct v4l2_subdev,
-						     entity);
-				ved = v4l2_get_subdevdata(sd);
-			} else if (is_media_entity_v4l2_video_device(entity)) {
-				struct video_device *vdev =
-					container_of(entity,
-						     struct video_device,
-						     entity);
-				ved = video_get_drvdata(vdev);
-			}
-			if (ved && ved->process_frame)
-				ved->process_frame(ved, link->sink, frame);
-		}
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(vimc_propagate_frame);
-
 /* Helper function to allocate and initialize pads */
 struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
 {
@@ -263,6 +228,21 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
 }
 EXPORT_SYMBOL_GPL(vimc_pads_init);
 
+struct media_entity *vimc_get_source_entity(struct media_entity *ent)
+{
+	struct media_pad *pad;
+	int i;
+
+	for (i = 0; i < ent->num_pads; i++) {
+		if (ent->pads[i].flags & MEDIA_PAD_FL_SOURCE)
+			continue;
+		pad = media_entity_remote_pad(&ent->pads[i]);
+		return pad ? pad->entity : NULL;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(vimc_get_source_entity);
+
 int vimc_pipeline_s_stream(struct media_entity *ent, int enable)
 {
 	struct v4l2_subdev *sd;
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 2e9981b18166..1cbafd31ecb0 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -113,22 +113,21 @@ struct vimc_pix_map {
 struct vimc_ent_device {
 	struct media_entity *ent;
 	struct media_pad *pads;
-	void (*process_frame)(struct vimc_ent_device *ved,
-			      struct media_pad *sink, const void *frame);
+	void * (*process_frame)(struct vimc_ent_device *ved,
+				const void *frame);
 	void (*vdev_get_format)(struct vimc_ent_device *ved,
 			      struct v4l2_pix_format *fmt);
 };
 
 /**
- * vimc_propagate_frame - propagate a frame through the topology
+ * vimc_get_source_entity - get the media_entity source of a given media_entity
  *
- * @src:	the source pad where the frame is being originated
- * @frame:	the frame to be propagated
+ * @ent:	reference media_entity
  *
- * This function will call the process_frame callback from the vimc_ent_device
- * struct of the nodes directly connected to the @src pad
+ * Helper function that return the media entity which is source from the given
+ * media entity.
  */
-int vimc_propagate_frame(struct media_pad *src, const void *frame);
+struct media_entity *vimc_get_source_entity(struct media_entity *ent);
 
 /**
  * vimc_pads_init - initialize pads
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 77887f66f323..7d77c63b99d2 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -321,7 +321,6 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb,
 static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd);
-	int ret;
 
 	if (enable) {
 		const struct vimc_pix_map *vpix;
@@ -351,22 +350,10 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 		if (!vdeb->src_frame)
 			return -ENOMEM;
 
-		/* Turn the stream on in the subdevices directly connected */
-		ret = vimc_pipeline_s_stream(&vdeb->sd.entity, 1);
-		if (ret) {
-			vfree(vdeb->src_frame);
-			vdeb->src_frame = NULL;
-			return ret;
-		}
 	} else {
 		if (!vdeb->src_frame)
 			return 0;
 
-		/* Disable streaming from the pipe */
-		ret = vimc_pipeline_s_stream(&vdeb->sd.entity, 0);
-		if (ret)
-			return ret;
-
 		vfree(vdeb->src_frame);
 		vdeb->src_frame = NULL;
 	}
@@ -480,9 +467,8 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
 	}
 }
 
-static void vimc_deb_process_frame(struct vimc_ent_device *ved,
-				   struct media_pad *sink,
-				   const void *sink_frame)
+static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
+				    const void *sink_frame)
 {
 	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
 						    ved);
@@ -491,7 +477,7 @@ static void vimc_deb_process_frame(struct vimc_ent_device *ved,
 
 	/* If the stream in this node is not active, just return */
 	if (!vdeb->src_frame)
-		return;
+		return ERR_PTR(-EINVAL);
 
 	for (i = 0; i < vdeb->sink_fmt.height; i++)
 		for (j = 0; j < vdeb->sink_fmt.width; j++) {
@@ -499,12 +485,8 @@ static void vimc_deb_process_frame(struct vimc_ent_device *ved,
 			vdeb->set_rgb_src(vdeb, i, j, rgb);
 		}
 
-	/* Propagate the frame through all source pads */
-	for (i = 1; i < vdeb->sd.entity.num_pads; i++) {
-		struct media_pad *pad = &vdeb->sd.entity.pads[i];
+	return vdeb->src_frame;
 
-		vimc_propagate_frame(pad, vdeb->src_frame);
-	}
 }
 
 static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index b0952ee86296..39b2a73dfcc1 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -217,7 +217,6 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
 static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
-	int ret;
 
 	if (enable) {
 		const struct vimc_pix_map *vpix;
@@ -245,22 +244,10 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 		if (!vsca->src_frame)
 			return -ENOMEM;
 
-		/* Turn the stream on in the subdevices directly connected */
-		ret = vimc_pipeline_s_stream(&vsca->sd.entity, 1);
-		if (ret) {
-			vfree(vsca->src_frame);
-			vsca->src_frame = NULL;
-			return ret;
-		}
 	} else {
 		if (!vsca->src_frame)
 			return 0;
 
-		/* Disable streaming from the pipe */
-		ret = vimc_pipeline_s_stream(&vsca->sd.entity, 0);
-		if (ret)
-			return ret;
-
 		vfree(vsca->src_frame);
 		vsca->src_frame = NULL;
 	}
@@ -346,26 +333,19 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
 			vimc_sca_scale_pix(vsca, i, j, sink_frame);
 }
 
-static void vimc_sca_process_frame(struct vimc_ent_device *ved,
-				   struct media_pad *sink,
-				   const void *sink_frame)
+static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
+				    const void *sink_frame)
 {
 	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
 						    ved);
-	unsigned int i;
 
 	/* If the stream in this node is not active, just return */
 	if (!vsca->src_frame)
-		return;
+		return ERR_PTR(-EINVAL);
 
 	vimc_sca_fill_src_frame(vsca, sink_frame);
 
-	/* Propagate the frame through all source pads */
-	for (i = 1; i < vsca->sd.entity.num_pads; i++) {
-		struct media_pad *pad = &vsca->sd.entity.pads[i];
-
-		vimc_propagate_frame(pad, vsca->src_frame);
-	}
+	return vsca->src_frame;
 };
 
 static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 32ca9c6172b1..93961a1e694f 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -16,8 +16,6 @@
  */
 
 #include <linux/component.h>
-#include <linux/freezer.h>
-#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
@@ -201,38 +199,27 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
 	.set_fmt		= vimc_sen_set_fmt,
 };
 
-static int vimc_sen_tpg_thread(void *data)
+static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
+				    const void *sink_frame)
 {
-	struct vimc_sen_device *vsen = data;
-	unsigned int i;
-
-	set_freezable();
-	set_current_state(TASK_UNINTERRUPTIBLE);
-
-	for (;;) {
-		try_to_freeze();
-		if (kthread_should_stop())
-			break;
-
-		tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
+	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
+						    ved);
+	const struct vimc_pix_map *vpix;
+	unsigned int frame_size;
 
-		/* Send the frame to all source pads */
-		for (i = 0; i < vsen->sd.entity.num_pads; i++)
-			vimc_propagate_frame(&vsen->sd.entity.pads[i],
-					     vsen->frame);
+	/* Calculate the frame size */
+	vpix = vimc_pix_map_by_code(vsen->mbus_format.code);
+	frame_size = vsen->mbus_format.width * vpix->bpp *
+		     vsen->mbus_format.height;
 
-		/* 60 frames per second */
-		schedule_timeout(HZ/60);
-	}
-
-	return 0;
+	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
+	return vsen->frame;
 }
 
 static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct vimc_sen_device *vsen =
 				container_of(sd, struct vimc_sen_device, sd);
-	int ret;
 
 	if (enable) {
 		const struct vimc_pix_map *vpix;
@@ -258,26 +245,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		/* configure the test pattern generator */
 		vimc_sen_tpg_s_format(vsen);
 
-		/* Initialize the image generator thread */
-		vsen->kthread_sen = kthread_run(vimc_sen_tpg_thread, vsen,
-					"%s-sen", vsen->sd.v4l2_dev->name);
-		if (IS_ERR(vsen->kthread_sen)) {
-			dev_err(vsen->dev, "%s: kernel_thread() failed\n",
-				vsen->sd.name);
-			vfree(vsen->frame);
-			vsen->frame = NULL;
-			return PTR_ERR(vsen->kthread_sen);
-		}
 	} else {
-		if (!vsen->kthread_sen)
-			return 0;
-
-		/* Stop image generator */
-		ret = kthread_stop(vsen->kthread_sen);
-		if (ret)
-			return ret;
 
-		vsen->kthread_sen = NULL;
 		vfree(vsen->frame);
 		vsen->frame = NULL;
 		return 0;
@@ -413,6 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 	if (ret)
 		goto err_free_hdl;
 
+	vsen->ved.process_frame = vimc_sen_process_frame;
 	dev_set_drvdata(comp, &vsen->ved);
 	vsen->dev = comp;
 
diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
new file mode 100644
index 000000000000..f614734538c2
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * vimc-streamer.c Virtual Media Controller Driver
+ *
+ * Copyright (C) 2018 Lucas A. M. Magalhães <lucmaga@gmail.com>
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
+
+#include "vimc-streamer.h"
+
+/*
+ * vimc_streamer_pipeline_disable - Disable stream in all ved in stream
+ *
+ * @stream: the pointer to the stream structure with the pipeline to be
+ *	    disabled.
+ *
+ * Calls s_stream to disable the stream in each entity of the pipeline
+ *
+ */
+static void vimc_streamer_pipeline_disable(struct vimc_stream *stream)
+{
+	struct media_entity *entity;
+	struct v4l2_subdev *sd;
+
+	do {
+		stream->pipe_size--;
+		entity = stream->ved_pipeline[stream->pipe_size]->ent;
+		entity = vimc_get_source_entity(entity);
+		stream->ved_pipeline[stream->pipe_size] = NULL;
+		/*
+		 *  This may occur only if the streamer was not correctly
+		 *  initialized.
+		 */
+		if (!entity)
+			continue;
+
+		if (!is_media_entity_v4l2_subdev(entity))
+			continue;
+
+		sd = media_entity_to_v4l2_subdev(entity);
+		v4l2_subdev_call(sd, video, s_stream, 0);
+	} while (stream->pipe_size);
+}
+
+/*
+ * vimc_streamer_pipeline_init - initializes the stream structure
+ *
+ * @stream: the pointer to the stream structure to be initialized
+ * @ved:    the pointer to the vimc entity initializing the stream
+ *
+ * Initializes the stream structure. Walks through the entity graph to
+ * construct the pipeline used later on the streamer thread.
+ * Calls s_stream to enable stream in all entities of the pipeline.
+ */
+static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
+				struct vimc_ent_device *ved)
+{
+	struct vimc_ent_device *source_ved = NULL;
+	struct media_entity *entity;
+	struct video_device *vdev;
+	struct v4l2_subdev *sd;
+	int ret = -EINVAL;
+
+	stream->pipe_size = 0;
+	stream->ved_pipeline[stream->pipe_size++] = ved;
+
+	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
+		if (!stream->ved_pipeline[stream->pipe_size-1])
+			return 0;
+		entity = stream->ved_pipeline[stream->pipe_size-1]->ent;
+		entity = vimc_get_source_entity(entity);
+		if (!entity)
+			return 0;
+		if (is_media_entity_v4l2_subdev(entity)) {
+			sd = media_entity_to_v4l2_subdev(entity);
+			ret = v4l2_subdev_call(sd, video, s_stream, 1);
+			if (ret && ret != -ENOIOCTLCMD)
+				break;
+			source_ved = v4l2_get_subdevdata(sd);
+		} else {
+			vdev = container_of(entity,
+					    struct video_device,
+					    entity);
+			source_ved = video_get_drvdata(vdev);
+		}
+
+		stream->ved_pipeline[stream->pipe_size++] = source_ved;
+		source_ved = NULL;
+	}
+
+	/*
+	 * If an error occur during initialization or the pipeline gets longer
+	 * than VIMC_STREAMER_PIPELINE_MAX_SIZE the stream is disabled and
+	 * return the error code.
+	 */
+	vimc_streamer_pipeline_disable(stream);
+	return ret;
+}
+
+static int vimc_streamer_thread(void *data)
+{
+	struct vimc_stream *stream = data;
+	int i;
+
+	set_freezable();
+	set_current_state(TASK_UNINTERRUPTIBLE);
+
+	for (;;) {
+		try_to_freeze();
+		if (kthread_should_stop())
+			break;
+
+		for (i = stream->pipe_size - 1; i >= 0; i--) {
+			stream->frame = stream->ved_pipeline[i]->process_frame(
+					stream->ved_pipeline[i],
+					stream->frame);
+			if (!stream->frame)
+				break;
+			if (IS_ERR(stream->frame))
+				break;
+		}
+		//wait for 60hz
+		schedule_timeout(HZ / 60);
+	}
+
+	return 0;
+}
+
+int vimc_streamer_s_stream(struct vimc_stream *stream,
+			   struct vimc_ent_device *ved,
+			   int enable)
+{
+	int ret;
+
+	if (!stream || !ved)
+		return -EINVAL;
+
+	if (enable) {
+		if (stream->kthread)
+			return 0;
+
+		ret = vimc_streamer_pipeline_init(stream, ved);
+		if (ret)
+			return ret;
+
+		stream->kthread = kthread_run(vimc_streamer_thread, stream,
+					      "vimc-streamer thread");
+
+		if (IS_ERR(stream->kthread))
+			return PTR_ERR(stream->kthread);
+
+	} else {
+		if (!stream->kthread)
+			return 0;
+
+		ret = kthread_stop(stream->kthread);
+		if (ret)
+			return ret;
+
+		stream->kthread = NULL;
+
+		vimc_streamer_pipeline_disable(stream);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vimc_streamer_s_stream);
+
+MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Streamer");
+MODULE_AUTHOR("Lucas A. M. Magalhães <lucmaga@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/vimc/vimc-streamer.h b/drivers/media/platform/vimc/vimc-streamer.h
new file mode 100644
index 000000000000..752af2e2d5a2
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-streamer.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * vimc-streamer.h Virtual Media Controller Driver
+ *
+ * Copyright (C) 2018 Lucas A. M. Magalhães <lucmaga@gmail.com>
+ *
+ */
+
+#ifndef _VIMC_STREAMER_H_
+#define _VIMC_STREAMER_H_
+
+#include <media/media-device.h>
+
+#include "vimc-common.h"
+
+#define VIMC_STREAMER_PIPELINE_MAX_SIZE 16
+
+struct vimc_stream {
+	struct media_pipeline pipe;
+	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
+	unsigned int pipe_size;
+	u8 *frame;
+	struct task_struct *kthread;
+};
+
+/**
+ * vimc_streamer_s_streamer - start/stop the stream
+ *
+ * @stream:	the pointer to the stream to start or stop
+ * @ved:	The last entity of the streamer pipeline
+ * @enable:	any non-zero number start the stream, zero stop
+ *
+ */
+int vimc_streamer_s_stream(struct vimc_stream *stream,
+			   struct vimc_ent_device *ved,
+			   int enable);
+
+#endif  //_VIMC_STREAMER_H_
-- 
2.20.0.rc1

Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Mauro Carvalho Chehab]

Hi Lucas,


Em Sat, 15 Dec 2018 14:46:31 -0200
Lucas A. M. Magalhães <lucmaga@gmail.com> escreveu:

> The previous code pipeline used the stack to walk on the graph and
> process a frame. Basically the vimc-sensor entity starts a thread that
> generates the frames and calls the propagate_process function to send
> this frame to each entity linked with a sink pad. The propagate_process
> will call the process_frame of the entities which will call the
> propagate_frame for each one of it's sink pad. This cycle will continue
> until it reaches a vimc-capture entity that will finally return and
> unstack.

I didn't review the code yet, but I have a few comments about the
way you're describing this patch.

When you mention about a "previous code pipeline". Well, by adding it
at the main body of the patch description, reviewers should expect
that you're mentioning an implementation that already reached upstream.

I suspect that this is not the case here, as I don't think we merged
any recursive algorithm using the stack, as this is something that
we shouldn't do at Kernelspace, as a 4K stack is usually not OK
with recursive algorithms.

So, it seems that this entire patch description (as-is) is bogus[1].

[1] if this is not the case and a recursive approach was indeed
sneaked into the Kernel, this is a bug. So, you should really
use the "Fixes:" meta-tag indicating what changeset this patch is
fixing, and a "Cc: stable@vger.kernel.org", in order to hint
stable maintainers that this require backports.

Please notice that the patch description will be stored forever
at the git tree. Mentioning something that were never merged
(and that, years from now people will hardly remember, and will
have lots of trouble to seek as you didn't even mentioned any
ML archive with the past solution) shouldn't be done.

So, you should rewrite the entire patch description explaining
what the current approach took by this patch does. Then, in order
to make easier for reviewers to compare with a previous implementation,
you can add a "---" line and then a description about why this approach
is better than the first version, e. g. something like:

	[PATCH v2] media: vimc: Add vimc-streamer for stream control

	Add a logic that will create a linear pipeline walking 
	backwards on the graph. When the stream starts it will simply
	loop through the pipeline calling the respective process_frame
	function for each entity on the pipeline.

	Signed-off-by: Your Name <your@email>

	---

	v2: The previous approach were to use a recursive function that
	it was using the stack to walk on the graph and
	process a frame. Basically the vimc-sensor entity starts a thread that
	generates the frames and calls the propagate_process function to send
	this frame to each entity linked with a sink pad. The propagate_process
	will call the process_frame of the entities which will call the
	propagate_frame for each one of it's sink pad. This cycle will continue
	until it reaches a vimc-capture entity that will finally return and
	unstack.
	...

If the past approach was written by somebody else (or if you sent it
a long time ago), please add an URL (if possible using 
https://lore.kernel.org/linux-media/ archive) pointing to the previous 
approach, in order to help us to check what you're referring to.

Regards,
Mauro

Thanks,
Mauro

Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Helen Koike]

Hi Mauro,

On 12/15/18 4:01 PM, Mauro Carvalho Chehab wrote:
> Hi Lucas,
> 
> 
> Em Sat, 15 Dec 2018 14:46:31 -0200
> Lucas A. M. Magalhães <lucmaga@gmail.com> escreveu:
> 
>> The previous code pipeline used the stack to walk on the graph and
>> process a frame. Basically the vimc-sensor entity starts a thread that
>> generates the frames and calls the propagate_process function to send
>> this frame to each entity linked with a sink pad. The propagate_process
>> will call the process_frame of the entities which will call the
>> propagate_frame for each one of it's sink pad. This cycle will continue
>> until it reaches a vimc-capture entity that will finally return and
>> unstack.
> 
> I didn't review the code yet, but I have a few comments about the
> way you're describing this patch.
> 
> When you mention about a "previous code pipeline". Well, by adding it
> at the main body of the patch description, reviewers should expect
> that you're mentioning an implementation that already reached upstream.
> 
> I suspect that this is not the case here, as I don't think we merged
> any recursive algorithm using the stack, as this is something that
> we shouldn't do at Kernelspace, as a 4K stack is usually not OK
> with recursive algorithms.
> 
> So, it seems that this entire patch description (as-is) is bogus[1].
> 
> [1] if this is not the case and a recursive approach was indeed
> sneaked into the Kernel, this is a bug. So, you should really
> use the "Fixes:" meta-tag indicating what changeset this patch is
> fixing, and a "Cc: stable@vger.kernel.org", in order to hint
> stable maintainers that this require backports.

Just fyi, this is not the case, the current implementation in mainline
is bogus indeed (implemented by me when I was starting kernel
development, sorry about that and thanks Lucas for sending a fix). Not
only when propagating the frame [1] but also when activating the
pipeline [2].

But in any case this should be better written in the commit message.


[1]
Every entity calls vimc_propagate_frame()
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n506
That calls the process_frame() of each entity directly connected, that
calls vimc_propagate_frame() again:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n237

[2]
.s_stream is calling the .s_stream of the subdevices directly connected
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n355


I was actually wondering if this is worthy in sending this to stable, as
this implementation is not a real problem, because the topology in vimc
is hardcoded and limited, and according to:
https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst
"It must fix a real bug that bothers people"

So as the topology is fixed (in the current implementation), the max
number of nested calls is 4 (in the sensor->debayer->scaler->capture
path), this doesn't triggers any bug to users. But this will be a
problem once we have the configfs API in vimc.

You could say that if your memory is low, this can be a problem in the
current implementation, but then your system won't have memory for any 4
nested function calls anyway (which I think the kernel wouldn't work at
all).

Mauro, with that said, do you still think we should send this to stable?

Thanks
Helen

> 
> Please notice that the patch description will be stored forever
> at the git tree. Mentioning something that were never merged
> (and that, years from now people will hardly remember, and will
> have lots of trouble to seek as you didn't even mentioned any
> ML archive with the past solution) shouldn't be done.
> 
> So, you should rewrite the entire patch description explaining
> what the current approach took by this patch does. Then, in order
> to make easier for reviewers to compare with a previous implementation,
> you can add a "---" line and then a description about why this approach
> is better than the first version, e. g. something like:
> 
> 	[PATCH v2] media: vimc: Add vimc-streamer for stream control
> 
> 	Add a logic that will create a linear pipeline walking 
> 	backwards on the graph. When the stream starts it will simply
> 	loop through the pipeline calling the respective process_frame
> 	function for each entity on the pipeline.
> 
> 	Signed-off-by: Your Name <your@email>
> 
> 	---
> 
> 	v2: The previous approach were to use a recursive function that
> 	it was using the stack to walk on the graph and
> 	process a frame. Basically the vimc-sensor entity starts a thread that
> 	generates the frames and calls the propagate_process function to send
> 	this frame to each entity linked with a sink pad. The propagate_process
> 	will call the process_frame of the entities which will call the
> 	propagate_frame for each one of it's sink pad. This cycle will continue
> 	until it reaches a vimc-capture entity that will finally return and
> 	unstack.
> 	...
> 
> If the past approach was written by somebody else (or if you sent it
> a long time ago), please add an URL (if possible using 
> https://lore.kernel.org/linux-media/ archive) pointing to the previous 
> approach, in order to help us to check what you're referring to.
> 
> Regards,
> Mauro
> 
> Thanks,
> Mauro
> 

Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Mauro Carvalho Chehab]

Em Sat, 15 Dec 2018 16:38:41 -0200
Helen Koike <helen.koike@collabora.com> escreveu:

> Hi Mauro,
> 
> On 12/15/18 4:01 PM, Mauro Carvalho Chehab wrote:
> > Hi Lucas,
> > 
> > 
> > Em Sat, 15 Dec 2018 14:46:31 -0200
> > Lucas A. M. Magalhães <lucmaga@gmail.com> escreveu:
> >   
> >> The previous code pipeline used the stack to walk on the graph and
> >> process a frame. Basically the vimc-sensor entity starts a thread that
> >> generates the frames and calls the propagate_process function to send
> >> this frame to each entity linked with a sink pad. The propagate_process
> >> will call the process_frame of the entities which will call the
> >> propagate_frame for each one of it's sink pad. This cycle will continue
> >> until it reaches a vimc-capture entity that will finally return and
> >> unstack.  
> > 
> > I didn't review the code yet, but I have a few comments about the
> > way you're describing this patch.
> > 
> > When you mention about a "previous code pipeline". Well, by adding it
> > at the main body of the patch description, reviewers should expect
> > that you're mentioning an implementation that already reached upstream.
> > 
> > I suspect that this is not the case here, as I don't think we merged
> > any recursive algorithm using the stack, as this is something that
> > we shouldn't do at Kernelspace, as a 4K stack is usually not OK
> > with recursive algorithms.
> > 
> > So, it seems that this entire patch description (as-is) is bogus[1].
> > 
> > [1] if this is not the case and a recursive approach was indeed
> > sneaked into the Kernel, this is a bug. So, you should really
> > use the "Fixes:" meta-tag indicating what changeset this patch is
> > fixing, and a "Cc: stable@vger.kernel.org", in order to hint
> > stable maintainers that this require backports.  
> 
> Just fyi, this is not the case, the current implementation in mainline
> is bogus indeed (implemented by me when I was starting kernel
> development, sorry about that and thanks Lucas for sending a fix). Not
> only when propagating the frame [1] but also when activating the
> pipeline [2].
> 
> But in any case this should be better written in the commit message.
> 
> 
> [1]
> Every entity calls vimc_propagate_frame()
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n506
> That calls the process_frame() of each entity directly connected, that
> calls vimc_propagate_frame() again:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n237
> 
> [2]
> .s_stream is calling the .s_stream of the subdevices directly connected
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n355
> 
> 
> I was actually wondering if this is worthy in sending this to stable, as
> this implementation is not a real problem, because the topology in vimc
> is hardcoded and limited, and according to:
> https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst
> "It must fix a real bug that bothers people"
> 
> So as the topology is fixed (in the current implementation), the max
> number of nested calls is 4 (in the sensor->debayer->scaler->capture
> path), this doesn't triggers any bug to users. But this will be a
> problem once we have the configfs API in vimc.
> 
> You could say that if your memory is low, this can be a problem in the
> current implementation, but then your system won't have memory for any 4
> nested function calls anyway (which I think the kernel wouldn't work at
> all).

That basically depends on how much memory each call eats at stack.

It is not always trivial about the amount of memory a stack call uses,
and enabling KASAN can affect it a lot, as it changes some gcc optimization
parameters, and there are some of those that use stack to either
speedup the code or to allow checking for memory overrun. I'd say that it
would be safe if, at the worse case scenario, it would be allocating no
more than ~500 bytes at stack. Above that, it could be problematic.

On a quick glance, I would be expecting, that, for 4 nested calls, it
would use up to 256 bytes on 64 bits archs. For each call, it would use: 

	1 64-bits pointer for function return pointer;
	2 64-bits pointers for the two function arguments;
	5 64-bits pointers for the temporary values.

All assuming that gcc won't be using registers for the above nor do any
other optimization. But you have to check the asm code to be sure.

Also, as I said, different Kconfig options could change the amount of
memory spent (optimization for size is enabled? KASAN is enabled? what
type of KASAN?). 

> 
> Mauro, with that said, do you still think we should send this to stable?

Yes, let's properly document it (please check the abount of stack it
is using with and without KASAN, for the worse case scenario) and
send it to stable. 

Better safe than sorry.

> 
> Thanks
> Helen
> 
> > 
> > Please notice that the patch description will be stored forever
> > at the git tree. Mentioning something that were never merged
> > (and that, years from now people will hardly remember, and will
> > have lots of trouble to seek as you didn't even mentioned any
> > ML archive with the past solution) shouldn't be done.
> > 
> > So, you should rewrite the entire patch description explaining
> > what the current approach took by this patch does. Then, in order
> > to make easier for reviewers to compare with a previous implementation,
> > you can add a "---" line and then a description about why this approach
> > is better than the first version, e. g. something like:
> > 
> > 	[PATCH v2] media: vimc: Add vimc-streamer for stream control
> > 
> > 	Add a logic that will create a linear pipeline walking 
> > 	backwards on the graph. When the stream starts it will simply
> > 	loop through the pipeline calling the respective process_frame
> > 	function for each entity on the pipeline.
> > 
> > 	Signed-off-by: Your Name <your@email>
> > 
> > 	---
> > 
> > 	v2: The previous approach were to use a recursive function that
> > 	it was using the stack to walk on the graph and
> > 	process a frame. Basically the vimc-sensor entity starts a thread that
> > 	generates the frames and calls the propagate_process function to send
> > 	this frame to each entity linked with a sink pad. The propagate_process
> > 	will call the process_frame of the entities which will call the
> > 	propagate_frame for each one of it's sink pad. This cycle will continue
> > 	until it reaches a vimc-capture entity that will finally return and
> > 	unstack.
> > 	...
> > 
> > If the past approach was written by somebody else (or if you sent it
> > a long time ago), please add an URL (if possible using 
> > https://lore.kernel.org/linux-media/ archive) pointing to the previous 
> > approach, in order to help us to check what you're referring to.
> > 
> > Regards,
> > Mauro
> > 
> > Thanks,
> > Mauro
> >   



Thanks,
Mauro

Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control

[Helen Koike]

Hi Lucas,

Thank you for your patch, just some small comments below.

On 12/15/18 2:46 PM, Lucas A. M. Magalhães wrote:
> The previous code pipeline used the stack to walk on the graph and
> process a frame. Basically the vimc-sensor entity starts a thread that
> generates the frames and calls the propagate_process function to send
> this frame to each entity linked with a sink pad. The propagate_process
> will call the process_frame of the entities which will call the
> propagate_frame for each one of it's sink pad. This cycle will continue
> until it reaches a vimc-capture entity that will finally return and
> unstack.
> 
> This solution had many problems:
>   * It was a little bit slow
>   * It was susceptible to a stack overflow as it made indiscriminate
>     use of the stack.
>   * It doesn't allow frame rate control
>   * It was complex to understand
>   * It doesn't allow pipeline control
> 
> This commit proposes an alternative way to control vimc streams by
> having a streamer object. This object will create a linear pipeline
> walking backwards on the graph. When the stream starts it will simply
> loop through the pipeline calling the respective process_frame function
> for each entity on the pipeline.
> 
> This solution has some premises which are true for now:
>   * Two paths can never be enabled and streaming at the same time.
>   * There is no entity streaming frames to two source pads at the same
>     time.
>   * There is no entity receiving frames from two sink pads at the same
>     time.
> 
> Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>

I won't comment on the commit message, as Mauro already sent a good review.

> ---
> Hi,
> 
> This patch introduces a streamer controller library for the vimc
> driver. It's a step towards a optimized mode I've been discussing with
> Helen.
> I plan to pass a tpg struct through the pipeline. This tpg struct
> will be configured in each entity and the capture will generate the
> frames with the correct format at the end of the pipeline.
> 
> Thanks,
> Lucas
> 
>  drivers/media/platform/vimc/Makefile        |   3 +-
>  drivers/media/platform/vimc/vimc-capture.c  |  18 +-
>  drivers/media/platform/vimc/vimc-common.c   |  50 ++----
>  drivers/media/platform/vimc/vimc-common.h   |  15 +-
>  drivers/media/platform/vimc/vimc-debayer.c  |  26 +--
>  drivers/media/platform/vimc/vimc-scaler.c   |  28 +---
>  drivers/media/platform/vimc/vimc-sensor.c   |  56 ++-----
>  drivers/media/platform/vimc/vimc-streamer.c | 176 ++++++++++++++++++++
>  drivers/media/platform/vimc/vimc-streamer.h |  38 +++++
>  9 files changed, 268 insertions(+), 142 deletions(-)
>  create mode 100644 drivers/media/platform/vimc/vimc-streamer.c
>  create mode 100644 drivers/media/platform/vimc/vimc-streamer.h
> 
> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
> index 4b2e3de7856e..c4fc8e7d365a 100644
> --- a/drivers/media/platform/vimc/Makefile
> +++ b/drivers/media/platform/vimc/Makefile
> @@ -5,6 +5,7 @@ vimc_common-objs := vimc-common.o
>  vimc_debayer-objs := vimc-debayer.o
>  vimc_scaler-objs := vimc-scaler.o
>  vimc_sensor-objs := vimc-sensor.o
> +vimc_streamer-objs := vimc-streamer.o
>  
>  obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc_capture.o vimc_common.o vimc-debayer.o \
> -				vimc_scaler.o vimc_sensor.o
> +			    vimc_scaler.o vimc_sensor.o vimc_streamer.o
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 3f7e9ed56633..80d7515ec420 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -24,6 +24,7 @@
>  #include <media/videobuf2-vmalloc.h>
>  
>  #include "vimc-common.h"
> +#include "vimc-streamer.h"
>  
>  #define VIMC_CAP_DRV_NAME "vimc-capture"
>  
> @@ -44,7 +45,7 @@ struct vimc_cap_device {
>  	spinlock_t qlock;
>  	struct mutex lock;
>  	u32 sequence;
> -	struct media_pipeline pipe;
> +	struct vimc_stream stream;
>  };
>  
>  static const struct v4l2_pix_format fmt_default = {
> @@ -248,14 +249,13 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	vcap->sequence = 0;
>  
>  	/* Start the media pipeline */
> -	ret = media_pipeline_start(entity, &vcap->pipe);
> +	ret = media_pipeline_start(entity, &vcap->stream.pipe);
>  	if (ret) {
>  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>  		return ret;
>  	}
>  
> -	/* Enable streaming from the pipe */
> -	ret = vimc_pipeline_s_stream(&vcap->vdev.entity, 1);
> +	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
>  	if (ret) {
>  		media_pipeline_stop(entity);
>  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> @@ -273,8 +273,7 @@ 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);
> +	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
>  
>  	/* Stop the media pipeline */
>  	media_pipeline_stop(&vcap->vdev.entity);
> @@ -355,8 +354,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>  	kfree(vcap);
>  }
>  
> -static void vimc_cap_process_frame(struct vimc_ent_device *ved,
> -				   struct media_pad *sink, const void *frame)
> +static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> +				    const void *frame)
>  {
>  	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>  						    ved);
> @@ -370,7 +369,7 @@ static void vimc_cap_process_frame(struct vimc_ent_device *ved,
>  					    typeof(*vimc_buf), list);
>  	if (!vimc_buf) {
>  		spin_unlock(&vcap->qlock);
> -		return;
> +		return ERR_PTR(-EAGAIN);
>  	}
>  
>  	/* Remove this entry from the list */
> @@ -391,6 +390,7 @@ static void vimc_cap_process_frame(struct vimc_ent_device *ved,
>  	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);
> +	return NULL;
>  }
>  
>  static int vimc_cap_comp_bind(struct device *comp, struct device *master,
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index dee1b9dfc4f6..8eb27ac589e2 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -207,41 +207,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat)
>  }
>  EXPORT_SYMBOL_GPL(vimc_pix_map_by_pixelformat);
>  
> -int vimc_propagate_frame(struct media_pad *src, const void *frame)
> -{
> -	struct media_link *link;
> -
> -	if (!(src->flags & MEDIA_PAD_FL_SOURCE))
> -		return -EINVAL;
> -
> -	/* Send this frame to all sink pads that are direct linked */
> -	list_for_each_entry(link, &src->entity->links, list) {
> -		if (link->source == src &&
> -		    (link->flags & MEDIA_LNK_FL_ENABLED)) {
> -			struct vimc_ent_device *ved = NULL;
> -			struct media_entity *entity = link->sink->entity;
> -
> -			if (is_media_entity_v4l2_subdev(entity)) {
> -				struct v4l2_subdev *sd =
> -					container_of(entity, struct v4l2_subdev,
> -						     entity);
> -				ved = v4l2_get_subdevdata(sd);
> -			} else if (is_media_entity_v4l2_video_device(entity)) {
> -				struct video_device *vdev =
> -					container_of(entity,
> -						     struct video_device,
> -						     entity);
> -				ved = video_get_drvdata(vdev);
> -			}
> -			if (ved && ved->process_frame)
> -				ved->process_frame(ved, link->sink, frame);
> -		}
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(vimc_propagate_frame);
> -
>  /* Helper function to allocate and initialize pads */
>  struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
>  {
> @@ -263,6 +228,21 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
>  }
>  EXPORT_SYMBOL_GPL(vimc_pads_init);
>  
> +struct media_entity *vimc_get_source_entity(struct media_entity *ent)

You can use const here:

const struct media_entity *ent

> +{
> +	struct media_pad *pad;
> +	int i;
> +
> +	for (i = 0; i < ent->num_pads; i++) {
> +		if (ent->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> +			continue;
> +		pad = media_entity_remote_pad(&ent->pads[i]);
> +		return pad ? pad->entity : NULL;

Just a suggestion (feel free to take it or not):

		if (ent->pads[i].flags & MEDIA_PAD_FL_SINK) {
			pad = media_entity_remote_pad(&ent->pads[i]);
			return pad ? pad->entity : NULL;
		}

and IMO it is easier to read.

> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(vimc_get_source_entity);
> +
>  int vimc_pipeline_s_stream(struct media_entity *ent, int enable)
>  {
>  	struct v4l2_subdev *sd;
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 2e9981b18166..1cbafd31ecb0 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -113,22 +113,21 @@ struct vimc_pix_map {
>  struct vimc_ent_device {
>  	struct media_entity *ent;
>  	struct media_pad *pads;
> -	void (*process_frame)(struct vimc_ent_device *ved,
> -			      struct media_pad *sink, const void *frame);
> +	void * (*process_frame)(struct vimc_ent_device *ved,
> +				const void *frame);
>  	void (*vdev_get_format)(struct vimc_ent_device *ved,
>  			      struct v4l2_pix_format *fmt);
>  };
>  
>  /**
> - * vimc_propagate_frame - propagate a frame through the topology
> + * vimc_get_source_entity - get the media_entity source of a given media_entity


pads type are either sink or source, not entities.

suggestion:

get the remote entity connected to a sink pad

or:

get the remote entity with a source pad

or:
get the remote entity who owns the source pad in the link

>   *
> - * @src:	the source pad where the frame is being originated
> - * @frame:	the frame to be propagated
> + * @ent:	reference media_entity
>   *
> - * This function will call the process_frame callback from the vimc_ent_device
> - * struct of the nodes directly connected to the @src pad
> + * Helper function that return the media entity which is source from the given
> + * media entity.

s/return/returns

I would also reword this a bit:

Helper function that returns the media entity who owns the source pad
across of an enabled link in @ent.

(but take it with a grain of salt as my English is not that good)

>   */
> -int vimc_propagate_frame(struct media_pad *src, const void *frame);
> +struct media_entity *vimc_get_source_entity(struct media_entity *ent);

This function is only used by the streamer code, so I don't think
putting it in common makes much sense for now (unless you think it might
be useful for latter), so maybe just declaring it static in
vimc-streamer.c is ok.

>  
>  /**
>   * vimc_pads_init - initialize pads
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 77887f66f323..7d77c63b99d2 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -321,7 +321,6 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb,
>  static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd);
> -	int ret;
>  
>  	if (enable) {
>  		const struct vimc_pix_map *vpix;
> @@ -351,22 +350,10 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!vdeb->src_frame)
>  			return -ENOMEM;
>  
> -		/* Turn the stream on in the subdevices directly connected */
> -		ret = vimc_pipeline_s_stream(&vdeb->sd.entity, 1);
> -		if (ret) {
> -			vfree(vdeb->src_frame);
> -			vdeb->src_frame = NULL;
> -			return ret;
> -		}
>  	} else {
>  		if (!vdeb->src_frame)
>  			return 0;
>  
> -		/* Disable streaming from the pipe */
> -		ret = vimc_pipeline_s_stream(&vdeb->sd.entity, 0);
> -		if (ret)
> -			return ret;
> -
>  		vfree(vdeb->src_frame);
>  		vdeb->src_frame = NULL;
>  	}
> @@ -480,9 +467,8 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
>  	}
>  }
>  
> -static void vimc_deb_process_frame(struct vimc_ent_device *ved,
> -				   struct media_pad *sink,
> -				   const void *sink_frame)
> +static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> +				    const void *sink_frame)
>  {
>  	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>  						    ved);
> @@ -491,7 +477,7 @@ static void vimc_deb_process_frame(struct vimc_ent_device *ved,
>  
>  	/* If the stream in this node is not active, just return */
>  	if (!vdeb->src_frame)
> -		return;
> +		return ERR_PTR(-EINVAL);
>  
>  	for (i = 0; i < vdeb->sink_fmt.height; i++)
>  		for (j = 0; j < vdeb->sink_fmt.width; j++) {
> @@ -499,12 +485,8 @@ static void vimc_deb_process_frame(struct vimc_ent_device *ved,
>  			vdeb->set_rgb_src(vdeb, i, j, rgb);
>  		}
>  
> -	/* Propagate the frame through all source pads */
> -	for (i = 1; i < vdeb->sd.entity.num_pads; i++) {
> -		struct media_pad *pad = &vdeb->sd.entity.pads[i];
> +	return vdeb->src_frame;
>  
> -		vimc_propagate_frame(pad, vdeb->src_frame);
> -	}
>  }
>  
>  static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index b0952ee86296..39b2a73dfcc1 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -217,7 +217,6 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
>  static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> -	int ret;
>  
>  	if (enable) {
>  		const struct vimc_pix_map *vpix;
> @@ -245,22 +244,10 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!vsca->src_frame)
>  			return -ENOMEM;
>  
> -		/* Turn the stream on in the subdevices directly connected */
> -		ret = vimc_pipeline_s_stream(&vsca->sd.entity, 1);
> -		if (ret) {
> -			vfree(vsca->src_frame);
> -			vsca->src_frame = NULL;
> -			return ret;
> -		}
>  	} else {
>  		if (!vsca->src_frame)
>  			return 0;
>  
> -		/* Disable streaming from the pipe */
> -		ret = vimc_pipeline_s_stream(&vsca->sd.entity, 0);
> -		if (ret)
> -			return ret;
> -
>  		vfree(vsca->src_frame);
>  		vsca->src_frame = NULL;
>  	}
> @@ -346,26 +333,19 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
>  			vimc_sca_scale_pix(vsca, i, j, sink_frame);
>  }
>  
> -static void vimc_sca_process_frame(struct vimc_ent_device *ved,
> -				   struct media_pad *sink,
> -				   const void *sink_frame)
> +static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> +				    const void *sink_frame)
>  {
>  	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>  						    ved);
> -	unsigned int i;
>  
>  	/* If the stream in this node is not active, just return */
>  	if (!vsca->src_frame)
> -		return;
> +		return ERR_PTR(-EINVAL);
>  
>  	vimc_sca_fill_src_frame(vsca, sink_frame);
>  
> -	/* Propagate the frame through all source pads */
> -	for (i = 1; i < vsca->sd.entity.num_pads; i++) {
> -		struct media_pad *pad = &vsca->sd.entity.pads[i];
> -
> -		vimc_propagate_frame(pad, vsca->src_frame);
> -	}
> +	return vsca->src_frame;
>  };
>  
>  static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 32ca9c6172b1..93961a1e694f 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -16,8 +16,6 @@
>   */
>  
>  #include <linux/component.h>
> -#include <linux/freezer.h>
> -#include <linux/kthread.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> @@ -201,38 +199,27 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
>  	.set_fmt		= vimc_sen_set_fmt,
>  };
>  
> -static int vimc_sen_tpg_thread(void *data)
> +static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> +				    const void *sink_frame)
>  {
> -	struct vimc_sen_device *vsen = data;
> -	unsigned int i;
> -
> -	set_freezable();
> -	set_current_state(TASK_UNINTERRUPTIBLE);
> -
> -	for (;;) {
> -		try_to_freeze();
> -		if (kthread_should_stop())
> -			break;
> -
> -		tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
> +	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> +						    ved);
> +	const struct vimc_pix_map *vpix;
> +	unsigned int frame_size;
>  
> -		/* Send the frame to all source pads */
> -		for (i = 0; i < vsen->sd.entity.num_pads; i++)
> -			vimc_propagate_frame(&vsen->sd.entity.pads[i],
> -					     vsen->frame);
> +	/* Calculate the frame size */
> +	vpix = vimc_pix_map_by_code(vsen->mbus_format.code);
> +	frame_size = vsen->mbus_format.width * vpix->bpp *
> +		     vsen->mbus_format.height;
>  
> -		/* 60 frames per second */
> -		schedule_timeout(HZ/60);
> -	}
> -
> -	return 0;
> +	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
> +	return vsen->frame;
>  }
>  
>  static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct vimc_sen_device *vsen =
>  				container_of(sd, struct vimc_sen_device, sd);
> -	int ret;
>  
>  	if (enable) {
>  		const struct vimc_pix_map *vpix;
> @@ -258,26 +245,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>  		/* configure the test pattern generator */
>  		vimc_sen_tpg_s_format(vsen);
>  
> -		/* Initialize the image generator thread */
> -		vsen->kthread_sen = kthread_run(vimc_sen_tpg_thread, vsen,
> -					"%s-sen", vsen->sd.v4l2_dev->name);
> -		if (IS_ERR(vsen->kthread_sen)) {
> -			dev_err(vsen->dev, "%s: kernel_thread() failed\n",
> -				vsen->sd.name);
> -			vfree(vsen->frame);
> -			vsen->frame = NULL;
> -			return PTR_ERR(vsen->kthread_sen);
> -		}
>  	} else {
> -		if (!vsen->kthread_sen)
> -			return 0;
> -
> -		/* Stop image generator */
> -		ret = kthread_stop(vsen->kthread_sen);
> -		if (ret)
> -			return ret;
>  
> -		vsen->kthread_sen = NULL;
>  		vfree(vsen->frame);
>  		vsen->frame = NULL;
>  		return 0;
> @@ -413,6 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  	if (ret)
>  		goto err_free_hdl;
>  
> +	vsen->ved.process_frame = vimc_sen_process_frame;
>  	dev_set_drvdata(comp, &vsen->ved);
>  	vsen->dev = comp;
>  
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> new file mode 100644
> index 000000000000..f614734538c2
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * vimc-streamer.c Virtual Media Controller Driver
> + *
> + * Copyright (C) 2018 Lucas A. M. Magalhães <lucmaga@gmail.com>
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/freezer.h>
> +#include <linux/kthread.h>
> +
> +#include "vimc-streamer.h"
> +
> +/*
> + * vimc_streamer_pipeline_disable - Disable stream in all ved in stream
> + *
> + * @stream: the pointer to the stream structure with the pipeline to be
> + *	    disabled.
> + *
> + * Calls s_stream to disable the stream in each entity of the pipeline
> + *
> + */
> +static void vimc_streamer_pipeline_disable(struct vimc_stream *stream)
> +{
> +	struct media_entity *entity;
> +	struct v4l2_subdev *sd;
> +
> +	do {
> +		stream->pipe_size--;
> +		entity = stream->ved_pipeline[stream->pipe_size]->ent;
> +		entity = vimc_get_source_entity(entity);
> +		stream->ved_pipeline[stream->pipe_size] = NULL;
> +		/*
> +		 *  This may occur only if the streamer was not correctly
> +		 *  initialized.
> +		 */
> +		if (!entity)
> +			continue;

I don't think this can happen, if so, then there is a bug in
vimc_streamer_pipeline_init()

> +
> +		if (!is_media_entity_v4l2_subdev(entity))
> +			continue;
> +
> +		sd = media_entity_to_v4l2_subdev(entity);
> +		v4l2_subdev_call(sd, video, s_stream, 0);

Just a suggestion, feel free to take it or not:

		if (is_media_entity_v4l2_subdev(entity)) {
			sd = media_entity_to_v4l2_subdev(entity);
			v4l2_subdev_call(sd, video, s_stream, 0);
		}

> +	} while (stream->pipe_size);
> +}
> +
> +/*
> + * vimc_streamer_pipeline_init - initializes the stream structure
> + *
> + * @stream: the pointer to the stream structure to be initialized
> + * @ved:    the pointer to the vimc entity initializing the stream
> + *
> + * Initializes the stream structure. Walks through the entity graph to
> + * construct the pipeline used later on the streamer thread.
> + * Calls s_stream to enable stream in all entities of the pipeline.
> + */
> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> +				struct vimc_ent_device *ved)
> +{
> +	struct vimc_ent_device *source_ved = NULL;

I don't think you need to initialize it to NULL.

> +	struct media_entity *entity;
> +	struct video_device *vdev;
> +	struct v4l2_subdev *sd;
> +	int ret = -EINVAL;
> +
> +	stream->pipe_size = 0;
> +	stream->ved_pipeline[stream->pipe_size++] = ved;
> +
> +	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> +		if (!stream->ved_pipeline[stream->pipe_size-1])
> +			return 0;
> +		entity = stream->ved_pipeline[stream->pipe_size-1]->ent;
> +		entity = vimc_get_source_entity(entity);
> +		if (!entity)
> +			return 0;
> +		if (is_media_entity_v4l2_subdev(entity)) {
> +			sd = media_entity_to_v4l2_subdev(entity);
> +			ret = v4l2_subdev_call(sd, video, s_stream, 1);
> +			if (ret && ret != -ENOIOCTLCMD)
> +				break;
> +			source_ved = v4l2_get_subdevdata(sd);
> +		} else {
> +			vdev = container_of(entity,
> +					    struct video_device,
> +					    entity);
> +			source_ved = video_get_drvdata(vdev);
> +		}
> +
> +		stream->ved_pipeline[stream->pipe_size++] = source_ved;
> +		source_ved = NULL;

You don't need to set it to NULL.

> +	}
> +
> +	/*
> +	 * If an error occur during initialization or the pipeline gets longer

s/occur/occurs

durint the initialization

> +	 * than VIMC_STREAMER_PIPELINE_MAX_SIZE the stream is disabled and
> +	 * return the error code.

it returns

> +	 */
> +	vimc_streamer_pipeline_disable(stream);
> +	return ret;
> +}

vimc_streamer_pipeline_disable() undo what vimc_streamer_pipeline_init()
do, I was thinking maybe to change the name to be clear they are oposite.
Maybe vimc_streamer_pipeline_enable() and vimc_streamer_pipeline_disable() ?

> +
> +static int vimc_streamer_thread(void *data)
> +{
> +	struct vimc_stream *stream = data;
> +	int i;
> +
> +	set_freezable();
> +	set_current_state(TASK_UNINTERRUPTIBLE);
> +
> +	for (;;) {
> +		try_to_freeze();
> +		if (kthread_should_stop())
> +			break;
> +
> +		for (i = stream->pipe_size - 1; i >= 0; i--) {
> +			stream->frame = stream->ved_pipeline[i]->process_frame(
> +					stream->ved_pipeline[i],
> +					stream->frame);
> +			if (!stream->frame)
> +				break;
> +			if (IS_ERR(stream->frame))
> +				break;
> +		}
> +		//wait for 60hz
> +		schedule_timeout(HZ / 60);
> +	}
> +
> +	return 0;
> +}
> +
> +int vimc_streamer_s_stream(struct vimc_stream *stream,
> +			   struct vimc_ent_device *ved,
> +			   int enable)
> +{
> +	int ret;
> +
> +	if (!stream || !ved)
> +		return -EINVAL;
> +
> +	if (enable) {
> +		if (stream->kthread)
> +			return 0;
> +
> +		ret = vimc_streamer_pipeline_init(stream, ved);
> +		if (ret)
> +			return ret;
> +
> +		stream->kthread = kthread_run(vimc_streamer_thread, stream,
> +					      "vimc-streamer thread");
> +
> +		if (IS_ERR(stream->kthread))
> +			return PTR_ERR(stream->kthread);
> +
> +	} else {
> +		if (!stream->kthread)
> +			return 0;
> +
> +		ret = kthread_stop(stream->kthread);
> +		if (ret)
> +			return ret;
> +
> +		stream->kthread = NULL;
> +
> +		vimc_streamer_pipeline_disable(stream);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vimc_streamer_s_stream);
> +
> +MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Streamer");
> +MODULE_AUTHOR("Lucas A. M. Magalhães <lucmaga@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/vimc/vimc-streamer.h b/drivers/media/platform/vimc/vimc-streamer.h
> new file mode 100644
> index 000000000000..752af2e2d5a2
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-streamer.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * vimc-streamer.h Virtual Media Controller Driver
> + *
> + * Copyright (C) 2018 Lucas A. M. Magalhães <lucmaga@gmail.com>
> + *
> + */
> +
> +#ifndef _VIMC_STREAMER_H_
> +#define _VIMC_STREAMER_H_
> +
> +#include <media/media-device.h>
> +
> +#include "vimc-common.h"
> +
> +#define VIMC_STREAMER_PIPELINE_MAX_SIZE 16
> +
> +struct vimc_stream {
> +	struct media_pipeline pipe;
> +	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
> +	unsigned int pipe_size;
> +	u8 *frame;
> +	struct task_struct *kthread;
> +};
> +
> +/**
> + * vimc_streamer_s_streamer - start/stop the stream
> + *
> + * @stream:	the pointer to the stream to start or stop
> + * @ved:	The last entity of the streamer pipeline
> + * @enable:	any non-zero number start the stream, zero stop
> + *
> + */
> +int vimc_streamer_s_stream(struct vimc_stream *stream,
> +			   struct vimc_ent_device *ved,
> +			   int enable);
> +
> +#endif  //_VIMC_STREAMER_H_
> 

Regards,
Helen