search for: fence_queue

...> + wait_queue_t fence_wake; > }; > > int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring); > @@ -2268,6 +2269,7 @@ struct radeon_device { > struct radeon_mman mman; > struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; > wait_queue_head_t fence_queue; > + unsigned fence_context; > struct mutex ring_lock; > struct radeon_ring ring[RADEON_NUM_RINGS]; > bool ib_pool_ready; > @@ -2358,11 +2360,6 @@ u32 cik_mm_rdoorbell(struct radeon_device > *rdev, u32 index); > void cik_mm_wdoorbell(struct radeon_device *rdev, u...

...> + radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring); > + return false; > + } > + > + fence->fence_wake.flags = 0; > + fence->fence_wake.private = NULL; > + fence->fence_wake.func = radeon_fence_check_signaled; > + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); > + fence_get(f); That looks like a race condition to me. The fence needs to be added to the wait queue before the check, not after. Apart from that the whole approach looks like a really bad idea to me. How for example is lockup detection supposed to happen with...

...* RB, DMA, etc. */ unsigned ring; + + wait_queue_t fence_wake; }; int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring); @@ -2256,6 +2257,7 @@ struct radeon_device { struct radeon_mman mman; struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; wait_queue_head_t fence_queue; + unsigned fence_context; struct mutex ring_lock; struct radeon_ring ring[RADEON_NUM_RINGS]; bool ib_pool_ready; @@ -2346,11 +2348,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 index); void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, u32 v); /* - * Cast he...

Am 19.05.2014 15:35, schrieb Maarten Lankhorst: > op 19-05-14 14:30, Christian K?nig schreef: >> Am 19.05.2014 12:10, schrieb Maarten Lankhorst: >>> op 19-05-14 10:27, Christian K?nig schreef: >>>> Am 19.05.2014 10:00, schrieb Maarten Lankhorst: >>>> [SNIP] >>>> The problem here is that the whole approach collides with the way >>>>

...ke; > }; > > int radeon_fence_driver_start_ring(struct radeon_device *rdev, int > ring); > @@ -2256,6 +2257,7 @@ struct radeon_device { > struct radeon_mman mman; > struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; > wait_queue_head_t fence_queue; > + unsigned fence_context; > struct mutex ring_lock; > struct radeon_ring ring[RADEON_NUM_RINGS]; > bool ib_pool_ready; > @@ -2346,11 +2348,6 @@ u32 cik_mm_rdoorbell(struct radeon_device > *rdev, u32 index); > vo...

...* RB, DMA, etc. */ unsigned ring; + + wait_queue_t fence_wake; }; int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring); @@ -2268,6 +2269,7 @@ struct radeon_device { struct radeon_mman mman; struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; wait_queue_head_t fence_queue; + unsigned fence_context; struct mutex ring_lock; struct radeon_ring ring[RADEON_NUM_RINGS]; bool ib_pool_ready; @@ -2358,11 +2360,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 index); void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, u32 v); /* - * Cast he...

...MA, etc. */ unsigned ring; + + wait_queue_t fence_wake; }; int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring); @@ -2256,6 +2257,7 @@ struct radeon_device { struct radeon_mman mman; struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; wait_queue_head_t fence_queue; + unsigned fence_context; struct mutex ring_lock; struct radeon_ring ring[RADEON_NUM_RINGS]; bool ib_pool_ready; @@ -2346,11 +2348,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 index); void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, u32 v); /* - * C...

...* RB, DMA, etc. */ unsigned ring; + + wait_queue_t fence_wake; }; int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring); @@ -2268,6 +2269,7 @@ struct radeon_device { struct radeon_mman mman; struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; wait_queue_head_t fence_queue; + unsigned fence_context; struct mutex ring_lock; struct radeon_ring ring[RADEON_NUM_RINGS]; bool ib_pool_ready; @@ -2358,11 +2360,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 index); void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, u32 v); /* - * Cast he...

...t;>>>>>>>>>>> + fence->fence_wake.private = NULL; >>>>>>>>>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>>>>>>>>> + __add_wait_queue(&fence->rdev->fence_queue, >>>>>>>>>>>>> &fence->fence_wake); >>>>>>>>>>>>> + fence_get(f); >>>>>>>>>>>> That looks like a race condition to me. The fence needs to >>>>>>>>&gt...

...t;>>>>>>>>> + fence->fence_wake.private = NULL; >>>>>>>>>>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>>>>>>>>>> + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); >>>>>>>>>>>>>> + fence_get(f); >>>>>>>>>>>>> That looks like a race condition to me. The fence needs to be added to the wait queue before the check, not after. >>>>>>&g...

...e_drv; + struct radeon_device *rdev; + unsigned long iring; + + fence_drv = container_of(work, struct radeon_fence_driver, fence_check_work.work); + rdev = fence_drv->rdev; + iring = fence_drv - &rdev->fence_drv[0]; + + if (__radeon_fence_process(rdev, iring)) + wake_up_all(&rdev->fence_queue); + else if (radeon_ring_is_lockup(rdev, iring, &rdev->ring[iring])) { + /* good news we believe it's a lockup */ + dev_warn(rdev->dev, "GPU lockup (current fence id " + "0x%016llx last fence id 0x%016llx on ring %ld)\n", + (uint64_t)atomic64_read(&fenc...

...t; + return false; >>> + } >>> + >>> + fence->fence_wake.flags = 0; >>> + fence->fence_wake.private = NULL; >>> + fence->fence_wake.func = radeon_fence_check_signaled; >>> + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); >>> + fence_get(f); >> That looks like a race condition to me. The fence needs to be added >> to the wait queue before the check, not after. >> >> Apart from that the whole approach looks like a really bad idea to >> me. How f...

...} >>>>> + >>>>> + fence->fence_wake.flags = 0; >>>>> + fence->fence_wake.private = NULL; >>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>> + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); >>>>> + fence_get(f); >>>> That looks like a race condition to me. The fence needs to be added >>>> to the wait queue before the check, not after. >>>> >>>> Apart from that the whole approach looks like...

...ke; > }; > > int radeon_fence_driver_start_ring(struct radeon_device *rdev, int > ring); > @@ -2256,6 +2257,7 @@ struct radeon_device { > struct radeon_mman mman; > struct radeon_fence_driver fence_drv[RADEON_NUM_RINGS]; > wait_queue_head_t fence_queue; > + unsigned fence_context; > struct mutex ring_lock; > struct radeon_ring ring[RADEON_NUM_RINGS]; > bool ib_pool_ready; > @@ -2346,11 +2348,6 @@ u32 cik_mm_rdoorbell(struct radeon_device > *rdev, u32 index); > vo...

...; + unsigned long iring; > + > + fence_drv = container_of(work, struct radeon_fence_driver, fence_check_work.work); > + rdev = fence_drv->rdev; > + iring = fence_drv - &rdev->fence_drv[0]; > + > + if (__radeon_fence_process(rdev, iring)) > + wake_up_all(&rdev->fence_queue); > + else if (radeon_ring_is_lockup(rdev, iring, &rdev->ring[iring])) { > + /* good news we believe it's a lockup */ > + dev_warn(rdev->dev, "GPU lockup (current fence id " > + "0x%016llx last fence id 0x%016llx on ring %ld)\n", > + (uint64_...

...>>>>>>> + fence->fence_wake.flags = 0; >>>>>>> + fence->fence_wake.private = NULL; >>>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>>> + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); >>>>>>> + fence_get(f); >>>>>> That looks like a race condition to me. The fence needs to be >>>>>> added to the wait queue before the check, not after. >>>>>> >>>>>> Apart...

...e_wake.flags = 0; >>>>>>>>>>> + fence->fence_wake.private = NULL; >>>>>>>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>>>>>>> + __add_wait_queue(&fence->rdev->fence_queue, >>>>>>>>>>> &fence->fence_wake); >>>>>>>>>>> + fence_get(f); >>>>>>>>>> That looks like a race condition to me. The fence needs to be >>>>>>>>>> added to the wa...

...;>>> + >>>>>> + fence->fence_wake.flags = 0; >>>>>> + fence->fence_wake.private = NULL; >>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>> + __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); >>>>>> + fence_get(f); >>>>> That looks like a race condition to me. The fence needs to be added to the wait queue before the check, not after. >>>>> >>>>> Apart from that the whole approach looks like a...

...> + fence->fence_wake.flags = 0; >>>>>>>>> + fence->fence_wake.private = NULL; >>>>>>>>> + fence->fence_wake.func = radeon_fence_check_signaled; >>>>>>>>> + __add_wait_queue(&fence->rdev->fence_queue, >>>>>>>>> &fence->fence_wake); >>>>>>>>> + fence_get(f); >>>>>>>> That looks like a race condition to me. The fence needs to be >>>>>>>> added to the wait queue before the check, not a...

...() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place? > It doesn't need to be completely reliable, or finish immediately. > > And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked. I raised this already somewhere else, but should we have some common infrastructure in the core fence code to recheck fences periodically? radeon doesn't seem to be the only hw where this isn't reliable enough. Of course timer-...