[PATCH v3 0/8] panfrost: Locking and runtime PM fixes

[PATCH v3 0/8] panfrost: Locking and runtime PM fixes

[Rob Herring]

With further testing of recent changes with lockdep identified some
locking issues. Avoiding lockdep issues means we need to avoid some
locks in panfrost_mmu_unmap which in turn means avoiding runtime PM
resume. In the process of re-working runtime PM several runtime PM
and locking clean-ups have been identified.

v3:
 - Applied patches 1, 4, 5, and 6
 - Fix race in job timeout handling with ISR
 - Remove some no longer needed locks
 - Fix panfrost_mmu_unmap when autosuspend delay is > 0
 - Disable AS MMU when freeing page tables

v2:
 - Drop already applied 'drm/panfrost: Fix sleeping while atomic in
   panfrost_gem_open'
 - Runtime PM clean-ups
 - Keep panfrost_gem_purge and use mutex_trylock there
 - Rework panfrost_mmu_unmap runtime PM

Rob

Rob Herring (8):
  drm/panfrost: Rework runtime PM initialization
  drm/panfrost: Hold runtime PM reference until jobs complete
  drm/panfrost: Remove unnecessary mmu->lock mutex
  drm/panfrost: Rework page table flushing and runtime PM interaction
  drm/panfrost: Split mmu_hw_do_operation into locked and unlocked
    version
  drm/panfrost: Add cache/TLB flush before switching address space
  drm/panfrost: Flush and disable address space when freeing page tables
  drm/panfrost: Remove unnecessary hwaccess_lock spin_lock

 drivers/gpu/drm/panfrost/panfrost_device.c | 10 ---
 drivers/gpu/drm/panfrost/panfrost_device.h |  3 -
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 ++-
 drivers/gpu/drm/panfrost/panfrost_job.c    | 43 ++++++----
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 91 ++++++++++------------
 5 files changed, 76 insertions(+), 81 deletions(-)

--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/8] drm/panfrost: Rework runtime PM initialization

[Rob Herring]

There's a few issues with the runtime PM initialization.

The documentation states pm_runtime_set_active() should be called before
pm_runtime_enable(). The pm_runtime_put_autosuspend() could suspend the GPU
before panfrost_perfcnt_init() is called which touches the h/w. The
autosuspend delay keeps things from breaking. There's no need explicitly
power off the GPU only to wake back up with pm_runtime_get_sync(). Just
delaying pm_runtime_enable to the end of probe is sufficient.

Lets move all the runtime PM calls into the probe() function so they are
all in one place and are done after all initialization.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Move autosuspend setup after pm_runtime_enable as only autosuspend changes
   trigger suspend.
 - Fix autosuspend delay to 50ms instead of 0.

 drivers/gpu/drm/panfrost/panfrost_device.c |  9 ---------
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 ++++++----
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 4da71bb56c20..73805210834e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -5,7 +5,6 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>

 #include "panfrost_device.h"
@@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	if (err)
 		goto err_out4;

-	/* runtime PM will wake us up later */
-	panfrost_gpu_power_off(pfdev);
-
-	pm_runtime_set_active(pfdev->dev);
-	pm_runtime_get_sync(pfdev->dev);
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
-
 	err = panfrost_perfcnt_init(pfdev);
 	if (err)
 		goto err_out5;
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index d74442d71048..bc2ddeb55f5d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev)
 	mutex_init(&pfdev->shrinker_lock);
 	INIT_LIST_HEAD(&pfdev->shrinker_list);

-	pm_runtime_use_autosuspend(pfdev->dev);
-	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
-	pm_runtime_enable(pfdev->dev);
-
 	err = panfrost_device_init(pfdev);
 	if (err) {
 		if (err != -EPROBE_DEFER)
@@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev)
 		goto err_out1;
 	}

+	pm_runtime_set_active(pfdev->dev);
+	pm_runtime_mark_last_busy(pfdev->dev);
+	pm_runtime_enable(pfdev->dev);
+	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
+	pm_runtime_use_autosuspend(pfdev->dev);
+
 	/*
 	 * Register the DRM device with the core and the connectors with
 	 * sysfs
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/8] drm/panfrost: Hold runtime PM reference until jobs complete

[Rob Herring]

Doing a pm_runtime_put as soon as a job is submitted is wrong as it should
not happen until the job completes. It works currently because we are
relying on the autosuspend timeout to keep the h/w enabled.

Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Fix race between clearing pfdev->jobs[] in timeout and the ISR using the job_lock
 - Maintain pm_runtime_put in the panfrost_job_hw_submit error path

 drivers/gpu/drm/panfrost/panfrost_job.c | 39 ++++++++++++++++++-------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 05c85f45a0de..18bcc9bac6d2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -150,8 +150,10 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 	if (ret < 0)
 		return;

-	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
-		goto end;
+	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) {
+		pm_runtime_put_sync_autosuspend(pfdev->dev);
+		return;
+	}

 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);

@@ -187,10 +189,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 	job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);

 	spin_unlock_irqrestore(&pfdev->hwaccess_lock, flags);
-
-end:
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
 }

 static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
@@ -369,6 +367,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
 	struct panfrost_job *job = to_panfrost_job(sched_job);
 	struct panfrost_device *pfdev = job->pfdev;
 	int js = panfrost_job_get_slot(job);
+	unsigned long flags;
 	int i;

 	/*
@@ -394,6 +393,15 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
 	if (sched_job)
 		drm_sched_increase_karma(sched_job);

+	spin_lock_irqsave(&pfdev->js->job_lock, flags);
+	for (i = 0; i < NUM_JOB_SLOTS; i++) {
+		if (pfdev->jobs[i]) {
+			pm_runtime_put_noidle(pfdev->dev);
+			pfdev->jobs[i] = NULL;
+		}
+	}
+	spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
+
 	/* panfrost_core_dump(pfdev); */

 	panfrost_devfreq_record_transition(pfdev, js);
@@ -450,12 +458,21 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
 		}

 		if (status & JOB_INT_MASK_DONE(j)) {
-			struct panfrost_job *job = pfdev->jobs[j];
+			struct panfrost_job *job;
+
+			spin_lock(&pfdev->js->job_lock);
+			job = pfdev->jobs[j];
+			/* Only NULL if job timeout occurred */
+			if (job) {
+				pfdev->jobs[j] = NULL;
+
+				panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
+				panfrost_devfreq_record_transition(pfdev, j);

-			pfdev->jobs[j] = NULL;
-			panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
-			panfrost_devfreq_record_transition(pfdev, j);
-			dma_fence_signal(job->done_fence);
+				dma_fence_signal_locked(job->done_fence);
+				pm_runtime_put_autosuspend(pfdev->dev);
+			}
+			spin_unlock(&pfdev->js->job_lock);
 		}

 		status &= ~mask;
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/8] drm/panfrost: Remove unnecessary mmu->lock mutex

[Rob Herring]

There's no need to serialize io-pgtable calls and the as_lock is
sufficient to serialize flush operations, so we can remove the per
page table lock.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Suggested-by: Robin Murphy <robin.murphy@arm.com>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - new patch

 drivers/gpu/drm/panfrost/panfrost_device.h | 1 -
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index f503c566e99f..b7fa08ed3a23 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -104,7 +104,6 @@ struct panfrost_device {
 struct panfrost_mmu {
 	struct io_pgtable_cfg pgtbl_cfg;
 	struct io_pgtable_ops *pgtbl_ops;
-	struct mutex lock;
 	int as;
 	atomic_t as_count;
 	struct list_head list;
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 842bdd7cf6be..3a8bcfa7e7b6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -228,8 +228,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 	struct io_pgtable_ops *ops = mmu->pgtbl_ops;
 	u64 start_iova = iova;

-	mutex_lock(&mmu->lock);
-
 	for_each_sg(sgt->sgl, sgl, sgt->nents, count) {
 		unsigned long paddr = sg_dma_address(sgl);
 		size_t len = sg_dma_len(sgl);
@@ -249,8 +247,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
 			    AS_COMMAND_FLUSH_PT);

-	mutex_unlock(&mmu->lock);
-
 	return 0;
 }

@@ -304,8 +300,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 	if (ret < 0)
 		return;

-	mutex_lock(&bo->mmu->lock);
-
 	while (unmapped_len < len) {
 		size_t unmapped_page;
 		size_t pgsize = get_pgsize(iova, len - unmapped_len);
@@ -321,8 +315,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
 			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);

-	mutex_unlock(&bo->mmu->lock);
-
 	pm_runtime_mark_last_busy(pfdev->dev);
 	pm_runtime_put_autosuspend(pfdev->dev);
 	bo->is_mapped = false;
@@ -356,7 +348,6 @@ int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv)
 	struct panfrost_mmu *mmu = &priv->mmu;
 	struct panfrost_device *pfdev = priv->pfdev;

-	mutex_init(&mmu->lock);
 	INIT_LIST_HEAD(&mmu->list);
 	mmu->as = -1;

--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/8] drm/panfrost: Rework page table flushing and runtime PM interaction

[Rob Herring]

There is no point in resuming the h/w just to do flush operations and
doing so takes several locks which cause lockdep issues with the shrinker.
Rework the flush operations to only happen when the h/w is already awake.
This avoids taking any locks associated with resuming which trigger
lockdep warnings.

Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Use pm_runtime_get_noresume() and pm_runtime_active() instead of
   pm_runtime_get_if_not_used(). The problem is pm_runtime_get_if_not_used()
   returns 0 (no get) if in the period between the last put()
   and before the autosuspend timeout when the h/w is still active.

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 38 ++++++++++++-------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 3a8bcfa7e7b6..2204e60f7808 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -220,6 +220,22 @@ static size_t get_pgsize(u64 addr, size_t size)
 	return SZ_2M;
 }

+void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
+			      struct panfrost_mmu *mmu,
+			      u64 iova, size_t size)
+{
+	if (mmu->as < 0)
+		return;
+
+	pm_runtime_get_noresume(pfdev->dev);
+
+	/* Flush the PTs only if we're already awake */
+	if (pm_runtime_active(pfdev->dev))
+		mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
+
+	pm_runtime_put_sync_autosuspend(pfdev->dev);
+}
+
 static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 		      u64 iova, int prot, struct sg_table *sgt)
 {
@@ -244,8 +260,7 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 		}
 	}

-	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
-			    AS_COMMAND_FLUSH_PT);
+	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);

 	return 0;
 }
@@ -255,7 +270,6 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
 	struct drm_gem_object *obj = &bo->base.base;
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 	struct sg_table *sgt;
-	int ret;
 	int prot = IOMMU_READ | IOMMU_WRITE;

 	if (WARN_ON(bo->is_mapped))
@@ -268,14 +282,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
 	if (WARN_ON(IS_ERR(sgt)))
 		return PTR_ERR(sgt);

-	ret = pm_runtime_get_sync(pfdev->dev);
-	if (ret < 0)
-		return ret;
-
 	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
-
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
 	bo->is_mapped = true;

 	return 0;
@@ -289,17 +296,12 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 	u64 iova = bo->node.start << PAGE_SHIFT;
 	size_t len = bo->node.size << PAGE_SHIFT;
 	size_t unmapped_len = 0;
-	int ret;

 	if (WARN_ON(!bo->is_mapped))
 		return;

 	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);

-	ret = pm_runtime_get_sync(pfdev->dev);
-	if (ret < 0)
-		return;
-
 	while (unmapped_len < len) {
 		size_t unmapped_page;
 		size_t pgsize = get_pgsize(iova, len - unmapped_len);
@@ -312,11 +314,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 		unmapped_len += pgsize;
 	}

-	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
-			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);
-
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
+	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
 	bo->is_mapped = false;
 }

--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 5/8] drm/panfrost: Split mmu_hw_do_operation into locked and unlocked version

[Rob Herring]

In preparation to call mmu_hw_do_operation with the as_lock already held,
Add a mmu_hw_do_operation_locked function.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - new patch

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 26 ++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 2204e60f7808..3407b00d0a3a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -80,19 +80,11 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
 }


-static int mmu_hw_do_operation(struct panfrost_device *pfdev,
-			       struct panfrost_mmu *mmu,
-			       u64 iova, size_t size, u32 op)
+static int mmu_hw_do_operation_locked(struct panfrost_device *pfdev, int as_nr,
+				      u64 iova, size_t size, u32 op)
 {
-	int ret, as_nr;
-
-	spin_lock(&pfdev->as_lock);
-	as_nr = mmu->as;
-
-	if (as_nr < 0) {
-		spin_unlock(&pfdev->as_lock);
+	if (as_nr < 0)
 		return 0;
-	}

 	if (op != AS_COMMAND_UNLOCK)
 		lock_region(pfdev, as_nr, iova, size);
@@ -101,10 +93,18 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
 	write_cmd(pfdev, as_nr, op);

 	/* Wait for the flush to complete */
-	ret = wait_ready(pfdev, as_nr);
+	return wait_ready(pfdev, as_nr);
+}

-	spin_unlock(&pfdev->as_lock);
+static int mmu_hw_do_operation(struct panfrost_device *pfdev,
+			       struct panfrost_mmu *mmu,
+			       u64 iova, size_t size, u32 op)
+{
+	int ret;

+	spin_lock(&pfdev->as_lock);
+	ret = mmu_hw_do_operation_locked(pfdev, mmu->as, iova, size, op);
+	spin_unlock(&pfdev->as_lock);
 	return ret;
 }

--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 6/8] drm/panfrost: Add cache/TLB flush before switching address space

[Rob Herring]

It's not entirely clear if this is required, but add a flush of GPU caches
and TLBs before we change an address space to new page tables.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 ++
 1 file changed, 2 insertions(+)
v3:
 - New patch

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 3407b00d0a3a..d1ebde3327fe 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -115,6 +115,8 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
 	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
 	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;

+	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
+
 	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), transtab & 0xffffffffUL);
 	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), transtab >> 32);

--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Rob Herring]

Currently, page tables are freed without disabling the address space first.
This probably is fine as we'll switch to new page tables when the address
space is allocated again and runtime PM suspend will reset the GPU
clearing the registers. However, it's better to clean up after ourselves.
There is also a problem that we could be accessing the h/w in
tlb_inv_context() when suspended.

Rework the disable code to make sure we flush caches/TLBs and disable the
address space before freeing the page tables if we are not suspended. As
the tlb_inv_context() hook is only called when freeing the page tables and
we do a flush before disabling the AS, lets remove the flush from
tlb_inv_context and avoid any runtime PM issues.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index d1ebde3327fe..387d830cb7cf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
 }

-static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
+static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
 {
+	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
+
 	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
 	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);

@@ -321,11 +323,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 }

 static void mmu_tlb_inv_context_s1(void *cookie)
-{
-	struct panfrost_file_priv *priv = cookie;
-
-	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
-}
+{}

 static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 				     size_t granule, bool leaf, void *cookie)
@@ -374,6 +372,11 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)

 	spin_lock(&pfdev->as_lock);
 	if (mmu->as >= 0) {
+		pm_runtime_get_noresume(pfdev->dev);
+		if (pm_runtime_active(pfdev->dev))
+			panfrost_mmu_disable(pfdev, mmu->as);
+		pm_runtime_put_autosuspend(pfdev->dev);
+
 		clear_bit(mmu->as, &pfdev->as_alloc_mask);
 		clear_bit(mmu->as, &pfdev->as_in_use_mask);
 		list_del(&mmu->list);
@@ -618,5 +621,4 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 void panfrost_mmu_fini(struct panfrost_device *pfdev)
 {
 	mmu_write(pfdev, MMU_INT_MASK, 0);
-	mmu_disable(pfdev, 0);
 }
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 8/8] drm/panfrost: Remove unnecessary hwaccess_lock spin_lock

[Rob Herring]

With the introduction of the as_lock to serialize address space registers,
the hwaccess_lock is only used within the job code and is not protecting
anything. panfrost_job_hw_submit() only accesses registers for 1 job slot
and it's already serialized by drm_sched.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - New patch

 drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
 drivers/gpu/drm/panfrost/panfrost_device.h | 2 --
 drivers/gpu/drm/panfrost/panfrost_job.c    | 4 ----
 3 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 73805210834e..46b0b02e4289 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -124,7 +124,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	INIT_LIST_HEAD(&pfdev->scheduled_jobs);
 	INIT_LIST_HEAD(&pfdev->as_lru_list);

-	spin_lock_init(&pfdev->hwaccess_lock);
 	spin_lock_init(&pfdev->as_lock);

 	err = panfrost_clk_init(pfdev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index b7fa08ed3a23..9c39b9794811 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -63,8 +63,6 @@ struct panfrost_device {
 	struct drm_device *ddev;
 	struct platform_device *pdev;

-	spinlock_t hwaccess_lock;
-
 	void __iomem *iomem;
 	struct clk *clock;
 	struct clk *bus_clock;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 18bcc9bac6d2..a58551668d9a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -141,7 +141,6 @@ static void panfrost_job_write_affinity(struct panfrost_device *pfdev,
 static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 {
 	struct panfrost_device *pfdev = job->pfdev;
-	unsigned long flags;
 	u32 cfg;
 	u64 jc_head = job->jc;
 	int ret;
@@ -158,7 +157,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);

 	panfrost_devfreq_record_transition(pfdev, js);
-	spin_lock_irqsave(&pfdev->hwaccess_lock, flags);

 	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
 	job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
@@ -187,8 +185,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 				job, js, jc_head);

 	job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
-
-	spin_unlock_irqrestore(&pfdev->hwaccess_lock, flags);
 }

 static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/8] panfrost: Locking and runtime PM fixes

[Alyssa Rosenzweig]

[-- Attachment #1.1: Type: text/plain, Size: 1867 bytes --]

A-b from me; let's see what others say.

On Mon, Aug 26, 2019 at 05:33:09PM -0500, Rob Herring wrote:
> With further testing of recent changes with lockdep identified some
> locking issues. Avoiding lockdep issues means we need to avoid some
> locks in panfrost_mmu_unmap which in turn means avoiding runtime PM
> resume. In the process of re-working runtime PM several runtime PM
> and locking clean-ups have been identified.
> 
> v3:
>  - Applied patches 1, 4, 5, and 6
>  - Fix race in job timeout handling with ISR
>  - Remove some no longer needed locks
>  - Fix panfrost_mmu_unmap when autosuspend delay is > 0
>  - Disable AS MMU when freeing page tables
> 
> v2:
>  - Drop already applied 'drm/panfrost: Fix sleeping while atomic in
>    panfrost_gem_open'
>  - Runtime PM clean-ups
>  - Keep panfrost_gem_purge and use mutex_trylock there
>  - Rework panfrost_mmu_unmap runtime PM
> 
> Rob
> 
> Rob Herring (8):
>   drm/panfrost: Rework runtime PM initialization
>   drm/panfrost: Hold runtime PM reference until jobs complete
>   drm/panfrost: Remove unnecessary mmu->lock mutex
>   drm/panfrost: Rework page table flushing and runtime PM interaction
>   drm/panfrost: Split mmu_hw_do_operation into locked and unlocked
>     version
>   drm/panfrost: Add cache/TLB flush before switching address space
>   drm/panfrost: Flush and disable address space when freeing page tables
>   drm/panfrost: Remove unnecessary hwaccess_lock spin_lock
> 
>  drivers/gpu/drm/panfrost/panfrost_device.c | 10 ---
>  drivers/gpu/drm/panfrost/panfrost_device.h |  3 -
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 ++-
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 43 ++++++----
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    | 91 ++++++++++------------
>  5 files changed, 76 insertions(+), 81 deletions(-)
> 
> --
> 2.20.1

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/panfrost: Rework runtime PM initialization

[Robin Murphy]

On 26/08/2019 23:33, Rob Herring wrote:
> There's a few issues with the runtime PM initialization.
> 
> The documentation states pm_runtime_set_active() should be called before
> pm_runtime_enable(). The pm_runtime_put_autosuspend() could suspend the GPU
> before panfrost_perfcnt_init() is called which touches the h/w. The
> autosuspend delay keeps things from breaking. There's no need explicitly
> power off the GPU only to wake back up with pm_runtime_get_sync(). Just
> delaying pm_runtime_enable to the end of probe is sufficient.
> 
> Lets move all the runtime PM calls into the probe() function so they are
> all in one place and are done after all initialization.

Other the nitpick of whether the pm_runtime_enable() call should come at 
the end after the autosuspend setup,

Reviewed-by: Robin Murphy :robin.murphy@arm.com>

> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>   - Move autosuspend setup after pm_runtime_enable as only autosuspend changes
>     trigger suspend.
>   - Fix autosuspend delay to 50ms instead of 0.
> 
>   drivers/gpu/drm/panfrost/panfrost_device.c |  9 ---------
>   drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 ++++++----
>   2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 4da71bb56c20..73805210834e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -5,7 +5,6 @@
>   #include <linux/clk.h>
>   #include <linux/reset.h>
>   #include <linux/platform_device.h>
> -#include <linux/pm_runtime.h>
>   #include <linux/regulator/consumer.h>
> 
>   #include "panfrost_device.h"
> @@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>   	if (err)
>   		goto err_out4;
> 
> -	/* runtime PM will wake us up later */
> -	panfrost_gpu_power_off(pfdev);
> -
> -	pm_runtime_set_active(pfdev->dev);
> -	pm_runtime_get_sync(pfdev->dev);
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
> -
>   	err = panfrost_perfcnt_init(pfdev);
>   	if (err)
>   		goto err_out5;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index d74442d71048..bc2ddeb55f5d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev)
>   	mutex_init(&pfdev->shrinker_lock);
>   	INIT_LIST_HEAD(&pfdev->shrinker_list);
> 
> -	pm_runtime_use_autosuspend(pfdev->dev);
> -	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> -	pm_runtime_enable(pfdev->dev);
> -
>   	err = panfrost_device_init(pfdev);
>   	if (err) {
>   		if (err != -EPROBE_DEFER)
> @@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev)
>   		goto err_out1;
>   	}
> 
> +	pm_runtime_set_active(pfdev->dev);
> +	pm_runtime_mark_last_busy(pfdev->dev);
> +	pm_runtime_enable(pfdev->dev);
> +	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> +	pm_runtime_use_autosuspend(pfdev->dev);
> +
>   	/*
>   	 * Register the DRM device with the core and the connectors with
>   	 * sysfs
> --
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/8] drm/panfrost: Remove unnecessary mmu->lock mutex

[Robin Murphy]

On 26/08/2019 23:33, Rob Herring wrote:
> There's no need to serialize io-pgtable calls and the as_lock is
> sufficient to serialize flush operations, so we can remove the per
> page table lock.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>   - new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_device.h | 1 -
>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 9 ---------
>   2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index f503c566e99f..b7fa08ed3a23 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -104,7 +104,6 @@ struct panfrost_device {
>   struct panfrost_mmu {
>   	struct io_pgtable_cfg pgtbl_cfg;
>   	struct io_pgtable_ops *pgtbl_ops;
> -	struct mutex lock;
>   	int as;
>   	atomic_t as_count;
>   	struct list_head list;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 842bdd7cf6be..3a8bcfa7e7b6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -228,8 +228,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   	struct io_pgtable_ops *ops = mmu->pgtbl_ops;
>   	u64 start_iova = iova;
> 
> -	mutex_lock(&mmu->lock);
> -
>   	for_each_sg(sgt->sgl, sgl, sgt->nents, count) {
>   		unsigned long paddr = sg_dma_address(sgl);
>   		size_t len = sg_dma_len(sgl);
> @@ -249,8 +247,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
>   			    AS_COMMAND_FLUSH_PT);
> 
> -	mutex_unlock(&mmu->lock);
> -
>   	return 0;
>   }
> 
> @@ -304,8 +300,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   	if (ret < 0)
>   		return;
> 
> -	mutex_lock(&bo->mmu->lock);
> -
>   	while (unmapped_len < len) {
>   		size_t unmapped_page;
>   		size_t pgsize = get_pgsize(iova, len - unmapped_len);
> @@ -321,8 +315,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
>   			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);
> 
> -	mutex_unlock(&bo->mmu->lock);
> -
>   	pm_runtime_mark_last_busy(pfdev->dev);
>   	pm_runtime_put_autosuspend(pfdev->dev);
>   	bo->is_mapped = false;
> @@ -356,7 +348,6 @@ int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv)
>   	struct panfrost_mmu *mmu = &priv->mmu;
>   	struct panfrost_device *pfdev = priv->pfdev;
> 
> -	mutex_init(&mmu->lock);
>   	INIT_LIST_HEAD(&mmu->list);
>   	mmu->as = -1;
> 
> --
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/8] drm/panfrost: Rework page table flushing and runtime PM interaction

[Robin Murphy]

On 26/08/2019 23:33, Rob Herring wrote:
> There is no point in resuming the h/w just to do flush operations and
> doing so takes several locks which cause lockdep issues with the shrinker.
> Rework the flush operations to only happen when the h/w is already awake.
> This avoids taking any locks associated with resuming which trigger
> lockdep warnings.

Dunno if it's worth explicitly commenting that this is because resume 
already implies a full flush, but regardless,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>   - Use pm_runtime_get_noresume() and pm_runtime_active() instead of
>     pm_runtime_get_if_not_used(). The problem is pm_runtime_get_if_not_used()
>     returns 0 (no get) if in the period between the last put()
>     and before the autosuspend timeout when the h/w is still active.
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 38 ++++++++++++-------------
>   1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3a8bcfa7e7b6..2204e60f7808 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -220,6 +220,22 @@ static size_t get_pgsize(u64 addr, size_t size)
>   	return SZ_2M;
>   }
> 
> +void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
> +			      struct panfrost_mmu *mmu,
> +			      u64 iova, size_t size)
> +{
> +	if (mmu->as < 0)
> +		return;
> +
> +	pm_runtime_get_noresume(pfdev->dev);
> +
> +	/* Flush the PTs only if we're already awake */
> +	if (pm_runtime_active(pfdev->dev))
> +		mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
> +
> +	pm_runtime_put_sync_autosuspend(pfdev->dev);
> +}
> +
>   static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   		      u64 iova, int prot, struct sg_table *sgt)
>   {
> @@ -244,8 +260,7 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   		}
>   	}
> 
> -	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
> -			    AS_COMMAND_FLUSH_PT);
> +	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);
> 
>   	return 0;
>   }
> @@ -255,7 +270,6 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>   	struct drm_gem_object *obj = &bo->base.base;
>   	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>   	struct sg_table *sgt;
> -	int ret;
>   	int prot = IOMMU_READ | IOMMU_WRITE;
> 
>   	if (WARN_ON(bo->is_mapped))
> @@ -268,14 +282,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>   	if (WARN_ON(IS_ERR(sgt)))
>   		return PTR_ERR(sgt);
> 
> -	ret = pm_runtime_get_sync(pfdev->dev);
> -	if (ret < 0)
> -		return ret;
> -
>   	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
> -
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
>   	bo->is_mapped = true;
> 
>   	return 0;
> @@ -289,17 +296,12 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   	u64 iova = bo->node.start << PAGE_SHIFT;
>   	size_t len = bo->node.size << PAGE_SHIFT;
>   	size_t unmapped_len = 0;
> -	int ret;
> 
>   	if (WARN_ON(!bo->is_mapped))
>   		return;
> 
>   	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);
> 
> -	ret = pm_runtime_get_sync(pfdev->dev);
> -	if (ret < 0)
> -		return;
> -
>   	while (unmapped_len < len) {
>   		size_t unmapped_page;
>   		size_t pgsize = get_pgsize(iova, len - unmapped_len);
> @@ -312,11 +314,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   		unmapped_len += pgsize;
>   	}
> 
> -	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
> -			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);
> -
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
> +	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
>   	bo->is_mapped = false;
>   }
> 
> --
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Robin Murphy]

On 26/08/2019 23:33, Rob Herring wrote:
> Currently, page tables are freed without disabling the address space first.
> This probably is fine as we'll switch to new page tables when the address
> space is allocated again and runtime PM suspend will reset the GPU
> clearing the registers. However, it's better to clean up after ourselves.
> There is also a problem that we could be accessing the h/w in
> tlb_inv_context() when suspended.
> 
> Rework the disable code to make sure we flush caches/TLBs and disable the
> address space before freeing the page tables if we are not suspended. As
> the tlb_inv_context() hook is only called when freeing the page tables and
> we do a flush before disabling the AS, lets remove the flush from
> tlb_inv_context and avoid any runtime PM issues.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>   - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d1ebde3327fe..387d830cb7cf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>   	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>   }
> 
> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>   {
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>   	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>   	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> 
> @@ -321,11 +323,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   }
> 
>   static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
> 
>   static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>   				     size_t granule, bool leaf, void *cookie)
> @@ -374,6 +372,11 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
> 
>   	spin_lock(&pfdev->as_lock);
>   	if (mmu->as >= 0) {
> +		pm_runtime_get_noresume(pfdev->dev);
> +		if (pm_runtime_active(pfdev->dev))
> +			panfrost_mmu_disable(pfdev, mmu->as);
> +		pm_runtime_put_autosuspend(pfdev->dev);
> +
>   		clear_bit(mmu->as, &pfdev->as_alloc_mask);
>   		clear_bit(mmu->as, &pfdev->as_in_use_mask);
>   		list_del(&mmu->list);
> @@ -618,5 +621,4 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>   void panfrost_mmu_fini(struct panfrost_device *pfdev)
>   {
>   	mmu_write(pfdev, MMU_INT_MASK, 0);
> -	mmu_disable(pfdev, 0);
>   }
> --
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/8] drm/panfrost: Add cache/TLB flush before switching address space

[Robin Murphy]

On 26/08/2019 23:33, Rob Herring wrote:
> It's not entirely clear if this is required, but add a flush of GPU caches
> and TLBs before we change an address space to new page tables.

This might work out to be partially redundant with some of the revamped 
flushing in #7, but unless it proves to be a performance bottleneck, 
having both does make things seem more robust and easier to reason about.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 ++
>   1 file changed, 2 insertions(+)
> v3:
>   - New patch
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3407b00d0a3a..d1ebde3327fe 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -115,6 +115,8 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>   	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
>   	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
> 
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>   	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), transtab & 0xffffffffUL);
>   	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), transtab >> 32);
> 
> --
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/panfrost: Rework runtime PM initialization

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> There's a few issues with the runtime PM initialization.
> 
> The documentation states pm_runtime_set_active() should be called before
> pm_runtime_enable(). The pm_runtime_put_autosuspend() could suspend the GPU
> before panfrost_perfcnt_init() is called which touches the h/w. The
> autosuspend delay keeps things from breaking. There's no need explicitly
> power off the GPU only to wake back up with pm_runtime_get_sync(). Just
> delaying pm_runtime_enable to the end of probe is sufficient.
> 
> Lets move all the runtime PM calls into the probe() function so they are
> all in one place and are done after all initialization.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - Move autosuspend setup after pm_runtime_enable as only autosuspend changes
>    trigger suspend.
>  - Fix autosuspend delay to 50ms instead of 0.
> 
>  drivers/gpu/drm/panfrost/panfrost_device.c |  9 ---------
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 ++++++----
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 4da71bb56c20..73805210834e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -5,7 +5,6 @@
>  #include <linux/clk.h>
>  #include <linux/reset.h>
>  #include <linux/platform_device.h>
> -#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> 
>  #include "panfrost_device.h"
> @@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	if (err)
>  		goto err_out4;
> 
> -	/* runtime PM will wake us up later */
> -	panfrost_gpu_power_off(pfdev);
> -
> -	pm_runtime_set_active(pfdev->dev);
> -	pm_runtime_get_sync(pfdev->dev);
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
> -
>  	err = panfrost_perfcnt_init(pfdev);
>  	if (err)
>  		goto err_out5;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index d74442d71048..bc2ddeb55f5d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev)
>  	mutex_init(&pfdev->shrinker_lock);
>  	INIT_LIST_HEAD(&pfdev->shrinker_list);
> 
> -	pm_runtime_use_autosuspend(pfdev->dev);
> -	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> -	pm_runtime_enable(pfdev->dev);
> -
>  	err = panfrost_device_init(pfdev);
>  	if (err) {
>  		if (err != -EPROBE_DEFER)
> @@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev)
>  		goto err_out1;
>  	}
> 
> +	pm_runtime_set_active(pfdev->dev);
> +	pm_runtime_mark_last_busy(pfdev->dev);
> +	pm_runtime_enable(pfdev->dev);
> +	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> +	pm_runtime_use_autosuspend(pfdev->dev);
> +
>  	/*
>  	 * Register the DRM device with the core and the connectors with
>  	 * sysfs
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/8] drm/panfrost: Hold runtime PM reference until jobs complete

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> Doing a pm_runtime_put as soon as a job is submitted is wrong as it should
> not happen until the job completes. It works currently because we are
> relying on the autosuspend timeout to keep the h/w enabled.
> 
> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - Fix race between clearing pfdev->jobs[] in timeout and the ISR using the job_lock
>  - Maintain pm_runtime_put in the panfrost_job_hw_submit error path
> 
>  drivers/gpu/drm/panfrost/panfrost_job.c | 39 ++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 05c85f45a0de..18bcc9bac6d2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -150,8 +150,10 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  	if (ret < 0)
>  		return;
> 
> -	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
> -		goto end;
> +	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) {
> +		pm_runtime_put_sync_autosuspend(pfdev->dev);
> +		return;
> +	}
> 
>  	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
> 
> @@ -187,10 +189,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  	job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
> 
>  	spin_unlock_irqrestore(&pfdev->hwaccess_lock, flags);
> -
> -end:
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
>  }
> 
>  static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
> @@ -369,6 +367,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
>  	struct panfrost_job *job = to_panfrost_job(sched_job);
>  	struct panfrost_device *pfdev = job->pfdev;
>  	int js = panfrost_job_get_slot(job);
> +	unsigned long flags;
>  	int i;
> 
>  	/*
> @@ -394,6 +393,15 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
>  	if (sched_job)
>  		drm_sched_increase_karma(sched_job);
> 
> +	spin_lock_irqsave(&pfdev->js->job_lock, flags);
> +	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> +		if (pfdev->jobs[i]) {
> +			pm_runtime_put_noidle(pfdev->dev);
> +			pfdev->jobs[i] = NULL;
> +		}
> +	}
> +	spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
> +
>  	/* panfrost_core_dump(pfdev); */
> 
>  	panfrost_devfreq_record_transition(pfdev, js);
> @@ -450,12 +458,21 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>  		}
> 
>  		if (status & JOB_INT_MASK_DONE(j)) {
> -			struct panfrost_job *job = pfdev->jobs[j];
> +			struct panfrost_job *job;
> +
> +			spin_lock(&pfdev->js->job_lock);
> +			job = pfdev->jobs[j];
> +			/* Only NULL if job timeout occurred */
> +			if (job) {
> +				pfdev->jobs[j] = NULL;
> +
> +				panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
> +				panfrost_devfreq_record_transition(pfdev, j);
> 
> -			pfdev->jobs[j] = NULL;
> -			panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
> -			panfrost_devfreq_record_transition(pfdev, j);
> -			dma_fence_signal(job->done_fence);
> +				dma_fence_signal_locked(job->done_fence);
> +				pm_runtime_put_autosuspend(pfdev->dev);
> +			}
> +			spin_unlock(&pfdev->js->job_lock);
>  		}
> 
>  		status &= ~mask;
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/8] drm/panfrost: Remove unnecessary mmu->lock mutex

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> There's no need to serialize io-pgtable calls and the as_lock is
> sufficient to serialize flush operations, so we can remove the per
> page table lock.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - new patch
> 
>  drivers/gpu/drm/panfrost/panfrost_device.h | 1 -
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    | 9 ---------
>  2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index f503c566e99f..b7fa08ed3a23 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -104,7 +104,6 @@ struct panfrost_device {
>  struct panfrost_mmu {
>  	struct io_pgtable_cfg pgtbl_cfg;
>  	struct io_pgtable_ops *pgtbl_ops;
> -	struct mutex lock;
>  	int as;
>  	atomic_t as_count;
>  	struct list_head list;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 842bdd7cf6be..3a8bcfa7e7b6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -228,8 +228,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  	struct io_pgtable_ops *ops = mmu->pgtbl_ops;
>  	u64 start_iova = iova;
> 
> -	mutex_lock(&mmu->lock);
> -
>  	for_each_sg(sgt->sgl, sgl, sgt->nents, count) {
>  		unsigned long paddr = sg_dma_address(sgl);
>  		size_t len = sg_dma_len(sgl);
> @@ -249,8 +247,6 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
>  			    AS_COMMAND_FLUSH_PT);
> 
> -	mutex_unlock(&mmu->lock);
> -
>  	return 0;
>  }
> 
> @@ -304,8 +300,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  	if (ret < 0)
>  		return;
> 
> -	mutex_lock(&bo->mmu->lock);
> -
>  	while (unmapped_len < len) {
>  		size_t unmapped_page;
>  		size_t pgsize = get_pgsize(iova, len - unmapped_len);
> @@ -321,8 +315,6 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
>  			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);
> 
> -	mutex_unlock(&bo->mmu->lock);
> -
>  	pm_runtime_mark_last_busy(pfdev->dev);
>  	pm_runtime_put_autosuspend(pfdev->dev);
>  	bo->is_mapped = false;
> @@ -356,7 +348,6 @@ int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv)
>  	struct panfrost_mmu *mmu = &priv->mmu;
>  	struct panfrost_device *pfdev = priv->pfdev;
> 
> -	mutex_init(&mmu->lock);
>  	INIT_LIST_HEAD(&mmu->list);
>  	mmu->as = -1;
> 
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/8] drm/panfrost: Rework page table flushing and runtime PM interaction

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> There is no point in resuming the h/w just to do flush operations and
> doing so takes several locks which cause lockdep issues with the shrinker.
> Rework the flush operations to only happen when the h/w is already awake.
> This avoids taking any locks associated with resuming which trigger
> lockdep warnings.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - Use pm_runtime_get_noresume() and pm_runtime_active() instead of
>    pm_runtime_get_if_not_used(). The problem is pm_runtime_get_if_not_used()
>    returns 0 (no get) if in the period between the last put()
>    and before the autosuspend timeout when the h/w is still active.
> 
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 38 ++++++++++++-------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3a8bcfa7e7b6..2204e60f7808 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -220,6 +220,22 @@ static size_t get_pgsize(u64 addr, size_t size)
>  	return SZ_2M;
>  }
> 
> +void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
> +			      struct panfrost_mmu *mmu,
> +			      u64 iova, size_t size)
> +{
> +	if (mmu->as < 0)
> +		return;
> +
> +	pm_runtime_get_noresume(pfdev->dev);
> +
> +	/* Flush the PTs only if we're already awake */
> +	if (pm_runtime_active(pfdev->dev))
> +		mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
> +
> +	pm_runtime_put_sync_autosuspend(pfdev->dev);
> +}
> +
>  static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  		      u64 iova, int prot, struct sg_table *sgt)
>  {
> @@ -244,8 +260,7 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  		}
>  	}
> 
> -	mmu_hw_do_operation(pfdev, mmu, start_iova, iova - start_iova,
> -			    AS_COMMAND_FLUSH_PT);
> +	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);
> 
>  	return 0;
>  }
> @@ -255,7 +270,6 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>  	struct drm_gem_object *obj = &bo->base.base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>  	struct sg_table *sgt;
> -	int ret;
>  	int prot = IOMMU_READ | IOMMU_WRITE;
> 
>  	if (WARN_ON(bo->is_mapped))
> @@ -268,14 +282,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>  	if (WARN_ON(IS_ERR(sgt)))
>  		return PTR_ERR(sgt);
> 
> -	ret = pm_runtime_get_sync(pfdev->dev);
> -	if (ret < 0)
> -		return ret;
> -
>  	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
> -
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
>  	bo->is_mapped = true;
> 
>  	return 0;
> @@ -289,17 +296,12 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  	u64 iova = bo->node.start << PAGE_SHIFT;
>  	size_t len = bo->node.size << PAGE_SHIFT;
>  	size_t unmapped_len = 0;
> -	int ret;
> 
>  	if (WARN_ON(!bo->is_mapped))
>  		return;
> 
>  	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);
> 
> -	ret = pm_runtime_get_sync(pfdev->dev);
> -	if (ret < 0)
> -		return;
> -
>  	while (unmapped_len < len) {
>  		size_t unmapped_page;
>  		size_t pgsize = get_pgsize(iova, len - unmapped_len);
> @@ -312,11 +314,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  		unmapped_len += pgsize;
>  	}
> 
> -	mmu_hw_do_operation(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT,
> -			    bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);
> -
> -	pm_runtime_mark_last_busy(pfdev->dev);
> -	pm_runtime_put_autosuspend(pfdev->dev);
> +	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
>  	bo->is_mapped = false;
>  }
> 
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 5/8] drm/panfrost: Split mmu_hw_do_operation into locked and unlocked version

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> In preparation to call mmu_hw_do_operation with the as_lock already held,
> Add a mmu_hw_do_operation_locked function.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - new patch
> 
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 26 ++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 2204e60f7808..3407b00d0a3a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -80,19 +80,11 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
>  }
> 
> 
> -static int mmu_hw_do_operation(struct panfrost_device *pfdev,
> -			       struct panfrost_mmu *mmu,
> -			       u64 iova, size_t size, u32 op)
> +static int mmu_hw_do_operation_locked(struct panfrost_device *pfdev, int as_nr,
> +				      u64 iova, size_t size, u32 op)
>  {
> -	int ret, as_nr;
> -
> -	spin_lock(&pfdev->as_lock);
> -	as_nr = mmu->as;
> -
> -	if (as_nr < 0) {
> -		spin_unlock(&pfdev->as_lock);
> +	if (as_nr < 0)
>  		return 0;
> -	}
> 
>  	if (op != AS_COMMAND_UNLOCK)
>  		lock_region(pfdev, as_nr, iova, size);
> @@ -101,10 +93,18 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
>  	write_cmd(pfdev, as_nr, op);
> 
>  	/* Wait for the flush to complete */
> -	ret = wait_ready(pfdev, as_nr);
> +	return wait_ready(pfdev, as_nr);
> +}
> 
> -	spin_unlock(&pfdev->as_lock);
> +static int mmu_hw_do_operation(struct panfrost_device *pfdev,
> +			       struct panfrost_mmu *mmu,
> +			       u64 iova, size_t size, u32 op)
> +{
> +	int ret;
> 
> +	spin_lock(&pfdev->as_lock);
> +	ret = mmu_hw_do_operation_locked(pfdev, mmu->as, iova, size, op);
> +	spin_unlock(&pfdev->as_lock);
>  	return ret;
>  }
> 
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> Currently, page tables are freed without disabling the address space first.
> This probably is fine as we'll switch to new page tables when the address
> space is allocated again and runtime PM suspend will reset the GPU
> clearing the registers. However, it's better to clean up after ourselves.
> There is also a problem that we could be accessing the h/w in
> tlb_inv_context() when suspended.
> 
> Rework the disable code to make sure we flush caches/TLBs and disable the
> address space before freeing the page tables if we are not suspended. As
> the tlb_inv_context() hook is only called when freeing the page tables and
> we do a flush before disabling the AS, lets remove the flush from
> tlb_inv_context and avoid any runtime PM issues.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> 
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d1ebde3327fe..387d830cb7cf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>  	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>  }
> 
> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>  {
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>  	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>  	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> 

At the end of this function we have:

| 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);

which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
one thing I have just noticed is that write_cmd() doesn't wait for
AS_ACTIVE to be cleared. This means that the GPU has accepted the
command but might not have finished the flush.

When freeing page tables we obviously need to wait for the MMU flush to
complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
because there's a back-to-back set of MMU commands so the second one
will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
mmu_hw_do_operation() waits for the flush to complete.

I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
write_cmd - could they use mmu_hw_do_operation_locked() instead?

> @@ -321,11 +323,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  }
> 
>  static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
> 
>  static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>  				     size_t granule, bool leaf, void *cookie)
> @@ -374,6 +372,11 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
> 
>  	spin_lock(&pfdev->as_lock);
>  	if (mmu->as >= 0) {
> +		pm_runtime_get_noresume(pfdev->dev);
> +		if (pm_runtime_active(pfdev->dev))
> +			panfrost_mmu_disable(pfdev, mmu->as);
> +		pm_runtime_put_autosuspend(pfdev->dev);
> +
>  		clear_bit(mmu->as, &pfdev->as_alloc_mask);
>  		clear_bit(mmu->as, &pfdev->as_in_use_mask);
>  		list_del(&mmu->list);
> @@ -618,5 +621,4 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  void panfrost_mmu_fini(struct panfrost_device *pfdev)
>  {
>  	mmu_write(pfdev, MMU_INT_MASK, 0);
> -	mmu_disable(pfdev, 0);
>  }
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 8/8] drm/panfrost: Remove unnecessary hwaccess_lock spin_lock

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> With the introduction of the as_lock to serialize address space registers,
> the hwaccess_lock is only used within the job code and is not protecting
> anything. panfrost_job_hw_submit() only accesses registers for 1 job slot
> and it's already serialized by drm_sched.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> v3:
>  - New patch
> 
>  drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
>  drivers/gpu/drm/panfrost/panfrost_device.h | 2 --
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 4 ----
>  3 files changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 73805210834e..46b0b02e4289 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -124,7 +124,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	INIT_LIST_HEAD(&pfdev->scheduled_jobs);
>  	INIT_LIST_HEAD(&pfdev->as_lru_list);
> 
> -	spin_lock_init(&pfdev->hwaccess_lock);
>  	spin_lock_init(&pfdev->as_lock);
> 
>  	err = panfrost_clk_init(pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index b7fa08ed3a23..9c39b9794811 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -63,8 +63,6 @@ struct panfrost_device {
>  	struct drm_device *ddev;
>  	struct platform_device *pdev;
> 
> -	spinlock_t hwaccess_lock;
> -
>  	void __iomem *iomem;
>  	struct clk *clock;
>  	struct clk *bus_clock;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 18bcc9bac6d2..a58551668d9a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -141,7 +141,6 @@ static void panfrost_job_write_affinity(struct panfrost_device *pfdev,
>  static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  {
>  	struct panfrost_device *pfdev = job->pfdev;
> -	unsigned long flags;
>  	u32 cfg;
>  	u64 jc_head = job->jc;
>  	int ret;
> @@ -158,7 +157,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
> 
>  	panfrost_devfreq_record_transition(pfdev, js);
> -	spin_lock_irqsave(&pfdev->hwaccess_lock, flags);
> 
>  	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
>  	job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
> @@ -187,8 +185,6 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  				job, js, jc_head);
> 
>  	job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
> -
> -	spin_unlock_irqrestore(&pfdev->hwaccess_lock, flags);
>  }
> 
>  static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Rob Herring]

On Wed, Aug 28, 2019 at 5:55 AM Steven Price <steven.price@arm.com> wrote:
>
> On 26/08/2019 23:33, Rob Herring wrote:
> > Currently, page tables are freed without disabling the address space first.
> > This probably is fine as we'll switch to new page tables when the address
> > space is allocated again and runtime PM suspend will reset the GPU
> > clearing the registers. However, it's better to clean up after ourselves.
> > There is also a problem that we could be accessing the h/w in
> > tlb_inv_context() when suspended.
> >
> > Rework the disable code to make sure we flush caches/TLBs and disable the
> > address space before freeing the page tables if we are not suspended. As
> > the tlb_inv_context() hook is only called when freeing the page tables and
> > we do a flush before disabling the AS, lets remove the flush from
> > tlb_inv_context and avoid any runtime PM issues.
> >
> > Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Steven Price <steven.price@arm.com>
> > Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v3:
> >  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> >
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index d1ebde3327fe..387d830cb7cf 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
> >       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
> >  }
> >
> > -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> > +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> >  {
> > +     mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> > +
> >       mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
> >       mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> >
>
> At the end of this function we have:
>
> |       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>
> which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
> one thing I have just noticed is that write_cmd() doesn't wait for
> AS_ACTIVE to be cleared. This means that the GPU has accepted the
> command but might not have finished the flush.
>
> When freeing page tables we obviously need to wait for the MMU flush to
> complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
> because there's a back-to-back set of MMU commands so the second one
> will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
> mmu_hw_do_operation() waits for the flush to complete.

I've copied what's in kbase which doesn't wait AFAICT.

> I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
> write_cmd - could they use mmu_hw_do_operation_locked() instead?

mmu_hw_do_operation_locked() also does a lock_region. I guess that
would be harmless?

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 7/8] drm/panfrost: Flush and disable address space when freeing page tables

[Steven Price]

On 28/08/2019 13:35, Rob Herring wrote:
> On Wed, Aug 28, 2019 at 5:55 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 26/08/2019 23:33, Rob Herring wrote:
>>> Currently, page tables are freed without disabling the address space first.
>>> This probably is fine as we'll switch to new page tables when the address
>>> space is allocated again and runtime PM suspend will reset the GPU
>>> clearing the registers. However, it's better to clean up after ourselves.
>>> There is also a problem that we could be accessing the h/w in
>>> tlb_inv_context() when suspended.
>>>
>>> Rework the disable code to make sure we flush caches/TLBs and disable the
>>> address space before freeing the page tables if we are not suspended. As
>>> the tlb_inv_context() hook is only called when freeing the page tables and
>>> we do a flush before disabling the AS, lets remove the flush from
>>> tlb_inv_context and avoid any runtime PM issues.
>>>
>>> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
>>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> Cc: Steven Price <steven.price@arm.com>
>>> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> v3:
>>>  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
>>>
>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index d1ebde3327fe..387d830cb7cf 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>>>       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>>  }
>>>
>>> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>>  {
>>> +     mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
>>> +
>>>       mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>>>       mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
>>>
>>
>> At the end of this function we have:
>>
>> |       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>
>> which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
>> one thing I have just noticed is that write_cmd() doesn't wait for
>> AS_ACTIVE to be cleared. This means that the GPU has accepted the
>> command but might not have finished the flush.
>>
>> When freeing page tables we obviously need to wait for the MMU flush to
>> complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
>> because there's a back-to-back set of MMU commands so the second one
>> will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
>> mmu_hw_do_operation() waits for the flush to complete.
> 
> I've copied what's in kbase which doesn't wait AFAICT.

Hmm, well it does in that it calls kbase_mmu_flush_invalidate_noretain()
before disable_as() [1]. But that is equivalent to what you have above.

Oh well, I guess matching kbase is safer here, even if kbase seems to be
doing extra work.

[1]
https://gitlab.freedesktop.org/panfrost/mali_kbase/blob/master/driver/product/kernel/drivers/gpu/arm/midgard/mali_kbase_mmu.c#L1568

>> I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
>> write_cmd - could they use mmu_hw_do_operation_locked() instead?
> 
> mmu_hw_do_operation_locked() also does a lock_region. I guess that
> would be harmless?

Yes, locking regions should be harmless - it just prevents the MMU from
translating the locked region. However you do need to ensure that the
region is unlocked again and UPDATE isn't specified to perform the UNLOCK.

So actually all things considered your patch looks fine - it matches
kbase (even if I don't fully understand why kbase is doing it that way),
and fixes a bug. So:

Reviewed-by: Steven Price <steven.price@arm.com>

(and sorry for the noise!)

Steve
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/8] drm/panfrost: Add cache/TLB flush before switching address space

[Steven Price]

On 26/08/2019 23:33, Rob Herring wrote:
> It's not entirely clear if this is required, but add a flush of GPU caches
> and TLBs before we change an address space to new page tables.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Equally I'm not sure this is required, but it looks like there might be
a path where a AS is reclaimed and then immediately freed (client closes
fd) which would need a flush (with wait) to ensure that translations had
definitely finished. And it won't do any harm, so:

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 ++
>  1 file changed, 2 insertions(+)
> v3:
>  - New patch
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3407b00d0a3a..d1ebde3327fe 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -115,6 +115,8 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>  	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
>  	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
> 
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>  	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), transtab & 0xffffffffUL);
>  	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), transtab >> 32);
> 
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel