| Message ID | 20240726114715.226468-6-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Fri, Jul 26, 2024 at 05:17:15PM +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@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 154 ++++++++++++++++++++++- > 1 file changed, 150 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 25f2cc97..32ec0fdf 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; > > 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,19 @@ 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_) { > + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); > + ret = dewarper_->configure(cfg, outputCfgs); > + useDewarper_ = ret ? false : true; > + } > } else if (hasSelfPath_) { > ret = selfPath_.configure(cfg, format); > streamConfig[1] = IPAStream(cfg.pixelFormat, > @@ -839,6 +869,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 +899,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 */ s/ISP/ISP./ > + if (useDewarper_) { > + ret = mainPath_.exportBuffers(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 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > error: > paramBuffers_.clear(); > statBuffers_.clear(); > + mainPathBuffers_.clear(); > > return ret; > } > @@ -903,8 +947,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_) > @@ -961,6 +1009,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL > << "Failed to start statistics " << camera->id(); > return ret; > } > + > + if (useDewarper_) { > + ret = dewarper_->start(); You don't stop the dewarper in the error paths below. > + if (ret) { > + LOG(RkISP1, Error) << "Failed to start dewarper"; > + return ret; And there's no error handling here either. > + } u> + } > } > > if (data->mainPath_->isEnabled()) { > @@ -1015,6 +1071,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 +1104,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > info->paramBuffer->cookie()); > } > > + const auto &crop = request->controls().get(controls::ScalerCrop); > + if (crop && useDewarper_) { > + Rectangle appliedRect = crop.value(); > + int ret = dewarper_->setInputCrop(&data->mainPathStream_, &appliedRect); This doesn't seem right, you're applying the crop rectangle too early. > + if (!ret) { > + if (appliedRect != crop.value()) { > + /* > + * \todo How to handle these case? > + * Do we aim for pixel perfect set rectangles? > + */ This needs to be addressed, we need a decision (and a rationale), and we need to document it and handle the outcome accordingly. A warning message isn't a good solution. > + LOG(RkISP1, Warning) > + << "Applied rectangle " << appliedRect.toString() > + << " differs from requested " << crop.value().toString(); > + } > + > + info->scalerCrop = appliedRect; > + } > + } > + > data->frame_++; > > return 0; > @@ -1110,6 +1188,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > { > ControlInfoMap::Map rkisp1Controls; > > + if (dewarper_) { > + auto [minCrop, maxCrop] = dewarper_->inputCropBounds(&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 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > { > + std::shared_ptr<MediaDevice> dwpMediaDevice; Declare the variable below, when you assign it. > const MediaPad *pad; > > DeviceMatch dm("rkisp1"); > @@ -1250,6 +1335,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"; LOG(RkISP1, Debug) << "Found DW100 dewarper " << dewarper_->deviceNode() << " but invalid"; I think this should be a warning at least. > + dewarper_.reset(); > + } > + } > + > /* > * Enumerate all sensors connected to the ISP and create one > * camera instance for each of them. > @@ -1296,7 +1401,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > return; > > const FrameMetadata &metadata = buffer->metadata(); > - Request *request = buffer->request(); > + Request *request = info->request; Is this because internal buffers have no request associated with them ? > > if (metadata.status != FrameMetadata::FrameCancelled) { > /* > @@ -1313,11 +1418,52 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > data->delayedCtrls_->get(metadata.sequence); > data->ipa_->processStatsBuffer(info->frame, 0, ctrls); > } > + Not needed. > } else { > if (isRaw_) > info->metadataProcessed = true; > } > > + if (useDewarper_) { > + /* Do not queue cancelled frames to dewarper. */ > + if (metadata.status == FrameMetadata::FrameCancelled) { > + for (auto it : request->buffers()) > + completeBuffer(request, it.second); Will this work with multiple streams, won't the other stream also try to complete its own buffer ? Or do you assume here that there's a single stream, given that the DW100 is only found in the i.M8XMP ? A comment would help, but even better, you should try to complete only the buffer for this stream instead of completing all buffers. That should make the code more future-proof. Update: after reading the whole patch, there are clear assumptions that there will be a single stream when the converter is used. I suppose that's OK, but they're expressed in different ways that make the code sometimes more complex to follow. For instance in PipelineHandlerRkISP1::exportFrameBuffers() you completely bypass the main/self path check when there's a converter, while here you iterate over all buffers in the request as if there could be multiple streams. Things may benefit from being cleaned up a bit to keep the code in a maintainable state. > + > + tryCompleteRequest(info); > + return; > + } > + > + /* > + * 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); You can move this above and call it with if (!useDewarper_) { ... return; } and lower the indentation level of the rest of the code. > +} > + > +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()); This means that the scaler crop metadata will be set only when a request has been queued with the scaler crop control. From that point onwards, all request that complete will have the scaler crop metadata. Shouldn't you set it for every frame ? > + > completeBuffer(request, buffer); > tryCompleteRequest(info); > }
Hi Umang, On Sat, Aug 03, 2024 at 12:49:32AM +0300, Laurent Pinchart wrote: > On Fri, Jul 26, 2024 at 05:17:15PM +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@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 154 ++++++++++++++++++++++- > > 1 file changed, 150 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 25f2cc97..32ec0fdf 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; > > > > 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,19 @@ 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_) { > > + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); > > + ret = dewarper_->configure(cfg, outputCfgs); > > + useDewarper_ = ret ? false : true; > > + } > > } else if (hasSelfPath_) { > > ret = selfPath_.configure(cfg, format); > > streamConfig[1] = IPAStream(cfg.pixelFormat, > > @@ -839,6 +869,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 +899,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 */ > > s/ISP/ISP./ > > > + if (useDewarper_) { > > + ret = mainPath_.exportBuffers(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 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > > error: > > paramBuffers_.clear(); > > statBuffers_.clear(); > > + mainPathBuffers_.clear(); > > > > return ret; > > } > > @@ -903,8 +947,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_) > > @@ -961,6 +1009,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL > > << "Failed to start statistics " << camera->id(); > > return ret; > > } > > + > > + if (useDewarper_) { > > + ret = dewarper_->start(); > > You don't stop the dewarper in the error paths below. > > > + if (ret) { > > + LOG(RkISP1, Error) << "Failed to start dewarper"; > > + return ret; > > And there's no error handling here either. I've just posted "[PATCH v3 0/2] libcamera: Simplify error handling with ScopeExitActions class" which may be useful for you here. > > + } > > + } > > } > > > > if (data->mainPath_->isEnabled()) { > > @@ -1015,6 +1071,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 +1104,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > info->paramBuffer->cookie()); > > } > > > > + const auto &crop = request->controls().get(controls::ScalerCrop); > > + if (crop && useDewarper_) { > > + Rectangle appliedRect = crop.value(); > > + int ret = dewarper_->setInputCrop(&data->mainPathStream_, &appliedRect); > > This doesn't seem right, you're applying the crop rectangle too early. > > > + if (!ret) { > > + if (appliedRect != crop.value()) { > > + /* > > + * \todo How to handle these case? > > + * Do we aim for pixel perfect set rectangles? > > + */ > > This needs to be addressed, we need a decision (and a rationale), and we > need to document it and handle the outcome accordingly. A warning > message isn't a good solution. > > > + LOG(RkISP1, Warning) > > + << "Applied rectangle " << appliedRect.toString() > > + << " differs from requested " << crop.value().toString(); > > + } > > + > > + info->scalerCrop = appliedRect; > > + } > > + } > > + > > data->frame_++; > > > > return 0; > > @@ -1110,6 +1188,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > > { > > ControlInfoMap::Map rkisp1Controls; > > > > + if (dewarper_) { > > + auto [minCrop, maxCrop] = dewarper_->inputCropBounds(&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 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > > > bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > { > > + std::shared_ptr<MediaDevice> dwpMediaDevice; > > Declare the variable below, when you assign it. > > > const MediaPad *pad; > > > > DeviceMatch dm("rkisp1"); > > @@ -1250,6 +1335,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"; > LOG(RkISP1, Debug) > << "Found DW100 dewarper " << dewarper_->deviceNode() > << " but invalid"; > > I think this should be a warning at least. > > > + dewarper_.reset(); > > + } > > + } > > + > > /* > > * Enumerate all sensors connected to the ISP and create one > > * camera instance for each of them. > > @@ -1296,7 +1401,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > > return; > > > > const FrameMetadata &metadata = buffer->metadata(); > > - Request *request = buffer->request(); > > + Request *request = info->request; > > Is this because internal buffers have no request associated with them ? > > > > > if (metadata.status != FrameMetadata::FrameCancelled) { > > /* > > @@ -1313,11 +1418,52 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > > data->delayedCtrls_->get(metadata.sequence); > > data->ipa_->processStatsBuffer(info->frame, 0, ctrls); > > } > > + > > Not needed. > > > } else { > > if (isRaw_) > > info->metadataProcessed = true; > > } > > > > + if (useDewarper_) { > > + /* Do not queue cancelled frames to dewarper. */ > > + if (metadata.status == FrameMetadata::FrameCancelled) { > > + for (auto it : request->buffers()) > > + completeBuffer(request, it.second); > > Will this work with multiple streams, won't the other stream also try to > complete its own buffer ? Or do you assume here that there's a single > stream, given that the DW100 is only found in the i.M8XMP ? A comment > would help, but even better, you should try to complete only the buffer > for this stream instead of completing all buffers. That should make the > code more future-proof. > > Update: after reading the whole patch, there are clear assumptions that > there will be a single stream when the converter is used. I suppose > that's OK, but they're expressed in different ways that make the code > sometimes more complex to follow. For instance in > PipelineHandlerRkISP1::exportFrameBuffers() you completely bypass the > main/self path check when there's a converter, while here you iterate > over all buffers in the request as if there could be multiple streams. > Things may benefit from being cleaned up a bit to keep the code in a > maintainable state. > > > + > > + tryCompleteRequest(info); > > + return; > > + } > > + > > + /* > > + * 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); > > You can move this above and call it with > > if (!useDewarper_) { > ... > return; > } > > and lower the indentation level of the rest of the code. > > > +} > > + > > +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()); > > This means that the scaler crop metadata will be set only when a request > has been queued with the scaler crop control. From that point onwards, > all request that complete will have the scaler crop metadata. Shouldn't > you set it for every frame ? > > > + > > completeBuffer(request, buffer); > > tryCompleteRequest(info); > > }
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 25f2cc97..32ec0fdf 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; 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,19 @@ 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_) { + outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); + ret = dewarper_->configure(cfg, outputCfgs); + useDewarper_ = ret ? false : true; + } } else if (hasSelfPath_) { ret = selfPath_.configure(cfg, format); streamConfig[1] = IPAStream(cfg.pixelFormat, @@ -839,6 +869,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 +899,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_.exportBuffers(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 +932,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) error: paramBuffers_.clear(); statBuffers_.clear(); + mainPathBuffers_.clear(); return ret; } @@ -903,8 +947,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_) @@ -961,6 +1009,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 +1071,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 +1104,25 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) info->paramBuffer->cookie()); } + const auto &crop = request->controls().get(controls::ScalerCrop); + if (crop && useDewarper_) { + Rectangle appliedRect = crop.value(); + int ret = dewarper_->setInputCrop(&data->mainPathStream_, &appliedRect); + if (!ret) { + if (appliedRect != crop.value()) { + /* + * \todo How to handle these case? + * Do we aim for pixel perfect set rectangles? + */ + LOG(RkISP1, Warning) + << "Applied rectangle " << appliedRect.toString() + << " differs from requested " << crop.value().toString(); + } + + info->scalerCrop = appliedRect; + } + } + data->frame_++; return 0; @@ -1110,6 +1188,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) { ControlInfoMap::Map rkisp1Controls; + if (dewarper_) { + auto [minCrop, maxCrop] = dewarper_->inputCropBounds(&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 +1257,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) { + std::shared_ptr<MediaDevice> dwpMediaDevice; const MediaPad *pad; DeviceMatch dm("rkisp1"); @@ -1250,6 +1335,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. @@ -1296,7 +1401,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) { /* @@ -1313,11 +1418,52 @@ 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_) { + /* Do not queue cancelled frames to dewarper. */ + if (metadata.status == FrameMetadata::FrameCancelled) { + for (auto it : request->buffers()) + completeBuffer(request, it.second); + + tryCompleteRequest(info); + return; + } + + /* + * 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); }
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@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 154 ++++++++++++++++++++++- 1 file changed, 150 insertions(+), 4 deletions(-)