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

["Lazar, Lijo" <lijo.lazar@amd.com>]

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.

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[];
>>>   };
>>>