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

[Laurent Pinchart]

Hi Paul,

On Tue, Jun 04, 2019 at 11:53:43AM -0400, Paul Elder wrote:
> On Tue, Jun 04, 2019 at 03:13:28PM +0300, Laurent Pinchart wrote:
> > 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.

I agree.

> >> +	} 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().

It makes sense. The init() method may later receive parameters specific
to the pipeline handler. They could be passed to createIPA(), but we can
decide about that later.

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

-- 
Regards,

Laurent Pinchart