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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 11, 2016 at 04:24:45PM +0100, Chris Wilson wrote:
> On Mon, Jul 11, 2016 at 12:10:40PM -0300, Gustavo Padovan wrote:
> > 2016-07-11 Chris Wilson <chris@chris-wilson.co.uk>:
> > 
> > > 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.
> > > 
> > > Testcase: igt/vgem_basic/dmabuf-fence
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Sean Paul <seanpaul@chromium.org>
> > > Cc: Zach Reizner <zachr@google.com>
> > > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > Acked-by: Zach Reizner <zachr@google.com>
> > > ---
> > >  drivers/gpu/drm/vgem/Makefile     |   2 +-
> > >  drivers/gpu/drm/vgem/vgem_drv.c   |  34 ++++++
> > >  drivers/gpu/drm/vgem/vgem_drv.h   |  18 ++++
> > >  drivers/gpu/drm/vgem/vgem_fence.c | 220 ++++++++++++++++++++++++++++++++++++++
> > >  include/uapi/drm/vgem_drm.h       |  62 +++++++++++
> > >  5 files changed, 335 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 b5fb968d2d5c..2659e5cda857 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..88ce21010e28 100644
> > > --- a/drivers/gpu/drm/vgem/vgem_drv.h
> > > +++ b/drivers/gpu/drm/vgem/vgem_drv.h
> > > @@ -32,9 +32,27 @@
> > >  #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;
> > > +	u64 fence_context;
> > > +	atomic_t fence_seqno;
> > > +};
> > > +
> > >  #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..649e9e1cee35
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/vgem/vgem_fence.c
> > > @@ -0,0 +1,220 @@
> > > +/*
> > > + * 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 "file";
> > > +}
> > > +
> > > +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", 0);
> > > +}
> > 
> > I think this would be mainly used for debug purposes but it would be
> > nice to actually return the current timeline value here, i.e., the
> > seqno of the last signalled fence.
> 
> The fences can and will be signaled in any order, there is no timeline
> here under the control of userspace, we only give them fences.
>  
> > This would also allow to return if the fence was signaled or not
> > in vgem_fence_signaled() instead of just return false.
> 
> That doesn't fit the out-of-order unbound nature of the interface. The
> interface is just a collection of fences that userspace associates with
> the buffer that it may signal at any time. (Having no strict timeline is
> an advantage!)

Fences on the same timeline are supposed to be signalled in-order. If you
want full out-of-order fences then you need to grab a new timeline number
for each one. Drivers can and do merge fences on the same timeline and
just carry the one with the largest seqno around.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel