[{"id":834,"web_url":"https://patchwork.libcamera.org/comment/834/","msgid":"<20190221160528.GI11484@bigcity.dyn.berto.se>","date":"2019-02-21T16:05:28","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipu3: Break-out ipu3\n\theader file","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2019-02-20 14:17:55 +0100, Jacopo Mondi wrote:\n> As the class grows, break out the class definitions in a separate header\n> file, which can be used by other ipu3-related cpp files that will be\n> added in next commits.\n\nI would mention that there is no other change in this commit then moving \nthe class definition to a separate header file.\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +-----------------\n> src/libcamera/pipeline/ipu3/ipu3.h | 85 ++++++++++++++++++++++++++++\n> 2 files changed, 86 insertions(+), 55 deletions(-)\n> create mode 100644 src/libcamera/pipeline/ipu3/ipu3.h\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9065073913a2..07029dd763c9 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -14,6 +14,7 @@\n> \n> #include \"device_enumerator.h\"\n> #include \"log.h\"\n> +#include \"ipu3.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> #include \"utils.h\"\n> @@ -24,61 +25,6 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPU3)\n> \n> -class PipelineHandlerIPU3 : public PipelineHandler\n> -{\n> -public:\n> -\tPipelineHandlerIPU3(CameraManager *manager);\n> -\t~PipelineHandlerIPU3();\n> -\n> -\tstd::map\n> -\tstreamConfiguration(Camera *camera,\n> -\t\t\t std::vector &streams) override;\n> -\tint configureStreams(Camera *camera,\n> -\t\t\t std::map &config) override;\n> -\n> -\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> -\tint freeBuffers(Camera *camera, Stream *stream) override;\n> -\n> -\tint start(const Camera *camera) override;\n> -\tvoid stop(const Camera *camera) override;\n> -\n> -\tint queueRequest(const Camera *camera, Request *request) override;\n> -\n> -\tbool match(DeviceEnumerator *enumerator);\n> -\n> -private:\n> -\tclass IPU3CameraData : public CameraData\n> -\t{\n> -\tpublic:\n> -\t\tIPU3CameraData()\n> -\t\t\t: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}\n> -\n> -\t\t~IPU3CameraData()\n> -\t\t{\n> -\t\t\tdelete cio2_;\n> -\t\t\tdelete csi2_;\n> -\t\t\tdelete sensor_;\n> -\t\t}\n> -\n> -\t\tV4L2Device *cio2_;\n> -\t\tV4L2Subdevice *csi2_;\n> -\t\tV4L2Subdevice *sensor_;\n> -\n> -\t\tStream stream_;\n> -\t};\n> -\n> -\tIPU3CameraData *cameraData(const Camera *camera)\n> -\t{\n> -\t\treturn static_cast(\n> -\t\t\tPipelineHandler::cameraData(camera));\n> -\t}\n> -\n> -\tvoid registerCameras();\n> -\n> -\tstd::shared_ptr cio2_;\n> -\tstd::shared_ptr imgu_;\n> -};\n> -\n> PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> \t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n> {\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h\n> new file mode 100644\n> index 000000000000..48c2a3e16980\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.h\n> @@ -0,0 +1,85 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipu3.h - Pipeline handler for Intel IPU3\n> + */\n> +\n> +#ifndef __LIBCAMERA_PIPELINE_IPU3_H__\n> +#define __LIBCAMERA_PIPELINE_IPU3_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +#include \"media_device.h\"\n> +#include \"pipeline_handler.h\"\n> +#include \"v4l2_device.h\"\n> +#include \"v4l2_subdevice.h\"\n\nDo you really need to include all these headers here? Is it not enough \nto include the bulk of them in the cpp file?\n\n> +\n> +namespace libcamera {\n> +\n> +class PipelineHandlerIPU3 : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerIPU3(CameraManager *manager);\n> +\t~PipelineHandlerIPU3();\n> +\n> +\tstd::map\n> +\tstreamConfiguration(Camera *camera,\n> +\t\t\t std::vector &streams) override;\n> +\tint configureStreams(Camera *camera,\n> +\t\t\t std::map &config) override;\n> +\n> +\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> +\tint freeBuffers(Camera *camera, Stream *stream) override;\n> +\n> +\tint start(const Camera *camera) override;\n> +\tvoid stop(const Camera *camera) override;\n> +\n> +\tint queueRequest(const Camera *camera, Request *request) override;\n> +\n> +\tbool match(DeviceEnumerator *enumerator);\n> +\n> +private:\n> +\tclass IPU3CameraData : public CameraData\n> +\t{\n> +\tpublic:\n> +\t\tIPU3CameraData()\n> +\t\t\t: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}\n> +\n> +\t\t~IPU3CameraData()\n> +\t\t{\n> +\t\t\tdelete cio2_;\n> +\t\t\tdelete csi2_;\n> +\t\t\tdelete sensor_;\n> +\t\t}\n> +\n> +\t\tV4L2Device *cio2_;\n> +\t\tV4L2Subdevice *csi2_;\n> +\t\tV4L2Subdevice *sensor_;\n> +\n> +\t\tStream stream_;\n> +\t};\n> +\n> +\tIPU3CameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tvoid registerCameras();\n> +\n> +\tstd::shared_ptr cio2_;\n> +\tstd::shared_ptr imgu_;\n> +\tIMGUDevice imgu0_;\n> +\tIMGUDevice imgu1_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_IPU3_H__ */\n> -- \n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47E57600FB\n\tfor ;\n\tThu, 21 Feb 2019 17:05:30 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id v7so20768059lfd.2\n\tfor ;\n\tThu, 21 Feb 2019 08:05:30 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr12sm4845669ljc.97.2019.02.21.08.05.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 21 Feb 2019 08:05:28 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=4iwrtE0F5nRoYjiYmlVdSvsK+bvCxmBNgKafk5SuyHo=;\n\tb=nWfbbW67Q7D37nM27P8P7/BQzgUFvWdpXZUfbXxPaLVsTxvJ/NhTjIpR31JmG2luZZ\n\tzlVlIP3qdQxUYzKlhMJcmpggETemUOQGCg1P+hK0QB18Z9oMWHjstKHgcLmdxlkDYySL\n\t5OytexUIy40d50TdIA9tvzRqdmUaEkothXiTuuk559mLIjGVdIsn6VQevIX6Uq04dFHK\n\t3uwgJ/MFMJECwRtzfl8spRlYHMKMPYsMSEefBAjHnsm07RbX7+JA0PFcAuVZjko5nDBp\n\tgT2ZiCps49CYGwSi1nen0bncEJCAbPgbWRw8C/+YToB/0hKgfI/LFW1qFJzVs8dfPWwW\n\tFx/g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=4iwrtE0F5nRoYjiYmlVdSvsK+bvCxmBNgKafk5SuyHo=;\n\tb=N/URFSPc3HuP1i/xnc3M0F85BAS81oo1BjEKNEgQL1nWIJt2TxuXHHNePy8C8Jlihr\n\tqNVhbt6TZpgcxyZvyl4tza5kZY78YLkKWgchAaGC37LEJfBxQW+3Ol5FZae8ZS2JZYMK\n\tGlNT7xtHwWg3b9hrU+kVhdq5NA7HqfMxPbppUhEL5r1mfssdkCbBctG2unC4jNe/54OQ\n\tuloWbXIP4tobkc5q0cT84BPjKGhmuzGU8Eh1HUgwXR/j55GIOEDzFtQsp6tdml5Eq+eP\n\tnH5FfWNkx3AI3rrOorDiTo0kXaHTMtnZfOjoC9P9l2nUfoP0HHOCmovNDn/aKxhFfB9q\n\tILvA==","X-Gm-Message-State":"AHQUAuatg3YINeuRmTBnNtM/p5t4luBS1qCWKCBdRQT5VRY6UvT30xVb\n\tSsyX7jsMxY0xE2Dq1b6CpFU3ETGbaZY=","X-Google-Smtp-Source":"AHgI3Ia0ST/Og7kBukDXSkWRm5cZVporEnMhQPu0UDKJb5ocaAbSYBKhf/SqQHDlyYiwEcLbFRLHEA==","X-Received":"by 2002:a19:a98f:: with SMTP id\n\ts137mr25413749lfe.18.1550765129553; \n\tThu, 21 Feb 2019 08:05:29 -0800 (PST)","Date":"Thu, 21 Feb 2019 17:05:28 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190221160528.GI11484@bigcity.dyn.berto.se>","References":"<20190220131757.14004-1-jacopo@jmondi.org>\n\t<20190220131757.14004-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190220131757.14004-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipu3: Break-out ipu3\n\theader file","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 21 Feb 2019 16:05:30 -0000"}},{"id":841,"web_url":"https://patchwork.libcamera.org/comment/841/","msgid":"<20190221234401.GL3485@pendragon.ideasonboard.com>","date":"2019-02-21T23:44:01","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipu3: Break-out ipu3\n\theader file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Feb 21, 2019 at 05:05:28PM +0100, Niklas Söderlund wrote:\n> On 2019-02-20 14:17:55 +0100, Jacopo Mondi wrote:\n> > As the class grows, break out the class definitions in a separate header\n> > file, which can be used by other ipu3-related cpp files that will be\n> > added in next commits.\n> \n> I would mention that there is no other change in this commit then moving \n> the class definition to a separate header file.\n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +-----------------\n> > src/libcamera/pipeline/ipu3/ipu3.h | 85 ++++++++++++++++++++++++++++\n> > 2 files changed, 86 insertions(+), 55 deletions(-)\n> > create mode 100644 src/libcamera/pipeline/ipu3/ipu3.h\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 9065073913a2..07029dd763c9 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -14,6 +14,7 @@\n> > \n> > #include \"device_enumerator.h\"\n> > #include \"log.h\"\n> > +#include \"ipu3.h\"\n\nAlphabetical order ? Bonus point to anyone who submits a patch for\ncheckstyle.py to catch these errors :-)\n\n> > #include \"media_device.h\"\n> > #include \"pipeline_handler.h\"\n> > #include \"utils.h\"\n> > @@ -24,61 +25,6 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(IPU3)\n> > \n> > -class PipelineHandlerIPU3 : public PipelineHandler\n> > -{\n> > -public:\n> > -\tPipelineHandlerIPU3(CameraManager *manager);\n> > -\t~PipelineHandlerIPU3();\n> > -\n> > -\tstd::map\n> > -\tstreamConfiguration(Camera *camera,\n> > -\t\t\t std::vector &streams) override;\n> > -\tint configureStreams(Camera *camera,\n> > -\t\t\t std::map &config) override;\n> > -\n> > -\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > -\tint freeBuffers(Camera *camera, Stream *stream) override;\n> > -\n> > -\tint start(const Camera *camera) override;\n> > -\tvoid stop(const Camera *camera) override;\n> > -\n> > -\tint queueRequest(const Camera *camera, Request *request) override;\n> > -\n> > -\tbool match(DeviceEnumerator *enumerator);\n> > -\n> > -private:\n> > -\tclass IPU3CameraData : public CameraData\n> > -\t{\n> > -\tpublic:\n> > -\t\tIPU3CameraData()\n> > -\t\t\t: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}\n> > -\n> > -\t\t~IPU3CameraData()\n> > -\t\t{\n> > -\t\t\tdelete cio2_;\n> > -\t\t\tdelete csi2_;\n> > -\t\t\tdelete sensor_;\n> > -\t\t}\n> > -\n> > -\t\tV4L2Device *cio2_;\n> > -\t\tV4L2Subdevice *csi2_;\n> > -\t\tV4L2Subdevice *sensor_;\n> > -\n> > -\t\tStream stream_;\n> > -\t};\n> > -\n> > -\tIPU3CameraData *cameraData(const Camera *camera)\n> > -\t{\n> > -\t\treturn static_cast(\n> > -\t\t\tPipelineHandler::cameraData(camera));\n> > -\t}\n> > -\n> > -\tvoid registerCameras();\n> > -\n> > -\tstd::shared_ptr cio2_;\n> > -\tstd::shared_ptr imgu_;\n> > -};\n> > -\n> > PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> > \t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n> > {\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h\n> > new file mode 100644\n> > index 000000000000..48c2a3e16980\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.h\n> > @@ -0,0 +1,85 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipu3.h - Pipeline handler for Intel IPU3\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_PIPELINE_IPU3_H__\n> > +#define __LIBCAMERA_PIPELINE_IPU3_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"device_enumerator.h\"\n> > +#include \"media_device.h\"\n> > +#include \"pipeline_handler.h\"\n> > +#include \"v4l2_device.h\"\n> > +#include \"v4l2_subdevice.h\"\n> \n> Do you really need to include all these headers here? Is it not enough \n> to include the bulk of them in the cpp file?\n\nI think most of the classes can indeed be forwared-declared.\n\nThe patch otherwise looks fine to me, but neither patch 4/5 nor patch\n5/5 need to access the PipelineHandlerIPU3 class. Do we really need to\nsplit it out to ipu3.h ? I don't challenge the need of an ipu3.h header\nto store the CIO2 and ImgU classes, but it seems that\nPipelineHandlerIPU3 itself could stay in ipu3.cpp.\n\n> > +\n> > +namespace libcamera {\n> > +\n> > +class PipelineHandlerIPU3 : public PipelineHandler\n> > +{\n> > +public:\n> > +\tPipelineHandlerIPU3(CameraManager *manager);\n> > +\t~PipelineHandlerIPU3();\n> > +\n> > +\tstd::map\n> > +\tstreamConfiguration(Camera *camera,\n> > +\t\t\t std::vector &streams) override;\n> > +\tint configureStreams(Camera *camera,\n> > +\t\t\t std::map &config) override;\n> > +\n> > +\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > +\tint freeBuffers(Camera *camera, Stream *stream) override;\n> > +\n> > +\tint start(const Camera *camera) override;\n> > +\tvoid stop(const Camera *camera) override;\n> > +\n> > +\tint queueRequest(const Camera *camera, Request *request) override;\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator);\n> > +\n> > +private:\n> > +\tclass IPU3CameraData : public CameraData\n> > +\t{\n> > +\tpublic:\n> > +\t\tIPU3CameraData()\n> > +\t\t\t: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}\n> > +\n> > +\t\t~IPU3CameraData()\n> > +\t\t{\n> > +\t\t\tdelete cio2_;\n> > +\t\t\tdelete csi2_;\n> > +\t\t\tdelete sensor_;\n> > +\t\t}\n> > +\n> > +\t\tV4L2Device *cio2_;\n> > +\t\tV4L2Subdevice *csi2_;\n> > +\t\tV4L2Subdevice *sensor_;\n> > +\n> > +\t\tStream stream_;\n> > +\t};\n> > +\n> > +\tIPU3CameraData *cameraData(const Camera *camera)\n> > +\t{\n> > +\t\treturn static_cast(\n> > +\t\t\tPipelineHandler::cameraData(camera));\n> > +\t}\n> > +\n> > +\tvoid registerCameras();\n> > +\n> > +\tstd::shared_ptr cio2_;\n> > +\tstd::shared_ptr imgu_;\n> > +\tIMGUDevice imgu0_;\n> > +\tIMGUDevice imgu1_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_IPU3_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 A3CB0600FB\n\tfor ;\n\tFri, 22 Feb 2019 00:44:06 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18ADD255;\n\tFri, 22 Feb 2019 00:44:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1550792646;\n\tbh=Uvyt3uV9HCH5jeQofeDrY2UWSO+H5WOsnP4ezMoDeCg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jGB2RYsQt7VvFod7jETXNZY54Kv/cTujBzSsVERJDh6tGJSQjdmM2yLM+4dG4c7ok\n\tARBB+PHbt4uVqsFYV3ViP3x4Dn6BZqUQbQ/1eiHc1qQNrEIwK1lAkhOSVZ/tR47lcb\n\tgRQTLSvA2zJ3qMTrBFA/bQJzomg0aOgx5e1hRwc8=","Date":"Fri, 22 Feb 2019 01:44:01 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190221234401.GL3485@pendragon.ideasonboard.com>","References":"<20190220131757.14004-1-jacopo@jmondi.org>\n\t<20190220131757.14004-4-jacopo@jmondi.org>\n\t<20190221160528.GI11484@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190221160528.GI11484@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipu3: Break-out ipu3\n\theader file","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 21 Feb 2019 23:44:06 -0000"}}]