On 18/01/2023 17:33, Stephan Gerhold wrote:
On Wed, Jan 18, 2023 at 11:50:20AM +0000, Bryan O'Donoghue wrote:
On 18/01/2023 09:59, Stephan Gerhold wrote:
On Tue, Jan 17, 2023 at 02:48:43AM +0000, Bryan O'Donoghue wrote:
[...]
+ mdss: display-subsystem@1a00000 {
+ compatible = "qcom,mdss";
+ reg = <0x01a00000 0x1000>,
+ <0x01ac8000 0x3000>;
+ reg-names = "mdss_phys", "vbif_phys";
+
+ interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+
+ clocks = <&gcc GCC_MDSS_AHB_CLK>,
+ <&gcc GCC_MDSS_AXI_CLK>,
+ <&gcc GCC_MDSS_VSYNC_CLK>;
+ clock-names = "iface",
+ "bus",
+ "vsync";
+
+ power-domains = <&gcc MDSS_GDSC>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ #interrupt-cells = <1>;
+ ranges;
+
+ mdp: display-controller@1a01000 {
+ compatible = "qcom,mdp5";
+ reg = <0x01a01000 0x89000>;
+ reg-names = "mdp_phys";
+
+ interrupt-parent = <&mdss>;
+ interrupts = <0>;
+
+ clocks = <&gcc GCC_MDSS_AHB_CLK>,
+ <&gcc GCC_MDSS_AXI_CLK>,
+ <&gcc GCC_MDSS_MDP_CLK>,
+ <&gcc GCC_MDSS_VSYNC_CLK>,
+ <&gcc GCC_MDP_TBU_CLK>,
+ <&gcc GCC_MDP_RT_TBU_CLK>;
+ clock-names = "iface",
+ "bus",
+ "core",
+ "vsync",
+ "tbu",
+ "tbu_rt";
+
+ iommus = <&apps_iommu 4>;
+
+ interconnects = <&snoc_mm MASTER_MDP_PORT0 &bimc SLAVE_EBI_CH0>,
+ <&snoc_mm MASTER_MDP_PORT1 &bimc SLAVE_EBI_CH0>,
+ <&pcnoc MASTER_SPDM &snoc SLAVE_IMEM>;
+ interconnect-names = "mdp0-mem", "mdp1-mem", "register-mem";
As I mentioned a already in a direct email at some point, AFAIU adding
interconnects should be an [almost-] all or nothing step. If you only
add interconnects for MDP then everything else that needs bandwidth will
either break or only continue working as a mere side effect of MDP
voting for permanent high bandwidth.
We did discuss that. You'll also recall we concluded we would have to revert
this patch to make that happen.
commit 76a748e2c1aa976d0c7fef872fa6ff93ce334a8a
Author: Leo Yan <leo.yan@xxxxxxxxxx>
Date: Sat Apr 16 09:26:34 2022 +0800
interconnect: qcom: msm8939: Use icc_sync_state
but then why not revert for all of these SoCs too ?
drivers/interconnect/qcom/msm8939.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/msm8974.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/msm8996.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/osm-l3.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sc7180.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sc7280.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sc8180x.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sc8280xp.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sdm845.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sdx55.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sdx65.c: .sync_state = icc_sync_state,
drivers/interconnect/qcom/sm6350.c: .sync_state = icc_sync_state,
until such time as we have an all or nothing interconnect setup for each of
those SoCs ?
Yes I take your point "some peripherals will appear to work only as a result
of the AHB vote the MDP casts here" but, that is a bug in the definition of
that hypothetical peripheral.
The MDP/display won't run without the interconnect here and the only way to
pull it is to remove sync_state which begs the question why not pull
sync_state for all SoCs without a perfect interconnect description ?
I think that would be a retrograde step.
Most of the SoCs you list do have "interconnects" defined for most
components, which means the situation for them is quite a different
level.
8974 defines two interconnects one for the mdp, one of the gpu. So a
headless setup as you describe would encounter the same situation
potentially.
I simulated this on the BQ Aquaris M5 (MSM8939) that has most
functionality set up already in postmarketOS. First the results without
any changes (interconnects enabled like in your patch here):
To me, that is indicative of more work being required to vote
appropriately for required bandwidth - AHB clocks basically in our
hypothetical setup.
The display certainly won't work without voting for bandwidth it needs.
If there's work to be done to _enable_ headless mode - and there is, we
can do the work to figure out who isn't voting for bandwidth.
Probably the CPU - absent cpufreq, CPR, the operating points. A good -
probably correct guess is we aren't ramping cpufreq, aren't ramping CCI
and aren't voting for the inter-chip CCI "front side" so when the system
boots headless and "does stuff" the cpufrequency stays low, the votes
aren't cast and everything seems to crawl.
I still think its a contrived example though. CPR will come right after
the core dtsi and we can put the theory to the test.
;)
---
bod
