[libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use appropriate media bus format

[Niklas Söderlund]

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

-- 
Regards,
Niklas Söderlund