| Message ID | 20200610130339.22998-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote: > Use the new pixel format constants to replace usage of macros from > drm_fourcc.h. > > The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific > formats that are not defined in the libcamera public API. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v2: > > - Drop include drm_fourcc.h from IPU3 pipeline handler > > Changes since v1: > > - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- > src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- > 4 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index b805fea71c2d..f0428b1baed4 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -14,6 +14,7 @@ > #include <linux/media-bus-format.h> > > #include <libcamera/camera.h> > +#include <libcamera/formats.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) > class IPU3CameraData; > > static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { > - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, > - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, > - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, > - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, > + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, > + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, > + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, > + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, > }; > > class ImgUDevice > @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) > { > /* The only pixel format the driver supports is NV12. */ > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > + cfg.pixelFormat = formats::NV12; > > if (scale) { > /* > @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > const Size size = cfg.size; > const IPU3Stream *stream; > > - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) > + if (cfg.pixelFormat.modifier()) I wonder if this will work, with this the raw stream would be picked for any format with a modifier. I'm thinking about applications erroneously configuring the raw stream for CSI2 packed layout instead of IPU3 and the validate would accept it. I think we need to keep the drm_fourcc dependency here verify the modifier is the one for IPU3 layout. > stream = &data_->rawStream_; > else if (cfg.size == sensorFormat_.size) > stream = &data_->outStream_; > @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > StreamConfiguration cfg = {}; > IPU3Stream *stream = nullptr; > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > + cfg.pixelFormat = formats::NV12; > > switch (role) { > case StreamRole::StillCapture: > @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > return 0; > > *outputFormat = {}; > - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); > + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); > outputFormat->size = cfg.size; > outputFormat->planesCount = 2; > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7f23666ea8f4..60985b716833 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -13,12 +13,12 @@ > > #include <libcamera/camera.h> > #include <libcamera/control_ids.h> > +#include <libcamera/formats.h> > #include <libcamera/ipa/raspberrypi.h> > #include <libcamera/logging.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > -#include <linux/drm_fourcc.h> > #include <linux/videodev2.h> > > #include "libcamera/internal/camera_sensor.h" > @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { > /* If we cannot find a native format, use a default one. */ > - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); > + cfgPixFmt = formats::NV12; > status = Adjusted; > } > } > @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > break; > > case StreamRole::StillCapture: > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > + cfg.pixelFormat = formats::NV12; > /* Return the largest sensor resolution. */ > cfg.size = data->sensor_->resolution(); > cfg.bufferCount = 1; > break; > > case StreamRole::VideoRecording: > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > + cfg.pixelFormat = formats::NV12; > cfg.size = { 1920, 1080 }; > cfg.bufferCount = 4; > break; > > case StreamRole::Viewfinder: > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); > + cfg.pixelFormat = formats::ARGB8888; > cfg.size = { 800, 600 }; > cfg.bufferCount = 4; > break; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 900f873ab028..e439f3149bce 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/buffer.h> > #include <libcamera/camera.h> > #include <libcamera/control_ids.h> > +#include <libcamera/formats.h> > #include <libcamera/ipa/rkisp1.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > { > static const std::array<PixelFormat, 8> formats{ > - PixelFormat(DRM_FORMAT_YUYV), > - PixelFormat(DRM_FORMAT_YVYU), > - PixelFormat(DRM_FORMAT_VYUY), > - PixelFormat(DRM_FORMAT_NV16), > - PixelFormat(DRM_FORMAT_NV61), > - PixelFormat(DRM_FORMAT_NV21), > - PixelFormat(DRM_FORMAT_NV12), > + formats::YUYV, > + formats::YVYU, > + formats::VYUY, > + formats::NV16, > + formats::NV61, > + formats::NV21, > + formats::NV12, > /* \todo Add support for 8-bit greyscale to DRM formats */ > }; > > @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == > formats.end()) { > LOG(RkISP1, Debug) << "Adjusting format to NV12"; > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), > + cfg.pixelFormat = formats::NV12, > status = Adjusted; > } > > @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > return config; > > StreamConfiguration cfg{}; > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > + cfg.pixelFormat = formats::NV12; > cfg.size = data->sensor_->resolution(); > > config->addConfiguration(cfg); > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 3881545b8a53..b6530662a9ba 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -17,6 +17,7 @@ > #include <libcamera/camera.h> > #include <libcamera/control_ids.h> > #include <libcamera/controls.h> > +#include <libcamera/formats.h> > #include <libcamera/ipa/ipa_interface.h> > #include <libcamera/ipa/ipa_module_info.h> > #include <libcamera/request.h> > @@ -108,8 +109,8 @@ private: > namespace { > > static const std::map<PixelFormat, uint32_t> pixelformats{ > - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, > - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, > + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, > + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, > }; > > } /* namespace */ > @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { > LOG(VIMC, Debug) << "Adjusting format to BGR888"; > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > + cfg.pixelFormat = formats::BGR888; > status = Adjusted; > } > > @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > * but it isn't functional within the pipeline. > */ > if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { > - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > + if (pixelformat.first != formats::BGR888) { > LOG(VIMC, Info) > << "Skipping unsupported pixel format " > << pixelformat.first.toString(); > @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > StreamConfiguration cfg(formats); > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > + cfg.pixelFormat = formats::BGR888; > cfg.size = { 1920, 1080 }; > cfg.bufferCount = 4; > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote: > On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote: > > Use the new pixel format constants to replace usage of macros from > > drm_fourcc.h. > > > > The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific > > formats that are not defined in the libcamera public API. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v2: > > > > - Drop include drm_fourcc.h from IPU3 pipeline handler > > > > Changes since v1: > > > > - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- > > src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- > > 4 files changed, 30 insertions(+), 27 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index b805fea71c2d..f0428b1baed4 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -14,6 +14,7 @@ > > #include <linux/media-bus-format.h> > > > > #include <libcamera/camera.h> > > +#include <libcamera/formats.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) > > class IPU3CameraData; > > > > static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { > > - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, > > - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, > > - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, > > - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, > > + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, > > + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, > > + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, > > + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, > > }; > > > > class ImgUDevice > > @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, > > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) > > { > > /* The only pixel format the driver supports is NV12. */ > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > + cfg.pixelFormat = formats::NV12; > > > > if (scale) { > > /* > > @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > const Size size = cfg.size; > > const IPU3Stream *stream; > > > > - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) > > + if (cfg.pixelFormat.modifier()) > > I wonder if this will work, with this the raw stream would be picked for > any format with a modifier. I'm thinking about applications erroneously > configuring the raw stream for CSI2 packed layout instead of IPU3 and > the validate would accept it. We have this code further down in the function: if (stream->raw_) { const auto &itFormat = sensorMbusToPixel.find(sensorFormat_.mbus_code); if (itFormat == sensorMbusToPixel.end()) return Invalid; cfg.pixelFormat = itFormat->second; cfg.size = sensorFormat_.size; } I think validate() thus correctly updates the pixel format. Is there something I'm missing ? > I think we need to keep the drm_fourcc dependency here verify the > modifier is the one for IPU3 layout. What should happen if it's not ? Isn't is better to map a CSI-2 packed format, even if not supported, to the raw stream, than to the viewfinder or output stream as done today ? Another option would be to check the fourcc value to see if it matches one of the bayer formats, and ignoring the modifier. > > stream = &data_->rawStream_; > > else if (cfg.size == sensorFormat_.size) > > stream = &data_->outStream_; > > @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > StreamConfiguration cfg = {}; > > IPU3Stream *stream = nullptr; > > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > + cfg.pixelFormat = formats::NV12; > > > > switch (role) { > > case StreamRole::StillCapture: > > @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > > return 0; > > > > *outputFormat = {}; > > - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); > > + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); > > outputFormat->size = cfg.size; > > outputFormat->planesCount = 2; > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7f23666ea8f4..60985b716833 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -13,12 +13,12 @@ > > > > #include <libcamera/camera.h> > > #include <libcamera/control_ids.h> > > +#include <libcamera/formats.h> > > #include <libcamera/ipa/raspberrypi.h> > > #include <libcamera/logging.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > -#include <linux/drm_fourcc.h> > > #include <linux/videodev2.h> > > > > #include "libcamera/internal/camera_sensor.h" > > @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > > > if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { > > /* If we cannot find a native format, use a default one. */ > > - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); > > + cfgPixFmt = formats::NV12; > > status = Adjusted; > > } > > } > > @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > break; > > > > case StreamRole::StillCapture: > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > + cfg.pixelFormat = formats::NV12; > > /* Return the largest sensor resolution. */ > > cfg.size = data->sensor_->resolution(); > > cfg.bufferCount = 1; > > break; > > > > case StreamRole::VideoRecording: > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > + cfg.pixelFormat = formats::NV12; > > cfg.size = { 1920, 1080 }; > > cfg.bufferCount = 4; > > break; > > > > case StreamRole::Viewfinder: > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); > > + cfg.pixelFormat = formats::ARGB8888; > > cfg.size = { 800, 600 }; > > cfg.bufferCount = 4; > > break; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 900f873ab028..e439f3149bce 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/buffer.h> > > #include <libcamera/camera.h> > > #include <libcamera/control_ids.h> > > +#include <libcamera/formats.h> > > #include <libcamera/ipa/rkisp1.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > { > > static const std::array<PixelFormat, 8> formats{ > > - PixelFormat(DRM_FORMAT_YUYV), > > - PixelFormat(DRM_FORMAT_YVYU), > > - PixelFormat(DRM_FORMAT_VYUY), > > - PixelFormat(DRM_FORMAT_NV16), > > - PixelFormat(DRM_FORMAT_NV61), > > - PixelFormat(DRM_FORMAT_NV21), > > - PixelFormat(DRM_FORMAT_NV12), > > + formats::YUYV, > > + formats::YVYU, > > + formats::VYUY, > > + formats::NV16, > > + formats::NV61, > > + formats::NV21, > > + formats::NV12, > > /* \todo Add support for 8-bit greyscale to DRM formats */ > > }; > > > > @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == > > formats.end()) { > > LOG(RkISP1, Debug) << "Adjusting format to NV12"; > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), > > + cfg.pixelFormat = formats::NV12, > > status = Adjusted; > > } > > > > @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > return config; > > > > StreamConfiguration cfg{}; > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > + cfg.pixelFormat = formats::NV12; > > cfg.size = data->sensor_->resolution(); > > > > config->addConfiguration(cfg); > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > > index 3881545b8a53..b6530662a9ba 100644 > > --- a/src/libcamera/pipeline/vimc/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > > @@ -17,6 +17,7 @@ > > #include <libcamera/camera.h> > > #include <libcamera/control_ids.h> > > #include <libcamera/controls.h> > > +#include <libcamera/formats.h> > > #include <libcamera/ipa/ipa_interface.h> > > #include <libcamera/ipa/ipa_module_info.h> > > #include <libcamera/request.h> > > @@ -108,8 +109,8 @@ private: > > namespace { > > > > static const std::map<PixelFormat, uint32_t> pixelformats{ > > - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, > > - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, > > + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, > > + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, > > }; > > > > } /* namespace */ > > @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); > > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { > > LOG(VIMC, Debug) << "Adjusting format to BGR888"; > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > + cfg.pixelFormat = formats::BGR888; > > status = Adjusted; > > } > > > > @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > * but it isn't functional within the pipeline. > > */ > > if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { > > - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > > + if (pixelformat.first != formats::BGR888) { > > LOG(VIMC, Info) > > << "Skipping unsupported pixel format " > > << pixelformat.first.toString(); > > @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > > StreamConfiguration cfg(formats); > > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > + cfg.pixelFormat = formats::BGR888; > > cfg.size = { 1920, 1080 }; > > cfg.bufferCount = 4; > >
Hi Laurent, On 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote: > > On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote: > > > Use the new pixel format constants to replace usage of macros from > > > drm_fourcc.h. > > > > > > The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific > > > formats that are not defined in the libcamera public API. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Changes since v2: > > > > > > - Drop include drm_fourcc.h from IPU3 pipeline handler > > > > > > Changes since v1: > > > > > > - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- > > > src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- > > > 4 files changed, 30 insertions(+), 27 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index b805fea71c2d..f0428b1baed4 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -14,6 +14,7 @@ > > > #include <linux/media-bus-format.h> > > > > > > #include <libcamera/camera.h> > > > +#include <libcamera/formats.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) > > > class IPU3CameraData; > > > > > > static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { > > > - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, > > > - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, > > > - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, > > > - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, > > > }; > > > > > > class ImgUDevice > > > @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, > > > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) > > > { > > > /* The only pixel format the driver supports is NV12. */ > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > + cfg.pixelFormat = formats::NV12; > > > > > > if (scale) { > > > /* > > > @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > const Size size = cfg.size; > > > const IPU3Stream *stream; > > > > > > - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) > > > + if (cfg.pixelFormat.modifier()) > > > > I wonder if this will work, with this the raw stream would be picked for > > any format with a modifier. I'm thinking about applications erroneously > > configuring the raw stream for CSI2 packed layout instead of IPU3 and > > the validate would accept it. > > We have this code further down in the function: > > if (stream->raw_) { > const auto &itFormat = > sensorMbusToPixel.find(sensorFormat_.mbus_code); > if (itFormat == sensorMbusToPixel.end()) > return Invalid; > > cfg.pixelFormat = itFormat->second; > cfg.size = sensorFormat_.size; > } You are correct, thanks for pointing this out. Please add my, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > I think validate() thus correctly updates the pixel format. Is there > something I'm missing ? > > > I think we need to keep the drm_fourcc dependency here verify the > > modifier is the one for IPU3 layout. > > What should happen if it's not ? Isn't is better to map a CSI-2 packed > format, even if not supported, to the raw stream, than to the viewfinder > or output stream as done today ? > > Another option would be to check the fourcc value to see if it matches > one of the bayer formats, and ignoring the modifier. > > > > stream = &data_->rawStream_; > > > else if (cfg.size == sensorFormat_.size) > > > stream = &data_->outStream_; > > > @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > StreamConfiguration cfg = {}; > > > IPU3Stream *stream = nullptr; > > > > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > + cfg.pixelFormat = formats::NV12; > > > > > > switch (role) { > > > case StreamRole::StillCapture: > > > @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > > > return 0; > > > > > > *outputFormat = {}; > > > - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); > > > + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); > > > outputFormat->size = cfg.size; > > > outputFormat->planesCount = 2; > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 7f23666ea8f4..60985b716833 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -13,12 +13,12 @@ > > > > > > #include <libcamera/camera.h> > > > #include <libcamera/control_ids.h> > > > +#include <libcamera/formats.h> > > > #include <libcamera/ipa/raspberrypi.h> > > > #include <libcamera/logging.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > -#include <linux/drm_fourcc.h> > > > #include <linux/videodev2.h> > > > > > > #include "libcamera/internal/camera_sensor.h" > > > @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > > > > > if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { > > > /* If we cannot find a native format, use a default one. */ > > > - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); > > > + cfgPixFmt = formats::NV12; > > > status = Adjusted; > > > } > > > } > > > @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > break; > > > > > > case StreamRole::StillCapture: > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > + cfg.pixelFormat = formats::NV12; > > > /* Return the largest sensor resolution. */ > > > cfg.size = data->sensor_->resolution(); > > > cfg.bufferCount = 1; > > > break; > > > > > > case StreamRole::VideoRecording: > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > + cfg.pixelFormat = formats::NV12; > > > cfg.size = { 1920, 1080 }; > > > cfg.bufferCount = 4; > > > break; > > > > > > case StreamRole::Viewfinder: > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); > > > + cfg.pixelFormat = formats::ARGB8888; > > > cfg.size = { 800, 600 }; > > > cfg.bufferCount = 4; > > > break; > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 900f873ab028..e439f3149bce 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -16,6 +16,7 @@ > > > #include <libcamera/buffer.h> > > > #include <libcamera/camera.h> > > > #include <libcamera/control_ids.h> > > > +#include <libcamera/formats.h> > > > #include <libcamera/ipa/rkisp1.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > { > > > static const std::array<PixelFormat, 8> formats{ > > > - PixelFormat(DRM_FORMAT_YUYV), > > > - PixelFormat(DRM_FORMAT_YVYU), > > > - PixelFormat(DRM_FORMAT_VYUY), > > > - PixelFormat(DRM_FORMAT_NV16), > > > - PixelFormat(DRM_FORMAT_NV61), > > > - PixelFormat(DRM_FORMAT_NV21), > > > - PixelFormat(DRM_FORMAT_NV12), > > > + formats::YUYV, > > > + formats::YVYU, > > > + formats::VYUY, > > > + formats::NV16, > > > + formats::NV61, > > > + formats::NV21, > > > + formats::NV12, > > > /* \todo Add support for 8-bit greyscale to DRM formats */ > > > }; > > > > > > @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == > > > formats.end()) { > > > LOG(RkISP1, Debug) << "Adjusting format to NV12"; > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), > > > + cfg.pixelFormat = formats::NV12, > > > status = Adjusted; > > > } > > > > > > @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > return config; > > > > > > StreamConfiguration cfg{}; > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > > + cfg.pixelFormat = formats::NV12; > > > cfg.size = data->sensor_->resolution(); > > > > > > config->addConfiguration(cfg); > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > > > index 3881545b8a53..b6530662a9ba 100644 > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > > > @@ -17,6 +17,7 @@ > > > #include <libcamera/camera.h> > > > #include <libcamera/control_ids.h> > > > #include <libcamera/controls.h> > > > +#include <libcamera/formats.h> > > > #include <libcamera/ipa/ipa_interface.h> > > > #include <libcamera/ipa/ipa_module_info.h> > > > #include <libcamera/request.h> > > > @@ -108,8 +109,8 @@ private: > > > namespace { > > > > > > static const std::map<PixelFormat, uint32_t> pixelformats{ > > > - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, > > > - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, > > > + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, > > > + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, > > > }; > > > > > > } /* namespace */ > > > @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > > const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); > > > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { > > > LOG(VIMC, Debug) << "Adjusting format to BGR888"; > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > > + cfg.pixelFormat = formats::BGR888; > > > status = Adjusted; > > > } > > > > > > @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > * but it isn't functional within the pipeline. > > > */ > > > if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { > > > - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > > > + if (pixelformat.first != formats::BGR888) { > > > LOG(VIMC, Info) > > > << "Skipping unsupported pixel format " > > > << pixelformat.first.toString(); > > > @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > > > > StreamConfiguration cfg(formats); > > > > > > - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > > + cfg.pixelFormat = formats::BGR888; > > > cfg.size = { 1920, 1080 }; > > > cfg.bufferCount = 4; > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Tue, Jun 16, 2020 at 05:29:52PM +0200, Niklas Söderlund wrote: > On 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote: > > On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote: > >> On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote: > >>> Use the new pixel format constants to replace usage of macros from > >>> drm_fourcc.h. > >>> > >>> The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific > >>> formats that are not defined in the libcamera public API. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> Changes since v2: > >>> > >>> - Drop include drm_fourcc.h from IPU3 pipeline handler > >>> > >>> Changes since v1: > >>> > >>> - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler > >>> --- > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- > >>> src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- > >>> 4 files changed, 30 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> index b805fea71c2d..f0428b1baed4 100644 > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> @@ -14,6 +14,7 @@ > >>> #include <linux/media-bus-format.h> > >>> > >>> #include <libcamera/camera.h> > >>> +#include <libcamera/formats.h> > >>> #include <libcamera/request.h> > >>> #include <libcamera/stream.h> > >>> > >>> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) > >>> class IPU3CameraData; > >>> > >>> static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { > >>> - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, > >>> - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, > >>> - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, > >>> - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, > >>> + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, > >>> + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, > >>> + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, > >>> + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, > >>> }; > >>> > >>> class ImgUDevice > >>> @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, > >>> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) > >>> { > >>> /* The only pixel format the driver supports is NV12. */ > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > >>> + cfg.pixelFormat = formats::NV12; > >>> > >>> if (scale) { > >>> /* > >>> @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > >>> const Size size = cfg.size; > >>> const IPU3Stream *stream; > >>> > >>> - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) > >>> + if (cfg.pixelFormat.modifier()) > >> > >> I wonder if this will work, with this the raw stream would be picked for > >> any format with a modifier. I'm thinking about applications erroneously > >> configuring the raw stream for CSI2 packed layout instead of IPU3 and > >> the validate would accept it. > > > > We have this code further down in the function: > > > > if (stream->raw_) { > > const auto &itFormat = > > sensorMbusToPixel.find(sensorFormat_.mbus_code); > > if (itFormat == sensorMbusToPixel.end()) > > return Invalid; > > > > cfg.pixelFormat = itFormat->second; > > cfg.size = sensorFormat_.size; > > } > > You are correct, thanks for pointing this out. Please add my, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Would you prefer the following though (to be folded in this patch) ? diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f0428b1baed4..93faf8e19e29 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -364,10 +364,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() for (unsigned int i = 0; i < config_.size(); ++i) { StreamConfiguration &cfg = config_[i]; const PixelFormat pixelFormat = cfg.pixelFormat; + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); const Size size = cfg.size; const IPU3Stream *stream; - if (cfg.pixelFormat.modifier()) + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) stream = &data_->rawStream_; else if (cfg.size == sensorFormat_.size) stream = &data_->outStream_; A tad bit more expensive at runtime, but more explicit. Let me know which version you like best. > > I think validate() thus correctly updates the pixel format. Is there > > something I'm missing ? > > > >> I think we need to keep the drm_fourcc dependency here verify the > >> modifier is the one for IPU3 layout. > > > > What should happen if it's not ? Isn't is better to map a CSI-2 packed > > format, even if not supported, to the raw stream, than to the viewfinder > > or output stream as done today ? > > > > Another option would be to check the fourcc value to see if it matches > > one of the bayer formats, and ignoring the modifier. > > > >>> stream = &data_->rawStream_; > >>> else if (cfg.size == sensorFormat_.size) > >>> stream = &data_->outStream_; > >>> @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > >>> StreamConfiguration cfg = {}; > >>> IPU3Stream *stream = nullptr; > >>> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > >>> + cfg.pixelFormat = formats::NV12; > >>> > >>> switch (role) { > >>> case StreamRole::StillCapture: > >>> @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > >>> return 0; > >>> > >>> *outputFormat = {}; > >>> - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); > >>> + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); > >>> outputFormat->size = cfg.size; > >>> outputFormat->planesCount = 2; > >>> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> index 7f23666ea8f4..60985b716833 100644 > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> @@ -13,12 +13,12 @@ > >>> > >>> #include <libcamera/camera.h> > >>> #include <libcamera/control_ids.h> > >>> +#include <libcamera/formats.h> > >>> #include <libcamera/ipa/raspberrypi.h> > >>> #include <libcamera/logging.h> > >>> #include <libcamera/request.h> > >>> #include <libcamera/stream.h> > >>> > >>> -#include <linux/drm_fourcc.h> > >>> #include <linux/videodev2.h> > >>> > >>> #include "libcamera/internal/camera_sensor.h" > >>> @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > >>> > >>> if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { > >>> /* If we cannot find a native format, use a default one. */ > >>> - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); > >>> + cfgPixFmt = formats::NV12; > >>> status = Adjusted; > >>> } > >>> } > >>> @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > >>> break; > >>> > >>> case StreamRole::StillCapture: > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > >>> + cfg.pixelFormat = formats::NV12; > >>> /* Return the largest sensor resolution. */ > >>> cfg.size = data->sensor_->resolution(); > >>> cfg.bufferCount = 1; > >>> break; > >>> > >>> case StreamRole::VideoRecording: > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > >>> + cfg.pixelFormat = formats::NV12; > >>> cfg.size = { 1920, 1080 }; > >>> cfg.bufferCount = 4; > >>> break; > >>> > >>> case StreamRole::Viewfinder: > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); > >>> + cfg.pixelFormat = formats::ARGB8888; > >>> cfg.size = { 800, 600 }; > >>> cfg.bufferCount = 4; > >>> break; > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> index 900f873ab028..e439f3149bce 100644 > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> @@ -16,6 +16,7 @@ > >>> #include <libcamera/buffer.h> > >>> #include <libcamera/camera.h> > >>> #include <libcamera/control_ids.h> > >>> +#include <libcamera/formats.h> > >>> #include <libcamera/ipa/rkisp1.h> > >>> #include <libcamera/request.h> > >>> #include <libcamera/stream.h> > >>> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > >>> CameraConfiguration::Status RkISP1CameraConfiguration::validate() > >>> { > >>> static const std::array<PixelFormat, 8> formats{ > >>> - PixelFormat(DRM_FORMAT_YUYV), > >>> - PixelFormat(DRM_FORMAT_YVYU), > >>> - PixelFormat(DRM_FORMAT_VYUY), > >>> - PixelFormat(DRM_FORMAT_NV16), > >>> - PixelFormat(DRM_FORMAT_NV61), > >>> - PixelFormat(DRM_FORMAT_NV21), > >>> - PixelFormat(DRM_FORMAT_NV12), > >>> + formats::YUYV, > >>> + formats::YVYU, > >>> + formats::VYUY, > >>> + formats::NV16, > >>> + formats::NV61, > >>> + formats::NV21, > >>> + formats::NV12, > >>> /* \todo Add support for 8-bit greyscale to DRM formats */ > >>> }; > >>> > >>> @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > >>> if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == > >>> formats.end()) { > >>> LOG(RkISP1, Debug) << "Adjusting format to NV12"; > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), > >>> + cfg.pixelFormat = formats::NV12, > >>> status = Adjusted; > >>> } > >>> > >>> @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > >>> return config; > >>> > >>> StreamConfiguration cfg{}; > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > >>> + cfg.pixelFormat = formats::NV12; > >>> cfg.size = data->sensor_->resolution(); > >>> > >>> config->addConfiguration(cfg); > >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > >>> index 3881545b8a53..b6530662a9ba 100644 > >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp > >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > >>> @@ -17,6 +17,7 @@ > >>> #include <libcamera/camera.h> > >>> #include <libcamera/control_ids.h> > >>> #include <libcamera/controls.h> > >>> +#include <libcamera/formats.h> > >>> #include <libcamera/ipa/ipa_interface.h> > >>> #include <libcamera/ipa/ipa_module_info.h> > >>> #include <libcamera/request.h> > >>> @@ -108,8 +109,8 @@ private: > >>> namespace { > >>> > >>> static const std::map<PixelFormat, uint32_t> pixelformats{ > >>> - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, > >>> - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, > >>> + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, > >>> + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, > >>> }; > >>> > >>> } /* namespace */ > >>> @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > >>> const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); > >>> if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { > >>> LOG(VIMC, Debug) << "Adjusting format to BGR888"; > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > >>> + cfg.pixelFormat = formats::BGR888; > >>> status = Adjusted; > >>> } > >>> > >>> @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >>> * but it isn't functional within the pipeline. > >>> */ > >>> if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { > >>> - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > >>> + if (pixelformat.first != formats::BGR888) { > >>> LOG(VIMC, Info) > >>> << "Skipping unsupported pixel format " > >>> << pixelformat.first.toString(); > >>> @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >>> > >>> StreamConfiguration cfg(formats); > >>> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > >>> + cfg.pixelFormat = formats::BGR888; > >>> cfg.size = { 1920, 1080 }; > >>> cfg.bufferCount = 4; > >>>
Hi Laurent, On 2020-06-16 21:55:06 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Tue, Jun 16, 2020 at 05:29:52PM +0200, Niklas Söderlund wrote: > > On 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote: > > > On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote: > > >> On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote: > > >>> Use the new pixel format constants to replace usage of macros from > > >>> drm_fourcc.h. > > >>> > > >>> The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific > > >>> formats that are not defined in the libcamera public API. > > >>> > > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>> --- > > >>> Changes since v2: > > >>> > > >>> - Drop include drm_fourcc.h from IPU3 pipeline handler > > >>> > > >>> Changes since v1: > > >>> > > >>> - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler > > >>> --- > > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- > > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- > > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- > > >>> src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- > > >>> 4 files changed, 30 insertions(+), 27 deletions(-) > > >>> > > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>> index b805fea71c2d..f0428b1baed4 100644 > > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>> @@ -14,6 +14,7 @@ > > >>> #include <linux/media-bus-format.h> > > >>> > > >>> #include <libcamera/camera.h> > > >>> +#include <libcamera/formats.h> > > >>> #include <libcamera/request.h> > > >>> #include <libcamera/stream.h> > > >>> > > >>> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) > > >>> class IPU3CameraData; > > >>> > > >>> static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { > > >>> - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, > > >>> - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, > > >>> - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, > > >>> - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, > > >>> + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, > > >>> + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, > > >>> + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, > > >>> + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, > > >>> }; > > >>> > > >>> class ImgUDevice > > >>> @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, > > >>> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) > > >>> { > > >>> /* The only pixel format the driver supports is NV12. */ > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfg.pixelFormat = formats::NV12; > > >>> > > >>> if (scale) { > > >>> /* > > >>> @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > >>> const Size size = cfg.size; > > >>> const IPU3Stream *stream; > > >>> > > >>> - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) > > >>> + if (cfg.pixelFormat.modifier()) > > >> > > >> I wonder if this will work, with this the raw stream would be picked for > > >> any format with a modifier. I'm thinking about applications erroneously > > >> configuring the raw stream for CSI2 packed layout instead of IPU3 and > > >> the validate would accept it. > > > > > > We have this code further down in the function: > > > > > > if (stream->raw_) { > > > const auto &itFormat = > > > sensorMbusToPixel.find(sensorFormat_.mbus_code); > > > if (itFormat == sensorMbusToPixel.end()) > > > return Invalid; > > > > > > cfg.pixelFormat = itFormat->second; > > > cfg.size = sensorFormat_.size; > > > } > > > > You are correct, thanks for pointing this out. Please add my, > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Would you prefer the following though (to be folded in this patch) ? > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f0428b1baed4..93faf8e19e29 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -364,10 +364,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > for (unsigned int i = 0; i < config_.size(); ++i) { > StreamConfiguration &cfg = config_[i]; > const PixelFormat pixelFormat = cfg.pixelFormat; > + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > const Size size = cfg.size; > const IPU3Stream *stream; > > - if (cfg.pixelFormat.modifier()) > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > stream = &data_->rawStream_; > else if (cfg.size == sensorFormat_.size) > stream = &data_->outStream_; > > A tad bit more expensive at runtime, but more explicit. Let me know > which version you like best. I like this changed. Both versions are OK with me with this extra twist gaining bonus points. If you squash it in feel free to keep my R-b. > > > > I think validate() thus correctly updates the pixel format. Is there > > > something I'm missing ? > > > > > >> I think we need to keep the drm_fourcc dependency here verify the > > >> modifier is the one for IPU3 layout. > > > > > > What should happen if it's not ? Isn't is better to map a CSI-2 packed > > > format, even if not supported, to the raw stream, than to the viewfinder > > > or output stream as done today ? > > > > > > Another option would be to check the fourcc value to see if it matches > > > one of the bayer formats, and ignoring the modifier. > > > > > >>> stream = &data_->rawStream_; > > >>> else if (cfg.size == sensorFormat_.size) > > >>> stream = &data_->outStream_; > > >>> @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > >>> StreamConfiguration cfg = {}; > > >>> IPU3Stream *stream = nullptr; > > >>> > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfg.pixelFormat = formats::NV12; > > >>> > > >>> switch (role) { > > >>> case StreamRole::StillCapture: > > >>> @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, > > >>> return 0; > > >>> > > >>> *outputFormat = {}; > > >>> - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); > > >>> + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); > > >>> outputFormat->size = cfg.size; > > >>> outputFormat->planesCount = 2; > > >>> > > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> index 7f23666ea8f4..60985b716833 100644 > > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> @@ -13,12 +13,12 @@ > > >>> > > >>> #include <libcamera/camera.h> > > >>> #include <libcamera/control_ids.h> > > >>> +#include <libcamera/formats.h> > > >>> #include <libcamera/ipa/raspberrypi.h> > > >>> #include <libcamera/logging.h> > > >>> #include <libcamera/request.h> > > >>> #include <libcamera/stream.h> > > >>> > > >>> -#include <linux/drm_fourcc.h> > > >>> #include <linux/videodev2.h> > > >>> > > >>> #include "libcamera/internal/camera_sensor.h" > > >>> @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > >>> > > >>> if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { > > >>> /* If we cannot find a native format, use a default one. */ > > >>> - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfgPixFmt = formats::NV12; > > >>> status = Adjusted; > > >>> } > > >>> } > > >>> @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > >>> break; > > >>> > > >>> case StreamRole::StillCapture: > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfg.pixelFormat = formats::NV12; > > >>> /* Return the largest sensor resolution. */ > > >>> cfg.size = data->sensor_->resolution(); > > >>> cfg.bufferCount = 1; > > >>> break; > > >>> > > >>> case StreamRole::VideoRecording: > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfg.pixelFormat = formats::NV12; > > >>> cfg.size = { 1920, 1080 }; > > >>> cfg.bufferCount = 4; > > >>> break; > > >>> > > >>> case StreamRole::Viewfinder: > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); > > >>> + cfg.pixelFormat = formats::ARGB8888; > > >>> cfg.size = { 800, 600 }; > > >>> cfg.bufferCount = 4; > > >>> break; > > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> index 900f873ab028..e439f3149bce 100644 > > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> @@ -16,6 +16,7 @@ > > >>> #include <libcamera/buffer.h> > > >>> #include <libcamera/camera.h> > > >>> #include <libcamera/control_ids.h> > > >>> +#include <libcamera/formats.h> > > >>> #include <libcamera/ipa/rkisp1.h> > > >>> #include <libcamera/request.h> > > >>> #include <libcamera/stream.h> > > >>> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > >>> CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > >>> { > > >>> static const std::array<PixelFormat, 8> formats{ > > >>> - PixelFormat(DRM_FORMAT_YUYV), > > >>> - PixelFormat(DRM_FORMAT_YVYU), > > >>> - PixelFormat(DRM_FORMAT_VYUY), > > >>> - PixelFormat(DRM_FORMAT_NV16), > > >>> - PixelFormat(DRM_FORMAT_NV61), > > >>> - PixelFormat(DRM_FORMAT_NV21), > > >>> - PixelFormat(DRM_FORMAT_NV12), > > >>> + formats::YUYV, > > >>> + formats::YVYU, > > >>> + formats::VYUY, > > >>> + formats::NV16, > > >>> + formats::NV61, > > >>> + formats::NV21, > > >>> + formats::NV12, > > >>> /* \todo Add support for 8-bit greyscale to DRM formats */ > > >>> }; > > >>> > > >>> @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > >>> if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == > > >>> formats.end()) { > > >>> LOG(RkISP1, Debug) << "Adjusting format to NV12"; > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), > > >>> + cfg.pixelFormat = formats::NV12, > > >>> status = Adjusted; > > >>> } > > >>> > > >>> @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > >>> return config; > > >>> > > >>> StreamConfiguration cfg{}; > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); > > >>> + cfg.pixelFormat = formats::NV12; > > >>> cfg.size = data->sensor_->resolution(); > > >>> > > >>> config->addConfiguration(cfg); > > >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > > >>> index 3881545b8a53..b6530662a9ba 100644 > > >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp > > >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > > >>> @@ -17,6 +17,7 @@ > > >>> #include <libcamera/camera.h> > > >>> #include <libcamera/control_ids.h> > > >>> #include <libcamera/controls.h> > > >>> +#include <libcamera/formats.h> > > >>> #include <libcamera/ipa/ipa_interface.h> > > >>> #include <libcamera/ipa/ipa_module_info.h> > > >>> #include <libcamera/request.h> > > >>> @@ -108,8 +109,8 @@ private: > > >>> namespace { > > >>> > > >>> static const std::map<PixelFormat, uint32_t> pixelformats{ > > >>> - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, > > >>> - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, > > >>> + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, > > >>> + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, > > >>> }; > > >>> > > >>> } /* namespace */ > > >>> @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > >>> const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); > > >>> if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { > > >>> LOG(VIMC, Debug) << "Adjusting format to BGR888"; > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > >>> + cfg.pixelFormat = formats::BGR888; > > >>> status = Adjusted; > > >>> } > > >>> > > >>> @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > >>> * but it isn't functional within the pipeline. > > >>> */ > > >>> if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { > > >>> - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > > >>> + if (pixelformat.first != formats::BGR888) { > > >>> LOG(VIMC, Info) > > >>> << "Skipping unsupported pixel format " > > >>> << pixelformat.first.toString(); > > >>> @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > >>> > > >>> StreamConfiguration cfg(formats); > > >>> > > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > >>> + cfg.pixelFormat = formats::BGR888; > > >>> cfg.size = { 1920, 1080 }; > > >>> cfg.bufferCount = 4; > > >>> > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index b805fea71c2d..f0428b1baed4 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -14,6 +14,7 @@ #include <linux/media-bus-format.h> #include <libcamera/camera.h> +#include <libcamera/formats.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3) class IPU3CameraData; static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = { - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) }, - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) }, - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) }, - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) }, + { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 }, + { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 }, + { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 }, + { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 }, }; class ImgUDevice @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera, void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale) { /* The only pixel format the driver supports is NV12. */ - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); + cfg.pixelFormat = formats::NV12; if (scale) { /* @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() const Size size = cfg.size; const IPU3Stream *stream; - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED) + if (cfg.pixelFormat.modifier()) stream = &data_->rawStream_; else if (cfg.size == sensorFormat_.size) stream = &data_->outStream_; @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, StreamConfiguration cfg = {}; IPU3Stream *stream = nullptr; - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); + cfg.pixelFormat = formats::NV12; switch (role) { case StreamRole::StillCapture: @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output, return 0; *outputFormat = {}; - outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12)); + outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12); outputFormat->size = cfg.size; outputFormat->planesCount = 2; diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 7f23666ea8f4..60985b716833 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -13,12 +13,12 @@ #include <libcamera/camera.h> #include <libcamera/control_ids.h> +#include <libcamera/formats.h> #include <libcamera/ipa/raspberrypi.h> #include <libcamera/logging.h> #include <libcamera/request.h> #include <libcamera/stream.h> -#include <linux/drm_fourcc.h> #include <linux/videodev2.h> #include "libcamera/internal/camera_sensor.h" @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) { /* If we cannot find a native format, use a default one. */ - cfgPixFmt = PixelFormat(DRM_FORMAT_NV12); + cfgPixFmt = formats::NV12; status = Adjusted; } } @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, break; case StreamRole::StillCapture: - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); + cfg.pixelFormat = formats::NV12; /* Return the largest sensor resolution. */ cfg.size = data->sensor_->resolution(); cfg.bufferCount = 1; break; case StreamRole::VideoRecording: - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); + cfg.pixelFormat = formats::NV12; cfg.size = { 1920, 1080 }; cfg.bufferCount = 4; break; case StreamRole::Viewfinder: - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888); + cfg.pixelFormat = formats::ARGB8888; cfg.size = { 800, 600 }; cfg.bufferCount = 4; break; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 900f873ab028..e439f3149bce 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -16,6 +16,7 @@ #include <libcamera/buffer.h> #include <libcamera/camera.h> #include <libcamera/control_ids.h> +#include <libcamera/formats.h> #include <libcamera/ipa/rkisp1.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, CameraConfiguration::Status RkISP1CameraConfiguration::validate() { static const std::array<PixelFormat, 8> formats{ - PixelFormat(DRM_FORMAT_YUYV), - PixelFormat(DRM_FORMAT_YVYU), - PixelFormat(DRM_FORMAT_VYUY), - PixelFormat(DRM_FORMAT_NV16), - PixelFormat(DRM_FORMAT_NV61), - PixelFormat(DRM_FORMAT_NV21), - PixelFormat(DRM_FORMAT_NV12), + formats::YUYV, + formats::YVYU, + formats::VYUY, + formats::NV16, + formats::NV61, + formats::NV21, + formats::NV12, /* \todo Add support for 8-bit greyscale to DRM formats */ }; @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { LOG(RkISP1, Debug) << "Adjusting format to NV12"; - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12), + cfg.pixelFormat = formats::NV12, status = Adjusted; } @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera return config; StreamConfiguration cfg{}; - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12); + cfg.pixelFormat = formats::NV12; cfg.size = data->sensor_->resolution(); config->addConfiguration(cfg); diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index 3881545b8a53..b6530662a9ba 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -17,6 +17,7 @@ #include <libcamera/camera.h> #include <libcamera/control_ids.h> #include <libcamera/controls.h> +#include <libcamera/formats.h> #include <libcamera/ipa/ipa_interface.h> #include <libcamera/ipa/ipa_module_info.h> #include <libcamera/request.h> @@ -108,8 +109,8 @@ private: namespace { static const std::map<PixelFormat, uint32_t> pixelformats{ - { PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 }, - { PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 }, + { formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 }, + { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 }, }; } /* namespace */ @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() const std::vector<libcamera::PixelFormat> formats = cfg.formats().pixelformats(); if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) { LOG(VIMC, Debug) << "Adjusting format to BGR888"; - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); + cfg.pixelFormat = formats::BGR888; status = Adjusted; } @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, * but it isn't functional within the pipeline. */ if (data->media_->version() < KERNEL_VERSION(5, 7, 0)) { - if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { + if (pixelformat.first != formats::BGR888) { LOG(VIMC, Info) << "Skipping unsupported pixel format " << pixelformat.first.toString(); @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, StreamConfiguration cfg(formats); - cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); + cfg.pixelFormat = formats::BGR888; cfg.size = { 1920, 1080 }; cfg.bufferCount = 4;
Use the new pixel format constants to replace usage of macros from drm_fourcc.h. The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific formats that are not defined in the libcamera public API. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v2: - Drop include drm_fourcc.h from IPU3 pipeline handler Changes since v1: - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler --- src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++-------- .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++----- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++--------- src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++----- 4 files changed, 30 insertions(+), 27 deletions(-)