[5/7] drm/amdkfd: Remove duplicate pqm_uninit()

Submitted by Edward O'Callaghan on Sept. 10, 2016, 1:31 a.m.

Details

Message ID 1473471099-16914-6-git-send-email-funfunctor@folklore1984.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Edward O'Callaghan Sept. 10, 2016, 1:31 a.m.
pqm_uninit() will be called in kfd_process_notifier_release(), which
is when the process exits. Calling it in kfd_unbind_process_from_device()
is duplicate and in fact incorrect, because pqm should not be
uninitalized as long as the process still exists.

Original author: Yong Zhao <yong.zhao@amd.com>
Hand ported for mainline.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
 1 file changed, 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 8d78052..ef2266a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -427,8 +427,6 @@  void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
 			if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
 				kfd_dbgmgr_destroy(dev->dbgmgr);
 
-			pqm_uninit(&p->pqm);
-
 			pdd = kfd_get_process_device_data(dev, p);
 
 			if (!pdd) {

Comments

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> pqm_uninit() will be called in kfd_process_notifier_release(), which
> is when the process exits. Calling it in kfd_unbind_process_from_device()
> is duplicate and in fact incorrect, because pqm should not be
> uninitalized as long as the process still exists.

Hmm, I don't agree with this patch and the above explanation.

kfd_unbind_process_from_device is called from the IOMMUv2 driver, when
its callback from the mm subsystem is called (mn_release). That
callback is called when the process _is_ shutting down. The IOMMUv2
driver then unmaps all its mappings, and if we won't destroy our
queues before that happens (as pqm_uninit does), and we have a pending
operation on the queue which would get submitted before
kfd_process_notifier_release() is called, then I believe we may get an
IOMMU fault.

    Oded

>
> Original author: Yong Zhao <yong.zhao@amd.com>
> Hand ported for mainline.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 8d78052..ef2266a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -427,8 +427,6 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
>                         if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
>                                 kfd_dbgmgr_destroy(dev->dbgmgr);
>
> -                       pqm_uninit(&p->pqm);
> -
>                         pdd = kfd_get_process_device_data(dev, p);
>
>                         if (!pdd) {
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Yong is currently working on a follow-up fix to handle process shutdown
on APUs as well as S3 suspend correctly.

Regards,
  Felix


On 16-09-10 12:06 PM, Oded Gabbay wrote:
> On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
> <funfunctor@folklore1984.net> wrote:
>> pqm_uninit() will be called in kfd_process_notifier_release(), which
>> is when the process exits. Calling it in kfd_unbind_process_from_device()
>> is duplicate and in fact incorrect, because pqm should not be
>> uninitalized as long as the process still exists.
> Hmm, I don't agree with this patch and the above explanation.
>
> kfd_unbind_process_from_device is called from the IOMMUv2 driver, when
> its callback from the mm subsystem is called (mn_release). That
> callback is called when the process _is_ shutting down. The IOMMUv2
> driver then unmaps all its mappings, and if we won't destroy our
> queues before that happens (as pqm_uninit does), and we have a pending
> operation on the queue which would get submitted before
> kfd_process_notifier_release() is called, then I believe we may get an
> IOMMU fault.
>
>     Oded
>
>> Original author: Yong Zhao <yong.zhao@amd.com>
>> Hand ported for mainline.
>>
>> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 8d78052..ef2266a 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -427,8 +427,6 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
>>                         if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
>>                                 kfd_dbgmgr_destroy(dev->dbgmgr);
>>
>> -                       pqm_uninit(&p->pqm);
>> -
>>                         pdd = kfd_get_process_device_data(dev, p);
>>
>>                         if (!pdd) {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx