Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

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
>>>>>>>>
>>>
>