| Message ID | 20190823063248.13295-4-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Qcom smmu-500 wait-for-safe handling for sdm845 | expand |
On Fri, Aug 23, 2019 at 12:03 PM Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > Add reset hook for sdm845 based platforms to turn off > the wait-for-safe sequence. > > Understanding how wait-for-safe logic affects USB and UFS performance > on MTP845 and DB845 boards: > > Qcom's implementation of arm,mmu-500 adds a WAIT-FOR-SAFE logic > to address under-performance issues in real-time clients, such as > Display, and Camera. > On receiving an invalidation requests, the SMMU forwards SAFE request > to these clients and waits for SAFE ack signal from real-time clients. > The SAFE signal from such clients is used to qualify the start of > invalidation. > This logic is controlled by chicken bits, one for each - MDP (display), > IFE0, and IFE1 (camera), that can be accessed only from secure software > on sdm845. > > This configuration, however, degrades the performance of non-real time > clients, such as USB, and UFS etc. This happens because, with wait-for-safe > logic enabled the hardware tries to throttle non-real time clients while > waiting for SAFE ack signals from real-time clients. > > On mtp845 and db845 devices, with wait-for-safe logic enabled by the > bootloaders we see degraded performance of USB and UFS when kernel > enables the smmu stage-1 translations for these clients. > Turn off this wait-for-safe logic from the kernel gets us back the perf > of USB and UFS devices until we re-visit this when we start seeing perf > issues on display/camera on upstream supported SDM845 platforms. > The bootloaders on these boards implement secure monitor callbacks to > handle a specific command - QCOM_SCM_SVC_SMMU_PROGRAM with which the > logic can be toggled. > > There are other boards such as cheza whose bootloaders don't enable this > logic. Such boards don't implement callbacks to handle the specific SCM > call so disabling this logic for such boards will be a no-op. > > This change is inspired by the downstream change from Patrick Daly > to address performance issues with display and camera by handling > this wait-for-safe within separte io-pagetable ops to do TLB > maintenance. So a big thanks to him for the change and for all the > offline discussions. > > Without this change the UFS reads are pretty slow: > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=10 conv=sync > 10+0 records in > 10+0 records out > 10485760 bytes (10.0MB) copied, 22.394903 seconds, 457.2KB/s > real 0m 22.39s > user 0m 0.00s > sys 0m 0.01s > > With this change they are back to rock! > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=300 conv=sync > 300+0 records in > 300+0 records out > 314572800 bytes (300.0MB) copied, 1.030541 seconds, 291.1MB/s > real 0m 1.03s > user 0m 0.00s > sys 0m 0.54s > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > --- > drivers/iommu/arm-smmu-impl.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c > index 3f88cd078dd5..0aef87c41f9c 100644 > --- a/drivers/iommu/arm-smmu-impl.c > +++ b/drivers/iommu/arm-smmu-impl.c > @@ -6,6 +6,7 @@ > > #include <linux/bitfield.h> > #include <linux/of.h> > +#include <linux/qcom_scm.h> > > #include "arm-smmu.h" > > @@ -102,7 +103,6 @@ static struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smm > return &cs->smmu; > } > > - > #define ARM_MMU500_ACTLR_CPRE (1 << 1) > > #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) > @@ -147,6 +147,28 @@ static const struct arm_smmu_impl arm_mmu500_impl = { > .reset = arm_mmu500_reset, > }; > > +static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu) > +{ > + int ret; > + > + arm_mmu500_reset(smmu); > + > + /* > + * To address performance degradation in non-real time clients, > + * such as USB and UFS, turn off wait-for-safe on sdm845 based boards, > + * such as MTP and db845, whose firmwares implement secure monitor > + * call handlers to turn on/off the wait-for-safe logic. > + */ > + ret = qcom_scm_qsmmu500_wait_safe_toggle(0); > + if (ret) > + dev_warn(smmu->dev, "Failed to turn off SAFE logic\n"); > + > + return 0; > +} > + > +const struct arm_smmu_impl qcom_sdm845_smmu500_impl = { > + .reset = qcom_sdm845_smmu500_reset, > +}; > > struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) > { > @@ -170,5 +192,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) > "calxeda,smmu-secure-config-access")) > smmu->impl = &calxeda_impl; > > + if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-500")) > + smmu->impl = &qcom_sdm845_smmu500_impl; > + > return smmu; > } Hi Robin, How does this change look now? Let me know if there are any concerns. I will be happy to incorporate them. Thanks & Regards Vivek
Quoting Vivek Gautam (2019-08-22 23:32:48) > diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c > index 3f88cd078dd5..0aef87c41f9c 100644 > --- a/drivers/iommu/arm-smmu-impl.c > +++ b/drivers/iommu/arm-smmu-impl.c > @@ -102,7 +103,6 @@ static struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smm > return &cs->smmu; > } > > - > #define ARM_MMU500_ACTLR_CPRE (1 << 1) > > #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) Drop this hunk? > @@ -147,6 +147,28 @@ static const struct arm_smmu_impl arm_mmu500_impl = { > .reset = arm_mmu500_reset, > }; > > +static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu) > +{ > + int ret; > + > + arm_mmu500_reset(smmu); > + > + /* > + * To address performance degradation in non-real time clients, > + * such as USB and UFS, turn off wait-for-safe on sdm845 based boards, > + * such as MTP and db845, whose firmwares implement secure monitor > + * call handlers to turn on/off the wait-for-safe logic. > + */ > + ret = qcom_scm_qsmmu500_wait_safe_toggle(0); > + if (ret) > + dev_warn(smmu->dev, "Failed to turn off SAFE logic\n"); > + > + return 0; return ret? Or intentionally don't return an error for failure? > +} > + > +const struct arm_smmu_impl qcom_sdm845_smmu500_impl = { static? > + .reset = qcom_sdm845_smmu500_reset, > +}; > > struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) > {
On 23/08/2019 07:32, Vivek Gautam wrote: > Add reset hook for sdm845 based platforms to turn off > the wait-for-safe sequence. > > Understanding how wait-for-safe logic affects USB and UFS performance > on MTP845 and DB845 boards: > > Qcom's implementation of arm,mmu-500 adds a WAIT-FOR-SAFE logic > to address under-performance issues in real-time clients, such as > Display, and Camera. > On receiving an invalidation requests, the SMMU forwards SAFE request > to these clients and waits for SAFE ack signal from real-time clients. > The SAFE signal from such clients is used to qualify the start of > invalidation. > This logic is controlled by chicken bits, one for each - MDP (display), > IFE0, and IFE1 (camera), that can be accessed only from secure software > on sdm845. > > This configuration, however, degrades the performance of non-real time > clients, such as USB, and UFS etc. This happens because, with wait-for-safe > logic enabled the hardware tries to throttle non-real time clients while > waiting for SAFE ack signals from real-time clients. > > On mtp845 and db845 devices, with wait-for-safe logic enabled by the > bootloaders we see degraded performance of USB and UFS when kernel > enables the smmu stage-1 translations for these clients. > Turn off this wait-for-safe logic from the kernel gets us back the perf > of USB and UFS devices until we re-visit this when we start seeing perf > issues on display/camera on upstream supported SDM845 platforms. > The bootloaders on these boards implement secure monitor callbacks to > handle a specific command - QCOM_SCM_SVC_SMMU_PROGRAM with which the > logic can be toggled. > > There are other boards such as cheza whose bootloaders don't enable this > logic. Such boards don't implement callbacks to handle the specific SCM > call so disabling this logic for such boards will be a no-op. > > This change is inspired by the downstream change from Patrick Daly > to address performance issues with display and camera by handling > this wait-for-safe within separte io-pagetable ops to do TLB > maintenance. So a big thanks to him for the change and for all the > offline discussions. > > Without this change the UFS reads are pretty slow: > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=10 conv=sync > 10+0 records in > 10+0 records out > 10485760 bytes (10.0MB) copied, 22.394903 seconds, 457.2KB/s > real 0m 22.39s > user 0m 0.00s > sys 0m 0.01s > > With this change they are back to rock! > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=300 conv=sync > 300+0 records in > 300+0 records out > 314572800 bytes (300.0MB) copied, 1.030541 seconds, 291.1MB/s > real 0m 1.03s > user 0m 0.00s > sys 0m 0.54s > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > --- > drivers/iommu/arm-smmu-impl.c | 27 ++++++++++++++++++++++++++- I'd be inclined to introduce the inevitable arm-smmu-qcom.c from the start, and save worrying about moving this out later. Other than that, though, the general self-contained shape of it all is every bit as beautiful as I'd hoped :D Robin. > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c > index 3f88cd078dd5..0aef87c41f9c 100644 > --- a/drivers/iommu/arm-smmu-impl.c > +++ b/drivers/iommu/arm-smmu-impl.c > @@ -6,6 +6,7 @@ > > #include <linux/bitfield.h> > #include <linux/of.h> > +#include <linux/qcom_scm.h> > > #include "arm-smmu.h" > > @@ -102,7 +103,6 @@ static struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smm > return &cs->smmu; > } > > - > #define ARM_MMU500_ACTLR_CPRE (1 << 1) > > #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) > @@ -147,6 +147,28 @@ static const struct arm_smmu_impl arm_mmu500_impl = { > .reset = arm_mmu500_reset, > }; > > +static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu) > +{ > + int ret; > + > + arm_mmu500_reset(smmu); > + > + /* > + * To address performance degradation in non-real time clients, > + * such as USB and UFS, turn off wait-for-safe on sdm845 based boards, > + * such as MTP and db845, whose firmwares implement secure monitor > + * call handlers to turn on/off the wait-for-safe logic. > + */ > + ret = qcom_scm_qsmmu500_wait_safe_toggle(0); > + if (ret) > + dev_warn(smmu->dev, "Failed to turn off SAFE logic\n"); > + > + return 0; > +} > + > +const struct arm_smmu_impl qcom_sdm845_smmu500_impl = { > + .reset = qcom_sdm845_smmu500_reset, > +}; > > struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) > { > @@ -170,5 +192,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) > "calxeda,smmu-secure-config-access")) > smmu->impl = &calxeda_impl; > > + if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-500")) > + smmu->impl = &qcom_sdm845_smmu500_impl; > + > return smmu; > } >
Hi Robin, On 2019-09-10 18:56, Robin Murphy wrote: > On 23/08/2019 07:32, Vivek Gautam wrote: >> Add reset hook for sdm845 based platforms to turn off >> the wait-for-safe sequence. >> >> Understanding how wait-for-safe logic affects USB and UFS performance >> on MTP845 and DB845 boards: >> >> Qcom's implementation of arm,mmu-500 adds a WAIT-FOR-SAFE logic >> to address under-performance issues in real-time clients, such as >> Display, and Camera. >> On receiving an invalidation requests, the SMMU forwards SAFE request >> to these clients and waits for SAFE ack signal from real-time clients. >> The SAFE signal from such clients is used to qualify the start of >> invalidation. >> This logic is controlled by chicken bits, one for each - MDP >> (display), >> IFE0, and IFE1 (camera), that can be accessed only from secure >> software >> on sdm845. >> >> This configuration, however, degrades the performance of non-real time >> clients, such as USB, and UFS etc. This happens because, with >> wait-for-safe >> logic enabled the hardware tries to throttle non-real time clients >> while >> waiting for SAFE ack signals from real-time clients. >> >> On mtp845 and db845 devices, with wait-for-safe logic enabled by the >> bootloaders we see degraded performance of USB and UFS when kernel >> enables the smmu stage-1 translations for these clients. >> Turn off this wait-for-safe logic from the kernel gets us back the >> perf >> of USB and UFS devices until we re-visit this when we start seeing >> perf >> issues on display/camera on upstream supported SDM845 platforms. >> The bootloaders on these boards implement secure monitor callbacks to >> handle a specific command - QCOM_SCM_SVC_SMMU_PROGRAM with which the >> logic can be toggled. >> >> There are other boards such as cheza whose bootloaders don't enable >> this >> logic. Such boards don't implement callbacks to handle the specific >> SCM >> call so disabling this logic for such boards will be a no-op. >> >> This change is inspired by the downstream change from Patrick Daly >> to address performance issues with display and camera by handling >> this wait-for-safe within separte io-pagetable ops to do TLB >> maintenance. So a big thanks to him for the change and for all the >> offline discussions. >> >> Without this change the UFS reads are pretty slow: >> $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=10 conv=sync >> 10+0 records in >> 10+0 records out >> 10485760 bytes (10.0MB) copied, 22.394903 seconds, 457.2KB/s >> real 0m 22.39s >> user 0m 0.00s >> sys 0m 0.01s >> >> With this change they are back to rock! >> $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=300 conv=sync >> 300+0 records in >> 300+0 records out >> 314572800 bytes (300.0MB) copied, 1.030541 seconds, 291.1MB/s >> real 0m 1.03s >> user 0m 0.00s >> sys 0m 0.54s >> >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> --- >> drivers/iommu/arm-smmu-impl.c | 27 ++++++++++++++++++++++++++- > > I'd be inclined to introduce the inevitable arm-smmu-qcom.c from the > start, and save worrying about moving this out later. Other than that, > though, the general self-contained shape of it all is every bit as > beautiful as I'd hoped :D > Have posted v5 with your suggestion. Thanks, Sai
Hi Stephen, On 2019-09-06 20:37, Stephen Boyd wrote: > Quoting Vivek Gautam (2019-08-22 23:32:48) >> diff --git a/drivers/iommu/arm-smmu-impl.c >> b/drivers/iommu/arm-smmu-impl.c >> index 3f88cd078dd5..0aef87c41f9c 100644 >> --- a/drivers/iommu/arm-smmu-impl.c >> +++ b/drivers/iommu/arm-smmu-impl.c >> @@ -102,7 +103,6 @@ static struct arm_smmu_device >> *cavium_smmu_impl_init(struct arm_smmu_device *smm >> return &cs->smmu; >> } >> >> - >> #define ARM_MMU500_ACTLR_CPRE (1 << 1) >> >> #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) > > Drop this hunk? > >> @@ -147,6 +147,28 @@ static const struct arm_smmu_impl arm_mmu500_impl >> = { >> .reset = arm_mmu500_reset, >> }; >> >> +static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu) >> +{ >> + int ret; >> + >> + arm_mmu500_reset(smmu); >> + >> + /* >> + * To address performance degradation in non-real time >> clients, >> + * such as USB and UFS, turn off wait-for-safe on sdm845 based >> boards, >> + * such as MTP and db845, whose firmwares implement secure >> monitor >> + * call handlers to turn on/off the wait-for-safe logic. >> + */ >> + ret = qcom_scm_qsmmu500_wait_safe_toggle(0); >> + if (ret) >> + dev_warn(smmu->dev, "Failed to turn off SAFE >> logic\n"); >> + >> + return 0; > > return ret? Or intentionally don't return an error for failure? > >> +} >> + >> +const struct arm_smmu_impl qcom_sdm845_smmu500_impl = { > > static? > >> + .reset = qcom_sdm845_smmu500_reset, >> +}; >> >> struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device >> *smmu) >> { Have addressed all your comments in v5. Thanks, Sai
diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c index 3f88cd078dd5..0aef87c41f9c 100644 --- a/drivers/iommu/arm-smmu-impl.c +++ b/drivers/iommu/arm-smmu-impl.c @@ -6,6 +6,7 @@ #include <linux/bitfield.h> #include <linux/of.h> +#include <linux/qcom_scm.h> #include "arm-smmu.h" @@ -102,7 +103,6 @@ static struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smm return &cs->smmu; } - #define ARM_MMU500_ACTLR_CPRE (1 << 1) #define ARM_MMU500_ACR_CACHE_LOCK (1 << 26) @@ -147,6 +147,28 @@ static const struct arm_smmu_impl arm_mmu500_impl = { .reset = arm_mmu500_reset, }; +static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu) +{ + int ret; + + arm_mmu500_reset(smmu); + + /* + * To address performance degradation in non-real time clients, + * such as USB and UFS, turn off wait-for-safe on sdm845 based boards, + * such as MTP and db845, whose firmwares implement secure monitor + * call handlers to turn on/off the wait-for-safe logic. + */ + ret = qcom_scm_qsmmu500_wait_safe_toggle(0); + if (ret) + dev_warn(smmu->dev, "Failed to turn off SAFE logic\n"); + + return 0; +} + +const struct arm_smmu_impl qcom_sdm845_smmu500_impl = { + .reset = qcom_sdm845_smmu500_reset, +}; struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) { @@ -170,5 +192,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) "calxeda,smmu-secure-config-access")) smmu->impl = &calxeda_impl; + if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-500")) + smmu->impl = &qcom_sdm845_smmu500_impl; + return smmu; }
Add reset hook for sdm845 based platforms to turn off the wait-for-safe sequence. Understanding how wait-for-safe logic affects USB and UFS performance on MTP845 and DB845 boards: Qcom's implementation of arm,mmu-500 adds a WAIT-FOR-SAFE logic to address under-performance issues in real-time clients, such as Display, and Camera. On receiving an invalidation requests, the SMMU forwards SAFE request to these clients and waits for SAFE ack signal from real-time clients. The SAFE signal from such clients is used to qualify the start of invalidation. This logic is controlled by chicken bits, one for each - MDP (display), IFE0, and IFE1 (camera), that can be accessed only from secure software on sdm845. This configuration, however, degrades the performance of non-real time clients, such as USB, and UFS etc. This happens because, with wait-for-safe logic enabled the hardware tries to throttle non-real time clients while waiting for SAFE ack signals from real-time clients. On mtp845 and db845 devices, with wait-for-safe logic enabled by the bootloaders we see degraded performance of USB and UFS when kernel enables the smmu stage-1 translations for these clients. Turn off this wait-for-safe logic from the kernel gets us back the perf of USB and UFS devices until we re-visit this when we start seeing perf issues on display/camera on upstream supported SDM845 platforms. The bootloaders on these boards implement secure monitor callbacks to handle a specific command - QCOM_SCM_SVC_SMMU_PROGRAM with which the logic can be toggled. There are other boards such as cheza whose bootloaders don't enable this logic. Such boards don't implement callbacks to handle the specific SCM call so disabling this logic for such boards will be a no-op. This change is inspired by the downstream change from Patrick Daly to address performance issues with display and camera by handling this wait-for-safe within separte io-pagetable ops to do TLB maintenance. So a big thanks to him for the change and for all the offline discussions. Without this change the UFS reads are pretty slow: $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=10 conv=sync 10+0 records in 10+0 records out 10485760 bytes (10.0MB) copied, 22.394903 seconds, 457.2KB/s real 0m 22.39s user 0m 0.00s sys 0m 0.01s With this change they are back to rock! $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=300 conv=sync 300+0 records in 300+0 records out 314572800 bytes (300.0MB) copied, 1.030541 seconds, 291.1MB/s real 0m 1.03s user 0m 0.00s sys 0m 0.54s Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> --- drivers/iommu/arm-smmu-impl.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)