Re: [PATCH v2 4/7] drm/amdgpu: Add suspend function to clear the GPU power profile.

[Yadav, Arvind]

On 8/22/2023 12:01 PM, Lazar, Lijo wrote:
> On 8/21/2023 12:17 PM, Arvind Yadav wrote:
> > This patch adds a suspend function that will clear the GPU
> > power profile before going into suspend state.
> >
> > v2:
> > - Add the new suspend function based on review comment.
> >
> > Cc: Shashank Sharma <shashank.sharma@xxxxxxx>
> > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Signed-off-by: Arvind Yadav <Arvind.Yadav@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  2 ++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c  | 23 +++++++++++++++++++
> >   drivers/gpu/drm/amd/include/amdgpu_workload.h |  2 ++
> >   3 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index cd3bf641b630..3b70e657b439 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4212,6 +4212,8 @@ int amdgpu_device_suspend(struct drm_device 
> > *dev, bool fbcon)
> >         amdgpu_ras_suspend(adev);
> >   +    amdgpu_workload_profile_suspend(adev);
> > +
> >       amdgpu_device_ip_suspend_phase1(adev);
> >         if (!adev->in_s0ix)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
> > index 6367eb88a44d..44ca8e986984 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
> > @@ -174,6 +174,29 @@ void amdgpu_workload_profile_set(struct 
> > amdgpu_device *adev,
> >       mutex_unlock(&workload->workload_lock);
> >   }
> >   +void amdgpu_workload_profile_suspend(struct amdgpu_device *adev)
> > +{
> > +    struct amdgpu_smu_workload *workload = &adev->smu_workload;
> > +    int ret;
> > +
> > +    mutex_lock(&workload->workload_lock);
> > + cancel_delayed_work_sync(&workload->smu_delayed_work);
>
> Another deadlock candidate. Between fini() and suspend(), the only 
> difference probably could be initialization status. If so, just use a 
> helper that is used during fini() and suspend().
Before going to suspend(), we need to cancel the work and clear all the 
profiles but in fini() we are destroying the mutex. also it will be 
called when we are unloading everything.

~Arvind

> Thanks,
> Lijo
>
> > +
> > +    /* Clear all the set GPU power profile*/
> > +    for (int index = fls(workload->submit_workload_status);
> > +         index > 0; index--) {
> > +        if (workload->submit_workload_status & (1 << index)) {
> > + atomic_set(&workload->power_profile_ref[index], 0);
> > +            ret = amdgpu_power_profile_clear(adev, index);
> > +            if (ret)
> > +                DRM_WARN("Failed to clear power profile %s, err = 
> > %d\n",
> > +                     amdgpu_workload_mode_name[index], ret);
> > +        }
> > +    }
> > +    workload->submit_workload_status = 0;
> > +    mutex_unlock(&workload->workload_lock);
> > +}
> > +
> >   void amdgpu_workload_profile_init(struct amdgpu_device *adev)
> >   {
> >       adev->smu_workload.adev = adev;
> > diff --git a/drivers/gpu/drm/amd/include/amdgpu_workload.h 
> > b/drivers/gpu/drm/amd/include/amdgpu_workload.h
> > index ee1f87257f2d..0acd8769ec52 100644
> > --- a/drivers/gpu/drm/amd/include/amdgpu_workload.h
> > +++ b/drivers/gpu/drm/amd/include/amdgpu_workload.h
> > @@ -52,6 +52,8 @@ void amdgpu_workload_profile_put(struct 
> > amdgpu_device *adev,
> >   void amdgpu_workload_profile_set(struct amdgpu_device *adev,
> >                    uint32_t ring_type);
> >   +void amdgpu_workload_profile_suspend(struct amdgpu_device *adev);
> > +
> >   void amdgpu_workload_profile_init(struct amdgpu_device *adev);
> >     void amdgpu_workload_profile_fini(struct amdgpu_device *adev);