[libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Account for a missing Unicam embedded data node

[Laurent Pinchart]

Hi Naush,

On Tue, Oct 26, 2021 at 08:53:17AM +0100, Naushir Patuck wrote:
> On Mon, 25 Oct 2021 at 18:04, Laurent Pinchart wrote:
> > On Fri, Oct 22, 2021 at 03:39:06PM +0100, Naushir Patuck wrote:
> > > The unicam driver no longer regesters an embedded data node if the sensor does
> >
> > s/regesters/registers/
> >
> > > not provide this stream. Account for this in the pipeline handler match routine
> > > by not assuming it is always present.
> > >
> > > Add a warning if Unicam and the CamHelper do not agree on the presense of sensor
> > > embedded data, and disable its useage in these cases.
> >
> > s/useage/usage/
> >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Review-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 2b70b877e70a..c5e9607c7d95 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -984,7 +984,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >       DeviceMatch unicam("unicam");
> > >       DeviceMatch isp("bcm2835-isp");
> > >
> > > -     unicam.add("unicam-embedded");
> > >       unicam.add("unicam-image");
> > >
> > >       isp.add("bcm2835-isp0-output0"); /* Input */
> > > @@ -1005,9 +1004,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >               return false;
> > >
> > >       /* Locate and open the unicam video streams. */
> > > -     data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> > >       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> > >
> > > +     /* 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);
> > > +  data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
> > > + &RPiCameraData::unicamBufferDequeue);
> >
> > It would be nice to keep signal connection uniform, but that's no big
> > deal.
> 
> Not sure I understand here.  If the embedded node is absent in cases where the sensor
> does not support it, data->unicam_[Unicam::Embedded].dev() will not be valid, so cannot
> connect to anything?

I meant that it would have been nice to keep the signal connection with
all the other ones below, but it's really not a big deal, just an
OCD-triggered comment :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +     }
> > > +
> > >       /* 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"));
> > > @@ -1017,7 +1023,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >       /* Wire up all the buffer connections. */
> > >       data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> > >  data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);
> > > -  data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);
> > >       data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);
> > >       data->isp_[Isp::Output0].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
> > >       data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
> > > @@ -1045,6 +1050,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >               return false;
> > >       }
> > >
> > > +     if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> > > +             LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
> > > +             sensorConfig.sensorMetadata = false;
> > > +     }
> > > +
> > >       /*
> > >        * Open all Unicam and ISP streams. The exception is the embedded data
> > >        * stream, which only gets opened below if the IPA reports that the sensor

-- 
Regards,

Laurent Pinchart