[Intel-gfx] [PATCH 01/19] drm/i915: introduce simple gemfs
Chris Wilson
chris at chris-wilson.co.uk
Wed Jun 21 21:19:12 UTC 2017
Quoting Matthew Auld (2017-06-21 21:33:27)
> 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.
>
> v2: various improvements suggested by Joonas
>
> 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>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> drivers/gpu/drm/i915/i915_gem.c | 44 ++++++++-
> drivers/gpu/drm/i915/i915_gemfs.c | 114 +++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gemfs.h | 40 ++++++++
> drivers/gpu/drm/i915/selftests/mock_gem_device.c | 10 +-
> 6 files changed, 208 insertions(+), 4 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/i915_gemfs.c
> create mode 100644 drivers/gpu/drm/i915/i915_gemfs.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f8227318dcaf..29e3cfdf56ce 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -46,6 +46,7 @@ i915-y += i915_cmd_parser.o \
> i915_gem_tiling.o \
> i915_gem_timeline.o \
> i915_gem_userptr.o \
> + i915_gemfs.o \
> i915_trace_points.o \
> i915_vma.o \
> intel_breadcrumbs.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 30e89456fc61..376cd93a973a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2248,6 +2248,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;
You don't Yhink this might be better off in drm_i915_private.mm for the
time being?
> +static int i915_drm_gem_object_init(struct drm_device *dev,
> + struct drm_gem_object *obj,
> + size_t size)
> +{
> + struct drm_i915_private *i915 = to_i915(dev);
> + struct file *filp;
> +
> + drm_gem_private_object_init(dev, obj, size);
> +
> + filp = i915_gemfs_file_setup(i915, "i915 mm object", size);
I'm betting spaces aren't expected. i915_gem_object or just i915?
> + if (IS_ERR(filp))
> + return PTR_ERR(filp);
> +
> + obj->filp = filp;
> +
> + return 0;
> +}
> +
> +static void i915_drm_gem_object_release(struct drm_gem_object *obj)
> +{
> + if (obj->filp)
> + i915_gemfs_unlink(obj->filp);
> +
> + drm_gem_object_release(obj);
drm_gem_object_release() does fput() as one expects, but you add an
unlink. Why? Because of a lack of control over shmem_file_setup.
> +}
> +
> struct drm_i915_gem_object *
> i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
> {
> @@ -4331,7 +4358,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);
This is in dire need of a naming overhaul. (Something like
i915_gem_object_create_shmem and caried through.) Not your problem.
Although i915_drm_gem is very, very odd and should not survive much
longer. (Odd because we have a lot of drm_i915_gem precedence.)
> if (ret)
> goto fail;
>
> @@ -4449,7 +4476,8 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> drm_prime_gem_destroy(&obj->base, NULL);
>
> reservation_object_fini(&obj->__builtin_resv);
> - drm_gem_object_release(&obj->base);
> +
> + i915_drm_gem_object_release(&obj->base);
> i915_gem_info_remove_obj(i915, obj->base.size);
>
> kfree(obj->bit_17);
> +static const struct dentry_operations anon_ops = {
> + .d_dname = simple_dname
> +};
> +
> +int i915_gemfs_init(struct drm_i915_private *i915)
> +{
> + struct file_system_type *type;
> + struct vfsmount *gemfs;
> +
> + type = get_fs_type("tmpfs");
> + if (!type)
> + return -ENODEV;
> +
> + gemfs = kern_mount(type);
> + if (IS_ERR(gemfs))
> + return PTR_ERR(gemfs);
> +
> + i915->gemfs = gemfs;
> +
> + return 0;
> +}
> +
> +void i915_gemfs_fini(struct drm_i915_private *i915)
> +{
> + kern_unmount(i915->gemfs);
> + i915->gemfs = NULL;
Why the nullify?
> +}
> +
> +struct file *i915_gemfs_file_setup(struct drm_i915_private *i915,
> + const char *name, size_t size)
> +{
> + struct super_block *sb = i915->gemfs->mnt_sb;
> + struct inode *dir = d_inode(sb->s_root);
> + struct inode *inode;
> + struct path path;
> + struct qstr this;
> + struct file *res;
> + int ret;
> +
> + if (size < 0 || size > MAX_LFS_FILESIZE)
> + return ERR_PTR(-EINVAL);
You will want to run through smatch. (size < 0 can never be true.)
> +
> + this.name = name;
> + this.len = strlen(name);
> + this.hash = 0;
> +
> + path.mnt = mntget(i915->gemfs);
> + path.dentry = d_alloc_pseudo(sb, &this);
> + if (!path.dentry) {
> + res = ERR_PTR(-ENOMEM);
> + goto put_path;
> + }
> + d_set_d_op(path.dentry, &anon_ops);
> +
> + ret = dir->i_op->create(dir, path.dentry, S_IFREG | S_IRWXUGO, false);
> + if (ret) {
> + res = ERR_PTR(ret);
> + goto put_path;
> + }
Ah, so this leaves it linked. How about just dropping the link right
away? And petition for shmem_file_setup() to take the super_block.
Hmm, I think that will be better from the start.
-Chris
More information about the Intel-gfx
mailing list