[libcamera-devel] [PATCH v1] pipeline: raspberrypi: Remove enum BuffferMask from the mojom interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 22 20:53:45 CET 2022
Hi Naush,
Thank you for the patch.
On Tue, Nov 22, 2022 at 03:42:35PM +0000, Naushir Patuck via libcamera-devel wrote:
> The BufferMask enum provides a way of identifying which stream a frame buffer
> belongs to. This enum is defined in the raspberrypi.mojom interface file.
> However, the IPA does not need these enum definitions to mmap buffers that it
> uses.
>
> Move this enum out of the raspberrypi.mojom interface file and put it into
> the RPi namespace visible only to the pipeline handler. This removes the
> need to include the auto-generated IPA interface header in the RPi::Stream
> definition.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
Seems reasonable.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/ipa/raspberrypi.mojom | 8 --------
> src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---
> .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++++++++----------
> .../pipeline/raspberrypi/rpi_stream.cpp | 6 ++----
> .../pipeline/raspberrypi/rpi_stream.h | 13 +++++++++---
> 5 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 40f78d9e3b3f..d53644fe296c 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -8,14 +8,6 @@ module ipa.RPi;
>
> import "include/libcamera/ipa/core.mojom";
>
> -enum BufferMask {
> - MaskID = 0x00ffff,
> - MaskStats = 0x010000,
> - MaskEmbeddedData = 0x020000,
> - MaskBayerData = 0x040000,
> - MaskExternalBuffer = 0x100000,
> -};
> -
> /* Size of the LS grid allocation. */
> const uint32 MaxLsGridSize = 0x8000;
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index beb076dc4909..4e10c57d7f87 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -515,7 +515,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)
>
> reportMetadata();
>
> - statsMetadataComplete.emit(bufferId & MaskID, libcameraMetadata_);
> + statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> }
>
> void IPARPi::signalQueueRequest(const ControlList &controls)
> @@ -534,7 +534,7 @@ void IPARPi::signalIspPrepare(const ISPConfig &data)
> frameCount_++;
>
> /* Ready to push the input buffer into the ISP. */
> - runIsp.emit(data.bayerBufferId & MaskID);
> + runIsp.emit(data.bayerBufferId);
> }
>
> void IPARPi::reportMetadata()
> @@ -1001,7 +1001,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>
> void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> {
> - embeddedComplete.emit(bufferId & MaskID);
> + embeddedComplete.emit(bufferId);
> }
>
> void IPARPi::prepareISP(const ISPConfig &data)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 087c71b65700..0e0b71945640 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1484,10 +1484,10 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> * Pass the stats and embedded data buffers to the IPA. No other
> * buffers need to be passed.
> */
> - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::RPi::MaskStats);
> + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPi::MaskStats);
> if (data->sensorMetadata_)
> mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(),
> - ipa::RPi::MaskEmbeddedData);
> + RPi::MaskEmbeddedData);
>
> return 0;
> }
> @@ -1727,7 +1727,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> if (!isRunning())
> return;
>
> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
>
> handleStreamBuffer(buffer, &isp_[Isp::Stats]);
>
> @@ -1763,9 +1763,9 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> if (!isRunning())
> return;
>
> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
>
> - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> << ", timestamp: " << buffer->metadata().timestamp;
>
> isp_[Isp::Input].queueBuffer(buffer);
> @@ -1778,7 +1778,7 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
> if (!isRunning())
> return;
>
> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> handleState();
> }
> @@ -1931,7 +1931,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> * application until after the IPA signals so.
> */
> if (stream == &isp_[Isp::Stats]) {
> - ipa_->signalStatReady(ipa::RPi::MaskStats | static_cast<unsigned int>(index));
> + ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index));
> } else {
> /* Any other ISP output can be handed back to the application now. */
> handleStreamBuffer(buffer, stream);
> @@ -2006,7 +2006,7 @@ void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *strea
> {
> unsigned int id = stream->getBufferId(buffer);
>
> - if (!(id & ipa::RPi::MaskExternalBuffer))
> + if (!(id & RPi::MaskExternalBuffer))
> return;
>
> /* Stop the Stream object from tracking the buffer. */
> @@ -2174,13 +2174,13 @@ void RPiCameraData::tryRunPipeline()
> << " Bayer buffer id: " << bayerId;
>
> ipa::RPi::ISPConfig ispPrepare;
> - ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;
> + ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> ispPrepare.controls = std::move(bayerFrame.controls);
>
> if (embeddedBuffer) {
> unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> - ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;
> + ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> ispPrepare.embeddedBufferPresent = true;
>
> LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 79f7be130ed4..2bb10f25d6ca 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -8,8 +8,6 @@
>
> #include <libcamera/base/log.h>
>
> -#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> -
> namespace libcamera {
>
> LOG_DEFINE_CATEGORY(RPISTREAM)
> @@ -74,7 +72,7 @@ int Stream::getBufferId(FrameBuffer *buffer) const
>
> void Stream::setExternalBuffer(FrameBuffer *buffer)
> {
> - bufferMap_.emplace(ipa::RPi::MaskExternalBuffer | id_.get(), buffer);
> + bufferMap_.emplace(BufferMask::MaskExternalBuffer | id_.get(), buffer);
> }
>
> void Stream::removeExternalBuffer(FrameBuffer *buffer)
> @@ -82,7 +80,7 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
> int id = getBufferId(buffer);
>
> /* Ensure we have this buffer in the stream, and it is marked external. */
> - ASSERT(id != -1 && (id & ipa::RPi::MaskExternalBuffer));
> + ASSERT(id != -1 && (id & BufferMask::MaskExternalBuffer));
> bufferMap_.erase(id);
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 3c0b5c8ebab4..b8bd79cf1535 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -12,7 +12,6 @@
> #include <unordered_map>
> #include <vector>
>
> -#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/v4l2_videodevice.h"
> @@ -23,6 +22,14 @@ namespace RPi {
>
> using BufferMap = std::unordered_map<unsigned int, FrameBuffer *>;
>
> +enum BufferMask {
> + MaskID = 0x00ffff,
> + MaskStats = 0x010000,
> + MaskEmbeddedData = 0x020000,
> + MaskBayerData = 0x040000,
> + MaskExternalBuffer = 0x100000,
> +};
> +
> /*
> * Device stream abstraction for either an internal or external stream.
> * Used for both Unicam and the ISP.
> @@ -31,13 +38,13 @@ class Stream : public libcamera::Stream
> {
> public:
> Stream()
> - : id_(ipa::RPi::MaskID)
> + : id_(BufferMask::MaskID)
> {
> }
>
> Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> : external_(false), importOnly_(importOnly), name_(name),
> - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(ipa::RPi::MaskID)
> + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID)
> {
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list