Re: [PATCH v2] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 13, 2016 at 05:29:21PM -0300, Gustavo Padovan wrote:
> 2016-07-12 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> 
> > 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.
> > 
> > Testcase: igt/vgem_basic/dmabuf-fence
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > Cc: Zach Reizner <zachr@xxxxxxxxxx>
> > Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Acked-by: Zach Reizner <zachr@xxxxxxxxxx>
> > ---
> >  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..63095331c446
> > --- /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;
> 
> With one fence per context you can actually return if the fence was
> signalled or not. May be useful for debug.

That's already stored inside the fence->flags FENCE_FLAG_SIGNALED_BIT.
If it it set (which it will be after we are signaled),
fence->ops->signaled() is not called. So we know that if this function
is ever called, the fence is unsignaled.

> > +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), 0);
> 
> Usually 0 would mean that the fence is already signalled, so I'd better
> set it to 1.

Hmm, yes, we can improve the debug output using that:

diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
index 63095331c446..b7da11419ad6 100644
--- a/drivers/gpu/drm/vgem/vgem_fence.c
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -52,13 +52,13 @@ static bool vgem_fence_enable_signaling(struct fence *fence)
 
 static void vgem_fence_value_str(struct fence *fence, char *str, int size)
 {
-       snprintf(str, size, "%u", 0);
+       snprintf(str, size, "%u", fence->seqno);
 }
 
 static void vgem_fence_timeline_value_str(struct fence *fence, char *str,
                                          int size)
 {
-       snprintf(str, size, "%u", 0);
+       snprintf(str, size, "%u", fence_is_signaled(fence) ? fence->seqno : 0);
 }
 
 const struct fence_ops vgem_fence_ops = {
@@ -81,7 +81,7 @@ static struct fence *vgem_fence_create(struct vgem_file *vfile)
 
        spin_lock_init(&fence->lock);
        fence_init(&fence->base, &vgem_fence_ops, &fence->lock,
-                  fence_context_alloc(1), 0);
+                  fence_context_alloc(1), 1);
 
        return &fence->base;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux