[{"id":20422,"web_url":"https://patchwork.libcamera.org/comment/20422/","msgid":"<163515303235.175846.9127010523437740944@Monstersaurus>","date":"2021-10-25T09:10:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Enable to\n\tset a test pattern mode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nQuoting Hirokazu Honda (2021-10-05 05:37:23)\n> This adds a function to set a camera sensor driver a test pattern\n> mode.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/camera_sensor.h | 5 +++\n> src/libcamera/camera_sensor.cpp | 46 +++++++++++++++++++---\n> 2 files changed, 45 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d25a1165..df35df74 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -24,6 +24,7 @@\n> namespace libcamera {\n> \n> class BayerFormat;\n> +struct CameraSensorProperties;\n\nTo maintain sort ordering, I'd put this in a separate group after the\nclass forward declarations.\n\n> class MediaEntity;\n> \n> class CameraSensor : protected Loggable\n> @@ -44,6 +45,7 @@ public:\n> {\n> return testPatternModes_;\n> }\n> + int setTestPatternMode(int32_t testPatternMode);\n> \n> V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> const Size &size) const;\n> @@ -78,6 +80,8 @@ private:\n> std::unique_ptr subdev_;\n> unsigned int pad_;\n> \n> + const CameraSensorProperties *props_;\n> +\n> std::string model_;\n> std::string id_;\n> \n> @@ -85,6 +89,7 @@ private:\n> std::vector mbusCodes_;\n> std::vector sizes_;\n> std::vector testPatternModes_;\n> + int32_t testPatternMode_;\n> \n> Size pixelArraySize_;\n> Rectangle activeArea_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 9fdb8c09..25b8c626 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -54,8 +54,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> * Once constructed the instance must be initialized with init().\n> */\n> CameraSensor::CameraSensor(const MediaEntity *entity)\n> - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> - properties_(properties::properties)\n> + : entity_(entity), pad_(UINT_MAX), testPatternMode_(-1),\n> + bayerFormat_(nullptr), properties_(properties::properties)\n> {\n> }\n> \n> @@ -300,14 +300,14 @@ void CameraSensor::initVimcDefaultProperties()\n> \n> void CameraSensor::initStaticProperties()\n> {\n> - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> - if (!props)\n> + props_ = CameraSensorProperties::get(model_);\n> + if (!props_)\n\nI know it didn't have one before, but I wonder if we should print a\nDEBUG or WARNING level print here? It seems that the properties are\ngoing to be used more, so if they arent' available for a given sensor, a\nwarning could be helpful.\n\n> return;\n> \n> /* Register the properties retrieved from the sensor database. */\n> - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> + properties_.set(properties::UnitCellSize, props_->unitCellSize);\n> \n> - initTestPatternModes(props->testPatternModes);\n> + initTestPatternModes(props_->testPatternModes);\n> }\n> \n> void CameraSensor::initTestPatternModes(\n> @@ -531,6 +531,40 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n> \n> +/**\n> + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> + * \\param[in] testPatternMode test pattern mode control value to set the camera\n> + * sensor\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setTestPatternMode(int32_t testPatternMode)\n> +{\n> + if (testPatternMode_ == testPatternMode)\n> + return 0;\n> +\n> + if (!props_)\n\nUnless it's better to print only when the properties that were needed\nwere not found?\n\nBut other than that, it looks quite straightforward.\n\nReviewed-by: Kieran Bingham \n\n> + return -EINVAL;\n> +\n> + auto it = props_->testPatternModes.find(testPatternMode);\n> + if (it == props_->testPatternModes.end()) {\n> + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> + << testPatternMode;\n> + return -EINVAL;\n> + }\n> +\n> + ControlList ctrls{ controls() };\n> + ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> +\n> + int ret = setControls(&ctrls);\n> + if (ret)\n> + return ret;\n> +\n> + testPatternMode_ = testPatternMode;\n> +\n> + return 0;\n> +}\n> +\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> -- \n> 2.33.0.800.g4c38ced690-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7A4EFBF415\n\tfor ;\n\tMon, 25 Oct 2021 09:10:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2AEA64870;\n\tMon, 25 Oct 2021 11:10:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D066C60124\n\tfor ;\n\tMon, 25 Oct 2021 11:10:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A754E0A;\n\tMon, 25 Oct 2021 11:10:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GxoFDsHF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635153034;\n\tbh=9O7QF7l1hNI7bkRhdTiAh1UKz9PQJBHESWo/v5REzK4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=GxoFDsHF5YRBAaVeeC+zoEf0MReVE8Q9+uOBtNnmlQzutJdzqr/BhDHLFCHxDu4fz\n\tSF9RjZ6oBt+Lyb5gi/0Pi3C1Y8HWbC9cVBpEjGWx/y8MpC6aJU3GYaU+Yb+ZAPMGhY\n\tfQbsr8QEnT79p5Hu8QpuRrCaZQlQjArO5suWWHsU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211005043723.568685-1-hiroh@chromium.org>","References":"<20211005043723.568685-1-hiroh@chromium.org>","From":"Kieran Bingham ","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","Date":"Mon, 25 Oct 2021 10:10:32 +0100","Message-ID":"<163515303235.175846.9127010523437740944@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Enable to\n\tset a test pattern mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]