Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow IMX477 and IMX296 to be always on, to simplify multi-camera sync #5774

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

njhollinghurst
Copy link
Contributor

The issue is that when the cameras are powered down, they clamp sync signals (XVS, XHS, XTRIG) to ground. As they are active-low, and not all the cameras start streaming at exactly the same time, it could result in spurious triggers.

A previous workaround involved low-impedance pull-ups so that all cameras would see a high quiescent level. This is potentially unsafe and does not scale to large multi-camera setups.

This change allows cameras to be brought up with XVS, XHS, XTRIG pins in a high-impedance state.

@6by9
Copy link
Contributor

6by9 commented Dec 6, 2023

Looks reasonable to me.

imx477_write_reg(imx477, IMX477_REG_MC_MODE,
IMX477_REG_VALUE_08BIT, (trigger_mode > 0) ? 1 : 0);
imx477_write_reg(imx477, IMX477_REG_MS_SEL,
IMX477_REG_VALUE_08BIT, (trigger_mode <= 1) ? 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the '<=' a typo? It doesn't fit the pattern of IMX477_REG_XVS_IO_CTRL and IMX477_REG_EXTOUT_EN, but if trigger_mode is guaranteed to only be 1 or 2 then the behaviour is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to get the correct state for trigger mode 0. We want it to match the reset state (the Software Reference Manual p137 mentions only "default" which I guess is the same thing: values 0,1,0,0 respectively. The doc doesn't explain the difference between MS_SEL and EXTOUT_EN but from the names it sounds reasonable. It might be that they are ignored when MC_MODE==0).

}
/* Set vsync trigger mode: 0=standalone, 1=source, 2=sink */
imx477_write_reg(imx477, IMX477_REG_MC_MODE,
IMX477_REG_VALUE_08BIT, (trigger_mode > 0) ? 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the ? 1 : 0s look unnecessary, but they may be acceptable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It (almost) matches the previous style, but can be removed.

Copy link
Contributor

@naushir naushir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@njhollinghurst
Copy link
Contributor Author

I have re-drafted this while investigating an interaction with XVS sync out -- there may be some (pre-existing?) glitch on start streaming which perhaps needs those register writes to be re-ordered; and when we stop streaming it now does something weird (which we didn't previously see because the camera would be powered down).

@njhollinghurst
Copy link
Contributor Author

Update: Nearly got this right, for IMX477 at least. There's still an awkward case for multi-camera sync. The problem is that after stopping the master camera streaming (now without POR) and configuring it to start again, XVS can start to emit spurious pulses every 2ms or so (which can cause the sink camera to get out of step).

By moving the I2C writes for trigger mode closer to the "start streaming" write, the problem can be prevented 90% of the time, but it remains sensitive to the real time interval between I2C writes. Using a multi-write transfer may improve it further but would clutter the driver (and is probably not watertight). cc @P33M

One hopes that a different set of register incantations might cure this, but I haven't discovered one yet.

Don't assume the camera has been reset each time we start streaming,
but always write registers relating to trigger_mode, even in mode 0.

IMX477: Stop driving XVS on stop streaming, to avoid spurious pulses.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
Leave the camera's power supplies up, to prevent the camera
clamping its 1.8V digital I/Os to ground. This may be useful
when synchronizing multiple camera systems using XVS or XTRIG.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst
Copy link
Contributor Author

I've pushed the mitigations against IMX477 jabbering (not needed for IMX296, as we only support that with external input).
I think this is as good as we can manage for now, and a monotonic improvement.

@njhollinghurst njhollinghurst marked this pull request as ready for review December 15, 2023 16:38
@pelwell pelwell merged commit 3ed57f2 into rpi-6.1.y Dec 17, 2023
12 checks passed
@popcornmix popcornmix deleted the camera-always-on branch December 17, 2023 13:45
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Dec 21, 2023
kernel: ARM: dts: bcm2712-rpi-5-b: Allow RTC to be disabled
See: raspberrypi/linux#5803

kernel: Improve Pi 5 I2C bus timing
See: raspberrypi/linux#5802

kernel: input: edt-ft5x06: Correct prefix length in snprintf
See: raspberrypi/linux#5798

kernel: Allow IMX477 and IMX296 to be always on, to simplify multi-camera sync
See: raspberrypi/linux#5774
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Dec 21, 2023
kernel: ARM: dts: bcm2712-rpi-5-b: Allow RTC to be disabled
See: raspberrypi/linux#5803

kernel: Improve Pi 5 I2C bus timing
See: raspberrypi/linux#5802

kernel: input: edt-ft5x06: Correct prefix length in snprintf
See: raspberrypi/linux#5798

kernel: Allow IMX477 and IMX296 to be always on, to simplify multi-camera sync
See: raspberrypi/linux#5774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants