Re: [PATCH 13/17] drm/radeon: Remove custom dma_fence_ops->wait implementation

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Apr 29, 2018 at 09:08:31AM +0200, Christian König wrote:
> NAK, there is a subtitle but major difference:
> 
> > -		if (rdev->needs_reset) {
> > -			t = -EDEADLK;
> > -			break;
> > -		}
> 
> Without that the whole radeon GPU reset code breaks.

Oops I've missed that. How does this work when you register a callback
using ->enable_signaling and then block on it? Everything just dies?

We have lots of users of that for buffer/fence sharing. A really ugly, but
probably working fix for this would be a kthread worker that just looks
for ->needs_reset and force-completes all fences with
dma_fence_set_error(-EIO), which is kinda what's supposed to happen here
anyway.
-Daniel

> 
> Regards,
> Christian.
> 
> 
> Am 27.04.2018 um 08:17 schrieb Daniel Vetter:
> > It's a copy of dma_fence_default_wait, written slightly differently.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> >   drivers/gpu/drm/radeon/radeon_fence.c | 63 ---------------------------
> >   1 file changed, 63 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> > index e86f2bd38410..32690a525bfc 100644
> > --- a/drivers/gpu/drm/radeon/radeon_fence.c
> > +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> > @@ -1051,72 +1051,9 @@ static const char *radeon_fence_get_timeline_name(struct dma_fence *f)
> >   	}
> >   }
> > -static inline bool radeon_test_signaled(struct radeon_fence *fence)
> > -{
> > -	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags);
> > -}
> > -
> > -struct radeon_wait_cb {
> > -	struct dma_fence_cb base;
> > -	struct task_struct *task;
> > -};
> > -
> > -static void
> > -radeon_fence_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> > -{
> > -	struct radeon_wait_cb *wait =
> > -		container_of(cb, struct radeon_wait_cb, base);
> > -
> > -	wake_up_process(wait->task);
> > -}
> > -
> > -static signed long radeon_fence_default_wait(struct dma_fence *f, bool intr,
> > -					     signed long t)
> > -{
> > -	struct radeon_fence *fence = to_radeon_fence(f);
> > -	struct radeon_device *rdev = fence->rdev;
> > -	struct radeon_wait_cb cb;
> > -
> > -	cb.task = current;
> > -
> > -	if (dma_fence_add_callback(f, &cb.base, radeon_fence_wait_cb))
> > -		return t;
> > -
> > -	while (t > 0) {
> > -		if (intr)
> > -			set_current_state(TASK_INTERRUPTIBLE);
> > -		else
> > -			set_current_state(TASK_UNINTERRUPTIBLE);
> > -
> > -		/*
> > -		 * radeon_test_signaled must be called after
> > -		 * set_current_state to prevent a race with wake_up_process
> > -		 */
> > -		if (radeon_test_signaled(fence))
> > -			break;
> > -
> > -		if (rdev->needs_reset) {
> > -			t = -EDEADLK;
> > -			break;
> > -		}
> > -
> > -		t = schedule_timeout(t);
> > -
> > -		if (t > 0 && intr && signal_pending(current))
> > -			t = -ERESTARTSYS;
> > -	}
> > -
> > -	__set_current_state(TASK_RUNNING);
> > -	dma_fence_remove_callback(f, &cb.base);
> > -
> > -	return t;
> > -}
> > -
> >   const struct dma_fence_ops radeon_fence_ops = {
> >   	.get_driver_name = radeon_fence_get_driver_name,
> >   	.get_timeline_name = radeon_fence_get_timeline_name,
> >   	.enable_signaling = radeon_fence_enable_signaling,
> >   	.signaled = radeon_fence_is_signaled,
> > -	.wait = radeon_fence_default_wait,
> > -	.release = NULL,
> >   };
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx