[libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace explicit DRM FourCCs with libcamera formats
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jun 17 11:32:13 CEST 2020
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 at 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 at 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list