[libcamera-devel] [PATCH v3 10/10] libcamera: pipeline: raspberrypi: Handle any externally allocated FrameBuffer
Naushir Patuck
naush at raspberrypi.com
Mon Jul 20 10:06:21 CEST 2020
Hi Niklas,
Thank you for the review comments, for this and all the patches in the series.
On Sat, 18 Jul 2020 at 17:04, Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> Hi Naushir,
>
> Thanks for your work.
>
> On 2020-07-17 09:54:10 +0100, Naushir Patuck wrote:
> > Handle the case where a FrameBuffer that has been externally allocated
> > (i.e. not through the v4l2 video device) is passed into a Request.
> >
> > We must store the buffer pointer in our internal buffer list to identify
> > when used, as well as mmap the buffer in the IPA if needed.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 94 +++++++++++--------
> > .../pipeline/raspberrypi/rpi_stream.cpp | 5 +
> > .../pipeline/raspberrypi/rpi_stream.h | 1 +
> > 3 files changed, 63 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index e400c10c..99896eee 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -171,8 +171,8 @@ public:
> > RPi::RPiDevice<Isp, 4> isp_;
> > /* The vector below is just for convenience when iterating over all streams. */
> > std::vector<RPi::RPiStream *> streams_;
> > - /* Buffers passed to the IPA. */
> > - std::vector<IPABuffer> ipaBuffers_;
> > + /* Stores the ids of the buffers mapped in the IPA. */
> > + std::vector<unsigned int> ipaBufferIds_;
> >
> > /* VCSM allocation helper. */
> > ::RPi::Vcsm vcsm_;
> > @@ -192,7 +192,6 @@ public:
> > std::queue<FrameBuffer *> bayerQueue_;
> > std::queue<FrameBuffer *> embeddedQueue_;
> > std::deque<Request *> requestQueue_;
> > -
> > unsigned int dropFrameCount_;
> >
> > private:
> > @@ -243,6 +242,8 @@ private:
> > int queueAllBuffers(Camera *camera);
> > int prepareBuffers(Camera *camera);
> > void freeBuffers(Camera *camera);
> > + void mapBuffers(Camera *camera, const std::vector<FrameBuffer *> &buffers,
> > + unsigned int startId);
> >
> > MediaDevice *unicam_;
> > MediaDevice *isp_;
> > @@ -736,20 +737,38 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >
> > /* Push all buffers supplied in the Request to the respective streams. */
> > for (auto stream : data->streams_) {
> > - if (stream->isExternal()) {
> > - FrameBuffer *buffer = request->findBuffer(stream);
> > + if (!stream->isExternal())
> > + continue;
> > +
> > + FrameBuffer *buffer = request->findBuffer(stream);
> > + if (buffer && stream->getBufferIndex(buffer) == -1) {
> > /*
> > - * A nullptr in buffer means that we should queue an internally
> > - * allocated buffer.
> > - *
> > - * The below queueBuffer() call will do nothing if there are not
> > - * enough internal buffers allocated, but this will be handled by
> > - * queuing the request for buffers in the RPiStream object.
> > + * This buffer is not recognised, so it must have been allocated
> > + * outside the v4l2 device. Store it in the stream buffer list
> > + * so we can track it.
> > */
> > - int ret = stream->queueBuffer(buffer);
> > - if (ret)
> > - return ret;
> > + stream->setExternalBuffer(buffer);
>
> There is noting wrong tracking external buffers in this way but I feel
> it's ill prepared for the scenario where a new external buffer is used
> each time. As the tracking is not undone when foreign buffer buffer
> leaves the pipeline handler, perhaps never to be seen again.
That is a very good point. I only assumed that the external buffers
would be continually recycled, but that is not necessarily the case.
I'm going to have to think about this one a bit more - it is quite an
awkward case to deal with in many ways. I think what I might do is
remove this patch from the set, resubmit with your suggestions for the
other patches, and work on this one separately. Given that no apps
currently do this, I think it should be ok to ignore this for now :-)
Regards,
Naush
>
> > +
> > + /* Also get the IPA to mmap it if needed. */
> > + if (stream == &data->unicam_[Unicam::Embedded]) {
> > + mapBuffers(camera, { buffer },
> > + RPiIpaMask::EMBEDDED_DATA | (stream->getBuffers().size() - 1));
> > + } else if (stream == &data->isp_[Isp::Stats]) {
> > + mapBuffers(camera, { buffer },
> > + RPiIpaMask::STATS | (stream->getBuffers().size() - 1));
> > + }
>
> Same argument here as above but with a hight cost. What if the buffers
> mapped here are never reused and a new buffer is attached to each new
> request? Maybe some form of time-to-live would be a middle ground for
> efficiency between always mapping a foreign buffer when it enters the
> pipeline and unmap it when it exits?
>
> > }
> > + /*
> > + * A nullptr in buffer means that we should queue an internally
> > + * allocated buffer.
> > + *
> > + * The below queueBuffer() call will do nothing if there are not
> > + * enough internal buffers allocated, but this will be handled by
> > + * queuing the request for buffers in the RPiStream object.
> > + */
> > + int ret = stream->queueBuffer(buffer);
> > + if (ret)
> > + return ret;
> > }
> >
> > /* Push the request to the back of the queue. */
> > @@ -888,7 +907,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > {
> > RPiCameraData *data = cameraData(camera);
> > - unsigned int index;
> > int ret;
> >
> > /*
> > @@ -908,42 +926,44 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > return ret;
> > }
> >
> > + /*
> > + * Pass the stats and embedded data buffers to the IPA. No other
> > + * buffers need to be passed.
> > + */
> > + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);
> > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);
> > +
> > + return 0;
> > +}
> > +
> > +void PipelineHandlerRPi::mapBuffers(Camera *camera, const std::vector<FrameBuffer *> &buffers,
> > + unsigned int startId)
> > +{
> > + RPiCameraData *data = cameraData(camera);
> > + std::vector<IPABuffer> ipaBuffers;
> > + unsigned int id = startId;
> > /*
> > * Link the FrameBuffers with the index of their position in the vector
> > - * stored in the RPi stream object.
> > + * stored in the RPi stream object - along with an identifer mask.
> > *
> > * This will allow us to identify buffers passed between the pipeline
> > * handler and the IPA.
> > */
> > - index = 0;
> > - for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,
> > - .planes = b->planes() });
> > - index++;
> > - }
> > -
> > - index = 0;
> > - for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> > - .planes = b->planes() });
> > - index++;
> > + for (auto const &b : buffers) {
> > + ipaBuffers.push_back({ .id = id, .planes = b->planes() });
> > + data->ipaBufferIds_.push_back(id);
> > + id++;
> > }
> >
> > - data->ipa_->mapBuffers(data->ipaBuffers_);
> > -
> > - return 0;
> > + data->ipa_->mapBuffers(ipaBuffers);
> > }
> >
> > void PipelineHandlerRPi::freeBuffers(Camera *camera)
> > {
> > RPiCameraData *data = cameraData(camera);
> >
> > - std::vector<unsigned int> ids;
> > - for (IPABuffer &ipabuf : data->ipaBuffers_)
> > - ids.push_back(ipabuf.id);
> > -
> > - data->ipa_->unmapBuffers(ids);
> > - data->ipaBuffers_.clear();
> > + data->ipa_->unmapBuffers(data->ipaBufferIds_);
> > + data->ipaBufferIds_.clear();
> >
> > for (auto const stream : data->streams_)
> > stream->releaseBuffers();
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 61226e94..e4158e3a 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -48,6 +48,11 @@ void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *bu
> > [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> > }
> >
> > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > +{
> > + bufferList_.push_back(buffer);
> > +}
> > +
> > const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> > {
> > return bufferList_;
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > index a6fd5c8e..e2534a32 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -41,6 +41,7 @@ public:
> > void reset();
> > std::string name() const;
> > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > + void setExternalBuffer(FrameBuffer *buffer);
> > const std::vector<FrameBuffer *> &getBuffers() const;
> > void releaseBuffers();
> > int prepareBuffers(unsigned int count);
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list