Re: [PATCH] drm/panfrost: Implement per FD address spaces

On 08/08/2019 23:29, Rob Herring wrote:

Up until now, a single shared GPU address space was used. This is not ideal as there's no protection between processes and doesn't work for supporting the same GPU/CPU VA feature. Most importantly, this will hopefully mitigate Alyssa's fear of WebGL, whatever that is.

Most of the changes here are moving struct drm_mm and struct panfrost_mmu objects from the per device struct to the per FD struct. The critical function is panfrost_mmu_as_get() which handles allocating and switching the h/w address spaces.

Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Cc: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Signed-off-by: Rob Herring robh@kernel.org

---

This depends on madvise support (now in drm-misc) and the heap/no-exec series (just the rework). Seeems to be working pretty well for me, but more testing would be helpful. I've run multiple 'glmark2-es2-drm --off-screen' instances and Gnome Shell. Running more than 8 clients (at least for T860) will hit the address space switch code paths.

Rob

drivers/gpu/drm/panfrost/TODO | 4 - drivers/gpu/drm/panfrost/panfrost_device.c | 2 + drivers/gpu/drm/panfrost/panfrost_device.h | 24 ++- drivers/gpu/drm/panfrost/panfrost_drv.c | 31 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 15 +- drivers/gpu/drm/panfrost/panfrost_gem.h | 3 + drivers/gpu/drm/panfrost/panfrost_job.c | 12 +- drivers/gpu/drm/panfrost/panfrost_mmu.c | 220 +++++++++++++++------ drivers/gpu/drm/panfrost/panfrost_mmu.h | 8 + 9 files changed, 239 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/TODO b/drivers/gpu/drm/panfrost/TODO index e7727b292355..536a0d4f8d29 100644 --- a/drivers/gpu/drm/panfrost/TODO +++ b/drivers/gpu/drm/panfrost/TODO @@ -6,10 +6,6 @@

• Bifrost specific feature and issue handling
• Coherent DMA support

-- Per FD address space support. The h/w supports multiple addresses spaces.

• The hard part is handling when more address spaces are needed than what

• the h/w provides.

• Support userspace controlled GPU virtual addresses. Needed for Vulkan. (Tomeu)

• Compute job support. So called 'compute only' jobs need to be plumbed up to

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 9814f4ccbd26..4da71bb56c20 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -123,8 +123,10 @@ int panfrost_device_init(struct panfrost_device *pfdev) mutex_init(&pfdev->sched_lock); mutex_init(&pfdev->reset_lock); 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); if (err) {

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index 4e5641db9c7e..f503c566e99f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -5,6 +5,8 @@ #ifndef __PANFROST_DEVICE_H__ #define __PANFROST_DEVICE_H__

+#include <linux/atomic.h> +#include <linux/io-pgtable.h> #include <linux/spinlock.h> #include <drm/drm_device.h> #include <drm/drm_mm.h> @@ -63,9 +65,6 @@ struct panfrost_device {

spinlock_t hwaccess_lock;

• struct drm_mm mm;
• spinlock_t mm_lock;
• void __iomem *iomem; struct clk *clock; struct clk *bus_clock;

@@ -74,7 +73,11 @@ struct panfrost_device {

struct panfrost_features features;

• struct panfrost_mmu *mmu;

• spinlock_t as_lock;

• unsigned long as_in_use_mask;

• unsigned long as_alloc_mask;

• struct list_head as_lru_list;

• struct panfrost_job_slot *js;

  struct panfrost_job *jobs[NUM_JOB_SLOTS];

@@ -98,10 +101,23 @@ struct panfrost_device { } devfreq; };

+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;

+};

struct panfrost_file_priv { struct panfrost_device *pfdev;

struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];

• struct panfrost_mmu mmu;
• struct drm_mm mm;
• spinlock_t mm_lock;

};

static inline struct panfrost_device *to_panfrost_device(struct drm_device *ddev) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index a1352750984c..7c8aa1a8054f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -403,6 +403,7 @@ static void panfrost_drm_mm_color_adjust(const struct drm_mm_node *node, static int panfrost_open(struct drm_device *dev, struct drm_file *file) {

• int ret; struct panfrost_device *pfdev = dev->dev_private; struct panfrost_file_priv *panfrost_priv;

@@ -413,7 +414,28 @@ panfrost_open(struct drm_device *dev, struct drm_file *file) panfrost_priv->pfdev = pfdev; file->driver_priv = panfrost_priv;

• return panfrost_job_open(panfrost_priv);

• spin_lock_init(&panfrost_priv->mm_lock);
• /* 4G enough for now. can be 48-bit */
• panfrost_priv->mm.color_adjust = panfrost_drm_mm_color_adjust;
• drm_mm_init(&panfrost_priv->mm, SZ_32M >> PAGE_SHIFT, (SZ_4G - SZ_32M) >> PAGE_SHIFT);
• ret = panfrost_mmu_pgtable_alloc(panfrost_priv);
• if (ret)

• goto err_pgtable;
  

• ret = panfrost_job_open(panfrost_priv);
• if (ret)

• goto err_job;
  

• return 0;

+err_job:

• panfrost_mmu_pgtable_free(panfrost_priv);

+err_pgtable:

• drm_mm_takedown(&panfrost_priv->mm);
• kfree(panfrost_priv);

Looks like this kfree was missing before... :)

• return ret;

}

static void @@ -424,6 +446,8 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file) panfrost_perfcnt_close(panfrost_priv); panfrost_job_close(panfrost_priv);

• panfrost_mmu_pgtable_free(panfrost_priv);
• drm_mm_takedown(&panfrost_priv->mm); kfree(panfrost_priv);

}

@@ -496,14 +520,9 @@ static int panfrost_probe(struct platform_device *pdev) ddev->dev_private = pfdev; pfdev->ddev = ddev;

• spin_lock_init(&pfdev->mm_lock); mutex_init(&pfdev->shrinker_lock); INIT_LIST_HEAD(&pfdev->shrinker_list);

• /* 4G enough for now. can be 48-bit */

• drm_mm_init(&pfdev->mm, SZ_32M >> PAGE_SHIFT, (SZ_4G - SZ_32M) >> PAGE_SHIFT);

• pfdev->mm.color_adjust = panfrost_drm_mm_color_adjust;

• pm_runtime_use_autosuspend(pfdev->dev); pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ pm_runtime_enable(pfdev->dev);

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 37a3a6ed4617..93204b44ef90 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -47,8 +47,8 @@ static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_p size_t size = obj->size; u64 align; struct panfrost_gem_object *bo = to_panfrost_bo(obj);

• struct panfrost_device *pfdev = obj->dev->dev_private; unsigned long color = bo->noexec ? PANFROST_BO_NOEXEC : 0;

• struct panfrost_file_priv *priv = file_priv->driver_priv;

  /*

  • Executable buffers cannot cross a 16MB boundary as the program

@@ -61,8 +61,9 @@ static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_p else align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;

• spin_lock(&pfdev->mm_lock);
• ret = drm_mm_insert_node_generic(&pfdev->mm, &bo->node,

• bo->mmu = &priv->mmu;
• spin_lock(&priv->mm_lock);
• ret = drm_mm_insert_node_generic(&priv->mm, &bo->node, size >> PAGE_SHIFT, align, color, 0); if (ret) goto out;

@@ -73,22 +74,22 @@ static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_p drm_mm_remove_node(&bo->node); } out:

• spin_unlock(&pfdev->mm_lock);

• spin_unlock(&priv->mm_lock); return ret;

}

static void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv) { struct panfrost_gem_object *bo = to_panfrost_bo(obj);

• struct panfrost_device *pfdev = obj->dev->dev_private;

• struct panfrost_file_priv *priv = file_priv->driver_priv;

  if (bo->is_mapped) panfrost_mmu_unmap(bo);

• spin_lock(&pfdev->mm_lock);

• spin_lock(&priv->mm_lock); if (drm_mm_node_allocated(&bo->node)) drm_mm_remove_node(&bo->node);

• spin_unlock(&pfdev->mm_lock);

• spin_unlock(&priv->mm_lock);

}

static int panfrost_gem_pin(struct drm_gem_object *obj) diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index e10f58316915..50920819cc16 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -7,10 +7,13 @@ #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_mm.h>

+struct panfrost_mmu;

struct panfrost_gem_object { struct drm_gem_shmem_object base; struct sg_table *sgts;

• struct panfrost_mmu *mmu; struct drm_mm_node node; bool is_mapped :1; bool noexec :1;

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index d567ce98494c..aea84610b8cd 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -153,6 +153,8 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) goto end;

• cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
• panfrost_devfreq_record_transition(pfdev, js); spin_lock_irqsave(&pfdev->hwaccess_lock, flags);

@@ -163,8 +165,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)

/* start MMU, medium priority, cache clean/flush on end, clean/flush on * start */

• /* TODO: different address spaces */
• cfg = JS_CONFIG_THREAD_PRI(8) |

• cfg |= JS_CONFIG_THREAD_PRI(8) | JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE | JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;

@@ -280,6 +281,9 @@ static void panfrost_job_cleanup(struct kref *ref) kvfree(job->bos); }

• for (i = 0; i < NUM_JOB_SLOTS; i++)

• if (job == job->pfdev->jobs[i])
  

• 	job->pfdev->jobs[i] = NULL;
  

I'm a bit worried about this. As far as I can see previously the jobs array would just contain dangling pointers - which admittedly wasn't ideal. However do we have sufficient locks here to ensure that this could be racing with a panfrost_job_run() on the same slot? I'm worried that between the check and the assignment to NULL it is possible (admittedly very unlikely) for panfrost_job_run() to assign a new pointer which would then be clobbered.

kfree(job); }

@@ -377,8 +381,9 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) if (dma_fence_is_signaled(job->done_fence)) return;

• dev_err(pfdev->dev, "gpu sched timeout, js=%d, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",

• dev_err(pfdev->dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p", js,

• job_read(pfdev, JS_CONFIG(js)),
  

  job_read(pfdev, JS_STATUS(js)), job_read(pfdev, JS_HEAD_LO(js)), job_read(pfdev, JS_TAIL_LO(js)),

@@ -448,6 +453,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) }

if (status & JOB_INT_MASK_DONE(j)) {

• 	panfrost_mmu_as_put(pfdev, &pfdev->jobs[j]->file_priv->mmu);
  panfrost_devfreq_record_transition(pfdev, j);
  dma_fence_signal(pfdev->jobs[j]->done_fence);
  

  }

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 2ed411f09d80..f8da7557d1be 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright 2019 Linaro, Ltd, Rob Herring robh@kernel.org */ +#include <linux/atomic.h> #include <linux/bitfield.h> #include <linux/delay.h> #include <linux/dma-mapping.h> @@ -22,12 +23,6 @@ #define mmu_write(dev, reg, data) writel(data, dev->iomem + reg) #define mmu_read(dev, reg) readl(dev->iomem + reg)

-struct panfrost_mmu {

• struct io_pgtable_cfg pgtbl_cfg;
• struct io_pgtable_ops *pgtbl_ops;
• struct mutex lock;

-};

static int wait_ready(struct panfrost_device *pfdev, u32 as_nr) { int ret; @@ -91,6 +86,9 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev, u32 as_nr, unsigned long flags; int ret;

• if (as_nr < 0)

• return 0;
  

Do we need any synchronisation with panfrost_mmu_as_get() for this to be reliable? As far as I can see this check can race with the assignment of an address space. Perhaps panfrost_mmu_as_get() (or maybe panfrost_mmu_enable) should hold mmu->lock?

spin_lock_irqsave(&pfdev->hwaccess_lock, flags);

if (op != AS_COMMAND_UNLOCK) @@ -107,9 +105,10 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev, u32 as_nr, return ret; }

-static void panfrost_mmu_enable(struct panfrost_device *pfdev, u32 as_nr) +static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu) {

• struct io_pgtable_cfg *cfg = &pfdev->mmu->pgtbl_cfg;

• int as_nr = mmu->as;
• struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg; u64 transtab = cfg->arm_mali_lpae_cfg.transtab; u64 memattr = cfg->arm_mali_lpae_cfg.memattr;

@@ -136,9 +135,87 @@ static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr) write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE); }

+u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu) +{

• int as = mmu->as;
• spin_lock(&pfdev->as_lock);
• as = mmu->as;
• if (as >= 0) {

• int en = atomic_fetch_inc(&mmu->as_count);
  

• WARN_ON(en > 2);
  

• list_move(&mmu->list, &pfdev->as_lru_list);
  

• spin_unlock(&pfdev->as_lock);
  

• if (!en)
  

• 	panfrost_mmu_enable(pfdev, mmu);
  

I don't see why we need to re-enable here - if (as >= 0) then the AS should be present on the GPU and mmu_hw_do_operation() will have been doing any necessary MMU commands.

• return as;
  

• }
• /* Check for a free AS */
• as = ffz(pfdev->as_alloc_mask);
• if (!(BIT(as) & pfdev->features.as_present)) {

• struct panfrost_mmu *lru_mmu;
  

• list_for_each_entry_reverse(lru_mmu, &pfdev->as_lru_list, list) {
  

• 	if (!atomic_read(&lru_mmu->as_count))
  

• 		break;
  

• }
  

• WARN_ON(!lru_mmu);
  

• list_del_init(&lru_mmu->list);
  

• as = lru_mmu->as;
  

• WARN_ON(as < 0);
  

• lru_mmu->as = -1;
  

This should be holding a lock to synchronise with the mmu_hw_do_operation().

• }
• /* Assign the free or reclaimed AS to the */
• mmu->as = as;
• set_bit(as, &pfdev->as_alloc_mask);
• atomic_set(&mmu->as_count, 1);
• list_add(&mmu->list, &pfdev->as_lru_list);
• dev_dbg(pfdev->dev, "Assigned AS%d to mmu %p, alloc_mask=%lx", as, mmu, pfdev->as_alloc_mask);
• spin_unlock(&pfdev->as_lock);
• panfrost_mmu_enable(pfdev, mmu);
• return as;

+}

+void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu) +{

• atomic_dec(&mmu->as_count);
• WARN_ON(atomic_read(&mmu->as_count) < 0);

+}

+static void panfrost_mmu_as_reset(struct panfrost_device *pfdev, struct panfrost_mmu *mmu) +{

• spin_lock(&pfdev->as_lock);
• atomic_set(&mmu->as_count, 0);
• if (mmu->as >= 0) {

• clear_bit(mmu->as, &pfdev->as_alloc_mask);
  

• list_del_init(&mmu->list);
  

• mmu->as = -1;
  

• }
• spin_unlock(&pfdev->as_lock);

+}

void panfrost_mmu_reset(struct panfrost_device *pfdev) {

• panfrost_mmu_enable(pfdev, 0);

• struct drm_file *file;

• mutex_lock(&pfdev->ddev->filelist_mutex);

• list_for_each_entry(file, &pfdev->ddev->filelist, lhead) {

• struct panfrost_file_priv *priv = file->driver_priv;
  

• panfrost_mmu_as_reset(pfdev, &priv->mmu);
  

• }

• mutex_unlock(&pfdev->ddev->filelist_mutex);

  mmu_write(pfdev, MMU_INT_CLEAR, ~0); mmu_write(pfdev, MMU_INT_MASK, ~0);

@@ -152,21 +229,21 @@ static size_t get_pgsize(u64 addr, size_t size) return SZ_2M; }

-static int mmu_map_sg(struct panfrost_device *pfdev, u64 iova,

•       int prot, struct sg_table *sgt)
  

+static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,

•       u64 iova, int prot, struct sg_table *sgt)
  

{ unsigned int count; struct scatterlist *sgl;

• struct io_pgtable_ops *ops = pfdev->mmu->pgtbl_ops;

• struct io_pgtable_ops *ops = mmu->pgtbl_ops; u64 start_iova = iova;

• mutex_lock(&pfdev->mmu->lock);

• 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);

• dev_dbg(pfdev->dev, "map: iova=%llx, paddr=%lx, len=%zx", iova, paddr, len);
  

• dev_dbg(pfdev->dev, "map: as=%d, iova=%llx, paddr=%lx, len=%zx", mmu->as, iova, paddr, len);
  
  

  while (len) { size_t pgsize = get_pgsize(iova | paddr, len);

@@ -178,10 +255,10 @@ static int mmu_map_sg(struct panfrost_device *pfdev, u64 iova, } }

• mmu_hw_do_operation(pfdev, 0, start_iova, iova - start_iova,

• mmu_hw_do_operation(pfdev, mmu->as, start_iova, iova - start_iova, AS_COMMAND_FLUSH_PT);

• mutex_unlock(&pfdev->mmu->lock);

• mutex_unlock(&mmu->lock);

  return 0;

} @@ -208,7 +285,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo) if (ret < 0) return ret;

• mmu_map_sg(pfdev, bo->node.start << PAGE_SHIFT, prot, sgt);

• 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);

@@ -221,7 +298,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo) { struct drm_gem_object *obj = &bo->base.base; struct panfrost_device *pfdev = to_panfrost_device(obj->dev);

• struct io_pgtable_ops *ops = pfdev->mmu->pgtbl_ops;

• struct io_pgtable_ops *ops = bo->mmu->pgtbl_ops; u64 iova = bo->node.start << PAGE_SHIFT; size_t len = bo->node.size << PAGE_SHIFT; size_t unmapped_len = 0;

@@ -230,13 +307,13 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo) if (WARN_ON(!bo->is_mapped)) return;

• dev_dbg(pfdev->dev, "unmap: iova=%llx, len=%zx", iova, len);

• 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(&pfdev->mmu->lock);

• mutex_lock(&bo->mmu->lock);

  while (unmapped_len < len) { size_t unmapped_page;

@@ -250,10 +327,10 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo) unmapped_len += pgsize; }

• mmu_hw_do_operation(pfdev, 0, bo->node.start << PAGE_SHIFT,

• mmu_hw_do_operation(pfdev, bo->mmu->as, bo->node.start << PAGE_SHIFT, bo->node.size << PAGE_SHIFT, AS_COMMAND_FLUSH_PT);

• mutex_unlock(&pfdev->mmu->lock);

• mutex_unlock(&bo->mmu->lock);

  pm_runtime_mark_last_busy(pfdev->dev); pm_runtime_put_autosuspend(pfdev->dev);

@@ -262,9 +339,10 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)

static void mmu_tlb_inv_context_s1(void *cookie) {

• struct panfrost_device *pfdev = cookie;

• struct panfrost_file_priv *priv = cookie;

• mmu_hw_do_operation(pfdev, 0, 0, ~0UL, AS_COMMAND_FLUSH_MEM);

• /* Only flush if we have an assigned AS */

I'm not sure if this comment makes sense here - seems like it belongs in mmu_hw_do_operation()

• mmu_hw_do_operation(priv->pfdev, priv->mmu.as, 0, ~0UL, AS_COMMAND_FLUSH_MEM);

}

static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size, @@ -283,16 +361,69 @@ static const struct iommu_gather_ops mmu_tlb_ops = { .tlb_sync = mmu_tlb_sync_context, };

+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;
• mmu->pgtbl_cfg = (struct io_pgtable_cfg) {

• .pgsize_bitmap	= SZ_4K | SZ_2M,
  

• .ias		= FIELD_GET(0xff, pfdev->features.mmu_features),
  

• .oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
  

• .tlb		= &mmu_tlb_ops,
  

• .iommu_dev	= pfdev->dev,
  

• };
• mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,

• 			      priv);
  

• if (!mmu->pgtbl_ops)

• return -EINVAL;
  

• return 0;

+}

+void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv) +{

• struct panfrost_device *pfdev = priv->pfdev;
• struct panfrost_mmu *mmu = &priv->mmu;
• spin_lock(&pfdev->as_lock);
• if (mmu->as >= 0) {

• clear_bit(mmu->as, &pfdev->as_alloc_mask);
  

• clear_bit(mmu->as, &pfdev->as_in_use_mask);
  

• list_del(&mmu->list);
  

• }
• spin_unlock(&pfdev->as_lock);
• free_io_pgtable_ops(mmu->pgtbl_ops);

+}

static struct drm_mm_node *addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr) {

• struct drm_mm_node *node;

• struct drm_mm_node *node = NULL; u64 offset = addr >> PAGE_SHIFT;
• struct drm_file *file;

• drm_mm_for_each_node(node, &pfdev->mm) {

• if (offset >= node->start && offset < (node->start + node->size))
  

• 	return node;
  

• mutex_lock(&pfdev->ddev->filelist_mutex);
• list_for_each_entry(file, &pfdev->ddev->filelist, lhead) {

You could walk as_lru_list rather than every file as you know you are looking for a fd which is assigned an address space.

• struct panfrost_file_priv *priv = file->driver_priv;
  

• if (as != priv->mmu.as)
  

• 	continue;
  

• drm_mm_for_each_node(node, &priv->mm) {
  

• 	if (offset >= node->start && offset < (node->start + node->size))
  

• 		goto out;
  

• }
  

  }

• return NULL;

+out:

• mutex_unlock(&pfdev->ddev->filelist_mutex);
• return node;

}

#define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE) @@ -317,6 +448,8 @@ int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, u64 addr) node->start << PAGE_SHIFT); return -EINVAL; }

• WARN_ON(bo->mmu->as == -1);

You could strengthen this to WARN_ON(bo->mmu->as != as)

• /* Assume 2MB alignment and size multiple */ addr &= ~((u64)SZ_2M - 1); page_offset = addr >> PAGE_SHIFT;

@@ -367,11 +500,11 @@ int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, u64 addr) goto err_map; }

• mmu_map_sg(pfdev, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);

• mmu_map_sg(pfdev, bo->mmu, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);

  bo->is_mapped = true;

• dev_dbg(pfdev->dev, "mapped page fault @ %llx", addr);

• dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", bo->mmu->as, addr);

Assuming the above WARN_ON strengthing then: s/bo->mmu->as/as/

Steve

return 0;

@@ -480,15 +613,8 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)

int panfrost_mmu_init(struct panfrost_device *pfdev) {

• struct io_pgtable_ops *pgtbl_ops; int err, irq;

• pfdev->mmu = devm_kzalloc(pfdev->dev, sizeof(*pfdev->mmu), GFP_KERNEL);

• if (!pfdev->mmu)

• return -ENOMEM;
  

• mutex_init(&pfdev->mmu->lock);

• irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "mmu"); if (irq <= 0) return -ENODEV;

@@ -501,22 +627,6 @@ int panfrost_mmu_init(struct panfrost_device *pfdev) dev_err(pfdev->dev, "failed to request mmu irq"); return err; }

• pfdev->mmu->pgtbl_cfg = (struct io_pgtable_cfg) {

• .pgsize_bitmap	= SZ_4K | SZ_2M,
  

• .ias		= FIELD_GET(0xff, pfdev->features.mmu_features),
  

• .oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
  

• .tlb		= &mmu_tlb_ops,
  

• .iommu_dev	= pfdev->dev,
  

• };

• pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &pfdev->mmu->pgtbl_cfg,

• 			 pfdev);
  

• if (!pgtbl_ops)

• return -ENOMEM;
  

• pfdev->mmu->pgtbl_ops = pgtbl_ops;

• panfrost_mmu_enable(pfdev, 0);

  return 0;

} @@ -525,6 +635,4 @@ void panfrost_mmu_fini(struct panfrost_device *pfdev) { mmu_write(pfdev, MMU_INT_MASK, 0); mmu_disable(pfdev, 0);

• free_io_pgtable_ops(pfdev->mmu->pgtbl_ops);

} diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h index d5f9b24537db..7c5b6775ae23 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h @@ -5,6 +5,8 @@ #define __PANFROST_MMU_H__

struct panfrost_gem_object; +struct panfrost_file_priv; +struct panfrost_mmu;

int panfrost_mmu_map(struct panfrost_gem_object *bo); void panfrost_mmu_unmap(struct panfrost_gem_object *bo); @@ -13,4 +15,10 @@ int panfrost_mmu_init(struct panfrost_device *pfdev); void panfrost_mmu_fini(struct panfrost_device *pfdev); void panfrost_mmu_reset(struct panfrost_device *pfdev);

+u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu); +void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);

+int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv); +void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv);

#endif