(unknown)

(unknown)

[Rob Herring]

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

With further testing of recent changes with lockdep and other locking
checks enabled, we've found several bugs in the shrinker code and one
sleep while atomic in panfrost_gem_open(). This series addresses those
issues.

Delaying the unmapping of pages turns out to be a bad idea. Instead we 
need to rework panfrost_mmu_unmap() to not do a runtime PM resume which 
takes several locks and causes more lockdep warnings. Unfortunately, 
there initially appeared to be some mismatches between the runtime PM 
state and the h/w. The result is several fixes to the runtime PM 
initialization and handling in jobs. With this, the changes to 
panfrost_mmu_unmap() are working correctly.

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: Fix possible suspend in panfrost_remove
  drm/panfrost: Rework runtime PM initialization
  drm/panfrost: Hold runtime PM reference until jobs complete
  drm/shmem: Do dma_unmap_sg before purging pages
  drm/shmem: Use mutex_trylock in drm_gem_shmem_purge
  drm/panfrost: Use mutex_trylock in panfrost_gem_purge
  drm/panfrost: Rework page table flushing and runtime PM interaction
  drm/panfrost: Remove unnecessary flushing from tlb_inv_context

 drivers/gpu/drm/drm_gem_shmem_helper.c        | 13 ++++-
 drivers/gpu/drm/panfrost/panfrost_device.c    |  9 ----
 drivers/gpu/drm/panfrost/panfrost_drv.c       | 16 ++++---
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  | 11 +++--
 drivers/gpu/drm/panfrost/panfrost_job.c       | 16 ++++---
 drivers/gpu/drm/panfrost/panfrost_mmu.c       | 47 +++++++++----------
 include/drm/drm_gem_shmem_helper.h            |  2 +-
 7 files changed, 59 insertions(+), 55 deletions(-)

-- 
2.20.1

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

[PATCH v2 1/8] drm/panfrost: Fix possible suspend in panfrost_remove

[Rob Herring]

Calls to panfrost_device_fini() access the h/w, but we already done a
pm_runtime_put_sync_autosuspend() beforehand. This only works if the
autosuspend delay is long enough. A 0ms delay will hang the system when
removing the device. Fix this by moving the pm_runtime_put_sync_suspend()
after the panfrost_device_fini() call.

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>
---
v2: new patch

 drivers/gpu/drm/panfrost/panfrost_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 44a558c6e17e..d74442d71048 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -570,11 +570,13 @@ static int panfrost_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(ddev);
 	panfrost_gem_shrinker_cleanup(ddev);
+
 	pm_runtime_get_sync(pfdev->dev);
-	pm_runtime_put_sync_autosuspend(pfdev->dev);
-	pm_runtime_disable(pfdev->dev);
 	panfrost_devfreq_fini(pfdev);
 	panfrost_device_fini(pfdev);
+	pm_runtime_put_sync_suspend(pfdev->dev);
+	pm_runtime_disable(pfdev->dev);
+
 	drm_dev_put(ddev);
 	return 0;
 }
-- 
2.20.1

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

Re: [PATCH v2 1/8] drm/panfrost: Fix possible suspend in panfrost_remove

[Steven Price]

On 23/08/2019 03:12, Rob Herring wrote:
> Calls to panfrost_device_fini() access the h/w, but we already done a
> pm_runtime_put_sync_autosuspend() beforehand. This only works if the
> autosuspend delay is long enough. A 0ms delay will hang the system when
> removing the device. Fix this by moving the pm_runtime_put_sync_suspend()
> after the panfrost_device_fini() call.
> 
> 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>

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_drv.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 44a558c6e17e..d74442d71048 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -570,11 +570,13 @@ static int panfrost_remove(struct platform_device *pdev)
>   
>   	drm_dev_unregister(ddev);
>   	panfrost_gem_shrinker_cleanup(ddev);
> +
>   	pm_runtime_get_sync(pfdev->dev);
> -	pm_runtime_put_sync_autosuspend(pfdev->dev);
> -	pm_runtime_disable(pfdev->dev);
>   	panfrost_devfreq_fini(pfdev);
>   	panfrost_device_fini(pfdev);
> +	pm_runtime_put_sync_suspend(pfdev->dev);
> +	pm_runtime_disable(pfdev->dev);
> +
>   	drm_dev_put(ddev);
>   	return 0;
>   }
> 

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

[PATCH v2 2/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: 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>
---
v2: new patch

 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..f27e3d6aec12 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_use_autosuspend(pfdev->dev);
+	pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */
+	pm_runtime_mark_last_busy(pfdev->dev);
+	pm_runtime_enable(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 v2 2/8] drm/panfrost: Rework runtime PM initialization

[Robin Murphy]

On 23/08/2019 03:12, 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: 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>
> ---
> v2: new patch
> 
>   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..f27e3d6aec12 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_use_autosuspend(pfdev->dev);
> +	pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */

Hmm... different timeout, same comment - something seems amiss there, 
unless perhaps it's all within rounding error anyway?

Other than that, the overall change looks sensible to me.

Robin.

> +	pm_runtime_mark_last_busy(pfdev->dev);
> +	pm_runtime_enable(pfdev->dev);
> +
>   	/*
>   	 * Register the DRM device with the core and the connectors with
>   	 * sysfs
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/panfrost: Rework runtime PM initialization

[Rob Herring]

On Fri, Aug 23, 2019 at 5:54 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 23/08/2019 03:12, 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: 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>
> > ---
> > v2: new patch
> >
> >   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..f27e3d6aec12 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_use_autosuspend(pfdev->dev);
> > +     pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */
>
> Hmm... different timeout, same comment - something seems amiss there,
> unless perhaps it's all within rounding error anyway?

Leftover debugging to force issues. It should be 50.

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

[PATCH v2 3/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 because we are relying on the
autosuspend timeout to keep the h/w enabled.

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>
---
v2: new patch

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 05c85f45a0de..80c9cab9a01b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -151,7 +151,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 		return;
 
 	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
-		goto end;
+		return;
 
 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
 
@@ -187,10 +187,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,
@@ -388,9 +384,13 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
 
 	mutex_lock(&pfdev->reset_lock);
 
-	for (i = 0; i < NUM_JOB_SLOTS; i++)
+	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		drm_sched_stop(&pfdev->js->queue[i].sched, sched_job);
-
+		if (pfdev->jobs[i]) {
+			pm_runtime_put_noidle(pfdev->dev);
+			pfdev->jobs[i] = NULL;
+		}
+	}
 	if (sched_job)
 		drm_sched_increase_karma(sched_job);
 
@@ -455,7 +455,9 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
 			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);
+			pm_runtime_put_autosuspend(pfdev->dev);
 		}
 
 		status &= ~mask;
-- 
2.20.1

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

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

[Steven Price]

On 23/08/2019 03:12, 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 because we are relying on the
> autosuspend timeout to keep the h/w enabled.
> 
> 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>

Nice find, but I'm a bit worried about one thing.

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_job.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 05c85f45a0de..80c9cab9a01b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -151,7 +151,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>   		return;
>   
>   	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
> -		goto end;
> +		return;
>   
>   	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
>   
> @@ -187,10 +187,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,
> @@ -388,9 +384,13 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
>   
>   	mutex_lock(&pfdev->reset_lock);
>   
> -	for (i = 0; i < NUM_JOB_SLOTS; i++)
> +	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>   		drm_sched_stop(&pfdev->js->queue[i].sched, sched_job);
> -
> +		if (pfdev->jobs[i]) {
> +			pm_runtime_put_noidle(pfdev->dev);
> +			pfdev->jobs[i] = NULL;

I can't see what prevents this racing with panfrost_job_irq_handler() - 
the job could be completing at the same time as we assign NULL. Then 
panfrost_job_irq_handler() will happily dereference the NULL pointer...

Admittedly this patch is an improvement over the situation before :)

Steve

> +		}
> +	}
>   	if (sched_job)
>   		drm_sched_increase_karma(sched_job);
>   
> @@ -455,7 +455,9 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>   			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);
> +			pm_runtime_put_autosuspend(pfdev->dev);
>   		}
>   
>   		status &= ~mask;
> 

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

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

[Rob Herring]

On Fri, Aug 23, 2019 at 9:50 AM Steven Price <steven.price@arm.com> wrote:
>
> On 23/08/2019 03:12, 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 because we are relying on the
> > autosuspend timeout to keep the h/w enabled.
> >
> > 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>
>
> Nice find, but I'm a bit worried about one thing.
>
> > ---
> > v2: new patch
> >
> >   drivers/gpu/drm/panfrost/panfrost_job.c | 16 +++++++++-------
> >   1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index 05c85f45a0de..80c9cab9a01b 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -151,7 +151,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> >               return;
> >
> >       if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
> > -             goto end;
> > +             return;
> >
> >       cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
> >
> > @@ -187,10 +187,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,
> > @@ -388,9 +384,13 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
> >
> >       mutex_lock(&pfdev->reset_lock);
> >
> > -     for (i = 0; i < NUM_JOB_SLOTS; i++)
> > +     for (i = 0; i < NUM_JOB_SLOTS; i++) {
> >               drm_sched_stop(&pfdev->js->queue[i].sched, sched_job);
> > -
> > +             if (pfdev->jobs[i]) {
> > +                     pm_runtime_put_noidle(pfdev->dev);
> > +                     pfdev->jobs[i] = NULL;
>
> I can't see what prevents this racing with panfrost_job_irq_handler() -
> the job could be completing at the same time as we assign NULL. Then
> panfrost_job_irq_handler() will happily dereference the NULL pointer...

The fact that 1 job's timeout handler is cleaning up all the other
jobs is messy. I'm not sure if there's a better way...

We could perhaps disable the job interrupt at the beginning though I
think we can still have a race as we can't use disable_irq() with a
shared irq. Or do this after the reset.

> Admittedly this patch is an improvement over the situation before :)

Yes, and I'd like to stop digging a deeper hole...

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

[PATCH v2 4/8] drm/shmem: Do dma_unmap_sg before purging pages

[Rob Herring]

Calling dma_unmap_sg() in drm_gem_shmem_free_object() is too late if the
backing pages have already been released by the shrinker. The result is
the following abort:

Unable to handle kernel paging request at virtual address ffff8000098ed000
Mem abort info:
  ESR = 0x96000147
  Exception class = DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000147
  CM = 1, WnR = 1
swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000002f51000
[ffff8000098ed000] pgd=00000000401f8003, pud=00000000401f7003, pmd=00000000401b1003, pte=00e80000098ed712
Internal error: Oops: 96000147 [#1] SMP
Modules linked in: panfrost gpu_sched
CPU: 5 PID: 902 Comm: gnome-shell Not tainted 5.3.0-rc1+ #95
Hardware name: 96boards Rock960 (DT)
pstate: 40000005 (nZcv daif -PAN -UAO)
pc : __dma_inv_area+0x40/0x58
lr : arch_sync_dma_for_cpu+0x28/0x30
sp : ffff00001321ba30
x29: ffff00001321ba30 x28: ffff00001321bd08
x27: 0000000000000000 x26: 0000000000000009
x25: 0000ffffc1f86170 x24: 0000000000000000
x23: 0000000000000000 x22: 0000000000000000
x21: 0000000000021000 x20: ffff80003bb2d810
x19: 00000000098ed000 x18: 0000000000000000
x17: 0000000000000000 x16: ffff800023fd9480
x15: 0000000000000000 x14: 0000000000000000
x13: 0000000000000000 x12: 0000000000000000
x11: 00000000fffb9fff x10: 0000000000000000
x9 : 0000000000000000 x8 : ffff800023fd9c18
x7 : 0000000000000000 x6 : 00000000ffffffff
x5 : 0000000000000000 x4 : 0000000000021000
Purging 5693440 bytes
x3 : 000000000000003f x2 : 0000000000000040
x1 : ffff80000990e000 x0 : ffff8000098ed000
Call trace:
 __dma_inv_area+0x40/0x58
 dma_direct_sync_single_for_cpu+0x7c/0x80
 dma_direct_unmap_page+0x80/0x88
 dma_direct_unmap_sg+0x54/0x80
 drm_gem_shmem_free_object+0xfc/0x108
 panfrost_gem_free_object+0x118/0x128 [panfrost]
 drm_gem_object_free+0x18/0x90
 drm_gem_object_put_unlocked+0x58/0x80
 drm_gem_object_handle_put_unlocked+0x64/0xb0
 drm_gem_object_release_handle+0x70/0x98
 drm_gem_handle_delete+0x64/0xb0
 drm_gem_close_ioctl+0x28/0x38
 drm_ioctl_kernel+0xb8/0x110
 drm_ioctl+0x244/0x3f0
 do_vfs_ioctl+0xbc/0x910
 ksys_ioctl+0x78/0xa8
 __arm64_sys_ioctl+0x1c/0x28
 el0_svc_common.constprop.0+0x88/0x150
 el0_svc_handler+0x28/0x78
 el0_svc+0x8/0xc
 Code: 8a230000 54000060 d50b7e20 14000002 (d5087620)

Fixes: 17acb9f35ed7 ("drm/shmem: Add madvise state and purge helpers")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index df8f2c8adb2b..5423ec56b535 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -390,6 +390,12 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)
 
 	WARN_ON(!drm_gem_shmem_is_purgeable(shmem));
 
+	dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
+		     shmem->sgt->nents, DMA_BIDIRECTIONAL);
+	sg_free_table(shmem->sgt);
+	kfree(shmem->sgt);
+	shmem->sgt = NULL;
+
 	drm_gem_shmem_put_pages_locked(shmem);
 
 	shmem->madv = -1;
-- 
2.20.1

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

[PATCH v2 5/8] drm/shmem: Use mutex_trylock in drm_gem_shmem_purge

[Rob Herring]

Lockdep reports a circular locking dependency with pages_lock taken in
the shrinker callback. The deadlock can't actually happen with current
users at least as a BO will never be purgeable when pages_lock is held.
To be safe, let's use mutex_trylock() instead and bail if a BO is locked
already.

WARNING: possible circular locking dependency detected
5.3.0-rc1+ #100 Tainted: G             L
------------------------------------------------------
kswapd0/171 is trying to acquire lock:
000000009b9823fd (&shmem->pages_lock){+.+.}, at: drm_gem_shmem_purge+0x20/0x40

but task is already holding lock:
00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (fs_reclaim){+.+.}:
       fs_reclaim_acquire.part.18+0x34/0x40
       fs_reclaim_acquire+0x20/0x28
       __kmalloc_node+0x6c/0x4c0
       kvmalloc_node+0x38/0xa8
       drm_gem_get_pages+0x80/0x1d0
       drm_gem_shmem_get_pages+0x58/0xa0
       drm_gem_shmem_get_pages_sgt+0x48/0xd0
       panfrost_mmu_map+0x38/0xf8 [panfrost]
       panfrost_gem_open+0xc0/0xe8 [panfrost]
       drm_gem_handle_create_tail+0xe8/0x198
       drm_gem_handle_create+0x3c/0x50
       panfrost_gem_create_with_handle+0x70/0xa0 [panfrost]
       panfrost_ioctl_create_bo+0x48/0x80 [panfrost]
       drm_ioctl_kernel+0xb8/0x110
       drm_ioctl+0x244/0x3f0
       do_vfs_ioctl+0xbc/0x910
       ksys_ioctl+0x78/0xa8
       __arm64_sys_ioctl+0x1c/0x28
       el0_svc_common.constprop.0+0x90/0x168
       el0_svc_handler+0x28/0x78
       el0_svc+0x8/0xc

-> #0 (&shmem->pages_lock){+.+.}:
       __lock_acquire+0xa2c/0x1d70
       lock_acquire+0xdc/0x228
       __mutex_lock+0x8c/0x800
       mutex_lock_nested+0x1c/0x28
       drm_gem_shmem_purge+0x20/0x40
       panfrost_gem_shrinker_scan+0xc0/0x180 [panfrost]
       do_shrink_slab+0x208/0x500
       shrink_slab+0x10c/0x2c0
       shrink_node+0x28c/0x4d8
       balance_pgdat+0x2c8/0x570
       kswapd+0x22c/0x638
       kthread+0x128/0x130
       ret_from_fork+0x10/0x18

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(fs_reclaim);
                               lock(&shmem->pages_lock);
                               lock(fs_reclaim);
  lock(&shmem->pages_lock);

 *** DEADLOCK ***

3 locks held by kswapd0/171:
 #0: 00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40
 #1: 00000000ceb37808 (shrinker_rwsem){++++}, at: shrink_slab+0xbc/0x2c0
 #2: 00000000f31efa81 (&pfdev->shrinker_lock){+.+.}, at: panfrost_gem_shrinker_scan+0x34/0x180 [panfrost]

Fixes: 17acb9f35ed7 ("drm/shmem: Add madvise state and purge helpers")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++--
 include/drm/drm_gem_shmem_helper.h     | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 5423ec56b535..f5918707672f 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -415,13 +415,16 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)
 }
 EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
 
-void drm_gem_shmem_purge(struct drm_gem_object *obj)
+bool drm_gem_shmem_purge(struct drm_gem_object *obj)
 {
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 
-	mutex_lock(&shmem->pages_lock);
+	if (!mutex_trylock(&shmem->pages_lock))
+		return false;
 	drm_gem_shmem_purge_locked(obj);
 	mutex_unlock(&shmem->pages_lock);
+
+	return true;
 }
 EXPORT_SYMBOL(drm_gem_shmem_purge);
 
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index ce1600fdfc3e..01f514521687 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -134,7 +134,7 @@ static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem
 }
 
 void drm_gem_shmem_purge_locked(struct drm_gem_object *obj);
-void drm_gem_shmem_purge(struct drm_gem_object *obj);
+bool drm_gem_shmem_purge(struct drm_gem_object *obj);
 
 struct drm_gem_shmem_object *
 drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
-- 
2.20.1

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

Re: [PATCH v2 5/8] drm/shmem: Use mutex_trylock in drm_gem_shmem_purge

[Steven Price]

On 23/08/2019 03:12, Rob Herring wrote:
> Lockdep reports a circular locking dependency with pages_lock taken in
> the shrinker callback. The deadlock can't actually happen with current
> users at least as a BO will never be purgeable when pages_lock is held.
> To be safe, let's use mutex_trylock() instead and bail if a BO is locked
> already.
> 
> WARNING: possible circular locking dependency detected
> 5.3.0-rc1+ #100 Tainted: G             L
> ------------------------------------------------------
> kswapd0/171 is trying to acquire lock:
> 000000009b9823fd (&shmem->pages_lock){+.+.}, at: drm_gem_shmem_purge+0x20/0x40
> 
> but task is already holding lock:
> 00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (fs_reclaim){+.+.}:
>         fs_reclaim_acquire.part.18+0x34/0x40
>         fs_reclaim_acquire+0x20/0x28
>         __kmalloc_node+0x6c/0x4c0
>         kvmalloc_node+0x38/0xa8
>         drm_gem_get_pages+0x80/0x1d0
>         drm_gem_shmem_get_pages+0x58/0xa0
>         drm_gem_shmem_get_pages_sgt+0x48/0xd0
>         panfrost_mmu_map+0x38/0xf8 [panfrost]
>         panfrost_gem_open+0xc0/0xe8 [panfrost]
>         drm_gem_handle_create_tail+0xe8/0x198
>         drm_gem_handle_create+0x3c/0x50
>         panfrost_gem_create_with_handle+0x70/0xa0 [panfrost]
>         panfrost_ioctl_create_bo+0x48/0x80 [panfrost]
>         drm_ioctl_kernel+0xb8/0x110
>         drm_ioctl+0x244/0x3f0
>         do_vfs_ioctl+0xbc/0x910
>         ksys_ioctl+0x78/0xa8
>         __arm64_sys_ioctl+0x1c/0x28
>         el0_svc_common.constprop.0+0x90/0x168
>         el0_svc_handler+0x28/0x78
>         el0_svc+0x8/0xc
> 
> -> #0 (&shmem->pages_lock){+.+.}:
>         __lock_acquire+0xa2c/0x1d70
>         lock_acquire+0xdc/0x228
>         __mutex_lock+0x8c/0x800
>         mutex_lock_nested+0x1c/0x28
>         drm_gem_shmem_purge+0x20/0x40
>         panfrost_gem_shrinker_scan+0xc0/0x180 [panfrost]
>         do_shrink_slab+0x208/0x500
>         shrink_slab+0x10c/0x2c0
>         shrink_node+0x28c/0x4d8
>         balance_pgdat+0x2c8/0x570
>         kswapd+0x22c/0x638
>         kthread+0x128/0x130
>         ret_from_fork+0x10/0x18
> 
> other info that might help us debug this:
> 
>   Possible unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(fs_reclaim);
>                                 lock(&shmem->pages_lock);
>                                 lock(fs_reclaim);
>    lock(&shmem->pages_lock);
> 
>   *** DEADLOCK ***
> 
> 3 locks held by kswapd0/171:
>   #0: 00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40
>   #1: 00000000ceb37808 (shrinker_rwsem){++++}, at: shrink_slab+0xbc/0x2c0
>   #2: 00000000f31efa81 (&pfdev->shrinker_lock){+.+.}, at: panfrost_gem_shrinker_scan+0x34/0x180 [panfrost]
> 
> Fixes: 17acb9f35ed7 ("drm/shmem: Add madvise state and purge helpers")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

I thought I'd already given my R-b for this one, but just in case:

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

Steve

> ---
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++--
>   include/drm/drm_gem_shmem_helper.h     | 2 +-
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 5423ec56b535..f5918707672f 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -415,13 +415,16 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)
>   }
>   EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
>   
> -void drm_gem_shmem_purge(struct drm_gem_object *obj)
> +bool drm_gem_shmem_purge(struct drm_gem_object *obj)
>   {
>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>   
> -	mutex_lock(&shmem->pages_lock);
> +	if (!mutex_trylock(&shmem->pages_lock))
> +		return false;
>   	drm_gem_shmem_purge_locked(obj);
>   	mutex_unlock(&shmem->pages_lock);
> +
> +	return true;
>   }
>   EXPORT_SYMBOL(drm_gem_shmem_purge);
>   
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index ce1600fdfc3e..01f514521687 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -134,7 +134,7 @@ static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem
>   }
>   
>   void drm_gem_shmem_purge_locked(struct drm_gem_object *obj);
> -void drm_gem_shmem_purge(struct drm_gem_object *obj);
> +bool drm_gem_shmem_purge(struct drm_gem_object *obj);
>   
>   struct drm_gem_shmem_object *
>   drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
> 

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

[PATCH v2 6/8] drm/panfrost: Use mutex_trylock in panfrost_gem_purge

[Rob Herring]

Lockdep reports a circular locking dependency with pages_lock taken in
the shrinker callback. The deadlock can't actually happen with current
users at least as a BO will never be purgeable when pages_lock is held.
To be safe, let's use mutex_trylock() instead and bail if a BO is locked
already.

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>
---
v2: new patch

 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index d191632b6197..458f0fa68111 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -36,15 +36,18 @@ panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc
 	return count;
 }
 
-static void panfrost_gem_purge(struct drm_gem_object *obj)
+static bool panfrost_gem_purge(struct drm_gem_object *obj)
 {
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-	mutex_lock(&shmem->pages_lock);
+
+	if (!mutex_trylock(&shmem->pages_lock))
+		return false;
 
 	panfrost_mmu_unmap(to_panfrost_bo(obj));
 	drm_gem_shmem_purge_locked(obj);
 
 	mutex_unlock(&shmem->pages_lock);
+	return true;
 }
 
 static unsigned long
@@ -61,8 +64,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 	list_for_each_entry_safe(shmem, tmp, &pfdev->shrinker_list, madv_list) {
 		if (freed >= sc->nr_to_scan)
 			break;
-		if (drm_gem_shmem_is_purgeable(shmem)) {
-			panfrost_gem_purge(&shmem->base);
+		if (drm_gem_shmem_is_purgeable(shmem) &&
+		    panfrost_gem_purge(&shmem->base)) {
 			freed += shmem->base.size >> PAGE_SHIFT;
 			list_del_init(&shmem->madv_list);
 		}
-- 
2.20.1

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

Re: [PATCH v2 6/8] drm/panfrost: Use mutex_trylock in panfrost_gem_purge

[Steven Price]

On 23/08/2019 03:12, Rob Herring wrote:
> Lockdep reports a circular locking dependency with pages_lock taken in
> the shrinker callback. The deadlock can't actually happen with current
> users at least as a BO will never be purgeable when pages_lock is held.
> To be safe, let's use mutex_trylock() instead and bail if a BO is locked
> already.
> 
> 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>

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index d191632b6197..458f0fa68111 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -36,15 +36,18 @@ panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc
>   	return count;
>   }
>   
> -static void panfrost_gem_purge(struct drm_gem_object *obj)
> +static bool panfrost_gem_purge(struct drm_gem_object *obj)
>   {
>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> -	mutex_lock(&shmem->pages_lock);
> +
> +	if (!mutex_trylock(&shmem->pages_lock))
> +		return false;
>   
>   	panfrost_mmu_unmap(to_panfrost_bo(obj));
>   	drm_gem_shmem_purge_locked(obj);
>   
>   	mutex_unlock(&shmem->pages_lock);
> +	return true;
>   }
>   
>   static unsigned long
> @@ -61,8 +64,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
>   	list_for_each_entry_safe(shmem, tmp, &pfdev->shrinker_list, madv_list) {
>   		if (freed >= sc->nr_to_scan)
>   			break;
> -		if (drm_gem_shmem_is_purgeable(shmem)) {
> -			panfrost_gem_purge(&shmem->base);
> +		if (drm_gem_shmem_is_purgeable(shmem) &&
> +		    panfrost_gem_purge(&shmem->base)) {
>   			freed += shmem->base.size >> PAGE_SHIFT;
>   			list_del_init(&shmem->madv_list);
>   		}
> 

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

[PATCH v2 7/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.

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>
---
v2: new patch

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 842bdd7cf6be..ccf671a9c3fb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -220,6 +220,23 @@ 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;
+
+	/* Flush the PTs only if we're already awake */
+	if (!pm_runtime_get_if_in_use(pfdev->dev))
+		return;
+
+	mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
+
+	pm_runtime_mark_last_busy(pfdev->dev);
+	pm_runtime_put_autosuspend(pfdev->dev);
+}
+
 static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 		      u64 iova, int prot, struct sg_table *sgt)
 {
@@ -246,11 +263,10 @@ 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);
 
+	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);
+
 	return 0;
 }
 
@@ -259,7 +275,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))
@@ -272,14 +287,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;
@@ -293,17 +301,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;
-
 	mutex_lock(&bo->mmu->lock);
 
 	while (unmapped_len < len) {
@@ -318,13 +321,9 @@ 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);
-
 	mutex_unlock(&bo->mmu->lock);
 
-	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 v2 7/8] drm/panfrost: Rework page table flushing and runtime PM interaction

[Robin Murphy]

On 23/08/2019 03:12, 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.
> 
> 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>
> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
>   1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 842bdd7cf6be..ccf671a9c3fb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -220,6 +220,23 @@ 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;
> +
> +	/* Flush the PTs only if we're already awake */
> +	if (!pm_runtime_get_if_in_use(pfdev->dev))
> +		return;

Is the MMU guaranteed to be reset on resume such that the TLBs will 
always come up clean? Otherwise there are potentially corners where 
stale entries that we skip here might hang around if the hardware lies 
about powering things down.

Robin.

> +
> +	mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
> +
> +	pm_runtime_mark_last_busy(pfdev->dev);
> +	pm_runtime_put_autosuspend(pfdev->dev);
> +}
> +
>   static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   		      u64 iova, int prot, struct sg_table *sgt)
>   {
> @@ -246,11 +263,10 @@ 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);
>   
> +	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);
> +
>   	return 0;
>   }
>   
> @@ -259,7 +275,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))
> @@ -272,14 +287,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;
> @@ -293,17 +301,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;
> -
>   	mutex_lock(&bo->mmu->lock);
>   
>   	while (unmapped_len < len) {
> @@ -318,13 +321,9 @@ 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);
> -
>   	mutex_unlock(&bo->mmu->lock);
>   
> -	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;
>   }
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Steven Price]

On 23/08/2019 12:11, Robin Murphy wrote:
> On 23/08/2019 03:12, 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.
>>
>> 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>
>> ---
>> v2: new patch
>>
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
>>   1 file changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c 
>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 842bdd7cf6be..ccf671a9c3fb 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -220,6 +220,23 @@ 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;
>> +
>> +    /* Flush the PTs only if we're already awake */
>> +    if (!pm_runtime_get_if_in_use(pfdev->dev))
>> +        return;
> 
> Is the MMU guaranteed to be reset on resume such that the TLBs will 
> always come up clean? Otherwise there are potentially corners where 
> stale entries that we skip here might hang around if the hardware lies 
> about powering things down.

Assuming runtime PM has actually committed to the power off, then on 
power on panfrost_device_resume() will be called which performs a reset 
of the GPU which will clear the L2/TLBs so there shouldn't be any 
problem there.

As far as I can see from the code that is how pm_runtime_get_if_in_use() 
works - it will return true unless the suspend() callback has been called.

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

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

[Robin Murphy]

On 23/08/2019 16:05, Steven Price wrote:
> On 23/08/2019 12:11, Robin Murphy wrote:
>> On 23/08/2019 03:12, 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.
>>>
>>> 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>
>>> ---
>>> v2: new patch
>>>
>>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
>>>   1 file changed, 20 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c 
>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index 842bdd7cf6be..ccf671a9c3fb 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -220,6 +220,23 @@ 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;
>>> +
>>> +    /* Flush the PTs only if we're already awake */
>>> +    if (!pm_runtime_get_if_in_use(pfdev->dev))
>>> +        return;
>>
>> Is the MMU guaranteed to be reset on resume such that the TLBs will 
>> always come up clean? Otherwise there are potentially corners where 
>> stale entries that we skip here might hang around if the hardware lies 
>> about powering things down.
> 
> Assuming runtime PM has actually committed to the power off, then on 
> power on panfrost_device_resume() will be called which performs a reset 
> of the GPU which will clear the L2/TLBs so there shouldn't be any 
> problem there.

OK, if panfrost_gpu_soft_reset() is sufficient to guarantee clean TLBs 
then this looks equivalent to what we did for arm-smmu, so I've no 
complaints in that regard.

However on second look I've now noticed the panfrost_mmu_flush_range() 
calls being moved outside of mmu->lock protection. Forgive me if there's 
basic DRM knowledge I'm missing here, but is there any possibility for 
multiple threads to create/import/free objects simultaneously on the 
same FD such that two mmu_hw_do_operation() calls could race and 
interfere with each other in terms of the 
AS_LOCKADDR/AS_COMMAND/AS_STATUS dance?

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

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

[Rob Herring]

On Fri, Aug 23, 2019 at 10:44 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 23/08/2019 16:05, Steven Price wrote:
> > On 23/08/2019 12:11, Robin Murphy wrote:
> >> On 23/08/2019 03:12, 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.
> >>>
> >>> 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>
> >>> ---
> >>> v2: new patch
> >>>
> >>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
> >>>   1 file changed, 20 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>> index 842bdd7cf6be..ccf671a9c3fb 100644
> >>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>> @@ -220,6 +220,23 @@ 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;
> >>> +
> >>> +    /* Flush the PTs only if we're already awake */
> >>> +    if (!pm_runtime_get_if_in_use(pfdev->dev))
> >>> +        return;
> >>
> >> Is the MMU guaranteed to be reset on resume such that the TLBs will
> >> always come up clean? Otherwise there are potentially corners where
> >> stale entries that we skip here might hang around if the hardware lies
> >> about powering things down.
> >
> > Assuming runtime PM has actually committed to the power off, then on
> > power on panfrost_device_resume() will be called which performs a reset
> > of the GPU which will clear the L2/TLBs so there shouldn't be any
> > problem there.
>
> OK, if panfrost_gpu_soft_reset() is sufficient to guarantee clean TLBs
> then this looks equivalent to what we did for arm-smmu, so I've no
> complaints in that regard.
>
> However on second look I've now noticed the panfrost_mmu_flush_range()
> calls being moved outside of mmu->lock protection. Forgive me if there's
> basic DRM knowledge I'm missing here, but is there any possibility for
> multiple threads to create/import/free objects simultaneously on the
> same FD such that two mmu_hw_do_operation() calls could race and
> interfere with each other in terms of the
> AS_LOCKADDR/AS_COMMAND/AS_STATUS dance?

Yes, we could have multiple threads. Not really any good reason it's
moved out of the mmu->lock other than just to avoid any nesting
(though that seemed fine in testing). The newly added as_lock will
serialize mmu_hw_do_operation(). So the mmu->lock is just serializing
page table writes.

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

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

[Robin Murphy]

On 23/08/2019 16:57, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 10:44 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 23/08/2019 16:05, Steven Price wrote:
>>> On 23/08/2019 12:11, Robin Murphy wrote:
>>>> On 23/08/2019 03:12, 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.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> v2: new patch
>>>>>
>>>>>    drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
>>>>>    1 file changed, 20 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>>>> index 842bdd7cf6be..ccf671a9c3fb 100644
>>>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>>>> @@ -220,6 +220,23 @@ 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;
>>>>> +
>>>>> +    /* Flush the PTs only if we're already awake */
>>>>> +    if (!pm_runtime_get_if_in_use(pfdev->dev))
>>>>> +        return;
>>>>
>>>> Is the MMU guaranteed to be reset on resume such that the TLBs will
>>>> always come up clean? Otherwise there are potentially corners where
>>>> stale entries that we skip here might hang around if the hardware lies
>>>> about powering things down.
>>>
>>> Assuming runtime PM has actually committed to the power off, then on
>>> power on panfrost_device_resume() will be called which performs a reset
>>> of the GPU which will clear the L2/TLBs so there shouldn't be any
>>> problem there.
>>
>> OK, if panfrost_gpu_soft_reset() is sufficient to guarantee clean TLBs
>> then this looks equivalent to what we did for arm-smmu, so I've no
>> complaints in that regard.
>>
>> However on second look I've now noticed the panfrost_mmu_flush_range()
>> calls being moved outside of mmu->lock protection. Forgive me if there's
>> basic DRM knowledge I'm missing here, but is there any possibility for
>> multiple threads to create/import/free objects simultaneously on the
>> same FD such that two mmu_hw_do_operation() calls could race and
>> interfere with each other in terms of the
>> AS_LOCKADDR/AS_COMMAND/AS_STATUS dance?
> 
> Yes, we could have multiple threads. Not really any good reason it's
> moved out of the mmu->lock other than just to avoid any nesting
> (though that seemed fine in testing). The newly added as_lock will
> serialize mmu_hw_do_operation(). So the mmu->lock is just serializing
> page table writes.

Urgh, sorry, once again I'd stopped looking at -next and was 
cross-referencing my current rc3-based working tree :(

In that case, you may even be able to remove mmu->lock entirely, since 
io-pgtable-arm doesn't need external locking itself. And for this patch,

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

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

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

[Rob Herring]

On Fri, Aug 23, 2019 at 11:16 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 23/08/2019 16:57, Rob Herring wrote:
> > On Fri, Aug 23, 2019 at 10:44 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 23/08/2019 16:05, Steven Price wrote:
> >>> On 23/08/2019 12:11, Robin Murphy wrote:
> >>>> On 23/08/2019 03:12, 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.
> >>>>>
> >>>>> 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>
> >>>>> ---
> >>>>> v2: new patch
> >>>>>
> >>>>>    drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
> >>>>>    1 file changed, 20 insertions(+), 21 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>>>> index 842bdd7cf6be..ccf671a9c3fb 100644
> >>>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> >>>>> @@ -220,6 +220,23 @@ 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;
> >>>>> +
> >>>>> +    /* Flush the PTs only if we're already awake */
> >>>>> +    if (!pm_runtime_get_if_in_use(pfdev->dev))
> >>>>> +        return;
> >>>>
> >>>> Is the MMU guaranteed to be reset on resume such that the TLBs will
> >>>> always come up clean? Otherwise there are potentially corners where
> >>>> stale entries that we skip here might hang around if the hardware lies
> >>>> about powering things down.
> >>>
> >>> Assuming runtime PM has actually committed to the power off, then on
> >>> power on panfrost_device_resume() will be called which performs a reset
> >>> of the GPU which will clear the L2/TLBs so there shouldn't be any
> >>> problem there.
> >>
> >> OK, if panfrost_gpu_soft_reset() is sufficient to guarantee clean TLBs
> >> then this looks equivalent to what we did for arm-smmu, so I've no
> >> complaints in that regard.
> >>
> >> However on second look I've now noticed the panfrost_mmu_flush_range()
> >> calls being moved outside of mmu->lock protection. Forgive me if there's
> >> basic DRM knowledge I'm missing here, but is there any possibility for
> >> multiple threads to create/import/free objects simultaneously on the
> >> same FD such that two mmu_hw_do_operation() calls could race and
> >> interfere with each other in terms of the
> >> AS_LOCKADDR/AS_COMMAND/AS_STATUS dance?
> >
> > Yes, we could have multiple threads. Not really any good reason it's
> > moved out of the mmu->lock other than just to avoid any nesting
> > (though that seemed fine in testing). The newly added as_lock will
> > serialize mmu_hw_do_operation(). So the mmu->lock is just serializing
> > page table writes.
>
> Urgh, sorry, once again I'd stopped looking at -next and was
> cross-referencing my current rc3-based working tree :(
>
> In that case, you may even be able to remove mmu->lock entirely, since
> io-pgtable-arm doesn't need external locking itself. And for this patch,

I was wondering about that, but hadn't gotten around to investigating.

>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> Cheers,
> Robin.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Steven Price]

On 23/08/2019 03:12, 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.
> 
> 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>

But one comment below...

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
>   1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 842bdd7cf6be..ccf671a9c3fb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -220,6 +220,23 @@ 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;
> +
> +	/* Flush the PTs only if we're already awake */
> +	if (!pm_runtime_get_if_in_use(pfdev->dev))
> +		return;
> +
> +	mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
> +
> +	pm_runtime_mark_last_busy(pfdev->dev);

This isn't really a change, but: I'm not sure why we want to signal we 
were busy just because we had to do some cache maintenance? We might 
actually be faster leaving the GPU off so there's no need to do extra 
flushes on the GPU?

Steve

> +	pm_runtime_put_autosuspend(pfdev->dev);
> +}
> +
>   static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>   		      u64 iova, int prot, struct sg_table *sgt)
>   {
> @@ -246,11 +263,10 @@ 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);
>   
> +	panfrost_mmu_flush_range(pfdev, mmu, start_iova, iova - start_iova);
> +
>   	return 0;
>   }
>   
> @@ -259,7 +275,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))
> @@ -272,14 +287,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;
> @@ -293,17 +301,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;
> -
>   	mutex_lock(&bo->mmu->lock);
>   
>   	while (unmapped_len < len) {
> @@ -318,13 +321,9 @@ 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);
> -
>   	mutex_unlock(&bo->mmu->lock);
>   
> -	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;
>   }
>   
> 

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

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

[Rob Herring]

On Fri, Aug 23, 2019 at 10:09 AM Steven Price <steven.price@arm.com> wrote:
>
> On 23/08/2019 03:12, 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.
> >
> > 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>
>
> But one comment below...
>
> > ---
> > v2: new patch
> >
> >   drivers/gpu/drm/panfrost/panfrost_mmu.c | 41 ++++++++++++-------------
> >   1 file changed, 20 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index 842bdd7cf6be..ccf671a9c3fb 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -220,6 +220,23 @@ 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;
> > +
> > +     /* Flush the PTs only if we're already awake */
> > +     if (!pm_runtime_get_if_in_use(pfdev->dev))
> > +             return;
> > +
> > +     mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
> > +
> > +     pm_runtime_mark_last_busy(pfdev->dev);
>
> This isn't really a change, but: I'm not sure why we want to signal we
> were busy just because we had to do some cache maintenance? We might
> actually be faster leaving the GPU off so there's no need to do extra
> flushes on the GPU?

I don't know, good question. Powering up and down has its cost too. We
only get here if we were already active. A flush on a map probably is
going to be followed by a job. An unmap may be the end of activity or
not.

If we don't call pm_runtime_mark_last_busy, then maybe we also want to
do a pm_runtime_put_suspend (i.e. suspend immediately) instead. That
may only matter if we're the last one which could only happen during
this get/put. I'm not sure what happens if a suspend is requested
followed by an autosuspend.

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

[PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Rob Herring]

tlb_inv_context() hook is only called when freeing the page tables. There's
no point in flushing only to free the page tables immediately following.
There is also a problem that we could be accessing the h/w when suspended.

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>
---
v2: new patch

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ccf671a9c3fb..9f85275a896c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -328,11 +328,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)
-- 
2.20.1

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

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Robin Murphy]

On 23/08/2019 03:12, Rob Herring wrote:
> tlb_inv_context() hook is only called when freeing the page tables. There's
> no point in flushing only to free the page tables immediately following.

FWIW, in general the point of flushing is *because* we're about to free 
the pagetables - if there's any possibility that the hardware could 
continue to issue translation table walks (speculative or otherwise) 
after those pages have been reused by someone else, TLB badness may ensue.

For panfrost in particular I suspect we can probably get away without 
it, at least for the moment, but it might be worth moving the flush to 
mmu_disable() for complete peace of mind (which kind of preempts the 
sort of thing that per-process AS switching will want anyway).

Robin.

> There is also a problem that we could be accessing the h/w when suspended.
> 
> 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>
> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ccf671a9c3fb..9f85275a896c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -328,11 +328,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)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Rob Herring]

On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 23/08/2019 03:12, Rob Herring wrote:
> > tlb_inv_context() hook is only called when freeing the page tables. There's
> > no point in flushing only to free the page tables immediately following.
>
> FWIW, in general the point of flushing is *because* we're about to free
> the pagetables - if there's any possibility that the hardware could
> continue to issue translation table walks (speculative or otherwise)
> after those pages have been reused by someone else, TLB badness may ensue.
>
> For panfrost in particular I suspect we can probably get away without
> it, at least for the moment, but it might be worth moving the flush to
> mmu_disable() for complete peace of mind (which kind of preempts the
> sort of thing that per-process AS switching will want anyway).

There's bigger problem that mmu_disable() is still only called for AS0
and only for driver unload. I guess we should fix that and then figure
out where a flush is needed if at all. I would think changing the TTBR
would be enough to quiesce the h/w and TLBs. That seems to be the case
in my testing of switching address spaces.

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

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Robin Murphy]

On 23/08/2019 14:18, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 23/08/2019 03:12, Rob Herring wrote:
>>> tlb_inv_context() hook is only called when freeing the page tables. There's
>>> no point in flushing only to free the page tables immediately following.
>>
>> FWIW, in general the point of flushing is *because* we're about to free
>> the pagetables - if there's any possibility that the hardware could
>> continue to issue translation table walks (speculative or otherwise)
>> after those pages have been reused by someone else, TLB badness may ensue.
>>
>> For panfrost in particular I suspect we can probably get away without
>> it, at least for the moment, but it might be worth moving the flush to
>> mmu_disable() for complete peace of mind (which kind of preempts the
>> sort of thing that per-process AS switching will want anyway).
> 
> There's bigger problem that mmu_disable() is still only called for AS0
> and only for driver unload.

Sure, but AS0 is the only one we ever enable, and conceptually we do 
that once at probe time (AFAICS it stays nominally live through resets 
and resumes), so while it may be the least-clever AS usage possible it's 
at least self-consistent. And at the moment the only time we'll call 
.tlb_inv_context is from panfrost_mmu_fini() when we're unconditionally 
poking registers anyway, so either way I don't think there's any actual 
problem today - I'm viewing this patch as getting things straight in 
preparation for the future.

> I guess we should fix that and then figure
> out where a flush is needed if at all. I would think changing the TTBR
> would be enough to quiesce the h/w and TLBs. That seems to be the case
> in my testing of switching address spaces.

 From a quick scan through kbase, kbase_mmu_disable() appears to perform 
an full FLUSH_MEM before rewriting TRANSTAB, and it looks like that does 
get called when scheduling out a context. That's in line with what I was 
imagining, so unless we know better for sure, we may as well play it 
safe and follow the same pattern.

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

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Rob Herring]

On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 23/08/2019 14:18, Rob Herring wrote:
> > On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 23/08/2019 03:12, Rob Herring wrote:
> >>> tlb_inv_context() hook is only called when freeing the page tables. There's
> >>> no point in flushing only to free the page tables immediately following.
> >>
> >> FWIW, in general the point of flushing is *because* we're about to free
> >> the pagetables - if there's any possibility that the hardware could
> >> continue to issue translation table walks (speculative or otherwise)
> >> after those pages have been reused by someone else, TLB badness may ensue.
> >>
> >> For panfrost in particular I suspect we can probably get away without
> >> it, at least for the moment, but it might be worth moving the flush to
> >> mmu_disable() for complete peace of mind (which kind of preempts the
> >> sort of thing that per-process AS switching will want anyway).
> >
> > There's bigger problem that mmu_disable() is still only called for AS0
> > and only for driver unload.
>
> Sure, but AS0 is the only one we ever enable, and conceptually we do
> that once at probe time (AFAICS it stays nominally live through resets
> and resumes), so while it may be the least-clever AS usage possible it's
> at least self-consistent. And at the moment the only time we'll call
> .tlb_inv_context is from panfrost_mmu_fini() when we're unconditionally
> poking registers anyway, so either way I don't think there's any actual
> problem today - I'm viewing this patch as getting things straight in
> preparation for the future.

It was only AS0, but we now have per FD AS.

> > I guess we should fix that and then figure
> > out where a flush is needed if at all. I would think changing the TTBR
> > would be enough to quiesce the h/w and TLBs. That seems to be the case
> > in my testing of switching address spaces.
>
>  From a quick scan through kbase, kbase_mmu_disable() appears to perform
> an full FLUSH_MEM before rewriting TRANSTAB, and it looks like that does
> get called when scheduling out a context. That's in line with what I was
> imagining, so unless we know better for sure, we may as well play it
> safe and follow the same pattern.

Agreed.

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

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Robin Murphy]

On 23/08/2019 15:26, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 23/08/2019 14:18, Rob Herring wrote:
>>> On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 23/08/2019 03:12, Rob Herring wrote:
>>>>> tlb_inv_context() hook is only called when freeing the page tables. There's
>>>>> no point in flushing only to free the page tables immediately following.
>>>>
>>>> FWIW, in general the point of flushing is *because* we're about to free
>>>> the pagetables - if there's any possibility that the hardware could
>>>> continue to issue translation table walks (speculative or otherwise)
>>>> after those pages have been reused by someone else, TLB badness may ensue.
>>>>
>>>> For panfrost in particular I suspect we can probably get away without
>>>> it, at least for the moment, but it might be worth moving the flush to
>>>> mmu_disable() for complete peace of mind (which kind of preempts the
>>>> sort of thing that per-process AS switching will want anyway).
>>>
>>> There's bigger problem that mmu_disable() is still only called for AS0
>>> and only for driver unload.
>>
>> Sure, but AS0 is the only one we ever enable, and conceptually we do
>> that once at probe time (AFAICS it stays nominally live through resets
>> and resumes), so while it may be the least-clever AS usage possible it's
>> at least self-consistent. And at the moment the only time we'll call
>> .tlb_inv_context is from panfrost_mmu_fini() when we're unconditionally
>> poking registers anyway, so either way I don't think there's any actual
>> problem today - I'm viewing this patch as getting things straight in
>> preparation for the future.
> 
> It was only AS0, but we now have per FD AS.

Ah, silly me, I forgot to look at -next...

>>> I guess we should fix that and then figure
>>> out where a flush is needed if at all. I would think changing the TTBR
>>> would be enough to quiesce the h/w and TLBs. That seems to be the case
>>> in my testing of switching address spaces.
>>
>>   From a quick scan through kbase, kbase_mmu_disable() appears to perform
>> an full FLUSH_MEM before rewriting TRANSTAB, and it looks like that does
>> get called when scheduling out a context. That's in line with what I was
>> imagining, so unless we know better for sure, we may as well play it
>> safe and follow the same pattern.
> 
> Agreed.

I guess in that case we probably want it in panfrost_mmu_as_put() when 
as_count falls to 0, to balance the corresponding enable in as_get(). If 
the tables are only freed later when the FD is closed and whichever AS 
is probably in use by some other job, that's even more reason that 
.tlb_inv_context is now the wrong place to be touching hardware.

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

Re: [PATCH v2 8/8] drm/panfrost: Remove unnecessary flushing from tlb_inv_context

[Steven Price]

On 23/08/2019 03:12, Rob Herring wrote:
> tlb_inv_context() hook is only called when freeing the page tables. There's
> no point in flushing only to free the page tables immediately following.
> There is also a problem that we could be accessing the h/w when suspended.
> 
> 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>

Although it might be worth trying to capture the justification about 
this in a comment somewhere - there's been a fair bit of discussion 
about this...

Steve

> ---
> v2: new patch
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ccf671a9c3fb..9f85275a896c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -328,11 +328,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)
> 

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