| Message ID | 20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hello Jean-Michel, On Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote: > IPA was configured after all the pipeline devices were started, > including IPA itself... s/..././ :) > Move it at the end of configure() call. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2ea13ec9..bb61ef4a 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > + std::map<uint32_t, ControlInfoMap> entityControls; > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + data->ipa_->configure(entityControls); > + > return 0; > } > > @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) > { > - std::map<uint32_t, ControlInfoMap> entityControls; > IPU3CameraData *data = cameraData(camera); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret) > goto error; > > - entityControls.emplace(0, data->cio2_.sensor()->controls()); > - data->ipa_->configure(entityControls); > - > return 0; > > error: > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jean-Michel, Thank you for the patch. On Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote: > IPA was configured after all the pipeline devices were started, > including IPA itself... > Move it at the end of configure() call. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> The change looks good. Could you make sure it doesn't break anything if we call configure() twice without any start() in-between ? This check should be added to lc-compliance (once we get it merged), in the meantime, you can modify any test application to duplicate the configure() call. Bonus points if this can be run under valgrind too. Once done, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2ea13ec9..bb61ef4a 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > + std::map<uint32_t, ControlInfoMap> entityControls; > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + data->ipa_->configure(entityControls); > + > return 0; > } > > @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) > { > - std::map<uint32_t, ControlInfoMap> entityControls; > IPU3CameraData *data = cameraData(camera); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret) > goto error; > > - entityControls.emplace(0, data->cio2_.sensor()->controls()); > - data->ipa_->configure(entityControls); > - > return 0; > > error:
Hi Laurent, On 16/03/2021 21:55, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote: >> IPA was configured after all the pipeline devices were started, >> including IPA itself... >> Move it at the end of configure() call. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > The change looks good. Could you make sure it doesn't break anything if > we call configure() twice without any start() in-between ? This check > should be added to lc-compliance (once we get it merged), in the > meantime, you can modify any test application to duplicate the > configure() call. Bonus points if this can be run under valgrind too. I have cheated with test/camera/capture.cpp, I get too configure() calls successfully without any issue. As I really like bonus points, I also passed valgrind on a 'cam' call: ==28310== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info ==28310== Command: cam -c1 -C100 ==28310== [29:33:24.510402583] [28310] INFO Camera camera_manager.cpp:293 libcamera v0.0.0+2409-d179b494-dirty (2021-03-17T13:55:04+01:00) [29:33:27.824662259] [28313] INFO IPU3 ipu3.cpp:1128 Registered Camera[0] "\_SB_.PCI0.LNK1" connected to CSI-2 receiver 1 Using camera \_SB_.PCI0.LNK1 [29:33:28.243120313] [28310] INFO Camera camera.cpp:890 configuring streams: (0) 1280x720-NV12 Capture 100 frames 106409.888872 (0.00 fps) stream0 seq: 020006 bytesused: 1382400 106409.896575 (129.82 fps) stream0 seq: 020007 bytesused: 1382400 <snip> 106413.324588 (29.91 fps) stream0 seq: 020104 bytesused: 1382400 106413.357822 (30.09 fps) stream0 seq: 020105 bytesused: 1382400 ==28310== ==28310== HEAP SUMMARY: ==28310== in use at exit: 0 bytes in 0 blocks ==28310== total heap usage: 18,382 allocs, 18,382 frees, 2,117,494 bytes allocated ==28310== ==28310== All heap blocks were freed -- no leaks are possible ==28310== ==28310== For lists of detected and suppressed errors, rerun with: -s ==28310== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) > Once done, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thank you :-). > >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 2ea13ec9..bb61ef4a 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) >> return ret; >> } >> >> + std::map<uint32_t, ControlInfoMap> entityControls; >> + entityControls.emplace(0, data->cio2_.sensor()->controls()); >> + data->ipa_->configure(entityControls); >> + >> return 0; >> } >> >> @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) >> >> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) >> { >> - std::map<uint32_t, ControlInfoMap> entityControls; >> IPU3CameraData *data = cameraData(camera); >> CIO2Device *cio2 = &data->cio2_; >> ImgUDevice *imgu = data->imgu_; >> @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis >> if (ret) >> goto error; >> >> - entityControls.emplace(0, data->cio2_.sensor()->controls()); >> - data->ipa_->configure(entityControls); >> - >> return 0; >> >> error: >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2ea13ec9..bb61ef4a 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; } + std::map<uint32_t, ControlInfoMap> entityControls; + entityControls.emplace(0, data->cio2_.sensor()->controls()); + data->ipa_->configure(entityControls); + return 0; } @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { - std::map<uint32_t, ControlInfoMap> entityControls; IPU3CameraData *data = cameraData(camera); CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret) goto error; - entityControls.emplace(0, data->cio2_.sensor()->controls()); - data->ipa_->configure(entityControls); - return 0; error:
IPA was configured after all the pipeline devices were started, including IPA itself... Move it at the end of configure() call. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)