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

[Christian König]

On 01.02.2012 12:31, Simon Farnsworth wrote:
> 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:
Well it's probably easier to type that down again instead of searching 
in a two month old reflog....

Patches are attached, but be aware that they are WIP and beside compile 
testing completely untested.

Whats missing is the kernel interface, but that shouldn't be to 
complicated to implement.

Christian.

>
> 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.