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

[Laurent Pinchart]

Hi Kieran,

On Fri, Aug 06, 2021 at 01:12:08PM +0100, Kieran Bingham wrote:
> On 06/08/2021 12:18, Laurent Pinchart wrote:
> > 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 ?

Sorry, I meant without the colon :-) In my head it's like "I've painted
the bikeshed in red" vs. "I've painted the bikeshed in: red". The latter
doesn't have the same natural flow. It would be different for "Bikeshed
colour: red".

Feel free to keep or drop the colon, as you prefer.

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