在 2024-06-17星期一的 15:59 +0200,Christian König写道: > Am 17.06.24 um 15:43 schrieb Icenowy Zheng: > > 在 2024-06-17星期一的 15:09 +0200,Christian König写道: > > > Am 17.06.24 um 15:03 schrieb Icenowy Zheng: > > > > 在 2024-06-17星期一的 14:35 +0200,Christian König写道: > > > > > Am 17.06.24 um 12:58 schrieb Icenowy Zheng: > > > > > > The duplication of EOP packets for GFX7/8, with the former > > > > > > one > > > > > > have > > > > > > seq-1 written and the latter one have seq written, seems to > > > > > > confuse > > > > > > some > > > > > > hardware platform (e.g. Loongson 7A series PCIe > > > > > > controllers). > > > > > > > > > > > > Make the content of the duplicated EOP packet the same with > > > > > > the > > > > > > real > > > > > > one, only masking any possible interrupts. > > > > > Well completely NAK to that, exactly that disables the > > > > > workaround. > > > > > > > > > > The CPU needs to see two different values written here. > > > > Why do the CPU need to see two different values here? Only the > > > > second > > > > packet will raise an interrupt before and after applying this > > > > patch, > > > > and the first packet's result should just be overriden on > > > > ordinary > > > > platforms. The CPU won't see the first one, until it's polling > > > > for > > > > the > > > > address for a very short interval, so short that the GPU CP > > > > couldn't > > > > execute 2 commands. > > > Yes exactly that. We need to make two writes, one with the old > > > value > > > (seq - 1) and a second with the real value (seq). > > > > > > Otherwise it is possible that a polling CPU would see the > > > sequence > > > before the second EOP is issued with results in incoherent view > > > of > > > memory. > > In this case shouldn't we write seq-1 before any work, and then > > write > > seq after work, like what is done in Mesa? > > No. This hw workaround requires that two consecutive write operations > happen directly behind each other on the PCIe bus with two different > values. Well to be honest the workaround code in Mesa seems to not be working in this way ... > > To make the software logic around that work without any changes we > use > the values seq - 1 and seq because those are guaranteed to be > different > and not trigger any unwanted software behavior. > > Only then we can guarantee that we have a coherent view of system > memory. Any more details about it? BTW in this case, could I try to write it for 3 times instead of 2, with seq-1, seq and seq? > > > As what I see, Mesa uses another command buffer to emit a > > EVENT_WRITE_EOP writing 0, and commit this command buffer before > > the > > real command buffer. > > > > > > Or do you mean the GPU needs to see two different values being > > > > written, > > > > or they will be merged into only one write request? > > > > > > > > Please give out more information about this workaround, > > > > otherwise > > > > the > > > > GPU hang problem on Loongson platforms will persist. > > > Well if Loongson can't handle two consecutive write operations to > > > the > > > same address with different values then you have a massive > > > platform > > > bug. > > I think the issue is triggered when two consecutive write > > operations > > and one IRQ is present, which is exactly the case of this function. > > Well then you have a massive platform bug. > > Two consecutive writes to the same bus address are perfectly legal > from > the PCIe specification and can happen all the time, even without this > specific hw workaround. Yes I know it, and I am not from Loongson, just some user trying to mess around it. > > Regards, > Christian. > > > > > > That is something which can happen all the time throughout the > > > operation. > > > > > > Regards, > > > Christian. > > > > > > > > Regards, > > > > > Christian. > > > > > > > > > > > Fixes: bf26da927a1c ("drm/amdgpu: add cache flush > > > > > > workaround to > > > > > > gfx8 emit_fence") > > > > > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK > > > > > > parts") > > > > > > Signed-off-by: Icenowy Zheng <uwu@xxxxxxxxxx> > > > > > > --- > > > > > > drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 12 +++++------- > > > > > > drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 12 ++++-------- > > > > > > 2 files changed, 9 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > > > > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > > > > > index 541dbd70d8c75..778f27f1a34fe 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > > > > > @@ -2117,9 +2117,8 @@ static void > > > > > > gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 > > > > > > addr, > > > > > > { > > > > > > bool write64bit = flags & > > > > > > AMDGPU_FENCE_FLAG_64BIT; > > > > > > bool int_sel = flags & AMDGPU_FENCE_FLAG_INT; > > > > > > - /* Workaround for cache flush problems. First send > > > > > > a > > > > > > dummy > > > > > > EOP > > > > > > - * event down the pipe with seq one below. > > > > > > - */ > > > > > > + > > > > > > + /* Workaround for cache flush problems, send EOP > > > > > > twice. > > > > > > */ > > > > > > amdgpu_ring_write(ring, > > > > > > PACKET3(PACKET3_EVENT_WRITE_EOP, > > > > > > 4)); > > > > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN | > > > > > > EOP_TC_ACTION_EN | > > > > > > @@ -2127,11 +2126,10 @@ static void > > > > > > gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 > > > > > > addr, > > > > > > EVENT_INDEX(5))); > > > > > > amdgpu_ring_write(ring, addr & 0xfffffffc); > > > > > > amdgpu_ring_write(ring, (upper_32_bits(addr) & > > > > > > 0xffff) > > > > > > > > > > > > > - DATA_SEL(1) | INT_SEL(0)); > > > > > > - amdgpu_ring_write(ring, lower_32_bits(seq - 1)); > > > > > > - amdgpu_ring_write(ring, upper_32_bits(seq - 1)); > > > > > > + DATA_SEL(write64bit ? 2 : > > > > > > 1) | > > > > > > INT_SEL(0)); > > > > > > + amdgpu_ring_write(ring, lower_32_bits(seq)); > > > > > > + amdgpu_ring_write(ring, upper_32_bits(seq)); > > > > > > > > > > > > - /* Then send the real EOP event down the pipe. */ > > > > > > amdgpu_ring_write(ring, > > > > > > PACKET3(PACKET3_EVENT_WRITE_EOP, > > > > > > 4)); > > > > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN | > > > > > > EOP_TC_ACTION_EN | > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > > > > > b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > > > > > index 2f0e72caee1af..39a7d60f1fd69 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > > > > > @@ -6153,9 +6153,7 @@ static void > > > > > > gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 > > > > > > addr, > > > > > > bool write64bit = flags & > > > > > > AMDGPU_FENCE_FLAG_64BIT; > > > > > > bool int_sel = flags & AMDGPU_FENCE_FLAG_INT; > > > > > > > > > > > > - /* Workaround for cache flush problems. First send > > > > > > a > > > > > > dummy > > > > > > EOP > > > > > > - * event down the pipe with seq one below. > > > > > > - */ > > > > > > + /* Workaround for cache flush problems, send EOP > > > > > > twice. > > > > > > */ > > > > > > amdgpu_ring_write(ring, > > > > > > PACKET3(PACKET3_EVENT_WRITE_EOP, > > > > > > 4)); > > > > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN | > > > > > > EOP_TC_ACTION_EN | > > > > > > @@ -6164,12 +6162,10 @@ static void > > > > > > gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 > > > > > > addr, > > > > > > EVENT_INDEX(5))); > > > > > > amdgpu_ring_write(ring, addr & 0xfffffffc); > > > > > > amdgpu_ring_write(ring, (upper_32_bits(addr) & > > > > > > 0xffff) > > > > > > > > > > > > > - DATA_SEL(1) | INT_SEL(0)); > > > > > > - amdgpu_ring_write(ring, lower_32_bits(seq - 1)); > > > > > > - amdgpu_ring_write(ring, upper_32_bits(seq - 1)); > > > > > > + DATA_SEL(write64bit ? 2 : 1) | > > > > > > INT_SEL(0)); > > > > > > + amdgpu_ring_write(ring, lower_32_bits(seq)); > > > > > > + amdgpu_ring_write(ring, upper_32_bits(seq)); > > > > > > > > > > > > - /* Then send the real EOP event down the pipe: > > > > > > - * EVENT_WRITE_EOP - flush caches, send int */ > > > > > > amdgpu_ring_write(ring, > > > > > > PACKET3(PACKET3_EVENT_WRITE_EOP, > > > > > > 4)); > > > > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN | > > > > > > EOP_TC_ACTION_EN | >