| Message ID | 20201021002418.21764-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Wed, Oct 21, 2020 at 03:24:16AM +0300, Laurent Pinchart wrote: > The rkisp1 and simple pipeline handlers can fail to register any camera, > if initialization of all the detected cameras fail. In that case, they > still return success from their match function. As no camera gets > registered, the pipeline handler is immediately destroyed, releasing the > acquired media devices, and the camera manager immediately tries to > match the same pipeline handler with the same media device, causing an > endless loop. > > Fix it by returning false from the match function if no camera gets > registered. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++--- > src/libcamera/pipeline/simple/simple.cpp | 5 ++++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2352ab3b234a..c74a2e9bd548 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1107,10 +1107,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > if (!pad) > return false; > > - for (MediaLink *link : pad->links()) > - createCamera(link->source()->entity()); > + bool registered = false; > + for (MediaLink *link : pad->links()) { > + if (!createCamera(link->source()->entity())) > + registered = true; > + } > > - return true; > + return registered; > } > > /* ----------------------------------------------------------------------------- > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8c00c0ffc121..8868a43beeb4 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -821,6 +821,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > } > > /* Initialize each pipeline and register a corresponding camera. */ > + bool registered = false; > + Intentional empty line ? > for (std::unique_ptr<SimpleCameraData> &data : pipelines) { > int ret = data->init(); > if (ret < 0) > @@ -830,9 +832,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > Camera::create(this, data->sensor_->id(), > data->streams()); > registerCamera(std::move(camera), std::move(data)); > + registered = true; So the actual camera matching happens in data->init() here ? Anyway, looks good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > > - return true; > + return registered; > } > > V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity) > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, I assume this was intedned as a cover letter but ended up as a copy of '[PATCH] libcamera: pipeline: Fail match() when no camera is registered' :-) I can't see a diff between the two so if I missed something please let me know. On 2020-10-21 03:24:16 +0300, Laurent Pinchart wrote: > The rkisp1 and simple pipeline handlers can fail to register any camera, > if initialization of all the detected cameras fail. In that case, they > still return success from their match function. As no camera gets > registered, the pipeline handler is immediately destroyed, releasing the > acquired media devices, and the camera manager immediately tries to > match the same pipeline handler with the same media device, causing an > endless loop. > > Fix it by returning false from the match function if no camera gets > registered. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++--- > src/libcamera/pipeline/simple/simple.cpp | 5 ++++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2352ab3b234a..c74a2e9bd548 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1107,10 +1107,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > if (!pad) > return false; > > - for (MediaLink *link : pad->links()) > - createCamera(link->source()->entity()); > + bool registered = false; > + for (MediaLink *link : pad->links()) { > + if (!createCamera(link->source()->entity())) > + registered = true; > + } > > - return true; > + return registered; > } > > /* ----------------------------------------------------------------------------- > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8c00c0ffc121..8868a43beeb4 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -821,6 +821,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > } > > /* Initialize each pipeline and register a corresponding camera. */ > + bool registered = false; > + > for (std::unique_ptr<SimpleCameraData> &data : pipelines) { > int ret = data->init(); > if (ret < 0) > @@ -830,9 +832,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > Camera::create(this, data->sensor_->id(), > data->streams()); > registerCamera(std::move(camera), std::move(data)); > + registered = true; > } > > - return true; > + return registered; > } > > V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity) > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for looking into this! On 21.10.2020 03:24, Laurent Pinchart wrote: > The rkisp1 and simple pipeline handlers can fail to register any camera, > if initialization of all the detected cameras fail. In that case, they > still return success from their match function. As no camera gets > registered, the pipeline handler is immediately destroyed, releasing the > acquired media devices, and the camera manager immediately tries to > match the same pipeline handler with the same media device, causing an > endless loop. > > Fix it by returning false from the match function if no camera gets > registered. I've faced this endless loop once when in the libcamera sources I commented out all the formats supported by the sensor - thus making data->init() to fail. Your patch does fix this issue - has tested it with simple pipeline handler. Tested-by: Andrey Konovalov <andrey.konovalov@linaro.org> Thanks, Andrey > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++--- > src/libcamera/pipeline/simple/simple.cpp | 5 ++++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2352ab3b234a..c74a2e9bd548 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1107,10 +1107,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > if (!pad) > return false; > > - for (MediaLink *link : pad->links()) > - createCamera(link->source()->entity()); > + bool registered = false; > + for (MediaLink *link : pad->links()) { > + if (!createCamera(link->source()->entity())) > + registered = true; > + } > > - return true; > + return registered; > } > > /* ----------------------------------------------------------------------------- > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8c00c0ffc121..8868a43beeb4 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -821,6 +821,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > } > > /* Initialize each pipeline and register a corresponding camera. */ > + bool registered = false; > + > for (std::unique_ptr<SimpleCameraData> &data : pipelines) { > int ret = data->init(); > if (ret < 0) > @@ -830,9 +832,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > Camera::create(this, data->sensor_->id(), > data->streams()); > registerCamera(std::move(camera), std::move(data)); > + registered = true; > } > > - return true; > + return registered; > } > > V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity) >
Hi Jacopo, On Wed, Oct 21, 2020 at 12:29:25PM +0200, Jacopo Mondi wrote: > On Wed, Oct 21, 2020 at 03:24:16AM +0300, Laurent Pinchart wrote: > > The rkisp1 and simple pipeline handlers can fail to register any camera, > > if initialization of all the detected cameras fail. In that case, they > > still return success from their match function. As no camera gets > > registered, the pipeline handler is immediately destroyed, releasing the > > acquired media devices, and the camera manager immediately tries to > > match the same pipeline handler with the same media device, causing an > > endless loop. > > > > Fix it by returning false from the match function if no camera gets > > registered. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++--- > > src/libcamera/pipeline/simple/simple.cpp | 5 ++++- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 2352ab3b234a..c74a2e9bd548 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -1107,10 +1107,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > if (!pad) > > return false; > > > > - for (MediaLink *link : pad->links()) > > - createCamera(link->source()->entity()); > > + bool registered = false; > > + for (MediaLink *link : pad->links()) { > > + if (!createCamera(link->source()->entity())) > > + registered = true; > > + } > > > > - return true; > > + return registered; > > } > > > > /* ----------------------------------------------------------------------------- > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 8c00c0ffc121..8868a43beeb4 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -821,6 +821,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > > } > > > > /* Initialize each pipeline and register a corresponding camera. */ > > + bool registered = false; > > + > > Intentional empty line ? Yes, I like a bit of space sometimes :-) I think I tend to not add a blank line when the for statement itself operates on the variable that was just declared. > > for (std::unique_ptr<SimpleCameraData> &data : pipelines) { > > int ret = data->init(); > > if (ret < 0) > > @@ -830,9 +832,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > > Camera::create(this, data->sensor_->id(), > > data->streams()); > > registerCamera(std::move(camera), std::move(data)); > > + registered = true; > > So the actual camera matching happens in data->init() here ? Not so much the matching, but init() indeed analyzes the pipeline in details to make sure everything is right, so it can fail. It shouldn't, as we have an explicit match with the driver name table, but if something wrong happens, it's best to avoid infinite loops. > Anyway, looks good > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > } > > > > - return true; > > + return registered; > > } > > > > V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 2352ab3b234a..c74a2e9bd548 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1107,10 +1107,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) if (!pad) return false; - for (MediaLink *link : pad->links()) - createCamera(link->source()->entity()); + bool registered = false; + for (MediaLink *link : pad->links()) { + if (!createCamera(link->source()->entity())) + registered = true; + } - return true; + return registered; } /* ----------------------------------------------------------------------------- diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 8c00c0ffc121..8868a43beeb4 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -821,6 +821,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) } /* Initialize each pipeline and register a corresponding camera. */ + bool registered = false; + for (std::unique_ptr<SimpleCameraData> &data : pipelines) { int ret = data->init(); if (ret < 0) @@ -830,9 +832,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) Camera::create(this, data->sensor_->id(), data->streams()); registerCamera(std::move(camera), std::move(data)); + registered = true; } - return true; + return registered; } V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
The rkisp1 and simple pipeline handlers can fail to register any camera, if initialization of all the detected cameras fail. In that case, they still return success from their match function. As no camera gets registered, the pipeline handler is immediately destroyed, releasing the acquired media devices, and the camera manager immediately tries to match the same pipeline handler with the same media device, causing an endless loop. Fix it by returning false from the match function if no camera gets registered. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++--- src/libcamera/pipeline/simple/simple.cpp | 5 ++++- 2 files changed, 10 insertions(+), 4 deletions(-)