[libcamera-devel,v2,10/10] libcamera: pipeline: raspberrypi: Handle any externally allocated FrameBuffer
diff mbox series

Message ID 20200715140703.566298-11-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Zero-copy RAW stream work
Related show

Commit Message

Naushir Patuck July 15, 2020, 2:07 p.m. UTC 
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@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 90 +++++++++++--------
 .../pipeline/raspberrypi/rpi_stream.cpp       |  5 ++
 .../pipeline/raspberrypi/rpi_stream.h         |  1 +
 3 files changed, 61 insertions(+), 35 deletions(-)

Comments

Naushir Patuck July 16, 2020, 2:03 p.m. UTC | #1 
On Wed, 15 Jul 2020 at 15:07, Naushir Patuck <naush@raspberrypi.com> 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@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 90 +++++++++++--------
>  .../pipeline/raspberrypi/rpi_stream.cpp       |  5 ++
>  .../pipeline/raspberrypi/rpi_stream.h         |  1 +
>  3 files changed, 61 insertions(+), 35 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index fc6fbfc3..a32ec687 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -172,8 +172,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_;
> @@ -193,7 +193,6 @@ public:
>         std::queue<FrameBuffer *> bayerQueue_;
>         std::queue<FrameBuffer *> embeddedQueue_;
>         std::deque<Request *> requestQueue_;
> -
>         unsigned int dropFrameCount_;
>
>  private:
> @@ -246,6 +245,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_;
> @@ -730,18 +731,36 @@ 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 (stream->getBufferIndex(buffer) == -1) {

This should check for buffer being valid, else we could dereference a nullptr!

>                         /*
> -                        * If we have been asked to drop frames by the IPA, passing
> -                        * in a nullptr here will queue an internally allocated buffer.
> -                        * The Request buffer will be stored in another queue to be
> -                        * queued to the device - in sync with the Request.
> +                        * 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(data->dropFrameCount_ ? nullptr : buffer);
> -                       if (ret)
> -                               return ret;
> +                       stream->setExternalBuffer(buffer);
> +
> +                       /* 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));
> +                       }
>                 }
> +               /*
> +                * If we have been asked to drop frames by the IPA, passing
> +                * in a nullptr here will queue an internally allocated buffer.
> +                * The Request buffer will be stored in another queue to be
> +                * queued to the device - in sync with the Request.
> +                */
> +               int ret = stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer);
> +               if (ret)
> +                       return ret;
>         }
>
>         /* Push the request to the back of the queue. */
> @@ -919,7 +938,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>         RPiCameraData *data = cameraData(camera);
> -       unsigned int index;
>         int ret;
>
>         /*
> @@ -939,42 +957,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++;
> +       for (auto const &b : buffers) {
> +               ipaBuffers.push_back({ .id = id, .planes = b->planes() });
> +               data->ipaBufferIds_.push_back(id);
> +               id++;
>         }
>
> -       index = 0;
> -       for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> -               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> -                                             .planes = b->planes() });
> -               index++;
> -       }
> -
> -       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 97f87ad7..862a094a 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -48,6 +48,11 @@ void RPiStream::setExternalBuffers(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 16b90fac..266441df 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 setExternalBuffers(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
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index fc6fbfc3..a32ec687 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -172,8 +172,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_;
@@ -193,7 +193,6 @@  public:
 	std::queue<FrameBuffer *> bayerQueue_;
 	std::queue<FrameBuffer *> embeddedQueue_;
 	std::deque<Request *> requestQueue_;
-
 	unsigned int dropFrameCount_;
 
 private:
@@ -246,6 +245,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_;
@@ -730,18 +731,36 @@  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 (stream->getBufferIndex(buffer) == -1) {
 			/*
-			 * If we have been asked to drop frames by the IPA, passing
-			 * in a nullptr here will queue an internally allocated buffer.
-			 * The Request buffer will be stored in another queue to be
-			 * queued to the device - in sync with the Request.
+			 * 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(data->dropFrameCount_ ? nullptr : buffer);
-			if (ret)
-				return ret;
+			stream->setExternalBuffer(buffer);
+
+			/* 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));
+			}
 		}
+		/*
+		 * If we have been asked to drop frames by the IPA, passing
+		 * in a nullptr here will queue an internally allocated buffer.
+		 * The Request buffer will be stored in another queue to be
+		 * queued to the device - in sync with the Request.
+		 */
+		int ret = stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer);
+		if (ret)
+			return ret;
 	}
 
 	/* Push the request to the back of the queue. */
@@ -919,7 +938,6 @@  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
 int PipelineHandlerRPi::prepareBuffers(Camera *camera)
 {
 	RPiCameraData *data = cameraData(camera);
-	unsigned int index;
 	int ret;
 
 	/*
@@ -939,42 +957,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++;
+	for (auto const &b : buffers) {
+		ipaBuffers.push_back({ .id = id, .planes = b->planes() });
+		data->ipaBufferIds_.push_back(id);
+		id++;
 	}
 
-	index = 0;
-	for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
-		data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
-					      .planes = b->planes() });
-		index++;
-	}
-
-	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 97f87ad7..862a094a 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
@@ -48,6 +48,11 @@  void RPiStream::setExternalBuffers(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 16b90fac..266441df 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 setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
+	void setExternalBuffer(FrameBuffer *buffer);
 	const std::vector<FrameBuffer *> &getBuffers() const;
 	void releaseBuffers();
 	int prepareBuffers(unsigned int count);