[PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

[Christian König]

Am 12.03.21 um 11:24 schrieb Nirmoy:
>
> On 3/12/21 10:52 AM, Christian König wrote:
>> Am 12.03.21 um 10:49 schrieb Nirmoy Das:
>>> KIQ ring is being operated by kfd as well as amdgpu.
>>> KFD is using kiq lock, we should the same from amdgpu side
>>> as well.
>>
>> Ah, now I knew which functions you mean. This is not strictly 
>> necessary because that stuff is only called during bootup and not 
>> later on.
>
>
> OK, not so serious issue as I was thinking then.
>
>
>>
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index 1915b9b95106..8fe370e5358d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -467,13 +467,17 @@ int amdgpu_gfx_disable_kcq(struct 
>>> amdgpu_device *adev)
>>>       if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
>>>           return -EINVAL;
>>>   +    spin_lock(&adev->gfx.kiq.ring_lock);
>>>       if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
>>> -                    adev->gfx.num_compute_rings))
>>> +                    adev->gfx.num_compute_rings)) {
>>> +        spin_unlock(&adev->gfx.kiq.ring_lock);
>>>           return -ENOMEM;
>>> +    }
>>>         for (i = 0; i < adev->gfx.num_compute_rings; i++)
>>>           kiq->pmf->kiq_unmap_queues(kiq_ring, 
>>> &adev->gfx.compute_ring[i],
>>>                          RESET_QUEUES, 0, 0);
>>> +    spin_unlock(&adev->gfx.kiq.ring_lock);
>>>         return amdgpu_ring_test_helper(kiq_ring);
>>
>> The ring test accesses the ring buffer as well.
>
>
> Shall I send a v2 ?

I'm not 100% sure. Please test this with lockdep enabled, if that 
doesn't raise anything we can probably commit it just to be on the clean 
side.

Regards,
Christian.

>
>
> Thanks,
>
> Nirmoy
>
>
>>
>>
>> Regards,
>> Christian.
>>
>>>   }
>>> @@ -518,18 +522,20 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device 
>>> *adev)
>>>         DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, 
>>> kiq_ring->pipe,
>>>                               kiq_ring->queue);
>>> -
>>> +    spin_lock(&adev->gfx.kiq.ring_lock);
>>>       r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
>>>                       adev->gfx.num_compute_rings +
>>>                       kiq->pmf->set_resources_size);
>>>       if (r) {
>>>           DRM_ERROR("Failed to lock KIQ (%d).\n", r);
>>> +        spin_unlock(&adev->gfx.kiq.ring_lock);
>>>           return r;
>>>       }
>>>         kiq->pmf->kiq_set_resources(kiq_ring, queue_mask);
>>>       for (i = 0; i < adev->gfx.num_compute_rings; i++)
>>>           kiq->pmf->kiq_map_queues(kiq_ring, 
>>> &adev->gfx.compute_ring[i]);
>>> +    spin_unlock(&adev->gfx.kiq.ring_lock);
>>>         r = amdgpu_ring_test_helper(kiq_ring);
>>>       if (r)
>>