> IOCTL will pass a seq value, or 0 > And 0 means it want to let KMD choose the latest seq value > > So this patch is needed for the caller which input 0 as seq value Yeah, I understand that. The problem is we already abused seq value 0 to be the "always" signaled fence number. So your patch is accidentally redefining this. But we could use something like ~0ull instead. > Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; > Fence = amdgpu_ctx_get_fence(ctx, seq); > > BTW: above code is from hybrid kernel branch, not in upstream branch > > You can see above logic actually wrong, because the consistence of the sequence is not guaranteed. > Caller should not access sequence directly, > And sequence should be accessed during protection of the ring_lock Yeah, I see that this is actually a race. Please use ~0ull to signal that we want to wait for the latest fence instead of 0. With that done the change looks fine to me. Christian. Am 07.04.2017 um 15:32 schrieb Liu, Monk: > IOCTL will pass a seq value, or 0 > And 0 means it want to let KMD choose the latest seq value > > So this patch is needed for the caller which input 0 as seq value > > I found some code invoke ctx_get_fence() like: > > Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; > Fence = amdgpu_ctx_get_fence(ctx, seq); > > BTW: above code is from hybrid kernel branch, not in upstream branch > > You can see above logic actually wrong, because the consistence of the sequence is not guaranteed. > Caller should not access sequence directly, > And sequence should be accessed during protection of the ring_lock > > > Any idea ? > > > -----é?®ä»¶å??件----- > å??件人: Christian König [mailto:deathsimple at vodafone.de] > å??é??æ?¶é?´: 2017å¹´4æ??7æ?¥ 19:28 > æ?¶ä»¶äºº: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org > 主é¢?: Re: [PATCH] drm/amdgpu:fix race condition bug > > Am 07.04.2017 um 13:26 schrieb Christian König: >> Am 07.04.2017 um 12:52 schrieb Monk Liu: >>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad >>> Signed-off-by: Monk Liu <Monk.Liu at amd.com> >> NAK, that is a not backward compatible change to the IOCTL interface. > To make it clear, something like "if(seq == ~0L)" should work. > > Christian. > >> And BTW what's the background of it? >> >> Christian. >> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >>> index 6d86eae..b8c11fe 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct >>> amdgpu_ctx *ctx, >>> spin_lock(&ctx->ring_lock); >>> + if (!seq) >>> + seq = ctx->rings[ring->idx].sequence - 1; >>> + >>> if (seq >= cring->sequence) { >>> spin_unlock(&ctx->ring_lock); >>> return ERR_PTR(-EINVAL); >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
