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

[Laurent Pinchart]

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.

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

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

-- 
Regards,

Laurent Pinchart