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

15170 diff mbox series

From: Naushir Patuck <naush@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@ideasonboard.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

Hi Laurent,

Thank you for your work.

On Sun, 19 Dec 2021 at 23:27, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> From: Naushir Patuck <naush@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@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 <naushir@gmail.com>@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush <naushir@gmail.com>@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 @@
> +{
> +    "rpi.black_level":
> +    {
> +        "black_level": 4096
> +    },
> +    "rpi.dpc":
> +    {
> +    },
> +    "rpi.lux":
> +    {
> +        "reference_shutter_speed": 19184,
> +        "reference_gain": 1.0,
> +        "reference_aperture": 1.0,
> +        "reference_lux": 432,
> +        "reference_Y": 13773
> +    },
> +    "rpi.noise":
> +    {
> +        "reference_constant": 0,
> +        "reference_slope": 2.957
> +    },
> +    "rpi.geq":
> +    {
> +        "offset": 185,
> +        "slope": 0.0105
> +    },
> +    "rpi.sdn":
> +    {
> +    },
> +    "rpi.awb":
> +    {
> +        "priors":
> +        [
> +            {
> +                "lux": 0, "prior":
> +                [
> +                    2000, 1.0, 3000, 0.0, 13000, 0.0
> +                ]
> +            },
> +            {
> +                "lux": 800, "prior":
> +                [
> +                    2000, 0.0, 6000, 2.0, 13000, 2.0
> +                ]
> +            },
> +            {
> +                "lux": 1500, "prior":
> +                [
> +                    2000, 0.0, 4000, 1.0, 6000, 6.0, 6500, 7.0, 7000,
> 1.0, 13000, 1.0
> +                ]
> +            }
> +        ],
> +        "modes":
> +        {
> +            "auto":
> +            {
> +                "lo": 2500,
> +                "hi": 8000
> +            },
> +            "incandescent":
> +            {
> +                "lo": 2500,
> +                "hi": 3000
> +            },
> +            "tungsten":
> +            {
> +                "lo": 3000,
> +                "hi": 3500
> +            },
> +            "fluorescent":
> +            {
> +                "lo": 4000,
> +                "hi": 4700
> +            },
> +            "indoor":
> +            {
> +                "lo": 3000,
> +                "hi": 5000
> +            },
> +            "daylight":
> +            {
> +                "lo": 5500,
> +                "hi": 6500
> +            },
> +            "cloudy":
> +            {
> +                "lo": 7000,
> +                "hi": 8600
> +            }
> +        },
> +        "bayes": 1,
> +        "ct_curve":
> +        [
> +            2960.0, 0.5114, 0.3294, 3720.0, 0.4222, 0.4164, 3770.0,
> 0.4194, 0.4194, 6040.0, 0.2871, 0.5702, 8450.0,
> +            0.2285, 0.6454
> +        ],
> +        "sensitivity_r": 1.0,
> +        "sensitivity_b": 1.0,
> +        "transverse_pos": 0.01,
> +        "transverse_neg": 0.01157
> +    },
> +    "rpi.agc":
> +    {
> +        "metering_modes":
> +        {
> +            "centre-weighted":
> +            {
> +                "weights":
> +                [
> +                    3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0
> +                ]
> +            },
> +            "spot":
> +            {
> +                "weights":
> +                [
> +                    2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
> +                ]
> +            },
> +            "matrix":
> +            {
> +                "weights":
> +                [
> +                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
> +                ]
> +            }
> +        },
> +        "exposure_modes":
> +        {
> +            "normal":
> +            {
> +                "shutter":
> +                [
> +                    100, 10000, 30000, 60000, 120000
> +                ],
> +                "gain":
> +                [
> +                    1.0, 2.0, 4.0, 6.0, 6.0
> +                ]
> +            },
> +            "short":
> +            {
> +                "shutter":
> +                [
> +                    100, 5000, 10000, 20000, 120000
> +                ],
> +                "gain":
> +                [
> +                    1.0, 2.0, 4.0, 6.0, 6.0
> +                ]
> +            }
> +        },
> +        "constraint_modes":
> +        {
> +            "normal":
> +            [
> +                {
> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0,
> "y_target":
> +                    [
> +                        0, 0.5, 1000, 0.5
> +                    ]
> +                }
> +            ],
> +            "highlight":
> +            [
> +                {
> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0,
> "y_target":
> +                    [
> +                        0, 0.5, 1000, 0.5
> +                    ]
> +                },
> +                {
> +                    "bound": "UPPER", "q_lo": 0.98, "q_hi": 1.0,
> "y_target":
> +                    [
> +                        0, 0.8, 1000, 0.8
> +                    ]
> +                }
> +            ]
> +        },
> +        "y_target":
> +        [
> +            0, 0.16, 1000, 0.165, 10000, 0.17
> +        ]
> +    },
> +    "rpi.alsc":
> +    {
> +        "omega": 1.3,
> +        "n_iter": 100,
> +        "luminance_strength": 0.5,
> +        "calibrations_Cr":
> +        [
> +            {
> +                "ct": 4000, "table":
> +                [
> +                    2.554, 2.554, 2.541, 2.534, 2.495, 2.506, 2.516,
> 2.517, 2.518, 2.515, 2.513, 2.495, 2.481, 2.533, 2.533, 2.521,
> +                    2.522, 2.534, 2.539, 2.531, 2.531, 2.506, 2.506,
> 2.513, 2.513, 2.509, 2.498, 2.496, 2.508, 2.517, 2.521, 2.521,
> +                    2.509, 2.517, 2.534, 2.529, 2.531, 2.521, 2.517,
> 2.517, 2.515, 2.514, 2.506, 2.499, 2.508, 2.508, 2.521, 2.537,
> +                    2.507, 2.508, 2.517, 2.516, 2.495, 2.487, 2.519,
> 2.534, 2.535, 2.531, 2.499, 2.494, 2.501, 2.511, 2.526, 2.526,
> +                    2.509, 2.517, 2.507, 2.501, 2.494, 2.519, 2.539,
> 2.539, 2.537, 2.537, 2.533, 2.499, 2.503, 2.511, 2.529, 2.525,
> +                    2.521, 2.522, 2.476, 2.501, 2.501, 2.539, 2.546,
> 2.538, 2.531, 2.538, 2.541, 2.531, 2.529, 2.526, 2.529, 2.525,
> +                    2.516, 2.519, 2.469, 2.499, 2.499, 2.543, 2.543,
> 2.531, 2.528, 2.534, 2.541, 2.535, 2.531, 2.526, 2.531, 2.528,
> +                    2.509, 2.515, 2.465, 2.487, 2.487, 2.539, 2.543,
> 2.539, 2.533, 2.549, 2.542, 2.531, 2.529, 2.524, 2.532, 2.533,
> +                    2.499, 2.499, 2.475, 2.482, 2.471, 2.509, 2.539,
> 2.544, 2.543, 2.545, 2.533, 2.498, 2.521, 2.521, 2.537, 2.536,
> +                    2.499, 2.488, 2.488, 2.488, 2.471, 2.462, 2.509,
> 2.539, 2.539, 2.532, 2.498, 2.498, 2.518, 2.518, 2.539, 2.539,
> +                    2.483, 2.484, 2.488, 2.488, 2.502, 2.496, 2.508,
> 2.514, 2.518, 2.517, 2.521, 2.518, 2.518, 2.518, 2.525, 2.539,
> +                    2.483, 2.487, 2.478, 2.478, 2.507, 2.509, 2.514,
> 2.513, 2.514, 2.517, 2.536, 2.559, 2.501, 2.501, 2.503, 2.525
> +                ]
> +            }
> +        ],
> +        "calibrations_Cb":
> +        [
> +            {
> +                "ct": 4000, "table":
> +                [
> +                    2.619, 2.603, 2.599, 2.597, 2.595, 2.594, 2.589,
> 2.587, 2.586, 2.589, 2.592, 2.597, 2.601, 2.608, 2.621, 2.621,
> +                    2.619, 2.615, 2.603, 2.601, 2.596, 2.595, 2.591,
> 2.589, 2.589, 2.592, 2.599, 2.593, 2.601, 2.613, 2.622, 2.631,
> +                    2.617, 2.617, 2.612, 2.611, 2.604, 2.598, 2.593,
> 2.591, 2.592, 2.591, 2.593, 2.595, 2.599, 2.614, 2.623, 2.631,
> +                    2.624, 2.619, 2.615, 2.612, 2.605, 2.602, 2.597,
> 2.596, 2.592, 2.592, 2.595, 2.599, 2.602, 2.606, 2.619, 2.624,
> +                    2.629, 2.627, 2.627, 2.617, 2.609, 2.598, 2.612,
> 2.623, 2.615, 2.604, 2.589, 2.595, 2.599, 2.608, 2.611, 2.614,
> +                    2.629, 2.632, 2.637, 2.627, 2.612, 2.612, 2.629,
> 2.631, 2.628, 2.621, 2.604, 2.597, 2.598, 2.604, 2.609, 2.609,
> +                    2.635, 2.636, 2.642, 2.628, 2.623, 2.623, 2.636,
> 2.636, 2.634, 2.628, 2.616, 2.599, 2.597, 2.601, 2.603, 2.601,
> +                    2.641, 2.639, 2.646, 2.632, 2.627, 2.625, 2.632,
> 2.635, 2.634, 2.627, 2.614, 2.596, 2.595, 2.599, 2.599, 2.598,
> +                    2.643, 2.644, 2.651, 2.649, 2.629, 2.617, 2.624,
> 2.629, 2.625, 2.614, 2.586, 2.599, 2.595, 2.597, 2.592, 2.595,
> +                    2.645, 2.646, 2.649, 2.649, 2.638, 2.624, 2.616,
> 2.617, 2.609, 2.604, 2.603, 2.603, 2.595, 2.589, 2.587, 2.592,
> +                    2.641, 2.643, 2.649, 2.647, 2.638, 2.618, 2.615,
> 2.608, 2.602, 2.595, 2.596, 2.595, 2.593, 2.584, 2.581, 2.583,
> +                    2.638, 2.637, 2.647, 2.634, 2.634, 2.618, 2.621,
> 2.621, 2.611, 2.602, 2.596, 2.583, 2.581, 2.581, 2.576, 2.574
> +                ]
> +            }
> +        ],
> +        "luminance_lut":
> +        [
> +            1.308, 1.293, 1.228, 1.175, 1.139, 1.108, 1.092, 1.082,
> 1.082, 1.086, 1.097, 1.114, 1.149, 1.199, 1.279, 1.303,
> +            1.293, 1.249, 1.199, 1.162, 1.136, 1.109, 1.087, 1.077,
> 1.072, 1.081, 1.095, 1.103, 1.133, 1.172, 1.225, 1.282,
> +            1.251, 1.212, 1.186, 1.159, 1.129, 1.114, 1.102, 1.088,
> 1.088, 1.088, 1.095, 1.117, 1.123, 1.158, 1.198, 1.249,
> +            1.223, 1.192, 1.177, 1.163, 1.147, 1.139, 1.132, 1.112,
> 1.111, 1.107, 1.113, 1.118, 1.139, 1.155, 1.186, 1.232,
> +            1.207, 1.186, 1.171, 1.162, 1.168, 1.163, 1.153, 1.138,
> 1.129, 1.128, 1.132, 1.136, 1.149, 1.167, 1.189, 1.216,
> +            1.198, 1.186, 1.176, 1.176, 1.177, 1.185, 1.171, 1.157,
> 1.146, 1.144, 1.146, 1.149, 1.161, 1.181, 1.201, 1.221,
> +            1.203, 1.181, 1.176, 1.178, 1.191, 1.189, 1.188, 1.174,
> 1.159, 1.153, 1.158, 1.161, 1.169, 1.185, 1.211, 1.227,
> +            1.211, 1.179, 1.177, 1.187, 1.194, 1.196, 1.194, 1.187,
> 1.176, 1.169, 1.171, 1.171, 1.175, 1.189, 1.214, 1.226,
> +            1.219, 1.182, 1.184, 1.191, 1.195, 1.199, 1.197, 1.194,
> 1.188, 1.185, 1.179, 1.179, 1.182, 1.194, 1.212, 1.227,
> +            1.237, 1.192, 1.194, 1.194, 1.198, 1.199, 1.198, 1.197,
> 1.196, 1.193, 1.189, 1.189, 1.192, 1.203, 1.214, 1.231,
> +            1.282, 1.199, 1.199, 1.197, 1.199, 1.199, 1.192, 1.193,
> 1.193, 1.194, 1.196, 1.197, 1.206, 1.216, 1.228, 1.244,
> +            1.309, 1.236, 1.204, 1.203, 1.202, 1.194, 1.194, 1.188,
> 1.192, 1.192, 1.199, 1.201, 1.212, 1.221, 1.235, 1.247
> +        ],
> +        "sigma": 0.005,
> +        "sigma_Cb": 0.005
> +    },
> +    "rpi.contrast":
> +    {
> +        "ce_enable": 1,
> +        "gamma_curve":
> +        [
> +            0, 0, 1024, 5040, 2048, 9338, 3072, 12356, 4096, 15312, 5120,
> 18051, 6144, 20790, 7168, 23193,
> +            8192, 25744, 9216, 27942, 10240, 30035, 11264, 32005, 12288,
> 33975, 13312, 35815, 14336, 37600, 15360, 39168,
> +            16384, 40642, 18432, 43379, 20480, 45749, 22528, 47753,
> 24576, 49621, 26624, 51253, 28672, 52698, 30720, 53796,
> +            32768, 54876, 36864, 57012, 40960, 58656, 45056, 59954,
> 49152, 61183, 53248, 62355, 57344, 63419, 61440, 64476,
> +            65535, 65535
> +        ]
> +    },
> +    "rpi.ccm":
> +    {
> +        "ccms":
> +        [
> +            {
> +                "ct": 2960, "ccm":
> +                [
> +                    1.61465, -0.49757, -0.11708, -0.30813, 1.53862,
> -0.23049, 0.04267, -0.76988, 1.72721
> +                ]
> +            },
> +            {
> +                "ct": 3720, "ccm":
> +                [
> +                    1.63316, -0.55061, -0.08255, -0.27238, 1.54441,
> -0.27203, 0.01948, -0.66461, 1.64513
> +                ]
> +            },
> +            {
> +                "ct": 3770, "ccm":
> +                [
> +                    1.62378, -0.54465, -0.07913, -0.26849, 1.54674,
> -0.27825, 0.01751, -0.64966, 1.63214
> +                ]
> +            },
> +            {
> +                "ct": 6000, "ccm":
> +                [
> +                    1.56877, -0.55713, -0.01164, -0.13524, 1.48494,
> -0.34969, 0.01184, -0.59069, 1.57885
> +                ]
> +            },
> +            {
> +                "ct": 6040, "ccm":
> +                [
> +                    1.58628, -0.57451, -0.01177, -0.13959, 1.48858,
> -0.34898, 0.01138, -0.58747, 1.57609
> +                ]
> +            },
> +            {
> +                "ct": 8450, "ccm":
> +                [
> +                    1.51714, -0.33619, -0.18094, -0.16148, 2.03049,
> -0.86901, 0.00999, -0.92095, 1.91096
> +                ]
> +            }
> +        ]
> +    },
> +    "rpi.sharpen":
> +    {
> +    }
> +}
> 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
>
>

Hi Laurent, Naush

Thanks for submitting this!

On Mon, 20 Dec 2021 at 07:43, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Laurent,
>
> Thank you for your work.
>
> On Sun, 19 Dec 2021 at 23:27, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>>
>> From: Naushir Patuck <naush@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@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@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@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 @@
>> +{
>> +    "rpi.black_level":
>> +    {
>> +        "black_level": 4096
>> +    },
>> +    "rpi.dpc":
>> +    {
>> +    },
>> +    "rpi.lux":
>> +    {
>> +        "reference_shutter_speed": 19184,
>> +        "reference_gain": 1.0,
>> +        "reference_aperture": 1.0,
>> +        "reference_lux": 432,
>> +        "reference_Y": 13773
>> +    },
>> +    "rpi.noise":
>> +    {
>> +        "reference_constant": 0,
>> +        "reference_slope": 2.957
>> +    },
>> +    "rpi.geq":
>> +    {
>> +        "offset": 185,
>> +        "slope": 0.0105
>> +    },
>> +    "rpi.sdn":
>> +    {
>> +    },
>> +    "rpi.awb":
>> +    {
>> +        "priors":
>> +        [
>> +            {
>> +                "lux": 0, "prior":
>> +                [
>> +                    2000, 1.0, 3000, 0.0, 13000, 0.0
>> +                ]
>> +            },
>> +            {
>> +                "lux": 800, "prior":
>> +                [
>> +                    2000, 0.0, 6000, 2.0, 13000, 2.0
>> +                ]
>> +            },
>> +            {
>> +                "lux": 1500, "prior":
>> +                [
>> +                    2000, 0.0, 4000, 1.0, 6000, 6.0, 6500, 7.0, 7000, 1.0, 13000, 1.0
>> +                ]
>> +            }
>> +        ],
>> +        "modes":
>> +        {
>> +            "auto":
>> +            {
>> +                "lo": 2500,
>> +                "hi": 8000
>> +            },
>> +            "incandescent":
>> +            {
>> +                "lo": 2500,
>> +                "hi": 3000
>> +            },
>> +            "tungsten":
>> +            {
>> +                "lo": 3000,
>> +                "hi": 3500
>> +            },
>> +            "fluorescent":
>> +            {
>> +                "lo": 4000,
>> +                "hi": 4700
>> +            },
>> +            "indoor":
>> +            {
>> +                "lo": 3000,
>> +                "hi": 5000
>> +            },
>> +            "daylight":
>> +            {
>> +                "lo": 5500,
>> +                "hi": 6500
>> +            },
>> +            "cloudy":
>> +            {
>> +                "lo": 7000,
>> +                "hi": 8600
>> +            }
>> +        },
>> +        "bayes": 1,
>> +        "ct_curve":
>> +        [
>> +            2960.0, 0.5114, 0.3294, 3720.0, 0.4222, 0.4164, 3770.0, 0.4194, 0.4194, 6040.0, 0.2871, 0.5702, 8450.0,
>> +            0.2285, 0.6454

One thing I've learnt to be a bit careful of is having AWB modes that
give ranges too far outside the range of the CT curve. The CT curve
simply gets extrapolated and you can sometimes get some slightly weird
results. So I'd probably clip all those modes that list 2500K to
something nearer 2800K or 2900K. But this is a relatively minor thing,
and actually will afflict some of our other tuning files too. It would
obviously be a good idea to make the tuning tool handle this, only I'd
never really appreciated the issues back at that time.

>> +        ],
>> +        "sensitivity_r": 1.0,
>> +        "sensitivity_b": 1.0,
>> +        "transverse_pos": 0.01,
>> +        "transverse_neg": 0.01157
>> +    },
>> +    "rpi.agc":
>> +    {
>> +        "metering_modes":
>> +        {
>> +            "centre-weighted":
>> +            {
>> +                "weights":
>> +                [
>> +                    3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0
>> +                ]
>> +            },
>> +            "spot":
>> +            {
>> +                "weights":
>> +                [
>> +                    2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
>> +                ]
>> +            },
>> +            "matrix":
>> +            {
>> +                "weights":
>> +                [
>> +                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
>> +                ]
>> +            }
>> +        },
>> +        "exposure_modes":
>> +        {
>> +            "normal":
>> +            {
>> +                "shutter":
>> +                [
>> +                    100, 10000, 30000, 60000, 120000
>> +                ],
>> +                "gain":
>> +                [
>> +                    1.0, 2.0, 4.0, 6.0, 6.0
>> +                ]
>> +            },
>> +            "short":
>> +            {
>> +                "shutter":
>> +                [
>> +                    100, 5000, 10000, 20000, 120000
>> +                ],
>> +                "gain":
>> +                [
>> +                    1.0, 2.0, 4.0, 6.0, 6.0
>> +                ]
>> +            }
>> +        },
>> +        "constraint_modes":
>> +        {
>> +            "normal":
>> +            [
>> +                {
>> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
>> +                    [
>> +                        0, 0.5, 1000, 0.5
>> +                    ]
>> +                }
>> +            ],
>> +            "highlight":
>> +            [
>> +                {
>> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
>> +                    [
>> +                        0, 0.5, 1000, 0.5
>> +                    ]
>> +                },
>> +                {
>> +                    "bound": "UPPER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
>> +                    [
>> +                        0, 0.8, 1000, 0.8
>> +                    ]
>> +                }
>> +            ]
>> +        },
>> +        "y_target":
>> +        [
>> +            0, 0.16, 1000, 0.165, 10000, 0.17
>> +        ]
>> +    },
>> +    "rpi.alsc":
>> +    {
>> +        "omega": 1.3,
>> +        "n_iter": 100,
>> +        "luminance_strength": 0.5,
>> +        "calibrations_Cr":
>> +        [
>> +            {
>> +                "ct": 4000, "table":
>> +                [
>> +                    2.554, 2.554, 2.541, 2.534, 2.495, 2.506, 2.516, 2.517, 2.518, 2.515, 2.513, 2.495, 2.481, 2.533, 2.533, 2.521,
>> +                    2.522, 2.534, 2.539, 2.531, 2.531, 2.506, 2.506, 2.513, 2.513, 2.509, 2.498, 2.496, 2.508, 2.517, 2.521, 2.521,
>> +                    2.509, 2.517, 2.534, 2.529, 2.531, 2.521, 2.517, 2.517, 2.515, 2.514, 2.506, 2.499, 2.508, 2.508, 2.521, 2.537,
>> +                    2.507, 2.508, 2.517, 2.516, 2.495, 2.487, 2.519, 2.534, 2.535, 2.531, 2.499, 2.494, 2.501, 2.511, 2.526, 2.526,
>> +                    2.509, 2.517, 2.507, 2.501, 2.494, 2.519, 2.539, 2.539, 2.537, 2.537, 2.533, 2.499, 2.503, 2.511, 2.529, 2.525,
>> +                    2.521, 2.522, 2.476, 2.501, 2.501, 2.539, 2.546, 2.538, 2.531, 2.538, 2.541, 2.531, 2.529, 2.526, 2.529, 2.525,
>> +                    2.516, 2.519, 2.469, 2.499, 2.499, 2.543, 2.543, 2.531, 2.528, 2.534, 2.541, 2.535, 2.531, 2.526, 2.531, 2.528,
>> +                    2.509, 2.515, 2.465, 2.487, 2.487, 2.539, 2.543, 2.539, 2.533, 2.549, 2.542, 2.531, 2.529, 2.524, 2.532, 2.533,
>> +                    2.499, 2.499, 2.475, 2.482, 2.471, 2.509, 2.539, 2.544, 2.543, 2.545, 2.533, 2.498, 2.521, 2.521, 2.537, 2.536,
>> +                    2.499, 2.488, 2.488, 2.488, 2.471, 2.462, 2.509, 2.539, 2.539, 2.532, 2.498, 2.498, 2.518, 2.518, 2.539, 2.539,
>> +                    2.483, 2.484, 2.488, 2.488, 2.502, 2.496, 2.508, 2.514, 2.518, 2.517, 2.521, 2.518, 2.518, 2.518, 2.525, 2.539,
>> +                    2.483, 2.487, 2.478, 2.478, 2.507, 2.509, 2.514, 2.513, 2.514, 2.517, 2.536, 2.559, 2.501, 2.501, 2.503, 2.525
>> +                ]
>> +            }
>> +        ],
>> +        "calibrations_Cb":
>> +        [
>> +            {
>> +                "ct": 4000, "table":
>> +                [
>> +                    2.619, 2.603, 2.599, 2.597, 2.595, 2.594, 2.589, 2.587, 2.586, 2.589, 2.592, 2.597, 2.601, 2.608, 2.621, 2.621,
>> +                    2.619, 2.615, 2.603, 2.601, 2.596, 2.595, 2.591, 2.589, 2.589, 2.592, 2.599, 2.593, 2.601, 2.613, 2.622, 2.631,
>> +                    2.617, 2.617, 2.612, 2.611, 2.604, 2.598, 2.593, 2.591, 2.592, 2.591, 2.593, 2.595, 2.599, 2.614, 2.623, 2.631,
>> +                    2.624, 2.619, 2.615, 2.612, 2.605, 2.602, 2.597, 2.596, 2.592, 2.592, 2.595, 2.599, 2.602, 2.606, 2.619, 2.624,
>> +                    2.629, 2.627, 2.627, 2.617, 2.609, 2.598, 2.612, 2.623, 2.615, 2.604, 2.589, 2.595, 2.599, 2.608, 2.611, 2.614,
>> +                    2.629, 2.632, 2.637, 2.627, 2.612, 2.612, 2.629, 2.631, 2.628, 2.621, 2.604, 2.597, 2.598, 2.604, 2.609, 2.609,
>> +                    2.635, 2.636, 2.642, 2.628, 2.623, 2.623, 2.636, 2.636, 2.634, 2.628, 2.616, 2.599, 2.597, 2.601, 2.603, 2.601,
>> +                    2.641, 2.639, 2.646, 2.632, 2.627, 2.625, 2.632, 2.635, 2.634, 2.627, 2.614, 2.596, 2.595, 2.599, 2.599, 2.598,
>> +                    2.643, 2.644, 2.651, 2.649, 2.629, 2.617, 2.624, 2.629, 2.625, 2.614, 2.586, 2.599, 2.595, 2.597, 2.592, 2.595,
>> +                    2.645, 2.646, 2.649, 2.649, 2.638, 2.624, 2.616, 2.617, 2.609, 2.604, 2.603, 2.603, 2.595, 2.589, 2.587, 2.592,
>> +                    2.641, 2.643, 2.649, 2.647, 2.638, 2.618, 2.615, 2.608, 2.602, 2.595, 2.596, 2.595, 2.593, 2.584, 2.581, 2.583,
>> +                    2.638, 2.637, 2.647, 2.634, 2.634, 2.618, 2.621, 2.621, 2.611, 2.602, 2.596, 2.583, 2.581, 2.581, 2.576, 2.574
>> +                ]
>> +            }
>> +        ],

I think having colour tables for only one CT is "a bit weak", but
still considerably better than nothing. I now have my fancy
super-expensive calibration kit that will let us measure images at
about 3000K and 6000K, so we should probably use that.

>> +        "luminance_lut":
>> +        [
>> +            1.308, 1.293, 1.228, 1.175, 1.139, 1.108, 1.092, 1.082, 1.082, 1.086, 1.097, 1.114, 1.149, 1.199, 1.279, 1.303,
>> +            1.293, 1.249, 1.199, 1.162, 1.136, 1.109, 1.087, 1.077, 1.072, 1.081, 1.095, 1.103, 1.133, 1.172, 1.225, 1.282,
>> +            1.251, 1.212, 1.186, 1.159, 1.129, 1.114, 1.102, 1.088, 1.088, 1.088, 1.095, 1.117, 1.123, 1.158, 1.198, 1.249,
>> +            1.223, 1.192, 1.177, 1.163, 1.147, 1.139, 1.132, 1.112, 1.111, 1.107, 1.113, 1.118, 1.139, 1.155, 1.186, 1.232,
>> +            1.207, 1.186, 1.171, 1.162, 1.168, 1.163, 1.153, 1.138, 1.129, 1.128, 1.132, 1.136, 1.149, 1.167, 1.189, 1.216,
>> +            1.198, 1.186, 1.176, 1.176, 1.177, 1.185, 1.171, 1.157, 1.146, 1.144, 1.146, 1.149, 1.161, 1.181, 1.201, 1.221,
>> +            1.203, 1.181, 1.176, 1.178, 1.191, 1.189, 1.188, 1.174, 1.159, 1.153, 1.158, 1.161, 1.169, 1.185, 1.211, 1.227,
>> +            1.211, 1.179, 1.177, 1.187, 1.194, 1.196, 1.194, 1.187, 1.176, 1.169, 1.171, 1.171, 1.175, 1.189, 1.214, 1.226,
>> +            1.219, 1.182, 1.184, 1.191, 1.195, 1.199, 1.197, 1.194, 1.188, 1.185, 1.179, 1.179, 1.182, 1.194, 1.212, 1.227,
>> +            1.237, 1.192, 1.194, 1.194, 1.198, 1.199, 1.198, 1.197, 1.196, 1.193, 1.189, 1.189, 1.192, 1.203, 1.214, 1.231,
>> +            1.282, 1.199, 1.199, 1.197, 1.199, 1.199, 1.192, 1.193, 1.193, 1.194, 1.196, 1.197, 1.206, 1.216, 1.228, 1.244,
>> +            1.309, 1.236, 1.204, 1.203, 1.202, 1.194, 1.194, 1.188, 1.192, 1.192, 1.199, 1.201, 1.212, 1.221, 1.235, 1.247

That's really a surprisingly small amount of luminance correction!

>> +        ],
>> +        "sigma": 0.005,
>> +        "sigma_Cb": 0.005

Measuring two CTs will probably mean it can give us better "sigma" values.

>> +    },
>> +    "rpi.contrast":
>> +    {
>> +        "ce_enable": 1,
>> +        "gamma_curve":
>> +        [
>> +            0, 0, 1024, 5040, 2048, 9338, 3072, 12356, 4096, 15312, 5120, 18051, 6144, 20790, 7168, 23193,
>> +            8192, 25744, 9216, 27942, 10240, 30035, 11264, 32005, 12288, 33975, 13312, 35815, 14336, 37600, 15360, 39168,
>> +            16384, 40642, 18432, 43379, 20480, 45749, 22528, 47753, 24576, 49621, 26624, 51253, 28672, 52698, 30720, 53796,
>> +            32768, 54876, 36864, 57012, 40960, 58656, 45056, 59954, 49152, 61183, 53248, 62355, 57344, 63419, 61440, 64476,
>> +            65535, 65535
>> +        ]
>> +    },
>> +    "rpi.ccm":
>> +    {
>> +        "ccms":
>> +        [
>> +            {
>> +                "ct": 2960, "ccm":
>> +                [
>> +                    1.61465, -0.49757, -0.11708, -0.30813, 1.53862, -0.23049, 0.04267, -0.76988, 1.72721
>> +                ]
>> +            },
>> +            {
>> +                "ct": 3720, "ccm":
>> +                [
>> +                    1.63316, -0.55061, -0.08255, -0.27238, 1.54441, -0.27203, 0.01948, -0.66461, 1.64513
>> +                ]
>> +            },
>> +            {
>> +                "ct": 3770, "ccm":
>> +                [
>> +                    1.62378, -0.54465, -0.07913, -0.26849, 1.54674, -0.27825, 0.01751, -0.64966, 1.63214
>> +                ]
>> +            },
>> +            {
>> +                "ct": 6000, "ccm":
>> +                [
>> +                    1.56877, -0.55713, -0.01164, -0.13524, 1.48494, -0.34969, 0.01184, -0.59069, 1.57885
>> +                ]
>> +            },
>> +            {
>> +                "ct": 6040, "ccm":
>> +                [
>> +                    1.58628, -0.57451, -0.01177, -0.13959, 1.48858, -0.34898, 0.01138, -0.58747, 1.57609
>> +                ]
>> +            },
>> +            {
>> +                "ct": 8450, "ccm":
>> +                [
>> +                    1.51714, -0.33619, -0.18094, -0.16148, 2.03049, -0.86901, 0.00999, -0.92095, 1.91096
>> +                ]
>> +            }
>> +        ]
>> +    },
>> +    "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@raspberrypi.com>

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

Hi David,

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@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@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@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@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 @@
> >> +{
> >> +    "rpi.black_level":
> >> +    {
> >> +        "black_level": 4096
> >> +    },
> >> +    "rpi.dpc":
> >> +    {
> >> +    },
> >> +    "rpi.lux":
> >> +    {
> >> +        "reference_shutter_speed": 19184,
> >> +        "reference_gain": 1.0,
> >> +        "reference_aperture": 1.0,
> >> +        "reference_lux": 432,
> >> +        "reference_Y": 13773
> >> +    },
> >> +    "rpi.noise":
> >> +    {
> >> +        "reference_constant": 0,
> >> +        "reference_slope": 2.957
> >> +    },
> >> +    "rpi.geq":
> >> +    {
> >> +        "offset": 185,
> >> +        "slope": 0.0105
> >> +    },
> >> +    "rpi.sdn":
> >> +    {
> >> +    },
> >> +    "rpi.awb":
> >> +    {
> >> +        "priors":
> >> +        [
> >> +            {
> >> +                "lux": 0, "prior":
> >> +                [
> >> +                    2000, 1.0, 3000, 0.0, 13000, 0.0
> >> +                ]
> >> +            },
> >> +            {
> >> +                "lux": 800, "prior":
> >> +                [
> >> +                    2000, 0.0, 6000, 2.0, 13000, 2.0
> >> +                ]
> >> +            },
> >> +            {
> >> +                "lux": 1500, "prior":
> >> +                [
> >> +                    2000, 0.0, 4000, 1.0, 6000, 6.0, 6500, 7.0, 7000, 1.0, 13000, 1.0
> >> +                ]
> >> +            }
> >> +        ],
> >> +        "modes":
> >> +        {
> >> +            "auto":
> >> +            {
> >> +                "lo": 2500,
> >> +                "hi": 8000
> >> +            },
> >> +            "incandescent":
> >> +            {
> >> +                "lo": 2500,
> >> +                "hi": 3000
> >> +            },
> >> +            "tungsten":
> >> +            {
> >> +                "lo": 3000,
> >> +                "hi": 3500
> >> +            },
> >> +            "fluorescent":
> >> +            {
> >> +                "lo": 4000,
> >> +                "hi": 4700
> >> +            },
> >> +            "indoor":
> >> +            {
> >> +                "lo": 3000,
> >> +                "hi": 5000
> >> +            },
> >> +            "daylight":
> >> +            {
> >> +                "lo": 5500,
> >> +                "hi": 6500
> >> +            },
> >> +            "cloudy":
> >> +            {
> >> +                "lo": 7000,
> >> +                "hi": 8600
> >> +            }
> >> +        },
> >> +        "bayes": 1,
> >> +        "ct_curve":
> >> +        [
> >> +            2960.0, 0.5114, 0.3294, 3720.0, 0.4222, 0.4164, 3770.0, 0.4194, 0.4194, 6040.0, 0.2871, 0.5702, 8450.0,
> >> +            0.2285, 0.6454
> 
> One thing I've learnt to be a bit careful of is having AWB modes that
> give ranges too far outside the range of the CT curve. The CT curve
> simply gets extrapolated and you can sometimes get some slightly weird
> results. So I'd probably clip all those modes that list 2500K to
> something nearer 2800K or 2900K. But this is a relatively minor thing,
> and actually will afflict some of our other tuning files too. It would
> obviously be a good idea to make the tuning tool handle this, only I'd
> never really appreciated the issues back at that time.
>
> >> +        ],
> >> +        "sensitivity_r": 1.0,
> >> +        "sensitivity_b": 1.0,
> >> +        "transverse_pos": 0.01,
> >> +        "transverse_neg": 0.01157
> >> +    },
> >> +    "rpi.agc":
> >> +    {
> >> +        "metering_modes":
> >> +        {
> >> +            "centre-weighted":
> >> +            {
> >> +                "weights":
> >> +                [
> >> +                    3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0
> >> +                ]
> >> +            },
> >> +            "spot":
> >> +            {
> >> +                "weights":
> >> +                [
> >> +                    2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
> >> +                ]
> >> +            },
> >> +            "matrix":
> >> +            {
> >> +                "weights":
> >> +                [
> >> +                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
> >> +                ]
> >> +            }
> >> +        },
> >> +        "exposure_modes":
> >> +        {
> >> +            "normal":
> >> +            {
> >> +                "shutter":
> >> +                [
> >> +                    100, 10000, 30000, 60000, 120000
> >> +                ],
> >> +                "gain":
> >> +                [
> >> +                    1.0, 2.0, 4.0, 6.0, 6.0
> >> +                ]
> >> +            },
> >> +            "short":
> >> +            {
> >> +                "shutter":
> >> +                [
> >> +                    100, 5000, 10000, 20000, 120000
> >> +                ],
> >> +                "gain":
> >> +                [
> >> +                    1.0, 2.0, 4.0, 6.0, 6.0
> >> +                ]
> >> +            }
> >> +        },
> >> +        "constraint_modes":
> >> +        {
> >> +            "normal":
> >> +            [
> >> +                {
> >> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> >> +                    [
> >> +                        0, 0.5, 1000, 0.5
> >> +                    ]
> >> +                }
> >> +            ],
> >> +            "highlight":
> >> +            [
> >> +                {
> >> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> >> +                    [
> >> +                        0, 0.5, 1000, 0.5
> >> +                    ]
> >> +                },
> >> +                {
> >> +                    "bound": "UPPER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> >> +                    [
> >> +                        0, 0.8, 1000, 0.8
> >> +                    ]
> >> +                }
> >> +            ]
> >> +        },
> >> +        "y_target":
> >> +        [
> >> +            0, 0.16, 1000, 0.165, 10000, 0.17
> >> +        ]
> >> +    },
> >> +    "rpi.alsc":
> >> +    {
> >> +        "omega": 1.3,
> >> +        "n_iter": 100,
> >> +        "luminance_strength": 0.5,
> >> +        "calibrations_Cr":
> >> +        [
> >> +            {
> >> +                "ct": 4000, "table":
> >> +                [
> >> +                    2.554, 2.554, 2.541, 2.534, 2.495, 2.506, 2.516, 2.517, 2.518, 2.515, 2.513, 2.495, 2.481, 2.533, 2.533, 2.521,
> >> +                    2.522, 2.534, 2.539, 2.531, 2.531, 2.506, 2.506, 2.513, 2.513, 2.509, 2.498, 2.496, 2.508, 2.517, 2.521, 2.521,
> >> +                    2.509, 2.517, 2.534, 2.529, 2.531, 2.521, 2.517, 2.517, 2.515, 2.514, 2.506, 2.499, 2.508, 2.508, 2.521, 2.537,
> >> +                    2.507, 2.508, 2.517, 2.516, 2.495, 2.487, 2.519, 2.534, 2.535, 2.531, 2.499, 2.494, 2.501, 2.511, 2.526, 2.526,
> >> +                    2.509, 2.517, 2.507, 2.501, 2.494, 2.519, 2.539, 2.539, 2.537, 2.537, 2.533, 2.499, 2.503, 2.511, 2.529, 2.525,
> >> +                    2.521, 2.522, 2.476, 2.501, 2.501, 2.539, 2.546, 2.538, 2.531, 2.538, 2.541, 2.531, 2.529, 2.526, 2.529, 2.525,
> >> +                    2.516, 2.519, 2.469, 2.499, 2.499, 2.543, 2.543, 2.531, 2.528, 2.534, 2.541, 2.535, 2.531, 2.526, 2.531, 2.528,
> >> +                    2.509, 2.515, 2.465, 2.487, 2.487, 2.539, 2.543, 2.539, 2.533, 2.549, 2.542, 2.531, 2.529, 2.524, 2.532, 2.533,
> >> +                    2.499, 2.499, 2.475, 2.482, 2.471, 2.509, 2.539, 2.544, 2.543, 2.545, 2.533, 2.498, 2.521, 2.521, 2.537, 2.536,
> >> +                    2.499, 2.488, 2.488, 2.488, 2.471, 2.462, 2.509, 2.539, 2.539, 2.532, 2.498, 2.498, 2.518, 2.518, 2.539, 2.539,
> >> +                    2.483, 2.484, 2.488, 2.488, 2.502, 2.496, 2.508, 2.514, 2.518, 2.517, 2.521, 2.518, 2.518, 2.518, 2.525, 2.539,
> >> +                    2.483, 2.487, 2.478, 2.478, 2.507, 2.509, 2.514, 2.513, 2.514, 2.517, 2.536, 2.559, 2.501, 2.501, 2.503, 2.525
> >> +                ]
> >> +            }
> >> +        ],
> >> +        "calibrations_Cb":
> >> +        [
> >> +            {
> >> +                "ct": 4000, "table":
> >> +                [
> >> +                    2.619, 2.603, 2.599, 2.597, 2.595, 2.594, 2.589, 2.587, 2.586, 2.589, 2.592, 2.597, 2.601, 2.608, 2.621, 2.621,
> >> +                    2.619, 2.615, 2.603, 2.601, 2.596, 2.595, 2.591, 2.589, 2.589, 2.592, 2.599, 2.593, 2.601, 2.613, 2.622, 2.631,
> >> +                    2.617, 2.617, 2.612, 2.611, 2.604, 2.598, 2.593, 2.591, 2.592, 2.591, 2.593, 2.595, 2.599, 2.614, 2.623, 2.631,
> >> +                    2.624, 2.619, 2.615, 2.612, 2.605, 2.602, 2.597, 2.596, 2.592, 2.592, 2.595, 2.599, 2.602, 2.606, 2.619, 2.624,
> >> +                    2.629, 2.627, 2.627, 2.617, 2.609, 2.598, 2.612, 2.623, 2.615, 2.604, 2.589, 2.595, 2.599, 2.608, 2.611, 2.614,
> >> +                    2.629, 2.632, 2.637, 2.627, 2.612, 2.612, 2.629, 2.631, 2.628, 2.621, 2.604, 2.597, 2.598, 2.604, 2.609, 2.609,
> >> +                    2.635, 2.636, 2.642, 2.628, 2.623, 2.623, 2.636, 2.636, 2.634, 2.628, 2.616, 2.599, 2.597, 2.601, 2.603, 2.601,
> >> +                    2.641, 2.639, 2.646, 2.632, 2.627, 2.625, 2.632, 2.635, 2.634, 2.627, 2.614, 2.596, 2.595, 2.599, 2.599, 2.598,
> >> +                    2.643, 2.644, 2.651, 2.649, 2.629, 2.617, 2.624, 2.629, 2.625, 2.614, 2.586, 2.599, 2.595, 2.597, 2.592, 2.595,
> >> +                    2.645, 2.646, 2.649, 2.649, 2.638, 2.624, 2.616, 2.617, 2.609, 2.604, 2.603, 2.603, 2.595, 2.589, 2.587, 2.592,
> >> +                    2.641, 2.643, 2.649, 2.647, 2.638, 2.618, 2.615, 2.608, 2.602, 2.595, 2.596, 2.595, 2.593, 2.584, 2.581, 2.583,
> >> +                    2.638, 2.637, 2.647, 2.634, 2.634, 2.618, 2.621, 2.621, 2.611, 2.602, 2.596, 2.583, 2.581, 2.581, 2.576, 2.574
> >> +                ]
> >> +            }
> >> +        ],
> 
> I think having colour tables for only one CT is "a bit weak", but
> still considerably better than nothing. I now have my fancy
> super-expensive calibration kit that will let us measure images at
> about 3000K and 6000K, so we should probably use that.
> 
> >> +        "luminance_lut":
> >> +        [
> >> +            1.308, 1.293, 1.228, 1.175, 1.139, 1.108, 1.092, 1.082, 1.082, 1.086, 1.097, 1.114, 1.149, 1.199, 1.279, 1.303,
> >> +            1.293, 1.249, 1.199, 1.162, 1.136, 1.109, 1.087, 1.077, 1.072, 1.081, 1.095, 1.103, 1.133, 1.172, 1.225, 1.282,
> >> +            1.251, 1.212, 1.186, 1.159, 1.129, 1.114, 1.102, 1.088, 1.088, 1.088, 1.095, 1.117, 1.123, 1.158, 1.198, 1.249,
> >> +            1.223, 1.192, 1.177, 1.163, 1.147, 1.139, 1.132, 1.112, 1.111, 1.107, 1.113, 1.118, 1.139, 1.155, 1.186, 1.232,
> >> +            1.207, 1.186, 1.171, 1.162, 1.168, 1.163, 1.153, 1.138, 1.129, 1.128, 1.132, 1.136, 1.149, 1.167, 1.189, 1.216,
> >> +            1.198, 1.186, 1.176, 1.176, 1.177, 1.185, 1.171, 1.157, 1.146, 1.144, 1.146, 1.149, 1.161, 1.181, 1.201, 1.221,
> >> +            1.203, 1.181, 1.176, 1.178, 1.191, 1.189, 1.188, 1.174, 1.159, 1.153, 1.158, 1.161, 1.169, 1.185, 1.211, 1.227,
> >> +            1.211, 1.179, 1.177, 1.187, 1.194, 1.196, 1.194, 1.187, 1.176, 1.169, 1.171, 1.171, 1.175, 1.189, 1.214, 1.226,
> >> +            1.219, 1.182, 1.184, 1.191, 1.195, 1.199, 1.197, 1.194, 1.188, 1.185, 1.179, 1.179, 1.182, 1.194, 1.212, 1.227,
> >> +            1.237, 1.192, 1.194, 1.194, 1.198, 1.199, 1.198, 1.197, 1.196, 1.193, 1.189, 1.189, 1.192, 1.203, 1.214, 1.231,
> >> +            1.282, 1.199, 1.199, 1.197, 1.199, 1.199, 1.192, 1.193, 1.193, 1.194, 1.196, 1.197, 1.206, 1.216, 1.228, 1.244,
> >> +            1.309, 1.236, 1.204, 1.203, 1.202, 1.194, 1.194, 1.188, 1.192, 1.192, 1.199, 1.201, 1.212, 1.221, 1.235, 1.247
> 
> That's really a surprisingly small amount of luminance correction!
> 
> >> +        ],
> >> +        "sigma": 0.005,
> >> +        "sigma_Cb": 0.005
> 
> Measuring two CTs will probably mean it can give us better "sigma" values.
> 
> >> +    },
> >> +    "rpi.contrast":
> >> +    {
> >> +        "ce_enable": 1,
> >> +        "gamma_curve":
> >> +        [
> >> +            0, 0, 1024, 5040, 2048, 9338, 3072, 12356, 4096, 15312, 5120, 18051, 6144, 20790, 7168, 23193,
> >> +            8192, 25744, 9216, 27942, 10240, 30035, 11264, 32005, 12288, 33975, 13312, 35815, 14336, 37600, 15360, 39168,
> >> +            16384, 40642, 18432, 43379, 20480, 45749, 22528, 47753, 24576, 49621, 26624, 51253, 28672, 52698, 30720, 53796,
> >> +            32768, 54876, 36864, 57012, 40960, 58656, 45056, 59954, 49152, 61183, 53248, 62355, 57344, 63419, 61440, 64476,
> >> +            65535, 65535
> >> +        ]
> >> +    },
> >> +    "rpi.ccm":
> >> +    {
> >> +        "ccms":
> >> +        [
> >> +            {
> >> +                "ct": 2960, "ccm":
> >> +                [
> >> +                    1.61465, -0.49757, -0.11708, -0.30813, 1.53862, -0.23049, 0.04267, -0.76988, 1.72721
> >> +                ]
> >> +            },
> >> +            {
> >> +                "ct": 3720, "ccm":
> >> +                [
> >> +                    1.63316, -0.55061, -0.08255, -0.27238, 1.54441, -0.27203, 0.01948, -0.66461, 1.64513
> >> +                ]
> >> +            },
> >> +            {
> >> +                "ct": 3770, "ccm":
> >> +                [
> >> +                    1.62378, -0.54465, -0.07913, -0.26849, 1.54674, -0.27825, 0.01751, -0.64966, 1.63214
> >> +                ]
> >> +            },
> >> +            {
> >> +                "ct": 6000, "ccm":
> >> +                [
> >> +                    1.56877, -0.55713, -0.01164, -0.13524, 1.48494, -0.34969, 0.01184, -0.59069, 1.57885
> >> +                ]
> >> +            },
> >> +            {
> >> +                "ct": 6040, "ccm":
> >> +                [
> >> +                    1.58628, -0.57451, -0.01177, -0.13959, 1.48858, -0.34898, 0.01138, -0.58747, 1.57609
> >> +                ]
> >> +            },
> >> +            {
> >> +                "ct": 8450, "ccm":
> >> +                [
> >> +                    1.51714, -0.33619, -0.18094, -0.16148, 2.03049, -0.86901, 0.00999, -0.92095, 1.91096
> >> +                ]
> >> +            }
> >> +        ]
> >> +    },
> >> +    "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@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 ? :-)

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

Hi Laurent

On Tue, 21 Dec 2021 at 23:39, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> 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@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@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@raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush@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 @@
> > >> +{
> > >> +    "rpi.black_level":
> > >> +    {
> > >> +        "black_level": 4096
> > >> +    },
> > >> +    "rpi.dpc":
> > >> +    {
> > >> +    },
> > >> +    "rpi.lux":
> > >> +    {
> > >> +        "reference_shutter_speed": 19184,
> > >> +        "reference_gain": 1.0,
> > >> +        "reference_aperture": 1.0,
> > >> +        "reference_lux": 432,
> > >> +        "reference_Y": 13773
> > >> +    },
> > >> +    "rpi.noise":
> > >> +    {
> > >> +        "reference_constant": 0,
> > >> +        "reference_slope": 2.957
> > >> +    },
> > >> +    "rpi.geq":
> > >> +    {
> > >> +        "offset": 185,
> > >> +        "slope": 0.0105
> > >> +    },
> > >> +    "rpi.sdn":
> > >> +    {
> > >> +    },
> > >> +    "rpi.awb":
> > >> +    {
> > >> +        "priors":
> > >> +        [
> > >> +            {
> > >> +                "lux": 0, "prior":
> > >> +                [
> > >> +                    2000, 1.0, 3000, 0.0, 13000, 0.0
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "lux": 800, "prior":
> > >> +                [
> > >> +                    2000, 0.0, 6000, 2.0, 13000, 2.0
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "lux": 1500, "prior":
> > >> +                [
> > >> +                    2000, 0.0, 4000, 1.0, 6000, 6.0, 6500, 7.0, 7000, 1.0, 13000, 1.0
> > >> +                ]
> > >> +            }
> > >> +        ],
> > >> +        "modes":
> > >> +        {
> > >> +            "auto":
> > >> +            {
> > >> +                "lo": 2500,
> > >> +                "hi": 8000
> > >> +            },
> > >> +            "incandescent":
> > >> +            {
> > >> +                "lo": 2500,
> > >> +                "hi": 3000
> > >> +            },
> > >> +            "tungsten":
> > >> +            {
> > >> +                "lo": 3000,
> > >> +                "hi": 3500
> > >> +            },
> > >> +            "fluorescent":
> > >> +            {
> > >> +                "lo": 4000,
> > >> +                "hi": 4700
> > >> +            },
> > >> +            "indoor":
> > >> +            {
> > >> +                "lo": 3000,
> > >> +                "hi": 5000
> > >> +            },
> > >> +            "daylight":
> > >> +            {
> > >> +                "lo": 5500,
> > >> +                "hi": 6500
> > >> +            },
> > >> +            "cloudy":
> > >> +            {
> > >> +                "lo": 7000,
> > >> +                "hi": 8600
> > >> +            }
> > >> +        },
> > >> +        "bayes": 1,
> > >> +        "ct_curve":
> > >> +        [
> > >> +            2960.0, 0.5114, 0.3294, 3720.0, 0.4222, 0.4164, 3770.0, 0.4194, 0.4194, 6040.0, 0.2871, 0.5702, 8450.0,
> > >> +            0.2285, 0.6454
> >
> > One thing I've learnt to be a bit careful of is having AWB modes that
> > give ranges too far outside the range of the CT curve. The CT curve
> > simply gets extrapolated and you can sometimes get some slightly weird
> > results. So I'd probably clip all those modes that list 2500K to
> > something nearer 2800K or 2900K. But this is a relatively minor thing,
> > and actually will afflict some of our other tuning files too. It would
> > obviously be a good idea to make the tuning tool handle this, only I'd
> > never really appreciated the issues back at that time.
> >
> > >> +        ],
> > >> +        "sensitivity_r": 1.0,
> > >> +        "sensitivity_b": 1.0,
> > >> +        "transverse_pos": 0.01,
> > >> +        "transverse_neg": 0.01157
> > >> +    },
> > >> +    "rpi.agc":
> > >> +    {
> > >> +        "metering_modes":
> > >> +        {
> > >> +            "centre-weighted":
> > >> +            {
> > >> +                "weights":
> > >> +                [
> > >> +                    3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0
> > >> +                ]
> > >> +            },
> > >> +            "spot":
> > >> +            {
> > >> +                "weights":
> > >> +                [
> > >> +                    2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
> > >> +                ]
> > >> +            },
> > >> +            "matrix":
> > >> +            {
> > >> +                "weights":
> > >> +                [
> > >> +                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
> > >> +                ]
> > >> +            }
> > >> +        },
> > >> +        "exposure_modes":
> > >> +        {
> > >> +            "normal":
> > >> +            {
> > >> +                "shutter":
> > >> +                [
> > >> +                    100, 10000, 30000, 60000, 120000
> > >> +                ],
> > >> +                "gain":
> > >> +                [
> > >> +                    1.0, 2.0, 4.0, 6.0, 6.0
> > >> +                ]
> > >> +            },
> > >> +            "short":
> > >> +            {
> > >> +                "shutter":
> > >> +                [
> > >> +                    100, 5000, 10000, 20000, 120000
> > >> +                ],
> > >> +                "gain":
> > >> +                [
> > >> +                    1.0, 2.0, 4.0, 6.0, 6.0
> > >> +                ]
> > >> +            }
> > >> +        },
> > >> +        "constraint_modes":
> > >> +        {
> > >> +            "normal":
> > >> +            [
> > >> +                {
> > >> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> > >> +                    [
> > >> +                        0, 0.5, 1000, 0.5
> > >> +                    ]
> > >> +                }
> > >> +            ],
> > >> +            "highlight":
> > >> +            [
> > >> +                {
> > >> +                    "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> > >> +                    [
> > >> +                        0, 0.5, 1000, 0.5
> > >> +                    ]
> > >> +                },
> > >> +                {
> > >> +                    "bound": "UPPER", "q_lo": 0.98, "q_hi": 1.0, "y_target":
> > >> +                    [
> > >> +                        0, 0.8, 1000, 0.8
> > >> +                    ]
> > >> +                }
> > >> +            ]
> > >> +        },
> > >> +        "y_target":
> > >> +        [
> > >> +            0, 0.16, 1000, 0.165, 10000, 0.17
> > >> +        ]
> > >> +    },
> > >> +    "rpi.alsc":
> > >> +    {
> > >> +        "omega": 1.3,
> > >> +        "n_iter": 100,
> > >> +        "luminance_strength": 0.5,
> > >> +        "calibrations_Cr":
> > >> +        [
> > >> +            {
> > >> +                "ct": 4000, "table":
> > >> +                [
> > >> +                    2.554, 2.554, 2.541, 2.534, 2.495, 2.506, 2.516, 2.517, 2.518, 2.515, 2.513, 2.495, 2.481, 2.533, 2.533, 2.521,
> > >> +                    2.522, 2.534, 2.539, 2.531, 2.531, 2.506, 2.506, 2.513, 2.513, 2.509, 2.498, 2.496, 2.508, 2.517, 2.521, 2.521,
> > >> +                    2.509, 2.517, 2.534, 2.529, 2.531, 2.521, 2.517, 2.517, 2.515, 2.514, 2.506, 2.499, 2.508, 2.508, 2.521, 2.537,
> > >> +                    2.507, 2.508, 2.517, 2.516, 2.495, 2.487, 2.519, 2.534, 2.535, 2.531, 2.499, 2.494, 2.501, 2.511, 2.526, 2.526,
> > >> +                    2.509, 2.517, 2.507, 2.501, 2.494, 2.519, 2.539, 2.539, 2.537, 2.537, 2.533, 2.499, 2.503, 2.511, 2.529, 2.525,
> > >> +                    2.521, 2.522, 2.476, 2.501, 2.501, 2.539, 2.546, 2.538, 2.531, 2.538, 2.541, 2.531, 2.529, 2.526, 2.529, 2.525,
> > >> +                    2.516, 2.519, 2.469, 2.499, 2.499, 2.543, 2.543, 2.531, 2.528, 2.534, 2.541, 2.535, 2.531, 2.526, 2.531, 2.528,
> > >> +                    2.509, 2.515, 2.465, 2.487, 2.487, 2.539, 2.543, 2.539, 2.533, 2.549, 2.542, 2.531, 2.529, 2.524, 2.532, 2.533,
> > >> +                    2.499, 2.499, 2.475, 2.482, 2.471, 2.509, 2.539, 2.544, 2.543, 2.545, 2.533, 2.498, 2.521, 2.521, 2.537, 2.536,
> > >> +                    2.499, 2.488, 2.488, 2.488, 2.471, 2.462, 2.509, 2.539, 2.539, 2.532, 2.498, 2.498, 2.518, 2.518, 2.539, 2.539,
> > >> +                    2.483, 2.484, 2.488, 2.488, 2.502, 2.496, 2.508, 2.514, 2.518, 2.517, 2.521, 2.518, 2.518, 2.518, 2.525, 2.539,
> > >> +                    2.483, 2.487, 2.478, 2.478, 2.507, 2.509, 2.514, 2.513, 2.514, 2.517, 2.536, 2.559, 2.501, 2.501, 2.503, 2.525
> > >> +                ]
> > >> +            }
> > >> +        ],
> > >> +        "calibrations_Cb":
> > >> +        [
> > >> +            {
> > >> +                "ct": 4000, "table":
> > >> +                [
> > >> +                    2.619, 2.603, 2.599, 2.597, 2.595, 2.594, 2.589, 2.587, 2.586, 2.589, 2.592, 2.597, 2.601, 2.608, 2.621, 2.621,
> > >> +                    2.619, 2.615, 2.603, 2.601, 2.596, 2.595, 2.591, 2.589, 2.589, 2.592, 2.599, 2.593, 2.601, 2.613, 2.622, 2.631,
> > >> +                    2.617, 2.617, 2.612, 2.611, 2.604, 2.598, 2.593, 2.591, 2.592, 2.591, 2.593, 2.595, 2.599, 2.614, 2.623, 2.631,
> > >> +                    2.624, 2.619, 2.615, 2.612, 2.605, 2.602, 2.597, 2.596, 2.592, 2.592, 2.595, 2.599, 2.602, 2.606, 2.619, 2.624,
> > >> +                    2.629, 2.627, 2.627, 2.617, 2.609, 2.598, 2.612, 2.623, 2.615, 2.604, 2.589, 2.595, 2.599, 2.608, 2.611, 2.614,
> > >> +                    2.629, 2.632, 2.637, 2.627, 2.612, 2.612, 2.629, 2.631, 2.628, 2.621, 2.604, 2.597, 2.598, 2.604, 2.609, 2.609,
> > >> +                    2.635, 2.636, 2.642, 2.628, 2.623, 2.623, 2.636, 2.636, 2.634, 2.628, 2.616, 2.599, 2.597, 2.601, 2.603, 2.601,
> > >> +                    2.641, 2.639, 2.646, 2.632, 2.627, 2.625, 2.632, 2.635, 2.634, 2.627, 2.614, 2.596, 2.595, 2.599, 2.599, 2.598,
> > >> +                    2.643, 2.644, 2.651, 2.649, 2.629, 2.617, 2.624, 2.629, 2.625, 2.614, 2.586, 2.599, 2.595, 2.597, 2.592, 2.595,
> > >> +                    2.645, 2.646, 2.649, 2.649, 2.638, 2.624, 2.616, 2.617, 2.609, 2.604, 2.603, 2.603, 2.595, 2.589, 2.587, 2.592,
> > >> +                    2.641, 2.643, 2.649, 2.647, 2.638, 2.618, 2.615, 2.608, 2.602, 2.595, 2.596, 2.595, 2.593, 2.584, 2.581, 2.583,
> > >> +                    2.638, 2.637, 2.647, 2.634, 2.634, 2.618, 2.621, 2.621, 2.611, 2.602, 2.596, 2.583, 2.581, 2.581, 2.576, 2.574
> > >> +                ]
> > >> +            }
> > >> +        ],
> >
> > I think having colour tables for only one CT is "a bit weak", but
> > still considerably better than nothing. I now have my fancy
> > super-expensive calibration kit that will let us measure images at
> > about 3000K and 6000K, so we should probably use that.
> >
> > >> +        "luminance_lut":
> > >> +        [
> > >> +            1.308, 1.293, 1.228, 1.175, 1.139, 1.108, 1.092, 1.082, 1.082, 1.086, 1.097, 1.114, 1.149, 1.199, 1.279, 1.303,
> > >> +            1.293, 1.249, 1.199, 1.162, 1.136, 1.109, 1.087, 1.077, 1.072, 1.081, 1.095, 1.103, 1.133, 1.172, 1.225, 1.282,
> > >> +            1.251, 1.212, 1.186, 1.159, 1.129, 1.114, 1.102, 1.088, 1.088, 1.088, 1.095, 1.117, 1.123, 1.158, 1.198, 1.249,
> > >> +            1.223, 1.192, 1.177, 1.163, 1.147, 1.139, 1.132, 1.112, 1.111, 1.107, 1.113, 1.118, 1.139, 1.155, 1.186, 1.232,
> > >> +            1.207, 1.186, 1.171, 1.162, 1.168, 1.163, 1.153, 1.138, 1.129, 1.128, 1.132, 1.136, 1.149, 1.167, 1.189, 1.216,
> > >> +            1.198, 1.186, 1.176, 1.176, 1.177, 1.185, 1.171, 1.157, 1.146, 1.144, 1.146, 1.149, 1.161, 1.181, 1.201, 1.221,
> > >> +            1.203, 1.181, 1.176, 1.178, 1.191, 1.189, 1.188, 1.174, 1.159, 1.153, 1.158, 1.161, 1.169, 1.185, 1.211, 1.227,
> > >> +            1.211, 1.179, 1.177, 1.187, 1.194, 1.196, 1.194, 1.187, 1.176, 1.169, 1.171, 1.171, 1.175, 1.189, 1.214, 1.226,
> > >> +            1.219, 1.182, 1.184, 1.191, 1.195, 1.199, 1.197, 1.194, 1.188, 1.185, 1.179, 1.179, 1.182, 1.194, 1.212, 1.227,
> > >> +            1.237, 1.192, 1.194, 1.194, 1.198, 1.199, 1.198, 1.197, 1.196, 1.193, 1.189, 1.189, 1.192, 1.203, 1.214, 1.231,
> > >> +            1.282, 1.199, 1.199, 1.197, 1.199, 1.199, 1.192, 1.193, 1.193, 1.194, 1.196, 1.197, 1.206, 1.216, 1.228, 1.244,
> > >> +            1.309, 1.236, 1.204, 1.203, 1.202, 1.194, 1.194, 1.188, 1.192, 1.192, 1.199, 1.201, 1.212, 1.221, 1.235, 1.247
> >
> > That's really a surprisingly small amount of luminance correction!
> >
> > >> +        ],
> > >> +        "sigma": 0.005,
> > >> +        "sigma_Cb": 0.005
> >
> > Measuring two CTs will probably mean it can give us better "sigma" values.
> >
> > >> +    },
> > >> +    "rpi.contrast":
> > >> +    {
> > >> +        "ce_enable": 1,
> > >> +        "gamma_curve":
> > >> +        [
> > >> +            0, 0, 1024, 5040, 2048, 9338, 3072, 12356, 4096, 15312, 5120, 18051, 6144, 20790, 7168, 23193,
> > >> +            8192, 25744, 9216, 27942, 10240, 30035, 11264, 32005, 12288, 33975, 13312, 35815, 14336, 37600, 15360, 39168,
> > >> +            16384, 40642, 18432, 43379, 20480, 45749, 22528, 47753, 24576, 49621, 26624, 51253, 28672, 52698, 30720, 53796,
> > >> +            32768, 54876, 36864, 57012, 40960, 58656, 45056, 59954, 49152, 61183, 53248, 62355, 57344, 63419, 61440, 64476,
> > >> +            65535, 65535
> > >> +        ]
> > >> +    },
> > >> +    "rpi.ccm":
> > >> +    {
> > >> +        "ccms":
> > >> +        [
> > >> +            {
> > >> +                "ct": 2960, "ccm":
> > >> +                [
> > >> +                    1.61465, -0.49757, -0.11708, -0.30813, 1.53862, -0.23049, 0.04267, -0.76988, 1.72721
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "ct": 3720, "ccm":
> > >> +                [
> > >> +                    1.63316, -0.55061, -0.08255, -0.27238, 1.54441, -0.27203, 0.01948, -0.66461, 1.64513
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "ct": 3770, "ccm":
> > >> +                [
> > >> +                    1.62378, -0.54465, -0.07913, -0.26849, 1.54674, -0.27825, 0.01751, -0.64966, 1.63214
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "ct": 6000, "ccm":
> > >> +                [
> > >> +                    1.56877, -0.55713, -0.01164, -0.13524, 1.48494, -0.34969, 0.01184, -0.59069, 1.57885
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "ct": 6040, "ccm":
> > >> +                [
> > >> +                    1.58628, -0.57451, -0.01177, -0.13959, 1.48858, -0.34898, 0.01138, -0.58747, 1.57609
> > >> +                ]
> > >> +            },
> > >> +            {
> > >> +                "ct": 8450, "ccm":
> > >> +                [
> > >> +                    1.51714, -0.33619, -0.18094, -0.16148, 2.03049, -0.86901, 0.00999, -0.92095, 1.91096
> > >> +                ]
> > >> +            }
> > >> +        ]
> > >> +    },
> > >> +    "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@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!

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.

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

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@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@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@raspberrypi.com>
> > > > Reviewed-by: Naushir Patuck <naush@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@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',

Hi Laurent

On Wed, 22 Dec 2021 at 09:43, Laurent Pinchart
<laurent.pinchart@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@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@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@raspberrypi.com>
> > > > > Reviewed-by: Naushir Patuck <naush@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@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

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@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@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@raspberrypi.com>
> > > > > > Reviewed-by: Naushir Patuck <naush@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@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 ?

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

Hi Laurent

On Wed, 22 Dec 2021 at 10:49, Laurent Pinchart
<laurent.pinchart@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@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@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@raspberrypi.com>
> > > > > > > Reviewed-by: Naushir Patuck <naush@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@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

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@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@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@raspberrypi.com>
> > > > > > > > Reviewed-by: Naushir Patuck <naush@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@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',