[PATCH v2 0/3] media: add v4l2_pipeline_stream_{enable,disable} helpers

[PATCH v2 0/3] media: add v4l2_pipeline_stream_{enable,disable} helpers

[Helen Koike]

Hi,

Media drivers need to iterate through the pipeline and call .s_stream()
callbacks in the subdevices.

Instead of repeating code, add helpers for this.

These helpers will go walk through the pipeline only visiting entities
that participates in the stream, i.e. it follows links from sink to source
(and not the opposite).

Which means that in a topology like this https://bit.ly/3b2MxjI
calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
.s_stream(true) for rkisp1_resizer_selfpath.

stream_count variable was added in v4l2_subdevice to handle nested calls
to the helpers.
This is useful when the driver allows streaming from more then one
capture device sharing subdevices.

This patch came from the error I was facing when multistreaming from
rkisp1 driver, where stoping one capture would call s_stream(false) in
the pipeline, causing a stall in the second capture device.

Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
be required with this patchset.

This patchset was tested on rkisp1 and vimc drivers.

Other cleanup might be possible (but I won't add in this patchset as I
don't have the hw to test):
	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97

Changes in V2:
====================
The first version was calling the s_stream() callbacks from sensor to
capture.

This was generating errors in the Scarlet Chromebook, when the sensor
was being enabled before the ISP.

It make sense to enable subdevices from capture to sensor instead (which
is what most drivers do already).

This v2 drops the changes from mc-entity.c, and re-implement helpers in
v4l2-common.c

Overview of patches:
====================

Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
adding stream_count in the subdevice struct.

Patch 2/3 cleanup rkisp1 driver to use the helpers.

Patch 3/3 cleanup vimc driver to use the helpers.

Helen Koike (3):
  media: v4l2-common: add helper functions to call s_stream() callbacks
  media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
    helpers
  media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers

 drivers/media/platform/vimc/vimc-capture.c    |  28 +++--
 drivers/media/platform/vimc/vimc-streamer.c   |  49 +-------
 drivers/media/v4l2-core/v4l2-common.c         | 117 ++++++++++++++++++
 drivers/staging/media/rkisp1/rkisp1-capture.c |  76 +-----------
 include/media/v4l2-common.h                   |  28 +++++
 include/media/v4l2-subdev.h                   |   2 +
 6 files changed, 173 insertions(+), 127 deletions(-)

-- 
2.26.0

[PATCH v2 1/3] media: v4l2-common: add helper functions to call s_stream() callbacks

[Helen Koike]

Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate
through the subdevices in a given stream (i.e following links from sink
to source) and call .s_stream() callback.

Add stream_count on the subdevice object for simultaneous streaming from
different video devices which shares subdevices.

Prevent calling .s_stream(true) if it was already called previously by
another stream.

Prevent calling .s_stream(false) from one stream when there are still
others active.

If .s_stream(true) fails, call .s_stream(false) in the reverse order.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Changes in v2:
- re-write helpers to not use media walkers

 drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++
 include/media/v4l2-common.h           |  28 ++++++
 include/media/v4l2-subdev.h           |   2 +
 3 files changed, 147 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index d0e5ebc736f9f..379d4bf4f8128 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
+
+/*
+ * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline
+ * @subdevs: the array to be filled
+ * @size: the array size
+ *
+ * Walk from a video node, following link from sink to source and fill the
+ * array with subdevices in the path.
+ *
+ * Note: this function follows the first enabled link in a sink pad found in a
+ * given entity. Thus it won't work if there are entities with multiple enabled
+ * links to its sink pads in the topology.
+ *
+ * Return the number of subdevices filled in the array.
+ */
+static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev,
+					      struct v4l2_subdev **subdevs,
+					      unsigned int size)
+{
+	struct media_entity *entity = &vdev->entity;
+	unsigned int idx = 0;
+
+	while (1) {
+		struct media_pad *src_pad = NULL;
+		unsigned int i;
+
+		/* Find remote source pad */
+		for (i = 0; i < entity->num_pads; i++) {
+			struct media_pad *sink_pad = &entity->pads[i];
+
+			if (!(sink_pad->flags & MEDIA_PAD_FL_SINK))
+				continue;
+
+			src_pad = media_entity_remote_pad(sink_pad);
+			if (src_pad &&
+			    is_media_entity_v4l2_subdev(src_pad->entity))
+				break;
+		}
+		if (i == entity->num_pads)
+			break;
+
+		if (idx >= size) {
+			WARN_ON(1);
+			return 0;
+		}
+
+		entity = src_pad->entity;
+		subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
+	}
+
+	return idx;
+}
+
+__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
+{
+	struct media_device *mdev = vdev->entity.graph_obj.mdev;
+	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
+	struct v4l2_subdev *sd;
+	unsigned int i, size;
+	int ret;
+
+	mutex_lock(&mdev->graph_mutex);
+
+	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
+
+	for (i = 0; i < size; i++) {
+		sd = subdevs[i];
+		if (sd->stream_count++)
+			continue;
+		dev_dbg(mdev->dev,
+			"enabling stream for '%s'\n", sd->entity.name);
+		ret = v4l2_subdev_call(sd, video, s_stream, true);
+		if (ret && ret != -ENOIOCTLCMD)
+			goto err_stream_disable;
+	}
+
+	mutex_unlock(&mdev->graph_mutex);
+	return 0;
+
+err_stream_disable:
+	do {
+		sd = subdevs[i];
+		if (--sd->stream_count)
+			continue;
+		dev_dbg(mdev->dev,
+			"disabling stream for '%s'\n", sd->entity.name);
+		v4l2_subdev_call(sd, video, s_stream, false);
+	} while (i--);
+
+	mutex_unlock(&mdev->graph_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
+
+void v4l2_pipeline_stream_disable(struct video_device *vdev)
+{
+	struct media_device *mdev = vdev->entity.graph_obj.mdev;
+	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
+	unsigned int size;
+
+	mutex_lock(&mdev->graph_mutex);
+
+	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
+
+	while (size--) {
+		struct v4l2_subdev *sd = subdevs[size];
+
+		if (--sd->stream_count)
+			continue;
+		dev_dbg(mdev->dev,
+			"disabling stream for '%s'\n", sd->entity.name);
+		v4l2_subdev_call(sd, video, s_stream, false);
+	}
+
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 150ee16ebd811..e833610b0f66d 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
 			u32 width, u32 height);
 
+/**
+ * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream
+ * @vdev: Starting video device
+ *
+ * Call .s_stream(true) callback in all the subdevices participating in the
+ * stream, i.e. following links from sink to source.
+ *
+ * Calls to this function can be nested, in which case the same number of
+ * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
+ * The  pipeline pointer must be identical for all nested calls to
+ * v4l2_pipeline_stream_enable().
+ */
+__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev);
+
+/**
+ * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream
+ * @vdev: Starting video device
+ *
+ * Call .s_stream(true) callback in all the subdevices participating in the
+ * stream, i.e. following links from sink to source.
+ *
+ * Calls to this function can be nested, in which case the same number of
+ * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
+ * The  pipeline pointer must be identical for all nested calls to
+ * v4l2_pipeline_stream_enable().
+ */
+void v4l2_pipeline_stream_disable(struct video_device *vdev);
+
 static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
 {
 	/*
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a4848de598521..20f913a9f70c5 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data {
  * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
  *		     device using v4l2_device_register_sensor_subdev().
  * @pdata: common part of subdevice platform data
+ * @stream_count: Stream count for the subdevice.
  *
  * Each instance of a subdev driver should create this struct, either
  * stand-alone or embedded in a larger struct.
@@ -869,6 +870,7 @@ struct v4l2_subdev {
 	struct v4l2_async_notifier *notifier;
 	struct v4l2_async_notifier *subdev_notifier;
 	struct v4l2_subdev_platform_data *pdata;
+	unsigned int stream_count;
 };
 
 
-- 
2.26.0

[PATCH v2 2/3] media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable} helpers

[Helen Koike]

Use v4l2_pipeline_stream_{enable,disable} to call .s_stream() subdevice
callbacks through the pipeline.

Tested by streaming on RockPi4 with imx219 and on Scarlet Chromebook.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Changes in v2:
- rebase on top of new helpers prototypes

 drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +------------------
 1 file changed, 3 insertions(+), 73 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 24fe6a7888aa4..0c2a357c4a12a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -838,71 +838,6 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap,
 	spin_unlock_irqrestore(&cap->buf.lock, flags);
 }
 
-/*
- * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb
- * @from: entity at which to start pipeline walk
- * @until: entity at which to stop pipeline walk
- *
- * Walk the entities chain starting at the pipeline video node and stop
- * all subdevices in the chain.
- *
- * If the until argument isn't NULL, stop the pipeline walk when reaching the
- * until entity. This is used to disable a partially started pipeline due to a
- * subdev start error.
- */
-static int rkisp1_pipeline_sink_walk(struct media_entity *from,
-				     struct media_entity *until,
-				     int (*cb)(struct media_entity *from,
-					       struct media_entity *curr))
-{
-	struct media_entity *entity = from;
-	struct media_pad *pad;
-	unsigned int i;
-	int ret;
-
-	while (1) {
-		pad = NULL;
-		/* Find remote source pad */
-		for (i = 0; i < entity->num_pads; i++) {
-			struct media_pad *spad = &entity->pads[i];
-
-			if (!(spad->flags & MEDIA_PAD_FL_SINK))
-				continue;
-			pad = media_entity_remote_pad(spad);
-			if (pad && is_media_entity_v4l2_subdev(pad->entity))
-				break;
-		}
-		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
-			break;
-
-		entity = pad->entity;
-		if (entity == until)
-			break;
-
-		ret = cb(from, entity);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
-static int rkisp1_pipeline_disable_cb(struct media_entity *from,
-				      struct media_entity *curr)
-{
-	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
-
-	return v4l2_subdev_call(sd, video, s_stream, false);
-}
-
-static int rkisp1_pipeline_enable_cb(struct media_entity *from,
-				     struct media_entity *curr)
-{
-	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
-
-	return v4l2_subdev_call(sd, video, s_stream, true);
-}
-
 static void rkisp1_stream_stop(struct rkisp1_capture *cap)
 {
 	int ret;
@@ -929,11 +864,7 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue)
 
 	rkisp1_stream_stop(cap);
 	media_pipeline_stop(&node->vdev.entity);
-	ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL,
-					rkisp1_pipeline_disable_cb);
-	if (ret)
-		dev_err(rkisp1->dev,
-			"pipeline stream-off failed error:%d\n", ret);
+	v4l2_pipeline_stream_disable(&node->vdev);
 
 	rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR);
 
@@ -1005,8 +936,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	rkisp1_stream_start(cap);
 
 	/* start sub-devices */
-	ret = rkisp1_pipeline_sink_walk(entity, NULL,
-					rkisp1_pipeline_enable_cb);
+	ret = v4l2_pipeline_stream_enable(&cap->vnode.vdev);
 	if (ret)
 		goto err_stop_stream;
 
@@ -1019,7 +949,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	return 0;
 
 err_pipe_disable:
-	rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb);
+	v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);
 err_stop_stream:
 	rkisp1_stream_stop(cap);
 	v4l2_pipeline_pm_put(entity);
-- 
2.26.0

[PATCH v2 3/3] media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers

[Helen Koike]

Use v4l2_pipeline_stream_{enable,disable} to call .s_stream() subdevice
callbacks through the pipeline.

Tested streaming works with:

media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/1920x1440]'
v4l2-ctl -d /dev/video2 -v width=1200,height=450
v4l2-ctl -d /dev/video0 -v pixelformat=BA81
v4l2-ctl -d /dev/video1 -v pixelformat=BA81
v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 --stream-to=/tmp/test.raw

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Changes in v2:
- rebase on top of new helpers prototypes

 drivers/media/platform/vimc/vimc-capture.c  | 28 +++++++-----
 drivers/media/platform/vimc/vimc-streamer.c | 49 +++------------------
 2 files changed, 23 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 23e740c1c5c00..2206bfdec1c32 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -233,21 +233,27 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	vcap->sequence = 0;
 
-	/* Start the media pipeline */
 	ret = media_pipeline_start(entity, &vcap->stream.pipe);
-	if (ret) {
-		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
-		return ret;
-	}
+	if (ret)
+		goto err_return_all_buffers;
+
+	ret = v4l2_pipeline_stream_enable(&vcap->vdev);
+	if (ret)
+		goto err_stop_media_pipe;
 
 	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
-	if (ret) {
-		media_pipeline_stop(entity);
-		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
-		return ret;
-	}
+	if (ret)
+		goto err_stop_stream;
 
 	return 0;
+
+err_stop_stream:
+	v4l2_pipeline_stream_disable(&vcap->vdev);
+err_stop_media_pipe:
+	media_pipeline_stop(entity);
+err_return_all_buffers:
+	vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
+	return ret;
 }
 
 /*
@@ -260,6 +266,8 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
 
 	vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
 
+	v4l2_pipeline_stream_disable(&vcap->vdev);
+
 	/* Stop the media pipeline */
 	media_pipeline_stop(&vcap->vdev.entity);
 
diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
index 65feb3c596db5..c0085f4695c2f 100644
--- a/drivers/media/platform/vimc/vimc-streamer.c
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -36,33 +36,6 @@ static struct media_entity *vimc_get_source_entity(struct media_entity *ent)
 	return NULL;
 }
 
-/**
- * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
- *
- * @stream: the pointer to the stream structure with the pipeline to be
- *	    disabled.
- *
- * Calls s_stream to disable the stream in each entity of the pipeline
- *
- */
-static void vimc_streamer_pipeline_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;
-
-		sd = media_entity_to_v4l2_subdev(ved->ent);
-		v4l2_subdev_call(sd, video, s_stream, 0);
-	}
-}
-
 /**
  * vimc_streamer_pipeline_init - Initializes the stream structure
  *
@@ -82,27 +55,15 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 	struct media_entity *entity;
 	struct video_device *vdev;
 	struct v4l2_subdev *sd;
-	int ret = 0;
 
 	stream->pipe_size = 0;
 	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
 		if (!ved) {
-			vimc_streamer_pipeline_terminate(stream);
+			stream->pipe_size = 0;
 			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);
-			ret = v4l2_subdev_call(sd, video, s_stream, 1);
-			if (ret && ret != -ENOIOCTLCMD) {
-				dev_err(ved->dev, "subdev_call error %s\n",
-					ved->ent->name);
-				vimc_streamer_pipeline_terminate(stream);
-				return ret;
-			}
-		}
-
 		entity = vimc_get_source_entity(ved->ent);
 		/* Check if the end of the pipeline was reached */
 		if (!entity) {
@@ -111,7 +72,7 @@ static int vimc_streamer_pipeline_init(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);
+				stream->pipe_size = 0;
 				return -EPIPE;
 			}
 			return 0;
@@ -129,7 +90,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 		}
 	}
 
-	vimc_streamer_pipeline_terminate(stream);
+	stream->pipe_size = 0;
 	return -EINVAL;
 }
 
@@ -210,7 +171,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_pipeline_terminate(stream);
+			stream->pipe_size = 0;
 			stream->kthread = NULL;
 			return ret;
 		}
@@ -231,7 +192,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 
 		stream->kthread = NULL;
 
-		vimc_streamer_pipeline_terminate(stream);
+		stream->pipe_size = 0;
 	}
 
 	return 0;
-- 
2.26.0

Re: [PATCH v2 1/3] media: v4l2-common: add helper functions to call s_stream() callbacks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9817 bytes --]

Hi Helen,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.6 next-20200404]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Helen-Koike/media-add-v4l2_pipeline_stream_-enable-disable-helpers/20200405-050004
base:   git://linuxtv.org/media_tree.git master
config: s390-randconfig-a001-20200405 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.3.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_subdevs_get':
>> drivers/media/v4l2-core/v4l2-common.c:463:37: error: 'struct video_device' has no member named 'entity'
     463 |  struct media_entity *entity = &vdev->entity;
         |                                     ^~
   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/media/v4l2-core/v4l2-common.c:37:
>> include/linux/kernel.h:987:51: error: 'struct v4l2_subdev' has no member named 'entity'
     987 |  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
         |                                                   ^~
   include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert'
     330 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
     350 |  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:987:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     987 |  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
         |  ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:987:20: note: in expansion of macro '__same_type'
     987 |  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
         |                    ^~~~~~~~~~~
>> include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
     888 |   container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
         |   ^~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
     491 |   subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from <command-line>:
>> include/linux/compiler_types.h:129:35: error: 'struct v4l2_subdev' has no member named 'entity'
     129 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
         |                                   ^~~~~~~~~~~~~~~~~~
   include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
      17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
         |                                ^~~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:990:21: note: in expansion of macro 'offsetof'
     990 |  ((type *)(__mptr - offsetof(type, member))); })
         |                     ^~~~~~~~
>> include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
     888 |   container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
         |   ^~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
     491 |   subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_enable':
   drivers/media/v4l2-core/v4l2-common.c:499:34: error: 'struct video_device' has no member named 'entity'
     499 |  struct media_device *mdev = vdev->entity.graph_obj.mdev;
         |                                  ^~
   In file included from include/linux/device.h:15,
                    from include/media/v4l2-dev.h:15,
                    from include/media/v4l2-common.h:18,
                    from drivers/media/v4l2-core/v4l2-common.c:47:
>> drivers/media/v4l2-core/v4l2-common.c:514:36: error: 'struct v4l2_subdev' has no member named 'entity'
     514 |    "enabling stream for '%s'\n", sd->entity.name);
         |                                    ^~
   include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
     122 |   dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
         |                                               ^~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:529:37: error: 'struct v4l2_subdev' has no member named 'entity'
     529 |    "disabling stream for '%s'\n", sd->entity.name);
         |                                     ^~
   include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
     122 |   dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
         |                                               ^~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_disable':
   drivers/media/v4l2-core/v4l2-common.c:540:34: error: 'struct video_device' has no member named 'entity'
     540 |  struct media_device *mdev = vdev->entity.graph_obj.mdev;
         |                                  ^~
   In file included from include/linux/device.h:15,
                    from include/media/v4l2-dev.h:15,
                    from include/media/v4l2-common.h:18,
                    from drivers/media/v4l2-core/v4l2-common.c:47:
   drivers/media/v4l2-core/v4l2-common.c:554:37: error: 'struct v4l2_subdev' has no member named 'entity'
     554 |    "disabling stream for '%s'\n", sd->entity.name);
         |                                     ^~
   include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
     122 |   dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
         |                                               ^~~~~~~~~~~

vim +463 drivers/media/v4l2-core/v4l2-common.c

   444	
   445	/*
   446	 * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline
   447	 * @subdevs: the array to be filled
   448	 * @size: the array size
   449	 *
   450	 * Walk from a video node, following link from sink to source and fill the
   451	 * array with subdevices in the path.
   452	 *
   453	 * Note: this function follows the first enabled link in a sink pad found in a
   454	 * given entity. Thus it won't work if there are entities with multiple enabled
   455	 * links to its sink pads in the topology.
   456	 *
   457	 * Return the number of subdevices filled in the array.
   458	 */
   459	static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev,
   460						      struct v4l2_subdev **subdevs,
   461						      unsigned int size)
   462	{
 > 463		struct media_entity *entity = &vdev->entity;
   464		unsigned int idx = 0;
   465	
   466		while (1) {
   467			struct media_pad *src_pad = NULL;
   468			unsigned int i;
   469	
   470			/* Find remote source pad */
   471			for (i = 0; i < entity->num_pads; i++) {
   472				struct media_pad *sink_pad = &entity->pads[i];
   473	
   474				if (!(sink_pad->flags & MEDIA_PAD_FL_SINK))
   475					continue;
   476	
   477				src_pad = media_entity_remote_pad(sink_pad);
   478				if (src_pad &&
   479				    is_media_entity_v4l2_subdev(src_pad->entity))
   480					break;
   481			}
   482			if (i == entity->num_pads)
   483				break;
   484	
   485			if (idx >= size) {
   486				WARN_ON(1);
   487				return 0;
   488			}
   489	
   490			entity = src_pad->entity;
 > 491			subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
   492		}
   493	
   494		return idx;
   495	}
   496	
   497	__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
   498	{
 > 499		struct media_device *mdev = vdev->entity.graph_obj.mdev;
   500		struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
   501		struct v4l2_subdev *sd;
   502		unsigned int i, size;
   503		int ret;
   504	
   505		mutex_lock(&mdev->graph_mutex);
   506	
   507		size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
   508	
   509		for (i = 0; i < size; i++) {
   510			sd = subdevs[i];
   511			if (sd->stream_count++)
   512				continue;
   513			dev_dbg(mdev->dev,
 > 514				"enabling stream for '%s'\n", sd->entity.name);
   515			ret = v4l2_subdev_call(sd, video, s_stream, true);
   516			if (ret && ret != -ENOIOCTLCMD)
   517				goto err_stream_disable;
   518		}
   519	
   520		mutex_unlock(&mdev->graph_mutex);
   521		return 0;
   522	
   523	err_stream_disable:
   524		do {
   525			sd = subdevs[i];
   526			if (--sd->stream_count)
   527				continue;
   528			dev_dbg(mdev->dev,
   529				"disabling stream for '%s'\n", sd->entity.name);
   530			v4l2_subdev_call(sd, video, s_stream, false);
   531		} while (i--);
   532	
   533		mutex_unlock(&mdev->graph_mutex);
   534		return ret;
   535	}
   536	EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
   537	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33388 bytes --]

Re: [PATCH v2 2/3] media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable} helpers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4227 bytes --]

Hi Helen,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.6 next-20200404]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Helen-Koike/media-add-v4l2_pipeline_stream_-enable-disable-helpers/20200405-050004
base:   git://linuxtv.org/media_tree.git master
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/rkisp1/rkisp1-capture.c: In function 'rkisp1_vb2_start_streaming':
>> drivers/staging/media/rkisp1/rkisp1-capture.c:952:31: error: passing argument 1 of 'v4l2_pipeline_stream_disable' from incompatible pointer type [-Werror=incompatible-pointer-types]
     952 |  v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);
         |                               ^~~~~~
         |                               |
         |                               struct media_entity *
   In file included from drivers/staging/media/rkisp1/rkisp1-capture.c:13:
   include/media/v4l2-common.h:548:56: note: expected 'struct video_device *' but argument is of type 'struct media_entity *'
     548 | void v4l2_pipeline_stream_disable(struct video_device *vdev);
         |                                   ~~~~~~~~~~~~~~~~~~~~~^~~~
>> drivers/staging/media/rkisp1/rkisp1-capture.c:952:2: error: too many arguments to function 'v4l2_pipeline_stream_disable'
     952 |  v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/staging/media/rkisp1/rkisp1-capture.c:13:
   include/media/v4l2-common.h:548:6: note: declared here
     548 | void v4l2_pipeline_stream_disable(struct video_device *vdev);
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/v4l2_pipeline_stream_disable +952 drivers/staging/media/rkisp1/rkisp1-capture.c

   913	
   914	static int
   915	rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
   916	{
   917		struct rkisp1_capture *cap = queue->drv_priv;
   918		struct media_entity *entity = &cap->vnode.vdev.entity;
   919		int ret;
   920	
   921		ret = rkisp1_dummy_buf_create(cap);
   922		if (ret)
   923			goto err_ret_buffers;
   924	
   925		ret = pm_runtime_get_sync(cap->rkisp1->dev);
   926		if (ret) {
   927			dev_err(cap->rkisp1->dev, "power up failed %d\n", ret);
   928			goto err_destroy_dummy;
   929		}
   930		ret = v4l2_pipeline_pm_get(entity);
   931		if (ret) {
   932			dev_err(cap->rkisp1->dev, "open cif pipeline failed %d\n", ret);
   933			goto err_pipe_pm_put;
   934		}
   935	
   936		rkisp1_stream_start(cap);
   937	
   938		/* start sub-devices */
   939		ret = v4l2_pipeline_stream_enable(&cap->vnode.vdev);
   940		if (ret)
   941			goto err_stop_stream;
   942	
   943		ret = media_pipeline_start(entity, &cap->rkisp1->pipe);
   944		if (ret) {
   945			dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret);
   946			goto err_pipe_disable;
   947		}
   948	
   949		return 0;
   950	
   951	err_pipe_disable:
 > 952		v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);
   953	err_stop_stream:
   954		rkisp1_stream_stop(cap);
   955		v4l2_pipeline_pm_put(entity);
   956	err_pipe_pm_put:
   957		pm_runtime_put(cap->rkisp1->dev);
   958	err_destroy_dummy:
   959		rkisp1_dummy_buf_destroy(cap);
   960	err_ret_buffers:
   961		rkisp1_return_all_buffers(cap, VB2_BUF_STATE_QUEUED);
   962	
   963		return ret;
   964	}
   965	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53907 bytes --]

Re: [PATCH v2 1/3] media: v4l2-common: add helper functions to call s_stream() callbacks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9058 bytes --]

Hi Helen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.6 next-20200404]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Helen-Koike/media-add-v4l2_pipeline_stream_-enable-disable-helpers/20200405-050004
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-g003-20200405 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_subdevs_get':
   drivers/media/v4l2-core/v4l2-common.c:463:37: error: 'struct video_device' has no member named 'entity'
     struct media_entity *entity = &vdev->entity;
                                        ^~
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/media/v4l2-core/v4l2-common.c:37:
   include/linux/kernel.h:987:51: error: 'struct v4l2_subdev' has no member named 'entity'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
                                                      ^
   include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert'
      if (!(condition))     \
            ^~~~~~~~~
   include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:987:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
     ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:987:20: note: in expansion of macro '__same_type'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
                       ^~~~~~~~~~~
   include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
      container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
      ^~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
      subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from <command-line>:0:0:
   include/linux/compiler_types.h:129:35: error: 'struct v4l2_subdev' has no member named 'entity'
    #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
                                      ^
   include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
    #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
                                   ^~~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:990:21: note: in expansion of macro 'offsetof'
     ((type *)(__mptr - offsetof(type, member))); })
                        ^~~~~~~~
   include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
      container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
      ^~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
      subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_enable':
   drivers/media/v4l2-core/v4l2-common.c:499:34: error: 'struct video_device' has no member named 'entity'
     struct media_device *mdev = vdev->entity.graph_obj.mdev;
                                     ^~
   In file included from include/linux/printk.h:331:0,
                    from include/linux/kernel.h:15,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/media/v4l2-core/v4l2-common.c:37:
   drivers/media/v4l2-core/v4l2-common.c:514:36: error: 'struct v4l2_subdev' has no member named 'entity'
       "enabling stream for '%s'\n", sd->entity.name);
                                       ^
   include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
      func(&id, ##__VA_ARGS__);  \
                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
     ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:513:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(mdev->dev,
      ^~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:529:37: error: 'struct v4l2_subdev' has no member named 'entity'
       "disabling stream for '%s'\n", sd->entity.name);
                                        ^
   include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
      func(&id, ##__VA_ARGS__);  \
                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
     ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:528:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(mdev->dev,
      ^~~~~~~
   drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_disable':
   drivers/media/v4l2-core/v4l2-common.c:540:34: error: 'struct video_device' has no member named 'entity'
     struct media_device *mdev = vdev->entity.graph_obj.mdev;
                                     ^~
   In file included from include/linux/printk.h:331:0,
                    from include/linux/kernel.h:15,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/media/v4l2-core/v4l2-common.c:37:
   drivers/media/v4l2-core/v4l2-common.c:554:37: error: 'struct v4l2_subdev' has no member named 'entity'
       "disabling stream for '%s'\n", sd->entity.name);
                                        ^
   include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
      func(&id, ##__VA_ARGS__);  \
                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
     ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-common.c:553:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(mdev->dev,
      ^~~~~~~

vim +/dev_dbg +513 drivers/media/v4l2-core/v4l2-common.c

   496	
   497	__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
   498	{
   499		struct media_device *mdev = vdev->entity.graph_obj.mdev;
   500		struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
   501		struct v4l2_subdev *sd;
   502		unsigned int i, size;
   503		int ret;
   504	
   505		mutex_lock(&mdev->graph_mutex);
   506	
   507		size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
   508	
   509		for (i = 0; i < size; i++) {
   510			sd = subdevs[i];
   511			if (sd->stream_count++)
   512				continue;
 > 513			dev_dbg(mdev->dev,
   514				"enabling stream for '%s'\n", sd->entity.name);
   515			ret = v4l2_subdev_call(sd, video, s_stream, true);
   516			if (ret && ret != -ENOIOCTLCMD)
   517				goto err_stream_disable;
   518		}
   519	
   520		mutex_unlock(&mdev->graph_mutex);
   521		return 0;
   522	
   523	err_stream_disable:
   524		do {
   525			sd = subdevs[i];
   526			if (--sd->stream_count)
   527				continue;
   528			dev_dbg(mdev->dev,
   529				"disabling stream for '%s'\n", sd->entity.name);
   530			v4l2_subdev_call(sd, video, s_stream, false);
   531		} while (i--);
   532	
   533		mutex_unlock(&mdev->graph_mutex);
   534		return ret;
   535	}
   536	EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
   537	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37174 bytes --]

Re: [PATCH v2 1/3] media: v4l2-common: add helper functions to call s_stream() callbacks

[Niklas Söderlund]

Hi Helen,

Thanks for your work.

On 2020-04-03 18:33:10 -0300, Helen Koike wrote:
> Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate
> through the subdevices in a given stream (i.e following links from sink
> to source) and call .s_stream() callback.
> 
> Add stream_count on the subdevice object for simultaneous streaming from
> different video devices which shares subdevices.
> 
> Prevent calling .s_stream(true) if it was already called previously by
> another stream.
> 
> Prevent calling .s_stream(false) from one stream when there are still
> others active.
> 
> If .s_stream(true) fails, call .s_stream(false) in the reverse order.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> Changes in v2:
> - re-write helpers to not use media walkers
> 
>  drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++
>  include/media/v4l2-common.h           |  28 ++++++
>  include/media/v4l2-subdev.h           |   2 +
>  3 files changed, 147 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index d0e5ebc736f9f..379d4bf4f8128 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> +
> +/*
> + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline
> + * @subdevs: the array to be filled
> + * @size: the array size

Should this be documented as the maximum number of elements that can fit 
in the subdevs array?

> + *
> + * Walk from a video node, following link from sink to source and fill the
> + * array with subdevices in the path.
> + *
> + * Note: this function follows the first enabled link in a sink pad found in a
> + * given entity. Thus it won't work if there are entities with multiple enabled
> + * links to its sink pads in the topology.

I wonder if it would be more useful to make this function return all 
subdevs in the pipeline that have an enabled link going from sink to 
source while walking from the video device?

When reading the commit messages I thought this could be useful for the 
rcar-vin driver. By not finding all subdevices in the pipeline this 
would not be possible as there on some platforms are a CSI-2 bus where 
the CSI-2 transmitter have 4 sink pads and one source pads so the whole 
pipeline would not be started.

> + *
> + * Return the number of subdevices filled in the array.
> + */
> +static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev,
> +					      struct v4l2_subdev **subdevs,
> +					      unsigned int size)
> +{
> +	struct media_entity *entity = &vdev->entity;
> +	unsigned int idx = 0;
> +
> +	while (1) {
> +		struct media_pad *src_pad = NULL;
> +		unsigned int i;
> +
> +		/* Find remote source pad */
> +		for (i = 0; i < entity->num_pads; i++) {
> +			struct media_pad *sink_pad = &entity->pads[i];
> +
> +			if (!(sink_pad->flags & MEDIA_PAD_FL_SINK))
> +				continue;
> +
> +			src_pad = media_entity_remote_pad(sink_pad);
> +			if (src_pad &&
> +			    is_media_entity_v4l2_subdev(src_pad->entity))
> +				break;
> +		}
> +		if (i == entity->num_pads)
> +			break;
> +
> +		if (idx >= size) {
> +			WARN_ON(1);
> +			return 0;

Would it make sens to have this function return int and a negative error 
code here? Is this now how other areas of V4L2 deal with when a provided 
array is too small ? I'm thinking about if this function could be 
exported for use by drivers in the future.

> +		}
> +
> +		entity = src_pad->entity;
> +		subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
> +	}
> +
> +	return idx;
> +}
> +
> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
> +{
> +	struct media_device *mdev = vdev->entity.graph_obj.mdev;
> +	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
> +	struct v4l2_subdev *sd;
> +	unsigned int i, size;
> +	int ret;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +
> +	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
> +
> +	for (i = 0; i < size; i++) {
> +		sd = subdevs[i];
> +		if (sd->stream_count++)
> +			continue;
> +		dev_dbg(mdev->dev,
> +			"enabling stream for '%s'\n", sd->entity.name);

Small nit, would it make sens to print the sd->stream_count also in this 
debug statement (and the similar ones bellow) ?

> +		ret = v4l2_subdev_call(sd, video, s_stream, true);
> +		if (ret && ret != -ENOIOCTLCMD)
> +			goto err_stream_disable;
> +	}
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +	return 0;
> +
> +err_stream_disable:
> +	do {
> +		sd = subdevs[i];
> +		if (--sd->stream_count)
> +			continue;
> +		dev_dbg(mdev->dev,
> +			"disabling stream for '%s'\n", sd->entity.name);
> +		v4l2_subdev_call(sd, video, s_stream, false);
> +	} while (i--);
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
> +
> +void v4l2_pipeline_stream_disable(struct video_device *vdev)
> +{
> +	struct media_device *mdev = vdev->entity.graph_obj.mdev;
> +	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
> +	unsigned int size;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +
> +	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
> +
> +	while (size--) {
> +		struct v4l2_subdev *sd = subdevs[size];
> +
> +		if (--sd->stream_count)
> +			continue;
> +		dev_dbg(mdev->dev,
> +			"disabling stream for '%s'\n", sd->entity.name);
> +		v4l2_subdev_call(sd, video, s_stream, false);
> +	}
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 150ee16ebd811..e833610b0f66d 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  			u32 width, u32 height);
>  
> +/**
> + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream
> + * @vdev: Starting video device
> + *
> + * Call .s_stream(true) callback in all the subdevices participating in the
> + * stream, i.e. following links from sink to source.
> + *
> + * Calls to this function can be nested, in which case the same number of
> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
> + * The  pipeline pointer must be identical for all nested calls to
> + * v4l2_pipeline_stream_enable().
> + */
> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev);
> +
> +/**
> + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream
> + * @vdev: Starting video device
> + *
> + * Call .s_stream(true) callback in all the subdevices participating in the
> + * stream, i.e. following links from sink to source.
> + *
> + * Calls to this function can be nested, in which case the same number of
> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
> + * The  pipeline pointer must be identical for all nested calls to
> + * v4l2_pipeline_stream_enable().
> + */
> +void v4l2_pipeline_stream_disable(struct video_device *vdev);
> +
>  static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
>  {
>  	/*
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a4848de598521..20f913a9f70c5 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data {
>   * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>   *		     device using v4l2_device_register_sensor_subdev().
>   * @pdata: common part of subdevice platform data
> + * @stream_count: Stream count for the subdevice.
>   *
>   * Each instance of a subdev driver should create this struct, either
>   * stand-alone or embedded in a larger struct.
> @@ -869,6 +870,7 @@ struct v4l2_subdev {
>  	struct v4l2_async_notifier *notifier;
>  	struct v4l2_async_notifier *subdev_notifier;
>  	struct v4l2_subdev_platform_data *pdata;
> +	unsigned int stream_count;
>  };
>  
>  
> -- 
> 2.26.0
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/3] media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable} helpers

[Niklas Söderlund]

Hi Helen,

Thanks for your work.

On 2020-04-03 18:33:11 -0300, Helen Koike wrote:
> Use v4l2_pipeline_stream_{enable,disable} to call .s_stream() subdevice
> callbacks through the pipeline.
> 
> Tested by streaming on RockPi4 with imx219 and on Scarlet Chromebook.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> Changes in v2:
> - rebase on top of new helpers prototypes
> 
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +------------------
>  1 file changed, 3 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 24fe6a7888aa4..0c2a357c4a12a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -838,71 +838,6 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap,
>  	spin_unlock_irqrestore(&cap->buf.lock, flags);
>  }
>  
> -/*
> - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb
> - * @from: entity at which to start pipeline walk
> - * @until: entity at which to stop pipeline walk
> - *
> - * Walk the entities chain starting at the pipeline video node and stop
> - * all subdevices in the chain.
> - *
> - * If the until argument isn't NULL, stop the pipeline walk when reaching the
> - * until entity. This is used to disable a partially started pipeline due to a
> - * subdev start error.
> - */
> -static int rkisp1_pipeline_sink_walk(struct media_entity *from,
> -				     struct media_entity *until,
> -				     int (*cb)(struct media_entity *from,
> -					       struct media_entity *curr))
> -{
> -	struct media_entity *entity = from;
> -	struct media_pad *pad;
> -	unsigned int i;
> -	int ret;
> -
> -	while (1) {
> -		pad = NULL;
> -		/* Find remote source pad */
> -		for (i = 0; i < entity->num_pads; i++) {
> -			struct media_pad *spad = &entity->pads[i];
> -
> -			if (!(spad->flags & MEDIA_PAD_FL_SINK))
> -				continue;
> -			pad = media_entity_remote_pad(spad);
> -			if (pad && is_media_entity_v4l2_subdev(pad->entity))
> -				break;
> -		}
> -		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> -			break;
> -
> -		entity = pad->entity;
> -		if (entity == until)
> -			break;
> -
> -		ret = cb(from, entity);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int rkisp1_pipeline_disable_cb(struct media_entity *from,
> -				      struct media_entity *curr)
> -{
> -	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
> -
> -	return v4l2_subdev_call(sd, video, s_stream, false);
> -}
> -
> -static int rkisp1_pipeline_enable_cb(struct media_entity *from,
> -				     struct media_entity *curr)
> -{
> -	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
> -
> -	return v4l2_subdev_call(sd, video, s_stream, true);
> -}
> -
>  static void rkisp1_stream_stop(struct rkisp1_capture *cap)
>  {
>  	int ret;
> @@ -929,11 +864,7 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue)
>  
>  	rkisp1_stream_stop(cap);
>  	media_pipeline_stop(&node->vdev.entity);
> -	ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL,
> -					rkisp1_pipeline_disable_cb);
> -	if (ret)
> -		dev_err(rkisp1->dev,
> -			"pipeline stream-off failed error:%d\n", ret);
> +	v4l2_pipeline_stream_disable(&node->vdev);
>  
>  	rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR);
>  
> @@ -1005,8 +936,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  	rkisp1_stream_start(cap);
>  
>  	/* start sub-devices */
> -	ret = rkisp1_pipeline_sink_walk(entity, NULL,
> -					rkisp1_pipeline_enable_cb);
> +	ret = v4l2_pipeline_stream_enable(&cap->vnode.vdev);
>  	if (ret)
>  		goto err_stop_stream;
>  
> @@ -1019,7 +949,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  	return 0;
>  
>  err_pipe_disable:
> -	rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb);
> +	v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);

This does not match the prototype for v4l2_pipeline_stream_disable() or 
am I missing something ?

>  err_stop_stream:
>  	rkisp1_stream_stop(cap);
>  	v4l2_pipeline_pm_put(entity);
> -- 
> 2.26.0
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 0/3] media: add v4l2_pipeline_stream_{enable,disable} helpers

[Niklas Söderlund]

Hi Helen,

On 2020-04-03 18:33:09 -0300, Helen Koike wrote:
> Hi,
> 
> Media drivers need to iterate through the pipeline and call .s_stream()
> callbacks in the subdevices.
> 
> Instead of repeating code, add helpers for this.
> 
> These helpers will go walk through the pipeline only visiting entities
> that participates in the stream, i.e. it follows links from sink to source
> (and not the opposite).
> 
> Which means that in a topology like this https://bit.ly/3b2MxjI
> calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
> .s_stream(true) for rkisp1_resizer_selfpath.
> 
> stream_count variable was added in v4l2_subdevice to handle nested calls
> to the helpers.
> This is useful when the driver allows streaming from more then one
> capture device sharing subdevices.
> 
> This patch came from the error I was facing when multistreaming from
> rkisp1 driver, where stoping one capture would call s_stream(false) in
> the pipeline, causing a stall in the second capture device.
> 
> Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
> be required with this patchset.
> 
> This patchset was tested on rkisp1 and vimc drivers.

I'm just curious, with this series applied can I stream simultaneously 
on multiple video devises using vimc?

> 
> Other cleanup might be possible (but I won't add in this patchset as I
> don't have the hw to test):
> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97
> 
> Changes in V2:
> ====================
> The first version was calling the s_stream() callbacks from sensor to
> capture.
> 
> This was generating errors in the Scarlet Chromebook, when the sensor
> was being enabled before the ISP.
> 
> It make sense to enable subdevices from capture to sensor instead (which
> is what most drivers do already).
> 
> This v2 drops the changes from mc-entity.c, and re-implement helpers in
> v4l2-common.c
> 
> Overview of patches:
> ====================
> 
> Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
> adding stream_count in the subdevice struct.
> 
> Patch 2/3 cleanup rkisp1 driver to use the helpers.
> 
> Patch 3/3 cleanup vimc driver to use the helpers.
> 
> Helen Koike (3):
>   media: v4l2-common: add helper functions to call s_stream() callbacks
>   media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
>     helpers
>   media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers
> 
>  drivers/media/platform/vimc/vimc-capture.c    |  28 +++--
>  drivers/media/platform/vimc/vimc-streamer.c   |  49 +-------
>  drivers/media/v4l2-core/v4l2-common.c         | 117 ++++++++++++++++++
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  76 +-----------
>  include/media/v4l2-common.h                   |  28 +++++
>  include/media/v4l2-subdev.h                   |   2 +
>  6 files changed, 173 insertions(+), 127 deletions(-)
> 
> -- 
> 2.26.0
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 1/3] media: v4l2-common: add helper functions to call s_stream() callbacks

[Helen Koike]

Hi Niklas,

Thanks for your review.

On 4/7/20 4:24 PM, Niklas Söderlund wrote:
> Hi Helen,
> 
> Thanks for your work.
> 
> On 2020-04-03 18:33:10 -0300, Helen Koike wrote:
>> Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate
>> through the subdevices in a given stream (i.e following links from sink
>> to source) and call .s_stream() callback.
>>
>> Add stream_count on the subdevice object for simultaneous streaming from
>> different video devices which shares subdevices.
>>
>> Prevent calling .s_stream(true) if it was already called previously by
>> another stream.
>>
>> Prevent calling .s_stream(false) from one stream when there are still
>> others active.
>>
>> If .s_stream(true) fails, call .s_stream(false) in the reverse order.
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> Changes in v2:
>> - re-write helpers to not use media walkers
>>
>>  drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++
>>  include/media/v4l2-common.h           |  28 ++++++
>>  include/media/v4l2-subdev.h           |   2 +
>>  3 files changed, 147 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index d0e5ebc736f9f..379d4bf4f8128 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>> +
>> +/*
>> + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline
>> + * @subdevs: the array to be filled
>> + * @size: the array size
> 
> Should this be documented as the maximum number of elements that can fit 
> in the subdevs array?

sure, I'll update it.

> 
>> + *
>> + * Walk from a video node, following link from sink to source and fill the
>> + * array with subdevices in the path.
>> + *
>> + * Note: this function follows the first enabled link in a sink pad found in a
>> + * given entity. Thus it won't work if there are entities with multiple enabled
>> + * links to its sink pads in the topology.
> 
> I wonder if it would be more useful to make this function return all 
> subdevs in the pipeline that have an enabled link going from sink to 
> source while walking from the video device?
> 
> When reading the commit messages I thought this could be useful for the 
> rcar-vin driver. By not finding all subdevices in the pipeline this 
> would not be possible as there on some platforms are a CSI-2 bus where 
> the CSI-2 transmitter have 4 sink pads and one source pads so the whole 
> pipeline would not be started.

right, I'm not familiar with rcar-vin, I didn't know we had drivers using more then
a sink link to subdevices that could be enabled at the same time.

v1 of this patchset was returning all the nodes with enabled links going from sink to 
source, but in reverse order.

I guess I can re-work v1, and add a flag to return in reverse order, I'll check.

> 
>> + *
>> + * Return the number of subdevices filled in the array.
>> + */
>> +static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev,
>> +					      struct v4l2_subdev **subdevs,
>> +					      unsigned int size)
>> +{
>> +	struct media_entity *entity = &vdev->entity;
>> +	unsigned int idx = 0;
>> +
>> +	while (1) {
>> +		struct media_pad *src_pad = NULL;
>> +		unsigned int i;
>> +
>> +		/* Find remote source pad */
>> +		for (i = 0; i < entity->num_pads; i++) {
>> +			struct media_pad *sink_pad = &entity->pads[i];
>> +
>> +			if (!(sink_pad->flags & MEDIA_PAD_FL_SINK))
>> +				continue;
>> +
>> +			src_pad = media_entity_remote_pad(sink_pad);
>> +			if (src_pad &&
>> +			    is_media_entity_v4l2_subdev(src_pad->entity))
>> +				break;
>> +		}
>> +		if (i == entity->num_pads)
>> +			break;
>> +
>> +		if (idx >= size) {
>> +			WARN_ON(1);
>> +			return 0;
> 
> Would it make sens to have this function return int and a negative error 
> code here? Is this now how other areas of V4L2 deal with when a provided 
> array is too small ? I'm thinking about if this function could be 
> exported for use by drivers in the future.

we can do that, sure.

> 
>> +		}
>> +
>> +		entity = src_pad->entity;
>> +		subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
>> +	}
>> +
>> +	return idx;
>> +}
>> +
>> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
>> +{
>> +	struct media_device *mdev = vdev->entity.graph_obj.mdev;
>> +	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
>> +	struct v4l2_subdev *sd;
>> +	unsigned int i, size;
>> +	int ret;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +
>> +	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
>> +
>> +	for (i = 0; i < size; i++) {
>> +		sd = subdevs[i];
>> +		if (sd->stream_count++)
>> +			continue;
>> +		dev_dbg(mdev->dev,
>> +			"enabling stream for '%s'\n", sd->entity.name);
> 
> Small nit, would it make sens to print the sd->stream_count also in this 
> debug statement (and the similar ones bellow) ?

Only if we move this print before the "continue" above, otherwise it will always print 1.

Thanks,
Helen

> 
>> +		ret = v4l2_subdev_call(sd, video, s_stream, true);
>> +		if (ret && ret != -ENOIOCTLCMD)
>> +			goto err_stream_disable;
>> +	}
>> +
>> +	mutex_unlock(&mdev->graph_mutex);
>> +	return 0;
>> +
>> +err_stream_disable:
>> +	do {
>> +		sd = subdevs[i];
>> +		if (--sd->stream_count)
>> +			continue;
>> +		dev_dbg(mdev->dev,
>> +			"disabling stream for '%s'\n", sd->entity.name);
>> +		v4l2_subdev_call(sd, video, s_stream, false);
>> +	} while (i--);
>> +
>> +	mutex_unlock(&mdev->graph_mutex);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
>> +
>> +void v4l2_pipeline_stream_disable(struct video_device *vdev)
>> +{
>> +	struct media_device *mdev = vdev->entity.graph_obj.mdev;
>> +	struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
>> +	unsigned int size;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +
>> +	size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
>> +
>> +	while (size--) {
>> +		struct v4l2_subdev *sd = subdevs[size];
>> +
>> +		if (--sd->stream_count)
>> +			continue;
>> +		dev_dbg(mdev->dev,
>> +			"disabling stream for '%s'\n", sd->entity.name);
>> +		v4l2_subdev_call(sd, video, s_stream, false);
>> +	}
>> +
>> +	mutex_unlock(&mdev->graph_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable);
>> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
>> index 150ee16ebd811..e833610b0f66d 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>>  			u32 width, u32 height);
>>  
>> +/**
>> + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream
>> + * @vdev: Starting video device
>> + *
>> + * Call .s_stream(true) callback in all the subdevices participating in the
>> + * stream, i.e. following links from sink to source.
>> + *
>> + * Calls to this function can be nested, in which case the same number of
>> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
>> + * The  pipeline pointer must be identical for all nested calls to
>> + * v4l2_pipeline_stream_enable().
>> + */
>> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev);
>> +
>> +/**
>> + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream
>> + * @vdev: Starting video device
>> + *
>> + * Call .s_stream(true) callback in all the subdevices participating in the
>> + * stream, i.e. following links from sink to source.
>> + *
>> + * Calls to this function can be nested, in which case the same number of
>> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming.
>> + * The  pipeline pointer must be identical for all nested calls to
>> + * v4l2_pipeline_stream_enable().
>> + */
>> +void v4l2_pipeline_stream_disable(struct video_device *vdev);
>> +
>>  static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
>>  {
>>  	/*
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index a4848de598521..20f913a9f70c5 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data {
>>   * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>>   *		     device using v4l2_device_register_sensor_subdev().
>>   * @pdata: common part of subdevice platform data
>> + * @stream_count: Stream count for the subdevice.
>>   *
>>   * Each instance of a subdev driver should create this struct, either
>>   * stand-alone or embedded in a larger struct.
>> @@ -869,6 +870,7 @@ struct v4l2_subdev {
>>  	struct v4l2_async_notifier *notifier;
>>  	struct v4l2_async_notifier *subdev_notifier;
>>  	struct v4l2_subdev_platform_data *pdata;
>> +	unsigned int stream_count;
>>  };
>>  
>>  
>> -- 
>> 2.26.0
>>
> 

Re: [PATCH v2 2/3] media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable} helpers

[Helen Koike]

On 4/7/20 4:34 PM, Niklas Söderlund wrote:
> Hi Helen,
> 
> Thanks for your work.
> 
> On 2020-04-03 18:33:11 -0300, Helen Koike wrote:
>> Use v4l2_pipeline_stream_{enable,disable} to call .s_stream() subdevice
>> callbacks through the pipeline.
>>
>> Tested by streaming on RockPi4 with imx219 and on Scarlet Chromebook.
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> Changes in v2:
>> - rebase on top of new helpers prototypes
>>
>>  drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +------------------
>>  1 file changed, 3 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 24fe6a7888aa4..0c2a357c4a12a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -838,71 +838,6 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap,
>>  	spin_unlock_irqrestore(&cap->buf.lock, flags);
>>  }
>>  
>> -/*
>> - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb
>> - * @from: entity at which to start pipeline walk
>> - * @until: entity at which to stop pipeline walk
>> - *
>> - * Walk the entities chain starting at the pipeline video node and stop
>> - * all subdevices in the chain.
>> - *
>> - * If the until argument isn't NULL, stop the pipeline walk when reaching the
>> - * until entity. This is used to disable a partially started pipeline due to a
>> - * subdev start error.
>> - */
>> -static int rkisp1_pipeline_sink_walk(struct media_entity *from,
>> -				     struct media_entity *until,
>> -				     int (*cb)(struct media_entity *from,
>> -					       struct media_entity *curr))
>> -{
>> -	struct media_entity *entity = from;
>> -	struct media_pad *pad;
>> -	unsigned int i;
>> -	int ret;
>> -
>> -	while (1) {
>> -		pad = NULL;
>> -		/* Find remote source pad */
>> -		for (i = 0; i < entity->num_pads; i++) {
>> -			struct media_pad *spad = &entity->pads[i];
>> -
>> -			if (!(spad->flags & MEDIA_PAD_FL_SINK))
>> -				continue;
>> -			pad = media_entity_remote_pad(spad);
>> -			if (pad && is_media_entity_v4l2_subdev(pad->entity))
>> -				break;
>> -		}
>> -		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>> -			break;
>> -
>> -		entity = pad->entity;
>> -		if (entity == until)
>> -			break;
>> -
>> -		ret = cb(from, entity);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static int rkisp1_pipeline_disable_cb(struct media_entity *from,
>> -				      struct media_entity *curr)
>> -{
>> -	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
>> -
>> -	return v4l2_subdev_call(sd, video, s_stream, false);
>> -}
>> -
>> -static int rkisp1_pipeline_enable_cb(struct media_entity *from,
>> -				     struct media_entity *curr)
>> -{
>> -	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr);
>> -
>> -	return v4l2_subdev_call(sd, video, s_stream, true);
>> -}
>> -
>>  static void rkisp1_stream_stop(struct rkisp1_capture *cap)
>>  {
>>  	int ret;
>> @@ -929,11 +864,7 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue)
>>  
>>  	rkisp1_stream_stop(cap);
>>  	media_pipeline_stop(&node->vdev.entity);
>> -	ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL,
>> -					rkisp1_pipeline_disable_cb);
>> -	if (ret)
>> -		dev_err(rkisp1->dev,
>> -			"pipeline stream-off failed error:%d\n", ret);
>> +	v4l2_pipeline_stream_disable(&node->vdev);
>>  
>>  	rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR);
>>  
>> @@ -1005,8 +936,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>>  	rkisp1_stream_start(cap);
>>  
>>  	/* start sub-devices */
>> -	ret = rkisp1_pipeline_sink_walk(entity, NULL,
>> -					rkisp1_pipeline_enable_cb);
>> +	ret = v4l2_pipeline_stream_enable(&cap->vnode.vdev);
>>  	if (ret)
>>  		goto err_stop_stream;
>>  
>> @@ -1019,7 +949,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>>  	return 0;
>>  
>>  err_pipe_disable:
>> -	rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb);
>> +	v4l2_pipeline_stream_disable(entity, &cap->rkisp1->pipe);
> 
> This does not match the prototype for v4l2_pipeline_stream_disable() or 
> am I missing something ?

You are right, I must have messed with my branches, because I'm sure I compiled and tested it.

Thanks for catching this.
Helen

> 
>>  err_stop_stream:
>>  	rkisp1_stream_stop(cap);
>>  	v4l2_pipeline_pm_put(entity);
>> -- 
>> 2.26.0
>>
> 

Re: [PATCH v2 0/3] media: add v4l2_pipeline_stream_{enable,disable} helpers

[Helen Koike]

On 4/7/20 4:36 PM, Niklas Söderlund wrote:
> Hi Helen,
> 
> On 2020-04-03 18:33:09 -0300, Helen Koike wrote:
>> Hi,
>>
>> Media drivers need to iterate through the pipeline and call .s_stream()
>> callbacks in the subdevices.
>>
>> Instead of repeating code, add helpers for this.
>>
>> These helpers will go walk through the pipeline only visiting entities
>> that participates in the stream, i.e. it follows links from sink to source
>> (and not the opposite).
>>
>> Which means that in a topology like this https://bit.ly/3b2MxjI
>> calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
>> .s_stream(true) for rkisp1_resizer_selfpath.
>>
>> stream_count variable was added in v4l2_subdevice to handle nested calls
>> to the helpers.
>> This is useful when the driver allows streaming from more then one
>> capture device sharing subdevices.
>>
>> This patch came from the error I was facing when multistreaming from
>> rkisp1 driver, where stoping one capture would call s_stream(false) in
>> the pipeline, causing a stall in the second capture device.
>>
>> Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
>> be required with this patchset.
>>
>> This patchset was tested on rkisp1 and vimc drivers.
> 
> I'm just curious, with this series applied can I stream simultaneously 
> on multiple video devises using vimc?

No, this patch only removes the requirement of patch 1/3 in the series
"vimc: Allow multiple capture devices to use the same sensor", since the counter
is being added in the core, so it won't be required to add it for each subdevice.
The other patches in that series are still required.


Regards,
Helen

> 
>>
>> Other cleanup might be possible (but I won't add in this patchset as I
>> don't have the hw to test):
>> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
>> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
>> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
>> 	https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97
>>
>> Changes in V2:
>> ====================
>> The first version was calling the s_stream() callbacks from sensor to
>> capture.
>>
>> This was generating errors in the Scarlet Chromebook, when the sensor
>> was being enabled before the ISP.
>>
>> It make sense to enable subdevices from capture to sensor instead (which
>> is what most drivers do already).
>>
>> This v2 drops the changes from mc-entity.c, and re-implement helpers in
>> v4l2-common.c
>>
>> Overview of patches:
>> ====================
>>
>> Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
>> adding stream_count in the subdevice struct.
>>
>> Patch 2/3 cleanup rkisp1 driver to use the helpers.
>>
>> Patch 3/3 cleanup vimc driver to use the helpers.
>>
>> Helen Koike (3):
>>   media: v4l2-common: add helper functions to call s_stream() callbacks
>>   media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
>>     helpers
>>   media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers
>>
>>  drivers/media/platform/vimc/vimc-capture.c    |  28 +++--
>>  drivers/media/platform/vimc/vimc-streamer.c   |  49 +-------
>>  drivers/media/v4l2-core/v4l2-common.c         | 117 ++++++++++++++++++
>>  drivers/staging/media/rkisp1/rkisp1-capture.c |  76 +-----------
>>  include/media/v4l2-common.h                   |  28 +++++
>>  include/media/v4l2-subdev.h                   |   2 +
>>  6 files changed, 173 insertions(+), 127 deletions(-)
>>
>> -- 
>> 2.26.0
>>
> 

Re: [PATCH v2 0/3] media: add v4l2_pipeline_stream_{enable,disable} helpers

[Niklas Söderlund]

Hi Helen,

Thanks for your reply.

On 2020-04-07 21:05:03 -0300, Helen Koike wrote:
> No, this patch only removes the requirement of patch 1/3 in the series
> "vimc: Allow multiple capture devices to use the same sensor", since the counter
> is being added in the core, so it won't be required to add it for each subdevice.
> The other patches in that series are still required.

OK, just checking. One step in the that direction at least :-)

-- 
Regards,
Niklas Söderlund