Re: [PATCH 10/13] drm/radeon: return -ENOENT in fence_wait_*

["Michel Dänzer" <michel@daenzer.net>]

On Fre, 2012-04-20 at 10:49 +0200, Christian König wrote: 
> On 20.04.2012 09:20, Michel Dänzer wrote:
> > On Fre, 2012-04-20 at 00:39 +0200, Christian König wrote:
> >> Signed-off-by: Christian König<deathsimple@vodafone.de>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_fence.c |    4 ++--
> >>   1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> >> index 1a9765a..764ab7e 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_fence.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> >> @@ -286,7 +286,7 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring)
> >>   	}
> >>   	if (list_empty(&rdev->fence_drv[ring].emitted)) {
> >>   		write_unlock_irqrestore(&rdev->fence_lock, irq_flags);
> >> -		return 0;
> >> +		return -ENOENT;
> >>   	}
> >>   	fence = list_entry(rdev->fence_drv[ring].emitted.next,
> >>   			   struct radeon_fence, list);
> >> @@ -310,7 +310,7 @@ int radeon_fence_wait_last(struct radeon_device *rdev, int ring)
> >>   	}
> >>   	if (list_empty(&rdev->fence_drv[ring].emitted)) {
> >>   		write_unlock_irqrestore(&rdev->fence_lock, irq_flags);
> >> -		return 0;
> >> +		return -ENOENT;
> >>   	}
> >>   	fence = list_entry(rdev->fence_drv[ring].emitted.prev,
> >>   			   struct radeon_fence, list);
> > It seems weird to declare a fence wait as failed when there are no
> > outstanding fences in the first place. If there are callers which
> > require outstanding fences, they should probably handle that themselves.
> 
> Why that sounds so weird? Ok, maybe for radeon_fence_wait_last that's 
> questionable,

Indeed. It happens not to break radeon_suspend_kms because it doesn't
check the return value, but otherwise it would fail spuriously.


> but for radeon_fence_wait_next it's quite clear to me that 
> we should signal the caller that there is no fence to wait for.
> 
> The problem I wanted to fix with that is the usage of 
> radeon_fence_wait_next in radeon_ring_alloc (for example):
> > int radeon_ring_alloc(struct radeon_device *rdev, struct radeon_ring 
> > *ring, unsigned ndw)
> > {
> >         int r;
> >
> >         /* Align requested size with padding so unlock_commit can
> >          * pad safely */
> >         ndw = (ndw + ring->align_mask) & ~ring->align_mask;
> >         while (ndw > (ring->ring_free_dw - 1)) {
> >                 radeon_ring_free_size(rdev, ring);
> >                 if (ndw < ring->ring_free_dw) {
> >                         break;
> >                 }
> >                 r = radeon_fence_wait_next(rdev, 
> > radeon_ring_index(rdev, ring));
> >                 if (r)
> >                         return r;
> >         }
> >         ring->count_dw = ndw;
> >         ring->wptr_old = ring->wptr;
> >         return 0;
> > }
> If the ring is full, but actually has no more fences in it (which in my 
> case was caused by my stupidity and actually shouldn't happen otherwise) 
> this loop will just busy wait with a critical mutex locked for something 
> that never happens.

My suggestion was to explicitly check for that in radeon_ring_alloc. But
I guess right now it doesn't really matter, as it's the only caller. :)


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel