From patchwork Thu Mar 4 08:17:22 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 11487 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 96E53BD80E for ; Thu, 4 Mar 2021 08:17:39 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7DAEB68A9C; Thu, 4 Mar 2021 09:17:38 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="XvayiDeW"; dkim-atps=neutral Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E958602EC for ; Thu, 4 Mar 2021 09:17:37 +0100 (CET) Received: by mail-wr1-x42a.google.com with SMTP id u16so8587593wrt.1 for ; Thu, 04 Mar 2021 00:17:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=nQyawJzd3tRJ8JoKcC5tO+9/7UXwcSecMyb9bdIf+D4=; b=XvayiDeWgh1f1lhdOFWRaqZS5LVddPPM4WJW+inaGii3EbhoK0C7zkRwpqawmCXsze tuDP9afrg8qqLk6wAzzdb3uDhg9zhlyfF2rWvyG2kZRH5BFu7MWKmNpYoVCHvj+myI2n JXXDDeZOmywl4NM10wNs0DAG4IA8DhqIgw/LQY3+2PDubCrCVZtggDAG/nS0aPqwIsT7 vhd1ebpmLIFcbFWtmHWoah2LyPrzktfJ0irxIB3PEPkYOgMQ4WF7x3pUVCzP6PaLjQKH CkX0EOPafJstXdvufZjwK7ac1MYSMQXoKCy2bgu8jzDrdoxzS/eLqohbPWRZmLAnuzdg i1lg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=nQyawJzd3tRJ8JoKcC5tO+9/7UXwcSecMyb9bdIf+D4=; b=cdZ+rVoAnKpFthuwCQOSQDw0oS0ti4oO28SRsChP/8xvhzxzn2O9f0+Cy78Dvepzdq Yh0x6SNehJOCGShoemwArYkZCXQZQRGN+WbDT/NVUhT4irSoJew/NSfB58k65xH5jMGz 46JyVeNUjOHX8J77X+33DHJtM4Sbif0bSTaizOmfTbBjcuKSucHN6XKPmbpqzJENhdHa oM5/7symHl3JYlzbSlwbt9ymTHWXt8pZ0gp2VgXeqMk2NUgELlcGXHRVlFIxfIO+CHdo dnupYoX1+LNUvp8X6mOF+E55K2zV891nzHqv6mRNTv21jwPIqFVBgaGuv9gzjMewGNVQ /qhg== X-Gm-Message-State: AOAM5322qxhVmhm9eu0hASsf2il7doNL6U9ahBntWgOJmjLMN69LTv5S Mx6CILCiBXviK/ypaTt49EJYVxJfBytc5ge+ X-Google-Smtp-Source: ABdhPJwAVHNl9v672/OG1grqY13vv7KYvmQg52vizWr+iVLjDusp24LFv2a1P/AR+wvd/FJF8pQYng== X-Received: by 2002:adf:b60f:: with SMTP id f15mr2677846wre.83.1614845856375; Thu, 04 Mar 2021 00:17:36 -0800 (PST) Received: from naush-laptop.patuck.local ([88.97.76.4]) by smtp.gmail.com with ESMTPSA id b186sm6997926wmc.44.2021.03.04.00.17.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Mar 2021 00:17:36 -0800 (PST) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Date: Thu, 4 Mar 2021 08:17:22 +0000 Message-Id: <20210304081728.1058394-2-naush@raspberrypi.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210304081728.1058394-1-naush@raspberrypi.com> References: <20210304081728.1058394-1-naush@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 1/7] libcamera: delayed_controls: Add notion of priority write X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" If an exposure time change adjusts the vblanking limits, and we set both VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the latter may fail if the value is outside of the limits calculated by the old VBLANK value. This is a limitation in V4L2 and cannot be fixed by setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl. The workaround here is to have the DelayedControls object mark the VBLANK control as "priority write", which then write VBLANK separately from (and ahead of) any other controls. This way, the sensor driver will update the EXPOSURE control with new limits before the new values is presented, and will thus be seen as valid. To support this, a new struct DelayedControls::ControlParams is used in the constructor to provide the control delay value as well as the priority write flag. Signed-off-by: Naushir Patuck Tested-by: David Plowman Reviewed-by: Paul Elder Tested-by: Jean-Michel Hautbois Reviewed-by: Kieran Bingham --- include/libcamera/internal/delayed_controls.h | 9 +++- src/libcamera/delayed_controls.cpp | 54 +++++++++++++------ src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-- .../pipeline/raspberrypi/raspberrypi.cpp | 13 +++-- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-- test/delayed_contols.cpp | 20 +++---- 6 files changed, 68 insertions(+), 44 deletions(-) diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h index dc447a882514..564d9f2e2440 100644 --- a/include/libcamera/internal/delayed_controls.h +++ b/include/libcamera/internal/delayed_controls.h @@ -19,8 +19,13 @@ class V4L2Device; class DelayedControls { public: + struct ControlParams { + unsigned int delay; + bool priorityWrite; + }; + DelayedControls(V4L2Device *device, - const std::unordered_map &delays); + const std::unordered_map &controlParams); void reset(); @@ -64,7 +69,7 @@ private: V4L2Device *device_; /* \todo Evaluate if we should index on ControlId * or unsigned int */ - std::unordered_map delays_; + std::unordered_map controlParams_; unsigned int maxDelay_; bool running_; diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index ab1d40057c5f..3ed1dfebd035 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls) /** * \brief Construct a DelayedControls instance * \param[in] device The V4L2 device the controls have to be applied to - * \param[in] delays Map of the numerical V4L2 control ids to their associated - * delays (in frames) + * \param[in] controlParams Map of the numerical V4L2 control ids to their + * associated control parameters. * - * Only controls specified in \a delays are handled. If it's desired to mix - * delayed controls and controls that take effect immediately the immediate - * controls must be listed in the \a delays map with a delay value of 0. + * The control parameters comprise of delays (in frames) and a priority write + * flag. If this flag is set, the relevant control is written separately from, + * ahead of the reset of the batched controls. + * + * Only controls specified in \a controlParams are handled. If it's desired to + * mix delayed controls and controls that take effect immediately the immediate + * controls must be listed in the \a controlParams map with a delay value of 0. */ DelayedControls::DelayedControls(V4L2Device *device, - const std::unordered_map &delays) + const std::unordered_map &controlParams) : device_(device), maxDelay_(0) { const ControlInfoMap &controls = device_->controls(); @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device, * Create a map of control ids to delays for controls exposed by the * device. */ - for (auto const &delay : delays) { - auto it = controls.find(delay.first); + for (auto const ¶m : controlParams) { + auto it = controls.find(param.first); if (it == controls.end()) { LOG(DelayedControls, Error) << "Delay request for control id " - << utils::hex(delay.first) + << utils::hex(param.first) << " but control is not exposed by device " << device_->deviceNode(); continue; @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device, const ControlId *id = it->first; - delays_[id] = delay.second; + controlParams_[id] = param.second; LOG(DelayedControls, Debug) - << "Set a delay of " << delays_[id] + << "Set a delay of " << controlParams_[id].delay + << " and priority write flag " << controlParams_[id].priorityWrite << " for " << id->name(); - maxDelay_ = std::max(maxDelay_, delays_[id]); + maxDelay_ = std::max(maxDelay_, controlParams_[id].delay); } reset(); @@ -97,8 +102,8 @@ void DelayedControls::reset() /* Retrieve control as reported by the device. */ std::vector ids; - for (auto const &delay : delays_) - ids.push_back(delay.first->id()); + for (auto const ¶m : controlParams_) + ids.push_back(param.first->id()); ControlList controls = device_->getControls(ids); @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls) const ControlId *id = it->second; - if (delays_.find(id) == delays_.end()) + if (controlParams_.find(id) == controlParams_.end()) return false; Info &info = values_[id][queueCount_]; @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence) ControlList out(device_->controls()); for (const auto &ctrl : values_) { const ControlId *id = ctrl.first; - unsigned int delayDiff = maxDelay_ - delays_[id]; + unsigned int delayDiff = maxDelay_ - controlParams_[id].delay; unsigned int index = std::max(0, writeCount_ - delayDiff); const Info &info = ctrl.second[index]; if (info.updated) { - out.set(id->id(), info); + if (controlParams_[id].priorityWrite) { + /* + * This control must be written now, it could + * affect validity of the other controls. + */ + ControlList priority(device_->controls()); + priority.set(id->id(), info); + device_->setControls(&priority); + } else { + /* + * Batch up the list of controls and write them + * at the end of the function. + */ + out.set(id->id(), info); + } + LOG(DelayedControls, Debug) << "Setting " << id->name() << " to " << info.toString() diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 3e6b88af4188..ac92f066a07e 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras() * a sensor database. For now use generic values taken from * the Raspberry Pi and listed as 'generic values'. */ - std::unordered_map delays = { - { V4L2_CID_ANALOGUE_GAIN, 1 }, - { V4L2_CID_EXPOSURE, 2 }, + std::unordered_map params = { + { V4L2_CID_ANALOGUE_GAIN, { 1, false } }, + { V4L2_CID_EXPOSURE, { 2, false } }, }; data->delayedCtrls_ = std::make_unique(cio2->sensor()->device(), - delays); + params); data->cio2_.frameStart().connect(data->delayedCtrls_.get(), &DelayedControls::applyControls); diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index a60415d93705..ba74ace183db 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) if (result.params & ipa::RPi::ConfigSensorParams) { /* * Setup our delayed control writer with the sensor default - * gain and exposure delays. + * gain and exposure delays. Mark VBLANK for priority write. */ - std::unordered_map delays = { - { V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay }, - { V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay }, - { V4L2_CID_VBLANK, result.sensorConfig.vblank } + std::unordered_map params = { + { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } }, + { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } }, + { V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } } }; - delayedCtrls_ = std::make_unique(unicam_[Unicam::Image].dev(), delays); - + delayedCtrls_ = std::make_unique(unicam_[Unicam::Image].dev(), params); sensorMetadata_ = result.sensorConfig.sensorMetadata; } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index a794501a9c8d..17c0f9751cd3 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) * a sensor database. For now use generic values taken from * the Raspberry Pi and listed as generic values. */ - std::unordered_map delays = { - { V4L2_CID_ANALOGUE_GAIN, 1 }, - { V4L2_CID_EXPOSURE, 2 }, + std::unordered_map params = { + { V4L2_CID_ANALOGUE_GAIN, { 1, false } }, + { V4L2_CID_EXPOSURE, { 2, false } }, }; data->delayedCtrls_ = std::make_unique(data->sensor_->device(), - delays); + params); isp_->frameStart.connect(data->delayedCtrls_.get(), &DelayedControls::applyControls); diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp index 50169b12e566..3855eb18ecd4 100644 --- a/test/delayed_contols.cpp +++ b/test/delayed_contols.cpp @@ -72,8 +72,8 @@ protected: int singleControlNoDelay() { - std::unordered_map delays = { - { V4L2_CID_BRIGHTNESS, 0 }, + std::unordered_map delays = { + { V4L2_CID_BRIGHTNESS, { 0, false } }, }; std::unique_ptr delayed = std::make_unique(dev_.get(), delays); @@ -109,8 +109,8 @@ protected: int singleControlWithDelay() { - std::unordered_map delays = { - { V4L2_CID_BRIGHTNESS, 1 }, + std::unordered_map delays = { + { V4L2_CID_BRIGHTNESS, { 1, false } }, }; std::unique_ptr delayed = std::make_unique(dev_.get(), delays); @@ -150,9 +150,9 @@ protected: int dualControlsWithDelay(uint32_t startOffset) { - std::unordered_map delays = { - { V4L2_CID_BRIGHTNESS, 1 }, - { V4L2_CID_CONTRAST, 2 }, + std::unordered_map delays = { + { V4L2_CID_BRIGHTNESS, { 1, false } }, + { V4L2_CID_CONTRAST, { 2, false } }, }; std::unique_ptr delayed = std::make_unique(dev_.get(), delays); @@ -197,9 +197,9 @@ protected: int dualControlsMultiQueue() { - std::unordered_map delays = { - { V4L2_CID_BRIGHTNESS, 1 }, - { V4L2_CID_CONTRAST, 2 }, + std::unordered_map delays = { + { V4L2_CID_BRIGHTNESS, { 1, false } }, + { V4L2_CID_CONTRAST, { 2, false } } }; std::unique_ptr delayed = std::make_unique(dev_.get(), delays);