| Message ID | 20211208151527.1404715-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > Enumerate the sensor device entities in PipelineHandlerRPi::match() and loop > over PipelineHandlerRPi::registerCamera() for each sensor found. This will > allow the pipeline handler to register multiple cameras attached to a single > Unicam instance with a Video Mux device. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 35 +++++++++++-------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 86851ac467ad..756878c70036 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -310,7 +310,7 @@ private: > return static_cast<RPiCameraData *>(camera->_d()); > } > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity); > int queueAllBuffers(Camera *camera); > int prepareBuffers(Camera *camera); > void freeBuffers(Camera *camera); > @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > return false; > } > > - int ret = registerCamera(unicamDevice, ispDevice); > - if (ret) { > - LOG(RPI, Error) << "Failed to register camera: " << ret; > - return false; > + /* > + * The loop below is used to register multiple cameras behind one or more > + * video mux devices that are attaced to a particular Unicam instance. s/attaced/attached/ > + * Obviously these cameras cannot be used simultaneously. We need to expose mutual exclusion between cameras to applications through the libcamera public API. It doesn't have to be a blocker for the series, but should be done soon after, otherwise trying to use both cameras will result in incorrect behaviour (and possibly even crashes). Have you given any thought to how this could be done ? > + */ > + unsigned int numCameras = 0; > + for (MediaEntity *entity : unicamDevice->entities()) { > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > + continue; > + > + int ret = registerCamera(unicamDevice, ispDevice, entity); > + if (ret) > + LOG(RPI, Error) << "Failed to register camera " > + << entity->name() << ": " << ret; > + else > + numCameras++; I tend to deal with error first, but that's a personal preference: int ret = registerCamera(unicamDevice, ispDevice, entity); if (ret) { LOG(RPI, Error) << "Failed to register camera " << entity->name() << ": " << ret; continue; } numCameras++; > } > > - return true; > + return !!numCameras; This looks fine, but by itself may cause a regression. Patch 2/2 will be merged with this one, so the problem would only occur during bisection, but still, I think we should squash the two patches together. > } > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity) > { > std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this); > > @@ -1079,14 +1091,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > - /* Identify the sensor. */ > - for (MediaEntity *entity : unicam->entities()) { > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > - data->sensor_ = std::make_unique<CameraSensor>(entity); > - break; > - } > - } > - > + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); > if (!data->sensor_) > return -EINVAL; >
Hi Laurent, Thank you for your feedback! On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > > Enumerate the sensor device entities in PipelineHandlerRPi::match() and > loop > > over PipelineHandlerRPi::registerCamera() for each sensor found. This > will > > allow the pipeline handler to register multiple cameras attached to a > single > > Unicam instance with a Video Mux device. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 +++++++++++-------- > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 86851ac467ad..756878c70036 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -310,7 +310,7 @@ private: > > return static_cast<RPiCameraData *>(camera->_d()); > > } > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, > MediaEntity *sensorEntity); > > int queueAllBuffers(Camera *camera); > > int prepareBuffers(Camera *camera); > > void freeBuffers(Camera *camera); > > @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > > return false; > > } > > > > - int ret = registerCamera(unicamDevice, ispDevice); > > - if (ret) { > > - LOG(RPI, Error) << "Failed to register camera: " << ret; > > - return false; > > + /* > > + * The loop below is used to register multiple cameras behind one > or more > > + * video mux devices that are attaced to a particular Unicam > instance. > > s/attaced/attached/ > > > + * Obviously these cameras cannot be used simultaneously. > > We need to expose mutual exclusion between cameras to applications > through the libcamera public API. It doesn't have to be a blocker for > the series, but should be done soon after, otherwise trying to use both > cameras will result in incorrect behaviour (and possibly even crashes). > Have you given any thought to how this could be done ? > Is this not already handled in the Camera class? If I have a mux with 2 devices attached to a single Unicam instance, hence single pipeline handler, I cannot run camera in one process and camera 1 in another. Camera::acquire() would vail to lock the pipeline handler: if (!d->pipe_->lock()) { LOG(Camera, Info) << "Pipeline handler in use by another process"; return -EBUSY; } Perhaps this long point should be turned into Error level? I get the same result if I try to run both cameras in a single process (e.g. with cam -c 1 -C -c 2 -C). > > > + */ > > + unsigned int numCameras = 0; > > + for (MediaEntity *entity : unicamDevice->entities()) { > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > > + continue; > > + > > + int ret = registerCamera(unicamDevice, ispDevice, entity); > > + if (ret) > > + LOG(RPI, Error) << "Failed to register camera " > > + << entity->name() << ": " << ret; > > + else > > + numCameras++; > > I tend to deal with error first, but that's a personal preference: > > int ret = registerCamera(unicamDevice, ispDevice, entity); > if (ret) { > LOG(RPI, Error) << "Failed to register camera " > << entity->name() << ": " << ret; > continue; > } > > numCameras++; > > > } > > > > - return true; > > + return !!numCameras; > > This looks fine, but by itself may cause a regression. Patch 2/2 will be > merged with this one, so the problem would only occur during bisection, > but still, I think we should squash the two patches together. > I'm sure I am missing something obvious, but why would this patch in isolation cause a regression? The only additional function gained here would be to allow the pipeline handler to enumerate multiple cameras attached to a mux, but not allow one of the cameras to be run - same as before this change :-) Regards, Naush > > } > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice > *isp) > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice > *isp, MediaEntity *sensorEntity) > > { > > std::unique_ptr<RPiCameraData> data = > std::make_unique<RPiCameraData>(this); > > > > @@ -1079,14 +1091,7 @@ int > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > > > - /* Identify the sensor. */ > > - for (MediaEntity *entity : unicam->entities()) { > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > > - data->sensor_ = > std::make_unique<CameraSensor>(entity); > > - break; > > - } > > - } > > - > > + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); > > if (!data->sensor_) > > return -EINVAL; > > > > -- > Regards, > > Laurent Pinchart >
Quoting Naushir Patuck (2021-12-09 08:57:52) > Hi Laurent, > > Thank you for your feedback! > > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > Hi Naush, > > > > Thank you for the patch. > > > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > > > Enumerate the sensor device entities in PipelineHandlerRPi::match() and > > loop > > > over PipelineHandlerRPi::registerCamera() for each sensor found. This > > will > > > allow the pipeline handler to register multiple cameras attached to a > > single > > > Unicam instance with a Video Mux device. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 +++++++++++-------- > > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 86851ac467ad..756878c70036 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -310,7 +310,7 @@ private: > > > return static_cast<RPiCameraData *>(camera->_d()); > > > } > > > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, > > MediaEntity *sensorEntity); > > > int queueAllBuffers(Camera *camera); > > > int prepareBuffers(Camera *camera); > > > void freeBuffers(Camera *camera); > > > @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > > *enumerator) > > > return false; > > > } > > > > > > - int ret = registerCamera(unicamDevice, ispDevice); > > > - if (ret) { > > > - LOG(RPI, Error) << "Failed to register camera: " << ret; > > > - return false; > > > + /* > > > + * The loop below is used to register multiple cameras behind one > > or more > > > + * video mux devices that are attaced to a particular Unicam > > instance. > > > > s/attaced/attached/ > > > > > + * Obviously these cameras cannot be used simultaneously. > > > > We need to expose mutual exclusion between cameras to applications > > through the libcamera public API. It doesn't have to be a blocker for > > the series, but should be done soon after, otherwise trying to use both > > cameras will result in incorrect behaviour (and possibly even crashes). > > Have you given any thought to how this could be done ? > > > > Is this not already handled in the Camera class? If I have a mux with 2 > devices > attached to a single Unicam instance, hence single pipeline handler, I > cannot > run camera in one process and camera 1 in another. Camera::acquire() would > vail to lock the pipeline handler: > > if (!d->pipe_->lock()) { > LOG(Camera, Info) > << "Pipeline handler in use by another process"; > return -EBUSY; > } > > Perhaps this long point should be turned into Error level? I get the same > result > if I try to run both cameras in a single process (e.g. with cam -c 1 -C -c > 2 -C). Hrm ... this seems like a bug somewhere at core level then. A single pipeline handler should be able to support multiple cameras, (when it is able to)... That's why we have per-camera CameraData isn't it ? But if we 'fix' that bug, we should indeed make it easy for pipeline handlers to 'specify' the access controls to their cameras... Or perhaps the intention was to have one pipeline handler per camera that can be accessed independently? After all, the matching does keep calling until the match() returns false... so it will keep retrying to keep constructing a new instance of the pipeline handler each time? > > > + */ > > > + unsigned int numCameras = 0; > > > + for (MediaEntity *entity : unicamDevice->entities()) { > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > > > + continue; > > > + > > > + int ret = registerCamera(unicamDevice, ispDevice, entity); > > > + if (ret) > > > + LOG(RPI, Error) << "Failed to register camera " > > > + << entity->name() << ": " << ret; > > > + else > > > + numCameras++; > > > > I tend to deal with error first, but that's a personal preference: > > > > int ret = registerCamera(unicamDevice, ispDevice, entity); > > if (ret) { > > LOG(RPI, Error) << "Failed to register camera " > > << entity->name() << ": " << ret; > > continue; > > } > > > > numCameras++; > > > > > } > > > > > > - return true; > > > + return !!numCameras; > > > > This looks fine, but by itself may cause a regression. Patch 2/2 will be > > merged with this one, so the problem would only occur during bisection, > > but still, I think we should squash the two patches together. > > > > I'm sure I am missing something obvious, but why would this patch in > isolation > cause a regression? The only additional function gained here would be to > allow the pipeline handler to enumerate multiple cameras attached to a mux, > but not allow one of the cameras to be run - same as before this change :-) I can't see any regression either. This looks like a refactor, code-reorganisation to me (for the better, for the purpose of this series). > Regards, > Naush > > > > > } > > > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice > > *isp) > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice > > *isp, MediaEntity *sensorEntity) > > > { > > > std::unique_ptr<RPiCameraData> data = > > std::make_unique<RPiCameraData>(this); > > > > > > @@ -1079,14 +1091,7 @@ int > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), > > &RPiCameraData::ispOutputDequeue); > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), > > &RPiCameraData::ispOutputDequeue); > > > > > > - /* Identify the sensor. */ > > > - for (MediaEntity *entity : unicam->entities()) { > > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > > > - data->sensor_ = > > std::make_unique<CameraSensor>(entity); > > > - break; > > > - } > > > - } > > > - > > > + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); > > > if (!data->sensor_) > > > return -EINVAL; > > > > > > > -- > > Regards, > > > > Laurent Pinchart > >
Hi Kieran, On Thu, 9 Dec 2021 at 09:20, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2021-12-09 08:57:52) > > Hi Laurent, > > > > Thank you for your feedback! > > > > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart < > > laurent.pinchart@ideasonboard.com> wrote: > > > > > Hi Naush, > > > > > > Thank you for the patch. > > > > > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > > > > Enumerate the sensor device entities in PipelineHandlerRPi::match() > and > > > loop > > > > over PipelineHandlerRPi::registerCamera() for each sensor found. This > > > will > > > > allow the pipeline handler to register multiple cameras attached to a > > > single > > > > Unicam instance with a Video Mux device. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 > +++++++++++-------- > > > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 86851ac467ad..756878c70036 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -310,7 +310,7 @@ private: > > > > return static_cast<RPiCameraData *>(camera->_d()); > > > > } > > > > > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, > > > MediaEntity *sensorEntity); > > > > int queueAllBuffers(Camera *camera); > > > > int prepareBuffers(Camera *camera); > > > > void freeBuffers(Camera *camera); > > > > @@ -1029,16 +1029,28 @@ bool > PipelineHandlerRPi::match(DeviceEnumerator > > > *enumerator) > > > > return false; > > > > } > > > > > > > > - int ret = registerCamera(unicamDevice, ispDevice); > > > > - if (ret) { > > > > - LOG(RPI, Error) << "Failed to register camera: " << > ret; > > > > - return false; > > > > + /* > > > > + * The loop below is used to register multiple cameras behind > one > > > or more > > > > + * video mux devices that are attaced to a particular Unicam > > > instance. > > > > > > s/attaced/attached/ > > > > > > > + * Obviously these cameras cannot be used simultaneously. > > > > > > We need to expose mutual exclusion between cameras to applications > > > through the libcamera public API. It doesn't have to be a blocker for > > > the series, but should be done soon after, otherwise trying to use both > > > cameras will result in incorrect behaviour (and possibly even crashes). > > > Have you given any thought to how this could be done ? > > > > > > > Is this not already handled in the Camera class? If I have a mux with 2 > > devices > > attached to a single Unicam instance, hence single pipeline handler, I > > cannot > > run camera in one process and camera 1 in another. Camera::acquire() > would > > vail to lock the pipeline handler: > > > > if (!d->pipe_->lock()) { > > LOG(Camera, Info) > > << "Pipeline handler in use by another process"; > > return -EBUSY; > > } > > > > Perhaps this long point should be turned into Error level? I get the same > > result > > if I try to run both cameras in a single process (e.g. with cam -c 1 -C > -c > > 2 -C). > > Hrm ... this seems like a bug somewhere at core level then. A single > pipeline handler should be able to support multiple cameras, (when it is > able to)... > Laurent did point me to a WIP branch of his that supports multi-camera setups. I have not looked at it yet, but perhaps this is fixed there. > > That's why we have per-camera CameraData isn't it ? > > But if we 'fix' that bug, we should indeed make it easy for pipeline > handlers to 'specify' the access controls to their cameras... > > > Or perhaps the intention was to have one pipeline handler per camera > that can be accessed independently? After all, the matching does keep > calling until the match() returns false... so it will keep retrying to > keep constructing a new instance of the pipeline handler each time? > On my CM4 setup, I was able to run "cam -c 1 -C -c 2 -C" and successfully stream from both sensors. So presumably that was setting up 2x pipeline handlers and working independently. This was on the master branch, not Laurent's WIP branch. I should go back and make sure that still works! Naush > > > > > + */ > > > > + unsigned int numCameras = 0; > > > > + for (MediaEntity *entity : unicamDevice->entities()) { > > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > > > > + continue; > > > > + > > > > + int ret = registerCamera(unicamDevice, ispDevice, > entity); > > > > + if (ret) > > > > + LOG(RPI, Error) << "Failed to register camera " > > > > + << entity->name() << ": " << > ret; > > > > + else > > > > + numCameras++; > > > > > > I tend to deal with error first, but that's a personal preference: > > > > > > int ret = registerCamera(unicamDevice, ispDevice, > entity); > > > if (ret) { > > > LOG(RPI, Error) << "Failed to register camera " > > > << entity->name() << ": " << > ret; > > > continue; > > > } > > > > > > numCameras++; > > > > > > > } > > > > > > > > - return true; > > > > + return !!numCameras; > > > > > > This looks fine, but by itself may cause a regression. Patch 2/2 will > be > > > merged with this one, so the problem would only occur during bisection, > > > but still, I think we should squash the two patches together. > > > > > > > I'm sure I am missing something obvious, but why would this patch in > > isolation > > cause a regression? The only additional function gained here would be to > > allow the pipeline handler to enumerate multiple cameras attached to a > mux, > > but not allow one of the cameras to be run - same as before this change > :-) > > I can't see any regression either. This looks like a refactor, > code-reorganisation to me (for the better, for the purpose of this > series). > > > Regards, > > Naush > > > > > > > > } > > > > > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, > MediaDevice > > > *isp) > > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, > MediaDevice > > > *isp, MediaEntity *sensorEntity) > > > > { > > > > std::unique_ptr<RPiCameraData> data = > > > std::make_unique<RPiCameraData>(this); > > > > > > > > @@ -1079,14 +1091,7 @@ int > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice > *isp) > > > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), > > > &RPiCameraData::ispOutputDequeue); > > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), > > > &RPiCameraData::ispOutputDequeue); > > > > > > > > - /* Identify the sensor. */ > > > > - for (MediaEntity *entity : unicam->entities()) { > > > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > > > > - data->sensor_ = > > > std::make_unique<CameraSensor>(entity); > > > > - break; > > > > - } > > > > - } > > > > - > > > > + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); > > > > if (!data->sensor_) > > > > return -EINVAL; > > > > > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > >
Hello, On Thu, Dec 09, 2021 at 09:27:37AM +0000, Naushir Patuck wrote: > On Thu, 9 Dec 2021 at 09:20, Kieran Bingham wrote: > > Quoting Naushir Patuck (2021-12-09 08:57:52) > > > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart wrote: > > > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > > > > > Enumerate the sensor device entities in PipelineHandlerRPi::match() and loop > > > > > over PipelineHandlerRPi::registerCamera() for each sensor found. This will > > > > > allow the pipeline handler to register multiple cameras attached to a single > > > > > Unicam instance with a Video Mux device. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > --- > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 +++++++++++-------- > > > > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index 86851ac467ad..756878c70036 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -310,7 +310,7 @@ private: > > > > > return static_cast<RPiCameraData *>(camera->_d()); > > > > > } > > > > > > > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > > > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity); > > > > > int queueAllBuffers(Camera *camera); > > > > > int prepareBuffers(Camera *camera); > > > > > void freeBuffers(Camera *camera); > > > > > @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > > > return false; > > > > > } > > > > > > > > > > - int ret = registerCamera(unicamDevice, ispDevice); > > > > > - if (ret) { > > > > > - LOG(RPI, Error) << "Failed to register camera: " << ret; > > > > > - return false; > > > > > + /* > > > > > + * The loop below is used to register multiple cameras behind one or more > > > > > + * video mux devices that are attaced to a particular Unicam instance. > > > > > > > > s/attaced/attached/ > > > > > > > > > + * Obviously these cameras cannot be used simultaneously. > > > > > > > > We need to expose mutual exclusion between cameras to applications > > > > through the libcamera public API. It doesn't have to be a blocker for > > > > the series, but should be done soon after, otherwise trying to use both > > > > cameras will result in incorrect behaviour (and possibly even crashes). > > > > Have you given any thought to how this could be done ? > > > > > > Is this not already handled in the Camera class? If I have a mux with 2 devices > > > attached to a single Unicam instance, hence single pipeline handler, I cannot > > > run camera in one process and camera 1 in another. Camera::acquire() would > > > vail to lock the pipeline handler: > > > > > > if (!d->pipe_->lock()) { > > > LOG(Camera, Info) > > > << "Pipeline handler in use by another process"; > > > return -EBUSY; > > > } > > > > > > Perhaps this long point should be turned into Error level? I get the same result > > > if I try to run both cameras in a single process (e.g. with cam -c 1 -C -c 2 -C). > > > > Hrm ... this seems like a bug somewhere at core level then. A single > > pipeline handler should be able to support multiple cameras, (when it is > > able to)... > > Laurent did point me to a WIP branch of his that supports multi-camera setups. > I have not looked at it yet, but perhaps this is fixed there. That's right. There's work in progress to suppose usage of multiple cameras from the same pipeline handler. It will unlock valid use cases, but will require pipeline handlers that don't support concurrent usage of cameras to implement mutual exclusion. > > That's why we have per-camera CameraData isn't it ? > > > > But if we 'fix' that bug, we should indeed make it easy for pipeline > > handlers to 'specify' the access controls to their cameras... > > > > Or perhaps the intention was to have one pipeline handler per camera > > that can be accessed independently? After all, the matching does keep > > calling until the match() returns false... so it will keep retrying to > > keep constructing a new instance of the pipeline handler each time? > > On my CM4 setup, I was able to run "cam -c 1 -C -c 2 -C" and successfully > stream from both sensors. So presumably that was setting up 2x pipeline > handlers and working independently. This was on the master branch, not > Laurent's WIP branch. I should go back and make sure that still works! Correct, with one pipeline handler instance per camera, that will work nicely out of the box. It's one of the perks of implementing support for multiple Unicam instances through multiple pipeline handler instances. The situation becomes more complex with a single pipeline handler instance that registers multiple cameras that can still be used concurrently. > > > > > + */ > > > > > + unsigned int numCameras = 0; > > > > > + for (MediaEntity *entity : unicamDevice->entities()) { > > > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > > > > > + continue; > > > > > + > > > > > + int ret = registerCamera(unicamDevice, ispDevice, entity); > > > > > + if (ret) > > > > > + LOG(RPI, Error) << "Failed to register camera " > > > > > + << entity->name() << ": " << ret; > > > > > + else > > > > > + numCameras++; > > > > > > > > I tend to deal with error first, but that's a personal preference: > > > > > > > > int ret = registerCamera(unicamDevice, ispDevice, entity); > > > > if (ret) { > > > > LOG(RPI, Error) << "Failed to register camera " > > > > << entity->name() << ": " << ret; > > > > continue; > > > > } > > > > > > > > numCameras++; > > > > > > > > > } > > > > > > > > > > - return true; > > > > > + return !!numCameras; > > > > > > > > This looks fine, but by itself may cause a regression. Patch 2/2 will be > > > > merged with this one, so the problem would only occur during bisection, > > > > but still, I think we should squash the two patches together. > > > > > > I'm sure I am missing something obvious, but why would this patch in isolation > > > cause a regression? The only additional function gained here would be to > > > allow the pipeline handler to enumerate multiple cameras attached to a mux, > > > but not allow one of the cameras to be run - same as before this change :-) > > > > I can't see any regression either. This looks like a refactor, > > code-reorganisation to me (for the better, for the purpose of this > > series). The video nodes will be opened once per camera sensor, which shouldn't fail. setFormat() is called in configure() only, so as long as the user doesn't try to use the second camera, it should be alright. Maybe you could give it a try ? My concern is that this series may introduce some subtle breakages, and someone bisecting the issue may try to run this patch alone without 2/2. > > > > > } > > > > > > > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > > > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity) > > > > > { > > > > > std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this); > > > > > > > > > > @@ -1079,14 +1091,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > > > > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > > > > > > > > > - /* Identify the sensor. */ > > > > > - for (MediaEntity *entity : unicam->entities()) { > > > > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > > > > > - data->sensor_ = std::make_unique<CameraSensor>(entity); > > > > > - break; > > > > > - } > > > > > - } > > > > > - > > > > > + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); > > > > > if (!data->sensor_) > > > > > return -EINVAL; > > > > >
Hi Laurent, On Thu, 9 Dec 2021 at 23:48, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Thu, Dec 09, 2021 at 09:27:37AM +0000, Naushir Patuck wrote: > > On Thu, 9 Dec 2021 at 09:20, Kieran Bingham wrote: > > > Quoting Naushir Patuck (2021-12-09 08:57:52) > > > > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart wrote: > > > > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote: > > > > > > Enumerate the sensor device entities in > PipelineHandlerRPi::match() and loop > > > > > > over PipelineHandlerRPi::registerCamera() for each sensor found. > This will > > > > > > allow the pipeline handler to register multiple cameras attached > to a single > > > > > > Unicam instance with a Video Mux device. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 > +++++++++++-------- > > > > > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > index 86851ac467ad..756878c70036 100644 > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > @@ -310,7 +310,7 @@ private: > > > > > > return static_cast<RPiCameraData *>(camera->_d()); > > > > > > } > > > > > > > > > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp); > > > > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp, > MediaEntity *sensorEntity); > > > > > > int queueAllBuffers(Camera *camera); > > > > > > int prepareBuffers(Camera *camera); > > > > > > void freeBuffers(Camera *camera); > > > > > > @@ -1029,16 +1029,28 @@ bool > PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > > > > return false; > > > > > > } > > > > > > > > > > > > - int ret = registerCamera(unicamDevice, ispDevice); > > > > > > - if (ret) { > > > > > > - LOG(RPI, Error) << "Failed to register camera: " > << ret; > > > > > > - return false; > > > > > > + /* > > > > > > + * The loop below is used to register multiple cameras > behind one or more > > > > > > + * video mux devices that are attaced to a particular > Unicam instance. > > > > > > > > > > s/attaced/attached/ > > > > > > > > > > > + * Obviously these cameras cannot be used simultaneously. > > > > > > > > > > We need to expose mutual exclusion between cameras to applications > > > > > through the libcamera public API. It doesn't have to be a blocker > for > > > > > the series, but should be done soon after, otherwise trying to use > both > > > > > cameras will result in incorrect behaviour (and possibly even > crashes). > > > > > Have you given any thought to how this could be done ? > > > > > > > > Is this not already handled in the Camera class? If I have a mux > with 2 devices > > > > attached to a single Unicam instance, hence single pipeline handler, > I cannot > > > > run camera in one process and camera 1 in another. > Camera::acquire() would > > > > vail to lock the pipeline handler: > > > > > > > > if (!d->pipe_->lock()) { > > > > LOG(Camera, Info) > > > > << "Pipeline handler in use by another process"; > > > > return -EBUSY; > > > > } > > > > > > > > Perhaps this long point should be turned into Error level? I get the > same result > > > > if I try to run both cameras in a single process (e.g. with cam -c 1 > -C -c 2 -C). > > > > > > Hrm ... this seems like a bug somewhere at core level then. A single > > > pipeline handler should be able to support multiple cameras, (when it > is > > > able to)... > > > > Laurent did point me to a WIP branch of his that supports multi-camera > setups. > > I have not looked at it yet, but perhaps this is fixed there. > > That's right. There's work in progress to suppose usage of multiple > cameras from the same pipeline handler. It will unlock valid use cases, > but will require pipeline handlers that don't support concurrent usage > of cameras to implement mutual exclusion. > > > > That's why we have per-camera CameraData isn't it ? > > > > > > But if we 'fix' that bug, we should indeed make it easy for pipeline > > > handlers to 'specify' the access controls to their cameras... > > > > > > Or perhaps the intention was to have one pipeline handler per camera > > > that can be accessed independently? After all, the matching does keep > > > calling until the match() returns false... so it will keep retrying to > > > keep constructing a new instance of the pipeline handler each time? > > > > On my CM4 setup, I was able to run "cam -c 1 -C -c 2 -C" and successfully > > stream from both sensors. So presumably that was setting up 2x pipeline > > handlers and working independently. This was on the master branch, not > > Laurent's WIP branch. I should go back and make sure that still works! > > Correct, with one pipeline handler instance per camera, that will work > nicely out of the box. It's one of the perks of implementing support for > multiple Unicam instances through multiple pipeline handler instances. > > The situation becomes more complex with a single pipeline handler > instance that registers multiple cameras that can still be used > concurrently. > > > > > > > + */ > > > > > > + unsigned int numCameras = 0; > > > > > > + for (MediaEntity *entity : unicamDevice->entities()) { > > > > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) > > > > > > + continue; > > > > > > + > > > > > > + int ret = registerCamera(unicamDevice, ispDevice, > entity); > > > > > > + if (ret) > > > > > > + LOG(RPI, Error) << "Failed to register > camera " > > > > > > + << entity->name() << ": " > << ret; > > > > > > + else > > > > > > + numCameras++; > > > > > > > > > > I tend to deal with error first, but that's a personal preference: > > > > > > > > > > int ret = registerCamera(unicamDevice, ispDevice, > entity); > > > > > if (ret) { > > > > > LOG(RPI, Error) << "Failed to register > camera " > > > > > << entity->name() << ": " > << ret; > > > > > continue; > > > > > } > > > > > > > > > > numCameras++; > > > > > > > > > > > } > > > > > > > > > > > > - return true; > > > > > > + return !!numCameras; > > > > > > > > > > This looks fine, but by itself may cause a regression. Patch 2/2 > will be > > > > > merged with this one, so the problem would only occur during > bisection, > > > > > but still, I think we should squash the two patches together. > > > > > > > > I'm sure I am missing something obvious, but why would this patch in > isolation > > > > cause a regression? The only additional function gained here would > be to > > > > allow the pipeline handler to enumerate multiple cameras attached to > a mux, > > > > but not allow one of the cameras to be run - same as before this > change :-) > > > > > > I can't see any regression either. This looks like a refactor, > > > code-reorganisation to me (for the better, for the purpose of this > > > series). > > The video nodes will be opened once per camera sensor, which shouldn't > fail. setFormat() is called in configure() only, so as long as the user > doesn't try to use the second camera, it should be alright. > > Maybe you could give it a try ? My concern is that this series may > introduce some subtle breakages, and someone bisecting the issue may try > to run this patch alone without 2/2. > I give this a quick try and it seemed to be fine: - Run "cam -c 1 -C" in one terminal. Camera starts streaming as expected. - Run "cam -c 2 -C" in a second terminal. Camera fails in Camera::acquire() as the pipeline handler is locked. This should be the expected safe behaviour, correct? Naush > > > > > > > } > > > > > > > > > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, > MediaDevice *isp) > > > > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, > MediaDevice *isp, MediaEntity *sensorEntity) > > > > > > { > > > > > > std::unique_ptr<RPiCameraData> data = > std::make_unique<RPiCameraData>(this); > > > > > > > > > > > > @@ -1079,14 +1091,7 @@ int > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) > > > > > > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > > > > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispOutputDequeue); > > > > > > > > > > > > - /* Identify the sensor. */ > > > > > > - for (MediaEntity *entity : unicam->entities()) { > > > > > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > > > > > > - data->sensor_ = > std::make_unique<CameraSensor>(entity); > > > > > > - break; > > > > > > - } > > > > > > - } > > > > > > - > > > > > > + data->sensor_ = > std::make_unique<CameraSensor>(sensorEntity); > > > > > > if (!data->sensor_) > > > > > > return -EINVAL; > > > > > > > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 86851ac467ad..756878c70036 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -310,7 +310,7 @@ private: return static_cast<RPiCameraData *>(camera->_d()); } - int registerCamera(MediaDevice *unicam, MediaDevice *isp); + int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity); int queueAllBuffers(Camera *camera); int prepareBuffers(Camera *camera); void freeBuffers(Camera *camera); @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) return false; } - int ret = registerCamera(unicamDevice, ispDevice); - if (ret) { - LOG(RPI, Error) << "Failed to register camera: " << ret; - return false; + /* + * The loop below is used to register multiple cameras behind one or more + * video mux devices that are attaced to a particular Unicam instance. + * Obviously these cameras cannot be used simultaneously. + */ + unsigned int numCameras = 0; + for (MediaEntity *entity : unicamDevice->entities()) { + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR) + continue; + + int ret = registerCamera(unicamDevice, ispDevice, entity); + if (ret) + LOG(RPI, Error) << "Failed to register camera " + << entity->name() << ": " << ret; + else + numCameras++; } - return true; + return !!numCameras; } -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity) { std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this); @@ -1079,14 +1091,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp) data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); - /* Identify the sensor. */ - for (MediaEntity *entity : unicam->entities()) { - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { - data->sensor_ = std::make_unique<CameraSensor>(entity); - break; - } - } - + data->sensor_ = std::make_unique<CameraSensor>(sensorEntity); if (!data->sensor_) return -EINVAL;