| Message ID | 20210616095610.3593281-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 6/16/21 3:26 PM, Kieran Bingham wrote: > Registering a camera for VIMC without an IPA will fail later when > attempting to configure. > > The IPA is required for VIMC so fail early if it can't be loaded. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/vimc/vimc.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 9ebd723be171..8af0e92012e6 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -431,6 +431,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); > } else { > LOG(VIMC, Warning) << "no matching IPA found"; > + return false; Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > } > > /* Create and register the camera. */
Hi Kieran, Thank you for the patch. On Wed, Jun 16, 2021 at 10:56:10AM +0100, Kieran Bingham wrote: > Registering a camera for VIMC without an IPA will fail later when > attempting to configure. > > The IPA is required for VIMC so fail early if it can't be loaded. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Assuming this doesn't break any unit test, the change looks good. > --- > src/libcamera/pipeline/vimc/vimc.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 9ebd723be171..8af0e92012e6 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -431,6 +431,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); > } else { > LOG(VIMC, Warning) << "no matching IPA found"; > + return false; > } I would have rewritten the code as if (!data->ipa_) { LOG(VIMC, Warning) << "no matching IPA found"; return false; } std::string conf = data->ipa_->configurationFile("vimc.conf"); data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); and while at it, turned the message to an Error. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > /* Create and register the camera. */
Hi Laurent, On 16/06/2021 13:33, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Wed, Jun 16, 2021 at 10:56:10AM +0100, Kieran Bingham wrote: >> Registering a camera for VIMC without an IPA will fail later when >> attempting to configure. >> >> The IPA is required for VIMC so fail early if it can't be loaded. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Assuming this doesn't break any unit test, the change looks good. > >> --- >> src/libcamera/pipeline/vimc/vimc.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp >> index 9ebd723be171..8af0e92012e6 100644 >> --- a/src/libcamera/pipeline/vimc/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp >> @@ -431,6 +431,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) >> data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); >> } else { >> LOG(VIMC, Warning) << "no matching IPA found"; >> + return false; >> } > > I would have rewritten the code as > > if (!data->ipa_) { > LOG(VIMC, Warning) << "no matching IPA found"; > return false; > } > > std::string conf = data->ipa_->configurationFile("vimc.conf"); > data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); > > and while at it, turned the message to an Error. That's worth the update indeed. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> >> /* Create and register the camera. */ >
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index 9ebd723be171..8af0e92012e6 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -431,6 +431,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) data->ipa_->init(IPASettings{ conf, data->sensor_->model() }); } else { LOG(VIMC, Warning) << "no matching IPA found"; + return false; } /* Create and register the camera. */
Registering a camera for VIMC without an IPA will fail later when attempting to configure. The IPA is required for VIMC so fail early if it can't be loaded. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/vimc/vimc.cpp | 1 + 1 file changed, 1 insertion(+)