[libcamera-devel] [PATCH] libcamera: Move DRM headers to include/drm

Thu May 21 13:33:50 CEST 2020

Hi Laurent,

On 19/05/2020 15:30, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tue, May 19, 2020 at 01:12:19PM +0100, Kieran Bingham wrote:
>> On 19/05/2020 13:01, Kieran Bingham wrote:
>>> The local copy of the DRM headers are stored under include/linux/drm*.
>>>
>>> When installed on a host system, they are instead installed to
>>> /usr/include/drm/*, and as such building a userspace application against
>>> the libcamera headers currently fails to find linux/drm_fourcc.h.
> 
> This seems to however be distribution-dependent :-S Ubuntu provides
> /usr/include/drm/drm_fourcc.h in the linux-libc-dev package, and Gentoo
> in the sys-kernel/linux-headers package, but Debian and Arch Linux don't
> seem to have any package that would provide the file in that directory.
> They however have packages that provide the file in
> /usr/<arch>/include/drm/ (linux-libc-dev-<arch> for Debian), and I
> assume /usr/<arch>/include is part of the standard header search paths
> that the compiler is configured to use. It could thus work, but should
> be tested.

I would expect that, that extra path is due to differences in how they
package their 'standard' compiler. I would 'bet' that the compiler lives
under /usr/<arch>/... too, and they've just extended the sysroot.

gcc --print-sysroot would be interesting to see on those platforms.

Or also:

 gcc -xc++ -E -v -

will report the configuration being used.

Mine reports:

> $ gcc -xc++ -E -v -
> Using built-in specs.
> COLLECT_GCC=gcc
> OFFLOAD_TARGET_NAMES=nvptx-none:hsa
> OFFLOAD_TARGET_DEFAULT=1
> Target: x86_64-linux-gnu
> 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
> Thread model: posix
> gcc version 9.3.0 (Ubuntu 9.3.0-10ubuntu2) 
> COLLECT_GCC_OPTIONS='-E' '-v' '-mtune=generic' '-march=x86-64'
>  /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
> ignoring duplicate directory "/usr/include/x86_64-linux-gnu/c++/9"
> ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
> ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/9/include-fixed"
> ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include"
> #include "..." search starts here:
> #include <...> search starts here:
>  /usr/include/c++/9
>  /usr/include/x86_64-linux-gnu/c++/9
>  /usr/include/c++/9/backward
>  /usr/lib/gcc/x86_64-linux-gnu/9/include
>  /usr/local/include
>  /usr/include/x86_64-linux-gnu
>  /usr/include
> End of search list.

> 
> I wonder if a better solution wouldn't be to define our own format
> constants in pixelformats.h:

Ayeee ... so yet another standard ... (Ok, so ours would be the same as
DRM ... but ....)

If we go down that route, that feels like it deserves it's own full
format library...

> namespace {
> 
> namespace formats {
> 
> constexpr uint32_t __fourcc(char a, char b, char c, char d)
> {
> 	return (static_cast<uint32_t>(a) <<  0) |
> 	       (static_cast<uint32_t>(b) <<  8) |
> 	       (static_cast<uint32_t>(c) << 16) |
> 	       (static_cast<uint32_t>(d) << 24);
> }
> 
> } /* namespace */
> 
> constexpr PixelFormat RGB888{ __fourcc('R', 'G', '2', '4') };
> ...
> 
> } /* namespace formats */
> 
> The values would match drm_fourcc.h, and the file could even be
> generated from the private copy of drm_fourcc.h. Applications could then
> use formats::RGB888 instead of PixelFormat(DRM_FORMAT_RGB888), which

Well, I do like removing the DRM_FORMAT prefixes from 'libcamera' code...

> could simplify applications that don't need to deal with display.
> Constructing a PixelFormat from a FourCC retrieved from a display device
> would still work fine, so interoperability is guaranteed without the
> need for a translation layer between DRM/KMS FourCCs and libcamera
> formats.
> 
> All this requires some more thoughts (and work). Your patch doesn't
> introduce any regression, so, in the meantime,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

At the moment, external applications can't be compiled against libcamera
at all - so I think this needs some sort of immediate resolution at the
least.

> Please see below for some comments.
> 
>>> Fix up the file locations, and references throughout the project.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> ---
>>>  include/{linux => drm}/drm.h                       | 0
>>>  include/{linux => drm}/drm_fourcc.h                | 0
>>>  include/{linux => drm}/drm_mode.h                  | 0
>>>  include/libcamera/pixelformats.h                   | 2 +-
>>>  src/gstreamer/gstlibcamera-utils.cpp               | 2 +-
>>>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 +-
>>>  src/libcamera/v4l2_pixelformat.cpp                 | 2 +-
>>>  7 files changed, 4 insertions(+), 4 deletions(-)
>>>  rename include/{linux => drm}/drm.h (100%)
>>>  rename include/{linux => drm}/drm_fourcc.h (100%)
>>>  rename include/{linux => drm}/drm_mode.h (100%)
>>>
>>> diff --git a/include/linux/drm.h b/include/drm/drm.h
>>> similarity index 100%
>>> rename from include/linux/drm.h
>>> rename to include/drm/drm.h
>>> diff --git a/include/linux/drm_fourcc.h b/include/drm/drm_fourcc.h
>>> similarity index 100%
>>> rename from include/linux/drm_fourcc.h
>>> rename to include/drm/drm_fourcc.h
>>> diff --git a/include/linux/drm_mode.h b/include/drm/drm_mode.h
>>> similarity index 100%
>>> rename from include/linux/drm_mode.h
>>> rename to include/drm/drm_mode.h
>>> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
>>> index 89966e5e664c..e3cdb711b828 100644
>>> --- a/include/libcamera/pixelformats.h
>>> +++ b/include/libcamera/pixelformats.h
>>> @@ -11,7 +11,7 @@
>>>  #include <stdint.h>
>>>  #include <string>
>>>  
>>> -#include <linux/drm_fourcc.h>
>>> +#include <drm/drm_fourcc.h>
>>>  
>>>  namespace libcamera {
>>>  
>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
>>> index a3cb0746e012..41ca2b90867d 100644
>>> --- a/src/gstreamer/gstlibcamera-utils.cpp
>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp
>>> @@ -8,7 +8,7 @@
>>>  
>>>  #include "gstlibcamera-utils.h"
>>
>> This header includes libcamera/stream.h, which includes
>> libcamera/pixelformats.h, which includes drm/drm_fourcc.h... so we can
>> drop the drm include below - unless we want to put it there to
>> explicitly show we need it.
> 
> If we decide to move towards a libcamera formats namespace, we will drop
> drm_fourcc.h completely, so we could already do so to be prepared for
> the future. If we decide to keep using drm_fourcc.h, I wonder if it
> should be included in pixelformats.h, or explicitly included by the
> source files that require it. I would then be tempted to leave it here.

Including it does have the benefit of showing that this file is directly
using the drm_fourcc namespaced pixelformats...

> 
>>>  
>>> -#include <linux/drm_fourcc.h>
>>> +#include <drm/drm_fourcc.h>
>>>  
>>>  using namespace libcamera;
>>>  
>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> index 07ca9f5d7f53..198ec295a840 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> @@ -18,7 +18,7 @@
>>>  #include <libcamera/request.h>
>>>  #include <libcamera/stream.h>
>>>  
>>> -#include <linux/drm_fourcc.h>
>>> +#include <drm/drm_fourcc.h>
>>
>> And here, I think we should better include libcamera/pixelformats.h over
>> the drm/drm_fourcc.h anyway.
>>
>> And in fact, libcamera/stream.h already includes
>> libcamera/pixelformats.h as well.
>>
>> Should we include libcamera/pixelformats.h explicitly ?
> 
> I tend to include headers explicitly when using classes that they
> provide instead of relying on indirect includes. The only exception I
> apply to that rule is to not include headers in .cpp files that are
> included by the corresponding .h file already, but that's not a rule I
> would enforce for everybody.
> 
>>>  #include <linux/videodev2.h>
>>>  
>>>  #include "libcamera/internal/camera_sensor.h"
>>> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
>>> index 36776be99e59..1c4a05b37e23 100644
>>> --- a/src/libcamera/v4l2_pixelformat.cpp
>>> +++ b/src/libcamera/v4l2_pixelformat.cpp
>>> @@ -12,7 +12,7 @@
>>>  #include <map>
>>>  #include <string.h>
>>>  
>>> -#include <linux/drm_fourcc.h>
>>> +#include <drm/drm_fourcc.h>
>>
>> This is included in libcamera/pixelformats.h below so isn't needed.
> 
> Same comment as above for gstlibcamera-utils.cpp.

Ok - so no need to remove any further lines, and just the 'fix' to apply.

> 
>>>  #include <libcamera/pixelformats.h>
>>>  
>>>
> 

-- 
Regards
--
Kieran