[{"id":11905,"web_url":"https://patchwork.libcamera.org/comment/11905/","msgid":"<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>","date":"2020-08-06T08:05:37","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":64,"url":"https://patchwork.libcamera.org/api/people/64/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"Hi Jacopo,\n\nSome review comments below:\n\nOn 05/08/2020 12:57, Jacopo Mondi wrote:\n> The V4L2 selection API are also used to access the pixel array\n\nare -> is\n\n> properties of an image sensor, such as the size and position of active\n> pixels and the cropped area of the pixel matrix used to produce images.\n> \n> Currently no clear definition of the different areas that compose an\n> image sensor pixel array matrix is provided in the specification, and\n> the actual meaning of each selection target when applied to an image\n> sensor was not provided.\n> \n> Provide in the sub-device documentation the definition of the pixel\n> matrix properties and the selection target associated to each of them.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> 1 file changed, 81 insertions(+)\n> \n> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> index 134d2fb909fa4..c47861dff9b9b 100644\n> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> the image size either up or down. :ref:`v4l2-selection-flags`\n> \n> +.. _v4l2-subdev-pixel-array-properties:\n> +\n> +Selection targets for image sensors properties\n> +----------------------------------------------\n> +\n> +The V4L2 selection API can be used on sub-devices that represent an image\n> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> +:ref:`selection ` ioctls.\n> +\n> +Sub-device drivers for image sensor usually register a single source pad, but in\n> +the case they expose more, the pixel array properties can be accessed from\n> +any of them.\n> +\n> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n\nV4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n\n(same mistake is made elsewhere).\n\n> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> +the immutable properties of the several different areas that compose the sensor\n> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> +units. The logical disposition of pixels is defined by the sensor read-out\n> +starting point and direction, and may differ from the physical disposition of\n> +the pixel units in the pixel array matrix.\n> +\n> +Each pixel matrix portion is contained in a larger rectangle, with the most\n> +largest being the one that describes the pixel matrix physical size. This\n> +defines a hierarchical positional system, where each rectangle is defined\n> +relatively to the largest available one among the ones exposed by the\n> +sub-device driver. Each selection target and the associated pixel array portion\n> +it represents are below presented in order from the largest to the smallest one.\n> +\n> +Pixel array physical size\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The image sensor chip is composed by a number of physical pixels, not all of\n> +them readable by the application processor. Invalid or unreadable lines might\n> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> +they might be tagged with an invalid data type (DT) so that the receiver\n> +automatically discard them. The size of the whole pixel matrix area is\n> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> +defined as position (0, 0). All the other selection targets are defined\n> +relatively to this, larger, rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n\nIt is a good idea to mention that if there are no invalid or unreadable pixels/lines,\nthen V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n\n> +\n> +Pixel array readable area\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> +area of the pixel array matrix, including pixels with valid image data and pixel\n> +used for calibration purposes, such as optical black pixels. It is not unlikely\n> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> +and columns of pixels. Those does not concur in the definition of the\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n\nMention that BOUNDS is enclosed by NATIVE_SIZE.\n\n> +\n> +Pixel array active area\n> +^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The portion of the pixel array which contains valid image data is defined as the\n> +active area of the pixel matrix. The active pixel array is is accessed by mean\n\nmean -> means\n\n> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> +resolution the sensor can produce and defines the dimension of the full\n> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n\nBOUNDS -> DEFAULT\n\n> +immutable property of the image sensor, it does not change at run-time and\n> +cannot be modified from userspace.\n\nMention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n\n> +\n> +Analog crop rectangle\n\nWhy analog? It's just the crop rectangle, nothing analog about it.\n\n> +^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The sensor driver might decide, in order to adjust the image resolution to best\n> +match the one requested by applications, to only process a part of the active\n> +pixel array matrix. The selected area is read-out and processed by the image\n> +sensor on-board ISP in order to produce images of the desired size and\n> +resolution while possible maintaing the largest possible field-of-view. The\n\nmaintaing -> maintaining\n\nActually, I'd drop 'while possible maintaing the largest possible field-of-view'\nentirely. It doesn't make much sense.\n\n> +cropped portion of the pixel array which is used to produce images is returned\n> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n\nrepresent -> represents\n\n> +change at runtime as it depends on the currently configured sensor mode and\n> +desired image resolution. If the sub-device driver supports that, userspace\n> +can set the analog crop rectangle to select which portion of the pixel array\n\ns/analog//\n\n> +to read out.\n\nMention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n\nMake a note that CROP can also be used to obtain optical black pixels.\n\n> +\n> \n> Types of selection targets\n> --------------------------\n> \n\nRegards,\n\n\tHans","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 1C69BBD87A\n\tfor ;\n\tThu, 6 Aug 2020 08:05:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CD826071B;\n\tThu, 6 Aug 2020 10:05:40 +0200 (CEST)","from lb1-smtp-cloud9.xs4all.net (lb1-smtp-cloud9.xs4all.net\n\t[194.109.24.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0AE3760390\n\tfor ;\n\tThu, 6 Aug 2020 10:05:39 +0200 (CEST)","from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d])\n\tby smtp-cloud9.xs4all.net with ESMTPA\n\tid 3au9k7DTxuuXO3auAknLXe; Thu, 06 Aug 2020 10:05:38 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"W2ta8g1i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s1;\n\tt=1596701138; bh=/CYthU5Pf8xk60aWrMyQKDk1aLojvqVPQaoetFG+X1s=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=W2ta8g1ix7WVVAbU30fESit1wNQuQpjPuK52kp9/QXE+oVEXQgds9lwXwMlFABZbp\n\tejlVgqnPXIMMButQRERja+vmw30Z724qhAOMryoQl1GZuR8Yu7rlHVNLrAgTzF/YJh\n\tuKK8/HsVSP6ZVbJe6Rrd8fDzl69M1WPYT3SKGHkgEjo99XmjIyXMf26e4tL9ZHzBxH\n\tDMUVK9gMF7rhvBik8Shwzkyn2CbbsuA7ZY4yhCA8JV0/WzCW2uPfP4vwJICOxAEAiq\n\tX+eiIgYgP24itPtn/Ls8+slV/WCCetisPviPeQ2bgA1VIkUOEoN6UqlcurdZ3S+XGI\n\t9ZRRnUuqvWEOw==","To":"Jacopo Mondi ,\n\tSakari Ailus ,\n\tLaurent Pinchart ","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>","From":"Hans Verkuil ","Message-ID":"<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>","Date":"Thu, 6 Aug 2020 10:05:37 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200805105721.15445-2-jacopo@jmondi.org>","Content-Language":"en-US","X-CMAE-Envelope":"MS4wfBzjn0BuX/wCVZ+SWEC3aKN/n6kZ2WJhRhl9vxwJaXF+ndDVA0Qvr8WBu30fJVWKZbfLPIypzIq0aYNbI42jqCmqheRssqZz9YngkHJu/Ijbxz5QnxQU\n\tCTptfi6PZ7yXp1FuI02rZKh8QaFv88OTl4Wg1dAPsMU1fXNpbU7UHbAh4scfvYS8W9UEq6AEtf+yS3LOaq9yaIf26Ny62akZuAuZSZcVlf5YE59RNi7TILnF\n\tYP96II8qpeY05NxBhJzhyvmgkQP+UYk11sSyWaCdPS3AotqY8amNceuvsVR3IdvhDq8rs0jNfTOV0itp2mjCPtx/e7/8NorOTlfV3AGQ8zRgCGOz18WLmtAj\n\t0TcXgGsjsq/1rx4l2pJTCOqAjyCWgU1yPUFvWZ2Sc1lLKjNkWNy3UYoUhcc92zaKmanilkxOas8JJNm1bbYfJ2vL9jkA4BmtIWY6sgf+XYZeET909WONKPjt\n\tVhjZ/VR+5x9z4ocBUhBKZLhj7MYhy3L2PzdC1hlS3LufGmbafmjvVNlFTD8=","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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":"Ricardo Ribalda Delgado ,\n\tlibcamera-devel@lists.libcamera.org,\n\tSowjanya Komatineni ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11909,"web_url":"https://patchwork.libcamera.org/comment/11909/","msgid":"<20200806095038.tc6mmwknqdinaeth@uno.localdomain>","date":"2020-08-06T09:50:38","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hans,\n\nOn Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> Hi Jacopo,\n>\n> Some review comments below:\n>\n> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> > The V4L2 selection API are also used to access the pixel array\n>\n> are -> is\n>\n> > properties of an image sensor, such as the size and position of active\n> > pixels and the cropped area of the pixel matrix used to produce images.\n> >\n> > Currently no clear definition of the different areas that compose an\n> > image sensor pixel array matrix is provided in the specification, and\n> > the actual meaning of each selection target when applied to an image\n> > sensor was not provided.\n> >\n> > Provide in the sub-device documentation the definition of the pixel\n> > matrix properties and the selection target associated to each of them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > 1 file changed, 81 insertions(+)\n> >\n> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > index 134d2fb909fa4..c47861dff9b9b 100644\n> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > the image size either up or down. :ref:`v4l2-selection-flags`\n> >\n> > +.. _v4l2-subdev-pixel-array-properties:\n> > +\n> > +Selection targets for image sensors properties\n> > +----------------------------------------------\n> > +\n> > +The V4L2 selection API can be used on sub-devices that represent an image\n> > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > +:ref:`selection ` ioctls.\n> > +\n> > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > +the case they expose more, the pixel array properties can be accessed from\n> > +any of them.\n> > +\n> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n>\n> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n>\n> (same mistake is made elsewhere).\n\nAh ups, I used TGT_NATIVE consistently, seems like I thought that was\nthe right name\n\n>\n> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > +the immutable properties of the several different areas that compose the sensor\n> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > +units. The logical disposition of pixels is defined by the sensor read-out\n> > +starting point and direction, and may differ from the physical disposition of\n> > +the pixel units in the pixel array matrix.\n> > +\n> > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > +largest being the one that describes the pixel matrix physical size. This\n> > +defines a hierarchical positional system, where each rectangle is defined\n> > +relatively to the largest available one among the ones exposed by the\n> > +sub-device driver. Each selection target and the associated pixel array portion\n> > +it represents are below presented in order from the largest to the smallest one.\n> > +\n> > +Pixel array physical size\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The image sensor chip is composed by a number of physical pixels, not all of\n> > +them readable by the application processor. Invalid or unreadable lines might\n> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > +they might be tagged with an invalid data type (DT) so that the receiver\n> > +automatically discard them. The size of the whole pixel matrix area is\n> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > +defined as position (0, 0). All the other selection targets are defined\n> > +relatively to this, larger, rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n>\n> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,\n> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n\nYes it is! I'll add it here\n\n>\n> > +\n> > +Pixel array readable area\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> > +area of the pixel array matrix, including pixels with valid image data and pixel\n> > +used for calibration purposes, such as optical black pixels. It is not unlikely\n> > +that valid pixels and optical black pixels are surrounded by non-readable rows\n> > +and columns of pixels. Those does not concur in the definition of the\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n>\n> Mention that BOUNDS is enclosed by NATIVE_SIZE.\n>\n\nI tried to express that in the intro section with\n\n\"Each pixel matrix portion is contained in a larger rectangle, with the most\nlargest being the one that describes the pixel matrix physical size.\"\n\nBut I guess it's worth to express that for each target!\n\n> > +\n> > +Pixel array active area\n> > +^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The portion of the pixel array which contains valid image data is defined as the\n> > +active area of the pixel matrix. The active pixel array is is accessed by mean\n>\n> mean -> means\n>\n> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> > +resolution the sensor can produce and defines the dimension of the full\n> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n>\n> BOUNDS -> DEFAULT\n>\n\nups\n\n> > +immutable property of the image sensor, it does not change at run-time and\n> > +cannot be modified from userspace.\n>\n> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n>\n> > +\n> > +Analog crop rectangle\n>\n> Why analog? It's just the crop rectangle, nothing analog about it.\n>\n\nWe used the 'analogCrop' term in libcamera to differentiate the\ncropping which happens on the sensor pixel array matrix to select the\nregion to process and produce image from. Sensor with an on-board\nscaler can perform other cropping steps to implement, in example digital\nzoom, so we expect to have a 'digital crop' phase as well. RAW\nsensors, in example, will only have an analogCrop rectangle.\n\nQuoting the libcamera definition of analog crop:\n\n * horizontal and vertical sizes define the portion of the pixel array which\n * is read-out and provided to the sensor's internal processing pipeline, before\n * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n * take place.\n\nshould I keep it or remove it ?\n\n> > +^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The sensor driver might decide, in order to adjust the image resolution to best\n> > +match the one requested by applications, to only process a part of the active\n> > +pixel array matrix. The selected area is read-out and processed by the image\n> > +sensor on-board ISP in order to produce images of the desired size and\n> > +resolution while possible maintaing the largest possible field-of-view. The\n>\n> maintaing -> maintaining\n>\n> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'\n> entirely. It doesn't make much sense.\n\nAck\n\n>\n> > +cropped portion of the pixel array which is used to produce images is returned\n> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n>\n> represent -> represents\n>\n> > +change at runtime as it depends on the currently configured sensor mode and\n> > +desired image resolution. If the sub-device driver supports that, userspace\n> > +can set the analog crop rectangle to select which portion of the pixel array\n>\n> s/analog//\n>\n> > +to read out.\n>\n> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n>\n> Make a note that CROP can also be used to obtain optical black pixels.\n>\n\nWhat about:\n\n+desired image resolution. If the sub-device driver supports that, userspace\n+can set the analog crop rectangle to select which portion of the pixel array\n+to read out including, if supported, optical black pixels.\n\n?\n\nThanks\n j\n\n> > +\n> >\n> > Types of selection targets\n> > --------------------------\n> >\n>\n> Regards,\n>\n> \tHans","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 79BA7BD87A\n\tfor ;\n\tThu, 6 Aug 2020 09:47:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 103346071B;\n\tThu, 6 Aug 2020 11:47:05 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D2A460390\n\tfor ;\n\tThu, 6 Aug 2020 11:47:03 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id D5BFE20000D;\n\tThu, 6 Aug 2020 09:46:58 +0000 (UTC)"],"Date":"Thu, 6 Aug 2020 11:50:38 +0200","From":"Jacopo Mondi ","To":"Hans Verkuil ","Message-ID":"<20200806095038.tc6mmwknqdinaeth@uno.localdomain>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11910,"web_url":"https://patchwork.libcamera.org/comment/11910/","msgid":"","date":"2020-08-06T09:58:31","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":64,"url":"https://patchwork.libcamera.org/api/people/64/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 06/08/2020 11:50, Jacopo Mondi wrote:\n> Hi Hans,\n> \n> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n>> Hi Jacopo,\n>>\n>> Some review comments below:\n>>\n>> On 05/08/2020 12:57, Jacopo Mondi wrote:\n>>> The V4L2 selection API are also used to access the pixel array\n>>\n>> are -> is\n>>\n>>> properties of an image sensor, such as the size and position of active\n>>> pixels and the cropped area of the pixel matrix used to produce images.\n>>>\n>>> Currently no clear definition of the different areas that compose an\n>>> image sensor pixel array matrix is provided in the specification, and\n>>> the actual meaning of each selection target when applied to an image\n>>> sensor was not provided.\n>>>\n>>> Provide in the sub-device documentation the definition of the pixel\n>>> matrix properties and the selection target associated to each of them.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n>>> 1 file changed, 81 insertions(+)\n>>>\n>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n>>> index 134d2fb909fa4..c47861dff9b9b 100644\n>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n>>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n>>> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n>>> the image size either up or down. :ref:`v4l2-selection-flags`\n>>>\n>>> +.. _v4l2-subdev-pixel-array-properties:\n>>> +\n>>> +Selection targets for image sensors properties\n>>> +----------------------------------------------\n>>> +\n>>> +The V4L2 selection API can be used on sub-devices that represent an image\n>>> +sensor to retrieve the sensor's pixel array matrix properties by using the\n>>> +:ref:`selection ` ioctls.\n>>> +\n>>> +Sub-device drivers for image sensor usually register a single source pad, but in\n>>> +the case they expose more, the pixel array properties can be accessed from\n>>> +any of them.\n>>> +\n>>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n>>\n>> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n>>\n>> (same mistake is made elsewhere).\n> \n> Ah ups, I used TGT_NATIVE consistently, seems like I thought that was\n> the right name\n> \n>>\n>>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n>>> +the immutable properties of the several different areas that compose the sensor\n>>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n>>> +units. The logical disposition of pixels is defined by the sensor read-out\n>>> +starting point and direction, and may differ from the physical disposition of\n>>> +the pixel units in the pixel array matrix.\n>>> +\n>>> +Each pixel matrix portion is contained in a larger rectangle, with the most\n>>> +largest being the one that describes the pixel matrix physical size. This\n>>> +defines a hierarchical positional system, where each rectangle is defined\n>>> +relatively to the largest available one among the ones exposed by the\n>>> +sub-device driver. Each selection target and the associated pixel array portion\n>>> +it represents are below presented in order from the largest to the smallest one.\n>>> +\n>>> +Pixel array physical size\n>>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n>>> +\n>>> +The image sensor chip is composed by a number of physical pixels, not all of\n>>> +them readable by the application processor. Invalid or unreadable lines might\n>>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n>>> +they might be tagged with an invalid data type (DT) so that the receiver\n>>> +automatically discard them. The size of the whole pixel matrix area is\n>>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n>>> +defined as position (0, 0). All the other selection targets are defined\n>>> +relatively to this, larger, rectangle. The rectangle returned by\n>>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n>>> +does not change at run-time and cannot be modified from userspace.\n>>\n>> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,\n>> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n> \n> Yes it is! I'll add it here\n> \n>>\n>>> +\n>>> +Pixel array readable area\n>>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n>>> +\n>>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n>>> +area of the pixel array matrix, including pixels with valid image data and pixel\n>>> +used for calibration purposes, such as optical black pixels. It is not unlikely\n>>> +that valid pixels and optical black pixels are surrounded by non-readable rows\n>>> +and columns of pixels. Those does not concur in the definition of the\n>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n>>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n>>> +does not change at run-time and cannot be modified from userspace.\n>>\n>> Mention that BOUNDS is enclosed by NATIVE_SIZE.\n>>\n> \n> I tried to express that in the intro section with\n> \n> \"Each pixel matrix portion is contained in a larger rectangle, with the most\n> largest being the one that describes the pixel matrix physical size.\"\n> \n> But I guess it's worth to express that for each target!\n> \n>>> +\n>>> +Pixel array active area\n>>> +^^^^^^^^^^^^^^^^^^^^^^^\n>>> +\n>>> +The portion of the pixel array which contains valid image data is defined as the\n>>> +active area of the pixel matrix. The active pixel array is is accessed by mean\n>>\n>> mean -> means\n>>\n>>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n>>> +resolution the sensor can produce and defines the dimension of the full\n>>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n>>\n>> BOUNDS -> DEFAULT\n>>\n> \n> ups\n> \n>>> +immutable property of the image sensor, it does not change at run-time and\n>>> +cannot be modified from userspace.\n>>\n>> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n>>\n>>> +\n>>> +Analog crop rectangle\n>>\n>> Why analog? It's just the crop rectangle, nothing analog about it.\n>>\n> \n> We used the 'analogCrop' term in libcamera to differentiate the\n> cropping which happens on the sensor pixel array matrix to select the\n> region to process and produce image from. Sensor with an on-board\n> scaler can perform other cropping steps to implement, in example digital\n> zoom, so we expect to have a 'digital crop' phase as well. RAW\n> sensors, in example, will only have an analogCrop rectangle.\n> \n> Quoting the libcamera definition of analog crop:\n> \n> * horizontal and vertical sizes define the portion of the pixel array which\n> * is read-out and provided to the sensor's internal processing pipeline, before\n> * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> * take place.\n> \n> should I keep it or remove it ?\n\nIt's a very confusing term. Especially since this API can also be used with analog\nvideo capture devices (Composite/S-Video) where the video signal actually is analog.\n\nIn the V4L2 API there is no such thing as 'analog crop', so please remove it.\n\n> \n>>> +^^^^^^^^^^^^^^^^^^^^^\n>>> +\n>>> +The sensor driver might decide, in order to adjust the image resolution to best\n>>> +match the one requested by applications, to only process a part of the active\n>>> +pixel array matrix. The selected area is read-out and processed by the image\n>>> +sensor on-board ISP in order to produce images of the desired size and\n>>> +resolution while possible maintaing the largest possible field-of-view. The\n>>\n>> maintaing -> maintaining\n>>\n>> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'\n>> entirely. It doesn't make much sense.\n> \n> Ack\n> \n>>\n>>> +cropped portion of the pixel array which is used to produce images is returned\n>>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n>>\n>> represent -> represents\n>>\n>>> +change at runtime as it depends on the currently configured sensor mode and\n>>> +desired image resolution. If the sub-device driver supports that, userspace\n>>> +can set the analog crop rectangle to select which portion of the pixel array\n>>\n>> s/analog//\n>>\n>>> +to read out.\n>>\n>> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n>>\n>> Make a note that CROP can also be used to obtain optical black pixels.\n>>\n> \n> What about:\n> \n> +desired image resolution. If the sub-device driver supports that, userspace\n> +can set the analog crop rectangle to select which portion of the pixel array\n> +to read out including, if supported, optical black pixels.\n\nHmm, that's a bit awkward. How about:\n\n+desired image resolution. If supported by the sub-device driver, userspace\n+can set the crop rectangle to select which portion of the pixel array\n+to read out. This may include optical black pixels if those are part of\n+V4L2_SEL_TGT_CROP_BOUNDS.\n\nRegards,\n\n\tHans\n\n> \n> ?\n> \n> Thanks\n> j\n> \n>>> +\n>>>\n>>> Types of selection targets\n>>> --------------------------\n>>>\n>>\n>> Regards,\n>>\n>> \tHans","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 A0C50BD86F\n\tfor ;\n\tThu, 6 Aug 2020 09:58:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D43A60730;\n\tThu, 6 Aug 2020 11:58:35 +0200 (CEST)","from lb3-smtp-cloud7.xs4all.net (lb3-smtp-cloud7.xs4all.net\n\t[194.109.24.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EB5F60390\n\tfor ;\n\tThu, 6 Aug 2020 11:58:34 +0200 (CEST)","from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid 3cfPkgYiMywL53cfQk41BL; Thu, 06 Aug 2020 11:58:33 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"GMKF/dyq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s1;\n\tt=1596707913; bh=5JXbKgf63QVLKGGLkUFPCqfAkSQdJKzIvSOnHkq4rVM=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=GMKF/dyqKNpyC7GNHZAIXlI2nPI2WBuH0/KScNw0qUOm/FJues3eMXLlGJQL68vQ7\n\tRPaOfzzIv338agcqLNIOLvwHpLGKdsDLo3mj/O7KouJlqpxMU7aiw4LYRpVooCqj83\n\t0DDTrjjOuYQTR2+2cKFNImnNxNzBTUZti0TBG/SE72nfS8twuyhnRRtv7G07B3lggg\n\t71FJ5IP+tUJHuE9gxsKDghFVlVUXOLsd0czi8yrUKVtOi7BKFs4AHKMFKDloZnPBGg\n\tYjPRqaIe81yVcvdE4DfYMcv3SjMp5Ki6NyDTG58Ure+o/UMuUkwt4VezYQ2BgoaxFX\n\tfNY/rA7L0fQcg==","To":"Jacopo Mondi ","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>","From":"Hans Verkuil ","Message-ID":"","Date":"Thu, 6 Aug 2020 11:58:31 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200806095038.tc6mmwknqdinaeth@uno.localdomain>","Content-Language":"en-US","X-CMAE-Envelope":"MS4wfOlaVSnd2HzorfG+f3KpcOhibeiNtEKTMg46QVABODSvwqAfMLtR2/zrrFllfZiDqCRCev0xlV2WpeGAdD1EOQA7E4Q5+kE7kG6yBPoUsj/w6KFbVKM7\n\theKgceLEHjeHA717zvv3T0OeGQnXnwlaGwFaajMeCaeh1K4rh/otCqiNgD7IPXaEFxsF+w1bIeUZtY1o8IJYkObc1tFC09+Cj8xkkqSwgz0Hhp2rKP8yMS12\n\tt+PaQd18hQ9mIgPMdHZlK6yjs0XBLLZSB6Kzmvpt4xCfp1OwBVE9t518Db4Y8x/xAYyPGZHJ941xmVei8fB0xLAmVUhU1Hm+jURPowne9ZzWQXZQ7SBhjrZf\n\tq9RKALUqsAhvKIosPs5jMRtTm/TQoIUFRbxhYJg3RL2KKO1Pq+BgfwhDydA1vBwkZym1y6I7pZGreegQkgGjzdmKgt+rUTg0i94pvHFW03sCBpOpH9IXRD1f\n\tC6N7fRORccceTimvc4nJhKCRqI0xiOd+gV/ovA9P0DoAkmIKiqco5lwNHhY=","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11916,"web_url":"https://patchwork.libcamera.org/comment/11916/","msgid":"<20200805222447.GP13316@paasikivi.fi.intel.com>","date":"2020-08-05T22:24:47","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Jacopo,\n\nThanks for the patchset.\n\nThis improves selection documentation quite a bit. Please see my comments\nbelow.\n\nOn Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> The V4L2 selection API are also used to access the pixel array\n> properties of an image sensor, such as the size and position of active\n> pixels and the cropped area of the pixel matrix used to produce images.\n> \n> Currently no clear definition of the different areas that compose an\n> image sensor pixel array matrix is provided in the specification, and\n> the actual meaning of each selection target when applied to an image\n> sensor was not provided.\n> \n> Provide in the sub-device documentation the definition of the pixel\n> matrix properties and the selection target associated to each of them.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> 1 file changed, 81 insertions(+)\n> \n> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> index 134d2fb909fa4..c47861dff9b9b 100644\n> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> the image size either up or down. :ref:`v4l2-selection-flags`\n> \n> +.. _v4l2-subdev-pixel-array-properties:\n> +\n> +Selection targets for image sensors properties\n> +----------------------------------------------\n> +\n> +The V4L2 selection API can be used on sub-devices that represent an image\n> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> +:ref:`selection ` ioctls.\n> +\n> +Sub-device drivers for image sensor usually register a single source pad, but in\n> +the case they expose more, the pixel array properties can be accessed from\n> +any of them.\n\nIs this a hypothetical case or are there examples?\n\nAlso note that camera sensor drivers may expose more than one sub-devices,\nonly one of which represents the pixel array.\n\n> +\n> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> +the immutable properties of the several different areas that compose the sensor\n> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> +units. The logical disposition of pixels is defined by the sensor read-out\n> +starting point and direction, and may differ from the physical disposition of\n> +the pixel units in the pixel array matrix.\n> +\n> +Each pixel matrix portion is contained in a larger rectangle, with the most\n\ns/larger\\K/ or equal/\n\ns/most//\n\n> +largest being the one that describes the pixel matrix physical size. This\n> +defines a hierarchical positional system, where each rectangle is defined\n\ns/,//\n\n> +relatively to the largest available one among the ones exposed by the\n> +sub-device driver. Each selection target and the associated pixel array portion\n> +it represents are below presented in order from the largest to the smallest one.\n> +\n> +Pixel array physical size\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The image sensor chip is composed by a number of physical pixels, not all of\n> +them readable by the application processor. Invalid or unreadable lines might\n> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> +they might be tagged with an invalid data type (DT) so that the receiver\n> +automatically discard them. The size of the whole pixel matrix area is\n> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> +defined as position (0, 0). All the other selection targets are defined\n> +relatively to this, larger, rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n> +\n> +Pixel array readable area\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> +area of the pixel array matrix, including pixels with valid image data and pixel\n> +used for calibration purposes, such as optical black pixels. It is not unlikely\n> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> +and columns of pixels. Those does not concur in the definition of the\n\nHow about: \"Only pixels that can be read out are included in the\nV4L2_SEL_TGT_CROP_BOUNDS rectangle.\"?\n\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n> +\n> +Pixel array active area\n> +^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The portion of the pixel array which contains valid image data is defined as the\n> +active area of the pixel matrix. The active pixel array is is accessed by mean\n\ns/accessed/described/\n\nAnother word than \"active\" here would be great as we already have active\nand try contexts for selections.\n\n> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n\ns/the larger//\n\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> +resolution the sensor can produce and defines the dimension of the full\n\ns/resolution/size/\n\n> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n\ns/-/ /g\n\n> +immutable property of the image sensor, it does not change at run-time and\n> +cannot be modified from userspace.\n> +\n> +Analog crop rectangle\n> +^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The sensor driver might decide, in order to adjust the image resolution to best\n> +match the one requested by applications, to only process a part of the active\n\ns/, to\\K/ instruct the hardware to/\n\n> +pixel array matrix. The selected area is read-out and processed by the image\n> +sensor on-board ISP in order to produce images of the desired size and\n> +resolution while possible maintaing the largest possible field-of-view. The\n\ns/size\\K[^.]?*\\./m\n\n> +cropped portion of the pixel array which is used to produce images is returned\n\ns/produce/read out/\ns/returned/configured/\n\n> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n\ns/by/using/\n\nI'd leave out the rest of the sentence after \"target\" above.\n\n> +change at runtime as it depends on the currently configured sensor mode and\n> +desired image resolution. If the sub-device driver supports that, userspace\n> +can set the analog crop rectangle to select which portion of the pixel array\n> +to read out.\n\nHow about instead:\n\nRegister list based drivers generally do not allow setting analogue crop\nrectangles.\n\n> +\n> \n> Types of selection targets\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 88245BD87A\n\tfor ;\n\tThu, 6 Aug 2020 11:18:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2745A60737;\n\tThu, 6 Aug 2020 13:18:51 +0200 (CEST)","from mga09.intel.com (mga09.intel.com [134.134.136.24])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2959D6055A\n\tfor ;\n\tThu, 6 Aug 2020 13:18:49 +0200 (CEST)","from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t06 Aug 2020 04:18:48 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby fmsmga003-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Aug 2020 04:18:46 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid A6C592063B; Thu, 6 Aug 2020 01:24:47 +0300 (EEST)"],"IronPort-SDR":["wbF7OFbWeNkxzpFTyipArU+hzApfi84bfXXnj2BRjUeVvZhWQW+e99YGIr7kpRBVFGTvIrlnur\n\tysZ7tP7t4ycA==","ieMMfsqZx0pTh7/zf3J69mjIyq0ji7rBM+oYHid+tMVHj0mpxXmAW5TwkMAyuRXtxBXWbJT+v0\n\tnzPzlo13hf6A=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9704\"; a=\"153912762\"","E=Sophos;i=\"5.75,441,1589266800\"; d=\"scan'208\";a=\"153912762\"","E=Sophos;i=\"5.75,441,1589266800\"; d=\"scan'208\";a=\"331196517\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Thu, 6 Aug 2020 01:24:47 +0300","From":"Sakari Ailus ","To":"Jacopo Mondi ","Message-ID":"<20200805222447.GP13316@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200805105721.15445-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11921,"web_url":"https://patchwork.libcamera.org/comment/11921/","msgid":"<20200806125445.GA16270@paasikivi.fi.intel.com>","date":"2020-08-06T12:54:45","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Hans,\n\nOn Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n> On 06/08/2020 11:50, Jacopo Mondi wrote:\n> > Hi Hans,\n> > \n> > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> >> Hi Jacopo,\n> >>\n> >> Some review comments below:\n> >>\n> >> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> >>> +Analog crop rectangle\n> >>\n> >> Why analog? It's just the crop rectangle, nothing analog about it.\n> >>\n> > \n> > We used the 'analogCrop' term in libcamera to differentiate the\n> > cropping which happens on the sensor pixel array matrix to select the\n> > region to process and produce image from. Sensor with an on-board\n> > scaler can perform other cropping steps to implement, in example digital\n> > zoom, so we expect to have a 'digital crop' phase as well. RAW\n> > sensors, in example, will only have an analogCrop rectangle.\n> > \n> > Quoting the libcamera definition of analog crop:\n> > \n> > * horizontal and vertical sizes define the portion of the pixel array which\n> > * is read-out and provided to the sensor's internal processing pipeline, before\n> > * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> > * take place.\n> > \n> > should I keep it or remove it ?\n> \n> It's a very confusing term. Especially since this API can also be used with analog\n> video capture devices (Composite/S-Video) where the video signal actually is analog.\n> \n> In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n\nThere isn't in the V4L2 API but I don't see why we couldn't document it\nhere. Analogue crop is an established term related to raw (perhaps others,\ntoo?) camera sensors which describes cropping that is implemented by not\nreading parts of the pixel array.\n\nAs this documentation only applies to camera sensors, I think it's entirely\nappropriate to document this here, and using that term.","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 09488BD87A\n\tfor ;\n\tThu, 6 Aug 2020 12:55:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D30A4607BB;\n\tThu, 6 Aug 2020 14:55:22 +0200 (CEST)","from mga17.intel.com (mga17.intel.com [192.55.52.151])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDDD460554\n\tfor ;\n\tThu, 6 Aug 2020 14:55:20 +0200 (CEST)","from orsmga008.jf.intel.com ([10.7.209.65])\n\tby fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t06 Aug 2020 05:55:19 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby orsmga008-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Aug 2020 05:55:17 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid 31AF22063B; Thu, 6 Aug 2020 15:54:45 +0300 (EEST)"],"IronPort-SDR":["rWKzSqjMdumDn6vR3Jbn5qPIw7mfn90G3jwTpAG/lBSKFTJQbx5MXx8NKosaXXMN+g0bcb3jKC\n\twueMYflrsqug==","YX3VVoSLAH25zeI826HufpuAkgBKlYI1Rzw1c6QT37yHvVX1mYKl2U2FENI2LpBDswB117SBzj\n\tg0sc8ZCWTvag=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9704\"; a=\"132873584\"","E=Sophos;i=\"5.75,441,1589266800\"; d=\"scan'208\";a=\"132873584\"","E=Sophos;i=\"5.75,441,1589266800\"; d=\"scan'208\";a=\"323421183\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Thu, 6 Aug 2020 15:54:45 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Message-ID":"<20200806125445.GA16270@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11925,"web_url":"https://patchwork.libcamera.org/comment/11925/","msgid":"<1ee9f378-8df1-7c03-2136-ce7870934443@xs4all.nl>","date":"2020-08-06T13:22:34","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":64,"url":"https://patchwork.libcamera.org/api/people/64/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 06/08/2020 14:54, Sakari Ailus wrote:\n> Hi Hans,\n> \n> On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n>> On 06/08/2020 11:50, Jacopo Mondi wrote:\n>>> Hi Hans,\n>>>\n>>> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n>>>> Hi Jacopo,\n>>>>\n>>>> Some review comments below:\n>>>>\n>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:\n>>>>> +Analog crop rectangle\n>>>>\n>>>> Why analog? It's just the crop rectangle, nothing analog about it.\n>>>>\n>>>\n>>> We used the 'analogCrop' term in libcamera to differentiate the\n>>> cropping which happens on the sensor pixel array matrix to select the\n>>> region to process and produce image from. Sensor with an on-board\n>>> scaler can perform other cropping steps to implement, in example digital\n>>> zoom, so we expect to have a 'digital crop' phase as well. RAW\n>>> sensors, in example, will only have an analogCrop rectangle.\n>>>\n>>> Quoting the libcamera definition of analog crop:\n>>>\n>>> * horizontal and vertical sizes define the portion of the pixel array which\n>>> * is read-out and provided to the sensor's internal processing pipeline, before\n>>> * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n>>> * take place.\n>>>\n>>> should I keep it or remove it ?\n>>\n>> It's a very confusing term. Especially since this API can also be used with analog\n>> video capture devices (Composite/S-Video) where the video signal actually is analog.\n>>\n>> In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n> \n> There isn't in the V4L2 API but I don't see why we couldn't document it\n> here. Analogue crop is an established term related to raw (perhaps others,\n> too?) camera sensors which describes cropping that is implemented by not\n> reading parts of the pixel array.\n> \n> As this documentation only applies to camera sensors, I think it's entirely\n> appropriate to document this here, and using that term.\n> \n\nIt's always been called just 'crop' in the V4L2 API, so renaming it suddenly\nto something else is IMHO confusing. What you can do, however, is that in the\ndescription of the \"crop rectangle\" you mention that \"it is also known as\n\"analog crop\" in the context of camera sensors.\n\nWith perhaps some more extensive explanation of the term.\n\nRegards,\n\n\tHans","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 8B008BD87A\n\tfor ;\n\tThu, 6 Aug 2020 13:22:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E07760836;\n\tThu, 6 Aug 2020 15:22:37 +0200 (CEST)","from lb1-smtp-cloud7.xs4all.net (lb1-smtp-cloud7.xs4all.net\n\t[194.109.24.24])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3EF4A6038F\n\tfor ;\n\tThu, 6 Aug 2020 15:22:36 +0200 (CEST)","from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid 3fqskhfymywL53fqtk4ZKC; Thu, 06 Aug 2020 15:22:35 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"BZ9RgeKW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s1;\n\tt=1596720156; bh=RUNHmjWKtE61EzaljHMlZ14pOR3jiEYUObArwQ/lc5k=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=BZ9RgeKWTMhePMWLmav9dt6PJCdqwPJMPWlUU16kbG1ry0kH5WNpCwNydKllbnRBC\n\tBqCbihIRJM/z5YNJ9JpCM7aAlTBW13wqhgqRNRUL5189IFCioYR3Pmfj+1BCkby+66\n\tH2wbjUjwvdsdey++cqEw8b6c4ijOWikhDuta0Jr72fn3VZNC7HO9v/YocnkXzxiV9Z\n\tM1Pi7LB4GtGkZHZtz8puCqNizEa8XknLRH4zUq4yFtS1Ya5d7KHBLT3HSJulhOrT0g\n\t8OcrK9yGKSnZ0JpHzXLF1obtwBfEJd4cGB7jqR5735ThRlyfT47i0B1Uc+iiwIkK02\n\t0YuCJ1N7ijggg==","To":"Sakari Ailus ","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t\n\t<20200806125445.GA16270@paasikivi.fi.intel.com>","From":"Hans Verkuil ","Message-ID":"<1ee9f378-8df1-7c03-2136-ce7870934443@xs4all.nl>","Date":"Thu, 6 Aug 2020 15:22:34 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200806125445.GA16270@paasikivi.fi.intel.com>","Content-Language":"en-US","X-CMAE-Envelope":"MS4wfBdgPbFoVgDv1u1ASPkUZgMGyJ3087RIhCO6Dqw7qrrTM8JL/KY1l310LgZT0CvwMfMz2fmw/UMt6t6dVs95bFNmEUT9dXKJjx+RyTmKzVaC7g8dV3A1\n\tPKP9xX03hwpzbbNsu6oX8N2pDx7vndtmYlX4S3g2QSHJ3ri2HjEsG9rDLptCSzoM+rny5u8bzKV4DF0LawKKu51QL1tPsluP4/7TzpmR51oY0zO7YvXXMMM+\n\tdDxkLno2uep4hq6HXC7CT3FAJl4PIovFbF/g5bOUaJiRg7/QgOJsbzQJuiyeTk9/zoFkiZdELVY17m1DSNstRBw+Z2F4geOjMGT6xn6HMKGttU0m/BhdfsNS\n\tse7FV9IBKUk0JCxX1a3HRoHKA/N4P19yfVtA0q+nmqG0QymITHw9M2UcYi74mSIIcqhiVLDwLUd2aMpW8pKrmvse+hzm7VT0zoOVg4kzdx/kQj/U87iRDTG+\n\tB7kJX4I/3sD919kYC4I+uY9vY2bICncp/QUweqCNe/5QIsn0J4Q4OR7/Znk=","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11955,"web_url":"https://patchwork.libcamera.org/comment/11955/","msgid":"<20200809171757.GC5981@pendragon.ideasonboard.com>","date":"2020-08-09T17:17:57","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo, Hans,\n\nOn Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n> On 06/08/2020 11:50, Jacopo Mondi wrote:\n> > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> >> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> >>> The V4L2 selection API are also used to access the pixel array\n> >>\n> >> are -> is\n> >>\n> >>> properties of an image sensor, such as the size and position of active\n> >>> pixels and the cropped area of the pixel matrix used to produce images.\n> >>>\n> >>> Currently no clear definition of the different areas that compose an\n> >>> image sensor pixel array matrix is provided in the specification, and\n> >>> the actual meaning of each selection target when applied to an image\n> >>> sensor was not provided.\n> >>>\n> >>> Provide in the sub-device documentation the definition of the pixel\n> >>> matrix properties and the selection target associated to each of them.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> >>> 1 file changed, 81 insertions(+)\n> >>>\n> >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> >>> index 134d2fb909fa4..c47861dff9b9b 100644\n> >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> >>> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> >>> the image size either up or down. :ref:`v4l2-selection-flags`\n> >>>\n> >>> +.. _v4l2-subdev-pixel-array-properties:\n> >>> +\n> >>> +Selection targets for image sensors properties\n\nMaybe \"Selection targets for image sensors\", and renaming the reference\nto v4l2-subdev-selections-image-sensors ? This section is about how\nselection rectangles are used for sensors, right ?\n\n> >>> +----------------------------------------------\n\nI'd move this further down, after \"Types of selection targets\", as that\nsection contains generic information that applies to sensors too.\n\n> >>> +\n> >>> +The V4L2 selection API can be used on sub-devices that represent an image\n> >>> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> >>> +:ref:`selection ` ioctls.\n> >>> +\n> >>> +Sub-device drivers for image sensor usually register a single source pad, but in\n> >>> +the case they expose more, the pixel array properties can be accessed from\n> >>> +any of them.\n\nIs this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for\ninstance, can be accessed from any source pad indifferently. Do we have\nsensor drivers that create multiple source pads in a subdev today ? If\nnot I'd suggest dropping this, and adding it later if needed (when we'll\nhave a better idea of how that would work).\n\nI think what should be explained here, as also mentioned by Sakari, is\nthat camera sensors can be exposed as multiple subdevs. The text below\nis related to the pixel array, which is always the first subdev, with\none source pad and no sink pad. The other subdevs, modelling additional\nprocessing blocks in the sensor, may use the selection API, but that's\nout of scope for this patch.\n\nAs we'll also need to document how other subdevs use the selection API,\nas well as how sensors are usually handled, would it make sense to move\nthis to a separate file ? Sakari has proposed in [1] to create a new\nDocumentation/driver-api/media/camera-sensor.rst file. It would make\nsense to centralize all sensor information there. This doesn't mean this\nseries should depend on Sakari's patch, we can handle merge conflicts\ndepending on what gets merged first.\n\n[1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/\n\n> >>> +\n> >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> >>\n> >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n> >>\n> >> (same mistake is made elsewhere).\n> > \n> > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was\n> > the right name\n> > \n> >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> >>> +the immutable properties of the several different areas that compose the sensor\n> >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n\nV4L2_TGT_CROP isn't immutable, is it ?\n\n> >>> +units. The logical disposition of pixels is defined by the sensor read-out\n> >>> +starting point and direction, and may differ from the physical disposition of\n> >>> +the pixel units in the pixel array matrix.\n> >>> +\n> >>> +Each pixel matrix portion is contained in a larger rectangle, with the most\n> >>> +largest being the one that describes the pixel matrix physical size. This\n> >>> +defines a hierarchical positional system, where each rectangle is defined\n> >>> +relatively to the largest available one among the ones exposed by the\n> >>> +sub-device driver. Each selection target and the associated pixel array portion\n> >>> +it represents are below presented in order from the largest to the smallest one.\n\nI find this quite confusing. As Hans suggested, I think each target\nshould define its boundaries. I'd drop this paragraph completely, as you\nalready explain below that all rectangles are defined relatively to\nV4L2_SEL_TGT_NATIVE_SIZE.\n\n> >>> +\n> >>> +Pixel array physical size\n> >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> >>> +\n> >>> +The image sensor chip is composed by a number of physical pixels, not all of\n> >>> +them readable by the application processor. Invalid or unreadable lines might\n> >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> >>> +they might be tagged with an invalid data type (DT) so that the receiver\n> >>> +automatically discard them.\n\n\"might\" is a bit weak for unreadable lines, there's no way they can be\ntransmitted if they can't be read :-)\n\nOne way to generalize this a bit would be to explain, after the first\nsentence, that not all pixels may be read by the sensor, that among the\npixels that are read invalid ones may not be transmitted on the bus, and\nthat among transmitted pixels not all of them may be possible to capture\non the receiver side. For instance, invalid lines may be transmitted as\npart of the vertical blanking on parallel buses, or tagged as blanking\ndata or null data on CSI-2 buses. Most receivers are not able to capture\neither.\n\n(On a side note, strictly speaking, a CSI-2 receiver that would be able\nto capture null or blanking packets wouldn't be compliant with the CSI-2\nspec.)\n\n> >>> The size of the whole pixel matrix area is\n> >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> >>> +defined as position (0, 0). All the other selection targets are defined\n> >>> +relatively to this, larger, rectangle. The rectangle returned by\n\ns/, larger,/\n\n> >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> >>> +does not change at run-time and cannot be modified from userspace.\n> >>\n> >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,\n> >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n> > \n> > Yes it is! I'll add it here\n\nShould it be added below instead, where you define\nV4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that\nisn't defined yet when possible.\n\n> >>> +\n> >>> +Pixel array readable area\n> >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> >>> +\n> >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> >>> +area of the pixel array matrix, including pixels with valid image data and pixel\n> >>> +used for calibration purposes, such as optical black pixels. It is not unlikely\n\ns/not unlikely/likely ? Or just \"common\".\n\n> >>> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> >>> +and columns of pixels. Those does not concur in the definition of the\n\ns/does/do/\n\nI'm not sure \"concur\" is the right word. Did you mean \"those are not\npart of the V4L2_SEL_TGT_CROP_BOUNDS rectangle\" ?\n\n> >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> >>> +does not change at run-time and cannot be modified from userspace.\n> >>\n> >> Mention that BOUNDS is enclosed by NATIVE_SIZE.\n> > \n> > I tried to express that in the intro section with\n> > \n> > \"Each pixel matrix portion is contained in a larger rectangle, with the most\n> > largest being the one that describes the pixel matrix physical size.\"\n> > \n> > But I guess it's worth to express that for each target!\n> > \n> >>> +\n> >>> +Pixel array active area\n> >>> +^^^^^^^^^^^^^^^^^^^^^^^\n> >>> +\n> >>> +The portion of the pixel array which contains valid image data is defined as the\n> >>> +active area of the pixel matrix. The active pixel array is is accessed by mean\n> >>\n\ns/is is/is/\n\n> >> mean -> means\n> >>\n> >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> >>> +resolution the sensor can produce and defines the dimension of the full\n> >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n> >>\n> >> BOUNDS -> DEFAULT\n> > \n> > ups\n> > \n> >>> +immutable property of the image sensor, it does not change at run-time and\n> >>> +cannot be modified from userspace.\n> >>\n> >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n> >>\n> >>> +\n> >>> +Analog crop rectangle\n> >>\n> >> Why analog? It's just the crop rectangle, nothing analog about it.\n> > \n> > We used the 'analogCrop' term in libcamera to differentiate the\n> > cropping which happens on the sensor pixel array matrix to select the\n> > region to process and produce image from. Sensor with an on-board\n> > scaler can perform other cropping steps to implement, in example digital\n> > zoom, so we expect to have a 'digital crop' phase as well. RAW\n> > sensors, in example, will only have an analogCrop rectangle.\n> > \n> > Quoting the libcamera definition of analog crop:\n> > \n> > * horizontal and vertical sizes define the portion of the pixel array which\n> > * is read-out and provided to the sensor's internal processing pipeline, before\n> > * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> > * take place.\n> > \n> > should I keep it or remove it ?\n> \n> It's a very confusing term. Especially since this API can also be used with analog\n> video capture devices (Composite/S-Video) where the video signal actually is analog.\n> \n> In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n\nJacopo is right, sensors usually perform cropping in the analog domain\n(by not reading out all pixels from the pixel array), and also support\ncropping in later stages, after binning/skipping, and after further\nscaling. Note that all of these crop operations are optional. Although\nnot common, it's also not unconceivable that a sensor wouldn't support\ncropping at all.\n\nThis being said, it only makes sense to talk about analog crop when\nmultiple crop operations are performed, and thus in the context of the\nwhole sensor, with multiple subdevs. If we explain this, as proposed\nabove, and make it clear that the usage of the selection rectangles\ndefined here applies to the pixel array only, we can drop the \"analog\"\nterm, and just talk about cropping in the pixel array.\n\n> >>> +^^^^^^^^^^^^^^^^^^^^^\n> >>> +\n> >>> +The sensor driver might decide, in order to adjust the image resolution to best\n> >>> +match the one requested by applications, to only process a part of the active\n> >>> +pixel array matrix.\n\nI don't think that's the right approach. With MC-based devices, the\nphilosophy is to expose all configuration parameters to userspace. It's\nnot about sensor drivers making decisions, but about userspace deciding\nto crop at the pixel array level.\n\nThis being said, I'm aware the decision is made by drivers when they're\nmode-based. Please see below for that.\n\n> >>> The selected area is read-out and processed by the image\n> >>> +sensor on-board ISP in order to produce images of the desired size and\n> >>> +resolution while possible maintaing the largest possible field-of-view. The\n> >>\n> >> maintaing -> maintaining\n> >>\n> >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'\n> >> entirely. It doesn't make much sense.\n> > \n> > Ack\n\nIn general, in this section, as we're documenting the pixel array, let's\nnot talk about the ISP.\n\n> >>> +cropped portion of the pixel array which is used to produce images is returned\n> >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n> >>\n> >> represent -> represents\n> >>\n> >>> +change at runtime as it depends on the currently configured sensor mode and\n> >>> +desired image resolution. If the sub-device driver supports that, userspace\n> >>> +can set the analog crop rectangle to select which portion of the pixel array\n> >>\n> >> s/analog//\n> >>\n> >>> +to read out.\n\nI think it's better to focus on the best case, and document usage of\ncrop rectangles in general first, for drivers that expose full\nconfigurability of the sensor. A separate section should then then make\na note of how mode-based drivers differ, which is mostly in the\nV4L2_SEL_TGT_CROP target being read-only, and on the single subdev\nhiding all the processing steps, with the crop target thus being the\nresult of all cropping operations, analog and digital.\n\nSakari's patch has a bit of information about this, it may be useful to\nreuse it or integrate with it somehow.\n\n> >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n> >>\n> >> Make a note that CROP can also be used to obtain optical black pixels.\n> > \n> > What about:\n> > \n> > +desired image resolution. If the sub-device driver supports that, userspace\n> > +can set the analog crop rectangle to select which portion of the pixel array\n> > +to read out including, if supported, optical black pixels.\n> \n> Hmm, that's a bit awkward. How about:\n> \n> +desired image resolution. If supported by the sub-device driver, userspace\n> +can set the crop rectangle to select which portion of the pixel array\n> +to read out. This may include optical black pixels if those are part of\n> +V4L2_SEL_TGT_CROP_BOUNDS.\n>\n> >>> +\n> >>>\n> >>> Types of selection targets\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 62FFCBD87D\n\tfor ;\n\tSun, 9 Aug 2020 17:18:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D80BC60E4D;\n\tSun, 9 Aug 2020 19:18:11 +0200 (CEST)","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 8B42660D5B\n\tfor ;\n\tSun, 9 Aug 2020 19:18:10 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C9408F9;\n\tSun, 9 Aug 2020 19:18:09 +0200 (CEST)"],"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=\"SsdgS5gc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596993490;\n\tbh=+awDotmJl2iGw8B9k4ju6xBNHbyBmWAieBdRi/rr+FE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SsdgS5gcmUXmm2YagR7d0uz+5f9t1w09aj1RKiQBA34eIweiYXVSW0waxmBDfi6K0\n\tPbOWIjtqVtzlJK/Vx9y+jsSOWKaVo8txrQeyIeVKpQBepqm4Y+7kob/ZWLjSF3R6/l\n\tbUzIOZdlftL7lwYmV7P4NomjpUMyzD96gB6OXVdg=","Date":"Sun, 9 Aug 2020 20:17:57 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200809171757.GC5981@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11958,"web_url":"https://patchwork.libcamera.org/comment/11958/","msgid":"<20200809175821.GF5981@pendragon.ideasonboard.com>","date":"2020-08-09T17:58:21","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> The V4L2 selection API are also used to access the pixel array\n> properties of an image sensor, such as the size and position of active\n> pixels and the cropped area of the pixel matrix used to produce images.\n> \n> Currently no clear definition of the different areas that compose an\n> image sensor pixel array matrix is provided in the specification, and\n> the actual meaning of each selection target when applied to an image\n> sensor was not provided.\n> \n> Provide in the sub-device documentation the definition of the pixel\n> matrix properties and the selection target associated to each of them.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> 1 file changed, 81 insertions(+)\n> \n> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> index 134d2fb909fa4..c47861dff9b9b 100644\n> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> the image size either up or down. :ref:`v4l2-selection-flags`\n> \n> +.. _v4l2-subdev-pixel-array-properties:\n> +\n> +Selection targets for image sensors properties\n> +----------------------------------------------\n> +\n> +The V4L2 selection API can be used on sub-devices that represent an image\n> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> +:ref:`selection ` ioctls.\n> +\n> +Sub-device drivers for image sensor usually register a single source pad, but in\n> +the case they expose more, the pixel array properties can be accessed from\n> +any of them.\n> +\n> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> +the immutable properties of the several different areas that compose the sensor\n> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> +units. The logical disposition of pixels is defined by the sensor read-out\n> +starting point and direction, and may differ from the physical disposition of\n> +the pixel units in the pixel array matrix.\n> +\n> +Each pixel matrix portion is contained in a larger rectangle, with the most\n> +largest being the one that describes the pixel matrix physical size. This\n> +defines a hierarchical positional system, where each rectangle is defined\n> +relatively to the largest available one among the ones exposed by the\n> +sub-device driver. Each selection target and the associated pixel array portion\n> +it represents are below presented in order from the largest to the smallest one.\n> +\n> +Pixel array physical size\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The image sensor chip is composed by a number of physical pixels, not all of\n> +them readable by the application processor. Invalid or unreadable lines might\n> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> +they might be tagged with an invalid data type (DT) so that the receiver\n> +automatically discard them. The size of the whole pixel matrix area is\n> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> +defined as position (0, 0). All the other selection targets are defined\n> +relatively to this, larger, rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n\nAs I think I've mentioned previously (not sure if it was by e-mail or on\nIRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\nV4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\nWhat's the advantage of exposing them in the API, when the sensors\ndoesn't provide them to the rest of the pipeline ?\n\n> +Pixel array readable area\n> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> +area of the pixel array matrix, including pixels with valid image data and pixel\n> +used for calibration purposes, such as optical black pixels. It is not unlikely\n> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> +and columns of pixels. Those does not concur in the definition of the\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> +does not change at run-time and cannot be modified from userspace.\n> +\n> +Pixel array active area\n> +^^^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The portion of the pixel array which contains valid image data is defined as the\n> +active area of the pixel matrix. The active pixel array is is accessed by mean\n> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> +resolution the sensor can produce and defines the dimension of the full\n> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n> +immutable property of the image sensor, it does not change at run-time and\n> +cannot be modified from userspace.\n> +\n> +Analog crop rectangle\n> +^^^^^^^^^^^^^^^^^^^^^\n> +\n> +The sensor driver might decide, in order to adjust the image resolution to best\n> +match the one requested by applications, to only process a part of the active\n> +pixel array matrix. The selected area is read-out and processed by the image\n> +sensor on-board ISP in order to produce images of the desired size and\n> +resolution while possible maintaing the largest possible field-of-view. The\n> +cropped portion of the pixel array which is used to produce images is returned\n> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n> +change at runtime as it depends on the currently configured sensor mode and\n> +desired image resolution. If the sub-device driver supports that, userspace\n> +can set the analog crop rectangle to select which portion of the pixel array\n> +to read out.\n> +\n> \n> Types of selection targets\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 54E50BD87C\n\tfor ;\n\tSun, 9 Aug 2020 17:58:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD1D060E4D;\n\tSun, 9 Aug 2020 19:58:34 +0200 (CEST)","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 DFF1460D5B\n\tfor ;\n\tSun, 9 Aug 2020 19:58:33 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4116BF9;\n\tSun, 9 Aug 2020 19:58:33 +0200 (CEST)"],"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=\"LGOCh/GY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596995913;\n\tbh=fbcUoZWgysfD+dQCQy5xuMx4BrHrBVaa4EthxJrw2VA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LGOCh/GYSjk/dKm+z5K4kxGfOnY1Xy1coNN2NIhrt8y5AJHv95d9YI84MsCVyibpV\n\tHbcd+k3F18yoTMzL/9vZGGcF7rlm8DiST1LFNEpolLT+HpRxyR+lwf1tIZ/uJhVKqR\n\tmx9eHrKe+CaLkKt4j60xFqDybEGXhhfdqbW9YJHI=","Date":"Sun, 9 Aug 2020 20:58:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200809175821.GF5981@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200805105721.15445-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11961,"web_url":"https://patchwork.libcamera.org/comment/11961/","msgid":"<20200810081245.s5av6vg2nvduo3qa@uno.localdomain>","date":"2020-08-10T08:12:45","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Sakari,\n thanks for feedback.\n\nOn Thu, Aug 06, 2020 at 01:24:47AM +0300, Sakari Ailus wrote:\n> Hi Jacopo,\n>\n> Thanks for the patchset.\n>\n> This improves selection documentation quite a bit. Please see my comments\n> below.\n>\n\nThanks. Laurent just mentioned your effort on documenting sensor\ndriver drivers, I would be happy to somehow merge the two efforts.\n\n> On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > The V4L2 selection API are also used to access the pixel array\n> > properties of an image sensor, such as the size and position of active\n> > pixels and the cropped area of the pixel matrix used to produce images.\n> >\n> > Currently no clear definition of the different areas that compose an\n> > image sensor pixel array matrix is provided in the specification, and\n> > the actual meaning of each selection target when applied to an image\n> > sensor was not provided.\n> >\n> > Provide in the sub-device documentation the definition of the pixel\n> > matrix properties and the selection target associated to each of them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > 1 file changed, 81 insertions(+)\n> >\n> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > index 134d2fb909fa4..c47861dff9b9b 100644\n> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > the image size either up or down. :ref:`v4l2-selection-flags`\n> >\n> > +.. _v4l2-subdev-pixel-array-properties:\n> > +\n> > +Selection targets for image sensors properties\n> > +----------------------------------------------\n> > +\n> > +The V4L2 selection API can be used on sub-devices that represent an image\n> > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > +:ref:`selection ` ioctls.\n> > +\n> > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > +the case they expose more, the pixel array properties can be accessed from\n> > +any of them.\n>\n> Is this a hypothetical case or are there examples?\n>\n> Also note that camera sensor drivers may expose more than one sub-devices,\n> only one of which represents the pixel array.\n>\n\nYes, I just tried to mention that as a possible use case, but I admit\nI might be a bit confused about that. I would, as suggested by\nLaurent, drop this part and add it back when we'll have a case at\nhands.\n\n> > +\n> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > +the immutable properties of the several different areas that compose the sensor\n> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > +units. The logical disposition of pixels is defined by the sensor read-out\n> > +starting point and direction, and may differ from the physical disposition of\n> > +the pixel units in the pixel array matrix.\n> > +\n> > +Each pixel matrix portion is contained in a larger rectangle, with the most\n>\n> s/larger\\K/ or equal/\n>\n> s/most//\n>\n> > +largest being the one that describes the pixel matrix physical size. This\n> > +defines a hierarchical positional system, where each rectangle is defined\n>\n> s/,//\n>\n\nAck on both sections.\n\n> > +relatively to the largest available one among the ones exposed by the\n> > +sub-device driver. Each selection target and the associated pixel array portion\n> > +it represents are below presented in order from the largest to the smallest one.\n> > +\n> > +Pixel array physical size\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The image sensor chip is composed by a number of physical pixels, not all of\n> > +them readable by the application processor. Invalid or unreadable lines might\n> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > +they might be tagged with an invalid data type (DT) so that the receiver\n> > +automatically discard them. The size of the whole pixel matrix area is\n> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > +defined as position (0, 0). All the other selection targets are defined\n> > +relatively to this, larger, rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n> > +\n> > +Pixel array readable area\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> > +area of the pixel array matrix, including pixels with valid image data and pixel\n> > +used for calibration purposes, such as optical black pixels. It is not unlikely\n> > +that valid pixels and optical black pixels are surrounded by non-readable rows\n> > +and columns of pixels. Those does not concur in the definition of the\n>\n> How about: \"Only pixels that can be read out are included in the\n> V4L2_SEL_TGT_CROP_BOUNDS rectangle.\"?\n>\n\nI'll more extensively reply on this to laurent's comment on the \"Pixel\narray physical size\" section, as it also address this comment you have\nhere. There's a SMIA++ question for you there :)\n\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n> > +\n> > +Pixel array active area\n> > +^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The portion of the pixel array which contains valid image data is defined as the\n> > +active area of the pixel matrix. The active pixel array is is accessed by mean\n>\n> s/accessed/described/\n>\n> Another word than \"active\" here would be great as we already have active\n> and try contexts for selections.\n>\n\nI feel like we would need to define a stricter gloassary as well.\nTo replace active I would usually say \"Valid for image capture\",\n\"pixel that contain valid image data\". But they're both longer\n\n> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n>\n> s/the larger//\n>\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> > +resolution the sensor can produce and defines the dimension of the full\n>\n> s/resolution/size/\n>\n> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n>\n> s/-/ /g\n>\n> > +immutable property of the image sensor, it does not change at run-time and\n> > +cannot be modified from userspace.\n> > +\n> > +Analog crop rectangle\n> > +^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The sensor driver might decide, in order to adjust the image resolution to best\n> > +match the one requested by applications, to only process a part of the active\n>\n> s/, to\\K/ instruct the hardware to/\n>\n> > +pixel array matrix. The selected area is read-out and processed by the image\n> > +sensor on-board ISP in order to produce images of the desired size and\n> > +resolution while possible maintaing the largest possible field-of-view. The\n>\n> s/size\\K[^.]?*\\./m\n>\n> > +cropped portion of the pixel array which is used to produce images is returned\n>\n> s/produce/read out/\n> s/returned/configured/\n>\n> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n>\n> s/by/using/\n>\n> I'd leave out the rest of the sentence after \"target\" above.\n\nI added this as I initially listed CROP in the immutable targets, and\nI was trying to specify it's actually not here. I'll remove the first\nambiguity then I could drop this patch.\n\n>\n> > +change at runtime as it depends on the currently configured sensor mode and\n> > +desired image resolution. If the sub-device driver supports that, userspace\n> > +can set the analog crop rectangle to select which portion of the pixel array\n> > +to read out.\n>\n> How about instead:\n>\n> Register list based drivers generally do not allow setting analogue crop\n> rectangles.\n>\n\nI could do that. I'm always a bit uncertain on mentioning 'register\nlist drivers', 'mode-based drivers' etc as we don't have a real\ndefinition of them in the documentation..\n\nThanks\n j\n\n\n> > +\n> >\n> > Types of selection targets\n> > --------------------------\n>\n> --\n> Kind regards,\n>\n> Sakari Ailus","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 159CCBD87D\n\tfor ;\n\tMon, 10 Aug 2020 08:09:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5AFF60E89;\n\tMon, 10 Aug 2020 10:09:08 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 582C86038B\n\tfor ;\n\tMon, 10 Aug 2020 10:09:07 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id A3634FF80A;\n\tMon, 10 Aug 2020 08:09:04 +0000 (UTC)"],"X-Originating-IP":"82.52.18.94","Date":"Mon, 10 Aug 2020 10:12:45 +0200","From":"Jacopo Mondi ","To":"Sakari Ailus ","Message-ID":"<20200810081245.s5av6vg2nvduo3qa@uno.localdomain>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200805222447.GP13316@paasikivi.fi.intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200805222447.GP13316@paasikivi.fi.intel.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11962,"web_url":"https://patchwork.libcamera.org/comment/11962/","msgid":"<20200810081437.nymrroxmrskapbqg@uno.localdomain>","date":"2020-08-10T08:14:37","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Sakari, Laurent\n tanks for comments.\n\n\nOn Sun, Aug 09, 2020 at 08:17:57PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo, Hans,\n>\n> On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n> > On 06/08/2020 11:50, Jacopo Mondi wrote:\n> > > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> > >> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> > >>> The V4L2 selection API are also used to access the pixel array\n> > >>\n> > >> are -> is\n> > >>\n> > >>> properties of an image sensor, such as the size and position of active\n> > >>> pixels and the cropped area of the pixel matrix used to produce images.\n> > >>>\n> > >>> Currently no clear definition of the different areas that compose an\n> > >>> image sensor pixel array matrix is provided in the specification, and\n> > >>> the actual meaning of each selection target when applied to an image\n> > >>> sensor was not provided.\n> > >>>\n> > >>> Provide in the sub-device documentation the definition of the pixel\n> > >>> matrix properties and the selection target associated to each of them.\n> > >>>\n> > >>> Signed-off-by: Jacopo Mondi \n> > >>> ---\n> > >>> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > >>> 1 file changed, 81 insertions(+)\n> > >>>\n> > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > >>> index 134d2fb909fa4..c47861dff9b9b 100644\n> > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > >>> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > >>> the image size either up or down. :ref:`v4l2-selection-flags`\n> > >>>\n> > >>> +.. _v4l2-subdev-pixel-array-properties:\n> > >>> +\n> > >>> +Selection targets for image sensors properties\n>\n> Maybe \"Selection targets for image sensors\", and renaming the reference\n> to v4l2-subdev-selections-image-sensors ? This section is about how\n> selection rectangles are used for sensors, right ?\n>\n> > >>> +----------------------------------------------\n>\n> I'd move this further down, after \"Types of selection targets\", as that\n> section contains generic information that applies to sensors too.\n\nAck on both points.\n\n>\n> > >>> +\n> > >>> +The V4L2 selection API can be used on sub-devices that represent an image\n> > >>> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > >>> +:ref:`selection ` ioctls.\n> > >>> +\n> > >>> +Sub-device drivers for image sensor usually register a single source pad, but in\n> > >>> +the case they expose more, the pixel array properties can be accessed from\n> > >>> +any of them.\n>\n> Is this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for\n> instance, can be accessed from any source pad indifferently. Do we have\n> sensor drivers that create multiple source pads in a subdev today ? If\n> not I'd suggest dropping this, and adding it later if needed (when we'll\n> have a better idea of how that would work).\n\nYes, this was meant to cover cases which I still have not a clear idea\nabout but I suspect people might have question about looking at their\ndrivers. I'm totally fine adding it later when required.\n\n>\n> I think what should be explained here, as also mentioned by Sakari, is\n> that camera sensors can be exposed as multiple subdevs. The text below\n> is related to the pixel array, which is always the first subdev, with\n> one source pad and no sink pad. The other subdevs, modelling additional\n> processing blocks in the sensor, may use the selection API, but that's\n> out of scope for this patch.\n>\n> As we'll also need to document how other subdevs use the selection API,\n> as well as how sensors are usually handled, would it make sense to move\n> this to a separate file ? Sakari has proposed in [1] to create a new\n> Documentation/driver-api/media/camera-sensor.rst file. It would make\n> sense to centralize all sensor information there. This doesn't mean this\n> series should depend on Sakari's patch, we can handle merge conflicts\n> depending on what gets merged first.\n\nI totally missed that one but I equally totally welcome that change. I\nwould be happy to rebase this work on top of Sakari's patch which I\nwill soon give a read to.\n\n>\n> [1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/\n>\n> > >>> +\n> > >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > >>\n> > >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n> > >>\n> > >> (same mistake is made elsewhere).\n> > >\n> > > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was\n> > > the right name\n> > >\n> > >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > >>> +the immutable properties of the several different areas that compose the sensor\n> > >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n>\n> V4L2_TGT_CROP isn't immutable, is it ?\n>\n\nRight, I noticed that, but didn't want to be too heavy with createing\na special section for CROP. But you're right, it's not immutable so it\nshould not be mentioned here.\n\n> > >>> +units. The logical disposition of pixels is defined by the sensor read-out\n> > >>> +starting point and direction, and may differ from the physical disposition of\n> > >>> +the pixel units in the pixel array matrix.\n> > >>> +\n> > >>> +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > >>> +largest being the one that describes the pixel matrix physical size. This\n> > >>> +defines a hierarchical positional system, where each rectangle is defined\n> > >>> +relatively to the largest available one among the ones exposed by the\n> > >>> +sub-device driver. Each selection target and the associated pixel array portion\n> > >>> +it represents are below presented in order from the largest to the smallest one.\n>\n> I find this quite confusing. As Hans suggested, I think each target\n> should define its boundaries. I'd drop this paragraph completely, as you\n> already explain below that all rectangles are defined relatively to\n> V4L2_SEL_TGT_NATIVE_SIZE.\n>\n\nAck.\n\n> > >>> +\n> > >>> +Pixel array physical size\n> > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > >>> +\n> > >>> +The image sensor chip is composed by a number of physical pixels, not all of\n> > >>> +them readable by the application processor. Invalid or unreadable lines might\n> > >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > >>> +they might be tagged with an invalid data type (DT) so that the receiver\n> > >>> +automatically discard them.\n>\n> \"might\" is a bit weak for unreadable lines, there's no way they can be\n> transmitted if they can't be read :-)\n\nThis paragraph reflects my confusion on the subject. My interpretation\nis that for CSI-2 sensors, you cannot crop a black pixels area and\ncapture just them. At the contrary, the black pixels are sent on the\nbus (maybe that can be optionally enabled/disabled) tagged with a\nspecial DT and interleaved with image data and you have to instruct\nyour receiver to discard or accept that DT, and have black pixels\ncaptured to a separate memory area, or at buffer end (that depends on\nthe receiver's architecture I guess). At the same time, I won't be\nsurprised if some sensor's allow you to explicitly crop on black\npixels areas and only put them on the bus. Knowing how the SMIA++\nstandard handles that part might help establishing an expected\nbehaviour (I really, really, wish we had as a community any leverage\nto influence sensor manufacturer towards a standardized behaviour, it\nwould be time for the industry to do so. ).\n\nIf that's correct, I wonder how would that possibly work with parallel\nsensors, where you cannot tag data on the bus. I there assume you have\nto explicitly select the black region to capture.\n\nI there tried to, confusingly, express that different behaviour and\nstay as much as possible generic not to rule out any existing case.\n\n>\n> One way to generalize this a bit would be to explain, after the first\n> sentence, that not all pixels may be read by the sensor, that among the\n> pixels that are read invalid ones may not be transmitted on the bus, and\n> that among transmitted pixels not all of them may be possible to capture\n> on the receiver side. For instance, invalid lines may be transmitted as\n> part of the vertical blanking on parallel buses, or tagged as blanking\n> data or null data on CSI-2 buses. Most receivers are not able to capture\n> either.\n>\n> (On a side note, strictly speaking, a CSI-2 receiver that would be able\n> to capture null or blanking packets wouldn't be compliant with the CSI-2\n> spec.)\n>\n> > >>> The size of the whole pixel matrix area is\n> > >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > >>> +defined as position (0, 0). All the other selection targets are defined\n> > >>> +relatively to this, larger, rectangle. The rectangle returned by\n>\n> s/, larger,/\n>\n> > >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > >>> +does not change at run-time and cannot be modified from userspace.\n> > >>\n> > >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,\n> > >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n> > >\n> > > Yes it is! I'll add it here\n>\n> Should it be added below instead, where you define\n> V4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that\n> isn't defined yet when possible.\n>\n\nI have a question for Sakari on this target, but I'll deflect it to\nthe reply to your comment on patch 1/4.\n\n> > >>> +\n> > >>> +Pixel array readable area\n> > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > >>> +\n> > >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> > >>> +area of the pixel array matrix, including pixels with valid image data and pixel\n> > >>> +used for calibration purposes, such as optical black pixels. It is not unlikely\n>\n> s/not unlikely/likely ? Or just \"common\".\n>\n> > >>> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> > >>> +and columns of pixels. Those does not concur in the definition of the\n>\n> s/does/do/\n>\n> I'm not sure \"concur\" is the right word. Did you mean \"those are not\n> part of the V4L2_SEL_TGT_CROP_BOUNDS rectangle\" ?\n\nYes, I meant they should not be counted in the definition of the BOUND\nrectangle sizes.\n\n>\n> > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> > >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> > >>> +does not change at run-time and cannot be modified from userspace.\n> > >>\n> > >> Mention that BOUNDS is enclosed by NATIVE_SIZE.\n> > >\n> > > I tried to express that in the intro section with\n> > >\n> > > \"Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > largest being the one that describes the pixel matrix physical size.\"\n> > >\n> > > But I guess it's worth to express that for each target!\n> > >\n> > >>> +\n> > >>> +Pixel array active area\n> > >>> +^^^^^^^^^^^^^^^^^^^^^^^\n> > >>> +\n> > >>> +The portion of the pixel array which contains valid image data is defined as the\n> > >>> +active area of the pixel matrix. The active pixel array is is accessed by mean\n> > >>\n>\n> s/is is/is/\n>\n> > >> mean -> means\n> > >>\n> > >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> > >>> +resolution the sensor can produce and defines the dimension of the full\n> > >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n> > >>\n> > >> BOUNDS -> DEFAULT\n> > >\n> > > ups\n> > >\n> > >>> +immutable property of the image sensor, it does not change at run-time and\n> > >>> +cannot be modified from userspace.\n> > >>\n> > >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n> > >>\n> > >>> +\n> > >>> +Analog crop rectangle\n> > >>\n> > >> Why analog? It's just the crop rectangle, nothing analog about it.\n> > >\n> > > We used the 'analogCrop' term in libcamera to differentiate the\n> > > cropping which happens on the sensor pixel array matrix to select the\n> > > region to process and produce image from. Sensor with an on-board\n> > > scaler can perform other cropping steps to implement, in example digital\n> > > zoom, so we expect to have a 'digital crop' phase as well. RAW\n> > > sensors, in example, will only have an analogCrop rectangle.\n> > >\n> > > Quoting the libcamera definition of analog crop:\n> > >\n> > > * horizontal and vertical sizes define the portion of the pixel array which\n> > > * is read-out and provided to the sensor's internal processing pipeline, before\n> > > * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> > > * take place.\n> > >\n> > > should I keep it or remove it ?\n> >\n> > It's a very confusing term. Especially since this API can also be used with analog\n> > video capture devices (Composite/S-Video) where the video signal actually is analog.\n> >\n> > In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n>\n> Jacopo is right, sensors usually perform cropping in the analog domain\n> (by not reading out all pixels from the pixel array), and also support\n> cropping in later stages, after binning/skipping, and after further\n> scaling. Note that all of these crop operations are optional. Although\n> not common, it's also not unconceivable that a sensor wouldn't support\n> cropping at all.\n>\n> This being said, it only makes sense to talk about analog crop when\n> multiple crop operations are performed, and thus in the context of the\n> whole sensor, with multiple subdevs. If we explain this, as proposed\n> above, and make it clear that the usage of the selection rectangles\n> defined here applies to the pixel array only, we can drop the \"analog\"\n> term, and just talk about cropping in the pixel array.\n>\n\nIt's fine as long as it removes any ambiguity.\n\n> > >>> +^^^^^^^^^^^^^^^^^^^^^\n> > >>> +\n> > >>> +The sensor driver might decide, in order to adjust the image resolution to best\n> > >>> +match the one requested by applications, to only process a part of the active\n> > >>> +pixel array matrix.\n>\n> I don't think that's the right approach. With MC-based devices, the\n> philosophy is to expose all configuration parameters to userspace. It's\n> not about sensor drivers making decisions, but about userspace deciding\n> to crop at the pixel array level.\n>\n> This being said, I'm aware the decision is made by drivers when they're\n> mode-based. Please see below for that.\n\nCorrect, and I should now be used enough to the 'userspace drives'\napproach to remember about documenting it :)\n\n>\n> > >>> The selected area is read-out and processed by the image\n> > >>> +sensor on-board ISP in order to produce images of the desired size and\n> > >>> +resolution while possible maintaing the largest possible field-of-view. The\n> > >>\n> > >> maintaing -> maintaining\n> > >>\n> > >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'\n> > >> entirely. It doesn't make much sense.\n> > >\n> > > Ack\n>\n> In general, in this section, as we're documenting the pixel array, let's\n> not talk about the ISP.\n>\n> > >>> +cropped portion of the pixel array which is used to produce images is returned\n> > >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n> > >>\n> > >> represent -> represents\n> > >>\n> > >>> +change at runtime as it depends on the currently configured sensor mode and\n> > >>> +desired image resolution. If the sub-device driver supports that, userspace\n> > >>> +can set the analog crop rectangle to select which portion of the pixel array\n> > >>\n> > >> s/analog//\n> > >>\n> > >>> +to read out.\n>\n> I think it's better to focus on the best case, and document usage of\n> crop rectangles in general first, for drivers that expose full\n> configurability of the sensor. A separate section should then then make\n> a note of how mode-based drivers differ, which is mostly in the\n> V4L2_SEL_TGT_CROP target being read-only, and on the single subdev\n> hiding all the processing steps, with the crop target thus being the\n> result of all cropping operations, analog and digital.\n>\n> Sakari's patch has a bit of information about this, it may be useful to\n> reuse it or integrate with it somehow.\n>\n\nI'll try to see how the two parts can be piled one on top of the\nother.\n\nThanks\n j\n\n\n> > >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n> > >>\n> > >> Make a note that CROP can also be used to obtain optical black pixels.\n> > >\n> > > What about:\n> > >\n> > > +desired image resolution. If the sub-device driver supports that, userspace\n> > > +can set the analog crop rectangle to select which portion of the pixel array\n> > > +to read out including, if supported, optical black pixels.\n> >\n> > Hmm, that's a bit awkward. How about:\n> >\n> > +desired image resolution. If supported by the sub-device driver, userspace\n> > +can set the crop rectangle to select which portion of the pixel array\n> > +to read out. This may include optical black pixels if those are part of\n> > +V4L2_SEL_TGT_CROP_BOUNDS.\n> >\n> > >>> +\n> > >>>\n> > >>> Types of selection targets\n> > >>> --------------------------\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 945AEBD87D\n\tfor ;\n\tMon, 10 Aug 2020 08:11:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 033D960E89;\n\tMon, 10 Aug 2020 10:11:11 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DB886038B\n\tfor ;\n\tMon, 10 Aug 2020 10:11:10 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id CDCF9240004;\n\tMon, 10 Aug 2020 08:11:03 +0000 (UTC)"],"Date":"Mon, 10 Aug 2020 10:14:37 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200810081437.nymrroxmrskapbqg@uno.localdomain>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t\n\t<20200809171757.GC5981@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200809171757.GC5981@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11963,"web_url":"https://patchwork.libcamera.org/comment/11963/","msgid":"<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>","date":"2020-08-10T08:17:57","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > The V4L2 selection API are also used to access the pixel array\n> > properties of an image sensor, such as the size and position of active\n> > pixels and the cropped area of the pixel matrix used to produce images.\n> >\n> > Currently no clear definition of the different areas that compose an\n> > image sensor pixel array matrix is provided in the specification, and\n> > the actual meaning of each selection target when applied to an image\n> > sensor was not provided.\n> >\n> > Provide in the sub-device documentation the definition of the pixel\n> > matrix properties and the selection target associated to each of them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > 1 file changed, 81 insertions(+)\n> >\n> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > index 134d2fb909fa4..c47861dff9b9b 100644\n> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > the image size either up or down. :ref:`v4l2-selection-flags`\n> >\n> > +.. _v4l2-subdev-pixel-array-properties:\n> > +\n> > +Selection targets for image sensors properties\n> > +----------------------------------------------\n> > +\n> > +The V4L2 selection API can be used on sub-devices that represent an image\n> > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > +:ref:`selection ` ioctls.\n> > +\n> > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > +the case they expose more, the pixel array properties can be accessed from\n> > +any of them.\n> > +\n> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > +the immutable properties of the several different areas that compose the sensor\n> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > +units. The logical disposition of pixels is defined by the sensor read-out\n> > +starting point and direction, and may differ from the physical disposition of\n> > +the pixel units in the pixel array matrix.\n> > +\n> > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > +largest being the one that describes the pixel matrix physical size. This\n> > +defines a hierarchical positional system, where each rectangle is defined\n> > +relatively to the largest available one among the ones exposed by the\n> > +sub-device driver. Each selection target and the associated pixel array portion\n> > +it represents are below presented in order from the largest to the smallest one.\n> > +\n> > +Pixel array physical size\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The image sensor chip is composed by a number of physical pixels, not all of\n> > +them readable by the application processor. Invalid or unreadable lines might\n> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > +they might be tagged with an invalid data type (DT) so that the receiver\n> > +automatically discard them. The size of the whole pixel matrix area is\n> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > +defined as position (0, 0). All the other selection targets are defined\n> > +relatively to this, larger, rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n>\n> As I think I've mentioned previously (not sure if it was by e-mail or on\n> IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> What's the advantage of exposing them in the API, when the sensors\n> doesn't provide them to the rest of the pipeline ?\n>\n\nI don't know :) I'm also bit confused on what's the purpose of\nNATIVE, this commit seems to suggest it was meant to replace\nCROP_BOUNDS, but I'm not sure about that.\n\ncommit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\nAuthor: Sakari Ailus \nDate: Thu Nov 6 16:54:33 2014 -0300\n\n [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n\n Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n interface.\n\nSakari, do you recall if that's was the original plan ?\n\nThanks\n j\n\n> > +Pixel array readable area\n> > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> > +area of the pixel array matrix, including pixels with valid image data and pixel\n> > +used for calibration purposes, such as optical black pixels. It is not unlikely\n> > +that valid pixels and optical black pixels are surrounded by non-readable rows\n> > +and columns of pixels. Those does not concur in the definition of the\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> > +does not change at run-time and cannot be modified from userspace.\n> > +\n> > +Pixel array active area\n> > +^^^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The portion of the pixel array which contains valid image data is defined as the\n> > +active area of the pixel matrix. The active pixel array is is accessed by mean\n> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> > +resolution the sensor can produce and defines the dimension of the full\n> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n> > +immutable property of the image sensor, it does not change at run-time and\n> > +cannot be modified from userspace.\n> > +\n> > +Analog crop rectangle\n> > +^^^^^^^^^^^^^^^^^^^^^\n> > +\n> > +The sensor driver might decide, in order to adjust the image resolution to best\n> > +match the one requested by applications, to only process a part of the active\n> > +pixel array matrix. The selected area is read-out and processed by the image\n> > +sensor on-board ISP in order to produce images of the desired size and\n> > +resolution while possible maintaing the largest possible field-of-view. The\n> > +cropped portion of the pixel array which is used to produce images is returned\n> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n> > +change at runtime as it depends on the currently configured sensor mode and\n> > +desired image resolution. If the sub-device driver supports that, userspace\n> > +can set the analog crop rectangle to select which portion of the pixel array\n> > +to read out.\n> > +\n> >\n> > Types of selection targets\n> > --------------------------\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 3B65CBD87C\n\tfor ;\n\tMon, 10 Aug 2020 08:14:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A093260F27;\n\tMon, 10 Aug 2020 10:14:18 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B594D6038B\n\tfor ;\n\tMon, 10 Aug 2020 10:14:17 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id A743D240003;\n\tMon, 10 Aug 2020 08:14:15 +0000 (UTC)"],"Date":"Mon, 10 Aug 2020 10:17:57 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200809175821.GF5981@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12030,"web_url":"https://patchwork.libcamera.org/comment/12030/","msgid":"<20200818081444.GP24582@paasikivi.fi.intel.com>","date":"2020-08-18T08:14:44","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"On Thu, Aug 06, 2020 at 03:22:34PM +0200, Hans Verkuil wrote:\n> On 06/08/2020 14:54, Sakari Ailus wrote:\n> > Hi Hans,\n> > \n> > On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n> >> On 06/08/2020 11:50, Jacopo Mondi wrote:\n> >>> Hi Hans,\n> >>>\n> >>> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> >>>> Hi Jacopo,\n> >>>>\n> >>>> Some review comments below:\n> >>>>\n> >>>> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> >>>>> +Analog crop rectangle\n> >>>>\n> >>>> Why analog? It's just the crop rectangle, nothing analog about it.\n> >>>>\n> >>>\n> >>> We used the 'analogCrop' term in libcamera to differentiate the\n> >>> cropping which happens on the sensor pixel array matrix to select the\n> >>> region to process and produce image from. Sensor with an on-board\n> >>> scaler can perform other cropping steps to implement, in example digital\n> >>> zoom, so we expect to have a 'digital crop' phase as well. RAW\n> >>> sensors, in example, will only have an analogCrop rectangle.\n> >>>\n> >>> Quoting the libcamera definition of analog crop:\n> >>>\n> >>> * horizontal and vertical sizes define the portion of the pixel array which\n> >>> * is read-out and provided to the sensor's internal processing pipeline, before\n> >>> * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> >>> * take place.\n> >>>\n> >>> should I keep it or remove it ?\n> >>\n> >> It's a very confusing term. Especially since this API can also be used with analog\n> >> video capture devices (Composite/S-Video) where the video signal actually is analog.\n> >>\n> >> In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n> > \n> > There isn't in the V4L2 API but I don't see why we couldn't document it\n> > here. Analogue crop is an established term related to raw (perhaps others,\n> > too?) camera sensors which describes cropping that is implemented by not\n> > reading parts of the pixel array.\n> > \n> > As this documentation only applies to camera sensors, I think it's entirely\n> > appropriate to document this here, and using that term.\n> > \n> \n> It's always been called just 'crop' in the V4L2 API, so renaming it suddenly\n> to something else is IMHO confusing. What you can do, however, is that in the\n\nThis has been actually implemented a decade ago but it seems the\ndocumentation has either never been there or has disappeared.\n\nMost drivers hide this as they work on the frame interval and output size\nalone, ignoring the rest. Despite that, generally camera sensors do have\nboth analogue and digital cropping capabilities with differing features\n(granularity and dependency to frame interval).\n\n> description of the \"crop rectangle\" you mention that \"it is also known as\n> \"analog crop\" in the context of camera sensors.\n\nJust saying it's a crop rectangle isn't exactly wrong but it's incomplete.\nThe frame interval calculation requires that information so this should be\nmore than just a side note.\n\n> \n> With perhaps some more extensive explanation of the term.","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 22210BE173\n\tfor ;\n\tTue, 18 Aug 2020 08:14:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A82C961930;\n\tTue, 18 Aug 2020 10:14:51 +0200 (CEST)","from mga17.intel.com (mga17.intel.com [192.55.52.151])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 346A660382\n\tfor ;\n\tTue, 18 Aug 2020 10:14:49 +0200 (CEST)","from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t18 Aug 2020 01:14:48 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby fmsmga004-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Aug 2020 01:14:46 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid 901DD2064F; Tue, 18 Aug 2020 11:14:44 +0300 (EEST)"],"IronPort-SDR":["WoOUBDj9tdOLftTxgsXwaIJW8nFCB4IljaeL6iVyb2D3kTWMDcZKGsudSvYLmgvCOiHRrT6zzt\n\taWUkfRmZEKRg==","HZO4S2gLG8tIwzkzNQiibRVVN/4jib24kwjpgU9CJb1syLYSYeyw3p1qNUzaEo4hq357ETKo/V\n\t7ddmLAdbNmtQ=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9716\"; a=\"134920313\"","E=Sophos;i=\"5.76,326,1592895600\"; d=\"scan'208\";a=\"134920313\"","E=Sophos;i=\"5.76,326,1592895600\"; d=\"scan'208\";a=\"320008310\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Tue, 18 Aug 2020 11:14:44 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Message-ID":"<20200818081444.GP24582@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t\n\t<20200806125445.GA16270@paasikivi.fi.intel.com>\n\t<1ee9f378-8df1-7c03-2136-ce7870934443@xs4all.nl>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<1ee9f378-8df1-7c03-2136-ce7870934443@xs4all.nl>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12031,"web_url":"https://patchwork.libcamera.org/comment/12031/","msgid":"<20200818081743.GQ24582@paasikivi.fi.intel.com>","date":"2020-08-18T08:17:43","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > The V4L2 selection API are also used to access the pixel array\n> > > properties of an image sensor, such as the size and position of active\n> > > pixels and the cropped area of the pixel matrix used to produce images.\n> > >\n> > > Currently no clear definition of the different areas that compose an\n> > > image sensor pixel array matrix is provided in the specification, and\n> > > the actual meaning of each selection target when applied to an image\n> > > sensor was not provided.\n> > >\n> > > Provide in the sub-device documentation the definition of the pixel\n> > > matrix properties and the selection target associated to each of them.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > 1 file changed, 81 insertions(+)\n> > >\n> > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > >\n> > > +.. _v4l2-subdev-pixel-array-properties:\n> > > +\n> > > +Selection targets for image sensors properties\n> > > +----------------------------------------------\n> > > +\n> > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > +:ref:`selection ` ioctls.\n> > > +\n> > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > +the case they expose more, the pixel array properties can be accessed from\n> > > +any of them.\n> > > +\n> > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > +the immutable properties of the several different areas that compose the sensor\n> > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > +starting point and direction, and may differ from the physical disposition of\n> > > +the pixel units in the pixel array matrix.\n> > > +\n> > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > +largest being the one that describes the pixel matrix physical size. This\n> > > +defines a hierarchical positional system, where each rectangle is defined\n> > > +relatively to the largest available one among the ones exposed by the\n> > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > +it represents are below presented in order from the largest to the smallest one.\n> > > +\n> > > +Pixel array physical size\n> > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > +\n> > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > +them readable by the application processor. Invalid or unreadable lines might\n> > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > +automatically discard them. The size of the whole pixel matrix area is\n> > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > +defined as position (0, 0). All the other selection targets are defined\n> > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > +does not change at run-time and cannot be modified from userspace.\n> >\n> > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > What's the advantage of exposing them in the API, when the sensors\n> > doesn't provide them to the rest of the pipeline ?\n> >\n> \n> I don't know :) I'm also bit confused on what's the purpose of\n> NATIVE, this commit seems to suggest it was meant to replace\n> CROP_BOUNDS, but I'm not sure about that.\n> \n> commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> Author: Sakari Ailus \n> Date: Thu Nov 6 16:54:33 2014 -0300\n> \n> [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> \n> Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> interface.\n> \n> Sakari, do you recall if that's was the original plan ?\n\nThat was to denote the size of the pixel array indeed. We didn't discuss\ndark or invalid pixels at the time.\n\nSo this was just there to tell that it's the pixel array you're cropping\nfrom.\n\nBut as long as it's API-wise compatible, I don't think anything prevents\nre-purposing this to include other areas. The documentation (AFAIR) does\nnot say this has to be the same as the crop bounds rectangle.","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 DBBD9BE173\n\tfor ;\n\tTue, 18 Aug 2020 08:17:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B3F461930;\n\tTue, 18 Aug 2020 10:17:51 +0200 (CEST)","from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6024E60382\n\tfor ;\n\tTue, 18 Aug 2020 10:17:50 +0200 (CEST)","from fmsmga006.fm.intel.com ([10.253.24.20])\n\tby orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t18 Aug 2020 01:17:47 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby fmsmga006-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Aug 2020 01:17:45 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid D15A82064F; Tue, 18 Aug 2020 11:17:43 +0300 (EEST)"],"IronPort-SDR":["Sf4kw9PlgrnCgVN1bVe8JQ6lOk9EE0k4/DrTSQNTGCIM7yH4ihEjVA4nfUN1/66Y26ctTIUbiw\n\tXZ3zOGypSC9Q==","TTRGiwxtx7cZjUDEU9IHppKQHqCh2VwifULxkyMMqJZZtjp5ITgUpTjMNj5rXFq9FeBjMbCIWr\n\tTlmxFEKclTJw=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9716\"; a=\"219176620\"","E=Sophos;i=\"5.76,326,1592895600\"; d=\"scan'208\";a=\"219176620\"","E=Sophos;i=\"5.76,326,1592895600\"; d=\"scan'208\";a=\"496765230\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Tue, 18 Aug 2020 11:17:43 +0300","From":"Sakari Ailus ","To":"Jacopo Mondi ","Message-ID":"<20200818081743.GQ24582@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12033,"web_url":"https://patchwork.libcamera.org/comment/12033/","msgid":"<20200819010623.GI2360@pendragon.ideasonboard.com>","date":"2020-08-19T01:06:23","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sakari,\n\nOn Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > The V4L2 selection API are also used to access the pixel array\n> > > > properties of an image sensor, such as the size and position of active\n> > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > >\n> > > > Currently no clear definition of the different areas that compose an\n> > > > image sensor pixel array matrix is provided in the specification, and\n> > > > the actual meaning of each selection target when applied to an image\n> > > > sensor was not provided.\n> > > >\n> > > > Provide in the sub-device documentation the definition of the pixel\n> > > > matrix properties and the selection target associated to each of them.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > 1 file changed, 81 insertions(+)\n> > > >\n> > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > >\n> > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > +\n> > > > +Selection targets for image sensors properties\n> > > > +----------------------------------------------\n> > > > +\n> > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > +:ref:`selection ` ioctls.\n> > > > +\n> > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > +any of them.\n> > > > +\n> > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > +the immutable properties of the several different areas that compose the sensor\n> > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > +starting point and direction, and may differ from the physical disposition of\n> > > > +the pixel units in the pixel array matrix.\n> > > > +\n> > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > +relatively to the largest available one among the ones exposed by the\n> > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > +\n> > > > +Pixel array physical size\n> > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > +\n> > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > +does not change at run-time and cannot be modified from userspace.\n> > >\n> > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > What's the advantage of exposing them in the API, when the sensors\n> > > doesn't provide them to the rest of the pipeline ?\n> > \n> > I don't know :) I'm also bit confused on what's the purpose of\n> > NATIVE, this commit seems to suggest it was meant to replace\n> > CROP_BOUNDS, but I'm not sure about that.\n> > \n> > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > Author: Sakari Ailus \n> > Date: Thu Nov 6 16:54:33 2014 -0300\n> > \n> > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > \n> > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > interface.\n> > \n> > Sakari, do you recall if that's was the original plan ?\n> \n> That was to denote the size of the pixel array indeed. We didn't discuss\n> dark or invalid pixels at the time.\n> \n> So this was just there to tell that it's the pixel array you're cropping\n> from.\n> \n> But as long as it's API-wise compatible, I don't think anything prevents\n> re-purposing this to include other areas. The documentation (AFAIR) does\n> not say this has to be the same as the crop bounds rectangle.\n\nWhat do you think would be best ? Should we include the non-readable\npixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\nbeing strictly smaller, or drop them completely from the API, with\nV4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\nmay be that we have to allow both to support existing drivers, but we\nshould pick one of the two options and make it mandatory for new\ndrivers.","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 1ED4DBE173\n\tfor ;\n\tWed, 19 Aug 2020 01:06:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1AA861E0E;\n\tWed, 19 Aug 2020 03:06:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 451D760384\n\tfor ;\n\tWed, 19 Aug 2020 03:06:42 +0200 (CEST)","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 2235229E;\n\tWed, 19 Aug 2020 03:06:41 +0200 (CEST)"],"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=\"sCUmfEKz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597799201;\n\tbh=oBjsbuSFIha5onCavYivgY+qhxtVcYmMQpVLZi67aPQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sCUmfEKzXzg5CiKd7AHbUO1aGAE/wWQ0v/uZ771dS9wAqSuXKB3nUsIz6Djcl+KgI\n\tZ3DIDAHp5KGOIEeJP30OKIFa+aWUTg8oImAH3tWDnTYyChQ6uQo6FE397Ql1M6OkMZ\n\tsbTRlW1SqMRTFE2KCxmnGJJnj19/qat9Yx4IVuIk=","Date":"Wed, 19 Aug 2020 04:06:23 +0300","From":"Laurent Pinchart ","To":"Sakari Ailus ","Message-ID":"<20200819010623.GI2360@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200818081743.GQ24582@paasikivi.fi.intel.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12034,"web_url":"https://patchwork.libcamera.org/comment/12034/","msgid":"<20200819011558.GJ2360@pendragon.ideasonboard.com>","date":"2020-08-19T01:15:58","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 10, 2020 at 10:14:37AM +0200, Jacopo Mondi wrote:\n> On Sun, Aug 09, 2020 at 08:17:57PM +0300, Laurent Pinchart wrote:\n> > On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:\n> > > On 06/08/2020 11:50, Jacopo Mondi wrote:\n> > > > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:\n> > > >> On 05/08/2020 12:57, Jacopo Mondi wrote:\n> > > >>> The V4L2 selection API are also used to access the pixel array\n> > > >>\n> > > >> are -> is\n> > > >>\n> > > >>> properties of an image sensor, such as the size and position of active\n> > > >>> pixels and the cropped area of the pixel matrix used to produce images.\n> > > >>>\n> > > >>> Currently no clear definition of the different areas that compose an\n> > > >>> image sensor pixel array matrix is provided in the specification, and\n> > > >>> the actual meaning of each selection target when applied to an image\n> > > >>> sensor was not provided.\n> > > >>>\n> > > >>> Provide in the sub-device documentation the definition of the pixel\n> > > >>> matrix properties and the selection target associated to each of them.\n> > > >>>\n> > > >>> Signed-off-by: Jacopo Mondi \n> > > >>> ---\n> > > >>> .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > >>> 1 file changed, 81 insertions(+)\n> > > >>>\n> > > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > >>> index 134d2fb909fa4..c47861dff9b9b 100644\n> > > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > >>> ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > >>> the image size either up or down. :ref:`v4l2-selection-flags`\n> > > >>>\n> > > >>> +.. _v4l2-subdev-pixel-array-properties:\n> > > >>> +\n> > > >>> +Selection targets for image sensors properties\n> >\n> > Maybe \"Selection targets for image sensors\", and renaming the reference\n> > to v4l2-subdev-selections-image-sensors ? This section is about how\n> > selection rectangles are used for sensors, right ?\n> >\n> > > >>> +----------------------------------------------\n> >\n> > I'd move this further down, after \"Types of selection targets\", as that\n> > section contains generic information that applies to sensors too.\n> \n> Ack on both points.\n> \n> > > >>> +\n> > > >>> +The V4L2 selection API can be used on sub-devices that represent an image\n> > > >>> +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > >>> +:ref:`selection ` ioctls.\n> > > >>> +\n> > > >>> +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > >>> +the case they expose more, the pixel array properties can be accessed from\n> > > >>> +any of them.\n> >\n> > Is this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for\n> > instance, can be accessed from any source pad indifferently. Do we have\n> > sensor drivers that create multiple source pads in a subdev today ? If\n> > not I'd suggest dropping this, and adding it later if needed (when we'll\n> > have a better idea of how that would work).\n> \n> Yes, this was meant to cover cases which I still have not a clear idea\n> about but I suspect people might have question about looking at their\n> drivers. I'm totally fine adding it later when required.\n> \n> > I think what should be explained here, as also mentioned by Sakari, is\n> > that camera sensors can be exposed as multiple subdevs. The text below\n> > is related to the pixel array, which is always the first subdev, with\n> > one source pad and no sink pad. The other subdevs, modelling additional\n> > processing blocks in the sensor, may use the selection API, but that's\n> > out of scope for this patch.\n> >\n> > As we'll also need to document how other subdevs use the selection API,\n> > as well as how sensors are usually handled, would it make sense to move\n> > this to a separate file ? Sakari has proposed in [1] to create a new\n> > Documentation/driver-api/media/camera-sensor.rst file. It would make\n> > sense to centralize all sensor information there. This doesn't mean this\n> > series should depend on Sakari's patch, we can handle merge conflicts\n> > depending on what gets merged first.\n> \n> I totally missed that one but I equally totally welcome that change. I\n> would be happy to rebase this work on top of Sakari's patch which I\n> will soon give a read to.\n> \n> >\n> > [1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/\n> >\n> > > >>> +\n> > > >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > >>\n> > > >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE\n> > > >>\n> > > >> (same mistake is made elsewhere).\n> > > >\n> > > > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was\n> > > > the right name\n> > > >\n> > > >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > >>> +the immutable properties of the several different areas that compose the sensor\n> > > >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> >\n> > V4L2_TGT_CROP isn't immutable, is it ?\n> >\n> \n> Right, I noticed that, but didn't want to be too heavy with createing\n> a special section for CROP. But you're right, it's not immutable so it\n> should not be mentioned here.\n> \n> > > >>> +units. The logical disposition of pixels is defined by the sensor read-out\n> > > >>> +starting point and direction, and may differ from the physical disposition of\n> > > >>> +the pixel units in the pixel array matrix.\n> > > >>> +\n> > > >>> +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > >>> +largest being the one that describes the pixel matrix physical size. This\n> > > >>> +defines a hierarchical positional system, where each rectangle is defined\n> > > >>> +relatively to the largest available one among the ones exposed by the\n> > > >>> +sub-device driver. Each selection target and the associated pixel array portion\n> > > >>> +it represents are below presented in order from the largest to the smallest one.\n> >\n> > I find this quite confusing. As Hans suggested, I think each target\n> > should define its boundaries. I'd drop this paragraph completely, as you\n> > already explain below that all rectangles are defined relatively to\n> > V4L2_SEL_TGT_NATIVE_SIZE.\n> \n> Ack.\n> \n> > > >>> +\n> > > >>> +Pixel array physical size\n> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > >>> +\n> > > >>> +The image sensor chip is composed by a number of physical pixels, not all of\n> > > >>> +them readable by the application processor. Invalid or unreadable lines might\n> > > >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > >>> +they might be tagged with an invalid data type (DT) so that the receiver\n> > > >>> +automatically discard them.\n> >\n> > \"might\" is a bit weak for unreadable lines, there's no way they can be\n> > transmitted if they can't be read :-)\n> \n> This paragraph reflects my confusion on the subject. My interpretation\n> is that for CSI-2 sensors, you cannot crop a black pixels area and\n> capture just them. At the contrary, the black pixels are sent on the\n> bus (maybe that can be optionally enabled/disabled) tagged with a\n> special DT and interleaved with image data and you have to instruct\n> your receiver to discard or accept that DT, and have black pixels\n> captured to a separate memory area, or at buffer end (that depends on\n> the receiver's architecture I guess).\n\nThis is all sensor specific I'm afraid :-( It's entirely conceivable\nthat a sensor would implement a single crop rectangle that can span the\nblack pixels, while another sensor could restrict cropping to the\neffective area, and enable/disable the black pixels output through a\nseparate configuration. The black pixels lines will however very likely\nbe cropped horizontally the same way as the visible pixels, as\nvariable-length lines isn't widely supported on the receiver side, but\nsome sensors could implement more options.\n\n> At the same time, I won't be\n> surprised if some sensor's allow you to explicitly crop on black\n> pixels areas and only put them on the bus. Knowing how the SMIA++\n> standard handles that part might help establishing an expected\n> behaviour (I really, really, wish we had as a community any leverage\n> to influence sensor manufacturer towards a standardized behaviour, it\n> would be time for the industry to do so. ).\n\nMIPI CCS ? Still, not all vendors will implement that, sometimes for\ngood reasons, mostly probably just \"because\".\n\n> If that's correct, I wonder how would that possibly work with parallel\n> sensors, where you cannot tag data on the bus. I there assume you have\n> to explicitly select the black region to capture.\n\nFrom a sensor point of view it makes no difference, it's only from a\nreceiver point of view that the situation may get more complex as you\ncan't filter on the data type.\n\n> I there tried to, confusingly, express that different behaviour and\n> stay as much as possible generic not to rule out any existing case.\n> \n> > One way to generalize this a bit would be to explain, after the first\n> > sentence, that not all pixels may be read by the sensor, that among the\n> > pixels that are read invalid ones may not be transmitted on the bus, and\n> > that among transmitted pixels not all of them may be possible to capture\n> > on the receiver side. For instance, invalid lines may be transmitted as\n> > part of the vertical blanking on parallel buses, or tagged as blanking\n> > data or null data on CSI-2 buses. Most receivers are not able to capture\n> > either.\n> >\n> > (On a side note, strictly speaking, a CSI-2 receiver that would be able\n> > to capture null or blanking packets wouldn't be compliant with the CSI-2\n> > spec.)\n> >\n> > > >>> The size of the whole pixel matrix area is\n> > > >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > >>> +defined as position (0, 0). All the other selection targets are defined\n> > > >>> +relatively to this, larger, rectangle. The rectangle returned by\n> >\n> > s/, larger,/\n> >\n> > > >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > >>> +does not change at run-time and cannot be modified from userspace.\n> > > >>\n> > > >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,\n> > > >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.\n> > > >\n> > > > Yes it is! I'll add it here\n> >\n> > Should it be added below instead, where you define\n> > V4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that\n> > isn't defined yet when possible.\n> \n> I have a question for Sakari on this target, but I'll deflect it to\n> the reply to your comment on patch 1/4.\n> \n> > > >>> +\n> > > >>> +Pixel array readable area\n> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > >>> +\n> > > >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable\n> > > >>> +area of the pixel array matrix, including pixels with valid image data and pixel\n> > > >>> +used for calibration purposes, such as optical black pixels. It is not unlikely\n> >\n> > s/not unlikely/likely ? Or just \"common\".\n> >\n> > > >>> +that valid pixels and optical black pixels are surrounded by non-readable rows\n> > > >>> +and columns of pixels. Those does not concur in the definition of the\n> >\n> > s/does/do/\n> >\n> > I'm not sure \"concur\" is the right word. Did you mean \"those are not\n> > part of the V4L2_SEL_TGT_CROP_BOUNDS rectangle\" ?\n> \n> Yes, I meant they should not be counted in the definition of the BOUND\n> rectangle sizes.\n> \n> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by\n> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it\n> > > >>> +does not change at run-time and cannot be modified from userspace.\n> > > >>\n> > > >> Mention that BOUNDS is enclosed by NATIVE_SIZE.\n> > > >\n> > > > I tried to express that in the intro section with\n> > > >\n> > > > \"Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > largest being the one that describes the pixel matrix physical size.\"\n> > > >\n> > > > But I guess it's worth to express that for each target!\n> > > >\n> > > >>> +\n> > > >>> +Pixel array active area\n> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^\n> > > >>> +\n> > > >>> +The portion of the pixel array which contains valid image data is defined as the\n> > > >>> +active area of the pixel matrix. The active pixel array is is accessed by mean\n> > > >>\n> >\n> > s/is is/is/\n> >\n> > > >> mean -> means\n> > > >>\n> > > >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger\n> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame\n> > > >>> +resolution the sensor can produce and defines the dimension of the full\n> > > >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an\n> > > >>\n> > > >> BOUNDS -> DEFAULT\n> > > >\n> > > > ups\n> > > >\n> > > >>> +immutable property of the image sensor, it does not change at run-time and\n> > > >>> +cannot be modified from userspace.\n> > > >>\n> > > >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS\n> > > >>\n> > > >>> +\n> > > >>> +Analog crop rectangle\n> > > >>\n> > > >> Why analog? It's just the crop rectangle, nothing analog about it.\n> > > >\n> > > > We used the 'analogCrop' term in libcamera to differentiate the\n> > > > cropping which happens on the sensor pixel array matrix to select the\n> > > > region to process and produce image from. Sensor with an on-board\n> > > > scaler can perform other cropping steps to implement, in example digital\n> > > > zoom, so we expect to have a 'digital crop' phase as well. RAW\n> > > > sensors, in example, will only have an analogCrop rectangle.\n> > > >\n> > > > Quoting the libcamera definition of analog crop:\n> > > >\n> > > > * horizontal and vertical sizes define the portion of the pixel array which\n> > > > * is read-out and provided to the sensor's internal processing pipeline, before\n> > > > * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> > > > * take place.\n> > > >\n> > > > should I keep it or remove it ?\n> > >\n> > > It's a very confusing term. Especially since this API can also be used with analog\n> > > video capture devices (Composite/S-Video) where the video signal actually is analog.\n> > >\n> > > In the V4L2 API there is no such thing as 'analog crop', so please remove it.\n> >\n> > Jacopo is right, sensors usually perform cropping in the analog domain\n> > (by not reading out all pixels from the pixel array), and also support\n> > cropping in later stages, after binning/skipping, and after further\n> > scaling. Note that all of these crop operations are optional. Although\n> > not common, it's also not unconceivable that a sensor wouldn't support\n> > cropping at all.\n> >\n> > This being said, it only makes sense to talk about analog crop when\n> > multiple crop operations are performed, and thus in the context of the\n> > whole sensor, with multiple subdevs. If we explain this, as proposed\n> > above, and make it clear that the usage of the selection rectangles\n> > defined here applies to the pixel array only, we can drop the \"analog\"\n> > term, and just talk about cropping in the pixel array.\n> \n> It's fine as long as it removes any ambiguity.\n> \n> > > >>> +^^^^^^^^^^^^^^^^^^^^^\n> > > >>> +\n> > > >>> +The sensor driver might decide, in order to adjust the image resolution to best\n> > > >>> +match the one requested by applications, to only process a part of the active\n> > > >>> +pixel array matrix.\n> >\n> > I don't think that's the right approach. With MC-based devices, the\n> > philosophy is to expose all configuration parameters to userspace. It's\n> > not about sensor drivers making decisions, but about userspace deciding\n> > to crop at the pixel array level.\n> >\n> > This being said, I'm aware the decision is made by drivers when they're\n> > mode-based. Please see below for that.\n> \n> Correct, and I should now be used enough to the 'userspace drives'\n> approach to remember about documenting it :)\n> \n> > > >>> The selected area is read-out and processed by the image\n> > > >>> +sensor on-board ISP in order to produce images of the desired size and\n> > > >>> +resolution while possible maintaing the largest possible field-of-view. The\n> > > >>\n> > > >> maintaing -> maintaining\n> > > >>\n> > > >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'\n> > > >> entirely. It doesn't make much sense.\n> > > >\n> > > > Ack\n> >\n> > In general, in this section, as we're documenting the pixel array, let's\n> > not talk about the ISP.\n> >\n> > > >>> +cropped portion of the pixel array which is used to produce images is returned\n> > > >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can\n> > > >>\n> > > >> represent -> represents\n> > > >>\n> > > >>> +change at runtime as it depends on the currently configured sensor mode and\n> > > >>> +desired image resolution. If the sub-device driver supports that, userspace\n> > > >>> +can set the analog crop rectangle to select which portion of the pixel array\n> > > >>\n> > > >> s/analog//\n> > > >>\n> > > >>> +to read out.\n> >\n> > I think it's better to focus on the best case, and document usage of\n> > crop rectangles in general first, for drivers that expose full\n> > configurability of the sensor. A separate section should then then make\n> > a note of how mode-based drivers differ, which is mostly in the\n> > V4L2_SEL_TGT_CROP target being read-only, and on the single subdev\n> > hiding all the processing steps, with the crop target thus being the\n> > result of all cropping operations, analog and digital.\n> >\n> > Sakari's patch has a bit of information about this, it may be useful to\n> > reuse it or integrate with it somehow.\n> \n> I'll try to see how the two parts can be piled one on top of the\n> other.\n> \n> > > >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.\n> > > >>\n> > > >> Make a note that CROP can also be used to obtain optical black pixels.\n> > > >\n> > > > What about:\n> > > >\n> > > > +desired image resolution. If the sub-device driver supports that, userspace\n> > > > +can set the analog crop rectangle to select which portion of the pixel array\n> > > > +to read out including, if supported, optical black pixels.\n> > >\n> > > Hmm, that's a bit awkward. How about:\n> > >\n> > > +desired image resolution. If supported by the sub-device driver, userspace\n> > > +can set the crop rectangle to select which portion of the pixel array\n> > > +to read out. This may include optical black pixels if those are part of\n> > > +V4L2_SEL_TGT_CROP_BOUNDS.\n> > >\n> > > >>> +\n> > > >>>\n> > > >>> Types of selection targets\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 50E84BD87C\n\tfor ;\n\tWed, 19 Aug 2020 01:16:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF9BD61E66;\n\tWed, 19 Aug 2020 03:16:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9B0260384\n\tfor ;\n\tWed, 19 Aug 2020 03:16:16 +0200 (CEST)","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 36FFE29E;\n\tWed, 19 Aug 2020 03:16:16 +0200 (CEST)"],"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=\"LSf/vDCc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597799776;\n\tbh=FRFB3lGY9BBJkwOsjIFDgaK/nuOzi6wqWSRm9mB+hOA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LSf/vDCcYSVK2nkX9r+uFHRACsju9r5/qOapVt9y9uMxmGq/Jk1qBiRws6lKJlKhU\n\tSH7EAVkSe6HwfqT/n9Kh93pF5/pl8pIARcxHG8gUlsmo/uzfvgHC6cjC27aO6nFat6\n\tBjvuvv94wO+LyL0S0PhUCiqxwVsY3sLh+mZSuSY4=","Date":"Wed, 19 Aug 2020 04:15:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200819011558.GJ2360@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<184f8787-ebf1-90e3-82b3-44fa66e65a84@xs4all.nl>\n\t<20200806095038.tc6mmwknqdinaeth@uno.localdomain>\n\t\n\t<20200809171757.GC5981@pendragon.ideasonboard.com>\n\t<20200810081437.nymrroxmrskapbqg@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200810081437.nymrroxmrskapbqg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tSakari Ailus ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12043,"web_url":"https://patchwork.libcamera.org/comment/12043/","msgid":"<20200819102000.GS24582@paasikivi.fi.intel.com>","date":"2020-08-19T10:20:00","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Laurent,\n\nOn Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:\n> Hi Sakari,\n> \n> On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > > The V4L2 selection API are also used to access the pixel array\n> > > > > properties of an image sensor, such as the size and position of active\n> > > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > > >\n> > > > > Currently no clear definition of the different areas that compose an\n> > > > > image sensor pixel array matrix is provided in the specification, and\n> > > > > the actual meaning of each selection target when applied to an image\n> > > > > sensor was not provided.\n> > > > >\n> > > > > Provide in the sub-device documentation the definition of the pixel\n> > > > > matrix properties and the selection target associated to each of them.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > > 1 file changed, 81 insertions(+)\n> > > > >\n> > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > > >\n> > > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > > +\n> > > > > +Selection targets for image sensors properties\n> > > > > +----------------------------------------------\n> > > > > +\n> > > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > > +:ref:`selection ` ioctls.\n> > > > > +\n> > > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > > +any of them.\n> > > > > +\n> > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > > +the immutable properties of the several different areas that compose the sensor\n> > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > > +starting point and direction, and may differ from the physical disposition of\n> > > > > +the pixel units in the pixel array matrix.\n> > > > > +\n> > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > > +relatively to the largest available one among the ones exposed by the\n> > > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > > +\n> > > > > +Pixel array physical size\n> > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > > +\n> > > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > > +does not change at run-time and cannot be modified from userspace.\n> > > >\n> > > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > > What's the advantage of exposing them in the API, when the sensors\n> > > > doesn't provide them to the rest of the pipeline ?\n> > > \n> > > I don't know :) I'm also bit confused on what's the purpose of\n> > > NATIVE, this commit seems to suggest it was meant to replace\n> > > CROP_BOUNDS, but I'm not sure about that.\n> > > \n> > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > > Author: Sakari Ailus \n> > > Date: Thu Nov 6 16:54:33 2014 -0300\n> > > \n> > > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > > \n> > > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > > interface.\n> > > \n> > > Sakari, do you recall if that's was the original plan ?\n> > \n> > That was to denote the size of the pixel array indeed. We didn't discuss\n> > dark or invalid pixels at the time.\n> > \n> > So this was just there to tell that it's the pixel array you're cropping\n> > from.\n> > \n> > But as long as it's API-wise compatible, I don't think anything prevents\n> > re-purposing this to include other areas. The documentation (AFAIR) does\n> > not say this has to be the same as the crop bounds rectangle.\n> \n> What do you think would be best ? Should we include the non-readable\n> pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\n> being strictly smaller, or drop them completely from the API, with\n> V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\n> may be that we have to allow both to support existing drivers, but we\n> should pick one of the two options and make it mandatory for new\n> drivers.\n\nThat's a very good question.\n\nWhat would be the purpose of adding pixels that cannot be read? I assume\nthey would not affect sensor timing either in that case, so there would be\nno difference whether they are there or not. The crop bounds should contain\neverything whereas for the default crop should reflect the area of the\nvisible pixels.\n\nI guess in theory the visible pixels could not be cropped by the sensor in\nanalogue cropping step, so it might be worth having a separate rectangle\nfor those, too.\n\nThere could also be sensors that read the dark pixels internally and use\nthem somehow without sending their data out. don't think we should try to\nmodel that though as it's likely entirely internal to the sensor in that\ncase.","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 79593BD87C\n\tfor ;\n\tWed, 19 Aug 2020 10:20:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A53161E0E;\n\tWed, 19 Aug 2020 12:20:10 +0200 (CEST)","from mga05.intel.com (mga05.intel.com [192.55.52.43])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2F1A60382\n\tfor ;\n\tWed, 19 Aug 2020 12:20:07 +0200 (CEST)","from orsmga002.jf.intel.com ([10.7.209.21])\n\tby fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t19 Aug 2020 03:20:05 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby orsmga002-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Aug 2020 03:20:03 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid D66DA2064F; Wed, 19 Aug 2020 13:20:00 +0300 (EEST)"],"IronPort-SDR":["Bw6Ln8iQXoKnxGLH2EwS369Jm6wFC4SuuuJd+QSf8o7ydbo2zuVLpgpmqtRmkEcabxSZ6BmTxu\n\tiCiPt0b5xl5w==","0srbmhImVGWsT/Uhkyx/gE+adq8PU56pKTJEFi3ooIIvMxMrCCo43RuawUA4/ZJ9bYJT1fO2zw\n\tyuytpeq8mcTw=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9717\"; a=\"239913210\"","E=Sophos;i=\"5.76,331,1592895600\"; d=\"scan'208\";a=\"239913210\"","E=Sophos;i=\"5.76,331,1592895600\"; d=\"scan'208\";a=\"310729878\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Wed, 19 Aug 2020 13:20:00 +0300","From":"Sakari Ailus ","To":"Laurent Pinchart ","Message-ID":"<20200819102000.GS24582@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>\n\t<20200819010623.GI2360@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200819010623.GI2360@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12045,"web_url":"https://patchwork.libcamera.org/comment/12045/","msgid":"<20200819123840.GG6049@pendragon.ideasonboard.com>","date":"2020-08-19T12:38:40","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sakari,\n\nOn Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:\n> On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:\n> > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > > > The V4L2 selection API are also used to access the pixel array\n> > > > > > properties of an image sensor, such as the size and position of active\n> > > > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > > > >\n> > > > > > Currently no clear definition of the different areas that compose an\n> > > > > > image sensor pixel array matrix is provided in the specification, and\n> > > > > > the actual meaning of each selection target when applied to an image\n> > > > > > sensor was not provided.\n> > > > > >\n> > > > > > Provide in the sub-device documentation the definition of the pixel\n> > > > > > matrix properties and the selection target associated to each of them.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > > > 1 file changed, 81 insertions(+)\n> > > > > >\n> > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > > > >\n> > > > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > > > +\n> > > > > > +Selection targets for image sensors properties\n> > > > > > +----------------------------------------------\n> > > > > > +\n> > > > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > > > +:ref:`selection ` ioctls.\n> > > > > > +\n> > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > > > +any of them.\n> > > > > > +\n> > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > > > +the immutable properties of the several different areas that compose the sensor\n> > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > > > +starting point and direction, and may differ from the physical disposition of\n> > > > > > +the pixel units in the pixel array matrix.\n> > > > > > +\n> > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > > > +relatively to the largest available one among the ones exposed by the\n> > > > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > > > +\n> > > > > > +Pixel array physical size\n> > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > > > +\n> > > > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > > > +does not change at run-time and cannot be modified from userspace.\n> > > > >\n> > > > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > > > What's the advantage of exposing them in the API, when the sensors\n> > > > > doesn't provide them to the rest of the pipeline ?\n> > > > \n> > > > I don't know :) I'm also bit confused on what's the purpose of\n> > > > NATIVE, this commit seems to suggest it was meant to replace\n> > > > CROP_BOUNDS, but I'm not sure about that.\n> > > > \n> > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > > > Author: Sakari Ailus \n> > > > Date: Thu Nov 6 16:54:33 2014 -0300\n> > > > \n> > > > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > > > \n> > > > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > > > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > > > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > > > interface.\n> > > > \n> > > > Sakari, do you recall if that's was the original plan ?\n> > > \n> > > That was to denote the size of the pixel array indeed. We didn't discuss\n> > > dark or invalid pixels at the time.\n> > > \n> > > So this was just there to tell that it's the pixel array you're cropping\n> > > from.\n> > > \n> > > But as long as it's API-wise compatible, I don't think anything prevents\n> > > re-purposing this to include other areas. The documentation (AFAIR) does\n> > > not say this has to be the same as the crop bounds rectangle.\n> > \n> > What do you think would be best ? Should we include the non-readable\n> > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\n> > being strictly smaller, or drop them completely from the API, with\n> > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\n> > may be that we have to allow both to support existing drivers, but we\n> > should pick one of the two options and make it mandatory for new\n> > drivers.\n> \n> That's a very good question.\n> \n> What would be the purpose of adding pixels that cannot be read? I assume\n> they would not affect sensor timing either in that case, so there would be\n> no difference whether they are there or not.\n\nTimings is a good point, could there be sensors that read those pixels\nbut don't send them out ? Maybe to avoid edge effects ? That would be\naccounted for in the H/V blank though, wouldn't it ?\n\n> The crop bounds should contain\n> everything whereas for the default crop should reflect the area of the\n> visible pixels.\n\nI believe there are sensors that have all pixels visible, but recommend\nnot using edge pixels as they are affected by edge effects, even if\nthose pixels can be read out and transferred. In that case\nV4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe\nV4L2_SEL_TGT_CROP_DEFAULT shouldn't ?\n\n> I guess in theory the visible pixels could not be cropped by the sensor in\n> analogue cropping step, so it might be worth having a separate rectangle\n> for those, too.\n\nI'm not sure to follow you here.\n\n> There could also be sensors that read the dark pixels internally and use\n> them somehow without sending their data out. don't think we should try to\n> model that though as it's likely entirely internal to the sensor in that\n> case.\n\nAgreed.","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 58DA8BD87C\n\tfor ;\n\tWed, 19 Aug 2020 12:39:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1E3D61E66;\n\tWed, 19 Aug 2020 14:38:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0C3760383\n\tfor ;\n\tWed, 19 Aug 2020 14:38:58 +0200 (CEST)","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 E87B529E;\n\tWed, 19 Aug 2020 14:38:57 +0200 (CEST)"],"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=\"X9t3hXMh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597840738;\n\tbh=GSxF4Qc+IzH3fH8iUKiDm14HOq/QLgPB/TjTmAa3Q/8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X9t3hXMhDhThSImNS43/UumhedD7gRRI37puchcoLGivSvbANN1btqGUeq1X+zIcE\n\tNqRnmhlDoXOYB4TDM92z8eVSb2hnD5CPP1eVOHYQQAXo/jkKwO+VzYivTLwx3XM+3h\n\tNNyQXB/fX5ONh3hzCGDuaihWOy1cD76ojqFz2UH8=","Date":"Wed, 19 Aug 2020 15:38:40 +0300","From":"Laurent Pinchart ","To":"Sakari Ailus ","Message-ID":"<20200819123840.GG6049@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>\n\t<20200819010623.GI2360@pendragon.ideasonboard.com>\n\t<20200819102000.GS24582@paasikivi.fi.intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200819102000.GS24582@paasikivi.fi.intel.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12077,"web_url":"https://patchwork.libcamera.org/comment/12077/","msgid":"<20200820151604.GZ24582@paasikivi.fi.intel.com>","date":"2020-08-20T15:16:04","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Laurent,\n\nOn Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:\n> Hi Sakari,\n> \n> On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:\n> > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > > > > The V4L2 selection API are also used to access the pixel array\n> > > > > > > properties of an image sensor, such as the size and position of active\n> > > > > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > > > > >\n> > > > > > > Currently no clear definition of the different areas that compose an\n> > > > > > > image sensor pixel array matrix is provided in the specification, and\n> > > > > > > the actual meaning of each selection target when applied to an image\n> > > > > > > sensor was not provided.\n> > > > > > >\n> > > > > > > Provide in the sub-device documentation the definition of the pixel\n> > > > > > > matrix properties and the selection target associated to each of them.\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > > > > 1 file changed, 81 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > > > > >\n> > > > > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > > > > +\n> > > > > > > +Selection targets for image sensors properties\n> > > > > > > +----------------------------------------------\n> > > > > > > +\n> > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > > > > +:ref:`selection ` ioctls.\n> > > > > > > +\n> > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > > > > +any of them.\n> > > > > > > +\n> > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > > > > +the immutable properties of the several different areas that compose the sensor\n> > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > > > > +starting point and direction, and may differ from the physical disposition of\n> > > > > > > +the pixel units in the pixel array matrix.\n> > > > > > > +\n> > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > > > > +relatively to the largest available one among the ones exposed by the\n> > > > > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > > > > +\n> > > > > > > +Pixel array physical size\n> > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > > > > +\n> > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > > > > +does not change at run-time and cannot be modified from userspace.\n> > > > > >\n> > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > > > > What's the advantage of exposing them in the API, when the sensors\n> > > > > > doesn't provide them to the rest of the pipeline ?\n> > > > > \n> > > > > I don't know :) I'm also bit confused on what's the purpose of\n> > > > > NATIVE, this commit seems to suggest it was meant to replace\n> > > > > CROP_BOUNDS, but I'm not sure about that.\n> > > > > \n> > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > > > > Author: Sakari Ailus \n> > > > > Date: Thu Nov 6 16:54:33 2014 -0300\n> > > > > \n> > > > > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > > > > \n> > > > > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > > > > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > > > > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > > > > interface.\n> > > > > \n> > > > > Sakari, do you recall if that's was the original plan ?\n> > > > \n> > > > That was to denote the size of the pixel array indeed. We didn't discuss\n> > > > dark or invalid pixels at the time.\n> > > > \n> > > > So this was just there to tell that it's the pixel array you're cropping\n> > > > from.\n> > > > \n> > > > But as long as it's API-wise compatible, I don't think anything prevents\n> > > > re-purposing this to include other areas. The documentation (AFAIR) does\n> > > > not say this has to be the same as the crop bounds rectangle.\n> > > \n> > > What do you think would be best ? Should we include the non-readable\n> > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\n> > > being strictly smaller, or drop them completely from the API, with\n> > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\n> > > may be that we have to allow both to support existing drivers, but we\n> > > should pick one of the two options and make it mandatory for new\n> > > drivers.\n> > \n> > That's a very good question.\n> > \n> > What would be the purpose of adding pixels that cannot be read? I assume\n> > they would not affect sensor timing either in that case, so there would be\n> > no difference whether they are there or not.\n> \n> Timings is a good point, could there be sensors that read those pixels\n> but don't send them out ? Maybe to avoid edge effects ? That would be\n> accounted for in the H/V blank though, wouldn't it ?\n\nI guess we could ignore it, as it takes place during what is indeed\nconsidered as blanking.\n\n> \n> > The crop bounds should contain\n> > everything whereas for the default crop should reflect the area of the\n> > visible pixels.\n> \n> I believe there are sensors that have all pixels visible, but recommend\n> not using edge pixels as they are affected by edge effects, even if\n> those pixels can be read out and transferred. In that case\n> V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe\n> V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?\n\nI guess so. But in practice I wonder if there are such implementations.\n\n> \n> > I guess in theory the visible pixels could not be cropped by the sensor in\n> > analogue cropping step, so it might be worth having a separate rectangle\n> > for those, too.\n> \n> I'm not sure to follow you here.\n\nI'm saying the sensor hardware could in theory be unable to read only the\nvisible pixels.","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 029D6BD87C\n\tfor ;\n\tThu, 20 Aug 2020 15:16:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63D8762082;\n\tThu, 20 Aug 2020 17:16:12 +0200 (CEST)","from mga03.intel.com (mga03.intel.com [134.134.136.65])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A23660381\n\tfor ;\n\tThu, 20 Aug 2020 17:16:10 +0200 (CEST)","from orsmga002.jf.intel.com ([10.7.209.21])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t20 Aug 2020 08:16:08 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby orsmga002-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Aug 2020 08:16:06 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid 32FFD203ED; Thu, 20 Aug 2020 18:16:04 +0300 (EEST)"],"IronPort-SDR":["qp4IjSepo84x49MMnPeTWB4j1IR/+I4pB6ZFajmlBZ7s4Ocnhloo7ZpZ1oeig+nEFt1U/dJV++\n\txdWaMbS4hdqg==","kDyAF9WNh6MW7wZP47ACWkp7EK9tbnreurZwp1+STsorO/1AOaQnQUjGtKoRpQ1eAJEWfbfJzL\n\ts13I2bU4ePRQ=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9718\"; a=\"155289286\"","E=Sophos;i=\"5.76,333,1592895600\"; d=\"scan'208\";a=\"155289286\"","E=Sophos;i=\"5.76,333,1592895600\"; d=\"scan'208\";a=\"311143848\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Thu, 20 Aug 2020 18:16:04 +0300","From":"Sakari Ailus ","To":"Laurent Pinchart ","Message-ID":"<20200820151604.GZ24582@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>\n\t<20200819010623.GI2360@pendragon.ideasonboard.com>\n\t<20200819102000.GS24582@paasikivi.fi.intel.com>\n\t<20200819123840.GG6049@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200819123840.GG6049@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13914,"web_url":"https://patchwork.libcamera.org/comment/13914/","msgid":"<20201126130943.GJ3905@pendragon.ideasonboard.com>","date":"2020-11-26T13:09:43","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sakari,\n\nOn Thu, Aug 20, 2020 at 06:16:04PM +0300, Sakari Ailus wrote:\n> On Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:\n> > On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:\n> > > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> > > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > > > > > The V4L2 selection API are also used to access the pixel array\n> > > > > > > > properties of an image sensor, such as the size and position of active\n> > > > > > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > > > > > >\n> > > > > > > > Currently no clear definition of the different areas that compose an\n> > > > > > > > image sensor pixel array matrix is provided in the specification, and\n> > > > > > > > the actual meaning of each selection target when applied to an image\n> > > > > > > > sensor was not provided.\n> > > > > > > >\n> > > > > > > > Provide in the sub-device documentation the definition of the pixel\n> > > > > > > > matrix properties and the selection target associated to each of them.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > ---\n> > > > > > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > > > > > 1 file changed, 81 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > > > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > > > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > > > > > >\n> > > > > > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > > > > > +\n> > > > > > > > +Selection targets for image sensors properties\n> > > > > > > > +----------------------------------------------\n> > > > > > > > +\n> > > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > > > > > +:ref:`selection ` ioctls.\n> > > > > > > > +\n> > > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > > > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > > > > > +any of them.\n> > > > > > > > +\n> > > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > > > > > +the immutable properties of the several different areas that compose the sensor\n> > > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > > > > > +starting point and direction, and may differ from the physical disposition of\n> > > > > > > > +the pixel units in the pixel array matrix.\n> > > > > > > > +\n> > > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > > > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > > > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > > > > > +relatively to the largest available one among the ones exposed by the\n> > > > > > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > > > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > > > > > +\n> > > > > > > > +Pixel array physical size\n> > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > > > > > +\n> > > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > > > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > > > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > > > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > > > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > > > > > +does not change at run-time and cannot be modified from userspace.\n> > > > > > >\n> > > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > > > > > What's the advantage of exposing them in the API, when the sensors\n> > > > > > > doesn't provide them to the rest of the pipeline ?\n> > > > > > \n> > > > > > I don't know :) I'm also bit confused on what's the purpose of\n> > > > > > NATIVE, this commit seems to suggest it was meant to replace\n> > > > > > CROP_BOUNDS, but I'm not sure about that.\n> > > > > > \n> > > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > > > > > Author: Sakari Ailus \n> > > > > > Date: Thu Nov 6 16:54:33 2014 -0300\n> > > > > > \n> > > > > > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > > > > > \n> > > > > > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > > > > > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > > > > > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > > > > > interface.\n> > > > > > \n> > > > > > Sakari, do you recall if that's was the original plan ?\n> > > > > \n> > > > > That was to denote the size of the pixel array indeed. We didn't discuss\n> > > > > dark or invalid pixels at the time.\n> > > > > \n> > > > > So this was just there to tell that it's the pixel array you're cropping\n> > > > > from.\n> > > > > \n> > > > > But as long as it's API-wise compatible, I don't think anything prevents\n> > > > > re-purposing this to include other areas. The documentation (AFAIR) does\n> > > > > not say this has to be the same as the crop bounds rectangle.\n> > > > \n> > > > What do you think would be best ? Should we include the non-readable\n> > > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\n> > > > being strictly smaller, or drop them completely from the API, with\n> > > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\n> > > > may be that we have to allow both to support existing drivers, but we\n> > > > should pick one of the two options and make it mandatory for new\n> > > > drivers.\n> > > \n> > > That's a very good question.\n> > > \n> > > What would be the purpose of adding pixels that cannot be read? I assume\n> > > they would not affect sensor timing either in that case, so there would be\n> > > no difference whether they are there or not.\n> > \n> > Timings is a good point, could there be sensors that read those pixels\n> > but don't send them out ? Maybe to avoid edge effects ? That would be\n> > accounted for in the H/V blank though, wouldn't it ?\n> \n> I guess we could ignore it, as it takes place during what is indeed\n> considered as blanking.\n\nMakes sense.\n\n> > > The crop bounds should contain\n> > > everything whereas for the default crop should reflect the area of the\n> > > visible pixels.\n> > \n> > I believe there are sensors that have all pixels visible, but recommend\n> > not using edge pixels as they are affected by edge effects, even if\n> > those pixels can be read out and transferred. In that case\n> > V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe\n> > V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?\n> \n> I guess so. But in practice I wonder if there are such implementations.\n\nI think it's actually quite common, sensors often have visible pixels on\nthe edges that are not counted in the nominal sensor resolution, but are\nstill commonly read out and consumed by the demosaicing operation.\nIdeally we should report both the nominal active array (the pixels\nguaranteed by the manufacturer to be good), and the potentially larger\nexposed pixels array that include margins of potentially lower quality.\n\n> > > I guess in theory the visible pixels could not be cropped by the sensor in\n> > > analogue cropping step, so it might be worth having a separate rectangle\n> > > for those, too.\n> > \n> > I'm not sure to follow you here.\n> \n> I'm saying the sensor hardware could in theory be unable to read only the\n> visible pixels.","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 C4882BE08A\n\tfor ;\n\tThu, 26 Nov 2020 13:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CA1263459;\n\tThu, 26 Nov 2020 14:09:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7500D63408\n\tfor ;\n\tThu, 26 Nov 2020 14:09:52 +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 DAF60A1B;\n\tThu, 26 Nov 2020 14:09:51 +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=\"vT+gx+Sm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606396192;\n\tbh=1lXXA72vsjXNCvh2A5eOphuMlu50DDD4mtuLEOBpvec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vT+gx+Sm7gDYL/s+QGIRMo+UBkakEGsGKztsL3CaQepi6xApuBoivGX+K41ZkVyvr\n\tvb8rxSChuGC6w/U9234q1N0vt0KvRt8jo8vYgyeMKzNW35JmZHoWffM5JKAvksVZXx\n\tade2fNVyJbNnI2JusQw6Ix08iLcncYaUWGWQbUWc=","Date":"Thu, 26 Nov 2020 15:09:43 +0200","From":"Laurent Pinchart ","To":"Sakari Ailus ","Message-ID":"<20201126130943.GJ3905@pendragon.ideasonboard.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>\n\t<20200819010623.GI2360@pendragon.ideasonboard.com>\n\t<20200819102000.GS24582@paasikivi.fi.intel.com>\n\t<20200819123840.GG6049@pendragon.ideasonboard.com>\n\t<20200820151604.GZ24582@paasikivi.fi.intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820151604.GZ24582@paasikivi.fi.intel.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13946,"web_url":"https://patchwork.libcamera.org/comment/13946/","msgid":"<20201127132221.GQ3940@paasikivi.fi.intel.com>","date":"2020-11-27T13:22:21","subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Laurent,\n\nOn Thu, Nov 26, 2020 at 03:09:43PM +0200, Laurent Pinchart wrote:\n> Hi Sakari,\n> \n> On Thu, Aug 20, 2020 at 06:16:04PM +0300, Sakari Ailus wrote:\n> > On Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:\n> > > > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:\n> > > > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:\n> > > > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:\n> > > > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:\n> > > > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:\n> > > > > > > > > The V4L2 selection API are also used to access the pixel array\n> > > > > > > > > properties of an image sensor, such as the size and position of active\n> > > > > > > > > pixels and the cropped area of the pixel matrix used to produce images.\n> > > > > > > > >\n> > > > > > > > > Currently no clear definition of the different areas that compose an\n> > > > > > > > > image sensor pixel array matrix is provided in the specification, and\n> > > > > > > > > the actual meaning of each selection target when applied to an image\n> > > > > > > > > sensor was not provided.\n> > > > > > > > >\n> > > > > > > > > Provide in the sub-device documentation the definition of the pixel\n> > > > > > > > > matrix properties and the selection target associated to each of them.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > > ---\n> > > > > > > > > .../userspace-api/media/v4l/dev-subdev.rst | 81 +++++++++++++++++++\n> > > > > > > > > 1 file changed, 81 insertions(+)\n> > > > > > > > >\n> > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644\n> > > > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst\n> > > > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.\n> > > > > > > > > ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round\n> > > > > > > > > the image size either up or down. :ref:`v4l2-selection-flags`\n> > > > > > > > >\n> > > > > > > > > +.. _v4l2-subdev-pixel-array-properties:\n> > > > > > > > > +\n> > > > > > > > > +Selection targets for image sensors properties\n> > > > > > > > > +----------------------------------------------\n> > > > > > > > > +\n> > > > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image\n> > > > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the\n> > > > > > > > > +:ref:`selection ` ioctls.\n> > > > > > > > > +\n> > > > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in\n> > > > > > > > > +the case they expose more, the pixel array properties can be accessed from\n> > > > > > > > > +any of them.\n> > > > > > > > > +\n> > > > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,\n> > > > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve\n> > > > > > > > > +the immutable properties of the several different areas that compose the sensor\n> > > > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel\n> > > > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out\n> > > > > > > > > +starting point and direction, and may differ from the physical disposition of\n> > > > > > > > > +the pixel units in the pixel array matrix.\n> > > > > > > > > +\n> > > > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most\n> > > > > > > > > +largest being the one that describes the pixel matrix physical size. This\n> > > > > > > > > +defines a hierarchical positional system, where each rectangle is defined\n> > > > > > > > > +relatively to the largest available one among the ones exposed by the\n> > > > > > > > > +sub-device driver. Each selection target and the associated pixel array portion\n> > > > > > > > > +it represents are below presented in order from the largest to the smallest one.\n> > > > > > > > > +\n> > > > > > > > > +Pixel array physical size\n> > > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^\n> > > > > > > > > +\n> > > > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of\n> > > > > > > > > +them readable by the application processor. Invalid or unreadable lines might\n> > > > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors\n> > > > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver\n> > > > > > > > > +automatically discard them. The size of the whole pixel matrix area is\n> > > > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner\n> > > > > > > > > +defined as position (0, 0). All the other selection targets are defined\n> > > > > > > > > +relatively to this, larger, rectangle. The rectangle returned by\n> > > > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it\n> > > > > > > > > +does not change at run-time and cannot be modified from userspace.\n> > > > > > > >\n> > > > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on\n> > > > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==\n> > > > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.\n> > > > > > > > What's the advantage of exposing them in the API, when the sensors\n> > > > > > > > doesn't provide them to the rest of the pipeline ?\n> > > > > > > \n> > > > > > > I don't know :) I'm also bit confused on what's the purpose of\n> > > > > > > NATIVE, this commit seems to suggest it was meant to replace\n> > > > > > > CROP_BOUNDS, but I'm not sure about that.\n> > > > > > > \n> > > > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54\n> > > > > > > Author: Sakari Ailus \n> > > > > > > Date: Thu Nov 6 16:54:33 2014 -0300\n> > > > > > > \n> > > > > > > [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE\n> > > > > > > \n> > > > > > > Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent\n> > > > > > > of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for\n> > > > > > > V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility\n> > > > > > > interface.\n> > > > > > > \n> > > > > > > Sakari, do you recall if that's was the original plan ?\n> > > > > > \n> > > > > > That was to denote the size of the pixel array indeed. We didn't discuss\n> > > > > > dark or invalid pixels at the time.\n> > > > > > \n> > > > > > So this was just there to tell that it's the pixel array you're cropping\n> > > > > > from.\n> > > > > > \n> > > > > > But as long as it's API-wise compatible, I don't think anything prevents\n> > > > > > re-purposing this to include other areas. The documentation (AFAIR) does\n> > > > > > not say this has to be the same as the crop bounds rectangle.\n> > > > > \n> > > > > What do you think would be best ? Should we include the non-readable\n> > > > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then\n> > > > > being strictly smaller, or drop them completely from the API, with\n> > > > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It\n> > > > > may be that we have to allow both to support existing drivers, but we\n> > > > > should pick one of the two options and make it mandatory for new\n> > > > > drivers.\n> > > > \n> > > > That's a very good question.\n> > > > \n> > > > What would be the purpose of adding pixels that cannot be read? I assume\n> > > > they would not affect sensor timing either in that case, so there would be\n> > > > no difference whether they are there or not.\n> > > \n> > > Timings is a good point, could there be sensors that read those pixels\n> > > but don't send them out ? Maybe to avoid edge effects ? That would be\n> > > accounted for in the H/V blank though, wouldn't it ?\n> > \n> > I guess we could ignore it, as it takes place during what is indeed\n> > considered as blanking.\n> \n> Makes sense.\n> \n> > > > The crop bounds should contain\n> > > > everything whereas for the default crop should reflect the area of the\n> > > > visible pixels.\n> > > \n> > > I believe there are sensors that have all pixels visible, but recommend\n> > > not using edge pixels as they are affected by edge effects, even if\n> > > those pixels can be read out and transferred. In that case\n> > > V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe\n> > > V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?\n> > \n> > I guess so. But in practice I wonder if there are such implementations.\n> \n> I think it's actually quite common, sensors often have visible pixels on\n> the edges that are not counted in the nominal sensor resolution, but are\n> still commonly read out and consumed by the demosaicing operation.\n> Ideally we should report both the nominal active array (the pixels\n> guaranteed by the manufacturer to be good), and the potentially larger\n> exposed pixels array that include margins of potentially lower quality.\n\nYes, I think so, too.\n\nBut I do also think we'll need a new target for the purpose; this is really\nabout telling the pixels inside the area are of good quality, and it's\nunrelated to cropping.\n\nI wonder what to call it though. V4L2_SEL_TGT_PIXEL_PRETTY? :-)\n\n> \n> > > > I guess in theory the visible pixels could not be cropped by the sensor in\n> > > > analogue cropping step, so it might be worth having a separate rectangle\n> > > > for those, too.\n> > > \n> > > I'm not sure to follow you here.\n> > \n> > I'm saying the sensor hardware could in theory be unable to read only the\n> > visible pixels.\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 707A5BE176\n\tfor ;\n\tFri, 27 Nov 2020 13:22:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 064D16346E;\n\tFri, 27 Nov 2020 14:22:31 +0100 (CET)","from mga02.intel.com (mga02.intel.com [134.134.136.20])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55B55632EE\n\tfor ;\n\tFri, 27 Nov 2020 14:22:29 +0100 (CET)","from orsmga004.jf.intel.com ([10.7.209.38])\n\tby orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t27 Nov 2020 05:22:25 -0800","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby orsmga004-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Nov 2020 05:22:23 -0800","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid D27FD205FC; Fri, 27 Nov 2020 15:22:21 +0200 (EET)"],"IronPort-SDR":["ZB+x7Ry3Plw9o6AA60E6zGwk5yP94DYvWaOwBAAY0Pbqtajbd4vDoxSlS/09qxcrOqHE2yOr44\n\tN76l1ecgXjZg==","IfOO58YvmdW3zeiMB0gXjeDZJl5yHd52qNQB0FjU2MvYmO1/pCkFYENj7nhhr5d+cz359ey6Cw\n\t3tMgANGxZpJA=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9817\"; a=\"159440303\"","E=Sophos;i=\"5.78,374,1599548400\"; d=\"scan'208\";a=\"159440303\"","E=Sophos;i=\"5.78,374,1599548400\"; d=\"scan'208\";a=\"479679569\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Fri, 27 Nov 2020 15:22:21 +0200","From":"Sakari Ailus ","To":"Laurent Pinchart ","Message-ID":"<20201127132221.GQ3940@paasikivi.fi.intel.com>","References":"<20200805105721.15445-1-jacopo@jmondi.org>\n\t<20200805105721.15445-2-jacopo@jmondi.org>\n\t<20200809175821.GF5981@pendragon.ideasonboard.com>\n\t<20200810081757.zeeqiigrlfpxppxs@uno.localdomain>\n\t<20200818081743.GQ24582@paasikivi.fi.intel.com>\n\t<20200819010623.GI2360@pendragon.ideasonboard.com>\n\t<20200819102000.GS24582@paasikivi.fi.intel.com>\n\t<20200819123840.GG6049@pendragon.ideasonboard.com>\n\t<20200820151604.GZ24582@paasikivi.fi.intel.com>\n\t<20201126130943.GJ3905@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126130943.GJ3905@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array\n\tproperties","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,\n\tSowjanya Komatineni ,\n\tRicardo Ribalda Delgado ,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]