-
Notifications
You must be signed in to change notification settings - Fork 31
media: mt9m114: Changes to make it work with atomisp devices #7
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
Conversation
Make aptina_pll_calculate() debug log the calculated p1 min and max values, this makes it easier to see how the m, n and p1 values were chosen. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org>
Add support for platforms that do not have a clock provider, but instead specify the clock frequency by using the "clock-frequency" property. E.g. ACPI platforms turn the clock on/off through ACPI power-resources depending on the runtime-pm state, so there is no clock provider. Signed-off-by: Hans de Goede <hansg@kernel.org> --- Note as discussed during review of v1, this needs to be moved over to the solution from: https://lore.kernel.org/r/20250321130329.342236-1-mehdi.djait@linux.intel.com once that has landed upstream. I'll submit a follow-up patch to move to that solution once it has landed upstream.
Before this change the driver used hardcoded PLL m, n and p values to achieve a 48MHz pixclock when used with an external clock with a frequency of 24 MHz. Use aptina_pll_calculate() to allow the driver to work with different external clock frequencies. The m, n, and p values will be unchanged with a 24 MHz extclk and this has also been tested with a 19.2 MHz clock where m gets increased from 32 to 40. Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org> --- Changes in v2: - Add select VIDEO_APTINA_PLL to Kconfig - Use correct aptina_pll_limits - Honor 80 chars limit
As the comment above the defines says, the minimum values are undocumented so the lowest values seen in register lists are used. The version of the mt9m114 driver shipped together with the atomisp code uses 21 for vblank in its register lists, lower MT9M114_MIN_VBLANK accordingly. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org>
The current default hblank and vblank values are based on reaching 30 fps with the pixel-array outputting 1280x960, but the default format for the pixel-array source pad and the isp sink pad is 1296x976, correct the default hblank and vblank values to take this into account. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org> --- Changes in v2: - Update comment about resolution / pixrate / FPS to: * Set the default to achieve full resolution (1296x976 analog crop * rectangle, 1280x960 output size) at 30fps with a 48 MHz pixclock.
The PLL gets programmed to achieve a 48 MHz pixelclock, with the current vblank + hblank defaults this results in a fps of: 48000000 / ((1296 + 307) * (976 + 23) = 29.974 fps Tweak the defaults to get closer to 30 fps: 48000000 / ((1296 + 308) * (976 + 21) = 30.015 fps This improves things from being 0.026 fps too low to 0.015 fps too high. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org>
mt9m114_probe() requests the reset GPIO in output low state: sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); and then almost immediately afterwards calls mt9m114_power_on() which does: gpiod_set_value(sensor->reset, 1); fsleep(duration); gpiod_set_value(sensor->reset, 0); which means that if the reset pin was high before this code runs that it will very briefly be driven low because of passing GPIOD_OUT_LOW when requesting the GPIO only to be driven high again possibly directly after that. Such a very brief driving low of the reset pin may put the chip in a confused state. Request the GPIO in high (reset the chip) state instead to avoid this, turning the initial gpiod_set_value() in mt9m114_power_on() into a no-op. and the fsleep() ensures that it will stay high long enough to properly reset the chip. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org>
Put sensor back in reset on power-down. Signed-off-by: Hans de Goede <hansg@kernel.org> --- Changes in v2 - After setting reset high wait 20 clk cycles before disabling the clk and regulators
As indicated by the comment in mt9m114_ifp_set_fmt(): /* If the output format is RAW10, bypass the scaler. */ if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) ... The intend of the driver is that the scalar is bypassed when the ISP source/output pad's pixel-format is set to MEDIA_BUS_FMT_SGRBG10_1X10. This patch makes 2 changes which are required to get this to work properly: 1. Set the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit in the MT9M114_CAM_OUTPUT_FORMAT register. 2. Disable cropping/composing by setting crop and compose selections on the ISP sink/input format to the format widthxheight @ 0x0. Signed-off-by: Hans de Goede <hansg@kernel.org> --- Changes in v2: - When bypassing the scalar make ifp_get_selection() / ifp_set_selection() fill sel->r with a rectangle of (0,0)/wxh and return 0 instead of returning -EINVAL
Drop the start-, stop-streaming sequence from initialize. When streaming is started with a runtime-suspended sensor, mt9m114_start_streaming() will runtime-resume the sensor which calls mt9m114_initialize() immediately followed by calling mt9m114_set_state(ENTER_CONFIG_CHANGE). This results in the following state changes in quick succession: mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING mt9m114_set_state(ENTER_SUSPEND) -> transitions to SUSPENDED mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING these quick state changes confuses the CSI receiver on atomisp devices causing streaming to not work. Drop the state changes from mt9m114_initialize() so that only a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made when streaming is started with a runtime-suspend sensor. This means that the sensor may have config changes pending when mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(), when streaming was not started within the 1 second runtime-pm timeout. Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from mt9m114_runtime_suspend() if necessary. Signed-off-by: Hans de Goede <hansg@kernel.org>
With IPU# bridges, endpoints may only be created when the IPU bridge is initialized. This may happen after the sensor driver's first probe(). Signed-off-by: Hans de Goede <hansg@kernel.org>
Add support for the mt9m114 sensor being enumerated through ACPI using the INT33F0 HID as found on the Asus T100TA. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Hans de Goede <hansg@kernel.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the MT9M114 sensor driver to integrate new PLL configuration support via the aptina-pll interface, refactors PLL macros and structures, and adds support for ACPI while refining sensor configuration for RAW10.
- Update PLL divider macros and structure to use off‐by‐one corrections
- Introduce new configuration flags and adjust crop handling for RAW10 mode
- Add ACPI device ID support and use optional clock/reset retrieval
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
drivers/media/i2c/mt9m114.c | Integrates aptina-pll, adjusts PLL calculations, and refines sensor configuration handling |
drivers/media/i2c/aptina-pll.c | Adds debugging information in PLL calculation |
drivers/media/i2c/Kconfig | Adds dependency on VIDEO_APTINA_PLL |
This pull-req was created to test Copilot code-review, see: This is not intended for merging, closing this now. |
A crash in conntrack was reported while trying to unlink the conntrack entry from the hash bucket list: [exception RIP: __nf_ct_delete_from_lists+172] [..] #7 [ff539b5a2b043aa0] nf_ct_delete at ffffffffc124d421 [nf_conntrack] #8 [ff539b5a2b043ad0] nf_ct_gc_expired at ffffffffc124d999 [nf_conntrack] #9 [ff539b5a2b043ae0] __nf_conntrack_find_get at ffffffffc124efbc [nf_conntrack] [..] The nf_conn struct is marked as allocated from slab but appears to be in a partially initialised state: ct hlist pointer is garbage; looks like the ct hash value (hence crash). ct->status is equal to IPS_CONFIRMED|IPS_DYING, which is expected ct->timeout is 30000 (=30s), which is unexpected. Everything else looks like normal udp conntrack entry. If we ignore ct->status and pretend its 0, the entry matches those that are newly allocated but not yet inserted into the hash: - ct hlist pointers are overloaded and store/cache the raw tuple hash - ct->timeout matches the relative time expected for a new udp flow rather than the absolute 'jiffies' value. If it were not for the presence of IPS_CONFIRMED, __nf_conntrack_find_get() would have skipped the entry. Theory is that we did hit following race: cpu x cpu y cpu z found entry E found entry E E is expired <preemption> nf_ct_delete() return E to rcu slab init_conntrack E is re-inited, ct->status set to 0 reply tuplehash hnnode.pprev stores hash value. cpu y found E right before it was deleted on cpu x. E is now re-inited on cpu z. cpu y was preempted before checking for expiry and/or confirm bit. ->refcnt set to 1 E now owned by skb ->timeout set to 30000 If cpu y were to resume now, it would observe E as expired but would skip E due to missing CONFIRMED bit. nf_conntrack_confirm gets called sets: ct->status |= CONFIRMED This is wrong: E is not yet added to hashtable. cpu y resumes, it observes E as expired but CONFIRMED: <resumes> nf_ct_expired() -> yes (ct->timeout is 30s) confirmed bit set. cpu y will try to delete E from the hashtable: nf_ct_delete() -> set DYING bit __nf_ct_delete_from_lists Even this scenario doesn't guarantee a crash: cpu z still holds the table bucket lock(s) so y blocks: wait for spinlock held by z CONFIRMED is set but there is no guarantee ct will be added to hash: "chaintoolong" or "clash resolution" logic both skip the insert step. reply hnnode.pprev still stores the hash value. unlocks spinlock return NF_DROP <unblocks, then crashes on hlist_nulls_del_rcu pprev> In case CPU z does insert the entry into the hashtable, cpu y will unlink E again right away but no crash occurs. Without 'cpu y' race, 'garbage' hlist is of no consequence: ct refcnt remains at 1, eventually skb will be free'd and E gets destroyed via: nf_conntrack_put -> nf_conntrack_destroy -> nf_ct_destroy. To resolve this, move the IPS_CONFIRMED assignment after the table insertion but before the unlock. Pablo points out that the confirm-bit-store could be reordered to happen before hlist add resp. the timeout fixup, so switch to set_bit and before_atomic memory barrier to prevent this. It doesn't matter if other CPUs can observe a newly inserted entry right before the CONFIRMED bit was set: Such event cannot be distinguished from above "E is the old incarnation" case: the entry will be skipped. Also change nf_ct_should_gc() to first check the confirmed bit. The gc sequence is: 1. Check if entry has expired, if not skip to next entry 2. Obtain a reference to the expired entry. 3. Call nf_ct_should_gc() to double-check step 1. nf_ct_should_gc() is thus called only for entries that already failed an expiry check. After this patch, once the confirmed bit check passes ct->timeout has been altered to reflect the absolute 'best before' date instead of a relative time. Step 3 will therefore not remove the entry. Without this change to nf_ct_should_gc() we could still get this sequence: 1. Check if entry has expired. 2. Obtain a reference. 3. Call nf_ct_should_gc() to double-check step 1: 4 - entry is still observed as expired 5 - meanwhile, ct->timeout is corrected to absolute value on other CPU and confirm bit gets set 6 - confirm bit is seen 7 - valid entry is removed again First do check 6), then 4) so the gc expiry check always picks up either confirmed bit unset (entry gets skipped) or expiry re-check failure for re-inited conntrack objects. This change cannot be backported to releases before 5.19. Without commit 8a75a2c ("netfilter: conntrack: remove unconfirmed list") |= IPS_CONFIRMED line cannot be moved without further changes. Cc: Razvan Cojocaru <rzvncj@gmail.com> Link: https://lore.kernel.org/netfilter-devel/20250627142758.25664-1-fw@strlen.de/ Link: https://lore.kernel.org/netfilter-devel/4239da15-83ff-4ca4-939d-faef283471bb@gmail.com/ Fixes: 1397af5 ("netfilter: conntrack: remove the percpu dying list") Signed-off-by: Florian Westphal <fw@strlen.de> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
No description provided.