[libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi: Conditionally open the embedded data node

[Naushir Patuck]

Hi Laurent,


On Tue, 23 Mar 2021 at 13:06, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Mar 23, 2021 at 12:27:35PM +0000, Naushir Patuck wrote:
> > Conditionally open the embedded data node in pipeline_handler::match()
> > based on whether the ipa::init() result reports if the sensor supports
> > embedded data or not.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Tested-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 +++++++------------
> >  1 file changed, 21 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index ce1994186d66..4a3f7cbe3065 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -138,7 +138,7 @@ class RPiCameraData : public CameraData
> >  {
> >  public:
> >       RPiCameraData(PipelineHandler *pipe)
> > -             : CameraData(pipe), embeddedNodeOpened_(false),
> state_(State::Stopped),
> > +             : CameraData(pipe), state_(State::Stopped),
> >                 supportsFlips_(false), flipsAlterBayerOrder_(false),
> >                 updateScalerCrop_(true), dropFrameCount_(0),
> ispOutputCount_(0)
> >       {
> > @@ -183,7 +183,6 @@ public:
> >
> >       std::unique_ptr<DelayedControls> delayedCtrls_;
> >       bool sensorMetadata_;
> > -     bool embeddedNodeOpened_;
> >
> >       /*
> >        * All the functions in this class are called from a single calling
> > @@ -749,19 +748,13 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> >
> >       /*
> > -      * The IPA will set data->sensorMetadata_ to true if embedded data
> is
> > -      * supported on this sensor. If so, open the Unicam embedded data
> > -      * node and configure the output format.
> > +      * Configure the Unicam embedded data output format only if the
> sensor
> > +      * supports it.
> >        */
> >       if (data->sensorMetadata_) {
> >               format = {};
> >               format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);
> >
> > -             if (!data->embeddedNodeOpened_) {
> > -                     data->unicam_[Unicam::Embedded].dev()->open();
> > -                     data->embeddedNodeOpened_ = true;
> > -             }
> > -
> >               LOG(RPI, Debug) << "Setting embedded data format.";
> >               ret =
> data->unicam_[Unicam::Embedded].dev()->setFormat(&format);
> >               if (ret) {
> > @@ -778,14 +771,6 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                */
> >               if (rawStream)
> >                       data->unicam_[Unicam::Embedded].setExternal(true);
> > -     } else {
> > -             /*
> > -              * No embedded data present, so we do not want to iterate
> over
> > -              * the embedded data stream when starting and stopping.
> > -              */
> > -             data->streams_.erase(std::remove(data->streams_.begin(),
> data->streams_.end(),
> > -
> &data->unicam_[Unicam::Embedded]),
> > -                                  data->streams_.end());
> >       }
> >
> >       /*
> > @@ -989,24 +974,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator
> *enumerator)
> >       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"));
> >
> > -     /* This is just for convenience so that we can easily iterate over
> all streams. */
> > -     for (auto &stream : data->unicam_)
> > -             data->streams_.push_back(&stream);
> > -     for (auto &stream : data->isp_)
> > -             data->streams_.push_back(&stream);
> > -
> > -     /*
> > -      * Open all Unicam and ISP streams. The exception is the embedded
> data
> > -      * stream, which only gets opened if the IPA reports that the
> sensor
> > -      * supports embedded data. This happens in
> RPiCameraData::configureIPA().
> > -      */
> > -     for (auto const stream : data->streams_) {
> > -             if (stream != &data->unicam_[Unicam::Embedded]) {
> > -                     if (stream->dev()->open())
> > -                             return false;
> > -             }
> > -     }
> > -
> >       /* 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);
> > @@ -1036,6 +1003,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator
> *enumerator)
> >               return 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
> > +      * supports embedded data.
> > +      *
> > +      * The below grouping is just for convenience so that we can easily
> > +      * iterate over all streams in one go.
> > +      */
> > +     data->streams_.push_back(&data->unicam_[Unicam::Image]);
> > +     if (sensorConfig.sensorMetadata)
> > +             data->streams_.push_back(&data->unicam_[Unicam::Embedded]);
> > +
> > +     for (auto &stream : data->isp_) {
> > +             data->streams_.push_back(&stream);
> > +             if (stream.dev()->open())
> > +                     return false;
> > +     }
>
> Unless I'm mistaken, you're not opening the unicam devices anymore.
> That's why I had too loops in my proposal:
>
>         for (auto &stream : data->isp_)
>                 data->streams_.push_back(&stream);
>
>         for (auto stream : data->streams_) {
>                 if (stream->dev()->open())
>                         return false;
>         }
>
> If that's indeed the case, could you make sure to test the next version
> ? :-)
>


Sorry :-(
I did hit this error and have the correct change locally but did not amend
the commit!
Will submit an update shortly.





>
> > +
> >       /*
> >        * Setup our delayed control writer with the sensor default
> >        * gain and exposure delays. Mark VBLANK for priority write.
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210323/dbb668f9/attachment.htm>