[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
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?
--
Earthling Michel D?nzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
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.
--
Earthling Michel D?nzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
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
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
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. ;-)
~Maarten
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
On 01.08.2014 19:12, Maarten Lankhorst wrote:
> Hey,
>
> 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.
--
Earthling Michel D?nzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
On 01-08-14 16:13, Michel D?nzer wrote:
> On 01.08.2014 19:12, Maarten Lankhorst wrote:
>> Hey,
>>
>> 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.
~Maarten
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
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.
--
Earthling Michel D?nzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
Hey,
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.
~Maarten
[PATCH 17/19] drm/radeon: use rcu waits in some ioctls
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/radeon/radeon_gem.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c
b/drivers/gpu/drm/radeon/radeon_gem.c
index d09650c1d720..7ba883843668 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -107,9 +107,12 @@ static int radeon_gem_set_domain(struct drm_gem_object
*gobj,
}
if (domain == RADEON_GEM_DOMAIN_CPU) {
/* Asking for cpu access wait for object idle */
- r = radeon_bo_wait(robj, NULL, false);
- if (r) {
- printk(KERN_ERR "Failed to wait for object !\n");
+ r = reservation_object_wait_timeout_rcu(robj->tbo.resv, true,
true, 30 * HZ);
+ if (!r)
+ r = -EBUSY;
+
+ if (r < 0 && r != -EINTR) {
+ printk(KERN_ERR "Failed to wait for object: %i\n", r);
return r;
}
}
@@ -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);
