On 11/30/2018 10:53 AM, Koenig, Christian wrote:
> Am 30.11.18 um 16:14 schrieb Grodzovsky, Andrey:
>> On 11/30/2018 04:03 AM, Christian König wrote:
>>> Am 29.11.18 um 21:36 schrieb Andrey Grodzovsky:
>>>> XGMI hive has some resources allocted on device init which
>>>> needs to be deallocated when the device is unregistered.
>>>>
>>>> Add per hive wq to allow all the nodes in hive to run resets
>>>> concurently - this should speed up the total reset time to avoid
>>>> breaching the PSP FW timeout.
>>> Do you really want a workqueue? That sounds like the exact opposite of
>>> what you describe here.
>>>
>>> E.g. a workqueue is there to serialize all work items scheduled to it.
>>>
>>> What you most likely rather want is a work item per device which are
>>> scheduled, run in parallel and are joined back together before
>>> dropping the per hive lock.
>>>
>>> Christian.
>> As you can see in second patch that exactly how I use this wq. I define
>> per adev work item all of which I enqueue during GPU reset routine.
>> I believe the wq I created will processes work items in parallel since
>> it's is served by per CPU worker thread (thread pool) while a
>> serializing wq is
>> created using the 'create_singlethread_workqueue' interface which
>> creates an ordered wq which process at most one item simultaneously.
> That is correct, but you are still limited to one work item processed
> per CPU which is not necessary what you want.
I was wrong to say it's single worker thread per CPU, it's actually work
thread pool
per CPU, so even with single CPU in the system you get concurrency of
execution.
>
>> I did have my doubts about creating a dedicated wq instead of reusing
>> existing system wide wq such as system_unbound_wq because this adds new
>> kernel threads but eventually I opted for my own queue because there is
>> a hard requirement from the PSP FW to complete all ASIC resets in
>> defined finite amount
>> of time (1s i believe) otherwise some nodes in the XGMI hive might
>> switch to single GPU mode due to timeout.
>> If I rely on system wide wq then during stress times this queue might
>> fail to process my work items within that time limit because it's
>> overloaded by work items from other queue clients.
>> Maybe I should squash the 2 changes into one to make all of this more
>> clear ?
> No, that won't help. You would rather need to document that properly.
>
> But creating a wq just for a rarely used device reset is completely
> overkill.
>
> Stuff like that should go to the system_highpri_wq work queue which
> should make already sure that it never hits the PSP timeout.
>
> Christian.
I agree in general with that, but it looks like system_highpri_wq is
'BOUND' wq meaning it's items are
limited to the thread pool of the CPU from which they were queued only,
anyway, since I mostly just wait in sleep inside PSP reset
function that should be still OK.
Will try that.
Andrey
>
>> Andrey
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 24 ++++++++++++++++++++++++
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 3 +++
>>>> 2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>>> index fb37e69..9ac2dc5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>>> @@ -61,6 +61,8 @@ struct amdgpu_hive_info
>>>> *amdgpu_get_xgmi_hive(struct amdgpu_device *adev)
>>>> INIT_LIST_HEAD(&tmp->device_list);
>>>> mutex_init(&tmp->hive_lock);
>>>> + tmp->reset_queue = alloc_workqueue("xgmi-hive", WQ_UNBOUND |
>>>> WQ_HIGHPRI, 0);
>>>> +
>>>> return tmp;
>>>> }
>>>> @@ -135,3 +137,25 @@ int amdgpu_xgmi_add_device(struct
>>>> amdgpu_device *adev)
>>>> mutex_unlock(&xgmi_mutex);
>>>> return ret;
>>>> }
>>>> +
>>>> +void amdgpu_xgmi_remove_device(struct amdgpu_device *adev)
>>>> +{
>>>> + struct amdgpu_hive_info *hive;
>>>> +
>>>> + if ((adev->asic_type < CHIP_VEGA20) || (adev->flags & AMD_IS_APU))
>>>> + return;
>>>> +
>>>> + mutex_lock(&xgmi_mutex);
>>>> +
>>>> + hive = amdgpu_get_xgmi_hive(adev);
>>>> + if (!hive)
>>>> + goto exit;
>>>> +
>>>> + if (!(hive->number_devices--)) {
>>>> + mutex_destroy(&hive->hive_lock);
>>>> + destroy_workqueue(hive->reset_queue);
>>>> + }
>>>> +
>>>> +exit:
>>>> + mutex_unlock(&xgmi_mutex);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>>> index 6335bfd..285ab93 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>>> @@ -30,10 +30,13 @@ struct amdgpu_hive_info {
>>>> struct psp_xgmi_topology_info topology_info;
>>>> int number_devices;
>>>> struct mutex hive_lock;
>>>> + /* hive members reset wq */
>>>> + struct workqueue_struct *reset_queue;
>>>> };
>>>> struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct
>>>> amdgpu_device *adev);
>>>> int amdgpu_xgmi_update_topology(struct amdgpu_hive_info *hive,
>>>> struct amdgpu_device *adev);
>>>> int amdgpu_xgmi_add_device(struct amdgpu_device *adev);
>>>> +void amdgpu_xgmi_remove_device(struct amdgpu_device *adev);
>>>> #endif
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
