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

[Jacopo Mondi]

Hello,
   sorry to jump in on a non-design minor issue, but I've just noticed while
reading the patch that:

On Tue, Jun 04, 2019 at 11:53:43AM -0400, Paul Elder wrote:
> 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();

If you keep init() as a separate call, and fail if (!ipa), you should
drop the else here.



> >
> > 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190604/8d729bf9/attachment.sig>