Re: [PULL REQUEST] ttm fence conversion

["Christian König" <deathsimple@vodafone.de>]

Am 01.09.2014 um 15:33 schrieb Maarten Lankhorst:
> Hey,
>
> Op 01-09-14 om 14:31 schreef Christian König:
>> Please wait a second with that.
>>
>> I didn't had a chance to test this yet and nobody has yet given it's rb on at least the radeon changes in this branch.
> Ok, my fault. I thought it was implicitly acked. I haven't made any functional changes to these patches,
> just some small fixups and a fix to make it apply after the upstream removal of  RADEON_FENCE_SIGNALED_SEQ.

Yeah, but the resulting patch looks to complex for my taste and should 
be simplified a bit more. Here is a more detailed review:

> +    wait_queue_t fence_wake;
Only a nitpick, but please fix the indention and maybe add a comment.

> +    struct work_struct delayed_irq_work;
Just drop that, the new fall back work item should take care of this 
when the unfortunate case happens that somebody tries to 
enable_signaling in the middle of a GPU reset.

>  /*
> - * Cast helper
> - */
> -#define to_radeon_fence(p) ((struct radeon_fence *)(p))
> -
> -/*
Please define the new cast helper in radeon.h as well.

>      if (!rdev->needs_reset) {
> -        up_write(&rdev->exclusive_lock);
> +        downgrade_write(&rdev->exclusive_lock);
> +        wake_up_all(&rdev->fence_queue);
> +        up_read(&rdev->exclusive_lock);
>          return 0;
>      }
Just drop that as well, no need to wake up anybody here.

>  downgrade_write(&rdev->exclusive_lock);
> +    wake_up_all(&rdev->fence_queue);
Same here, the IB test will wake up all fences for recheck anyway.

> + * radeon_fence_read_seq - Returns the current fence value without 
> updating
> + *
> + * @rdev: radeon_device pointer
> + * @ring: ring index to return the seqno of
> + */
> +static uint64_t radeon_fence_read_seq(struct radeon_device *rdev, int 
> ring)
> +{
> +    uint64_t last_seq = atomic64_read(&rdev->fence_drv[ring].last_seq);
> +    uint64_t last_emitted = rdev->fence_drv[ring].sync_seq[ring];
> +    uint64_t seq = radeon_fence_read(rdev, ring);
> +
> +    seq = radeon_fence_read(rdev, ring);
> +    seq |= last_seq & 0xffffffff00000000LL;
> +    if (seq < last_seq) {
> +        seq &= 0xffffffff;
> +        seq |= last_emitted & 0xffffffff00000000LL;
> +    }
> +    return seq;
> +}
Completely drop that and just check the last_seq signaled as set by 
radeon_fence_activity.

> +        if (!ret)
> +            FENCE_TRACE(&fence->base, "signaled from irq context\n");
> +        else
> +            FENCE_TRACE(&fence->base, "was already signaled\n");
Is all that text tracing necessary? Probably better define a trace point 
here.

> +    if (atomic64_read(&rdev->fence_drv[fence->ring].last_seq) >= 
> fence->seq ||
> +        !rdev->ddev->irq_enabled)
> +        return false;
Checking irq_enabled here might not be such a good idea if the fence 
code don't has a fall back on it's own. What exactly happens if 
enable_signaling returns false?

> +static signed long radeon_fence_default_wait(struct fence *f, bool intr,
> +                         signed long timeout)
> +{
> +    struct radeon_fence *fence = to_radeon_fence(f);
> +    struct radeon_device *rdev = fence->rdev;
> +    bool signaled;
> +
> +    fence_enable_sw_signaling(&fence->base);
> +
> +    /*
> +     * This function has to return -EDEADLK, but cannot hold
> +     * exclusive_lock during the wait because some callers
> +     * may already hold it. This means checking needs_reset without
> +     * lock, and not fiddling with any gpu internals.
> +     *
> +     * The callback installed with fence_enable_sw_signaling will
> +     * run before our wait_event_*timeout call, so we will see
> +     * both the signaled fence and the changes to needs_reset.
> +     */
> +
> +    if (intr)
> +        timeout = wait_event_interruptible_timeout(rdev->fence_queue,
> +                               ((signaled = 
> (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))) || 
> rdev->needs_reset),
> +                               timeout);
> +    else
> +        timeout = wait_event_timeout(rdev->fence_queue,
> +                         ((signaled = 
> (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))) || 
> rdev->needs_reset),
> +                         timeout);
> +
> +    if (timeout > 0 && !signaled)
> +        return -EDEADLK;
> +    return timeout;
> +}
This at least needs to be properly formated, but I think since we now 
don't need extra handling any more we don't need an extra wait function 
as well.

Regards,
Christian.