Am 19.08.2013 12:17, schrieb Maarten Lankhorst:
> [SNIP]
> @@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev,
> int ring)
> }
> } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
>
> - if (wake) {
> + if (wake)
> rdev->fence_drv[ring].last_activity = jiffies;
> - wake_up_all(&rdev->fence_queue);
> - }
> + return wake;
> }
Very bad idea, when sequence numbers change, you always want to wake up
the whole fence queue here.
> [SNIP]
> +/**
> + * radeon_fence_enable_signaling - enable signalling on fence
> + * @fence: fence
> + *
> + * This function is called with fence_queue lock held, and adds a callback
> + * to fence_queue that checks if this fence is signaled, and if so it
> + * signals the fence and removes itself.
> + */
> +static bool radeon_fence_enable_signaling(struct fence *f)
> +{
> + struct radeon_fence *fence = to_radeon_fence(f);
> +
> + if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >=
> fence->seq ||
> + !fence->rdev->ddev->irq_enabled)
> + return false;
> +
Do I get that right that you rely on IRQs to be enabled and working
here? Cause that would be a quite bad idea from the conceptual side.
> + radeon_irq_kms_sw_irq_get(fence->rdev, fence->ring);
> +
> + if (__radeon_fence_process(fence->rdev, fence->ring))
> + wake_up_all_locked(&fence->rdev->fence_queue);
> +
> + /* did fence get signaled after we enabled the sw irq? */
> + if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >=
> fence->seq) {
> + 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);
> +
> + return true;
> +}
> +
> /**
> * radeon_fence_signaled - check if a fence has signaled
> *
>
Christian.
