| Message ID | 20210112045140.10979-3-email@uajain.com |
|---|---|
| State | Accepted |
| Commit | 44058f1c68a9789f0e9fbdfcb64d99d6313ed121 |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Tue, Jan 12, 2021 at 10:21:40AM +0530, Umang Jain wrote: > On the bail out path, always ensure that ImgU and CIO2 are stopped > before freeing the buffers. V4L2VideoDevice class guarantees that > calling stop() without having to call start() is harmless, hence use s/having to call/having called/ > this guarantee to simplify error paths. > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f1151733..73304ea7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -617,15 +617,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con > goto error; > > ret = imgu->start(); > - if (ret) { > - imgu->stop(); > - cio2->stop(); > + if (ret) > goto error; > - } > > return 0; > > error: > + imgu->stop(); > + cio2->stop(); cio2->stop() also calls V4L2VideoDevice::requestBuffers(), which calls VIDIOC_REQBUFS(0). This shouldn't be an issue, but we could possibly optimize it. It can be done on top if needed. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > freeBuffers(camera); > LOG(IPU3, Error) << "Failed to start camera " << camera->id(); >
Hi Umang, On 12/01/2021 05:38, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Jan 12, 2021 at 10:21:40AM +0530, Umang Jain wrote: >> On the bail out path, always ensure that ImgU and CIO2 are stopped >> before freeing the buffers. V4L2VideoDevice class guarantees that >> calling stop() without having to call start() is harmless, hence use > > s/having to call/having called/ > >> this guarantee to simplify error paths. >> >> Signed-off-by: Umang Jain <email@uajain.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index f1151733..73304ea7 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -617,15 +617,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con >> goto error; >> >> ret = imgu->start(); >> - if (ret) { >> - imgu->stop(); >> - cio2->stop(); >> + if (ret) >> goto error; >> - } >> >> return 0; >> >> error: >> + imgu->stop(); >> + cio2->stop(); > > cio2->stop() also calls V4L2VideoDevice::requestBuffers(), which calls > VIDIOC_REQBUFS(0). This shouldn't be an issue, but we could possibly > optimize it. It can be done on top if needed. I think we can do optimisations later, this series fixes a bug from what I understand it - so I'll apply with the corrections to the commit log above. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> freeBuffers(camera); >> LOG(IPU3, Error) << "Failed to start camera " << camera->id(); >> >
Hi Umang On Tue, Jan 12, 2021 at 10:21:40AM +0530, Umang Jain wrote: > On the bail out path, always ensure that ImgU and CIO2 are stopped > before freeing the buffers. V4L2VideoDevice class guarantees that > calling stop() without having to call start() is harmless, hence use > this guarantee to simplify error paths. > > Signed-off-by: Umang Jain <email@uajain.com> Thanks Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f1151733..73304ea7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -617,15 +617,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con > goto error; > > ret = imgu->start(); > - if (ret) { > - imgu->stop(); > - cio2->stop(); > + if (ret) > goto error; > - } > > return 0; > > error: > + imgu->stop(); > + cio2->stop(); > freeBuffers(camera); > LOG(IPU3, Error) << "Failed to start camera " << camera->id(); > > -- > 2.26.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f1151733..73304ea7 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -617,15 +617,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con goto error; ret = imgu->start(); - if (ret) { - imgu->stop(); - cio2->stop(); + if (ret) goto error; - } return 0; error: + imgu->stop(); + cio2->stop(); freeBuffers(camera); LOG(IPU3, Error) << "Failed to start camera " << camera->id();
On the bail out path, always ensure that ImgU and CIO2 are stopped before freeing the buffers. V4L2VideoDevice class guarantees that calling stop() without having to call start() is harmless, hence use this guarantee to simplify error paths. Signed-off-by: Umang Jain <email@uajain.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)