hal: re-enable the sensor clock before every probe, not once per process - #184
Merged
Conversation
getsensorid() reads the sensor over i2c, so the sensor has to be clocked to
answer. On Ingenic that clock is turned on by setup_hal_ingenic(), which runs
from hw_detect_system() inside getchipname() -- and getchipname() caches:
if (*sysid) return sysid;
so the HAL setup happens exactly once per process. That is fine while nothing
takes the clock away again. Something does: a vendor SDK gates it off when it
tears its pipeline down, leaving /proc/jz/clock/cgu_cim/enable reading
"disabled". Every probe after that reads an unclocked sensor and reports that
the board has none.
The symptom is confusing in a specific way. A fresh process always gets the
right answer, because it runs the HAL setup again on its first getchipname() --
so `ipcinfo -l` from a shell answers correctly at the very moment a long-lived
caller is being told there is no sensor. That reads as a caller bug, or as
flaky hardware, rather than as this.
So make it a HAL hook and call it before each probe. NULL for every SoC that
needs nothing done, which is all of them bar Ingenic today, and cleared in
setup_hal_fallback() so detection cannot inherit a previous target's.
Measured on a t31 (sc2332) with a caller that probes once per pipeline reload:
before first probe finds sc2332_i2c, every later one finds nothing
after sc2332_i2c on all of them, across repeated SDK teardowns
Found from majestic, where it surfaced as "sensor autodetection failed" and
then "Cannot start SDK" on every SIGHUP reload.
PR Summary by QodoRe-enable Ingenic sensor clock before every probe
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getsensorid()reads the sensor over i2c, so the sensor has to be clocked to answer. On Ingenic that clock is turned on bysetup_hal_ingenic(), which runs fromhw_detect_system()insidegetchipname()— andgetchipname()caches:So the HAL setup happens exactly once per process. That holds while nothing takes the clock away again — and something does. A vendor SDK gates it off when it tears its pipeline down:
/proc/jz/clock/cgu_cim/enableenableddisabledEvery probe after that reads an unclocked sensor and reports that the board has none.
Why this is easy to misdiagnose
A fresh process always gets the right answer, because its first
getchipname()runs the HAL setup again and turns the clock back on. Soipcinfo -lfrom a shell answerssc2332_i2cat the very moment a long-lived caller is being told there is no sensor.That reads as a bug in the caller, or as flaky hardware. It also makes it look intermittent: any probe that happens to follow an
ipcinforun succeeds, as does one attempted while the SDK is already down — there was no teardown to gate the clock off.Change
Make it a HAL hook and call it before each probe, rather than relying on a setup that runs once.
NULLfor every SoC that needs nothing done — all of them but Ingenic today — and cleared insetup_hal_fallback()so detection cannot inherit a previous target's hook.27 lines, no behaviour change on any SoC that doesn't set it.
Verified on hardware
t31 (sc2332), with a caller that probes once per pipeline reload:
sc2332_i2c, every later one finds nothingsc2332_i2con all of them, across repeated SDK teardownsMeasured by running that caller with its own detection cache disabled, so every reload performed a real i2c probe:
Context
Found from majestic, where this surfaced as
sensor autodetection failedand thenCannot start SDKon every SIGHUP reload — the camera lost video until someone restarted it by hand. Majestic is fixing its side by caching the first detection, since the sensor cannot change while the process runs; this is the underlying reason a re-probe could not work, and fixing it here means any consumer that probes more than once gets a correct answer.Only Ingenic is verified on hardware — it is also the only SoC whose behaviour changes.