[Intel-gfx] [PATCH 1/4] drm/i915: Allow i915 to manage the vma offset nodes instead of drm core

[Chris Wilson]

Quoting Abdiel Janulgue (2019-11-15 11:45:46)
> -static int create_mmap_offset(struct drm_i915_gem_object *obj)
> +void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
> +{
> +       struct i915_mmap_offset *mmo;
> +
> +       mutex_lock(&obj->mmo_lock);
> +       list_for_each_entry(mmo, &obj->mmap_offsets, offset) {
> +               /* vma_node_unmap for GTT mmaps handled already in
> +                * __i915_gem_object_release_mmap_gtt
> +                */
> +               if (mmo->mmap_type != I915_MMAP_TYPE_GTT)

Tempted to say always do it, but that would be a waste indeed.

> +                       drm_vma_node_unmap(&mmo->vma_node,
> +                                          obj->base.dev->anon_inode->i_mapping);
> +       }
> +       mutex_unlock(&obj->mmo_lock);
> +}

> +void i915_mmap_offset_destroy(struct i915_mmap_offset *mmo, struct mutex *mutex)
> +{
> +       if (mmo->file)
> +               drm_vma_node_revoke(&mmo->vma_node, mmo->file);

Wait a sec.

The mmo is global, one per object per type. Not one per object per type
per client.

We shouldn't be associated with a single mmo->file. That is enough
address space magnification for a single process to be able to exhaust
the entire global address space...

How's this meant to work?

> @@ -118,6 +132,11 @@ struct drm_i915_gem_object {
>         unsigned int userfault_count;
>         struct list_head userfault_link;
>  
> +       /* Protects access to mmap offsets */
> +       struct mutex mmo_lock;
> +       struct list_head mmap_offsets;
> +       bool readonly:1;

Go on, steal a bit from flags.
-Chris