* [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor @ 2020-07-24 12:02 Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 1/3] media: vimc: Add usage count to subdevices Kaaira Gupta ` (4 more replies) 0 siblings, 5 replies; 27+ messages in thread From: Kaaira Gupta @ 2020-07-24 12:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund Cc: 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/3] media: vimc: Add usage count to subdevices 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta @ 2020-07-24 12:02 ` Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 2/3] media: vimc: Serialize vimc_streamer_s_stream() Kaaira Gupta ` (3 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: Kaaira Gupta @ 2020-07-24 12:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund Cc: 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/3] media: vimc: Serialize vimc_streamer_s_stream() 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 1/3] media: vimc: Add usage count to subdevices Kaaira Gupta @ 2020-07-24 12:02 ` Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 3/3] media: vimc: Join pipeline if one already exists Kaaira Gupta ` (2 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: Kaaira Gupta @ 2020-07-24 12:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund Cc: 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/3] media: vimc: Join pipeline if one already exists 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 1/3] media: vimc: Add usage count to subdevices Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 2/3] media: vimc: Serialize vimc_streamer_s_stream() Kaaira Gupta @ 2020-07-24 12:02 ` Kaaira Gupta 2020-07-28 12:24 ` Dafna Hirschfeld 2020-07-24 12:15 ` [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Niklas Söderlund 2020-07-30 10:51 ` Laurent Pinchart 4 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-24 12:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund Cc: 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists 2020-07-24 12:02 ` [PATCH v2 3/3] media: vimc: Join pipeline if one already exists Kaaira Gupta @ 2020-07-28 12:24 ` Dafna Hirschfeld 2020-07-28 12:48 ` Kaaira Gupta 0 siblings, 1 reply; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-28 12:24 UTC (permalink / raw) To: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists 2020-07-28 12:24 ` Dafna Hirschfeld @ 2020-07-28 12:48 ` Kaaira Gupta 2020-07-29 10:58 ` Dafna Hirschfeld 0 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-28 12:48 UTC (permalink / raw) To: Dafna Hirschfeld Cc: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: Join pipeline if one already exists 2020-07-28 12:48 ` Kaaira Gupta @ 2020-07-29 10:58 ` Dafna Hirschfeld 0 siblings, 0 replies; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-29 10:58 UTC (permalink / raw) To: Kaaira Gupta Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta ` (2 preceding siblings ...) 2020-07-24 12:02 ` [PATCH v2 3/3] media: vimc: Join pipeline if one already exists Kaaira Gupta @ 2020-07-24 12:15 ` Niklas Söderlund 2020-07-24 12:21 ` Kaaira Gupta 2020-07-30 10:51 ` Laurent Pinchart 4 siblings, 1 reply; 27+ messages in thread From: Niklas Söderlund @ 2020-07-24 12:15 UTC (permalink / raw) To: Kaaira Gupta Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-24 12:15 ` [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Niklas Söderlund @ 2020-07-24 12:21 ` Kaaira Gupta 2020-07-27 14:31 ` Kieran Bingham 0 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-24 12:21 UTC (permalink / raw) To: Niklas Söderlund Cc: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-24 12:21 ` Kaaira Gupta @ 2020-07-27 14:31 ` Kieran Bingham 2020-07-27 17:54 ` Helen Koike 0 siblings, 1 reply; 27+ messages in thread From: Kieran Bingham @ 2020-07-27 14:31 UTC (permalink / raw) To: Kaaira Gupta, Niklas Söderlund Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Dafna Hirschfeld 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-27 14:31 ` Kieran Bingham @ 2020-07-27 17:54 ` Helen Koike 2020-07-28 11:39 ` Kaaira Gupta 0 siblings, 1 reply; 27+ messages in thread From: Helen Koike @ 2020-07-27 17:54 UTC (permalink / raw) To: kieran.bingham, Kaaira Gupta, Niklas Söderlund Cc: Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Dafna Hirschfeld 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-27 17:54 ` Helen Koike @ 2020-07-28 11:39 ` Kaaira Gupta 2020-07-28 12:07 ` Dafna Hirschfeld 0 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-28 11:39 UTC (permalink / raw) To: Helen Koike Cc: kieran.bingham, Kaaira Gupta, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Dafna Hirschfeld 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 > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-28 11:39 ` Kaaira Gupta @ 2020-07-28 12:07 ` Dafna Hirschfeld 2020-07-28 14:00 ` Dafna Hirschfeld 0 siblings, 1 reply; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-28 12:07 UTC (permalink / raw) To: Kaaira Gupta, Helen Koike Cc: kieran.bingham, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel 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 >>> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-28 12:07 ` Dafna Hirschfeld @ 2020-07-28 14:00 ` Dafna Hirschfeld 2020-07-28 14:26 ` Kaaira Gupta 2020-07-29 13:05 ` Kieran Bingham 0 siblings, 2 replies; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-28 14:00 UTC (permalink / raw) To: Kaaira Gupta, Helen Koike Cc: kieran.bingham, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 >>>> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-28 14:00 ` Dafna Hirschfeld @ 2020-07-28 14:26 ` Kaaira Gupta 2020-07-29 13:05 ` Kieran Bingham 1 sibling, 0 replies; 27+ messages in thread From: Kaaira Gupta @ 2020-07-28 14:26 UTC (permalink / raw) To: Dafna Hirschfeld Cc: Kaaira Gupta, Helen Koike, kieran.bingham, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-28 14:00 ` Dafna Hirschfeld 2020-07-28 14:26 ` Kaaira Gupta @ 2020-07-29 13:05 ` Kieran Bingham 2020-07-29 13:16 ` Dafna Hirschfeld 1 sibling, 1 reply; 27+ messages in thread From: Kieran Bingham @ 2020-07-29 13:05 UTC (permalink / raw) To: Dafna Hirschfeld, Kaaira Gupta, Helen Koike Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-29 13:05 ` Kieran Bingham @ 2020-07-29 13:16 ` Dafna Hirschfeld 2020-07-29 13:27 ` Kieran Bingham 0 siblings, 1 reply; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-29 13:16 UTC (permalink / raw) To: kieran.bingham, Kaaira Gupta, Helen Koike Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 >>>>>> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-29 13:16 ` Dafna Hirschfeld @ 2020-07-29 13:27 ` Kieran Bingham 2020-07-29 15:24 ` Dafna Hirschfeld 0 siblings, 1 reply; 27+ messages in thread From: Kieran Bingham @ 2020-07-29 13:27 UTC (permalink / raw) To: Dafna Hirschfeld, Kaaira Gupta, Helen Koike Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-29 13:27 ` Kieran Bingham @ 2020-07-29 15:24 ` Dafna Hirschfeld 2020-07-31 17:22 ` Kaaira Gupta 2020-08-05 15:18 ` Helen Koike 0 siblings, 2 replies; 27+ messages in thread From: Dafna Hirschfeld @ 2020-07-29 15:24 UTC (permalink / raw) To: kieran.bingham, Kaaira Gupta, Helen Koike Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 >>>>>>>> >>> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-29 15:24 ` Dafna Hirschfeld @ 2020-07-31 17:22 ` Kaaira Gupta 2020-08-04 10:24 ` Kieran Bingham 2020-08-05 15:18 ` Helen Koike 1 sibling, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-31 17:22 UTC (permalink / raw) To: Dafna Hirschfeld, kieran.bingham, Laurent Pinchart, Hans Verkuil, Helen Koike Cc: Kaaira Gupta, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Ezequiel Garcia 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 > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-31 17:22 ` Kaaira Gupta @ 2020-08-04 10:24 ` Kieran Bingham 2020-08-04 18:49 ` Kaaira Gupta 0 siblings, 1 reply; 27+ messages in thread From: Kieran Bingham @ 2020-08-04 10:24 UTC (permalink / raw) To: Kaaira Gupta, Dafna Hirschfeld, Laurent Pinchart, Hans Verkuil, Helen Koike Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Ezequiel Garcia 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-08-04 10:24 ` Kieran Bingham @ 2020-08-04 18:49 ` Kaaira Gupta 2020-08-04 18:52 ` Kaaira Gupta 0 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-08-04 18:49 UTC (permalink / raw) To: Kieran Bingham Cc: Kaaira Gupta, Dafna Hirschfeld, Laurent Pinchart, Hans Verkuil, Helen Koike, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Ezequiel Garcia 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-08-04 18:49 ` Kaaira Gupta @ 2020-08-04 18:52 ` Kaaira Gupta 0 siblings, 0 replies; 27+ messages in thread From: Kaaira Gupta @ 2020-08-04 18:52 UTC (permalink / raw) To: Kieran Bingham Cc: Dafna Hirschfeld, Laurent Pinchart, Hans Verkuil, Helen Koike, Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Ezequiel Garcia 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-29 15:24 ` Dafna Hirschfeld 2020-07-31 17:22 ` Kaaira Gupta @ 2020-08-05 15:18 ` Helen Koike 1 sibling, 0 replies; 27+ messages in thread From: Helen Koike @ 2020-08-05 15:18 UTC (permalink / raw) To: Dafna Hirschfeld, kieran.bingham, Kaaira Gupta Cc: Niklas Söderlund, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil, Laurent Pinchart, Ezequiel Garcia 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 >>>>>>>>> >>>> >> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta ` (3 preceding siblings ...) 2020-07-24 12:15 ` [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Niklas Söderlund @ 2020-07-30 10:51 ` Laurent Pinchart 2020-07-30 18:09 ` Kaaira Gupta 4 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2020-07-30 10:51 UTC (permalink / raw) To: Kaaira Gupta Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-30 10:51 ` Laurent Pinchart @ 2020-07-30 18:09 ` Kaaira Gupta 2020-07-30 22:21 ` Laurent Pinchart 0 siblings, 1 reply; 27+ messages in thread From: Kaaira Gupta @ 2020-07-30 18:09 UTC (permalink / raw) To: Laurent Pinchart Cc: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor 2020-07-30 18:09 ` Kaaira Gupta @ 2020-07-30 22:21 ` Laurent Pinchart 0 siblings, 0 replies; 27+ messages in thread From: Laurent Pinchart @ 2020-07-30 22:21 UTC (permalink / raw) To: Kaaira Gupta Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-08-05 17:01 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-24 12:02 [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 1/3] media: vimc: Add usage count to subdevices Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 2/3] media: vimc: Serialize vimc_streamer_s_stream() Kaaira Gupta 2020-07-24 12:02 ` [PATCH v2 3/3] media: vimc: Join pipeline if one already exists Kaaira Gupta 2020-07-28 12:24 ` Dafna Hirschfeld 2020-07-28 12:48 ` Kaaira Gupta 2020-07-29 10:58 ` Dafna Hirschfeld 2020-07-24 12:15 ` [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor Niklas Söderlund 2020-07-24 12:21 ` Kaaira Gupta 2020-07-27 14:31 ` Kieran Bingham 2020-07-27 17:54 ` Helen Koike 2020-07-28 11:39 ` Kaaira Gupta 2020-07-28 12:07 ` Dafna Hirschfeld 2020-07-28 14:00 ` Dafna Hirschfeld 2020-07-28 14:26 ` Kaaira Gupta 2020-07-29 13:05 ` Kieran Bingham 2020-07-29 13:16 ` Dafna Hirschfeld 2020-07-29 13:27 ` Kieran Bingham 2020-07-29 15:24 ` Dafna Hirschfeld 2020-07-31 17:22 ` Kaaira Gupta 2020-08-04 10:24 ` Kieran Bingham 2020-08-04 18:49 ` Kaaira Gupta 2020-08-04 18:52 ` Kaaira Gupta 2020-08-05 15:18 ` Helen Koike 2020-07-30 10:51 ` Laurent Pinchart 2020-07-30 18:09 ` Kaaira Gupta 2020-07-30 22:21 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).