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.