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

[David Plowman]

Hi Laurent

On Wed, 22 Dec 2021 at 10:49, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> (CC'ing Sakari)
>
> 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?

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