[libcamera-devel] [PATCH] libcamera: pipeline: vimc: Fail without an IPA

[Kieran Bingham]

Hi Laurent,

On 16/06/2021 13:33, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Jun 16, 2021 at 10:56:10AM +0100, Kieran Bingham wrote:
>> Registering a camera for VIMC without an IPA will fail later when
>> attempting to configure.
>>
>> The IPA is required for VIMC so fail early if it can't be loaded.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> Assuming this doesn't break any unit test, the change looks good.
> 
>> ---
>>  src/libcamera/pipeline/vimc/vimc.cpp | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index 9ebd723be171..8af0e92012e6 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -431,6 +431,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>>  		data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
>>  	} else {
>>  		LOG(VIMC, Warning) << "no matching IPA found";
>> +		return false;
>>  	}
> 
> I would have rewritten the code as
> 
> 	if (!data->ipa_) {
> 		LOG(VIMC, Warning) << "no matching IPA found";
> 		return false;
> 	}
> 
> 	std::string conf = data->ipa_->configurationFile("vimc.conf");
> 	data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
> 
> and while at it, turned the message to an Error.


That's worth the update indeed.


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>>  
>>  	/* Create and register the camera. */
> 

-- 
Regards
--
Kieran