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@xxxxxxxxxxxxx/
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
