[{"id":4507,"web_url":"https://patchwork.libcamera.org/comment/4507/","msgid":"<20200425124243.GA5869@pendragon.ideasonboard.com>","date":"2020-04-25T12:42:43","subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","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 Fri, Apr 24, 2020 at 11:52:55PM +0200, Jacopo Mondi wrote:\n> Implement get selection for the V4L2Subdevice class to support\n> retrieving three targets: the subdevice native size, the analog crop and\n> the active pixel area.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_subdevice.h | 6 ++\n> src/libcamera/v4l2_subdevice.cpp | 92 ++++++++++++++++++++++++++\n> 2 files changed, 98 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 4a04eadfb1f9..763ffadb61fb 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -46,6 +46,10 @@ public:\n> \n> \tconst MediaEntity *entity() const { return entity_; }\n> \n> +\tint getNativeSize(unsigned int pad, Size *size);\n> +\tint getActiveArea(unsigned int pad, Rectangle *rect);\n\nI think we're mixing two layers here. The V4L2 API defines selection\ntargets. The fact the crop bounds or crop default targets map to the\nfull size and active area is specific to camera sensors. I would thus\nmove these two functions to the CameraSensor class. As this will require\naccessing different selection targets, we could add a target argument to\ngetCrop(), but I think it may just be simpler to drop setCrop() and\nsetCompose() and make getSelection() and setSelection() public.\n\n> +\tint getCropRectangle(unsigned int pad, Rectangle *rect);\n\nAs the counterpart function is called setCrop(), shouldn't this be\ncalled getCrop() ?\n\n> +\n> \tint setCrop(unsigned int pad, Rectangle *rect);\n> \tint setCompose(unsigned int pad, Rectangle *rect);\n> \n> @@ -67,6 +71,8 @@ private:\n> \tstd::vector enumPadSizes(unsigned int pad,\n> \t\t\t\t\t unsigned int code);\n> \n> +\tint getSelection(unsigned int pad, unsigned int target,\n> +\t\t\t Rectangle *rect);\n> \tint setSelection(unsigned int pad, unsigned int target,\n> \t\t\t Rectangle *rect);\n> \n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 5a479a96b795..4dcf8ce48754 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -133,6 +133,68 @@ int V4L2Subdevice::open()\n> * \\return The subdevice's associated media entity.\n> */\n> \n> +/**\n> + * \\brief Get the sub-device native size\n> + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[out] size The sub-device native area size\n> + *\n> + * Retrieve the size of the sub-device native area.\n> + * If the sub-device represent an image sensor, the native area describes\n> + * the pixel array dimensions, including inactive and calibration pixels.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENOTTY The native size information is not available\n> + */\n> +int V4L2Subdevice::getNativeSize(unsigned int pad, Size *size)\n> +{\n> +\tRectangle rect = {};\n> +\tint ret = getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tsize->width = rect.width;\n> +\tsize->height = rect.height;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the active area rectangle\n> + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[out] rect The rectangle describing the sub-device active area\n> + *\n> + * Retrieve the rectangle describing the sub-device active area.\n> + * If the sub-device represent an image sensor, the active area describes\n> + * the pixel array active portion.\n> + *\n> + * The returned \\a rect 'x' and 'y' fields represent the displacement from the\n> + * native size rectangle top-left corner.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENOTTY The active areas size information is not available\n> + */\n> +int V4L2Subdevice::getActiveArea(unsigned int pad, Rectangle *rect)\n> +{\n> +\treturn getSelection(pad, V4L2_SEL_TGT_CROP_DEFAULT, rect);\n> +}\n> +\n> +/**\n> + * \\brief Get the crop rectangle\n> + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[out] rect The rectangle describing the cropped area rectangle\n> + *\n> + * Retrieve the rectangle representing the cropped area. If the sub-device\n> + * represents an image sensor, the cropped area describes the pixel array\n> + * portion from which the final image is produced from.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENOTTY The crop rectangle size information is not available\n> + */\n> +int V4L2Subdevice::getCropRectangle(unsigned int pad, Rectangle *rect)\n> +{\n> +\treturn getSelection(pad, V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> /**\n> * \\brief Set a crop rectangle on one of the V4L2 subdevice pads\n> * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> @@ -343,6 +405,36 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> \treturn sizes;\n> }\n> \n> +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> +\t\t\t\tRectangle *rect)\n> +{\n> +\tstruct v4l2_subdev_selection sel = {};\n> +\n> +\tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> +\tsel.pad = pad;\n> +\tsel.target = target;\n> +\tsel.flags = 0;\n> +\n> +\tint ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);\n> +\tif (ret < 0 && ret != -ENOTTY) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> +\t\t\t<< pad << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\tif (ret == -ENOTTY) {\n> +\t\tLOG(V4L2, Debug) << \"Selection API not available\";\n> +\t\treturn ret;\n> +\t}\n\nDo you expect valid users of this function when the subdev doesn't\nimplement the selection API ? I can imagine that a caller would check\nfor -ENOTTY and use some fallback mechanism to make this error\nnon-fatal, in which case a debug message could be better than an error\nmessage, but do we have any practical use case for that ? Is it only\nmeant to cover a transition period until sensor drivers get converted ?\n\n> +\n> +\trect->x = sel.r.left;\n> +\trect->y = sel.r.top;\n> +\trect->width = sel.r.width;\n> +\trect->height = sel.r.height;\n> +\n> +\treturn 0;\n> +}\n> +\n> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \t\t\t\tRectangle *rect)\n> {","headers":{"Return-Path":"","Received":["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 0CDAD603FB\n\tfor ;\n\tSat, 25 Apr 2020 14:42:59 +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 215D24F7;\n\tSat, 25 Apr 2020 14:42:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"N6vXrD7B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587818578;\n\tbh=RLFoAtar8WC0gn22cfgPdNHP5QuT7mXneyd2ekzUfso=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N6vXrD7B7+deVsSl5G30ZJE0sS2pXPgbU7UZACU2O7rOe+ZvqByiiVZJ0xIwdiaZR\n\tRWh31p5jZUYLpmGEO2Wkmtus7CAJqdwoEXH5kquLYIzY9+FjPJkzfdx+omenMfaSO7\n\tQgjEamWtUeYmBuqe1CVPrhp2Y9/2ee+M8Ml/i9qw=","Date":"Sat, 25 Apr 2020 15:42:43 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425124243.GA5869@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200424215304.558317-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","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","X-List-Received-Date":"Sat, 25 Apr 2020 12:42:59 -0000"}},{"id":4510,"web_url":"https://patchwork.libcamera.org/comment/4510/","msgid":"<20200425133835.zudj6zrhezqir7jd@uno.localdomain>","date":"2020-04-25T13:38:35","subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Apr 25, 2020 at 03:42:43PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 24, 2020 at 11:52:55PM +0200, Jacopo Mondi wrote:\n> > Implement get selection for the V4L2Subdevice class to support\n> > retrieving three targets: the subdevice native size, the analog crop and\n> > the active pixel area.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_subdevice.h | 6 ++\n> > src/libcamera/v4l2_subdevice.cpp | 92 ++++++++++++++++++++++++++\n> > 2 files changed, 98 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 4a04eadfb1f9..763ffadb61fb 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -46,6 +46,10 @@ public:\n> >\n> > \tconst MediaEntity *entity() const { return entity_; }\n> >\n> > +\tint getNativeSize(unsigned int pad, Size *size);\n> > +\tint getActiveArea(unsigned int pad, Rectangle *rect);\n>\n> I think we're mixing two layers here. The V4L2 API defines selection\n> targets. The fact the crop bounds or crop default targets map to the\n> full size and active area is specific to camera sensors. I would thus\n> move these two functions to the CameraSensor class. As this will require\n> accessing different selection targets, we could add a target argument to\n> getCrop(), but I think it may just be simpler to drop setCrop() and\n> setCompose() and make getSelection() and setSelection() public.\n>\n\nThat' a thin distinction imho. My prefere here was to minimize the\namount of V4L2 dependencies in CameraSensor, so that it will still use\na 'getNativeSize' if we have to support an API != V4L2.\n\nI get your point though, and as you can read from function\ndocumentation\n\"if the sub-device represents a camera sensor then...\" I agree the\nv4l2 subdev should not expose sensor-specific concepts maybe.\n\nAlthough, it might require more work later to change this. I'm fine\nchangin this, but be aware that it implies polluting the library with\nthe V4L2_SEL_TGT_* flags.\n\n\n> > +\tint getCropRectangle(unsigned int pad, Rectangle *rect);\n>\n> As the counterpart function is called setCrop(), shouldn't this be\n> called getCrop() ?\n>\n\nack\n\n> > +\n> > \tint setCrop(unsigned int pad, Rectangle *rect);\n> > \tint setCompose(unsigned int pad, Rectangle *rect);\n> >\n> > @@ -67,6 +71,8 @@ private:\n> > \tstd::vector enumPadSizes(unsigned int pad,\n> > \t\t\t\t\t unsigned int code);\n> >\n> > +\tint getSelection(unsigned int pad, unsigned int target,\n> > +\t\t\t Rectangle *rect);\n> > \tint setSelection(unsigned int pad, unsigned int target,\n> > \t\t\t Rectangle *rect);\n> >\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 5a479a96b795..4dcf8ce48754 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -133,6 +133,68 @@ int V4L2Subdevice::open()\n> > * \\return The subdevice's associated media entity.\n> > */\n> >\n> > +/**\n> > + * \\brief Get the sub-device native size\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[out] size The sub-device native area size\n> > + *\n> > + * Retrieve the size of the sub-device native area.\n> > + * If the sub-device represent an image sensor, the native area describes\n> > + * the pixel array dimensions, including inactive and calibration pixels.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENOTTY The native size information is not available\n> > + */\n> > +int V4L2Subdevice::getNativeSize(unsigned int pad, Size *size)\n> > +{\n> > +\tRectangle rect = {};\n> > +\tint ret = getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tsize->width = rect.width;\n> > +\tsize->height = rect.height;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get the active area rectangle\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[out] rect The rectangle describing the sub-device active area\n> > + *\n> > + * Retrieve the rectangle describing the sub-device active area.\n> > + * If the sub-device represent an image sensor, the active area describes\n> > + * the pixel array active portion.\n> > + *\n> > + * The returned \\a rect 'x' and 'y' fields represent the displacement from the\n> > + * native size rectangle top-left corner.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENOTTY The active areas size information is not available\n> > + */\n> > +int V4L2Subdevice::getActiveArea(unsigned int pad, Rectangle *rect)\n> > +{\n> > +\treturn getSelection(pad, V4L2_SEL_TGT_CROP_DEFAULT, rect);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get the crop rectangle\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[out] rect The rectangle describing the cropped area rectangle\n> > + *\n> > + * Retrieve the rectangle representing the cropped area. If the sub-device\n> > + * represents an image sensor, the cropped area describes the pixel array\n> > + * portion from which the final image is produced from.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENOTTY The crop rectangle size information is not available\n> > + */\n> > +int V4L2Subdevice::getCropRectangle(unsigned int pad, Rectangle *rect)\n> > +{\n> > +\treturn getSelection(pad, V4L2_SEL_TGT_CROP, rect);\n> > +}\n> > +\n> > /**\n> > * \\brief Set a crop rectangle on one of the V4L2 subdevice pads\n> > * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > @@ -343,6 +405,36 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > \treturn sizes;\n> > }\n> >\n> > +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > +\t\t\t\tRectangle *rect)\n> > +{\n> > +\tstruct v4l2_subdev_selection sel = {};\n> > +\n> > +\tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > +\tsel.pad = pad;\n> > +\tsel.target = target;\n> > +\tsel.flags = 0;\n> > +\n> > +\tint ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);\n> > +\tif (ret < 0 && ret != -ENOTTY) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > +\t\t\t<< pad << \": \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\tif (ret == -ENOTTY) {\n> > +\t\tLOG(V4L2, Debug) << \"Selection API not available\";\n> > +\t\treturn ret;\n> > +\t}\n>\n> Do you expect valid users of this function when the subdev doesn't\n> implement the selection API ? I can imagine that a caller would check\n> for -ENOTTY and use some fallback mechanism to make this error\n> non-fatal, in which case a debug message could be better than an error\n> message, but do we have any practical use case for that ? Is it only\n> meant to cover a transition period until sensor drivers get converted ?\n>\n\nI see you have found that out by yourself\n\n> > +\n> > +\trect->x = sel.r.left;\n> > +\trect->y = sel.r.top;\n> > +\trect->width = sel.r.width;\n> > +\trect->height = sel.r.height;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \t\t\t\tRectangle *rect)\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B53A603FB\n\tfor ;\n\tSat, 25 Apr 2020 15:35:27 +0200 (CEST)","from uno.localdomain\n\t(host240-55-dynamic.3-87-r.retail.telecomitalia.it [87.3.55.240])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 54FC120000A;\n\tSat, 25 Apr 2020 13:35:26 +0000 (UTC)"],"Date":"Sat, 25 Apr 2020 15:38:35 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425133835.zudj6zrhezqir7jd@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-5-jacopo@jmondi.org>\n\t<20200425124243.GA5869@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425124243.GA5869@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","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","X-List-Received-Date":"Sat, 25 Apr 2020 13:35:27 -0000"}},{"id":4515,"web_url":"https://patchwork.libcamera.org/comment/4515/","msgid":"<20200425165312.GB11157@pendragon.ideasonboard.com>","date":"2020-04-25T16:53:12","subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sat, Apr 25, 2020 at 03:38:35PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 25, 2020 at 03:42:43PM +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 24, 2020 at 11:52:55PM +0200, Jacopo Mondi wrote:\n> > > Implement get selection for the V4L2Subdevice class to support\n> > > retrieving three targets: the subdevice native size, the analog crop and\n> > > the active pixel area.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/include/v4l2_subdevice.h | 6 ++\n> > > src/libcamera/v4l2_subdevice.cpp | 92 ++++++++++++++++++++++++++\n> > > 2 files changed, 98 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > > index 4a04eadfb1f9..763ffadb61fb 100644\n> > > --- a/src/libcamera/include/v4l2_subdevice.h\n> > > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > > @@ -46,6 +46,10 @@ public:\n> > >\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > >\n> > > +\tint getNativeSize(unsigned int pad, Size *size);\n> > > +\tint getActiveArea(unsigned int pad, Rectangle *rect);\n> >\n> > I think we're mixing two layers here. The V4L2 API defines selection\n> > targets. The fact the crop bounds or crop default targets map to the\n> > full size and active area is specific to camera sensors. I would thus\n> > move these two functions to the CameraSensor class. As this will require\n> > accessing different selection targets, we could add a target argument to\n> > getCrop(), but I think it may just be simpler to drop setCrop() and\n> > setCompose() and make getSelection() and setSelection() public.\n> \n> That' a thin distinction imho. My prefere here was to minimize the\n> amount of V4L2 dependencies in CameraSensor, so that it will still use\n> a 'getNativeSize' if we have to support an API != V4L2.\n\nI think getNativeSize() should be exposed by CameraSensor, so that\npipeline handlers don't need to care how it's retrieved. Internally,\nCameraSensor can depend on V4L2 for now, and we could have a different\nimplementation later if we need to support a different API. We will\nalready have different implementations today, even just based on V4L2,\nto support camera sensors exposed through one, two or three subdevs.\n\n> I get your point though, and as you can read from function\n> documentation\n> \"if the sub-device represents a camera sensor then...\" I agree the\n> v4l2 subdev should not expose sensor-specific concepts maybe.\n> \n> Although, it might require more work later to change this. I'm fine\n> changin this, but be aware that it implies polluting the library with\n> the V4L2_SEL_TGT_* flags.\n\nI think that's fine, the users of the class use V4L2 concepts\nexplicitly, so adding V4L2_SEL_TGT_* isn't an issue in my opinion.\n\n> > > +\tint getCropRectangle(unsigned int pad, Rectangle *rect);\n> >\n> > As the counterpart function is called setCrop(), shouldn't this be\n> > called getCrop() ?\n> \n> ack\n> \n> > > +\n> > > \tint setCrop(unsigned int pad, Rectangle *rect);\n> > > \tint setCompose(unsigned int pad, Rectangle *rect);\n> > >\n> > > @@ -67,6 +71,8 @@ private:\n> > > \tstd::vector enumPadSizes(unsigned int pad,\n> > > \t\t\t\t\t unsigned int code);\n> > >\n> > > +\tint getSelection(unsigned int pad, unsigned int target,\n> > > +\t\t\t Rectangle *rect);\n> > > \tint setSelection(unsigned int pad, unsigned int target,\n> > > \t\t\t Rectangle *rect);\n> > >\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 5a479a96b795..4dcf8ce48754 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -133,6 +133,68 @@ int V4L2Subdevice::open()\n> > > * \\return The subdevice's associated media entity.\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Get the sub-device native size\n> > > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > + * \\param[out] size The sub-device native area size\n> > > + *\n> > > + * Retrieve the size of the sub-device native area.\n> > > + * If the sub-device represent an image sensor, the native area describes\n> > > + * the pixel array dimensions, including inactive and calibration pixels.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -ENOTTY The native size information is not available\n> > > + */\n> > > +int V4L2Subdevice::getNativeSize(unsigned int pad, Size *size)\n> > > +{\n> > > +\tRectangle rect = {};\n> > > +\tint ret = getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tsize->width = rect.width;\n> > > +\tsize->height = rect.height;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get the active area rectangle\n> > > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > + * \\param[out] rect The rectangle describing the sub-device active area\n> > > + *\n> > > + * Retrieve the rectangle describing the sub-device active area.\n> > > + * If the sub-device represent an image sensor, the active area describes\n> > > + * the pixel array active portion.\n> > > + *\n> > > + * The returned \\a rect 'x' and 'y' fields represent the displacement from the\n> > > + * native size rectangle top-left corner.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -ENOTTY The active areas size information is not available\n> > > + */\n> > > +int V4L2Subdevice::getActiveArea(unsigned int pad, Rectangle *rect)\n> > > +{\n> > > +\treturn getSelection(pad, V4L2_SEL_TGT_CROP_DEFAULT, rect);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get the crop rectangle\n> > > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > + * \\param[out] rect The rectangle describing the cropped area rectangle\n> > > + *\n> > > + * Retrieve the rectangle representing the cropped area. If the sub-device\n> > > + * represents an image sensor, the cropped area describes the pixel array\n> > > + * portion from which the final image is produced from.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -ENOTTY The crop rectangle size information is not available\n> > > + */\n> > > +int V4L2Subdevice::getCropRectangle(unsigned int pad, Rectangle *rect)\n> > > +{\n> > > +\treturn getSelection(pad, V4L2_SEL_TGT_CROP, rect);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Set a crop rectangle on one of the V4L2 subdevice pads\n> > > * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > @@ -343,6 +405,36 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > \treturn sizes;\n> > > }\n> > >\n> > > +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > +\t\t\t\tRectangle *rect)\n> > > +{\n> > > +\tstruct v4l2_subdev_selection sel = {};\n> > > +\n> > > +\tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > +\tsel.pad = pad;\n> > > +\tsel.target = target;\n> > > +\tsel.flags = 0;\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);\n> > > +\tif (ret < 0 && ret != -ENOTTY) {\n> > > +\t\tLOG(V4L2, Error)\n> > > +\t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > > +\t\t\t<< pad << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\tif (ret == -ENOTTY) {\n> > > +\t\tLOG(V4L2, Debug) << \"Selection API not available\";\n> > > +\t\treturn ret;\n> > > +\t}\n> >\n> > Do you expect valid users of this function when the subdev doesn't\n> > implement the selection API ? I can imagine that a caller would check\n> > for -ENOTTY and use some fallback mechanism to make this error\n> > non-fatal, in which case a debug message could be better than an error\n> > message, but do we have any practical use case for that ? Is it only\n> > meant to cover a transition period until sensor drivers get converted ?\n> \n> I see you have found that out by yourself\n> \n> > > +\n> > > +\trect->x = sel.r.left;\n> > > +\trect->y = sel.r.top;\n> > > +\trect->width = sel.r.width;\n> > > +\trect->height = sel.r.height;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > \t\t\t\tRectangle *rect)\n> > > {","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 703BC603FC\n\tfor ;\n\tSat, 25 Apr 2020 18:53:27 +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 D95984F7;\n\tSat, 25 Apr 2020 18:53:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GeQMNHQs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587833607;\n\tbh=J0ESDyhf8buWJvyKLig5odBhDEkw4mUewbVvbne4lOw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GeQMNHQsMbk8bu7/nIhi36KvMdjElmvNp2haw8cbTCnlGmnUzIQpDLUqzpLXyA+Dy\n\tN0MCHOEVpBWHplr6oaCx/fFtjWeHBM64qtWuhgYZNB5ix6NCwSubHf5w1vlrbCoPKg\n\tBMB3uSIm/XOFFhrQzgVq2p5b9Pr1AOjySX6qgEX4=","Date":"Sat, 25 Apr 2020 19:53:12 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425165312.GB11157@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-5-jacopo@jmondi.org>\n\t<20200425124243.GA5869@pendragon.ideasonboard.com>\n\t<20200425133835.zudj6zrhezqir7jd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425133835.zudj6zrhezqir7jd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice:\n\tImplement get selection","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","X-List-Received-Date":"Sat, 25 Apr 2020 16:53:27 -0000"}}]