| Message ID | 20220923125546.903671-2-fsylvestre@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@baylibre.com> > Signed-off-by: Paul Elder <paul.elder@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 };
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); +} + 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); 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 }, + { 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); + } 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); } 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; @@ -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 };