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

[Simon Farnsworth]

Christian,

You said elsewhere that you have half-finished patches that illustrate the
interface Jerome prefers - if you send them to me, I can work on finishing
them.

Responding to the rest of the message:

On Tuesday 31 January 2012, Jerome Glisse <j.glisse at gmail.com> wrote:
> On Tue, Jan 31, 2012 at 01:55:43PM -0500, David Airlie wrote:
> > 
> > Some comments below.
> > 
> > > +	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.
>
I'll remove the pin/unpin calls - I was copying use from elsewhere in the
kernel, when trying to work out why it didn't work as expected.

> 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.
>
I'm using wait_event_interruptible_timeout - userspace can pass me a timeout
if it knows how long it expects to wait (and recover if it takes too long),
and will be interrupted by signals. In an earlier version of the code, I
didn't enable the EOP interrupts, and was able to recover userspace simply
by sending it a SIGSTOP/SIGCONT pair (until the next fence, when it stalled
again).

In that respect, this is no different to the existing situation in userspace
- if the GPU is reset before the EVENT_WRITE_EOP executes, userspace will
spin until a signal interrupts it. All that changes if userspace uses this
ioctl is that userspace will sleep instead of burning a hole in my CPU
budget - the recovery strategy is unchanged.

> 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 ? :)
>
If someone has half-complete patches illustrating what you mean, I can take
on the work of finishing them off. I've cc'd Christian on this mail, as it
looks like he has a starting point.

One slight advantage this interface has over a "wait on submitted CS ioctl"
interface is that, given suitable userspace changes, it becomes possible to
submit multiple fences in one IB, and wait for them individually (or only
wait for a subset of them). However, I'm not sure that that's enough of an
advantage to outweigh the disadvantages, especially given that the userspace
changes required are fairly intrusive.

> Cheers,
> Jerome
> 
> > 
> > 
> > > +	radeon_irq_kms_sw_irq_get(rdev, ring);
> > > +
> > > +	fence_data = (uint32_t*)buffer_data;
> > > +	timeout =
> > > an
> > > + * interrupt and the value in the buffer might have changed.
> > > + */
> > > +struct drm_radeon_gem_wait_user_fence {
> > > +	uint32_t                handle;
> > > +	uint32_t                ring;
> > > +	uint64_t                offset;
> > > +	uint32_t                value;
> > > +	uint64_t                timeout_usec;
> > 
> > Align things here, 64 then 64 then 32 32 32 and a 32 pad.
> >
Will do in v2.
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20120201/91602f15/attachment.pgp>