Re: [RFC PATCH] drm/panfrost: Add support for mapping BOs on GPU page faults

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 12, 2019 at 02:54:56PM +0200, Tomeu Vizoso wrote:
> On Mon, 10 Jun 2019 at 19:06, Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > The midgard/bifrost GPUs need to allocate GPU memory which is allocated
> > on GPU page faults and not pinned in memory. The vendor driver calls
> > this functionality GROW_ON_GPF.
> >
> > This implementation assumes that BOs allocated with the
> > PANFROST_BO_NOMAP flag are never mmapped or exported. Both of those may
> > actually work, but I'm unsure if there's some interaction there. It
> > would cause the whole object to be pinned in memory which would defeat
> > the point of this.
> >
> > Issues/questions/thoughts:
> >
> > What's the difference between i_mapping and f_mapping?
> >
> > What kind of clean-up on close is needed? Based on vgem faults, there
> > doesn't seem to be any refcounting. Assume userspace is responsible for
> > not freeing the BO while a page fault can occur?

This really shouldn't ever be possible, "rely on userspace to not oops the
kernel" is how dri1 worked :-)
> 
> Aren't we taking a reference on all BOs that a job relates to and
> unreferencing them once the job is done? I would think that that's
> enough, or am I missing something?

Yup, this is how this is supposed to work.
-Daniel

> 
> > What about evictions? Need to call mapping_set_unevictable()? Maybe we
> > want these pages to be swappable, but then we need some notification to
> > unmap them.
> 
> I'm not sure there's much point in swapping out pages with lifetimes
> of a few milliseconds.
> 
> > No need for runtime PM, because faults should only occur during job
> > submit?
> 
> Makes sense to me.
> 
> > Need to fault in 2MB sections when possible. It seems this has to be
> > done by getting an array of struct pages and then converting to a
> > scatterlist. Not sure if there is a better way to do that. There's also
> > some errata on T604 needing to fault in at least 8 pages at a time which
> > isn't handled.
> 
> The old GPUs will be the most interesting to support...
> 
> Will give this patch some testing tomorrow.
> 
> Thanks,
> 
> Tomeu
> 
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Steven Price <steven.price@xxxxxxx>
> > Cc: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
> > Cc: Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> >
> >  drivers/gpu/drm/panfrost/TODO           |  2 -
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 22 +++++--
> >  drivers/gpu/drm/panfrost/panfrost_gem.c |  3 +-
> >  drivers/gpu/drm/panfrost/panfrost_gem.h |  8 +++
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c | 76 +++++++++++++++++++++++--
> >  include/uapi/drm/panfrost_drm.h         |  2 +
> >  6 files changed, 100 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/TODO b/drivers/gpu/drm/panfrost/TODO
> > index c2e44add37d8..64129bf73933 100644
> > --- a/drivers/gpu/drm/panfrost/TODO
> > +++ b/drivers/gpu/drm/panfrost/TODO
> > @@ -14,8 +14,6 @@
> >    The hard part is handling when more address spaces are needed than what
> >    the h/w provides.
> >
> > -- Support pinning pages on demand (GPU page faults).
> > -
> >  - Support userspace controlled GPU virtual addresses. Needed for Vulkan. (Tomeu)
> >
> >  - Support for madvise and a shrinker.
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index d11e2281dde6..f08d41d5186a 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -44,9 +44,11 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> >  {
> >         int ret;
> >         struct drm_gem_shmem_object *shmem;
> > +       struct panfrost_gem_object *bo;
> >         struct drm_panfrost_create_bo *args = data;
> >
> > -       if (!args->size || args->flags || args->pad)
> > +       if (!args->size || args->pad ||
> > +           (args->flags & ~PANFROST_BO_NOMAP))
> >                 return -EINVAL;
> >
> >         shmem = drm_gem_shmem_create_with_handle(file, dev, args->size,
> > @@ -54,11 +56,16 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> >         if (IS_ERR(shmem))
> >                 return PTR_ERR(shmem);
> >
> > -       ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));
> > -       if (ret)
> > -               goto err_free;
> > +       bo = to_panfrost_bo(&shmem->base);
> >
> > -       args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;
> > +       if (!(args->flags & PANFROST_BO_NOMAP)) {
> > +               ret = panfrost_mmu_map(bo);
> > +               if (ret)
> > +                       goto err_free;
> > +       } else
> > +               bo->no_map = 1;
> > +
> > +       args->offset = bo->node.start << PAGE_SHIFT;
> >
> >         return 0;
> >
> > @@ -269,6 +276,11 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
> >                 return -ENOENT;
> >         }
> >
> > +       if (to_panfrost_bo(gem_obj)->no_map) {
> > +               DRM_DEBUG("Can't mmap 'no_map' GEM BO %d\n", args->handle);
> > +               return -EINVAL;
> > +       }
> > +
> >         ret = drm_gem_create_mmap_offset(gem_obj);
> >         if (ret == 0)
> >                 args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index 886875ae31d3..ed0b4f79610a 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -19,7 +19,8 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> >         struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> >         struct panfrost_device *pfdev = obj->dev->dev_private;
> >
> > -       panfrost_mmu_unmap(bo);
> > +       if (!bo->no_map)
> > +               panfrost_mmu_unmap(bo);
> >
> >         spin_lock(&pfdev->mm_lock);
> >         drm_mm_remove_node(&bo->node);
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > index 045000eb5fcf..aa210d7cbf3f 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > @@ -11,6 +11,7 @@ struct panfrost_gem_object {
> >         struct drm_gem_shmem_object base;
> >
> >         struct drm_mm_node node;
> > +       unsigned no_map: 1;
> >  };
> >
> >  static inline
> > @@ -19,6 +20,13 @@ struct  panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
> >         return container_of(to_drm_gem_shmem_obj(obj), struct panfrost_gem_object, base);
> >  }
> >
> > +static inline
> > +struct  panfrost_gem_object *drm_mm_node_to_panfrost_bo(struct drm_mm_node *node)
> > +{
> > +       return container_of(node, struct panfrost_gem_object, node);
> > +}
> > +
> > +
> >  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
> >
> >  struct drm_gem_object *
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index 41d184fab07f..5a36495a38f3 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -3,6 +3,7 @@
> >  /* Copyright (C) 2019 Arm Ltd. */
> >  #include <linux/bitfield.h>
> >  #include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> > @@ -10,6 +11,7 @@
> >  #include <linux/iommu.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/shmem_fs.h>
> >  #include <linux/sizes.h>
> >
> >  #include "panfrost_device.h"
> > @@ -277,6 +279,60 @@ static const struct iommu_gather_ops mmu_tlb_ops = {
> >         .tlb_sync       = mmu_tlb_sync_context,
> >  };
> >
> > +static struct drm_mm_node *addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
> > +{
> > +       struct drm_mm_node *node;
> > +       u64 offset = addr >> PAGE_SHIFT;
> > +
> > +       drm_mm_for_each_node(node, &pfdev->mm) {
> > +               if (offset >= node->start && offset < (node->start + node->size))
> > +                       return node;
> > +       }
> > +       return NULL;
> > +}
> > +
> > +int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, u64 addr)
> > +{
> > +       struct page *page;
> > +       dma_addr_t dma_addr;
> > +       struct drm_mm_node *node;
> > +       struct panfrost_gem_object *bo;
> > +       struct io_pgtable_ops *ops = pfdev->mmu->pgtbl_ops;
> > +       pgoff_t page_offset = addr >> PAGE_SHIFT;
> > +
> > +       addr &= PAGE_MASK;
> > +
> > +       node = addr_to_drm_mm_node(pfdev, as, addr);
> > +       if (!node)
> > +               return -ENOENT;
> > +
> > +       page_offset -= node->start;
> > +       bo = drm_mm_node_to_panfrost_bo(node);
> > +       if (WARN_ON(!bo->no_map))
> > +               return -EINVAL;
> > +
> > +       dev_info(pfdev->dev, "mapping page fault @ %llx", addr);
> > +
> > +       page = shmem_read_mapping_page(bo->base.base.filp->f_mapping,
> > +                                      page_offset);
> > +       if (IS_ERR(page))
> > +               return PTR_ERR(page);
> > +
> > +       dma_addr = dma_map_page(pfdev->dev, page, 0, PAGE_SIZE, DMA_BIDIRECTIONAL);
> > +
> > +       mutex_lock(&pfdev->mmu->lock);
> > +
> > +       ops->map(ops, addr, dma_addr, PAGE_SIZE, IOMMU_WRITE | IOMMU_READ);
> > +
> > +       mmu_write(pfdev, MMU_INT_CLEAR, BIT(as));
> > +
> > +       mmu_hw_do_operation(pfdev, as, addr, PAGE_SIZE, AS_COMMAND_FLUSH_PT);
> > +
> > +       mutex_unlock(&pfdev->mmu->lock);
> > +
> > +       return 0;
> > +}
> > +
> >  static const char *access_type_name(struct panfrost_device *pfdev,
> >                 u32 fault_status)
> >  {
> > @@ -302,13 +358,11 @@ static irqreturn_t panfrost_mmu_irq_handler(int irq, void *data)
> >  {
> >         struct panfrost_device *pfdev = data;
> >         u32 status = mmu_read(pfdev, MMU_INT_STAT);
> > -       int i;
> > +       int i, ret;
> >
> >         if (!status)
> >                 return IRQ_NONE;
> >
> > -       dev_err(pfdev->dev, "mmu irq status=%x\n", status);
> > -
> >         for (i = 0; status; i++) {
> >                 u32 mask = BIT(i) | BIT(i + 16);
> >                 u64 addr;
> > @@ -329,6 +383,17 @@ static irqreturn_t panfrost_mmu_irq_handler(int irq, void *data)
> >                 access_type = (fault_status >> 8) & 0x3;
> >                 source_id = (fault_status >> 16);
> >
> > +               /* Page fault only */
> > +               if ((status & mask) == BIT(i)) {
> > +                       WARN_ON(exception_type < 0xC1 || exception_type > 0xC4);
> > +
> > +                       ret = panfrost_mmu_map_fault_addr(pfdev, i, addr);
> > +                       if (!ret) {
> > +                               status &= ~mask;
> > +                               continue;
> > +                       }
> > +               }
> > +
> >                 /* terminal fault, print info about the fault */
> >                 dev_err(pfdev->dev,
> >                         "Unhandled Page fault in AS%d at VA 0x%016llX\n"
> > @@ -370,8 +435,9 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
> >         if (irq <= 0)
> >                 return -ENODEV;
> >
> > -       err = devm_request_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
> > -                              IRQF_SHARED, "mmu", pfdev);
> > +       err = devm_request_threaded_irq(pfdev->dev, irq, NULL,
> > +                                       panfrost_mmu_irq_handler,
> > +                                       IRQF_ONESHOT, "mmu", pfdev);
> >
> >         if (err) {
> >                 dev_err(pfdev->dev, "failed to request mmu irq");
> > diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> > index a52e0283b90d..6d83baa0e4c1 100644
> > --- a/include/uapi/drm/panfrost_drm.h
> > +++ b/include/uapi/drm/panfrost_drm.h
> > @@ -71,6 +71,8 @@ struct drm_panfrost_wait_bo {
> >         __s64 timeout_ns;       /* absolute */
> >  };
> >
> > +#define PANFROST_BO_NOMAP      1
> > +
> >  /**
> >   * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
> >   *
> > --
> > 2.20.1
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux