[libcamera-devel,03/11] Adds rkisp1 metadata required for the Android HAL to use it.

17687 diff mbox series

From: Nicholas Roth <nicholas@rothemail.net>

---
 src/ipa/rkisp1/rkisp1.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

Hi Nicholas,

Thank you for the patch.

On Mon, Oct 24, 2022 at 12:55:35AM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas@rothemail.net>
> 
> ---
>  src/ipa/rkisp1/rkisp1.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index ba3c547e..c536852c 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -100,6 +100,13 @@ const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> +	/* libcamera requires a fixed value for minimum frame duration,
> +	 * but this depends on the frame size and the rkisp1 device datasheets
> +	 * measure this in pixels per second. Neither the datasheets nor the driver
> +	 * specify a maximum. The minimum below is for 1920x1920. The maximum
> +	 * corresponds to two seconds. */
> +	{ &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },

Could this be computed the same way we do it in the IPU3 IPA module ?

> +	{ &controls::draft::MaxLatency, ControlInfo(0) },

Why is this needed ?

>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
>

On Tue, Oct 25, 2022 at 02:49:25AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Nicholas,
> 
> Thank you for the patch.
> 
> On Mon, Oct 24, 2022 at 12:55:35AM -0500, Nicholas Roth via libcamera-devel wrote:
> > From: Nicholas Roth <nicholas@rothemail.net>
> > 
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index ba3c547e..c536852c 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -100,6 +100,13 @@ const ControlInfoMap::Map rkisp1Controls{
> >  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> >  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> >  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > +	/* libcamera requires a fixed value for minimum frame duration,
> > +	 * but this depends on the frame size and the rkisp1 device datasheets
> > +	 * measure this in pixels per second. Neither the datasheets nor the driver
> > +	 * specify a maximum. The minimum below is for 1920x1920. The maximum
> > +	 * corresponds to two seconds. */
> > +	{ &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },
> 
> Could this be computed the same way we do it in the IPU3 IPA module ?
> 
> > +	{ &controls::draft::MaxLatency, ControlInfo(0) },
> 
> Why is this needed ?

This control looks familiar in the context of android...

iirc from the CTS FULL support ordeal, MaxLatency needs to be between 0
and 4 inclusive for burst capture capability [1], which is required for
FULL (but not LIMITED).


Paul


[1] https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE

> >  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >  };
> >

On Tue, Oct 25, 2022 at 09:13:22AM +0900, Paul Elder via libcamera-devel wrote:
> On Tue, Oct 25, 2022 at 02:49:25AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Hi Nicholas,
> >
> > Thank you for the patch.
> >
> > On Mon, Oct 24, 2022 at 12:55:35AM -0500, Nicholas Roth via libcamera-devel wrote:
> > > From: Nicholas Roth <nicholas@rothemail.net>
> > >
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index ba3c547e..c536852c 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -100,6 +100,13 @@ const ControlInfoMap::Map rkisp1Controls{
> > >  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> > >  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> > >  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > > +	/* libcamera requires a fixed value for minimum frame duration,
> > > +	 * but this depends on the frame size and the rkisp1 device datasheets
> > > +	 * measure this in pixels per second. Neither the datasheets nor the driver
> > > +	 * specify a maximum. The minimum below is for 1920x1920. The maximum
> > > +	 * corresponds to two seconds. */
> > > +	{ &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },
> >
> > Could this be computed the same way we do it in the IPU3 IPA module ?
> >
> > > +	{ &controls::draft::MaxLatency, ControlInfo(0) },
> >
> > Why is this needed ?
>
> This control looks familiar in the context of android...
>
> iirc from the CTS FULL support ordeal, MaxLatency needs to be between 0
> and 4 inclusive for burst capture capability [1], which is required for
> FULL (but not LIMITED).
>

Careful, we're claiming full per-frame control capabilities with this
set to 0 :)

"PER_FRAME_CONTROL (v3.2) 0 Every frame has the requests immediately
applied.  Changing controls over multiple requests one after another
will produce results that have those controls applied atomically each
frame.

All FULL capability devices will have this as their maxLatency."

I would leave it unspecified so that the HAL will use -1

"UNKNOWN (v3.2) -1 Each new frame has some subset (potentially the
entire set) of the past requests applied to the camera settings.  By
submitting a series of identical requests, the camera device will
eventually have the camera settings applied, but it is unknown when
that exact point will be.

All LEGACY capability devices will have this as their maxLatency."

Or use the pipeline depth as its upper limit.

Nicholas was this to satisfy some correctness check, or are you
actually looking into have precise per-frame control ?

>
> Paul
>
>
> [1] https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE
>
> > >  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > >  };
> > >

> Careful, we're claiming full per-frame control capabilities with this set to 0 :)
 
Good catch— I’ll revert this line. I couldn’t easily find this in the ISP manual and wanted to see what we’d need for FULL instead of LIMITED support while I was debugging a camera2 crash.

> On Oct 25, 2022, at 8:20 AM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> 
> Careful, we're claiming full per-frame control capabilities with this
> set to 0 :)

+Kieran Bingham <kieran.bingham@ideasonboard.com> since Laurent is out.

> Could this be computed the same way we do it in the IPU3 IPA module ?

It looks like each of the computed IPU3 FrameDurationLimit values
correspond to the minimum and maximum v4l2 blanking intervals. I suppose
that makes sense, though I suspect the minimum frame duration will be
constrained by i2c bandwidth and ISP limitations at the highest resolutions
rather than the sensor blanking intervals based on the datasheet.

I'm not really sure what the best thing to do here is. I'm concerned that
if we follow the IPU3 computation, we could easily end up asking for the
ISP to push more pixels than it has throughput for. A very conservative
approach might be to cap this at a few FPS, which is the advertised rate at
the highest resolution, but I would not enjoy using the camera that way.
Curious to read others' thoughts on this.

As a side-note, I'm really not sure why the IPU3 IPA treats
FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly states:
      description: |
        The minimum and maximum (in that order) frame duration,
        expressed in microseconds.
...
      size: [2]

It looks like updateControls is only called once, during init(). We would
need a hook that changes this static configuration for each video mode.


On Tue, Oct 25, 2022 at 10:16 AM Nicholas Roth <nicholas@rothemail.net>
wrote:

> > Careful, we're claiming full per-frame control capabilities with this
> set to 0 :)
>
> Good catch— I’ll revert this line. I couldn’t easily find this in the ISP
> manual and wanted to see what we’d need for FULL instead of LIMITED support
> while I was debugging a camera2 crash.
>
> > On Oct 25, 2022, at 8:20 AM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Careful, we're claiming full per-frame control capabilities with this
> > set to 0 :)
>

Quoting Nicholas Roth (2022-10-26 05:40:11)
> +Kieran Bingham <kieran.bingham@ideasonboard.com> since Laurent is out.
> 
> > Could this be computed the same way we do it in the IPU3 IPA module ?
> 
> It looks like each of the computed IPU3 FrameDurationLimit values
> correspond to the minimum and maximum v4l2 blanking intervals. I suppose
> that makes sense, though I suspect the minimum frame duration will be
> constrained by i2c bandwidth and ISP limitations at the highest resolutions
> rather than the sensor blanking intervals based on the datasheet.
> 
> I'm not really sure what the best thing to do here is. I'm concerned that
> if we follow the IPU3 computation, we could easily end up asking for the
> ISP to push more pixels than it has throughput for. A very conservative
> approach might be to cap this at a few FPS, which is the advertised rate at
> the highest resolution, but I would not enjoy using the camera that way.
> Curious to read others' thoughts on this.
> 
> As a side-note, I'm really not sure why the IPU3 IPA treats
> FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly states:
>       description: |
>         The minimum and maximum (in that order) frame duration,
>         expressed in microseconds.
> ...
>       size: [2]

Uhhh - yes, the code over at IPAIPU3::updateControls() seems quite ...
odd. I'm not sure it's correct, it could be left over from some early
development. I'll add it to a task to take a look at.

> It looks like updateControls is only called once, during init(). We would
> need a hook that changes this static configuration for each video mode.

I believe it's supposed to be called after / during configure() too when
modes are updated. If it's missing, there's likely a bug there.



> On Tue, Oct 25, 2022 at 10:16 AM Nicholas Roth <nicholas@rothemail.net>
> wrote:
> 
> > > Careful, we're claiming full per-frame control capabilities with this
> > set to 0 :)
> >
> > Good catch— I’ll revert this line. I couldn’t easily find this in the ISP
> > manual and wanted to see what we’d need for FULL instead of LIMITED support
> > while I was debugging a camera2 crash.
> >
> > > On Oct 25, 2022, at 8:20 AM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Careful, we're claiming full per-frame control capabilities with this
> > > set to 0 :)
> >

It looks like indeed we have a way to call updateControls() at the
appropriate places, now that I've looked at the ISP code more closely.

It seems like we have at least the following options:
1 Compute the durations the way IPU3 does it (probably wrong)
2 Insert reasonable values (that sometimes might not work) and document
this in a bug
3 Do nothing, document as a bug that RKISP1 does not work as an Android HAL
without a special patch, including my suggested defaults there in the bug
and the "how to get this working on Android" guide.

Personally, I advocate for (2). Let me know what you think.

-Nicholas


On Wed, Oct 26, 2022 at 4:02 AM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Nicholas Roth (2022-10-26 05:40:11)
> > +Kieran Bingham <kieran.bingham@ideasonboard.com> since Laurent is out.
> >
> > > Could this be computed the same way we do it in the IPU3 IPA module ?
> >
> > It looks like each of the computed IPU3 FrameDurationLimit values
> > correspond to the minimum and maximum v4l2 blanking intervals. I suppose
> > that makes sense, though I suspect the minimum frame duration will be
> > constrained by i2c bandwidth and ISP limitations at the highest
> resolutions
> > rather than the sensor blanking intervals based on the datasheet.
> >
> > I'm not really sure what the best thing to do here is. I'm concerned that
> > if we follow the IPU3 computation, we could easily end up asking for the
> > ISP to push more pixels than it has throughput for. A very conservative
> > approach might be to cap this at a few FPS, which is the advertised rate
> at
> > the highest resolution, but I would not enjoy using the camera that way.
> > Curious to read others' thoughts on this.
> >
> > As a side-note, I'm really not sure why the IPU3 IPA treats
> > FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly
> states:
> >       description: |
> >         The minimum and maximum (in that order) frame duration,
> >         expressed in microseconds.
> > ...
> >       size: [2]
>
> Uhhh - yes, the code over at IPAIPU3::updateControls() seems quite ...
> odd. I'm not sure it's correct, it could be left over from some early
> development. I'll add it to a task to take a look at.
>
> > It looks like updateControls is only called once, during init(). We would
> > need a hook that changes this static configuration for each video mode.
>
> I believe it's supposed to be called after / during configure() too when
> modes are updated. If it's missing, there's likely a bug there.
>
>
>
> > On Tue, Oct 25, 2022 at 10:16 AM Nicholas Roth <nicholas@rothemail.net>
> > wrote:
> >
> > > > Careful, we're claiming full per-frame control capabilities with this
> > > set to 0 :)
> > >
> > > Good catch— I’ll revert this line. I couldn’t easily find this in the
> ISP
> > > manual and wanted to see what we’d need for FULL instead of LIMITED
> support
> > > while I was debugging a camera2 crash.
> > >
> > > > On Oct 25, 2022, at 8:20 AM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > Careful, we're claiming full per-frame control capabilities with this
> > > > set to 0 :)
> > >
>

Hi Nicholas,

On Tue, Oct 25, 2022 at 11:40:11PM -0500, Nicholas Roth wrote:
> +Kieran Bingham <kieran.bingham@ideasonboard.com> since Laurent is out.
>
> > Could this be computed the same way we do it in the IPU3 IPA module ?
>
> It looks like each of the computed IPU3 FrameDurationLimit values
> correspond to the minimum and maximum v4l2 blanking intervals. I suppose
> that makes sense, though I suspect the minimum frame duration will be
> constrained by i2c bandwidth and ISP limitations at the highest resolutions
> rather than the sensor blanking intervals based on the datasheet.
>

Mostly for sake of discussion, as you correctly noticed we report as
the camera frame duration the sensor's frame duration. I think this is
correct, as there might indeed be other bottlenecks in the capture
pipeline, but I guess it's hard to quantify them in terms of latency
and they depend on the current camera configuration, ie the number
and resolution of the streams that are processed in a given capture session.

I wonder if all platforms behaves the same when overloaded, some
might introduce latencies, some might drop buffers ? I'm also afraid
not all the numbers we would need to quantify something are actually
available...

The sensor frame duration is shared by all streams generated from the same
and can be reported as a camera global parameter.

I2c bandwidth doesn't seem a concern to me, during streaming i2c
traffic should be negligible and limited to the control of a few
sensor's parameters which are controlled at run-time, such as the
exposure time and gain.

There might of course be multiple processing steps to take into
account when actually computing the full duration of the frame since
the time it has been produced by the sensor to the time it is
delivered to applications.  Android in example has a dedicated
property for 'stalling' streams that goes through, in example, JPEG
encoding, something that is not exactly in scope for libcamera (and
which we don't fully handle in the HAL yet).

> I'm not really sure what the best thing to do here is. I'm concerned that
> if we follow the IPU3 computation, we could easily end up asking for the
> ISP to push more pixels than it has throughput for. A very conservative

I don't have numbers here and I presume they're very platform
dependent, but I don't think we actually hit that limit on IPU3 which
is where the HAL has been mostly tested on. Which platform are you
working with ?

> approach might be to cap this at a few FPS, which is the advertised rate at
> the highest resolution, but I would not enjoy using the camera that way.
> Curious to read others' thoughts on this.
>
> As a side-note, I'm really not sure why the IPU3 IPA treats
> FrameDurationLimits as a 3-tuple, when control_ids.yaml explicitly states:
>       description: |
>         The minimum and maximum (in that order) frame duration,
>         expressed in microseconds.
> ...
>       size: [2]

What you see initialized with 3 values is the ControlInfo, which
reports the max, min and default values for a control. The control
itself accepts two values as indicated by the documentation.

>
> It looks like updateControls is only called once, during init(). We would
> need a hook that changes this static configuration for each video mode.

I do see updateControls() be called by configure() as well, and I see
the pipeline handler correctly updating the ControlInfoMap of camera
controls. Am I missing something ?
>
>
> On Tue, Oct 25, 2022 at 10:16 AM Nicholas Roth <nicholas@rothemail.net>
> wrote:
>
> > > Careful, we're claiming full per-frame control capabilities with this
> > set to 0 :)
> >
> > Good catch— I’ll revert this line. I couldn’t easily find this in the ISP
> > manual and wanted to see what we’d need for FULL instead of LIMITED support
> > while I was debugging a camera2 crash.
> >
> > > On Oct 25, 2022, at 8:20 AM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Careful, we're claiming full per-frame control capabilities with this
> > > set to 0 :)
> >