[{"id":28056,"web_url":"https://patchwork.libcamera.org/comment/28056/","msgid":"<20231106000044.GC15635@pendragon.ideasonboard.com>","date":"2023-11-06T00:00:44","subject":"Re: [libcamera-devel] [PATCH v3 0/2] Add read-only flag to\n\tControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patches.\n\nOn Thu, Nov 02, 2023 at 05:24:37PM +0000, Kieran Bingham via libcamera-devel wrote:\n> This small patch series adds a read-only flag to ControlInfo. This is used\n> to test if a control, in particular V4L2_CID_HBLANK, can be written to by\n> userland. It replaces the slightly fragile workaround we have right now where\n> we test if the min and max values of a control are the same to determine if\n> a control is read-only.\n\nWhile that's indeed a bit of a hack, do we have drivers that expose\nHBLANK as read-only with different minimum and maximum values ? I don't\nsee any mention in the cover letter or in the patches of what problem(s)\nthis series fixes.\n\n> Only modified one ControlInfo constructor is modified which is used by the\n> V4L2Device class to allow this flag to be set, as setting it for a non-v4l2\n> control probably does not make sense at this point.\n\nThis is the part that bothers me a bit. If the feature is only used\ninternally, it shouldn't be exposed in the public API.\n\nOne possible workaround would be to add flag controls as being settable\nin a request and as being reported in metadata. This is a feature that\nis useful for applications, and it could then be used internally do\nindicate read-only internal controls.\n\n> Modifying other ControlInfo constructors fails quite quickly falling\n> into a range of template issues due to multiple signatures matching.\n> \n> The alternative here would be to rework this such that the ReadOnly flag\n> could be set /after/ construction with a setter allowing something like:\n> \n> V4L2_CTRL_TYPE_INTEGER64:\n> \t\treturn ControlInfo(static_cast(ctrl.minimum),\n> \t\t\t\t static_cast(ctrl.maximum),\n> -\t\t\t\t static_cast(ctrl.default_value));\n> +\t\t\t\t static_cast(ctrl.default_value))\n> \t\t\t\t\t.setReadOnly(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY);\n> \n> Of course that feels like it could remove the 'contract' of the read\n> only property by opening up the ability for a ReadOnly control to have\n> the ReadOnly flag unset - or by opening up the ability to arbitrarily\n> affect a 'property' of a ControlInfo after it has been made.\n> \n> \n> Since v2, I have made the following changes to Naush' patches:\n> - Initialised ReadOnly flag as false for ControlInfo(Span<>)\n> - Improved documentation\n> \n> Naushir Patuck (2):\n> libcamera: controls: Add read-only flag to ControlInfo\n> libcamera:camera_sensor, ipa: raspberrypi: Test readOnly() for HBLANK\n> control\n> \n> include/libcamera/controls.h | 5 ++++-\n> src/ipa/rpi/common/ipa_base.cpp | 12 +-----------\n> src/libcamera/camera_sensor.cpp | 14 ++------------\n> src/libcamera/controls.cpp | 22 ++++++++++++++++++----\n> src/libcamera/v4l2_device.cpp | 12 ++++++++----\n> 5 files changed, 33 insertions(+), 32 deletions(-)","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 4FC2FBDE6B\n\tfor ;\n\tMon, 6 Nov 2023 00:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6BFB629AA;\n\tMon, 6 Nov 2023 01:00:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14EBD61DC5\n\tfor ;\n\tMon, 6 Nov 2023 01:00:38 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAFA6DC9;\n\tMon, 6 Nov 2023 01:00:17 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699228839;\n\tbh=CUEBQCQHlmp8ctOu/Yl2NgStseBjTyt00QNI+1WZ2X8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=O48fTdvdf50RoU9jZBrQdj09EaLevtyqsV15ZZ7rNYog18zPz7B3caH09cKU6xOME\n\tppE+V8zZy5Ipi1F1ktLGBvExnxAn+BT5iMWqZZbQTz1HdYt5kAMNUwYNFXDVeG9K7a\n\tAF7OhrwLpe9MsUyEk2SDpNzDUcVHH5/IkZtt3HfNIx2NRXv2lzek6UJ3RlZwMpjuyv\n\tlX1RXr9O+05tUk8rX/dd5gzpQVGn63r1giAjvOUtyimzTkfqFJ6t6XV5P3kV9a+Q+x\n\timoPZ4+m6jHhf9nnvrrzBteSpAC1ifLEJDxmVOt4FdhgbGUTlkZyt4SK7n19bqZw8s\n\tY/yqcb+HFCq2w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1699228818;\n\tbh=CUEBQCQHlmp8ctOu/Yl2NgStseBjTyt00QNI+1WZ2X8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ISnSxSdVNgsT/cDkczC+Sk14ccOWGcMV3bKCn4dtBNjQWT6/iqmxNK7rrzk5ojliu\n\tdCbsqCjd8jJPKsVnUx3XNCQPl7+uGSQXdXyg79gjIdMrMAztiqxkCsCfdU4Ca0DZ6V\n\tvciyo15yEBbNdWbnlRTsOVNKMd7t7fMkfjql8a2k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ISnSxSdV\"; dkim-atps=neutral","Date":"Mon, 6 Nov 2023 02:00:44 +0200","To":"Kieran Bingham ","Message-ID":"<20231106000044.GC15635@pendragon.ideasonboard.com>","References":"<20231102172439.3543870-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231102172439.3543870-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 0/2] Add read-only flag to\n\tControlInfo","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]