From patchwork Sun Jan 24 14:05:06 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 10977 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 3CCB7BD808 for ; Sun, 24 Jan 2021 14:05:19 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C7CDA682A8; Sun, 24 Jan 2021 15:05:18 +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="ft2gtgQI"; dkim-atps=neutral Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 4382568297 for ; Sun, 24 Jan 2021 15:05:17 +0100 (CET) Received: by mail-wm1-x32f.google.com with SMTP id m2so8323198wmm.1 for ; Sun, 24 Jan 2021 06:05:17 -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=XZkrA0htXPzX/u0lkkrrfWXfV5BoVHTbgwzUv5sp3Hg=; b=ft2gtgQIyYh+e5VjH/rBTklKjjyM0ml20i7npXVPu+sH7UuQkfXjmkYBtrSltKOeXH WNo19i2ME4eSiYh3TUbhufTh6ot+EiLLNaEjzF1fkpaBjbsgpcQWsCIgwU9trnO/FqmA OhRuMrz6a3JSjrjkaKr3Y6pWyFUUkIFVg/EKQ3WkB8jLk4lgrJBKN9G5aBVj+5xhhdus rF9edFStX/TR3aB0Jk+itzDlbJuurtw/dApQrRHPBp+fCULOJXzID9mqJJkyFp4g8Nxe /u1fduvPvLL7YOOczeumtGdXzCJwy5sx/y/CSptKnLCkc0CDpY+4w+Uv+XemAYbAMjjy gqsA== 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=XZkrA0htXPzX/u0lkkrrfWXfV5BoVHTbgwzUv5sp3Hg=; b=WRfTurFcnimj78K5JGN1+76zA/dgyc0IDALGnD8tKv7KPuAYDAtRN7e4Gvs5Gj4AYl BA7r5n4XyAGk+Hz6cf0SrQFxZ6mQMUCH0VDD8NPyX3/jaR0reIALowLo62BAVTqOCN4A RqfzYP7raROdgQVAKzWqiRu/f/dcAdbEgY7xdtzoY0y2/XSANIAIYidof3rOXb82RMzr RI9NS8/AdUTG/8ZY05bsac6+0HEeLYg8sLL4uIr1xlCpuFnnTxUhTniz5XcMtjnpx09n KEYyadCSNWM6g/V18dLsH2h7ephx54yU/r3bbUswizZKV/7H5IA8X1zLhTFg2sMKkTPp s3gA== X-Gm-Message-State: AOAM5320Fgc4/aw0JcqrgmkPF49Vq73JZMmx7sHHJs/XfZOiDfMSJLih A90BZm0KpG7N6WNdNw8/R1RqppjSfCBDS34+ X-Google-Smtp-Source: ABdhPJwewkYsPs9UjYzhsoSiDREcSjjv5fx3/98xaS5p3QPXIH2m7hitbVmusffq/rQdbU5k1GyvaA== X-Received: by 2002:a1c:5454:: with SMTP id p20mr3990740wmi.128.1611497116691; Sun, 24 Jan 2021 06:05:16 -0800 (PST) Received: from naushir-VirtualBox.patuck.local ([88.97.76.4]) by smtp.gmail.com with ESMTPSA id u16sm19311053wrn.68.2021.01.24.06.05.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 24 Jan 2021 06:05:15 -0800 (PST) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Date: Sun, 24 Jan 2021 14:05:06 +0000 Message-Id: <20210124140506.786503-4-naush@raspberrypi.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210124140506.786503-1-naush@raspberrypi.com> References: <20210124140506.786503-1-naush@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 4/4] pipeline: raspberrypi: Add notion of immediate write to StaggeredCtrl 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 write both VBLANK and EXPOSURE controls as a set, 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 writing VBLANK before EXPOSURE. The workaround here is to have the StaggeredCtrl mark the VBLANK control as "immediate 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. StaggeredCtrl is due to be deprecated and replaced by DelayedCtrl, so this change serves more a working proof-of-concept on the workaround, and not much care has been taken to provide a nice new API for applying this immediate write flag to the control. A similar workaround must be available to DelayedCtrl eventually. Signed-off-by: Naushir Patuck --- src/ipa/raspberrypi/raspberrypi.cpp | 5 ++- .../pipeline/raspberrypi/raspberrypi.cpp | 11 ++++-- .../pipeline/raspberrypi/staggered_ctrl.cpp | 39 ++++++++++++++----- .../pipeline/raspberrypi/staggered_ctrl.h | 3 +- 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 75c9e404dcc1..fefca32ce187 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1040,8 +1040,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) /* * Due to the behavior of V4L2, the current value of VBLANK could clip the - * exposure time without us knowing. The next time though this function should - * clip exposure correctly. + * exposure time without us knowing. We get around this by ensuring the + * staggered write component submits VBLANK separately from, and before the + * EXPOSURE control. */ ctrls.set(V4L2_CID_VBLANK, vblanking); ctrls.set(V4L2_CID_EXPOSURE, exposureLines); diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 524cc960dd37..1485999ad2a0 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1229,12 +1229,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) /* * Setup our staggered control writer with the sensor default * gain and exposure delays. + * + * VBLANK must be flagged as "immediate write" to allow it to + * be set immediately instead of being batched with all other + * controls. This is needed so that any update to the EXPOSURE + * control will be validated based on the new VBLANK control value. */ if (!staggeredCtrl_) { staggeredCtrl_.init(unicam_[Unicam::Image].dev(), - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, - { V4L2_CID_EXPOSURE, result.data[resultIdx++] }, - { V4L2_CID_VBLANK, result.data[resultIdx++] } }); + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false }, + { V4L2_CID_EXPOSURE, result.data[resultIdx++], false }, + { V4L2_CID_VBLANK, result.data[resultIdx++], true } }); sensorMetadata_ = result.data[resultIdx++]; } } diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp index 62605c0fceee..07f8c95d4f2c 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp @@ -22,21 +22,29 @@ LOG_DEFINE_CATEGORY(RPI_S_W) namespace RPi { void StaggeredCtrl::init(V4L2VideoDevice *dev, - std::initializer_list> delayList) + std::initializer_list> delayList) { std::lock_guard lock(lock_); dev_ = dev; - delay_ = delayList; + delay_.clear(); ctrl_.clear(); - /* Find the largest delay across all controls. */ maxDelay_ = 0; - for (auto const &p : delay_) { + for (auto const &c : delayList) { + uint32_t id = std::get<0>(c); + uint8_t delay = std::get<1>(c); + bool immediateWrite = std::get<2>(c); + + delay_[id] = delay; + immediateWrite_[id] = immediateWrite; + LOG(RPI_S_W, Info) << "Init ctrl " - << utils::hex(p.first) << " with delay " - << static_cast(p.second); - maxDelay_ = std::max(maxDelay_, p.second); + << utils::hex(id) << " with delay " + << static_cast(delay); + + /* Find the largest delay across all controls. */ + maxDelay_ = std::max(maxDelay_, delay); } init_ = true; @@ -121,8 +129,21 @@ int StaggeredCtrl::write() int index = std::max(0, setCount_ - delayDiff); if (p.second[index].updated) { - /* We need to write this value out. */ - controls.set(p.first, p.second[index].value); + if (immediateWrite_[p.first]) { + /* + * This control must be written now, it could + * affect validity of the other controls. + */ + ControlList immediate(dev_->controls()); + immediate.set(p.first, p.second[index].value); + dev_->setControls(&immediate); + } else { + /* + * Batch up the list of controls and write them + * at the end of the function. + */ + controls.set(p.first, p.second[index].value); + } p.second[index].updated = false; LOG(RPI_S_W, Debug) << "Writing ctrl " << utils::hex(p.first) << " to " diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h index 382fa31a6853..7c920c3a13c7 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h @@ -34,7 +34,7 @@ public: } void init(V4L2VideoDevice *dev, - std::initializer_list> delayList); + std::initializer_list> delayList); void reset(); void get(std::unordered_map &ctrl, uint8_t offset = 0); @@ -85,6 +85,7 @@ private: uint8_t maxDelay_; V4L2VideoDevice *dev_; std::unordered_map delay_; + std::unordered_map immediateWrite_; std::unordered_map ctrl_; std::mutex lock_; };