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

[Laurent Pinchart]

Hi Kieran,

On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote:
> On 22/05/2020 21:37, Laurent Pinchart wrote:
> > 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.
> 
> >> 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.

Sure. If v5.7 already supports multiple formats then we should deal with
it now, but otherwise we can defer the version handling until we know in
which kernel version it will be available.

> >> +			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()).

You're right, my bad. Only validate() needs to be addressed in addition
to generateConfiguration().

> (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,

Laurent Pinchart