| Message ID | 20240904020448.52035-9-mailingradian@gmail.com |
|---|---|
| Headers | show |
| Series | Add SDM670 camera subsystem | expand |
Hi Richard, On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: > This adds support for the camera subsystem on the Snapdragon 670. > > As of next-20240902, camss seems to be a bit broken, but the same series > works on stable (although it is much less reliable now that the CCI clock > frequency is not being assigned). I am not understanding this bit: is this series making it better or not? Can you please clarify what is broken, what is less reliable and what works? Besides, I'm reading that this series has not been tested and it makes it difficult for me to take this in, considering that you are adding a new support. Thanks, Andi
On 05/09/2024 21:09, Andi Shyti wrote: > Hi Richard, > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: >> This adds support for the camera subsystem on the Snapdragon 670. >> >> As of next-20240902, camss seems to be a bit broken, but the same series >> works on stable (although it is much less reliable now that the CCI clock >> frequency is not being assigned). > > I am not understanding this bit: is this series making it better > or not? Can you please clarify what is broken, what is less > reliable and what works? > > Besides, I'm reading that this series has not been tested and it > makes it difficult for me to take this in, considering that you > are adding a new support. > > Thanks, > Andi Actually I completely missed the statement about "much less reliable not that the CCI clock frequency is not being assigned" @Richard - what does that mean ? --- bod
On 9/4/24 05:04, Richard Acayan wrote: > This adds support for the camera subsystem on the Snapdragon 670. > > As of next-20240902, camss seems to be a bit broken, but the same series > works on stable (although it is much less reliable now that the CCI clock > frequency is not being assigned). > Second that, please elaborate on "a bit broken" camss. Regarding the CCI clock frequency, it's a supply clock and it is kind of unconnected to the CAMSS driver, here I would expect that some particular clock frequency setting either works always or does not work at all without anything in the middle. -- Best wishes, Vladimir
On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: > Hi Richard, > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: > > This adds support for the camera subsystem on the Snapdragon 670. > > > > As of next-20240902, camss seems to be a bit broken, but the same series > > works on stable (although it is much less reliable now that the CCI clock > > frequency is not being assigned). > > I am not understanding this bit: is this series making it better > or not? Can you please clarify what is broken, what is less > reliable and what works? When applying this camss series and some camera sensor patches on linux-next, the Pixel 3a seems to hang when camera capture starts. When applying the same patches on stable, the camera does not cause the Pixel 3a to hang. When these device tree properties from the previous series were removed: assigned-clocks = <&camcc CAM_CC_CCI_CLK>; assigned-clock-rates = <37500000>; the CCI would sometimes fail to probe with the error: [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 On further testing, the rate can be set to 19.2 MHz, and there would be no failure (or rather, it wouldn't happen often enough for me to witness it). > Besides, I'm reading that this series has not been tested and it > makes it difficult for me to take this in, considering that you > are adding a new support. Of course. This revision of the series wasn't submitted to rush into v6.12-rc1. It can wait until everything is resolved. When device tree maintainers comment "not tested" on the documentation, it usually means that `make dt_bindings_check DT_SCHEMA_FILES=...` gives errors or warnings (even though the device tree and driver may work on the hardware). It's a separate test and one of the things I haven't scripted into my workflow, although it's still a responsibility.
Hi Richard, On Thu, Sep 05, 2024 at 10:36:28PM GMT, Richard Acayan wrote: > On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: > > Hi Richard, > > > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: > > > This adds support for the camera subsystem on the Snapdragon 670. > > > > > > As of next-20240902, camss seems to be a bit broken, but the same series > > > works on stable (although it is much less reliable now that the CCI clock > > > frequency is not being assigned). > > > > I am not understanding this bit: is this series making it better > > or not? Can you please clarify what is broken, what is less > > reliable and what works? > > When applying this camss series and some camera sensor patches on > linux-next, the Pixel 3a seems to hang when camera capture starts. > > When applying the same patches on stable, the camera does not cause the > Pixel 3a to hang. > > When these device tree properties from the previous series were removed: > > assigned-clocks = <&camcc CAM_CC_CCI_CLK>; > assigned-clock-rates = <37500000>; > > the CCI would sometimes fail to probe with the error: > > [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency > [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 > > On further testing, the rate can be set to 19.2 MHz, and there would be > no failure (or rather, it wouldn't happen often enough for me to witness > it). > > > Besides, I'm reading that this series has not been tested and it > > makes it difficult for me to take this in, considering that you > > are adding a new support. > > Of course. This revision of the series wasn't submitted to rush into > v6.12-rc1. It can wait until everything is resolved. > > When device tree maintainers comment "not tested" on the documentation, > it usually means that `make dt_bindings_check DT_SCHEMA_FILES=...` gives > errors or warnings (even though the device tree and driver may work on > the hardware). It's a separate test and one of the things I haven't > scripted into my workflow, although it's still a responsibility. OK, thanks for clarifying. Then, please, next time, to avoid confusion, make it an RFC; or, if the series is in an advanced state with little things to improve, state it clearly in the cover letter or after the '---' section. For now, thanks a lot, I will keep the R-b's for the time being (unless the reviewers are against) and I will wait for you to know the outcome of the tests. Andi
On 06/09/2024 03:36, Richard Acayan wrote: > On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: >> Hi Richard, >> >> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: >>> This adds support for the camera subsystem on the Snapdragon 670. >>> >>> As of next-20240902, camss seems to be a bit broken, but the same series >>> works on stable (although it is much less reliable now that the CCI clock >>> frequency is not being assigned). >> >> I am not understanding this bit: is this series making it better >> or not? Can you please clarify what is broken, what is less >> reliable and what works? > > When applying this camss series and some camera sensor patches on > linux-next, the Pixel 3a seems to hang when camera capture starts. > > When applying the same patches on stable, the camera does not cause the > Pixel 3a to hang. Right so -next isn't stable that's not exactly a revelation. > When these device tree properties from the previous series were removed: > > assigned-clocks = <&camcc CAM_CC_CCI_CLK>; > assigned-clock-rates = <37500000>; > > the CCI would sometimes fail to probe with the error: Right, we don't have clk_set_rate in the cci driver. Maybe just leave the assigned clock for this submission and we can do a sweep of fixes to CCI at a later stage including setting the clock instead of having it be assigned. > > [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency > [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 > > On further testing, the rate can be set to 19.2 MHz, and there would be > no failure (or rather, it wouldn't happen often enough for me to witness > it). That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part. --- bod
Hi Bryan, Richard, On 9/6/24 15:19, Bryan O'Donoghue wrote: > On 06/09/2024 03:36, Richard Acayan wrote: >> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: >>> Hi Richard, >>> >>> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: >>>> This adds support for the camera subsystem on the Snapdragon 670. >>>> >>>> As of next-20240902, camss seems to be a bit broken, but the same series >>>> works on stable (although it is much less reliable now that the CCI clock >>>> frequency is not being assigned). >>> >>> I am not understanding this bit: is this series making it better >>> or not? Can you please clarify what is broken, what is less >>> reliable and what works? >> >> When applying this camss series and some camera sensor patches on >> linux-next, the Pixel 3a seems to hang when camera capture starts. >> >> When applying the same patches on stable, the camera does not cause the >> Pixel 3a to hang. > > Right so -next isn't stable that's not exactly a revelation. > > >> When these device tree properties from the previous series were removed: >> >> assigned-clocks = <&camcc CAM_CC_CCI_CLK>; >> assigned-clock-rates = <37500000>; >> >> the CCI would sometimes fail to probe with the error: > > Right, we don't have clk_set_rate in the cci driver. > > Maybe just leave the assigned clock for this submission and we can do a > sweep of fixes to CCI at a later stage including setting the clock > instead of having it be assigned. first of all it would be nice to confirm that the setting of a particular clock frequency is actually needed. Fortunately it's pretty trivial to check it in runtime with a temporary modification in the board dts file, namely disable CAMSS in board dts file, but keep CCI enabled, then simply scan the bus with a regular "i2cdetect" tool in runtime. If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it is needed, otherwise (and this is my expectation) it is not needed neither in the dtsi files nor in the driver. >> >> [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency >> [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 >> >> On further testing, the rate can be set to 19.2 MHz, and there would be >> no failure (or rather, it wouldn't happen often enough for me to witness >> it). > > That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part. > I read it as the setting of 37.5MHz clock frequency is not needed, please correct me. -- Best wishes, Vladimir
On Fri, Sep 06, 2024 at 04:00:32PM +0300, Vladimir Zapolskiy wrote: > Hi Bryan, Richard, > > On 9/6/24 15:19, Bryan O'Donoghue wrote: > > On 06/09/2024 03:36, Richard Acayan wrote: > > > On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: > > > > Hi Richard, > > > > > > > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: > > > > > This adds support for the camera subsystem on the Snapdragon 670. > > > > > > > > > > As of next-20240902, camss seems to be a bit broken, but the same series > > > > > works on stable (although it is much less reliable now that the CCI clock > > > > > frequency is not being assigned). > > > > > > > > I am not understanding this bit: is this series making it better > > > > or not? Can you please clarify what is broken, what is less > > > > reliable and what works? > > > > > > When applying this camss series and some camera sensor patches on > > > linux-next, the Pixel 3a seems to hang when camera capture starts. > > > > > > When applying the same patches on stable, the camera does not cause the > > > Pixel 3a to hang. > > > > Right so -next isn't stable that's not exactly a revelation. > > > > > > > When these device tree properties from the previous series were removed: > > > > > > assigned-clocks = <&camcc CAM_CC_CCI_CLK>; > > > assigned-clock-rates = <37500000>; > > > > > > the CCI would sometimes fail to probe with the error: > > > > Right, we don't have clk_set_rate in the cci driver. > > > > Maybe just leave the assigned clock for this submission and we can do a > > sweep of fixes to CCI at a later stage including setting the clock > > instead of having it be assigned. > > first of all it would be nice to confirm that the setting of a particular > clock frequency is actually needed. > > Fortunately it's pretty trivial to check it in runtime with a temporary > modification in the board dts file, namely disable CAMSS in board dts file, > but keep CCI enabled, then simply scan the bus with a regular "i2cdetect" > tool in runtime. > > If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it > is needed, otherwise (and this is my expectation) it is not needed neither > in the dtsi files nor in the driver. > > > > > > > [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency > > > [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 > > > > > > On further testing, the rate can be set to 19.2 MHz, and there would be > > > no failure (or rather, it wouldn't happen often enough for me to witness > > > it). > > > > That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part. > > > > I read it as the setting of 37.5MHz clock frequency is not needed, please > correct me. It is not. My test setup just needs specific EPROBE_DEFER behaviour (my setup being postmarketOS with a full-disk encryption password prompt and camcc-sdm845 loaded after mounting the root filesystem). In drivers/base/platform.c, the platform_probe() function calls of_clk_set_defaults() then dev_pm_domain_attach() prior to probing the driver: static int platform_probe(struct device *_dev) { ... ret = of_clk_set_defaults(_dev->of_node, false); if (ret < 0) return ret; ret = dev_pm_domain_attach(_dev, true); if (ret) goto out; if (drv->probe) { ret = drv->probe(dev); if (ret) dev_pm_domain_detach(_dev, true); } ... } When handling the assigned-clock-rates property, of_clk_get_hw_from_clkspec() eventually returns ERR_PTR(-EPROBE_DEFER), being propagated all the way. When handling the power-domains property (if not avoided by deferring with the assigned clock), __genpd_dev_pm_attach() returns a value returned by driver_deferred_probe_check_state(), which is immediately -ETIMEDOUT.
Hi Richard. On 9/28/24 01:23, Richard Acayan wrote: > On Fri, Sep 06, 2024 at 04:00:32PM +0300, Vladimir Zapolskiy wrote: >> Hi Bryan, Richard, >> >> On 9/6/24 15:19, Bryan O'Donoghue wrote: >>> On 06/09/2024 03:36, Richard Acayan wrote: >>>> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: >>>>> Hi Richard, >>>>> >>>>> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: >>>>>> This adds support for the camera subsystem on the Snapdragon 670. >>>>>> >>>>>> As of next-20240902, camss seems to be a bit broken, but the same series >>>>>> works on stable (although it is much less reliable now that the CCI clock >>>>>> frequency is not being assigned). >>>>> >>>>> I am not understanding this bit: is this series making it better >>>>> or not? Can you please clarify what is broken, what is less >>>>> reliable and what works? >>>> >>>> When applying this camss series and some camera sensor patches on >>>> linux-next, the Pixel 3a seems to hang when camera capture starts. >>>> >>>> When applying the same patches on stable, the camera does not cause the >>>> Pixel 3a to hang. >>> >>> Right so -next isn't stable that's not exactly a revelation. >>> >>> >>>> When these device tree properties from the previous series were removed: >>>> >>>> assigned-clocks = <&camcc CAM_CC_CCI_CLK>; >>>> assigned-clock-rates = <37500000>; >>>> >>>> the CCI would sometimes fail to probe with the error: >>> >>> Right, we don't have clk_set_rate in the cci driver. >>> >>> Maybe just leave the assigned clock for this submission and we can do a >>> sweep of fixes to CCI at a later stage including setting the clock >>> instead of having it be assigned. >> >> first of all it would be nice to confirm that the setting of a particular >> clock frequency is actually needed. >> >> Fortunately it's pretty trivial to check it in runtime with a temporary >> modification in the board dts file, namely disable CAMSS in board dts file, >> but keep CCI enabled, then simply scan the bus with a regular "i2cdetect" >> tool in runtime. >> >> If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it >> is needed, otherwise (and this is my expectation) it is not needed neither >> in the dtsi files nor in the driver. >> >>>> >>>> [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency >>>> [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 >>>> >>>> On further testing, the rate can be set to 19.2 MHz, and there would be >>>> no failure (or rather, it wouldn't happen often enough for me to witness >>>> it). >>> >>> That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part. >>> >> >> I read it as the setting of 37.5MHz clock frequency is not needed, please >> correct me. > > It is not. My test setup just needs specific EPROBE_DEFER behaviour > (my setup being postmarketOS with a full-disk encryption password prompt > and camcc-sdm845 loaded after mounting the root filesystem). Good, let the assigned clock frequency be dropped from the dtsi file then. > In drivers/base/platform.c, the platform_probe() function calls > of_clk_set_defaults() then dev_pm_domain_attach() prior to probing the > driver: > > static int platform_probe(struct device *_dev) > { > ... > ret = of_clk_set_defaults(_dev->of_node, false); > if (ret < 0) > return ret; > > ret = dev_pm_domain_attach(_dev, true); > if (ret) > goto out; > > if (drv->probe) { > ret = drv->probe(dev); > if (ret) > dev_pm_domain_detach(_dev, true); > } > ... > } > > When handling the assigned-clock-rates property, > of_clk_get_hw_from_clkspec() eventually returns ERR_PTR(-EPROBE_DEFER), > being propagated all the way. > > When handling the power-domains property (if not avoided by deferring > with the assigned clock), __genpd_dev_pm_attach() returns a value > returned by driver_deferred_probe_check_state(), which is immediately > -ETIMEDOUT. I grasp it from the problem description, thank you for the explanation. For sake of simplicity please make camcc-sdm845 as a built-in driver while testing, it will allow to progress with the platform CAMSS support. The issue with the observed ETIMEDOUT is generic and it's kind of unrelated to the CCI/CAMSS support on SDM670. -- Best wishes, Vladimir