[{"id":24982,"web_url":"https://patchwork.libcamera.org/comment/24982/","msgid":"<20220914091901.ovq4vv2yrpwytyx7@uno.localdomain>","date":"2022-09-14T09:19:01","subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Xavier,\n\nOn Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:\n> This ensures that the V4L2Device controls attributes have been enumerated\n> in its open() method in case of file descriptor duplication.\n>\n\nDo I get this right or the actual issue is the controls are not\nenumerated for M2M devices because V4L2Device::open() (and consequentially\nV4L2Device::listControls()) is never called for them ?\n\nIf that's the case, I would squash 2 and 3 and clarify this in the\ncommit message.\n\n> Signed-off-by: Xavier Roumegue \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 12 ++----------\n> 1 file changed, 2 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 955e1508..d0427fef 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)\n> {\n> \tint ret;\n>\n> -\tUniqueFD newFd = handle.dup();\n> -\tif (!newFd.isValid()) {\n> -\t\tret = -errno;\n> -\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> -\t\t\t\t << strerror(-ret);\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tret = V4L2Device::setFd(std::move(newFd));\n> +\tret = V4L2Device::open(handle);\n> \tif (ret < 0) {\n> -\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> +\t\tLOG(V4L2, Error) << \"Failed to open file handle: \"\n> \t\t\t\t << strerror(-ret);\n\nV4L2Device::open() already complains out loud in case of failure. I\nwould drop the error message here.\n\nReviewed-by: Jacopo Mondi \n\n> \t\treturn ret;\n> \t}\n> --\n> 2.37.3\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 EA0E8C0DA4\n\tfor ;\n\tWed, 14 Sep 2022 09:19:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E5D261FA4;\n\tWed, 14 Sep 2022 11:19:05 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 045406099B\n\tfor ;\n\tWed, 14 Sep 2022 11:19:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 672AF6000D;\n\tWed, 14 Sep 2022 09:19:03 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663147145;\n\tbh=/7F+8v/R+4K9fpI+hli5wx27djyQv8DmV4r/ma+NKzg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=AqSpV+HmWiY5S3zO5w3G+0Eni3/5db5A05IVHDY2ugP6Jgc1Q/ni/k3T8q4lTGAx8\n\tEhThOY2AG+wMRpvRdJpG+fdCSlYFKwnD3+mURxxsiXGDzdR2QU5hVHZtI39CkZJ6YN\n\tkvLMca+bVrGI1URs4movSwt+2lOwFmsu49aNXZSe/slL3hENiwyGzWofsMbvXy77Nc\n\ty4aGETFowEZfD0KtNKgyY4ua/uVyhbzcokxLnPTUY0elzg8sM0YrIB0a27WjPalAUM\n\tMLKop5cBcrWAGsEMiu58BcNisnRBYzcRAC3Gy2V2UqLjmnei8b8ntbpeJdI2P4ip/W\n\tmrHl1WyJ0bwfg==","Date":"Wed, 14 Sep 2022 11:19:01 +0200","To":"Xavier Roumegue ","Message-ID":"<20220914091901.ovq4vv2yrpwytyx7@uno.localdomain>","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-4-xavier.roumegue@oss.nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908184850.1874303-4-xavier.roumegue@oss.nxp.com>","Subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25240,"web_url":"https://patchwork.libcamera.org/comment/25240/","msgid":"","date":"2022-10-03T14:14:10","subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Sep 14, 2022 at 11:19:01AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hello Xavier,\n> \n> On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:\n> > This ensures that the V4L2Device controls attributes have been enumerated\n> > in its open() method in case of file descriptor duplication.\n> \n> Do I get this right or the actual issue is the controls are not\n> enumerated for M2M devices because V4L2Device::open() (and consequentially\n> V4L2Device::listControls()) is never called for them ?\n> \n> If that's the case, I would squash 2 and 3 and clarify this in the\n> commit message.\n\nI agree. And as setFd() is now used internally only, I would turn it\nfrom a protected to a private function.\n\nI think I would prefer keeping the file handle duplication in\nV4L2VideoDevice::open() though. I'll propose a patch.\n\n> > Signed-off-by: Xavier Roumegue \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 12 ++----------\n> > 1 file changed, 2 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 955e1508..d0427fef 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)\n> > {\n> > \tint ret;\n> >\n> > -\tUniqueFD newFd = handle.dup();\n> > -\tif (!newFd.isValid()) {\n> > -\t\tret = -errno;\n> > -\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> > -\t\t\t\t << strerror(-ret);\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tret = V4L2Device::setFd(std::move(newFd));\n> > +\tret = V4L2Device::open(handle);\n> > \tif (ret < 0) {\n> > -\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> > +\t\tLOG(V4L2, Error) << \"Failed to open file handle: \"\n> > \t\t\t\t << strerror(-ret);\n> \n> V4L2Device::open() already complains out loud in case of failure. I\n> would drop the error message here.\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > \t\treturn ret;\n> > \t}","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 48F85BD16B\n\tfor ;\n\tMon, 3 Oct 2022 14:14:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC75D601BE;\n\tMon, 3 Oct 2022 16:14:15 +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 7CFC3600E9\n\tfor ;\n\tMon, 3 Oct 2022 16:14:13 +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 CD166440;\n\tMon, 3 Oct 2022 16:14:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664806455;\n\tbh=2fwPeDYr4dk99NEhMc1WZEnYA43n9anEXRfdlMrVzCw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Co/U19pjg1AEZtt2Epf0Msv7XsGrGFH7/2b2abBYfhQBEZ7EUaxnnyQu5yfoQtdVL\n\t3LkzdvqKaIaBT7+O5x3GC+iTHDpOG50kPQjo48GXWmbl7DbK8thI9rn2RphkcfFiWL\n\tGEIwpd2o/rdt9sxlkN/sstS7O7CThEL1gB5FNY7ZyxykrN2wqjU+LniLEIh52DrlJZ\n\tZLq5oGDDGcd9BEG4+YHdU/Skdb03/VqyG3TO69KxIPoL1g1xw+yf1crly3K7BIbnYv\n\tROBzJPnbGsHj0qng4Vr4Ol7J/AV5LYI83k8TRFC8wKkLY27Tzvvdm45NYifS39r6LB\n\tGwwHv7zmdwGwQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664806453;\n\tbh=2fwPeDYr4dk99NEhMc1WZEnYA43n9anEXRfdlMrVzCw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pem77sG1ha+Ma5RP97KsNKoZhs3bTD2pwDz3vXkw7LcrD/0NraqRS7xz+XuYCPfur\n\tXQhwg46jKddWl0h7Pu3LiMSroZqYf4s2onzWM2ghM6vlpuKqaIcs1M5tRMJ6NoOciB\n\t8MVQQEDwxtKuIRJPSi5TdcsjgCnoy4i1iS5pUhZc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pem77sG1\"; dkim-atps=neutral","Date":"Mon, 3 Oct 2022 17:14:10 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-4-xavier.roumegue@oss.nxp.com>\n\t<20220914091901.ovq4vv2yrpwytyx7@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220914091901.ovq4vv2yrpwytyx7@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25242,"web_url":"https://patchwork.libcamera.org/comment/25242/","msgid":"","date":"2022-10-03T19:07:47","subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 03, 2022 at 05:14:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Wed, Sep 14, 2022 at 11:19:01AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Hello Xavier,\n> > \n> > On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:\n> > > This ensures that the V4L2Device controls attributes have been enumerated\n> > > in its open() method in case of file descriptor duplication.\n> > \n> > Do I get this right or the actual issue is the controls are not\n> > enumerated for M2M devices because V4L2Device::open() (and consequentially\n> > V4L2Device::listControls()) is never called for them ?\n> > \n> > If that's the case, I would squash 2 and 3 and clarify this in the\n> > commit message.\n> \n> I agree. And as setFd() is now used internally only, I would turn it\n> from a protected to a private function.\n> \n> I think I would prefer keeping the file handle duplication in\n> V4L2VideoDevice::open() though. I'll propose a patch.\n\nDone in \"[PATCH 0/2] libcamera: v4l2_device: Fix control enumeration for\nM2M devices\".\n\n> > > Signed-off-by: Xavier Roumegue \n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 12 ++----------\n> > > 1 file changed, 2 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 955e1508..d0427fef 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)\n> > > {\n> > > \tint ret;\n> > >\n> > > -\tUniqueFD newFd = handle.dup();\n> > > -\tif (!newFd.isValid()) {\n> > > -\t\tret = -errno;\n> > > -\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> > > -\t\t\t\t << strerror(-ret);\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tret = V4L2Device::setFd(std::move(newFd));\n> > > +\tret = V4L2Device::open(handle);\n> > > \tif (ret < 0) {\n> > > -\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> > > +\t\tLOG(V4L2, Error) << \"Failed to open file handle: \"\n> > > \t\t\t\t << strerror(-ret);\n> > \n> > V4L2Device::open() already complains out loud in case of failure. I\n> > would drop the error message here.\n> > \n> > Reviewed-by: Jacopo Mondi \n> > \n> > > \t\treturn ret;\n> > > \t}","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B113C0DA4\n\tfor ;\n\tMon, 3 Oct 2022 19:07:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1AE7601C8;\n\tMon, 3 Oct 2022 21:07:51 +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 40417600E9\n\tfor ;\n\tMon, 3 Oct 2022 21:07:50 +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 C17A7440;\n\tMon, 3 Oct 2022 21:07:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664824072;\n\tbh=FSW9GB2S+Ji7sUP9CVgUUJ/fNAXH9FFzWAxXhuaNiNk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=VV2KrpazPrg8XKdis/mWaDUYN48HgOkbGjzbCtDePIenIgc5xJ3Y7nGRJJzDxwGw9\n\taHCiTIuuRpm2goyCVJAH01OMCOcONKj0VYBPmwWDPwN8CgkLDT/ctCr9nrnhhzL7Ej\n\t/Tnv9X6e5fVsxgxRE64fP1QJ1vCHsfTFcoqQhJV/YbAeYyVC/W9Y9AoIqo0FPNCDvP\n\tIZxwl8rXJARDHszL7S2eiYezUo2WzphHoKk27Gmjj6ix5wZc8XddJtgY50N//kg2+e\n\tHzSfVpcCYqawV/pb6X08tRXDxkJ78yzDKJJlEiOuraG7Cf5iZwGpJAUq36Pd4LoOfq\n\tMnCWhtP6aUvMA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664824070;\n\tbh=FSW9GB2S+Ji7sUP9CVgUUJ/fNAXH9FFzWAxXhuaNiNk=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=GEFr5BI4hqUuWItcPc7dHEzzyDD2TvPDTTU75VZ6zOTouPsLyD5ZdeTDf9Nb31UCJ\n\tT6ydPh6mAZq2FqW/aCo1WQefo9bfEbzatfonRfqAmsWY+bChWo3WRlzSxA7j76puA7\n\t3BoxDebnkVuZWZhJebXkJe9Py44GaANTgJUmg6y4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GEFr5BI4\"; dkim-atps=neutral","Date":"Mon, 3 Oct 2022 22:07:47 +0300","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-4-xavier.roumegue@oss.nxp.com>\n\t<20220914091901.ovq4vv2yrpwytyx7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice:\n\tMove FD duplication in V4L2Device::Open","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]