[libcamera-devel] [PATCH v4 9/9] libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer cookie
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 22 18:03:34 CEST 2020
Hi Naush,
Thank you for the patch.
On Mon, Jul 20, 2020 at 10:13:11AM +0100, 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>
> ---
> .../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);
I also have trouble understanding this patch (likely due to the trouble
understanding the previous ones though). bufferList_ is documented as
"All framebuffers associated with this device stream" and is filled when
exporting buffers or at start() time. I don't understand how this can
work, as applications can provide new frame buffers at any time. What am
I missing ?
>
> - 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list