| Message ID | 1517999482-17317-4-git-send-email-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > From: Sricharan R <sricharan@codeaurora.org> > > The smmu device probe/remove and add/remove master device callbacks > gets called when the smmu is not linked to its master, that is without > the context of the master device. So calling runtime apis in those places > separately. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > [vivek: Cleanup pm runtime calls] > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > --- > drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 9e2f917e16c2..c024f69c1682 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > - int irq; > + int ret, irq; > > if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) > return; > > + ret = pm_runtime_get_sync(smmu->dev); > + if (ret) > + return; pm_runtime_get_sync() will return 0 if the device was powered off, 1 if it was already/still powered on or runtime PM is not compiled in, or a negative value on error, so shouldn't the test be (ret < 0)? Moreover, I'm actually wondering if it makes any sense to power up the hardware just to program it and power it down again. In a system where the IOMMU is located within a power domain, it would cause the IOMMU block to lose its state anyway. Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops", perhaps it would make more sense to just control the clocks independently of runtime PM? Then, runtime PM could be used for real power management, e.g. really powering the block up and down, for further power saving. +Generally similar comments for other places in this patch. > + > /* > * Disable the context bank and free the page tables before freeing > * it. > @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > > free_io_pgtable_ops(smmu_domain->pgtbl_ops); > __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); > + > + pm_runtime_put_sync(smmu->dev); Is there any point in the put being sync here? [snip] > @@ -2131,6 +2152,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > if (err) > return err; > > + platform_set_drvdata(pdev, smmu); > + > + pm_runtime_enable(dev); I suspect this may be a disaster for systems where IOMMUs are located inside power domains, because the driver doesn't take care of the IOMMU block losing its state on physical power down, as I mentioned in my comments above. Best regards, Tomasz
On 13/02/18 08:24, Tomasz Figa wrote: > Hi Vivek, > > Thanks for the patch. Please see my comments inline. > > On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> The smmu device probe/remove and add/remove master device callbacks >> gets called when the smmu is not linked to its master, that is without >> the context of the master device. So calling runtime apis in those places >> separately. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> [vivek: Cleanup pm runtime calls] >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> --- >> drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 38 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 9e2f917e16c2..c024f69c1682 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >> - int irq; >> + int ret, irq; >> >> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >> return; >> >> + ret = pm_runtime_get_sync(smmu->dev); >> + if (ret) >> + return; > > pm_runtime_get_sync() will return 0 if the device was powered off, 1 > if it was already/still powered on or runtime PM is not compiled in, > or a negative value on error, so shouldn't the test be (ret < 0)? > > Moreover, I'm actually wondering if it makes any sense to power up the > hardware just to program it and power it down again. In a system where > the IOMMU is located within a power domain, it would cause the IOMMU > block to lose its state anyway. This is generally for the case where the SMMU internal state remains active, but the programming interface needs to be powered up in order to access it. > Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add > pm_runtime/sleep ops", perhaps it would make more sense to just > control the clocks independently of runtime PM? Then, runtime PM could > be used for real power management, e.g. really powering the block up > and down, for further power saving. Unfortunately that ends up pretty much unmanageable, because there are numerous different SMMU microarchitectures with fundamentally different clock/power domain schemes (multiplied by individual SoC integration possibilities). Since this is fundamentally a generic architectural driver, adding explicit clock support would probably make the whole thing about 50% clock code, with complicated decision trees around every hardware access calculating which clocks are necessary for a given operation on a given system. That maintainability aspect is why we've already nacked such a fine-grained approach in the past. Robin.
On Tue, Feb 13, 2018 at 9:57 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 13/02/18 08:24, Tomasz Figa wrote: >> >> Hi Vivek, >> >> Thanks for the patch. Please see my comments inline. >> >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> The smmu device probe/remove and add/remove master device callbacks >>> gets called when the smmu is not linked to its master, that is without >>> the context of the master device. So calling runtime apis in those places >>> separately. >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> [vivek: Cleanup pm runtime calls] >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> --- >>> drivers/iommu/arm-smmu.c | 42 >>> ++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 38 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 9e2f917e16c2..c024f69c1682 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct >>> iommu_domain *domain) >>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >>> struct arm_smmu_device *smmu = smmu_domain->smmu; >>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >>> - int irq; >>> + int ret, irq; >>> >>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >>> return; >>> >>> + ret = pm_runtime_get_sync(smmu->dev); >>> + if (ret) >>> + return; >> >> >> pm_runtime_get_sync() will return 0 if the device was powered off, 1 >> if it was already/still powered on or runtime PM is not compiled in, >> or a negative value on error, so shouldn't the test be (ret < 0)? >> >> Moreover, I'm actually wondering if it makes any sense to power up the >> hardware just to program it and power it down again. In a system where >> the IOMMU is located within a power domain, it would cause the IOMMU >> block to lose its state anyway. > > > This is generally for the case where the SMMU internal state remains active, > but the programming interface needs to be powered up in order to access it. That's true for Qualcomm SMMU, but I think that would be different for existing users of the driver? > >> Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add >> pm_runtime/sleep ops", perhaps it would make more sense to just >> control the clocks independently of runtime PM? Then, runtime PM could >> be used for real power management, e.g. really powering the block up >> and down, for further power saving. > > > Unfortunately that ends up pretty much unmanageable, because there are > numerous different SMMU microarchitectures with fundamentally different > clock/power domain schemes (multiplied by individual SoC integration > possibilities). Since this is fundamentally a generic architectural driver, > adding explicit clock support would probably make the whole thing about 50% > clock code, with complicated decision trees around every hardware access > calculating which clocks are necessary for a given operation on a given > system. That maintainability aspect is why we've already nacked such a > fine-grained approach in the past. Hmm, I think we are talking about different things here. My suggestion would not add much more code to the driver than this patch does, calls to arm_smmu_enable_clocks() instead of pm_runtime_get_sync() and arm_smmu_disable_clocks() instead of pm_runtime_put(). The implementation of both functions would be a simple call to clk_bulk_ API (possibly even no need to put this into functions, just call directly). Best regards, Tomasz
On Tue, Feb 13, 2018 at 7:22 PM, Tomasz Figa <tfiga@chromium.org> wrote: > On Tue, Feb 13, 2018 at 9:57 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> On 13/02/18 08:24, Tomasz Figa wrote: >>> >>> Hi Vivek, >>> >>> Thanks for the patch. Please see my comments inline. >>> >>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> >>>> From: Sricharan R <sricharan@codeaurora.org> >>>> >>>> The smmu device probe/remove and add/remove master device callbacks >>>> gets called when the smmu is not linked to its master, that is without >>>> the context of the master device. So calling runtime apis in those places >>>> separately. >>>> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> [vivek: Cleanup pm runtime calls] >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> --- >>>> drivers/iommu/arm-smmu.c | 42 >>>> ++++++++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 38 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 9e2f917e16c2..c024f69c1682 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct >>>> iommu_domain *domain) >>>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >>>> struct arm_smmu_device *smmu = smmu_domain->smmu; >>>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >>>> - int irq; >>>> + int ret, irq; >>>> >>>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >>>> return; >>>> >>>> + ret = pm_runtime_get_sync(smmu->dev); >>>> + if (ret) >>>> + return; >>> >>> >>> pm_runtime_get_sync() will return 0 if the device was powered off, 1 >>> if it was already/still powered on or runtime PM is not compiled in, >>> or a negative value on error, so shouldn't the test be (ret < 0)? >>> >>> Moreover, I'm actually wondering if it makes any sense to power up the >>> hardware just to program it and power it down again. In a system where >>> the IOMMU is located within a power domain, it would cause the IOMMU >>> block to lose its state anyway. >> >> >> This is generally for the case where the SMMU internal state remains active, >> but the programming interface needs to be powered up in order to access it. > > That's true for Qualcomm SMMU, but I think that would be different for > existing users of the driver? > >> >>> Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add >>> pm_runtime/sleep ops", perhaps it would make more sense to just >>> control the clocks independently of runtime PM? Then, runtime PM could >>> be used for real power management, e.g. really powering the block up >>> and down, for further power saving. >> >> >> Unfortunately that ends up pretty much unmanageable, because there are >> numerous different SMMU microarchitectures with fundamentally different >> clock/power domain schemes (multiplied by individual SoC integration >> possibilities). Since this is fundamentally a generic architectural driver, >> adding explicit clock support would probably make the whole thing about 50% >> clock code, with complicated decision trees around every hardware access >> calculating which clocks are necessary for a given operation on a given >> system. That maintainability aspect is why we've already nacked such a >> fine-grained approach in the past. > > Hmm, I think we are talking about different things here. My suggestion > would not add much more code to the driver than this patch does, calls > to arm_smmu_enable_clocks() instead of pm_runtime_get_sync() and > arm_smmu_disable_clocks() instead of pm_runtime_put(). The > implementation of both functions would be a simple call to clk_bulk_ > API (possibly even no need to put this into functions, just call > directly). Well, things are not so straight on msm. The IP clocks on msm are usually powered by (or i should rather say, controlled by) the same power domain that provides the VDD supply to iommu block. This is the behavior on msm8996 atleast that we are testing on right now. On later SoCs too things don't change drastically. So, you can't have the block in low power state until you program the register space and then power on the block to let it do its magic. Clocks and power domains are linked, and that's why we add them to the pm callbacks. This approach also looks generic to me since the platforms will either have such a link or they will not have. But, in either case you will have power and clocks available at the time when you need them. Thanks & regards Vivek > > Best regards, > Tomasz > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On Tue, Feb 13, 2018 at 1:54 PM, Tomasz Figa <tfiga@chromium.org> wrote: > Hi Vivek, > > Thanks for the patch. Please see my comments inline. > > On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> The smmu device probe/remove and add/remove master device callbacks >> gets called when the smmu is not linked to its master, that is without >> the context of the master device. So calling runtime apis in those places >> separately. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> [vivek: Cleanup pm runtime calls] >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> --- >> drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 38 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 9e2f917e16c2..c024f69c1682 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >> - int irq; >> + int ret, irq; >> >> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >> return; >> >> + ret = pm_runtime_get_sync(smmu->dev); >> + if (ret) >> + return; > > pm_runtime_get_sync() will return 0 if the device was powered off, 1 > if it was already/still powered on or runtime PM is not compiled in, > or a negative value on error, so shouldn't the test be (ret < 0)? Yes, I too noticed it while i was testing on a different platform, and was hitting a failure case. Will update at all places. > > Moreover, I'm actually wondering if it makes any sense to power up the > hardware just to program it and power it down again. In a system where > the IOMMU is located within a power domain, it would cause the IOMMU > block to lose its state anyway. > > Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add > pm_runtime/sleep ops", perhaps it would make more sense to just > control the clocks independently of runtime PM? Then, runtime PM could > be used for real power management, e.g. really powering the block up > and down, for further power saving. > > +Generally similar comments for other places in this patch. > >> + >> /* >> * Disable the context bank and free the page tables before freeing >> * it. >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> >> free_io_pgtable_ops(smmu_domain->pgtbl_ops); >> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); >> + >> + pm_runtime_put_sync(smmu->dev); > > Is there any point in the put being sync here? No, I don't think. Can manage with just a 'put' here. Will modify. best regards Vivek > > [snip] > >> @@ -2131,6 +2152,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >> if (err) >> return err; >> >> + platform_set_drvdata(pdev, smmu); >> + >> + pm_runtime_enable(dev); > > I suspect this may be a disaster for systems where IOMMUs are located > inside power domains, because the driver doesn't take care of the > IOMMU block losing its state on physical power down, as I mentioned in > my comments above. > > Best regards, > Tomasz
On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote: > From: Sricharan R <sricharan@codeaurora.org> > > The smmu device probe/remove and add/remove master device callbacks > gets called when the smmu is not linked to its master, that is without > the context of the master device. So calling runtime apis in those places > separately. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > [vivek: Cleanup pm runtime calls] > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > --- > drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 9e2f917e16c2..c024f69c1682 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > - int irq; > + int ret, irq; > > if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) > return; > > + ret = pm_runtime_get_sync(smmu->dev); > + if (ret) > + return; > + > /* > * Disable the context bank and free the page tables before freeing > * it. > @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > > free_io_pgtable_ops(smmu_domain->pgtbl_ops); > __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); > + > + pm_runtime_put_sync(smmu->dev); > } > > static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev) > while (i--) > cfg->smendx[i] = INVALID_SMENDX; > > - ret = arm_smmu_master_alloc_smes(dev); > + ret = pm_runtime_get_sync(smmu->dev); > if (ret) > goto out_cfg_free; Hey Vivek, I just hit a problem with this on sdm845. It turns out that pm_runtime_get_sync() returns a positive 1 if the device is already active. I hit this in the GPU code. The a6xx has two platform devices that each use a different sid on the iommu. The GPU is probed normally from a platform driver and it in turn initializes the GMU device by way of a phandle. Because the GMU isn't probed with a platform driver we need to call of_dma_configure() on the device to set up the IOMMU for the device which ends up calling through this path and we discover that the smmu->dev is already powered (pm_runtime_get_sync returns 1). I'm not immediately sure if this is a bug on sdm845 or not because a cursory inspection says that the SMMU device shouldn't be powered at this time but there might be a connection that I'm not seeing. Obviously if the SMMU was left powered thats a bad thing. But putting that aside it is obvious that this code should be accommodating of the possibility that the device is already powered, and so this should be if (ret < 0) goto out_cfg_free; With that the GPU/GMU successfully comes up on Sean Paul's display testing branch. Jordan
On Fri, Feb 23, 2018 at 5:22 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> The smmu device probe/remove and add/remove master device callbacks >> gets called when the smmu is not linked to its master, that is without >> the context of the master device. So calling runtime apis in those places >> separately. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> [vivek: Cleanup pm runtime calls] >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> --- >> drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 38 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 9e2f917e16c2..c024f69c1682 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >> - int irq; >> + int ret, irq; >> >> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >> return; >> >> + ret = pm_runtime_get_sync(smmu->dev); >> + if (ret) >> + return; >> + >> /* >> * Disable the context bank and free the page tables before freeing >> * it. >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> >> free_io_pgtable_ops(smmu_domain->pgtbl_ops); >> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); >> + >> + pm_runtime_put_sync(smmu->dev); >> } >> >> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) >> @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev) >> while (i--) >> cfg->smendx[i] = INVALID_SMENDX; >> >> - ret = arm_smmu_master_alloc_smes(dev); >> + ret = pm_runtime_get_sync(smmu->dev); >> if (ret) >> goto out_cfg_free; > > Hey Vivek, I just hit a problem with this on sdm845. It turns out that > pm_runtime_get_sync() returns a positive 1 if the device is already active. > > I hit this in the GPU code. The a6xx has two platform devices that each use a > different sid on the iommu. The GPU is probed normally from a platform driver > and it in turn initializes the GMU device by way of a phandle. > > Because the GMU isn't probed with a platform driver we need to call > of_dma_configure() on the device to set up the IOMMU for the device which ends > up calling through this path and we discover that the smmu->dev is already > powered (pm_runtime_get_sync returns 1). > > I'm not immediately sure if this is a bug on sdm845 or not because a cursory > inspection says that the SMMU device shouldn't be powered at this time but there > might be a connection that I'm not seeing. Obviously if the SMMU was left > powered thats a bad thing. But putting that aside it is obvious that this > code should be accommodating of the possibility that the device is already > powered, and so this should be > > if (ret < 0) > goto out_cfg_free; Right, as Tomasz also pointed, we should surely check the negative value of pm_runtime_get_sync(). From your description, it may be that the GPU has turned on the smmu, and then once if goes and probes the GMU, the GMU device also wants to turn-on the same smmu device. But that's already active. So pm_runtime_get_sync() returns 1. Am i making sense? regards Vivek > > With that the GPU/GMU successfully comes up on Sean Paul's display testing > branch. > > Jordan > > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 04:06:39PM +0530, Vivek Gautam wrote: > On Fri, Feb 23, 2018 at 5:22 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > > On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote: > >> From: Sricharan R <sricharan@codeaurora.org> > >> > >> The smmu device probe/remove and add/remove master device callbacks > >> gets called when the smmu is not linked to its master, that is without > >> the context of the master device. So calling runtime apis in those places > >> separately. > >> > >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> > >> [vivek: Cleanup pm runtime calls] > >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > >> --- > >> drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- > >> 1 file changed, 38 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > >> index 9e2f917e16c2..c024f69c1682 100644 > >> --- a/drivers/iommu/arm-smmu.c > >> +++ b/drivers/iommu/arm-smmu.c > >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > >> struct arm_smmu_device *smmu = smmu_domain->smmu; > >> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > >> - int irq; > >> + int ret, irq; > >> > >> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) > >> return; > >> > >> + ret = pm_runtime_get_sync(smmu->dev); > >> + if (ret) > >> + return; > >> + > >> /* > >> * Disable the context bank and free the page tables before freeing > >> * it. > >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > >> > >> free_io_pgtable_ops(smmu_domain->pgtbl_ops); > >> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); > >> + > >> + pm_runtime_put_sync(smmu->dev); > >> } > >> > >> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > >> @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev) > >> while (i--) > >> cfg->smendx[i] = INVALID_SMENDX; > >> > >> - ret = arm_smmu_master_alloc_smes(dev); > >> + ret = pm_runtime_get_sync(smmu->dev); > >> if (ret) > >> goto out_cfg_free; > > > > Hey Vivek, I just hit a problem with this on sdm845. It turns out that > > pm_runtime_get_sync() returns a positive 1 if the device is already active. > > > > I hit this in the GPU code. The a6xx has two platform devices that each use a > > different sid on the iommu. The GPU is probed normally from a platform driver > > and it in turn initializes the GMU device by way of a phandle. > > > > Because the GMU isn't probed with a platform driver we need to call > > of_dma_configure() on the device to set up the IOMMU for the device which ends > > up calling through this path and we discover that the smmu->dev is already > > powered (pm_runtime_get_sync returns 1). > > > > I'm not immediately sure if this is a bug on sdm845 or not because a cursory > > inspection says that the SMMU device shouldn't be powered at this time but there > > might be a connection that I'm not seeing. Obviously if the SMMU was left > > powered thats a bad thing. But putting that aside it is obvious that this > > code should be accommodating of the possibility that the device is already > > powered, and so this should be > > > > if (ret < 0) > > goto out_cfg_free; > > Right, as Tomasz also pointed, we should surely check the negative value of > pm_runtime_get_sync(). Sorry, I didn't notice that Tomasz had pointed it out as well. I wanted to quickly get it on the mailing list so you could catch it in your time zone. > From your description, it may be that the GPU has turned on the smmu, and > then once if goes and probes the GMU, the GMU device also wants to turn-on > the same smmu device. But that's already active. So pm_runtime_get_sync() > returns 1. > Am i making sense? My concern is that this is happening during the probe and we shouldn't be energizing the GPU at this point. But it is entirely possible that the bus is on for other reasons. I'll do a bit of digging today and see exactly which device is at fault. Jordan
On Fri, Feb 23, 2018 at 9:10 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > On Fri, Feb 23, 2018 at 04:06:39PM +0530, Vivek Gautam wrote: >> On Fri, Feb 23, 2018 at 5:22 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote: >> > On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote: >> >> From: Sricharan R <sricharan@codeaurora.org> >> >> >> >> The smmu device probe/remove and add/remove master device callbacks >> >> gets called when the smmu is not linked to its master, that is without >> >> the context of the master device. So calling runtime apis in those places >> >> separately. >> >> >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> >> [vivek: Cleanup pm runtime calls] >> >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> >> --- >> >> drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++---- >> >> 1 file changed, 38 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> >> index 9e2f917e16c2..c024f69c1682 100644 >> >> --- a/drivers/iommu/arm-smmu.c >> >> +++ b/drivers/iommu/arm-smmu.c >> >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); >> >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> >> struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >> >> - int irq; >> >> + int ret, irq; >> >> >> >> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) >> >> return; >> >> >> >> + ret = pm_runtime_get_sync(smmu->dev); >> >> + if (ret) >> >> + return; >> >> + >> >> /* >> >> * Disable the context bank and free the page tables before freeing >> >> * it. >> >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) >> >> >> >> free_io_pgtable_ops(smmu_domain->pgtbl_ops); >> >> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); >> >> + >> >> + pm_runtime_put_sync(smmu->dev); >> >> } >> >> >> >> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) >> >> @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev) >> >> while (i--) >> >> cfg->smendx[i] = INVALID_SMENDX; >> >> >> >> - ret = arm_smmu_master_alloc_smes(dev); >> >> + ret = pm_runtime_get_sync(smmu->dev); >> >> if (ret) >> >> goto out_cfg_free; >> > >> > Hey Vivek, I just hit a problem with this on sdm845. It turns out that >> > pm_runtime_get_sync() returns a positive 1 if the device is already active. >> > >> > I hit this in the GPU code. The a6xx has two platform devices that each use a >> > different sid on the iommu. The GPU is probed normally from a platform driver >> > and it in turn initializes the GMU device by way of a phandle. >> > >> > Because the GMU isn't probed with a platform driver we need to call >> > of_dma_configure() on the device to set up the IOMMU for the device which ends >> > up calling through this path and we discover that the smmu->dev is already >> > powered (pm_runtime_get_sync returns 1). >> > >> > I'm not immediately sure if this is a bug on sdm845 or not because a cursory >> > inspection says that the SMMU device shouldn't be powered at this time but there >> > might be a connection that I'm not seeing. Obviously if the SMMU was left >> > powered thats a bad thing. But putting that aside it is obvious that this >> > code should be accommodating of the possibility that the device is already >> > powered, and so this should be >> > >> > if (ret < 0) >> > goto out_cfg_free; >> >> Right, as Tomasz also pointed, we should surely check the negative value of >> pm_runtime_get_sync(). > > Sorry, I didn't notice that Tomasz had pointed it out as well. I wanted to > quickly get it on the mailing list so you could catch it in your time zone. > >> From your description, it may be that the GPU has turned on the smmu, and >> then once if goes and probes the GMU, the GMU device also wants to turn-on >> the same smmu device. But that's already active. So pm_runtime_get_sync() >> returns 1. >> Am i making sense? > > My concern is that this is happening during the probe and we shouldn't be > energizing the GPU at this point. But it is entirely possible that the > bus is on for other reasons. I'll do a bit of digging today and see exactly > which device is at fault. I will try to check it myself too. regards Vivek > > > Jordan > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 9e2f917e16c2..c024f69c1682 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_device *smmu = smmu_domain->smmu; struct arm_smmu_cfg *cfg = &smmu_domain->cfg; - int irq; + int ret, irq; if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) return; + ret = pm_runtime_get_sync(smmu->dev); + if (ret) + return; + /* * Disable the context bank and free the page tables before freeing * it. @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) free_io_pgtable_ops(smmu_domain->pgtbl_ops); __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); + + pm_runtime_put_sync(smmu->dev); } static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev) while (i--) cfg->smendx[i] = INVALID_SMENDX; - ret = arm_smmu_master_alloc_smes(dev); + ret = pm_runtime_get_sync(smmu->dev); if (ret) goto out_cfg_free; + ret = arm_smmu_master_alloc_smes(dev); + if (ret) + goto out_rpm_put; + iommu_device_link(&smmu->iommu, dev); + pm_runtime_put_sync(smmu->dev); + return 0; +out_rpm_put: + pm_runtime_put_sync(smmu->dev); out_cfg_free: kfree(cfg); out_free: @@ -1427,7 +1441,7 @@ static void arm_smmu_remove_device(struct device *dev) struct iommu_fwspec *fwspec = dev->iommu_fwspec; struct arm_smmu_master_cfg *cfg; struct arm_smmu_device *smmu; - + int ret; if (!fwspec || fwspec->ops != &arm_smmu_ops) return; @@ -1435,8 +1449,15 @@ static void arm_smmu_remove_device(struct device *dev) cfg = fwspec->iommu_priv; smmu = cfg->smmu; + ret = pm_runtime_get_sync(smmu->dev); + if (ret) + return; + iommu_device_unlink(&smmu->iommu, dev); arm_smmu_master_free_smes(fwspec); + + pm_runtime_put_sync(smmu->dev); + iommu_group_remove_device(dev); kfree(fwspec->iommu_priv); iommu_fwspec_free(dev); @@ -2131,6 +2152,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) if (err) return err; + platform_set_drvdata(pdev, smmu); + + pm_runtime_enable(dev); + + err = pm_runtime_get_sync(dev); + if (err) + return err; + err = arm_smmu_device_cfg_probe(smmu); if (err) return err; @@ -2172,9 +2201,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev) return err; } - platform_set_drvdata(pdev, smmu); arm_smmu_device_reset(smmu); arm_smmu_test_smr_masks(smmu); + pm_runtime_put_sync(dev); /* * For ACPI and generic DT bindings, an SMMU will be probed before @@ -2211,8 +2240,13 @@ static int arm_smmu_device_remove(struct platform_device *pdev) if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) dev_err(&pdev->dev, "removing device with active domains!\n"); + pm_runtime_get_sync(smmu->dev); /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); + pm_runtime_put_sync(smmu->dev); + + pm_runtime_disable(smmu->dev); + return 0; }