From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?windows-1252?Q?Christian_K=F6nig?= Subject: Re: [PULL REQUEST] ttm fence conversion Date: Mon, 01 Sep 2014 18:21:45 +0200 Message-ID: <54049D19.30802@vodafone.de> References: <540459E3.9060406@canonical.com> <54046722.1000207@vodafone.de> <540475A6.2020603@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from pegasos-out.vodafone.de (pegasos-out.vodafone.de [80.84.1.38]) by gabe.freedesktop.org (Postfix) with ESMTP id 9A6D189CD7 for ; Mon, 1 Sep 2014 09:22:27 -0700 (PDT) In-Reply-To: <540475A6.2020603@canonical.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Maarten Lankhorst , Dave Airlie Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org Am 01.09.2014 um 15:33 schrieb Maarten Lankhorst: > Hey, > > Op 01-09-14 om 14:31 schreef Christian K=F6nig: >> 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 funct= ional changes to these patches, > just some small fixups and a fix to make it apply after the upstream remo= val 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 =3D atomic64_read(&rdev->fence_drv[ring].last_seq); > + uint64_t last_emitted =3D rdev->fence_drv[ring].sync_seq[ring]; > + uint64_t seq =3D radeon_fence_read(rdev, ring); > + > + seq =3D radeon_fence_read(rdev, ring); > + seq |=3D last_seq & 0xffffffff00000000LL; > + if (seq < last_seq) { > + seq &=3D 0xffffffff; > + seq |=3D 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) >=3D = > 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 =3D to_radeon_fence(f); > + struct radeon_device *rdev =3D 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 =3D wait_event_interruptible_timeout(rdev->fence_queue, > + ((signaled =3D = > (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))) || = > rdev->needs_reset), > + timeout); > + else > + timeout =3D wait_event_timeout(rdev->fence_queue, > + ((signaled =3D = > (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.