From: "Sharma, Shashank" <shashank.sharma@amd.com>
To: "Lazar, Lijo" <lijo.lazar@amd.com>, amd-gfx@lists.freedesktop.org
Cc: alexander.deucher@amd.com, amaranath.somalapuram@amd.com,
christian.koenig@amd.com
Subject: Re: [PATCH v3 2/5] drm/amdgpu: add new functions to set GPU power profile
Date: Tue, 27 Sep 2022 14:23:41 +0200 [thread overview]
Message-ID: <6a90712b-d6bb-d1b8-e1b9-8e714be85094@amd.com> (raw)
In-Reply-To: <f8d1a1dc-056c-bfe0-2f61-cc9c0702e37b@amd.com>
On 9/27/2022 2:10 PM, Lazar, Lijo wrote:
>
>
> On 9/27/2022 5:11 PM, Sharma, Shashank wrote:
>>
>>
>> On 9/27/2022 11:58 AM, Lazar, Lijo wrote:
>>>
>>>
>>> On 9/27/2022 3:10 AM, Shashank Sharma wrote:
>>>> 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 it's all about pm subsystem variable accesses, this API should
>>> rather be inside amd/pm subsystem. No need to expose the variable
>>> outside pm subsytem. Also currently all amdgpu_dpm* calls are
>>> protected under one mutex. Then this extra lock won't be needed.
>>>
>>
>> This is tricky, this is not all about PM subsystem. Note that the job
>> management and scheduling is handled into amdgpu_ctx, so the workload
>> hint is set in context_management API. The API is consumed when the
>> job is actually run from amdgpu_run() layer. So its a joint interface
>> between context and PM.
>>
>
> If you take out amdgpu_workload_to_power_profile() line, everything else
> looks to touch only pm variables/functions.
That's not a line, that function converts a AMGPU_CTX hint to PPM
profile. And going by that logic, this whole code was kept in the
amdgpu_ctx.c file as well, coz this code is consuming the PM API. So to
avoid these conflicts and having a new file is a better idea.
You could still keep a
> wrapper though. Also dpm_* functions are protected, so the extra mutex
> can be avoided as well.
>
The lock also protects pm.workload_mode writes.
>>>> +
>>>> + if (adev->pm.workload_mode == hint)
>>>> + goto unlock;
>>>> +
>>>> + ret = amdgpu_dpm_switch_power_profile(adev, profile, 1);
>>>> + if (!ret)
>>>> + adev->pm.workload_mode = hint;
>>>> + atomic_inc(&adev->pm.workload_switch_ref);
>>>
>>> Why is this reference kept? The swtiching happens inside a lock and
>>> there is already a check not to switch if the hint matches with
>>> current workload.
>>>
>>
>> This reference is kept so that we would not reset the PM mode to
>> DEFAULT when some other context has switched the PP mode. If you see
>> the 4th patch, the PM mode will be changed when the job in that
>> context is run, and a pm_reset function will be scheduled when the job
>> is done. But in between if another job from another context has
>> changed the PM mode, the refrence count will prevent us from resetting
>> the PM mode.
>>
>
> This helps only if multiple jobs request the same mode. If they request
> different modes, then this is not helping much.
No that's certainly not the case. It's a counter, whose aim is to allow
a PP reset only when the counter is 0. Do note that the reset() happens
only in the job_free_cb(), which gets schedule later. If this counter is
not zero, which means another work has changed the profile in between,
and we should not reset it.
>
> It could be useful to profile some apps assuming it has exclusive access.
>
> However, in general, the API is not reliable from a user point as the
> mode requested can be overridden by some other job. Then a better thing
> to do is to document that and avoid the extra stuff around it.
>
As I mentioned before, like any PM feature, the benefits can be seen
only while running consistant workloads for long time. I an still add a
doc note in the UAPI page.
- Shashank
> Thanks,
> Lijo
>
>> - Shashank
>>
>>> Thanks,
>>> Lijo
>>>
>>>> +
>>>> +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;
>>>> +}
>>>> 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;
>>>>
next prev parent reply other threads:[~2022-09-27 12:23 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-26 21:40 [PATCH v3 0/5] GPU workload hints for better performance Shashank Sharma
2022-09-26 21:40 ` [PATCH v3 1/5] drm/amdgpu: add UAPI for workload hints to ctx ioctl Shashank Sharma
2022-09-27 6:07 ` Christian König
2022-09-27 14:28 ` Felix Kuehling
2023-03-21 3:05 ` Marek Olšák
2023-03-21 13:00 ` Sharma, Shashank
2023-03-21 13:54 ` Christian König
2023-03-22 14:05 ` Marek Olšák
2023-03-22 14:08 ` Christian König
2023-03-22 14:24 ` Marek Olšák
2023-03-22 14:29 ` Christian König
2023-03-22 14:36 ` Marek Olšák
2023-03-22 14:52 ` Alex Deucher
2023-03-22 15:11 ` Marek Olšák
2023-03-22 14:38 ` Sharma, Shashank
2022-09-26 21:40 ` [PATCH v3 2/5] drm/amdgpu: add new functions to set GPU power profile Shashank Sharma
2022-09-27 2:14 ` Quan, Evan
2022-09-27 7:29 ` Sharma, Shashank
2022-09-27 9:29 ` Quan, Evan
2022-09-27 10:00 ` Sharma, Shashank
2022-09-27 6:08 ` Christian König
2022-09-27 9:58 ` Lazar, Lijo
2022-09-27 11:41 ` Sharma, Shashank
2022-09-27 12:10 ` Lazar, Lijo
2022-09-27 12:23 ` Sharma, Shashank [this message]
2022-09-27 12:39 ` Lazar, Lijo
2022-09-27 12:53 ` Sharma, Shashank
2022-09-27 13:29 ` Lazar, Lijo
2022-09-27 13:47 ` Sharma, Shashank
2022-09-27 14:00 ` Lazar, Lijo
2022-09-27 14:20 ` Sharma, Shashank
2022-09-27 14:34 ` Lazar, Lijo
2022-09-27 14:50 ` Sharma, Shashank
2022-09-27 15:20 ` Felix Kuehling
2022-09-26 21:40 ` [PATCH v3 3/5] drm/amdgpu: set GPU workload via ctx IOCTL Shashank Sharma
2022-09-27 6:09 ` Christian König
2022-09-26 21:40 ` [PATCH v3 4/5] drm/amdgpu: switch GPU workload profile Shashank Sharma
2022-09-27 6:11 ` Christian König
2022-09-27 10:03 ` Lazar, Lijo
2022-09-27 11:47 ` Sharma, Shashank
2022-09-27 12:20 ` Lazar, Lijo
2022-09-27 12:25 ` Sharma, Shashank
2022-09-27 16:33 ` Michel Dänzer
2022-09-27 17:06 ` Sharma, Shashank
2022-09-27 17:29 ` Michel Dänzer
2022-09-26 21:40 ` [PATCH v3 5/5] drm/amdgpu: switch workload context to/from compute Shashank Sharma
2022-09-27 6:12 ` Christian König
2022-09-27 14:48 ` Felix Kuehling
2022-09-27 14:58 ` Sharma, Shashank
2022-09-27 15:23 ` Felix Kuehling
2022-09-27 15:38 ` Sharma, Shashank
2022-09-27 20:40 ` Alex Deucher
2022-09-28 7:05 ` Lazar, Lijo
2022-09-28 8:56 ` Sharma, Shashank
2022-09-28 9:00 ` Sharma, Shashank
2022-09-28 21:51 ` Alex Deucher
2022-09-29 8:48 ` Sharma, Shashank
2022-09-29 11:10 ` Lazar, Lijo
2022-09-29 13:20 ` Sharma, Shashank
2022-09-29 13:37 ` Lazar, Lijo
2022-09-29 14:00 ` Sharma, Shashank
2022-09-29 14:14 ` Lazar, Lijo
2022-09-29 14:40 ` Sharma, Shashank
2022-09-29 18:32 ` Alex Deucher
2022-09-30 5:08 ` Lazar, Lijo
2022-09-30 8:37 ` Sharma, Shashank
2022-09-30 9:13 ` Lazar, Lijo
2022-09-30 9:22 ` Sharma, Shashank
2022-09-30 9:54 ` Lazar, Lijo
2022-09-30 10:09 ` Sharma, Shashank
2022-09-29 18:07 ` Felix Kuehling
2022-09-30 4:46 ` Lazar, Lijo
2022-09-27 16:24 ` [PATCH v3 0/5] GPU workload hints for better performance Michel Dänzer
2022-09-27 16:59 ` Sharma, Shashank
2022-09-27 17:13 ` Michel Dänzer
2022-09-27 17:25 ` Sharma, Shashank
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6a90712b-d6bb-d1b8-e1b9-8e714be85094@amd.com \
--to=shashank.sharma@amd.com \
--cc=alexander.deucher@amd.com \
--cc=amaranath.somalapuram@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=lijo.lazar@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).