[libcamera-devel,00/10] Concurrent camera support in simple pipeline handler
mbox series

Message ID 20210805222436.6263-1-laurent.pinchart@ideasonboard.com
Headers show
Series
  • Concurrent camera support in simple pipeline handler
Related show

Message

Laurent Pinchart Aug. 5, 2021, 10:24 p.m. UTC 
Hello,

Another day, another patch series that has been sitting in my tree and
that I'd like to stop rebasing.

The series reworks the simple pipeline handler to correctly support
streaming from multiple cameras concurrently. The reason I haven't
posted it yet is that the libcamera core gets doesn't support this
feature yet. It's easy to work around by removing some checks for
testing, but will take more time to fix correctly.

The simple pipeline handler already supports registering multiple
cameras, and relies on the libcamera core blocking concurrent access to
different cameras backed by the same media device. It thus doesn't
implement internally mutual exclusion between cameras that share
hardware resources, but, based on the assumption that a single camera
will be used at a time, implements camera operation around a global
active camera pointer.

Patch 01/10 is the only libcamera core patch. It extends the MediaEntity
class to expose the type of userspace interface that the entity
implements. Patches 02/10 to 06/10 then rework the way the pipeline
handler stores data about pipelines and accesses video devices and
subdevs.

Patch 02/10 extends the SimpleCameraData entity representation to store
more information about graph traversal, and patch 04/10 stores the
entity corresponding to the video node with all other pipeline entities
in SimpleCameraData. Patches 03/10, 05/10 and 06/10 generalize handling
of video devices and subdev by opening them all together at match time
and storing them in a single container in the SimplePipelineHandler
class.

With this done, path 07/10 adds an entity reservation mechanism in the
SimplePipelineHandler class, to handle mutual exclusion when multiple
cameras use the same entities. With this change, the simple pipeline
handler handles concurrent access to cameras internally, without
depending on mutual exclusion implemented by the libcamera core anymore.
This doesn't result in any change of behaviour, but paves the way to
concurrent usage of multiple cameras that don't share entities once the
restriction in the libcamera core will be lifted.

Patch 08/10 continues in that direction by moving the converter from the
SimplePipelineHandler class to the SimpleCameraData class. This allows
usage of the converter by multiple cameras concurrently. Patch 09/10
then moves the bufferReady handler to the SimpleCameraData class as
well, to handle buffer completion in the correct context without relying
on the global active camera pointer.

Finally, patch 10/10 removes the active camera pointer as it's not used
anymore.

This series has only been partially tested, as the libcamera core
doesn't correctly support concurrent streaming from cameras belonging to
the same pipeline handler. The code was tested with a single camera and
didn't introduce any regression, and was also tested on i.MX8MP with a
few hacks in the libcamera core to lift the restrictions and a few
out-of-tree patches to support the i.MX8MP (those are not ready for
submission yet). I would thus understand a reluctance to get this merged
already, even if I believe that the absence of regression, combined with
the cleaner (in my opinion) implementation of the simple pipeline
handler are good enough reasons by themselves to already merge this.

Laurent Pinchart (10):
  libcamera: media_object: Expose entity type
  libcamera: pipeline: simple: Add sink and source pads to entity data
  libcamera: pipeline: simple: Delay opening of video device until
    init()
  libcamera: pipeline: simple: Store video node entity in camera data
  libcamera: pipeline: simple: Store all entity devices in common map
  libcamera: pipeline: simple: Open all video devices at match() time
  libcamera: pipeline: simple: Add pipeline pad reservation mechanism
  libcamera: pipeline: simple: Move converter to SimpleCameraData
  libcamera: pipeline: simple: Move bufferReady handler to
    SimpleCameraData
  libcamera: pipeline: simple: Remove
    SimplePipelineHandler::activeCamera_

 include/libcamera/internal/media_object.h |  11 +-
 src/libcamera/media_device.cpp            |   9 +-
 src/libcamera/media_object.cpp            |  50 ++-
 src/libcamera/pipeline/simple/simple.cpp  | 506 ++++++++++++++--------
 4 files changed, 377 insertions(+), 199 deletions(-)

Comments

Laurent Pinchart Aug. 8, 2021, 3:07 p.m. UTC | #1 
Hi Martin,

Would you be able to test this series on the Librem5 ? I'd like to make
sure it doesn't cause any regression.

On Fri, Aug 06, 2021 at 01:24:26AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> Another day, another patch series that has been sitting in my tree and
> that I'd like to stop rebasing.
> 
> The series reworks the simple pipeline handler to correctly support
> streaming from multiple cameras concurrently. The reason I haven't
> posted it yet is that the libcamera core gets doesn't support this
> feature yet. It's easy to work around by removing some checks for
> testing, but will take more time to fix correctly.
> 
> The simple pipeline handler already supports registering multiple
> cameras, and relies on the libcamera core blocking concurrent access to
> different cameras backed by the same media device. It thus doesn't
> implement internally mutual exclusion between cameras that share
> hardware resources, but, based on the assumption that a single camera
> will be used at a time, implements camera operation around a global
> active camera pointer.
> 
> Patch 01/10 is the only libcamera core patch. It extends the MediaEntity
> class to expose the type of userspace interface that the entity
> implements. Patches 02/10 to 06/10 then rework the way the pipeline
> handler stores data about pipelines and accesses video devices and
> subdevs.
> 
> Patch 02/10 extends the SimpleCameraData entity representation to store
> more information about graph traversal, and patch 04/10 stores the
> entity corresponding to the video node with all other pipeline entities
> in SimpleCameraData. Patches 03/10, 05/10 and 06/10 generalize handling
> of video devices and subdev by opening them all together at match time
> and storing them in a single container in the SimplePipelineHandler
> class.
> 
> With this done, path 07/10 adds an entity reservation mechanism in the
> SimplePipelineHandler class, to handle mutual exclusion when multiple
> cameras use the same entities. With this change, the simple pipeline
> handler handles concurrent access to cameras internally, without
> depending on mutual exclusion implemented by the libcamera core anymore.
> This doesn't result in any change of behaviour, but paves the way to
> concurrent usage of multiple cameras that don't share entities once the
> restriction in the libcamera core will be lifted.
> 
> Patch 08/10 continues in that direction by moving the converter from the
> SimplePipelineHandler class to the SimpleCameraData class. This allows
> usage of the converter by multiple cameras concurrently. Patch 09/10
> then moves the bufferReady handler to the SimpleCameraData class as
> well, to handle buffer completion in the correct context without relying
> on the global active camera pointer.
> 
> Finally, patch 10/10 removes the active camera pointer as it's not used
> anymore.
> 
> This series has only been partially tested, as the libcamera core
> doesn't correctly support concurrent streaming from cameras belonging to
> the same pipeline handler. The code was tested with a single camera and
> didn't introduce any regression, and was also tested on i.MX8MP with a
> few hacks in the libcamera core to lift the restrictions and a few
> out-of-tree patches to support the i.MX8MP (those are not ready for
> submission yet). I would thus understand a reluctance to get this merged
> already, even if I believe that the absence of regression, combined with
> the cleaner (in my opinion) implementation of the simple pipeline
> handler are good enough reasons by themselves to already merge this.
> 
> Laurent Pinchart (10):
>   libcamera: media_object: Expose entity type
>   libcamera: pipeline: simple: Add sink and source pads to entity data
>   libcamera: pipeline: simple: Delay opening of video device until
>     init()
>   libcamera: pipeline: simple: Store video node entity in camera data
>   libcamera: pipeline: simple: Store all entity devices in common map
>   libcamera: pipeline: simple: Open all video devices at match() time
>   libcamera: pipeline: simple: Add pipeline pad reservation mechanism
>   libcamera: pipeline: simple: Move converter to SimpleCameraData
>   libcamera: pipeline: simple: Move bufferReady handler to
>     SimpleCameraData
>   libcamera: pipeline: simple: Remove
>     SimplePipelineHandler::activeCamera_
> 
>  include/libcamera/internal/media_object.h |  11 +-
>  src/libcamera/media_device.cpp            |   9 +-
>  src/libcamera/media_object.cpp            |  50 ++-
>  src/libcamera/pipeline/simple/simple.cpp  | 506 ++++++++++++++--------
>  4 files changed, 377 insertions(+), 199 deletions(-)
Martin Kepplinger Aug. 24, 2021, 2:57 p.m. UTC | #2 
Am Sonntag, dem 08.08.2021 um 18:07 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> Would you be able to test this series on the Librem5 ? I'd like to
> make
> sure it doesn't cause any regression.

hi Laurent, could you send me the patches directly and tell me where
they apply?

thanks,
                          martin

> 
> On Fri, Aug 06, 2021 at 01:24:26AM +0300, Laurent Pinchart wrote:
> > Hello,
> > 
> > Another day, another patch series that has been sitting in my tree
> > and
> > that I'd like to stop rebasing.
> > 
> > The series reworks the simple pipeline handler to correctly support
> > streaming from multiple cameras concurrently. The reason I haven't
> > posted it yet is that the libcamera core gets doesn't support this
> > feature yet. It's easy to work around by removing some checks for
> > testing, but will take more time to fix correctly.
> > 
> > The simple pipeline handler already supports registering multiple
> > cameras, and relies on the libcamera core blocking concurrent
> > access to
> > different cameras backed by the same media device. It thus doesn't
> > implement internally mutual exclusion between cameras that share
> > hardware resources, but, based on the assumption that a single
> > camera
> > will be used at a time, implements camera operation around a global
> > active camera pointer.
> > 
> > Patch 01/10 is the only libcamera core patch. It extends the
> > MediaEntity
> > class to expose the type of userspace interface that the entity
> > implements. Patches 02/10 to 06/10 then rework the way the pipeline
> > handler stores data about pipelines and accesses video devices and
> > subdevs.
> > 
> > Patch 02/10 extends the SimpleCameraData entity representation to
> > store
> > more information about graph traversal, and patch 04/10 stores the
> > entity corresponding to the video node with all other pipeline
> > entities
> > in SimpleCameraData. Patches 03/10, 05/10 and 06/10 generalize
> > handling
> > of video devices and subdev by opening them all together at match
> > time
> > and storing them in a single container in the SimplePipelineHandler
> > class.
> > 
> > With this done, path 07/10 adds an entity reservation mechanism in
> > the
> > SimplePipelineHandler class, to handle mutual exclusion when
> > multiple
> > cameras use the same entities. With this change, the simple
> > pipeline
> > handler handles concurrent access to cameras internally, without
> > depending on mutual exclusion implemented by the libcamera core
> > anymore.
> > This doesn't result in any change of behaviour, but paves the way
> > to
> > concurrent usage of multiple cameras that don't share entities once
> > the
> > restriction in the libcamera core will be lifted.
> > 
> > Patch 08/10 continues in that direction by moving the converter
> > from the
> > SimplePipelineHandler class to the SimpleCameraData class. This
> > allows
> > usage of the converter by multiple cameras concurrently. Patch
> > 09/10
> > then moves the bufferReady handler to the SimpleCameraData class as
> > well, to handle buffer completion in the correct context without
> > relying
> > on the global active camera pointer.
> > 
> > Finally, patch 10/10 removes the active camera pointer as it's not
> > used
> > anymore.
> > 
> > This series has only been partially tested, as the libcamera core
> > doesn't correctly support concurrent streaming from cameras
> > belonging to
> > the same pipeline handler. The code was tested with a single camera
> > and
> > didn't introduce any regression, and was also tested on i.MX8MP
> > with a
> > few hacks in the libcamera core to lift the restrictions and a few
> > out-of-tree patches to support the i.MX8MP (those are not ready for
> > submission yet). I would thus understand a reluctance to get this
> > merged
> > already, even if I believe that the absence of regression, combined
> > with
> > the cleaner (in my opinion) implementation of the simple pipeline
> > handler are good enough reasons by themselves to already merge
> > this.
> > 
> > Laurent Pinchart (10):
> >   libcamera: media_object: Expose entity type
> >   libcamera: pipeline: simple: Add sink and source pads to entity
> > data
> >   libcamera: pipeline: simple: Delay opening of video device until
> >     init()
> >   libcamera: pipeline: simple: Store video node entity in camera
> > data
> >   libcamera: pipeline: simple: Store all entity devices in common
> > map
> >   libcamera: pipeline: simple: Open all video devices at match()
> > time
> >   libcamera: pipeline: simple: Add pipeline pad reservation
> > mechanism
> >   libcamera: pipeline: simple: Move converter to SimpleCameraData
> >   libcamera: pipeline: simple: Move bufferReady handler to
> >     SimpleCameraData
> >   libcamera: pipeline: simple: Remove
> >     SimplePipelineHandler::activeCamera_
> > 
> >  include/libcamera/internal/media_object.h |  11 +-
> >  src/libcamera/media_device.cpp            |   9 +-
> >  src/libcamera/media_object.cpp            |  50 ++-
> >  src/libcamera/pipeline/simple/simple.cpp  | 506 ++++++++++++++----
> > ----
> >  4 files changed, 377 insertions(+), 199 deletions(-)
>
Laurent Pinchart Aug. 25, 2021, 1:34 a.m. UTC | #3 
Hi Martin,

On Tue, Aug 24, 2021 at 04:57:15PM +0200, Martin Kepplinger wrote:
> Am Sonntag, dem 08.08.2021 um 18:07 +0300 schrieb Laurent Pinchart:
> > Hi Martin,
> > 
> > Would you be able to test this series on the Librem5 ? I'd like to make
> > sure it doesn't cause any regression.
> 
> hi Laurent, could you send me the patches directly and tell me where
> they apply?

Better, I can provide a branch :-)

https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=simple/multicam

> > On Fri, Aug 06, 2021 at 01:24:26AM +0300, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > Another day, another patch series that has been sitting in my tree and
> > > that I'd like to stop rebasing.
> > > 
> > > The series reworks the simple pipeline handler to correctly support
> > > streaming from multiple cameras concurrently. The reason I haven't
> > > posted it yet is that the libcamera core gets doesn't support this
> > > feature yet. It's easy to work around by removing some checks for
> > > testing, but will take more time to fix correctly.
> > > 
> > > The simple pipeline handler already supports registering multiple
> > > cameras, and relies on the libcamera core blocking concurrent access to
> > > different cameras backed by the same media device. It thus doesn't
> > > implement internally mutual exclusion between cameras that share
> > > hardware resources, but, based on the assumption that a single camera
> > > will be used at a time, implements camera operation around a global
> > > active camera pointer.
> > > 
> > > Patch 01/10 is the only libcamera core patch. It extends the MediaEntity
> > > class to expose the type of userspace interface that the entity
> > > implements. Patches 02/10 to 06/10 then rework the way the pipeline
> > > handler stores data about pipelines and accesses video devices and
> > > subdevs.
> > > 
> > > Patch 02/10 extends the SimpleCameraData entity representation to store
> > > more information about graph traversal, and patch 04/10 stores the
> > > entity corresponding to the video node with all other pipeline entities
> > > in SimpleCameraData. Patches 03/10, 05/10 and 06/10 generalize handling
> > > of video devices and subdev by opening them all together at match time
> > > and storing them in a single container in the SimplePipelineHandler
> > > class.
> > > 
> > > With this done, path 07/10 adds an entity reservation mechanism in the
> > > SimplePipelineHandler class, to handle mutual exclusion when multiple
> > > cameras use the same entities. With this change, the simple pipeline
> > > handler handles concurrent access to cameras internally, without
> > > depending on mutual exclusion implemented by the libcamera core anymore.
> > > This doesn't result in any change of behaviour, but paves the way to
> > > concurrent usage of multiple cameras that don't share entities once the
> > > restriction in the libcamera core will be lifted.
> > > 
> > > Patch 08/10 continues in that direction by moving the converter from the
> > > SimplePipelineHandler class to the SimpleCameraData class. This allows
> > > usage of the converter by multiple cameras concurrently. Patch> 09/10
> > > then moves the bufferReady handler to the SimpleCameraData class as
> > > well, to handle buffer completion in the correct context without relying
> > > on the global active camera pointer.
> > > 
> > > Finally, patch 10/10 removes the active camera pointer as it's not used
> > > anymore.
> > > 
> > > This series has only been partially tested, as the libcamera core
> > > doesn't correctly support concurrent streaming from cameras belonging to
> > > the same pipeline handler. The code was tested with a single camera and
> > > didn't introduce any regression, and was also tested on i.MX8MP with a
> > > few hacks in the libcamera core to lift the restrictions and a few
> > > out-of-tree patches to support the i.MX8MP (those are not ready for
> > > submission yet). I would thus understand a reluctance to get this merged
> > > already, even if I believe that the absence of regression, combined with
> > > the cleaner (in my opinion) implementation of the simple pipeline
> > > handler are good enough reasons by themselves to already merge this.
> > > 
> > > Laurent Pinchart (10):
> > >   libcamera: media_object: Expose entity type
> > >   libcamera: pipeline: simple: Add sink and source pads to entity data
> > >   libcamera: pipeline: simple: Delay opening of video device until init()
> > >   libcamera: pipeline: simple: Store video node entity in camera data
> > >   libcamera: pipeline: simple: Store all entity devices in common map
> > >   libcamera: pipeline: simple: Open all video devices at match() time
> > >   libcamera: pipeline: simple: Add pipeline pad reservation mechanism
> > >   libcamera: pipeline: simple: Move converter to SimpleCameraData
> > >   libcamera: pipeline: simple: Move bufferReady handler to SimpleCameraData
> > >   libcamera: pipeline: simple: Remove SimplePipelineHandler::activeCamera_
> > > 
> > >  include/libcamera/internal/media_object.h |  11 +-
> > >  src/libcamera/media_device.cpp            |   9 +-
> > >  src/libcamera/media_object.cpp            |  50 ++-
> > >  src/libcamera/pipeline/simple/simple.cpp  | 506 ++++++++++++++----
> > > ----
> > >  4 files changed, 377 insertions(+), 199 deletions(-)
Martin Kepplinger Aug. 30, 2021, 1:43 p.m. UTC | #4 
Am Mittwoch, dem 25.08.2021 um 04:34 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Tue, Aug 24, 2021 at 04:57:15PM +0200, Martin Kepplinger wrote:
> > Am Sonntag, dem 08.08.2021 um 18:07 +0300 schrieb Laurent Pinchart:
> > > Hi Martin,
> > > 
> > > Would you be able to test this series on the Librem5 ? I'd like
> > > to make
> > > sure it doesn't cause any regression.
> > 
> > hi Laurent, could you send me the patches directly and tell me
> > where
> > they apply?
> 
> Better, I can provide a branch :-)
> 
> https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=simple/multicam
> 
> > 


hi Laurent,

using the above branch, the cam application streams the highest mode
(by default) just like the libcamera master branch. I don't see any
difference.

Do you want to have anything else in this regard?

thanks,
                          martin
Laurent Pinchart Aug. 30, 2021, 2:38 p.m. UTC | #5 
Hi Martin,

On Mon, Aug 30, 2021 at 03:43:32PM +0200, Martin Kepplinger wrote:
> Am Mittwoch, dem 25.08.2021 um 04:34 +0300 schrieb Laurent Pinchart:
> > On Tue, Aug 24, 2021 at 04:57:15PM +0200, Martin Kepplinger wrote:
> > > Am Sonntag, dem 08.08.2021 um 18:07 +0300 schrieb Laurent Pinchart:
> > > > Hi Martin,
> > > > 
> > > > Would you be able to test this series on the Librem5 ? I'd like to make
> > > > sure it doesn't cause any regression.
> > > 
> > > hi Laurent, could you send me the patches directly and tell me
> > > where
> > > they apply?
> > 
> > Better, I can provide a branch :-)
> > 
> > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=simple/multicam
> 
> hi Laurent,
> 
> using the above branch, the cam application streams the highest mode
> (by default) just like the libcamera master branch. I don't see any
> difference.
> 
> Do you want to have anything else in this regard?

Thank you for testing. This is all I need. Can I add a

Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>

to all the patches in the series ?
Martin Kepplinger Aug. 30, 2021, 3:32 p.m. UTC | #6 
Am Montag, dem 30.08.2021 um 17:38 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Mon, Aug 30, 2021 at 03:43:32PM +0200, Martin Kepplinger wrote:
> > Am Mittwoch, dem 25.08.2021 um 04:34 +0300 schrieb Laurent
> > Pinchart:
> > > On Tue, Aug 24, 2021 at 04:57:15PM +0200, Martin Kepplinger
> > > wrote:
> > > > Am Sonntag, dem 08.08.2021 um 18:07 +0300 schrieb Laurent
> > > > Pinchart:
> > > > > Hi Martin,
> > > > > 
> > > > > Would you be able to test this series on the Librem5 ? I'd
> > > > > like to make
> > > > > sure it doesn't cause any regression.
> > > > 
> > > > hi Laurent, could you send me the patches directly and tell me
> > > > where
> > > > they apply?
> > > 
> > > Better, I can provide a branch :-)
> > > 
> > > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=simple/multicam
> > 
> > hi Laurent,
> > 
> > using the above branch, the cam application streams the highest
> > mode
> > (by default) just like the libcamera master branch. I don't see any
> > difference.
> > 
> > Do you want to have anything else in this regard?
> 
> Thank you for testing. This is all I need. Can I add a
> 
> Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> 
> to all the patches in the series ?
> 

yes, you can. thanks,

                           martin