Messages in this thread | | | Subject | Re: [PATCH v13 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops | From | Robin Murphy <> | Date | Thu, 26 Jul 2018 16:30:53 +0100 |
| |
On 26/07/18 08:12, Vivek Gautam wrote: > On Wed, Jul 25, 2018 at 11:46 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> On Tue, Jul 24, 2018 at 8:51 PM, Robin Murphy <robin.murphy@arm.com> wrote: >>> On 19/07/18 11:15, Vivek Gautam wrote: >>>> >>>> From: Sricharan R <sricharan@codeaurora.org> >>>> >>>> The smmu needs to be functional only when the respective >>>> master's using it are active. The device_link feature >>>> helps to track such functional dependencies, so that the >>>> iommu gets powered when the master device enables itself >>>> using pm_runtime. So by adapting the smmu driver for >>>> runtime pm, above said dependency can be addressed. >>>> >>>> This patch adds the pm runtime/sleep callbacks to the >>>> driver and also the functions to parse the smmu clocks >>>> from DT and enable them in resume/suspend. >>>> >>>> Also, while we enable the runtime pm add a pm sleep suspend >>>> callback that pushes devices to low power state by turning >>>> the clocks off in a system sleep. >>>> Also add corresponding clock enable path in resume callback. >>>> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>> [vivek: rework for clock and pm ops] >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> --- >>>> >>>> Changes since v12: >>>> - Added pm sleep .suspend callback. This disables the clocks. >>>> - Added corresponding change to enable clocks in .resume >>>> pm sleep callback. >>>> >>>> drivers/iommu/arm-smmu.c | 75 >>>> ++++++++++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 73 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index c73cfce1ccc0..9138a6fffe04 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c > > [snip] > >>>> platform_device *pdev) >>>> arm_smmu_device_remove(pdev); >>>> } >>>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>>> +{ >>>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>>> + >>>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>> >>> >>> If there's a power domain being automatically switched by genpd then we need >>> a reset here because we may have lost state entirely. Since I remembered the >>> otherwise-useless GPU SMMU on Juno is in a separate power domain, I gave it >>> a poking via sysfs with some debug stuff to dump sCR0 in these callbacks, >>> and the problem is clear: >>> >>> ... >>> [ 4.625551] arm-smmu 2b400000.iommu: genpd_runtime_suspend() >>> [ 4.631163] arm-smmu 2b400000.iommu: arm_smmu_runtime_suspend: 0x00201936 >>> [ 4.637897] arm-smmu 2b400000.iommu: suspend latency exceeded, 6733980 ns >>> [ 21.566983] arm-smmu 2b400000.iommu: genpd_runtime_resume() >>> [ 21.584796] arm-smmu 2b400000.iommu: arm_smmu_runtime_resume: 0x00220101 >>> [ 21.591452] arm-smmu 2b400000.iommu: resume latency exceeded, 6658020 ns >>> ... >> >> Qualcomm SoCs have retention enabled for SMMU registers so they don't >> lose state. >> ... >> [ 256.013367] arm-smmu b40000.arm,smmu: arm_smmu_runtime_suspend >> SCR0 = 0x201e36 >> [ 256.013367] >> [ 256.019160] arm-smmu b40000.arm,smmu: arm_smmu_runtime_resume >> SCR0 = 0x201e36 >> [ 256.019160] >> [ 256.027368] arm-smmu b40000.arm,smmu: arm_smmu_runtime_suspend >> SCR0 = 0x201e36 >> [ 256.027368] >> [ 256.036786] arm-smmu b40000.arm,smmu: arm_smmu_runtime_resume >> SCR0 = 0x201e36 >> ... >> >> However after adding arm_smmu_device_reset() in runtime_resume() I observe >> some performance degradation when kill an instance of 'kmscube' and >> start it again. >> The launch time with arm_smmu_device_reset() in runtime_resume() change is >> more. >> Could this be because of frequent TLB invalidation and sync?
Probably. Plus the reset procedure is a big chunk of MMIO accesses, which for a non-trivial SMMU configuration probably isn't negligible in itself. Unfortunately, unless you know for absolute certain that you don't need to do that, you do.
> Some more information that i gathered. > On Qcom SoCs besides the registers retention, TCU invalidates TLB cache on > a CX power collapse exit, which is the system wide suspend case. > The arm-smmu software is not aware of this CX power collapse / > auto-invalidation. > > So wouldn't doing an explicit TLB invalidations during runtime resume be > detrimental to performance?
Indeed it would be, but resuming with TLBs full of random valid-looking junk is even more so.
> I have one more doubt here - > We do runtime power cycle around arm_smmu_map/unmap() too. > Now during map/unmap we selectively do TLB maintenance (either > tlb_sync or tlb_add_flush). > But with runtime pm we want to do TLBIALL*. Is that a problem?
It's technically redundant to do both, true, but as we've covered in previous rounds of discussion it's very difficult to know *which* one is sufficient at any given time, so in order to make progress for now I think we have to settle with doing both.
Robin.
| |