Re: [PATCH 1/5] drm/i915: Update object placement flags to be mutable

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

 



On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@xxxxxxxxxxxxxxx> wrote:
>
> The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
> I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
> much of our code. Introduce a new mem_flags member to hold these
> and make sure checks for these flags being set are either done
> under the object lock or with pages properly pinned. The flags
> will change during migration under the object lock.

What are the rules for the gem_object_ops flags? Like is_shrinkable?
Can't we just move these there(IOMEM vs PAGE)?

>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_internal.c  |  4 +-
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  7 +++-
>  drivers/gpu/drm/i915/gem/i915_gem_object.c    | 38 +++++++++++++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_object.h    | 14 ++-----
>  .../gpu/drm/i915/gem/i915_gem_object_types.h  | 20 +++++-----
>  drivers/gpu/drm/i915/gem/i915_gem_pages.c     |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_phys.c      |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c     | 10 +++--
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c   |  4 +-
>  .../drm/i915/gem/selftests/huge_gem_object.c  |  4 +-
>  .../gpu/drm/i915/gem/selftests/huge_pages.c   |  5 +--
>  .../drm/i915/gem/selftests/i915_gem_mman.c    |  4 +-
>  .../drm/i915/gem/selftests/i915_gem_phys.c    |  3 +-
>  14 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index ce6b664b10aa..13b217f75055 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
>                 return ERR_PTR(-ENOMEM);
>
>         drm_gem_private_object_init(&i915->drm, &obj->base, size);
> -       i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
> -                            I915_BO_ALLOC_STRUCT_PAGE);
> +       i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
> +       obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
>         /*
>          * Mark the object as volatile, such that the pages are marked as
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index d1de97e4adfd..171a21ca930c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>
>         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))
> +           !i915_gem_object_has_iomem(obj))
>                 return -ENODEV;
>
>         mmo = mmap_offset_attach(obj, mmap_type, file);
> @@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
>         if (!obj)
>                 return -ENOENT;
>
> +       err = i915_gem_object_lock_interruptible(obj, NULL);
> +       if (err)
> +               goto out_put;
>         err = __assign_mmap_offset(obj, mmap_type, offset, file);
> +       i915_gem_object_unlock(obj);
> +out_put:
>         i915_gem_object_put(obj);
>         return err;
>  }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index cf18c430d51f..07e8ff9a8aae 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
>         return obj->mm.n_placements > 1;
>  }
>
> +/**
> + * i915_gem_object_has_struct_page - Whether the object is page-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the page backing may change under the caller.
> + *
> + * Return: True if page-backed, false otherwise.
> + */
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> +       if (IS_DGFX(to_i915(obj->base.dev)) &&
> +           i915_gem_object_evictable((void __force *)obj))
> +               assert_object_held_shared(obj);
> +#endif
> +       return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
> +}
> +
> +/**
> + * i915_gem_object_has_iomem - Whether the object is iomem-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the iomem backing may change under the caller.
> + *
> + * Return: True if iomem-backed, false otherwise.
> + */
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> +       if (IS_DGFX(to_i915(obj->base.dev)) &&
> +           i915_gem_object_evictable((void __force *)obj))
> +               assert_object_held_shared(obj);
> +#endif
> +       return obj->mem_flags & I915_BO_FLAG_IOMEM;
> +}
> +
>  void i915_gem_init__objects(struct drm_i915_private *i915)
>  {
>         INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index ff59e6c640e6..23e26f6b1db9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
>  /*
>   * If more than one potential simultaneous locker, assert held.
>   */
> -static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
> +static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
>  {
>         /*
>          * Note mm list lookup is protected by
> @@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
>         return obj->ops->flags & flags;
>  }
>
> -static inline bool
> -i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> -{
> -       return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
> -}
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
>
> -static inline bool
> -i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> -{
> -       return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> -}
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
>
>  static inline bool
>  i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
> 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 2a23b77424b3..fb9ccc3f50e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -33,10 +33,9 @@ struct i915_lut_handle {
>
>  struct drm_i915_gem_object_ops {
>         unsigned int flags;
> -#define I915_GEM_OBJECT_HAS_IOMEM      BIT(1)
> -#define I915_GEM_OBJECT_IS_SHRINKABLE  BIT(2)
> -#define I915_GEM_OBJECT_IS_PROXY       BIT(3)
> -#define I915_GEM_OBJECT_NO_MMAP                BIT(4)
> +#define I915_GEM_OBJECT_IS_SHRINKABLE  BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY       BIT(2)
> +#define I915_GEM_OBJECT_NO_MMAP                BIT(3)
>
>         /* Interface between the GEM object and its backing storage.
>          * get_pages() is called once prior to the use of the associated set
> @@ -201,17 +200,18 @@ struct drm_i915_gem_object {
>         unsigned long flags;
>  #define I915_BO_ALLOC_CONTIGUOUS BIT(0)
>  #define I915_BO_ALLOC_VOLATILE   BIT(1)
> -#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
> -#define I915_BO_ALLOC_CPU_CLEAR  BIT(3)
> -#define I915_BO_ALLOC_USER       BIT(4)
> +#define I915_BO_ALLOC_CPU_CLEAR  BIT(2)
> +#define I915_BO_ALLOC_USER       BIT(3)
>  #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
>                              I915_BO_ALLOC_VOLATILE | \
> -                            I915_BO_ALLOC_STRUCT_PAGE | \
>                              I915_BO_ALLOC_CPU_CLEAR | \
>                              I915_BO_ALLOC_USER)
> -#define I915_BO_READONLY         BIT(5)
> -#define I915_TILING_QUIRK_BIT    6 /* unknown swizzling; do not release! */
> +#define I915_BO_READONLY         BIT(4)
> +#define I915_TILING_QUIRK_BIT    5 /* unknown swizzling; do not release! */
>
> +       unsigned int mem_flags:2;
> +#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
> +#define I915_BO_FLAG_IOMEM       BIT(1)
>         /*
>          * Is the object to be mapped as read-only to the GPU
>          * Only honoured if hardware has relevant pte bit
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 086005c1c7ea..f2f850e31b8e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>         int err;
>
>         if (!i915_gem_object_has_struct_page(obj) &&
> -           !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> +           !i915_gem_object_has_iomem(obj))
>                 return ERR_PTR(-ENXIO);
>
>         assert_object_held(obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index be72ad0634ba..7986612f48fa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>         intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>
>         /* We're no longer struct page backed */
> -       obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
> +       obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
>         __i915_gem_object_set_pages(obj, st, sg->length);
>
>         return 0;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 5d16c4462fda..3648ae1d6628 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>                                 bool needs_clflush)
>  {
>         GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
> +       GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>
>         if (obj->mm.madv == I915_MADV_DONTNEED)
>                 obj->mm.dirty = false;
> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
>         struct pagevec pvec;
>         struct page *page;
>
> +       GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>         __i915_gem_object_release_shmem(obj, pages, true);
>
>         i915_gem_gtt_finish_pages(obj, pages);
> @@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
>
>  static void shmem_release(struct drm_i915_gem_object *obj)
>  {
> -       if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
> +       if (i915_gem_object_has_struct_page(obj))
>                 i915_gem_object_release_memory_region(obj);
>
>         fput(obj->base.filp);
> @@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
>         mapping_set_gfp_mask(mapping, mask);
>         GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>
> -       i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
> -                            I915_BO_ALLOC_STRUCT_PAGE);
> -
> +       i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
> +       obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>         obj->write_domain = I915_GEM_DOMAIN_CPU;
>         obj->read_domains = I915_GEM_DOMAIN_CPU;
>
> @@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
>         resource_size_t offset;
>         int err;
>
> +       GEM_WARN_ON(IS_DGFX(dev_priv));
>         obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
>         if (IS_ERR(obj))
>                 return obj;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 3748098b42d5..ae12a2be11a2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>
>  const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>         .name = "i915_gem_object_ttm",
> -       .flags = I915_GEM_OBJECT_HAS_IOMEM,
>
>         .get_pages = i915_ttm_get_pages,
>         .put_pages = i915_ttm_put_pages,
> @@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>         i915_gem_object_init_memory_region(obj, mem);
>         i915_gem_object_make_unshrinkable(obj);
>         obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
> +       obj->mem_flags |= I915_BO_FLAG_IOMEM;
>         i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
>         INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
>         mutex_init(&obj->ttm.get_io_page.lock);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 602f0ed983ec..41dfcb75f05b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>                 return -ENOMEM;
>
>         drm_gem_private_object_init(dev, &obj->base, args->user_size);
> -       i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
> -                            I915_BO_ALLOC_STRUCT_PAGE);
> +       i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
> +       obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
>         obj->read_domains = I915_GEM_DOMAIN_CPU;
>         obj->write_domain = I915_GEM_DOMAIN_CPU;
>         i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> index 0c8ecfdf5405..f963b8e1e37b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> @@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
>                 return ERR_PTR(-ENOMEM);
>
>         drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
> -       i915_gem_object_init(obj, &huge_ops, &lock_class,
> -                            I915_BO_ALLOC_STRUCT_PAGE);
> +       i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
> +       obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
>         obj->read_domains = I915_GEM_DOMAIN_CPU;
>         obj->write_domain = I915_GEM_DOMAIN_CPU;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index dadd485bc52f..ccc67ed1a84b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
>                 return ERR_PTR(-ENOMEM);
>
>         drm_gem_private_object_init(&i915->drm, &obj->base, size);
> -       i915_gem_object_init(obj, &huge_page_ops, &lock_class,
> -                            I915_BO_ALLOC_STRUCT_PAGE);
> -
> +       i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
> +       obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>         i915_gem_object_set_volatile(obj);
>
>         obj->write_domain = I915_GEM_DOMAIN_CPU;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index ca69a29b7f2a..bfb35270a1f0 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
>
>         if (type != I915_MMAP_TYPE_GTT &&
>             !i915_gem_object_has_struct_page(obj) &&
> -           !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> +           !i915_gem_object_has_iomem(obj))
>                 return false;
>
>         return true;
> @@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
>  static bool can_access(const struct drm_i915_gem_object *obj)
>  {
>         return i915_gem_object_has_struct_page(obj) ||
> -              i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> +              i915_gem_object_has_iomem(obj);
>  }
>
>  static int __igt_mmap_access(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> index 3a6ce87f8b52..d43d8dae0f69 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> @@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
>                 goto out;
>         }
>
> +       i915_gem_object_lock(obj, NULL);
>         if (!i915_gem_object_has_struct_page(obj)) {
> +               i915_gem_object_unlock(obj);
>                 err = -EINVAL;
>                 pr_err("shmem has no struct page\n");
>                 goto out_obj;
>         }
>
> -       i915_gem_object_lock(obj, NULL);
>         err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
>         i915_gem_object_unlock(obj);
>         if (err) {
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux