[PATCH v4 5/5] libcamera: rkisp1: Plumb the ConverterDW100 converter
Umang Jain
umang.jain at ideasonboard.com
Tue Jul 9 12:22:49 CEST 2024
Hi Paul
On 03/07/24 1:14 pm, Paul Elder wrote:
> On Wed, Jul 03, 2024 at 09:24:05AM +0530, Umang Jain wrote:
>> Hi Paul
>>
>> On 02/07/24 7:15 pm, Paul Elder wrote:
>>> On Thu, Jun 27, 2024 at 07:16:56PM +0530, Umang Jain wrote:
>>>> Plumb the ConverterDW100 converter in the rkisp1 pipeline handler.
>>>> If the dewarper is found, it is instantiated and buffers are exported
>>>> from it, instead of RkISP1Path. Internal buffers are allocated for the
>>>> RkISP1Path in case where dewarper is going to be used.
>>>>
>>>> The RKISP1 pipeline handler now supports scaler crop control through
>>>> ConverterDW100. Register the ScalerCrop control for the cameras created
>>>> in the RKISP1 pipeline handler.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>> ---
>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 144 +++++++++++++++++-
>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +-
>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 14 ++
>>>> 3 files changed, 161 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> index bfc44239..eb373be4 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> @@ -8,9 +8,11 @@
>>>> #include <algorithm>
>>>> #include <array>
>>>> #include <iomanip>
>>>> +#include <map>
>>>> #include <memory>
>>>> #include <numeric>
>>>> #include <queue>
>>>> +#include <vector>
>>>> #include <linux/media-bus-format.h>
>>>> #include <linux/rkisp1-config.h>
>>>> @@ -33,6 +35,7 @@
>>>> #include "libcamera/internal/camera.h"
>>>> #include "libcamera/internal/camera_sensor.h"
>>>> +#include "libcamera/internal/converter/converter_dw100.h"
>>>> #include "libcamera/internal/delayed_controls.h"
>>>> #include "libcamera/internal/device_enumerator.h"
>>>> #include "libcamera/internal/framebuffer.h"
>>>> @@ -62,6 +65,8 @@ struct RkISP1FrameInfo {
>>>> bool paramDequeued;
>>>> bool metadataProcessed;
>>>> +
>>>> + std::optional<Rectangle> scalerCrop;
>>>> };
>>>> class RkISP1Frames
>>>> @@ -181,6 +186,7 @@ private:
>>>> void bufferReady(FrameBuffer *buffer);
>>>> void paramReady(FrameBuffer *buffer);
>>>> void statReady(FrameBuffer *buffer);
>>>> + void dewarpBufferReady(FrameBuffer *buffer);
>>>> void frameStart(uint32_t sequence);
>>>> int allocateBuffers(Camera *camera);
>>>> @@ -200,6 +206,13 @@ private:
>>>> RkISP1MainPath mainPath_;
>>>> RkISP1SelfPath selfPath_;
>>>> + std::unique_ptr<ConverterDW100> dewarper_;
>>>> + bool useDewarper_;
>>>> +
>>>> + /* Internal buffers used when dewarper is being used */
>>>> + std::vector<std::unique_ptr<FrameBuffer>> mainPathBuffers_;
>>>> + std::queue<FrameBuffer *> availableMainPathBuffers_;
>>>> +
>>>> std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
>>>> std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
>>>> std::queue<FrameBuffer *> availableParamBuffers_;
>>>> @@ -222,6 +235,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>>>> FrameBuffer *paramBuffer = nullptr;
>>>> FrameBuffer *statBuffer = nullptr;
>>>> + FrameBuffer *mainPathBuffer = nullptr;
>>>> + FrameBuffer *selfPathBuffer = nullptr;
>>> I don't think you need the selfPathBuffer?
>> Just a comestic change. See selfPathBuffer hunk declaration below.
>> I have just moved it up here (not inlined)...
>>
> Yeah I realized that...
>
> I also wondered if this would be for ISP versions that do have the self
> path and a dewarper, but since we don't have any yet it doesn't really
> matter.
>
>>>> if (!isRaw) {
>>>> if (pipe_->availableParamBuffers_.empty()) {
>>>> @@ -239,10 +254,16 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>>>> statBuffer = pipe_->availableStatBuffers_.front();
>>>> pipe_->availableStatBuffers_.pop();
>>>> +
>>>> + if (pipe_->useDewarper_) {
>>>> + mainPathBuffer = pipe_->availableMainPathBuffers_.front();
>>>> + pipe_->availableMainPathBuffers_.pop();
>>>> + }
>>>> }
>>>> - FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
>>>> - FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
>>>> + if (!mainPathBuffer)
>>>> + mainPathBuffer = request->findBuffer(&data->mainPathStream_);
>>>> + selfPathBuffer = request->findBuffer(&data->selfPathStream_);
>>>> RkISP1FrameInfo *info = new RkISP1FrameInfo;
>>>> @@ -268,6 +289,7 @@ int RkISP1Frames::destroy(unsigned int frame)
>>>> pipe_->availableParamBuffers_.push(info->paramBuffer);
>>>> pipe_->availableStatBuffers_.push(info->statBuffer);
>>>> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
>>>> frameInfo_.erase(info->frame);
>>>> @@ -283,6 +305,7 @@ void RkISP1Frames::clear()
>>>> pipe_->availableParamBuffers_.push(info->paramBuffer);
>>>> pipe_->availableStatBuffers_.push(info->statBuffer);
>>>> + pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
>>>> delete info;
>>>> }
>>>> @@ -607,7 +630,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>> */
>>>> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>>>> - : PipelineHandler(manager), hasSelfPath_(true)
>>>> + : PipelineHandler(manager), hasSelfPath_(true), useDewarper_(false)
>>>> {
>>>> }
>>>> @@ -785,12 +808,26 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>>> << " crop " << rect;
>>>> std::map<unsigned int, IPAStream> streamConfig;
>>>> + std::vector<std::reference_wrapper<StreamConfiguration>> outputCfgs;
>>>> for (const StreamConfiguration &cfg : *config) {
>>>> if (cfg.stream() == &data->mainPathStream_) {
>>>> ret = mainPath_.configure(cfg, format);
>>>> streamConfig[0] = IPAStream(cfg.pixelFormat,
>>>> cfg.size);
>>>> + /* Configure dewarp */
>>>> + if (dewarper_ && !isRaw_) {
>>>> + /*
>>>> + * \todo Converter::configure() API should be changed
>>>> + * to use std::vector<std::reference_wrapper<const StreamConfiguration>> ?
>>>> + */
>>>> + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg));
>>>> + dewarper_->configure(cfg, outputCfgs);
>>>> + useDewarper_ = true;
>>>> + } else {
>>>> + useDewarper_ = false;
>>>> + }
>>>> +
>>>> } else if (hasSelfPath_) {
>>>> ret = selfPath_.configure(cfg, format);
>>>> streamConfig[1] = IPAStream(cfg.pixelFormat,
>>>> @@ -839,6 +876,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
>>>> RkISP1CameraData *data = cameraData(camera);
>>>> unsigned int count = stream->configuration().bufferCount;
>>>> + if (useDewarper_)
>>>> + return dewarper_->exportBuffers(&data->mainPathStream_, count, buffers);
>>>> +
>>>> if (stream == &data->mainPathStream_)
>>>> return mainPath_.exportBuffers(count, buffers);
>>>> else if (hasSelfPath_ && stream == &data->selfPathStream_)
>>>> @@ -866,6 +906,16 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>>>> ret = stat_->allocateBuffers(maxCount, &statBuffers_);
>>>> if (ret < 0)
>>>> goto error;
>>>> +
>>>> + /* If the dewarper is being used, allocate internal buffers for ISP */
>>>> + if (useDewarper_) {
>>>> + ret = mainPath_.allocateBuffers(maxCount, &mainPathBuffers_);
>>>> + if (ret < 0)
>>>> + goto error;
>>>> +
>>>> + for (std::unique_ptr<FrameBuffer> &buffer : mainPathBuffers_)
>>>> + availableMainPathBuffers_.push(buffer.get());
>>>> + }
>>>> }
>>>> for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
>>>> @@ -889,6 +939,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>>>> error:
>>>> paramBuffers_.clear();
>>>> statBuffers_.clear();
>>>> + mainPathBuffers_.clear();
>>>> return ret;
>>>> }
>>>> @@ -903,8 +954,12 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>>>> while (!availableParamBuffers_.empty())
>>>> availableParamBuffers_.pop();
>>>> + while (!availableMainPathBuffers_.empty())
>>>> + availableMainPathBuffers_.pop();
>>>> +
>>>> paramBuffers_.clear();
>>>> statBuffers_.clear();
>>>> + mainPathBuffers_.clear();
>>>> std::vector<unsigned int> ids;
>>>> for (IPABuffer &ipabuf : data->ipaBuffers_)
>>>> @@ -919,6 +974,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>>>> if (stat_->releaseBuffers())
>>>> LOG(RkISP1, Error) << "Failed to release stat buffers";
>>>> + if (useDewarper_)
>>>> + mainPath_.releaseBuffers();
>>>> +
>>>> return 0;
>>>> }
>>>> @@ -961,6 +1019,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>>>> << "Failed to start statistics " << camera->id();
>>>> return ret;
>>>> }
>>>> +
>>>> + if (useDewarper_) {
>>>> + ret = dewarper_->start();
>>>> + if (ret) {
>>>> + LOG(RkISP1, Error) << "Failed to start dewarper";
>>>> + return ret;
>>>> + }
>>>> + }
>>>> }
>>>> if (data->mainPath_->isEnabled()) {
>>>> @@ -1015,6 +1081,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>>>> if (ret)
>>>> LOG(RkISP1, Warning)
>>>> << "Failed to stop parameters for " << camera->id();
>>>> +
>>>> + if (useDewarper_)
>>>> + dewarper_->stop();
>>>> }
>>>> ASSERT(data->queuedRequests_.empty());
>>>> @@ -1045,6 +1114,14 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>>>> info->paramBuffer->cookie());
>>>> }
>>>> + const auto &crop = request->controls().get(controls::ScalerCrop);
>>>> + if (crop && useDewarper_) {
>>>> + Rectangle rect = crop.value();
>>>> + int ret = dewarper_->setScalerCrop(&data->mainPathStream_, &rect);
>>>> + if (!ret)
>>>> + info->scalerCrop = crop;
>>>> + }
>>>> +
>>>> data->frame_++;
>>>> return 0;
>>>> @@ -1110,6 +1187,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>>>> {
>>>> ControlInfoMap::Map rkisp1Controls;
>>>> + if (dewarper_) {
>>> Does this not need to check useDewarper_?
>> useDewarper_ is only true in the configured when a non-RAW stream is
>> configured. updateControls() is called in configure() and createCamera().
>>
>> If I use useDewarper_, scalerCrop control would not be registered for the
>> camera. The goal here is to register the scaler crop control for the camera
>> (when being created). To use the dewarper or not, depends on the pipeline
>> configuration and what has been configured.
> Hm that's true. Do we have a policy on whether or not to report controls
> that depend on the configuration? Or maybe the control is reported to
> exist but the bounds should change depending on the configuration? iirc
> things like FrameDurationLimits are like that?
Yes, the updateControls() also run as part of configure() call. So the
bounds get updated whenever the configuration changes.
>
>
> Paul
>
>>
>>> Otherwise looks good to me.
>>>
>>>
>>> Paul
>>>
>>>> + auto [minCrop, maxCrop] = dewarper_->getCropBounds(&data->mainPathStream_);
>>>> +
>>>> + rkisp1Controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
>>>> + }
>>>> +
>>>> /* Add the IPA registered controls to list of camera controls. */
>>>> for (const auto &ipaControl : data->ipaControls_)
>>>> rkisp1Controls[ipaControl.first] = ipaControl.second;
>>>> @@ -1173,6 +1256,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>>>> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>>>> {
>>>> + std::shared_ptr<MediaDevice> dwpMediaDevice;
>>>> const MediaPad *pad;
>>>> DeviceMatch dm("rkisp1");
>>>> @@ -1237,6 +1321,26 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>>>> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
>>>> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>>>> + /* If dewarper is present, create its instance. */
>>>> + DeviceMatch dwp("dw100");
>>>> + dwp.add("dw100-source");
>>>> + dwp.add("dw100-sink");
>>>> +
>>>> + dwpMediaDevice = enumerator->search(dwp);
>>>> + if (dwpMediaDevice) {
>>>> + dewarper_ = std::make_unique<ConverterDW100>(std::move(dwpMediaDevice));
>>>> + if (dewarper_->isValid()) {
>>>> + dewarper_->outputBufferReady.connect(
>>>> + this, &PipelineHandlerRkISP1::dewarpBufferReady);
>>>> +
>>>> + LOG(RkISP1, Info) << "Using DW100 dewarper " << dewarper_->deviceNode();
>>>> + } else {
>>>> + LOG(RkISP1, Debug) << "Found DW100 dewarper " << dewarper_->deviceNode()
>>>> + << " but invalid";
>>>> + dewarper_.reset();
>>>> + }
>>>> + }
>>>> +
>>>> /*
>>>> * Enumerate all sensors connected to the ISP and create one
>>>> * camera instance for each of them.
>>>> @@ -1283,7 +1387,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>>>> return;
>>>> const FrameMetadata &metadata = buffer->metadata();
>>>> - Request *request = buffer->request();
>>>> + Request *request = info->request;
>>>> if (metadata.status != FrameMetadata::FrameCancelled) {
>>>> /*
>>>> @@ -1300,11 +1404,43 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>>>> data->delayedCtrls_->get(metadata.sequence);
>>>> data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
>>>> }
>>>> +
>>>> } else {
>>>> if (isRaw_)
>>>> info->metadataProcessed = true;
>>>> }
>>>> + if (useDewarper_) {
>>>> + /*
>>>> + * Queue input and output buffers to the dewarper. The output
>>>> + * buffers for the dewarper are the buffers of the request, supplied
>>>> + * by the application.
>>>> + */
>>>> + int ret = dewarper_->queueBuffers(buffer, request->buffers());
>>>> + if (ret < 0)
>>>> + LOG(RkISP1, Error) << "Cannot queue buffers to dewarper: "
>>>> + << strerror(-ret);
>>>> +
>>>> + return;
>>>> + }
>>>> +
>>>> + completeBuffer(request, buffer);
>>>> + tryCompleteRequest(info);
>>>> +}
>>>> +
>>>> +void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
>>>> +{
>>>> + ASSERT(activeCamera_);
>>>> + RkISP1CameraData *data = cameraData(activeCamera_);
>>>> + Request *request = buffer->request();
>>>> +
>>>> + RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());
>>>> + if (!info)
>>>> + return;
>>>> +
>>>> + if (info->scalerCrop)
>>>> + request->metadata().set(controls::ScalerCrop, info->scalerCrop.value());
>>>> +
>>>> completeBuffer(request, buffer);
>>>> tryCompleteRequest(info);
>>>> }
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>> index c49017d1..c96ec1d6 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>> @@ -56,7 +56,7 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = {
>>>> RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>>>> const Size &minResolution, const Size &maxResolution)
>>>> - : name_(name), running_(false), formats_(formats),
>>>> + : name_(name), running_(false), internalBufs_(false), formats_(formats),
>>>> minResolution_(minResolution), maxResolution_(maxResolution),
>>>> link_(nullptr)
>>>> {
>>>> @@ -402,10 +402,12 @@ int RkISP1Path::start()
>>>> if (running_)
>>>> return -EBUSY;
>>>> - /* \todo Make buffer count user configurable. */
>>>> - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
>>>> - if (ret)
>>>> - return ret;
>>>> + if (!internalBufs_) {
>>>> + /* \todo Make buffer count user configurable. */
>>>> + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> ret = video_->streamOn();
>>>> if (ret) {
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>> index 08edefec..b7fa82d0 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>> @@ -55,6 +55,19 @@ public:
>>>> return video_->exportBuffers(bufferCount, buffers);
>>>> }
>>>> + int allocateBuffers(unsigned int bufferCount,
>>>> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>>>> + {
>>>> + internalBufs_ = true;
>>>> + return video_->allocateBuffers(bufferCount, buffers);
>>>> + }
>>>> +
>>>> + int releaseBuffers()
>>>> + {
>>>> + ASSERT(internalBufs_);
>>>> + return video_->releaseBuffers();
>>>> + }
>>>> +
>>>> int start();
>>>> void stop();
>>>> @@ -68,6 +81,7 @@ private:
>>>> const char *name_;
>>>> bool running_;
>>>> + bool internalBufs_;
>>>> const Span<const PixelFormat> formats_;
>>>> std::set<PixelFormat> streamFormats_;
>>>> --
>>>> 2.44.0
>>>>
More information about the libcamera-devel
mailing list