On 1 June 2017 at 11:49, Joonas Lahtinen
<joonas.lahtinen@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> <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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
