| Message ID | 20170809190309.6177-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 9, 2017 at 12:03 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > To further facilitate GEM testing, allow testing of drm_syncobj by > attaching them to vgem fences. These fences are already employed by igt > for testing inter-driver fence handling (across dmabuf and sync_file). > > An igt example use would be like: > > int vgem = drm_driver_open(DRIVER_VGEM); > uint32_t handle = vgem_create_dummy(vgem); > This is a bit nasty... Why do we need a BO? Why can't we just create and attach a fence without the BO? > uint32_t syncobj = drm_syncobj_create(vgem); > uint32_t fence = drmIoctl(vgem, > DRM_IOCTL_VGEM_FENCE_ATTACH, > &(struct vgem_fence_attach){ > .handle = handle, > .flags = VGEM_FENCE_SYNCOBJ, > .syncobj = syncobj, > }); > > /* ... use syncobj for profit ... */ > > vgem_fence_signal(vgem, fence); > > For wider use though, there is little immediate benefit to syncobj > over the vgem fence as both are handles in an idr (the fence here is not > a sync-file fd like in most other drivers). The main benefit for syncobj > is that it allows to create channels between objects and drivers by > virtue of its persistence beyond the vgem fence itself. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/vgem/vgem_drv.c | 4 +++- > drivers/gpu/drm/vgem/vgem_fence.c | 26 ++++++++++++++++++++++---- > include/drm/drm_syncobj.h | 2 ++ > include/uapi/drm/vgem_drm.h | 3 ++- > 4 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_ > drv.c > index 12289673f457..a0202e1eaf6b 100644 > --- a/drivers/gpu/drm/vgem/vgem_drv.c > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > @@ -432,7 +432,9 @@ static void vgem_release(struct drm_device *dev) > } > > static struct drm_driver vgem_driver = { > - .driver_features = DRIVER_GEM | DRIVER_PRIME, > + .driver_features = (DRIVER_GEM | > + DRIVER_PRIME | > + DRIVER_SYNCOBJ), > .release = vgem_release, > .open = vgem_open, > .postclose = vgem_postclose, > diff --git a/drivers/gpu/drm/vgem/vgem_fence.c > b/drivers/gpu/drm/vgem/vgem_fence.c > index 3109c8308eb5..988e860c03d3 100644 > --- a/drivers/gpu/drm/vgem/vgem_fence.c > +++ b/drivers/gpu/drm/vgem/vgem_fence.c > @@ -23,6 +23,8 @@ > #include <linux/dma-buf.h> > #include <linux/reservation.h> > > +#include <drm/drm_syncobj.h> > + > #include "vgem_drv.h" > > #define VGEM_FENCE_TIMEOUT (10*HZ) > @@ -156,20 +158,30 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, > struct drm_vgem_fence_attach *arg = data; > struct vgem_file *vfile = file->driver_priv; > struct reservation_object *resv; > + struct drm_syncobj *sync = NULL; > struct drm_gem_object *obj; > struct dma_fence *fence; > int ret; > > - if (arg->flags & ~VGEM_FENCE_WRITE) > - return -EINVAL; > - > - if (arg->pad) > + if (arg->flags & ~(VGEM_FENCE_WRITE | VGEM_FENCE_SYNCOBJ)) > return -EINVAL; > > obj = drm_gem_object_lookup(file, arg->handle); > if (!obj) > return -ENOENT; > > + if (arg->flags & VGEM_FENCE_SYNCOBJ) { > + sync = drm_syncobj_find(file, arg->syncobj); > + if (!sync) { > + ret = -ENOENT; > + goto err; > + } > + > + /* We don't check if the current syncobj is busy or not, we > + * will just replace it with ourselves. > + */ > + } > + > ret = attach_dmabuf(dev, obj); > if (ret) > goto err; > @@ -207,12 +219,18 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, > ret = 0; > } > } > + > + if (ret == 0 && sync) > + drm_syncobj_replace_fence(sync, fence); > + > err_fence: > if (ret) { > dma_fence_signal(fence); > dma_fence_put(fence); > } > err: > + if (sync) > + drm_syncobj_put(sync); > drm_gem_object_unreference_unlocked(obj); > return ret; > } > diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h > index 89976da542b1..9010ab8343e5 100644 > --- a/include/drm/drm_syncobj.h > +++ b/include/drm/drm_syncobj.h > @@ -28,6 +28,8 @@ > > #include "linux/dma-fence.h" > > +struct drm_file; > + > /** > * struct drm_syncobj - sync object. > * > diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h > index bf66f5db6da8..94777197e561 100644 > --- a/include/uapi/drm/vgem_drm.h > +++ b/include/uapi/drm/vgem_drm.h > @@ -46,8 +46,9 @@ struct drm_vgem_fence_attach { > __u32 handle; > __u32 flags; > #define VGEM_FENCE_WRITE 0x1 > +#define VGEM_FENCE_SYNCOBJ 0x2 > __u32 out_fence; > - __u32 pad; > + __u32 syncobj; > }; > > struct drm_vgem_fence_signal { > -- > 2.13.3 > >
Quoting Jason Ekstrand (2017-08-10 02:02:43) > On Wed, Aug 9, 2017 at 12:03 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > To further facilitate GEM testing, allow testing of drm_syncobj by > attaching them to vgem fences. These fences are already employed by igt > for testing inter-driver fence handling (across dmabuf and sync_file). > > An igt example use would be like: > > int vgem = drm_driver_open(DRIVER_VGEM); > uint32_t handle = vgem_create_dummy(vgem); > > > This is a bit nasty... Why do we need a BO? Why can't we just create and > attach a fence without the BO? Attach a fence to what? vgem is a wrapper around memory with the fences for coordinating exclusive/shared access to that memory. If you remove that memory, you remove the only functionality vgem has. You would be left with just some fences each on their own abstract timeline. At that point, you might as well use sw_sync, at least that exports a notion of a timeline (which may or may not be useful). -Chris
On Wed, Aug 9, 2017 at 6:23 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jason Ekstrand (2017-08-10 02:02:43) > > On Wed, Aug 9, 2017 at 12:03 PM, Chris Wilson <chris@chris-wilson.co.uk> > wrote: > > > > To further facilitate GEM testing, allow testing of drm_syncobj by > > attaching them to vgem fences. These fences are already employed by > igt > > for testing inter-driver fence handling (across dmabuf and > sync_file). > > > > An igt example use would be like: > > > > int vgem = drm_driver_open(DRIVER_VGEM); > > uint32_t handle = vgem_create_dummy(vgem); > > > > > > This is a bit nasty... Why do we need a BO? Why can't we just create and > > attach a fence without the BO? > > Attach a fence to what? vgem is a wrapper around memory with the fences > for coordinating exclusive/shared access to that memory. If you remove > that memory, you remove the only functionality vgem has. > > You would be left with just some fences each on their own abstract > timeline. At that point, you might as well use sw_sync, at least that > exports a notion of a timeline (which may or may not be useful). > Then maybe sw_sync is a better tool for testing syncobj. I had one version of my i-g-t patches which used it and it worked out quite well. Maybe I should just go back to that.
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 12289673f457..a0202e1eaf6b 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -432,7 +432,9 @@ static void vgem_release(struct drm_device *dev) } static struct drm_driver vgem_driver = { - .driver_features = DRIVER_GEM | DRIVER_PRIME, + .driver_features = (DRIVER_GEM | + DRIVER_PRIME | + DRIVER_SYNCOBJ), .release = vgem_release, .open = vgem_open, .postclose = vgem_postclose, diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index 3109c8308eb5..988e860c03d3 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -23,6 +23,8 @@ #include <linux/dma-buf.h> #include <linux/reservation.h> +#include <drm/drm_syncobj.h> + #include "vgem_drv.h" #define VGEM_FENCE_TIMEOUT (10*HZ) @@ -156,20 +158,30 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, struct drm_vgem_fence_attach *arg = data; struct vgem_file *vfile = file->driver_priv; struct reservation_object *resv; + struct drm_syncobj *sync = NULL; struct drm_gem_object *obj; struct dma_fence *fence; int ret; - if (arg->flags & ~VGEM_FENCE_WRITE) - return -EINVAL; - - if (arg->pad) + if (arg->flags & ~(VGEM_FENCE_WRITE | VGEM_FENCE_SYNCOBJ)) return -EINVAL; obj = drm_gem_object_lookup(file, arg->handle); if (!obj) return -ENOENT; + if (arg->flags & VGEM_FENCE_SYNCOBJ) { + sync = drm_syncobj_find(file, arg->syncobj); + if (!sync) { + ret = -ENOENT; + goto err; + } + + /* We don't check if the current syncobj is busy or not, we + * will just replace it with ourselves. + */ + } + ret = attach_dmabuf(dev, obj); if (ret) goto err; @@ -207,12 +219,18 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, ret = 0; } } + + if (ret == 0 && sync) + drm_syncobj_replace_fence(sync, fence); + err_fence: if (ret) { dma_fence_signal(fence); dma_fence_put(fence); } err: + if (sync) + drm_syncobj_put(sync); drm_gem_object_unreference_unlocked(obj); return ret; } diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index 89976da542b1..9010ab8343e5 100644 --- a/include/drm/drm_syncobj.h +++ b/include/drm/drm_syncobj.h @@ -28,6 +28,8 @@ #include "linux/dma-fence.h" +struct drm_file; + /** * struct drm_syncobj - sync object. * diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h index bf66f5db6da8..94777197e561 100644 --- a/include/uapi/drm/vgem_drm.h +++ b/include/uapi/drm/vgem_drm.h @@ -46,8 +46,9 @@ struct drm_vgem_fence_attach { __u32 handle; __u32 flags; #define VGEM_FENCE_WRITE 0x1 +#define VGEM_FENCE_SYNCOBJ 0x2 __u32 out_fence; - __u32 pad; + __u32 syncobj; }; struct drm_vgem_fence_signal {
To further facilitate GEM testing, allow testing of drm_syncobj by attaching them to vgem fences. These fences are already employed by igt for testing inter-driver fence handling (across dmabuf and sync_file). An igt example use would be like: int vgem = drm_driver_open(DRIVER_VGEM); uint32_t handle = vgem_create_dummy(vgem); uint32_t syncobj = drm_syncobj_create(vgem); uint32_t fence = drmIoctl(vgem, DRM_IOCTL_VGEM_FENCE_ATTACH, &(struct vgem_fence_attach){ .handle = handle, .flags = VGEM_FENCE_SYNCOBJ, .syncobj = syncobj, }); /* ... use syncobj for profit ... */ vgem_fence_signal(vgem, fence); For wider use though, there is little immediate benefit to syncobj over the vgem fence as both are handles in an idr (the fence here is not a sync-file fd like in most other drivers). The main benefit for syncobj is that it allows to create channels between objects and drivers by virtue of its persistence beyond the vgem fence itself. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/vgem/vgem_drv.c | 4 +++- drivers/gpu/drm/vgem/vgem_fence.c | 26 ++++++++++++++++++++++---- include/drm/drm_syncobj.h | 2 ++ include/uapi/drm/vgem_drm.h | 3 ++- 4 files changed, 29 insertions(+), 6 deletions(-)