Re: [PATCH v3 4/5] drm/amdgpu: switch GPU workload profile

["Sharma, Shashank" <shashank.sharma@amd.com>]

On 9/27/2022 2:20 PM, Lazar, Lijo wrote:
> 
> 
> On 9/27/2022 5:17 PM, Sharma, Shashank wrote:
>>
>>
>> On 9/27/2022 12:03 PM, Lazar, Lijo wrote:
>>>
>>>
>>> On 9/27/2022 3:10 AM, Shashank Sharma wrote:
>>>> This patch and switches the GPU workload based profile based
>>>> on the workload hint information saved in the workload context.
>>>> The workload profile is reset to NONE when the job is done.
>>>>
>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c           |  2 ++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c |  4 ----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c          | 15 +++++++++++++++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.h          |  3 +++
>>>>   4 files changed, 20 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index b7bae833c804..de906a42144f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -237,6 +237,8 @@ static int amdgpu_cs_parser_init(struct 
>>>> amdgpu_cs_parser *p, union drm_amdgpu_cs
>>>>           goto free_all_kdata;
>>>>       }
>>>> +    p->job->workload_mode = p->ctx->workload_mode;
>>>> +
>>>>       if (p->uf_entry.tv.bo)
>>>>           p->job->uf_addr = uf_offset;
>>>>       kvfree(chunk_array);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> index a11cf29bc388..625114804121 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> @@ -55,15 +55,11 @@ int amdgpu_set_workload_profile(struct 
>>>> amdgpu_device *adev,
>>>>       mutex_lock(&adev->pm.smu_workload_lock);
>>>> -    if (adev->pm.workload_mode == hint)
>>>> -        goto unlock;
>>>> -
>>>
>>> What is the expectation when a GFX job + VCN job together (or in 
>>> general two jobs running in separate schedulers) and each prefers a 
>>> different workload type? FW will switch as requested.
>>
>> Well, I guess the last switched mode will take over. Do note that like 
>> most of the PM features, the real benefit of power profiles can be 
>> seen with consistant and similar workloads running for some time (Like 
>> gaming, video playback etc).
>>
> 
> Yes, so the extra protection layer wrapping around this is really not 
> helping (user doesn't know if the job is really run in the requested 
> mode). I would suggest to avoid that and document the usage of this API 
> as exclusive mode usage for some profiling use cases.
> 

As I mentioned in the other comment, this extra protection is not for 
not allowing it to change the mode, but from preventing PM reset from 
job_cleanup thread, while another work is in progress.

- Shashank

> Thanks,
> Lijo
> 
>> - Shashank
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>>       ret = amdgpu_dpm_switch_power_profile(adev, profile, 1);
>>>>       if (!ret)
>>>>           adev->pm.workload_mode = hint;
>>>>       atomic_inc(&adev->pm.workload_switch_ref);
>>>> -unlock:
>>>>       mutex_unlock(&adev->pm.smu_workload_lock);
>>>>       return ret;
>>>>   }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> index c2fd6f3076a6..9300e86ee7c5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> @@ -30,6 +30,7 @@
>>>>   #include "amdgpu.h"
>>>>   #include "amdgpu_trace.h"
>>>>   #include "amdgpu_reset.h"
>>>> +#include "amdgpu_ctx_workload.h"
>>>>   static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
>>>> drm_sched_job *s_job)
>>>>   {
>>>> @@ -144,6 +145,14 @@ void amdgpu_job_free_resources(struct 
>>>> amdgpu_job *job)
>>>>   static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>>>>   {
>>>>       struct amdgpu_job *job = to_amdgpu_job(s_job);
>>>> +    struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
>>>> +
>>>> +    if (job->workload_mode != AMDGPU_CTX_WORKLOAD_HINT_NONE) {
>>>> +        if (amdgpu_clear_workload_profile(ring->adev, 
>>>> job->workload_mode))
>>>> +            DRM_WARN("Failed to come out of workload profile %s\n",
>>>> +                amdgpu_workload_profile_name(job->workload_mode));
>>>> +        job->workload_mode = AMDGPU_CTX_WORKLOAD_HINT_NONE;
>>>> +    }
>>>>       drm_sched_job_cleanup(s_job);
>>>> @@ -256,6 +265,12 @@ static struct dma_fence *amdgpu_job_run(struct 
>>>> drm_sched_job *sched_job)
>>>>               DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>       }
>>>> +    if (job->workload_mode != AMDGPU_CTX_WORKLOAD_HINT_NONE) {
>>>> +        if (amdgpu_set_workload_profile(ring->adev, 
>>>> job->workload_mode))
>>>> +            DRM_WARN("Failed to set workload profile to %s\n",
>>>> +                  amdgpu_workload_profile_name(job->workload_mode));
>>>> +    }
>>>> +
>>>>       job->job_run_counter++;
>>>>       amdgpu_job_free_resources(job);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>>> index babc0af751c2..573e8692c814 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>>> @@ -68,6 +68,9 @@ struct amdgpu_job {
>>>>       /* job_run_counter >= 1 means a resubmit job */
>>>>       uint32_t        job_run_counter;
>>>> +    /* workload mode hint for pm */
>>>> +    uint32_t        workload_mode;
>>>> +
>>>>       uint32_t        num_ibs;
>>>>       struct amdgpu_ib    ibs[];
>>>>   };
>>>>