[{"id":2784,"web_url":"https://patchwork.libcamera.org/comment/2784/","msgid":"<20191004210203.GC1973@pendragon.ideasonboard.com>","date":"2019-10-04T21:02:03","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:\n> Add support to the dummy VIMC IPA for tracing operation by using a FIFO\n> channel that will be used by the IPA Interface test to verify\n> communications with the IPA.\n> \n> At the moment only add support for the init() operation as it's the only\n> defined one.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++\n> src/ipa/ipa_vimc.h | 21 ++++++++++++++\n> 2 files changed, 87 insertions(+)\n> create mode 100644 src/ipa/ipa_vimc.h\n> \n> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> index abc06e7f5fd5..5fb62129fef9 100644\n> --- a/src/ipa/ipa_vimc.cpp\n> +++ b/src/ipa/ipa_vimc.cpp\n> @@ -5,22 +5,88 @@\n> * ipa_vimc.cpp - Vimc Image Processing Algorithm module\n> */\n> \n> +#include \"ipa_vimc.h\"\n> +\n> #include \n> \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> #include \n> #include \n> \n> +#include \"log.h\"\n> +\n> namespace libcamera {\n> \n> +LOG_DEFINE_CATEGORY(IPAVimc)\n> +\n> class IPAVimc : public IPAInterface\n> {\n> public:\n> +\tIPAVimc();\n> +\t~IPAVimc();\n> +\n> \tint init();\n> +\n> +private:\n> +\tvoid initTrace();\n> +\tint trace(enum IPAOperationCodes operation);\n> +\n> +\tint fd_;\n> };\n> \n> +IPAVimc::IPAVimc()\n> +\t: fd_(-1)\n> +{\n> +\tinitTrace();\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\nWhile at it could you switch this to LOG() ?\n\n> +\n> +\treturn trace(IPAOperationInit);\n\nI would move this to the first line of the function, and ignore the\nreturn value. You can't really return a value from an IPA operation as\ncalls are potentially asynchronous. I would thus make the trace()\nmethod void.\n\n> +}\n> +\n> +void IPAVimc::initTrace()\n> +{\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\tLOG(IPAVimc, Debug) << \"Failed to open vimc IPA test FIFO: \"\n> +\t\t\t\t << strerror(errno) << std::endl;\n\nNo need for std::endl with LOG().\n\n\t\tLOG(IPAVimc, Debug)\n\t\t\t<< \"Failed to open vimc IPA test FIFO: \"\n\t\t\t<< strerror(errno);\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tfd_ = ret;\n> +}\n> +\n> +int IPAVimc::trace(enum IPAOperationCodes operation)\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn 0;\n> +\n> +\tint ret = ::write(fd_, &operation, sizeof(operation));\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(IPAVimc, Debug) << \"Failed to write to vimc IPA test FIFO: \"\n> +\t\t\t\t << strerror(-ret) << std::endl;\n\nSame here.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h\n> new file mode 100644\n> index 000000000000..62e40c5a6714\n> --- /dev/null\n> +++ b/src/ipa/ipa_vimc.h\n\nShouldn't this live in include/ipa/ ?\n\n> @@ -0,0 +1,21 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_vimc.h - Vimc Image Processing Algorithm module\n> + */\n> +\n> +#ifndef __LIBCAMERA_IPA_VIMC_H__\n> +#define __LIBCAMERA_IPA_VIMC_H__\n> +\n> +namespace libcamera {\n> +\n> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n\nShould we pass this through an environment variable in order to support\nparallel running of tests ?\n\n> +enum IPAOperationCodes {\n\ns/IPAOperationCodes/IPAOperationCode/\n\n> +\tIPAOperationNone,\n> +\tIPAOperationInit,\n> +};\n> +\n> +}; /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */","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 D2DAD6165B\n\tfor ;\n\tFri, 4 Oct 2019 23:02:17 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.229.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C527F2E5;\n\tFri, 4 Oct 2019 23:02:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570222937;\n\tbh=pYzmVR+8aoRw4BaDBUkghn0X+vmL4htUny+uGxgUYDk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A4T1ifzWSvc3AXQQxiRPR7cCcDx2manmLOUVkyUbT2EFcRqnNefS36FM5gXZrQYIO\n\tA/xKeEFZd6uWvONz3Jc+6CU8RCosGXyNimVyrlyEdypM9ch410CwFConc0rIkSu7vw\n\tv9r92pWglT3QesC6WEaL30bY0jnI1C+I8Ve+IO1A=","Date":"Sat, 5 Oct 2019 00:02:03 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004210203.GC1973@pendragon.ideasonboard.com>","References":"<20191004163734.15594-1-jacopo@jmondi.org>\n\t<20191004163734.15594-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191004163734.15594-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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 21:02:18 -0000"}},{"id":2785,"web_url":"https://patchwork.libcamera.org/comment/2785/","msgid":"<20191004210255.GD1973@pendragon.ideasonboard.com>","date":"2019-10-04T21:02:55","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:\n> On Fri, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:\n> > Add support to the dummy VIMC IPA for tracing operation by using a FIFO\n> > channel that will be used by the IPA Interface test to verify\n> > communications with the IPA.\n> > \n> > At the moment only add support for the init() operation as it's the only\n> > defined one.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++\n> > src/ipa/ipa_vimc.h | 21 ++++++++++++++\n> > 2 files changed, 87 insertions(+)\n> > create mode 100644 src/ipa/ipa_vimc.h\n> > \n> > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > index abc06e7f5fd5..5fb62129fef9 100644\n> > --- a/src/ipa/ipa_vimc.cpp\n> > +++ b/src/ipa/ipa_vimc.cpp\n> > @@ -5,22 +5,88 @@\n> > * ipa_vimc.cpp - Vimc Image Processing Algorithm module\n> > */\n> > \n> > +#include \"ipa_vimc.h\"\n> > +\n> > #include \n> > \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n\nBtw, if you want to split the C and C++ headers, the C headers should\ncome first according to the style guide.\n\n> > +\n> > #include \n> > #include \n> > \n> > +#include \"log.h\"\n> > +\n> > namespace libcamera {\n> > \n> > +LOG_DEFINE_CATEGORY(IPAVimc)\n> > +\n> > class IPAVimc : public IPAInterface\n> > {\n> > public:\n> > +\tIPAVimc();\n> > +\t~IPAVimc();\n> > +\n> > \tint init();\n> > +\n> > +private:\n> > +\tvoid initTrace();\n> > +\tint trace(enum IPAOperationCodes operation);\n> > +\n> > +\tint fd_;\n> > };\n> > \n> > +IPAVimc::IPAVimc()\n> > +\t: fd_(-1)\n> > +{\n> > +\tinitTrace();\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> While at it could you switch this to LOG() ?\n> \n> > +\n> > +\treturn trace(IPAOperationInit);\n> \n> I would move this to the first line of the function, and ignore the\n> return value. You can't really return a value from an IPA operation as\n> calls are potentially asynchronous. I would thus make the trace()\n> method void.\n> \n> > +}\n> > +\n> > +void IPAVimc::initTrace()\n> > +{\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\tLOG(IPAVimc, Debug) << \"Failed to open vimc IPA test FIFO: \"\n> > +\t\t\t\t << strerror(errno) << std::endl;\n> \n> No need for std::endl with LOG().\n> \n> \t\tLOG(IPAVimc, Debug)\n> \t\t\t<< \"Failed to open vimc IPA test FIFO: \"\n> \t\t\t<< strerror(errno);\n> \n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tfd_ = ret;\n> > +}\n> > +\n> > +int IPAVimc::trace(enum IPAOperationCodes operation)\n> > +{\n> > +\tif (fd_ < 0)\n> > +\t\treturn 0;\n> > +\n> > +\tint ret = ::write(fd_, &operation, sizeof(operation));\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(IPAVimc, Debug) << \"Failed to write to vimc IPA test FIFO: \"\n> > +\t\t\t\t << strerror(-ret) << std::endl;\n> \n> Same here.\n> \n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> > \n> > diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h\n> > new file mode 100644\n> > index 000000000000..62e40c5a6714\n> > --- /dev/null\n> > +++ b/src/ipa/ipa_vimc.h\n> \n> Shouldn't this live in include/ipa/ ?\n> \n> > @@ -0,0 +1,21 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipa_vimc.h - Vimc Image Processing Algorithm module\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_IPA_VIMC_H__\n> > +#define __LIBCAMERA_IPA_VIMC_H__\n> > +\n> > +namespace libcamera {\n> > +\n> > +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> \n> Should we pass this through an environment variable in order to support\n> parallel running of tests ?\n> \n> > +enum IPAOperationCodes {\n> \n> s/IPAOperationCodes/IPAOperationCode/\n> \n> > +\tIPAOperationNone,\n> > +\tIPAOperationInit,\n> > +};\n> > +\n> > +}; /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */","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 BC09A6165B\n\tfor ;\n\tFri, 4 Oct 2019 23:03:09 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.229.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E97F62E5;\n\tFri, 4 Oct 2019 23:03:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570222989;\n\tbh=QPYDkmWA5CvCvScoBVy+wZ1yE2LK9j8zPoNjaRZN4Ro=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LDJmZv4qMsae9tNKULgoz06NBgkkPd7g9xJvr/DMe+ZFdMz7AtPirYf738rEpNzBk\n\tFtff2WpcOiaXAmBq39ldELPyuee44XKfR5Lr13XRqKoJV4Uvk7CgpJkt9qUZEAJTkX\n\t9IOsDjDrx8jeQ2+sfdQDKWww6oXc1EadAxaFBpSs=","Date":"Sat, 5 Oct 2019 00:02:55 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004210255.GD1973@pendragon.ideasonboard.com>","References":"<20191004163734.15594-1-jacopo@jmondi.org>\n\t<20191004163734.15594-6-jacopo@jmondi.org>\n\t<20191004210203.GC1973@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191004210203.GC1973@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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 21:03:09 -0000"}},{"id":2787,"web_url":"https://patchwork.libcamera.org/comment/2787/","msgid":"<20191005125423.737e3e5j5igtfvtp@uno.localdomain>","date":"2019-10-05T12:54:23","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:\n> > Add support to the dummy VIMC IPA for tracing operation by using a FIFO\n> > channel that will be used by the IPA Interface test to verify\n> > communications with the IPA.\n> >\n> > At the moment only add support for the init() operation as it's the only\n> > defined one.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++\n> > src/ipa/ipa_vimc.h | 21 ++++++++++++++\n> > 2 files changed, 87 insertions(+)\n> > create mode 100644 src/ipa/ipa_vimc.h\n> >\n> > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > index abc06e7f5fd5..5fb62129fef9 100644\n> > --- a/src/ipa/ipa_vimc.cpp\n> > +++ b/src/ipa/ipa_vimc.cpp\n> > @@ -5,22 +5,88 @@\n> > * ipa_vimc.cpp - Vimc Image Processing Algorithm module\n> > */\n> >\n> > +#include \"ipa_vimc.h\"\n> > +\n> > #include \n> >\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > #include \n> > #include \n> >\n> > +#include \"log.h\"\n> > +\n> > namespace libcamera {\n> >\n> > +LOG_DEFINE_CATEGORY(IPAVimc)\n> > +\n> > class IPAVimc : public IPAInterface\n> > {\n> > public:\n> > +\tIPAVimc();\n> > +\t~IPAVimc();\n> > +\n> > \tint init();\n> > +\n> > +private:\n> > +\tvoid initTrace();\n> > +\tint trace(enum IPAOperationCodes operation);\n> > +\n> > +\tint fd_;\n> > };\n> >\n> > +IPAVimc::IPAVimc()\n> > +\t: fd_(-1)\n> > +{\n> > +\tinitTrace();\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> While at it could you switch this to LOG() ?\n>\n> > +\n> > +\treturn trace(IPAOperationInit);\n>\n> I would move this to the first line of the function, and ignore the\n> return value. You can't really return a value from an IPA operation as\n> calls are potentially asynchronous. I would thus make the trace()\n> method void.\n>\n\nAgreed\n\n> > +}\n> > +\n> > +void IPAVimc::initTrace()\n> > +{\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\tLOG(IPAVimc, Debug) << \"Failed to open vimc IPA test FIFO: \"\n> > +\t\t\t\t << strerror(errno) << std::endl;\n>\n> No need for std::endl with LOG().\n>\n> \t\tLOG(IPAVimc, Debug)\n> \t\t\t<< \"Failed to open vimc IPA test FIFO: \"\n> \t\t\t<< strerror(errno);\n>\n\nSorry, leftover!\n\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tfd_ = ret;\n> > +}\n> > +\n> > +int IPAVimc::trace(enum IPAOperationCodes operation)\n> > +{\n> > +\tif (fd_ < 0)\n> > +\t\treturn 0;\n> > +\n> > +\tint ret = ::write(fd_, &operation, sizeof(operation));\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(IPAVimc, Debug) << \"Failed to write to vimc IPA test FIFO: \"\n> > +\t\t\t\t << strerror(-ret) << std::endl;\n>\n> Same here.\n>\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> >\n> > diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h\n> > new file mode 100644\n> > index 000000000000..62e40c5a6714\n> > --- /dev/null\n> > +++ b/src/ipa/ipa_vimc.h\n>\n> Shouldn't this live in include/ipa/ ?\n>\n\nGood question...\nI assumed include/ would contain public headers only, and this is\nspecific to an IPA module, and only used outside from src/ipa/ for the\ntest.\n\nI would keep it private and provide tests a way to include it, unless\nI'm missing a use case for sharing this with other components...\n\n> > @@ -0,0 +1,21 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipa_vimc.h - Vimc Image Processing Algorithm module\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_IPA_VIMC_H__\n> > +#define __LIBCAMERA_IPA_VIMC_H__\n> > +\n> > +namespace libcamera {\n> > +\n> > +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n>\n> Should we pass this through an environment variable in order to support\n> parallel running of tests ?\n>\n\nI assume you mean multiple tests on the IPA Interface... Do we expect\nmore? one per operation? In any case, passing it from environment\nvariable mean both the VIMC IPA and the test should access it, and if\nit's not defined the test would be skip. Or do you mean passing it\nfrom the build environment? Because in that case it would not make\neasier to have create different paths for multiple pipes to be used in\nparallled by different tests\n\n> > +enum IPAOperationCodes {\n>\n> s/IPAOperationCodes/IPAOperationCode/\n>\n\nI tend to pluralize enumeration types, but we only actually have them\nin the singular form, I'll change this.\n\nThanks\n j\n\n> > +\tIPAOperationNone,\n> > +\tIPAOperationInit,\n> > +};\n> > +\n> > +}; /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C880760E1F\n\tfor ;\n\tSat, 5 Oct 2019 14:52:42 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 447F2100007;\n\tSat, 5 Oct 2019 12:52:42 +0000 (UTC)"],"Date":"Sat, 5 Oct 2019 14:54:23 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191005125423.737e3e5j5igtfvtp@uno.localdomain>","References":"<20191004163734.15594-1-jacopo@jmondi.org>\n\t<20191004163734.15594-6-jacopo@jmondi.org>\n\t<20191004210203.GC1973@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"l7blobffyzhd6vfv\"","Content-Disposition":"inline","In-Reply-To":"<20191004210203.GC1973@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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 12:52:43 -0000"}},{"id":2790,"web_url":"https://patchwork.libcamera.org/comment/2790/","msgid":"<20191005164917.GB11154@pendragon.ideasonboard.com>","date":"2019-10-05T16:49:17","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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, Oct 05, 2019 at 02:54:23PM +0200, Jacopo Mondi wrote:\n> On Sat, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:\n> > On Fri, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:\n> >> Add support to the dummy VIMC IPA for tracing operation by using a FIFO\n> >> channel that will be used by the IPA Interface test to verify\n> >> communications with the IPA.\n> >>\n> >> At the moment only add support for the init() operation as it's the only\n> >> defined one.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++\n> >> src/ipa/ipa_vimc.h | 21 ++++++++++++++\n> >> 2 files changed, 87 insertions(+)\n> >> create mode 100644 src/ipa/ipa_vimc.h\n> >>\n> >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> >> index abc06e7f5fd5..5fb62129fef9 100644\n> >> --- a/src/ipa/ipa_vimc.cpp\n> >> +++ b/src/ipa/ipa_vimc.cpp\n> >> @@ -5,22 +5,88 @@\n> >> * ipa_vimc.cpp - Vimc Image Processing Algorithm module\n> >> */\n> >>\n> >> +#include \"ipa_vimc.h\"\n> >> +\n> >> #include \n> >>\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> #include \n> >> #include \n> >>\n> >> +#include \"log.h\"\n> >> +\n> >> namespace libcamera {\n> >>\n> >> +LOG_DEFINE_CATEGORY(IPAVimc)\n> >> +\n> >> class IPAVimc : public IPAInterface\n> >> {\n> >> public:\n> >> +\tIPAVimc();\n> >> +\t~IPAVimc();\n> >> +\n> >> \tint init();\n> >> +\n> >> +private:\n> >> +\tvoid initTrace();\n> >> +\tint trace(enum IPAOperationCodes operation);\n> >> +\n> >> +\tint fd_;\n> >> };\n> >>\n> >> +IPAVimc::IPAVimc()\n> >> +\t: fd_(-1)\n> >> +{\n> >> +\tinitTrace();\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> > While at it could you switch this to LOG() ?\n> >\n> >> +\n> >> +\treturn trace(IPAOperationInit);\n> >\n> > I would move this to the first line of the function, and ignore the\n> > return value. You can't really return a value from an IPA operation as\n> > calls are potentially asynchronous. I would thus make the trace()\n> > method void.\n> \n> Agreed\n> \n> >> +}\n> >> +\n> >> +void IPAVimc::initTrace()\n> >> +{\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\tLOG(IPAVimc, Debug) << \"Failed to open vimc IPA test FIFO: \"\n> >> +\t\t\t\t << strerror(errno) << std::endl;\n> >\n> > No need for std::endl with LOG().\n> >\n> > \t\tLOG(IPAVimc, Debug)\n> > \t\t\t<< \"Failed to open vimc IPA test FIFO: \"\n> > \t\t\t<< strerror(errno);\n> >\n> \n> Sorry, leftover!\n> \n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tfd_ = ret;\n> >> +}\n> >> +\n> >> +int IPAVimc::trace(enum IPAOperationCodes operation)\n> >> +{\n> >> +\tif (fd_ < 0)\n> >> +\t\treturn 0;\n> >> +\n> >> +\tint ret = ::write(fd_, &operation, sizeof(operation));\n> >> +\tif (ret < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(IPAVimc, Debug) << \"Failed to write to vimc IPA test FIFO: \"\n> >> +\t\t\t\t << strerror(-ret) << std::endl;\n> >\n> > Same here.\n> >\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h\n> >> new file mode 100644\n> >> index 000000000000..62e40c5a6714\n> >> --- /dev/null\n> >> +++ b/src/ipa/ipa_vimc.h\n> >\n> > Shouldn't this live in include/ipa/ ?\n> \n> Good question...\n> I assumed include/ would contain public headers only, and this is\n> specific to an IPA module, and only used outside from src/ipa/ for the\n> test.\n> \n> I would keep it private and provide tests a way to include it, unless\n> I'm missing a use case for sharing this with other components...\n\nAs I just explained in a reply to v1, include/ipa/ is meant for headers\nthat describe the IPA-specific interfaces. The usual use case if for the\ninterface with pipeline handlers, but I think the interface with tests\nfor the VIMC IPA makes sense, as that's a special case.\n\n> >> @@ -0,0 +1,21 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * ipa_vimc.h - Vimc Image Processing Algorithm module\n> >> + */\n> >> +\n> >> +#ifndef __LIBCAMERA_IPA_VIMC_H__\n> >> +#define __LIBCAMERA_IPA_VIMC_H__\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> >\n> > Should we pass this through an environment variable in order to support\n> > parallel running of tests ?\n> \n> I assume you mean multiple tests on the IPA Interface... Do we expect\n> more? one per operation? In any case, passing it from environment\n> variable mean both the VIMC IPA and the test should access it, and if\n> it's not defined the test would be skip. Or do you mean passing it\n> from the build environment? Because in that case it would not make\n> easier to have create different paths for multiple pipes to be used in\n> parallled by different tests\n\nI meant the build environment, to avoid multiple tests trying to use the\nsame FIFO. The name could be constructed as \"/tmp/libcamera-test-ipa.\" +\nPID for instance, to make sure it's unique.\n\n> >> +enum IPAOperationCodes {\n> >\n> > s/IPAOperationCodes/IPAOperationCode/\n> \n> I tend to pluralize enumeration types, but we only actually have them\n> in the singular form, I'll change this.\n> \n> >> +\tIPAOperationNone,\n> >> +\tIPAOperationInit,\n> >> +};\n> >> +\n> >> +}; /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */","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 3CA4A60BC6\n\tfor ;\n\tSat, 5 Oct 2019 18:49:32 +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 4AA6EDD;\n\tSat, 5 Oct 2019 18:49:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570294171;\n\tbh=ijqKYES1lJGRtqRqEis+HmYraTgPPCTW4EB8wIFEYlg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=de1ArnFQ7U/fzHDWhyXoHxsGJ1ZlHLgK15RMYUhtPlRx8KNNddVUhZp6xlUAdicyS\n\ttdaTStjTj/ql3GyGZSfIp3Z0SpWtHr66+3LEPGkTn5TgpCYsTdvqIwRHZXB9mJ84NZ\n\t8E1tmNDXTo2nIVytb8sirCdU0KZ3BJ7Nxpntlj/Y=","Date":"Sat, 5 Oct 2019 19:49:17 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191005164917.GB11154@pendragon.ideasonboard.com>","References":"<20191004163734.15594-1-jacopo@jmondi.org>\n\t<20191004163734.15594-6-jacopo@jmondi.org>\n\t<20191004210203.GC1973@pendragon.ideasonboard.com>\n\t<20191005125423.737e3e5j5igtfvtp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191005125423.737e3e5j5igtfvtp@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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:49:32 -0000"}},{"id":2806,"web_url":"https://patchwork.libcamera.org/comment/2806/","msgid":"<20191006194148.oqm5qjrun6pisuqh@uno.localdomain>","date":"2019-10-06T19:41:48","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Oct 05, 2019 at 07:49:17PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sat, Oct 05, 2019 at 02:54:23PM +0200, Jacopo Mondi wrote:\n> > On Sat, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:\n> > > On Fri, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:\n> > >> Add support to the dummy VIMC IPA for tracing operation by using a FIFO\n> > >> channel that will be used by the IPA Interface test to verify\n> > >> communications with the IPA.\n> > >>\n> > >> At the moment only add support for the init() operation as it's the only\n> > >> defined one.\n> > >>\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++\n> > >> src/ipa/ipa_vimc.h | 21 ++++++++++++++\n> > >> 2 files changed, 87 insertions(+)\n> > >> create mode 100644 src/ipa/ipa_vimc.h\n> > >>\n> > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> > >> index abc06e7f5fd5..5fb62129fef9 100644\n> > >> --- a/src/ipa/ipa_vimc.cpp\n> > >> +++ b/src/ipa/ipa_vimc.cpp\n> > >> @@ -5,22 +5,88 @@\n> > >> * ipa_vimc.cpp - Vimc Image Processing Algorithm module\n> > >> */\n> > >>\n> > >> +#include \"ipa_vimc.h\"\n> > >> +\n> > >> #include \n> > >>\n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +\n> > >> #include \n> > >> #include \n> > >>\n> > >> +#include \"log.h\"\n> > >> +\n> > >> namespace libcamera {\n> > >>\n> > >> +LOG_DEFINE_CATEGORY(IPAVimc)\n> > >> +\n> > >> class IPAVimc : public IPAInterface\n> > >> {\n> > >> public:\n> > >> +\tIPAVimc();\n> > >> +\t~IPAVimc();\n> > >> +\n> > >> \tint init();\n> > >> +\n> > >> +private:\n> > >> +\tvoid initTrace();\n> > >> +\tint trace(enum IPAOperationCodes operation);\n> > >> +\n> > >> +\tint fd_;\n> > >> };\n> > >>\n> > >> +IPAVimc::IPAVimc()\n> > >> +\t: fd_(-1)\n> > >> +{\n> > >> +\tinitTrace();\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> > > While at it could you switch this to LOG() ?\n> > >\n> > >> +\n> > >> +\treturn trace(IPAOperationInit);\n> > >\n> > > I would move this to the first line of the function, and ignore the\n> > > return value. You can't really return a value from an IPA operation as\n> > > calls are potentially asynchronous. I would thus make the trace()\n> > > method void.\n> >\n> > Agreed\n> >\n> > >> +}\n> > >> +\n> > >> +void IPAVimc::initTrace()\n> > >> +{\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\tLOG(IPAVimc, Debug) << \"Failed to open vimc IPA test FIFO: \"\n> > >> +\t\t\t\t << strerror(errno) << std::endl;\n> > >\n> > > No need for std::endl with LOG().\n> > >\n> > > \t\tLOG(IPAVimc, Debug)\n> > > \t\t\t<< \"Failed to open vimc IPA test FIFO: \"\n> > > \t\t\t<< strerror(errno);\n> > >\n> >\n> > Sorry, leftover!\n> >\n> > >> +\t\treturn;\n> > >> +\t}\n> > >> +\n> > >> +\tfd_ = ret;\n> > >> +}\n> > >> +\n> > >> +int IPAVimc::trace(enum IPAOperationCodes operation)\n> > >> +{\n> > >> +\tif (fd_ < 0)\n> > >> +\t\treturn 0;\n> > >> +\n> > >> +\tint ret = ::write(fd_, &operation, sizeof(operation));\n> > >> +\tif (ret < 0) {\n> > >> +\t\tret = -errno;\n> > >> +\t\tLOG(IPAVimc, Debug) << \"Failed to write to vimc IPA test FIFO: \"\n> > >> +\t\t\t\t << strerror(-ret) << std::endl;\n> > >\n> > > Same here.\n> > >\n> > >> +\t\treturn ret;\n> > >> +\t}\n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >>\n> > >> diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h\n> > >> new file mode 100644\n> > >> index 000000000000..62e40c5a6714\n> > >> --- /dev/null\n> > >> +++ b/src/ipa/ipa_vimc.h\n> > >\n> > > Shouldn't this live in include/ipa/ ?\n> >\n> > Good question...\n> > I assumed include/ would contain public headers only, and this is\n> > specific to an IPA module, and only used outside from src/ipa/ for the\n> > test.\n> >\n> > I would keep it private and provide tests a way to include it, unless\n> > I'm missing a use case for sharing this with other components...\n>\n> As I just explained in a reply to v1, include/ipa/ is meant for headers\n> that describe the IPA-specific interfaces. The usual use case if for the\n> interface with pipeline handlers, but I think the interface with tests\n> for the VIMC IPA makes sense, as that's a special case.\n>\n\nAgreed, will move there\n\n> > >> @@ -0,0 +1,21 @@\n> > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> +/*\n> > >> + * Copyright (C) 2019, Google Inc.\n> > >> + *\n> > >> + * ipa_vimc.h - Vimc Image Processing Algorithm module\n> > >> + */\n> > >> +\n> > >> +#ifndef __LIBCAMERA_IPA_VIMC_H__\n> > >> +#define __LIBCAMERA_IPA_VIMC_H__\n> > >> +\n> > >> +namespace libcamera {\n> > >> +\n> > >> +static const char *vimcFifoPath = \"/tmp/vimc_ipa_fifo\";\n> > >\n> > > Should we pass this through an environment variable in order to support\n> > > parallel running of tests ?\n> >\n> > I assume you mean multiple tests on the IPA Interface... Do we expect\n> > more? one per operation? In any case, passing it from environment\n> > variable mean both the VIMC IPA and the test should access it, and if\n> > it's not defined the test would be skip. Or do you mean passing it\n> > from the build environment? Because in that case it would not make\n> > easier to have create different paths for multiple pipes to be used in\n> > parallled by different tests\n>\n> I meant the build environment, to avoid multiple tests trying to use the\n> same FIFO. The name could be constructed as \"/tmp/libcamera-test-ipa.\" +\n> PID for instance, to make sure it's unique.\n>\n\nGood idea! let's do that once we have more tests ;P\n\nThanks\n j\n> > >> +enum IPAOperationCodes {\n> > >\n> > > s/IPAOperationCodes/IPAOperationCode/\n> >\n> > I tend to pluralize enumeration types, but we only actually have them\n> > in the singular form, I'll change this.\n> >\n> > >> +\tIPAOperationNone,\n> > >> +\tIPAOperationInit,\n> > >> +};\n> > >> +\n> > >> +}; /* namespace libcamera */\n> > >> +\n> > >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D45F860E1E\n\tfor ;\n\tSun, 6 Oct 2019 21:40:02 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 6C96D40007;\n\tSun, 6 Oct 2019 19:40:02 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sun, 6 Oct 2019 21:41:48 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191006194148.oqm5qjrun6pisuqh@uno.localdomain>","References":"<20191004163734.15594-1-jacopo@jmondi.org>\n\t<20191004163734.15594-6-jacopo@jmondi.org>\n\t<20191004210203.GC1973@pendragon.ideasonboard.com>\n\t<20191005125423.737e3e5j5igtfvtp@uno.localdomain>\n\t<20191005164917.GB11154@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"sbv5gxebkfbczs36\"","Content-Disposition":"inline","In-Reply-To":"<20191005164917.GB11154@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for\n\ttracing operations","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":"Sun, 06 Oct 2019 19:40:03 -0000"}}]