[libcamera-devel] [PATCH RFC] rkisp1: adjust vblank to target framerate

[Laurent Pinchart]

Hello,

On Tue, Jun 06, 2023 at 07:27:12PM +0200, Benjamin Bara via libcamera-devel wrote:
> Hi Jacopo,
> 
> thanks for the feedback and sorry for the late response.

No need to apologize, or I'll have to do so too :-)

Would you mind not stripping the context in e-mail replies ? Doing so
makes it more difficult to jump in the middle of the conversation. If
there are very large parts that are clearly not relevant for any further
discussion on the topic then they can be trimmed, but in general
avoiding trimming would be nice.

This is a workflow issue, and I don't want to push everybody to comply
with my personal preferences, so if it causes any issue for you, please
follow the ancient wisdom from [1] and ignore this whole comment.

https://objects-us-east-1.dream.io/secrettoeverybody/images/paynoattention.png

> On Wed, 24 May 2023 at 11:04, Jacopo Mondi wrote:
> >
> >    I wonder if we shouldn't instead remove the call to setControls(0)
> >    in IPA::start() and return a list of v4l2 controls from
> >    IPA::start() as raspberrypi does so that the new VBLANK EXPOSURE
> >    and GAIN are computed all on the IPA side by re-using
> >    updateControls() which re-computes the limits for the
> >    Camera::controls as well.
> >
> >     If I'm not mistaken, with your current implementation the
> >     Camera::controls limits are not updated after start(), right ?
> 
> Exactly, they aren't.
> As I am fairly new to libcamera and so far only used libcamera in
> combination with gst-launch: Is it possible to change the frame duration
> after start() is called? Because IMHO, vblank is static, as long as the
> frame duration is static. Obviously, if the frame duration limit is
> dynamic after start() is called, then it would make sense to also have
> vblank recalculated afterwards. Under my assumption of a static frame
> duration, I guess it would even make sense to put it "before" or outside
> of the IPA-specific ph::start(), as it is just related to the camera
> sensor, and independent of the IPA - but I guess start() is the first
> call to libcamera where the frame durations are actually known.

The FrameDurationLimits control is meant to be dynamic. That's how it is
implemented for Raspberry Pi, and I think we should do the same on other
platforms.

> >     The only drawback I see with my proposal is that the
> >     re-computation of the EXPOSURE v4l2 control limits has to be done
> >     manually in the IPA instead of relaying on it being done by the
> >     driver when setting a new VBLANK as per your current
> >     implementation.
> 
> Yes, I think so too. This needs to be implemented per-sensor in the
> helper I guess. I skimmed a little bit through the camera sensor drivers
> and it looks like most of the drivers adapt the v4l2 exposure limits as
> soon as vblank is changed (except e.g. imx258 or imx415). So I guess at
> least it seems to be quite reliable.

I'm *really* annoyed by the V4L2 API here.

The fact that we need to modify VBLANK first before changing the
exposure, in two separate ioctls, is bad. I can live with it for the
time being.

The fact that the VBLANK limits change when the sensor output format is
changed is really bad. The fact that most sensors have a fixed
constraint on the vertical total size, not on vertical blank itself,
upsets me. Our troubles don't come from hardware constraints, but from a
really bad API design decision. If we exposed the vertical total size
instead of the vertical blanking, the maximum value would be fixed, and
userspace would be so much simpler.

I don't want to live with this anymore, we should really fix it on the
kernel side. I'd be happy to treat whoever makes my life less miserable
by fixing this issue to dinner at whatever conference we would attend
next :-)

> So IMHO, for the "given frame duration limit" case, I guess it just
> boils down to the question if the limits can change after calling
> start(). For other use cases, like reducing the frame rate to increase
> exposure when in saturation (or similar), your suggestion might fit
> better. What do you think?

-- 
Regards,

Laurent Pinchart