[PATCHv2 2/3] drm/amdkfd: Update queue unmap after VM fault with MES

[Kasiviswanathan, Harish]

[AMD Official Use Only - AMD Internal Distribution Only]

-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Felix Kuehling
Sent: Thursday, August 15, 2024 2:25 PM
To: Joshi, Mukul <Mukul.Joshi at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: Re: [PATCHv2 2/3] drm/amdkfd: Update queue unmap after VM fault with MES

On 2024-08-14 19:27, Mukul Joshi wrote:
> MEC FW expects MES to unmap all queues when a VM fault is observed
> on a queue and then resumed once the affected process is terminated.
> Use the MES Suspend and Resume APIs to achieve this.
>
> Signed-off-by: Mukul Joshi <mukul.joshi at amd.com>
> ---
> v1->v2:
> - Add MES FW version check.
> - Separate out the kfd_dqm_evict_pasid into another function.
> - Use amdgpu_mes_suspend/amdgpu_mes_resume to suspend/resume queues.
>
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c | 79 ++++++++++++++++++-
>   1 file changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index f6e211070299..cb5b866eee3b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -319,6 +319,42 @@ static int remove_all_queues_mes(struct device_queue_manager *dqm)
>       return retval;
>   }
>
> +static int suspend_all_queues_mes(struct device_queue_manager *dqm)
> +{
> +     struct amdgpu_device *adev = (struct amdgpu_device *)dqm->dev->adev;
> +     int r = 0;
> +
> +     if (dqm->is_hws_hang)
> +             return -EIO;


This check might need to be reconciled with the following commit. May be check has to be
if (!down_read_trylock(&dqm->dev->adev->reset_domain->sem))
                return -EIO;

git show d4ed16fa760e79235ef214318a8389ecedcb9ad6 --stat
commit d4ed16fa760e79235ef214318a8389ecedcb9ad6
Author: Yunxiang Li <Yunxiang.Li at amd.com>
Date:   Fri May 24 13:46:50 2024 -0400

    drm/amdgpu/kfd: remove is_hws_hang and is_resetting

> +
> +     r = amdgpu_mes_suspend(adev);
> +     if (r) {
> +             dev_err(adev->dev, "failed to suspend gangs from MES\n");
> +             dev_err(adev->dev, "MES might be in unrecoverable state, issue a GPU reset\n");
> +             kfd_hws_hang(dqm);
> +     }
> +
> +     return r;
> +}
> +
> +static int resume_all_queues_mes(struct device_queue_manager *dqm)
> +{
> +     struct amdgpu_device *adev = (struct amdgpu_device *)dqm->dev->adev;
> +     int r = 0;
> +
> +     if (dqm->is_hws_hang)
> +             return -EIO;
> +
> +     r = amdgpu_mes_resume(adev);
> +     if (r) {
> +             dev_err(adev->dev, "failed to resume gangs from MES\n");
> +             dev_err(adev->dev, "MES might be in unrecoverable state, issue a GPU reset\n");
> +             kfd_hws_hang(dqm);
> +     }
> +
> +     return r;
> +}
> +
>   static void increment_queue_count(struct device_queue_manager *dqm,
>                                 struct qcm_process_device *qpd,
>                                 struct queue *q)
> @@ -2835,6 +2871,40 @@ void device_queue_manager_uninit(struct device_queue_manager *dqm)
>       kfree(dqm);
>   }
>
> +static int kfd_dqm_evict_pasid_mes(struct device_queue_manager *dqm,
> +                                struct qcm_process_device *qpd)
> +{
> +     struct device *dev = dqm->dev->adev->dev;
> +     int ret = 0;
> +
> +     /* Check if process is already evicted */
> +     dqm_lock(dqm);
> +     if (qpd->evicted) {
> +             dqm_unlock(dqm);
> +             goto out;

qpd->evicted is a reference count. Without this shortcut,
dqm->ops.evict_process_queues will increment the ref count. You probably
need to increment it here before dropping the lock. Otherwise two things
can go wrong:

 1. The corresponding dqm->ops.restore_process_queues will underflow the
    reference count
 2. A race condition where the queues get restored too early

Regards,
   Felix


> +     }
> +     dqm_unlock(dqm);
> +
> +     ret = suspend_all_queues_mes(dqm);
> +     if (ret) {
> +             dev_err(dev, "Suspending all queues failed");
> +             goto out;
> +     }
> +
> +     ret = dqm->ops.evict_process_queues(dqm, qpd);
> +     if (ret) {
> +             dev_err(dev, "Evicting process queues failed");
> +             goto out;
> +     }
> +
> +     ret = resume_all_queues_mes(dqm);
> +     if (ret)
> +             dev_err(dev, "Resuming all queues failed");
> +
> +out:
> +     return ret;
> +}
> +
>   int kfd_dqm_evict_pasid(struct device_queue_manager *dqm, u32 pasid)
>   {
>       struct kfd_process_device *pdd;
> @@ -2845,8 +2915,13 @@ int kfd_dqm_evict_pasid(struct device_queue_manager *dqm, u32 pasid)
>               return -EINVAL;
>       WARN(debug_evictions, "Evicting pid %d", p->lead_thread->pid);
>       pdd = kfd_get_process_device_data(dqm->dev, p);
> -     if (pdd)
> -             ret = dqm->ops.evict_process_queues(dqm, &pdd->qpd);
> +     if (pdd) {
> +             if (dqm->dev->kfd->shared_resources.enable_mes)
> +                     ret = kfd_dqm_evict_pasid_mes(dqm, &pdd->qpd);
> +             else
> +                     ret = dqm->ops.evict_process_queues(dqm, &pdd->qpd);
> +     }
> +
>       kfd_unref_process(p);
>
>       return ret;