On 5/28/25 15:38, Alex Deucher wrote:
> On Wed, May 28, 2025 at 7:40 AM Christian König
> <christian.koe...@amd.com> wrote:
>>
>> On 5/28/25 06:19, Alex Deucher wrote:
>>> Re-emit the unprocessed state after resetting the queue.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++++++-------
>>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> index 3193eb88b6889..f6e04cf21abcc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> @@ -9537,6 +9537,7 @@ static int gfx_v10_0_reset_kgq(struct amdgpu_ring
>>> *ring, unsigned int vmid)
>>> struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
>>> struct amdgpu_ring *kiq_ring = &kiq->ring;
>>> unsigned long flags;
>>> + unsigned int i;
>>> u32 tmp;
>>> u64 addr;
>>> int r;
>>> @@ -9571,10 +9572,8 @@ static int gfx_v10_0_reset_kgq(struct amdgpu_ring
>>> *ring, unsigned int vmid)
>>> SOC15_REG_OFFSET(GC, 0,
>>> mmCP_VMID_RESET), 0, 0xffffffff);
>>> kiq->pmf->kiq_map_queues(kiq_ring, ring);
>>> amdgpu_ring_commit(kiq_ring);
>>> -
>>> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>> -
>>> r = amdgpu_ring_test_ring(kiq_ring);
>>
>> I don't think we should do a ring test on the KIQ here That basically
>> doesn't tells as much and might cause additional problems.
>
> We need some way to wait for the KIQ submission to complete. This is
> a simple way to accomplish that.
Ok, that makes sense.
>
>>
>>> + spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>> if (r)
>>> return r;
>>>
>>> @@ -9584,7 +9583,15 @@ static int gfx_v10_0_reset_kgq(struct amdgpu_ring
>>> *ring, unsigned int vmid)
>>> return r;
>>> }
>>>
>>> - return amdgpu_ring_test_ring(ring);
>>> + if (amdgpu_ring_alloc(ring, 8 + ring->ring_backup_entries_to_copy))
>>> + return -ENOMEM;
>>> + amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
>>> + ring->ring_backup_seq, 0);
>>> + for (i = 0; i < ring->ring_backup_entries_to_copy; i++)
>>> + amdgpu_ring_write(ring, ring->ring_backup[i]);
>>> + amdgpu_ring_commit(ring);
>>
>> I'm not sure if the commands are always relocatable. We should probably just
>> instruct the ring to re-start with the original RPTR/WPTR.
>>
>> That would also avoid the need to save/restore the ring content with the CPU.
>
> I tried that originally, but I couldn't make it work for a few reasons:
> 1. We need to have enforce isolation enabled otherwise we almost
> always reset the wrong VMID so then when we execute the rest of the
> pipeline, we hang again.
> 2. When enforce isolation is enabled, we need to signal the fence
> associated with the guilty job first otherwise we get stuck waiting on
> the pipeline sync when we execute the rest of the pipeline
So we execute the guilty job submission again? What prevents that one from
running?
Christian.
>
> Alex
>
>>
>> Regards,
>> Christian.
>>
>>> +
>>> + return gfx_v10_0_ring_test_ib(ring, AMDGPU_QUEUE_RESET_TIMEOUT);
>>> }
>>>
>>> static int gfx_v10_0_reset_kcq(struct amdgpu_ring *ring,
>>> @@ -9612,9 +9619,8 @@ static int gfx_v10_0_reset_kcq(struct amdgpu_ring
>>> *ring,
>>> kiq->pmf->kiq_unmap_queues(kiq_ring, ring, RESET_QUEUES,
>>> 0, 0);
>>> amdgpu_ring_commit(kiq_ring);
>>> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>> -
>>> r = amdgpu_ring_test_ring(kiq_ring);
>>> + spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>> if (r)
>>> return r;
>>>
>>> @@ -9891,7 +9897,6 @@ static const struct amdgpu_ring_funcs
>>> gfx_v10_0_ring_funcs_gfx = {
>>> .emit_wreg = gfx_v10_0_ring_emit_wreg,
>>> .emit_reg_wait = gfx_v10_0_ring_emit_reg_wait,
>>> .emit_reg_write_reg_wait = gfx_v10_0_ring_emit_reg_write_reg_wait,
>>> - .soft_recovery = gfx_v10_0_ring_soft_recovery,
>>> .emit_mem_sync = gfx_v10_0_emit_mem_sync,
>>> .reset = gfx_v10_0_reset_kgq,
>>> .emit_cleaner_shader = gfx_v10_0_ring_emit_cleaner_shader,
>>
