[libcamera-devel] [PATCH v2 09/10] libcamera: pipeline: store IPA in pipeline data

[Laurent Pinchart]

Hi Paul,

On Tue, Jun 04, 2019 at 12:24:03PM -0400, Paul Elder wrote:
> On Tue, Jun 04, 2019 at 03:07:44PM +0300, Laurent Pinchart wrote:
> > Hi Paul,
> > 
> > Thank you for the patch.
> 
> Thank you for the review.
> 
> > On Mon, Jun 03, 2019 at 07:16:36PM -0400, Paul Elder wrote:
> >> After the pipeline handler acquires an IPA, it should save it for future
> >> use. Store it in the pipeline data.
> >> 
> >> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >> ---
> >> New patch
> >> 
> >>  src/libcamera/include/pipeline_handler.h | 3 +++
> >>  src/libcamera/pipeline_handler.cpp       | 8 ++++++++
> >>  2 files changed, 11 insertions(+)
> >> 
> >> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> >> index 67971b3..a2d9624 100644
> >> --- a/src/libcamera/include/pipeline_handler.h
> >> +++ b/src/libcamera/include/pipeline_handler.h
> >> @@ -14,6 +14,7 @@
> >>  #include <string>
> >>  #include <vector>
> >>  
> >> +#include <libcamera/ipa_interface.h>
> >>  #include <libcamera/stream.h>
> >>  
> >>  namespace libcamera {
> >> @@ -93,6 +94,8 @@ protected:
> >>  	const char *name_;
> >>  	uint32_t version_;
> >>  
> >> +	std::unique_ptr<IPAInterface> ipa_;
> >> +
> >>  private:
> >>  	void mediaDeviceDisconnected(MediaDevice *media);
> >>  	virtual void disconnect();
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index b18f14d..d6e68b0 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -530,6 +530,14 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
> >>   * \brief Pipeline handler version
> >>   */
> >>  
> >> +/**
> >> + * \var PipelineHandler::ipa_
> >> + * \brief The IPA that the pipeline handler will use
> > 
> > "The IPA interface instance for this pipeline handler" ?
> > 
> > And thinking about it, will we always have a 1:1 mapping between
> > pipeline handlers and IPA instances ? In the IPU3 case, if we support
> 
> tbh, I was doubting a 1:1 mapping between pipeline handlers and IPA
> instances.
> 
> > capturing from two cameras simultaneously, I think we'll have one IPA
> > instance per camera, while both cameras will be handled by the same
> > pipeline handler. We could store the IPA instance in the camera data
> > instead. Or, for now, until we figure the details out, store it in the
> > vimc pipeline handler in patch 10/10 (thus dropping this patch). What do
> > you think ?
> 
> If the mapping is 1:1 between cameras and IPA instances then I think we
> can put the IPA instance in the camera data.

I don't think it will be the case either. The rkisp1 can handle two
cameras, but not at the same time, so they will likely be a single IPA
instance (although that's actually debatable). How about not trying to
make the code too generic yet and move the ipa_ member to the
PipelineHandlerVimc class for now ?

> >> + *
> >> + * The pipeline handler can acquire an IPA from the IPAManager. Once it is
> >> + * acquired, it will be stored here.
> >> + */
> >> +
> >>  /**
> >>   * \class PipelineHandlerFactory
> >>   * \brief Registration of PipelineHandler classes and creation of instances

-- 
Regards,

Laurent Pinchart