| Message ID | 20140731153432.15061.49403.stgit@patser (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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
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?
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
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);
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> --- drivers/gpu/drm/radeon/radeon_gem.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)