| Message ID | 20240701144122.3418955-6-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > For imx219 the black level was incorrectly set to 256. According to the > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. Might it have been expressed in 12 bits maybe ? (however the mainline imx219 driver only supports 10 and 8 bpp codes) > > For the imx258, BLC was not included at all. As only LSC data with > rather low maximum values, the image quality is expected to only get > better by adding black level correction. Not sure if this last paragraph is necessary. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Anyway, we now have them in camera sensor helpers Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > src/ipa/rkisp1/data/imx258.yaml | 1 + > 2 files changed, 1 insertion(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > index cbcc43b84ac7..0d99cb529392 100644 > --- a/src/ipa/rkisp1/data/imx219.yaml > +++ b/src/ipa/rkisp1/data/imx219.yaml > @@ -6,10 +6,6 @@ algorithms: > - Agc: > - Awb: > - BlackLevelCorrection: > - R: 256 > - Gr: 256 > - Gb: 256 > - B: 256 > - LensShadingCorrection: > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > index 43dddf20dcd2..202af36afbee 100644 > --- a/src/ipa/rkisp1/data/imx258.yaml > +++ b/src/ipa/rkisp1/data/imx258.yaml > @@ -5,6 +5,7 @@ version: 1 > algorithms: > - Agc: > - Awb: > + - BlackLevelCorrection: > - LensShadingCorrection: > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > -- > 2.43.0 >
On Tue, Jul 02, 2024 at 10:53:55AM +0200, Jacopo Mondi wrote: > On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > > For imx219 the black level was incorrectly set to 256. According to the > > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. > > Might it have been expressed in 12 bits maybe ? > (however the mainline imx219 driver only supports 10 and 8 bpp codes) I suspect that's because the sensor has a 10-bit ADC :-) I'm not sure why we would have expressed the value on 12 bits. I'm fine treating it as a mystery and moving on. > > For the imx258, BLC was not included at all. As only LSC data with > > rather low maximum values, the image quality is expected to only get > > better by adding black level correction. > > Not sure if this last paragraph is necessary. I don't get the part of the second sentence before the comma actually. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Anyway, we now have them in camera sensor helpers > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > > src/ipa/rkisp1/data/imx258.yaml | 1 + > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > > index cbcc43b84ac7..0d99cb529392 100644 > > --- a/src/ipa/rkisp1/data/imx219.yaml > > +++ b/src/ipa/rkisp1/data/imx219.yaml > > @@ -6,10 +6,6 @@ algorithms: > > - Agc: > > - Awb: > > - BlackLevelCorrection: > > - R: 256 > > - Gr: 256 > > - Gb: 256 > > - B: 256 > > - LensShadingCorrection: > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > > index 43dddf20dcd2..202af36afbee 100644 > > --- a/src/ipa/rkisp1/data/imx258.yaml > > +++ b/src/ipa/rkisp1/data/imx258.yaml > > @@ -5,6 +5,7 @@ version: 1 > > algorithms: > > - Agc: > > - Awb: > > + - BlackLevelCorrection: > > - LensShadingCorrection: > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ]
Hi Laurent, On Tue, Jul 02, 2024 at 04:19:52PM +0300, Laurent Pinchart wrote: > On Tue, Jul 02, 2024 at 10:53:55AM +0200, Jacopo Mondi wrote: > > On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > > > For imx219 the black level was incorrectly set to 256. According to the > > > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. > > > > Might it have been expressed in 12 bits maybe ? > > (however the mainline imx219 driver only supports 10 and 8 bpp codes) > > I suspect that's because the sensor has a 10-bit ADC :-) I'm not sure > why we would have expressed the value on 12 bits. I'm fine treating it > as a mystery and moving on. > For the record: The value was correct. It was expressed in 12 bits because the rkisp1 hardware expects that value to be with regards to 12bits. The new value is also correct, as we treat all values to be based on 16bits as described in the controls section. Conversion to the required bitdepth happens when passing the data to the hardware (this needs to be fixed in this series) > > > > For the imx258, BLC was not included at all. As only LSC data with > > > rather low maximum values, the image quality is expected to only get > > > better by adding black level correction. > > > > Not sure if this last paragraph is necessary. > > I don't get the part of the second sentence before the comma actually. Well something is missing in that sentence. I wanted to say that the LSC tables in that tuning file do so little (small factors) that adding BLC without recalculating the LSC tables should be ok. I'll drop it. Cheers, Stefan > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Anyway, we now have them in camera sensor helpers > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > > > src/ipa/rkisp1/data/imx258.yaml | 1 + > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > > > index cbcc43b84ac7..0d99cb529392 100644 > > > --- a/src/ipa/rkisp1/data/imx219.yaml > > > +++ b/src/ipa/rkisp1/data/imx219.yaml > > > @@ -6,10 +6,6 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 256 > > > - Gr: 256 > > > - Gb: 256 > > > - B: 256 > > > - LensShadingCorrection: > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > > > index 43dddf20dcd2..202af36afbee 100644 > > > --- a/src/ipa/rkisp1/data/imx258.yaml > > > +++ b/src/ipa/rkisp1/data/imx258.yaml > > > @@ -5,6 +5,7 @@ version: 1 > > > algorithms: > > > - Agc: > > > - Awb: > > > + - BlackLevelCorrection: > > > - LensShadingCorrection: > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > -- > Regards, > > Laurent Pinchart
Hi Stefan On Tue, Jul 02, 2024 at 04:03:50PM GMT, Stefan Klug wrote: > Hi Laurent, > > On Tue, Jul 02, 2024 at 04:19:52PM +0300, Laurent Pinchart wrote: > > On Tue, Jul 02, 2024 at 10:53:55AM +0200, Jacopo Mondi wrote: > > > On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > > > > For imx219 the black level was incorrectly set to 256. According to the > > > > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. > > > > > > Might it have been expressed in 12 bits maybe ? > > > (however the mainline imx219 driver only supports 10 and 8 bpp codes) > > > > I suspect that's because the sensor has a 10-bit ADC :-) I'm not sure > > why we would have expressed the value on 12 bits. I'm fine treating it > > as a mystery and moving on. > > > > For the record: The value was correct. It was expressed in 12 bits > because the rkisp1 hardware expects that value to be with regards to > 12bits. The new value is also correct, as we treat all values to be As the tuning file are platform-specific, what's the purpose of expressing the value as 16 bits if it has to be unconditionally converted to 12 ? > based on 16bits as described in the controls section. Conversion to the > required bitdepth happens when passing the data to the hardware (this > needs to be fixed in this series) > > > > > > > For the imx258, BLC was not included at all. As only LSC data with > > > > rather low maximum values, the image quality is expected to only get > > > > better by adding black level correction. > > > > > > Not sure if this last paragraph is necessary. > > > > I don't get the part of the second sentence before the comma actually. > > Well something is missing in that sentence. I wanted to say that the LSC > tables in that tuning file do so little (small factors) that adding BLC > without recalculating the LSC tables should be ok. I'll drop it. > > Cheers, > Stefan > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > > Anyway, we now have them in camera sensor helpers > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > > > > src/ipa/rkisp1/data/imx258.yaml | 1 + > > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > > > > index cbcc43b84ac7..0d99cb529392 100644 > > > > --- a/src/ipa/rkisp1/data/imx219.yaml > > > > +++ b/src/ipa/rkisp1/data/imx219.yaml > > > > @@ -6,10 +6,6 @@ algorithms: > > > > - Agc: > > > > - Awb: > > > > - BlackLevelCorrection: > > > > - R: 256 > > > > - Gr: 256 > > > > - Gb: 256 > > > > - B: 256 > > > > - LensShadingCorrection: > > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > > > > index 43dddf20dcd2..202af36afbee 100644 > > > > --- a/src/ipa/rkisp1/data/imx258.yaml > > > > +++ b/src/ipa/rkisp1/data/imx258.yaml > > > > @@ -5,6 +5,7 @@ version: 1 > > > > algorithms: > > > > - Agc: > > > > - Awb: > > > > + - BlackLevelCorrection: > > > > - LensShadingCorrection: > > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > -- > > Regards, > > > > Laurent Pinchart
Sorry I have not woken up yet On Wed, Jul 03, 2024 at 09:48:51AM GMT, Jacopo Mondi wrote: > Hi Stefan > > On Tue, Jul 02, 2024 at 04:03:50PM GMT, Stefan Klug wrote: > > Hi Laurent, > > > > On Tue, Jul 02, 2024 at 04:19:52PM +0300, Laurent Pinchart wrote: > > > On Tue, Jul 02, 2024 at 10:53:55AM +0200, Jacopo Mondi wrote: > > > > On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > > > > > For imx219 the black level was incorrectly set to 256. According to the > > > > > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. > > > > > > > > Might it have been expressed in 12 bits maybe ? > > > > (however the mainline imx219 driver only supports 10 and 8 bpp codes) > > > > > > I suspect that's because the sensor has a 10-bit ADC :-) I'm not sure > > > why we would have expressed the value on 12 bits. I'm fine treating it > > > as a mystery and moving on. > > > > > > > For the record: The value was correct. It was expressed in 12 bits > > because the rkisp1 hardware expects that value to be with regards to > > 12bits. The new value is also correct, as we treat all values to be > > As the tuning file are platform-specific, what's the purpose of > expressing the value as 16 bits if it has to be unconditionally > converted to 12 ? > As you're moving it to the platform-independent CameraSensorHelper, it is indeed fine to have it in 16 bits :/ Sorry for the noise > > based on 16bits as described in the controls section. Conversion to the > > required bitdepth happens when passing the data to the hardware (this > > needs to be fixed in this series) > > > > > > > > > > For the imx258, BLC was not included at all. As only LSC data with > > > > > rather low maximum values, the image quality is expected to only get > > > > > better by adding black level correction. > > > > > > > > Not sure if this last paragraph is necessary. > > > > > > I don't get the part of the second sentence before the comma actually. > > > > Well something is missing in that sentence. I wanted to say that the LSC > > tables in that tuning file do so little (small factors) that adding BLC > > without recalculating the LSC tables should be ok. I'll drop it. > > > > Cheers, > > Stefan > > > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > > > > Anyway, we now have them in camera sensor helpers > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > --- > > > > > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > > > > > src/ipa/rkisp1/data/imx258.yaml | 1 + > > > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > > > > > index cbcc43b84ac7..0d99cb529392 100644 > > > > > --- a/src/ipa/rkisp1/data/imx219.yaml > > > > > +++ b/src/ipa/rkisp1/data/imx219.yaml > > > > > @@ -6,10 +6,6 @@ algorithms: > > > > > - Agc: > > > > > - Awb: > > > > > - BlackLevelCorrection: > > > > > - R: 256 > > > > > - Gr: 256 > > > > > - Gb: 256 > > > > > - B: 256 > > > > > - LensShadingCorrection: > > > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > > > > > index 43dddf20dcd2..202af36afbee 100644 > > > > > --- a/src/ipa/rkisp1/data/imx258.yaml > > > > > +++ b/src/ipa/rkisp1/data/imx258.yaml > > > > > @@ -5,6 +5,7 @@ version: 1 > > > > > algorithms: > > > > > - Agc: > > > > > - Awb: > > > > > + - BlackLevelCorrection: > > > > > - LensShadingCorrection: > > > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
On Tue, Jul 02, 2024 at 04:19:52PM +0300, Laurent Pinchart wrote: > On Tue, Jul 02, 2024 at 10:53:55AM +0200, Jacopo Mondi wrote: > > On Mon, Jul 01, 2024 at 04:38:28PM GMT, Stefan Klug wrote: > > > For imx219 the black level was incorrectly set to 256. According to the > > > datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. > > > > Might it have been expressed in 12 bits maybe ? > > (however the mainline imx219 driver only supports 10 and 8 bpp codes) > > I suspect that's because the sensor has a 10-bit ADC :-) I'm not sure > why we would have expressed the value on 12 bits. I'm fine treating it > as a mystery and moving on. > > > > For the imx258, BLC was not included at all. As only LSC data with > > > rather low maximum values, the image quality is expected to only get > > > better by adding black level correction. > > > > Not sure if this last paragraph is necessary. > > I don't get the part of the second sentence before the comma actually. Same. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Anyway, we now have them in camera sensor helpers > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > > src/ipa/rkisp1/data/imx219.yaml | 4 ---- > > > src/ipa/rkisp1/data/imx258.yaml | 1 + > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml > > > index cbcc43b84ac7..0d99cb529392 100644 > > > --- a/src/ipa/rkisp1/data/imx219.yaml > > > +++ b/src/ipa/rkisp1/data/imx219.yaml > > > @@ -6,10 +6,6 @@ algorithms: > > > - Agc: > > > - Awb: > > > - BlackLevelCorrection: > > > - R: 256 > > > - Gr: 256 > > > - Gb: 256 > > > - B: 256 > > > - LensShadingCorrection: > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml > > > index 43dddf20dcd2..202af36afbee 100644 > > > --- a/src/ipa/rkisp1/data/imx258.yaml > > > +++ b/src/ipa/rkisp1/data/imx258.yaml > > > @@ -5,6 +5,7 @@ version: 1 > > > algorithms: > > > - Agc: > > > - Awb: > > > + - BlackLevelCorrection: > > > - LensShadingCorrection: > > > x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] > > > y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ]
diff --git a/src/ipa/rkisp1/data/imx219.yaml b/src/ipa/rkisp1/data/imx219.yaml index cbcc43b84ac7..0d99cb529392 100644 --- a/src/ipa/rkisp1/data/imx219.yaml +++ b/src/ipa/rkisp1/data/imx219.yaml @@ -6,10 +6,6 @@ algorithms: - Agc: - Awb: - BlackLevelCorrection: - R: 256 - Gr: 256 - Gb: 256 - B: 256 - LensShadingCorrection: x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] diff --git a/src/ipa/rkisp1/data/imx258.yaml b/src/ipa/rkisp1/data/imx258.yaml index 43dddf20dcd2..202af36afbee 100644 --- a/src/ipa/rkisp1/data/imx258.yaml +++ b/src/ipa/rkisp1/data/imx258.yaml @@ -5,6 +5,7 @@ version: 1 algorithms: - Agc: - Awb: + - BlackLevelCorrection: - LensShadingCorrection: x-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ] y-size: [ 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625, 0.0625 ]
For imx219 the black level was incorrectly set to 256. According to the datasheet it is 64d for raw 10. Mapped to 16bit, this becomes 4096. For the imx258, BLC was not included at all. As only LSC data with rather low maximum values, the image quality is expected to only get better by adding black level correction. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/data/imx219.yaml | 4 ---- src/ipa/rkisp1/data/imx258.yaml | 1 + 2 files changed, 1 insertion(+), 4 deletions(-)