[PATCH v3 0/9] media: vimc: Multiple stream support in vimc

[PATCH v3 0/9] media: vimc: Multiple stream support in vimc

[Kaaira Gupta]

This series adds supoort for two (or more) capture devices to be 
connected to the same sensors and run simultaneously.

Changes since v2:
	- This series introduces new patches, namely patch 1, 2, 4, 5,
	  7, and 9 to shift multiple captures to operate at a single
	  thread.

Kaaira Gupta (7):
  media: vimc: Move get_source_entity to vimc-common
  media: vimc: Add get_frame callback
  media: vimc: Separate starting stream from pipeline initialisation
  media: vimc: Separate closing of stream and thread
  media: vimc: Dynamically allocate stream struct
  media: vimc: Join pipeline if one already exists
  media: vimc: Run multiple captures on same thread

Niklas Söderlund (2):
  media: vimc: Add usage count to subdevices
  media: vimc: Serialize vimc_streamer_s_stream()

 .../media/test-drivers/vimc/vimc-capture.c    |  42 +++-
 drivers/media/test-drivers/vimc/vimc-common.c |  14 ++
 drivers/media/test-drivers/vimc/vimc-common.h |  21 +-
 .../media/test-drivers/vimc/vimc-debayer.c    |  26 ++-
 drivers/media/test-drivers/vimc/vimc-scaler.c |  25 +-
 drivers/media/test-drivers/vimc/vimc-sensor.c |  17 +-
 .../media/test-drivers/vimc/vimc-streamer.c   | 213 ++++++++++++------
 .../media/test-drivers/vimc/vimc-streamer.h   |   2 +
 8 files changed, 271 insertions(+), 89 deletions(-)

-- 
2.17.1

[PATCH v3 1/9] media: vimc: Move get_source_entity to vimc-common

[Kaaira Gupta]

Move the function vimc_get_source_entity() to vimc-common.c to make it
reusable.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-common.c | 14 +++++++++++
 drivers/media/test-drivers/vimc/vimc-common.h | 12 ++++++++++
 .../media/test-drivers/vimc/vimc-streamer.c   | 24 -------------------
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-common.c b/drivers/media/test-drivers/vimc/vimc-common.c
index 0d97b25ce21e..91c8992bb391 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.c
+++ b/drivers/media/test-drivers/vimc/vimc-common.c
@@ -417,3 +417,17 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 	media_entity_cleanup(&sd->entity);
 	return ret;
 }
+
+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;
+}
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index a289434e75ba..4c580d854007 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -230,4 +230,16 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
  */
 int vimc_vdev_link_validate(struct media_link *link);
 
+/**
+ * vimc_get_source_entity - get the entity connected with the first sink pad
+ *
+ * @ent:	reference media_entity
+ *
+ * Helper function that returns the media entity containing the source pad
+ * linked with the first sink pad from the given media entity pad list.
+ *
+ * Return: The source pad or NULL, if it wasn't found.
+ */
+struct media_entity *vimc_get_source_entity(struct media_entity *ent);
+
 #endif
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index 451a32c0d034..4f8384246042 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -12,30 +12,6 @@
 
 #include "vimc-streamer.h"
 
-/**
- * vimc_get_source_entity - get the entity connected with the first sink pad
- *
- * @ent:	reference media_entity
- *
- * Helper function that returns the media entity containing the source pad
- * linked with the first sink pad from the given media entity pad list.
- *
- * Return: The source pad or NULL, if it wasn't found.
- */
-static 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;
-}
-
 /**
  * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
  *
-- 
2.17.1

[PATCH v3 2/9] media: vimc: Add get_frame callback

[Kaaira Gupta]

In the process of making vimc compatible for multiple streams, we need
to create a frame passing process such that two different entities can
get the frame from a common entity. This isn't possible currently without
calling process_frame twice for the common entity, as process_frames
returns the frame which gets passed on.

So, to take care of this, add a get_frame callback to vimc device and
use it to get the frames for an entity from previous entity instead of
returning and passing the frames as an argument in process_frame.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 .../media/test-drivers/vimc/vimc-capture.c    | 18 +++++++++++++++---
 drivers/media/test-drivers/vimc/vimc-common.h |  7 ++++---
 .../media/test-drivers/vimc/vimc-debayer.c    | 19 ++++++++++++++++---
 drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++++++++++++++---
 drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +++++++++--
 .../media/test-drivers/vimc/vimc-streamer.c   | 10 ++++++----
 6 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index c63496b17b9a..a8cbb8e4d5ba 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device *ved)
 	video_unregister_device(&vcap->vdev);
 }
 
-static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
-				    const void *frame)
+static int vimc_cap_process_frame(struct vimc_ent_device *ved)
 {
 	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
 						    ved);
 	struct vimc_cap_buffer *vimc_buf;
+	struct v4l2_subdev *sd;
+	const void *frame;
 	void *vbuf;
 
 	spin_lock(&vcap->qlock);
@@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
 					    typeof(*vimc_buf), list);
 	if (!vimc_buf) {
 		spin_unlock(&vcap->qlock);
-		return ERR_PTR(-EAGAIN);
+		return -EAGAIN;
 	}
 
 	/* Remove this entry from the list */
@@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
 
 	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
 
+	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
+	ved = v4l2_get_subdevdata(sd);
+	frame = ved->get_frame(ved);
+
 	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);
+
+	return 0;
+}
+
+static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
+{
 	return NULL;
 }
 
@@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 	vcap->ved.ent = &vcap->vdev.entity;
 	vcap->ved.process_frame = vimc_cap_process_frame;
 	vcap->ved.vdev_get_format = vimc_cap_get_format;
+	vcap->ved.get_frame = vimc_cap_get_frame;
 	vcap->ved.dev = vimc->mdev.dev;
 
 	/* Initialize the video_device struct */
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index 4c580d854007..287d66edff49 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -85,7 +85,8 @@ struct vimc_pix_map {
  *
  * @dev:		a pointer of the device struct of the driver
  * @ent:		the pointer to struct media_entity for the node
- * @process_frame:	callback send a frame to that node
+ * @get_frame:		callback that sends a frame processed by the entity
+ * @process_frame:	callback that processes a frame
  * @vdev_get_format:	callback that returns the current format a pad, used
  *			only when is_media_entity_v4l2_video_device(ent) returns
  *			true
@@ -101,8 +102,8 @@ struct vimc_pix_map {
 struct vimc_ent_device {
 	struct device *dev;
 	struct media_entity *ent;
-	void * (*process_frame)(struct vimc_ent_device *ved,
-				const void *frame);
+	void * (*get_frame)(struct vimc_ent_device *ved);
+	int (*process_frame)(struct vimc_ent_device *ved);
 	void (*vdev_get_format)(struct vimc_ent_device *ved,
 			      struct v4l2_pix_format *fmt);
 };
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index c3f6fef34f68..f61e6e8899ac 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -491,17 +491,22 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
 	}
 }
 
-static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
-				    const void *sink_frame)
+static int vimc_deb_process_frame(struct vimc_ent_device *ved)
 {
 	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
 						    ved);
 	unsigned int rgb[3];
 	unsigned int i, j;
+	struct v4l2_subdev *sd;
+	const void *sink_frame;
 
 	/* If the stream in this node is not active, just return */
 	if (!vdeb->src_frame)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
+
+	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
+	ved = v4l2_get_subdevdata(sd);
+	sink_frame = ved->get_frame(ved);
 
 	for (i = 0; i < vdeb->sink_fmt.height; i++)
 		for (j = 0; j < vdeb->sink_fmt.width; j++) {
@@ -509,6 +514,13 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
 			vdeb->set_rgb_src(vdeb, i, j, rgb);
 		}
 
+	return 0;
+}
+
+static void *vimc_deb_get_frame(struct vimc_ent_device *ved)
+{
+	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
+						    ved);
 	return vdeb->src_frame;
 }
 
@@ -593,6 +605,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 		goto err_free_hdl;
 
 	vdeb->ved.process_frame = vimc_deb_process_frame;
+	vdeb->ved.get_frame = vimc_deb_get_frame;
 	vdeb->ved.dev = vimc->mdev.dev;
 	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
 
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 121fa7d62a2e..347f9cd4a168 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -455,18 +455,29 @@ 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,
-				    const void *sink_frame)
+static int vimc_sca_process_frame(struct vimc_ent_device *ved)
 {
 	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
 						    ved);
+	const void *sink_frame;
+	struct v4l2_subdev *sd;
 
 	/* If the stream in this node is not active, just return */
 	if (!vsca->src_frame)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
+	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
+	ved = v4l2_get_subdevdata(sd);
+	sink_frame = ved->get_frame(ved);
 	vimc_sca_fill_src_frame(vsca, sink_frame);
 
+	return 0;
+};
+
+static void *vimc_sca_get_frame(struct vimc_ent_device *ved)
+{
+	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
+						    ved);
 	return vsca->src_frame;
 };
 
@@ -505,6 +516,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 	}
 
 	vsca->ved.process_frame = vimc_sca_process_frame;
+	vsca->ved.get_frame = vimc_sca_get_frame;
 	vsca->ved.dev = vimc->mdev.dev;
 
 	/* Initialize the frame format */
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index ba5db5a150b4..32a2c39de2cd 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -190,8 +190,7 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
 	.set_fmt		= vimc_sen_set_fmt,
 };
 
-static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
-				    const void *sink_frame)
+static int vimc_sen_process_frame(struct vimc_ent_device *ved)
 {
 	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
 						    ved);
@@ -238,6 +237,13 @@ static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
 		break;
 	}
 
+	return 0;
+}
+
+static void *vimc_sen_get_frame(struct vimc_ent_device *ved)
+{
+	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
+						    ved);
 	return vsen->frame;
 }
 
@@ -429,6 +435,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 		goto err_free_tpg;
 
 	vsen->ved.process_frame = vimc_sen_process_frame;
+	vsen->ved.get_frame = vimc_sen_get_frame;
 	vsen->ved.dev = vimc->mdev.dev;
 
 	/* Initialize the frame format */
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index 4f8384246042..c1644d69686d 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -125,7 +125,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 static int vimc_streamer_thread(void *data)
 {
 	struct vimc_stream *stream = data;
-	u8 *frame = NULL;
+	struct vimc_ent_device *ved;
+	int ret;
 	int i;
 
 	set_freezable();
@@ -136,9 +137,10 @@ static int vimc_streamer_thread(void *data)
 			break;
 
 		for (i = stream->pipe_size - 1; i >= 0; i--) {
-			frame = stream->ved_pipeline[i]->process_frame(
-					stream->ved_pipeline[i], frame);
-			if (!frame || IS_ERR(frame))
+			ved = stream->ved_pipeline[i];
+			ret = ved->process_frame(ved);
+
+			if (ret)
 				break;
 		}
 		//wait for 60hz
-- 
2.17.1

[PATCH v3 3/9] media: vimc: Add usage count to subdevices

[Kaaira Gupta]

From: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Prepare for multiple video streams from the same sensor by adding a use
counter to vimc_ent_device. The counter is increased for every s_stream(1)
and decremented for every s_stream(0) call.

The subdevice stream is not started or stopped unless the usage count go
from 0 to 1 (started) or from 1 to 0 (stopped). This allows for multiple
s_stream() calls to try to either start or stop the device while only
the first/last call will actually effect the state of the device.

Initialise and increment use_count for capture as well, as use_count
will be used in subsequent patches for starting process_frame as well.

[Kaaira: moved use_count to vimc entity device instead of declaring it
for each subdevice, used use_count for capture as well and rebased
the patch on current HEAD of master to help with the current series]

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-capture.c | 3 +++
 drivers/media/test-drivers/vimc/vimc-common.h  | 2 ++
 drivers/media/test-drivers/vimc/vimc-debayer.c | 7 +++++++
 drivers/media/test-drivers/vimc/vimc-scaler.c  | 7 +++++++
 drivers/media/test-drivers/vimc/vimc-sensor.c  | 6 ++++++
 5 files changed, 25 insertions(+)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index a8cbb8e4d5ba..93418cb5a139 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -243,6 +243,7 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct media_entity *entity = &vcap->vdev.entity;
 	int ret;
 
+	atomic_inc(&vcap->ved.use_count);
 	vcap->sequence = 0;
 
 	/* Start the media pipeline */
@@ -270,6 +271,7 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
 
+	atomic_dec(&vcap->ved.use_count);
 	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
 
 	/* Stop the media pipeline */
@@ -424,6 +426,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 	vcap->vdev.entity.name = vcfg_name;
 	vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
 	vcap->pad.flags = MEDIA_PAD_FL_SINK;
+	atomic_set(&vcap->ved.use_count, 0);
 	ret = media_entity_pads_init(&vcap->vdev.entity,
 				     1, &vcap->pad);
 	if (ret)
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index 287d66edff49..c214f5ec7818 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -85,6 +85,7 @@ struct vimc_pix_map {
  *
  * @dev:		a pointer of the device struct of the driver
  * @ent:		the pointer to struct media_entity for the node
+ * @use_count:		a count to show the number of streams entity is part of
  * @get_frame:		callback that sends a frame processed by the entity
  * @process_frame:	callback that processes a frame
  * @vdev_get_format:	callback that returns the current format a pad, used
@@ -102,6 +103,7 @@ struct vimc_pix_map {
 struct vimc_ent_device {
 	struct device *dev;
 	struct media_entity *ent;
+	atomic_t use_count;
 	void * (*get_frame)(struct vimc_ent_device *ved);
 	int (*process_frame)(struct vimc_ent_device *ved);
 	void (*vdev_get_format)(struct vimc_ent_device *ved,
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index f61e6e8899ac..60c4c0ec2030 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -343,6 +343,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
+		if (atomic_inc_return(&vdeb->ved.use_count) != 1)
+			return 0;
+
 		if (vdeb->src_frame)
 			return 0;
 
@@ -368,6 +371,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 			return -ENOMEM;
 
 	} else {
+		if (atomic_dec_return(&vdeb->ved.use_count) != 0)
+			return 0;
+
 		if (!vdeb->src_frame)
 			return 0;
 
@@ -608,6 +614,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 	vdeb->ved.get_frame = vimc_deb_get_frame;
 	vdeb->ved.dev = vimc->mdev.dev;
 	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
+	atomic_set(&vdeb->ved.use_count, 0);
 
 	/* Initialize the frame format */
 	vdeb->sink_fmt = sink_fmt_default;
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 347f9cd4a168..d511e1f2152d 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -340,6 +340,9 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
+		if (atomic_inc_return(&vsca->ved.use_count) != 1)
+			return 0;
+
 		if (vsca->src_frame)
 			return 0;
 
@@ -363,6 +366,9 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 			return -ENOMEM;
 
 	} else {
+		if (atomic_dec_return(&vsca->ved.use_count) != 0)
+			return 0;
+
 		if (!vsca->src_frame)
 			return 0;
 
@@ -518,6 +524,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 	vsca->ved.process_frame = vimc_sca_process_frame;
 	vsca->ved.get_frame = vimc_sca_get_frame;
 	vsca->ved.dev = vimc->mdev.dev;
+	atomic_set(&vsca->ved.use_count, 0);
 
 	/* Initialize the frame format */
 	vsca->sink_fmt = sink_fmt_default;
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 32a2c39de2cd..ced8ef06b01e 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -256,6 +256,9 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
+		if (atomic_inc_return(&vsen->ved.use_count) != 1)
+			return 0;
+
 		vsen->start_stream_ts = ktime_get_ns();
 
 		/* Calculate the frame size */
@@ -275,6 +278,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		vimc_sen_tpg_s_format(vsen);
 
 	} else {
+		if (atomic_dec_return(&vsen->ved.use_count) != 0)
+			return 0;
 
 		vfree(vsen->frame);
 		vsen->frame = NULL;
@@ -437,6 +442,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 	vsen->ved.process_frame = vimc_sen_process_frame;
 	vsen->ved.get_frame = vimc_sen_get_frame;
 	vsen->ved.dev = vimc->mdev.dev;
+	atomic_set(&vsen->ved.use_count, 0);
 
 	/* Initialize the frame format */
 	vsen->mbus_format = fmt_default;
-- 
2.17.1

[PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Kaaira Gupta]

Separate the process of initialising pipeline array from starting
streaming for all entities in path of a stream. This is needed because
multiple streams can stream, but only one pipeline object is needed.

Process frames only for those entities in a pipeline which are
streaming. This is known through their use counts.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 .../media/test-drivers/vimc/vimc-streamer.c   | 95 ++++++++++++++++---
 1 file changed, 83 insertions(+), 12 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index c1644d69686d..cc40ecabe95a 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
 }
 
 /**
- * vimc_streamer_pipeline_init - Initializes the stream structure
+ * vimc_streamer_stream_start - Starts streaming for all entities
+ * in a stream
  *
- * @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 vimc_streamer_s_stream() to enable stream in all entities of
- * the pipeline.
+ * Walks through the entity graph to call vimc_streamer_s_stream()
+ * to enable stream in all entities in path of a stream.
  *
  * Return: 0 if success, error code otherwise.
  */
-static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
-				       struct vimc_ent_device *ved)
+static int vimc_streamer_stream_start(struct vimc_stream *stream,
+				      struct vimc_ent_device *ved)
 {
 	struct media_entity *entity;
 	struct video_device *vdev;
 	struct v4l2_subdev *sd;
+	int stream_size = 0;
 	int ret = 0;
 
-	stream->pipe_size = 0;
-	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
+	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
 		if (!ved) {
 			vimc_streamer_pipeline_terminate(stream);
 			return -EINVAL;
 		}
-		stream->ved_pipeline[stream->pipe_size++] = ved;
 
 		if (is_media_entity_v4l2_subdev(ved->ent)) {
 			sd = media_entity_to_v4l2_subdev(ved->ent);
@@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 					    entity);
 			ved = video_get_drvdata(vdev);
 		}
+		stream_size++;
+	}
+
+	vimc_streamer_pipeline_terminate(stream);
+	return -EINVAL;
+}
+
+/**
+ * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
+ *
+ * @stream: the pointer to the stream structure
+ * @ved:    the pointer to the vimc entity initializing the stream pipeline
+ *
+ * Walks through the entity graph to initialise ved_pipeline and updates
+ * pipe_size too.
+ *
+ * Return: 0 if success, error code otherwise.
+ */
+static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
+				       struct vimc_ent_device *ved)
+{
+	struct media_entity *entity;
+	struct media_device *mdev;
+	struct media_graph graph;
+	struct video_device *vdev;
+	struct v4l2_subdev *sd;
+	int ret;
+
+	entity = ved->ent;
+	mdev = entity->graph_obj.mdev;
+
+	ret = media_graph_walk_init(&graph, mdev);
+	if (ret)
+		return ret;
+
+	media_graph_walk_start(&graph, entity);
+
+	/*
+	 * Start pipeline array initialisation from RAW Capture only to get
+	 * entities in the correct order of their frame processing.
+	 */
+	if (!strncmp(entity->name, "RGB", 3)) {
+		entity = media_graph_walk_next(&graph);
+		mdev = entity->graph_obj.mdev;
+		media_graph_walk_cleanup(&graph);
+
+		ret = media_graph_walk_init(&graph, mdev);
+		if (ret)
+			return ret;
+		media_graph_walk_start(&graph, entity);
+	}
+
+	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
+		if (is_media_entity_v4l2_subdev(entity)) {
+			sd = media_entity_to_v4l2_subdev(entity);
+			ved = v4l2_get_subdevdata(sd);
+		} else {
+			vdev = container_of(entity, struct video_device, entity);
+			ved = video_get_drvdata(vdev);
+		}
+		stream->ved_pipeline[stream->pipe_size++] = ved;
+		entity = media_graph_walk_next(&graph);
+
+		if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
+			media_graph_walk_cleanup(&graph);
+			return 0;
+		}
 	}
 
 	vimc_streamer_pipeline_terminate(stream);
@@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
 
 		for (i = stream->pipe_size - 1; i >= 0; i--) {
 			ved = stream->ved_pipeline[i];
-			ret = ved->process_frame(ved);
 
+			if (atomic_read(&ved->use_count) == 0)
+				continue;
+
+			ret = ved->process_frame(ved);
 			if (ret)
 				break;
 		}
@@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		if (stream->kthread)
 			return 0;
 
+		ret = vimc_streamer_stream_start(stream, ved);
+		if (ret)
+			return ret;
+
 		ret = vimc_streamer_pipeline_init(stream, ved);
 		if (ret)
 			return ret;
-- 
2.17.1

[PATCH v3 5/9] media: vimc: Separate closing of stream and thread

[Kaaira Gupta]

Make separate functions for stopping streaming of entities in path of a
particular stream and stopping thread. This is needed to ensure that
thread doesn't stop when one device stops streaming in case of multiple
streams.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 .../media/test-drivers/vimc/vimc-streamer.c   | 82 ++++++++++++-------
 1 file changed, 52 insertions(+), 30 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index cc40ecabe95a..6b5ea1537952 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -13,29 +13,59 @@
 #include "vimc-streamer.h"
 
 /**
- * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
+ * vimc_streamer_pipeline_terminate - Terminate the thread
  *
- * @stream: the pointer to the stream structure with the pipeline to be
- *	    disabled.
+ * @stream: the pointer to the stream structure
  *
- * Calls s_stream to disable the stream in each entity of the pipeline
+ * Erases values of stream struct and terminates the thread
  *
  */
 static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
 {
 	struct vimc_ent_device *ved;
-	struct v4l2_subdev *sd;
 
 	while (stream->pipe_size) {
 		stream->pipe_size--;
 		ved = stream->ved_pipeline[stream->pipe_size];
 		stream->ved_pipeline[stream->pipe_size] = NULL;
+	}
 
-		if (!is_media_entity_v4l2_subdev(ved->ent))
-			continue;
+	kthread_stop(stream->kthread);
+	stream->kthread = NULL;
+}
 
-		sd = media_entity_to_v4l2_subdev(ved->ent);
-		v4l2_subdev_call(sd, video, s_stream, 0);
+/**
+ * vimc_streamer_stream_terminate - Disable stream in all ved in stream
+ *
+ * @ved: pointer to the ved for which stream needs to be disabled
+ *
+ * Calls s_stream to disable the stream in each entity of the stream
+ *
+ */
+static void vimc_streamer_stream_terminate(struct vimc_ent_device *ved)
+{
+	struct media_entity *entity = ved->ent;
+	struct video_device *vdev;
+	struct v4l2_subdev *sd;
+
+	while (entity) {
+		if (is_media_entity_v4l2_subdev(ved->ent)) {
+			sd = media_entity_to_v4l2_subdev(ved->ent);
+			v4l2_subdev_call(sd, video, s_stream, 0);
+		}
+		entity = vimc_get_source_entity(ved->ent);
+		if (!entity)
+			break;
+
+		if (is_media_entity_v4l2_subdev(entity)) {
+			sd = media_entity_to_v4l2_subdev(entity);
+			ved = v4l2_get_subdevdata(sd);
+		} else {
+			vdev = container_of(entity,
+					    struct video_device,
+					    entity);
+			ved = video_get_drvdata(vdev);
+		}
 	}
 }
 
@@ -43,25 +73,25 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
  * vimc_streamer_stream_start - Starts streaming for all entities
  * in a stream
  *
- * @ved:    the pointer to the vimc entity initializing the stream
+ * @cved:    the pointer to the vimc entity initializing the stream
  *
  * Walks through the entity graph to call vimc_streamer_s_stream()
  * to enable stream in all entities in path of a stream.
  *
  * Return: 0 if success, error code otherwise.
  */
-static int vimc_streamer_stream_start(struct vimc_stream *stream,
-				      struct vimc_ent_device *ved)
+static int vimc_streamer_stream_start(struct vimc_ent_device *cved)
 {
 	struct media_entity *entity;
 	struct video_device *vdev;
 	struct v4l2_subdev *sd;
+	struct vimc_ent_device *ved = cved;
 	int stream_size = 0;
 	int ret = 0;
 
 	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
 		if (!ved) {
-			vimc_streamer_pipeline_terminate(stream);
+			vimc_streamer_stream_terminate(cved);
 			return -EINVAL;
 		}
 
@@ -71,7 +101,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
 			if (ret && ret != -ENOIOCTLCMD) {
 				dev_err(ved->dev, "subdev_call error %s\n",
 					ved->ent->name);
-				vimc_streamer_pipeline_terminate(stream);
+				vimc_streamer_stream_terminate(cved);
 				return ret;
 			}
 		}
@@ -84,7 +114,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
 				dev_err(ved->dev,
 					"first entity in the pipe '%s' is not a source\n",
 					ved->ent->name);
-				vimc_streamer_pipeline_terminate(stream);
+				vimc_streamer_stream_terminate(cved);
 				pr_info ("first entry not source");
 				return -EPIPE;
 			}
@@ -104,7 +134,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
 		stream_size++;
 	}
 
-	vimc_streamer_pipeline_terminate(stream);
+	vimc_streamer_stream_terminate(cved);
 	return -EINVAL;
 }
 
@@ -120,13 +150,14 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
  * Return: 0 if success, error code otherwise.
  */
 static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
-				       struct vimc_ent_device *ved)
+				       struct vimc_ent_device *cved)
 {
 	struct media_entity *entity;
 	struct media_device *mdev;
 	struct media_graph graph;
 	struct video_device *vdev;
 	struct v4l2_subdev *sd;
+	struct vimc_ent_device *ved = cved;
 	int ret;
 
 	entity = ved->ent;
@@ -170,6 +201,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 		}
 	}
 
+	vimc_streamer_stream_terminate(cved);
 	vimc_streamer_pipeline_terminate(stream);
 	return -EINVAL;
 }
@@ -246,7 +278,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		if (stream->kthread)
 			return 0;
 
-		ret = vimc_streamer_stream_start(stream, ved);
+		ret = vimc_streamer_stream_start(ved);
 		if (ret)
 			return ret;
 
@@ -260,6 +292,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		if (IS_ERR(stream->kthread)) {
 			ret = PTR_ERR(stream->kthread);
 			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
+			vimc_streamer_stream_terminate(ved);
 			vimc_streamer_pipeline_terminate(stream);
 			stream->kthread = NULL;
 			return ret;
@@ -269,18 +302,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		if (!stream->kthread)
 			return 0;
 
-		ret = kthread_stop(stream->kthread);
-		/*
-		 * kthread_stop returns -EINTR in cases when streamon was
-		 * immediately followed by streamoff, and the thread didn't had
-		 * a chance to run. Ignore errors to stop the stream in the
-		 * pipeline.
-		 */
-		if (ret)
-			dev_dbg(ved->dev, "kthread_stop returned '%d'\n", ret);
-
-		stream->kthread = NULL;
-
+		vimc_streamer_stream_terminate(ved);
 		vimc_streamer_pipeline_terminate(stream);
 	}
 
-- 
2.17.1

[PATCH v3 6/9] media: vimc: Serialize vimc_streamer_s_stream()

[Kaaira Gupta]

From: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Prepare for multiple video streams from the same sensor by serializing
vimc_streamer_s_stream(). Multiple streams will allow for multiple
concurrent calls to this function that could involve the same
subdevices.

If that happens the internal state of the involved subdevices could go
out of sync as they are being started and stopped at the same time,
prevent this by serializing starting and stopping of the vimc streamer.

[Kaaira: only rebased the patch on current HEAD of media-tree]

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 .../media/test-drivers/vimc/vimc-streamer.c    | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index 6b5ea1537952..f5c9e2f3bbcb 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -269,22 +269,27 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 			   struct vimc_ent_device *ved,
 			   int enable)
 {
+	static DEFINE_MUTEX(vimc_streamer_lock);
 	int ret;
 
 	if (!stream || !ved)
 		return -EINVAL;
 
+	ret = mutex_lock_interruptible(&vimc_streamer_lock);
+	if (ret)
+		return ret;
+
 	if (enable) {
 		if (stream->kthread)
 			return 0;
 
 		ret = vimc_streamer_stream_start(ved);
 		if (ret)
-			return ret;
+			goto out;
 
 		ret = vimc_streamer_pipeline_init(stream, ved);
 		if (ret)
-			return ret;
+			goto out;
 
 		stream->kthread = kthread_run(vimc_streamer_thread, stream,
 					      "vimc-streamer thread");
@@ -294,17 +299,16 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
 			vimc_streamer_stream_terminate(ved);
 			vimc_streamer_pipeline_terminate(stream);
-			stream->kthread = NULL;
-			return ret;
 		}
 
 	} else {
 		if (!stream->kthread)
-			return 0;
+			goto out;
 
 		vimc_streamer_stream_terminate(ved);
 		vimc_streamer_pipeline_terminate(stream);
 	}
-
-	return 0;
+out:
+	mutex_unlock(&vimc_streamer_lock);
+	return ret;
 }
-- 
2.17.1

[PATCH v3 7/9] media: vimc: Dynamically allocate stream struct

[Kaaira Gupta]

Multiple streams will share same stream struct if we want them to run on
same thread. So remove it from vimc_cap struct so that it doesn't get
destroyed when one of the capture does, and allocate it memory
dynamically. Use kref with it as it will be used by multiple captures.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-capture.c  | 15 +++++++++++----
 drivers/media/test-drivers/vimc/vimc-streamer.c | 17 ++++++-----------
 drivers/media/test-drivers/vimc/vimc-streamer.h |  2 ++
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 93418cb5a139..73e5bdd17c57 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -28,7 +28,6 @@ struct vimc_cap_device {
 	spinlock_t qlock;
 	struct mutex lock;
 	u32 sequence;
-	struct vimc_stream stream;
 	struct media_pad pad;
 };
 
@@ -241,19 +240,25 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
 	struct media_entity *entity = &vcap->vdev.entity;
+	struct media_pipeline *pipe = NULL;
+	struct vimc_stream *stream;
 	int ret;
 
 	atomic_inc(&vcap->ved.use_count);
 	vcap->sequence = 0;
 
+	stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
+	kref_init(&stream->refcount);
+	pipe = &stream->pipe;
+
 	/* Start the media pipeline */
-	ret = media_pipeline_start(entity, &vcap->stream.pipe);
+	ret = media_pipeline_start(entity, pipe);
 	if (ret) {
 		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
 		return ret;
 	}
 
-	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
+	ret = vimc_streamer_s_stream(stream, &vcap->ved, 1);
 	if (ret) {
 		media_pipeline_stop(entity);
 		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
@@ -270,9 +275,11 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 static void vimc_cap_stop_streaming(struct vb2_queue *vq)
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
+	struct media_pipeline *pipe = vcap->ved.ent->pipe;
+	struct vimc_stream *stream = container_of(pipe, struct vimc_stream, pipe);
 
 	atomic_dec(&vcap->ved.use_count);
-	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
+	vimc_streamer_s_stream(stream, &vcap->ved, 0);
 
 	/* Stop the media pipeline */
 	media_pipeline_stop(&vcap->vdev.entity);
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index f5c9e2f3bbcb..fade37bee26d 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -20,18 +20,13 @@
  * Erases values of stream struct and terminates the thread
  *
  */
-static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
+static void vimc_streamer_pipeline_terminate(struct kref *ref)
 {
-	struct vimc_ent_device *ved;
-
-	while (stream->pipe_size) {
-		stream->pipe_size--;
-		ved = stream->ved_pipeline[stream->pipe_size];
-		stream->ved_pipeline[stream->pipe_size] = NULL;
-	}
+	struct vimc_stream *stream = container_of(ref, struct vimc_stream, refcount);
 
 	kthread_stop(stream->kthread);
 	stream->kthread = NULL;
+	kfree(stream);
 }
 
 /**
@@ -202,7 +197,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 	}
 
 	vimc_streamer_stream_terminate(cved);
-	vimc_streamer_pipeline_terminate(stream);
+	kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
 	return -EINVAL;
 }
 
@@ -298,7 +293,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 			ret = PTR_ERR(stream->kthread);
 			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
 			vimc_streamer_stream_terminate(ved);
-			vimc_streamer_pipeline_terminate(stream);
+			kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
 		}
 
 	} else {
@@ -306,7 +301,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 			goto out;
 
 		vimc_streamer_stream_terminate(ved);
-		vimc_streamer_pipeline_terminate(stream);
+		kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
 	}
 out:
 	mutex_unlock(&vimc_streamer_lock);
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.h b/drivers/media/test-drivers/vimc/vimc-streamer.h
index 3bb6731b8d4d..533c88675362 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.h
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.h
@@ -18,6 +18,7 @@
 /**
  * struct vimc_stream - struct that represents a stream in the pipeline
  *
+ * @refcount:		kref object associated with stream struct
  * @pipe:		the media pipeline object associated with this stream
  * @ved_pipeline:	array containing all the entities participating in the
  * 			stream. The order is from a video device (usually a
@@ -32,6 +33,7 @@
  * process frames for the stream.
  */
 struct vimc_stream {
+	struct kref refcount;
 	struct media_pipeline pipe;
 	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
 	unsigned int pipe_size;
-- 
2.17.1

[PATCH v3 8/9] media: vimc: Join pipeline if one already exists

[Kaaira Gupta]

Start another capture, if one is already running, by checking for
existing pipe. If it exists already, don't fail to start second capture,
instead join it to the already running pipeline.
Use the same stream struct used by already running capture.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-capture.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 73e5bdd17c57..4d20eda9335e 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -247,9 +247,15 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 	atomic_inc(&vcap->ved.use_count);
 	vcap->sequence = 0;
 
-	stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
-	kref_init(&stream->refcount);
-	pipe = &stream->pipe;
+	if (vcap->ved.ent->pipe) {
+		pipe = vcap->ved.ent->pipe;
+		stream = container_of(pipe, struct vimc_stream, pipe);
+		kref_get(&stream->refcount);
+	} else {
+		stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
+		kref_init(&stream->refcount);
+		pipe = &stream->pipe;
+	}
 
 	/* Start the media pipeline */
 	ret = media_pipeline_start(entity, pipe);
-- 
2.17.1

[PATCH v3 9/9] media: vimc: Run multiple captures on same thread

[Kaaira Gupta]

If multiple captures try to enable stream, start their stream but do not
initialise the pipeline again. Also, don't start the thread separately.
Starting their streams will update the use count and their frames would
be processed by the already running thread.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/media/test-drivers/vimc/vimc-streamer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index fade37bee26d..880c31759cc0 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -275,13 +275,14 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		return ret;
 
 	if (enable) {
-		if (stream->kthread)
-			return 0;
 
 		ret = vimc_streamer_stream_start(ved);
 		if (ret)
 			goto out;
 
+		if (stream->kthread)
+			goto out;
+
 		ret = vimc_streamer_pipeline_init(stream, ved);
 		if (ret)
 			goto out;
-- 
2.17.1

Re: [PATCH v3 1/9] media: vimc: Move get_source_entity to vimc-common

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> Move the function vimc_get_source_entity() to vimc-common.c to make it
> reusable.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  drivers/media/test-drivers/vimc/vimc-common.c | 14 +++++++++++
>  drivers/media/test-drivers/vimc/vimc-common.h | 12 ++++++++++
>  .../media/test-drivers/vimc/vimc-streamer.c   | 24 -------------------
>  3 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.c b/drivers/media/test-drivers/vimc/vimc-common.c
> index 0d97b25ce21e..91c8992bb391 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.c
> +++ b/drivers/media/test-drivers/vimc/vimc-common.c
> @@ -417,3 +417,17 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  	media_entity_cleanup(&sd->entity);
>  	return ret;
>  }
> +
> +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;
> +}
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> index a289434e75ba..4c580d854007 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -230,4 +230,16 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>   */
>  int vimc_vdev_link_validate(struct media_link *link);
>  
> +/**
> + * vimc_get_source_entity - get the entity connected with the first sink pad
> + *
> + * @ent:	reference media_entity
> + *
> + * Helper function that returns the media entity containing the source pad
> + * linked with the first sink pad from the given media entity pad list.
> + *
> + * Return: The source pad or NULL, if it wasn't found.
> + */
> +struct media_entity *vimc_get_source_entity(struct media_entity *ent);
> +
>  #endif
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index 451a32c0d034..4f8384246042 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -12,30 +12,6 @@
>  
>  #include "vimc-streamer.h"
>  
> -/**
> - * vimc_get_source_entity - get the entity connected with the first sink pad
> - *
> - * @ent:	reference media_entity
> - *
> - * Helper function that returns the media entity containing the source pad
> - * linked with the first sink pad from the given media entity pad list.
> - *
> - * Return: The source pad or NULL, if it wasn't found.
> - */
> -static 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;
> -}
> -
>  /**
>   * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
>   *
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 2/9] media: vimc: Add get_frame callback

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> In the process of making vimc compatible for multiple streams, we need
> to create a frame passing process such that two different entities can
> get the frame from a common entity. This isn't possible currently without
> calling process_frame twice for the common entity, as process_frames
> returns the frame which gets passed on.
> 
> So, to take care of this, add a get_frame callback to vimc device and
> use it to get the frames for an entity from previous entity instead of
> returning and passing the frames as an argument in process_frame.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  .../media/test-drivers/vimc/vimc-capture.c    | 18 +++++++++++++++---
>  drivers/media/test-drivers/vimc/vimc-common.h |  7 ++++---
>  .../media/test-drivers/vimc/vimc-debayer.c    | 19 ++++++++++++++++---
>  drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++++++++++++++---
>  drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +++++++++--
>  .../media/test-drivers/vimc/vimc-streamer.c   | 10 ++++++----
>  6 files changed, 65 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index c63496b17b9a..a8cbb8e4d5ba 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device *ved)
>  	video_unregister_device(&vcap->vdev);
>  }
>  
> -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> -				    const void *frame)
> +static int vimc_cap_process_frame(struct vimc_ent_device *ved)
>  {
>  	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>  						    ved);
>  	struct vimc_cap_buffer *vimc_buf;
> +	struct v4l2_subdev *sd;
> +	const void *frame;
>  	void *vbuf;
>  
>  	spin_lock(&vcap->qlock);
> @@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>  					    typeof(*vimc_buf), list);
>  	if (!vimc_buf) {
>  		spin_unlock(&vcap->qlock);
> -		return ERR_PTR(-EAGAIN);
> +		return -EAGAIN;
>  	}
>  
>  	/* Remove this entry from the list */
> @@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>  
>  	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
>  
> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> +	ved = v4l2_get_subdevdata(sd);
> +	frame = ved->get_frame(ved);

Hrm, this code block is used in several places throughout this patch,
and it aliases the function parameter ved to a new device which isn't
nice. Not a problem as long as it's not used for the original VED after
of course.

But I wonder if we should instead add a helper into vimc-common.c:

struct vimc_ent_device *vimc_get_source_ved(struct vimc_ent_device *ved)
{
	struct media_entity *ent;
	struct v4l2_subdev *sd;

	ent = vimc_get_source_entity(ved->ent);
	if (!ent)
		return NULL;

	sd = media_entity_to_v4l2_subdev(ent);

	return v4l2_get_subdevdata(sd);
}

It might not be necessary though, just an idea. If you like it, it can
be a patch on it's own after the vimc_get_source_entity() moving patch.


But it does show that vimc_get_source_entity() can return NULL which
might have to be checked... though perhaps we 'know' it will always be
valid ...

Also, following the links for each entity, for each frame sounds like
quite a lot of work. I wonder if the active source entity should be
cached in each VED ...

That could be done on top anyway...

Overall, this looks like it will work, so with comments addressed how
you wish,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +
>  	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);
> +
> +	return 0;
> +}
> +
> +static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
> +{
>  	return NULL;
>  }
>  
> @@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>  	vcap->ved.ent = &vcap->vdev.entity;
>  	vcap->ved.process_frame = vimc_cap_process_frame;
>  	vcap->ved.vdev_get_format = vimc_cap_get_format;
> +	vcap->ved.get_frame = vimc_cap_get_frame;
>  	vcap->ved.dev = vimc->mdev.dev;
>  
>  	/* Initialize the video_device struct */
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> index 4c580d854007..287d66edff49 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -85,7 +85,8 @@ struct vimc_pix_map {
>   *
>   * @dev:		a pointer of the device struct of the driver
>   * @ent:		the pointer to struct media_entity for the node
> - * @process_frame:	callback send a frame to that node
> + * @get_frame:		callback that sends a frame processed by the entity

s/sends a/obtains the/



> + * @process_frame:	callback that processes a frame
>   * @vdev_get_format:	callback that returns the current format a pad, used
>   *			only when is_media_entity_v4l2_video_device(ent) returns
>   *			true
> @@ -101,8 +102,8 @@ struct vimc_pix_map {
>  struct vimc_ent_device {
>  	struct device *dev;
>  	struct media_entity *ent;
> -	void * (*process_frame)(struct vimc_ent_device *ved,
> -				const void *frame);
> +	void * (*get_frame)(struct vimc_ent_device *ved);
> +	int (*process_frame)(struct vimc_ent_device *ved);
>  	void (*vdev_get_format)(struct vimc_ent_device *ved,
>  			      struct v4l2_pix_format *fmt);
>  };
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index c3f6fef34f68..f61e6e8899ac 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -491,17 +491,22 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
>  	}
>  }
>  
> -static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> -				    const void *sink_frame)
> +static int vimc_deb_process_frame(struct vimc_ent_device *ved)
>  {
>  	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>  						    ved);
>  	unsigned int rgb[3];
>  	unsigned int i, j;
> +	struct v4l2_subdev *sd;
> +	const void *sink_frame;
>  
>  	/* If the stream in this node is not active, just return */
>  	if (!vdeb->src_frame)
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
> +
> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> +	ved = v4l2_get_subdevdata(sd);
> +	sink_frame = ved->get_frame(ved);
>  
>  	for (i = 0; i < vdeb->sink_fmt.height; i++)
>  		for (j = 0; j < vdeb->sink_fmt.width; j++) {
> @@ -509,6 +514,13 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
>  			vdeb->set_rgb_src(vdeb, i, j, rgb);
>  		}
>  
> +	return 0;
> +}
> +
> +static void *vimc_deb_get_frame(struct vimc_ent_device *ved)
> +{
> +	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
> +						    ved);
>  	return vdeb->src_frame;
>  }
>  
> @@ -593,6 +605,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>  		goto err_free_hdl;
>  
>  	vdeb->ved.process_frame = vimc_deb_process_frame;
> +	vdeb->ved.get_frame = vimc_deb_get_frame;
>  	vdeb->ved.dev = vimc->mdev.dev;
>  	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
>  
> diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> index 121fa7d62a2e..347f9cd4a168 100644
> --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> +++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
> @@ -455,18 +455,29 @@ 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,
> -				    const void *sink_frame)
> +static int vimc_sca_process_frame(struct vimc_ent_device *ved)
>  {
>  	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>  						    ved);
> +	const void *sink_frame;
> +	struct v4l2_subdev *sd;
>  
>  	/* If the stream in this node is not active, just return */
>  	if (!vsca->src_frame)
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  
> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> +	ved = v4l2_get_subdevdata(sd);
> +	sink_frame = ved->get_frame(ved);
>  	vimc_sca_fill_src_frame(vsca, sink_frame);
>  
> +	return 0;
> +};
> +
> +static void *vimc_sca_get_frame(struct vimc_ent_device *ved)
> +{
> +	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> +						    ved);
>  	return vsca->src_frame;
>  };
>  
> @@ -505,6 +516,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>  	}
>  
>  	vsca->ved.process_frame = vimc_sca_process_frame;
> +	vsca->ved.get_frame = vimc_sca_get_frame;
>  	vsca->ved.dev = vimc->mdev.dev;
>  
>  	/* Initialize the frame format */
> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> index ba5db5a150b4..32a2c39de2cd 100644
> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> @@ -190,8 +190,7 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
>  	.set_fmt		= vimc_sen_set_fmt,
>  };
>  
> -static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> -				    const void *sink_frame)
> +static int vimc_sen_process_frame(struct vimc_ent_device *ved)
>  {
>  	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
>  						    ved);
> @@ -238,6 +237,13 @@ static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
>  		break;
>  	}
>  
> +	return 0;
> +}
> +
> +static void *vimc_sen_get_frame(struct vimc_ent_device *ved)
> +{
> +	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> +						    ved);
>  	return vsen->frame;
>  }
>  
> @@ -429,6 +435,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>  		goto err_free_tpg;
>  
>  	vsen->ved.process_frame = vimc_sen_process_frame;
> +	vsen->ved.get_frame = vimc_sen_get_frame;
>  	vsen->ved.dev = vimc->mdev.dev;
>  
>  	/* Initialize the frame format */
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index 4f8384246042..c1644d69686d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -125,7 +125,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  static int vimc_streamer_thread(void *data)
>  {
>  	struct vimc_stream *stream = data;
> -	u8 *frame = NULL;
> +	struct vimc_ent_device *ved;
> +	int ret;
>  	int i;
>  
>  	set_freezable();
> @@ -136,9 +137,10 @@ static int vimc_streamer_thread(void *data)
>  			break;
>  
>  		for (i = stream->pipe_size - 1; i >= 0; i--) {
> -			frame = stream->ved_pipeline[i]->process_frame(
> -					stream->ved_pipeline[i], frame);
> -			if (!frame || IS_ERR(frame))
> +			ved = stream->ved_pipeline[i];
> +			ret = ved->process_frame(ved);
> +
> +			if (ret)
>  				break;
>  		}
>  		//wait for 60hz
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Dafna Hirschfeld]

Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
> Separate the process of initialising pipeline array from starting
> streaming for all entities in path of a stream. This is needed because
> multiple streams can stream, but only one pipeline object is needed.
> 
> Process frames only for those entities in a pipeline which are
> streaming. This is known through their use counts.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>   .../media/test-drivers/vimc/vimc-streamer.c   | 95 ++++++++++++++++---
>   1 file changed, 83 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index c1644d69686d..cc40ecabe95a 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>   }
>   
>   /**
> - * vimc_streamer_pipeline_init - Initializes the stream structure
> + * vimc_streamer_stream_start - Starts streaming for all entities
> + * in a stream
>    *
> - * @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 vimc_streamer_s_stream() to enable stream in all entities of
> - * the pipeline.
> + * Walks through the entity graph to call vimc_streamer_s_stream()
> + * to enable stream in all entities in path of a stream.
>    *
>    * Return: 0 if success, error code otherwise.
>    */
> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> -				       struct vimc_ent_device *ved)
> +static int vimc_streamer_stream_start(struct vimc_stream *stream,
> +				      struct vimc_ent_device *ved)
>   {
>   	struct media_entity *entity;
>   	struct video_device *vdev;
>   	struct v4l2_subdev *sd;
> +	int stream_size = 0;
>   	int ret = 0;
>   
> -	stream->pipe_size = 0;
> -	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> +	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>   		if (!ved) {
>   			vimc_streamer_pipeline_terminate(stream);
>   			return -EINVAL;
>   		}
> -		stream->ved_pipeline[stream->pipe_size++] = ved;
>   
>   		if (is_media_entity_v4l2_subdev(ved->ent)) {
>   			sd = media_entity_to_v4l2_subdev(ved->ent);
> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>   					    entity);
>   			ved = video_get_drvdata(vdev);
>   		}
> +		stream_size++;
> +	}
> +
> +	vimc_streamer_pipeline_terminate(stream);
> +	return -EINVAL;
> +}
> +
> +/**
> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
> + *
> + * @stream: the pointer to the stream structure
> + * @ved:    the pointer to the vimc entity initializing the stream pipeline
> + *
> + * Walks through the entity graph to initialise ved_pipeline and updates
> + * pipe_size too.
> + *
> + * Return: 0 if success, error code otherwise.
> + */
> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> +				       struct vimc_ent_device *ved)
> +{
> +	struct media_entity *entity;
> +	struct media_device *mdev;
> +	struct media_graph graph;
> +	struct video_device *vdev;
> +	struct v4l2_subdev *sd;
> +	int ret;
> +
> +	entity = ved->ent;
> +	mdev = entity->graph_obj.mdev;
> +
> +	ret = media_graph_walk_init(&graph, mdev);
> +	if (ret)
> +		return ret;
> +
> +	media_graph_walk_start(&graph, entity);
> +
> +	/*
> +	 * Start pipeline array initialisation from RAW Capture only to get
> +	 * entities in the correct order of their frame processing.
> +	 */
> +	if (!strncmp(entity->name, "RGB", 3)) {

I don't understand this condition, way is it good for?

I think the function should be generic and not assume names of entities
or specific topology.


> +		entity = media_graph_walk_next(&graph);
> +		mdev = entity->graph_obj.mdev;
> +		media_graph_walk_cleanup(&graph);
> +
> +		ret = media_graph_walk_init(&graph, mdev);
> +		if (ret)
> +			return ret;
> +		media_graph_walk_start(&graph, entity);
> +	}
> +
> +	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> +		if (is_media_entity_v4l2_subdev(entity)) {
> +			sd = media_entity_to_v4l2_subdev(entity);
> +			ved = v4l2_get_subdevdata(sd);
> +		} else {
> +			vdev = container_of(entity, struct video_device, entity);
> +			ved = video_get_drvdata(vdev);
> +		}
> +		stream->ved_pipeline[stream->pipe_size++] = ved;
> +		entity = media_graph_walk_next(&graph);
> +
> +		if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {

I also don't understand this condition

> +			media_graph_walk_cleanup(&graph);
> +			return 0;
> +		}
>   	}

It is not clear what this function does, it looks like it adds to 'ved_pipeline'
all entities that are connected to the video node, in addition to the entities
that where there from previous calls, so some entities appear several times.

I think there is no need to use the graph walk here but to access the source entity
in each iteration, the way done in vimc_streamer_stream_start
also.
I think the code should iterate here until it reaches an entity that is already streaming,
this means that the entity is already in the `ved_pipeline`, also you should make sure
that the sensor is the first entity that process a frame, therefore the sensor should be
at the end/start of the list of entities. Generally each entity should appear exactly once
in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame'
on one entity should be after calling 'process_frame' on its source entity.
maybe it is easyer to implement if 'ved_pipeline' is a linked list.

Thanks,
Dafna

>   
>   	vimc_streamer_pipeline_terminate(stream);
> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
>   
>   		for (i = stream->pipe_size - 1; i >= 0; i--) {
>   			ved = stream->ved_pipeline[i];
> -			ret = ved->process_frame(ved);
>   
> +			if (atomic_read(&ved->use_count) == 0)
> +				continue;
> +
> +			ret = ved->process_frame(ved);
>   			if (ret)
>   				break;
>   		}
> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>   		if (stream->kthread)
>   			return 0;
>   
> +		ret = vimc_streamer_stream_start(stream, ved);
> +		if (ret)
> +			return ret;
> +
>   		ret = vimc_streamer_pipeline_init(stream, ved);
>   		if (ret)
>   			return ret;
> 

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Kaaira Gupta]

Hi,

On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
> 
> 
> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
> > Separate the process of initialising pipeline array from starting
> > streaming for all entities in path of a stream. This is needed because
> > multiple streams can stream, but only one pipeline object is needed.
> > 
> > Process frames only for those entities in a pipeline which are
> > streaming. This is known through their use counts.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >   .../media/test-drivers/vimc/vimc-streamer.c   | 95 ++++++++++++++++---
> >   1 file changed, 83 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index c1644d69686d..cc40ecabe95a 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> >   }
> >   /**
> > - * vimc_streamer_pipeline_init - Initializes the stream structure
> > + * vimc_streamer_stream_start - Starts streaming for all entities
> > + * in a stream
> >    *
> > - * @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 vimc_streamer_s_stream() to enable stream in all entities of
> > - * the pipeline.
> > + * Walks through the entity graph to call vimc_streamer_s_stream()
> > + * to enable stream in all entities in path of a stream.
> >    *
> >    * Return: 0 if success, error code otherwise.
> >    */
> > -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > -				       struct vimc_ent_device *ved)
> > +static int vimc_streamer_stream_start(struct vimc_stream *stream,
> > +				      struct vimc_ent_device *ved)
> >   {
> >   	struct media_entity *entity;
> >   	struct video_device *vdev;
> >   	struct v4l2_subdev *sd;
> > +	int stream_size = 0;
> >   	int ret = 0;
> > -	stream->pipe_size = 0;
> > -	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > +	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> >   		if (!ved) {
> >   			vimc_streamer_pipeline_terminate(stream);
> >   			return -EINVAL;
> >   		}
> > -		stream->ved_pipeline[stream->pipe_size++] = ved;
> >   		if (is_media_entity_v4l2_subdev(ved->ent)) {
> >   			sd = media_entity_to_v4l2_subdev(ved->ent);
> > @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >   					    entity);
> >   			ved = video_get_drvdata(vdev);
> >   		}
> > +		stream_size++;
> > +	}
> > +
> > +	vimc_streamer_pipeline_terminate(stream);
> > +	return -EINVAL;
> > +}
> > +
> > +/**
> > + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
> > + *
> > + * @stream: the pointer to the stream structure
> > + * @ved:    the pointer to the vimc entity initializing the stream pipeline
> > + *
> > + * Walks through the entity graph to initialise ved_pipeline and updates
> > + * pipe_size too.
> > + *
> > + * Return: 0 if success, error code otherwise.
> > + */
> > +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > +				       struct vimc_ent_device *ved)
> > +{
> > +	struct media_entity *entity;
> > +	struct media_device *mdev;
> > +	struct media_graph graph;
> > +	struct video_device *vdev;
> > +	struct v4l2_subdev *sd;
> > +	int ret;
> > +
> > +	entity = ved->ent;
> > +	mdev = entity->graph_obj.mdev;
> > +
> > +	ret = media_graph_walk_init(&graph, mdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	media_graph_walk_start(&graph, entity);
> > +
> > +	/*
> > +	 * Start pipeline array initialisation from RAW Capture only to get
> > +	 * entities in the correct order of their frame processing.
> > +	 */
> > +	if (!strncmp(entity->name, "RGB", 3)) {
> 
> I don't understand this condition, way is it good for?

I have explained that later in the reply

> 
> I think the function should be generic and not assume names of entities
> or specific topology.

It doesn't assume the topology, rather it is in place just to make sure
that the entities in ved_pipeline are in correct order.

> 
> 
> > +		entity = media_graph_walk_next(&graph);
> > +		mdev = entity->graph_obj.mdev;
> > +		media_graph_walk_cleanup(&graph);
> > +
> > +		ret = media_graph_walk_init(&graph, mdev);
> > +		if (ret)
> > +			return ret;
> > +		media_graph_walk_start(&graph, entity);
> > +	}
> > +
> > +	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > +		if (is_media_entity_v4l2_subdev(entity)) {
> > +			sd = media_entity_to_v4l2_subdev(entity);
> > +			ved = v4l2_get_subdevdata(sd);
> > +		} else {
> > +			vdev = container_of(entity, struct video_device, entity);
> > +			ved = video_get_drvdata(vdev);
> > +		}
> > +		stream->ved_pipeline[stream->pipe_size++] = ved;
> > +		entity = media_graph_walk_next(&graph);
> > +
> > +		if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
> 
> I also don't understand this condition
> 
> > +			media_graph_walk_cleanup(&graph);
> > +			return 0;
> > +		}
> >   	}
> 
> It is not clear what this function does, it looks like it adds to 'ved_pipeline'
> all entities that are connected to the video node, in addition to the entities
> that where there from previous calls, so some entities appear several times.

This function returns all the entities in a pipe, weather they are
streaming or not. For example, if only the RAW Capture 1 streams, or
RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
will have the same entities, in exactly same order, and all will occur just once.
Since media_graph_walk goes to each node of the graph, it returns back
to the first one (as its a graph), hence the last condition, ie,

	if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {

makes sure that the first entity is not added again to the array.

First condition, ie 

	if (!strncmp(entity->name, "RGB", 3)) {

Just makes sure that the search starts at RGB capture only. This is
because, in case of any other video node, the order of entities, as you
have mentioned later in the mail, will not be desirable, ie it won't
start at sensor and end at captures. So this condition just takes care
that all the entities in ved_pipeline array are in correct order
(starting at sensor, ending at captures).

Thanks,
Kaaira

> 
> I think there is no need to use the graph walk here but to access the source entity
> in each iteration, the way done in vimc_streamer_stream_start
> also.
> I think the code should iterate here until it reaches an entity that is already streaming,
> this means that the entity is already in the `ved_pipeline`, also you should make sure
> that the sensor is the first entity that process a frame, therefore the sensor should be
> at the end/start of the list of entities. Generally each entity should appear exactly once
> in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame'
> on one entity should be after calling 'process_frame' on its source entity.
> maybe it is easyer to implement if 'ved_pipeline' is a linked list.
> 
> Thanks,
> Dafna
> 
> >   	vimc_streamer_pipeline_terminate(stream);
> > @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
> >   		for (i = stream->pipe_size - 1; i >= 0; i--) {
> >   			ved = stream->ved_pipeline[i];
> > -			ret = ved->process_frame(ved);
> > +			if (atomic_read(&ved->use_count) == 0)
> > +				continue;
> > +
> > +			ret = ved->process_frame(ved);
> >   			if (ret)
> >   				break;
> >   		}
> > @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >   		if (stream->kthread)
> >   			return 0;
> > +		ret = vimc_streamer_stream_start(stream, ved);
> > +		if (ret)
> > +			return ret;
> > +
> >   		ret = vimc_streamer_pipeline_init(stream, ved);
> >   		if (ret)
> >   			return ret;
> > 

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Dafna Hirschfeld]

Hi,

Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
> Hi,
> 
> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
>>> Separate the process of initialising pipeline array from starting
>>> streaming for all entities in path of a stream. This is needed because
>>> multiple streams can stream, but only one pipeline object is needed.
>>>
>>> Process frames only for those entities in a pipeline which are
>>> streaming. This is known through their use counts.
>>>
>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
>>> ---
>>>    .../media/test-drivers/vimc/vimc-streamer.c   | 95 ++++++++++++++++---
>>>    1 file changed, 83 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>> index c1644d69686d..cc40ecabe95a 100644
>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>> @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>>>    }
>>>    /**
>>> - * vimc_streamer_pipeline_init - Initializes the stream structure
>>> + * vimc_streamer_stream_start - Starts streaming for all entities
>>> + * in a stream
>>>     *
>>> - * @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 vimc_streamer_s_stream() to enable stream in all entities of
>>> - * the pipeline.
>>> + * Walks through the entity graph to call vimc_streamer_s_stream()
>>> + * to enable stream in all entities in path of a stream.
>>>     *
>>>     * Return: 0 if success, error code otherwise.
>>>     */
>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>> -				       struct vimc_ent_device *ved)
>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream,
>>> +				      struct vimc_ent_device *ved)
>>>    {
>>>    	struct media_entity *entity;
>>>    	struct video_device *vdev;
>>>    	struct v4l2_subdev *sd;
>>> +	int stream_size = 0;
>>>    	int ret = 0;
>>> -	stream->pipe_size = 0;
>>> -	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>> +	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>    		if (!ved) {
>>>    			vimc_streamer_pipeline_terminate(stream);
>>>    			return -EINVAL;
>>>    		}
>>> -		stream->ved_pipeline[stream->pipe_size++] = ved;
>>>    		if (is_media_entity_v4l2_subdev(ved->ent)) {
>>>    			sd = media_entity_to_v4l2_subdev(ved->ent);
>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>>    					    entity);
>>>    			ved = video_get_drvdata(vdev);
>>>    		}
>>> +		stream_size++;
>>> +	}
>>> +
>>> +	vimc_streamer_pipeline_terminate(stream);
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +/**
>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
>>> + *
>>> + * @stream: the pointer to the stream structure
>>> + * @ved:    the pointer to the vimc entity initializing the stream pipeline
>>> + *
>>> + * Walks through the entity graph to initialise ved_pipeline and updates
>>> + * pipe_size too.
>>> + *
>>> + * Return: 0 if success, error code otherwise.
>>> + */
>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>> +				       struct vimc_ent_device *ved)
>>> +{
>>> +	struct media_entity *entity;
>>> +	struct media_device *mdev;
>>> +	struct media_graph graph;
>>> +	struct video_device *vdev;
>>> +	struct v4l2_subdev *sd;
>>> +	int ret;
>>> +
>>> +	entity = ved->ent;
>>> +	mdev = entity->graph_obj.mdev;
>>> +
>>> +	ret = media_graph_walk_init(&graph, mdev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	media_graph_walk_start(&graph, entity);
>>> +
>>> +	/*
>>> +	 * Start pipeline array initialisation from RAW Capture only to get
>>> +	 * entities in the correct order of their frame processing.
>>> +	 */
>>> +	if (!strncmp(entity->name, "RGB", 3)) {
>>
>> I don't understand this condition, way is it good for?
> 
> I have explained that later in the reply
> 
>>
>> I think the function should be generic and not assume names of entities
>> or specific topology.
> 
> It doesn't assume the topology, rather it is in place just to make sure
> that the entities in ved_pipeline are in correct order.
> 
>>
>>
>>> +		entity = media_graph_walk_next(&graph);
>>> +		mdev = entity->graph_obj.mdev;
>>> +		media_graph_walk_cleanup(&graph);
>>> +
>>> +		ret = media_graph_walk_init(&graph, mdev);
>>> +		if (ret)
>>> +			return ret;
>>> +		media_graph_walk_start(&graph, entity);

Hi, can you explain what this code does?
Why does it start the search in the next entity?

>>> +	}
>>> +
>>> +	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>> +		if (is_media_entity_v4l2_subdev(entity)) {
>>> +			sd = media_entity_to_v4l2_subdev(entity);
>>> +			ved = v4l2_get_subdevdata(sd);
>>> +		} else {
>>> +			vdev = container_of(entity, struct video_device, entity);
>>> +			ved = video_get_drvdata(vdev);
>>> +		}
>>> +		stream->ved_pipeline[stream->pipe_size++] = ved;
>>> +		entity = media_graph_walk_next(&graph);
>>> +
>>> +		if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
>>
>> I also don't understand this condition
>>
>>> +			media_graph_walk_cleanup(&graph);
>>> +			return 0;
>>> +		}
>>>    	}
>>
>> It is not clear what this function does, it looks like it adds to 'ved_pipeline'
>> all entities that are connected to the video node, in addition to the entities
>> that where there from previous calls, so some entities appear several times.
> 
> This function returns all the entities in a pipe, weather they are
> streaming or not. For example, if only the RAW Capture 1 streams, or
> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
> will have the same entities, in exactly same order, and all will occur just once.
> Since media_graph_walk goes to each node of the graph, it returns back
> to the first one (as its a graph), hence the last condition, ie,
> 
> 	if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
> 
> makes sure that the first entity is not added again to the array.
> 
> First condition, ie
> 
> 	if (!strncmp(entity->name, "RGB", 3)) {
> 
> Just makes sure that the search starts at RGB capture only. This is
> because, in case of any other video node, the order of entities, as you
> have mentioned later in the mail, will not be desirable, ie it won't
> start at sensor and end at captures. So this condition just takes care
> that all the entities in ved_pipeline array are in correct order
> (starting at sensor, ending at captures).

It is better to compare to the whole name of the entity to make it more clear.
Also I think it is better to document that this function is called only upon the
first streaming node.

I still think this function should be independent of the topology.
Maybe you can use Helen's patch that allow walking a graph only opposite to the link direction: https://patchwork.kernel.org/patch/11564899/
This ensures that the Sensor will be first in the graph walk. Then the streaming thread
iterates the ved_pipeline from 0 upwards and not from 'pipe_size' downwards.

Thanks,
Dafna



> 
> Thanks,
> Kaaira
> 
>>
>> I think there is no need to use the graph walk here but to access the source entity
>> in each iteration, the way done in vimc_streamer_stream_start
>> also.
>> I think the code should iterate here until it reaches an entity that is already streaming,
>> this means that the entity is already in the `ved_pipeline`, also you should make sure
>> that the sensor is the first entity that process a frame, therefore the sensor should be
>> at the end/start of the list of entities. Generally each entity should appear exactly once
>> in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame'
>> on one entity should be after calling 'process_frame' on its source entity.
>> maybe it is easyer to implement if 'ved_pipeline' is a linked list.
>>
>> Thanks,
>> Dafna
>>
>>>    	vimc_streamer_pipeline_terminate(stream);
>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
>>>    		for (i = stream->pipe_size - 1; i >= 0; i--) {
>>>    			ved = stream->ved_pipeline[i];
>>> -			ret = ved->process_frame(ved);
>>> +			if (atomic_read(&ved->use_count) == 0)
>>> +				continue;
>>> +
>>> +			ret = ved->process_frame(ved);
>>>    			if (ret)
>>>    				break;
>>>    		}
>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>>>    		if (stream->kthread)
>>>    			return 0;
>>> +		ret = vimc_streamer_stream_start(stream, ved);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>>    		ret = vimc_streamer_pipeline_init(stream, ved);
>>>    		if (ret)
>>>    			return ret;
>>>

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Kieran Bingham]

Hi Kaaira, Dafna,

On 28/08/2020 21:37, Dafna Hirschfeld wrote:
> Hi,
> 
> Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
>> Hi,
>>
>> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
>>>
>>>
>>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
>>>> Separate the process of initialising pipeline array from starting
>>>> streaming for all entities in path of a stream. This is needed because
>>>> multiple streams can stream, but only one pipeline object is needed.
>>>>
>>>> Process frames only for those entities in a pipeline which are
>>>> streaming. This is known through their use counts.
>>>>
>>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
>>>> ---
>>>>    .../media/test-drivers/vimc/vimc-streamer.c   | 95
>>>> ++++++++++++++++---
>>>>    1 file changed, 83 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>> b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>> index c1644d69686d..cc40ecabe95a 100644
>>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>> @@ -40,33 +40,30 @@ static void
>>>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>>>>    }
>>>>    /**
>>>> - * vimc_streamer_pipeline_init - Initializes the stream structure
>>>> + * vimc_streamer_stream_start - Starts streaming for all entities
>>>> + * in a stream
>>>>     *
>>>> - * @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 vimc_streamer_s_stream() to enable stream in all entities of
>>>> - * the pipeline.
>>>> + * Walks through the entity graph to call vimc_streamer_s_stream()
>>>> + * to enable stream in all entities in path of a stream.
>>>>     *
>>>>     * Return: 0 if success, error code otherwise.
>>>>     */
>>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>>> -                       struct vimc_ent_device *ved)
>>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream,
>>>> +                      struct vimc_ent_device *ved)
>>>>    {
>>>>        struct media_entity *entity;
>>>>        struct video_device *vdev;
>>>>        struct v4l2_subdev *sd;
>>>> +    int stream_size = 0;
>>>>        int ret = 0;
>>>> -    stream->pipe_size = 0;
>>>> -    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>> +    while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>>            if (!ved) {
>>>>                vimc_streamer_pipeline_terminate(stream);
>>>>                return -EINVAL;
>>>>            }
>>>> -        stream->ved_pipeline[stream->pipe_size++] = ved;
>>>>            if (is_media_entity_v4l2_subdev(ved->ent)) {
>>>>                sd = media_entity_to_v4l2_subdev(ved->ent);
>>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct
>>>> vimc_stream *stream,
>>>>                            entity);
>>>>                ved = video_get_drvdata(vdev);
>>>>            }
>>>> +        stream_size++;
>>>> +    }
>>>> +
>>>> +    vimc_streamer_pipeline_terminate(stream);
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
>>>> + *
>>>> + * @stream: the pointer to the stream structure
>>>> + * @ved:    the pointer to the vimc entity initializing the stream
>>>> pipeline

Which entity is this? Is it the start, or the end of the pipeline? I.e.
the sensor? or the capture ?

>>>> + *
>>>> + * Walks through the entity graph to initialise ved_pipeline and
>>>> updates
>>>> + * pipe_size too.
>>>> + *
>>>> + * Return: 0 if success, error code otherwise.
>>>> + */
>>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>>> +                       struct vimc_ent_device *ved)
>>>> +{
>>>> +    struct media_entity *entity;
>>>> +    struct media_device *mdev;
>>>> +    struct media_graph graph;
>>>> +    struct video_device *vdev;
>>>> +    struct v4l2_subdev *sd;
>>>> +    int ret;
>>>> +
>>>> +    entity = ved->ent;
>>>> +    mdev = entity->graph_obj.mdev;
>>>> +
>>>> +    ret = media_graph_walk_init(&graph, mdev);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    media_graph_walk_start(&graph, entity);
>>>> +
>>>> +    /*
>>>> +     * Start pipeline array initialisation from RAW Capture only to
>>>> get
>>>> +     * entities in the correct order of their frame processing.
>>>> +     */
>>>> +    if (!strncmp(entity->name, "RGB", 3)) {
>>>
>>> I don't understand this condition, way is it good for?
>>
>> I have explained that later in the reply

Matching on entity names is a bit awkward, as they could be changed I
guess quite easily, and easily missed in this string matching.

I haven't fully understood this code block yet to work out if there's
another way we could do this though, but reading ahead I see there might
be a way to 'walk the graph' on a per-stream basis which might be a good
way of factoring this path.

Although there still needs to be a construction of the paths available
to a stream which splits from the sensor.


>>
>>>
>>> I think the function should be generic and not assume names of entities
>>> or specific topology.
>>
>> It doesn't assume the topology, rather it is in place just to make sure
>> that the entities in ved_pipeline are in correct order.
>>
>>>
>>>
>>>> +        entity = media_graph_walk_next(&graph);
>>>> +        mdev = entity->graph_obj.mdev;
>>>> +        media_graph_walk_cleanup(&graph);
>>>> +
>>>> +        ret = media_graph_walk_init(&graph, mdev);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +        media_graph_walk_start(&graph, entity);
> 
> Hi, can you explain what this code does?
> Why does it start the search in the next entity?
> 
>>>> +    }
>>>> +
>>>> +    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>> +        if (is_media_entity_v4l2_subdev(entity)) {
>>>> +            sd = media_entity_to_v4l2_subdev(entity);
>>>> +            ved = v4l2_get_subdevdata(sd);
>>>> +        } else {
>>>> +            vdev = container_of(entity, struct video_device, entity);
>>>> +            ved = video_get_drvdata(vdev);
>>>> +        }
>>>> +        stream->ved_pipeline[stream->pipe_size++] = ved;
>>>> +        entity = media_graph_walk_next(&graph);
>>>> +
>>>> +        if (!strcmp(entity->name,
>>>> stream->ved_pipeline[0]->ent->name)) {
>>>
>>> I also don't understand this condition
>>>
>>>> +            media_graph_walk_cleanup(&graph);
>>>> +            return 0;
>>>> +        }
>>>>        }
>>>
>>> It is not clear what this function does, it looks like it adds to
>>> 'ved_pipeline'
>>> all entities that are connected to the video node, in addition to the
>>> entities
>>> that where there from previous calls, so some entities appear several
>>> times.
>>
>> This function returns all the entities in a pipe, weather they are
>> streaming or not. For example, if only the RAW Capture 1 streams, or
>> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
>> will have the same entities, in exactly same order, and all will occur
>> just once.
>> Since media_graph_walk goes to each node of the graph, it returns back
>> to the first one (as its a graph), hence the last condition, ie,
>>
>>     if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
>>
>> makes sure that the first entity is not added again to the array.
>>
>> First condition, ie
>>
>>     if (!strncmp(entity->name, "RGB", 3)) {
>>
>> Just makes sure that the search starts at RGB capture only. This is
>> because, in case of any other video node, the order of entities, as you
>> have mentioned later in the mail, will not be desirable, ie it won't
>> start at sensor and end at captures. So this condition just takes care
>> that all the entities in ved_pipeline array are in correct order
>> (starting at sensor, ending at captures).
> 
> It is better to compare to the whole name of the entity to make it more
> clear.
> Also I think it is better to document that this function is called only
> upon the
> first streaming node.

If this doesn't end up refactored with other helpers, then indeed a few
comments might help the readabilty here. The distinctions of each
re-initialisation of the graph walk are hard to interpret the purpose.



> 
> I still think this function should be independent of the topology.
> Maybe you can use Helen's patch that allow walking a graph only opposite
> to the link direction: https://patchwork.kernel.org/patch/11564899/
> This ensures that the Sensor will be first in the graph walk. Then the
> streaming thread
> iterates the ved_pipeline from 0 upwards and not from 'pipe_size'
> downwards.

Being able to use a direct helper to walk the pipeline cleanly sounds
promising indeed.

I suspect at some point int he next patches though - I'm going to see
something that needs to have full visibility of all paths enabled from
the sensor, as I think I recall that the thread will then process all
(enabled) entities in a single pass.
--
Kieran


> 
> Thanks,
> Dafna
> 
> 
> 
>>
>> Thanks,
>> Kaaira
>>
>>>
>>> I think there is no need to use the graph walk here but to access the
>>> source entity
>>> in each iteration, the way done in vimc_streamer_stream_start
>>> also.
>>> I think the code should iterate here until it reaches an entity that
>>> is already streaming,
>>> this means that the entity is already in the `ved_pipeline`, also you
>>> should make sure
>>> that the sensor is the first entity that process a frame, therefore
>>> the sensor should be
>>> at the end/start of the list of entities. Generally each entity
>>> should appear exactly once
>>> in the 'ved_pipeline' array and the entities should be ordered such
>>> that when calling 'process_frame'
>>> on one entity should be after calling 'process_frame' on its source
>>> entity.
>>> maybe it is easyer to implement if 'ved_pipeline' is a linked list.
>>>
>>> Thanks,
>>> Dafna
>>>
>>>>        vimc_streamer_pipeline_terminate(stream);
>>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
>>>>            for (i = stream->pipe_size - 1; i >= 0; i--) {
>>>>                ved = stream->ved_pipeline[i];
>>>> -            ret = ved->process_frame(ved);
>>>> +            if (atomic_read(&ved->use_count) == 0)
>>>> +                continue;
>>>> +
>>>> +            ret = ved->process_frame(ved);
>>>>                if (ret)
>>>>                    break;
>>>>            }
>>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream
>>>> *stream,
>>>>            if (stream->kthread)
>>>>                return 0;
>>>> +        ret = vimc_streamer_stream_start(stream, ved);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +
>>>>            ret = vimc_streamer_pipeline_init(stream, ved);
>>>>            if (ret)
>>>>                return ret;
>>>>

-- 
Regards
--
Kieran

Re: [PATCH v3 5/9] media: vimc: Separate closing of stream and thread

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> Make separate functions for stopping streaming of entities in path of a
> particular stream and stopping thread. This is needed to ensure that
> thread doesn't stop when one device stops streaming in case of multiple
> streams.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  .../media/test-drivers/vimc/vimc-streamer.c   | 82 ++++++++++++-------
>  1 file changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index cc40ecabe95a..6b5ea1537952 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -13,29 +13,59 @@
>  #include "vimc-streamer.h"
>  
>  /**
> - * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
> + * vimc_streamer_pipeline_terminate - Terminate the thread
>   *
> - * @stream: the pointer to the stream structure with the pipeline to be
> - *	    disabled.
> + * @stream: the pointer to the stream structure
>   *
> - * Calls s_stream to disable the stream in each entity of the pipeline
> + * Erases values of stream struct and terminates the thread

It would help if the function brief described it's purpose rather than
'what it does'. "Erases values of stream struct" is not helpful here, as
that's just a direct read of what happens in the code.

I'm guessing here, but how about:

"Tear down all resources belonging to the pipeline when there are no
longer any active streams being used. This includes stopping the active
processing thread"


But reading my text makes me worry about the ordering that might take
place. The thread should be stopped as soon as the last stream using it
is stopped. Presumably as the 'first thing' that happens to make sure
there is no concurrent processing while the stream is being disabled.

Hopefully there's no race condition ...


>   *
>   */
>  static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>  {
>  	struct vimc_ent_device *ved;
> -	struct v4l2_subdev *sd;
>  
>  	while (stream->pipe_size) {
>  		stream->pipe_size--;
>  		ved = stream->ved_pipeline[stream->pipe_size];
>  		stream->ved_pipeline[stream->pipe_size] = NULL;
> +	}
>  
> -		if (!is_media_entity_v4l2_subdev(ved->ent))
> -			continue;
> +	kthread_stop(stream->kthread);
> +	stream->kthread = NULL;
> +}
>  
> -		sd = media_entity_to_v4l2_subdev(ved->ent);
> -		v4l2_subdev_call(sd, video, s_stream, 0);
> +/**
> + * vimc_streamer_stream_terminate - Disable stream in all ved in stream
> + *
> + * @ved: pointer to the ved for which stream needs to be disabled
> + *
> + * Calls s_stream to disable the stream in each entity of the stream
> + *
> + */
> +static void vimc_streamer_stream_terminate(struct vimc_ent_device *ved)

I would call this vimc_streamer_stream_stop to match
vimc_streamer_stream_start() rather than terminate...

> +{
> +	struct media_entity *entity = ved->ent;
> +	struct video_device *vdev;
> +	struct v4l2_subdev *sd;
> +
> +	while (entity) {
> +		if (is_media_entity_v4l2_subdev(ved->ent)) {
> +			sd = media_entity_to_v4l2_subdev(ved->ent);
> +			v4l2_subdev_call(sd, video, s_stream, 0);
> +		}
> +		entity = vimc_get_source_entity(ved->ent);
> +		if (!entity)
> +			break;
> +
> +		if (is_media_entity_v4l2_subdev(entity)) {
> +			sd = media_entity_to_v4l2_subdev(entity);
> +			ved = v4l2_get_subdevdata(sd);
> +		} else {
> +			vdev = container_of(entity,
> +					    struct video_device,
> +					    entity);
> +			ved = video_get_drvdata(vdev);
> +		}

It looks like this is walking back through the linked graph, calling
s_stream(0) right?

I wonder if struct vimc_ent_device should have a pointer to the entity
it's connected to, to simplify this. ... presumably this is done
elsewhere too?

Although then that's still walking 'backwards' rather than forwards...




>  	}
>  }
>  
> @@ -43,25 +73,25 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>   * vimc_streamer_stream_start - Starts streaming for all entities
>   * in a stream
>   *
> - * @ved:    the pointer to the vimc entity initializing the stream
> + * @cved:    the pointer to the vimc entity initializing the stream
>   *
>   * Walks through the entity graph to call vimc_streamer_s_stream()
>   * to enable stream in all entities in path of a stream.
>   *
>   * Return: 0 if success, error code otherwise.
>   */
> -static int vimc_streamer_stream_start(struct vimc_stream *stream,
> -				      struct vimc_ent_device *ved)
> +static int vimc_streamer_stream_start(struct vimc_ent_device *cved)
>  {
>  	struct media_entity *entity;
>  	struct video_device *vdev;
>  	struct v4l2_subdev *sd;
> +	struct vimc_ent_device *ved = cved;
>  	int stream_size = 0;
>  	int ret = 0;
>  
>  	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>  		if (!ved) {
> -			vimc_streamer_pipeline_terminate(stream);
> +			vimc_streamer_stream_terminate(cved);
>  			return -EINVAL;
>  		}
>  
> @@ -71,7 +101,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
>  			if (ret && ret != -ENOIOCTLCMD) {
>  				dev_err(ved->dev, "subdev_call error %s\n",
>  					ved->ent->name);
> -				vimc_streamer_pipeline_terminate(stream);
> +				vimc_streamer_stream_terminate(cved);
>  				return ret;
>  			}
>  		}
> @@ -84,7 +114,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
>  				dev_err(ved->dev,
>  					"first entity in the pipe '%s' is not a source\n",
>  					ved->ent->name);
> -				vimc_streamer_pipeline_terminate(stream);
> +				vimc_streamer_stream_terminate(cved);
>  				pr_info ("first entry not source");
>  				return -EPIPE;
>  			}
> @@ -104,7 +134,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
>  		stream_size++;
>  	}
>  
> -	vimc_streamer_pipeline_terminate(stream);
> +	vimc_streamer_stream_terminate(cved);
>  	return -EINVAL;
>  }
>  
> @@ -120,13 +150,14 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
>   * Return: 0 if success, error code otherwise.
>   */
>  static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> -				       struct vimc_ent_device *ved)
> +				       struct vimc_ent_device *cved)
>  {
>  	struct media_entity *entity;
>  	struct media_device *mdev;
>  	struct media_graph graph;
>  	struct video_device *vdev;
>  	struct v4l2_subdev *sd;
> +	struct vimc_ent_device *ved = cved;
>  	int ret;
>  
>  	entity = ved->ent;
> @@ -170,6 +201,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  		}
>  	}
>  
> +	vimc_streamer_stream_terminate(cved);
>  	vimc_streamer_pipeline_terminate(stream);
>  	return -EINVAL;
>  }
> @@ -246,7 +278,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  		if (stream->kthread)
>  			return 0;
>  
> -		ret = vimc_streamer_stream_start(stream, ved);
> +		ret = vimc_streamer_stream_start(ved);
>  		if (ret)
>  			return ret;
>  
> @@ -260,6 +292,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  		if (IS_ERR(stream->kthread)) {
>  			ret = PTR_ERR(stream->kthread);
>  			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
> +			vimc_streamer_stream_terminate(ved);
>  			vimc_streamer_pipeline_terminate(stream);
>  			stream->kthread = NULL;

If vimc_streamer_pipeline_terminate() sets stream->kthread = NULL, it
doesn't need to be done again here.


>  			return ret;
> @@ -269,18 +302,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  		if (!stream->kthread)
>  			return 0;
>  
> -		ret = kthread_stop(stream->kthread);
> -		/*
> -		 * kthread_stop returns -EINTR in cases when streamon was
> -		 * immediately followed by streamoff, and the thread didn't had
> -		 * a chance to run. Ignore errors to stop the stream in the
> -		 * pipeline.
> -		 */

Do we need to retain that comment when stopping the thread?

> -		if (ret)
> -			dev_dbg(ved->dev, "kthread_stop returned '%d'\n", ret);
> -
> -		stream->kthread = NULL;
> -
> +		vimc_streamer_stream_terminate(ved);
>  		vimc_streamer_pipeline_terminate(stream);
>  	}
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> Multiple streams will share same stream struct if we want them to run on
> same thread. So remove it from vimc_cap struct so that it doesn't get
> destroyed when one of the capture does, and allocate it memory
> dynamically. Use kref with it as it will be used by multiple captures.
> 

Is the vimc_stream stuct the context of the streamer? or of each 'stream'?

If it's the streamer - then can't we store this (non-dynamically) as
part of the Sensor node, to avoid kzalloc/freeing it ?


> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/media/test-drivers/vimc/vimc-capture.c  | 15 +++++++++++----
>  drivers/media/test-drivers/vimc/vimc-streamer.c | 17 ++++++-----------
>  drivers/media/test-drivers/vimc/vimc-streamer.h |  2 ++
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 93418cb5a139..73e5bdd17c57 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -28,7 +28,6 @@ struct vimc_cap_device {
>  	spinlock_t qlock;
>  	struct mutex lock;
>  	u32 sequence;
> -	struct vimc_stream stream;
>  	struct media_pad pad;
>  };
>  
> @@ -241,19 +240,25 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>  	struct media_entity *entity = &vcap->vdev.entity;
> +	struct media_pipeline *pipe = NULL;
> +	struct vimc_stream *stream;
>  	int ret;
>  
>  	atomic_inc(&vcap->ved.use_count);
>  	vcap->sequence = 0;
>  
> +	stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> +	kref_init(&stream->refcount);
> +	pipe = &stream->pipe;
> +
>  	/* Start the media pipeline */
> -	ret = media_pipeline_start(entity, &vcap->stream.pipe);
> +	ret = media_pipeline_start(entity, pipe);
>  	if (ret) {
>  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>  		return ret;
>  	}
>  
> -	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
> +	ret = vimc_streamer_s_stream(stream, &vcap->ved, 1);
>  	if (ret) {
>  		media_pipeline_stop(entity);
>  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> @@ -270,9 +275,11 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  static void vimc_cap_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> +	struct media_pipeline *pipe = vcap->ved.ent->pipe;
> +	struct vimc_stream *stream = container_of(pipe, struct vimc_stream, pipe);

In fact, I see it's stored as part of the 'pipe' so there is one
vimc_stream per pipeline ...

So it could just be a structure on the pipe rather than obtaining a
pointer here.

I think it's probably 'ok' to have one streamer per pipe currently as
the raw input node is not functional, but I also thought that by having
the streamer as part of the sensor entity then there is one streamer
('one thread') per video source ... which would prevent this having to
be changed if someone later deals with the video node that allows
re-processing raw frames ?



>  	atomic_dec(&vcap->ved.use_count);
> -	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
> +	vimc_streamer_s_stream(stream, &vcap->ved, 0);
>  
>  	/* Stop the media pipeline */
>  	media_pipeline_stop(&vcap->vdev.entity);
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index f5c9e2f3bbcb..fade37bee26d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -20,18 +20,13 @@
>   * Erases values of stream struct and terminates the thread
>   *
>   */
> -static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> +static void vimc_streamer_pipeline_terminate(struct kref *ref)
>  {
> -	struct vimc_ent_device *ved;
> -
> -	while (stream->pipe_size) {
> -		stream->pipe_size--;
> -		ved = stream->ved_pipeline[stream->pipe_size];
> -		stream->ved_pipeline[stream->pipe_size] = NULL;
> -	}
> +	struct vimc_stream *stream = container_of(ref, struct vimc_stream, refcount);
>  
>  	kthread_stop(stream->kthread);
>  	stream->kthread = NULL;
> +	kfree(stream);
>  }
>  
>  /**
> @@ -202,7 +197,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  	}
>  
>  	vimc_streamer_stream_terminate(cved);
> -	vimc_streamer_pipeline_terminate(stream);
> +	kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
>  	return -EINVAL;
>  }
>  
> @@ -298,7 +293,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  			ret = PTR_ERR(stream->kthread);
>  			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
>  			vimc_streamer_stream_terminate(ved);
> -			vimc_streamer_pipeline_terminate(stream);
> +			kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
>  		}
>  
>  	} else {
> @@ -306,7 +301,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  			goto out;
>  
>  		vimc_streamer_stream_terminate(ved);
> -		vimc_streamer_pipeline_terminate(stream);
> +		kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
>  	}
>  out:
>  	mutex_unlock(&vimc_streamer_lock);
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.h b/drivers/media/test-drivers/vimc/vimc-streamer.h
> index 3bb6731b8d4d..533c88675362 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.h
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.h
> @@ -18,6 +18,7 @@
>  /**
>   * struct vimc_stream - struct that represents a stream in the pipeline
>   *
> + * @refcount:		kref object associated with stream struct

What does it track though?

We know it's associated with the stream struct because it's in the
vimc_stream struct.



>   * @pipe:		the media pipeline object associated with this stream
>   * @ved_pipeline:	array containing all the entities participating in the
>   * 			stream. The order is from a video device (usually a

The fact that this comment still says "all entities participating in the
stream" worries me a little, as I think now the Streamer is dealing with
multiple streams.

I think we need to be really clear with the differences of objects and
terminology.

For instance I think the current terms are something like this:

Streamer: Responsible for managing processing from a sensor device
through all entities.

Stream: A flow of frames to a single capture video device node.

Pipeline: All entities used within the vimc streamer ?

(I'm not sure if those are right, I'm writing down what my current
interpretations are, so if someone wants to/can clarify - please do).



> @@ -32,6 +33,7 @@
>   * process frames for the stream.
>   */
>  struct vimc_stream {
> +	struct kref refcount;
>  	struct media_pipeline pipe;
>  	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
>  	unsigned int pipe_size;
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 8/9] media: vimc: Join pipeline if one already exists

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> Start another capture, if one is already running, by checking for
> existing pipe. If it exists already, don't fail to start second capture,
> instead join it to the already running pipeline.
> Use the same stream struct used by already running capture.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/media/test-drivers/vimc/vimc-capture.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 73e5bdd17c57..4d20eda9335e 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -247,9 +247,15 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	atomic_inc(&vcap->ved.use_count);
>  	vcap->sequence = 0;
>  
> -	stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> -	kref_init(&stream->refcount);
> -	pipe = &stream->pipe;
> +	if (vcap->ved.ent->pipe) {
> +		pipe = vcap->ved.ent->pipe;
> +		stream = container_of(pipe, struct vimc_stream, pipe);
> +		kref_get(&stream->refcount);
> +	} else {
> +		stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> +		kref_init(&stream->refcount);
> +		pipe = &stream->pipe;
> +	}
>  

Of course if we move the stream to the sensor entity (which I still
think is a good idea), we'll need a way to easily get from capture
entity to the sensor entity. We could put a reference pointer directly
in the capture ? Or have each entity provide a poitner to it's previous
entity and walk backwards, which would also help on the other code path
that had to do lots of conversions from video or subdev entities or such...


>  	/* Start the media pipeline */
>  	ret = media_pipeline_start(entity, pipe);
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 9/9] media: vimc: Run multiple captures on same thread

[Kieran Bingham]

Hi Kaaira,

On 19/08/2020 19:04, Kaaira Gupta wrote:
> If multiple captures try to enable stream, start their stream but do not
> initialise the pipeline again. Also, don't start the thread separately.
> Starting their streams will update the use count and their frames would
> be processed by the already running thread.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/media/test-drivers/vimc/vimc-streamer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index fade37bee26d..880c31759cc0 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -275,13 +275,14 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>  		return ret;
>  
>  	if (enable) {
> -		if (stream->kthread)
> -			return 0;
>  
>  		ret = vimc_streamer_stream_start(ved);
>  		if (ret)
>  			goto out;
>  
> +		if (stream->kthread)
> +			goto out;
> +

This goto out makes it look like it's an error path. So that probably
warrants a comment along the lines of 'don't reinitialise the pipeline
if it has already started'. ?

I wonder if it's better to handle the pipeline_init during _stream_start
'only' in the code path where it's the first stream ?

Then similarly, the pipeline_terminate would happen on stream_stop
'only' when it's the last stream.

Or I guess that is handled by the refcount ... Hrm it would be nice to
be able to make/keep it symmetrical somehow.


>  		ret = vimc_streamer_pipeline_init(stream, ved);
>  		if (ret)
>  			goto out;
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 0/9] media: vimc: Multiple stream support in vimc

[Kieran Bingham]

Hi Kaaira,

Thank you for this series.

I have tested it, and indeed it works well, which is great work.
I know this has been hard to debug some of the code paths!

There are a few bits that are hard for me to understand, with the graph
walking/initialisation - so I think either some better comments or
refactoring might help there - and Dafna has suggested that there might
be a useful helper which will assist too. That needs to be checked
though, as I think your 'streamer' needs to have full visibility of the
whole pipeline, rather than just a single stream.

I wonder if it would help to rename it to make that clearer, as
'vimc_stream' sounds like it deals with only a single stream - but now
it deals with multiple - so the naming is a bit confusing.

--
Kieran

On 19/08/2020 19:04, Kaaira Gupta wrote:
> This series adds supoort for two (or more) capture devices to be 
> connected to the same sensors and run simultaneously.
> 
> Changes since v2:
> 	- This series introduces new patches, namely patch 1, 2, 4, 5,
> 	  7, and 9 to shift multiple captures to operate at a single
> 	  thread.
> 
> Kaaira Gupta (7):
>   media: vimc: Move get_source_entity to vimc-common
>   media: vimc: Add get_frame callback
>   media: vimc: Separate starting stream from pipeline initialisation
>   media: vimc: Separate closing of stream and thread
>   media: vimc: Dynamically allocate stream struct
>   media: vimc: Join pipeline if one already exists
>   media: vimc: Run multiple captures on same thread
> 
> Niklas Söderlund (2):
>   media: vimc: Add usage count to subdevices
>   media: vimc: Serialize vimc_streamer_s_stream()
> 
>  .../media/test-drivers/vimc/vimc-capture.c    |  42 +++-
>  drivers/media/test-drivers/vimc/vimc-common.c |  14 ++
>  drivers/media/test-drivers/vimc/vimc-common.h |  21 +-
>  .../media/test-drivers/vimc/vimc-debayer.c    |  26 ++-
>  drivers/media/test-drivers/vimc/vimc-scaler.c |  25 +-
>  drivers/media/test-drivers/vimc/vimc-sensor.c |  17 +-
>  .../media/test-drivers/vimc/vimc-streamer.c   | 213 ++++++++++++------
>  .../media/test-drivers/vimc/vimc-streamer.h   |   2 +
>  8 files changed, 271 insertions(+), 89 deletions(-)
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 2/9] media: vimc: Add get_frame callback

[Kaaira Gupta]

On Thu, Aug 20, 2020 at 04:36:56PM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > In the process of making vimc compatible for multiple streams, we need
> > to create a frame passing process such that two different entities can
> > get the frame from a common entity. This isn't possible currently without
> > calling process_frame twice for the common entity, as process_frames
> > returns the frame which gets passed on.
> > 
> > So, to take care of this, add a get_frame callback to vimc device and
> > use it to get the frames for an entity from previous entity instead of
> > returning and passing the frames as an argument in process_frame.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  .../media/test-drivers/vimc/vimc-capture.c    | 18 +++++++++++++++---
> >  drivers/media/test-drivers/vimc/vimc-common.h |  7 ++++---
> >  .../media/test-drivers/vimc/vimc-debayer.c    | 19 ++++++++++++++++---
> >  drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++++++++++++++---
> >  drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +++++++++--
> >  .../media/test-drivers/vimc/vimc-streamer.c   | 10 ++++++----
> >  6 files changed, 65 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index c63496b17b9a..a8cbb8e4d5ba 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device *ved)
> >  	video_unregister_device(&vcap->vdev);
> >  }
> >  
> > -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> > -				    const void *frame)
> > +static int vimc_cap_process_frame(struct vimc_ent_device *ved)
> >  {
> >  	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
> >  						    ved);
> >  	struct vimc_cap_buffer *vimc_buf;
> > +	struct v4l2_subdev *sd;
> > +	const void *frame;
> >  	void *vbuf;
> >  
> >  	spin_lock(&vcap->qlock);
> > @@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> >  					    typeof(*vimc_buf), list);
> >  	if (!vimc_buf) {
> >  		spin_unlock(&vcap->qlock);
> > -		return ERR_PTR(-EAGAIN);
> > +		return -EAGAIN;
> >  	}
> >  
> >  	/* Remove this entry from the list */
> > @@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> >  
> >  	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
> >  
> > +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> > +	ved = v4l2_get_subdevdata(sd);
> > +	frame = ved->get_frame(ved);
> 
> Hrm, this code block is used in several places throughout this patch,
> and it aliases the function parameter ved to a new device which isn't
> nice. Not a problem as long as it's not used for the original VED after
> of course.
> 
> But I wonder if we should instead add a helper into vimc-common.c:
> 
> struct vimc_ent_device *vimc_get_source_ved(struct vimc_ent_device *ved)
> {
> 	struct media_entity *ent;
> 	struct v4l2_subdev *sd;
> 
> 	ent = vimc_get_source_entity(ved->ent);
> 	if (!ent)
> 		return NULL;
> 
> 	sd = media_entity_to_v4l2_subdev(ent);
> 
> 	return v4l2_get_subdevdata(sd);
> }
> 
> It might not be necessary though, just an idea. If you like it, it can
> be a patch on it's own after the vimc_get_source_entity() moving patch.

Yes indeed the source ved is calculated at many places in the entire
driver (other than this patchset as well), so if you like I can add a
helper for that.

> 
> 
> But it does show that vimc_get_source_entity() can return NULL which
> might have to be checked... though perhaps we 'know' it will always be
> valid ...
> 
> Also, following the links for each entity, for each frame sounds like
> quite a lot of work. I wonder if the active source entity should be
> cached in each VED ...

is caching only for calculating the frame okay? If it is I can do it in
this series itself if you like

> 
> That could be done on top anyway...
> 
> Overall, this looks like it will work, so with comments addressed how
> you wish,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> > +
> >  	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);
> > +
> > +	return 0;
> > +}
> > +
> > +static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
> > +{
> >  	return NULL;
> >  }
> >  
> > @@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> >  	vcap->ved.ent = &vcap->vdev.entity;
> >  	vcap->ved.process_frame = vimc_cap_process_frame;
> >  	vcap->ved.vdev_get_format = vimc_cap_get_format;
> > +	vcap->ved.get_frame = vimc_cap_get_frame;
> >  	vcap->ved.dev = vimc->mdev.dev;
> >  
> >  	/* Initialize the video_device struct */
> > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > index 4c580d854007..287d66edff49 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > @@ -85,7 +85,8 @@ struct vimc_pix_map {
> >   *
> >   * @dev:		a pointer of the device struct of the driver
> >   * @ent:		the pointer to struct media_entity for the node
> > - * @process_frame:	callback send a frame to that node
> > + * @get_frame:		callback that sends a frame processed by the entity
> 
> s/sends a/obtains the/
> 
> 
> 
> > + * @process_frame:	callback that processes a frame
> >   * @vdev_get_format:	callback that returns the current format a pad, used
> >   *			only when is_media_entity_v4l2_video_device(ent) returns
> >   *			true
> > @@ -101,8 +102,8 @@ struct vimc_pix_map {
> >  struct vimc_ent_device {
> >  	struct device *dev;
> >  	struct media_entity *ent;
> > -	void * (*process_frame)(struct vimc_ent_device *ved,
> > -				const void *frame);
> > +	void * (*get_frame)(struct vimc_ent_device *ved);
> > +	int (*process_frame)(struct vimc_ent_device *ved);
> >  	void (*vdev_get_format)(struct vimc_ent_device *ved,
> >  			      struct v4l2_pix_format *fmt);
> >  };
> > diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > index c3f6fef34f68..f61e6e8899ac 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > @@ -491,17 +491,22 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
> >  	}
> >  }
> >  
> > -static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> > -				    const void *sink_frame)
> > +static int vimc_deb_process_frame(struct vimc_ent_device *ved)
> >  {
> >  	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
> >  						    ved);
> >  	unsigned int rgb[3];
> >  	unsigned int i, j;
> > +	struct v4l2_subdev *sd;
> > +	const void *sink_frame;
> >  
> >  	/* If the stream in this node is not active, just return */
> >  	if (!vdeb->src_frame)
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> > +
> > +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> > +	ved = v4l2_get_subdevdata(sd);
> > +	sink_frame = ved->get_frame(ved);
> >  
> >  	for (i = 0; i < vdeb->sink_fmt.height; i++)
> >  		for (j = 0; j < vdeb->sink_fmt.width; j++) {
> > @@ -509,6 +514,13 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> >  			vdeb->set_rgb_src(vdeb, i, j, rgb);
> >  		}
> >  
> > +	return 0;
> > +}
> > +
> > +static void *vimc_deb_get_frame(struct vimc_ent_device *ved)
> > +{
> > +	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
> > +						    ved);
> >  	return vdeb->src_frame;
> >  }
> >  
> > @@ -593,6 +605,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> >  		goto err_free_hdl;
> >  
> >  	vdeb->ved.process_frame = vimc_deb_process_frame;
> > +	vdeb->ved.get_frame = vimc_deb_get_frame;
> >  	vdeb->ved.dev = vimc->mdev.dev;
> >  	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
> >  
> > diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> > index 121fa7d62a2e..347f9cd4a168 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
> > @@ -455,18 +455,29 @@ 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,
> > -				    const void *sink_frame)
> > +static int vimc_sca_process_frame(struct vimc_ent_device *ved)
> >  {
> >  	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> >  						    ved);
> > +	const void *sink_frame;
> > +	struct v4l2_subdev *sd;
> >  
> >  	/* If the stream in this node is not active, just return */
> >  	if (!vsca->src_frame)
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> >  
> > +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> > +	ved = v4l2_get_subdevdata(sd);
> > +	sink_frame = ved->get_frame(ved);
> >  	vimc_sca_fill_src_frame(vsca, sink_frame);
> >  
> > +	return 0;
> > +};
> > +
> > +static void *vimc_sca_get_frame(struct vimc_ent_device *ved)
> > +{
> > +	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> > +						    ved);
> >  	return vsca->src_frame;
> >  };
> >  
> > @@ -505,6 +516,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> >  	}
> >  
> >  	vsca->ved.process_frame = vimc_sca_process_frame;
> > +	vsca->ved.get_frame = vimc_sca_get_frame;
> >  	vsca->ved.dev = vimc->mdev.dev;
> >  
> >  	/* Initialize the frame format */
> > diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> > index ba5db5a150b4..32a2c39de2cd 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> > @@ -190,8 +190,7 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
> >  	.set_fmt		= vimc_sen_set_fmt,
> >  };
> >  
> > -static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> > -				    const void *sink_frame)
> > +static int vimc_sen_process_frame(struct vimc_ent_device *ved)
> >  {
> >  	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> >  						    ved);
> > @@ -238,6 +237,13 @@ static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> >  		break;
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +static void *vimc_sen_get_frame(struct vimc_ent_device *ved)
> > +{
> > +	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> > +						    ved);
> >  	return vsen->frame;
> >  }
> >  
> > @@ -429,6 +435,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> >  		goto err_free_tpg;
> >  
> >  	vsen->ved.process_frame = vimc_sen_process_frame;
> > +	vsen->ved.get_frame = vimc_sen_get_frame;
> >  	vsen->ved.dev = vimc->mdev.dev;
> >  
> >  	/* Initialize the frame format */
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index 4f8384246042..c1644d69686d 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -125,7 +125,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >  static int vimc_streamer_thread(void *data)
> >  {
> >  	struct vimc_stream *stream = data;
> > -	u8 *frame = NULL;
> > +	struct vimc_ent_device *ved;
> > +	int ret;
> >  	int i;
> >  
> >  	set_freezable();
> > @@ -136,9 +137,10 @@ static int vimc_streamer_thread(void *data)
> >  			break;
> >  
> >  		for (i = stream->pipe_size - 1; i >= 0; i--) {
> > -			frame = stream->ved_pipeline[i]->process_frame(
> > -					stream->ved_pipeline[i], frame);
> > -			if (!frame || IS_ERR(frame))
> > +			ved = stream->ved_pipeline[i];
> > +			ret = ved->process_frame(ved);
> > +
> > +			if (ret)
> >  				break;
> >  		}
> >  		//wait for 60hz
> > 
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Kaaira Gupta]

Hi,
Sorry for the late reply

On Fri, Aug 28, 2020 at 10:37:14PM +0200, Dafna Hirschfeld wrote:
> Hi,
> 
> Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
> > Hi,
> > 
> > On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
> > > 
> > > 
> > > Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
> > > > Separate the process of initialising pipeline array from starting
> > > > streaming for all entities in path of a stream. This is needed because
> > > > multiple streams can stream, but only one pipeline object is needed.
> > > > 
> > > > Process frames only for those entities in a pipeline which are
> > > > streaming. This is known through their use counts.
> > > > 
> > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > > > ---
> > > >    .../media/test-drivers/vimc/vimc-streamer.c   | 95 ++++++++++++++++---
> > > >    1 file changed, 83 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > > > index c1644d69686d..cc40ecabe95a 100644
> > > > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > > > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > > > @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> > > >    }
> > > >    /**
> > > > - * vimc_streamer_pipeline_init - Initializes the stream structure
> > > > + * vimc_streamer_stream_start - Starts streaming for all entities
> > > > + * in a stream
> > > >     *
> > > > - * @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 vimc_streamer_s_stream() to enable stream in all entities of
> > > > - * the pipeline.
> > > > + * Walks through the entity graph to call vimc_streamer_s_stream()
> > > > + * to enable stream in all entities in path of a stream.
> > > >     *
> > > >     * Return: 0 if success, error code otherwise.
> > > >     */
> > > > -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > > > -				       struct vimc_ent_device *ved)
> > > > +static int vimc_streamer_stream_start(struct vimc_stream *stream,
> > > > +				      struct vimc_ent_device *ved)
> > > >    {
> > > >    	struct media_entity *entity;
> > > >    	struct video_device *vdev;
> > > >    	struct v4l2_subdev *sd;
> > > > +	int stream_size = 0;
> > > >    	int ret = 0;
> > > > -	stream->pipe_size = 0;
> > > > -	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > > > +	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > > >    		if (!ved) {
> > > >    			vimc_streamer_pipeline_terminate(stream);
> > > >    			return -EINVAL;
> > > >    		}
> > > > -		stream->ved_pipeline[stream->pipe_size++] = ved;
> > > >    		if (is_media_entity_v4l2_subdev(ved->ent)) {
> > > >    			sd = media_entity_to_v4l2_subdev(ved->ent);
> > > > @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > > >    					    entity);
> > > >    			ved = video_get_drvdata(vdev);
> > > >    		}
> > > > +		stream_size++;
> > > > +	}
> > > > +
> > > > +	vimc_streamer_pipeline_terminate(stream);
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +/**
> > > > + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
> > > > + *
> > > > + * @stream: the pointer to the stream structure
> > > > + * @ved:    the pointer to the vimc entity initializing the stream pipeline
> > > > + *
> > > > + * Walks through the entity graph to initialise ved_pipeline and updates
> > > > + * pipe_size too.
> > > > + *
> > > > + * Return: 0 if success, error code otherwise.
> > > > + */
> > > > +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > > > +				       struct vimc_ent_device *ved)
> > > > +{
> > > > +	struct media_entity *entity;
> > > > +	struct media_device *mdev;
> > > > +	struct media_graph graph;
> > > > +	struct video_device *vdev;
> > > > +	struct v4l2_subdev *sd;
> > > > +	int ret;
> > > > +
> > > > +	entity = ved->ent;
> > > > +	mdev = entity->graph_obj.mdev;
> > > > +
> > > > +	ret = media_graph_walk_init(&graph, mdev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	media_graph_walk_start(&graph, entity);
> > > > +
> > > > +	/*
> > > > +	 * Start pipeline array initialisation from RAW Capture only to get
> > > > +	 * entities in the correct order of their frame processing.
> > > > +	 */
> > > > +	if (!strncmp(entity->name, "RGB", 3)) {
> > > 
> > > I don't understand this condition, way is it good for?
> > 
> > I have explained that later in the reply
> > 
> > > 
> > > I think the function should be generic and not assume names of entities
> > > or specific topology.
> > 
> > It doesn't assume the topology, rather it is in place just to make sure
> > that the entities in ved_pipeline are in correct order.
> > 
> > > 
> > > 
> > > > +		entity = media_graph_walk_next(&graph);
> > > > +		mdev = entity->graph_obj.mdev;
> > > > +		media_graph_walk_cleanup(&graph);
> > > > +
> > > > +		ret = media_graph_walk_init(&graph, mdev);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +		media_graph_walk_start(&graph, entity);
> 
> Hi, can you explain what this code does?
> Why does it start the search in the next entity?

Yes, so there can be two cases in the current topology; one, where this
function is called by RAW capture, and other where it is called by RGB.
So sinse walking the graph (as already implemented) is a DFS, due to the
nature of the graph, when walking starts at RAW, it moves in this order
(raw-rgb-scaler-debayer-sensor), which is also the order we want in
ved_pipeline so that process_frame can be called in order..while when
RGB calls it, the waling is in this order
(rgb-raw-sensor-scaler-debayer). Hence, in case of RGB, we start the
walking from the next entity (ie RAW) so that the order is correct
again.
I will put better comments for this in the code

> 
> > > > +	}
> > > > +
> > > > +	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > > > +		if (is_media_entity_v4l2_subdev(entity)) {
> > > > +			sd = media_entity_to_v4l2_subdev(entity);
> > > > +			ved = v4l2_get_subdevdata(sd);
> > > > +		} else {
> > > > +			vdev = container_of(entity, struct video_device, entity);
> > > > +			ved = video_get_drvdata(vdev);
> > > > +		}
> > > > +		stream->ved_pipeline[stream->pipe_size++] = ved;
> > > > +		entity = media_graph_walk_next(&graph);
> > > > +
> > > > +		if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
> > > 
> > > I also don't understand this condition
> > > 
> > > > +			media_graph_walk_cleanup(&graph);
> > > > +			return 0;
> > > > +		}
> > > >    	}
> > > 
> > > It is not clear what this function does, it looks like it adds to 'ved_pipeline'
> > > all entities that are connected to the video node, in addition to the entities
> > > that where there from previous calls, so some entities appear several times.
> > 
> > This function returns all the entities in a pipe, weather they are
> > streaming or not. For example, if only the RAW Capture 1 streams, or
> > RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
> > will have the same entities, in exactly same order, and all will occur just once.
> > Since media_graph_walk goes to each node of the graph, it returns back
> > to the first one (as its a graph), hence the last condition, ie,
> > 
> > 	if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
> > 
> > makes sure that the first entity is not added again to the array.
> > 
> > First condition, ie
> > 
> > 	if (!strncmp(entity->name, "RGB", 3)) {
> > 
> > Just makes sure that the search starts at RGB capture only. This is
> > because, in case of any other video node, the order of entities, as you
> > have mentioned later in the mail, will not be desirable, ie it won't
> > start at sensor and end at captures. So this condition just takes care
> > that all the entities in ved_pipeline array are in correct order
> > (starting at sensor, ending at captures).
> 
> It is better to compare to the whole name of the entity to make it more clear.
> Also I think it is better to document that this function is called only upon the
> first streaming node.

Agreed, I will add that.

> 
> I still think this function should be independent of the topology.
> Maybe you can use Helen's patch that allow walking a graph only opposite to the link direction: https://patchwork.kernel.org/patch/11564899/
> This ensures that the Sensor will be first in the graph walk. Then the streaming thread
> iterates the ved_pipeline from 0 upwards and not from 'pipe_size' downwards.

I didn't understand how this can be used. This moves forwards for one
stream (from sensor to say rgb capture), but it this case, we need both
the captures at the end. Also, this would need us to pass a sensor
too, so it would require walking the graph twice, once from the current
capture to sensor (to see whoch capture is connected), and then from the
sensor forwards to the captures to add them to ved_pipeline, which I
don;t think is a good way. Can you suggest some othet way? Or do you
find iterating twice fine? If everyone agrees, I can change it

Thanks for the review :D
Kaaira

> 
> Thanks,
> Dafna
> 
> 
> 
> > 
> > Thanks,
> > Kaaira
> > 
> > > 
> > > I think there is no need to use the graph walk here but to access the source entity
> > > in each iteration, the way done in vimc_streamer_stream_start
> > > also.
> > > I think the code should iterate here until it reaches an entity that is already streaming,
> > > this means that the entity is already in the `ved_pipeline`, also you should make sure
> > > that the sensor is the first entity that process a frame, therefore the sensor should be
> > > at the end/start of the list of entities. Generally each entity should appear exactly once
> > > in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame'
> > > on one entity should be after calling 'process_frame' on its source entity.
> > > maybe it is easyer to implement if 'ved_pipeline' is a linked list.
> > > 
> > > Thanks,
> > > Dafna
> > > 
> > > >    	vimc_streamer_pipeline_terminate(stream);
> > > > @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
> > > >    		for (i = stream->pipe_size - 1; i >= 0; i--) {
> > > >    			ved = stream->ved_pipeline[i];
> > > > -			ret = ved->process_frame(ved);
> > > > +			if (atomic_read(&ved->use_count) == 0)
> > > > +				continue;
> > > > +
> > > > +			ret = ved->process_frame(ved);
> > > >    			if (ret)
> > > >    				break;
> > > >    		}
> > > > @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> > > >    		if (stream->kthread)
> > > >    			return 0;
> > > > +		ret = vimc_streamer_stream_start(stream, ved);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > >    		ret = vimc_streamer_pipeline_init(stream, ved);
> > > >    		if (ret)
> > > >    			return ret;
> > > > 

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Kaaira Gupta]

On Wed, Sep 02, 2020 at 10:56:46AM +0100, Kieran Bingham wrote:
> Hi Kaaira, Dafna,
> 
> On 28/08/2020 21:37, Dafna Hirschfeld wrote:
> > Hi,
> > 
> > Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
> >> Hi,
> >>
> >> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
> >>>
> >>>
> >>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
> >>>> Separate the process of initialising pipeline array from starting
> >>>> streaming for all entities in path of a stream. This is needed because
> >>>> multiple streams can stream, but only one pipeline object is needed.
> >>>>
> >>>> Process frames only for those entities in a pipeline which are
> >>>> streaming. This is known through their use counts.
> >>>>
> >>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> >>>> ---
> >>>>    .../media/test-drivers/vimc/vimc-streamer.c   | 95
> >>>> ++++++++++++++++---
> >>>>    1 file changed, 83 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
> >>>> b/drivers/media/test-drivers/vimc/vimc-streamer.c
> >>>> index c1644d69686d..cc40ecabe95a 100644
> >>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> >>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> >>>> @@ -40,33 +40,30 @@ static void
> >>>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> >>>>    }
> >>>>    /**
> >>>> - * vimc_streamer_pipeline_init - Initializes the stream structure
> >>>> + * vimc_streamer_stream_start - Starts streaming for all entities
> >>>> + * in a stream
> >>>>     *
> >>>> - * @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 vimc_streamer_s_stream() to enable stream in all entities of
> >>>> - * the pipeline.
> >>>> + * Walks through the entity graph to call vimc_streamer_s_stream()
> >>>> + * to enable stream in all entities in path of a stream.
> >>>>     *
> >>>>     * Return: 0 if success, error code otherwise.
> >>>>     */
> >>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >>>> -                       struct vimc_ent_device *ved)
> >>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream,
> >>>> +                      struct vimc_ent_device *ved)
> >>>>    {
> >>>>        struct media_entity *entity;
> >>>>        struct video_device *vdev;
> >>>>        struct v4l2_subdev *sd;
> >>>> +    int stream_size = 0;
> >>>>        int ret = 0;
> >>>> -    stream->pipe_size = 0;
> >>>> -    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> >>>> +    while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> >>>>            if (!ved) {
> >>>>                vimc_streamer_pipeline_terminate(stream);
> >>>>                return -EINVAL;
> >>>>            }
> >>>> -        stream->ved_pipeline[stream->pipe_size++] = ved;
> >>>>            if (is_media_entity_v4l2_subdev(ved->ent)) {
> >>>>                sd = media_entity_to_v4l2_subdev(ved->ent);
> >>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct
> >>>> vimc_stream *stream,
> >>>>                            entity);
> >>>>                ved = video_get_drvdata(vdev);
> >>>>            }
> >>>> +        stream_size++;
> >>>> +    }
> >>>> +
> >>>> +    vimc_streamer_pipeline_terminate(stream);
> >>>> +    return -EINVAL;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
> >>>> + *
> >>>> + * @stream: the pointer to the stream structure
> >>>> + * @ved:    the pointer to the vimc entity initializing the stream
> >>>> pipeline
> 
> Which entity is this? Is it the start, or the end of the pipeline? I.e.
> the sensor? or the capture ?

It is the capture, I will add it to the documentation..thanks

> 
> >>>> + *
> >>>> + * Walks through the entity graph to initialise ved_pipeline and
> >>>> updates
> >>>> + * pipe_size too.
> >>>> + *
> >>>> + * Return: 0 if success, error code otherwise.
> >>>> + */
> >>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >>>> +                       struct vimc_ent_device *ved)
> >>>> +{
> >>>> +    struct media_entity *entity;
> >>>> +    struct media_device *mdev;
> >>>> +    struct media_graph graph;
> >>>> +    struct video_device *vdev;
> >>>> +    struct v4l2_subdev *sd;
> >>>> +    int ret;
> >>>> +
> >>>> +    entity = ved->ent;
> >>>> +    mdev = entity->graph_obj.mdev;
> >>>> +
> >>>> +    ret = media_graph_walk_init(&graph, mdev);
> >>>> +    if (ret)
> >>>> +        return ret;
> >>>> +
> >>>> +    media_graph_walk_start(&graph, entity);
> >>>> +
> >>>> +    /*
> >>>> +     * Start pipeline array initialisation from RAW Capture only to
> >>>> get
> >>>> +     * entities in the correct order of their frame processing.
> >>>> +     */
> >>>> +    if (!strncmp(entity->name, "RGB", 3)) {
> >>>
> >>> I don't understand this condition, way is it good for?
> >>
> >> I have explained that later in the reply
> 
> Matching on entity names is a bit awkward, as they could be changed I
> guess quite easily, and easily missed in this string matching.

Agreed, I need to think of a better way to prevent this

> 
> I haven't fully understood this code block yet to work out if there's
> another way we could do this though, but reading ahead I see there might
> be a way to 'walk the graph' on a per-stream basis which might be a good
> way of factoring this path.
> 
> Although there still needs to be a construction of the paths available
> to a stream which splits from the sensor.
> 
> 
> >>
> >>>
> >>> I think the function should be generic and not assume names of entities
> >>> or specific topology.
> >>
> >> It doesn't assume the topology, rather it is in place just to make sure
> >> that the entities in ved_pipeline are in correct order.
> >>
> >>>
> >>>
> >>>> +        entity = media_graph_walk_next(&graph);
> >>>> +        mdev = entity->graph_obj.mdev;
> >>>> +        media_graph_walk_cleanup(&graph);
> >>>> +
> >>>> +        ret = media_graph_walk_init(&graph, mdev);
> >>>> +        if (ret)
> >>>> +            return ret;
> >>>> +        media_graph_walk_start(&graph, entity);
> > 
> > Hi, can you explain what this code does?
> > Why does it start the search in the next entity?
> > 
> >>>> +    }
> >>>> +
> >>>> +    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> >>>> +        if (is_media_entity_v4l2_subdev(entity)) {
> >>>> +            sd = media_entity_to_v4l2_subdev(entity);
> >>>> +            ved = v4l2_get_subdevdata(sd);
> >>>> +        } else {
> >>>> +            vdev = container_of(entity, struct video_device, entity);
> >>>> +            ved = video_get_drvdata(vdev);
> >>>> +        }
> >>>> +        stream->ved_pipeline[stream->pipe_size++] = ved;
> >>>> +        entity = media_graph_walk_next(&graph);
> >>>> +
> >>>> +        if (!strcmp(entity->name,
> >>>> stream->ved_pipeline[0]->ent->name)) {
> >>>
> >>> I also don't understand this condition
> >>>
> >>>> +            media_graph_walk_cleanup(&graph);
> >>>> +            return 0;
> >>>> +        }
> >>>>        }
> >>>
> >>> It is not clear what this function does, it looks like it adds to
> >>> 'ved_pipeline'
> >>> all entities that are connected to the video node, in addition to the
> >>> entities
> >>> that where there from previous calls, so some entities appear several
> >>> times.
> >>
> >> This function returns all the entities in a pipe, weather they are
> >> streaming or not. For example, if only the RAW Capture 1 streams, or
> >> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
> >> will have the same entities, in exactly same order, and all will occur
> >> just once.
> >> Since media_graph_walk goes to each node of the graph, it returns back
> >> to the first one (as its a graph), hence the last condition, ie,
> >>
> >>     if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
> >>
> >> makes sure that the first entity is not added again to the array.
> >>
> >> First condition, ie
> >>
> >>     if (!strncmp(entity->name, "RGB", 3)) {
> >>
> >> Just makes sure that the search starts at RGB capture only. This is
> >> because, in case of any other video node, the order of entities, as you
> >> have mentioned later in the mail, will not be desirable, ie it won't
> >> start at sensor and end at captures. So this condition just takes care
> >> that all the entities in ved_pipeline array are in correct order
> >> (starting at sensor, ending at captures).
> > 
> > It is better to compare to the whole name of the entity to make it more
> > clear.
> > Also I think it is better to document that this function is called only
> > upon the
> > first streaming node.
> 
> If this doesn't end up refactored with other helpers, then indeed a few
> comments might help the readabilty here. The distinctions of each
> re-initialisation of the graph walk are hard to interpret the purpose.
> 
> 
> 
> > 
> > I still think this function should be independent of the topology.
> > Maybe you can use Helen's patch that allow walking a graph only opposite
> > to the link direction: https://patchwork.kernel.org/patch/11564899/
> > This ensures that the Sensor will be first in the graph walk. Then the
> > streaming thread
> > iterates the ved_pipeline from 0 upwards and not from 'pipe_size'
> > downwards.
> 
> Being able to use a direct helper to walk the pipeline cleanly sounds
> promising indeed.
> 
> I suspect at some point int he next patches though - I'm going to see
> something that needs to have full visibility of all paths enabled from
> the sensor, as I think I recall that the thread will then process all
> (enabled) entities in a single pass.

Yes, that exactly is the problem :( This helper (that dafna has shared)
can walk through one path, while the thread (which processes the frames)
needs to view all the entites in all connected paths

> --
> Kieran
> 
> 
> > 
> > Thanks,
> > Dafna
> > 
> > 
> > 
> >>
> >> Thanks,
> >> Kaaira
> >>
> >>>
> >>> I think there is no need to use the graph walk here but to access the
> >>> source entity
> >>> in each iteration, the way done in vimc_streamer_stream_start
> >>> also.
> >>> I think the code should iterate here until it reaches an entity that
> >>> is already streaming,
> >>> this means that the entity is already in the `ved_pipeline`, also you
> >>> should make sure
> >>> that the sensor is the first entity that process a frame, therefore
> >>> the sensor should be
> >>> at the end/start of the list of entities. Generally each entity
> >>> should appear exactly once
> >>> in the 'ved_pipeline' array and the entities should be ordered such
> >>> that when calling 'process_frame'
> >>> on one entity should be after calling 'process_frame' on its source
> >>> entity.
> >>> maybe it is easyer to implement if 'ved_pipeline' is a linked list.
> >>>
> >>> Thanks,
> >>> Dafna
> >>>
> >>>>        vimc_streamer_pipeline_terminate(stream);
> >>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
> >>>>            for (i = stream->pipe_size - 1; i >= 0; i--) {
> >>>>                ved = stream->ved_pipeline[i];
> >>>> -            ret = ved->process_frame(ved);
> >>>> +            if (atomic_read(&ved->use_count) == 0)
> >>>> +                continue;
> >>>> +
> >>>> +            ret = ved->process_frame(ved);
> >>>>                if (ret)
> >>>>                    break;
> >>>>            }
> >>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream
> >>>> *stream,
> >>>>            if (stream->kthread)
> >>>>                return 0;
> >>>> +        ret = vimc_streamer_stream_start(stream, ved);
> >>>> +        if (ret)
> >>>> +            return ret;
> >>>> +
> >>>>            ret = vimc_streamer_pipeline_init(stream, ved);
> >>>>            if (ret)
> >>>>                return ret;
> >>>>
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 5/9] media: vimc: Separate closing of stream and thread

[Kaaira Gupta]

Hi,

On Wed, Sep 02, 2020 at 11:13:09AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > Make separate functions for stopping streaming of entities in path of a
> > particular stream and stopping thread. This is needed to ensure that
> > thread doesn't stop when one device stops streaming in case of multiple
> > streams.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  .../media/test-drivers/vimc/vimc-streamer.c   | 82 ++++++++++++-------
> >  1 file changed, 52 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index cc40ecabe95a..6b5ea1537952 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -13,29 +13,59 @@
> >  #include "vimc-streamer.h"
> >  
> >  /**
> > - * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
> > + * vimc_streamer_pipeline_terminate - Terminate the thread
> >   *
> > - * @stream: the pointer to the stream structure with the pipeline to be
> > - *	    disabled.
> > + * @stream: the pointer to the stream structure
> >   *
> > - * Calls s_stream to disable the stream in each entity of the pipeline
> > + * Erases values of stream struct and terminates the thread
> 
> It would help if the function brief described it's purpose rather than
> 'what it does'. "Erases values of stream struct" is not helpful here, as
> that's just a direct read of what happens in the code.

Okay, I will make these changes

> 
> I'm guessing here, but how about:
> 
> "Tear down all resources belonging to the pipeline when there are no
> longer any active streams being used. This includes stopping the active
> processing thread"
> 
> 
> But reading my text makes me worry about the ordering that might take
> place. The thread should be stopped as soon as the last stream using it
> is stopped. Presumably as the 'first thing' that happens to make sure
> there is no concurrent processing while the stream is being disabled.
> 
> Hopefully there's no race condition ...

There isn't..in further patches (when multiple streams are allowed),
pipeline_terminate is called only after both the streams terminate.

> 
> 
> >   *
> >   */
> >  static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> >  {
> >  	struct vimc_ent_device *ved;
> > -	struct v4l2_subdev *sd;
> >  
> >  	while (stream->pipe_size) {
> >  		stream->pipe_size--;
> >  		ved = stream->ved_pipeline[stream->pipe_size];
> >  		stream->ved_pipeline[stream->pipe_size] = NULL;
> > +	}
> >  
> > -		if (!is_media_entity_v4l2_subdev(ved->ent))
> > -			continue;
> > +	kthread_stop(stream->kthread);
> > +	stream->kthread = NULL;
> > +}
> >  
> > -		sd = media_entity_to_v4l2_subdev(ved->ent);
> > -		v4l2_subdev_call(sd, video, s_stream, 0);
> > +/**
> > + * vimc_streamer_stream_terminate - Disable stream in all ved in stream
> > + *
> > + * @ved: pointer to the ved for which stream needs to be disabled
> > + *
> > + * Calls s_stream to disable the stream in each entity of the stream
> > + *
> > + */
> > +static void vimc_streamer_stream_terminate(struct vimc_ent_device *ved)
> 
> I would call this vimc_streamer_stream_stop to match
> vimc_streamer_stream_start() rather than terminate...

Okay, noted..I will make this change

> 
> > +{
> > +	struct media_entity *entity = ved->ent;
> > +	struct video_device *vdev;
> > +	struct v4l2_subdev *sd;
> > +
> > +	while (entity) {
> > +		if (is_media_entity_v4l2_subdev(ved->ent)) {
> > +			sd = media_entity_to_v4l2_subdev(ved->ent);
> > +			v4l2_subdev_call(sd, video, s_stream, 0);
> > +		}
> > +		entity = vimc_get_source_entity(ved->ent);
> > +		if (!entity)
> > +			break;
> > +
> > +		if (is_media_entity_v4l2_subdev(entity)) {
> > +			sd = media_entity_to_v4l2_subdev(entity);
> > +			ved = v4l2_get_subdevdata(sd);
> > +		} else {
> > +			vdev = container_of(entity,
> > +					    struct video_device,
> > +					    entity);
> > +			ved = video_get_drvdata(vdev);
> > +		}
> 
> It looks like this is walking back through the linked graph, calling
> s_stream(0) right?

Yes

> 
> I wonder if struct vimc_ent_device should have a pointer to the entity
> it's connected to, to simplify this. ... presumably this is done
> elsewhere too?
> 
> Although then that's still walking 'backwards' rather than forwards...

I don't understand your concern here

> 
> 
> 
> 
> >  	}
> >  }
> >  
> > @@ -43,25 +73,25 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> >   * vimc_streamer_stream_start - Starts streaming for all entities
> >   * in a stream
> >   *
> > - * @ved:    the pointer to the vimc entity initializing the stream
> > + * @cved:    the pointer to the vimc entity initializing the stream
> >   *
> >   * Walks through the entity graph to call vimc_streamer_s_stream()
> >   * to enable stream in all entities in path of a stream.
> >   *
> >   * Return: 0 if success, error code otherwise.
> >   */
> > -static int vimc_streamer_stream_start(struct vimc_stream *stream,
> > -				      struct vimc_ent_device *ved)
> > +static int vimc_streamer_stream_start(struct vimc_ent_device *cved)
> >  {
> >  	struct media_entity *entity;
> >  	struct video_device *vdev;
> >  	struct v4l2_subdev *sd;
> > +	struct vimc_ent_device *ved = cved;
> >  	int stream_size = 0;
> >  	int ret = 0;
> >  
> >  	while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> >  		if (!ved) {
> > -			vimc_streamer_pipeline_terminate(stream);
> > +			vimc_streamer_stream_terminate(cved);
> >  			return -EINVAL;
> >  		}
> >  
> > @@ -71,7 +101,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
> >  			if (ret && ret != -ENOIOCTLCMD) {
> >  				dev_err(ved->dev, "subdev_call error %s\n",
> >  					ved->ent->name);
> > -				vimc_streamer_pipeline_terminate(stream);
> > +				vimc_streamer_stream_terminate(cved);
> >  				return ret;
> >  			}
> >  		}
> > @@ -84,7 +114,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
> >  				dev_err(ved->dev,
> >  					"first entity in the pipe '%s' is not a source\n",
> >  					ved->ent->name);
> > -				vimc_streamer_pipeline_terminate(stream);
> > +				vimc_streamer_stream_terminate(cved);
> >  				pr_info ("first entry not source");
> >  				return -EPIPE;
> >  			}
> > @@ -104,7 +134,7 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
> >  		stream_size++;
> >  	}
> >  
> > -	vimc_streamer_pipeline_terminate(stream);
> > +	vimc_streamer_stream_terminate(cved);
> >  	return -EINVAL;
> >  }
> >  
> > @@ -120,13 +150,14 @@ static int vimc_streamer_stream_start(struct vimc_stream *stream,
> >   * Return: 0 if success, error code otherwise.
> >   */
> >  static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> > -				       struct vimc_ent_device *ved)
> > +				       struct vimc_ent_device *cved)
> >  {
> >  	struct media_entity *entity;
> >  	struct media_device *mdev;
> >  	struct media_graph graph;
> >  	struct video_device *vdev;
> >  	struct v4l2_subdev *sd;
> > +	struct vimc_ent_device *ved = cved;
> >  	int ret;
> >  
> >  	entity = ved->ent;
> > @@ -170,6 +201,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >  		}
> >  	}
> >  
> > +	vimc_streamer_stream_terminate(cved);
> >  	vimc_streamer_pipeline_terminate(stream);
> >  	return -EINVAL;
> >  }
> > @@ -246,7 +278,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  		if (stream->kthread)
> >  			return 0;
> >  
> > -		ret = vimc_streamer_stream_start(stream, ved);
> > +		ret = vimc_streamer_stream_start(ved);
> >  		if (ret)
> >  			return ret;
> >  
> > @@ -260,6 +292,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  		if (IS_ERR(stream->kthread)) {
> >  			ret = PTR_ERR(stream->kthread);
> >  			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
> > +			vimc_streamer_stream_terminate(ved);
> >  			vimc_streamer_pipeline_terminate(stream);
> >  			stream->kthread = NULL;
> 
> If vimc_streamer_pipeline_terminate() sets stream->kthread = NULL, it
> doesn't need to be done again here.

Noted

> 
> 
> >  			return ret;
> > @@ -269,18 +302,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  		if (!stream->kthread)
> >  			return 0;
> >  
> > -		ret = kthread_stop(stream->kthread);
> > -		/*
> > -		 * kthread_stop returns -EINTR in cases when streamon was
> > -		 * immediately followed by streamoff, and the thread didn't had
> > -		 * a chance to run. Ignore errors to stop the stream in the
> > -		 * pipeline.
> > -		 */
> 
> Do we need to retain that comment when stopping the thread?

I am not sure, I think helen or dafna can help?

> 
> > -		if (ret)
> > -			dev_dbg(ved->dev, "kthread_stop returned '%d'\n", ret);
> > -
> > -		stream->kthread = NULL;
> > -
> > +		vimc_streamer_stream_terminate(ved);
> >  		vimc_streamer_pipeline_terminate(stream);
> >  	}
> >  
> > 
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct

[Kaaira Gupta]

On Wed, Sep 02, 2020 at 11:29:31AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > Multiple streams will share same stream struct if we want them to run on
> > same thread. So remove it from vimc_cap struct so that it doesn't get
> > destroyed when one of the capture does, and allocate it memory
> > dynamically. Use kref with it as it will be used by multiple captures.
> > 
> 
> Is the vimc_stream stuct the context of the streamer? or of each 'stream'?

of streamer

> 
> If it's the streamer - then can't we store this (non-dynamically) as
> part of the Sensor node, to avoid kzalloc/freeing it ?

I tried this after we discussed, but I kept on having some memory
issues..so I used this method as an alternate. If making vimc_streamer
struct dynamically is an issue with others as well (I understand why it
might be an issue, since it should not be dependent on which capture
calls initialised it), I can work on moving it to the sensor instead

> 
> 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/media/test-drivers/vimc/vimc-capture.c  | 15 +++++++++++----
> >  drivers/media/test-drivers/vimc/vimc-streamer.c | 17 ++++++-----------
> >  drivers/media/test-drivers/vimc/vimc-streamer.h |  2 ++
> >  3 files changed, 19 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index 93418cb5a139..73e5bdd17c57 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -28,7 +28,6 @@ struct vimc_cap_device {
> >  	spinlock_t qlock;
> >  	struct mutex lock;
> >  	u32 sequence;
> > -	struct vimc_stream stream;
> >  	struct media_pad pad;
> >  };
> >  
> > @@ -241,19 +240,25 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
> >  {
> >  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> >  	struct media_entity *entity = &vcap->vdev.entity;
> > +	struct media_pipeline *pipe = NULL;
> > +	struct vimc_stream *stream;
> >  	int ret;
> >  
> >  	atomic_inc(&vcap->ved.use_count);
> >  	vcap->sequence = 0;
> >  
> > +	stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> > +	kref_init(&stream->refcount);
> > +	pipe = &stream->pipe;
> > +
> >  	/* Start the media pipeline */
> > -	ret = media_pipeline_start(entity, &vcap->stream.pipe);
> > +	ret = media_pipeline_start(entity, pipe);
> >  	if (ret) {
> >  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> >  		return ret;
> >  	}
> >  
> > -	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
> > +	ret = vimc_streamer_s_stream(stream, &vcap->ved, 1);
> >  	if (ret) {
> >  		media_pipeline_stop(entity);
> >  		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> > @@ -270,9 +275,11 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
> >  static void vimc_cap_stop_streaming(struct vb2_queue *vq)
> >  {
> >  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> > +	struct media_pipeline *pipe = vcap->ved.ent->pipe;
> > +	struct vimc_stream *stream = container_of(pipe, struct vimc_stream, pipe);
> 
> In fact, I see it's stored as part of the 'pipe' so there is one
> vimc_stream per pipeline ...
> 
> So it could just be a structure on the pipe rather than obtaining a
> pointer here.
> 
> I think it's probably 'ok' to have one streamer per pipe currently as
> the raw input node is not functional, but I also thought that by having
> the streamer as part of the sensor entity then there is one streamer
> ('one thread') per video source ... which would prevent this having to
> be changed if someone later deals with the video node that allows
> re-processing raw frames ?
> 
> 
> 
> >  	atomic_dec(&vcap->ved.use_count);
> > -	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
> > +	vimc_streamer_s_stream(stream, &vcap->ved, 0);
> >  
> >  	/* Stop the media pipeline */
> >  	media_pipeline_stop(&vcap->vdev.entity);
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index f5c9e2f3bbcb..fade37bee26d 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -20,18 +20,13 @@
> >   * Erases values of stream struct and terminates the thread
> >   *
> >   */
> > -static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> > +static void vimc_streamer_pipeline_terminate(struct kref *ref)
> >  {
> > -	struct vimc_ent_device *ved;
> > -
> > -	while (stream->pipe_size) {
> > -		stream->pipe_size--;
> > -		ved = stream->ved_pipeline[stream->pipe_size];
> > -		stream->ved_pipeline[stream->pipe_size] = NULL;
> > -	}
> > +	struct vimc_stream *stream = container_of(ref, struct vimc_stream, refcount);
> >  
> >  	kthread_stop(stream->kthread);
> >  	stream->kthread = NULL;
> > +	kfree(stream);
> >  }
> >  
> >  /**
> > @@ -202,7 +197,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >  	}
> >  
> >  	vimc_streamer_stream_terminate(cved);
> > -	vimc_streamer_pipeline_terminate(stream);
> > +	kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> >  	return -EINVAL;
> >  }
> >  
> > @@ -298,7 +293,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  			ret = PTR_ERR(stream->kthread);
> >  			dev_err(ved->dev, "kthread_run failed with %d\n", ret);
> >  			vimc_streamer_stream_terminate(ved);
> > -			vimc_streamer_pipeline_terminate(stream);
> > +			kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> >  		}
> >  
> >  	} else {
> > @@ -306,7 +301,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  			goto out;
> >  
> >  		vimc_streamer_stream_terminate(ved);
> > -		vimc_streamer_pipeline_terminate(stream);
> > +		kref_put(&stream->refcount, vimc_streamer_pipeline_terminate);
> >  	}
> >  out:
> >  	mutex_unlock(&vimc_streamer_lock);
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.h b/drivers/media/test-drivers/vimc/vimc-streamer.h
> > index 3bb6731b8d4d..533c88675362 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.h
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.h
> > @@ -18,6 +18,7 @@
> >  /**
> >   * struct vimc_stream - struct that represents a stream in the pipeline
> >   *
> > + * @refcount:		kref object associated with stream struct
> 
> What does it track though?
> 
> We know it's associated with the stream struct because it's in the
> vimc_stream struct.

Vimc_streamer should be destroyed only when both the streams (if there
are two) have been terminated. So, it takes care of that.

> 
> 
> 
> >   * @pipe:		the media pipeline object associated with this stream
> >   * @ved_pipeline:	array containing all the entities participating in the
> >   * 			stream. The order is from a video device (usually a
> 
> The fact that this comment still says "all entities participating in the
> stream" worries me a little, as I think now the Streamer is dealing with
> multiple streams.
> 
> I think we need to be really clear with the differences of objects and
> terminology.

Yes, 'stream' here should be replaced with pipeline I think? As it
represents all the entities in the entire path connected with the sensor

> 
> For instance I think the current terms are something like this:
> 
> Streamer: Responsible for managing processing from a sensor device
> through all entities.
> 
> Stream: A flow of frames to a single capture video device node.
> 
> Pipeline: All entities used within the vimc streamer ?
> 
> (I'm not sure if those are right, I'm writing down what my current
> interpretations are, so if someone wants to/can clarify - please do).
> 
> 
> 
> > @@ -32,6 +33,7 @@
> >   * process frames for the stream.
> >   */
> >  struct vimc_stream {
> > +	struct kref refcount;
> >  	struct media_pipeline pipe;
> >  	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
> >  	unsigned int pipe_size;
> > 
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 9/9] media: vimc: Run multiple captures on same thread

[Kaaira Gupta]

On Wed, Sep 02, 2020 at 11:46:50AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > If multiple captures try to enable stream, start their stream but do not
> > initialise the pipeline again. Also, don't start the thread separately.
> > Starting their streams will update the use count and their frames would
> > be processed by the already running thread.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/media/test-drivers/vimc/vimc-streamer.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > index fade37bee26d..880c31759cc0 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> > @@ -275,13 +275,14 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> >  		return ret;
> >  
> >  	if (enable) {
> > -		if (stream->kthread)
> > -			return 0;
> >  
> >  		ret = vimc_streamer_stream_start(ved);
> >  		if (ret)
> >  			goto out;
> >  
> > +		if (stream->kthread)
> > +			goto out;
> > +
> 
> This goto out makes it look like it's an error path. So that probably
> warrants a comment along the lines of 'don't reinitialise the pipeline
> if it has already started'. ?
> 
> I wonder if it's better to handle the pipeline_init during _stream_start
> 'only' in the code path where it's the first stream ?

stream_start needs to be called for both (or all) the captures, while
init must be called just once...so I think keeping it here inside
s_stream and calling it just once will be okay too as that wouldn't need
refcounts..but yes I think I should keep them symmetrical for better
readability of code. What do you think is a better method?

> 
> Then similarly, the pipeline_terminate would happen on stream_stop
> 'only' when it's the last stream.
> 
> Or I guess that is handled by the refcount ... Hrm it would be nice to
> be able to make/keep it symmetrical somehow.
> 
> 
> >  		ret = vimc_streamer_pipeline_init(stream, ved);
> >  		if (ret)
> >  			goto out;
> > 
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 0/9] media: vimc: Multiple stream support in vimc

[Kaaira Gupta]

On Wed, Sep 02, 2020 at 11:51:59AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> Thank you for this series.
> 
> I have tested it, and indeed it works well, which is great work.
> I know this has been hard to debug some of the code paths!

Thanks for testing and helping! :D

> 
> There are a few bits that are hard for me to understand, with the graph
> walking/initialisation - so I think either some better comments or
> refactoring might help there - and Dafna has suggested that there might
> be a useful helper which will assist too. That needs to be checked
> though, as I think your 'streamer' needs to have full visibility of the
> whole pipeline, rather than just a single stream.
> 
> I wonder if it would help to rename it to make that clearer, as
> 'vimc_stream' sounds like it deals with only a single stream - but now
> it deals with multiple - so the naming is a bit confusing.

Hm, I too think that the name is confusing and doesn't show the correct
context..is vimc_streamer_referance a better name? What name do you
suggest?

Thanks, 
Kaaira

> 
> --
> Kieran
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
> > This series adds supoort for two (or more) capture devices to be 
> > connected to the same sensors and run simultaneously.
> > 
> > Changes since v2:
> > 	- This series introduces new patches, namely patch 1, 2, 4, 5,
> > 	  7, and 9 to shift multiple captures to operate at a single
> > 	  thread.
> > 
> > Kaaira Gupta (7):
> >   media: vimc: Move get_source_entity to vimc-common
> >   media: vimc: Add get_frame callback
> >   media: vimc: Separate starting stream from pipeline initialisation
> >   media: vimc: Separate closing of stream and thread
> >   media: vimc: Dynamically allocate stream struct
> >   media: vimc: Join pipeline if one already exists
> >   media: vimc: Run multiple captures on same thread
> > 
> > Niklas Söderlund (2):
> >   media: vimc: Add usage count to subdevices
> >   media: vimc: Serialize vimc_streamer_s_stream()
> > 
> >  .../media/test-drivers/vimc/vimc-capture.c    |  42 +++-
> >  drivers/media/test-drivers/vimc/vimc-common.c |  14 ++
> >  drivers/media/test-drivers/vimc/vimc-common.h |  21 +-
> >  .../media/test-drivers/vimc/vimc-debayer.c    |  26 ++-
> >  drivers/media/test-drivers/vimc/vimc-scaler.c |  25 +-
> >  drivers/media/test-drivers/vimc/vimc-sensor.c |  17 +-
> >  .../media/test-drivers/vimc/vimc-streamer.c   | 213 ++++++++++++------
> >  .../media/test-drivers/vimc/vimc-streamer.h   |   2 +
> >  8 files changed, 271 insertions(+), 89 deletions(-)
> > 
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v3 2/9] media: vimc: Add get_frame callback

[Dafna Hirschfeld]

Hi,

Am 20.08.20 um 17:36 schrieb Kieran Bingham:
> Hi Kaaira,
> 
> On 19/08/2020 19:04, Kaaira Gupta wrote:
>> In the process of making vimc compatible for multiple streams, we need
>> to create a frame passing process such that two different entities can
>> get the frame from a common entity. This isn't possible currently without
>> calling process_frame twice for the common entity, as process_frames
>> returns the frame which gets passed on.
>>
>> So, to take care of this, add a get_frame callback to vimc device and
>> use it to get the frames for an entity from previous entity instead of
>> returning and passing the frames as an argument in process_frame.
>>
>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
>> ---
>>   .../media/test-drivers/vimc/vimc-capture.c    | 18 +++++++++++++++---
>>   drivers/media/test-drivers/vimc/vimc-common.h |  7 ++++---
>>   .../media/test-drivers/vimc/vimc-debayer.c    | 19 ++++++++++++++++---
>>   drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++++++++++++++---
>>   drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +++++++++--
>>   .../media/test-drivers/vimc/vimc-streamer.c   | 10 ++++++----
>>   6 files changed, 65 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
>> index c63496b17b9a..a8cbb8e4d5ba 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
>> @@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device *ved)
>>   	video_unregister_device(&vcap->vdev);
>>   }
>>   
>> -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>> -				    const void *frame)
>> +static int vimc_cap_process_frame(struct vimc_ent_device *ved)
>>   {
>>   	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>>   						    ved);
>>   	struct vimc_cap_buffer *vimc_buf;
>> +	struct v4l2_subdev *sd;
>> +	const void *frame;
>>   	void *vbuf;
>>   
>>   	spin_lock(&vcap->qlock);
>> @@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>>   					    typeof(*vimc_buf), list);
>>   	if (!vimc_buf) {
>>   		spin_unlock(&vcap->qlock);
>> -		return ERR_PTR(-EAGAIN);
>> +		return -EAGAIN;
>>   	}
>>   
>>   	/* Remove this entry from the list */
>> @@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>>   
>>   	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
>>   
>> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
>> +	ved = v4l2_get_subdevdata(sd);
>> +	frame = ved->get_frame(ved);
> 
> Hrm, this code block is used in several places throughout this patch,
> and it aliases the function parameter ved to a new device which isn't
> nice. Not a problem as long as it's not used for the original VED after
> of course.
> 
> But I wonder if we should instead add a helper into vimc-common.c:
> 
> struct vimc_ent_device *vimc_get_source_ved(struct vimc_ent_device *ved)
> {
> 	struct media_entity *ent;
> 	struct v4l2_subdev *sd;
> 
> 	ent = vimc_get_source_entity(ved->ent);
> 	if (!ent)
> 		return NULL;
> 
> 	sd = media_entity_to_v4l2_subdev(ent);
> 
> 	return v4l2_get_subdevdata(sd);
> }
> 
> It might not be necessary though, just an idea. If you like it, it can
> be a patch on it's own after the vimc_get_source_entity() moving patch.
> 
> 
> But it does show that vimc_get_source_entity() can return NULL which
> might have to be checked... though perhaps we 'know' it will always be
> valid ...
> 
> Also, following the links for each entity, for each frame sounds like
> quite a lot of work. I wonder if the active source entity should be
> cached in each VED ...

I think that we can add a pad number as an argument for the function and
only ask for source entity for that pad.

> 
> That could be done on top anyway...
> 
> Overall, this looks like it will work, so with comments addressed how
> you wish,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
>> +
>>   	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);
>> +
>> +	return 0;
>> +}
>> +
>> +static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
>> +{
>>   	return NULL;
>>   }
>>   
>> @@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>>   	vcap->ved.ent = &vcap->vdev.entity;
>>   	vcap->ved.process_frame = vimc_cap_process_frame;
>>   	vcap->ved.vdev_get_format = vimc_cap_get_format;
>> +	vcap->ved.get_frame = vimc_cap_get_frame;
>>   	vcap->ved.dev = vimc->mdev.dev;
>>   
>>   	/* Initialize the video_device struct */
>> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
>> index 4c580d854007..287d66edff49 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-common.h
>> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
>> @@ -85,7 +85,8 @@ struct vimc_pix_map {
>>    *
>>    * @dev:		a pointer of the device struct of the driver
>>    * @ent:		the pointer to struct media_entity for the node
>> - * @process_frame:	callback send a frame to that node
>> + * @get_frame:		callback that sends a frame processed by the entity

I see that all entities implemnt the 'get_frame' callback by returning the frame
maybe we can instead make the frame a filed of vimc_ent_device instead of that
callback.

Thanks,
Dafna

> 
> s/sends a/obtains the/
> 
> 
> 
>> + * @process_frame:	callback that processes a frame
>>    * @vdev_get_format:	callback that returns the current format a pad, used
>>    *			only when is_media_entity_v4l2_video_device(ent) returns
>>    *			true
>> @@ -101,8 +102,8 @@ struct vimc_pix_map {
>>   struct vimc_ent_device {
>>   	struct device *dev;
>>   	struct media_entity *ent;
>> -	void * (*process_frame)(struct vimc_ent_device *ved,
>> -				const void *frame);
>> +	void * (*get_frame)(struct vimc_ent_device *ved);
>> +	int (*process_frame)(struct vimc_ent_device *ved);
>>   	void (*vdev_get_format)(struct vimc_ent_device *ved,
>>   			      struct v4l2_pix_format *fmt);
>>   };
>> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
>> index c3f6fef34f68..f61e6e8899ac 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
>> @@ -491,17 +491,22 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
>>   	}
>>   }
>>   
>> -static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
>> -				    const void *sink_frame)
>> +static int vimc_deb_process_frame(struct vimc_ent_device *ved)
>>   {
>>   	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>>   						    ved);
>>   	unsigned int rgb[3];
>>   	unsigned int i, j;
>> +	struct v4l2_subdev *sd;
>> +	const void *sink_frame;
>>   
>>   	/* If the stream in this node is not active, just return */
>>   	if (!vdeb->src_frame)
>> -		return ERR_PTR(-EINVAL);
>> +		return -EINVAL;
>> +
>> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
>> +	ved = v4l2_get_subdevdata(sd);
>> +	sink_frame = ved->get_frame(ved);
>>   
>>   	for (i = 0; i < vdeb->sink_fmt.height; i++)
>>   		for (j = 0; j < vdeb->sink_fmt.width; j++) {
>> @@ -509,6 +514,13 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
>>   			vdeb->set_rgb_src(vdeb, i, j, rgb);
>>   		}
>>   
>> +	return 0;
>> +}
>> +
>> +static void *vimc_deb_get_frame(struct vimc_ent_device *ved)
>> +{
>> +	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>> +						    ved);
>>   	return vdeb->src_frame;
>>   }
>>   
>> @@ -593,6 +605,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>>   		goto err_free_hdl;
>>   
>>   	vdeb->ved.process_frame = vimc_deb_process_frame;
>> +	vdeb->ved.get_frame = vimc_deb_get_frame;
>>   	vdeb->ved.dev = vimc->mdev.dev;
>>   	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
>>   
>> diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
>> index 121fa7d62a2e..347f9cd4a168 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
>> @@ -455,18 +455,29 @@ 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,
>> -				    const void *sink_frame)
>> +static int vimc_sca_process_frame(struct vimc_ent_device *ved)
>>   {
>>   	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>>   						    ved);
>> +	const void *sink_frame;
>> +	struct v4l2_subdev *sd;
>>   
>>   	/* If the stream in this node is not active, just return */
>>   	if (!vsca->src_frame)
>> -		return ERR_PTR(-EINVAL);
>> +		return -EINVAL;
>>   
>> +	sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
>> +	ved = v4l2_get_subdevdata(sd);
>> +	sink_frame = ved->get_frame(ved);
>>   	vimc_sca_fill_src_frame(vsca, sink_frame);
>>   
>> +	return 0;
>> +};
>> +
>> +static void *vimc_sca_get_frame(struct vimc_ent_device *ved)
>> +{
>> +	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>> +						    ved);
>>   	return vsca->src_frame;
>>   };
>>   
>> @@ -505,6 +516,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>   	}
>>   
>>   	vsca->ved.process_frame = vimc_sca_process_frame;
>> +	vsca->ved.get_frame = vimc_sca_get_frame;
>>   	vsca->ved.dev = vimc->mdev.dev;
>>   
>>   	/* Initialize the frame format */
>> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
>> index ba5db5a150b4..32a2c39de2cd 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
>> @@ -190,8 +190,7 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
>>   	.set_fmt		= vimc_sen_set_fmt,
>>   };
>>   
>> -static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
>> -				    const void *sink_frame)
>> +static int vimc_sen_process_frame(struct vimc_ent_device *ved)
>>   {
>>   	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
>>   						    ved);
>> @@ -238,6 +237,13 @@ static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
>>   		break;
>>   	}
>>   
>> +	return 0;
>> +}
>> +
>> +static void *vimc_sen_get_frame(struct vimc_ent_device *ved)
>> +{
>> +	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
>> +						    ved);
>>   	return vsen->frame;
>>   }
>>   
>> @@ -429,6 +435,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>>   		goto err_free_tpg;
>>   
>>   	vsen->ved.process_frame = vimc_sen_process_frame;
>> +	vsen->ved.get_frame = vimc_sen_get_frame;
>>   	vsen->ved.dev = vimc->mdev.dev;
>>   
>>   	/* Initialize the frame format */
>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
>> index 4f8384246042..c1644d69686d 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
>> @@ -125,7 +125,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>   static int vimc_streamer_thread(void *data)
>>   {
>>   	struct vimc_stream *stream = data;
>> -	u8 *frame = NULL;
>> +	struct vimc_ent_device *ved;
>> +	int ret;
>>   	int i;
>>   
>>   	set_freezable();
>> @@ -136,9 +137,10 @@ static int vimc_streamer_thread(void *data)
>>   			break;
>>   
>>   		for (i = stream->pipe_size - 1; i >= 0; i--) {
>> -			frame = stream->ved_pipeline[i]->process_frame(
>> -					stream->ved_pipeline[i], frame);
>> -			if (!frame || IS_ERR(frame))
>> +			ved = stream->ved_pipeline[i];
>> +			ret = ved->process_frame(ved);
>> +
>> +			if (ret)
>>   				break;
>>   		}
>>   		//wait for 60hz
>>
> 

Re: [PATCH v3 3/9] media: vimc: Add usage count to subdevices

[Dafna Hirschfeld]

Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Prepare for multiple video streams from the same sensor by adding a use
> counter to vimc_ent_device. The counter is increased for every s_stream(1)
> and decremented for every s_stream(0) call.
> 
> The subdevice stream is not started or stopped unless the usage count go
> from 0 to 1 (started) or from 1 to 0 (stopped). This allows for multiple
> s_stream() calls to try to either start or stop the device while only
> the first/last call will actually effect the state of the device.
> 
> Initialise and increment use_count for capture as well, as use_count
> will be used in subsequent patches for starting process_frame as well.
> 
> [Kaaira: moved use_count to vimc entity device instead of declaring it
> for each subdevice, used use_count for capture as well and rebased
> the patch on current HEAD of master to help with the current series]
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>

Hi,
maybe incrementing/decrementing the usage count can be
done from the streamer code instead of
the s_stream of each entity?

Thanks,
Dafna

> ---
>   drivers/media/test-drivers/vimc/vimc-capture.c | 3 +++
>   drivers/media/test-drivers/vimc/vimc-common.h  | 2 ++
>   drivers/media/test-drivers/vimc/vimc-debayer.c | 7 +++++++
>   drivers/media/test-drivers/vimc/vimc-scaler.c  | 7 +++++++
>   drivers/media/test-drivers/vimc/vimc-sensor.c  | 6 ++++++
>   5 files changed, 25 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index a8cbb8e4d5ba..93418cb5a139 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -243,6 +243,7 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>   	struct media_entity *entity = &vcap->vdev.entity;
>   	int ret;
>   
> +	atomic_inc(&vcap->ved.use_count);
>   	vcap->sequence = 0;
>   
>   	/* Start the media pipeline */
> @@ -270,6 +271,7 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
>   {
>   	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>   
> +	atomic_dec(&vcap->ved.use_count);
>   	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
>   
>   	/* Stop the media pipeline */
> @@ -424,6 +426,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>   	vcap->vdev.entity.name = vcfg_name;
>   	vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
>   	vcap->pad.flags = MEDIA_PAD_FL_SINK;
> +	atomic_set(&vcap->ved.use_count, 0);
>   	ret = media_entity_pads_init(&vcap->vdev.entity,
>   				     1, &vcap->pad);
>   	if (ret)
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> index 287d66edff49..c214f5ec7818 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -85,6 +85,7 @@ struct vimc_pix_map {
>    *
>    * @dev:		a pointer of the device struct of the driver
>    * @ent:		the pointer to struct media_entity for the node
> + * @use_count:		a count to show the number of streams entity is part of
>    * @get_frame:		callback that sends a frame processed by the entity
>    * @process_frame:	callback that processes a frame
>    * @vdev_get_format:	callback that returns the current format a pad, used
> @@ -102,6 +103,7 @@ struct vimc_pix_map {
>   struct vimc_ent_device {
>   	struct device *dev;
>   	struct media_entity *ent;
> +	atomic_t use_count;
>   	void * (*get_frame)(struct vimc_ent_device *ved);
>   	int (*process_frame)(struct vimc_ent_device *ved);
>   	void (*vdev_get_format)(struct vimc_ent_device *ved,
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index f61e6e8899ac..60c4c0ec2030 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -343,6 +343,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>   		const struct vimc_pix_map *vpix;
>   		unsigned int frame_size;
>   
> +		if (atomic_inc_return(&vdeb->ved.use_count) != 1)
> +			return 0;
> +
>   		if (vdeb->src_frame)
>   			return 0;
>   
> @@ -368,6 +371,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>   			return -ENOMEM;
>   
>   	} else {
> +		if (atomic_dec_return(&vdeb->ved.use_count) != 0)
> +			return 0;
> +
>   		if (!vdeb->src_frame)
>   			return 0;
>   
> @@ -608,6 +614,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>   	vdeb->ved.get_frame = vimc_deb_get_frame;
>   	vdeb->ved.dev = vimc->mdev.dev;
>   	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
> +	atomic_set(&vdeb->ved.use_count, 0);
>   
>   	/* Initialize the frame format */
>   	vdeb->sink_fmt = sink_fmt_default;
> diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> index 347f9cd4a168..d511e1f2152d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> +++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
> @@ -340,6 +340,9 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>   		const struct vimc_pix_map *vpix;
>   		unsigned int frame_size;
>   
> +		if (atomic_inc_return(&vsca->ved.use_count) != 1)
> +			return 0;
> +
>   		if (vsca->src_frame)
>   			return 0;
>   
> @@ -363,6 +366,9 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>   			return -ENOMEM;
>   
>   	} else {
> +		if (atomic_dec_return(&vsca->ved.use_count) != 0)
> +			return 0;
> +
>   		if (!vsca->src_frame)
>   			return 0;
>   
> @@ -518,6 +524,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>   	vsca->ved.process_frame = vimc_sca_process_frame;
>   	vsca->ved.get_frame = vimc_sca_get_frame;
>   	vsca->ved.dev = vimc->mdev.dev;
> +	atomic_set(&vsca->ved.use_count, 0);
>   
>   	/* Initialize the frame format */
>   	vsca->sink_fmt = sink_fmt_default;
> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> index 32a2c39de2cd..ced8ef06b01e 100644
> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> @@ -256,6 +256,9 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>   		const struct vimc_pix_map *vpix;
>   		unsigned int frame_size;
>   
> +		if (atomic_inc_return(&vsen->ved.use_count) != 1)
> +			return 0;
> +
>   		vsen->start_stream_ts = ktime_get_ns();
>   
>   		/* Calculate the frame size */
> @@ -275,6 +278,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>   		vimc_sen_tpg_s_format(vsen);
>   
>   	} else {
> +		if (atomic_dec_return(&vsen->ved.use_count) != 0)
> +			return 0;
>   
>   		vfree(vsen->frame);
>   		vsen->frame = NULL;
> @@ -437,6 +442,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>   	vsen->ved.process_frame = vimc_sen_process_frame;
>   	vsen->ved.get_frame = vimc_sen_get_frame;
>   	vsen->ved.dev = vimc->mdev.dev;
> +	atomic_set(&vsen->ved.use_count, 0);
>   
>   	/* Initialize the frame format */
>   	vsen->mbus_format = fmt_default;
> 

Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation

[Dafna Hirschfeld]

Am 12.09.20 um 12:21 schrieb Kaaira Gupta:
> On Wed, Sep 02, 2020 at 10:56:46AM +0100, Kieran Bingham wrote:
>> Hi Kaaira, Dafna,
>>
>> On 28/08/2020 21:37, Dafna Hirschfeld wrote:
>>> Hi,
>>>
>>> Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
>>>> Hi,
>>>>
>>>> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
>>>>>
>>>>>
>>>>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
>>>>>> Separate the process of initialising pipeline array from starting
>>>>>> streaming for all entities in path of a stream. This is needed because
>>>>>> multiple streams can stream, but only one pipeline object is needed.
>>>>>>
>>>>>> Process frames only for those entities in a pipeline which are
>>>>>> streaming. This is known through their use counts.
>>>>>>
>>>>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
>>>>>> ---
>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 95
>>>>>> ++++++++++++++++---
>>>>>>     1 file changed, 83 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>>>> b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>>>> index c1644d69686d..cc40ecabe95a 100644
>>>>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
>>>>>> @@ -40,33 +40,30 @@ static void
>>>>>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>>>>>>     }
>>>>>>     /**
>>>>>> - * vimc_streamer_pipeline_init - Initializes the stream structure
>>>>>> + * vimc_streamer_stream_start - Starts streaming for all entities
>>>>>> + * in a stream
>>>>>>      *
>>>>>> - * @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 vimc_streamer_s_stream() to enable stream in all entities of
>>>>>> - * the pipeline.
>>>>>> + * Walks through the entity graph to call vimc_streamer_s_stream()
>>>>>> + * to enable stream in all entities in path of a stream.
>>>>>>      *
>>>>>>      * Return: 0 if success, error code otherwise.
>>>>>>      */
>>>>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>>>>> -                       struct vimc_ent_device *ved)
>>>>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream,
>>>>>> +                      struct vimc_ent_device *ved)
>>>>>>     {
>>>>>>         struct media_entity *entity;
>>>>>>         struct video_device *vdev;
>>>>>>         struct v4l2_subdev *sd;
>>>>>> +    int stream_size = 0;
>>>>>>         int ret = 0;
>>>>>> -    stream->pipe_size = 0;
>>>>>> -    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>>>> +    while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>>>>             if (!ved) {
>>>>>>                 vimc_streamer_pipeline_terminate(stream);
>>>>>>                 return -EINVAL;
>>>>>>             }
>>>>>> -        stream->ved_pipeline[stream->pipe_size++] = ved;
>>>>>>             if (is_media_entity_v4l2_subdev(ved->ent)) {
>>>>>>                 sd = media_entity_to_v4l2_subdev(ved->ent);
>>>>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct
>>>>>> vimc_stream *stream,
>>>>>>                             entity);
>>>>>>                 ved = video_get_drvdata(vdev);
>>>>>>             }
>>>>>> +        stream_size++;
>>>>>> +    }
>>>>>> +
>>>>>> +    vimc_streamer_pipeline_terminate(stream);
>>>>>> +    return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
>>>>>> + *
>>>>>> + * @stream: the pointer to the stream structure
>>>>>> + * @ved:    the pointer to the vimc entity initializing the stream
>>>>>> pipeline
>>
>> Which entity is this? Is it the start, or the end of the pipeline? I.e.
>> the sensor? or the capture ?
> 
> It is the capture, I will add it to the documentation..thanks
> 
>>
>>>>>> + *
>>>>>> + * Walks through the entity graph to initialise ved_pipeline and
>>>>>> updates
>>>>>> + * pipe_size too.
>>>>>> + *
>>>>>> + * Return: 0 if success, error code otherwise.
>>>>>> + */
>>>>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>>>>>> +                       struct vimc_ent_device *ved)
>>>>>> +{
>>>>>> +    struct media_entity *entity;
>>>>>> +    struct media_device *mdev;
>>>>>> +    struct media_graph graph;
>>>>>> +    struct video_device *vdev;
>>>>>> +    struct v4l2_subdev *sd;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    entity = ved->ent;
>>>>>> +    mdev = entity->graph_obj.mdev;
>>>>>> +
>>>>>> +    ret = media_graph_walk_init(&graph, mdev);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    media_graph_walk_start(&graph, entity);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Start pipeline array initialisation from RAW Capture only to
>>>>>> get
>>>>>> +     * entities in the correct order of their frame processing.
>>>>>> +     */
>>>>>> +    if (!strncmp(entity->name, "RGB", 3)) {
>>>>>
>>>>> I don't understand this condition, way is it good for?
>>>>
>>>> I have explained that later in the reply
>>
>> Matching on entity names is a bit awkward, as they could be changed I
>> guess quite easily, and easily missed in this string matching.
> 
> Agreed, I need to think of a better way to prevent this
> 
>>
>> I haven't fully understood this code block yet to work out if there's
>> another way we could do this though, but reading ahead I see there might
>> be a way to 'walk the graph' on a per-stream basis which might be a good
>> way of factoring this path.
>>
>> Although there still needs to be a construction of the paths available
>> to a stream which splits from the sensor.
>>
>>
>>>>
>>>>>
>>>>> I think the function should be generic and not assume names of entities
>>>>> or specific topology.
>>>>
>>>> It doesn't assume the topology, rather it is in place just to make sure
>>>> that the entities in ved_pipeline are in correct order.
>>>>
>>>>>
>>>>>
>>>>>> +        entity = media_graph_walk_next(&graph);
>>>>>> +        mdev = entity->graph_obj.mdev;
>>>>>> +        media_graph_walk_cleanup(&graph);
>>>>>> +
>>>>>> +        ret = media_graph_walk_init(&graph, mdev);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +        media_graph_walk_start(&graph, entity);
>>>
>>> Hi, can you explain what this code does?
>>> Why does it start the search in the next entity?
>>>
>>>>>> +    }
>>>>>> +
>>>>>> +    while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
>>>>>> +        if (is_media_entity_v4l2_subdev(entity)) {
>>>>>> +            sd = media_entity_to_v4l2_subdev(entity);
>>>>>> +            ved = v4l2_get_subdevdata(sd);
>>>>>> +        } else {
>>>>>> +            vdev = container_of(entity, struct video_device, entity);
>>>>>> +            ved = video_get_drvdata(vdev);
>>>>>> +        }
>>>>>> +        stream->ved_pipeline[stream->pipe_size++] = ved;
>>>>>> +        entity = media_graph_walk_next(&graph);
>>>>>> +
>>>>>> +        if (!strcmp(entity->name,
>>>>>> stream->ved_pipeline[0]->ent->name)) {
>>>>>
>>>>> I also don't understand this condition
>>>>>
>>>>>> +            media_graph_walk_cleanup(&graph);
>>>>>> +            return 0;
>>>>>> +        }
>>>>>>         }
>>>>>
>>>>> It is not clear what this function does, it looks like it adds to
>>>>> 'ved_pipeline'
>>>>> all entities that are connected to the video node, in addition to the
>>>>> entities
>>>>> that where there from previous calls, so some entities appear several
>>>>> times.
>>>>
>>>> This function returns all the entities in a pipe, weather they are
>>>> streaming or not. For example, if only the RAW Capture 1 streams, or
>>>> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline
>>>> will have the same entities, in exactly same order, and all will occur
>>>> just once.
>>>> Since media_graph_walk goes to each node of the graph, it returns back
>>>> to the first one (as its a graph), hence the last condition, ie,
>>>>
>>>>      if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) {
>>>>
>>>> makes sure that the first entity is not added again to the array.
>>>>
>>>> First condition, ie
>>>>
>>>>      if (!strncmp(entity->name, "RGB", 3)) {
>>>>
>>>> Just makes sure that the search starts at RGB capture only. This is
>>>> because, in case of any other video node, the order of entities, as you
>>>> have mentioned later in the mail, will not be desirable, ie it won't
>>>> start at sensor and end at captures. So this condition just takes care
>>>> that all the entities in ved_pipeline array are in correct order
>>>> (starting at sensor, ending at captures).
>>>
>>> It is better to compare to the whole name of the entity to make it more
>>> clear.
>>> Also I think it is better to document that this function is called only
>>> upon the
>>> first streaming node.
>>
>> If this doesn't end up refactored with other helpers, then indeed a few
>> comments might help the readabilty here. The distinctions of each
>> re-initialisation of the graph walk are hard to interpret the purpose.
>>
>>
>>
>>>
>>> I still think this function should be independent of the topology.
>>> Maybe you can use Helen's patch that allow walking a graph only opposite
>>> to the link direction: https://patchwork.kernel.org/patch/11564899/
>>> This ensures that the Sensor will be first in the graph walk. Then the
>>> streaming thread
>>> iterates the ved_pipeline from 0 upwards and not from 'pipe_size'
>>> downwards.
>>
>> Being able to use a direct helper to walk the pipeline cleanly sounds
>> promising indeed.
>>
>> I suspect at some point int he next patches though - I'm going to see
>> something that needs to have full visibility of all paths enabled from
>> the sensor, as I think I recall that the thread will then process all
>> (enabled) entities in a single pass.
> 
> Yes, that exactly is the problem :( This helper (that dafna has shared)
> can walk through one path, while the thread (which processes the frames)
> needs to view all the entites in all connected paths

Hi, what I think would be a good solution is to first to change the type of ved_pipeline
from an array to a list:
""
- struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
+ struct list_head ved_pipeline;
""

then you can use Helen's patch to iterate the topology in a depth first form only
from sink to source and if an entity is already streaming then you don't add it to the
ved_pipeline list (since it is already there). Adding to the ved_pipeline list
is always to the end of the list. This ensures that for each entity, its source entity
already processed its frame.

This way, you iterate not the entities of the whole pipe but only the ones that are streaming.
Let's look at two scenarios:

1) "RGB/YUV Capture" start streaming and afterwards "Raw Capture 0" start streaming:
After the 'start_stream" of the "RGB/YUV Capture" , the ved_pipeline list is:

Sensor A => Debayer A => Scaler => "RGB/YUV Capture"

After the start_stream of the "Raw Capture 0" the ved_pipeline list is:

Sensor A => Debayer A => Scaler => "RGB/YUV Capture" => "Raw Capture 0"


2) "Raw Capture 0" starts streaming and afterwards "RGB/YUV Capture" start streaming:
After the 'start_stream" of the "Raw Capture 0" , the ved_pipeline list is:

Sensor A => "Raw Capture 0"

After the start_stream of the "RGB/YUV Capture" the ved_pipeline list is:

Sensor A => "Raw Capture 0" => Debayer A => Scaler => "RGB/YUV Capture"

Thanks,
Dafna

> 
>> --
>> Kieran
>>
>>
>>>
>>> Thanks,
>>> Dafna
>>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Kaaira
>>>>
>>>>>
>>>>> I think there is no need to use the graph walk here but to access the
>>>>> source entity
>>>>> in each iteration, the way done in vimc_streamer_stream_start
>>>>> also.
>>>>> I think the code should iterate here until it reaches an entity that
>>>>> is already streaming,
>>>>> this means that the entity is already in the `ved_pipeline`, also you
>>>>> should make sure
>>>>> that the sensor is the first entity that process a frame, therefore
>>>>> the sensor should be
>>>>> at the end/start of the list of entities. Generally each entity
>>>>> should appear exactly once
>>>>> in the 'ved_pipeline' array and the entities should be ordered such
>>>>> that when calling 'process_frame'
>>>>> on one entity should be after calling 'process_frame' on its source
>>>>> entity.
>>>>> maybe it is easyer to implement if 'ved_pipeline' is a linked list.
>>>>>
>>>>> Thanks,
>>>>> Dafna
>>>>>
>>>>>>         vimc_streamer_pipeline_terminate(stream);
>>>>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data)
>>>>>>             for (i = stream->pipe_size - 1; i >= 0; i--) {
>>>>>>                 ved = stream->ved_pipeline[i];
>>>>>> -            ret = ved->process_frame(ved);
>>>>>> +            if (atomic_read(&ved->use_count) == 0)
>>>>>> +                continue;
>>>>>> +
>>>>>> +            ret = ved->process_frame(ved);
>>>>>>                 if (ret)
>>>>>>                     break;
>>>>>>             }
>>>>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream
>>>>>> *stream,
>>>>>>             if (stream->kthread)
>>>>>>                 return 0;
>>>>>> +        ret = vimc_streamer_stream_start(stream, ved);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +
>>>>>>             ret = vimc_streamer_pipeline_init(stream, ved);
>>>>>>             if (ret)
>>>>>>                 return ret;
>>>>>>
>>
>> -- 
>> Regards
>> --
>> Kieran