search for: fence_wait

...et stuff relies crucially on being able to kick all waiters holding driver locks. Since the current fence code only exposes an opaque wait function without exposing the underlying wait queue we won't be able to sleep on both the fence queue and the reset queue. So would pose a problem if we add fence_wait calls to our driver. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

...!shared_count) { - kfree(shared); - shared = NULL; - } - } - - excl = reservation_object_get_excl(bo->resv); - if (excl && !fence_is_signaled(excl)) - fence_get(excl); - else - excl = NULL; - - __ttm_bo_unreserve(bo); - spin_unlock(&glob->lru_lock); - - if (excl) { - ret = fence_wait(excl, interruptible); - fence_put(excl); - } - - if (shared_count > 0) { - for (i = 0; i < shared_count; ++i) { - if (!ret) - ret = fence_wait(shared[i], interruptible); - fence_put(shared[i]); - } - kfree(shared); - } - - return ret; -} - /** * function ttm_bo_cleanup_refs_and_...

.... If you want to provide a is_signalled interface for atomic context then this should be optional, not mandatory. > > In case of enable_signaling it was the only sane solution, because > fence_signal can be called from irq context, and any calls after that to > fence_add_callback and fence_wait aren't allowed to do anything, so > fence_enable_sw_signaling and the default wait implementation must be > atomic. fence_wait itself doesn't have to be, so it's easy to grab > exclusive_lock there. I don't think you understood my point here: Completely drop enable_signal...

...o provide a is_signalled interface for atomic context then this should be optional, not mandatory. See below. >> In case of enable_signaling it was the only sane solution, because >> fence_signal can be called from irq context, and any calls after that to >> fence_add_callback and fence_wait aren't allowed to do anything, so >> fence_enable_sw_signaling and the default wait implementation must be >> atomic. fence_wait itself doesn't have to be, so it's easy to grab >> exclusive_lock there. > > I don't think you understood my point here: Completel...

...as long as it > wishes as long as enable_signaling isn't called yet. But that's just > the optimization to not required irqs to be turned on all the time. > > The other function is fence_is_signaled, which is used by code that is > interested in the fence state, together with fence_wait if it wants to > block and not just wants to know the momentary fence state. All the > other functions (the stuff that adds callbacks and the various _locked > and other versions) are just for fancy special cases. Well that's rather bad, cause IRQs aren't reliable enough on Radeon...

...ble to kick >> all waiters holding driver locks. Since the current fence code only >> exposes an opaque wait function without exposing the underlying wait >> queue we won't be able to sleep on both the fence queue and the reset >> queue. So would pose a problem if we add fence_wait calls to our >> driver. > > > And apart from that I really think that I misunderstood Maarten. But his > explanation sounds like i915 would do a reset because Radeon is locked up, > right? > > Well if that's really the case then I would question the interface even &gt...

...t; wishes as long as enable_signaling isn't called yet. But that's just >> the optimization to not required irqs to be turned on all the time. >> >> The other function is fence_is_signaled, which is used by code that is >> interested in the fence state, together with fence_wait if it wants to >> block and not just wants to know the momentary fence state. All the >> other functions (the stuff that adds callbacks and the various _locked >> and other versions) are just for fancy special cases. > > Well that's rather bad, cause IRQs aren't reli...

...ially on being able to kick > all waiters holding driver locks. Since the current fence code only > exposes an opaque wait function without exposing the underlying wait > queue we won't be able to sleep on both the fence queue and the reset > queue. So would pose a problem if we add fence_wait calls to our > driver. And apart from that I really think that I misunderstood Maarten. But his explanation sounds like i915 would do a reset because Radeon is locked up, right? Well if that's really the case then I would question the interface even more, cause that is really nonsense....

...>> all waiters holding driver locks. Since the current fence code only >>> exposes an opaque wait function without exposing the underlying wait >>> queue we won't be able to sleep on both the fence queue and the reset >>> queue. So would pose a problem if we add fence_wait calls to our >>> driver. >> >> And apart from that I really think that I misunderstood Maarten. But his >> explanation sounds like i915 would do a reset because Radeon is locked up, >> right? >> >> Well if that's really the case then I would question...

...>>>> >>>>>>> Does this modification look sane? >>>>>> Adding the timeout is on my todo list for quite some time as >>>>>> well, so this part makes sense. >>>>>> >>>>>>> +static long __radeon_fence_wait(struct fence *f, bool intr, >>>>>>> long timeout) >>>>>>> +{ >>>>>>> + struct radeon_fence *fence = to_radeon_fence(f); >>>>>>> + u64 target_seq[RADEON_NUM_RINGS] = {}; >>>>>>> + >>...

...ignalled); void *priv; @@ -172,6 +173,7 @@ nouveau_fence_emit(struct nouveau_fence *fence) } OUT_RING (chan, fence->sequence); FIRE_RING(chan); + fence->emitted_at = jiffies; return 0; } @@ -230,7 +232,8 @@ __nouveau_fence_signalled(void *sync_obj, void *sync_arg) int __nouveau_fence_wait(void *sync_obj, void *sync_arg, bool lazy, bool intr) { - unsigned long timeout = jiffies + (3 * DRM_HZ); + struct nouveau_fence *fence = nouveau_fence(sync_obj); + unsigned long timeout = fence->emitted_at + 3 * DRM_HZ; unsigned long sleep_time = NSEC_PER_MSEC / 1000; ktime_t t; int ret...

...tually not such a good idea. > > In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup. The lockup handling calls radeon_fence_wait, not the generic fence_wait. It doesn't call the exported wait function that takes the exclusive_lock in read mode. And lockdep should have complained if I screwed that up. :-) ~Maarten

...t;>>>>>> >>>>>>>> Does this modification look sane? >>>>>>> Adding the timeout is on my todo list for quite some time as well, so this part makes sense. >>>>>>> >>>>>>>> +static long __radeon_fence_wait(struct fence *f, bool intr, long timeout) >>>>>>>> +{ >>>>>>>> + struct radeon_fence *fence = to_radeon_fence(f); >>>>>>>> + u64 target_seq[RADEON_NUM_RINGS] = {}; >>>>>>>> + >>>>>&g...

...omic context then this should >> be optional, not mandatory. > See below. >>> In case of enable_signaling it was the only sane solution, because >>> fence_signal can be called from irq context, and any calls after >>> that to >>> fence_add_callback and fence_wait aren't allowed to do anything, so >>> fence_enable_sw_signaling and the default wait implementation must be >>> atomic. fence_wait itself doesn't have to be, so it's easy to grab >>> exclusive_lock there. >> >> I don't think you understood my p...

Am 23.07.2014 10:07, schrieb Daniel Vetter: > On Wed, Jul 23, 2014 at 9:58 AM, Christian K?nig > <deathsimple at vodafone.de> wrote: >> Just imagine an application using prime is locking up Radeon and because of >> that gets killed by the user. Nothing else in the system would use the >> Radeon hardware any more and so radeon gets only called by another driver

Am 22.07.2014 17:42, schrieb Daniel Vetter: > On Tue, Jul 22, 2014 at 5:35 PM, Christian K?nig > <christian.koenig at amd.com> wrote: >> Drivers exporting fences need to provide a fence->signaled and a fence->wait >> function, everything else like fence->enable_signaling or calling >> fence_signaled() from the driver is optional. >> >> Drivers

...ch a good idea. >> >> In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup. > The lockup handling calls radeon_fence_wait, not the generic fence_wait. It doesn't call the exported wait function that takes the exclusive_lock in read mode. > And lockdep should have complained if I screwed that up. :-) You screwed it up and lockdep didn't warn you about it :-P It's not a locking problem I'm talking a...

...; - reservation_object_add_excl_fence(resv, &fence->base); } -static void * -nouveau_bo_fence_ref(void *sync_obj) -{ - return nouveau_fence_ref(sync_obj); -} - -static bool -nouveau_bo_fence_signalled(void *sync_obj) -{ - return nouveau_fence_done(sync_obj); -} - -static int -nouveau_bo_fence_wait(void *sync_obj, bool lazy, bool intr) -{ - return nouveau_fence_wait(sync_obj, lazy, intr); -} - -static int -nouveau_bo_fence_flush(void *sync_obj) -{ - return 0; -} - struct ttm_bo_driver nouveau_bo_driver = { .ttm_tt_create = &nouveau_ttm_tt_create, .ttm_tt_populate = &nouveau_ttm_t...

...; - reservation_object_add_excl_fence(resv, &fence->base); } -static void * -nouveau_bo_fence_ref(void *sync_obj) -{ - return nouveau_fence_ref(sync_obj); -} - -static bool -nouveau_bo_fence_signalled(void *sync_obj) -{ - return nouveau_fence_done(sync_obj); -} - -static int -nouveau_bo_fence_wait(void *sync_obj, bool lazy, bool intr) -{ - return nouveau_fence_wait(sync_obj, lazy, intr); -} - -static int -nouveau_bo_fence_flush(void *sync_obj) -{ - return 0; -} - struct ttm_bo_driver nouveau_bo_driver = { .ttm_tt_create = &nouveau_ttm_tt_create, .ttm_tt_populate = &nouveau_ttm_t...

...all to fence_signal for as long as it wishes as long as enable_signaling isn't called yet. But that's just the optimization to not required irqs to be turned on all the time. The other function is fence_is_signaled, which is used by code that is interested in the fence state, together with fence_wait if it wants to block and not just wants to know the momentary fence state. All the other functions (the stuff that adds callbacks and the various _locked and other versions) are just for fancy special cases. >> Locking >> correctness is enforced with some extremely nasty lockdep annota...