Re: [PATCH 17/19] drm/radeon: use rcu waits in some ioctls

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

op 04-08-14 11:25, Michel Dänzer schreef:
> On 04.08.2014 17:56, Maarten Lankhorst wrote:
>> op 04-08-14 10:42, Michel Dänzer schreef:
>>> On 02.08.2014 02:07, Maarten Lankhorst wrote:
>>>> On 01-08-14 16:13, Michel Dänzer wrote:
>>>>> On 01.08.2014 19:12, Maarten Lankhorst wrote:
>>>>>> On 01-08-14 10:27, Michel Dänzer wrote:
>>>>>>> On 01.08.2014 00:34, Maarten Lankhorst wrote:
>>>>>>>> @@ -357,14 +360,20 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>>>>>>>  	struct drm_radeon_gem_wait_idle *args = data;
>>>>>>>>  	struct drm_gem_object *gobj;
>>>>>>>>  	struct radeon_bo *robj;
>>>>>>>> -	int r;
>>>>>>>> +	int r = 0;
>>>>>>>> +	long ret;
>>>>>>>>  
>>>>>>>>  	gobj = drm_gem_object_lookup(dev, filp, args->handle);
>>>>>>>>  	if (gobj == NULL) {
>>>>>>>>  		return -ENOENT;
>>>>>>>>  	}
>>>>>>>>  	robj = gem_to_radeon_bo(gobj);
>>>>>>>> -	r = radeon_bo_wait(robj, NULL, false);
>>>>>>>> +	ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, true, 30 * HZ);
>>>>>>>> +	if (ret == 0)
>>>>>>>> +		r = -EBUSY;
>>>>>>>> +	else if (ret < 0)
>>>>>>>> +		r = ret;
>>>>>>>> +
>>>>>>>>  	/* callback hw specific functions if any */
>>>>>>>>  	if (rdev->asic->ioctl_wait_idle)
>>>>>>>>  		robj->rdev->asic->ioctl_wait_idle(rdev, robj);
>>>>>>> Heads up, this conflicts with
>>>>>>> http://lists.freedesktop.org/archives/dri-devel/2014-August/065255.html
>>>>>>> which passes a non-NULL second argument to radeon_bo_wait() to get the
>>>>>>> BO's current domain.
>>>>>> Ok, I will fix it up and resend it later.
>>>>>>
>>>>>> Does it matter if I grab the current domain without grabbing the lock
>>>>>> here? Because it doesn't matter if it sees the old or new domain, it
>>>>>> could have been changed after returning too.
>>>>> It should be the domain where the BO is located when the fence we are
>>>>> waiting for here signals.
>>>> Could we compare domain before and after the rcu wait, and retry
>>>> waiting if they're different, and the new one is VRAM? (eg eviction
>>>> happened) That should prevent needing to lock the bo.
>>> Eviction normally only happens from VRAM, not to VRAM. :) So if you know
>>> whether the domain is VRAM or not after the wait, you can just proceed
>>> accordingly, I don't see why you'd need to wait again.
>> Because in the worst case you didn't wait on the fence that started
>> the eviction, but one before it. ;-)
> I'm afraid you've lost me. Can you determine the domain that
> radeon_bo_wait() would have returned?
>
>
Ok so..

wait ioctl:

waits on fence 1

(eviction to vram happens asynchronously, fence 1 on the bo gets replaced by fence 2)

wait 1 completes, new domain is VRAM

vram flush happens, but fence 2 is not signaled yet so not everything is copied.

wait ioctl returns

Or is it unimportant here, and the vram flush doesn't depend on the fence being completed?

The second wait would be forced by ttm_bo_vm_fault anyway.

~Maarten