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@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 >>>>> -- Regards -- Kieran
