| Message ID | 20190603231637.28554-11-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. 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@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 ? > + } else { > + ipa_->init(); I'm tempted to move the init() call to createIPA(), what do you think ? > + } > + > std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); > > /* Locate and open the capture video node. */
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@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
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@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. */
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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> #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); + if (ipa_ == nullptr) { + LOG(VIMC, Error) << "no matching IPA found"; + return false; + } else { + ipa_->init(); + } + std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); /* Locate and open the capture video node. */
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@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(+)