[libcamera-devel] [PATCH v3 12/13] pipeline: rkisp1: Support raw Bayer capture at runtime
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 26 18:58:04 CEST 2022
Hi Laurent,
first of all I've been testing this as well and it works, I can
capture raw images and they look correct, without any noticeable regression
in the other capture modes
Tested-by: Jacopo Mondi <jacopo at jmondi.org>
On Mon, Oct 24, 2022 at 03:03:55AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Florian Sylvestre <fsylvestre at baylibre.com>
>
> Implement support for raw Bayer capture at runtime, from start() to
> stop(). Support of raw formats in the camera configuration is split to a
> subsequent change to ease review.
>
> In raw mode, the ISP is bypassed. There is no need to provide parameter
> buffers, and the ISP will not generate statistics. This requires
> multiple changes in the buffer handling:
>
> - The params and stats buffers don't need to be allocated, and the
> corresponding video nodes don't need to be started or stopped.
>
> - The IPA module fillParamsBuffer() operation must not be called in
> queueRequestDevice(). As a result, the IPA module thus doesn't emit
> the paramsBufferReady signal. The main and self path video buffers
> must thus be queued directly in queueRequestDevice().
>
> - The tryCompleteRequest() function must not to wait until the params
> buffer has been dequeued.
>
> - When the frame buffer has been captured, the IPA module
> processStatsBuffer() operation must be called directly to fill request
> metadata.
>
> Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v3:
>
> - Split from "pipeline: rkisp1: Support raw Bayer capture"
> - Drop new completeRaw() IPA operation
> - Don't queue params buffers in raw capture mode
> - Fix assertion failure when stopping capture
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 148 ++++++++++++++---------
> 1 file changed, 93 insertions(+), 55 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index dcab5286aa56..e57411544f7a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -67,7 +67,8 @@ class RkISP1Frames
> public:
> RkISP1Frames(PipelineHandler *pipe);
>
> - RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request);
> + RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,
> + bool isRaw);
> int destroy(unsigned int frame);
> void clear();
>
> @@ -184,6 +185,7 @@ private:
> std::unique_ptr<V4L2Subdevice> csi_;
>
> bool hasSelfPath_;
> + bool isRaw_;
Will a single variable global to the whole pipeline handler prevents
any use case in future ?
Right now I don't think it's an issue, but it might be nicer to have
this per-CameraData.
However, in all the platforms we have, the ISP has a single input
hence it's not possible to have different applications using multiple
cameras at the same time, so this is not a real issue for now
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
>
> RkISP1MainPath mainPath_;
> RkISP1SelfPath selfPath_;
> @@ -203,28 +205,35 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> {
> }
>
> -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request)
> +RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,
> + bool isRaw)
> {
> unsigned int frame = data->frame_;
>
> - if (pipe_->availableParamBuffers_.empty()) {
> - LOG(RkISP1, Error) << "Parameters buffer underrun";
> - return nullptr;
> - }
> - FrameBuffer *paramBuffer = pipe_->availableParamBuffers_.front();
> + FrameBuffer *paramBuffer = nullptr;
> + FrameBuffer *statBuffer = nullptr;
> +
> + if (!isRaw) {
> + if (pipe_->availableParamBuffers_.empty()) {
> + LOG(RkISP1, Error) << "Parameters buffer underrun";
> + return nullptr;
> + }
> +
> + if (pipe_->availableStatBuffers_.empty()) {
> + LOG(RkISP1, Error) << "Statisitc buffer underrun";
> + return nullptr;
> + }
> +
> + paramBuffer = pipe_->availableParamBuffers_.front();
> + pipe_->availableParamBuffers_.pop();
>
> - if (pipe_->availableStatBuffers_.empty()) {
> - LOG(RkISP1, Error) << "Statisitc buffer underrun";
> - return nullptr;
> + statBuffer = pipe_->availableStatBuffers_.front();
> + pipe_->availableStatBuffers_.pop();
> }
> - FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
>
> FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
>
> - pipe_->availableParamBuffers_.pop();
> - pipe_->availableStatBuffers_.pop();
> -
> RkISP1FrameInfo *info = new RkISP1FrameInfo;
>
> info->frame = frame;
> @@ -665,6 +674,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> << "ISP input pad configured with " << format
> << " crop " << rect;
>
> + isRaw_ = false;
> +
> /* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> LOG(RkISP1, Debug)
> @@ -760,13 +771,15 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> data->selfPathStream_.configuration().bufferCount,
> });
>
> - ret = param_->allocateBuffers(maxCount, ¶mBuffers_);
> - if (ret < 0)
> - goto error;
> + if (!isRaw_) {
> + ret = param_->allocateBuffers(maxCount, ¶mBuffers_);
> + if (ret < 0)
> + goto error;
>
> - ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> - if (ret < 0)
> - goto error;
> + ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> + if (ret < 0)
> + goto error;
> + }
>
> for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> buffer->setCookie(ipaBufferId++);
> @@ -842,23 +855,25 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>
> data->frame_ = 0;
>
> - ret = param_->streamOn();
> - if (ret) {
> - data->ipa_->stop();
> - freeBuffers(camera);
> - LOG(RkISP1, Error)
> - << "Failed to start parameters " << camera->id();
> - return ret;
> - }
> + if (!isRaw_) {
> + ret = param_->streamOn();
> + if (ret) {
> + data->ipa_->stop();
> + freeBuffers(camera);
> + LOG(RkISP1, Error)
> + << "Failed to start parameters " << camera->id();
> + return ret;
> + }
>
> - ret = stat_->streamOn();
> - if (ret) {
> - param_->streamOff();
> - data->ipa_->stop();
> - freeBuffers(camera);
> - LOG(RkISP1, Error)
> - << "Failed to start statistics " << camera->id();
> - return ret;
> + ret = stat_->streamOn();
> + if (ret) {
> + param_->streamOff();
> + data->ipa_->stop();
> + freeBuffers(camera);
> + LOG(RkISP1, Error)
> + << "Failed to start statistics " << camera->id();
> + return ret;
> + }
> }
>
> if (data->mainPath_->isEnabled()) {
> @@ -903,15 +918,17 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> selfPath_.stop();
> mainPath_.stop();
>
> - ret = stat_->streamOff();
> - if (ret)
> - LOG(RkISP1, Warning)
> - << "Failed to stop statistics for " << camera->id();
> + if (!isRaw_) {
> + ret = stat_->streamOff();
> + if (ret)
> + LOG(RkISP1, Warning)
> + << "Failed to stop statistics for " << camera->id();
>
> - ret = param_->streamOff();
> - if (ret)
> - LOG(RkISP1, Warning)
> - << "Failed to stop parameters for " << camera->id();
> + ret = param_->streamOff();
> + if (ret)
> + LOG(RkISP1, Warning)
> + << "Failed to stop parameters for " << camera->id();
> + }
>
> ASSERT(data->queuedRequests_.empty());
> data->frameInfo_.clear();
> @@ -925,12 +942,21 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> {
> RkISP1CameraData *data = cameraData(camera);
>
> - RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
> + RkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);
> if (!info)
> return -ENOENT;
>
> data->ipa_->queueRequest(data->frame_, request->controls());
> - data->ipa_->fillParamsBuffer(data->frame_, info->paramBuffer->cookie());
> + if (isRaw_) {
> + if (info->mainPathBuffer)
> + data->mainPath_->queueBuffer(info->mainPathBuffer);
> +
> + if (data->selfPath_ && info->selfPathBuffer)
> + data->selfPath_->queueBuffer(info->selfPathBuffer);
> + } else {
> + data->ipa_->fillParamsBuffer(data->frame_,
> + info->paramBuffer->cookie());
> + }
>
> data->frame_++;
>
> @@ -1135,7 +1161,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> if (!info->metadataProcessed)
> return;
>
> - if (!info->paramDequeued)
> + if (!isRaw_ && !info->paramDequeued)
> return;
>
> data->frameInfo_.destroy(info->frame);
> @@ -1152,16 +1178,28 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> if (!info)
> return;
>
> + const FrameMetadata &metadata = buffer->metadata();
> Request *request = buffer->request();
>
> - /*
> - * Record the sensor's timestamp in the request metadata.
> - *
> - * \todo The sensor timestamp should be better estimated by connecting
> - * to the V4L2Device::frameStart signal.
> - */
> - request->metadata().set(controls::SensorTimestamp,
> - buffer->metadata().timestamp);
> + if (metadata.status != FrameMetadata::FrameCancelled) {
> + /*
> + * Record the sensor's timestamp in the request metadata.
> + *
> + * \todo The sensor timestamp should be better estimated by connecting
> + * to the V4L2Device::frameStart signal.
> + */
> + request->metadata().set(controls::SensorTimestamp,
> + metadata.timestamp);
> +
> + if (isRaw_) {
> + const ControlList &ctrls =
> + data->delayedCtrls_->get(metadata.sequence);
> + data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> + }
> + } else {
> + if (isRaw_)
> + info->metadataProcessed = true;
> + }
>
> completeBuffer(request, buffer);
> tryCompleteRequest(info);
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list