lkml.org 
[lkml]   [2020]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
    From
    Date
    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
    >>>>>>>>>
    >>>>
    >>

    \
     
     \ /
      Last update: 2020-08-05 19:03    [W:3.188 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site