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

[David Plowman]

Hi Laurent

On Wed, 22 Dec 2021 at 09:43, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

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

David

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