| Message ID | 20200324131247.20771-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote: > Pick the correct media bus format based on the video pixel format on the > capture node. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > This patch, while being correct (I think :-)), shouldn't be integrated, > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the > viewfinder reports the following supported native formats (in that > order): > > - DRM_FORMAT_ABGR8888 > - DRM_FORMAT_ARGB8888 > - DRM_FORMAT_BGR888 > - DRM_FORMAT_RGB888 > > The first two formats are not supported by the VIMC pipeline handler, > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. > > On Qt < 5.14.0, the third format isn't supported, so the fourth format, > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline > configuration, with the debayering subdev source pad being configured > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support > that, but in drivers/media/platform/vimc/vimc-debayer.c the > vimc_deb_set_fmt() function handles the source pad format with > > /* > * Do not change the format of the source pad, > * it is propagated from the sink > */ > if (VIMC_IS_SRC(fmt->pad)) { > fmt->format = *sink_fmt; > /* TODO: Add support for other formats */ > fmt->format.code = vdeb->src_code; > } > > This needs to be fixed on the kernel side. > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index b04a9726efa5..ace58381fe92 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -6,8 +6,8 @@ > */ > > #include <algorithm> > -#include <array> > #include <iomanip> > +#include <map> > #include <tuple> > > #include <linux/media-bus-format.h> > @@ -103,10 +103,10 @@ private: > > namespace { > > -static const std::array<PixelFormat, 3> pixelformats{ > - PixelFormat(DRM_FORMAT_RGB888), > - PixelFormat(DRM_FORMAT_BGR888), > - PixelFormat(DRM_FORMAT_BGRA8888), > +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 }, > + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, This looks odd BGRA8888 mapping to ARGB8888, should that not be DRM_FORMAT_ABGR8888? Also should the 4th bus format listed in the commit message be added to this map? > }; > > } /* namespace */ > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > StreamConfiguration &cfg = config_[0]; > > /* Adjust the pixel format. */ > - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == > - pixelformats.end()) { > + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { > LOG(VIMC, Debug) << "Adjusting format to RGB24"; > cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > status = Adjusted; > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > std::map<PixelFormat, std::vector<SizeRange>> formats; > > - for (PixelFormat pixelformat : pixelformats) { > + for (const auto &pixelformat : pixelformats) { > /* The scaler hardcodes a x3 scale-up ratio. */ > std::vector<SizeRange> sizes{ > SizeRange{ { 48, 48 }, { 4096, 2160 } } > }; > - formats[pixelformat] = sizes; > + formats[pixelformat.first] = sizes; > } > > StreamConfiguration cfg(formats); > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > if (ret) > return ret; > > - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; > + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; > ret = data->debayer_->setFormat(1, &subformat); > if (ret) > return ret; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Apr 08, 2020 at 12:47:08AM +0200, Niklas Söderlund wrote: > On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote: > > Pick the correct media bus format based on the video pixel format on the > > capture node. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > This patch, while being correct (I think :-)), shouldn't be integrated, > > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the > > viewfinder reports the following supported native formats (in that > > order): > > > > - DRM_FORMAT_ABGR8888 > > - DRM_FORMAT_ARGB8888 > > - DRM_FORMAT_BGR888 > > - DRM_FORMAT_RGB888 > > > > The first two formats are not supported by the VIMC pipeline handler, > > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, > > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. > > > > On Qt < 5.14.0, the third format isn't supported, so the fourth format, > > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline > > configuration, with the debayering subdev source pad being configured > > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support > > that, but in drivers/media/platform/vimc/vimc-debayer.c the > > vimc_deb_set_fmt() function handles the source pad format with > > > > /* > > * Do not change the format of the source pad, > > * it is propagated from the sink > > */ > > if (VIMC_IS_SRC(fmt->pad)) { > > fmt->format = *sink_fmt; > > /* TODO: Add support for other formats */ > > fmt->format.code = vdeb->src_code; > > } > > > > This needs to be fixed on the kernel side. > > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index b04a9726efa5..ace58381fe92 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -6,8 +6,8 @@ > > */ > > > > #include <algorithm> > > -#include <array> > > #include <iomanip> > > +#include <map> > > #include <tuple> > > > > #include <linux/media-bus-format.h> > > @@ -103,10 +103,10 @@ private: > > > > namespace { > > > > -static const std::array<PixelFormat, 3> pixelformats{ > > - PixelFormat(DRM_FORMAT_RGB888), > > - PixelFormat(DRM_FORMAT_BGR888), > > - PixelFormat(DRM_FORMAT_BGRA8888), > > +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 }, > > + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, > > This looks odd BGRA8888 mapping to ARGB8888, should that not be > DRM_FORMAT_ABGR8888? The vimc drivers maps MEDIA_BUS_FMT_ARGB8888_1X32 to V4L2_PIX_FMT_ARGB32, which corresponds to DRM_FORMAT_BGRA8888. The mapping may be considered awkward, but it matches the driver implementation. > Also should the 4th bus format listed in the commit message be added > to this map? Now I'm confused. The commit message lists DRM pixel formats, not bus formats. Is that what you meant ? The fourth one is DRM_FORMAT_RGB888, which is listed here, but maybe you meant the missing one from the four ? That would be DRM_FORMAT_ABGR8888, which corresponds to V4L2_PIX_FMT_RGBA32, which is not supported by vimc. I've thus left it out from this patch. > > }; > > > > } /* namespace */ > > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > StreamConfiguration &cfg = config_[0]; > > > > /* Adjust the pixel format. */ > > - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == > > - pixelformats.end()) { > > + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { > > LOG(VIMC, Debug) << "Adjusting format to RGB24"; > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > status = Adjusted; > > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > > std::map<PixelFormat, std::vector<SizeRange>> formats; > > > > - for (PixelFormat pixelformat : pixelformats) { > > + for (const auto &pixelformat : pixelformats) { > > /* The scaler hardcodes a x3 scale-up ratio. */ > > std::vector<SizeRange> sizes{ > > SizeRange{ { 48, 48 }, { 4096, 2160 } } > > }; > > - formats[pixelformat] = sizes; > > + formats[pixelformat.first] = sizes; > > } > > > > StreamConfiguration cfg(formats); > > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > > if (ret) > > return ret; > > > > - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; > > + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; > > ret = data->debayer_->setFormat(1, &subformat); > > if (ret) > > return ret;
Hi Laurent, On 2020-04-08 03:39:59 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Wed, Apr 08, 2020 at 12:47:08AM +0200, Niklas Söderlund wrote: > > On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote: > > > Pick the correct media bus format based on the video pixel format on the > > > capture node. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- > > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > > > This patch, while being correct (I think :-)), shouldn't be integrated, > > > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the > > > viewfinder reports the following supported native formats (in that > > > order): > > > > > > - DRM_FORMAT_ABGR8888 > > > - DRM_FORMAT_ARGB8888 > > > - DRM_FORMAT_BGR888 > > > - DRM_FORMAT_RGB888 > > > > > > The first two formats are not supported by the VIMC pipeline handler, > > > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, > > > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. > > > > > > On Qt < 5.14.0, the third format isn't supported, so the fourth format, > > > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline > > > configuration, with the debayering subdev source pad being configured > > > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support > > > that, but in drivers/media/platform/vimc/vimc-debayer.c the > > > vimc_deb_set_fmt() function handles the source pad format with > > > > > > /* > > > * Do not change the format of the source pad, > > > * it is propagated from the sink > > > */ > > > if (VIMC_IS_SRC(fmt->pad)) { > > > fmt->format = *sink_fmt; > > > /* TODO: Add support for other formats */ > > > fmt->format.code = vdeb->src_code; > > > } > > > > > > This needs to be fixed on the kernel side. > > > > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > > index b04a9726efa5..ace58381fe92 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -6,8 +6,8 @@ > > > */ > > > > > > #include <algorithm> > > > -#include <array> > > > #include <iomanip> > > > +#include <map> > > > #include <tuple> > > > > > > #include <linux/media-bus-format.h> > > > @@ -103,10 +103,10 @@ private: > > > > > > namespace { > > > > > > -static const std::array<PixelFormat, 3> pixelformats{ > > > - PixelFormat(DRM_FORMAT_RGB888), > > > - PixelFormat(DRM_FORMAT_BGR888), > > > - PixelFormat(DRM_FORMAT_BGRA8888), > > > +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 }, > > > + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, > > > > This looks odd BGRA8888 mapping to ARGB8888, should that not be > > DRM_FORMAT_ABGR8888? > > The vimc drivers maps MEDIA_BUS_FMT_ARGB8888_1X32 to > V4L2_PIX_FMT_ARGB32, which corresponds to DRM_FORMAT_BGRA8888. The > mapping may be considered awkward, but it matches the driver > implementation. I see, thanks for pointing it out. > > > Also should the 4th bus format listed in the commit message be added > > to this map? > > Now I'm confused. The commit message lists DRM pixel formats, not bus > formats. Is that what you meant ? Yes, sorry for saying bus format instead :-) > The fourth one is DRM_FORMAT_RGB888, > which is listed here, but maybe you meant the missing one from the four > ? Yes I was referring to the missing one from the list, not the 4th one. > That would be DRM_FORMAT_ABGR8888, which corresponds to > V4L2_PIX_FMT_RGBA32, which is not supported by vimc. I've thus left it > out from this patch. Sorry I was confused. I assumed vimc would would support it as the commit message stated 'viewfinder reports' and that was missing was support for it in the pipeline handler. Thus I asked if support for it should have been added. I now see what you mean, sorry for not verifying this in the vimc sources before sending my review. > > > > }; > > > > > > } /* namespace */ > > > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > > StreamConfiguration &cfg = config_[0]; > > > > > > /* Adjust the pixel format. */ > > > - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == > > > - pixelformats.end()) { > > > + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { > > > LOG(VIMC, Debug) << "Adjusting format to RGB24"; > > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > > status = Adjusted; > > > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > > > > std::map<PixelFormat, std::vector<SizeRange>> formats; > > > > > > - for (PixelFormat pixelformat : pixelformats) { > > > + for (const auto &pixelformat : pixelformats) { > > > /* The scaler hardcodes a x3 scale-up ratio. */ > > > std::vector<SizeRange> sizes{ > > > SizeRange{ { 48, 48 }, { 4096, 2160 } } > > > }; > > > - formats[pixelformat] = sizes; > > > + formats[pixelformat.first] = sizes; > > > } > > > > > > StreamConfiguration cfg(formats); > > > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > > > if (ret) > > > return ret; > > > > > > - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; > > > + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; > > > ret = data->debayer_->setFormat(1, &subformat); > > > if (ret) > > > return ret; > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 24/03/2020 13:12, Laurent Pinchart wrote: > Pick the correct media bus format based on the video pixel format on the > capture node. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > This patch, while being correct (I think :-)), shouldn't be integrated, > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the > viewfinder reports the following supported native formats (in that > order): In fact, VIMC camera support is currently broken anyway at the moment, (as per "[RFC PATCH] libcamera: pipeline: vimc: Skip unsupported formats") and I believe applying this patch doesn't actually break anything further. It does however help progress development of the VIMC pipeline handler for supporting extra formats when they are available, and as such I think it should be merged already ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > - DRM_FORMAT_ABGR8888 > - DRM_FORMAT_ARGB8888 > - DRM_FORMAT_BGR888 > - DRM_FORMAT_RGB888 > > The first two formats are not supported by the VIMC pipeline handler, > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. > > On Qt < 5.14.0, the third format isn't supported, so the fourth format, > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline > configuration, with the debayering subdev source pad being configured > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support > that, but in drivers/media/platform/vimc/vimc-debayer.c the > vimc_deb_set_fmt() function handles the source pad format with I wonder if the issue you describe there is a symptom of / fixed by: "[PATCH] qcam: viewfinder: Use correct DRM/QImage mappings" ... -- Kieran > > /* > * Do not change the format of the source pad, > * it is propagated from the sink > */ > if (VIMC_IS_SRC(fmt->pad)) { > fmt->format = *sink_fmt; > /* TODO: Add support for other formats */ > fmt->format.code = vdeb->src_code; > } > > This needs to be fixed on the kernel side. > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index b04a9726efa5..ace58381fe92 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -6,8 +6,8 @@ > */ > > #include <algorithm> > -#include <array> > #include <iomanip> > +#include <map> > #include <tuple> > > #include <linux/media-bus-format.h> > @@ -103,10 +103,10 @@ private: > > namespace { > > -static const std::array<PixelFormat, 3> pixelformats{ > - PixelFormat(DRM_FORMAT_RGB888), > - PixelFormat(DRM_FORMAT_BGR888), > - PixelFormat(DRM_FORMAT_BGRA8888), > +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 }, > + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, > }; > > } /* namespace */ > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > StreamConfiguration &cfg = config_[0]; > > /* Adjust the pixel format. */ > - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == > - pixelformats.end()) { > + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { > LOG(VIMC, Debug) << "Adjusting format to RGB24"; > cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > status = Adjusted; > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > std::map<PixelFormat, std::vector<SizeRange>> formats; > > - for (PixelFormat pixelformat : pixelformats) { > + for (const auto &pixelformat : pixelformats) { > /* The scaler hardcodes a x3 scale-up ratio. */ > std::vector<SizeRange> sizes{ > SizeRange{ { 48, 48 }, { 4096, 2160 } } > }; > - formats[pixelformat] = sizes; > + formats[pixelformat.first] = sizes; > } > > StreamConfiguration cfg(formats); > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > if (ret) > return ret; > > - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; > + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; > ret = data->debayer_->setFormat(1, &subformat); > if (ret) > return ret; >
Hi Kieran, On Thu, May 28, 2020 at 10:13:21AM +0100, Kieran Bingham wrote: > On 24/03/2020 13:12, Laurent Pinchart wrote: > > Pick the correct media bus format based on the video pixel format on the > > capture node. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > This patch, while being correct (I think :-)), shouldn't be integrated, > > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the > > viewfinder reports the following supported native formats (in that > > order): > > In fact, VIMC camera support is currently broken anyway at the moment, > (as per "[RFC PATCH] libcamera: pipeline: vimc: Skip unsupported > formats") and I believe applying this patch doesn't actually break > anything further. > > It does however help progress development of the VIMC pipeline handler > for supporting extra formats when they are available, and as such I > think it should be merged already ;-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I've now merged the patch. Do you plan to submit a new version of your vimc pipeline handler fix ? > > - DRM_FORMAT_ABGR8888 > > - DRM_FORMAT_ARGB8888 > > - DRM_FORMAT_BGR888 > > - DRM_FORMAT_RGB888 > > > > The first two formats are not supported by the VIMC pipeline handler, > > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, > > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. > > > > On Qt < 5.14.0, the third format isn't supported, so the fourth format, > > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline > > configuration, with the debayering subdev source pad being configured > > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support > > that, but in drivers/media/platform/vimc/vimc-debayer.c the > > vimc_deb_set_fmt() function handles the source pad format with > > I wonder if the issue you describe there is a symptom of / fixed by: > "[PATCH] qcam: viewfinder: Use correct DRM/QImage mappings" ... > > > > > /* > > * Do not change the format of the source pad, > > * it is propagated from the sink > > */ > > if (VIMC_IS_SRC(fmt->pad)) { > > fmt->format = *sink_fmt; > > /* TODO: Add support for other formats */ > > fmt->format.code = vdeb->src_code; > > } > > > > This needs to be fixed on the kernel side. > > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index b04a9726efa5..ace58381fe92 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -6,8 +6,8 @@ > > */ > > > > #include <algorithm> > > -#include <array> > > #include <iomanip> > > +#include <map> > > #include <tuple> > > > > #include <linux/media-bus-format.h> > > @@ -103,10 +103,10 @@ private: > > > > namespace { > > > > -static const std::array<PixelFormat, 3> pixelformats{ > > - PixelFormat(DRM_FORMAT_RGB888), > > - PixelFormat(DRM_FORMAT_BGR888), > > - PixelFormat(DRM_FORMAT_BGRA8888), > > +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 }, > > + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, > > }; > > > > } /* namespace */ > > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > > StreamConfiguration &cfg = config_[0]; > > > > /* Adjust the pixel format. */ > > - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == > > - pixelformats.end()) { > > + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { > > LOG(VIMC, Debug) << "Adjusting format to RGB24"; > > cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); > > status = Adjusted; > > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > > > std::map<PixelFormat, std::vector<SizeRange>> formats; > > > > - for (PixelFormat pixelformat : pixelformats) { > > + for (const auto &pixelformat : pixelformats) { > > /* The scaler hardcodes a x3 scale-up ratio. */ > > std::vector<SizeRange> sizes{ > > SizeRange{ { 48, 48 }, { 4096, 2160 } } > > }; > > - formats[pixelformat] = sizes; > > + formats[pixelformat.first] = sizes; > > } > > > > StreamConfiguration cfg(formats); > > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > > if (ret) > > return ret; > > > > - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; > > + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; > > ret = data->debayer_->setFormat(1, &subformat); > > if (ret) > > return ret;
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index b04a9726efa5..ace58381fe92 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -6,8 +6,8 @@ */ #include <algorithm> -#include <array> #include <iomanip> +#include <map> #include <tuple> #include <linux/media-bus-format.h> @@ -103,10 +103,10 @@ private: namespace { -static const std::array<PixelFormat, 3> pixelformats{ - PixelFormat(DRM_FORMAT_RGB888), - PixelFormat(DRM_FORMAT_BGR888), - PixelFormat(DRM_FORMAT_BGRA8888), +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 }, + { PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 }, }; } /* namespace */ @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() StreamConfiguration &cfg = config_[0]; /* Adjust the pixel format. */ - if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) == - pixelformats.end()) { + if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) { LOG(VIMC, Debug) << "Adjusting format to RGB24"; cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888); status = Adjusted; @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, std::map<PixelFormat, std::vector<SizeRange>> formats; - for (PixelFormat pixelformat : pixelformats) { + for (const auto &pixelformat : pixelformats) { /* The scaler hardcodes a x3 scale-up ratio. */ std::vector<SizeRange> sizes{ SizeRange{ { 48, 48 }, { 4096, 2160 } } }; - formats[pixelformat] = sizes; + formats[pixelformat.first] = sizes; } StreamConfiguration cfg(formats); @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) if (ret) return ret; - subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24; + subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second; ret = data->debayer_->setFormat(1, &subformat); if (ret) return ret;
Pick the correct media bus format based on the video pixel format on the capture node. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/vimc.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) This patch, while being correct (I think :-)), shouldn't be integrated, as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the viewfinder reports the following supported native formats (in that order): - DRM_FORMAT_ABGR8888 - DRM_FORMAT_ARGB8888 - DRM_FORMAT_BGR888 - DRM_FORMAT_RGB888 The first two formats are not supported by the VIMC pipeline handler, the last two are. The third format, DRM_FORMAT_BGR888, is thus picked, which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today. On Qt < 5.14.0, the third format isn't supported, so the fourth format, DRM_FORMAT_RGB888, will be picked. This results in a different pipeline configuration, with the debayering subdev source pad being configured with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support that, but in drivers/media/platform/vimc/vimc-debayer.c the vimc_deb_set_fmt() function handles the source pad format with /* * Do not change the format of the source pad, * it is propagated from the sink */ if (VIMC_IS_SRC(fmt->pad)) { fmt->format = *sink_fmt; /* TODO: Add support for other formats */ fmt->format.code = vdeb->src_code; } This needs to be fixed on the kernel side.