[libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc: add dummy IPA

[Paul Elder]

Hi Laurent,

On Tue, Jun 04, 2019 at 03:13:28PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.

Thank you for the review.

> On Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:
> > Make the vimc pipeline handler get the dummy IPA, to show how an IPA can
> > be acquired by a pipeline handler.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > Changes in v2:
> > - save IPA to pipeline data
> > - no IPA is fatal error
> > 
> >  src/libcamera/pipeline/vimc.cpp | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 78feeb8..f000c21 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -10,10 +10,12 @@
> >  
> >  #include <libcamera/camera.h>
> >  #include <libcamera/ipa/ipa_module_info.h>
> > +#include <libcamera/ipa/ipa_interface.h>
> 
> sort() :-)
> 
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >  
> >  #include "device_enumerator.h"
> > +#include "ipa_manager.h"
> >  #include "log.h"
> >  #include "media_device.h"
> >  #include "pipeline_handler.h"
> > @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> >  	if (!media)
> >  		return false;
> >  
> > +	IPAManager *ipaManager = IPAManager::instance();
> > +	ipa_ = ipaManager->createIPA(this);
> 
> You could also write this
> 
> 	ipa_ = IPAManager::instance()->createIPA(this);
> 
> > +	if (ipa_ == nullptr) {
> > +		LOG(VIMC, Error) << "no matching IPA found";
> > +		return false;
> 
> As the IPA isn't required yet, should we make this a non-fatal error and
> downgrade the message to a warning ? My review of v1 wasn't very clear,
> I meant to ask if it should really be an error, or just a warning. What
> do you think ?

I don't think it should be a fatal error, since it isn't required.

> > +	} else {
> > +		ipa_->init();
> 
> I'm tempted to move the init() call to createIPA(), what do you think ?

I'm not sure. I can't think of much reason for or against.

I suppose, createIPA() is meant to return the IPAInterface as-is
created by the factory, with no other calls to it, so init() should not
be called from createIPA().

> > +	}
> > +
> >  	std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
> >  
> >  	/* Locate and open the capture video node. */
> 

Thanks,

Paul