[libcamera-devel] [RFC PATCH] libcamera: pipeline: vimc: Skip unsupported formats

[Kaaira Gupta]

On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 22/05/2020 21:37, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote:
> >> Older kernels do not support all 'reported' formats. Skip them on those
> >> kernels.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> Marking this as RFC, as I don't actually yet know what the correct
> >> kernel version is to condition against.
> > 
> > I think it will be v5.9 if we're lucky :-)
> 
> 
> I think some progress has been made for 5.7, but I don't know the
> current state. Kaaira might know more here.

Hi! Support for other formats has been added to vimc, but it is a bit
buggy right now as it swaps red and blue channels in the final image. I
have sent a patch to fix that, as soon as it gets accepted, libcamera
can support them as well for newer kernel versions.

> 
> 
> >> Currently, the QCam application can not stream the VIMC Camera for users
> >> of QT Version less that 5.14 as that is when the BGR888 format becomes a
> >> 'preferred' native format.
> >>
> >> Users of QT < 5.14 will find that qcam will chose a 'preferred' format
> >> which is reported as supported by the kernel, when in fact it isn't.
> >>
> >> Ensure that those formats are not reported as supported by the vimc
> >> pipeline handler on older kernels.
> >>
> >>  src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> >> index 68d65bc785b0..78fb0ece21f8 100644
> >> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> >> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >>  	const StreamRoles &roles)
> >>  {
> >>  	CameraConfiguration *config = new VimcCameraConfiguration();
> >> +	VimcCameraData *data = cameraData(camera);
> >>  
> >>  	if (roles.empty())
> >>  		return config;
> >> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
> >>  
> >>  	for (const auto &pixelformat : pixelformats) {
> >> +		/*
> >> +		 * \todo: Update this when the kernel correctly supports other
> >> +		 * reported formats.
> >> +		 */
> >> +		if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) {
> > 
> > I wouldn't hardcode any kernel version until we know when the issue will
> > be fixed. I would remove the unsupported formats from the pixelformats
> > map now, and deal with versioning later.
> 
> Ok, well that will depend upon the current mainline state.
> 
> 
> >> +			if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {
> >> +				LOG(VIMC, Info)
> >> +					<< "Skipping unsupported pixel format"
> >> +					<< pixelformat.first.toString();
> >> +				continue;
> >> +			}
> >> +		}
> >> +
> > 
> > You also need to patch the VimcCameraConfiguration::validate() and
> 
> I started at validate in fact, but couldn't (quickly) get the CameraData
> 
> > PipelineHandlerVimc::configure() functions, as they both use the
> > pixelformats map. One option would be to use
> > StreamConfiguration::formats().pixelformats() in both locations, so only
> > one location would need to deal with kernel versions.
> 
> That makes sense in validate(), but not configure() (which should have
> already been validated()).
> 
> (Context, this is based upon a branch with your patch applied to provide
> the PixelFormat -> MBUS code mapping)
> 
> > 
> >>  		/* The scaler hardcodes a x3 scale-up ratio. */
> >>  		std::vector<SizeRange> sizes{
> >>  			SizeRange{ { 48, 48 }, { 4096, 2160 } }
> > 
> 
> -- 
> Regards
> --
> Kieran