Re: Re: [RFC PATCH 6/8] media: i2c: ov2312: add Omnivison OV2312 driver

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Jay,

On 10/3/26 05:05, Jai Luthra wrote:
> Hi Mirela,
>
> Quoting Mirela Rabulea (2026-10-02 21:46:11)
>> Hi Rishikesh, Jay, Laurent, Hans, Sakari,
>>
>> On 9/25/26 16:29, Rishikesh Donadkar wrote:
>>> From: Jai Luthra <j-luthra@ti.com>
>>>
>>> Omnivision OV2312 is an RGB-IR sensor, i.e. it uses a 4x4 R,G,B,Ir bayer
>>> pattern to capture both visible and near-infrared light. Every alternate
>>> frame, the sensor changes the exposure and IR flash strobe registers to
>>> stream an -
>>> A. IR-dominant frame on CSI-2 virtual channel 0
>>> B. RGB-dominant frame on CSI-2 virtual channel 1
>>>
>>> These A/B frames are routed as separate v4l2 streams, which may be
>>> mapped to two separate /dev/videoX nodes by the CSI-RX DMA driver.
>>>
>>> Both of these streams are captured at a resolution of 1600x1301, 30 fps
>>> each (60fps total). The extra row (1301 vs 1300) is an embedded line
>>> prepended to each frame by the sensor, containing the following register
>>> values:
>>>     0x4813 - VC (Virtual Channel)
>>>     0x321A - Group ID
>>>     0x3920 - Strobe
>>>     0x3501 - Exposure HI
>>>     0x3502 - Exposure LO
>>>     0x3508 - Gain HI
>>>     0x3509 - Gain LO
>>>     0x350e - Current Exposure HI
>>>     0x350f - Current Exposure LO
>>>
>>> This driver also supports a few v4l2 controls like horizontal/vertical
>>> flip, multi exposure and multi gain controls.
>>>
>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> [...]
>>> +static int ov2312_set_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> +       struct ov2312 *ov2312 = container_of(ctrl->handler,
>>> +                                            struct ov2312, ctrls);
>>> +       int ret;
>>> +
>>> +       /*
>>> +        * If the device is not powered up by the host driver do
>>> +        * not apply any controls to H/W at this time. Instead
>>> +        * the controls will be restored right after power-up.
>>> +        */
>>> +       if (pm_runtime_suspended(ov2312->dev))
>>> +               return 0;
>>> +
>>> +       switch (ctrl->id) {
>>> +       case V4L2_CID_EXPOSURE_MULTI:
>>> +       case V4L2_CID_AGAIN_MULTI:
>>> +       case V4L2_CID_DGAIN_MULTI:
>>> +               dev_dbg(ov2312->dev, "debug: %s: %s = [%u, %u]\n", __func__,
>>> +                               ctrl->name, ctrl->p_new.p_u32[0], ctrl->p_new.p_u32[1]);
>>> +
>>> +               ret = ov2312_set_AB_mode(ov2312);
>> So, the group hold for A/B context is set right away, when the control
>> arrives.
>>
>> While working with the Omnivision OX05B1S, which is also an RGB-IR
>> sensor, we run into this problem:
>>
>> The normal expected sequence is that the sensor will output alternating
>> frames VC0, VC1, VC0, VC1,...
>>
>> But  when user space tries to do automatic exposure and gain control via
>> v4l2 muti controls, if the driver applies the values immediately, if the
>> virtual channels are not switched within the proper timeframe, it is
>> possible to run into frame duplication (no more nice alternating frames
>> VC0, VC1, VC0, VC1,...but duplicate VC0,VC0 or VC1,VC1).
> Yes I remember seeing a similar thing on OV2312 as well when the AE/AGC
> algorithm is turned on, and thus exposure and gain controls are updated
> frequently.
>
> In our case the streams get mixed, so in the captured frames for VC0
> (IR-dominant) we would occassionally see frames with the exposure/gain
> values that we expect in VC1 (RGB-dominant) and vice-versa.
>
> I had assumed this mixing was due to asynchronous nature of the control
> updates coming in, where the exposure+gain latch on with a delay of 2
> frames, but the rest of the registers in the group (like VC ID, and IR
> strobe) have a variable delay depending upon when the group registers are
> programmed.
>
> Without the AE/AGC the frames are stable (no mixing), so we must be talking
> about the same or very similar issues.
>
>> The information we received from the sensor vendor is that group0 update
>> needs to be between 2 group0 launchpoints (similar for group 1). We can
>> use the status register to query the currently active context, and in
>> order to avoid frame duplication we can update each group only when its
>> context is active.
>>
> Ah, thanks for the pointer, a status register for group launch points
> sounds quite useful.
>
>> This is problematic in the v4l2-api context, it implies that even while
>> streaming, a v4l2 control cannot be committed to sensor registers right
>> away.
>>
>> Even with workarounds in the sensor driver, to defer for later the
>> updates for the inactive context, it is still problematic: defer for how
>> long, and problems with overloaded systems, a stress test can bring us
>> in a broken VC sequence, as there is no atomic way to determine the
>> current active context + update the right group.  A broken VC sequence
>> shows up for example in libcamera as lost frames.
>>
> Indeed, I think we need some sort of a mechanism to enforce timings of
> control updates in V4L2 framework. And I don't yet know if system stress
> would make this impossible to ensure without enabling RT?
>
> But knowing the sensor hardware provides status registers is a good sign
> that it should be possible to fix.
>
>> Rishikesh, Jai,
>>
>>    did you notice this problem on OV2312? A way to reproduce this is to
>> stress the driver with frequent repeated set controls (for the
>> multi-controls), and observe broken VC sequence (I observed it with
>> libcamera and on the CSI analyzer).
>>
> Yes frequent control updates reliably reproduce the stream mixing on OV2312
> as well.
>
> Would it be possible for you to share the downstream driver or the status
> register you have used? We could experiment if the same solution helps on
> OV2312 too.

Sure,

here is the latest driver in nxp tree, please be aware that it is ahead 
of the upstream version, with some experimental features:

https://github.com/nxp-imx/linux-imx/tree/lf-6.18.y/drivers/media/i2c/ox05b1s

This is the status register we query to determine the active context:

#define OX05B1S_REG_GH_SEL_REAL        CCI_REG8(0x322d)

Today I also sent the v5 of ox05b1s RGB-Ir driver upstream (less features):

https://lore.kernel.org/all/20261005175102.2358881-1-mirela.rabulea@nxp.com/

>
>> Laurent, Hans, Sakari,
>>
>> did you encounter similar situations? Any comments or proposals? The
>> concern here, to summarize, is: v4l2 control cannot be committed to
>> sensor registers right away (even when streaming) and we are also unsure
>> when the right moment to perform the register access may come.
>>
> I think this is a good candidate for discussion in the upcoming LPC BoF
> session [1]. Are you planning to travel to EOSS/LPC in Prague next week?
>
> I won't be travelling unfortunately, but hopefully Rishikesh, Laurent and
> others can discuss this.
>
> [1]: https://lore.kernel.org/all/22a0424b-c67d-4dcc-a31c-26acd764c653@ti.com/

Thanks for letting me know. Unfortunately I will also not travel, but a 
few folks from NXP will be there (cc-ed).

Daniel Baluta will be at OSS+ELC, leaving on 6 october from Bucharest, I 
asked him to attend the RGB-Ir presentation at ELC
Tomas Babinec will be meeting IoB in Prague on Wednesday, but will not 
be at LPC/OSS+ELC
Frank Li, I asked him to attend RGB-Ir presentation at LPC

If it is possible for me to attend remotely these 2 RGB-Ir 
presentations, I will do so.

Best regards,

Mirela

>> Regards,
>>
>> Mirela
>>
> Thanks,
> Jai
>
>>> +               break;
>>> +
>>> +       case V4L2_CID_HFLIP:
>>> +       case V4L2_CID_VFLIP:
>>> +               ret = ov2312_set_orientation(ov2312);
>>> +               break;
>>> +
>>> +       default:
>>> +               ret = -EINVAL;
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>