[Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)

Daniel Vetter daniel at ffwll.ch
Thu Jul 14 08:12:17 UTC 2016


On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote:
> vGEM buffers are useful for passing data between software clients and
> hardware renders. By allowing the user to create and attach fences to
> the exported vGEM buffers (on the dma-buf), the user can implement a
> deferred renderer and queue hardware operations like flipping and then
> signal the buffer readiness (i.e. this allows the user to schedule
> operations out-of-order, but have them complete in-order).
> 
> This also makes it much easier to write tightly controlled testcases for
> dma-buf fencing and signaling between hardware drivers.
> 
> v2: Don't pretend the fences exist in an ordered timeline, but allocate
> a separate fence-context for each fence so that the fences are
> unordered.
> v3: Make the debug output more interesting, and so the signaled status.
> 
> Testcase: igt/vgem_basic/dmabuf-fence
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul at chromium.org>
> Cc: Zach Reizner <zachr at google.com>
> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Acked-by: Zach Reizner <zachr at google.com>

One thing I completely forgotten: This allows userspace to hang kernel
drivers. i915 (and other gpu drivers) can recover using hangcheck, but
dumber drivers (v4l, if that ever happens) probably never except such a
case. We've had a similar discusion with the userspace fences exposed in
sw_fence, and decided to move all those ioctl into debugfs. I think we
should do the same for this vgem-based debugging of implicit sync. Sorry
for realizing this this late.
-Daniel

> ---
>  drivers/gpu/drm/vgem/Makefile     |   2 +-
>  drivers/gpu/drm/vgem/vgem_drv.c   |  34 +++++++
>  drivers/gpu/drm/vgem/vgem_drv.h   |  16 +++
>  drivers/gpu/drm/vgem/vgem_fence.c | 207 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/vgem_drm.h       |  62 ++++++++++++
>  5 files changed, 320 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
>  create mode 100644 include/uapi/drm/vgem_drm.h
> 
> diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
> index 3f4c7b842028..bfcdea1330e6 100644
> --- a/drivers/gpu/drm/vgem/Makefile
> +++ b/drivers/gpu/drm/vgem/Makefile
> @@ -1,4 +1,4 @@
>  ccflags-y := -Iinclude/drm
> -vgem-y := vgem_drv.o
> +vgem-y := vgem_drv.o vgem_fence.o
>  
>  obj-$(CONFIG_DRM_VGEM)	+= vgem.o
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 29c2aab3c1a7..c15bafb06665 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops = {
>  	.close = drm_gem_vm_close,
>  };
>  
> +static int vgem_open(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct vgem_file *vfile;
> +	int ret;
> +
> +	vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
> +	if (!vfile)
> +		return -ENOMEM;
> +
> +	file->driver_priv = vfile;
> +
> +	ret = vgem_fence_open(vfile);
> +	if (ret) {
> +		kfree(vfile);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct vgem_file *vfile = file->driver_priv;
> +
> +	vgem_fence_close(vfile);
> +	kfree(vfile);
> +}
> +
>  /* ioctls */
>  
>  static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> @@ -164,6 +192,8 @@ unref:
>  }
>  
>  static struct drm_ioctl_desc vgem_ioctls[] = {
> +	DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>  };
>  
>  static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
> @@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>  
>  static struct drm_driver vgem_driver = {
>  	.driver_features		= DRIVER_GEM | DRIVER_PRIME,
> +	.open				= vgem_open,
> +	.preclose			= vgem_preclose,
>  	.gem_free_object_unlocked	= vgem_gem_free_object,
>  	.gem_vm_ops			= &vgem_gem_vm_ops,
>  	.ioctls				= vgem_ioctls,
> +	.num_ioctls 			= ARRAY_SIZE(vgem_ioctls),
>  	.fops				= &vgem_driver_fops,
>  
>  	.dumb_create			= vgem_gem_dumb_create,
> @@ -328,5 +361,6 @@ module_init(vgem_init);
>  module_exit(vgem_exit);
>  
>  MODULE_AUTHOR("Red Hat, Inc.");
> +MODULE_AUTHOR("Intel Corporation");
>  MODULE_DESCRIPTION(DRIVER_DESC);
>  MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
> index 988cbaae7588..1f8798ad329c 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.h
> +++ b/drivers/gpu/drm/vgem/vgem_drv.h
> @@ -32,9 +32,25 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_gem.h>
>  
> +#include <uapi/drm/vgem_drm.h>
> +
> +struct vgem_file {
> +	struct idr fence_idr;
> +	struct mutex fence_mutex;
> +};
> +
>  #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
>  struct drm_vgem_gem_object {
>  	struct drm_gem_object base;
>  };
>  
> +int vgem_fence_open(struct vgem_file *file);
> +int vgem_fence_attach_ioctl(struct drm_device *dev,
> +			    void *data,
> +			    struct drm_file *file);
> +int vgem_fence_signal_ioctl(struct drm_device *dev,
> +			    void *data,
> +			    struct drm_file *file);
> +void vgem_fence_close(struct vgem_file *file);
> +
>  #endif
> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
> new file mode 100644
> index 000000000000..b7da11419ad6
> --- /dev/null
> +++ b/drivers/gpu/drm/vgem/vgem_fence.c
> @@ -0,0 +1,207 @@
> +/*
> + * Copyright 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software")
> + * to deal in the software without restriction, including without limitation
> + * on the rights to use, copy, modify, merge, publish, distribute, sub
> + * license, and/or sell copies of the Software, and to permit persons to whom
> + * them Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTIBILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES, OR OTHER LIABILITY, WHETHER
> + * IN AN ACTION OF CONTRACT, TORT, OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <linux/reservation.h>
> +
> +#include "vgem_drv.h"
> +
> +struct vgem_fence {
> +	struct fence base;
> +	struct spinlock lock;
> +};
> +
> +static const char *vgem_fence_get_driver_name(struct fence *fence)
> +{
> +	return "vgem";
> +}
> +
> +static const char *vgem_fence_get_timeline_name(struct fence *fence)
> +{
> +	return "unbound";
> +}
> +
> +static bool vgem_fence_signaled(struct fence *fence)
> +{
> +	return false;
> +}
> +
> +static bool vgem_fence_enable_signaling(struct fence *fence)
> +{
> +	return true;
> +}
> +
> +static void vgem_fence_value_str(struct fence *fence, char *str, int size)
> +{
> +	snprintf(str, size, "%u", fence->seqno);
> +}
> +
> +static void vgem_fence_timeline_value_str(struct fence *fence, char *str,
> +					  int size)
> +{
> +	snprintf(str, size, "%u", fence_is_signaled(fence) ? fence->seqno : 0);
> +}
> +
> +const struct fence_ops vgem_fence_ops = {
> +	.get_driver_name = vgem_fence_get_driver_name,
> +	.get_timeline_name = vgem_fence_get_timeline_name,
> +	.enable_signaling = vgem_fence_enable_signaling,
> +	.signaled = vgem_fence_signaled,
> +	.wait = fence_default_wait,
> +	.fence_value_str = vgem_fence_value_str,
> +	.timeline_value_str = vgem_fence_timeline_value_str,
> +};
> +
> +static struct fence *vgem_fence_create(struct vgem_file *vfile)
> +{
> +	struct vgem_fence *fence;
> +
> +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence)
> +		return NULL;
> +
> +	spin_lock_init(&fence->lock);
> +	fence_init(&fence->base, &vgem_fence_ops, &fence->lock,
> +		   fence_context_alloc(1), 1);
> +
> +	return &fence->base;
> +}
> +
> +static int attach_dmabuf(struct drm_device *dev,
> +			 struct drm_gem_object *obj)
> +{
> +	struct dma_buf *dmabuf;
> +
> +	if (obj->dma_buf)
> +		return 0;
> +
> +	dmabuf = dev->driver->gem_prime_export(dev, obj, 0);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	obj->dma_buf = dmabuf;
> +	drm_gem_object_reference(obj);
> +	return 0;
> +}
> +
> +int vgem_fence_attach_ioctl(struct drm_device *dev,
> +			    void *data,
> +			    struct drm_file *file)
> +{
> +	struct drm_vgem_fence_attach *arg = data;
> +	struct vgem_file *vfile = file->driver_priv;
> +	struct reservation_object *resv;
> +	struct drm_gem_object *obj;
> +	struct fence *fence;
> +	int ret;
> +
> +	if (arg->flags & ~VGEM_FENCE_WRITE)
> +		return -EINVAL;
> +
> +	if (arg->pad)
> +		return -EINVAL;
> +
> +	obj = drm_gem_object_lookup(file, arg->handle);
> +	if (!obj)
> +		return -ENOENT;
> +
> +	ret = attach_dmabuf(dev, obj);
> +	if (ret)
> +		goto out;
> +
> +	fence = vgem_fence_create(vfile);
> +	if (!fence) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = 0;
> +	resv = obj->dma_buf->resv;
> +	mutex_lock(&resv->lock.base);
> +	if (arg->flags & VGEM_FENCE_WRITE)
> +		reservation_object_add_excl_fence(resv, fence);
> +	else if ((ret = reservation_object_reserve_shared(resv)) == 0)
> +		reservation_object_add_shared_fence(resv, fence);
> +	mutex_unlock(&resv->lock.base);
> +
> +	if (ret == 0) {
> +		mutex_lock(&vfile->fence_mutex);
> +		ret = idr_alloc(&vfile->fence_idr, fence, 1, 0, GFP_KERNEL);
> +		mutex_unlock(&vfile->fence_mutex);
> +		if (ret > 0) {
> +			arg->out_fence = ret;
> +			ret = 0;
> +		}
> +	}
> +	if (ret) {
> +		fence_signal(fence);
> +		fence_put(fence);
> +	}
> +out:
> +	drm_gem_object_unreference_unlocked(obj);
> +	return ret;
> +}
> +
> +int vgem_fence_signal_ioctl(struct drm_device *dev,
> +			    void *data,
> +			    struct drm_file *file)
> +{
> +	struct vgem_file *vfile = file->driver_priv;
> +	struct drm_vgem_fence_signal *arg = data;
> +	struct fence *fence;
> +
> +	if (arg->flags)
> +		return -EINVAL;
> +
> +	mutex_lock(&vfile->fence_mutex);
> +	fence = idr_replace(&vfile->fence_idr, NULL, arg->fence);
> +	mutex_unlock(&vfile->fence_mutex);
> +	if (!fence)
> +		return -ENOENT;
> +	if (IS_ERR(fence))
> +		return PTR_ERR(fence);
> +
> +	fence_signal(fence);
> +	fence_put(fence);
> +	return 0;
> +}
> +
> +int vgem_fence_open(struct vgem_file *vfile)
> +{
> +	mutex_init(&vfile->fence_mutex);
> +	idr_init(&vfile->fence_idr);
> +
> +	return 0;
> +}
> +
> +static int __vgem_fence_idr_fini(int id, void *p, void *data)
> +{
> +	fence_signal(p);
> +	fence_put(p);
> +	return 0;
> +}
> +
> +void vgem_fence_close(struct vgem_file *vfile)
> +{
> +	idr_for_each(&vfile->fence_idr, __vgem_fence_idr_fini, vfile);
> +	idr_destroy(&vfile->fence_idr);
> +}
> diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h
> new file mode 100644
> index 000000000000..352d2fae8de9
> --- /dev/null
> +++ b/include/uapi/drm/vgem_drm.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright 2016 Intel Corporation
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef _UAPI_VGEM_DRM_H_
> +#define _UAPI_VGEM_DRM_H_
> +
> +#include "drm.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +/* Please note that modifications to all structs defined here are
> + * subject to backwards-compatibility constraints.
> + */
> +#define DRM_VGEM_FENCE_ATTACH	0x1
> +#define DRM_VGEM_FENCE_SIGNAL	0x2
> +
> +#define DRM_IOCTL_VGEM_FENCE_ATTACH	DRM_IOWR( DRM_COMMAND_BASE + DRM_VGEM_FENCE_ATTACH, struct drm_vgem_fence_attach)
> +#define DRM_IOCTL_VGEM_FENCE_SIGNAL	DRM_IOW( DRM_COMMAND_BASE + DRM_VGEM_FENCE_SIGNAL, struct drm_vgem_fence_signal)
> +
> +struct drm_vgem_fence_attach {
> +	__u32 handle;
> +	__u32 flags;
> +#define VGEM_FENCE_WRITE 0x1
> +	__u32 out_fence;
> +	__u32 pad;
> +};
> +
> +struct drm_vgem_fence_signal {
> +	__u32 fence;
> +	__u32 flags;
> +};
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif /* _UAPI_VGEM_DRM_H_ */
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the Intel-gfx mailing list