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

[Kieran Bingham]

On 06/08/2021 12:18, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Aug 06, 2021 at 12:07:11PM +0100, Kieran Bingham wrote:
>> On 06/08/2021 11:42, Laurent Pinchart wrote:
>>> 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?
> 
> During development, the IPA relied upon tuning database files copied
> from Chrome OS, duplicated in this repository, and installed in a second
> location on the target, in /usr/share/libcamera/ipa/ipu3/. Having two
> copies of the same files on the target devices is not only pointless,
> it also causes an issue with some Soraka models that use a different
> sensor module, with a different tuning file.
> 
> Use the tuning files from Chrome OS directly and drop our copies.
> 
> 
> 
> And I think you can delete the copies in the same patch.

Sure - as stated below - I can do that - but not post it. Deleting the
binary files generates a large binary diff in the patch, which doesn't
need to hit the list.



>>>> 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
> 
> I meant path, not patch.
> 
>>> 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
> 
> s/://
> 
> (I seem to dislike colons these day)
> 
>> 	* or through the configuration interfaces perhaps.
>> 	*/
> 
> Can you extend this to indicate that the /etc/camera/ipu3/ path is
> specific to Chrome OS ?
> 
>>>> +
>>>> +	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"
> 
> That's what I intended, yes :-)
> 
>> The colon is there as a clear demarcation of the separation between
>> 'text' and 'filename'....
> 
> Probably a matter of personal preference then ? I think it reads better
> with the colon.

... Perhaps - And now I'm confused. I also think it reads better with
the colon, that's why I put it there, and why I queried your suggested
removal.

So I guess I can leave it there if you prefer it ?

--
Kieran


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