[libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Add tuning file for IMX296 sensor

[Laurent Pinchart]

Hi David,

On Wed, Dec 22, 2021 at 12:12:41PM +0000, David Plowman wrote:
> On Wed, 22 Dec 2021 at 10:49, Laurent Pinchart wrote:
> > On Wed, Dec 22, 2021 at 10:26:48AM +0000, David Plowman wrote:
> > > On Wed, 22 Dec 2021 at 09:43, Laurent Pinchart wrote:
> > > > On Wed, Dec 22, 2021 at 08:18:03AM +0000, David Plowman wrote:
> > > > > On Tue, 21 Dec 2021 at 23:39, Laurent Pinchart wrote:
> > > > > > On Mon, Dec 20, 2021 at 08:58:45AM +0000, David Plowman wrote:
> > > > > > > On Mon, 20 Dec 2021 at 07:43, Naushir Patuck wrote:
> > > > > > > > On Sun, 19 Dec 2021 at 23:27, Laurent Pinchart wrote:
> > > > > > > >>
> > > > > > > >> From: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > >>
> > > > > > > >> The Sony IMX296 exists in two versions, colour (Bayer) and monochrome.
> > > > > > > >> The tuning file corresponds to the colour version.
> > > > > > > >>
> > > > > > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > > >
> > > > > > > > We might want to do a second pass of tuning at some point, but this is a good
> > > > > > > > starting point.
> > > > > > > >
> > > > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > >
> > > > > > > >> ---
> > > > > > > >>  src/ipa/raspberrypi/data/imx296.json | 305 +++++++++++++++++++++++++++
> > > > > > > >>  src/ipa/raspberrypi/data/meson.build |   1 +
> > > > > > > >>  2 files changed, 306 insertions(+)
> > > > > > > >>  create mode 100644 src/ipa/raspberrypi/data/imx296.json
> > > > > > > >>
> > > > > > > >> diff --git a/src/ipa/raspberrypi/data/imx296.json b/src/ipa/raspberrypi/data/imx296.json
> > > > > > > >> new file mode 100644
> > > > > > > >> index 000000000000..2444bd2eb940
> > > > > > > >> --- /dev/null
> > > > > > > >> +++ b/src/ipa/raspberrypi/data/imx296.json
> > > > > > > >> @@ -0,0 +1,305 @@
> > > >
> > > > [snip]
> > > >
> > > > > > > >> +    "rpi.sharpen":
> > > > > > > >> +    {
> > > > > > >
> > > > > > > Default sharpening tends to come out a bit strong the fewer megapixels
> > > > > > > there are (another thing to look into one day...), so reducing the
> > > > > > > strength here is probably advisable.
> > > > > > >
> > > > > > > So in summary, yes, I think another pass at this would be beneficial
> > > > > > > but it will work "well enough" initially, so:
> > > > > > >
> > > > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > >
> > > > > > The IMX296 sensors I have here are the monochrome version, so I can't
> > > > > > really calibrate the colour-related parameters. This tuning file comes
> > > > > > from a branch that was shared by Naush.
> > > > > >
> > > > > > Can I merge it as-is and let you play with your fancy calibration kit to
> > > > > > update the tuning data ? :-)
> > > > >
> > > > > Yes, of course, always happy to do that!
> > > >
> > > > Thank you.
> > > >
> > > > > I'm wondering if we should keep this file as a "colour module" tuning
> > > > > and create another one for the mono version with the colour bits
> > > > > removed, but I can take care of all that later. Does the driver know
> > > > > which kind of module it has (or is it hard-coded)? If so we could
> > > > > perhaps automate the selection of the correct tuning file if it
> > > > > advertises a suitably amended driver name. Alternatively we just rely
> > > > > on the environment variable workaround.
> > > >
> > > > That's a good idea. The driver has the information, and exposes it
> > > > through the media bus codes (Y10 for the monochrome version, SBGGR10 for
> > > > the colour version). The subdev and entity name is the same for both
> > > > versions.
> > > >
> > > > Do you think we should expose different entity names ? We could use
> > > > imx296ll for the monchrome version and imx296lq for the colour version.
> > > > That would however require two separate camera sensor helpers, which I
> > > > don't really like. We may be able to refactor the code to allow camera
> > > > helpers to match different names. Another option is to keep the entity
> > > > name unchanged, and derive the tuning file name from the media bus code.
> > > >
> > > > What do you think would be the best approach ?
> > >
> > > In the Raspberry Pi cam helper world you can put more than one
> > > "RegisterCamHelper"s at the bottom of the cam helper file. That should
> > > "just work". I'm a bit nervous about joining the mbus code into the
> > > tuning file name, when in the long run the answer is going to have to
> > > be that users/applications will specify the tuning file name for
> > > themselves. But I don't feel too strongly as it's all a bit
> > > imperfect...
> >
> > I don't think we should just join them, but we may possibly deribe a
> > -mono or -colour suffix for instance. I'm not entirely sure how that
> > would work and what issues it could cause, it feels like a bit of a
> > hack.
> >
> > On the other hand, if we convey this information through the entity
> > name, I'm also not sure where we'll stop. Will we need different entity
> > names for sensors with or without an IR filter for instance ?
> 
> Indeed, nothing is perfect. In the end there's no way around the fact
> that you can't determine the attached camera module automatically, we
> have to rely on the user/application to tell us.
> 
> So I'm fine with leaving the driver name as "imx296", we make the mono
> tuning file be "imx296.json" (you got there first!), and everyone else
> will just have to supply the right name if they have a colour (or in
> any way different) module.
> 
> Does that work?

That, or at that point we'll figure a scheme to select the right file
automatically. In any case, we can address this issue when it arises.

> > > > > > > >> +    }
> > > > > > > >> +}
> > > > > > > >> diff --git a/src/ipa/raspberrypi/data/meson.build b/src/ipa/raspberrypi/data/meson.build
> > > > > > > >> index e84cd0990c31..211811cfa915 100644
> > > > > > > >> --- a/src/ipa/raspberrypi/data/meson.build
> > > > > > > >> +++ b/src/ipa/raspberrypi/data/meson.build
> > > > > > > >> @@ -4,6 +4,7 @@ conf_files = files([
> > > > > > > >>      'imx219.json',
> > > > > > > >>      'imx219_noir.json',
> > > > > > > >>      'imx290.json',
> > > > > > > >> +    'imx296.json',
> > > > > > > >>      'imx378.json',
> > > > > > > >>      'imx477.json',
> > > > > > > >>      'imx477_noir.json',

-- 
Regards,

Laurent Pinchart