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@xxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
