[{"id":2747,"web_url":"https://patchwork.libcamera.org/comment/2747/","msgid":"<20191003184741.GC4737@pendragon.ideasonboard.com>","date":"2019-10-03T18:47:41","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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 Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:\n> Add support to the dummy VIMC IPA for communication with the IPA\n> interface test unit.\n> \n> Use the test channel, if available, to send a confirmation code when\n> the init() operation is called.\n\nI would move this patch before 4/5, to avoid introducing a test that\nwould fail in 4/5.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 54 insertions(+)\n> \n> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> index abc06e7f5fd5..781f5796b082 100644\n> --- a/src/ipa/ipa_vimc.cpp\n> +++ b/src/ipa/ipa_vimc.cpp\n> @@ -7,20 +7,74 @@\n> \n> #include \n> \n\nNo need for a blank line (and iostream should then go after fcntl.h).\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> #include \n> #include \n> \n> namespace libcamera {\n> \n> +/* Keep these in sync with the IPA Interface test unit. */\n> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n\nI wonder if the path to the FIFO should be passed to the IPA module\nthrough an environment variable.\n\n> +#define IPA_INIT_CODE 0x01\n\nHow about moving this to include/ipa/vimc.h ?\n\n> +\n> class IPAVimc : public IPAInterface\n> {\n> public:\n> +\tIPAVimc();\n> +\t~IPAVimc();\n> +\n> \tint init();\n> +\n> +private:\n> +\tunsigned int fd_;\n\nfds are int, not unsigned int.\n\n> };\n> \n> +IPAVimc::IPAVimc()\n> +\t: fd_(0)\n\n0 is a valid value, it should be initialised to -1.\n\n> +{\n> +\t/* Set up the test unit back-channel, if available. */\n> +\tstruct stat fifoStat;\n> +\tint ret = stat(vimcFifoPath, &fifoStat);\n> +\tif (ret)\n> +\t\treturn;\n> +\n> +\tret = ::open(vimcFifoPath, O_WRONLY);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr << \"Failed to open vimc IPA test fifo at: \"\n> +\t\t\t << vimcFifoPath << \": \" << strerror(errno)\n> +\t\t\t << std::endl;\n\nHow about using LOG() ?\n\n> +\t\treturn;\n> +\t}\n> +\tfd_ = ret;\n> +}\n> +\n> +IPAVimc::~IPAVimc()\n> +{\n> +\tif (fd_)\n> +\t\t::close(fd_);\n> +}\n> +\n> int IPAVimc::init()\n> {\n> \tstd::cout << \"initializing vimc IPA!\" << std::endl;\n> +\n> +\tif (!fd_)\n> +\t\treturn 0;\n> +\n> +\tint8_t data = IPA_INIT_CODE;\n> +\tint ret = ::write(fd_, &data, 1);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tstd::cerr << \"Failed to write to vimc IPA test fifo at: \"\n> +\t\t\t << vimcFifoPath << \": \" << strerror(-ret)\n> +\t\t\t << std::endl;\n> +\t\treturn ret;\n> +\t}\n\nThis will be enough for now, but to prepare for the future, should the\nwrite to FIFO be moved to a separate method that will later be called\nfrom the other IPAInterface operations ? I would then turn IPA_INIT_CODE\ninto an\n\nenum IPAMethod {\n\tIPAMethodInit,\n\t...\n};\n\nand call\n\nint IPAVimc::init()\n{\n\ttrace(IPAMethodInit);\n\n\tstd:cout << ...\n}\n\n(bikeshedding the trace method name is allowed :-)).\n\n> +\n> \treturn 0;\n> }\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 3420C60BE9\n\tfor ;\n\tThu, 3 Oct 2019 20:47:57 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.12])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E80E22E5;\n\tThu, 3 Oct 2019 20:47:55 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570128476;\n\tbh=U36wXcyopYppmVrjwDvMjnbXU5I9IKgFWd2gXZ7UzJc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bAIPQwU08Qr5T4VIEg1KJVn1ekqJyB1ofOwBLoyp1KWCRe0KB+WB4mhd7l8EK8Rvl\n\tgvDXKZMsDcsyg81Xq1gbpj5BOUYbYJgKs7sl7KYflNZh7One/ythmYNZy+90vTKX6l\n\tt4SW67KRPYSImAD/PgSKS+0tUGak32VWg4WkOtlE=","Date":"Thu, 3 Oct 2019 21:47:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191003184741.GC4737@pendragon.ideasonboard.com>","References":"<20191003152037.74617-1-jacopo@jmondi.org>\n\t<20191003152037.74617-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191003152037.74617-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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":"Thu, 03 Oct 2019 18:47:57 -0000"}},{"id":2775,"web_url":"https://patchwork.libcamera.org/comment/2775/","msgid":"<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>","date":"2019-10-04T08:31:15","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:\n> > Add support to the dummy VIMC IPA for communication with the IPA\n> > interface test unit.\n> >\n> > Use the test channel, if available, to send a confirmation code when\n> > the init() operation is called.\n>\n> I would move this patch before 4/5, to avoid introducing a test that\n> would fail in 4/5.\n>\n\nYeah, better!\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 54 insertions(+)\n> >\n> > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > index abc06e7f5fd5..781f5796b082 100644\n> > --- a/src/ipa/ipa_vimc.cpp\n> > +++ b/src/ipa/ipa_vimc.cpp\n> > @@ -7,20 +7,74 @@\n> >\n> > #include \n> >\n>\n> No need for a blank line (and iostream should then go after fcntl.h).\n>\n\nDon't we keep c++ and c headers inclusion directives separate ?\n\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > #include \n> > #include \n> >\n> > namespace libcamera {\n> >\n> > +/* Keep these in sync with the IPA Interface test unit. */\n> > +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n>\n> I wonder if the path to the FIFO should be passed to the IPA module\n> through an environment variable.\n>\n\nI thought how to avoid having to keep the two in sync, including a\nshared header\n\n> > +#define IPA_INIT_CODE 0x01\n>\n> How about moving this to include/ipa/vimc.h ?\n>\n\n...as you suggest here. It would however contain mostly definitions for\nthe testing infrastructure, and I was not sure how welcome that would\nbe. I'll go ahead with this since it seems there are no objections\n\n> > +\n> > class IPAVimc : public IPAInterface\n> > {\n> > public:\n> > +\tIPAVimc();\n> > +\t~IPAVimc();\n> > +\n> > \tint init();\n> > +\n> > +private:\n> > +\tunsigned int fd_;\n>\n> fds are int, not unsigned int.\n>\n\n? Only if they are used to store error codes. Negative file\ndescriptors ?\n\n> > };\n> >\n> > +IPAVimc::IPAVimc()\n> > +\t: fd_(0)\n>\n> 0 is a valid value, it should be initialised to -1.\n>\n\nAre we using stdin ?\n\n> > +{\n> > +\t/* Set up the test unit back-channel, if available. */\n> > +\tstruct stat fifoStat;\n> > +\tint ret = stat(vimcFifoPath, &fifoStat);\n> > +\tif (ret)\n> > +\t\treturn;\n> > +\n> > +\tret = ::open(vimcFifoPath, O_WRONLY);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr << \"Failed to open vimc IPA test fifo at: \"\n> > +\t\t\t << vimcFifoPath << \": \" << strerror(errno)\n> > +\t\t\t << std::endl;\n>\n> How about using LOG() ?\n>\n\nI saw cout/cerr where used an I thought IPA should avoid using LOG(),\nbut there are no reasons for doing so, as this IPA is linked against\nlibcamera anyhow\n\n> > +\t\treturn;\n> > +\t}\n> > +\tfd_ = ret;\n> > +}\n> > +\n> > +IPAVimc::~IPAVimc()\n> > +{\n> > +\tif (fd_)\n> > +\t\t::close(fd_);\n> > +}\n> > +\n> > int IPAVimc::init()\n> > {\n> > \tstd::cout << \"initializing vimc IPA!\" << std::endl;\n> > +\n> > +\tif (!fd_)\n> > +\t\treturn 0;\n> > +\n> > +\tint8_t data = IPA_INIT_CODE;\n> > +\tint ret = ::write(fd_, &data, 1);\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tstd::cerr << \"Failed to write to vimc IPA test fifo at: \"\n> > +\t\t\t << vimcFifoPath << \": \" << strerror(-ret)\n> > +\t\t\t << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n>\n> This will be enough for now, but to prepare for the future, should the\n> write to FIFO be moved to a separate method that will later be called\n> from the other IPAInterface operations ? I would then turn IPA_INIT_CODE\n> into an\n>\n> enum IPAMethod {\n> \tIPAMethodInit,\n> \t...\n\nCould we wait to see how the interface looks like before defining the\nenum\n\nI however agree a separate method makes sense already.\n\n> };\n>\n> and call\n>\n> int IPAVimc::init()\n> {\n> \ttrace(IPAMethodInit);\n>\n> \tstd:cout << ...\n> }\n>\n> (bikeshedding the trace method name is allowed :-)).\n>\n\ntrace is fine, or 'report', 'signal'...\n\n> > +\n> > \treturn 0;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["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 C3F7660E1F\n\tfor ;\n\tFri, 4 Oct 2019 10:29:36 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 4C203FF806;\n\tFri, 4 Oct 2019 08:29:36 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 4 Oct 2019 10:31:15 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>","References":"<20191003152037.74617-1-jacopo@jmondi.org>\n\t<20191003152037.74617-6-jacopo@jmondi.org>\n\t<20191003184741.GC4737@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mv3xxmk4oj7f6eu2\"","Content-Disposition":"inline","In-Reply-To":"<20191003184741.GC4737@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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":"Fri, 04 Oct 2019 08:29:36 -0000"}},{"id":2779,"web_url":"https://patchwork.libcamera.org/comment/2779/","msgid":"<20191004155609.GB8579@pendragon.ideasonboard.com>","date":"2019-10-04T15:56:09","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote:\n> > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:\n> >> Add support to the dummy VIMC IPA for communication with the IPA\n> >> interface test unit.\n> >>\n> >> Use the test channel, if available, to send a confirmation code when\n> >> the init() operation is called.\n> >\n> > I would move this patch before 4/5, to avoid introducing a test that\n> > would fail in 4/5.\n> \n> Yeah, better!\n> \n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++\n> >> 1 file changed, 54 insertions(+)\n> >>\n> >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> >> index abc06e7f5fd5..781f5796b082 100644\n> >> --- a/src/ipa/ipa_vimc.cpp\n> >> +++ b/src/ipa/ipa_vimc.cpp\n> >> @@ -7,20 +7,74 @@\n> >>\n> >> #include \n> >>\n> >\n> > No need for a blank line (and iostream should then go after fcntl.h).\n> \n> Don't we keep c++ and c headers inclusion directives separate ?\n\nWe do in a few files, and mix them in other files. I think we should\npick one. I have so far favoured mixing them, but if we want to separate\nthem that's OK with me too, as long as we're consistent.\n\nhttps://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes\ndocuments splitting the headers. Is that your preference too ? If so,\ncould you submit a patch to fix it library-wide ?\n\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> #include \n> >> #include \n> >>\n> >> namespace libcamera {\n> >>\n> >> +/* Keep these in sync with the IPA Interface test unit. */\n> >> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> >\n> > I wonder if the path to the FIFO should be passed to the IPA module\n> > through an environment variable.\n> \n> I thought how to avoid having to keep the two in sync, including a\n> shared header\n\nPassing it through an environment variable would also allow us to make\nthe name dynamic, which could be useful for running multiple tests in\nparallel.\n\n> >> +#define IPA_INIT_CODE 0x01\n> >\n> > How about moving this to include/ipa/vimc.h ?\n> \n> ...as you suggest here. It would however contain mostly definitions for\n> the testing infrastructure, and I was not sure how welcome that would\n> be. I'll go ahead with this since it seems there are no objections\n\nPlease do :-)\n\n> >> +\n> >> class IPAVimc : public IPAInterface\n> >> {\n> >> public:\n> >> +\tIPAVimc();\n> >> +\t~IPAVimc();\n> >> +\n> >> \tint init();\n> >> +\n> >> +private:\n> >> +\tunsigned int fd_;\n> >\n> > fds are int, not unsigned int.\n> \n> ? Only if they are used to store error codes. Negative file\n> descriptors ?\n\nNegative means invalid, which is useful to be able to store. The C\nlibrary functions that deal with fds use signed values, we should do\nhere too.\n\n> >> };\n> >>\n> >> +IPAVimc::IPAVimc()\n> >> +\t: fd_(0)\n> >\n> > 0 is a valid value, it should be initialised to -1.\n> \n> Are we using stdin ?\n\nWhether 0 will end up being a possible case in practice depends on\nmultiple factors. When the IPA is isolated stdin is closed for instance.\nLet's be safe.\n\n> >> +{\n> >> +\t/* Set up the test unit back-channel, if available. */\n> >> +\tstruct stat fifoStat;\n> >> +\tint ret = stat(vimcFifoPath, &fifoStat);\n> >> +\tif (ret)\n> >> +\t\treturn;\n> >> +\n> >> +\tret = ::open(vimcFifoPath, O_WRONLY);\n> >> +\tif (ret < 0) {\n> >> +\t\tstd::cerr << \"Failed to open vimc IPA test fifo at: \"\n> >> +\t\t\t << vimcFifoPath << \": \" << strerror(errno)\n> >> +\t\t\t << std::endl;\n> >\n> > How about using LOG() ?\n> \n> I saw cout/cerr where used an I thought IPA should avoid using LOG(),\n> but there are no reasons for doing so, as this IPA is linked against\n> libcamera anyhow\n\nCould you please convert the existing cout/cerr to LOG too ?\n\n> >> +\t\treturn;\n> >> +\t}\n> >> +\tfd_ = ret;\n> >> +}\n> >> +\n> >> +IPAVimc::~IPAVimc()\n> >> +{\n> >> +\tif (fd_)\n> >> +\t\t::close(fd_);\n> >> +}\n> >> +\n> >> int IPAVimc::init()\n> >> {\n> >> \tstd::cout << \"initializing vimc IPA!\" << std::endl;\n> >> +\n> >> +\tif (!fd_)\n> >> +\t\treturn 0;\n> >> +\n> >> +\tint8_t data = IPA_INIT_CODE;\n> >> +\tint ret = ::write(fd_, &data, 1);\n> >> +\tif (ret < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tstd::cerr << \"Failed to write to vimc IPA test fifo at: \"\n> >> +\t\t\t << vimcFifoPath << \": \" << strerror(-ret)\n> >> +\t\t\t << std::endl;\n> >> +\t\treturn ret;\n> >> +\t}\n> >\n> > This will be enough for now, but to prepare for the future, should the\n> > write to FIFO be moved to a separate method that will later be called\n> > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE\n> > into an\n> >\n> > enum IPAMethod {\n> > \tIPAMethodInit,\n> > \t...\n> \n> Could we wait to see how the interface looks like before defining the\n> enum\n\nWe can, but even if we have a single value for the enum now, is there a\ndrawback in using it ? It makes the API clearer as the type is then\nexplicit.\n\n> I however agree a separate method makes sense already.\n> \n> > };\n> >\n> > and call\n> >\n> > int IPAVimc::init()\n> > {\n> > \ttrace(IPAMethodInit);\n> >\n> > \tstd:cout << ...\n> > }\n> >\n> > (bikeshedding the trace method name is allowed :-)).\n> >\n> \n> trace is fine, or 'report', 'signal'...\n> \n> >> +\n> >> \treturn 0;\n> >> }\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 CF1C46101A\n\tfor ;\n\tFri, 4 Oct 2019 17:56:23 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5978592;\n\tFri, 4 Oct 2019 17:56:22 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570204583;\n\tbh=SmT0pRA1fJO40aOQKTB8O80Id4LM6MXws0PSPW/nyis=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SEHS+9uCJ/juq3IXjUvU7/+/LZn52hHfa3IAK26NplxFYUkLjPTINyBsGX9DlMJsF\n\tghkiMgiMqOdts5OKfWPvR2KM2XtOPpDhI3ssyBKiLhgQ24Hpb96rCu/cmitT6+rmIS\n\tQ2MkisxSKWivk+jWY4m0lWUjAwfgIhpB5hhY+cWk=","Date":"Fri, 4 Oct 2019 18:56:09 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004155609.GB8579@pendragon.ideasonboard.com>","References":"<20191003152037.74617-1-jacopo@jmondi.org>\n\t<20191003152037.74617-6-jacopo@jmondi.org>\n\t<20191003184741.GC4737@pendragon.ideasonboard.com>\n\t<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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":"Fri, 04 Oct 2019 15:56:24 -0000"}},{"id":2781,"web_url":"https://patchwork.libcamera.org/comment/2781/","msgid":"<20191004162051.nld3j2pmiviqpjew@uno.localdomain>","date":"2019-10-04T16:20:51","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Oct 04, 2019 at 06:56:09PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote:\n> > On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote:\n> > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:\n> > >> Add support to the dummy VIMC IPA for communication with the IPA\n> > >> interface test unit.\n> > >>\n> > >> Use the test channel, if available, to send a confirmation code when\n> > >> the init() operation is called.\n> > >\n> > > I would move this patch before 4/5, to avoid introducing a test that\n> > > would fail in 4/5.\n> >\n> > Yeah, better!\n> >\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++\n> > >> 1 file changed, 54 insertions(+)\n> > >>\n> > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > >> index abc06e7f5fd5..781f5796b082 100644\n> > >> --- a/src/ipa/ipa_vimc.cpp\n> > >> +++ b/src/ipa/ipa_vimc.cpp\n> > >> @@ -7,20 +7,74 @@\n> > >>\n> > >> #include \n> > >>\n> > >\n> > > No need for a blank line (and iostream should then go after fcntl.h).\n> >\n> > Don't we keep c++ and c headers inclusion directives separate ?\n>\n> We do in a few files, and mix them in other files. I think we should\n> pick one. I have so far favoured mixing them, but if we want to separate\n> them that's OK with me too, as long as we're consistent.\n>\n> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes\n\nThat's where my preference comes from.\n\n> documents splitting the headers. Is that your preference too ? If so,\n> could you submit a patch to fix it library-wide ?\n\nI'm not sure it's top priority at the moment, but let's keep it in\nmind. But let's pick one direction from now on, and I would stick wil\nthe cppguide\n\n>\n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +\n> > >> #include \n> > >> #include \n> > >>\n> > >> namespace libcamera {\n> > >>\n> > >> +/* Keep these in sync with the IPA Interface test unit. */\n> > >> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> > >\n> > > I wonder if the path to the FIFO should be passed to the IPA module\n> > > through an environment variable.\n> >\n> > I thought how to avoid having to keep the two in sync, including a\n> > shared header\n>\n> Passing it through an environment variable would also allow us to make\n> the name dynamic, which could be useful for running multiple tests in\n> parallel.\n>\n\nI've put it in the vimc_ipa.h header\n\n> > >> +#define IPA_INIT_CODE 0x01\n> > >\n> > > How about moving this to include/ipa/vimc.h ?\n\nwhich I have however added in src/ipa/\n\nIt's actually only used by vimc_ipa.c and the test. Is it worth\nplacing it in the public includes directory ?\n\n> >\n> > ...as you suggest here. It would however contain mostly definitions for\n> > the testing infrastructure, and I was not sure how welcome that would\n> > be. I'll go ahead with this since it seems there are no objections\n>\n> Please do :-)\n>\n> > >> +\n> > >> class IPAVimc : public IPAInterface\n> > >> {\n> > >> public:\n> > >> +\tIPAVimc();\n> > >> +\t~IPAVimc();\n> > >> +\n> > >> \tint init();\n> > >> +\n> > >> +private:\n> > >> +\tunsigned int fd_;\n> > >\n> > > fds are int, not unsigned int.\n> >\n> > ? Only if they are used to store error codes. Negative file\n> > descriptors ?\n>\n> Negative means invalid, which is useful to be able to store. The C\n> library functions that deal with fds use signed values, we should do\n> here too.\n>\n> > >> };\n> > >>\n> > >> +IPAVimc::IPAVimc()\n> > >> +\t: fd_(0)\n> > >\n> > > 0 is a valid value, it should be initialised to -1.\n> >\n> > Are we using stdin ?\n>\n> Whether 0 will end up being a possible case in practice depends on\n> multiple factors. When the IPA is isolated stdin is closed for instance.\n> Let's be safe.\n\nAgreed! Also the C library uses signed ints, I assumed they were\nunsigned.\n\n>\n> > >> +{\n> > >> +\t/* Set up the test unit back-channel, if available. */\n> > >> +\tstruct stat fifoStat;\n> > >> +\tint ret = stat(vimcFifoPath, &fifoStat);\n> > >> +\tif (ret)\n> > >> +\t\treturn;\n> > >> +\n> > >> +\tret = ::open(vimcFifoPath, O_WRONLY);\n> > >> +\tif (ret < 0) {\n> > >> +\t\tstd::cerr << \"Failed to open vimc IPA test fifo at: \"\n> > >> +\t\t\t << vimcFifoPath << \": \" << strerror(errno)\n> > >> +\t\t\t << std::endl;\n> > >\n> > > How about using LOG() ?\n> >\n> > I saw cout/cerr where used an I thought IPA should avoid using LOG(),\n> > but there are no reasons for doing so, as this IPA is linked against\n> > libcamera anyhow\n>\n> Could you please convert the existing cout/cerr to LOG too ?\n\nYup\n\n>\n> > >> +\t\treturn;\n> > >> +\t}\n> > >> +\tfd_ = ret;\n> > >> +}\n> > >> +\n> > >> +IPAVimc::~IPAVimc()\n> > >> +{\n> > >> +\tif (fd_)\n> > >> +\t\t::close(fd_);\n> > >> +}\n> > >> +\n> > >> int IPAVimc::init()\n> > >> {\n> > >> \tstd::cout << \"initializing vimc IPA!\" << std::endl;\n> > >> +\n> > >> +\tif (!fd_)\n> > >> +\t\treturn 0;\n> > >> +\n> > >> +\tint8_t data = IPA_INIT_CODE;\n> > >> +\tint ret = ::write(fd_, &data, 1);\n> > >> +\tif (ret < 0) {\n> > >> +\t\tret = -errno;\n> > >> +\t\tstd::cerr << \"Failed to write to vimc IPA test fifo at: \"\n> > >> +\t\t\t << vimcFifoPath << \": \" << strerror(-ret)\n> > >> +\t\t\t << std::endl;\n> > >> +\t\treturn ret;\n> > >> +\t}\n> > >\n> > > This will be enough for now, but to prepare for the future, should the\n> > > write to FIFO be moved to a separate method that will later be called\n> > > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE\n> > > into an\n> > >\n> > > enum IPAMethod {\n> > > \tIPAMethodInit,\n> > > \t...\n> >\n> > Could we wait to see how the interface looks like before defining the\n> > enum\n>\n> We can, but even if we have a single value for the enum now, is there a\n> drawback in using it ? It makes the API clearer as the type is then\n> explicit.\n\nYeah, I was afraid adding operations would have changed the way we\nwant to return tracing codes, but for now let's use an enum\n\n>\n> > I however agree a separate method makes sense already.\n> >\n> > > };\n> > >\n> > > and call\n> > >\n> > > int IPAVimc::init()\n> > > {\n> > > \ttrace(IPAMethodInit);\n> > >\n> > > \tstd:cout << ...\n> > > }\n> > >\n> > > (bikeshedding the trace method name is allowed :-)).\n> > >\n> >\n> > trace is fine, or 'report', 'signal'...\n> >\n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1416E6101A\n\tfor ;\n\tFri, 4 Oct 2019 18:19:08 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 8D810240009;\n\tFri, 4 Oct 2019 16:19:07 +0000 (UTC)"],"Date":"Fri, 4 Oct 2019 18:20:51 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004162051.nld3j2pmiviqpjew@uno.localdomain>","References":"<20191003152037.74617-1-jacopo@jmondi.org>\n\t<20191003152037.74617-6-jacopo@jmondi.org>\n\t<20191003184741.GC4737@pendragon.ideasonboard.com>\n\t<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>\n\t<20191004155609.GB8579@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"r52mzoy5t3bfsm3y\"","Content-Disposition":"inline","In-Reply-To":"<20191004155609.GB8579@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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":"Fri, 04 Oct 2019 16:19:08 -0000"}},{"id":2789,"web_url":"https://patchwork.libcamera.org/comment/2789/","msgid":"<20191005164345.GA11154@pendragon.ideasonboard.com>","date":"2019-10-05T16:43:45","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 04, 2019 at 06:20:51PM +0200, Jacopo Mondi wrote:\n> On Fri, Oct 04, 2019 at 06:56:09PM +0300, Laurent Pinchart wrote:\n> > On Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote:\n> > > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:\n> > > >> Add support to the dummy VIMC IPA for communication with the IPA\n> > > >> interface test unit.\n> > > >>\n> > > >> Use the test channel, if available, to send a confirmation code when\n> > > >> the init() operation is called.\n> > > >\n> > > > I would move this patch before 4/5, to avoid introducing a test that\n> > > > would fail in 4/5.\n> > >\n> > > Yeah, better!\n> > >\n> > > >> Signed-off-by: Jacopo Mondi \n> > > >> ---\n> > > >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++\n> > > >> 1 file changed, 54 insertions(+)\n> > > >>\n> > > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > > >> index abc06e7f5fd5..781f5796b082 100644\n> > > >> --- a/src/ipa/ipa_vimc.cpp\n> > > >> +++ b/src/ipa/ipa_vimc.cpp\n> > > >> @@ -7,20 +7,74 @@\n> > > >>\n> > > >> #include \n> > > >>\n> > > >\n> > > > No need for a blank line (and iostream should then go after fcntl.h).\n> > >\n> > > Don't we keep c++ and c headers inclusion directives separate ?\n> >\n> > We do in a few files, and mix them in other files. I think we should\n> > pick one. I have so far favoured mixing them, but if we want to separate\n> > them that's OK with me too, as long as we're consistent.\n> >\n> > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes\n> \n> That's where my preference comes from.\n> \n> > documents splitting the headers. Is that your preference too ? If so,\n> > could you submit a patch to fix it library-wide ?\n> \n> I'm not sure it's top priority at the moment, but let's keep it in\n> mind. But let's pick one direction from now on, and I would stick wil\n> the cppguide\n> \n> > > >> +#include \n> > > >> +#include \n> > > >> +#include \n> > > >> +#include \n> > > >> +\n> > > >> #include \n> > > >> #include \n> > > >>\n> > > >> namespace libcamera {\n> > > >>\n> > > >> +/* Keep these in sync with the IPA Interface test unit. */\n> > > >> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> > > >\n> > > > I wonder if the path to the FIFO should be passed to the IPA module\n> > > > through an environment variable.\n> > >\n> > > I thought how to avoid having to keep the two in sync, including a\n> > > shared header\n> >\n> > Passing it through an environment variable would also allow us to make\n> > the name dynamic, which could be useful for running multiple tests in\n> > parallel.\n> \n> I've put it in the vimc_ipa.h header\n> \n> > > >> +#define IPA_INIT_CODE 0x01\n> > > >\n> > > > How about moving this to include/ipa/vimc.h ?\n> \n> which I have however added in src/ipa/\n> \n> It's actually only used by vimc_ipa.c and the test. Is it worth\n> placing it in the public includes directory ?\n\nI think so. include/ipa/ is meant for headers that define the interface\nbetween an IPA and a pipeline handler. This case is slightly different,\nbut it's still an IPA interface, so I think adding it there makes sense.\nThe VIMC IPA is special anyway, and using include/ipa/ will avoid adding\nanother include directory to the search path.\n\n> > > ...as you suggest here. It would however contain mostly definitions for\n> > > the testing infrastructure, and I was not sure how welcome that would\n> > > be. I'll go ahead with this since it seems there are no objections\n> >\n> > Please do :-)\n> >\n> > > >> +\n> > > >> class IPAVimc : public IPAInterface\n> > > >> {\n> > > >> public:\n> > > >> +\tIPAVimc();\n> > > >> +\t~IPAVimc();\n> > > >> +\n> > > >> \tint init();\n> > > >> +\n> > > >> +private:\n> > > >> +\tunsigned int fd_;\n> > > >\n> > > > fds are int, not unsigned int.\n> > >\n> > > ? Only if they are used to store error codes. Negative file\n> > > descriptors ?\n> >\n> > Negative means invalid, which is useful to be able to store. The C\n> > library functions that deal with fds use signed values, we should do\n> > here too.\n> >\n> > > >> };\n> > > >>\n> > > >> +IPAVimc::IPAVimc()\n> > > >> +\t: fd_(0)\n> > > >\n> > > > 0 is a valid value, it should be initialised to -1.\n> > >\n> > > Are we using stdin ?\n> >\n> > Whether 0 will end up being a possible case in practice depends on\n> > multiple factors. When the IPA is isolated stdin is closed for instance.\n> > Let's be safe.\n> \n> Agreed! Also the C library uses signed ints, I assumed they were\n> unsigned.\n> \n> >\n> > > >> +{\n> > > >> +\t/* Set up the test unit back-channel, if available. */\n> > > >> +\tstruct stat fifoStat;\n> > > >> +\tint ret = stat(vimcFifoPath, &fifoStat);\n> > > >> +\tif (ret)\n> > > >> +\t\treturn;\n> > > >> +\n> > > >> +\tret = ::open(vimcFifoPath, O_WRONLY);\n> > > >> +\tif (ret < 0) {\n> > > >> +\t\tstd::cerr << \"Failed to open vimc IPA test fifo at: \"\n> > > >> +\t\t\t << vimcFifoPath << \": \" << strerror(errno)\n> > > >> +\t\t\t << std::endl;\n> > > >\n> > > > How about using LOG() ?\n> > >\n> > > I saw cout/cerr where used an I thought IPA should avoid using LOG(),\n> > > but there are no reasons for doing so, as this IPA is linked against\n> > > libcamera anyhow\n> >\n> > Could you please convert the existing cout/cerr to LOG too ?\n> \n> Yup\n> \n> >\n> > > >> +\t\treturn;\n> > > >> +\t}\n> > > >> +\tfd_ = ret;\n> > > >> +}\n> > > >> +\n> > > >> +IPAVimc::~IPAVimc()\n> > > >> +{\n> > > >> +\tif (fd_)\n> > > >> +\t\t::close(fd_);\n> > > >> +}\n> > > >> +\n> > > >> int IPAVimc::init()\n> > > >> {\n> > > >> \tstd::cout << \"initializing vimc IPA!\" << std::endl;\n> > > >> +\n> > > >> +\tif (!fd_)\n> > > >> +\t\treturn 0;\n> > > >> +\n> > > >> +\tint8_t data = IPA_INIT_CODE;\n> > > >> +\tint ret = ::write(fd_, &data, 1);\n> > > >> +\tif (ret < 0) {\n> > > >> +\t\tret = -errno;\n> > > >> +\t\tstd::cerr << \"Failed to write to vimc IPA test fifo at: \"\n> > > >> +\t\t\t << vimcFifoPath << \": \" << strerror(-ret)\n> > > >> +\t\t\t << std::endl;\n> > > >> +\t\treturn ret;\n> > > >> +\t}\n> > > >\n> > > > This will be enough for now, but to prepare for the future, should the\n> > > > write to FIFO be moved to a separate method that will later be called\n> > > > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE\n> > > > into an\n> > > >\n> > > > enum IPAMethod {\n> > > > \tIPAMethodInit,\n> > > > \t...\n> > >\n> > > Could we wait to see how the interface looks like before defining the\n> > > enum\n> >\n> > We can, but even if we have a single value for the enum now, is there a\n> > drawback in using it ? It makes the API clearer as the type is then\n> > explicit.\n> \n> Yeah, I was afraid adding operations would have changed the way we\n> want to return tracing codes, but for now let's use an enum\n> \n> >\n> > > I however agree a separate method makes sense already.\n> > >\n> > > > };\n> > > >\n> > > > and call\n> > > >\n> > > > int IPAVimc::init()\n> > > > {\n> > > > \ttrace(IPAMethodInit);\n> > > >\n> > > > \tstd:cout << ...\n> > > > }\n> > > >\n> > > > (bikeshedding the trace method name is allowed :-)).\n> > > >\n> > >\n> > > trace is fine, or 'report', 'signal'...\n> > >\n> > > >> +\n> > > >> \treturn 0;\n> > > >> }\n> > > >>\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 75E8960BC6\n\tfor ;\n\tSat, 5 Oct 2019 18:43:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(modemcable151.96-160-184.mc.videotron.ca [184.160.96.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 770D7DD;\n\tSat, 5 Oct 2019 18:43:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570293839;\n\tbh=3uRw7bWjMi0AX4iMapqDNyJlYNKd18i/jDfw0esIU3k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QJ3wYE110uU3EpEynlSHxoAfahLhg/KBRJ0wBbOwFH6Bs/z+xq3EBtovDlwKzDnt9\n\tCgEVMuHsS/cpzWbbMrNA0vBEisir2bMCkYChN9oHjbVcyW1Eo0hnYKuspBij0B44QD\n\tQ22rXpcGLQlccLDVDNJ+gjTUkUIJ1wEAvnTmVq70=","Date":"Sat, 5 Oct 2019 19:43:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191005164345.GA11154@pendragon.ideasonboard.com>","References":"<20191003152037.74617-1-jacopo@jmondi.org>\n\t<20191003152037.74617-6-jacopo@jmondi.org>\n\t<20191003184741.GC4737@pendragon.ideasonboard.com>\n\t<20191004083115.2sinxjhkzf6ty6tm@uno.localdomain>\n\t<20191004155609.GB8579@pendragon.ideasonboard.com>\n\t<20191004162051.nld3j2pmiviqpjew@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191004162051.nld3j2pmiviqpjew@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test\n\tback channel","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, 05 Oct 2019 16:43:59 -0000"}}]