[PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

This is version 2 of the patch series posted by Niklas for allowing
multiple streams in VIMC.
The original series can be found here:
https://patchwork.kernel.org/cover/10948831/

This series adds support for two (or more) capture devices to be 
connected to the same sensors and run simultaneously. Each capture device 
can be started and stopped independent of each other.

Patch 1/3 and 2/3 deals with solving the issues that arises once two 
capture devices can be part of the same pipeline. While 3/3 allows for 
two capture devices to be part of the same pipeline and thus allows for 
simultaneously use.

Changes since v1:
	- All three patches rebased on latest media-tree.
	Patch 3:
	- Search for an entity with a non-NULL pipe instead of searching
	  for sensor. This terminates the search at output itself.

Kaaira Gupta (3):
  media: vimc: Add usage count to subdevices
  media: vimc: Serialize vimc_streamer_s_stream()
  media: vimc: Join pipeline if one already exists

 .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
 .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
 drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
 drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
 .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
 5 files changed, 73 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] media: vimc: Add usage count to subdevices

[Kaaira Gupta]

Prepare for multiple video streams from the same sensor by adding a use
counter to each subdevice. 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.

[Kaaira: rebased the patch on current HEAD of media-tree
(8f2a4a9d5ff5202d0b3e3a144ebb9b67aabadd29)]

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-debayer.c | 8 ++++++++
 drivers/media/test-drivers/vimc/vimc-scaler.c  | 8 ++++++++
 drivers/media/test-drivers/vimc/vimc-sensor.c  | 9 ++++++++-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index c3f6fef34f68..93fe19d8d2b4 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -29,6 +29,7 @@ struct vimc_deb_pix_map {
 struct vimc_deb_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
+	atomic_t use_count;
 	/* The active format */
 	struct v4l2_mbus_framefmt sink_fmt;
 	u32 src_code;
@@ -343,6 +344,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->use_count) != 1)
+			return 0;
+
 		if (vdeb->src_frame)
 			return 0;
 
@@ -368,6 +372,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 			return -ENOMEM;
 
 	} else {
+		if (atomic_dec_return(&vdeb->use_count) != 0)
+			return 0;
+
 		if (!vdeb->src_frame)
 			return 0;
 
@@ -595,6 +602,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 	vdeb->ved.process_frame = vimc_deb_process_frame;
 	vdeb->ved.dev = vimc->mdev.dev;
 	vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
+	atomic_set(&vdeb->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 121fa7d62a2e..9b8458dbe57c 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -25,6 +25,7 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
 struct vimc_sca_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
+	atomic_t use_count;
 	/* NOTE: the source fmt is the same as the sink
 	 * with the width and hight multiplied by mult
 	 */
@@ -340,6 +341,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->use_count) != 1)
+			return 0;
+
 		if (vsca->src_frame)
 			return 0;
 
@@ -363,6 +367,9 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 			return -ENOMEM;
 
 	} else {
+		if (atomic_dec_return(&vsca->use_count) != 0)
+			return 0;
+
 		if (!vsca->src_frame)
 			return 0;
 
@@ -506,6 +513,7 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 
 	vsca->ved.process_frame = vimc_sca_process_frame;
 	vsca->ved.dev = vimc->mdev.dev;
+	atomic_set(&vsca->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 ba5db5a150b4..dbe169604e71 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -24,6 +24,7 @@ struct vimc_sen_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
 	struct tpg_data tpg;
+	atomic_t use_count;
 	u8 *frame;
 	enum vimc_sen_osd_mode osd_value;
 	u64 start_stream_ts;
@@ -250,8 +251,10 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
-		vsen->start_stream_ts = ktime_get_ns();
+		if (atomic_inc_return(&vsen->use_count) != 1)
+			return 0;
 
+		vsen->start_stream_ts = ktime_get_ns();
 		/* Calculate the frame size */
 		vpix = vimc_pix_map_by_code(vsen->mbus_format.code);
 		frame_size = vsen->mbus_format.width * vpix->bpp *
@@ -270,6 +273,9 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 
 	} else {
 
+		if (atomic_dec_return(&vsen->use_count) != 0)
+			return 0;
+
 		vfree(vsen->frame);
 		vsen->frame = NULL;
 	}
@@ -430,6 +436,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 
 	vsen->ved.process_frame = vimc_sen_process_frame;
 	vsen->ved.dev = vimc->mdev.dev;
+	atomic_set(&vsen->use_count, 0);
 
 	/* Initialize the frame format */
 	vsen->mbus_format = fmt_default;
-- 
2.17.1

[PATCH v2 2/3] media: vimc: Serialize vimc_streamer_s_stream()

[Kaaira Gupta]

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: rebased the patch on current HEAD of media-tree
(8f2a4a9d5ff5202d0b3e3a144ebb9b67aabadd29)]

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   | 23 ++++++++++++-------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index 451a32c0d034..5cd2f86a146a 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -192,18 +192,23 @@ 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;
+			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");
@@ -211,14 +216,12 @@ 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->kthread = NULL;
-			return ret;
+			goto out;
 		}
 
 	} else {
 		if (!stream->kthread)
-			return 0;
+			goto out;
 
 		ret = kthread_stop(stream->kthread);
 		/*
@@ -227,13 +230,17 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 		 * a chance to run. Ignore errors to stop the stream in the
 		 * pipeline.
 		 */
-		if (ret)
+		if (ret) {
 			dev_dbg(ved->dev, "kthread_stop returned '%d'\n", ret);
+			goto out;
+		}
 
 		stream->kthread = NULL;
 
 		vimc_streamer_pipeline_terminate(stream);
 	}
+out:
+	mutex_unlock(&vimc_streamer_lock);
 
-	return 0;
+	return ret;
 }
-- 
2.17.1

[PATCH v2 3/3] media: vimc: Join pipeline if one already exists

[Kaaira Gupta]

An output which is running is already part of a pipeline and trying to
start a new pipeline is not possible. This prevents two capture devices
from streaming at the same time.

Instead of failing to start the second capture device allow it to join
the already running pipeline. This allows two (or more) capture devices
to independently be started and stopped.

[Kaaira: Changed the search for an existing connected sensor, to search
for a non-NULL pipe instead, this helps to terminate the search at
output itself instead of going till the sensor, changed variable names,
commit message and conditions accordingly]

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-capture.c    | 35 ++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index c63496b17b9a..423d5e5a508d 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -237,16 +237,49 @@ static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
 	spin_unlock(&vcap->qlock);
 }
 
+static struct media_entity *vimc_cap_get_output(struct vimc_cap_device *vcap)
+{
+	struct media_entity *entity = &vcap->vdev.entity;
+	struct media_device *mdev = entity->graph_obj.mdev;
+	struct media_graph graph;
+
+	mutex_lock(&mdev->graph_mutex);
+	if (media_graph_walk_init(&graph, mdev)) {
+		mutex_unlock(&mdev->graph_mutex);
+		return NULL;
+	}
+
+	media_graph_walk_start(&graph, entity);
+
+	while ((entity = media_graph_walk_next(&graph)))
+		if (entity->pipe)
+			break;
+
+	mutex_unlock(&mdev->graph_mutex);
+
+	media_graph_walk_cleanup(&graph);
+
+	return entity;
+}
+
 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 media_entity *oent;
 	int ret;
 
 	vcap->sequence = 0;
 
 	/* Start the media pipeline */
-	ret = media_pipeline_start(entity, &vcap->stream.pipe);
+	oent = vimc_cap_get_output(vcap);
+	if (oent)
+		pipe = oent->pipe;
+	else
+		pipe = &vcap->stream.pipe;
+
+	ret = media_pipeline_start(entity, pipe);
 	if (ret) {
 		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
 		return ret;
-- 
2.17.1

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Niklas Söderlund]

Hi Kaaira,

Thanks for your work.

On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> This is version 2 of the patch series posted by Niklas for allowing
> multiple streams in VIMC.
> The original series can be found here:
> https://patchwork.kernel.org/cover/10948831/
> 
> This series adds support for two (or more) capture devices to be 
> connected to the same sensors and run simultaneously. Each capture device 
> can be started and stopped independent of each other.
> 
> Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> capture devices can be part of the same pipeline. While 3/3 allows for 
> two capture devices to be part of the same pipeline and thus allows for 
> simultaneously use.

I'm just curious if you are aware of this series? It would replace the 
need for 1/3 and 2/3 of this series right?

1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/

> 
> Changes since v1:
> 	- All three patches rebased on latest media-tree.
> 	Patch 3:
> 	- Search for an entity with a non-NULL pipe instead of searching
> 	  for sensor. This terminates the search at output itself.
> 
> Kaaira Gupta (3):
>   media: vimc: Add usage count to subdevices
>   media: vimc: Serialize vimc_streamer_s_stream()
>   media: vimc: Join pipeline if one already exists
> 
>  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>  5 files changed, 73 insertions(+), 10 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
Hi,

> Hi Kaaira,
> 
> Thanks for your work.

Thanks for yours :D

> 
> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> > This is version 2 of the patch series posted by Niklas for allowing
> > multiple streams in VIMC.
> > The original series can be found here:
> > https://patchwork.kernel.org/cover/10948831/
> > 
> > This series adds support for two (or more) capture devices to be 
> > connected to the same sensors and run simultaneously. Each capture device 
> > can be started and stopped independent of each other.
> > 
> > Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> > capture devices can be part of the same pipeline. While 3/3 allows for 
> > two capture devices to be part of the same pipeline and thus allows for 
> > simultaneously use.
> 
> I'm just curious if you are aware of this series? It would replace the 
> need for 1/3 and 2/3 of this series right?

v3 of this series replaces the need for 1/3, but not the current version
(ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
keep count of the calls to s_stream. Hence 1/3 becomes relevant again.

> 
> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> 
> > 
> > Changes since v1:
> > 	- All three patches rebased on latest media-tree.
> > 	Patch 3:
> > 	- Search for an entity with a non-NULL pipe instead of searching
> > 	  for sensor. This terminates the search at output itself.
> > 
> > Kaaira Gupta (3):
> >   media: vimc: Add usage count to subdevices
> >   media: vimc: Serialize vimc_streamer_s_stream()
> >   media: vimc: Join pipeline if one already exists
> > 
> >  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
> >  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> >  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> >  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> >  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
> >  5 files changed, 73 insertions(+), 10 deletions(-)
> > 
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kieran Bingham]

Hi all,

+Dafna for the thread discussion, as she's missed from the to/cc list.


On 24/07/2020 13:21, Kaaira Gupta wrote:
> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> Hi,
> 
>> Hi Kaaira,
>>
>> Thanks for your work.
> 
> Thanks for yours :D
> 
>>
>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>> This is version 2 of the patch series posted by Niklas for allowing
>>> multiple streams in VIMC.
>>> The original series can be found here:
>>> https://patchwork.kernel.org/cover/10948831/
>>>
>>> This series adds support for two (or more) capture devices to be 
>>> connected to the same sensors and run simultaneously. Each capture device 
>>> can be started and stopped independent of each other.
>>>
>>> Patch 1/3 and 2/3 deals with solving the issues that arises once two 
>>> capture devices can be part of the same pipeline. While 3/3 allows for 
>>> two capture devices to be part of the same pipeline and thus allows for 
>>> simultaneously use.
>>
>> I'm just curious if you are aware of this series? It would replace the 
>> need for 1/3 and 2/3 of this series right?
> 
> v3 of this series replaces the need for 1/3, but not the current version
> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
> keep count of the calls to s_stream. Hence 1/3 becomes relevant again.

So the question really is, how do we best make use of the two current
series, to achieve our goal of supporting multiple streams.

Having not parsed Dafna's series yet, do we need to combine elements of
both ? Or should we work towards starting with this series and get
dafna's patches built on top ?

Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?

(It might be noteworthy to say that Kaaira has reported successful
multiple stream operation from /this/ series and her development branch
on libcamera).


>> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>
>>>
>>> Changes since v1:
>>> 	- All three patches rebased on latest media-tree.
>>> 	Patch 3:
>>> 	- Search for an entity with a non-NULL pipe instead of searching
>>> 	  for sensor. This terminates the search at output itself.
>>>
>>> Kaaira Gupta (3):
>>>   media: vimc: Add usage count to subdevices
>>>   media: vimc: Serialize vimc_streamer_s_stream()
>>>   media: vimc: Join pipeline if one already exists
>>>
>>>  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>>>  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>  5 files changed, 73 insertions(+), 10 deletions(-)
>>>
>>> -- 
>>> 2.17.1
>>>
>>
>> -- 
>> Regards,
>> Niklas Söderlund

-- 
Regards
--
Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Helen Koike]

Hi,

On 7/27/20 11:31 AM, Kieran Bingham wrote:
> Hi all,
> 
> +Dafna for the thread discussion, as she's missed from the to/cc list.
> 
> 
> On 24/07/2020 13:21, Kaaira Gupta wrote:
>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>> Hi,
>>
>>> Hi Kaaira,
>>>
>>> Thanks for your work.
>>
>> Thanks for yours :D
>>
>>>
>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>> This is version 2 of the patch series posted by Niklas for allowing
>>>> multiple streams in VIMC.
>>>> The original series can be found here:
>>>> https://patchwork.kernel.org/cover/10948831/
>>>>
>>>> This series adds support for two (or more) capture devices to be 
>>>> connected to the same sensors and run simultaneously. Each capture device 
>>>> can be started and stopped independent of each other.
>>>>
>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once two 
>>>> capture devices can be part of the same pipeline. While 3/3 allows for 
>>>> two capture devices to be part of the same pipeline and thus allows for 
>>>> simultaneously use.
>>>
>>> I'm just curious if you are aware of this series? It would replace the 
>>> need for 1/3 and 2/3 of this series right?
>>
>> v3 of this series replaces the need for 1/3, but not the current version
>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
>> keep count of the calls to s_stream. Hence 1/3 becomes relevant again.
> 
> So the question really is, how do we best make use of the two current
> series, to achieve our goal of supporting multiple streams.
> 
> Having not parsed Dafna's series yet, do we need to combine elements of
> both ? Or should we work towards starting with this series and get
> dafna's patches built on top ?
> 
> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> 
> (It might be noteworthy to say that Kaaira has reported successful
> multiple stream operation from /this/ series and her development branch
> on libcamera).

Dafna's patch seems still under discussion, but I don't want to block progress in Vimc either.

So I was wondering if we can move forward with Vimc support for multistreaming,
without considering Dafna's patchset, and we can do the clean up later once we solve that.

What do you think?

Regards,
Helen

> 
> 
>>> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>
>>>>
>>>> Changes since v1:
>>>> 	- All three patches rebased on latest media-tree.
>>>> 	Patch 3:
>>>> 	- Search for an entity with a non-NULL pipe instead of searching
>>>> 	  for sensor. This terminates the search at output itself.
>>>>
>>>> Kaaira Gupta (3):
>>>>   media: vimc: Add usage count to subdevices
>>>>   media: vimc: Serialize vimc_streamer_s_stream()
>>>>   media: vimc: Join pipeline if one already exists
>>>>
>>>>  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>>>>  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>>  5 files changed, 73 insertions(+), 10 deletions(-)
>>>>
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>> -- 
>>> Regards,
>>> Niklas Söderlund
> 

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
> Hi,
> 
> On 7/27/20 11:31 AM, Kieran Bingham wrote:
> > Hi all,
> > 
> > +Dafna for the thread discussion, as she's missed from the to/cc list.
> > 
> > 
> > On 24/07/2020 13:21, Kaaira Gupta wrote:
> >> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> >> Hi,
> >>
> >>> Hi Kaaira,
> >>>
> >>> Thanks for your work.
> >>
> >> Thanks for yours :D
> >>
> >>>
> >>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> >>>> This is version 2 of the patch series posted by Niklas for allowing
> >>>> multiple streams in VIMC.
> >>>> The original series can be found here:
> >>>> https://patchwork.kernel.org/cover/10948831/
> >>>>
> >>>> This series adds support for two (or more) capture devices to be 
> >>>> connected to the same sensors and run simultaneously. Each capture device 
> >>>> can be started and stopped independent of each other.
> >>>>
> >>>> Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> >>>> capture devices can be part of the same pipeline. While 3/3 allows for 
> >>>> two capture devices to be part of the same pipeline and thus allows for 
> >>>> simultaneously use.
> >>>
> >>> I'm just curious if you are aware of this series? It would replace the 
> >>> need for 1/3 and 2/3 of this series right?
> >>
> >> v3 of this series replaces the need for 1/3, but not the current version
> >> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
> >> keep count of the calls to s_stream. Hence 1/3 becomes relevant again.
> > 
> > So the question really is, how do we best make use of the two current
> > series, to achieve our goal of supporting multiple streams.
> > 
> > Having not parsed Dafna's series yet, do we need to combine elements of
> > both ? Or should we work towards starting with this series and get
> > dafna's patches built on top ?
> > 
> > Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> > 
> > (It might be noteworthy to say that Kaaira has reported successful
> > multiple stream operation from /this/ series and her development branch
> > on libcamera).
> 
> Dafna's patch seems still under discussion, but I don't want to block progress in Vimc either.
> 
> So I was wondering if we can move forward with Vimc support for multistreaming,
> without considering Dafna's patchset, and we can do the clean up later once we solve that.
> 
> What do you think?

I agree with supporting multiple streams with VIMC with this patchset,
and then we can refactor the counters for s_stream in VIMC later (over
this series) if dafna includes them in subsequent version of her patchset.

> 
> Regards,
> Helen
> 
> > 
> > 
> >>> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> >>>
> >>>>
> >>>> Changes since v1:
> >>>> 	- All three patches rebased on latest media-tree.
> >>>> 	Patch 3:
> >>>> 	- Search for an entity with a non-NULL pipe instead of searching
> >>>> 	  for sensor. This terminates the search at output itself.
> >>>>
> >>>> Kaaira Gupta (3):
> >>>>   media: vimc: Add usage count to subdevices
> >>>>   media: vimc: Serialize vimc_streamer_s_stream()
> >>>>   media: vimc: Join pipeline if one already exists
> >>>>
> >>>>  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
> >>>>  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> >>>>  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> >>>>  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> >>>>  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
> >>>>  5 files changed, 73 insertions(+), 10 deletions(-)
> >>>>
> >>>> -- 
> >>>> 2.17.1
> >>>>
> >>>
> >>> -- 
> >>> Regards,
> >>> Niklas Söderlund
> > 

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Dafna Hirschfeld]

Hi

On 28.07.20 13:39, Kaaira Gupta wrote:
> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>> Hi,
>>
>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>> Hi all,
>>>
>>> +Dafna for the thread discussion, as she's missed from the to/cc list.
>>>
>>>
>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>> Hi,
>>>>
>>>>> Hi Kaaira,
>>>>>
>>>>> Thanks for your work.
>>>>
>>>> Thanks for yours :D
>>>>
>>>>>
>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>> This is version 2 of the patch series posted by Niklas for allowing
>>>>>> multiple streams in VIMC.
>>>>>> The original series can be found here:
>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>
>>>>>> This series adds support for two (or more) capture devices to be
>>>>>> connected to the same sensors and run simultaneously. Each capture device
>>>>>> can be started and stopped independent of each other.
>>>>>>
>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once two
>>>>>> capture devices can be part of the same pipeline. While 3/3 allows for
>>>>>> two capture devices to be part of the same pipeline and thus allows for
>>>>>> simultaneously use.

I wonder if these two patches are enough, since each vimc entity also have
a 'process_frame' callback, but only one allocated frame. That means
that the 'process_frame' can be called concurrently by two different streams
on the same frame and cause corruption.

Thanks,
Dafna

>>>>>
>>>>> I'm just curious if you are aware of this series? It would replace the
>>>>> need for 1/3 and 2/3 of this series right?
>>>>
>>>> v3 of this series replaces the need for 1/3, but not the current version
>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant again.
>>>
>>> So the question really is, how do we best make use of the two current
>>> series, to achieve our goal of supporting multiple streams.
>>>
>>> Having not parsed Dafna's series yet, do we need to combine elements of
>>> both ? Or should we work towards starting with this series and get
>>> dafna's patches built on top ?
>>>
>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>
>>> (It might be noteworthy to say that Kaaira has reported successful
>>> multiple stream operation from /this/ series and her development branch
>>> on libcamera).
>>
>> Dafna's patch seems still under discussion, but I don't want to block progress in Vimc either.
>>
>> So I was wondering if we can move forward with Vimc support for multistreaming,
>> without considering Dafna's patchset, and we can do the clean up later once we solve that.
>>
>> What do you think?
> 
> I agree with supporting multiple streams with VIMC with this patchset,
> and then we can refactor the counters for s_stream in VIMC later (over
> this series) if dafna includes them in subsequent version of her patchset.
> 

I also think that adding support in the code will take much longer and should not
stop us from supporting vimc independently.

Thanks,
Dafna

>>
>> Regards,
>> Helen
>>
>>>
>>>
>>>>> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>
>>>>>>
>>>>>> Changes since v1:
>>>>>> 	- All three patches rebased on latest media-tree.
>>>>>> 	Patch 3:
>>>>>> 	- Search for an entity with a non-NULL pipe instead of searching
>>>>>> 	  for sensor. This terminates the search at output itself.
>>>>>>
>>>>>> Kaaira Gupta (3):
>>>>>>    media: vimc: Add usage count to subdevices
>>>>>>    media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>    media: vimc: Join pipeline if one already exists
>>>>>>
>>>>>>   .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>>>>>>   .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>   drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>   drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>   .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>>>>   5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>> -- 
>>>>> Regards,
>>>>> Niklas Söderlund
>>>

Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists

[Dafna Hirschfeld]

On 24.07.20 14:02, Kaaira Gupta wrote:
> An output which is running is already part of a pipeline and trying to
> start a new pipeline is not possible. This prevents two capture devices
> from streaming at the same time.
> 
> Instead of failing to start the second capture device allow it to join
> the already running pipeline. This allows two (or more) capture devices
> to independently be started and stopped.
> 
> [Kaaira: Changed the search for an existing connected sensor, to search
> for a non-NULL pipe instead, this helps to terminate the search at
> output itself instead of going till the sensor, changed variable names,
> commit message and conditions accordingly]
> 
> 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-capture.c    | 35 ++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index c63496b17b9a..423d5e5a508d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -237,16 +237,49 @@ static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
>   	spin_unlock(&vcap->qlock);
>   }
>   
> +static struct media_entity *vimc_cap_get_output(struct vimc_cap_device *vcap)
> +{
> +	struct media_entity *entity = &vcap->vdev.entity;
> +	struct media_device *mdev = entity->graph_obj.mdev;
> +	struct media_graph graph;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	if (media_graph_walk_init(&graph, mdev)) {
> +		mutex_unlock(&mdev->graph_mutex);
> +		return NULL;
> +	}
> +
> +	media_graph_walk_start(&graph, entity);
> +
> +	while ((entity = media_graph_walk_next(&graph)))
> +		if (entity->pipe)
> +			break;
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +	media_graph_walk_cleanup(&graph);
> +
> +	return entity;
> +}
> +
>   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 media_entity *oent;
>   	int ret;
>   
>   	vcap->sequence = 0;
>   
>   	/* Start the media pipeline */
> -	ret = media_pipeline_start(entity, &vcap->stream.pipe);
> +	oent = vimc_cap_get_output(vcap);
> +	if (oent)
> +		pipe = oent->pipe;
> +	else
> +		pipe = &vcap->stream.pipe;
> +
> +	ret = media_pipeline_start(entity, pipe);
>   	if (ret) {
>   		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>   		return ret;
>

I think there is actually no need to iterate the graph. If the capture is already connected to another capture
that streams, it means that they both have the same pipe in the media core.
So actually the code can just be:

if (vcap->ved.ent->pipe)
	pipe = vcap->ved.ent->pipe;
else
	pipe = &vcap->stream.pipe;


(and no need the function vimc_cap_get_output)

Thanks,
Dafna
  

Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists

[Kaaira Gupta]

On Tue, Jul 28, 2020 at 02:24:37PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 24.07.20 14:02, Kaaira Gupta wrote:
> > An output which is running is already part of a pipeline and trying to
> > start a new pipeline is not possible. This prevents two capture devices
> > from streaming at the same time.
> > 
> > Instead of failing to start the second capture device allow it to join
> > the already running pipeline. This allows two (or more) capture devices
> > to independently be started and stopped.
> > 
> > [Kaaira: Changed the search for an existing connected sensor, to search
> > for a non-NULL pipe instead, this helps to terminate the search at
> > output itself instead of going till the sensor, changed variable names,
> > commit message and conditions accordingly]
> > 
> > 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-capture.c    | 35 ++++++++++++++++++-
> >   1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index c63496b17b9a..423d5e5a508d 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -237,16 +237,49 @@ static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
> >   	spin_unlock(&vcap->qlock);
> >   }
> > +static struct media_entity *vimc_cap_get_output(struct vimc_cap_device *vcap)
> > +{
> > +	struct media_entity *entity = &vcap->vdev.entity;
> > +	struct media_device *mdev = entity->graph_obj.mdev;
> > +	struct media_graph graph;
> > +
> > +	mutex_lock(&mdev->graph_mutex);
> > +	if (media_graph_walk_init(&graph, mdev)) {
> > +		mutex_unlock(&mdev->graph_mutex);
> > +		return NULL;
> > +	}
> > +
> > +	media_graph_walk_start(&graph, entity);
> > +
> > +	while ((entity = media_graph_walk_next(&graph)))
> > +		if (entity->pipe)
> > +			break;
> > +
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> > +	media_graph_walk_cleanup(&graph);
> > +
> > +	return entity;
> > +}
> > +
> >   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 media_entity *oent;
> >   	int ret;
> >   	vcap->sequence = 0;
> >   	/* Start the media pipeline */
> > -	ret = media_pipeline_start(entity, &vcap->stream.pipe);
> > +	oent = vimc_cap_get_output(vcap);
> > +	if (oent)
> > +		pipe = oent->pipe;
> > +	else
> > +		pipe = &vcap->stream.pipe;
> > +
> > +	ret = media_pipeline_start(entity, pipe);
> >   	if (ret) {
> >   		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> >   		return ret;
> > 
> 
> I think there is actually no need to iterate the graph. If the capture is already connected to another capture
> that streams, it means that they both have the same pipe in the media core.
> So actually the code can just be:

Hi,
iterating the graph takes care of the case when output already exists.
So in case where an output is needed from both Capture1 and RGB capture,
don't they represent two different pipes?

> 
> if (vcap->ved.ent->pipe)
> 	pipe = vcap->ved.ent->pipe;
> else
> 	pipe = &vcap->stream.pipe;
> 
> 
> (and no need the function vimc_cap_get_output)
> 
> Thanks,
> Dafna

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Dafna Hirschfeld]

On 28.07.20 14:07, Dafna Hirschfeld wrote:
> Hi
> 
> On 28.07.20 13:39, Kaaira Gupta wrote:
>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>> Hi,
>>>
>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>> Hi all,
>>>>
>>>> +Dafna for the thread discussion, as she's missed from the to/cc list.
>>>>
>>>>
>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>> Hi,
>>>>>
>>>>>> Hi Kaaira,
>>>>>>
>>>>>> Thanks for your work.
>>>>>
>>>>> Thanks for yours :D
>>>>>
>>>>>>
>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>> This is version 2 of the patch series posted by Niklas for allowing
>>>>>>> multiple streams in VIMC.
>>>>>>> The original series can be found here:
>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>
>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>> connected to the same sensors and run simultaneously. Each capture device
>>>>>>> can be started and stopped independent of each other.
>>>>>>>
>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once two
>>>>>>> capture devices can be part of the same pipeline. While 3/3 allows for
>>>>>>> two capture devices to be part of the same pipeline and thus allows for
>>>>>>> simultaneously use.
> 
> I wonder if these two patches are enough, since each vimc entity also have
> a 'process_frame' callback, but only one allocated frame. That means
> that the 'process_frame' can be called concurrently by two different streams
> on the same frame and cause corruption.
> 

I think we should somehow change the vimc-stream.c code so that we have only
one stream process per pipe. So if one capture is already streaming, then the new
capture that wants to stream uses the same thread so we don't have two threads
both calling 'process_frame'.

The second capture that wants to stream should iterate the topology downwards until
reaching an entity that already belong to the stream path of the other streaming capture
and tell the streamer it wants to read the frames this entity
produces.

Thanks,
Dafna

> Thanks,
> Dafna
> 
>>>>>>
>>>>>> I'm just curious if you are aware of this series? It would replace the
>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>
>>>>> v3 of this series replaces the need for 1/3, but not the current version
>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant again.
>>>>
>>>> So the question really is, how do we best make use of the two current
>>>> series, to achieve our goal of supporting multiple streams.
>>>>
>>>> Having not parsed Dafna's series yet, do we need to combine elements of
>>>> both ? Or should we work towards starting with this series and get
>>>> dafna's patches built on top ?
>>>>
>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>
>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>> multiple stream operation from /this/ series and her development branch
>>>> on libcamera).
>>>
>>> Dafna's patch seems still under discussion, but I don't want to block progress in Vimc either.
>>>
>>> So I was wondering if we can move forward with Vimc support for multistreaming,
>>> without considering Dafna's patchset, and we can do the clean up later once we solve that.
>>>
>>> What do you think?
>>
>> I agree with supporting multiple streams with VIMC with this patchset,
>> and then we can refactor the counters for s_stream in VIMC later (over
>> this series) if dafna includes them in subsequent version of her patchset.
>>
> 
> I also think that adding support in the code will take much longer and should not
> stop us from supporting vimc independently.
> 
> Thanks,
> Dafna
> 
>>>
>>> Regards,
>>> Helen
>>>
>>>>
>>>>
>>>>>> 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>>     - All three patches rebased on latest media-tree.
>>>>>>>     Patch 3:
>>>>>>>     - Search for an entity with a non-NULL pipe instead of searching
>>>>>>>       for sensor. This terminates the search at output itself.
>>>>>>>
>>>>>>> Kaaira Gupta (3):
>>>>>>>    media: vimc: Add usage count to subdevices
>>>>>>>    media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>    media: vimc: Join pipeline if one already exists
>>>>>>>
>>>>>>>   .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>>>>>>>   .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>   drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>   drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>   .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>>>>>   5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> -- 
>>>>>>> 2.17.1
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Regards,
>>>>>> Niklas Söderlund
>>>>

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

On Tue, Jul 28, 2020 at 04:00:46PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 28.07.20 14:07, Dafna Hirschfeld wrote:
> > Hi
> > 
> > On 28.07.20 13:39, Kaaira Gupta wrote:
> > > On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
> > > > Hi,
> > > > 
> > > > On 7/27/20 11:31 AM, Kieran Bingham wrote:
> > > > > Hi all,
> > > > > 
> > > > > +Dafna for the thread discussion, as she's missed from the to/cc list.
> > > > > 
> > > > > 
> > > > > On 24/07/2020 13:21, Kaaira Gupta wrote:
> > > > > > On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > > Hi Kaaira,
> > > > > > > 
> > > > > > > Thanks for your work.
> > > > > > 
> > > > > > Thanks for yours :D
> > > > > > 
> > > > > > > 
> > > > > > > On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> > > > > > > > This is version 2 of the patch series posted by Niklas for allowing
> > > > > > > > multiple streams in VIMC.
> > > > > > > > The original series can be found here:
> > > > > > > > https://patchwork.kernel.org/cover/10948831/
> > > > > > > > 
> > > > > > > > This series adds support for two (or more) capture devices to be
> > > > > > > > connected to the same sensors and run simultaneously. Each capture device
> > > > > > > > can be started and stopped independent of each other.
> > > > > > > > 
> > > > > > > > Patch 1/3 and 2/3 deals with solving the issues that arises once two
> > > > > > > > capture devices can be part of the same pipeline. While 3/3 allows for
> > > > > > > > two capture devices to be part of the same pipeline and thus allows for
> > > > > > > > simultaneously use.
> > 
> > I wonder if these two patches are enough, since each vimc entity also have
> > a 'process_frame' callback, but only one allocated frame. That means
> > that the 'process_frame' can be called concurrently by two different streams
> > on the same frame and cause corruption.
> > 
> 
> I think we should somehow change the vimc-stream.c code so that we have only
> one stream process per pipe. So if one capture is already streaming, then the new
> capture that wants to stream uses the same thread so we don't have two threads
> both calling 'process_frame'.

I didn't understand this well, can you please elaborate? How will it
lead to the new capture using same thread?

> 
> The second capture that wants to stream should iterate the topology downwards until
> reaching an entity that already belong to the stream path of the other streaming capture
> and tell the streamer it wants to read the frames this entity
> produces.

The first version of this series was doing this itself I think. But it
was for connecting the pipe(capture) at the sensor if one already
exists. 

> 
> Thanks,
> Dafna
> 
> > Thanks,
> > Dafna
> > 
> > > > > > > 
> > > > > > > I'm just curious if you are aware of this series? It would replace the
> > > > > > > need for 1/3 and 2/3 of this series right?
> > > > > > 
> > > > > > v3 of this series replaces the need for 1/3, but not the current version
> > > > > > (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
> > > > > > keep count of the calls to s_stream. Hence 1/3 becomes relevant again.
> > > > > 
> > > > > So the question really is, how do we best make use of the two current
> > > > > series, to achieve our goal of supporting multiple streams.
> > > > > 
> > > > > Having not parsed Dafna's series yet, do we need to combine elements of
> > > > > both ? Or should we work towards starting with this series and get
> > > > > dafna's patches built on top ?
> > > > > 
> > > > > Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> > > > > 
> > > > > (It might be noteworthy to say that Kaaira has reported successful
> > > > > multiple stream operation from /this/ series and her development branch
> > > > > on libcamera).
> > > > 
> > > > Dafna's patch seems still under discussion, but I don't want to block progress in Vimc either.
> > > > 
> > > > So I was wondering if we can move forward with Vimc support for multistreaming,
> > > > without considering Dafna's patchset, and we can do the clean up later once we solve that.
> > > > 
> > > > What do you think?
> > > 
> > > I agree with supporting multiple streams with VIMC with this patchset,
> > > and then we can refactor the counters for s_stream in VIMC later (over
> > > this series) if dafna includes them in subsequent version of her patchset.
> > > 
> > 
> > I also think that adding support in the code will take much longer and should not
> > stop us from supporting vimc independently.
> > 
> > Thanks,
> > Dafna
> > 
> > > > 
> > > > Regards,
> > > > Helen
> > > > 
> > > > > 
> > > > > 
> > > > > > > 1.  https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> > > > > > > 
> > > > > > > > 
> > > > > > > > Changes since v1:
> > > > > > > >     - All three patches rebased on latest media-tree.
> > > > > > > >     Patch 3:
> > > > > > > >     - Search for an entity with a non-NULL pipe instead of searching
> > > > > > > >       for sensor. This terminates the search at output itself.
> > > > > > > > 
> > > > > > > > Kaaira Gupta (3):
> > > > > > > >    media: vimc: Add usage count to subdevices
> > > > > > > >    media: vimc: Serialize vimc_streamer_s_stream()
> > > > > > > >    media: vimc: Join pipeline if one already exists
> > > > > > > > 
> > > > > > > >   .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
> > > > > > > >   .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> > > > > > > >   drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> > > > > > > >   drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> > > > > > > >   .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
> > > > > > > >   5 files changed, 73 insertions(+), 10 deletions(-)
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > 2.17.1
> > > > > > > > 
> > > > > > > 
> > > > > > > -- 
> > > > > > > Regards,
> > > > > > > Niklas Söderlund
> > > > > 

Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists

[Dafna Hirschfeld]

On 28.07.20 14:48, Kaaira Gupta wrote:
> On Tue, Jul 28, 2020 at 02:24:37PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 24.07.20 14:02, Kaaira Gupta wrote:
>>> An output which is running is already part of a pipeline and trying to
>>> start a new pipeline is not possible. This prevents two capture devices
>>> from streaming at the same time.
>>>
>>> Instead of failing to start the second capture device allow it to join
>>> the already running pipeline. This allows two (or more) capture devices
>>> to independently be started and stopped.
>>>
>>> [Kaaira: Changed the search for an existing connected sensor, to search
>>> for a non-NULL pipe instead, this helps to terminate the search at
>>> output itself instead of going till the sensor, changed variable names,
>>> commit message and conditions accordingly]
>>>
>>> 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-capture.c    | 35 ++++++++++++++++++-
>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
>>> index c63496b17b9a..423d5e5a508d 100644
>>> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
>>> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
>>> @@ -237,16 +237,49 @@ static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
>>>    	spin_unlock(&vcap->qlock);
>>>    }
>>> +static struct media_entity *vimc_cap_get_output(struct vimc_cap_device *vcap)
>>> +{
>>> +	struct media_entity *entity = &vcap->vdev.entity;
>>> +	struct media_device *mdev = entity->graph_obj.mdev;
>>> +	struct media_graph graph;
>>> +
>>> +	mutex_lock(&mdev->graph_mutex);
>>> +	if (media_graph_walk_init(&graph, mdev)) {
>>> +		mutex_unlock(&mdev->graph_mutex);
>>> +		return NULL;
>>> +	}
>>> +
>>> +	media_graph_walk_start(&graph, entity);
>>> +
>>> +	while ((entity = media_graph_walk_next(&graph)))
>>> +		if (entity->pipe)
>>> +			break;
>>> +
>>> +	mutex_unlock(&mdev->graph_mutex);
>>> +
>>> +	media_graph_walk_cleanup(&graph);
>>> +
>>> +	return entity;
>>> +}
>>> +
>>>    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 media_entity *oent;
>>>    	int ret;
>>>    	vcap->sequence = 0;
>>>    	/* Start the media pipeline */
>>> -	ret = media_pipeline_start(entity, &vcap->stream.pipe);
>>> +	oent = vimc_cap_get_output(vcap);
>>> +	if (oent)
>>> +		pipe = oent->pipe;
>>> +	else
>>> +		pipe = &vcap->stream.pipe;
>>> +
>>> +	ret = media_pipeline_start(entity, pipe);
>>>    	if (ret) {
>>>    		vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>>>    		return ret;
>>>
>>
>> I think there is actually no need to iterate the graph. If the capture is already connected to another capture
>> that streams, it means that they both have the same pipe in the media core.
>> So actually the code can just be:
> 
> Hi,
> iterating the graph takes care of the case when output already exists.
> So in case where an output is needed from both Capture1 and RGB capture,
> don't they represent two different pipes?

Actually no, the function __media_pipeline_start gives the same pipe to all entities that are reachable
with an enabled link from the entity that called 'media_pipeline_start' (the direction of the link does not matter in that case).
So that means that all entities that are connected have the same pipe.

The term 'stream' is a path from the capture down to the sensor, so we can have two streams sharing the same pipe.

Thanks,
Dafna

> 
>>
>> if (vcap->ved.ent->pipe)
>> 	pipe = vcap->ved.ent->pipe;
>> else
>> 	pipe = &vcap->stream.pipe;
>>
>>
>> (and no need the function vimc_cap_get_output)
>>
>> Thanks,
>> Dafna

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kieran Bingham]

Hi Dafna,

On 28/07/2020 15:00, Dafna Hirschfeld wrote:
> 
> 
> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>> Hi
>>
>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>> Hi,
>>>>
>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>> Hi all,
>>>>>
>>>>> +Dafna for the thread discussion, as she's missed from the to/cc list.
>>>>>
>>>>>
>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> Hi Kaaira,
>>>>>>>
>>>>>>> Thanks for your work.
>>>>>>
>>>>>> Thanks for yours :D
>>>>>>
>>>>>>>
>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>> This is version 2 of the patch series posted by Niklas for allowing
>>>>>>>> multiple streams in VIMC.
>>>>>>>> The original series can be found here:
>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>
>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>> capture device
>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>
>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>> two
>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>> allows for
>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>> allows for
>>>>>>>> simultaneously use.
>>
>> I wonder if these two patches are enough, since each vimc entity also
>> have
>> a 'process_frame' callback, but only one allocated frame. That means
>> that the 'process_frame' can be called concurrently by two different
>> streams
>> on the same frame and cause corruption.
>>
> 
> I think we should somehow change the vimc-stream.c code so that we have
> only
> one stream process per pipe. So if one capture is already streaming,
> then the new
> capture that wants to stream uses the same thread so we don't have two
> threads
> both calling 'process_frame'.


Yes, I think it looks and sounds like there are two threads running when
there are two streams.

so in effect, although they 'share a pipe', aren't they in effect just
sending two separate buffers through their stream-path?

If that's the case, then I don't think there's any frame corruption,
because they would both have grabbed their own frame separately.


I don't think that's a good example of the hardware though, as that
doesn't reflect what 'should' happen where the TPG runs once to generate
a frame at the sensor, which is then read by both the debayer entity and
the RAW capture device when there are two streams...


So I suspect trying to move to a single thread is desirable, but that
might be a fair bit of work also.

--
Kieran



> The second capture that wants to stream should iterate the topology
> downwards until
> reaching an entity that already belong to the stream path of the other
> streaming capture
> and tell the streamer it wants to read the frames this entity
> produces.
> 
> Thanks,
> Dafna
> 
>> Thanks,
>> Dafna
>>
>>>>>>>
>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>> replace the
>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>
>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>> version
>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>> again.
>>>>>
>>>>> So the question really is, how do we best make use of the two current
>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>
>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>> elements of
>>>>> both ? Or should we work towards starting with this series and get
>>>>> dafna's patches built on top ?
>>>>>
>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>
>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>> multiple stream operation from /this/ series and her development
>>>>> branch
>>>>> on libcamera).
>>>>
>>>> Dafna's patch seems still under discussion, but I don't want to
>>>> block progress in Vimc either.
>>>>
>>>> So I was wondering if we can move forward with Vimc support for
>>>> multistreaming,
>>>> without considering Dafna's patchset, and we can do the clean up
>>>> later once we solve that.
>>>>
>>>> What do you think?
>>>
>>> I agree with supporting multiple streams with VIMC with this patchset,
>>> and then we can refactor the counters for s_stream in VIMC later (over
>>> this series) if dafna includes them in subsequent version of her
>>> patchset.
>>>
>>
>> I also think that adding support in the code will take much longer and
>> should not
>> stop us from supporting vimc independently.
>>
>> Thanks,
>> Dafna
>>
>>>>
>>>> Regards,
>>>> Helen
>>>>
>>>>>
>>>>>
>>>>>>> 1. 
>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Changes since v1:
>>>>>>>>     - All three patches rebased on latest media-tree.
>>>>>>>>     Patch 3:
>>>>>>>>     - Search for an entity with a non-NULL pipe instead of
>>>>>>>> searching
>>>>>>>>       for sensor. This terminates the search at output itself.
>>>>>>>>
>>>>>>>> Kaaira Gupta (3):
>>>>>>>>    media: vimc: Add usage count to subdevices
>>>>>>>>    media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>    media: vimc: Join pipeline if one already exists
>>>>>>>>
>>>>>>>>   .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>> ++++++++++++++++++-
>>>>>>>>   .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>   drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>   drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>   .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>>>>>>   5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> Regards,
>>>>>>> Niklas Söderlund
>>>>>

-- 
Regards
--
Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Dafna Hirschfeld]

On 29.07.20 15:05, Kieran Bingham wrote:
> Hi Dafna,
> 
> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
>>
>>
>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>>> Hi
>>>
>>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>>> Hi,
>>>>>
>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc list.
>>>>>>
>>>>>>
>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>> Hi Kaaira,
>>>>>>>>
>>>>>>>> Thanks for your work.
>>>>>>>
>>>>>>> Thanks for yours :D
>>>>>>>
>>>>>>>>
>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>>> This is version 2 of the patch series posted by Niklas for allowing
>>>>>>>>> multiple streams in VIMC.
>>>>>>>>> The original series can be found here:
>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>>
>>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>>> capture device
>>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>>
>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>>> two
>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>>> allows for
>>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>>> allows for
>>>>>>>>> simultaneously use.
>>>
>>> I wonder if these two patches are enough, since each vimc entity also
>>> have
>>> a 'process_frame' callback, but only one allocated frame. That means
>>> that the 'process_frame' can be called concurrently by two different
>>> streams
>>> on the same frame and cause corruption.
>>>
>>
>> I think we should somehow change the vimc-stream.c code so that we have
>> only
>> one stream process per pipe. So if one capture is already streaming,
>> then the new
>> capture that wants to stream uses the same thread so we don't have two
>> threads
>> both calling 'process_frame'.
> 
> 
> Yes, I think it looks and sounds like there are two threads running when
> there are two streams.
> 
> so in effect, although they 'share a pipe', aren't they in effect just
> sending two separate buffers through their stream-path?
> 
> If that's the case, then I don't think there's any frame corruption,
> because they would both have grabbed their own frame separately.

But each entity allocates just one buffer. So the same buffer is used for
both stream.
What for example can happen is that the debayer of one stream can read the
sensor's buffer while the sensor itself writes to the buffer for the other
stream.

Thanks,
Dafna

> 
> 
> I don't think that's a good example of the hardware though, as that
> doesn't reflect what 'should' happen where the TPG runs once to generate
> a frame at the sensor, which is then read by both the debayer entity and
> the RAW capture device when there are two streams...
> 
> 
> So I suspect trying to move to a single thread is desirable, but that
> might be a fair bit of work also.
> 
> --
> Kieran
> 
> 
> 
>> The second capture that wants to stream should iterate the topology
>> downwards until
>> reaching an entity that already belong to the stream path of the other
>> streaming capture
>> and tell the streamer it wants to read the frames this entity
>> produces.
>>
>> Thanks,
>> Dafna
>>
>>> Thanks,
>>> Dafna
>>>
>>>>>>>>
>>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>>> replace the
>>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>>
>>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>>> version
>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to
>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>>> again.
>>>>>>
>>>>>> So the question really is, how do we best make use of the two current
>>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>>
>>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>>> elements of
>>>>>> both ? Or should we work towards starting with this series and get
>>>>>> dafna's patches built on top ?
>>>>>>
>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>>
>>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>>> multiple stream operation from /this/ series and her development
>>>>>> branch
>>>>>> on libcamera).
>>>>>
>>>>> Dafna's patch seems still under discussion, but I don't want to
>>>>> block progress in Vimc either.
>>>>>
>>>>> So I was wondering if we can move forward with Vimc support for
>>>>> multistreaming,
>>>>> without considering Dafna's patchset, and we can do the clean up
>>>>> later once we solve that.
>>>>>
>>>>> What do you think?
>>>>
>>>> I agree with supporting multiple streams with VIMC with this patchset,
>>>> and then we can refactor the counters for s_stream in VIMC later (over
>>>> this series) if dafna includes them in subsequent version of her
>>>> patchset.
>>>>
>>>
>>> I also think that adding support in the code will take much longer and
>>> should not
>>> stop us from supporting vimc independently.
>>>
>>> Thanks,
>>> Dafna
>>>
>>>>>
>>>>> Regards,
>>>>> Helen
>>>>>
>>>>>>
>>>>>>
>>>>>>>> 1.
>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Changes since v1:
>>>>>>>>>      - All three patches rebased on latest media-tree.
>>>>>>>>>      Patch 3:
>>>>>>>>>      - Search for an entity with a non-NULL pipe instead of
>>>>>>>>> searching
>>>>>>>>>        for sensor. This terminates the search at output itself.
>>>>>>>>>
>>>>>>>>> Kaaira Gupta (3):
>>>>>>>>>     media: vimc: Add usage count to subdevices
>>>>>>>>>     media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>>     media: vimc: Join pipeline if one already exists
>>>>>>>>>
>>>>>>>>>    .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>    .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>>    drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>>    drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>>    .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>>>>>>>>>    5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> 2.17.1
>>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> Regards,
>>>>>>>> Niklas Söderlund
>>>>>>
> 

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kieran Bingham]

Hi Dafna, Kaaira,

On 29/07/2020 14:16, Dafna Hirschfeld wrote:
> 
> 
> On 29.07.20 15:05, Kieran Bingham wrote:
>> Hi Dafna,
>>
>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
>>>
>>>
>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>>>> Hi
>>>>
>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
>>>>>>> list.
>>>>>>>
>>>>>>>
>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> Hi Kaaira,
>>>>>>>>>
>>>>>>>>> Thanks for your work.
>>>>>>>>
>>>>>>>> Thanks for yours :D
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
>>>>>>>>>> allowing
>>>>>>>>>> multiple streams in VIMC.
>>>>>>>>>> The original series can be found here:
>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>>>
>>>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>>>> capture device
>>>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>>>
>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>>>> two
>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>>>> allows for
>>>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>>>> allows for
>>>>>>>>>> simultaneously use.
>>>>
>>>> I wonder if these two patches are enough, since each vimc entity also
>>>> have
>>>> a 'process_frame' callback, but only one allocated frame. That means
>>>> that the 'process_frame' can be called concurrently by two different
>>>> streams
>>>> on the same frame and cause corruption.
>>>>
>>>
>>> I think we should somehow change the vimc-stream.c code so that we have
>>> only
>>> one stream process per pipe. So if one capture is already streaming,
>>> then the new
>>> capture that wants to stream uses the same thread so we don't have two
>>> threads
>>> both calling 'process_frame'.
>>
>>
>> Yes, I think it looks and sounds like there are two threads running when
>> there are two streams.
>>
>> so in effect, although they 'share a pipe', aren't they in effect just
>> sending two separate buffers through their stream-path?
>>
>> If that's the case, then I don't think there's any frame corruption,
>> because they would both have grabbed their own frame separately.
> 
> But each entity allocates just one buffer. So the same buffer is used for
> both stream.

Aha, ok, I hadn't realised there was only a single buffer available in
the pipeline for each entity. Indeed there is a risk of corruption in
that case.

> What for example can happen is that the debayer of one stream can read the
> sensor's buffer while the sensor itself writes to the buffer for the other
> stream.


So, In that case, we have currently got a scenario where each 'stream'
really is operating it's own pipe (even though all components are reused).

Two questions:

Is this acceptable, and we should just use a mutex to ensure the buffers
are not corrupted, but essentially each stream is a separate temporal
capture?


Or B:

Should we refactor to make sure that there is a single thread, and the
code which calls process_frame on each entity should become aware of the
potential for multiple paths at the point of the sensor.


I suspect option B is really the 'right' path to take, but it is more
complicated of course.

--
Kieran




> Thanks,
> Dafna
> 
>>
>>
>> I don't think that's a good example of the hardware though, as that
>> doesn't reflect what 'should' happen where the TPG runs once to generate
>> a frame at the sensor, which is then read by both the debayer entity and
>> the RAW capture device when there are two streams...
>>
>>
>> So I suspect trying to move to a single thread is desirable, but that
>> might be a fair bit of work also.
>>
>> -- 
>> Kieran
>>
>>
>>
>>> The second capture that wants to stream should iterate the topology
>>> downwards until
>>> reaching an entity that already belong to the stream path of the other
>>> streaming capture
>>> and tell the streamer it wants to read the frames this entity
>>> produces.
>>>
>>> Thanks,
>>> Dafna
>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>>>>>>>
>>>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>>>> replace the
>>>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>>>
>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>>>> version
>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
>>>>>>>> needed to
>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>>>> again.
>>>>>>>
>>>>>>> So the question really is, how do we best make use of the two
>>>>>>> current
>>>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>>>
>>>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>>>> elements of
>>>>>>> both ? Or should we work towards starting with this series and get
>>>>>>> dafna's patches built on top ?
>>>>>>>
>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>>>
>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>>>> multiple stream operation from /this/ series and her development
>>>>>>> branch
>>>>>>> on libcamera).
>>>>>>
>>>>>> Dafna's patch seems still under discussion, but I don't want to
>>>>>> block progress in Vimc either.
>>>>>>
>>>>>> So I was wondering if we can move forward with Vimc support for
>>>>>> multistreaming,
>>>>>> without considering Dafna's patchset, and we can do the clean up
>>>>>> later once we solve that.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> I agree with supporting multiple streams with VIMC with this patchset,
>>>>> and then we can refactor the counters for s_stream in VIMC later (over
>>>>> this series) if dafna includes them in subsequent version of her
>>>>> patchset.
>>>>>
>>>>
>>>> I also think that adding support in the code will take much longer and
>>>> should not
>>>> stop us from supporting vimc independently.
>>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Helen
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> 1.
>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Changes since v1:
>>>>>>>>>>      - All three patches rebased on latest media-tree.
>>>>>>>>>>      Patch 3:
>>>>>>>>>>      - Search for an entity with a non-NULL pipe instead of
>>>>>>>>>> searching
>>>>>>>>>>        for sensor. This terminates the search at output itself.
>>>>>>>>>>
>>>>>>>>>> Kaaira Gupta (3):
>>>>>>>>>>     media: vimc: Add usage count to subdevices
>>>>>>>>>>     media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>>>     media: vimc: Join pipeline if one already exists
>>>>>>>>>>
>>>>>>>>>>    .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>    .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>>>    drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>>>    drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>>>    .../media/test-drivers/vimc/vimc-streamer.c   | 23
>>>>>>>>>> +++++++-----
>>>>>>>>>>    5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> 2.17.1
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> Regards,
>>>>>>>>> Niklas Söderlund
>>>>>>>
>>

-- 
Regards
--
Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Dafna Hirschfeld]

On 29.07.20 15:27, Kieran Bingham wrote:
> Hi Dafna, Kaaira,
> 
> On 29/07/2020 14:16, Dafna Hirschfeld wrote:
>>
>>
>> On 29.07.20 15:05, Kieran Bingham wrote:
>>> Hi Dafna,
>>>
>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
>>>>
>>>>
>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>>>>> Hi
>>>>>
>>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
>>>>>>>> list.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>> Hi Kaaira,
>>>>>>>>>>
>>>>>>>>>> Thanks for your work.
>>>>>>>>>
>>>>>>>>> Thanks for yours :D
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
>>>>>>>>>>> allowing
>>>>>>>>>>> multiple streams in VIMC.
>>>>>>>>>>> The original series can be found here:
>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>>>>
>>>>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>>>>> capture device
>>>>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>>>>
>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>>>>> two
>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>>>>> allows for
>>>>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>>>>> allows for
>>>>>>>>>>> simultaneously use.
>>>>>
>>>>> I wonder if these two patches are enough, since each vimc entity also
>>>>> have
>>>>> a 'process_frame' callback, but only one allocated frame. That means
>>>>> that the 'process_frame' can be called concurrently by two different
>>>>> streams
>>>>> on the same frame and cause corruption.
>>>>>
>>>>
>>>> I think we should somehow change the vimc-stream.c code so that we have
>>>> only
>>>> one stream process per pipe. So if one capture is already streaming,
>>>> then the new
>>>> capture that wants to stream uses the same thread so we don't have two
>>>> threads
>>>> both calling 'process_frame'.
>>>
>>>
>>> Yes, I think it looks and sounds like there are two threads running when
>>> there are two streams.
>>>
>>> so in effect, although they 'share a pipe', aren't they in effect just
>>> sending two separate buffers through their stream-path?
>>>
>>> If that's the case, then I don't think there's any frame corruption,
>>> because they would both have grabbed their own frame separately.
>>
>> But each entity allocates just one buffer. So the same buffer is used for
>> both stream.
> 
> Aha, ok, I hadn't realised there was only a single buffer available in
> the pipeline for each entity. Indeed there is a risk of corruption in
> that case.
> 
>> What for example can happen is that the debayer of one stream can read the
>> sensor's buffer while the sensor itself writes to the buffer for the other
>> stream.
> 
> 
> So, In that case, we have currently got a scenario where each 'stream'
> really is operating it's own pipe (even though all components are reused).
> 
> Two questions:
> 
> Is this acceptable, and we should just use a mutex to ensure the buffers
> are not corrupted, but essentially each stream is a separate temporal
> capture?
> 
> 
> Or B:
> 
> Should we refactor to make sure that there is a single thread, and the
> code which calls process_frame on each entity should become aware of the
> potential for multiple paths at the point of the sensor.
> 
> 
> I suspect option B is really the 'right' path to take, but it is more
> complicated of course.

I also think option B is preferable.

Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
The stream thread can do a BFS scan from the sensor up to the captures
and call the 'process_frame' for each entity if 'is_streaming == true'.
When a new capture wants to stream it sets 'is_streaming = true'
on the entities on his streaming path.

Thanks,
Dafna


> 
> --
> Kieran
> 
> 
> 
> 
>> Thanks,
>> Dafna
>>
>>>
>>>
>>> I don't think that's a good example of the hardware though, as that
>>> doesn't reflect what 'should' happen where the TPG runs once to generate
>>> a frame at the sensor, which is then read by both the debayer entity and
>>> the RAW capture device when there are two streams...
>>>
>>>
>>> So I suspect trying to move to a single thread is desirable, but that
>>> might be a fair bit of work also.
>>>
>>> -- 
>>> Kieran
>>>
>>>
>>>
>>>> The second capture that wants to stream should iterate the topology
>>>> downwards until
>>>> reaching an entity that already belong to the stream path of the other
>>>> streaming capture
>>>> and tell the streamer it wants to read the frames this entity
>>>> produces.
>>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>>> Thanks,
>>>>> Dafna
>>>>>
>>>>>>>>>>
>>>>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>>>>> replace the
>>>>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>>>>
>>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>>>>> version
>>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
>>>>>>>>> needed to
>>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>>>>> again.
>>>>>>>>
>>>>>>>> So the question really is, how do we best make use of the two
>>>>>>>> current
>>>>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>>>>
>>>>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>>>>> elements of
>>>>>>>> both ? Or should we work towards starting with this series and get
>>>>>>>> dafna's patches built on top ?
>>>>>>>>
>>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>>>>
>>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>>>>> multiple stream operation from /this/ series and her development
>>>>>>>> branch
>>>>>>>> on libcamera).
>>>>>>>
>>>>>>> Dafna's patch seems still under discussion, but I don't want to
>>>>>>> block progress in Vimc either.
>>>>>>>
>>>>>>> So I was wondering if we can move forward with Vimc support for
>>>>>>> multistreaming,
>>>>>>> without considering Dafna's patchset, and we can do the clean up
>>>>>>> later once we solve that.
>>>>>>>
>>>>>>> What do you think?
>>>>>>
>>>>>> I agree with supporting multiple streams with VIMC with this patchset,
>>>>>> and then we can refactor the counters for s_stream in VIMC later (over
>>>>>> this series) if dafna includes them in subsequent version of her
>>>>>> patchset.
>>>>>>
>>>>>
>>>>> I also think that adding support in the code will take much longer and
>>>>> should not
>>>>> stop us from supporting vimc independently.
>>>>>
>>>>> Thanks,
>>>>> Dafna
>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Helen
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> 1.
>>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Changes since v1:
>>>>>>>>>>>       - All three patches rebased on latest media-tree.
>>>>>>>>>>>       Patch 3:
>>>>>>>>>>>       - Search for an entity with a non-NULL pipe instead of
>>>>>>>>>>> searching
>>>>>>>>>>>         for sensor. This terminates the search at output itself.
>>>>>>>>>>>
>>>>>>>>>>> Kaaira Gupta (3):
>>>>>>>>>>>      media: vimc: Add usage count to subdevices
>>>>>>>>>>>      media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>>>>      media: vimc: Join pipeline if one already exists
>>>>>>>>>>>
>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 23
>>>>>>>>>>> +++++++-----
>>>>>>>>>>>     5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.17.1
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> Regards,
>>>>>>>>>> Niklas Söderlund
>>>>>>>>
>>>
> 

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Laurent Pinchart]

Hi Kaaira,

Thank you for the patches.

On Fri, Jul 24, 2020 at 05:32:10PM +0530, Kaaira Gupta wrote:
> This is version 2 of the patch series posted by Niklas for allowing
> multiple streams in VIMC.
> The original series can be found here:
> https://patchwork.kernel.org/cover/10948831/
> 
> This series adds support for two (or more) capture devices to be 
> connected to the same sensors and run simultaneously. Each capture device 
> can be started and stopped independent of each other.
> 
> Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> capture devices can be part of the same pipeline. While 3/3 allows for 
> two capture devices to be part of the same pipeline and thus allows for 
> simultaneously use.

I think this is really nice work, as it will make the vimc driver even
more useful for testing purposes.

I however just noticed that the patches seem to have lost Niklas'
authorship. Niklas posted v1 ([1]), and while there's absolutely no
issue with taking over a patch series (especially when the original
author is aware of that, and approves :-)), it's customary to keep the
original authorship.

Authorship, as recorded in the commit's "Author:" field (displayed by
"git show" or "git log" for instance), is distinct from Signed-off-by.
The original Signed-off-by line needs to be preserved to indicate the
original author's commitment to the certificate of origin ([2]), but in
itself that doesn't acknowledge original authorship of the code.

I'm sure this is an oversight. Authorship can easily be changed with the
--author option to "git commit --amend".

$ git show -s
commit 8be3a53e18e0e1a98f288f6c7f5e9da3adbe9c49 (HEAD -> tmp)
Merge: fc10807db5ce 3c597282887f
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Jun 24 17:39:30 2020 -0700

    Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs

    Pull erofs fix from Gao Xiang:
     "Fix a regression which uses potential uninitialized high 32-bit value
      unexpectedly recently observed with specific compiler options"

    * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
      erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup
$ git commit --amend --author 'Laurent Pinchart <laurent.pinchart@ideasonboard.com>'
[tmp 6a7191c2aee9] Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
 Date: Wed Jun 24 17:39:30 2020 -0700
$ git show -s
commit 6a7191c2aee9e4a2ba375f14c821bc9b4d7f881b (HEAD -> tmp)
Merge: fc10807db5ce 3c597282887f
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Wed Jun 24 17:39:30 2020 -0700

    Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
    
    Pull erofs fix from Gao Xiang:
     "Fix a regression which uses potential uninitialized high 32-bit value
      unexpectedly recently observed with specific compiler options"
    
    * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
      erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup

Not that I would try to take ownership of a commit authored by Linus, I
doubt he would appreciate that :-)

Authorship is normally preserved through git-format-patch,
git-send-email and git-am:

- git-format-patch sets the "From:" line to the patch's author

- If the "From:" line is different than the mail sender, git-send-email
  replaces it with the sender's identity (as we don't want to forge
  e-mails with an incorrect sender). It then adds the original "From:"
  line *inside* the mail, just after the headers, right before the body
  of the commit message.

- git-am sets the author to the "From:" line from the e-mail's body if
  it exists, and uses the "From:" line from the e-mail's header (the
  sender's identity) otherwise.

If you use those tools authorship should get preserved automatically.

Of course new patches that you would add to the series should have your
authorship.

I hope this helps clarifying the process, please let me know if you have
any question.

[1] https://lore.kernel.org/linux-media/20190518010744.15195-1-niklas.soderlund+renesas@ragnatech.se/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n431

> Changes since v1:
> 	- All three patches rebased on latest media-tree.
> 	Patch 3:
> 	- Search for an entity with a non-NULL pipe instead of searching
> 	  for sensor. This terminates the search at output itself.
> 
> Kaaira Gupta (3):
>   media: vimc: Add usage count to subdevices
>   media: vimc: Serialize vimc_streamer_s_stream()
>   media: vimc: Join pipeline if one already exists
> 
>  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
>  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
>  5 files changed, 73 insertions(+), 10 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

On Thu, Jul 30, 2020 at 01:51:12PM +0300, Laurent Pinchart wrote:
Hi,

> Hi Kaaira,
> 
> Thank you for the patches.
> 
> On Fri, Jul 24, 2020 at 05:32:10PM +0530, Kaaira Gupta wrote:
> > This is version 2 of the patch series posted by Niklas for allowing
> > multiple streams in VIMC.
> > The original series can be found here:
> > https://patchwork.kernel.org/cover/10948831/
> > 
> > This series adds support for two (or more) capture devices to be 
> > connected to the same sensors and run simultaneously. Each capture device 
> > can be started and stopped independent of each other.
> > 
> > Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> > capture devices can be part of the same pipeline. While 3/3 allows for 
> > two capture devices to be part of the same pipeline and thus allows for 
> > simultaneously use.
> 
> I think this is really nice work, as it will make the vimc driver even
> more useful for testing purposes.
> 
> I however just noticed that the patches seem to have lost Niklas'
> authorship. Niklas posted v1 ([1]), and while there's absolutely no
> issue with taking over a patch series (especially when the original
> author is aware of that, and approves :-)), it's customary to keep the
> original authorship.

Hm, I had a meeting with Kieren yesterday where he explained this to me.
I wasn't aware of the distinction between authorship and a Signed-off
tag, I thought signed-off implies authorship. Now that I know this, I
will amend this in the next version I send.

Thanks!

> 
> Authorship, as recorded in the commit's "Author:" field (displayed by
> "git show" or "git log" for instance), is distinct from Signed-off-by.
> The original Signed-off-by line needs to be preserved to indicate the
> original author's commitment to the certificate of origin ([2]), but in
> itself that doesn't acknowledge original authorship of the code.
> 
> I'm sure this is an oversight. Authorship can easily be changed with the
> --author option to "git commit --amend".
> 
> $ git show -s
> commit 8be3a53e18e0e1a98f288f6c7f5e9da3adbe9c49 (HEAD -> tmp)
> Merge: fc10807db5ce 3c597282887f
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Jun 24 17:39:30 2020 -0700
> 
>     Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
> 
>     Pull erofs fix from Gao Xiang:
>      "Fix a regression which uses potential uninitialized high 32-bit value
>       unexpectedly recently observed with specific compiler options"
> 
>     * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
>       erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup
> $ git commit --amend --author 'Laurent Pinchart <laurent.pinchart@ideasonboard.com>'
> [tmp 6a7191c2aee9] Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
>  Date: Wed Jun 24 17:39:30 2020 -0700
> $ git show -s
> commit 6a7191c2aee9e4a2ba375f14c821bc9b4d7f881b (HEAD -> tmp)
> Merge: fc10807db5ce 3c597282887f
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Wed Jun 24 17:39:30 2020 -0700
> 
>     Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
>     
>     Pull erofs fix from Gao Xiang:
>      "Fix a regression which uses potential uninitialized high 32-bit value
>       unexpectedly recently observed with specific compiler options"
>     
>     * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
>       erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup
> 
> Not that I would try to take ownership of a commit authored by Linus, I
> doubt he would appreciate that :-)
> 
> Authorship is normally preserved through git-format-patch,
> git-send-email and git-am:
> 
> - git-format-patch sets the "From:" line to the patch's author
> 
> - If the "From:" line is different than the mail sender, git-send-email
>   replaces it with the sender's identity (as we don't want to forge
>   e-mails with an incorrect sender). It then adds the original "From:"
>   line *inside* the mail, just after the headers, right before the body
>   of the commit message.
> 
> - git-am sets the author to the "From:" line from the e-mail's body if
>   it exists, and uses the "From:" line from the e-mail's header (the
>   sender's identity) otherwise.
> 
> If you use those tools authorship should get preserved automatically.
> 
> Of course new patches that you would add to the series should have your
> authorship.
> 
> I hope this helps clarifying the process, please let me know if you have
> any question.
> 
> [1] https://lore.kernel.org/linux-media/20190518010744.15195-1-niklas.soderlund+renesas@ragnatech.se/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n431
> 
> > Changes since v1:
> > 	- All three patches rebased on latest media-tree.
> > 	Patch 3:
> > 	- Search for an entity with a non-NULL pipe instead of searching
> > 	  for sensor. This terminates the search at output itself.
> > 
> > Kaaira Gupta (3):
> >   media: vimc: Add usage count to subdevices
> >   media: vimc: Serialize vimc_streamer_s_stream()
> >   media: vimc: Join pipeline if one already exists
> > 
> >  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
> >  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> >  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> >  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> >  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
> >  5 files changed, 73 insertions(+), 10 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Laurent Pinchart]

Hi Kaaira,

On Thu, Jul 30, 2020 at 11:39:49PM +0530, Kaaira Gupta wrote:
> On Thu, Jul 30, 2020 at 01:51:12PM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 24, 2020 at 05:32:10PM +0530, Kaaira Gupta wrote:
> > > This is version 2 of the patch series posted by Niklas for allowing
> > > multiple streams in VIMC.
> > > The original series can be found here:
> > > https://patchwork.kernel.org/cover/10948831/
> > > 
> > > This series adds support for two (or more) capture devices to be 
> > > connected to the same sensors and run simultaneously. Each capture device 
> > > can be started and stopped independent of each other.
> > > 
> > > Patch 1/3 and 2/3 deals with solving the issues that arises once two 
> > > capture devices can be part of the same pipeline. While 3/3 allows for 
> > > two capture devices to be part of the same pipeline and thus allows for 
> > > simultaneously use.
> > 
> > I think this is really nice work, as it will make the vimc driver even
> > more useful for testing purposes.
> > 
> > I however just noticed that the patches seem to have lost Niklas'
> > authorship. Niklas posted v1 ([1]), and while there's absolutely no
> > issue with taking over a patch series (especially when the original
> > author is aware of that, and approves :-)), it's customary to keep the
> > original authorship.
> 
> Hm, I had a meeting with Kieren yesterday where he explained this to me.
> I wasn't aware of the distinction between authorship and a Signed-off
> tag, I thought signed-off implies authorship. Now that I know this, I
> will amend this in the next version I send.

No worries at all. The process is complicated :-)

> > Authorship, as recorded in the commit's "Author:" field (displayed by
> > "git show" or "git log" for instance), is distinct from Signed-off-by.
> > The original Signed-off-by line needs to be preserved to indicate the
> > original author's commitment to the certificate of origin ([2]), but in
> > itself that doesn't acknowledge original authorship of the code.
> > 
> > I'm sure this is an oversight. Authorship can easily be changed with the
> > --author option to "git commit --amend".
> > 
> > $ git show -s
> > commit 8be3a53e18e0e1a98f288f6c7f5e9da3adbe9c49 (HEAD -> tmp)
> > Merge: fc10807db5ce 3c597282887f
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Wed Jun 24 17:39:30 2020 -0700
> > 
> >     Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
> > 
> >     Pull erofs fix from Gao Xiang:
> >      "Fix a regression which uses potential uninitialized high 32-bit value
> >       unexpectedly recently observed with specific compiler options"
> > 
> >     * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
> >       erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup
> > $ git commit --amend --author 'Laurent Pinchart <laurent.pinchart@ideasonboard.com>'
> > [tmp 6a7191c2aee9] Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
> >  Date: Wed Jun 24 17:39:30 2020 -0700
> > $ git show -s
> > commit 6a7191c2aee9e4a2ba375f14c821bc9b4d7f881b (HEAD -> tmp)
> > Merge: fc10807db5ce 3c597282887f
> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date:   Wed Jun 24 17:39:30 2020 -0700
> > 
> >     Merge tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
> >     
> >     Pull erofs fix from Gao Xiang:
> >      "Fix a regression which uses potential uninitialized high 32-bit value
> >       unexpectedly recently observed with specific compiler options"
> >     
> >     * tag 'erofs-for-5.8-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs:
> >       erofs: fix partially uninitialized misuse in z_erofs_onlinepage_fixup
> > 
> > Not that I would try to take ownership of a commit authored by Linus, I
> > doubt he would appreciate that :-)
> > 
> > Authorship is normally preserved through git-format-patch,
> > git-send-email and git-am:
> > 
> > - git-format-patch sets the "From:" line to the patch's author
> > 
> > - If the "From:" line is different than the mail sender, git-send-email
> >   replaces it with the sender's identity (as we don't want to forge
> >   e-mails with an incorrect sender). It then adds the original "From:"
> >   line *inside* the mail, just after the headers, right before the body
> >   of the commit message.
> > 
> > - git-am sets the author to the "From:" line from the e-mail's body if
> >   it exists, and uses the "From:" line from the e-mail's header (the
> >   sender's identity) otherwise.
> > 
> > If you use those tools authorship should get preserved automatically.
> > 
> > Of course new patches that you would add to the series should have your
> > authorship.
> > 
> > I hope this helps clarifying the process, please let me know if you have
> > any question.
> > 
> > [1] https://lore.kernel.org/linux-media/20190518010744.15195-1-niklas.soderlund+renesas@ragnatech.se/
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n431
> > 
> > > Changes since v1:
> > > 	- All three patches rebased on latest media-tree.
> > > 	Patch 3:
> > > 	- Search for an entity with a non-NULL pipe instead of searching
> > > 	  for sensor. This terminates the search at output itself.
> > > 
> > > Kaaira Gupta (3):
> > >   media: vimc: Add usage count to subdevices
> > >   media: vimc: Serialize vimc_streamer_s_stream()
> > >   media: vimc: Join pipeline if one already exists
> > > 
> > >  .../media/test-drivers/vimc/vimc-capture.c    | 35 ++++++++++++++++++-
> > >  .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> > >  drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> > >  drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> > >  .../media/test-drivers/vimc/vimc-streamer.c   | 23 +++++++-----
> > >  5 files changed, 73 insertions(+), 10 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

Hi everyone,

On Wed, Jul 29, 2020 at 05:24:25PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 29.07.20 15:27, Kieran Bingham wrote:
> > Hi Dafna, Kaaira,
> > 
> > On 29/07/2020 14:16, Dafna Hirschfeld wrote:
> > > 
> > > 
> > > On 29.07.20 15:05, Kieran Bingham wrote:
> > > > Hi Dafna,
> > > > 
> > > > On 28/07/2020 15:00, Dafna Hirschfeld wrote:
> > > > > 
> > > > > 
> > > > > On 28.07.20 14:07, Dafna Hirschfeld wrote:
> > > > > > Hi
> > > > > > 
> > > > > > On 28.07.20 13:39, Kaaira Gupta wrote:
> > > > > > > On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 7/27/20 11:31 AM, Kieran Bingham wrote:
> > > > > > > > > Hi all,
> > > > > > > > > 
> > > > > > > > > +Dafna for the thread discussion, as she's missed from the to/cc
> > > > > > > > > list.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 24/07/2020 13:21, Kaaira Gupta wrote:
> > > > > > > > > > On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > > Hi Kaaira,
> > > > > > > > > > > 
> > > > > > > > > > > Thanks for your work.
> > > > > > > > > > 
> > > > > > > > > > Thanks for yours :D
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> > > > > > > > > > > > This is version 2 of the patch series posted by Niklas for
> > > > > > > > > > > > allowing
> > > > > > > > > > > > multiple streams in VIMC.
> > > > > > > > > > > > The original series can be found here:
> > > > > > > > > > > > https://patchwork.kernel.org/cover/10948831/
> > > > > > > > > > > > 
> > > > > > > > > > > > This series adds support for two (or more) capture devices to be
> > > > > > > > > > > > connected to the same sensors and run simultaneously. Each
> > > > > > > > > > > > capture device
> > > > > > > > > > > > can be started and stopped independent of each other.
> > > > > > > > > > > > 
> > > > > > > > > > > > Patch 1/3 and 2/3 deals with solving the issues that arises once
> > > > > > > > > > > > two
> > > > > > > > > > > > capture devices can be part of the same pipeline. While 3/3
> > > > > > > > > > > > allows for
> > > > > > > > > > > > two capture devices to be part of the same pipeline and thus
> > > > > > > > > > > > allows for
> > > > > > > > > > > > simultaneously use.
> > > > > > 
> > > > > > I wonder if these two patches are enough, since each vimc entity also
> > > > > > have
> > > > > > a 'process_frame' callback, but only one allocated frame. That means
> > > > > > that the 'process_frame' can be called concurrently by two different
> > > > > > streams
> > > > > > on the same frame and cause corruption.
> > > > > > 
> > > > > 
> > > > > I think we should somehow change the vimc-stream.c code so that we have
> > > > > only
> > > > > one stream process per pipe. So if one capture is already streaming,
> > > > > then the new
> > > > > capture that wants to stream uses the same thread so we don't have two
> > > > > threads
> > > > > both calling 'process_frame'.
> > > > 
> > > > 
> > > > Yes, I think it looks and sounds like there are two threads running when
> > > > there are two streams.
> > > > 
> > > > so in effect, although they 'share a pipe', aren't they in effect just
> > > > sending two separate buffers through their stream-path?
> > > > 
> > > > If that's the case, then I don't think there's any frame corruption,
> > > > because they would both have grabbed their own frame separately.
> > > 
> > > But each entity allocates just one buffer. So the same buffer is used for
> > > both stream.
> > 
> > Aha, ok, I hadn't realised there was only a single buffer available in
> > the pipeline for each entity. Indeed there is a risk of corruption in
> > that case.
> > 
> > > What for example can happen is that the debayer of one stream can read the
> > > sensor's buffer while the sensor itself writes to the buffer for the other
> > > stream.
> > 
> > 
> > So, In that case, we have currently got a scenario where each 'stream'
> > really is operating it's own pipe (even though all components are reused).
> > 
> > Two questions:
> > 
> > Is this acceptable, and we should just use a mutex to ensure the buffers
> > are not corrupted, but essentially each stream is a separate temporal
> > capture?
> > 
> > 
> > Or B:
> > 
> > Should we refactor to make sure that there is a single thread, and the
> > code which calls process_frame on each entity should become aware of the
> > potential for multiple paths at the point of the sensor.
> > 
> > 
> > I suspect option B is really the 'right' path to take, but it is more
> > complicated of course.
> 
> I also think option B is preferable.
> 
> Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
> The stream thread can do a BFS scan from the sensor up to the captures
> and call the 'process_frame' for each entity if 'is_streaming == true'.
> When a new capture wants to stream it sets 'is_streaming = true'
> on the entities on his streaming path.

It is s_stream(enable) that initialises a streaming pipeline, ie the one with
those components of the pipeline which are in stream path and then runs a
thread which calls process_frame on each and passes the frame to the
next entity in streaming pipeline. So currently, one thread is for one
"streaming pipeline". So there are two options I can think of if a
single thread is required,

1. Not creating a streaming pipeline, rather create a graph(?) which
connects both say Raw capture 1 and debayer B to sensor B if two streams
are asked for, and only one of them if one stream is asked..that will
not be a property of streamer, so I am not sure where it should be kept.
Then I could move creating a thread out of s_stream. Creating the thread
should wait for entire pipeline to be created, ie s_stream(enable) to
must be called by both the captures, and a graph made of all pipeline
components before thread initialisation starts. I am not sure how this
should be implemented.

2. Another option is to check if a stream already exists (by creating it
a property of vimc to keep a track of no. of streams maybe?), if it is
already present I could take the previous output of sensor (but
then it will have to be stored, so i don't think this is a nice idea),
and use it further (but thread will be different in this case).

What can be a better design for VIMC to have a single thread if two
streams are asked (apart/of the options I mentioned)?

Thanks
Kaaira

> 
> Thanks,
> Dafna
> 
> 
> > 
> > --
> > Kieran
> > 
> > 
> > 
> > 
> > > Thanks,
> > > Dafna
> > > 
> > > > 
> > > > 
> > > > I don't think that's a good example of the hardware though, as that
> > > > doesn't reflect what 'should' happen where the TPG runs once to generate
> > > > a frame at the sensor, which is then read by both the debayer entity and
> > > > the RAW capture device when there are two streams...
> > > > 
> > > > 
> > > > So I suspect trying to move to a single thread is desirable, but that
> > > > might be a fair bit of work also.
> > > > 
> > > > -- 
> > > > Kieran
> > > > 
> > > > 
> > > > 
> > > > > The second capture that wants to stream should iterate the topology
> > > > > downwards until
> > > > > reaching an entity that already belong to the stream path of the other
> > > > > streaming capture
> > > > > and tell the streamer it wants to read the frames this entity
> > > > > produces.
> > > > > 
> > > > > Thanks,
> > > > > Dafna
> > > > > 
> > > > > > Thanks,
> > > > > > Dafna
> > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > I'm just curious if you are aware of this series? It would
> > > > > > > > > > > replace the
> > > > > > > > > > > need for 1/3 and 2/3 of this series right?
> > > > > > > > > > 
> > > > > > > > > > v3 of this series replaces the need for 1/3, but not the current
> > > > > > > > > > version
> > > > > > > > > > (ie v4). v4 of patch 2/5 removes the stream_counter that is
> > > > > > > > > > needed to
> > > > > > > > > > keep count of the calls to s_stream. Hence 1/3 becomes relevant
> > > > > > > > > > again.
> > > > > > > > > 
> > > > > > > > > So the question really is, how do we best make use of the two
> > > > > > > > > current
> > > > > > > > > series, to achieve our goal of supporting multiple streams.
> > > > > > > > > 
> > > > > > > > > Having not parsed Dafna's series yet, do we need to combine
> > > > > > > > > elements of
> > > > > > > > > both ? Or should we work towards starting with this series and get
> > > > > > > > > dafna's patches built on top ?
> > > > > > > > > 
> > > > > > > > > Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> > > > > > > > > 
> > > > > > > > > (It might be noteworthy to say that Kaaira has reported successful
> > > > > > > > > multiple stream operation from /this/ series and her development
> > > > > > > > > branch
> > > > > > > > > on libcamera).
> > > > > > > > 
> > > > > > > > Dafna's patch seems still under discussion, but I don't want to
> > > > > > > > block progress in Vimc either.
> > > > > > > > 
> > > > > > > > So I was wondering if we can move forward with Vimc support for
> > > > > > > > multistreaming,
> > > > > > > > without considering Dafna's patchset, and we can do the clean up
> > > > > > > > later once we solve that.
> > > > > > > > 
> > > > > > > > What do you think?
> > > > > > > 
> > > > > > > I agree with supporting multiple streams with VIMC with this patchset,
> > > > > > > and then we can refactor the counters for s_stream in VIMC later (over
> > > > > > > this series) if dafna includes them in subsequent version of her
> > > > > > > patchset.
> > > > > > > 
> > > > > > 
> > > > > > I also think that adding support in the code will take much longer and
> > > > > > should not
> > > > > > stop us from supporting vimc independently.
> > > > > > 
> > > > > > Thanks,
> > > > > > Dafna
> > > > > > 
> > > > > > > > 
> > > > > > > > Regards,
> > > > > > > > Helen
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > > 1.
> > > > > > > > > > > https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Changes since v1:
> > > > > > > > > > > >       - All three patches rebased on latest media-tree.
> > > > > > > > > > > >       Patch 3:
> > > > > > > > > > > >       - Search for an entity with a non-NULL pipe instead of
> > > > > > > > > > > > searching
> > > > > > > > > > > >         for sensor. This terminates the search at output itself.
> > > > > > > > > > > > 
> > > > > > > > > > > > Kaaira Gupta (3):
> > > > > > > > > > > >      media: vimc: Add usage count to subdevices
> > > > > > > > > > > >      media: vimc: Serialize vimc_streamer_s_stream()
> > > > > > > > > > > >      media: vimc: Join pipeline if one already exists
> > > > > > > > > > > > 
> > > > > > > > > > > >     .../media/test-drivers/vimc/vimc-capture.c    | 35
> > > > > > > > > > > > ++++++++++++++++++-
> > > > > > > > > > > >     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> > > > > > > > > > > >     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> > > > > > > > > > > >     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> > > > > > > > > > > >     .../media/test-drivers/vimc/vimc-streamer.c   | 23
> > > > > > > > > > > > +++++++-----
> > > > > > > > > > > >     5 files changed, 73 insertions(+), 10 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > -- 
> > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > -- 
> > > > > > > > > > > Regards,
> > > > > > > > > > > Niklas Söderlund
> > > > > > > > > 
> > > > 
> > 

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kieran Bingham]

Hi Kaaira,

On 31/07/2020 18:22, Kaaira Gupta wrote:
> Hi everyone,
> 
> On Wed, Jul 29, 2020 at 05:24:25PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 29.07.20 15:27, Kieran Bingham wrote:
>>> Hi Dafna, Kaaira,
>>>
>>> On 29/07/2020 14:16, Dafna Hirschfeld wrote:
>>>>
>>>>
>>>> On 29.07.20 15:05, Kieran Bingham wrote:
>>>>> Hi Dafna,
>>>>>
>>>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
>>>>>>
>>>>>>
>>>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
>>>>>>>>>> list.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>>> Hi Kaaira,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for your work.
>>>>>>>>>>>
>>>>>>>>>>> Thanks for yours :D
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
>>>>>>>>>>>>> allowing
>>>>>>>>>>>>> multiple streams in VIMC.
>>>>>>>>>>>>> The original series can be found here:
>>>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>>>>>>
>>>>>>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>>>>>>> capture device
>>>>>>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>>>>>>> two
>>>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>>>>>>> allows for
>>>>>>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>>>>>>> allows for
>>>>>>>>>>>>> simultaneously use.
>>>>>>>
>>>>>>> I wonder if these two patches are enough, since each vimc entity also
>>>>>>> have
>>>>>>> a 'process_frame' callback, but only one allocated frame. That means
>>>>>>> that the 'process_frame' can be called concurrently by two different
>>>>>>> streams
>>>>>>> on the same frame and cause corruption.
>>>>>>>
>>>>>>
>>>>>> I think we should somehow change the vimc-stream.c code so that we have
>>>>>> only
>>>>>> one stream process per pipe. So if one capture is already streaming,
>>>>>> then the new
>>>>>> capture that wants to stream uses the same thread so we don't have two
>>>>>> threads
>>>>>> both calling 'process_frame'.
>>>>>
>>>>>
>>>>> Yes, I think it looks and sounds like there are two threads running when
>>>>> there are two streams.
>>>>>
>>>>> so in effect, although they 'share a pipe', aren't they in effect just
>>>>> sending two separate buffers through their stream-path?
>>>>>
>>>>> If that's the case, then I don't think there's any frame corruption,
>>>>> because they would both have grabbed their own frame separately.
>>>>
>>>> But each entity allocates just one buffer. So the same buffer is used for
>>>> both stream.
>>>
>>> Aha, ok, I hadn't realised there was only a single buffer available in
>>> the pipeline for each entity. Indeed there is a risk of corruption in
>>> that case.
>>>
>>>> What for example can happen is that the debayer of one stream can read the
>>>> sensor's buffer while the sensor itself writes to the buffer for the other
>>>> stream.
>>>
>>>
>>> So, In that case, we have currently got a scenario where each 'stream'
>>> really is operating it's own pipe (even though all components are reused).
>>>
>>> Two questions:
>>>
>>> Is this acceptable, and we should just use a mutex to ensure the buffers
>>> are not corrupted, but essentially each stream is a separate temporal
>>> capture?
>>>
>>>
>>> Or B:
>>>
>>> Should we refactor to make sure that there is a single thread, and the
>>> code which calls process_frame on each entity should become aware of the
>>> potential for multiple paths at the point of the sensor.
>>>
>>>
>>> I suspect option B is really the 'right' path to take, but it is more
>>> complicated of course.
>>
>> I also think option B is preferable.
>>
>> Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
>> The stream thread can do a BFS scan from the sensor up to the captures
>> and call the 'process_frame' for each entity if 'is_streaming == true'.
>> When a new capture wants to stream it sets 'is_streaming = true'
>> on the entities on his streaming path.
> 
> It is s_stream(enable) that initialises a streaming pipeline, ie the one with
> those components of the pipeline which are in stream path and then runs a
> thread which calls process_frame on each and passes the frame to the
> next entity in streaming pipeline. So currently, one thread is for one
> "streaming pipeline". So there are two options I can think of if a
> single thread is required,
> 
> 1. Not creating a streaming pipeline, rather create a graph(?) which
> connects both say Raw capture 1 and debayer B to sensor B if two streams
> are asked for, and only one of them if one stream is asked..that will
> not be a property of streamer, so I am not sure where it should be kept.
> Then I could move creating a thread out of s_stream. Creating the thread
> should wait for entire pipeline to be created, ie s_stream(enable) to
> must be called by both the captures, and a graph made of all pipeline
> components before thread initialisation starts. I am not sure how this
> should be implemented.

The graph already exists, and can be walked through the media controller
right?


> 2. Another option is to check if a stream already exists (by creating it
> a property of vimc to keep a track of no. of streams maybe?), if it is
> already present I could take the previous output of sensor (but
> then it will have to be stored, so i don't think this is a nice idea),
> and use it further (but thread will be different in this case).

I don't think I understand this one...


> What can be a better design for VIMC to have a single thread if two
> streams are asked (apart/of the options I mentioned)?

How about adding a count in s_stream so that the thread only gets
started when the use count is > 0, and stopped when the usage < 1.

That handles making sure that only one thread is available.

All calls into s_stream() will need to take a lock/mutex to protect /
prevent any action from occurring while the thread is performing a
process of the pipeline.


static int vimc_streamer_thread(void *data)
{
	struct vimc_stream *stream = data;
	u8 *frame = NULL;
	int i;

	set_freezable();

	for (;;) {
		try_to_freeze();
		if (kthread_should_stop())
			break;

+		/* take lock shared with s_stream */

		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))
				break;
		}

+		/* Release lock/mutex shared with s_stream

		//wait for 60hz
		set_current_state(TASK_UNINTERRUPTIBLE);
		schedule_timeout(HZ / 60);
	}

	return 0;
}



And you'll need to make the code which processes the pipeline aware of
the fact that there may be two pipelines to fulfil:

Pseudo patch/code:

static int vimc_streamer_thread(void *data)
{
-	struct vimc_stream *stream = data;
+	/* Something which knows about the whole device */
+	struct xxxxx *yyy = data;

+	u8 *raw;
	u8 *frame = NULL;
	int i;

	set_freezable();

	for (;;) {
		try_to_freeze();
		if (kthread_should_stop())
			break;

		/* take lock shared with s_stream */

+		/* Process the sensor first */
+		raw = stream->ved_pipeline[sensor]->process_frame(..);
+		error check;

+		/* (If connected) Process stream 1 */
+		if (raw)
+		frame = stream->ved_pipeline[raw]->process_frame();
+		error check;

+		/* If connected process the rest of the pipe */
+		for (i = after sensor; end_entity; i++) {
			frame = stream->ved_pipeline[i]->process_frame(
					stream->ved_pipeline[i], frame);
			if (!frame || IS_ERR(frame))
				break;
		}

		/* Release lock/mutex shared with s_stream

		//wait for 60hz
		set_current_state(TASK_UNINTERRUPTIBLE);
		schedule_timeout(HZ / 60);
	}

	return 0;
}



I may have missed something as the original loop was decrementing and
going backwards through the entities in stream->ved_pipeline.

I guess splitting that all out so instead it starts at the sensor, and
just walks the graph (handling any running/connected fork to two
entities appropriately) in a neater way would be another option rather
than hardcoding it, but either way the thread needs to operate at the
device level rather than the stream level.



> Thanks
> Kaaira
> 
>>
>> Thanks,
>> Dafna
>>
>>
>>>
>>> --
>>> Kieran
>>>
>>>
>>>
>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>>>
>>>>>
>>>>> I don't think that's a good example of the hardware though, as that
>>>>> doesn't reflect what 'should' happen where the TPG runs once to generate
>>>>> a frame at the sensor, which is then read by both the debayer entity and
>>>>> the RAW capture device when there are two streams...
>>>>>
>>>>>
>>>>> So I suspect trying to move to a single thread is desirable, but that
>>>>> might be a fair bit of work also.
>>>>>
>>>>> -- 
>>>>> Kieran
>>>>>
>>>>>
>>>>>
>>>>>> The second capture that wants to stream should iterate the topology
>>>>>> downwards until
>>>>>> reaching an entity that already belong to the stream path of the other
>>>>>> streaming capture
>>>>>> and tell the streamer it wants to read the frames this entity
>>>>>> produces.
>>>>>>
>>>>>> Thanks,
>>>>>> Dafna
>>>>>>
>>>>>>> Thanks,
>>>>>>> Dafna
>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>>>>>>> replace the
>>>>>>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>>>>>>
>>>>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>>>>>>> version
>>>>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
>>>>>>>>>>> needed to
>>>>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>>>>>>> again.
>>>>>>>>>>
>>>>>>>>>> So the question really is, how do we best make use of the two
>>>>>>>>>> current
>>>>>>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>>>>>>
>>>>>>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>>>>>>> elements of
>>>>>>>>>> both ? Or should we work towards starting with this series and get
>>>>>>>>>> dafna's patches built on top ?
>>>>>>>>>>
>>>>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>>>>>>
>>>>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>>>>>>> multiple stream operation from /this/ series and her development
>>>>>>>>>> branch
>>>>>>>>>> on libcamera).
>>>>>>>>>
>>>>>>>>> Dafna's patch seems still under discussion, but I don't want to
>>>>>>>>> block progress in Vimc either.
>>>>>>>>>
>>>>>>>>> So I was wondering if we can move forward with Vimc support for
>>>>>>>>> multistreaming,
>>>>>>>>> without considering Dafna's patchset, and we can do the clean up
>>>>>>>>> later once we solve that.
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> I agree with supporting multiple streams with VIMC with this patchset,
>>>>>>>> and then we can refactor the counters for s_stream in VIMC later (over
>>>>>>>> this series) if dafna includes them in subsequent version of her
>>>>>>>> patchset.
>>>>>>>>
>>>>>>>
>>>>>>> I also think that adding support in the code will take much longer and
>>>>>>> should not
>>>>>>> stop us from supporting vimc independently.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Dafna
>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Helen
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> 1.
>>>>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes since v1:
>>>>>>>>>>>>>       - All three patches rebased on latest media-tree.
>>>>>>>>>>>>>       Patch 3:
>>>>>>>>>>>>>       - Search for an entity with a non-NULL pipe instead of
>>>>>>>>>>>>> searching
>>>>>>>>>>>>>         for sensor. This terminates the search at output itself.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Kaaira Gupta (3):
>>>>>>>>>>>>>      media: vimc: Add usage count to subdevices
>>>>>>>>>>>>>      media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>>>>>>      media: vimc: Join pipeline if one already exists
>>>>>>>>>>>>>
>>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 23
>>>>>>>>>>>>> +++++++-----
>>>>>>>>>>>>>     5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> 2.17.1
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Niklas Söderlund
>>>>>>>>>>
>>>>>
>>>

-- 
Regards
--
Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

Hi

On Tue, Aug 04, 2020 at 11:24:56AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 31/07/2020 18:22, Kaaira Gupta wrote:
> > Hi everyone,
> > 
> > On Wed, Jul 29, 2020 at 05:24:25PM +0200, Dafna Hirschfeld wrote:
> >>
> >>
> >> On 29.07.20 15:27, Kieran Bingham wrote:
> >>> Hi Dafna, Kaaira,
> >>>
> >>> On 29/07/2020 14:16, Dafna Hirschfeld wrote:
> >>>>
> >>>>
> >>>> On 29.07.20 15:05, Kieran Bingham wrote:
> >>>>> Hi Dafna,
> >>>>>
> >>>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
> >>>>>>> Hi
> >>>>>>>
> >>>>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
> >>>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
> >>>>>>>>>> Hi all,
> >>>>>>>>>>
> >>>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
> >>>>>>>>>> list.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
> >>>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>>> Hi Kaaira,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks for your work.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for yours :D
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> >>>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
> >>>>>>>>>>>>> allowing
> >>>>>>>>>>>>> multiple streams in VIMC.
> >>>>>>>>>>>>> The original series can be found here:
> >>>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This series adds support for two (or more) capture devices to be
> >>>>>>>>>>>>> connected to the same sensors and run simultaneously. Each
> >>>>>>>>>>>>> capture device
> >>>>>>>>>>>>> can be started and stopped independent of each other.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
> >>>>>>>>>>>>> two
> >>>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
> >>>>>>>>>>>>> allows for
> >>>>>>>>>>>>> two capture devices to be part of the same pipeline and thus
> >>>>>>>>>>>>> allows for
> >>>>>>>>>>>>> simultaneously use.
> >>>>>>>
> >>>>>>> I wonder if these two patches are enough, since each vimc entity also
> >>>>>>> have
> >>>>>>> a 'process_frame' callback, but only one allocated frame. That means
> >>>>>>> that the 'process_frame' can be called concurrently by two different
> >>>>>>> streams
> >>>>>>> on the same frame and cause corruption.
> >>>>>>>
> >>>>>>
> >>>>>> I think we should somehow change the vimc-stream.c code so that we have
> >>>>>> only
> >>>>>> one stream process per pipe. So if one capture is already streaming,
> >>>>>> then the new
> >>>>>> capture that wants to stream uses the same thread so we don't have two
> >>>>>> threads
> >>>>>> both calling 'process_frame'.
> >>>>>
> >>>>>
> >>>>> Yes, I think it looks and sounds like there are two threads running when
> >>>>> there are two streams.
> >>>>>
> >>>>> so in effect, although they 'share a pipe', aren't they in effect just
> >>>>> sending two separate buffers through their stream-path?
> >>>>>
> >>>>> If that's the case, then I don't think there's any frame corruption,
> >>>>> because they would both have grabbed their own frame separately.
> >>>>
> >>>> But each entity allocates just one buffer. So the same buffer is used for
> >>>> both stream.
> >>>
> >>> Aha, ok, I hadn't realised there was only a single buffer available in
> >>> the pipeline for each entity. Indeed there is a risk of corruption in
> >>> that case.
> >>>
> >>>> What for example can happen is that the debayer of one stream can read the
> >>>> sensor's buffer while the sensor itself writes to the buffer for the other
> >>>> stream.
> >>>
> >>>
> >>> So, In that case, we have currently got a scenario where each 'stream'
> >>> really is operating it's own pipe (even though all components are reused).
> >>>
> >>> Two questions:
> >>>
> >>> Is this acceptable, and we should just use a mutex to ensure the buffers
> >>> are not corrupted, but essentially each stream is a separate temporal
> >>> capture?
> >>>
> >>>
> >>> Or B:
> >>>
> >>> Should we refactor to make sure that there is a single thread, and the
> >>> code which calls process_frame on each entity should become aware of the
> >>> potential for multiple paths at the point of the sensor.
> >>>
> >>>
> >>> I suspect option B is really the 'right' path to take, but it is more
> >>> complicated of course.
> >>
> >> I also think option B is preferable.
> >>
> >> Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
> >> The stream thread can do a BFS scan from the sensor up to the captures
> >> and call the 'process_frame' for each entity if 'is_streaming == true'.
> >> When a new capture wants to stream it sets 'is_streaming = true'
> >> on the entities on his streaming path.
> > 
> > It is s_stream(enable) that initialises a streaming pipeline, ie the one with
> > those components of the pipeline which are in stream path and then runs a
> > thread which calls process_frame on each and passes the frame to the
> > next entity in streaming pipeline. So currently, one thread is for one
> > "streaming pipeline". So there are two options I can think of if a
> > single thread is required,
> > 
> > 1. Not creating a streaming pipeline, rather create a graph(?) which
> > connects both say Raw capture 1 and debayer B to sensor B if two streams
> > are asked for, and only one of them if one stream is asked..that will
> > not be a property of streamer, so I am not sure where it should be kept.
> > Then I could move creating a thread out of s_stream. Creating the thread
> > should wait for entire pipeline to be created, ie s_stream(enable) to
> > must be called by both the captures, and a graph made of all pipeline
> > components before thread initialisation starts. I am not sure how this
> > should be implemented.
> 
> The graph already exists, and can be walked through the media controller
> right?

Yes, but actually, the current implementation of the thread does not
walk through the entire pipeline, rather ved_pipeline, which is the
portions of the pipeline which come in the streaming path of a stream
which calls the thread. This also answers your doubt why the pipeline is
decreamenting (which you have asked later in the mail). ved_pipeline is
an array of the entities in the path of the stream starting from
capture. Hence my suggestion was if I should make a data structure (a 
graph) which holds all the entities in one or more stream path.

> 
> 
> > 2. Another option is to check if a stream already exists (by creating it
> > a property of vimc to keep a track of no. of streams maybe?), if it is
> > already present I could take the previous output of sensor (but
> > then it will have to be stored, so i don't think this is a nice idea),
> > and use it further (but thread will be different in this case).
> 
> I don't think I understand this one...

I meant that to start a thread for the driver, rather than each stream,
maybe instead of creating a graph (as in the first option), maybe the
druver could know the number of steams alsready running, and hence when
s_stream is called for another, it knows how many and what type of
streams are running, which can then /know/ which entity's output of
process_frame(previous stream) to give on to the current stream. But I
wasn't sure about the implementation of the /knwing/part without
hardcoding it to take output from the sensor. But since that is the only
topology (hardcoded)we have currently, maybe that can be a solution,
hence I asked.

> 
> 
> > What can be a better design for VIMC to have a single thread if two
> > streams are asked (apart/of the options I mentioned)?
> 
> How about adding a count in s_stream so that the thread only gets
> started when the use count is > 0, and stopped when the usage < 1.
> 
> That handles making sure that only one thread is available.
> 
> All calls into s_stream() will need to take a lock/mutex to protect /
> prevent any action from occurring while the thread is performing a
> process of the pipeline.
> 
> 
> static int vimc_streamer_thread(void *data)
> {
> 	struct vimc_stream *stream = data;
> 	u8 *frame = NULL;
> 	int i;
> 
> 	set_freezable();
> 
> 	for (;;) {
> 		try_to_freeze();
> 		if (kthread_should_stop())
> 			break;
> 
> +		/* take lock shared with s_stream */
> 
> 		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))
> 				break;
> 		}
> 
> +		/* Release lock/mutex shared with s_stream
> 
> 		//wait for 60hz
> 		set_current_state(TASK_UNINTERRUPTIBLE);
> 		schedule_timeout(HZ / 60);
> 	}
> 
> 	return 0;
> }
> 
> 
> 
> And you'll need to make the code which processes the pipeline aware of
> the fact that there may be two pipelines to fulfil:
> 
> Pseudo patch/code:
> 
> static int vimc_streamer_thread(void *data)
> {
> -	struct vimc_stream *stream = data;
> +	/* Something which knows about the whole device */
> +	struct xxxxx *yyy = data;
> 
> +	u8 *raw;
> 	u8 *frame = NULL;
> 	int i;
> 
> 	set_freezable();
> 
> 	for (;;) {
> 		try_to_freeze();
> 		if (kthread_should_stop())
> 			break;
> 
> 		/* take lock shared with s_stream */
> 
> +		/* Process the sensor first */
> +		raw = stream->ved_pipeline[sensor]->process_frame(..);
> +		error check;
> 
> +		/* (If connected) Process stream 1 */
> +		if (raw)
> +		frame = stream->ved_pipeline[raw]->process_frame();
> +		error check;
> 
> +		/* If connected process the rest of the pipe */
> +		for (i = after sensor; end_entity; i++) {
> 			frame = stream->ved_pipeline[i]->process_frame(
> 					stream->ved_pipeline[i], frame);
> 			if (!frame || IS_ERR(frame))
> 				break;
> 		}
> 
> 		/* Release lock/mutex shared with s_stream
> 
> 		//wait for 60hz
> 		set_current_state(TASK_UNINTERRUPTIBLE);
> 		schedule_timeout(HZ / 60);
> 	}
> 
> 	return 0;
> }
> 
> 
> 
> I may have missed something as the original loop was decrementing and
> going backwards through the entities in stream->ved_pipeline.
> 
> I guess splitting that all out so instead it starts at the sensor, and
> just walks the graph (handling any running/connected fork to two
> entities appropriately) in a neater way would be another option rather
> than hardcoding it, but either way the thread needs to operate at the
> device level rather than the stream level.
> 
> 
> 
> > Thanks
> > Kaaira
> > 
> >>
> >> Thanks,
> >> Dafna
> >>
> >>
> >>>
> >>> --
> >>> Kieran
> >>>
> >>>
> >>>
> >>>
> >>>> Thanks,
> >>>> Dafna
> >>>>
> >>>>>
> >>>>>
> >>>>> I don't think that's a good example of the hardware though, as that
> >>>>> doesn't reflect what 'should' happen where the TPG runs once to generate
> >>>>> a frame at the sensor, which is then read by both the debayer entity and
> >>>>> the RAW capture device when there are two streams...
> >>>>>
> >>>>>
> >>>>> So I suspect trying to move to a single thread is desirable, but that
> >>>>> might be a fair bit of work also.
> >>>>>
> >>>>> -- 
> >>>>> Kieran
> >>>>>
> >>>>>
> >>>>>
> >>>>>> The second capture that wants to stream should iterate the topology
> >>>>>> downwards until
> >>>>>> reaching an entity that already belong to the stream path of the other
> >>>>>> streaming capture
> >>>>>> and tell the streamer it wants to read the frames this entity
> >>>>>> produces.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Dafna
> >>>>>>
> >>>>>>> Thanks,
> >>>>>>> Dafna
> >>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'm just curious if you are aware of this series? It would
> >>>>>>>>>>>> replace the
> >>>>>>>>>>>> need for 1/3 and 2/3 of this series right?
> >>>>>>>>>>>
> >>>>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
> >>>>>>>>>>> version
> >>>>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
> >>>>>>>>>>> needed to
> >>>>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
> >>>>>>>>>>> again.
> >>>>>>>>>>
> >>>>>>>>>> So the question really is, how do we best make use of the two
> >>>>>>>>>> current
> >>>>>>>>>> series, to achieve our goal of supporting multiple streams.
> >>>>>>>>>>
> >>>>>>>>>> Having not parsed Dafna's series yet, do we need to combine
> >>>>>>>>>> elements of
> >>>>>>>>>> both ? Or should we work towards starting with this series and get
> >>>>>>>>>> dafna's patches built on top ?
> >>>>>>>>>>
> >>>>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> >>>>>>>>>>
> >>>>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
> >>>>>>>>>> multiple stream operation from /this/ series and her development
> >>>>>>>>>> branch
> >>>>>>>>>> on libcamera).
> >>>>>>>>>
> >>>>>>>>> Dafna's patch seems still under discussion, but I don't want to
> >>>>>>>>> block progress in Vimc either.
> >>>>>>>>>
> >>>>>>>>> So I was wondering if we can move forward with Vimc support for
> >>>>>>>>> multistreaming,
> >>>>>>>>> without considering Dafna's patchset, and we can do the clean up
> >>>>>>>>> later once we solve that.
> >>>>>>>>>
> >>>>>>>>> What do you think?
> >>>>>>>>
> >>>>>>>> I agree with supporting multiple streams with VIMC with this patchset,
> >>>>>>>> and then we can refactor the counters for s_stream in VIMC later (over
> >>>>>>>> this series) if dafna includes them in subsequent version of her
> >>>>>>>> patchset.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I also think that adding support in the code will take much longer and
> >>>>>>> should not
> >>>>>>> stop us from supporting vimc independently.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Dafna
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Helen
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>> 1.
> >>>>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Changes since v1:
> >>>>>>>>>>>>>       - All three patches rebased on latest media-tree.
> >>>>>>>>>>>>>       Patch 3:
> >>>>>>>>>>>>>       - Search for an entity with a non-NULL pipe instead of
> >>>>>>>>>>>>> searching
> >>>>>>>>>>>>>         for sensor. This terminates the search at output itself.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Kaaira Gupta (3):
> >>>>>>>>>>>>>      media: vimc: Add usage count to subdevices
> >>>>>>>>>>>>>      media: vimc: Serialize vimc_streamer_s_stream()
> >>>>>>>>>>>>>      media: vimc: Join pipeline if one already exists
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-capture.c    | 35
> >>>>>>>>>>>>> ++++++++++++++++++-
> >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> >>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> >>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 23
> >>>>>>>>>>>>> +++++++-----
> >>>>>>>>>>>>>     5 files changed, 73 insertions(+), 10 deletions(-)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -- 
> >>>>>>>>>>>>> 2.17.1
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> -- 
> >>>>>>>>>>>> Regards,
> >>>>>>>>>>>> Niklas Söderlund
> >>>>>>>>>>
> >>>>>
> >>>
> 
> -- 
> Regards
> --
> Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Kaaira Gupta]

On Wed, Aug 05, 2020 at 12:19:52AM +0530, Kaaira Gupta wrote:
> Hi
> 
> On Tue, Aug 04, 2020 at 11:24:56AM +0100, Kieran Bingham wrote:
> > Hi Kaaira,
> > 
> > On 31/07/2020 18:22, Kaaira Gupta wrote:
> > > Hi everyone,
> > > 
> > > On Wed, Jul 29, 2020 at 05:24:25PM +0200, Dafna Hirschfeld wrote:
> > >>
> > >>
> > >> On 29.07.20 15:27, Kieran Bingham wrote:
> > >>> Hi Dafna, Kaaira,
> > >>>
> > >>> On 29/07/2020 14:16, Dafna Hirschfeld wrote:
> > >>>>
> > >>>>
> > >>>> On 29.07.20 15:05, Kieran Bingham wrote:
> > >>>>> Hi Dafna,
> > >>>>>
> > >>>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
> > >>>>>>> Hi
> > >>>>>>>
> > >>>>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
> > >>>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
> > >>>>>>>>> Hi,
> > >>>>>>>>>
> > >>>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
> > >>>>>>>>>> Hi all,
> > >>>>>>>>>>
> > >>>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
> > >>>>>>>>>> list.
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
> > >>>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
> > >>>>>>>>>>> Hi,
> > >>>>>>>>>>>
> > >>>>>>>>>>>> Hi Kaaira,
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Thanks for your work.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Thanks for yours :D
> > >>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
> > >>>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
> > >>>>>>>>>>>>> allowing
> > >>>>>>>>>>>>> multiple streams in VIMC.
> > >>>>>>>>>>>>> The original series can be found here:
> > >>>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> This series adds support for two (or more) capture devices to be
> > >>>>>>>>>>>>> connected to the same sensors and run simultaneously. Each
> > >>>>>>>>>>>>> capture device
> > >>>>>>>>>>>>> can be started and stopped independent of each other.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
> > >>>>>>>>>>>>> two
> > >>>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
> > >>>>>>>>>>>>> allows for
> > >>>>>>>>>>>>> two capture devices to be part of the same pipeline and thus
> > >>>>>>>>>>>>> allows for
> > >>>>>>>>>>>>> simultaneously use.
> > >>>>>>>
> > >>>>>>> I wonder if these two patches are enough, since each vimc entity also
> > >>>>>>> have
> > >>>>>>> a 'process_frame' callback, but only one allocated frame. That means
> > >>>>>>> that the 'process_frame' can be called concurrently by two different
> > >>>>>>> streams
> > >>>>>>> on the same frame and cause corruption.
> > >>>>>>>
> > >>>>>>
> > >>>>>> I think we should somehow change the vimc-stream.c code so that we have
> > >>>>>> only
> > >>>>>> one stream process per pipe. So if one capture is already streaming,
> > >>>>>> then the new
> > >>>>>> capture that wants to stream uses the same thread so we don't have two
> > >>>>>> threads
> > >>>>>> both calling 'process_frame'.
> > >>>>>
> > >>>>>
> > >>>>> Yes, I think it looks and sounds like there are two threads running when
> > >>>>> there are two streams.
> > >>>>>
> > >>>>> so in effect, although they 'share a pipe', aren't they in effect just
> > >>>>> sending two separate buffers through their stream-path?
> > >>>>>
> > >>>>> If that's the case, then I don't think there's any frame corruption,
> > >>>>> because they would both have grabbed their own frame separately.
> > >>>>
> > >>>> But each entity allocates just one buffer. So the same buffer is used for
> > >>>> both stream.
> > >>>
> > >>> Aha, ok, I hadn't realised there was only a single buffer available in
> > >>> the pipeline for each entity. Indeed there is a risk of corruption in
> > >>> that case.
> > >>>
> > >>>> What for example can happen is that the debayer of one stream can read the
> > >>>> sensor's buffer while the sensor itself writes to the buffer for the other
> > >>>> stream.
> > >>>
> > >>>
> > >>> So, In that case, we have currently got a scenario where each 'stream'
> > >>> really is operating it's own pipe (even though all components are reused).
> > >>>
> > >>> Two questions:
> > >>>
> > >>> Is this acceptable, and we should just use a mutex to ensure the buffers
> > >>> are not corrupted, but essentially each stream is a separate temporal
> > >>> capture?
> > >>>
> > >>>
> > >>> Or B:
> > >>>
> > >>> Should we refactor to make sure that there is a single thread, and the
> > >>> code which calls process_frame on each entity should become aware of the
> > >>> potential for multiple paths at the point of the sensor.
> > >>>
> > >>>
> > >>> I suspect option B is really the 'right' path to take, but it is more
> > >>> complicated of course.
> > >>
> > >> I also think option B is preferable.
> > >>
> > >> Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
> > >> The stream thread can do a BFS scan from the sensor up to the captures
> > >> and call the 'process_frame' for each entity if 'is_streaming == true'.
> > >> When a new capture wants to stream it sets 'is_streaming = true'
> > >> on the entities on his streaming path.
> > > 
> > > It is s_stream(enable) that initialises a streaming pipeline, ie the one with
> > > those components of the pipeline which are in stream path and then runs a
> > > thread which calls process_frame on each and passes the frame to the
> > > next entity in streaming pipeline. So currently, one thread is for one
> > > "streaming pipeline". So there are two options I can think of if a
> > > single thread is required,
> > > 
> > > 1. Not creating a streaming pipeline, rather create a graph(?) which
> > > connects both say Raw capture 1 and debayer B to sensor B if two streams
> > > are asked for, and only one of them if one stream is asked..that will
> > > not be a property of streamer, so I am not sure where it should be kept.
> > > Then I could move creating a thread out of s_stream. Creating the thread
> > > should wait for entire pipeline to be created, ie s_stream(enable) to
> > > must be called by both the captures, and a graph made of all pipeline
> > > components before thread initialisation starts. I am not sure how this
> > > should be implemented.
> > 
> > The graph already exists, and can be walked through the media controller
> > right?
> 
> Yes, but actually, the current implementation of the thread does not
> walk through the entire pipeline, rather ved_pipeline, which is the
> portions of the pipeline which come in the streaming path of a stream
> which calls the thread. This also answers your doubt why the pipeline is
> decreamenting (which you have asked later in the mail). ved_pipeline is
> an array of the entities in the path of the stream starting from
> capture. Hence my suggestion was if I should make a data structure (a 
> graph) which holds all the entities in one or more stream path.
> 
> > 
> > 
> > > 2. Another option is to check if a stream already exists (by creating it
> > > a property of vimc to keep a track of no. of streams maybe?), if it is
> > > already present I could take the previous output of sensor (but
> > > then it will have to be stored, so i don't think this is a nice idea),
> > > and use it further (but thread will be different in this case).
> > 
> > I don't think I understand this one...
> 
> I meant that to start a thread for the driver, rather than each stream,
> maybe instead of creating a graph (as in the first option), maybe the
> druver could know the number of steams alsready running, and hence when
> s_stream is called for another, it knows how many and what type of
> streams are running, which can then /know/ which entity's output of
> process_frame(previous stream) to give on to the current stream. But I
> wasn't sure about the implementation of the /knwing/part without
> hardcoding it to take output from the sensor. But since that is the only
> topology (hardcoded)we have currently, maybe that can be a solution,
> hence I asked.

The idea is actually almost same as the pseudo code/implementation you wrote
below :D

> 
> > 
> > 
> > > What can be a better design for VIMC to have a single thread if two
> > > streams are asked (apart/of the options I mentioned)?
> > 
> > How about adding a count in s_stream so that the thread only gets
> > started when the use count is > 0, and stopped when the usage < 1.
> > 
> > That handles making sure that only one thread is available.
> > 
> > All calls into s_stream() will need to take a lock/mutex to protect /
> > prevent any action from occurring while the thread is performing a
> > process of the pipeline.
> > 
> > 
> > static int vimc_streamer_thread(void *data)
> > {
> > 	struct vimc_stream *stream = data;
> > 	u8 *frame = NULL;
> > 	int i;
> > 
> > 	set_freezable();
> > 
> > 	for (;;) {
> > 		try_to_freeze();
> > 		if (kthread_should_stop())
> > 			break;
> > 
> > +		/* take lock shared with s_stream */
> > 
> > 		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))
> > 				break;
> > 		}
> > 
> > +		/* Release lock/mutex shared with s_stream
> > 
> > 		//wait for 60hz
> > 		set_current_state(TASK_UNINTERRUPTIBLE);
> > 		schedule_timeout(HZ / 60);
> > 	}
> > 
> > 	return 0;
> > }
> > 
> > 
> > 
> > And you'll need to make the code which processes the pipeline aware of
> > the fact that there may be two pipelines to fulfil:
> > 
> > Pseudo patch/code:
> > 
> > static int vimc_streamer_thread(void *data)
> > {
> > -	struct vimc_stream *stream = data;
> > +	/* Something which knows about the whole device */
> > +	struct xxxxx *yyy = data;
> > 
> > +	u8 *raw;
> > 	u8 *frame = NULL;
> > 	int i;
> > 
> > 	set_freezable();
> > 
> > 	for (;;) {
> > 		try_to_freeze();
> > 		if (kthread_should_stop())
> > 			break;
> > 
> > 		/* take lock shared with s_stream */
> > 
> > +		/* Process the sensor first */
> > +		raw = stream->ved_pipeline[sensor]->process_frame(..);
> > +		error check;
> > 
> > +		/* (If connected) Process stream 1 */
> > +		if (raw)
> > +		frame = stream->ved_pipeline[raw]->process_frame();
> > +		error check;
> > 
> > +		/* If connected process the rest of the pipe */
> > +		for (i = after sensor; end_entity; i++) {
> > 			frame = stream->ved_pipeline[i]->process_frame(
> > 					stream->ved_pipeline[i], frame);
> > 			if (!frame || IS_ERR(frame))
> > 				break;
> > 		}
> > 
> > 		/* Release lock/mutex shared with s_stream
> > 
> > 		//wait for 60hz
> > 		set_current_state(TASK_UNINTERRUPTIBLE);
> > 		schedule_timeout(HZ / 60);
> > 	}
> > 
> > 	return 0;
> > }
> > 
> > 
> > 
> > I may have missed something as the original loop was decrementing and
> > going backwards through the entities in stream->ved_pipeline.
> > 
> > I guess splitting that all out so instead it starts at the sensor, and
> > just walks the graph (handling any running/connected fork to two
> > entities appropriately) in a neater way would be another option rather
> > than hardcoding it, but either way the thread needs to operate at the
> > device level rather than the stream level.
> > 
> > 
> > 
> > > Thanks
> > > Kaaira
> > > 
> > >>
> > >> Thanks,
> > >> Dafna
> > >>
> > >>
> > >>>
> > >>> --
> > >>> Kieran
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>> Thanks,
> > >>>> Dafna
> > >>>>
> > >>>>>
> > >>>>>
> > >>>>> I don't think that's a good example of the hardware though, as that
> > >>>>> doesn't reflect what 'should' happen where the TPG runs once to generate
> > >>>>> a frame at the sensor, which is then read by both the debayer entity and
> > >>>>> the RAW capture device when there are two streams...
> > >>>>>
> > >>>>>
> > >>>>> So I suspect trying to move to a single thread is desirable, but that
> > >>>>> might be a fair bit of work also.
> > >>>>>
> > >>>>> -- 
> > >>>>> Kieran
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>> The second capture that wants to stream should iterate the topology
> > >>>>>> downwards until
> > >>>>>> reaching an entity that already belong to the stream path of the other
> > >>>>>> streaming capture
> > >>>>>> and tell the streamer it wants to read the frames this entity
> > >>>>>> produces.
> > >>>>>>
> > >>>>>> Thanks,
> > >>>>>> Dafna
> > >>>>>>
> > >>>>>>> Thanks,
> > >>>>>>> Dafna
> > >>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I'm just curious if you are aware of this series? It would
> > >>>>>>>>>>>> replace the
> > >>>>>>>>>>>> need for 1/3 and 2/3 of this series right?
> > >>>>>>>>>>>
> > >>>>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
> > >>>>>>>>>>> version
> > >>>>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
> > >>>>>>>>>>> needed to
> > >>>>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
> > >>>>>>>>>>> again.
> > >>>>>>>>>>
> > >>>>>>>>>> So the question really is, how do we best make use of the two
> > >>>>>>>>>> current
> > >>>>>>>>>> series, to achieve our goal of supporting multiple streams.
> > >>>>>>>>>>
> > >>>>>>>>>> Having not parsed Dafna's series yet, do we need to combine
> > >>>>>>>>>> elements of
> > >>>>>>>>>> both ? Or should we work towards starting with this series and get
> > >>>>>>>>>> dafna's patches built on top ?
> > >>>>>>>>>>
> > >>>>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
> > >>>>>>>>>>
> > >>>>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
> > >>>>>>>>>> multiple stream operation from /this/ series and her development
> > >>>>>>>>>> branch
> > >>>>>>>>>> on libcamera).
> > >>>>>>>>>
> > >>>>>>>>> Dafna's patch seems still under discussion, but I don't want to
> > >>>>>>>>> block progress in Vimc either.
> > >>>>>>>>>
> > >>>>>>>>> So I was wondering if we can move forward with Vimc support for
> > >>>>>>>>> multistreaming,
> > >>>>>>>>> without considering Dafna's patchset, and we can do the clean up
> > >>>>>>>>> later once we solve that.
> > >>>>>>>>>
> > >>>>>>>>> What do you think?
> > >>>>>>>>
> > >>>>>>>> I agree with supporting multiple streams with VIMC with this patchset,
> > >>>>>>>> and then we can refactor the counters for s_stream in VIMC later (over
> > >>>>>>>> this series) if dafna includes them in subsequent version of her
> > >>>>>>>> patchset.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> I also think that adding support in the code will take much longer and
> > >>>>>>> should not
> > >>>>>>> stop us from supporting vimc independently.
> > >>>>>>>
> > >>>>>>> Thanks,
> > >>>>>>> Dafna
> > >>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> Regards,
> > >>>>>>>>> Helen
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>>> 1.
> > >>>>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Changes since v1:
> > >>>>>>>>>>>>>       - All three patches rebased on latest media-tree.
> > >>>>>>>>>>>>>       Patch 3:
> > >>>>>>>>>>>>>       - Search for an entity with a non-NULL pipe instead of
> > >>>>>>>>>>>>> searching
> > >>>>>>>>>>>>>         for sensor. This terminates the search at output itself.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Kaaira Gupta (3):
> > >>>>>>>>>>>>>      media: vimc: Add usage count to subdevices
> > >>>>>>>>>>>>>      media: vimc: Serialize vimc_streamer_s_stream()
> > >>>>>>>>>>>>>      media: vimc: Join pipeline if one already exists
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-capture.c    | 35
> > >>>>>>>>>>>>> ++++++++++++++++++-
> > >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
> > >>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
> > >>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
> > >>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 23
> > >>>>>>>>>>>>> +++++++-----
> > >>>>>>>>>>>>>     5 files changed, 73 insertions(+), 10 deletions(-)
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> -- 
> > >>>>>>>>>>>>> 2.17.1
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> -- 
> > >>>>>>>>>>>> Regards,
> > >>>>>>>>>>>> Niklas Söderlund
> > >>>>>>>>>>
> > >>>>>
> > >>>
> > 
> > -- 
> > Regards
> > --
> > Kieran

Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Helen Koike]

Hi,

On 7/29/20 12:24 PM, Dafna Hirschfeld wrote:
> 
> 
> On 29.07.20 15:27, Kieran Bingham wrote:
>> Hi Dafna, Kaaira,
>>
>> On 29/07/2020 14:16, Dafna Hirschfeld wrote:
>>>
>>>
>>> On 29.07.20 15:05, Kieran Bingham wrote:
>>>> Hi Dafna,
>>>>
>>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote:
>>>>>
>>>>>
>>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote:
>>>>>> Hi
>>>>>>
>>>>>> On 28.07.20 13:39, Kaaira Gupta wrote:
>>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc
>>>>>>>>> list.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote:
>>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>> Hi Kaaira,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your work.
>>>>>>>>>>
>>>>>>>>>> Thanks for yours :D
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote:
>>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for
>>>>>>>>>>>> allowing
>>>>>>>>>>>> multiple streams in VIMC.
>>>>>>>>>>>> The original series can be found here:
>>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/
>>>>>>>>>>>>
>>>>>>>>>>>> This series adds support for two (or more) capture devices to be
>>>>>>>>>>>> connected to the same sensors and run simultaneously. Each
>>>>>>>>>>>> capture device
>>>>>>>>>>>> can be started and stopped independent of each other.
>>>>>>>>>>>>
>>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once
>>>>>>>>>>>> two
>>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3
>>>>>>>>>>>> allows for
>>>>>>>>>>>> two capture devices to be part of the same pipeline and thus
>>>>>>>>>>>> allows for
>>>>>>>>>>>> simultaneously use.
>>>>>>
>>>>>> I wonder if these two patches are enough, since each vimc entity also
>>>>>> have
>>>>>> a 'process_frame' callback, but only one allocated frame. That means
>>>>>> that the 'process_frame' can be called concurrently by two different
>>>>>> streams
>>>>>> on the same frame and cause corruption.
>>>>>>
>>>>>
>>>>> I think we should somehow change the vimc-stream.c code so that we have
>>>>> only
>>>>> one stream process per pipe. So if one capture is already streaming,
>>>>> then the new
>>>>> capture that wants to stream uses the same thread so we don't have two
>>>>> threads
>>>>> both calling 'process_frame'.
>>>>
>>>>
>>>> Yes, I think it looks and sounds like there are two threads running when
>>>> there are two streams.
>>>>
>>>> so in effect, although they 'share a pipe', aren't they in effect just
>>>> sending two separate buffers through their stream-path?
>>>>
>>>> If that's the case, then I don't think there's any frame corruption,
>>>> because they would both have grabbed their own frame separately.
>>>
>>> But each entity allocates just one buffer. So the same buffer is used for
>>> both stream.
>>
>> Aha, ok, I hadn't realised there was only a single buffer available in
>> the pipeline for each entity. Indeed there is a risk of corruption in
>> that case.
>>
>>> What for example can happen is that the debayer of one stream can read the
>>> sensor's buffer while the sensor itself writes to the buffer for the other
>>> stream.
>>
>>
>> So, In that case, we have currently got a scenario where each 'stream'
>> really is operating it's own pipe (even though all components are reused).
>>
>> Two questions:
>>
>> Is this acceptable, and we should just use a mutex to ensure the buffers
>> are not corrupted, but essentially each stream is a separate temporal
>> capture?
>>
>>
>> Or B:
>>
>> Should we refactor to make sure that there is a single thread, and the
>> code which calls process_frame on each entity should become aware of the
>> potential for multiple paths at the point of the sensor.
>>
>>
>> I suspect option B is really the 'right' path to take, but it is more
>> complicated of course.
> 
> I also think option B is preferable.

With this options we would force both stream to the
same frame rate, which I guess it make sense, since the sensor is producing frames
in a given pixel rate, the rest of the pipeline follows.

> 
> Maybe we can add a bool field 'is_streaming' to struct 'vimc_ent_device'
> The stream thread can do a BFS scan from the sensor up to the captures
> and call the 'process_frame' for each entity if 'is_streaming == true'.
> When a new capture wants to stream it sets 'is_streaming = true'
> on the entities on his streaming path.

I agree, we can have a BFS scan to build the array stream->ved_pipeline[]
in vimc_streamer_pipeline_init() with all the entities in the connected graph.

Or, if it is easier to implement, it doesn't need to be a BFS search, it could be one
pipe after the other from a join point, E.g.:

      /->e3->e4
e1->e2
      \->e5->e6

then ved_pipeline[] = [e1, e2, e3, e4, e5, e6]
Or ved_pipeline[] = [e1, e2, e5, e6, e3, e4]

Anyway would work.

Then you can check use_count (from patch 1/3)  to decide to call ".process_frame()" or not
on that entity.

So, if you start the stream on e4 only, you would have:
e1 - use_count = 1
e2 - use_count = 1
e3 - use_count = 1
e4 - use_count = 1
e5 - use_count = 0
e6 - use_count = 0

And if you start the stream on e6 later, all "use_count"s would be greater then 0,
so you just call .process_frame() when "use_count" > 0

You'll probably need to add a ".get_frame()" callback for each entity, and "process_frame()"
can call the "get_frame()" from the entity connected in its sink pad, so you don't need to
pass one frame from one entity to the next inside the thread as a parameter for .process_frame().

So I suggest:

- Refactor .process_frame to call .get_frame() from it's sink pad, instead of receiving the
frame as parameter (we are assuming we won't have an entity with more then one enabled sink pad).
- Update patch 1/3 to put the use_count on struct vimc_ent_device
- Update vimc_streamer_thread() to only call .process_frame() when use_count > 0
- Refactor vimc_streamer_pipeline_init() to build a proper ved_pipeline[] array with all the
entities in a pipe


What do you think?

Regards,
Helen

> 
> Thanks,
> Dafna
> 
> 
>>
>> -- 
>> Kieran
>>
>>
>>
>>
>>> Thanks,
>>> Dafna
>>>
>>>>
>>>>
>>>> I don't think that's a good example of the hardware though, as that
>>>> doesn't reflect what 'should' happen where the TPG runs once to generate
>>>> a frame at the sensor, which is then read by both the debayer entity and
>>>> the RAW capture device when there are two streams...
>>>>
>>>>
>>>> So I suspect trying to move to a single thread is desirable, but that
>>>> might be a fair bit of work also.
>>>>
>>>> -- 
>>>> Kieran
>>>>
>>>>
>>>>
>>>>> The second capture that wants to stream should iterate the topology
>>>>> downwards until
>>>>> reaching an entity that already belong to the stream path of the other
>>>>> streaming capture
>>>>> and tell the streamer it wants to read the frames this entity
>>>>> produces.
>>>>>
>>>>> Thanks,
>>>>> Dafna
>>>>>
>>>>>> Thanks,
>>>>>> Dafna
>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'm just curious if you are aware of this series? It would
>>>>>>>>>>> replace the
>>>>>>>>>>> need for 1/3 and 2/3 of this series right?
>>>>>>>>>>
>>>>>>>>>> v3 of this series replaces the need for 1/3, but not the current
>>>>>>>>>> version
>>>>>>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is
>>>>>>>>>> needed to
>>>>>>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant
>>>>>>>>>> again.
>>>>>>>>>
>>>>>>>>> So the question really is, how do we best make use of the two
>>>>>>>>> current
>>>>>>>>> series, to achieve our goal of supporting multiple streams.
>>>>>>>>>
>>>>>>>>> Having not parsed Dafna's series yet, do we need to combine
>>>>>>>>> elements of
>>>>>>>>> both ? Or should we work towards starting with this series and get
>>>>>>>>> dafna's patches built on top ?
>>>>>>>>>
>>>>>>>>> Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ?
>>>>>>>>>
>>>>>>>>> (It might be noteworthy to say that Kaaira has reported successful
>>>>>>>>> multiple stream operation from /this/ series and her development
>>>>>>>>> branch
>>>>>>>>> on libcamera).
>>>>>>>>
>>>>>>>> Dafna's patch seems still under discussion, but I don't want to
>>>>>>>> block progress in Vimc either.
>>>>>>>>
>>>>>>>> So I was wondering if we can move forward with Vimc support for
>>>>>>>> multistreaming,
>>>>>>>> without considering Dafna's patchset, and we can do the clean up
>>>>>>>> later once we solve that.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>
>>>>>>> I agree with supporting multiple streams with VIMC with this patchset,
>>>>>>> and then we can refactor the counters for s_stream in VIMC later (over
>>>>>>> this series) if dafna includes them in subsequent version of her
>>>>>>> patchset.
>>>>>>>
>>>>>>
>>>>>> I also think that adding support in the code will take much longer and
>>>>>> should not
>>>>>> stop us from supporting vimc independently.
>>>>>>
>>>>>> Thanks,
>>>>>> Dafna
>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Helen
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> 1.
>>>>>>>>>>> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschfeld@collabora.com/
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Changes since v1:
>>>>>>>>>>>>       - All three patches rebased on latest media-tree.
>>>>>>>>>>>>       Patch 3:
>>>>>>>>>>>>       - Search for an entity with a non-NULL pipe instead of
>>>>>>>>>>>> searching
>>>>>>>>>>>>         for sensor. This terminates the search at output itself.
>>>>>>>>>>>>
>>>>>>>>>>>> Kaaira Gupta (3):
>>>>>>>>>>>>      media: vimc: Add usage count to subdevices
>>>>>>>>>>>>      media: vimc: Serialize vimc_streamer_s_stream()
>>>>>>>>>>>>      media: vimc: Join pipeline if one already exists
>>>>>>>>>>>>
>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-capture.c    | 35
>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-debayer.c    |  8 +++++
>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-scaler.c |  8 +++++
>>>>>>>>>>>>     drivers/media/test-drivers/vimc/vimc-sensor.c |  9 ++++-
>>>>>>>>>>>>     .../media/test-drivers/vimc/vimc-streamer.c   | 23
>>>>>>>>>>>> +++++++-----
>>>>>>>>>>>>     5 files changed, 73 insertions(+), 10 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> 2.17.1
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> -- 
>>>>>>>>>>> Regards,
>>>>>>>>>>> Niklas Söderlund
>>>>>>>>>
>>>>
>>