Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682250 > unrolled thread

[PATCH V4 0/6] iommu/arm-smmu: Add runtime pm/sleep support

Back to article view | Back to linux.kernel

Contents

  [PATCH V4 0/6] iommu/arm-smmu: Add runtime pm/sleep support Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
    [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:10 +0200
          Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 06:00 +0200
    [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 07:20 +0200
          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-13 07:40 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-13 14:10 +0200
                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:20 +0200
                  Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-13 14:30 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-13 16:00 +0200
                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 17:00 +0200
                  Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 19:10 +0200
                    Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 19:50 +0200
                      Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 20:10 +0200
                        Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 20:30 +0200
                          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 21:10 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 21:40 +0200
                              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-17 13:50 +0200
                                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-17 14:30 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 21:40 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 21:40 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 16:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 16:10 +0200
          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 08:50 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Robin Murphy <robin.murphy@arm.com> - 2017-07-13 12:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:00 +0200
    [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between  masters and smmu Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between  masters and smmu Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 06:00 +0200
    [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Rob Herring <robh@kernel.org> - 2017-07-10 05:40 +0200
        Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-11 07:20 +0200
    [PATCH V4 6/6] iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for  qcom,msm8996-smmu-v2 clocks Rob Herring <robh@kernel.org> - 2017-07-10 05:50 +0200
        Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for  qcom,msm8996-smmu-v2 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-10 08:50 +0200

Page 1 of 2  [1] 2  Next page →

#1682250 — [PATCH V4 0/6] iommu/arm-smmu: Add runtime pm/sleep support

This series provides the support for turning on the arm-smmu's
clocks/power domains using runtime pm. This is done using the
recently introduced device links patches, which lets the symmu's
runtime to follow the master's runtime pm, so the smmu remains
powered only when the masters use it.

Took some reference from the exynos runtime patches [2].
Tested this with MDP, GPU, and VENUS devices on apq8096-db820c board.

Previous version of the patchset [1].

[V4]
   * Reworked the clock handling part. We now take clock names as data
     in the driver for supported compatible versions, and loop over them
     to get, enable, and disable the clocks.
   * Using qcom,msm8996 based compatibles for bindings instead of a generic
     qcom compatible.
   * Refactor MMU500 patch to just add the necessary clock names data and
     corresponding bindings.
   * Added the pm_runtime_get/put() calls in .unmap iommu op (fix added by
     Stanimir on top of previous patch version.
   * Added a patch to fix error path in arm_smmu_add_device()
   * Removed patch 3/5 of V3 patch series that added qcom,smmu-v2 bindings.

[V3]
   * Reworked the patches to keep the clocks init/enabling function
     separately for each compatible.

   * Added clocks bindings for MMU40x/500.

   * Added a new compatible for qcom,smmu-v2 implementation and
     the clock bindings for the same.

   * Rebased on top of 4.11-rc1

[V2]
   * Split the patches little differently.

   * Addressed comments.

   * Removed the patch #4 [3] from previous post
     for arm-smmu context save restore. Planning to
     post this separately after reworking/addressing Robin's
     feedback.

   * Reversed the sequence to disable clocks than enabling.
     This was required for those cases where the
     clocks are populated in a dependent order from DT.

[1] https://www.spinics.net/lists/arm-kernel/msg567488.html
[2] https://lkml.org/lkml/2016/10/20/70
[3] https://patchwork.kernel.org/patch/9389717/

Sricharan R (4):
  iommu/arm-smmu: Add pm_runtime/sleep ops
  iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
  iommu/arm-smmu: Add the device_link between masters and smmu
  iommu/arm-smmu: Add support for MMU40x/500 clocks

Vivek Gautam (2):
  iommu/arm-smmu: Fix the error path in arm_smmu_add_device
  iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks

 .../devicetree/bindings/iommu/arm,smmu.txt         |  42 +++++
 drivers/iommu/arm-smmu.c                           | 191 +++++++++++++++++++--
 2 files changed, 222 insertions(+), 11 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1682254 — [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

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.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
[vivek: Clock rework to loop over clock names data]
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 61b1f8729a7c..bfe613f8939c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,6 +48,7 @@
 #include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -196,6 +197,9 @@ struct arm_smmu_device {
 	u32				num_global_irqs;
 	u32				num_context_irqs;
 	unsigned int			*irqs;
+	int                             num_clks;
+	struct clk                      **clocks;
+	const char * const		*clk_names;
 
 	u32				cavium_id_base; /* Specific to Cavium */
 
@@ -272,6 +276,32 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
 	} while (arm_smmu_options[++i].opt);
 }
 
+static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < smmu->num_clks; ++i) {
+		ret = clk_prepare_enable(smmu->clocks[i]);
+		if (ret) {
+			dev_err(smmu->dev, "Couldn't enable %s clock\n",
+				smmu->clk_names[i]);
+			while (i--)
+				clk_disable_unprepare(smmu->clocks[i]);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
+{
+	int i = smmu->num_clks;
+
+	while (i--)
+		clk_disable_unprepare(smmu->clocks[i]);
+}
+
 static struct device_node *dev_get_dev_node(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
@@ -1626,6 +1656,36 @@ static int arm_smmu_id_size_to_bits(int size)
 	}
 }
 
+static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
+{
+	int i, err;
+	struct device *dev = smmu->dev;
+
+	if (smmu->num_clks < 1)
+		return 0;
+
+	smmu->clocks = devm_kcalloc(dev, smmu->num_clks,
+				    sizeof(*smmu->clocks), GFP_KERNEL);
+	if (!smmu->clocks)
+		return -ENOMEM;
+
+	for (i = 0; i < smmu->num_clks; i++) {
+		const char *cname = smmu->clk_names[i];
+		struct clk *c = devm_clk_get(dev, cname);
+
+		if (IS_ERR(c)) {
+			err = PTR_ERR(c);
+			if (err != -EPROBE_DEFER)
+				dev_err(dev, "Couldn't get clock: %s", cname);
+
+			return err;
+		}
+		smmu->clocks[i] = c;
+	}
+
+	return 0;
+}
+
 static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
@@ -1833,10 +1893,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 struct arm_smmu_match_data {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char * const *clks;
+	int num_clks;
 };
 
 #define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
 
 ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
@@ -1937,6 +1999,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 	data = of_device_get_match_data(dev);
 	smmu->version = data->version;
 	smmu->model = data->model;
+	smmu->clk_names = data->clks;
+	smmu->num_clks = data->num_clks;
 
 	parse_driver_options(smmu);
 
@@ -2035,6 +2099,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		smmu->irqs[i] = irq;
 	}
 
+	err = arm_smmu_init_clocks(smmu);
+	if (err)
+		return err;
+
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -2120,10 +2188,35 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int arm_smmu_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	return arm_smmu_enable_clocks(smmu);
+}
+
+static int arm_smmu_suspend(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	arm_smmu_disable_clocks(smmu);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_RUNTIME_PM_OPS(arm_smmu_suspend, arm_smmu_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
 		.name		= "arm-smmu",
 		.of_match_table	= of_match_ptr(arm_smmu_of_match),
+		.pm = &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686098 — Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

On 07/06, 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.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> [vivek: Clock rework to loop over clock names data]
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---

General comment, we have a bulk clk API now, but I guess we
failed to add the clk_bulk_prepare_enable() API that could be
used here. Perhaps you can add that API and then use it here to
reduce lines of code.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686111 — Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

On 07/12, Stephen Boyd wrote:
> On 07/06, 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.
> > 
> > Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> > Signed-off-by: Archit Taneja <architt@codeaurora.org>
> > [vivek: Clock rework to loop over clock names data]
> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> > ---
> 
> General comment, we have a bulk clk API now, but I guess we
> failed to add the clk_bulk_prepare_enable() API that could be
> used here. Perhaps you can add that API and then use it here to
> reduce lines of code.
> 

Bjorn just sent a patch for that API an hour ago.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686222 — Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

On 07/13/2017 04:31 AM, Stephen Boyd wrote:
> On 07/12, Stephen Boyd wrote:
>> On 07/06, 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.
>>>
>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>> [vivek: Clock rework to loop over clock names data]
>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>> ---
>> General comment, we have a bulk clk API now, but I guess we
>> failed to add the clk_bulk_prepare_enable() API that could be
>> used here. Perhaps you can add that API and then use it here to
>> reduce lines of code.

Sure, will use the bulk clock APIs to handle the clocks.

Best regards
Vivek

>>
> Bjorn just sent a patch for that API an hour ago.
>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1682255 — [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

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>
[stanimir: added runtime pm in .unmap iommu op]
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 54 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bfe613f8939c..ddbfa8ab69e6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -897,11 +897,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	void __iomem *cb_base;
-	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.
@@ -916,6 +920,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)
@@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 			     size_t size)
 {
-	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+	size_t ret;
 
 	if (!ops)
 		return 0;
 
-	return ops->unmap(ops, iova, size);
+	pm_runtime_get_sync(smmu_domain->smmu->dev);
+	ret = ops->unmap(ops, iova, size);
+	pm_runtime_put_sync(smmu_domain->smmu->dev);
+
+	return ret;
 }
 
 static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
@@ -1377,12 +1389,20 @@ 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) {
+		pm_runtime_put_sync(smmu->dev);
+		goto out_cfg_free;
+	}
+
 	iommu_device_link(&smmu->iommu, dev);
 
+	pm_runtime_put_sync(smmu->dev);
+
 	return 0;
 
 out_cfg_free:
@@ -1397,7 +1417,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;
@@ -1405,8 +1425,21 @@ static void arm_smmu_remove_device(struct device *dev)
 	cfg  = fwspec->iommu_priv;
 	smmu = cfg->smmu;
 
+	/*
+	 * The device link between the master device and
+	 * smmu is already purged at this point.
+	 * So enable the power to smmu explicitly.
+	 */
+
+	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);
@@ -2103,6 +2136,13 @@ 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;
@@ -2144,9 +2184,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
@@ -2185,6 +2225,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 
 	/* Turn the thing off */
 	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+	pm_runtime_force_suspend(smmu->dev);
+
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686100 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On 07/06, Vivek Gautam wrote:
> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>  			     size_t size)
>  {
> -	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> +	size_t ret;
>  
>  	if (!ops)
>  		return 0;
>  
> -	return ops->unmap(ops, iova, size);
> +	pm_runtime_get_sync(smmu_domain->smmu->dev);

Can these map/unmap ops be called from an atomic context? I seem
to recall that being a problem before.


> +	ret = ops->unmap(ops, iova, size);
> +	pm_runtime_put_sync(smmu_domain->smmu->dev);
> +
> +	return ret;
>  }
>  
>  static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686246 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi Stephen,


On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> On 07/06, Vivek Gautam wrote:
>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>   			     size_t size)
>>   {
>> -	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> +	size_t ret;
>>   
>>   	if (!ops)
>>   		return 0;
>>   
>> -	return ops->unmap(ops, iova, size);
>> +	pm_runtime_get_sync(smmu_domain->smmu->dev);
> Can these map/unmap ops be called from an atomic context? I seem
> to recall that being a problem before.

That's something which was dropped in the following patch merged in master:
523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock

Looks like we don't  need locks here anymore?

Best Regards
Vivek

>
>
>> +	ret = ops->unmap(ops, iova, size);
>> +	pm_runtime_put_sync(smmu_domain->smmu->dev);
>> +
>> +	return ret;
>>   }
>>   
>>   static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686254 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi Vivek,

On 7/13/2017 10:43 AM, Vivek Gautam wrote:
> Hi Stephen,
> 
> 
> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>> On 07/06, Vivek Gautam wrote:
>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>                    size_t size)
>>>   {
>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>> +    size_t ret;
>>>         if (!ops)
>>>           return 0;
>>>   -    return ops->unmap(ops, iova, size);
>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>> Can these map/unmap ops be called from an atomic context? I seem
>> to recall that being a problem before.
> 
> That's something which was dropped in the following patch merged in master:
> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> 
> Looks like we don't  need locks here anymore?

 Apart from the locking, wonder why a explicit pm_runtime is needed
 from unmap. Somehow looks like some path in the master using that
 should have enabled the pm ?

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[toc] | [prev] | [next] | [standalone]

#1686477 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> Hi Vivek,
>
> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> Hi Stephen,
>>
>>
>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>> On 07/06, Vivek Gautam wrote:
>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>                    size_t size)
>>>>   {
>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>> +    size_t ret;
>>>>         if (!ops)
>>>>           return 0;
>>>>   -    return ops->unmap(ops, iova, size);
>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>> Can these map/unmap ops be called from an atomic context? I seem
>>> to recall that being a problem before.
>>
>> That's something which was dropped in the following patch merged in master:
>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>
>> Looks like we don't  need locks here anymore?
>
>  Apart from the locking, wonder why a explicit pm_runtime is needed
>  from unmap. Somehow looks like some path in the master using that
>  should have enabled the pm ?
>

Yes, there are a bunch of scenarios where unmap can happen with
disabled master (but not in atomic context).  On the gpu side we
opportunistically keep a buffer mapping until the buffer is freed
(which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
an exported dmabuf while some other driver holds a reference to it
(which can be dropped when the v4l2 device is suspended).

Since unmap triggers tbl flush which touches iommu regs, the iommu
driver *definitely* needs a pm_runtime_get_sync().

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1686482 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi All,

On 2017-07-13 13:50, Rob Clark wrote:
> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>> On 07/06, Vivek Gautam wrote:
>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>    static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>                     size_t size)
>>>>>    {
>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>> +    size_t ret;
>>>>>          if (!ops)
>>>>>            return 0;
>>>>>    -    return ops->unmap(ops, iova, size);
>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>> to recall that being a problem before.
>>> That's something which was dropped in the following patch merged in master:
>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>
>>> Looks like we don't  need locks here anymore?
>>   Apart from the locking, wonder why a explicit pm_runtime is needed
>>   from unmap. Somehow looks like some path in the master using that
>>   should have enabled the pm ?
>>
> Yes, there are a bunch of scenarios where unmap can happen with
> disabled master (but not in atomic context).  On the gpu side we
> opportunistically keep a buffer mapping until the buffer is freed
> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> an exported dmabuf while some other driver holds a reference to it
> (which can be dropped when the v4l2 device is suspended).
>
> Since unmap triggers tbl flush which touches iommu regs, the iommu
> driver *definitely* needs a pm_runtime_get_sync().

Afair unmap might be called from atomic context as well, for example as
a result of dma_unmap_page(). In exynos IOMMU I simply check the runtime
PM state of IOMMU device. TLB flush is performed only when IOMMU is in 
active
state. If it is suspended, I assume that the IOMMU controller's context
is already lost and its respective power domain might be already turned off,
so there is no point in touching IOMMU registers.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1686489 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 8:02 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi All,
>
> On 2017-07-13 13:50, Rob Clark wrote:
>>
>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org>
>> wrote:
>>>
>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>>
>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>>
>>>>> On 07/06, Vivek Gautam wrote:
>>>>>>
>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain
>>>>>> *domain, unsigned long iova,
>>>>>>    static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned
>>>>>> long iova,
>>>>>>                     size_t size)
>>>>>>    {
>>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>> +    size_t ret;
>>>>>>          if (!ops)
>>>>>>            return 0;
>>>>>>    -    return ops->unmap(ops, iova, size);
>>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>>
>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>> to recall that being a problem before.
>>>>
>>>> That's something which was dropped in the following patch merged in
>>>> master:
>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>
>>>> Looks like we don't  need locks here anymore?
>>>
>>>   Apart from the locking, wonder why a explicit pm_runtime is needed
>>>   from unmap. Somehow looks like some path in the master using that
>>>   should have enabled the pm ?
>>>
>> Yes, there are a bunch of scenarios where unmap can happen with
>> disabled master (but not in atomic context).  On the gpu side we
>> opportunistically keep a buffer mapping until the buffer is freed
>> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> an exported dmabuf while some other driver holds a reference to it
>> (which can be dropped when the v4l2 device is suspended).
>>
>> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> driver *definitely* needs a pm_runtime_get_sync().
>
>
> Afair unmap might be called from atomic context as well, for example as
> a result of dma_unmap_page(). In exynos IOMMU I simply check the runtime
> PM state of IOMMU device. TLB flush is performed only when IOMMU is in
> active
> state. If it is suspended, I assume that the IOMMU controller's context
> is already lost and its respective power domain might be already turned off,
> so there is no point in touching IOMMU registers.
>

that seems like an interesting approach.. although I wonder if there
can be some race w/ new device memory access once clks are enabled
before tlb flush completes?  That would be rather bad, since this
approach is letting the backing pages of memory be freed before tlb
flush.

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1686500 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi Rob,

On 2017-07-13 14:10, Rob Clark wrote:
> On Thu, Jul 13, 2017 at 8:02 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 2017-07-13 13:50, Rob Clark wrote:
>>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org>
>>> wrote:
>>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>>> On 07/06, Vivek Gautam wrote:
>>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain
>>>>>>> *domain, unsigned long iova,
>>>>>>>     static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned
>>>>>>> long iova,
>>>>>>>                      size_t size)
>>>>>>>     {
>>>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>>> +    size_t ret;
>>>>>>>           if (!ops)
>>>>>>>             return 0;
>>>>>>>     -    return ops->unmap(ops, iova, size);
>>>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>>> to recall that being a problem before.
>>>>> That's something which was dropped in the following patch merged in
>>>>> master:
>>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>>
>>>>> Looks like we don't  need locks here anymore?
>>>>    Apart from the locking, wonder why a explicit pm_runtime is needed
>>>>    from unmap. Somehow looks like some path in the master using that
>>>>    should have enabled the pm ?
>>>>
>>> Yes, there are a bunch of scenarios where unmap can happen with
>>> disabled master (but not in atomic context).  On the gpu side we
>>> opportunistically keep a buffer mapping until the buffer is freed
>>> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>>> an exported dmabuf while some other driver holds a reference to it
>>> (which can be dropped when the v4l2 device is suspended).
>>>
>>> Since unmap triggers tbl flush which touches iommu regs, the iommu
>>> driver *definitely* needs a pm_runtime_get_sync().
>>
>> Afair unmap might be called from atomic context as well, for example as
>> a result of dma_unmap_page(). In exynos IOMMU I simply check the runtime
>> PM state of IOMMU device. TLB flush is performed only when IOMMU is in
>> active
>> state. If it is suspended, I assume that the IOMMU controller's context
>> is already lost and its respective power domain might be already turned off,
>> so there is no point in touching IOMMU registers.
>>
> that seems like an interesting approach.. although I wonder if there
> can be some race w/ new device memory access once clks are enabled
> before tlb flush completes?  That would be rather bad, since this
> approach is letting the backing pages of memory be freed before tlb
> flush.

Exynos IOMMU has spinlock for ensuring that there is no race between PM 
runtime
suspend and unmap/tlb flush.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1686539 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi,

On 7/13/2017 5:20 PM, Rob Clark wrote:
> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> Hi Vivek,
>>
>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>> Hi Stephen,
>>>
>>>
>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>> On 07/06, Vivek Gautam wrote:
>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>                    size_t size)
>>>>>   {
>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>> +    size_t ret;
>>>>>         if (!ops)
>>>>>           return 0;
>>>>>   -    return ops->unmap(ops, iova, size);
>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>> to recall that being a problem before.
>>>
>>> That's something which was dropped in the following patch merged in master:
>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>
>>> Looks like we don't  need locks here anymore?
>>
>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>>  from unmap. Somehow looks like some path in the master using that
>>  should have enabled the pm ?
>>
> 
> Yes, there are a bunch of scenarios where unmap can happen with
> disabled master (but not in atomic context).  On the gpu side we
> opportunistically keep a buffer mapping until the buffer is freed
> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> an exported dmabuf while some other driver holds a reference to it
> (which can be dropped when the v4l2 device is suspended).
> 
> Since unmap triggers tbl flush which touches iommu regs, the iommu
> driver *definitely* needs a pm_runtime_get_sync().

 Ok, with that being the case, there are two things here,

 1) If the device links are still intact at these places where unmap is called,
    then pm_runtime from the master would setup the all the clocks. That would
    avoid reintroducing the locking indirectly here.

 2) If not, then doing it here is the only way. But for both cases, since
    the unmap can be called from atomic context, resume handler here should
    avoid doing clk_prepare_enable , instead move the clk_prepare to the init.

Regards,
 Sricharan


-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[toc] | [prev] | [next] | [standalone]

#1686596 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> Hi,
>
> On 7/13/2017 5:20 PM, Rob Clark wrote:
>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>>> Hi Vivek,
>>>
>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>> Hi Stephen,
>>>>
>>>>
>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>> On 07/06, Vivek Gautam wrote:
>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>>                    size_t size)
>>>>>>   {
>>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>> +    size_t ret;
>>>>>>         if (!ops)
>>>>>>           return 0;
>>>>>>   -    return ops->unmap(ops, iova, size);
>>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>> to recall that being a problem before.
>>>>
>>>> That's something which was dropped in the following patch merged in master:
>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>
>>>> Looks like we don't  need locks here anymore?
>>>
>>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>>>  from unmap. Somehow looks like some path in the master using that
>>>  should have enabled the pm ?
>>>
>>
>> Yes, there are a bunch of scenarios where unmap can happen with
>> disabled master (but not in atomic context).  On the gpu side we
>> opportunistically keep a buffer mapping until the buffer is freed
>> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> an exported dmabuf while some other driver holds a reference to it
>> (which can be dropped when the v4l2 device is suspended).
>>
>> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> driver *definitely* needs a pm_runtime_get_sync().
>
>  Ok, with that being the case, there are two things here,
>
>  1) If the device links are still intact at these places where unmap is called,
>     then pm_runtime from the master would setup the all the clocks. That would
>     avoid reintroducing the locking indirectly here.
>
>  2) If not, then doing it here is the only way. But for both cases, since
>     the unmap can be called from atomic context, resume handler here should
>     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>

I do kinda like the approach Marek suggested.. of deferring the tlb
flush until resume.  I'm wondering if we could combine that with
putting the mmu in a stalled state when we suspend (and not resume the
mmu until after the pending tlb flush)?

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1687572 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> > Hi,
> >
> > On 7/13/2017 5:20 PM, Rob Clark wrote:
> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >>> Hi Vivek,
> >>>
> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
> >>>> Hi Stephen,
> >>>>
> >>>>
> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> >>>>> On 07/06, Vivek Gautam wrote:
> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
> >>>>>>                    size_t size)
> >>>>>>   {
> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> >>>>>> +    size_t ret;
> >>>>>>         if (!ops)
> >>>>>>           return 0;
> >>>>>>   -    return ops->unmap(ops, iova, size);
> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
> >>>>> Can these map/unmap ops be called from an atomic context? I seem
> >>>>> to recall that being a problem before.
> >>>>
> >>>> That's something which was dropped in the following patch merged in master:
> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> >>>>
> >>>> Looks like we don't  need locks here anymore?
> >>>
> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
> >>>  from unmap. Somehow looks like some path in the master using that
> >>>  should have enabled the pm ?
> >>>
> >>
> >> Yes, there are a bunch of scenarios where unmap can happen with
> >> disabled master (but not in atomic context).  On the gpu side we
> >> opportunistically keep a buffer mapping until the buffer is freed
> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> >> an exported dmabuf while some other driver holds a reference to it
> >> (which can be dropped when the v4l2 device is suspended).
> >>
> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
> >> driver *definitely* needs a pm_runtime_get_sync().
> >
> >  Ok, with that being the case, there are two things here,
> >
> >  1) If the device links are still intact at these places where unmap is called,
> >     then pm_runtime from the master would setup the all the clocks. That would
> >     avoid reintroducing the locking indirectly here.
> >
> >  2) If not, then doing it here is the only way. But for both cases, since
> >     the unmap can be called from atomic context, resume handler here should
> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
> >
> 
> I do kinda like the approach Marek suggested.. of deferring the tlb
> flush until resume.  I'm wondering if we could combine that with
> putting the mmu in a stalled state when we suspend (and not resume the
> mmu until after the pending tlb flush)?

I'm not sure that a stalled state is what we're after here, because we need
to take care to prevent any table walks if we've freed the underlying pages.
What we could try to do is disable the SMMU (put into global bypass) and
invalidate the TLB when performing a suspend operation, then we just ignore
invalidation whilst the clocks are stopped and, on resume, enable the SMMU
again.

That said, I don't think we can tolerate suspend/resume racing with
map/unmap, and it's not clear to me how we avoid that without penalising
the fastpath.

Will

[toc] | [prev] | [next] | [standalone]

#1687588 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> > Hi,
>> >
>> > On 7/13/2017 5:20 PM, Rob Clark wrote:
>> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >>> Hi Vivek,
>> >>>
>> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> >>>> Hi Stephen,
>> >>>>
>> >>>>
>> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>> >>>>> On 07/06, Vivek Gautam wrote:
>> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>> >>>>>>                    size_t size)
>> >>>>>>   {
>> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> >>>>>> +    size_t ret;
>> >>>>>>         if (!ops)
>> >>>>>>           return 0;
>> >>>>>>   -    return ops->unmap(ops, iova, size);
>> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>> >>>>> Can these map/unmap ops be called from an atomic context? I seem
>> >>>>> to recall that being a problem before.
>> >>>>
>> >>>> That's something which was dropped in the following patch merged in master:
>> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>> >>>>
>> >>>> Looks like we don't  need locks here anymore?
>> >>>
>> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>> >>>  from unmap. Somehow looks like some path in the master using that
>> >>>  should have enabled the pm ?
>> >>>
>> >>
>> >> Yes, there are a bunch of scenarios where unmap can happen with
>> >> disabled master (but not in atomic context).  On the gpu side we
>> >> opportunistically keep a buffer mapping until the buffer is freed
>> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> >> an exported dmabuf while some other driver holds a reference to it
>> >> (which can be dropped when the v4l2 device is suspended).
>> >>
>> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> >> driver *definitely* needs a pm_runtime_get_sync().
>> >
>> >  Ok, with that being the case, there are two things here,
>> >
>> >  1) If the device links are still intact at these places where unmap is called,
>> >     then pm_runtime from the master would setup the all the clocks. That would
>> >     avoid reintroducing the locking indirectly here.
>> >
>> >  2) If not, then doing it here is the only way. But for both cases, since
>> >     the unmap can be called from atomic context, resume handler here should
>> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>> >
>>
>> I do kinda like the approach Marek suggested.. of deferring the tlb
>> flush until resume.  I'm wondering if we could combine that with
>> putting the mmu in a stalled state when we suspend (and not resume the
>> mmu until after the pending tlb flush)?
>
> I'm not sure that a stalled state is what we're after here, because we need
> to take care to prevent any table walks if we've freed the underlying pages.
> What we could try to do is disable the SMMU (put into global bypass) and
> invalidate the TLB when performing a suspend operation, then we just ignore
> invalidation whilst the clocks are stopped and, on resume, enable the SMMU
> again.

wouldn't stalled just block any memory transactions by device(s) using
the context bank?  Putting it in bypass isn't really a good thing if
there is any chance the device can sneak in a memory access before
we've taking it back out of bypass (ie. makes gpu a giant userspace
controlled root hole).

BR,
-R

> That said, I don't think we can tolerate suspend/resume racing with
> map/unmap, and it's not clear to me how we avoid that without penalising
> the fastpath.
>
> Will

[toc] | [prev] | [next] | [standalone]

#1687592 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> > Hi,
> >> >
> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> >>> Hi Vivek,
> >> >>>
> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
> >> >>>> Hi Stephen,
> >> >>>>
> >> >>>>
> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> >> >>>>> On 07/06, Vivek Gautam wrote:
> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
> >> >>>>>>                    size_t size)
> >> >>>>>>   {
> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> >> >>>>>> +    size_t ret;
> >> >>>>>>         if (!ops)
> >> >>>>>>           return 0;
> >> >>>>>>   -    return ops->unmap(ops, iova, size);
> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
> >> >>>>> to recall that being a problem before.
> >> >>>>
> >> >>>> That's something which was dropped in the following patch merged in master:
> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> >> >>>>
> >> >>>> Looks like we don't  need locks here anymore?
> >> >>>
> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
> >> >>>  from unmap. Somehow looks like some path in the master using that
> >> >>>  should have enabled the pm ?
> >> >>>
> >> >>
> >> >> Yes, there are a bunch of scenarios where unmap can happen with
> >> >> disabled master (but not in atomic context).  On the gpu side we
> >> >> opportunistically keep a buffer mapping until the buffer is freed
> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> >> >> an exported dmabuf while some other driver holds a reference to it
> >> >> (which can be dropped when the v4l2 device is suspended).
> >> >>
> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
> >> >> driver *definitely* needs a pm_runtime_get_sync().
> >> >
> >> >  Ok, with that being the case, there are two things here,
> >> >
> >> >  1) If the device links are still intact at these places where unmap is called,
> >> >     then pm_runtime from the master would setup the all the clocks. That would
> >> >     avoid reintroducing the locking indirectly here.
> >> >
> >> >  2) If not, then doing it here is the only way. But for both cases, since
> >> >     the unmap can be called from atomic context, resume handler here should
> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
> >> >
> >>
> >> I do kinda like the approach Marek suggested.. of deferring the tlb
> >> flush until resume.  I'm wondering if we could combine that with
> >> putting the mmu in a stalled state when we suspend (and not resume the
> >> mmu until after the pending tlb flush)?
> >
> > I'm not sure that a stalled state is what we're after here, because we need
> > to take care to prevent any table walks if we've freed the underlying pages.
> > What we could try to do is disable the SMMU (put into global bypass) and
> > invalidate the TLB when performing a suspend operation, then we just ignore
> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
> > again.
> 
> wouldn't stalled just block any memory transactions by device(s) using
> the context bank?  Putting it in bypass isn't really a good thing if
> there is any chance the device can sneak in a memory access before
> we've taking it back out of bypass (ie. makes gpu a giant userspace
> controlled root hole).

If it doesn't deadlock, then yes, it will stall transactions. However, that
doesn't mean it necessarily prevents page table walks. Instead of bypass, we
could configure all the streams to terminate, but this race still worries me
somewhat. I thought that the SMMU would only be suspended if all of its
masters were suspended, so if the GPU wants to come out of suspend then the
SMMU should be resumed first.

It would be helpful if somebody could figure out exactly what can race with
the suspend/resume calls here.

Will

[toc] | [prev] | [next] | [standalone]

#1687595 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
>> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> > Hi,
>> >> >
>> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
>> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> >>> Hi Vivek,
>> >> >>>
>> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> >> >>>> Hi Stephen,
>> >> >>>>
>> >> >>>>
>> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>> >> >>>>> On 07/06, Vivek Gautam wrote:
>> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>> >> >>>>>>                    size_t size)
>> >> >>>>>>   {
>> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> >> >>>>>> +    size_t ret;
>> >> >>>>>>         if (!ops)
>> >> >>>>>>           return 0;
>> >> >>>>>>   -    return ops->unmap(ops, iova, size);
>> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
>> >> >>>>> to recall that being a problem before.
>> >> >>>>
>> >> >>>> That's something which was dropped in the following patch merged in master:
>> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>> >> >>>>
>> >> >>>> Looks like we don't  need locks here anymore?
>> >> >>>
>> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>> >> >>>  from unmap. Somehow looks like some path in the master using that
>> >> >>>  should have enabled the pm ?
>> >> >>>
>> >> >>
>> >> >> Yes, there are a bunch of scenarios where unmap can happen with
>> >> >> disabled master (but not in atomic context).  On the gpu side we
>> >> >> opportunistically keep a buffer mapping until the buffer is freed
>> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> >> >> an exported dmabuf while some other driver holds a reference to it
>> >> >> (which can be dropped when the v4l2 device is suspended).
>> >> >>
>> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> >> >> driver *definitely* needs a pm_runtime_get_sync().
>> >> >
>> >> >  Ok, with that being the case, there are two things here,
>> >> >
>> >> >  1) If the device links are still intact at these places where unmap is called,
>> >> >     then pm_runtime from the master would setup the all the clocks. That would
>> >> >     avoid reintroducing the locking indirectly here.
>> >> >
>> >> >  2) If not, then doing it here is the only way. But for both cases, since
>> >> >     the unmap can be called from atomic context, resume handler here should
>> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>> >> >
>> >>
>> >> I do kinda like the approach Marek suggested.. of deferring the tlb
>> >> flush until resume.  I'm wondering if we could combine that with
>> >> putting the mmu in a stalled state when we suspend (and not resume the
>> >> mmu until after the pending tlb flush)?
>> >
>> > I'm not sure that a stalled state is what we're after here, because we need
>> > to take care to prevent any table walks if we've freed the underlying pages.
>> > What we could try to do is disable the SMMU (put into global bypass) and
>> > invalidate the TLB when performing a suspend operation, then we just ignore
>> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
>> > again.
>>
>> wouldn't stalled just block any memory transactions by device(s) using
>> the context bank?  Putting it in bypass isn't really a good thing if
>> there is any chance the device can sneak in a memory access before
>> we've taking it back out of bypass (ie. makes gpu a giant userspace
>> controlled root hole).
>
> If it doesn't deadlock, then yes, it will stall transactions. However, that
> doesn't mean it necessarily prevents page table walks.

btw, I guess the concern about pagetable walk is that the unmap could
have removed some sub-level of the pt that the tlb walk would hit?
Would deferring freeing those pages help?

> Instead of bypass, we
> could configure all the streams to terminate, but this race still worries me
> somewhat. I thought that the SMMU would only be suspended if all of its
> masters were suspended, so if the GPU wants to come out of suspend then the
> SMMU should be resumed first.

I believe this should be true.. on the gpu side, I'm mostly trying to
avoid having to power the gpu back on to free buffers.  (On the v4l2
side, somewhere in the core videobuf code would also need to be made
to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)

BR,
-R

> It would be helpful if somebody could figure out exactly what can race with
> the suspend/resume calls here.
>
> Will

[toc] | [prev] | [next] | [standalone]

#1687608 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
> >> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
> >> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
> >> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> >> >>> Hi Vivek,
> >> >> >>>
> >> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
> >> >> >>>> Hi Stephen,
> >> >> >>>>
> >> >> >>>>
> >> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> >> >> >>>>> On 07/06, Vivek Gautam wrote:
> >> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> >> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
> >> >> >>>>>>                    size_t size)
> >> >> >>>>>>   {
> >> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> >> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> >> >> >>>>>> +    size_t ret;
> >> >> >>>>>>         if (!ops)
> >> >> >>>>>>           return 0;
> >> >> >>>>>>   -    return ops->unmap(ops, iova, size);
> >> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
> >> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
> >> >> >>>>> to recall that being a problem before.
> >> >> >>>>
> >> >> >>>> That's something which was dropped in the following patch merged in master:
> >> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> >> >> >>>>
> >> >> >>>> Looks like we don't  need locks here anymore?
> >> >> >>>
> >> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
> >> >> >>>  from unmap. Somehow looks like some path in the master using that
> >> >> >>>  should have enabled the pm ?
> >> >> >>>
> >> >> >>
> >> >> >> Yes, there are a bunch of scenarios where unmap can happen with
> >> >> >> disabled master (but not in atomic context).  On the gpu side we
> >> >> >> opportunistically keep a buffer mapping until the buffer is freed
> >> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> >> >> >> an exported dmabuf while some other driver holds a reference to it
> >> >> >> (which can be dropped when the v4l2 device is suspended).
> >> >> >>
> >> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
> >> >> >> driver *definitely* needs a pm_runtime_get_sync().
> >> >> >
> >> >> >  Ok, with that being the case, there are two things here,
> >> >> >
> >> >> >  1) If the device links are still intact at these places where unmap is called,
> >> >> >     then pm_runtime from the master would setup the all the clocks. That would
> >> >> >     avoid reintroducing the locking indirectly here.
> >> >> >
> >> >> >  2) If not, then doing it here is the only way. But for both cases, since
> >> >> >     the unmap can be called from atomic context, resume handler here should
> >> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
> >> >> >
> >> >>
> >> >> I do kinda like the approach Marek suggested.. of deferring the tlb
> >> >> flush until resume.  I'm wondering if we could combine that with
> >> >> putting the mmu in a stalled state when we suspend (and not resume the
> >> >> mmu until after the pending tlb flush)?
> >> >
> >> > I'm not sure that a stalled state is what we're after here, because we need
> >> > to take care to prevent any table walks if we've freed the underlying pages.
> >> > What we could try to do is disable the SMMU (put into global bypass) and
> >> > invalidate the TLB when performing a suspend operation, then we just ignore
> >> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
> >> > again.
> >>
> >> wouldn't stalled just block any memory transactions by device(s) using
> >> the context bank?  Putting it in bypass isn't really a good thing if
> >> there is any chance the device can sneak in a memory access before
> >> we've taking it back out of bypass (ie. makes gpu a giant userspace
> >> controlled root hole).
> >
> > If it doesn't deadlock, then yes, it will stall transactions. However, that
> > doesn't mean it necessarily prevents page table walks.
> 
> btw, I guess the concern about pagetable walk is that the unmap could
> have removed some sub-level of the pt that the tlb walk would hit?
> Would deferring freeing those pages help?

Could do, but it sounds like a lot of complication that I think we can fix
by making the suspend operation put the SMMU into a "clean" state.

> > Instead of bypass, we
> > could configure all the streams to terminate, but this race still worries me
> > somewhat. I thought that the SMMU would only be suspended if all of its
> > masters were suspended, so if the GPU wants to come out of suspend then the
> > SMMU should be resumed first.
> 
> I believe this should be true.. on the gpu side, I'm mostly trying to
> avoid having to power the gpu back on to free buffers.  (On the v4l2
> side, somewhere in the core videobuf code would also need to be made
> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)

Right, and we shouldn't have to resume it if we suspend it in a clean state,
with the TLBs invalidated.

Will

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web