Re: [PATCH v3 2/5] drm/amdgpu: add new functions to set GPU power profile

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

On 9/27/2022 11:29 AM, Quan, Evan wrote:
> [AMD Official Use Only - General]
> 
> 
> 
>> -----Original Message-----
>> From: Sharma, Shashank <Shashank.Sharma@amd.com>
>> Sent: Tuesday, September 27, 2022 3:30 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Somalapuram,
>> Amaranath <Amaranath.Somalapuram@amd.com>; Koenig, Christian
>> <Christian.Koenig@amd.com>
>> Subject: Re: [PATCH v3 2/5] drm/amdgpu: add new functions to set GPU
>> power profile
>>
>> Hello Evan,
>>
>> On 9/27/2022 4:14 AM, Quan, Evan wrote:
>>> [AMD Official Use Only - General]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Shashank Sharma
>>>> Sent: Tuesday, September 27, 2022 5:40 AM
>>>> To: amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Somalapuram,
>>>> Amaranath <Amaranath.Somalapuram@amd.com>; Koenig, Christian
>>>> <Christian.Koenig@amd.com>; Sharma, Shashank
>>>> <Shashank.Sharma@amd.com>
>>>> Subject: [PATCH v3 2/5] drm/amdgpu: add new functions to set GPU
>>>> power profile
>>>>
>>>> This patch adds new functions which will allow a user to change the
>>>> GPU power profile based a GPU workload hint flag.
>>>>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/Makefile           |  2 +-
>>>>    .../gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c  | 97
>>>> +++++++++++++++++++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  1 +
>>>>    .../gpu/drm/amd/include/amdgpu_ctx_workload.h | 54 +++++++++++
>>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h       |  5 +
>>>>    5 files changed, 158 insertions(+), 1 deletion(-)
>>>>    create mode 100644
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>>    create mode 100644
>>>> drivers/gpu/drm/amd/include/amdgpu_ctx_workload.h
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> index 5a283d12f8e1..34679c657ecc 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> @@ -50,7 +50,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>>>>    	atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>>    	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>>>    	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o
>>>> amdgpu_pll.o \
>>>> -	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>> +	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o
>>>> amdgpu_ctx_workload.o amdgpu_sync.o \
>>>>    	amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o
>>>> amdgpu_virt.o \
>>>>    	amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>>>    	amdgpu_debugfs.o amdgpu_ids.o amdgpu_gmc.o \ diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> new file mode 100644
>>>> index 000000000000..a11cf29bc388
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx_workload.c
>>>> @@ -0,0 +1,97 @@
>>>> +/*
>>>> + * Copyright 2022 Advanced Micro Devices, Inc.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person
>>>> +obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without
>>>> + limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>> + sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom
>>>> + the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be
>>>> + included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>> KIND,
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
>> NO
>>>> EVENT SHALL
>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>>>> DAMAGES OR
>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>> OTHERWISE,
>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
>>>> THE USE OR
>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>> + *
>>>> + */
>>>> +#include <drm/drm.h>
>>>> +#include "kgd_pp_interface.h"
>>>> +#include "amdgpu_ctx_workload.h"
>>>> +
>>>> +static enum PP_SMC_POWER_PROFILE
>>>> +amdgpu_workload_to_power_profile(uint32_t hint) {
>>>> +	switch (hint) {
>>>> +	case AMDGPU_CTX_WORKLOAD_HINT_NONE:
>>>> +	default:
>>>> +		return PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
>>>> +
>>>> +	case AMDGPU_CTX_WORKLOAD_HINT_3D:
>>>> +		return PP_SMC_POWER_PROFILE_FULLSCREEN3D;
>>>> +	case AMDGPU_CTX_WORKLOAD_HINT_VIDEO:
>>>> +		return PP_SMC_POWER_PROFILE_VIDEO;
>>>> +	case AMDGPU_CTX_WORKLOAD_HINT_VR:
>>>> +		return PP_SMC_POWER_PROFILE_VR;
>>>> +	case AMDGPU_CTX_WORKLOAD_HINT_COMPUTE:
>>>> +		return PP_SMC_POWER_PROFILE_COMPUTE;
>>>> +	}
>>>> +}
>>>> +
>>>> +int amdgpu_set_workload_profile(struct amdgpu_device *adev,
>>>> +				uint32_t hint)
>>>> +{
>>>> +	int ret = 0;
>>>> +	enum PP_SMC_POWER_PROFILE profile =
>>>> +			amdgpu_workload_to_power_profile(hint);
>>>> +
>>>> +	if (adev->pm.workload_mode == hint)
>>>> +		return 0;
>>>> +
>>>> +	mutex_lock(&adev->pm.smu_workload_lock);
>>>> +
>>>> +	if (adev->pm.workload_mode == hint)
>>>> +		goto unlock;
>>> [Quan, Evan] This seems redundant with code above. I saw you dropped
>> this in Patch4.
>>> But I kind of feel this should be the one which needs to be kept.
>>
>> Yes, this shuffle happened during the rebase-testing of V3, will update this.
>>
>>>> +
>>>> +	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;
>>>> +}
>>>> +
>>>> +int amdgpu_clear_workload_profile(struct amdgpu_device *adev,
>>>> +				  uint32_t hint)
>>>> +{
>>>> +	int ret = 0;
>>>> +	enum PP_SMC_POWER_PROFILE profile =
>>>> +			amdgpu_workload_to_power_profile(hint);
>>>> +
>>>> +	if (hint == AMDGPU_CTX_WORKLOAD_HINT_NONE)
>>>> +		return 0;
>>>> +
>>>> +	/* Do not reset GPU power profile if another reset is coming */
>>>> +	if (atomic_dec_return(&adev->pm.workload_switch_ref) > 0)
>>>> +		return 0;
>>>> +
>>>> +	mutex_lock(&adev->pm.smu_workload_lock);
>>>> +
>>>> +	if (adev->pm.workload_mode != hint)
>>>> +		goto unlock;
>>>> +
>>>> +	ret = amdgpu_dpm_switch_power_profile(adev, profile, 0);
>>>> +	if (!ret)
>>>> +		adev->pm.workload_mode =
>>>> AMDGPU_CTX_WORKLOAD_HINT_NONE;
>>>> +
>>>> +unlock:
>>>> +	mutex_unlock(&adev->pm.smu_workload_lock);
>>>> +	return ret;
>>>> +}
>>> [Quan, Evan] Instead of setting to
>> AMDGPU_CTX_WORKLOAD_HINT_NONE, better to reset it back to original
>> workload profile mode.
>>> That can make it compatible with existing sysfs interface which has similar
>> functionality for setting workload profile mode.
>>
>> This API is specifically written to remove any workload profile applied, hense
>> named as "clear_workload_profile" and the intention is reset. As you can see
>> in the next patch, the work profile is being set from the job_run and reset
>> again once the job execution is done.
>>
>> If there is another set() in progress, the reference counter takes care of that.
>> So I would like to keep it this way.
> [Quan, Evan] What I meant is some case like below:
> 1. User sets a workload profile mode via sysfs interface (e.g. setting compute mode via "echo 5 > /sys/class/drm/card0/device/pp_power_profile_mode")
> 2. Then a job was launched with a different workload profile mode requested(e.g. 3D_FULL_SCREEN mode).
> 3. Finally on the job ended, better to switch back to original compute mode, not just reset it back to NONE. Does that make sense?
> 
> BR
> Evan

To be honest, once we have a proper UAPI to set the power profile, we 
should not use a sysfs interface at all (or use it mostly for debug 
purposes). Also I am not sure if you can read back the current power 
profile from FW/HW, can you ?

- Shashank


>>
>> - Shashank
>>
>>> /**
>>>    * DOC: pp_power_profile_mode
>>>    *
>>>    * The amdgpu driver provides a sysfs API for adjusting the heuristics
>>>    * related to switching between power levels in a power state.  The file
>>>    * pp_power_profile_mode is used for this.
>>>    *
>>>    * Reading this file outputs a list of all of the predefined power profiles
>>>    * and the relevant heuristics settings for that profile.
>>>    *
>>>    * To select a profile or create a custom profile, first select manual using
>>>    * power_dpm_force_performance_level.  Writing the number of a
>> predefined
>>>    * profile to pp_power_profile_mode will enable those heuristics.  To
>>>    * create a custom set of heuristics, write a string of numbers to the file
>>>    * starting with the number of the custom profile along with a setting
>>>    * for each heuristic parameter.  Due to differences across asic families
>>>    * the heuristic parameters vary from family to family.
>>>    *
>>>    */
>>>
>>> BR
>>> Evan
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index be7aff2d4a57..1f0f64662c04 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -3554,6 +3554,7 @@ int amdgpu_device_init(struct amdgpu_device
>>>> *adev,
>>>>    	mutex_init(&adev->psp.mutex);
>>>>    	mutex_init(&adev->notifier_lock);
>>>>    	mutex_init(&adev->pm.stable_pstate_ctx_lock);
>>>> +	mutex_init(&adev->pm.smu_workload_lock);
>>>>    	mutex_init(&adev->benchmark_mutex);
>>>>
>>>>    	amdgpu_device_init_apu_flags(adev);
>>>> diff --git a/drivers/gpu/drm/amd/include/amdgpu_ctx_workload.h
>>>> b/drivers/gpu/drm/amd/include/amdgpu_ctx_workload.h
>>>> new file mode 100644
>>>> index 000000000000..6060fc53c3b0
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/amd/include/amdgpu_ctx_workload.h
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * Copyright 2022 Advanced Micro Devices, Inc.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person
>>>> +obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without
>>>> + limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>> + sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom
>>>> + the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be
>>>> + included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>> KIND,
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
>> NO
>>>> EVENT SHALL
>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>>>> DAMAGES OR
>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>> OTHERWISE,
>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
>>>> THE USE OR
>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>> + *
>>>> + */
>>>> +#ifndef _AMDGPU_CTX_WL_H_
>>>> +#define _AMDGPU_CTX_WL_H_
>>>> +#include <drm/amdgpu_drm.h>
>>>> +#include "amdgpu.h"
>>>> +
>>>> +/* Workload mode names */
>>>> +static const char * const amdgpu_workload_mode_name[] = {
>>>> +	"None",
>>>> +	"3D",
>>>> +	"Video",
>>>> +	"VR",
>>>> +	"Compute",
>>>> +	"Unknown",
>>>> +};
>>>> +
>>>> +static inline const
>>>> +char *amdgpu_workload_profile_name(uint32_t profile) {
>>>> +	if (profile >= AMDGPU_CTX_WORKLOAD_HINT_NONE &&
>>>> +		profile < AMDGPU_CTX_WORKLOAD_HINT_MAX)
>>>> +		return
>>>>
>> amdgpu_workload_mode_name[AMDGPU_CTX_WORKLOAD_INDEX(profile
>>>> )];
>>>> +
>>>> +	return
>>>>
>> amdgpu_workload_mode_name[AMDGPU_CTX_WORKLOAD_HINT_MAX];
>>>> +}
>>>> +
>>>> +int amdgpu_clear_workload_profile(struct amdgpu_device *adev,
>>>> +				uint32_t hint);
>>>> +
>>>> +int amdgpu_set_workload_profile(struct amdgpu_device *adev,
>>>> +				uint32_t hint);
>>>> +
>>>> +#endif
>>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>>> index 65624d091ed2..565131f789d0 100644
>>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>>> @@ -361,6 +361,11 @@ struct amdgpu_pm {
>>>>    	struct mutex            stable_pstate_ctx_lock;
>>>>    	struct amdgpu_ctx       *stable_pstate_ctx;
>>>>
>>>> +	/* SMU workload mode */
>>>> +	struct mutex smu_workload_lock;
>>>> +	uint32_t workload_mode;
>>>> +	atomic_t workload_switch_ref;
>>>> +
>>>>    	struct config_table_setting config_table;
>>>>    	/* runtime mode */
>>>>    	enum amdgpu_runpm_mode rpm_mode;
>>>> --
>>>> 2.34.1