[PATCH v3 3/6] drm/amdgpu: delay the use of amdgpu_vm_set_task_info

[Tvrtko Ursulin]

On 24/09/2024 09:23, Christian König wrote:
> Am 23.09.24 um 12:25 schrieb Tvrtko Ursulin:
>>
>> On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote:
>>> At this point the vm is locked so we safely modify it without risk of
>>> concurrent access.
>>
>> To which particular lock this is referring to and does this imply 
>> previous placement was unsafe?
> 
> We use the root PDs dma_resv object as VM lock to protect most field 
> inside the VM structure, only a few are protected by an additional 
> spinlock.
> 
> And yes, previously it was possible that you got a mangled process/task 
> name because no lock was protecting the task_info structure.

Got it, thanks Christian!

In this case I only suggest to be more explicit in the commit message 
and clearly say it is fixing an existing bug. Like it stands I wasn't 
sure if it was that, or the movement was just enabling the changes which 
come later in the series.

Regards,

Tvrtko

>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 1e475eb01417..891128ecee6d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -309,9 +309,6 @@ static int amdgpu_cs_pass1(struct 
>>> amdgpu_cs_parser *p,
>>>           p->gang_leader->uf_addr = uf_offset;
>>>       kvfree(chunk_array);
>>>   -    /* Use this opportunity to fill in task info for the vm */
>>> -    amdgpu_vm_set_task_info(vm);
>>> -
>>>       return 0;
>>>     free_all_kdata:
>>> @@ -1180,6 +1177,9 @@ static int amdgpu_cs_vm_handling(struct 
>>> amdgpu_cs_parser *p)
>>>           job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>>>       }
>>>   +    /* Use this opportunity to fill in task info for the vm */
>>> +    amdgpu_vm_set_task_info(vm);
>>> +
>>>       if (adev->debug_vm) {
>>>           /* Invalidate all BOs to test for userspace bugs */
>>>           amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>