Re: [Intel-gfx] [PATCH v4 15/15] drm/i915: Use ttm mmap handling for ttm bo's.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 2021-05-26 om 19:40 schreef Thomas Hellström:
>
> On 5/26/21 1:32 PM, Thomas Hellström wrote:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> Use the ttm handlers for servicing page faults, and vm_access.
>>
>> We do our own validation of read-only access, otherwise use the
>> ttm handlers as much as possible.
>>
>> Because the ttm handlers expect the vma_node at vma->base, we slightly
>> need to massage the mmap handlers to look at vma_node->driver_private
>> to fetch the bo, if it's NULL, we assume i915's normal mmap_offset uapi
>> is used.
>>
>> This is the easiest way to achieve compatibility without changing ttm's
>> semantics.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  78 +++++++----
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |   6 +-
>>   .../gpu/drm/i915/gem/i915_gem_object_types.h  |   3 +
>>   drivers/gpu/drm/i915/gem/i915_gem_pages.c     |   3 +-
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 122 +++++++++++++++++-
>>   .../drm/i915/gem/selftests/i915_gem_mman.c    |  90 +++++++------
>>   drivers/gpu/drm/i915/selftests/igt_mmap.c     |  25 +++-
>>   drivers/gpu/drm/i915/selftests/igt_mmap.h     |  12 +-
>>   8 files changed, 247 insertions(+), 92 deletions(-)
>
> There are a couple of checkpatch.pl --strict warnings/checks with this patch.
>
>
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index fd1c9714f8d8..af04ea593091 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -19,6 +19,7 @@
>>   #include "i915_gem_mman.h"
>>   #include "i915_trace.h"
>>   #include "i915_user_extensions.h"
>> +#include "i915_gem_ttm.h"
>>   #include "i915_vma.h"
>>     static inline bool
>> @@ -622,6 +623,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>       struct i915_mmap_offset *mmo;
>>       int err;
>>   +    GEM_BUG_ON(obj->ops->mmap_offset || obj->ops->mmap_ops);
>> +
>>       mmo = lookup_mmo(obj, mmap_type);
>>       if (mmo)
>>           goto out;
>> @@ -664,40 +667,47 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>   }
>>     static int
>> -__assign_mmap_offset(struct drm_file *file,
>> -             u32 handle,
>> +__assign_mmap_offset(struct drm_i915_gem_object *obj,
>>                enum i915_mmap_type mmap_type,
>> -             u64 *offset)
>> +             u64 *offset, struct drm_file *file)
>>   {
>> -    struct drm_i915_gem_object *obj;
>>       struct i915_mmap_offset *mmo;
>> -    int err;
>>   -    obj = i915_gem_object_lookup(file, handle);
>> -    if (!obj)
>> -        return -ENOENT;
>> +    if (i915_gem_object_never_mmap(obj))
>> +        return -ENODEV;
>>   -    if (i915_gem_object_never_mmap(obj)) {
>> -        err = -ENODEV;
>> -        goto out;
>> +    if (obj->ops->mmap_offset)  {
>> +        *offset = obj->ops->mmap_offset(obj);
>> +        return 0;
>>       }
>>         if (mmap_type != I915_MMAP_TYPE_GTT &&
>>           !i915_gem_object_has_struct_page(obj) &&
>> -        !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM)) {
>> -        err = -ENODEV;
>> -        goto out;
>> -    }
>> +        !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> +        return -ENODEV;
>>         mmo = mmap_offset_attach(obj, mmap_type, file);
>> -    if (IS_ERR(mmo)) {
>> -        err = PTR_ERR(mmo);
>> -        goto out;
>> -    }
>> +    if (IS_ERR(mmo))
>> +        return PTR_ERR(mmo);
>>         *offset = drm_vma_node_offset_addr(&mmo->vma_node);
>> -    err = 0;
>> -out:
>> +    return 0;
>> +}
>> +
>> +static int
>> +__assign_mmap_offset_handle(struct drm_file *file,
>> +                u32 handle,
>> +                enum i915_mmap_type mmap_type,
>> +                u64 *offset)
>> +{
>> +    struct drm_i915_gem_object *obj;
>> +    int err;
>> +
>> +    obj = i915_gem_object_lookup(file, handle);
>> +    if (!obj)
>> +        return -ENOENT;
>> +
>> +    err = __assign_mmap_offset(obj, mmap_type, offset, file);
>>       i915_gem_object_put(obj);
>>       return err;
>>   }
>> @@ -717,7 +727,7 @@ i915_gem_dumb_mmap_offset(struct drm_file *file,
>>       else
>>           mmap_type = I915_MMAP_TYPE_GTT;
>>   -    return __assign_mmap_offset(file, handle, mmap_type, offset);
>> +    return __assign_mmap_offset_handle(file, handle, mmap_type, offset);
>>   }
>>     /**
>> @@ -785,7 +795,7 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
>>           return -EINVAL;
>>       }
>>   -    return __assign_mmap_offset(file, args->handle, type, &args->offset);
>> +    return __assign_mmap_offset_handle(file, args->handle, type, &args->offset);
>>   }
>>     static void vm_open(struct vm_area_struct *vma)
>> @@ -889,8 +899,16 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>            * destroyed and will be invalid when the vma manager lock
>>            * is released.
>>            */
>> -        mmo = container_of(node, struct i915_mmap_offset, vma_node);
>> -        obj = i915_gem_object_get_rcu(mmo->obj);
>> +        if (!node->driver_private) {
>> +            mmo = container_of(node, struct i915_mmap_offset, vma_node);
>> +            obj = i915_gem_object_get_rcu(mmo->obj);
>> +
>> +            GEM_BUG_ON(obj && obj->ops->mmap_ops);
>> +        } else {
>> +            obj = i915_gem_object_get_rcu(container_of(node, struct drm_i915_gem_object, base.vma_node));
>> +
>> +            GEM_BUG_ON(obj && !obj->ops->mmap_ops);
>> +        }
>>       }
>>       drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>>       rcu_read_unlock();
>> @@ -912,7 +930,6 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>       }
>>         vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>> -    vma->vm_private_data = mmo;
>>         /*
>>        * We keep the ref on mmo->obj, not vm_file, but we require
>> @@ -926,6 +943,15 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>       /* Drop the initial creation reference, the vma is now holding one. */
>>       fput(anon);
>>   +    if (obj->ops->mmap_ops) {
>> +        vma->vm_page_prot = pgprot_decrypted(vm_get_page_prot(vma->vm_flags));
>> +        vma->vm_ops = obj->ops->mmap_ops;
>> +        vma->vm_private_data = node->driver_private;
>> +        return 0;
>> +    }
>> +
>> +    vma->vm_private_data = mmo;
>> +
>>       switch (mmo->mmap_type) {
>>       case I915_MMAP_TYPE_WC:
>>           vma->vm_page_prot =
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index a3ad8cf4eefd..ff59e6c640e6 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -342,14 +342,14 @@ struct scatterlist *
>>   __i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
>>                struct i915_gem_object_page_iter *iter,
>>                unsigned int n,
>> -             unsigned int *offset, bool allow_alloc);
>> +             unsigned int *offset, bool allow_alloc, bool dma);
>>     static inline struct scatterlist *
>>   i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
>>                  unsigned int n,
>>                  unsigned int *offset, bool allow_alloc)
>>   {
>> -    return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, allow_alloc);
>> +    return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, allow_alloc, false);
>>   }
>>     static inline struct scatterlist *
>> @@ -357,7 +357,7 @@ i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj,
>>                  unsigned int n,
>>                  unsigned int *offset, bool allow_alloc)
>>   {
>> -    return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, allow_alloc);
>> +    return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, allow_alloc, true);
>>   }
>>     struct page *
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> index 68313474e6a6..2a23b77424b3 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -61,6 +61,7 @@ struct drm_i915_gem_object_ops {
>>                const struct drm_i915_gem_pread *arg);
>>       int (*pwrite)(struct drm_i915_gem_object *obj,
>>                 const struct drm_i915_gem_pwrite *arg);
>> +    u64 (*mmap_offset)(struct drm_i915_gem_object *obj);
>>         int (*dmabuf_export)(struct drm_i915_gem_object *obj);
>>   @@ -79,6 +80,7 @@ struct drm_i915_gem_object_ops {
>>       void (*delayed_free)(struct drm_i915_gem_object *obj);
>>       void (*release)(struct drm_i915_gem_object *obj);
>>   +    const struct vm_operations_struct *mmap_ops;
>>       const char *name; /* friendly name for debug, e.g. lockdep classes */
>>   };
>>   @@ -328,6 +330,7 @@ struct drm_i915_gem_object {
>>         struct {
>>           struct sg_table *cached_io_st;
>> +        struct i915_gem_object_page_iter get_io_page;
>>           bool created:1;
>>       } ttm;
>>   diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> index 6444e097016d..086005c1c7ea 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> @@ -467,9 +467,8 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
>>                struct i915_gem_object_page_iter *iter,
>>                unsigned int n,
>>                unsigned int *offset,
>> -             bool allow_alloc)
>> +             bool allow_alloc, bool dma)
>>   {
>> -    const bool dma = iter == &obj->mm.get_dma_page;
>>       struct scatterlist *sg;
>>       unsigned int idx, count;
>>   diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 17598930a99e..d0be957326e0 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -13,6 +13,7 @@
>>   #include "gem/i915_gem_object.h"
>>   #include "gem/i915_gem_region.h"
>>   #include "gem/i915_gem_ttm.h"
>> +#include "gem/i915_gem_mman.h"
>>     #define I915_PL_LMEM0 TTM_PL_PRIV
>>   #define I915_PL_SYSTEM TTM_PL_SYSTEM
>> @@ -158,11 +159,20 @@ static int i915_ttm_move_notify(struct ttm_buffer_object *bo)
>>     static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
>>   {
>> -    if (obj->ttm.cached_io_st) {
>> -        sg_free_table(obj->ttm.cached_io_st);
>> -        kfree(obj->ttm.cached_io_st);
>> -        obj->ttm.cached_io_st = NULL;
>> -    }
>> +    struct radix_tree_iter iter;
>> +    void __rcu **slot;
>> +
>> +    if (!obj->ttm.cached_io_st)
>> +        return;
>> +
>> +    rcu_read_lock();
>> +    radix_tree_for_each_slot(slot, &obj->ttm.get_io_page.radix, &iter, 0)
>> +        radix_tree_delete(&obj->ttm.get_io_page.radix, iter.index);
>> +    rcu_read_unlock();
>> +
>> +    sg_free_table(obj->ttm.cached_io_st);
>> +    kfree(obj->ttm.cached_io_st);
>> +    obj->ttm.cached_io_st = NULL;
>>   }
>>     static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>> @@ -338,12 +348,42 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>>       ttm_bo_move_sync_cleanup(bo, dst_mem);
>>       i915_ttm_free_cached_io_st(obj);
>>   -    if (!dst_man->use_tt)
>> +    if (!dst_man->use_tt) {
>>           obj->ttm.cached_io_st = dst_st;
>> +        obj->ttm.get_io_page.sg_pos = dst_st->sgl;
>> +        obj->ttm.get_io_page.sg_idx = 0;
>> +    }
>>         return 0;
>>   }
>>   +static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
>> +{
>> +    if (mem->mem_type < I915_PL_LMEM0)
>> +        return 0;
>> +
>> +    /* We may need to revisit this later, but this allows all caching to be used in mmap */
>> +    mem->bus.caching = ttm_cached;
>
> Since we're now using the TTM bo offsets, we might as well just make this ttm_write_combined now.

Correct. That would be the correct value. TTM will use the correct caching that way.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx