Re: [RFC 4/6] drm/amdgpu: Serialize non TDR gpu recovery with TDRs

[Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>]

On 2021-12-21 2:59 a.m., Christian König wrote:
> Am 20.12.21 um 23:17 schrieb Andrey Grodzovsky:
> > On 2021-12-20 2:20 a.m., Christian König wrote:
> > > Am 17.12.21 um 23:27 schrieb Andrey Grodzovsky:
> > > > Use reset domain wq also for non TDR gpu recovery trigers
> > > > such as sysfs and RAS. We must serialize all possible
> > > > GPU recoveries to gurantee no concurrency there.
> > > > For TDR call the original recovery function directly since
> > > > it's already executed from within the wq. For others just
> > > > use a wrapper to qeueue work and wait on it to finish.
> > > >
> > > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 ++
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 
> > > > +++++++++++++++++++++-
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  2 +-
> > > >   3 files changed, 35 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > index b5ff76aae7e0..8e96b9a14452 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > @@ -1296,6 +1296,8 @@ bool amdgpu_device_has_job_running(struct 
> > > > amdgpu_device *adev);
> > > >   bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
> > > >   int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> > > >                     struct amdgpu_job* job);
> > > > +int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev,
> > > > +                  struct amdgpu_job *job);
> > > >   void amdgpu_device_pci_config_reset(struct amdgpu_device *adev);
> > > >   int amdgpu_device_pci_reset(struct amdgpu_device *adev);
> > > >   bool amdgpu_device_need_post(struct amdgpu_device *adev);
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > index b595e6d699b5..55cd67b9ede2 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > @@ -4979,7 +4979,7 @@ static void amdgpu_device_recheck_guilty_jobs(
> > > >    * Returns 0 for success or an error on failure.
> > > >    */
> > > >   -int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> > > > +int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev,
> > > >                     struct amdgpu_job *job)
> > > >   {
> > > >       struct list_head device_list, *device_list_handle = NULL;
> > > > @@ -5236,6 +5236,37 @@ int amdgpu_device_gpu_recover(struct 
> > > > amdgpu_device *adev,
> > > >       return r;
> > > >   }
> > > >   +struct recover_work_struct {
> > >
> > > Please add an amdgpu_ prefix to the name.
> > >
> > > > +    struct work_struct base;
> > > > +    struct amdgpu_device *adev;
> > > > +    struct amdgpu_job *job;
> > > > +    int ret;
> > > > +};
> > > > +
> > > > +static void amdgpu_device_queue_gpu_recover_work(struct 
> > > > work_struct *work)
> > > > +{
> > > > +    struct recover_work_struct *recover_work = container_of(work, 
> > > > struct recover_work_struct, base);
> > > > +
> > > > +    recover_work->ret = 
> > > > amdgpu_device_gpu_recover_imp(recover_work->adev, recover_work->job);
> > > > +}
> > > > +/*
> > > > + * Serialize gpu recover into reset domain single threaded wq
> > > > + */
> > > > +int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> > > > +                    struct amdgpu_job *job)
> > > > +{
> > > > +    struct recover_work_struct work = {.adev = adev, .job = job};
> > > > +
> > > > +    INIT_WORK(&work.base, amdgpu_device_queue_gpu_recover_work);
> > > > +
> > > > +    if (!queue_work(adev->reset_domain.wq, &work.base))
> > > > +        return -EAGAIN;
> > > > +
> > > > +    flush_work(&work.base);
> > > > +
> > > > +    return work.ret;
> > > > +}
> > >
> > > Maybe that should be part of the scheduler code? Not really sure, 
> > > just an idea.
> >
> >
> > Seems to me that since the reset domain is almost always above a 
> > single scheduler granularity then it wouldn't feet very well there.
>
> Yeah, but what if we introduce an drm_sched_recover_queue and 
> drm_sched_recover_work object?
>
> It's probably ok to go forward with that for now, but this handling 
> makes quite some sense to have independent of which driver is using 
> it. So as soon as we see a second similar implementation we should 
> move it into common code.
>
> Regards,
> Christian.


Agree, the only point i would stress is that we need an entity separate 
from from drm_gpu_scheduler, something like
drm_gpu_reset_domain which  should point to or be pointed by a set of 
schedulers that should go through
reset together.

Andrey


> > Andrey
> >
> >
> > > Apart from that looks good to me,
> > > Christian.
> > >
> > > > +
> > > >   /**
> > > >    * amdgpu_device_get_pcie_info - fence pcie info about the PCIE slot
> > > >    *
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > index bfc47bea23db..38c9fd7b7ad4 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > @@ -63,7 +63,7 @@ static enum drm_gpu_sched_stat 
> > > > amdgpu_job_timedout(struct drm_sched_job *s_job)
> > > >             ti.process_name, ti.tgid, ti.task_name, ti.pid);
> > > >         if (amdgpu_device_should_recover_gpu(ring->adev)) {
> > > > -        amdgpu_device_gpu_recover(ring->adev, job);
> > > > +        amdgpu_device_gpu_recover_imp(ring->adev, job);
> > > >       } else {
> > > >           drm_sched_suspend_timeout(&ring->sched);
> > > >           if (amdgpu_sriov_vf(adev))