[libcamera-devel] [PATCH v3 2/2] pipeline: raspberrypi: Tidy the camera enumeration and registration logic
Naushir Patuck
naush at raspberrypi.com
Thu Nov 25 11:02:30 CET 2021
On Thu, 25 Nov 2021 at 09:58, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Quoting Naushir Patuck (2021-11-25 09:49:31)
> > Hi Laurent,
> >
> > Thank you for your feedback.
> >
> > On Wed, 24 Nov 2021 at 23:16, Laurent Pinchart <
> > laurent.pinchart at ideasonboard.com> wrote:
> >
> > > Hi Naush,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Nov 23, 2021 at 01:10:40PM +0000, Naushir Patuck wrote:
> > > > When acquiring the media device, it is not necessary to match all
> entity
> > > names,
> > > > so remove it. Aditionally, we do not need to keep the MediaEntity
> > > pointers for
> > > > the Unicam and ISP devices stored within the PipelineHandlerRPi
> class.
> > > Instead
> > > > these can be stored locally in PipelineHandlerRPi::match().
> > > >
> > > > PipelineHandlerRPi::registerCamera() now returns an int error code
> > > instead of a
> > > > boolean for pass/fail.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 88
> +++++++++++--------
> > > > 1 file changed, 52 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 12fd38061241..c5034480820e 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -311,14 +311,11 @@ private:
> > > > return static_cast<RPiCameraData *>(camera->_d());
> > > > }
> > > >
> > > > - bool registerCamera();
> > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp);
> > > > int queueAllBuffers(Camera *camera);
> > > > int prepareBuffers(Camera *camera);
> > > > void freeBuffers(Camera *camera);
> > > > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers,
> > > unsigned int mask);
> > > > -
> > > > - MediaDevice *unicam_;
> > > > - MediaDevice *isp_;
> > > > };
> > > >
> > > > RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData
> > > *data)
> > > > @@ -509,7 +506,7 @@ CameraConfiguration::Status
> > > RPiCameraConfiguration::validate()
> > > > }
> > > >
> > > > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > > > - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> > > > + : PipelineHandler(manager)
> > > > {
> > > > }
> > > >
> > > > @@ -993,49 +990,65 @@ int
> PipelineHandlerRPi::queueRequestDevice(Camera
> > > *camera, Request *request)
> > > >
> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > > > {
> > > > + MediaDevice *unicamDevice, *ispDevice;
> > > > +
> > > > DeviceMatch unicam("unicam");
> > > > - DeviceMatch isp("bcm2835-isp");
> > > > + unicamDevice = acquireMediaDevice(enumerator, unicam);
> > >
> > > You can declare the variable here. Same for ispDevice below.
> > >
> >
> > Ack.
> >
> >
> > >
> > > >
> > > > - unicam.add("unicam-image");
> > > > + if (!unicamDevice) {
> > > > + LOG(RPI, Debug) << "Unable to acquire a Unicam
> instance";
> > > > + return false;
> > > > + }
> > > >
> > > > - isp.add("bcm2835-isp0-output0"); /* Input */
> > > > - isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> > > > - isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> > > > - isp.add("bcm2835-isp0-capture3"); /* Stats */
> > > > + DeviceMatch isp("bcm2835-isp");
> > > > + ispDevice = acquireMediaDevice(enumerator, isp);
> > > >
> > > > - unicam_ = acquireMediaDevice(enumerator, unicam);
> > > > - if (!unicam_)
> > > > + if (!ispDevice) {
> > > > + LOG(RPI, Debug) << "Unable to acquire ISP instance";
> > > > return false;
> > > > + }
> > > >
> > > > - isp_ = acquireMediaDevice(enumerator, isp);
> > > > - if (!isp_)
> > > > + int ret = registerCamera(unicamDevice, ispDevice);
> > > > + if (ret) {
> > > > + LOG(RPI, Error) << "Failed to register camera: " <<
> ret;
> > > > return false;
> > > > + }
> > > >
> > > > - return registerCamera();
> > > > + return true;
> > > > }
> > > >
> > > > -bool PipelineHandlerRPi::registerCamera()
> > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam,
> MediaDevice
> > > *isp)
> > > > {
> > > > std::unique_ptr<RPiCameraData> data =
> > > std::make_unique<RPiCameraData>(this);
> > > > +
> > > > if (!data->dmaHeap_.isValid())
> > > > - return false;
> > > > + return -ENOMEM;
> > > > +
> > > > + MediaEntity *unicamImage =
> unicam->getEntityByName("unicam-image");
> > > > + MediaEntity *ispOutput0 =
> > > isp->getEntityByName("bcm2835-isp0-output0");
> > > > + MediaEntity *ispCapture1 =
> > > isp->getEntityByName("bcm2835-isp0-capture1");
> > > > + MediaEntity *ispCapture2 =
> > > isp->getEntityByName("bcm2835-isp0-capture2");
> > > > + MediaEntity *ispCapture3 =
> > > isp->getEntityByName("bcm2835-isp0-capture3");
> > > > +
> > > > + if (!unicamImage || !ispOutput0 || !ispCapture1 ||
> !ispCapture2 ||
> > > !ispCapture3)
> > > > + return -ENOENT;
> > > >
> > > > /* Locate and open the unicam video streams. */
> > > > - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> > > unicam_->getEntityByName("unicam-image"));
> > > > + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> > > unicamImage);
> > > >
> > > > /* An embedded data node will not be present if the sensor does
> > > not support it. */
> > > > - MediaEntity *embeddedEntity =
> > > unicam_->getEntityByName("unicam-embedded");
> > > > - if (embeddedEntity) {
> > > > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam
> > > Embedded", embeddedEntity);
> > > > + MediaEntity *unicamEmbedded =
> > > unicam->getEntityByName("unicam-embedded");
> > > > + if (unicamEmbedded) {
> > > > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam
> > > Embedded", unicamEmbedded);
> > > >
> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
> > > >
> > > &RPiCameraData::unicamBufferDequeue);
> > > > }
> > > >
> > > > /* Tag the ISP input stream as an import stream. */
> > > > - data->isp_[Isp::Input] = RPi::Stream("ISP Input",
> > > isp_->getEntityByName("bcm2835-isp0-output0"), true);
> > > > - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0",
> > > isp_->getEntityByName("bcm2835-isp0-capture1"));
> > > > - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1",
> > > isp_->getEntityByName("bcm2835-isp0-capture2"));
> > > > - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats",
> > > isp_->getEntityByName("bcm2835-isp0-capture3"));
> > > > + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0,
> > > true);
> > > > + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0",
> ispCapture1);
> > > > + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1",
> ispCapture2);
> > > > + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> > > >
> > > > /* Wire up all the buffer connections. */
> > > >
> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),
> > > &RPiCameraData::frameStarted);
> > > > @@ -1046,7 +1059,7 @@ bool PipelineHandlerRPi::registerCamera()
> > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),
> > > &RPiCameraData::ispOutputDequeue);
> > > >
> > > > /* Identify the sensor. */
> > > > - for (MediaEntity *entity : unicam_->entities()) {
> > > > + for (MediaEntity *entity : unicam->entities()) {
> > > > if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
> > > > data->sensor_ =
> > > std::make_unique<CameraSensor>(entity);
> > > > break;
> > > > @@ -1054,23 +1067,23 @@ bool PipelineHandlerRPi::registerCamera()
> > > > }
> > > >
> > > > if (!data->sensor_)
> > > > - return false;
> > > > + return -EINVAL;
> > > >
> > > > if (data->sensor_->init())
> > > > - return false;
> > > > + return -EINVAL;
> > > >
> > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);
> > > >
> > > > ipa::RPi::SensorConfig sensorConfig;
> > > > if (data->loadIPA(&sensorConfig)) {
> > > > LOG(RPI, Error) << "Failed to load a suitable IPA
> library";
> > > > - return false;
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> > > > + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
> > > > LOG(RPI, Warning) << "Mismatch between Unicam and
> > > CamHelper for embedded data usage!";
> > > > sensorConfig.sensorMetadata = false;
> > > > - if (embeddedEntity)
> > > > + if (unicamEmbedded)
> > > >
> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
> > > > }
> > > >
> > > > @@ -1091,12 +1104,12 @@ bool PipelineHandlerRPi::registerCamera()
> > > >
> > > > for (auto stream : data->streams_) {
> > > > if (stream->dev()->open())
> > > > - return false;
> > > > + continue;
> > >
> > > Is this correct ? If some of the devices can't be opened, shouldn't it
> > > be a fatal error ?
> > >
> >
>
> By 'fatal' I presume you mean - will prevent registration of this
> camera, not 'will prevent libcamera from continuing' ?
>
> It could be that the 'other' camera works fine, and that's what the user
> wants to use ?
>
> But it should certainly be loud about the error in opening...
>
An open() failure here ought to return early and cause the registration to
fail. But other cameras might work fine, so it's not "fatal" in that
respect.
>
> > Good catch, yes it should!
> > I'll fix this up for the next version.
> >
> > Regards,
> > Naush
> >
> >
> >
> > >
> > > > }
> > > >
> > > > if
> > > (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
> > > > LOG(RPI, Error) << "Unicam driver does not use the
> > > MediaController, please update your kernel!";
> > > > - return false;
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > /*
> > > > @@ -1158,7 +1171,7 @@ bool PipelineHandlerRPi::registerCamera()
> > > >
> > > > if (!bayerFormat.isValid()) {
> > > > LOG(RPI, Error) << "No Bayer format found";
> > > > - return false;
> > > > + return -EINVAL;
> > > > }
> > > > data->nativeBayerOrder_ = bayerFormat.order;
> > > >
> > > > @@ -1178,7 +1191,10 @@ bool PipelineHandlerRPi::registerCamera()
> > > > Camera::create(std::move(data), id, streams);
> > > > PipelineHandler::registerCamera(std::move(camera));
> > > >
> > > > - return true;
> > > > + LOG(RPI, Info) << "Registered camera " << id
> > > > + << " to Unicam device " << unicam->deviceNode()
> > > > + << " and ISP device " << isp->deviceNode();
> > > > + return 0;
> > > > }
> > > >
> > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211125/43a07da0/attachment-0001.htm>
More information about the libcamera-devel
mailing list