| Message ID | 20191007204906.19571-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iommu/arm-smmu: fix "hang" when games exit | expand |
On 2019-10-07 9:49 pm, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > When games, browser, or anything using a lot of GPU buffers exits, there > can be many hundreds or thousands of buffers to unmap and free. If the > GPU is otherwise suspended, this can cause arm-smmu to resume/suspend > for each buffer, resulting 5-10 seconds worth of reprogramming the > context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc). > To the user it would appear that the system just locked up. > > A simple solution is to use pm_runtime_put_autosuspend() instead, so we > don't immediately suspend the SMMU device. Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > v1: original > v2: unconditionally use autosuspend, rather than deciding based on what > consumer does > > drivers/iommu/arm-smmu.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 3f1d55fb43c4..b7b41f5001bc 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > { > if (pm_runtime_enabled(smmu->dev)) > - pm_runtime_put(smmu->dev); > + pm_runtime_put_autosuspend(smmu->dev); > } > > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > @@ -1445,6 +1445,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > /* Looks ok, so add the device to the domain */ > ret = arm_smmu_domain_add_master(smmu_domain, fwspec); > > + pm_runtime_set_autosuspend_delay(smmu->dev, 20); > + pm_runtime_use_autosuspend(smmu->dev); > + > rpm_put: > arm_smmu_rpm_put(smmu); > return ret; >
Hi Rob, On Mon, Oct 07, 2019 at 01:49:06PM -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > When games, browser, or anything using a lot of GPU buffers exits, there > can be many hundreds or thousands of buffers to unmap and free. If the > GPU is otherwise suspended, this can cause arm-smmu to resume/suspend > for each buffer, resulting 5-10 seconds worth of reprogramming the > context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc). > To the user it would appear that the system just locked up. > > A simple solution is to use pm_runtime_put_autosuspend() instead, so we > don't immediately suspend the SMMU device. Please can you reword the subject to be a bit more useful? The commit message is great, but the subject is a bit like "fix bug in code" to me. > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > v1: original > v2: unconditionally use autosuspend, rather than deciding based on what > consumer does > > drivers/iommu/arm-smmu.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 3f1d55fb43c4..b7b41f5001bc 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > { > if (pm_runtime_enabled(smmu->dev)) > - pm_runtime_put(smmu->dev); > + pm_runtime_put_autosuspend(smmu->dev); > } > > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > @@ -1445,6 +1445,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > /* Looks ok, so add the device to the domain */ > ret = arm_smmu_domain_add_master(smmu_domain, fwspec); Please can you put a comment here explaining what this is doing? An abridged version of the commit message is fine. > + pm_runtime_set_autosuspend_delay(smmu->dev, 20); > + pm_runtime_use_autosuspend(smmu->dev); Cheers, Will
On Mon, Oct 28, 2019 at 3:20 PM Will Deacon <will@kernel.org> wrote: > > Hi Rob, > > On Mon, Oct 07, 2019 at 01:49:06PM -0700, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > When games, browser, or anything using a lot of GPU buffers exits, there > > can be many hundreds or thousands of buffers to unmap and free. If the > > GPU is otherwise suspended, this can cause arm-smmu to resume/suspend > > for each buffer, resulting 5-10 seconds worth of reprogramming the > > context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc). > > To the user it would appear that the system just locked up. > > > > A simple solution is to use pm_runtime_put_autosuspend() instead, so we > > don't immediately suspend the SMMU device. > > Please can you reword the subject to be a bit more useful? The commit > message is great, but the subject is a bit like "fix bug in code" to me. yeah, not the best $subject, but I wasn't quite sure how to fit something better in a reasonable # of chars.. maybe something like: "iommu/arm-smmu: optimize unmap but avoiding toggling runpm state"? BR, -R > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > v1: original > > v2: unconditionally use autosuspend, rather than deciding based on what > > consumer does > > > > drivers/iommu/arm-smmu.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > > index 3f1d55fb43c4..b7b41f5001bc 100644 > > --- a/drivers/iommu/arm-smmu.c > > +++ b/drivers/iommu/arm-smmu.c > > @@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > > static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > > { > > if (pm_runtime_enabled(smmu->dev)) > > - pm_runtime_put(smmu->dev); > > + pm_runtime_put_autosuspend(smmu->dev); > > } > > > > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > > @@ -1445,6 +1445,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > /* Looks ok, so add the device to the domain */ > > ret = arm_smmu_domain_add_master(smmu_domain, fwspec); > > Please can you put a comment here explaining what this is doing? An abridged > version of the commit message is fine. > > > + pm_runtime_set_autosuspend_delay(smmu->dev, 20); > > + pm_runtime_use_autosuspend(smmu->dev); > > Cheers, > > Will
On 2019-10-28 10:38 pm, Rob Clark wrote: > On Mon, Oct 28, 2019 at 3:20 PM Will Deacon <will@kernel.org> wrote: >> >> Hi Rob, >> >> On Mon, Oct 07, 2019 at 01:49:06PM -0700, Rob Clark wrote: >>> From: Rob Clark <robdclark@chromium.org> >>> >>> When games, browser, or anything using a lot of GPU buffers exits, there >>> can be many hundreds or thousands of buffers to unmap and free. If the >>> GPU is otherwise suspended, this can cause arm-smmu to resume/suspend >>> for each buffer, resulting 5-10 seconds worth of reprogramming the >>> context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc). >>> To the user it would appear that the system just locked up. >>> >>> A simple solution is to use pm_runtime_put_autosuspend() instead, so we >>> don't immediately suspend the SMMU device. >> >> Please can you reword the subject to be a bit more useful? The commit >> message is great, but the subject is a bit like "fix bug in code" to me. > > yeah, not the best $subject, but I wasn't quite sure how to fit > something better in a reasonable # of chars.. maybe something like: > "iommu/arm-smmu: optimize unmap but avoiding toggling runpm state"? FWIW, I'd be inclined to frame it as something like "avoid pathological RPM behaviour for unmaps". Robin.
On Mon, Oct 28, 2019 at 10:51:53PM +0000, Robin Murphy wrote: > On 2019-10-28 10:38 pm, Rob Clark wrote: > > On Mon, Oct 28, 2019 at 3:20 PM Will Deacon <will@kernel.org> wrote: > > > On Mon, Oct 07, 2019 at 01:49:06PM -0700, Rob Clark wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > > > > > When games, browser, or anything using a lot of GPU buffers exits, there > > > > can be many hundreds or thousands of buffers to unmap and free. If the > > > > GPU is otherwise suspended, this can cause arm-smmu to resume/suspend > > > > for each buffer, resulting 5-10 seconds worth of reprogramming the > > > > context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc). > > > > To the user it would appear that the system just locked up. > > > > > > > > A simple solution is to use pm_runtime_put_autosuspend() instead, so we > > > > don't immediately suspend the SMMU device. > > > > > > Please can you reword the subject to be a bit more useful? The commit > > > message is great, but the subject is a bit like "fix bug in code" to me. > > > > yeah, not the best $subject, but I wasn't quite sure how to fit > > something better in a reasonable # of chars.. maybe something like: > > "iommu/arm-smmu: optimize unmap but avoiding toggling runpm state"? > > FWIW, I'd be inclined to frame it as something like "avoid pathological RPM > behaviour for unmaps". LGTM! Will
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 3f1d55fb43c4..b7b41f5001bc 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) { if (pm_runtime_enabled(smmu->dev)) - pm_runtime_put(smmu->dev); + pm_runtime_put_autosuspend(smmu->dev); } static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) @@ -1445,6 +1445,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) /* Looks ok, so add the device to the domain */ ret = arm_smmu_domain_add_master(smmu_domain, fwspec); + pm_runtime_set_autosuspend_delay(smmu->dev, 20); + pm_runtime_use_autosuspend(smmu->dev); + rpm_put: arm_smmu_rpm_put(smmu); return ret;