| Message ID | 20240920090920.1342694-4-pierre-eric.pelloux-prayer@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM_SET_NAME ioctl | expand |
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? Regards, Tvrtko > Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@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) {
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. Regards, Christian. > > Regards, > > Tvrtko > >> Signed-off-by: Pierre-Eric Pelloux-Prayer >> <pierre-eric.pelloux-prayer@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) {
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@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) { >
Le 24/09/2024 à 10:43, Tvrtko Ursulin a écrit : > > 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. Good idea, will do. Pierre-Eric > > Regards, > > Tvrtko > >>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@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) { >>
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) {
At this point the vm is locked so we safely modify it without risk of concurrent access. Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)