Re: Re: [PATCH] drm/radeon: Add support for userspace fence waits

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

 



On -10.01.-28163 20:59, Jerome Glisse wrote:
On Tue, Jan 31, 2012 at 02:13:00PM -0500, Alex Deucher wrote:
On Tue, Jan 31, 2012 at 2:07 PM, Jerome Glisse<j.glisse@xxxxxxxxx>  wrote:
On Tue, Jan 31, 2012 at 01:55:43PM -0500, David Airlie wrote:

Some comments below.

+   struct radeon_device *rdev = dev->dev_private;
+   struct drm_gem_object *gobj;
+   struct radeon_bo *robj;
+   void *buffer_data;
+   uint32_t *fence_data;
+   int r = 0;
+   long timeout;
+   int ring = RADEON_RING_TYPE_GFX_INDEX;
+
+   /* If you're implementing this for other rings, you'll want to
share
+      code with radeon_cs_get_ring in radeon_cs.c */
+   if (args->ring != RADEON_CS_RING_GFX) {
+           return -EINVAL;
+   }
+
+   gobj = drm_gem_object_lookup(dev, filp, args->handle);
+   if (gobj == NULL) {
+           return -ENOENT;
+   }
+   robj = gem_to_radeon_bo(gobj);
+
+   if (gobj->size<  args->offset) {
+           r = -EINVAL;
+           goto unreference;
+   }
+
+   r = radeon_bo_reserve(robj, true);
+   if (r) {
+           goto unreference;
+   }
+
+   r = radeon_bo_pin(robj, RADEON_GEM_DOMAIN_GTT, NULL);
+   if (r) {
+           goto unreserve;
+   }
+
+   r = radeon_bo_kmap(robj,&buffer_data);
+   if (r) {
+           goto unpin;
+   }
+

Do you need to pin it? I think if you have it reserved and you are in here you shouldn't need to. (unless kmapping requires a pin)/
No you don't need to pin, actually it's bad to pin the buffer you might
want to wait on might be in vram.

Also you never assume that things could go wrong and that your value
might never be written to the buffer. So it will wait forever in the
kernel.

As i said in the other mail i would rather as a wait on submited cs
ioctl and add some return value from cs ioctl. I can't remember the
outcome of this discusion we had when we were doing virtual memory
support. Alex ? Michel ? better memory than i do ? :)

I vaguely recall the discussion, but I don't remember the details,
I'll have to look through my old mail.  Maybe a new CS ioctl flag
requesting EOP irqs for the CS would be a better approach than a
separate ioctl.

Alex
I think the idea was to return u64 value of specific ring and only
enable irq if new wait cs ioctl was call, a little bit like bo wait.

Will try to dig through my mail too.

Cheers,
Jerome

As far as I remember we wanted the CS ioctl to return ring+fence number (expanded to 64bit) and add another ioctl to wait for a ring to reach that fence number. That has the clear advantages that a) we no longer need a fence bo for each application and b) not all rings can write a fence value from an userspace IB.

I've started to implement that and at least had the 64bit expansion of fence values and the returning of them in the CS ioctl running, but then got to work on other things first. I probably have those patches still laying around somewhere, on the other hand it should be pretty easy to implement...

Regards,
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux