[{"id":14435,"web_url":"https://patchwork.libcamera.org/comment/14435/","msgid":"","date":"2021-01-04T13:08:28","subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-12-31 00:05:59 +0100, Jacopo Mondi wrote:\n> The CameraSensor class requires the sensor driver to report\n> information through V4L2 controls and through the V4L2 selection API,\n> and uses that information to register Camera properties and to\n> construct CameraSensorInfo class instances to provide them to the IPA.\n> \n> Currently, validation of the kernel support happens each time a\n> feature is requested, with slighly similar debug/error messages\n> output to the user in case a feature is not supported.\n> \n> Rationalize this by validating the sensor driver requirements in a\n> single function\n> \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Paul Elder \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 84 ++++++++++++++++++++++\n> 2 files changed, 85 insertions(+)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index f80d836161a5..aee10aa6e3c7 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -69,6 +69,7 @@ protected:\n> \n> private:\n> \tint generateId();\n> +\tint validateSensorDriver();\n> \tint initProperties();\n> \n> \tconst MediaEntity *entity_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index e786821d4ba2..048fcd6a1fae 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -207,6 +207,10 @@ int CameraSensor::init()\n> \t */\n> \tresolution_ = sizes_.back();\n> \n> +\tret = validateSensorDriver();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tret = initProperties();\n> \tif (ret)\n> \t\treturn ret;\n> @@ -214,6 +218,86 @@ int CameraSensor::init()\n> \treturn 0;\n> }\n> \n> +int CameraSensor::validateSensorDriver()\n> +{\n> +\t/*\n> +\t * Make sure the sensor driver supports the mandatory controls\n> +\t * required by the CameraSensor class.\n> +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> +\t */\n> +\tconst std::vector mandatoryControls{\n> +\t\tV4L2_CID_PIXEL_RATE,\n> +\t\tV4L2_CID_HBLANK,\n> +\t};\n> +\n> +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> +\tif (ctrls.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n\nHere and below I think this should be reworked to a single LOG() call. \nWhen we go more multithreading there is a possibility other threads may \ninject other LOG() calls between the ones here which may be confusing, \nhow about?\n\nLOG(CameraSensor, Error)\n << \"Mandatory V4L2 controls not available\" << std::endl\n << \"The sensor kernel driver needs to be fixed\" << std::endl\n << \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n\nWith this fixed,\n\nReviewed-by: Niklas Söderlund \n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Optional controls are used to register optional sensor properties. If\n> +\t * not present, some values will be defaulted.\n> +\t */\n> +\tconst std::vector optionalControls{\n> +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\t};\n> +\n> +\tctrls = subdev_->getControls(optionalControls);\n> +\tif (ctrls.empty())\n> +\t\tLOG(CameraSensor, Debug) << \"Optional V4L2 controls not supported\";\n> +\n> +\t/*\n> +\t * Make sure the required selection targets are supported.\n> +\t *\n> +\t * Failures in reading any of the targets are not deemed to be fatal,\n> +\t * but some properties and features, like constructing a\n> +\t * CameraSensorInfo for the IPA module, won't be supported.\n> +\t *\n> +\t * \\todo Make support for selection targets mandatory as soon as all\n> +\t * test platforms have been updated.\n> +\t */\n> +\tint err = 0;\n> +\tRectangle rect;\n> +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Warning)\n> +\t\t\t<< \"Failed to retrieve the readable pixel array size\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Warning)\n> +\t\t\t<< \"Failed to retrieve the active pixel array size\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Warning)\n> +\t\t\t<< \"Failed to retrieve the sensor crop rectangle\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tif (err) {\n> +\t\tLOG(CameraSensor, Warning)\n> +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> +\t\tLOG(CameraSensor, Warning)\n> +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> int CameraSensor::initProperties()\n> {\n> \t/*\n> -- \n> 2.29.2\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 211E5C0F1A\n\tfor ;\n\tMon, 4 Jan 2021 13:08:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB08162006;\n\tMon, 4 Jan 2021 14:08:31 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5986E60110\n\tfor ;\n\tMon, 4 Jan 2021 14:08:30 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id l11so64195542lfg.0\n\tfor ;\n\tMon, 04 Jan 2021 05:08:30 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb12sm7276889lfb.139.2021.01.04.05.08.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 04 Jan 2021 05:08:29 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"1y8WcoyT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=550FrPcnOnzqyWoyHgcwGoz9gem/pVT4dm3eXm1cmMk=;\n\tb=1y8WcoyTWzCpUT9p4mqo6rSnJ93QKl6vKQ7Zen2Tvd4rJX8wPCS6MGS06ou2ogw1b9\n\thSbau8i3sWUmjdM4/lURELUq1zr5Y2M3RChP34hPpRJqjNkys70qyIt9d59V8JyngpWR\n\tJ049mluV7WhkNhGfxbDWjNU2tJgBp2DabOT0FJdIO70NDioLwPf5cQqZBPQA5o6wxdKv\n\tWDCtGZT/vcyI0Oe3EdQuDjcbikW0s5q/lMnDa/AOLtRN5jfXe+xyKRVAjM0m9JlD53Y0\n\tIVSTHHw6GndnX+pC7UuZjHdDYOSFj0DAuWJtksP+Md0vbjRmJtQyL0yLaji9oYqHjgVP\n\thLSQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=550FrPcnOnzqyWoyHgcwGoz9gem/pVT4dm3eXm1cmMk=;\n\tb=hHH3AXs4aNB1EsCzqgB3MQZ/BX2fA3exQbji2XfAeD4C6VxBCprJz4FpXzJ08uw/AV\n\tY4waAn9ekJktqeqNlpjIfs19hSJTMYGp+75X7/7UvcwsHcWPXB96f0rkPPqupGZgkCLM\n\tHERfSL2WBWWJA+o5fc8oxIDyq36VXS5tXtJlutGVwm0lupQBMY1D+LNpYAAKy0tM2xaC\n\tUjSBpQIrS+umQU+2gh6V4yMQOL7Npvb/4sfD/8C7ngpCnvlr6Ap71WmNcXoS8iE0Gz5t\n\tNv1imscQ3D4Nv6Bo4ykBoPY/eiaAqOHrOz+VJWK0Eg1UmSR8dXByN1oppyLcJRMbg/d5\n\t52vw==","X-Gm-Message-State":"AOAM530mNFVoypmgv301X7noKIgtX8avO/8St1vTmuBexp7kuLjITzDC\n\t9i1nnrwqsGPIMPPyLQrefL/aKyJ6kz5y0A==","X-Google-Smtp-Source":"ABdhPJyQ2EbtSHxIAZ2e1kcH6Z9yPzStD9DXUiA8faxS80uNGZ1vckBkgt140PuoPQYX3fbdNepg0A==","X-Received":"by 2002:a2e:b701:: with SMTP id\n\tj1mr38231927ljo.134.1609765709719; \n\tMon, 04 Jan 2021 05:08:29 -0800 (PST)","Date":"Mon, 4 Jan 2021 14:08:28 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201230230603.123486-1-jacopo@jmondi.org>\n\t<20201230230603.123486-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201230230603.123486-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14437,"web_url":"https://patchwork.libcamera.org/comment/14437/","msgid":"","date":"2021-01-04T13:12:23","subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jan 04, 2021 at 02:08:28PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n> \n> Thanks for your work.\n> \n> On 2020-12-31 00:05:59 +0100, Jacopo Mondi wrote:\n> > The CameraSensor class requires the sensor driver to report\n> > information through V4L2 controls and through the V4L2 selection API,\n> > and uses that information to register Camera properties and to\n> > construct CameraSensorInfo class instances to provide them to the IPA.\n> > \n> > Currently, validation of the kernel support happens each time a\n> > feature is requested, with slighly similar debug/error messages\n> > output to the user in case a feature is not supported.\n> > \n> > Rationalize this by validating the sensor driver requirements in a\n> > single function\n> > \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 84 ++++++++++++++++++++++\n> > 2 files changed, 85 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index f80d836161a5..aee10aa6e3c7 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -69,6 +69,7 @@ protected:\n> > \n> > private:\n> > \tint generateId();\n> > +\tint validateSensorDriver();\n> > \tint initProperties();\n> > \n> > \tconst MediaEntity *entity_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index e786821d4ba2..048fcd6a1fae 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -207,6 +207,10 @@ int CameraSensor::init()\n> > \t */\n> > \tresolution_ = sizes_.back();\n> > \n> > +\tret = validateSensorDriver();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \tret = initProperties();\n> > \tif (ret)\n> > \t\treturn ret;\n> > @@ -214,6 +218,86 @@ int CameraSensor::init()\n> > \treturn 0;\n> > }\n> > \n> > +int CameraSensor::validateSensorDriver()\n> > +{\n> > +\t/*\n> > +\t * Make sure the sensor driver supports the mandatory controls\n> > +\t * required by the CameraSensor class.\n> > +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> > +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> > +\t */\n> > +\tconst std::vector mandatoryControls{\n> > +\t\tV4L2_CID_PIXEL_RATE,\n> > +\t\tV4L2_CID_HBLANK,\n> > +\t};\n> > +\n> > +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > +\tif (ctrls.empty()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> \n> Here and below I think this should be reworked to a single LOG() call. \n> When we go more multithreading there is a possibility other threads may \n> inject other LOG() calls between the ones here which may be confusing, \n> how about?\n> \n> LOG(CameraSensor, Error)\n> << \"Mandatory V4L2 controls not available\" << std::endl\n> << \"The sensor kernel driver needs to be fixed\" << std::endl\n> << \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n\nThen we won't have the timestamp and other prefixes added to the lines\nbeside the first one :-S\n\n> With this fixed,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Optional controls are used to register optional sensor properties. If\n> > +\t * not present, some values will be defaulted.\n> > +\t */\n> > +\tconst std::vector optionalControls{\n> > +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > +\t};\n> > +\n> > +\tctrls = subdev_->getControls(optionalControls);\n> > +\tif (ctrls.empty())\n> > +\t\tLOG(CameraSensor, Debug) << \"Optional V4L2 controls not supported\";\n> > +\n> > +\t/*\n> > +\t * Make sure the required selection targets are supported.\n> > +\t *\n> > +\t * Failures in reading any of the targets are not deemed to be fatal,\n> > +\t * but some properties and features, like constructing a\n> > +\t * CameraSensorInfo for the IPA module, won't be supported.\n> > +\t *\n> > +\t * \\todo Make support for selection targets mandatory as soon as all\n> > +\t * test platforms have been updated.\n> > +\t */\n> > +\tint err = 0;\n> > +\tRectangle rect;\n> > +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t<< \"Failed to retrieve the readable pixel array size\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t<< \"Failed to retrieve the active pixel array size\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t<< \"Failed to retrieve the sensor crop rectangle\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tif (err) {\n> > +\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > +\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int CameraSensor::initProperties()\n> > {\n> > \t/*","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 F034DC0F1C\n\tfor ;\n\tMon, 4 Jan 2021 13:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF5CD62009;\n\tMon, 4 Jan 2021 14:12:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9111F60110\n\tfor ;\n\tMon, 4 Jan 2021 14:12:36 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23AF72E0;\n\tMon, 4 Jan 2021 14:12:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Yimuaoaa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609765956;\n\tbh=7sxOD6ZNCCeeMGLE7uzXfRnhaCVznwnsz9aiVp6VqK0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yimuaoaa/OWxS5YP0RlEgy8y/w/c11jq5uVUPZU/E6lR+MW4vu1ctTLkVyC36a0kw\n\t+D9t+C12bDZwif5+RVCmTD+VrXrKjsLEmgbwtE+bZSWMZgf9LwaPIQkcRL0N63bRXn\n\tTIKKzs546rpv993aco3M92Is+QVfN9zxQbf3uCaE=","Date":"Mon, 4 Jan 2021 15:12:23 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201230230603.123486-1-jacopo@jmondi.org>\n\t<20201230230603.123486-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14439,"web_url":"https://patchwork.libcamera.org/comment/14439/","msgid":"","date":"2021-01-04T13:19:02","subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2021-01-04 15:12:23 +0200, Laurent Pinchart wrote:\n> On Mon, Jan 04, 2021 at 02:08:28PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> > \n> > Thanks for your work.\n> > \n> > On 2020-12-31 00:05:59 +0100, Jacopo Mondi wrote:\n> > > The CameraSensor class requires the sensor driver to report\n> > > information through V4L2 controls and through the V4L2 selection API,\n> > > and uses that information to register Camera properties and to\n> > > construct CameraSensorInfo class instances to provide them to the IPA.\n> > > \n> > > Currently, validation of the kernel support happens each time a\n> > > feature is requested, with slighly similar debug/error messages\n> > > output to the user in case a feature is not supported.\n> > > \n> > > Rationalize this by validating the sensor driver requirements in a\n> > > single function\n> > > \n> > > Reviewed-by: Laurent Pinchart \n> > > Reviewed-by: Paul Elder \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > src/libcamera/camera_sensor.cpp | 84 ++++++++++++++++++++++\n> > > 2 files changed, 85 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index f80d836161a5..aee10aa6e3c7 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -69,6 +69,7 @@ protected:\n> > > \n> > > private:\n> > > \tint generateId();\n> > > +\tint validateSensorDriver();\n> > > \tint initProperties();\n> > > \n> > > \tconst MediaEntity *entity_;\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index e786821d4ba2..048fcd6a1fae 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -207,6 +207,10 @@ int CameraSensor::init()\n> > > \t */\n> > > \tresolution_ = sizes_.back();\n> > > \n> > > +\tret = validateSensorDriver();\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > \tret = initProperties();\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > @@ -214,6 +218,86 @@ int CameraSensor::init()\n> > > \treturn 0;\n> > > }\n> > > \n> > > +int CameraSensor::validateSensorDriver()\n> > > +{\n> > > +\t/*\n> > > +\t * Make sure the sensor driver supports the mandatory controls\n> > > +\t * required by the CameraSensor class.\n> > > +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> > > +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> > > +\t */\n> > > +\tconst std::vector mandatoryControls{\n> > > +\t\tV4L2_CID_PIXEL_RATE,\n> > > +\t\tV4L2_CID_HBLANK,\n> > > +\t};\n> > > +\n> > > +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > > +\tif (ctrls.empty()) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > \n> > Here and below I think this should be reworked to a single LOG() call. \n> > When we go more multithreading there is a possibility other threads may \n> > inject other LOG() calls between the ones here which may be confusing, \n> > how about?\n> > \n> > LOG(CameraSensor, Error)\n> > << \"Mandatory V4L2 controls not available\" << std::endl\n> > << \"The sensor kernel driver needs to be fixed\" << std::endl\n> > << \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> \n> Then we won't have the timestamp and other prefixes added to the lines\n> beside the first one :-S\n\nTrue, but is that really a problem? Are not all three lines part of the \nsame \"message\"? In a perfect world I would rather extend our LOG() \nimplementation to support multiline messages by only printing the \ntimestamp+prefixes for the first line and then indent the rest with \nwhitespaces to make it clear that some lines belongs together.\n\nI do not feel strongly about this so if I'm in minority feel free to \nkeep my R-b while keeping the multiple LOG() calls.\n\n> \n> > With this fixed,\n> > \n> > Reviewed-by: Niklas Söderlund \n> > \n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Optional controls are used to register optional sensor properties. If\n> > > +\t * not present, some values will be defaulted.\n> > > +\t */\n> > > +\tconst std::vector optionalControls{\n> > > +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > +\t};\n> > > +\n> > > +\tctrls = subdev_->getControls(optionalControls);\n> > > +\tif (ctrls.empty())\n> > > +\t\tLOG(CameraSensor, Debug) << \"Optional V4L2 controls not supported\";\n> > > +\n> > > +\t/*\n> > > +\t * Make sure the required selection targets are supported.\n> > > +\t *\n> > > +\t * Failures in reading any of the targets are not deemed to be fatal,\n> > > +\t * but some properties and features, like constructing a\n> > > +\t * CameraSensorInfo for the IPA module, won't be supported.\n> > > +\t *\n> > > +\t * \\todo Make support for selection targets mandatory as soon as all\n> > > +\t * test platforms have been updated.\n> > > +\t */\n> > > +\tint err = 0;\n> > > +\tRectangle rect;\n> > > +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > +\tif (ret) {\n> > > +\t\tLOG(CameraSensor, Warning)\n> > > +\t\t\t<< \"Failed to retrieve the readable pixel array size\";\n> > > +\t\terr = -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > > +\tif (ret) {\n> > > +\t\tLOG(CameraSensor, Warning)\n> > > +\t\t\t<< \"Failed to retrieve the active pixel array size\";\n> > > +\t\terr = -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> > > +\tif (ret) {\n> > > +\t\tLOG(CameraSensor, Warning)\n> > > +\t\t\t<< \"Failed to retrieve the sensor crop rectangle\";\n> > > +\t\terr = -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tif (err) {\n> > > +\t\tLOG(CameraSensor, Warning)\n> > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > +\t\tLOG(CameraSensor, Warning)\n> > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int CameraSensor::initProperties()\n> > > {\n> > > \t/*\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 B44ABC0F1A\n\tfor ;\n\tMon, 4 Jan 2021 13:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 387256200A;\n\tMon, 4 Jan 2021 14:19:06 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABA1160110\n\tfor ;\n\tMon, 4 Jan 2021 14:19:04 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id m25so64140345lfc.11\n\tfor ;\n\tMon, 04 Jan 2021 05:19:04 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv5sm7305861lfd.103.2021.01.04.05.19.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 04 Jan 2021 05:19:03 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"qMcpDjjN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=CY/eCiWeeJUr9HKcOD4r5feUGGtGqIoZkNyWVtv4T1s=;\n\tb=qMcpDjjN+uZRAF3cCVq4/VZcJxEU8ofK6QxdVpH2LPG+Pp1BSdyFf5fxLiRrMzsuli\n\terzF02Q2HBg+0bJiATlEBOrOAn2uC/RPX5NNr4y7qeDfmvAUMV5BFkWyPVuaLjoBSGqA\n\t/yM6WRBJ3i1L76DBQ6bXKYwHSjwl+Dg/NFwFJfEy+iVdhxNRHgtzt/hesdovVe9kih3L\n\t4i+zzyv7YRecfiMPH8l2Ufd3uYL+8DEe2wh9dp1ZdBh4k0WR04cKE2Mw0Ul5wG9/AsPa\n\tLmBJZoT4bGhybCf3SOy+vaKm8z0PLuY8ypqHP1CGl+McaDRyR4suqtJdrtvtGW/EBnFa\n\tLXHg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=CY/eCiWeeJUr9HKcOD4r5feUGGtGqIoZkNyWVtv4T1s=;\n\tb=mQswH7GWJIvCGeXxKmfa0hqiiAXueyaVW9WrkryCWw+MQHn4puTTwpOpbzq+zCeJO2\n\ttKfyFzidHsIXTPWsFQQOQgBrlCVbCsIpgAX6ybSjx9qZ2YM8ipoYi4ZRGcIr1LXWemz1\n\ty98P79eAj44ehuJrxUA/fdx/VRBkS/3+UfouPSzBzrCB/8lXmvM8sPA95+SHeKjnOzcJ\n\tv40VFiZjKDu6Hb72786AfWOVchaY5oPK7wn8NWZv7DIMRXyFMNHS9wfMPQznLrFvy+P8\n\t38wLA4H7yojc2d09fsglOj9DFRpHcufg9bDtEKNe/yU94mE9CqMMqoDVNTw7RrDcJMma\n\tJuCw==","X-Gm-Message-State":"AOAM533T5YA0IbYkVMkXH9f8SoZQugfvXJHeqSEQRm1Jw2sN44Zda6A/\n\t41k3KgcUKJvO69bbtVi9pMdA9UMXYaWFqA==","X-Google-Smtp-Source":"ABdhPJwtoj8BiFNqAtsvJPVKX5NFccJRKcCyla4mRUoOzxcJUeLvlmY2cV8HMvO1xS/Ql2mWPLQXeQ==","X-Received":"by 2002:a05:651c:316:: with SMTP id\n\ta22mr33832296ljp.473.1609766344034; \n\tMon, 04 Jan 2021 05:19:04 -0800 (PST)","Date":"Mon, 4 Jan 2021 14:19:02 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20201230230603.123486-1-jacopo@jmondi.org>\n\t<20201230230603.123486-3-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14443,"web_url":"https://patchwork.libcamera.org/comment/14443/","msgid":"<20210104175614.mf6fn2lalb3l2g7d@uno.localdomain>","date":"2021-01-04T17:56:14","subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Jan 04, 2021 at 02:19:02PM +0100, Niklas Söderlund wrote:\n> On 2021-01-04 15:12:23 +0200, Laurent Pinchart wrote:\n> > On Mon, Jan 04, 2021 at 02:08:28PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2020-12-31 00:05:59 +0100, Jacopo Mondi wrote:\n> > > > The CameraSensor class requires the sensor driver to report\n> > > > information through V4L2 controls and through the V4L2 selection API,\n> > > > and uses that information to register Camera properties and to\n> > > > construct CameraSensorInfo class instances to provide them to the IPA.\n> > > >\n> > > > Currently, validation of the kernel support happens each time a\n> > > > feature is requested, with slighly similar debug/error messages\n> > > > output to the user in case a feature is not supported.\n> > > >\n> > > > Rationalize this by validating the sensor driver requirements in a\n> > > > single function\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > > Reviewed-by: Paul Elder \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > src/libcamera/camera_sensor.cpp | 84 ++++++++++++++++++++++\n> > > > 2 files changed, 85 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index f80d836161a5..aee10aa6e3c7 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -69,6 +69,7 @@ protected:\n> > > >\n> > > > private:\n> > > > \tint generateId();\n> > > > +\tint validateSensorDriver();\n> > > > \tint initProperties();\n> > > >\n> > > > \tconst MediaEntity *entity_;\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index e786821d4ba2..048fcd6a1fae 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -207,6 +207,10 @@ int CameraSensor::init()\n> > > > \t */\n> > > > \tresolution_ = sizes_.back();\n> > > >\n> > > > +\tret = validateSensorDriver();\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > \tret = initProperties();\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > > @@ -214,6 +218,86 @@ int CameraSensor::init()\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +int CameraSensor::validateSensorDriver()\n> > > > +{\n> > > > +\t/*\n> > > > +\t * Make sure the sensor driver supports the mandatory controls\n> > > > +\t * required by the CameraSensor class.\n> > > > +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> > > > +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> > > > +\t */\n> > > > +\tconst std::vector mandatoryControls{\n> > > > +\t\tV4L2_CID_PIXEL_RATE,\n> > > > +\t\tV4L2_CID_HBLANK,\n> > > > +\t};\n> > > > +\n> > > > +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > > > +\tif (ctrls.empty()) {\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > >\n> > > Here and below I think this should be reworked to a single LOG() call.\n> > > When we go more multithreading there is a possibility other threads may\n> > > inject other LOG() calls between the ones here which may be confusing,\n> > > how about?\n> > >\n> > > LOG(CameraSensor, Error)\n> > > << \"Mandatory V4L2 controls not available\" << std::endl\n> > > << \"The sensor kernel driver needs to be fixed\" << std::endl\n> > > << \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> >\n> > Then we won't have the timestamp and other prefixes added to the lines\n> > beside the first one :-S\n>\n> True, but is that really a problem? Are not all three lines part of the\n> same \"message\"? In a perfect world I would rather extend our LOG()\n> implementation to support multiline messages by only printing the\n> timestamp+prefixes for the first line and then indent the rest with\n> whitespaces to make it clear that some lines belongs together.\n>\n> I do not feel strongly about this so if I'm in minority feel free to\n> keep my R-b while keeping the multiple LOG() calls.\n>\n\nPrinting out without timestamp break the logs symmetry, it renders the\noutput really confusing to look at.\n\nPrinting longer lines is not convenient, as it makes the log\nunreadable on smaller screen.\n\nI prefer to keep what I have here if possible.\n\n> >\n> > > With this fixed,\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n\nThanks\n j\n\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\t/*\n> > > > +\t * Optional controls are used to register optional sensor properties. If\n> > > > +\t * not present, some values will be defaulted.\n> > > > +\t */\n> > > > +\tconst std::vector optionalControls{\n> > > > +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> > > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > > +\t};\n> > > > +\n> > > > +\tctrls = subdev_->getControls(optionalControls);\n> > > > +\tif (ctrls.empty())\n> > > > +\t\tLOG(CameraSensor, Debug) << \"Optional V4L2 controls not supported\";\n> > > > +\n> > > > +\t/*\n> > > > +\t * Make sure the required selection targets are supported.\n> > > > +\t *\n> > > > +\t * Failures in reading any of the targets are not deemed to be fatal,\n> > > > +\t * but some properties and features, like constructing a\n> > > > +\t * CameraSensorInfo for the IPA module, won't be supported.\n> > > > +\t *\n> > > > +\t * \\todo Make support for selection targets mandatory as soon as all\n> > > > +\t * test platforms have been updated.\n> > > > +\t */\n> > > > +\tint err = 0;\n> > > > +\tRectangle rect;\n> > > > +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(CameraSensor, Warning)\n> > > > +\t\t\t<< \"Failed to retrieve the readable pixel array size\";\n> > > > +\t\terr = -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(CameraSensor, Warning)\n> > > > +\t\t\t<< \"Failed to retrieve the active pixel array size\";\n> > > > +\t\terr = -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(CameraSensor, Warning)\n> > > > +\t\t\t<< \"Failed to retrieve the sensor crop rectangle\";\n> > > > +\t\terr = -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tif (err) {\n> > > > +\t\tLOG(CameraSensor, Warning)\n> > > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > > +\t\tLOG(CameraSensor, Warning)\n> > > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > int CameraSensor::initProperties()\n> > > > {\n> > > > \t/*\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> --\n> Regards,\n> Niklas Söderlund","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 8B130C0F1C\n\tfor ;\n\tMon, 4 Jan 2021 17:56:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E5FA6200B;\n\tMon, 4 Jan 2021 18:56:02 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB26C60110\n\tfor ;\n\tMon, 4 Jan 2021 18:56:00 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id D77E820003;\n\tMon, 4 Jan 2021 17:55:59 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 4 Jan 2021 18:56:14 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210104175614.mf6fn2lalb3l2g7d@uno.localdomain>","References":"<20201230230603.123486-1-jacopo@jmondi.org>\n\t<20201230230603.123486-3-jacopo@jmondi.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14444,"web_url":"https://patchwork.libcamera.org/comment/14444/","msgid":"","date":"2021-01-05T04:07:19","subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jan 04, 2021 at 06:56:14PM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 04, 2021 at 02:19:02PM +0100, Niklas Söderlund wrote:\n> > On 2021-01-04 15:12:23 +0200, Laurent Pinchart wrote:\n> > > On Mon, Jan 04, 2021 at 02:08:28PM +0100, Niklas Söderlund wrote:\n> > > > On 2020-12-31 00:05:59 +0100, Jacopo Mondi wrote:\n> > > > > The CameraSensor class requires the sensor driver to report\n> > > > > information through V4L2 controls and through the V4L2 selection API,\n> > > > > and uses that information to register Camera properties and to\n> > > > > construct CameraSensorInfo class instances to provide them to the IPA.\n> > > > >\n> > > > > Currently, validation of the kernel support happens each time a\n> > > > > feature is requested, with slighly similar debug/error messages\n> > > > > output to the user in case a feature is not supported.\n> > > > >\n> > > > > Rationalize this by validating the sensor driver requirements in a\n> > > > > single function\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Reviewed-by: Paul Elder \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > src/libcamera/camera_sensor.cpp | 84 ++++++++++++++++++++++\n> > > > > 2 files changed, 85 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > index f80d836161a5..aee10aa6e3c7 100644\n> > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > @@ -69,6 +69,7 @@ protected:\n> > > > >\n> > > > > private:\n> > > > > \tint generateId();\n> > > > > +\tint validateSensorDriver();\n> > > > > \tint initProperties();\n> > > > >\n> > > > > \tconst MediaEntity *entity_;\n> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > index e786821d4ba2..048fcd6a1fae 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -207,6 +207,10 @@ int CameraSensor::init()\n> > > > > \t */\n> > > > > \tresolution_ = sizes_.back();\n> > > > >\n> > > > > +\tret = validateSensorDriver();\n> > > > > +\tif (ret)\n> > > > > +\t\treturn ret;\n> > > > > +\n> > > > > \tret = initProperties();\n> > > > > \tif (ret)\n> > > > > \t\treturn ret;\n> > > > > @@ -214,6 +218,86 @@ int CameraSensor::init()\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > +int CameraSensor::validateSensorDriver()\n> > > > > +{\n> > > > > +\t/*\n> > > > > +\t * Make sure the sensor driver supports the mandatory controls\n> > > > > +\t * required by the CameraSensor class.\n> > > > > +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> > > > > +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> > > > > +\t */\n> > > > > +\tconst std::vector mandatoryControls{\n> > > > > +\t\tV4L2_CID_PIXEL_RATE,\n> > > > > +\t\tV4L2_CID_HBLANK,\n> > > > > +\t};\n> > > > > +\n> > > > > +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > > > > +\tif (ctrls.empty()) {\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > > >\n> > > > Here and below I think this should be reworked to a single LOG() call.\n> > > > When we go more multithreading there is a possibility other threads may\n> > > > inject other LOG() calls between the ones here which may be confusing,\n> > > > how about?\n> > > >\n> > > > LOG(CameraSensor, Error)\n> > > > << \"Mandatory V4L2 controls not available\" << std::endl\n> > > > << \"The sensor kernel driver needs to be fixed\" << std::endl\n> > > > << \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > >\n> > > Then we won't have the timestamp and other prefixes added to the lines\n> > > beside the first one :-S\n> >\n> > True, but is that really a problem? Are not all three lines part of the\n> > same \"message\"? In a perfect world I would rather extend our LOG()\n> > implementation to support multiline messages by only printing the\n> > timestamp+prefixes for the first line and then indent the rest with\n> > whitespaces to make it clear that some lines belongs together.\n> >\n> > I do not feel strongly about this so if I'm in minority feel free to\n> > keep my R-b while keeping the multiple LOG() calls.\n> >\n> \n> Printing out without timestamp break the logs symmetry, it renders the\n> output really confusing to look at.\n> \n> Printing longer lines is not convenient, as it makes the log\n> unreadable on smaller screen.\n> \n> I prefer to keep what I have here if possible.\n\nI agree with Niklas that the log infrastructure should be fixed to\nhandle multi-line messages better, and in the meantime, I also prefer\navoiding breaking the log formatting.\n\n> > > > With this fixed,\n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * Optional controls are used to register optional sensor properties. If\n> > > > > +\t * not present, some values will be defaulted.\n> > > > > +\t */\n> > > > > +\tconst std::vector optionalControls{\n> > > > > +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> > > > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > > > +\t};\n> > > > > +\n> > > > > +\tctrls = subdev_->getControls(optionalControls);\n> > > > > +\tif (ctrls.empty())\n> > > > > +\t\tLOG(CameraSensor, Debug) << \"Optional V4L2 controls not supported\";\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * Make sure the required selection targets are supported.\n> > > > > +\t *\n> > > > > +\t * Failures in reading any of the targets are not deemed to be fatal,\n> > > > > +\t * but some properties and features, like constructing a\n> > > > > +\t * CameraSensorInfo for the IPA module, won't be supported.\n> > > > > +\t *\n> > > > > +\t * \\todo Make support for selection targets mandatory as soon as all\n> > > > > +\t * test platforms have been updated.\n> > > > > +\t */\n> > > > > +\tint err = 0;\n> > > > > +\tRectangle rect;\n> > > > > +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(CameraSensor, Warning)\n> > > > > +\t\t\t<< \"Failed to retrieve the readable pixel array size\";\n> > > > > +\t\terr = -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(CameraSensor, Warning)\n> > > > > +\t\t\t<< \"Failed to retrieve the active pixel array size\";\n> > > > > +\t\terr = -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(CameraSensor, Warning)\n> > > > > +\t\t\t<< \"Failed to retrieve the sensor crop rectangle\";\n> > > > > +\t\terr = -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (err) {\n> > > > > +\t\tLOG(CameraSensor, Warning)\n> > > > > +\t\t\t<< \"The sensor kernel driver needs to be fixed\";\n> > > > > +\t\tLOG(CameraSensor, Warning)\n> > > > > +\t\t\t<< \"See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information\";\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > int CameraSensor::initProperties()\n> > > > > {\n> > > > > \t/*","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 C24A7C0F1A\n\tfor ;\n\tTue, 5 Jan 2021 04:07:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 428D862010;\n\tTue, 5 Jan 2021 05:07:34 +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 507CB60317\n\tfor ;\n\tTue, 5 Jan 2021 05:07:32 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 829433D7;\n\tTue, 5 Jan 2021 05:07:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qaXOyBD2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609819651;\n\tbh=5kwjIV1uXH33zAvLC5HkCXxN8bzCW1uk1pt9gQz4B28=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qaXOyBD2oiVgtbPmpzAupeShS95pmTDj/NNpiwVXCb+TRDufo7iUhzHPCypDyrN+t\n\tvEadepcQgzV2CH4Qf0XwBsdEJOWKmwqEkbZYzqXAuih3VGdnw7cl7EfV6ZziojaTfi\n\tOrWnIdhKccxAo4y8eCU6U2WPUAtx7RyEtrke4qQY=","Date":"Tue, 5 Jan 2021 06:07:19 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201230230603.123486-1-jacopo@jmondi.org>\n\t<20201230230603.123486-3-jacopo@jmondi.org>\n\t\n\t\n\t\n\t<20210104175614.mf6fn2lalb3l2g7d@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210104175614.mf6fn2lalb3l2g7d@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor:\n\tValidate driver support","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]