[libcamera-devel] [PATCH v4 9/9] libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer cookie
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 21 13:27:51 CEST 2020
Hi Naush,
On 20/07/2020 10:13, Naushir Patuck wrote:
> The FrameBuffer cookie may be set by the application, so this cannot
> be set by the pipeline handler as well. Revert to using a simple index
> into the buffer list to identify buffers passing to and from the IPA.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
I wonder if this means we should add an internal cookie or ability to
identify the IPA buffers in the buffer class itself.
But still, this looks like it works and fixes the issue.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> .../pipeline/raspberrypi/raspberrypi.cpp | 60 ++++++++++---------
> .../pipeline/raspberrypi/rpi_stream.cpp | 14 +++--
> .../pipeline/raspberrypi/rpi_stream.h | 2 +-
> 3 files changed, 40 insertions(+), 36 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index c28fe997..e4fc5ac7 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -876,7 +876,8 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> {
> RPiCameraData *data = cameraData(camera);
> - int count, ret;
> + unsigned int index;
> + int ret;
>
> /*
> * Decide how many internal buffers to allocate. For now, simply look
> @@ -896,30 +897,24 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> }
>
> /*
> - * Add cookies to the ISP Input buffers so that we can link them with
> - * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> - */
> - count = 0;
> - for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> - b->setCookie(count++);
> - }
> -
> - /*
> - * Add cookies to the stats and embedded data buffers and link them with
> - * the IPA.
> + * Link the FrameBuffers with the index of their position in the vector
> + * stored in the RPi stream object.
> + *
> + * This will allow us to identify buffers passed between the pipeline
> + * handler and the IPA.
> */
> - count = 0;
> + index = 0;
> for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> - b->setCookie(count++);
> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,
> .planes = b->planes() });
> + index++;
> }
>
> - count = 0;
> + index = 0;
> for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> - b->setCookie(count++);
> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> .planes = b->planes() });
> + index++;
> }
>
> data->ipa_->mapBuffers(data->ipaBuffers_);
> @@ -1097,7 +1092,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> unsigned int bufferId = action.data[0];
> FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>
> - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> << ", timestamp: " << buffer->metadata().timestamp;
>
> isp_[Isp::Input].queueBuffer(buffer);
> @@ -1117,12 +1112,14 @@ done:
> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> {
> RPi::RPiStream *stream = nullptr;
> + int index;
>
> if (state_ == State::Stopped)
> return;
>
> for (RPi::RPiStream &s : unicam_) {
> - if (s.findFrameBuffer(buffer)) {
> + index = s.getBufferIndex(buffer);
> + if (index != -1) {
> stream = &s;
> break;
> }
> @@ -1132,7 +1129,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> ASSERT(stream);
>
> LOG(RPI, Debug) << "Stream " << stream->name() << " buffer dequeue"
> - << ", buffer id " << buffer->cookie()
> + << ", buffer id " << index
> << ", timestamp: " << buffer->metadata().timestamp;
>
> if (stream == &unicam_[Unicam::Image]) {
> @@ -1172,7 +1169,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> return;
>
> LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> - << ", buffer id " << buffer->cookie()
> + << ", buffer id " << unicam_[Unicam::Image].getBufferIndex(buffer)
> << ", timestamp: " << buffer->metadata().timestamp;
>
> /* The ISP input buffer gets re-queued into Unicam. */
> @@ -1183,12 +1180,14 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> {
> RPi::RPiStream *stream = nullptr;
> + int index;
>
> if (state_ == State::Stopped)
> return;
>
> for (RPi::RPiStream &s : isp_) {
> - if (s.findFrameBuffer(buffer)) {
> + index = s.getBufferIndex(buffer);
> + if (index != -1) {
> stream = &s;
> break;
> }
> @@ -1198,7 +1197,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> ASSERT(stream);
>
> LOG(RPI, Debug) << "Stream " << stream->name() << " buffer complete"
> - << ", buffer id " << buffer->cookie()
> + << ", buffer id " << index
> << ", timestamp: " << buffer->metadata().timestamp;
>
> /*
> @@ -1208,7 +1207,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> if (stream == &isp_[Isp::Stats]) {
> IPAOperationData op;
> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> - op.data = { RPiIpaMask::STATS | buffer->cookie() };
> + op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> ipa_->processEvent(op);
> } else {
> /* Any other ISP output can be handed back to the application now. */
> @@ -1427,13 +1426,16 @@ void RPiCameraData::tryRunPipeline()
> /* Set our state to say the pipeline is active. */
> state_ = State::Busy;
>
> + unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);
> + unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);
> +
> LOG(RPI, Debug) << "Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:"
> - << " Bayer buffer id: " << bayerBuffer->cookie()
> - << " Embedded buffer id: " << embeddedBuffer->cookie();
> + << " Bayer buffer id: " << bayerIndex
> + << " Embedded buffer id: " << embeddedIndex;
>
> op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie(),
> - RPiIpaMask::BAYER_DATA | bayerBuffer->cookie() };
> + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> + RPiIpaMask::BAYER_DATA | bayerIndex };
> ipa_->processEvent(op);
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 6635a751..73eb04a1 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -178,15 +178,17 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
> }
> }
>
> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +int RPiStream::getBufferIndex(FrameBuffer *buffer) const
> {
> if (importOnly_)
> - return false;
> + return -1;
>
> - if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> - return true;
> + /* Find the buffer in the list, and return the index position. */
> + auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);
> + if (it != bufferList_.end())
> + return std::distance(bufferList_.begin(), it);
>
> - return false;
> + return -1;
> }
>
> void RPiStream::clearBuffers()
> @@ -199,7 +201,7 @@ void RPiStream::clearBuffers()
>
> int RPiStream::queueToDevice(FrameBuffer *buffer)
> {
> - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> + LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferIndex(buffer)
> << " for " << name_;
>
> int ret = dev_->queueBuffer(buffer);
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 6222c867..45cf5237 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -48,7 +48,7 @@ public:
> int queueAllBuffers();
> int queueBuffer(FrameBuffer *buffer);
> void returnBuffer(FrameBuffer *buffer);
> - bool findFrameBuffer(FrameBuffer *buffer) const;
> + int getBufferIndex(FrameBuffer *buffer) const;
>
> private:
> void clearBuffers();
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list