[0/5] ipa: Add black level to camera sensor helpers
mbox series

Message ID 20240701144122.3418955-1-stefan.klug@ideasonboard.com
Headers show
Series
  • ipa: Add black level to camera sensor helpers
Related show

Message

Stefan Klug July 1, 2024, 2:38 p.m. UTC
For the tuning process, the sensor black level needs to be known. Many
sensors have either a fixed black level, or the current sensor kernel
driver sets a fixed value. It therefore makes sense to keep that
information inside libcamera. When the need arises to be more flexible
we can extend the sensor drivers to report black level information. This
series adds black level support to the camera sensor helpers and the
corresponding metadata to the rkisp1 ipa.

My tuning series will be rebased on top of this one.

Best regards,
Stefan

Stefan Klug (5):
  ipa: libipa: Add black levels to camera sensor helper
  ipa: rkisp1: Move camHelper into IPAContext
  ipa: rkisp1: blc: Query black levels from camera sensor helper
  ipa: rkisp1: blc: Report sensor black levels in metadata
  ipa: rkisp1: data: Update tuning files for imx219 and imx258

 src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++
 src/ipa/libipa/camera_sensor_helper.h   |  6 ++++
 src/ipa/rkisp1/algorithms/blc.cpp       | 47 ++++++++++++++++++++++---
 src/ipa/rkisp1/algorithms/blc.h         |  5 ++-
 src/ipa/rkisp1/data/imx219.yaml         |  4 ---
 src/ipa/rkisp1/data/imx258.yaml         |  1 +
 src/ipa/rkisp1/data/uncalibrated.yaml   |  1 +
 src/ipa/rkisp1/ipa_context.h            |  4 +++
 src/ipa/rkisp1/rkisp1.cpp               | 26 +++++++-------
 9 files changed, 89 insertions(+), 23 deletions(-)

Comments

Jacopo Mondi July 2, 2024, 8:56 a.m. UTC | #1
Hi Stefan

On Mon, Jul 01, 2024 at 04:38:23PM GMT, Stefan Klug wrote:
> For the tuning process, the sensor black level needs to be known. Many
> sensors have either a fixed black level, or the current sensor kernel
> driver sets a fixed value. It therefore makes sense to keep that

Where do kernel driver handles BLC and how ?

> information inside libcamera. When the need arises to be more flexible
> we can extend the sensor drivers to report black level information. This
> series adds black level support to the camera sensor helpers and the
> corresponding metadata to the rkisp1 ipa.

I always considered the camera_sensor_properties database a better
place where to store immutable properties like this one, but the
database is not accessibile by IPA. Have you gone for
CameraSensorHelper for this reason ?

How does this play with the idea of moving CameraSensorHelpers on the
pipeline side (something on our roadmap since a long time ?). Will we
need in this case to anyway pass sensor's data from pipeline to the
IPA ?

>
> My tuning series will be rebased on top of this one.
>
> Best regards,
> Stefan
>
> Stefan Klug (5):
>   ipa: libipa: Add black levels to camera sensor helper
>   ipa: rkisp1: Move camHelper into IPAContext
>   ipa: rkisp1: blc: Query black levels from camera sensor helper
>   ipa: rkisp1: blc: Report sensor black levels in metadata
>   ipa: rkisp1: data: Update tuning files for imx219 and imx258
>
>  src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++
>  src/ipa/libipa/camera_sensor_helper.h   |  6 ++++
>  src/ipa/rkisp1/algorithms/blc.cpp       | 47 ++++++++++++++++++++++---
>  src/ipa/rkisp1/algorithms/blc.h         |  5 ++-
>  src/ipa/rkisp1/data/imx219.yaml         |  4 ---
>  src/ipa/rkisp1/data/imx258.yaml         |  1 +
>  src/ipa/rkisp1/data/uncalibrated.yaml   |  1 +
>  src/ipa/rkisp1/ipa_context.h            |  4 +++
>  src/ipa/rkisp1/rkisp1.cpp               | 26 +++++++-------
>  9 files changed, 89 insertions(+), 23 deletions(-)
>
> --
> 2.43.0
>
Laurent Pinchart July 2, 2024, 12:57 p.m. UTC | #2
On Tue, Jul 02, 2024 at 10:56:59AM +0200, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Jul 01, 2024 at 04:38:23PM GMT, Stefan Klug wrote:
> > For the tuning process, the sensor black level needs to be known. Many
> > sensors have either a fixed black level, or the current sensor kernel
> > driver sets a fixed value. It therefore makes sense to keep that
> 
> Where do kernel driver handles BLC and how ?

Currently they either leave the corresponding registers to their
power-on default values, or program them with fixed values. I'm not
aware of any driver in mainline that makes the data pedestal
configurable from userspace.

> > information inside libcamera. When the need arises to be more flexible
> > we can extend the sensor drivers to report black level information. This
> > series adds black level support to the camera sensor helpers and the
> > corresponding metadata to the rkisp1 ipa.

It could be useful to explain a bit better that we're covering the data
pedestal only at this point, and how that differs from the black level.
Ideally that information should be captured somewhere in libcamera.
Maybe in the documentation of the CameraSensorHelper::blackLevels()
function ?

> I always considered the camera_sensor_properties database a better
> place where to store immutable properties like this one, but the
> database is not accessibile by IPA. Have you gone for
> CameraSensorHelper for this reason ?

I told Stefan to go for CameraSensorHelper for that reason, yes.
Eventually the two sets of helpers should be merged into one.

> How does this play with the idea of moving CameraSensorHelpers on the
> pipeline side (something on our roadmap since a long time ?). Will we
> need in this case to anyway pass sensor's data from pipeline to the
> IPA ?

I'd rather address it at that point, with a solution that covers all the
information the IPA module needs, instead of adding a partial
implementation today that we would need to rework tomorrow.

> > My tuning series will be rebased on top of this one.
> >
> > Best regards,
> > Stefan
> >
> > Stefan Klug (5):
> >   ipa: libipa: Add black levels to camera sensor helper
> >   ipa: rkisp1: Move camHelper into IPAContext
> >   ipa: rkisp1: blc: Query black levels from camera sensor helper
> >   ipa: rkisp1: blc: Report sensor black levels in metadata
> >   ipa: rkisp1: data: Update tuning files for imx219 and imx258
> >
> >  src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++
> >  src/ipa/libipa/camera_sensor_helper.h   |  6 ++++
> >  src/ipa/rkisp1/algorithms/blc.cpp       | 47 ++++++++++++++++++++++---
> >  src/ipa/rkisp1/algorithms/blc.h         |  5 ++-
> >  src/ipa/rkisp1/data/imx219.yaml         |  4 ---
> >  src/ipa/rkisp1/data/imx258.yaml         |  1 +
> >  src/ipa/rkisp1/data/uncalibrated.yaml   |  1 +
> >  src/ipa/rkisp1/ipa_context.h            |  4 +++
> >  src/ipa/rkisp1/rkisp1.cpp               | 26 +++++++-------
> >  9 files changed, 89 insertions(+), 23 deletions(-)
> >