Re: [PATCH 01/15] drm/i915: really simple gemfs

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@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?

> +	if (IS_ERR(res))
> +		goto unlink;
> +
> +	return res;
> +
> +unlink:
> +	dir->i_op->unlink(dir, path.dentry);
> +put_path:
> +	path_put(&path);

Might throw newline here.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx