[Intel-gfx] [PATCH 01/15] drm/i915: really simple gemfs

[Matthew Auld]

On 1 June 2017 at 11:49, Joonas Lahtinen
<joonas.lahtinen at linux.intel.com> wrote:
> On ke, 2017-05-31 at 19:51 +0100, Matthew Auld wrote:
>> Not a fully blown gemfs, just our very own tmpfs kernel mount. Doing so
>> moves us away from the shmemfs shm_mnt, and gives us the much needed
>> flexibility to do things like set our own mount options, namely huge=
>> which should allow us to enable the use of transparent-huge-pages for
>> our shmem backed objects.
>>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>
> <SNIP>
>
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2227,6 +2227,9 @@ struct drm_i915_private {
>>       DECLARE_HASHTABLE(mm_structs, 7);
>>       struct mutex mm_lock;
>>
>> +     /* Our tmpfs instance used for shmem backed objects */
>> +     struct vfsmount *gemfs_mnt;
>
> "gemfs" might be good enough, should not cause any confusion?
>
>> @@ -4169,4 +4172,14 @@ static inline bool i915_gem_object_is_coherent(struct drm_i915_gem_object *obj)
>>               HAS_LLC(to_i915(obj->base.dev)));
>>  }
>>
>> +/* i915_gemfs.c */
>
> i915_gemfs.h please. Lets not bloat i915_drv.h more when effort is in
> place to strip it down.
>
>> +struct vfsmount *i915_gemfs_create(void);
>
> Not "int gemfs_init(struct drm_i915_privat *i915)" and _fini?
>
> I doubt we should be creating more of these.
>
>> @@ -4268,7 +4286,7 @@ i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
>>       if (obj == NULL)
>>               return ERR_PTR(-ENOMEM);
>>
>> -     ret = drm_gem_object_init(&dev_priv->drm, &obj->base, size);
>> +     ret = i915_drm_gem_object_init(&dev_priv->drm, &obj->base, size);
>>       if (ret)
>>               goto fail;
>
> As Chris mentioned, smells bit like we could be targeting DRM scope in
> the future.
>
>> @@ -4383,6 +4401,9 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>>                       drm_prime_gem_destroy(&obj->base, NULL);
>>
>>               reservation_object_fini(&obj->__builtin_resv);
>> +
>
> For code below, do drop a note here that we want to do part of
> drm_gem_object_release's work in advance. Or rather than commenting,
> make it explicitly clear by having i915_gem_object_release() with this
> hunk of code.
>
>> +             if (obj->base.filp)
>> +                     i915_gemfs_unlink(obj->base.filp);
>>               drm_gem_object_release(&obj->base);
>>               i915_gem_info_remove_obj(i915, obj->base.size);
>>
>> @@ -4843,6 +4864,10 @@ i915_gem_load_init(struct drm_i915_private *dev_priv)
>>  {
>>       int err = -ENOMEM;
>>
>> +     dev_priv->gemfs_mnt = i915_gemfs_create();
>> +     if (IS_ERR(dev_priv->gemfs_mnt))
>> +             return PTR_ERR(dev_priv->gemfs_mnt);
>
>         err = i915_gemfs_init(dev_priv);
>         if (err)
>                 return err;
>
>> +
>>       dev_priv->objects = KMEM_CACHE(drm_i915_gem_object, SLAB_HWCACHE_ALIGN);
>>       if (!dev_priv->objects)
>
>                 err = -ENOMEM;
>                 goto err_gemfs;
>
>> @@ -4930,6 +4956,8 @@ void i915_gem_load_cleanup(struct drm_i915_private *dev_priv)
>>
>>       /* And ensure that our DESTROY_BY_RCU slabs are truly destroyed */
>>       rcu_barrier();
>> +
>> +     i915_gemfs_destroy(dev_priv->gemfs_mnt);
>
>         i915_gemfs_fini();
>
>> +struct file *i915_gemfs_file_setup(struct vfsmount *gemfs_mnt,
>> +                                const char *name, size_t size)
>> +{
>
> <SNIP>
>
>> +
>> +     inode = d_inode(path.dentry);
>> +     inode->i_size = size;
>> +
>> +     res = alloc_file(&path, FMODE_WRITE | FMODE_READ, inode->i_fop);
>
> shmem is passing their own fops, we don't need to? shmem_mmap seems to
> have some transparent huge page code at least which would be missed,
> no?
We pass the same fops, since shmem_file_operations == inode->i_fop.
See shmem_get_inode.

I'll try to address the other comments, thanks.