[libcamera-devel] [IPU3-IPA PATCH] ipu3: Use ChromeOS tuning file paths

[Kieran Bingham]

Hi Laurent,

On 06/08/2021 11:42, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Aug 06, 2021 at 11:22:05AM +0100, Kieran Bingham wrote:
>> During development, the IPA relied upon dupliated tuning file
> 
> Did development run into a shortage of 'C's as well ? :-)
> 
>> information to load as the aiqb.
>>
>> Use the ChromeOS files directly, and remove the libcamera dupliations.
> 
> Same here.
> 
> The commit message could be improved a bit, it's not clear what's
> duplicated and why it should change.

The tuning files are duplicated.

The IPA relied upon duplicated tuning file information. Use the ChromeOS
files directly and remove the libcamera duplications.

What would you change to make it clearer? I can repeat the "tuning
files" if you need?


>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>
>> A separate patch also deletes the duplicated tuning files from this
>> repostiory, but as it's simply a remove of the unused files, and renders
>> as a git-binary patch, I will not post it to the list, but simply apply
>> it and remove the unused files if/when this patch is integrated.
> 
> +1 for not carrying copies of the tuning files.

Precisely, this was a development shortcut, and the time to remove them
had evidently long passed :-)


> 
>>  data/meson.build | 12 ------------
>>  ipu3.cpp         | 13 ++++++++-----
>>  meson.build      |  1 -
>>  3 files changed, 8 insertions(+), 18 deletions(-)
>>  delete mode 100644 data/meson.build
>>
>> diff --git a/data/meson.build b/data/meson.build
>> deleted file mode 100644
>> index 3d6e5f56502a..000000000000
>> --- a/data/meson.build
>> +++ /dev/null
>> @@ -1,12 +0,0 @@
>> -# SPDX-License-Identifier: CC0-1.0
>> -
>> -ipa_data_dir = get_option('datadir') / 'libcamera' / 'ipa'
>> -
>> -ipu3_aiqb_data = files([
>> -    '00ov13858.aiqb',
>> -    '01ov5670.aiqb',
>> -])
>> -
>> -install_data(ipu3_aiqb_data,
>> -             install_dir : ipa_data_dir / 'ipu3')
>> -
>> diff --git a/ipu3.cpp b/ipu3.cpp
>> index 4fce64785e3f..a9923fe7daf4 100644
>> --- a/ipu3.cpp
>> +++ b/ipu3.cpp
>> @@ -94,9 +94,9 @@ int IPAIPU3::init(const IPASettings &settings)
>>  	* or through the configuration interfaces perhaps.
>>  	*/
>>  	std::map<std::string, std::string> aiqb_paths = {
>> -		{ "ov13858", "/usr/share/libcamera/ipa/ipu3/00ov13858.aiqb" },
>> -		{ "ov5670", "/usr/share/libcamera/ipa/ipu3/01ov5670.aiqb" },
>> -		{ "imx258", "/etc/camera/ipu3/00imx258.aiqb" },
>> +		{ "ov13858", "00ov13858.aiqb" },
>> +		{ "ov5670", "01ov5670.aiqb" },
>> +		{ "imx258", "00imx258.aiqb" },
>>  	};
>>  
>>  	LOG(IPAIPU3, Info) << "Initialising IPA IPU3 for "
>> @@ -108,8 +108,11 @@ int IPAIPU3::init(const IPASettings &settings)
>>  		return -EINVAL;
>>  	}
>>  
>> -	LOG(IPAIPU3, Info) << "Using tuning file: " << it->second;
>> -	ret = aiqb_.load(it->second.c_str());
>> +	std::string tuningPath = "/etc/camera/ipu3/";
>> +	std::string tuningFile = tuningPath + it->second;
> 
> A comment to explain that the patch is specific to Chrome OS would be
> nice, in case someone wants to use this on a different platform in the
> future.

There's actually already a comment above this block saying:

	/*
	* Temporary mapping of the sensor name to the AIQB data file.
	*
	* \todo: This mapping table should be handled more generically
	* or through the configuration interfaces perhaps.
	*/


>> +
>> +	LOG(IPAIPU3, Info) << "Using tuning file: " << tuningFile;
> 
> s/://


This would print

 "Using tuning file /etc/camera/ipu3/00imx258.aiqb"
as opposed to
 "Using tuning file: /etc/camera/ipu3/00imx258.aiqb"

The colon is there as a clear demarcation of the separation between
'text' and 'filename'....


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +	ret = aiqb_.load(tuningFile.c_str());
>>  	if (ret) {
>>  		LOG(IPAIPU3, Error) << "Failed to load AIQB";
>>  		return -ENODATA;
>> diff --git a/meson.build b/meson.build
>> index e7d6c6b45208..6ea2793d92b8 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -96,7 +96,6 @@ ipu3_ipa_deps = [
>>  
>>  subdir('aic')
>>  subdir('aiq')
>> -subdir('data')
>>  subdir('src')
>>  subdir('stats')
>>  
>