[{"id":4848,"web_url":"https://patchwork.libcamera.org/comment/4848/","msgid":"<37b9b58b-c132-a683-564c-a328a4bc1565@ideasonboard.com>","date":"2020-05-19T12:12:19","subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 19/05/2020 13:01, Kieran Bingham wrote:\n> The local copy of the DRM headers are stored under include/linux/drm*.\n> \n> When installed on a host system, they are instead installed to\n> /usr/include/drm/*, and as such building a userspace application against\n> the libcamera headers currently fails to find linux/drm_fourcc.h.\n> \n> Fix up the file locations, and references throughout the project.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/{linux => drm}/drm.h | 0\n> include/{linux => drm}/drm_fourcc.h | 0\n> include/{linux => drm}/drm_mode.h | 0\n> include/libcamera/pixelformats.h | 2 +-\n> src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> src/libcamera/v4l2_pixelformat.cpp | 2 +-\n> 7 files changed, 4 insertions(+), 4 deletions(-)\n> rename include/{linux => drm}/drm.h (100%)\n> rename include/{linux => drm}/drm_fourcc.h (100%)\n> rename include/{linux => drm}/drm_mode.h (100%)\n> \n> diff --git a/include/linux/drm.h b/include/drm/drm.h\n> similarity index 100%\n> rename from include/linux/drm.h\n> rename to include/drm/drm.h\n> diff --git a/include/linux/drm_fourcc.h b/include/drm/drm_fourcc.h\n> similarity index 100%\n> rename from include/linux/drm_fourcc.h\n> rename to include/drm/drm_fourcc.h\n> diff --git a/include/linux/drm_mode.h b/include/drm/drm_mode.h\n> similarity index 100%\n> rename from include/linux/drm_mode.h\n> rename to include/drm/drm_mode.h\n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 89966e5e664c..e3cdb711b828 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -11,7 +11,7 @@\n> #include \n> #include \n> \n> -#include \n> +#include \n> \n> namespace libcamera {\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index a3cb0746e012..41ca2b90867d 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -8,7 +8,7 @@\n> \n> #include \"gstlibcamera-utils.h\"\n\nThis header includes libcamera/stream.h, which includes\nlibcamera/pixelformats.h, which includes drm/drm_fourcc.h... so we can\ndrop the drm include below - unless we want to put it there to\nexplicitly show we need it.\n\n> \n> -#include \n> +#include \n> \n> using namespace libcamera;\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 07ca9f5d7f53..198ec295a840 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -18,7 +18,7 @@\n> #include \n> #include \n> \n> -#include \n> +#include \n\nAnd here, I think we should better include libcamera/pixelformats.h over\nthe drm/drm_fourcc.h anyway.\n\nAnd in fact, libcamera/stream.h already includes\nlibcamera/pixelformats.h as well.\n\nShould we include libcamera/pixelformats.h explicitly ?\n\n> #include \n> \n> #include \"libcamera/internal/camera_sensor.h\"\n> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp\n> index 36776be99e59..1c4a05b37e23 100644\n> --- a/src/libcamera/v4l2_pixelformat.cpp\n> +++ b/src/libcamera/v4l2_pixelformat.cpp\n> @@ -12,7 +12,7 @@\n> #include \n> #include \n> \n> -#include \n> +#include \n\nThis is included in libcamera/pixelformats.h below so isn't needed.\n\n> \n> #include \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 32EA9603D9\n\tfor ;\n\tTue, 19 May 2020 14:12:23 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A406830C;\n\tTue, 19 May 2020 14:12:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pLWioUO8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589890342;\n\tbh=1qoqUWquzu6eG5esSsaFunSe8ta5PBpSuk+7uM6Gcl8=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=pLWioUO8f0ekvH/C4S0NyGxpdiqeh1bM85gooYYxZWIzw0j5+niELBNCeHmZK1GBN\n\tjTjrEfSeE+vglD79+8bdEkFgIsAZRrCO/+AlSfxbyTqT9DXg/LM+yx+rUwiTJ+v/Ca\n\t1wU1KeQY0pZJw0cjq0KQUrchTNoCUkCWshWuDEpQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kieran Bingham ,\n\tLibCamera Devel ","References":"<20200519120130.2399495-1-kieran.bingham@ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<37b9b58b-c132-a683-564c-a328a4bc1565@ideasonboard.com>","Date":"Tue, 19 May 2020 13:12:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200519120130.2399495-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","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":"Tue, 19 May 2020 12:12:23 -0000"}},{"id":4861,"web_url":"https://patchwork.libcamera.org/comment/4861/","msgid":"<20200519143012.GD3820@pendragon.ideasonboard.com>","date":"2020-05-19T14:30:12","subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, May 19, 2020 at 01:12:19PM +0100, Kieran Bingham wrote:\n> On 19/05/2020 13:01, Kieran Bingham wrote:\n> > The local copy of the DRM headers are stored under include/linux/drm*.\n> > \n> > When installed on a host system, they are instead installed to\n> > /usr/include/drm/*, and as such building a userspace application against\n> > the libcamera headers currently fails to find linux/drm_fourcc.h.\n\nThis seems to however be distribution-dependent :-S Ubuntu provides\n/usr/include/drm/drm_fourcc.h in the linux-libc-dev package, and Gentoo\nin the sys-kernel/linux-headers package, but Debian and Arch Linux don't\nseem to have any package that would provide the file in that directory.\nThey however have packages that provide the file in\n/usr//include/drm/ (linux-libc-dev- for Debian), and I\nassume /usr//include is part of the standard header search paths\nthat the compiler is configured to use. It could thus work, but should\nbe tested.\n\nI wonder if a better solution wouldn't be to define our own format\nconstants in pixelformats.h:\n\nnamespace {\n\nnamespace formats {\n\nconstexpr uint32_t __fourcc(char a, char b, char c, char d)\n{\n\treturn (static_cast(a) << 0) |\n\t (static_cast(b) << 8) |\n\t (static_cast(c) << 16) |\n\t (static_cast(d) << 24);\n}\n\n} /* namespace */\n\nconstexpr PixelFormat RGB888{ __fourcc('R', 'G', '2', '4') };\n...\n\n} /* namespace formats */\n\nThe values would match drm_fourcc.h, and the file could even be\ngenerated from the private copy of drm_fourcc.h. Applications could then\nuse formats::RGB888 instead of PixelFormat(DRM_FORMAT_RGB888), which\ncould simplify applications that don't need to deal with display.\nConstructing a PixelFormat from a FourCC retrieved from a display device\nwould still work fine, so interoperability is guaranteed without the\nneed for a translation layer between DRM/KMS FourCCs and libcamera\nformats.\n\nAll this requires some more thoughts (and work). Your patch doesn't\nintroduce any regression, so, in the meantime,\n\nAcked-by: Laurent Pinchart \n\nPlease see below for some comments.\n\n> > Fix up the file locations, and references throughout the project.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/{linux => drm}/drm.h | 0\n> > include/{linux => drm}/drm_fourcc.h | 0\n> > include/{linux => drm}/drm_mode.h | 0\n> > include/libcamera/pixelformats.h | 2 +-\n> > src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/libcamera/v4l2_pixelformat.cpp | 2 +-\n> > 7 files changed, 4 insertions(+), 4 deletions(-)\n> > rename include/{linux => drm}/drm.h (100%)\n> > rename include/{linux => drm}/drm_fourcc.h (100%)\n> > rename include/{linux => drm}/drm_mode.h (100%)\n> > \n> > diff --git a/include/linux/drm.h b/include/drm/drm.h\n> > similarity index 100%\n> > rename from include/linux/drm.h\n> > rename to include/drm/drm.h\n> > diff --git a/include/linux/drm_fourcc.h b/include/drm/drm_fourcc.h\n> > similarity index 100%\n> > rename from include/linux/drm_fourcc.h\n> > rename to include/drm/drm_fourcc.h\n> > diff --git a/include/linux/drm_mode.h b/include/drm/drm_mode.h\n> > similarity index 100%\n> > rename from include/linux/drm_mode.h\n> > rename to include/drm/drm_mode.h\n> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> > index 89966e5e664c..e3cdb711b828 100644\n> > --- a/include/libcamera/pixelformats.h\n> > +++ b/include/libcamera/pixelformats.h\n> > @@ -11,7 +11,7 @@\n> > #include \n> > #include \n> > \n> > -#include \n> > +#include \n> > \n> > namespace libcamera {\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index a3cb0746e012..41ca2b90867d 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -8,7 +8,7 @@\n> > \n> > #include \"gstlibcamera-utils.h\"\n> \n> This header includes libcamera/stream.h, which includes\n> libcamera/pixelformats.h, which includes drm/drm_fourcc.h... so we can\n> drop the drm include below - unless we want to put it there to\n> explicitly show we need it.\n\nIf we decide to move towards a libcamera formats namespace, we will drop\ndrm_fourcc.h completely, so we could already do so to be prepared for\nthe future. If we decide to keep using drm_fourcc.h, I wonder if it\nshould be included in pixelformats.h, or explicitly included by the\nsource files that require it. I would then be tempted to leave it here.\n\n> > \n> > -#include \n> > +#include \n> > \n> > using namespace libcamera;\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 07ca9f5d7f53..198ec295a840 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -18,7 +18,7 @@\n> > #include \n> > #include \n> > \n> > -#include \n> > +#include \n> \n> And here, I think we should better include libcamera/pixelformats.h over\n> the drm/drm_fourcc.h anyway.\n> \n> And in fact, libcamera/stream.h already includes\n> libcamera/pixelformats.h as well.\n> \n> Should we include libcamera/pixelformats.h explicitly ?\n\nI tend to include headers explicitly when using classes that they\nprovide instead of relying on indirect includes. The only exception I\napply to that rule is to not include headers in .cpp files that are\nincluded by the corresponding .h file already, but that's not a rule I\nwould enforce for everybody.\n\n> > #include \n> > \n> > #include \"libcamera/internal/camera_sensor.h\"\n> > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp\n> > index 36776be99e59..1c4a05b37e23 100644\n> > --- a/src/libcamera/v4l2_pixelformat.cpp\n> > +++ b/src/libcamera/v4l2_pixelformat.cpp\n> > @@ -12,7 +12,7 @@\n> > #include \n> > #include \n> > \n> > -#include \n> > +#include \n> \n> This is included in libcamera/pixelformats.h below so isn't needed.\n\nSame comment as above for gstlibcamera-utils.cpp.\n\n> > #include \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 78BDF603D9\n\tfor ;\n\tTue, 19 May 2020 16:30:25 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90E5830C;\n\tTue, 19 May 2020 16:30:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZTuQfJL6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589898625;\n\tbh=hZRqyv0KWUbsdICfeofjdPZ21dPq29kwox4UxFGlohI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZTuQfJL6KYcw0PgILoqvLITWkEr6QHl1kbrmexEOnCHkR/920eLLVAFu5bN5pUT+6\n\tNDawwMA4bsSCh7UlxUz9isfcHp/KJo7RmZLh4cMinnLEQWH9Uj2t+kOG5FNT0yW+OE\n\tUxS3lDPKZw162PSq9eKuv6nqIpxcCx41eEVBiQO0=","Date":"Tue, 19 May 2020 17:30:12 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200519143012.GD3820@pendragon.ideasonboard.com>","References":"<20200519120130.2399495-1-kieran.bingham@ideasonboard.com>\n\t<37b9b58b-c132-a683-564c-a328a4bc1565@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<37b9b58b-c132-a683-564c-a328a4bc1565@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","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":"Tue, 19 May 2020 14:30:25 -0000"}},{"id":4880,"web_url":"https://patchwork.libcamera.org/comment/4880/","msgid":"<6b4d6444-59de-b2df-45dc-fbbe71399859@ideasonboard.com>","date":"2020-05-21T11:33:50","subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 19/05/2020 15:30, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, May 19, 2020 at 01:12:19PM +0100, Kieran Bingham wrote:\n>> On 19/05/2020 13:01, Kieran Bingham wrote:\n>>> The local copy of the DRM headers are stored under include/linux/drm*.\n>>>\n>>> When installed on a host system, they are instead installed to\n>>> /usr/include/drm/*, and as such building a userspace application against\n>>> the libcamera headers currently fails to find linux/drm_fourcc.h.\n> \n> This seems to however be distribution-dependent :-S Ubuntu provides\n> /usr/include/drm/drm_fourcc.h in the linux-libc-dev package, and Gentoo\n> in the sys-kernel/linux-headers package, but Debian and Arch Linux don't\n> seem to have any package that would provide the file in that directory.\n> They however have packages that provide the file in\n> /usr//include/drm/ (linux-libc-dev- for Debian), and I\n> assume /usr//include is part of the standard header search paths\n> that the compiler is configured to use. It could thus work, but should\n> be tested.\n\n\nI would expect that, that extra path is due to differences in how they\npackage their 'standard' compiler. I would 'bet' that the compiler lives\nunder /usr//... too, and they've just extended the sysroot.\n\ngcc --print-sysroot would be interesting to see on those platforms.\n\nOr also:\n\n gcc -xc++ -E -v -\n\nwill report the configuration being used.\n\n\nMine reports:\n\n> $ gcc -xc++ -E -v -\n> Using built-in specs.\n> COLLECT_GCC=gcc\n> OFFLOAD_TARGET_NAMES=nvptx-none:hsa\n> OFFLOAD_TARGET_DEFAULT=1\n> Target: x86_64-linux-gnu\n> Configured with: ../src/configure -v --with-pkgversion='Ubuntu 9.3.0-10ubuntu2' --with-bugurl=file:///usr/share/doc/gcc-9/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,gm2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-9 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none,hsa --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu\n> Thread model: posix\n> gcc version 9.3.0 (Ubuntu 9.3.0-10ubuntu2) \n> COLLECT_GCC_OPTIONS='-E' '-v' '-mtune=generic' '-march=x86-64'\n> /usr/lib/gcc/x86_64-linux-gnu/9/cc1plus -E -quiet -v -imultiarch x86_64-linux-gnu -D_GNU_SOURCE - -mtune=generic -march=x86-64 -fasynchronous-unwind-tables -fstack-protector-strong -Wformat -Wformat-security -fstack-clash-protection -fcf-protection\n> ignoring duplicate directory \"/usr/include/x86_64-linux-gnu/c++/9\"\n> ignoring nonexistent directory \"/usr/local/include/x86_64-linux-gnu\"\n> ignoring nonexistent directory \"/usr/lib/gcc/x86_64-linux-gnu/9/include-fixed\"\n> ignoring nonexistent directory \"/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include\"\n> #include \"...\" search starts here:\n> #include <...> search starts here:\n> /usr/include/c++/9\n> /usr/include/x86_64-linux-gnu/c++/9\n> /usr/include/c++/9/backward\n> /usr/lib/gcc/x86_64-linux-gnu/9/include\n> /usr/local/include\n> /usr/include/x86_64-linux-gnu\n> /usr/include\n> End of search list.\n\n\n> \n> I wonder if a better solution wouldn't be to define our own format\n> constants in pixelformats.h:\n\nAyeee ... so yet another standard ... (Ok, so ours would be the same as\nDRM ... but ....)\n\nIf we go down that route, that feels like it deserves it's own full\nformat library...\n\n\n> namespace {\n> \n> namespace formats {\n> \n> constexpr uint32_t __fourcc(char a, char b, char c, char d)\n> {\n> \treturn (static_cast(a) << 0) |\n> \t (static_cast(b) << 8) |\n> \t (static_cast(c) << 16) |\n> \t (static_cast(d) << 24);\n> }\n> \n> } /* namespace */\n> \n> constexpr PixelFormat RGB888{ __fourcc('R', 'G', '2', '4') };\n> ...\n> \n> } /* namespace formats */\n> \n> The values would match drm_fourcc.h, and the file could even be\n> generated from the private copy of drm_fourcc.h. Applications could then\n> use formats::RGB888 instead of PixelFormat(DRM_FORMAT_RGB888), which\n\nWell, I do like removing the DRM_FORMAT prefixes from 'libcamera' code...\n\n> could simplify applications that don't need to deal with display.\n> Constructing a PixelFormat from a FourCC retrieved from a display device\n> would still work fine, so interoperability is guaranteed without the\n> need for a translation layer between DRM/KMS FourCCs and libcamera\n> formats.\n> \n> All this requires some more thoughts (and work). Your patch doesn't\n> introduce any regression, so, in the meantime,\n> \n> Acked-by: Laurent Pinchart \n\nAt the moment, external applications can't be compiled against libcamera\nat all - so I think this needs some sort of immediate resolution at the\nleast.\n\n\n> Please see below for some comments.\n> \n>>> Fix up the file locations, and references throughout the project.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> include/{linux => drm}/drm.h | 0\n>>> include/{linux => drm}/drm_fourcc.h | 0\n>>> include/{linux => drm}/drm_mode.h | 0\n>>> include/libcamera/pixelformats.h | 2 +-\n>>> src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>>> src/libcamera/v4l2_pixelformat.cpp | 2 +-\n>>> 7 files changed, 4 insertions(+), 4 deletions(-)\n>>> rename include/{linux => drm}/drm.h (100%)\n>>> rename include/{linux => drm}/drm_fourcc.h (100%)\n>>> rename include/{linux => drm}/drm_mode.h (100%)\n>>>\n>>> diff --git a/include/linux/drm.h b/include/drm/drm.h\n>>> similarity index 100%\n>>> rename from include/linux/drm.h\n>>> rename to include/drm/drm.h\n>>> diff --git a/include/linux/drm_fourcc.h b/include/drm/drm_fourcc.h\n>>> similarity index 100%\n>>> rename from include/linux/drm_fourcc.h\n>>> rename to include/drm/drm_fourcc.h\n>>> diff --git a/include/linux/drm_mode.h b/include/drm/drm_mode.h\n>>> similarity index 100%\n>>> rename from include/linux/drm_mode.h\n>>> rename to include/drm/drm_mode.h\n>>> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n>>> index 89966e5e664c..e3cdb711b828 100644\n>>> --- a/include/libcamera/pixelformats.h\n>>> +++ b/include/libcamera/pixelformats.h\n>>> @@ -11,7 +11,7 @@\n>>> #include \n>>> #include \n>>> \n>>> -#include \n>>> +#include \n>>> \n>>> namespace libcamera {\n>>> \n>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>>> index a3cb0746e012..41ca2b90867d 100644\n>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>> @@ -8,7 +8,7 @@\n>>> \n>>> #include \"gstlibcamera-utils.h\"\n>>\n>> This header includes libcamera/stream.h, which includes\n>> libcamera/pixelformats.h, which includes drm/drm_fourcc.h... so we can\n>> drop the drm include below - unless we want to put it there to\n>> explicitly show we need it.\n> \n> If we decide to move towards a libcamera formats namespace, we will drop\n> drm_fourcc.h completely, so we could already do so to be prepared for\n> the future. If we decide to keep using drm_fourcc.h, I wonder if it\n> should be included in pixelformats.h, or explicitly included by the\n> source files that require it. I would then be tempted to leave it here.\n\nIncluding it does have the benefit of showing that this file is directly\nusing the drm_fourcc namespaced pixelformats...\n\n> \n>>> \n>>> -#include \n>>> +#include \n>>> \n>>> using namespace libcamera;\n>>> \n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index 07ca9f5d7f53..198ec295a840 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -18,7 +18,7 @@\n>>> #include \n>>> #include \n>>> \n>>> -#include \n>>> +#include \n>>\n>> And here, I think we should better include libcamera/pixelformats.h over\n>> the drm/drm_fourcc.h anyway.\n>>\n>> And in fact, libcamera/stream.h already includes\n>> libcamera/pixelformats.h as well.\n>>\n>> Should we include libcamera/pixelformats.h explicitly ?\n> \n> I tend to include headers explicitly when using classes that they\n> provide instead of relying on indirect includes. The only exception I\n> apply to that rule is to not include headers in .cpp files that are\n> included by the corresponding .h file already, but that's not a rule I\n> would enforce for everybody.\n> \n>>> #include \n>>> \n>>> #include \"libcamera/internal/camera_sensor.h\"\n>>> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp\n>>> index 36776be99e59..1c4a05b37e23 100644\n>>> --- a/src/libcamera/v4l2_pixelformat.cpp\n>>> +++ b/src/libcamera/v4l2_pixelformat.cpp\n>>> @@ -12,7 +12,7 @@\n>>> #include \n>>> #include \n>>> \n>>> -#include \n>>> +#include \n>>\n>> This is included in libcamera/pixelformats.h below so isn't needed.\n> \n> Same comment as above for gstlibcamera-utils.cpp.\n\nOk - so no need to remove any further lines, and just the 'fix' to apply.\n\n\n\n> \n>>> #include \n>>> \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 24383603D8\n\tfor ;\n\tThu, 21 May 2020 13:33:56 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 484E624D;\n\tThu, 21 May 2020 13:33:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FY7Kb+Fc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1590060835;\n\tbh=+HVFyppjhdIxu0dOeDSip2/ExqG2RZmTKG6JBkxQ9bM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FY7Kb+FcMhRISzmsvktfI9UlRWwEYiUK2brJ/DZy6TGnh5jgXbuIsHRgq7WnAgyYD\n\t63VZQ/wpysR6R2NnHlpLjOzLfN5+BKpd6r/MpvYLOCvgKtvRfjeIPVJ5YJCTKOpGZ+\n\tIcpY1fUpGGJE1rYREfHYAyiLkg0ATy480iUjqAUs=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20200519120130.2399495-1-kieran.bingham@ideasonboard.com>\n\t<37b9b58b-c132-a683-564c-a328a4bc1565@ideasonboard.com>\n\t<20200519143012.GD3820@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<6b4d6444-59de-b2df-45dc-fbbe71399859@ideasonboard.com>","Date":"Thu, 21 May 2020 12:33:50 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200519143012.GD3820@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"base64","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Move DRM headers to\n\tinclude/drm","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, 21 May 2020 11:33:56 -0000"}}]