[libcamera-devel] [PATCH 1/1] pipeline: rkisp1: Implement Bayer formats support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 26 13:15:40 CEST 2022
Hi Florian,
Thank you for the patch.
On Fri, Sep 23, 2022 at 02:55:46PM +0200, Florian Sylvestre via libcamera-devel wrote:
> Implement raw mode for RkISP1:
> - The ISP resizer doesn't support raw formats, so when in raw mode we force the
> output resolution to be the same as the sensor one.
> - In raw mode, the ISP is bypassed, so we never get statistics buffers.
> This means that the IPA is never instructed to set the controls nor the
> metadata.
> Add a completeRaw() function to the IPA for the purpose of instructing the IPA
> to set controls and metadata when a frame is ready, as opposed to when the
> statistics are ready.
> We also need to skip queueing the stats buffer when in raw mode to prevent the
> statistics bufferReady slot to be triggered at stream off.
>
> Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> include/libcamera/ipa/rkisp1.mojom | 1 +
> src/ipa/rkisp1/rkisp1.cpp | 10 +++
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++++++++-
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++-
> 4 files changed, 120 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index eaf3955e..931ef357 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -27,6 +27,7 @@ interface IPARkISP1Interface {
> [async] fillParamsBuffer(uint32 frame, uint32 bufferId);
> [async] processStatsBuffer(uint32 frame, uint32 bufferId,
> libcamera.ControlList sensorControls);
> + [async] completeRaw(uint32 frame);
> };
>
> interface IPARkISP1EventInterface {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 61a091e6..4c784e8f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -63,6 +63,7 @@ public:
> void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
> void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> const ControlList &sensorControls) override;
> + void completeRaw(const uint32_t frame) override;
>
> protected:
> std::string logPrefix() const override;
> @@ -352,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> prepareMetadata(frame, aeState);
> }
>
> +void IPARkISP1::completeRaw(const uint32_t frame)
> +{
> + unsigned int aeState = 0;
> +
> + setControls(frame);
> +
> + prepareMetadata(frame, aeState);
> +}
> +
Is this enough to ensure the IPA module will operate properly when
capturing raw frames ? The algorithms will never be run due to a lack of
statistics, so I don't think manual exposure and gain will be taken into
account correctly.
This should probably be fixed in a separate patch, possibly before this
one to prepare for raw support on the IPA side first and then enabling
it on the pipeline handler side.
> void IPARkISP1::setControls(unsigned int frame)
> {
> RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 25fbf9f1..bade024d 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -182,6 +182,7 @@ private:
> std::unique_ptr<V4L2Subdevice> csi_;
>
> bool hasSelfPath_;
> + bool isRaw_;
>
> RkISP1MainPath mainPath_;
> RkISP1SelfPath selfPath_;
> @@ -363,7 +364,9 @@ void RkISP1CameraData::paramFilled(unsigned int frame)
> return;
>
> pipe->param_->queueBuffer(info->paramBuffer);
> - pipe->stat_->queueBuffer(info->statBuffer);
> +
> + if (!pipe->isRaw_)
> + pipe->stat_->queueBuffer(info->statBuffer);
Shouldn't you also skip queuing ISP parameters ?
>
> if (info->mainPathBuffer)
> mainPath_->queueBuffer(info->mainPathBuffer);
> @@ -413,6 +416,21 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> return true;
> }
>
> +std::map<PixelFormat, uint32_t> rawFormats = {
> + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
Could you please sort the bayer pattern alphabetically ? Same below
where applicable.
> + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> +};
> +
> CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> {
> const CameraSensor *sensor = data_->sensor_.get();
> @@ -504,8 +522,23 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> /* Select the sensor format. */
> Size maxSize;
> - for (const StreamConfiguration &cfg : config_)
> + PixelFormat rawFormat;
> + bool hasRawFormat = false;
> + for (StreamConfiguration &cfg : config_) {
> + if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> + PixelFormatInfo::ColourEncodingRAW) {
> + hasRawFormat = true;
> + rawFormat = cfg.pixelFormat;
> +
> + /* Raw format cannot be resized by ISP. */
> + if (cfg.size != sensor->resolution()) {
> + cfg.size = sensor->resolution();
> + status = Adjusted;
> + }
> + }
> +
> maxSize = std::max(maxSize, cfg.size);
> + }
If an application requests two streams, I don't think this logic will
work correctly. We can't capture two raw streams, or one raw and one YUV
stream.
>
> sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> MEDIA_BUS_FMT_SGBRG12_1X12,
> @@ -520,6 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> MEDIA_BUS_FMT_SGRBG8_1X8,
> MEDIA_BUS_FMT_SRGGB8_1X8 },
> maxSize);
> +
> + if (hasRawFormat) {
> + auto mbus = rawFormats.find(rawFormat);
> + if (mbus != rawFormats.end())
> + sensorFormat_ = sensor->getFormat({ mbus->second }, maxSize);
> + }
> +
> if (sensorFormat_.size.isNull())
> sensorFormat_.size = sensor->resolution();
>
> @@ -659,8 +699,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> << "ISP input pad configured with " << format
> << " crop " << rect;
>
> + const PixelFormat &streamFormat = config->at(0).pixelFormat;
> + const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> + isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +
> /* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> - format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> + if (!isRaw_)
> + format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> +
> LOG(RkISP1, Debug)
> << "Configuring ISP output pad with " << format
> << " crop " << rect;
> @@ -1152,6 +1198,17 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> request->metadata().set(controls::SensorTimestamp,
> buffer->metadata().timestamp);
>
> + if (isRaw_) {
> + ASSERT(activeCamera_);
> + RkISP1CameraData *data = cameraData(activeCamera_);
> +
> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> + if (!info)
> + return;
> +
> + data->ipa_->completeRaw(info->frame);
> + }
> +
> completeBuffer(request, buffer);
> tryCompleteRequest(request);
As PipelineHandlerRkISP1::tryCompleteRequest() calls find() and all of
its callers now do as well, I'd modify the function to take the info
pointer instead of the request pointer.
> }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 2d38f0fb..0a33d9ed 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -132,6 +132,42 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> case formats::NV21:
> ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;
> break;
> + case formats::SRGGB8:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8;
> + break;
> + case formats::SGRBG8:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8;
> + break;
> + case formats::SGBRG8:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8;
> + break;
> + case formats::SBGGR8:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8;
> + break;
> + case formats::SRGGB10:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10;
> + break;
> + case formats::SGRBG10:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + break;
> + case formats::SGBRG10:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10;
> + break;
> + case formats::SBGGR10:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + break;
> + case formats::SRGGB12:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12;
> + break;
> + case formats::SGRBG12:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12;
> + break;
> + case formats::SGBRG12:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12;
> + break;
> + case formats::SBGGR12:
> + ispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12;
> + break;
> default:
> ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> break;
This is growing big, how about extending the rawFormats map to cover all
formats, and using it here ?
> @@ -207,14 +243,25 @@ void RkISP1Path::stop()
> namespace {
> constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
> constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
> +constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
> formats::YUYV,
> formats::NV16,
> formats::NV61,
> formats::NV21,
> formats::NV12,
> formats::R8,
> - /* \todo Add support for RAW formats. */
> + formats::SRGGB8,
> + formats::SGRBG8,
> + formats::SGBRG8,
> + formats::SBGGR8,
> + formats::SRGGB10,
> + formats::SGRBG10,
> + formats::SGBRG10,
> + formats::SBGGR10,
> + formats::SRGGB12,
> + formats::SGRBG12,
> + formats::SGBRG12,
> + formats::SBGGR12,
> };
>
> constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list