Hi,
Media drivers need to iterate through the pipeline and call .s_stream()
callbacks in the subdevices.
Instead of repeating code, add helpers for this.
These helpers will go walk through the pipeline only visiting entities
that participates in the stream, i.e. it follows links from sink to source
(and not the opposite).
Which means that in a topology like this https://bit.ly/3b2MxjI
calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
.s_stream(true) for rkisp1_resizer_selfpath.
stream_count variable was added in v4l2_subdevice to handle nested calls
to the helpers.
This is useful when the driver allows streaming from more then one
capture device sharing subdevices.
This patch came from the error I was facing when multistreaming from
rkisp1 driver, where stoping one capture would call s_stream(false) in
the pipeline, causing a stall in the second capture device.
Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
be required with this patchset.
This patchset was tested on rkisp1 and vimc drivers.
Other cleanup might be possible (but I won't add in this patchset as I
don't have the hw to test):
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97
Changes in V2:
====================
The first version was calling the s_stream() callbacks from sensor to
capture.
This was generating errors in the Scarlet Chromebook, when the sensor
was being enabled before the ISP.
It make sense to enable subdevices from capture to sensor instead (which
is what most drivers do already).
This v2 drops the changes from mc-entity.c, and re-implement helpers in
v4l2-common.c
Overview of patches:
====================
Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
adding stream_count in the subdevice struct.
Patch 2/3 cleanup rkisp1 driver to use the helpers.
Patch 3/3 cleanup vimc driver to use the helpers.
Helen Koike (3):
media: v4l2-common: add helper functions to call s_stream() callbacks
media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
helpers
media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers
drivers/media/platform/vimc/vimc-capture.c | 28 +++--
drivers/media/platform/vimc/vimc-streamer.c | 49 +-------
drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++
drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +-----------
include/media/v4l2-common.h | 28 +++++
include/media/v4l2-subdev.h | 2 +
6 files changed, 173 insertions(+), 127 deletions(-)
--
2.26.0
Use v4l2_pipeline_stream_{enable,disable} to call .s_stream() subdevice
callbacks through the pipeline.
Tested streaming works with:
media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/1920x1440]'
v4l2-ctl -d /dev/video2 -v width=1200,height=450
v4l2-ctl -d /dev/video0 -v pixelformat=BA81
v4l2-ctl -d /dev/video1 -v pixelformat=BA81
v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 --stream-to=/tmp/test.raw
Signed-off-by: Helen Koike <[email protected]>
---
Changes in v2:
- rebase on top of new helpers prototypes
drivers/media/platform/vimc/vimc-capture.c | 28 +++++++-----
drivers/media/platform/vimc/vimc-streamer.c | 49 +++------------------
2 files changed, 23 insertions(+), 54 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 23e740c1c5c00..2206bfdec1c32 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -233,21 +233,27 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
vcap->sequence = 0;
- /* Start the media pipeline */
ret = media_pipeline_start(entity, &vcap->stream.pipe);
- if (ret) {
- vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
- return ret;
- }
+ if (ret)
+ goto err_return_all_buffers;
+
+ ret = v4l2_pipeline_stream_enable(&vcap->vdev);
+ if (ret)
+ goto err_stop_media_pipe;
ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
- if (ret) {
- media_pipeline_stop(entity);
- vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
- return ret;
- }
+ if (ret)
+ goto err_stop_stream;
return 0;
+
+err_stop_stream:
+ v4l2_pipeline_stream_disable(&vcap->vdev);
+err_stop_media_pipe:
+ media_pipeline_stop(entity);
+err_return_all_buffers:
+ vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
+ return ret;
}
/*
@@ -260,6 +266,8 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 0);
+ v4l2_pipeline_stream_disable(&vcap->vdev);
+
/* Stop the media pipeline */
media_pipeline_stop(&vcap->vdev.entity);
diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
index 65feb3c596db5..c0085f4695c2f 100644
--- a/drivers/media/platform/vimc/vimc-streamer.c
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -36,33 +36,6 @@ static struct media_entity *vimc_get_source_entity(struct media_entity *ent)
return NULL;
}
-/**
- * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
- *
- * @stream: the pointer to the stream structure with the pipeline to be
- * disabled.
- *
- * Calls s_stream to disable the stream in each entity of the pipeline
- *
- */
-static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
-{
- struct vimc_ent_device *ved;
- struct v4l2_subdev *sd;
-
- while (stream->pipe_size) {
- stream->pipe_size--;
- ved = stream->ved_pipeline[stream->pipe_size];
- stream->ved_pipeline[stream->pipe_size] = NULL;
-
- if (!is_media_entity_v4l2_subdev(ved->ent))
- continue;
-
- sd = media_entity_to_v4l2_subdev(ved->ent);
- v4l2_subdev_call(sd, video, s_stream, 0);
- }
-}
-
/**
* vimc_streamer_pipeline_init - Initializes the stream structure
*
@@ -82,27 +55,15 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
struct media_entity *entity;
struct video_device *vdev;
struct v4l2_subdev *sd;
- int ret = 0;
stream->pipe_size = 0;
while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
if (!ved) {
- vimc_streamer_pipeline_terminate(stream);
+ stream->pipe_size = 0;
return -EINVAL;
}
stream->ved_pipeline[stream->pipe_size++] = ved;
- if (is_media_entity_v4l2_subdev(ved->ent)) {
- sd = media_entity_to_v4l2_subdev(ved->ent);
- ret = v4l2_subdev_call(sd, video, s_stream, 1);
- if (ret && ret != -ENOIOCTLCMD) {
- dev_err(ved->dev, "subdev_call error %s\n",
- ved->ent->name);
- vimc_streamer_pipeline_terminate(stream);
- return ret;
- }
- }
-
entity = vimc_get_source_entity(ved->ent);
/* Check if the end of the pipeline was reached */
if (!entity) {
@@ -111,7 +72,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
dev_err(ved->dev,
"first entity in the pipe '%s' is not a source\n",
ved->ent->name);
- vimc_streamer_pipeline_terminate(stream);
+ stream->pipe_size = 0;
return -EPIPE;
}
return 0;
@@ -129,7 +90,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
}
}
- vimc_streamer_pipeline_terminate(stream);
+ stream->pipe_size = 0;
return -EINVAL;
}
@@ -210,7 +171,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
if (IS_ERR(stream->kthread)) {
ret = PTR_ERR(stream->kthread);
dev_err(ved->dev, "kthread_run failed with %d\n", ret);
- vimc_streamer_pipeline_terminate(stream);
+ stream->pipe_size = 0;
stream->kthread = NULL;
return ret;
}
@@ -231,7 +192,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
stream->kthread = NULL;
- vimc_streamer_pipeline_terminate(stream);
+ stream->pipe_size = 0;
}
return 0;
--
2.26.0
Hi Helen,
On 2020-04-03 18:33:09 -0300, Helen Koike wrote:
> Hi,
>
> Media drivers need to iterate through the pipeline and call .s_stream()
> callbacks in the subdevices.
>
> Instead of repeating code, add helpers for this.
>
> These helpers will go walk through the pipeline only visiting entities
> that participates in the stream, i.e. it follows links from sink to source
> (and not the opposite).
>
> Which means that in a topology like this https://bit.ly/3b2MxjI
> calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
> .s_stream(true) for rkisp1_resizer_selfpath.
>
> stream_count variable was added in v4l2_subdevice to handle nested calls
> to the helpers.
> This is useful when the driver allows streaming from more then one
> capture device sharing subdevices.
>
> This patch came from the error I was facing when multistreaming from
> rkisp1 driver, where stoping one capture would call s_stream(false) in
> the pipeline, causing a stall in the second capture device.
>
> Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
> be required with this patchset.
>
> This patchset was tested on rkisp1 and vimc drivers.
I'm just curious, with this series applied can I stream simultaneously
on multiple video devises using vimc?
>
> Other cleanup might be possible (but I won't add in this patchset as I
> don't have the hw to test):
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97
>
> Changes in V2:
> ====================
> The first version was calling the s_stream() callbacks from sensor to
> capture.
>
> This was generating errors in the Scarlet Chromebook, when the sensor
> was being enabled before the ISP.
>
> It make sense to enable subdevices from capture to sensor instead (which
> is what most drivers do already).
>
> This v2 drops the changes from mc-entity.c, and re-implement helpers in
> v4l2-common.c
>
> Overview of patches:
> ====================
>
> Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
> adding stream_count in the subdevice struct.
>
> Patch 2/3 cleanup rkisp1 driver to use the helpers.
>
> Patch 3/3 cleanup vimc driver to use the helpers.
>
> Helen Koike (3):
> media: v4l2-common: add helper functions to call s_stream() callbacks
> media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
> helpers
> media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers
>
> drivers/media/platform/vimc/vimc-capture.c | 28 +++--
> drivers/media/platform/vimc/vimc-streamer.c | 49 +-------
> drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++
> drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +-----------
> include/media/v4l2-common.h | 28 +++++
> include/media/v4l2-subdev.h | 2 +
> 6 files changed, 173 insertions(+), 127 deletions(-)
>
> --
> 2.26.0
>
--
Regards,
Niklas S?derlund
On 4/7/20 4:36 PM, Niklas Söderlund wrote:
> Hi Helen,
>
> On 2020-04-03 18:33:09 -0300, Helen Koike wrote:
>> Hi,
>>
>> Media drivers need to iterate through the pipeline and call .s_stream()
>> callbacks in the subdevices.
>>
>> Instead of repeating code, add helpers for this.
>>
>> These helpers will go walk through the pipeline only visiting entities
>> that participates in the stream, i.e. it follows links from sink to source
>> (and not the opposite).
>>
>> Which means that in a topology like this https://bit.ly/3b2MxjI
>> calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call
>> .s_stream(true) for rkisp1_resizer_selfpath.
>>
>> stream_count variable was added in v4l2_subdevice to handle nested calls
>> to the helpers.
>> This is useful when the driver allows streaming from more then one
>> capture device sharing subdevices.
>>
>> This patch came from the error I was facing when multistreaming from
>> rkisp1 driver, where stoping one capture would call s_stream(false) in
>> the pipeline, causing a stall in the second capture device.
>>
>> Also, the vimc patch https://patchwork.kernel.org/patch/10948833/ won't
>> be required with this patchset.
>>
>> This patchset was tested on rkisp1 and vimc drivers.
>
> I'm just curious, with this series applied can I stream simultaneously
> on multiple video devises using vimc?
No, this patch only removes the requirement of patch 1/3 in the series
"vimc: Allow multiple capture devices to use the same sensor", since the counter
is being added in the core, so it won't be required to add it for each subdevice.
The other patches in that series are still required.
Regards,
Helen
>
>>
>> Other cleanup might be possible (but I won't add in this patchset as I
>> don't have the hw to test):
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97
>>
>> Changes in V2:
>> ====================
>> The first version was calling the s_stream() callbacks from sensor to
>> capture.
>>
>> This was generating errors in the Scarlet Chromebook, when the sensor
>> was being enabled before the ISP.
>>
>> It make sense to enable subdevices from capture to sensor instead (which
>> is what most drivers do already).
>>
>> This v2 drops the changes from mc-entity.c, and re-implement helpers in
>> v4l2-common.c
>>
>> Overview of patches:
>> ====================
>>
>> Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by
>> adding stream_count in the subdevice struct.
>>
>> Patch 2/3 cleanup rkisp1 driver to use the helpers.
>>
>> Patch 3/3 cleanup vimc driver to use the helpers.
>>
>> Helen Koike (3):
>> media: v4l2-common: add helper functions to call s_stream() callbacks
>> media: staging: rkisp1: use v4l2_pipeline_stream_{enable,disable}
>> helpers
>> media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers
>>
>> drivers/media/platform/vimc/vimc-capture.c | 28 +++--
>> drivers/media/platform/vimc/vimc-streamer.c | 49 +-------
>> drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++
>> drivers/staging/media/rkisp1/rkisp1-capture.c | 76 +-----------
>> include/media/v4l2-common.h | 28 +++++
>> include/media/v4l2-subdev.h | 2 +
>> 6 files changed, 173 insertions(+), 127 deletions(-)
>>
>> --
>> 2.26.0
>>
>
Hi Helen,
Thanks for your reply.
On 2020-04-07 21:05:03 -0300, Helen Koike wrote:
> No, this patch only removes the requirement of patch 1/3 in the series
> "vimc: Allow multiple capture devices to use the same sensor", since the counter
> is being added in the core, so it won't be required to add it for each subdevice.
> The other patches in that series are still required.
OK, just checking. One step in the that direction at least :-)
--
Regards,
Niklas S?derlund