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

[Laurent Pinchart]

Hi David,

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 ?

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