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