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

[Kieran Bingham]

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.


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