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

Tue May 19 16:30:12 CEST 2020

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 wonder if a better solution wouldn't be to define our own format
constants in pixelformats.h:

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
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>

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.

> >  
> > -#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.

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

-- 
Regards,

Laurent Pinchart