[PATCH v1 0/3] send message to pmfw when SMT changes

[PATCH v1 0/3] send message to pmfw when SMT changes

[Wenyou Yang]

When the CPU SMT changes on the fly, send the message to pmfw
to notify the SMT status changed.

Wenyou Yang (3):
  cpu/smt: add a notifier to notify the SMT changes
  drm/amd/pm: send the SMT-enable message to pmfw
  drm/amd/pm: vangogh: support to send SMT enable message

 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41 +++++++++++++++++++
 drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
 .../pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h    |  3 +-
 drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h  |  3 +-
 .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  | 19 +++++++++
 include/linux/cpu.h                           |  5 +++
 kernel/cpu.c                                  | 11 ++++-
 7 files changed, 84 insertions(+), 3 deletions(-)

-- 
2.39.2

[PATCH v1 1/3] cpu/smt: add a notifier to notify the SMT changes

[Wenyou Yang]

Add the notifier chain to notify the cpu SMT status changes

Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
---
 include/linux/cpu.h |  5 +++++
 kernel/cpu.c        | 11 ++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 314802f98b9d..9a842317fe2d 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -213,6 +213,11 @@ enum cpuhp_smt_control {
 	CPU_SMT_NOT_IMPLEMENTED,
 };
 
+enum cpuhp_smt_status {
+	SMT_ENABLED,
+	SMT_DISABLED,
+};
+
 #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
 extern enum cpuhp_smt_control cpu_smt_control;
 extern void cpu_smt_disable(bool force);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6c0a92ca6bb5..accae0fa9868 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -89,6 +89,9 @@ static DEFINE_PER_CPU(struct cpuhp_cpu_state, cpuhp_state) = {
 cpumask_t cpus_booted_once_mask;
 #endif
 
+RAW_NOTIFIER_HEAD(smt_notifier_head);
+EXPORT_SYMBOL(smt_notifier_head);
+
 #if defined(CONFIG_LOCKDEP) && defined(CONFIG_SMP)
 static struct lockdep_map cpuhp_state_up_map =
 	STATIC_LOCKDEP_MAP_INIT("cpuhp_state-up", &cpuhp_state_up_map);
@@ -2281,8 +2284,10 @@ int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
 		 */
 		cpuhp_offline_cpu_device(cpu);
 	}
-	if (!ret)
+	if (!ret) {
 		cpu_smt_control = ctrlval;
+		raw_notifier_call_chain(&smt_notifier_head, SMT_DISABLED, NULL);
+	}
 	cpu_maps_update_done();
 	return ret;
 }
@@ -2303,7 +2308,11 @@ int cpuhp_smt_enable(void)
 		/* See comment in cpuhp_smt_disable() */
 		cpuhp_online_cpu_device(cpu);
 	}
+	if (!ret)
+		raw_notifier_call_chain(&smt_notifier_head, SMT_ENABLED, NULL);
+
 	cpu_maps_update_done();
+
 	return ret;
 }
 #endif
-- 
2.39.2

[PATCH v1 2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Wenyou Yang]

When the CPU SMT status change in the fly, sent the SMT-enable
message to pmfw to notify it that the SMT status changed.

Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41 +++++++++++++++++++
 drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index b5d64749990e..5cd85a9d149d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -22,6 +22,7 @@
 
 #define SWSMU_CODE_LAYER_L1
 
+#include <linux/cpu.h>
 #include <linux/firmware.h>
 #include <linux/pci.h>
 
@@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle, uint32_t speed);
 static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
 static int smu_set_mp1_state(void *handle, enum pp_mp1_state mp1_state);
 
+static int smt_notifier_callback(struct notifier_block *nb, unsigned long action, void *data);
+
+extern struct raw_notifier_head smt_notifier_head;
+
+static struct notifier_block smt_notifier = {
+	.notifier_call = smt_notifier_callback,
+};
+
 static int smu_sys_get_pp_feature_mask(void *handle,
 				       char *buf)
 {
@@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device *adev)
 	return 0;
 }
 
+static struct smu_context *current_smu;
+
 static int smu_early_init(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
@@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
 	mutex_init(&smu->message_lock);
 
 	adev->powerplay.pp_handle = smu;
+	current_smu = smu;
 	adev->powerplay.pp_funcs = &swsmu_pm_funcs;
 
 	r = smu_set_funcs(adev);
@@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
 	if (!smu->ppt_funcs->get_fan_control_mode)
 		smu->adev->pm.no_fan = true;
 
+	raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
+
 	return 0;
 }
 
@@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
 
 	smu_fini_microcode(smu);
 
+	raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
+
 	return 0;
 }
 
@@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct smu_context *smu, uint32_t size)
 
 	return ret;
 }
+
+static int smu_set_cpu_smt_enable(struct smu_context *smu, bool enable)
+{
+	int ret = -EINVAL;
+
+	if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
+		ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
+
+	return ret;
+}
+
+static int smt_notifier_callback(struct notifier_block *nb,
+				 unsigned long action, void *data)
+{
+	struct smu_context *smu = current_smu;
+	int ret = NOTIFY_OK;
+
+	ret = (action == SMT_ENABLED) ?
+				smu_set_cpu_smt_enable(smu, true) :
+				smu_set_cpu_smt_enable(smu, false);
+	if (ret)
+		ret = NOTIFY_BAD;
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
index 09469c750a96..7c6594bba796 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
@@ -1354,6 +1354,11 @@ struct pptable_funcs {
 	 * @init_pptable_microcode: Prepare the pptable microcode to upload via PSP
 	 */
 	int (*init_pptable_microcode)(struct smu_context *smu);
+
+	/**
+	 * @set_cpu_smt_enable: Set the CPU SMT status
+	 */
+	int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
 };
 
 typedef enum {
-- 
2.39.2

Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Limonciello, Mario]

On 3/22/2023 00:48, Wenyou Yang wrote:
> When the CPU SMT status change in the fly, sent the SMT-enable
> message to pmfw to notify it that the SMT status changed.
> 
> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41 +++++++++++++++++++
>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index b5d64749990e..5cd85a9d149d 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -22,6 +22,7 @@
>   
>   #define SWSMU_CODE_LAYER_L1
>   
> +#include <linux/cpu.h>
>   #include <linux/firmware.h>
>   #include <linux/pci.h>
>   
> @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle, uint32_t speed);
>   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
>   static int smu_set_mp1_state(void *handle, enum pp_mp1_state mp1_state);
>   
> +static int smt_notifier_callback(struct notifier_block *nb, unsigned long action, void *data);
> +
> +extern struct raw_notifier_head smt_notifier_head;
> +
> +static struct notifier_block smt_notifier = {
> +	.notifier_call = smt_notifier_callback,
> +};
> +
>   static int smu_sys_get_pp_feature_mask(void *handle,
>   				       char *buf)
>   {
> @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device *adev)
>   	return 0;
>   }
>   
> +static struct smu_context *current_smu;
> +
>   static int smu_early_init(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> @@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
>   	mutex_init(&smu->message_lock);
>   
>   	adev->powerplay.pp_handle = smu;
> +	current_smu = smu;
>   	adev->powerplay.pp_funcs = &swsmu_pm_funcs;
>   
>   	r = smu_set_funcs(adev);
> @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
>   	if (!smu->ppt_funcs->get_fan_control_mode)
>   		smu->adev->pm.no_fan = true;
>   
> +	raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
> +
>   	return 0;
>   }
>   
> @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
>   
>   	smu_fini_microcode(smu);
>   
> +	raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
> +
>   	return 0;
>   }
>   
> @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct smu_context *smu, uint32_t size)
>   
>   	return ret;
>   }
> +
> +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool enable)
> +{
> +	int ret = -EINVAL;
> +
> +	if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
> +		ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
> +
> +	return ret;
> +}
> +
> +static int smt_notifier_callback(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct smu_context *smu = current_smu;
> +	int ret = NOTIFY_OK;

This initialization is pointless, it's clobbered in the next line.

> +
> +	ret = (action == SMT_ENABLED) ?
> +				smu_set_cpu_smt_enable(smu, true) :
> +				smu_set_cpu_smt_enable(smu, false);

How about this instead, it should be more readable:

	ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);

> +	if (ret)
> +		ret = NOTIFY_BAD;
> +
> +	return ret;

How about instead:

	dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action == SMT_ENABLED 
? "en" : "dis", ret);

	return ret ? NOTIFY_BAD : NOTIFY_OK;

> +}
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index 09469c750a96..7c6594bba796 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -1354,6 +1354,11 @@ struct pptable_funcs {
>   	 * @init_pptable_microcode: Prepare the pptable microcode to upload via PSP
>   	 */
>   	int (*init_pptable_microcode)(struct smu_context *smu);
> +
> +	/**
> +	 * @set_cpu_smt_enable: Set the CPU SMT status
> +	 */
> +	int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
>   };
>   
>   typedef enum {

Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Limonciello, Mario]

On 3/23/2023 12:41, Limonciello, Mario wrote:
> On 3/22/2023 00:48, Wenyou Yang wrote:
>> When the CPU SMT status change in the fly, sent the SMT-enable
>> message to pmfw to notify it that the SMT status changed.
>>
>> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41 +++++++++++++++++++
>>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c 
>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> index b5d64749990e..5cd85a9d149d 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> @@ -22,6 +22,7 @@
>>   #define SWSMU_CODE_LAYER_L1
>> +#include <linux/cpu.h>
>>   #include <linux/firmware.h>
>>   #include <linux/pci.h>
>> @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle, 
>> uint32_t speed);
>>   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
>>   static int smu_set_mp1_state(void *handle, enum pp_mp1_state 
>> mp1_state);
>> +static int smt_notifier_callback(struct notifier_block *nb, unsigned 
>> long action, void *data);
>> +
>> +extern struct raw_notifier_head smt_notifier_head;
>> +
>> +static struct notifier_block smt_notifier = {
>> +    .notifier_call = smt_notifier_callback,
>> +};
>> +
>>   static int smu_sys_get_pp_feature_mask(void *handle,
>>                          char *buf)
>>   {
>> @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device *adev)
>>       return 0;
>>   }
>> +static struct smu_context *current_smu;
>> +
>>   static int smu_early_init(void *handle)
>>   {
>>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> @@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
>>       mutex_init(&smu->message_lock);
>>       adev->powerplay.pp_handle = smu;
>> +    current_smu = smu;

Although this series is intended for the Van Gogh case right now, I 
dont't think this would scale well for multiple GPUs in a system.

I think that instead you may want to move the notifier callback to be a 
level "higher" in amdgpu.  Perhaps amdgpu_device.c?  Then when that 
notifier call is received you'll want to walk through the PCI device 
space to find any GPUs that are bound with AMDGPU a series of 
wrappers/calls that end up calling smu_set_cpu_smt_enable with the 
approriate arguments.


>>       adev->powerplay.pp_funcs = &swsmu_pm_funcs;
>>       r = smu_set_funcs(adev);
>> @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
>>       if (!smu->ppt_funcs->get_fan_control_mode)
>>           smu->adev->pm.no_fan = true;
>> +    raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
>> +
>>       return 0;
>>   }
>> @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
>>       smu_fini_microcode(smu);
>> +    raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
>> +
>>       return 0;
>>   }
>> @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct 
>> smu_context *smu, uint32_t size)
>>       return ret;
>>   }
>> +
>> +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool enable)
>> +{
>> +    int ret = -EINVAL;
>> +
>> +    if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
>> +        ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
>> +
>> +    return ret;
>> +}
>> +
>> +static int smt_notifier_callback(struct notifier_block *nb,
>> +                 unsigned long action, void *data)
>> +{
>> +    struct smu_context *smu = current_smu;
>> +    int ret = NOTIFY_OK;
> 
> This initialization is pointless, it's clobbered in the next line.
> 
>> +
>> +    ret = (action == SMT_ENABLED) ?
>> +                smu_set_cpu_smt_enable(smu, true) :
>> +                smu_set_cpu_smt_enable(smu, false);
> 
> How about this instead, it should be more readable:
> 
>      ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);
> 
>> +    if (ret)
>> +        ret = NOTIFY_BAD;
>> +
>> +    return ret;
> 
> How about instead:
> 
>      dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action == 
> SMT_ENABLED ? "en" : "dis", ret);
> 
>      return ret ? NOTIFY_BAD : NOTIFY_OK;
> 
>> +}
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h 
>> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>> index 09469c750a96..7c6594bba796 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>> @@ -1354,6 +1354,11 @@ struct pptable_funcs {
>>        * @init_pptable_microcode: Prepare the pptable microcode to 
>> upload via PSP
>>        */
>>       int (*init_pptable_microcode)(struct smu_context *smu);
>> +
>> +    /**
>> +     * @set_cpu_smt_enable: Set the CPU SMT status
>> +     */
>> +    int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
>>   };
>>   typedef enum {
> 

Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Lazar, Lijo]

On 3/23/2023 11:36 PM, Limonciello, Mario wrote:
> On 3/23/2023 12:41, Limonciello, Mario wrote:
>> On 3/22/2023 00:48, Wenyou Yang wrote:
>>> When the CPU SMT status change in the fly, sent the SMT-enable
>>> message to pmfw to notify it that the SMT status changed.
>>>
>>> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41 +++++++++++++++++++
>>>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
>>>   2 files changed, 46 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c 
>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index b5d64749990e..5cd85a9d149d 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -22,6 +22,7 @@
>>>   #define SWSMU_CODE_LAYER_L1
>>> +#include <linux/cpu.h>
>>>   #include <linux/firmware.h>
>>>   #include <linux/pci.h>
>>> @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle, 
>>> uint32_t speed);
>>>   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
>>>   static int smu_set_mp1_state(void *handle, enum pp_mp1_state 
>>> mp1_state);
>>> +static int smt_notifier_callback(struct notifier_block *nb, unsigned 
>>> long action, void *data);
>>> +
>>> +extern struct raw_notifier_head smt_notifier_head;
>>> +
>>> +static struct notifier_block smt_notifier = {
>>> +    .notifier_call = smt_notifier_callback,
>>> +};
>>> +
>>>   static int smu_sys_get_pp_feature_mask(void *handle,
>>>                          char *buf)
>>>   {
>>> @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device *adev)
>>>       return 0;
>>>   }
>>> +static struct smu_context *current_smu;
>>> +
>>>   static int smu_early_init(void *handle)
>>>   {
>>>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>> @@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
>>>       mutex_init(&smu->message_lock);
>>>       adev->powerplay.pp_handle = smu;
>>> +    current_smu = smu;
> 
> Although this series is intended for the Van Gogh case right now, I 
> dont't think this would scale well for multiple GPUs in a system.
> 
> I think that instead you may want to move the notifier callback to be a 
> level "higher" in amdgpu.  Perhaps amdgpu_device.c?  Then when that 
> notifier call is received you'll want to walk through the PCI device 
> space to find any GPUs that are bound with AMDGPU a series of 
> wrappers/calls that end up calling smu_set_cpu_smt_enable with the 
> approriate arguments.
> 

This is not required when the notifier is registered only within Vangogh 
ppt function. Then follow Evan's suggestion of keeping the notifier 
block inside smu. From the notifier block, it can find the smu block and 
then call cpu_smt_enable/disable. That way notifier callback comes only 
once even with multiple dGPUs + Vangogh and processed for the 
corresponding smu.

This notifier doesn't need to be registered for platforms only with 
dGPUs or APUs which don't need this.

Thanks,
Lijo

> 
>>>       adev->powerplay.pp_funcs = &swsmu_pm_funcs;
>>>       r = smu_set_funcs(adev);
>>> @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
>>>       if (!smu->ppt_funcs->get_fan_control_mode)
>>>           smu->adev->pm.no_fan = true;
>>> +    raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
>>> +
>>>       return 0;
>>>   }
>>> @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
>>>       smu_fini_microcode(smu);
>>> +    raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
>>> +
>>>       return 0;
>>>   }
>>> @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct 
>>> smu_context *smu, uint32_t size)
>>>       return ret;
>>>   }
>>> +
>>> +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool enable)
>>> +{
>>> +    int ret = -EINVAL;
>>> +
>>> +    if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
>>> +        ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int smt_notifier_callback(struct notifier_block *nb,
>>> +                 unsigned long action, void *data)
>>> +{
>>> +    struct smu_context *smu = current_smu;
>>> +    int ret = NOTIFY_OK;
>>
>> This initialization is pointless, it's clobbered in the next line.
>>
>>> +
>>> +    ret = (action == SMT_ENABLED) ?
>>> +                smu_set_cpu_smt_enable(smu, true) :
>>> +                smu_set_cpu_smt_enable(smu, false);
>>
>> How about this instead, it should be more readable:
>>
>>      ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);
>>
>>> +    if (ret)
>>> +        ret = NOTIFY_BAD;
>>> +
>>> +    return ret;
>>
>> How about instead:
>>
>>      dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action == 
>> SMT_ENABLED ? "en" : "dis", ret);
>>
>>      return ret ? NOTIFY_BAD : NOTIFY_OK;
>>
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h 
>>> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>> index 09469c750a96..7c6594bba796 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>> @@ -1354,6 +1354,11 @@ struct pptable_funcs {
>>>        * @init_pptable_microcode: Prepare the pptable microcode to 
>>> upload via PSP
>>>        */
>>>       int (*init_pptable_microcode)(struct smu_context *smu);
>>> +
>>> +    /**
>>> +     * @set_cpu_smt_enable: Set the CPU SMT status
>>> +     */
>>> +    int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
>>>   };
>>>   typedef enum {
>>
> 

RE: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Limonciello, Mario]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, March 23, 2023 21:29
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Yang, WenYou
> <WenYou.Yang@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: Li, Ying <YING.LI@amd.com>; Liu, Kun <Kun.Liu2@amd.com>; Liang,
> Richard qi <Richardqi.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw
> 
> 
> 
> On 3/23/2023 11:36 PM, Limonciello, Mario wrote:
> > On 3/23/2023 12:41, Limonciello, Mario wrote:
> >> On 3/22/2023 00:48, Wenyou Yang wrote:
> >>> When the CPU SMT status change in the fly, sent the SMT-enable
> >>> message to pmfw to notify it that the SMT status changed.
> >>>
> >>> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> >>> ---
> >>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41
> +++++++++++++++++++
> >>>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
> >>>   2 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> index b5d64749990e..5cd85a9d149d 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> @@ -22,6 +22,7 @@
> >>>   #define SWSMU_CODE_LAYER_L1
> >>> +#include <linux/cpu.h>
> >>>   #include <linux/firmware.h>
> >>>   #include <linux/pci.h>
> >>> @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle,
> >>> uint32_t speed);
> >>>   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
> >>>   static int smu_set_mp1_state(void *handle, enum pp_mp1_state
> >>> mp1_state);
> >>> +static int smt_notifier_callback(struct notifier_block *nb, unsigned
> >>> long action, void *data);
> >>> +
> >>> +extern struct raw_notifier_head smt_notifier_head;
> >>> +
> >>> +static struct notifier_block smt_notifier = {
> >>> +    .notifier_call = smt_notifier_callback,
> >>> +};
> >>> +
> >>>   static int smu_sys_get_pp_feature_mask(void *handle,
> >>>                          char *buf)
> >>>   {
> >>> @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device
> *adev)
> >>>       return 0;
> >>>   }
> >>> +static struct smu_context *current_smu;
> >>> +
> >>>   static int smu_early_init(void *handle)
> >>>   {
> >>>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>> @@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
> >>>       mutex_init(&smu->message_lock);
> >>>       adev->powerplay.pp_handle = smu;
> >>> +    current_smu = smu;
> >
> > Although this series is intended for the Van Gogh case right now, I
> > dont't think this would scale well for multiple GPUs in a system.
> >
> > I think that instead you may want to move the notifier callback to be a
> > level "higher" in amdgpu.  Perhaps amdgpu_device.c?  Then when that
> > notifier call is received you'll want to walk through the PCI device
> > space to find any GPUs that are bound with AMDGPU a series of
> > wrappers/calls that end up calling smu_set_cpu_smt_enable with the
> > approriate arguments.
> >
> 
> This is not required when the notifier is registered only within Vangogh
> ppt function. Then follow Evan's suggestion of keeping the notifier
> block inside smu. From the notifier block, it can find the smu block and
> then call cpu_smt_enable/disable. That way notifier callback comes only
> once even with multiple dGPUs + Vangogh and processed for the
> corresponding smu.
> 
> This notifier doesn't need to be registered for platforms only with
> dGPUs or APUs which don't need this.

They don't right now, but I was thinking how this could scale to other
APUs or dGPUs if they are interested in adding support for this message
too.

> 
> Thanks,
> Lijo
> 
> >
> >>>       adev->powerplay.pp_funcs = &swsmu_pm_funcs;
> >>>       r = smu_set_funcs(adev);
> >>> @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
> >>>       if (!smu->ppt_funcs->get_fan_control_mode)
> >>>           smu->adev->pm.no_fan = true;
> >>> +    raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
> >>> +
> >>>       return 0;
> >>>   }
> >>> @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
> >>>       smu_fini_microcode(smu);
> >>> +    raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
> >>> +
> >>>       return 0;
> >>>   }
> >>> @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct
> >>> smu_context *smu, uint32_t size)
> >>>       return ret;
> >>>   }
> >>> +
> >>> +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool
> enable)
> >>> +{
> >>> +    int ret = -EINVAL;
> >>> +
> >>> +    if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
> >>> +        ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
> >>> +
> >>> +    return ret;
> >>> +}
> >>> +
> >>> +static int smt_notifier_callback(struct notifier_block *nb,
> >>> +                 unsigned long action, void *data)
> >>> +{
> >>> +    struct smu_context *smu = current_smu;
> >>> +    int ret = NOTIFY_OK;
> >>
> >> This initialization is pointless, it's clobbered in the next line.
> >>
> >>> +
> >>> +    ret = (action == SMT_ENABLED) ?
> >>> +                smu_set_cpu_smt_enable(smu, true) :
> >>> +                smu_set_cpu_smt_enable(smu, false);
> >>
> >> How about this instead, it should be more readable:
> >>
> >>      ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);
> >>
> >>> +    if (ret)
> >>> +        ret = NOTIFY_BAD;
> >>> +
> >>> +    return ret;
> >>
> >> How about instead:
> >>
> >>      dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action ==
> >> SMT_ENABLED ? "en" : "dis", ret);
> >>
> >>      return ret ? NOTIFY_BAD : NOTIFY_OK;
> >>
> >>> +}
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> index 09469c750a96..7c6594bba796 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> @@ -1354,6 +1354,11 @@ struct pptable_funcs {
> >>>        * @init_pptable_microcode: Prepare the pptable microcode to
> >>> upload via PSP
> >>>        */
> >>>       int (*init_pptable_microcode)(struct smu_context *smu);
> >>> +
> >>> +    /**
> >>> +     * @set_cpu_smt_enable: Set the CPU SMT status
> >>> +     */
> >>> +    int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
> >>>   };
> >>>   typedef enum {
> >>
> >

Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Lazar, Lijo]

[-- Attachment #1: Type: text/plain, Size: 7934 bytes --]

[AMD Official Use Only - General]



The notifier block is embedded in smu context of a device. If there are 4 devices and 3 of them are interested, they register using the notifier block within their smu context. Notifier is called in a chain and when received they use the container_of to get the smu context and communicate with the respective device's FW on the actions to do.

BTW, I don't know why dGPU PMFW would be interested in SMT change. On AMD APU + dGPU we already have things like smartshift and it will take care if communicated to APU alone.

Thanks,
Lijo
________________________________
From: Limonciello, Mario <Mario.Limonciello@amd.com>
Sent: Friday, March 24, 2023 11:19:55 PM
To: Lazar, Lijo <Lijo.Lazar@amd.com>; Yang, WenYou <WenYou.Yang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
Cc: Li, Ying <YING.LI@amd.com>; Liu, Kun <Kun.Liu2@amd.com>; Liang, Richard qi <Richardqi.Liang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: RE: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[AMD Official Use Only - General]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, March 23, 2023 21:29
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Yang, WenYou
> <WenYou.Yang@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: Li, Ying <YING.LI@amd.com>; Liu, Kun <Kun.Liu2@amd.com>; Liang,
> Richard qi <Richardqi.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw
>
>
>
> On 3/23/2023 11:36 PM, Limonciello, Mario wrote:
> > On 3/23/2023 12:41, Limonciello, Mario wrote:
> >> On 3/22/2023 00:48, Wenyou Yang wrote:
> >>> When the CPU SMT status change in the fly, sent the SMT-enable
> >>> message to pmfw to notify it that the SMT status changed.
> >>>
> >>> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> >>> ---
> >>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41
> +++++++++++++++++++
> >>>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
> >>>   2 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> index b5d64749990e..5cd85a9d149d 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> @@ -22,6 +22,7 @@
> >>>   #define SWSMU_CODE_LAYER_L1
> >>> +#include <linux/cpu.h>
> >>>   #include <linux/firmware.h>
> >>>   #include <linux/pci.h>
> >>> @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle,
> >>> uint32_t speed);
> >>>   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
> >>>   static int smu_set_mp1_state(void *handle, enum pp_mp1_state
> >>> mp1_state);
> >>> +static int smt_notifier_callback(struct notifier_block *nb, unsigned
> >>> long action, void *data);
> >>> +
> >>> +extern struct raw_notifier_head smt_notifier_head;
> >>> +
> >>> +static struct notifier_block smt_notifier = {
> >>> +    .notifier_call = smt_notifier_callback,
> >>> +};
> >>> +
> >>>   static int smu_sys_get_pp_feature_mask(void *handle,
> >>>                          char *buf)
> >>>   {
> >>> @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device
> *adev)
> >>>       return 0;
> >>>   }
> >>> +static struct smu_context *current_smu;
> >>> +
> >>>   static int smu_early_init(void *handle)
> >>>   {
> >>>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>> @@ -645,6 +656,7 @@ static int smu_early_init(void *handle)
> >>>       mutex_init(&smu->message_lock);
> >>>       adev->powerplay.pp_handle = smu;
> >>> +    current_smu = smu;
> >
> > Although this series is intended for the Van Gogh case right now, I
> > dont't think this would scale well for multiple GPUs in a system.
> >
> > I think that instead you may want to move the notifier callback to be a
> > level "higher" in amdgpu.  Perhaps amdgpu_device.c?  Then when that
> > notifier call is received you'll want to walk through the PCI device
> > space to find any GPUs that are bound with AMDGPU a series of
> > wrappers/calls that end up calling smu_set_cpu_smt_enable with the
> > approriate arguments.
> >
>
> This is not required when the notifier is registered only within Vangogh
> ppt function. Then follow Evan's suggestion of keeping the notifier
> block inside smu. From the notifier block, it can find the smu block and
> then call cpu_smt_enable/disable. That way notifier callback comes only
> once even with multiple dGPUs + Vangogh and processed for the
> corresponding smu.
>
> This notifier doesn't need to be registered for platforms only with
> dGPUs or APUs which don't need this.

They don't right now, but I was thinking how this could scale to other
APUs or dGPUs if they are interested in adding support for this message
too.

>
> Thanks,
> Lijo
>
> >
> >>>       adev->powerplay.pp_funcs = &swsmu_pm_funcs;
> >>>       r = smu_set_funcs(adev);
> >>> @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
> >>>       if (!smu->ppt_funcs->get_fan_control_mode)
> >>>           smu->adev->pm.no_fan = true;
> >>> +    raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
> >>> +
> >>>       return 0;
> >>>   }
> >>> @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
> >>>       smu_fini_microcode(smu);
> >>> +    raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
> >>> +
> >>>       return 0;
> >>>   }
> >>> @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct
> >>> smu_context *smu, uint32_t size)
> >>>       return ret;
> >>>   }
> >>> +
> >>> +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool
> enable)
> >>> +{
> >>> +    int ret = -EINVAL;
> >>> +
> >>> +    if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
> >>> +        ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
> >>> +
> >>> +    return ret;
> >>> +}
> >>> +
> >>> +static int smt_notifier_callback(struct notifier_block *nb,
> >>> +                 unsigned long action, void *data)
> >>> +{
> >>> +    struct smu_context *smu = current_smu;
> >>> +    int ret = NOTIFY_OK;
> >>
> >> This initialization is pointless, it's clobbered in the next line.
> >>
> >>> +
> >>> +    ret = (action == SMT_ENABLED) ?
> >>> +                smu_set_cpu_smt_enable(smu, true) :
> >>> +                smu_set_cpu_smt_enable(smu, false);
> >>
> >> How about this instead, it should be more readable:
> >>
> >>      ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);
> >>
> >>> +    if (ret)
> >>> +        ret = NOTIFY_BAD;
> >>> +
> >>> +    return ret;
> >>
> >> How about instead:
> >>
> >>      dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action ==
> >> SMT_ENABLED ? "en" : "dis", ret);
> >>
> >>      return ret ? NOTIFY_BAD : NOTIFY_OK;
> >>
> >>> +}
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> index 09469c750a96..7c6594bba796 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >>> @@ -1354,6 +1354,11 @@ struct pptable_funcs {
> >>>        * @init_pptable_microcode: Prepare the pptable microcode to
> >>> upload via PSP
> >>>        */
> >>>       int (*init_pptable_microcode)(struct smu_context *smu);
> >>> +
> >>> +    /**
> >>> +     * @set_cpu_smt_enable: Set the CPU SMT status
> >>> +     */
> >>> +    int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
> >>>   };
> >>>   typedef enum {
> >>
> >

[-- Attachment #2: Type: text/html, Size: 12828 bytes --]

RE: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw

[Yang, WenYou]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, March 24, 2023 1:42 AM
> To: Yang, WenYou <WenYou.Yang@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: Li, Ying <YING.LI@amd.com>; Liu, Kun <Kun.Liu2@amd.com>; amd-
> gfx@lists.freedesktop.org; Liang, Richard qi <Richardqi.Liang@amd.com>
> Subject: Re: [v1,2/3] drm/amd/pm: send the SMT-enable message to pmfw
> 
> On 3/22/2023 00:48, Wenyou Yang wrote:
> > When the CPU SMT status change in the fly, sent the SMT-enable message
> > to pmfw to notify it that the SMT status changed.
> >
> > Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 41
> +++++++++++++++++++
> >   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +++
> >   2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index b5d64749990e..5cd85a9d149d 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -22,6 +22,7 @@
> >
> >   #define SWSMU_CODE_LAYER_L1
> >
> > +#include <linux/cpu.h>
> >   #include <linux/firmware.h>
> >   #include <linux/pci.h>
> >
> > @@ -69,6 +70,14 @@ static int smu_set_fan_speed_rpm(void *handle,
> uint32_t speed);
> >   static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
> >   static int smu_set_mp1_state(void *handle, enum pp_mp1_state
> > mp1_state);
> >
> > +static int smt_notifier_callback(struct notifier_block *nb, unsigned
> > +long action, void *data);
> > +
> > +extern struct raw_notifier_head smt_notifier_head;
> > +
> > +static struct notifier_block smt_notifier = {
> > +	.notifier_call = smt_notifier_callback, };
> > +
> >   static int smu_sys_get_pp_feature_mask(void *handle,
> >   				       char *buf)
> >   {
> > @@ -625,6 +634,8 @@ static int smu_set_funcs(struct amdgpu_device
> *adev)
> >   	return 0;
> >   }
> >
> > +static struct smu_context *current_smu;
> > +
> >   static int smu_early_init(void *handle)
> >   {
> >   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> @@
> > -645,6 +656,7 @@ static int smu_early_init(void *handle)
> >   	mutex_init(&smu->message_lock);
> >
> >   	adev->powerplay.pp_handle = smu;
> > +	current_smu = smu;
> >   	adev->powerplay.pp_funcs = &swsmu_pm_funcs;
> >
> >   	r = smu_set_funcs(adev);
> > @@ -1105,6 +1117,8 @@ static int smu_sw_init(void *handle)
> >   	if (!smu->ppt_funcs->get_fan_control_mode)
> >   		smu->adev->pm.no_fan = true;
> >
> > +	raw_notifier_chain_register(&smt_notifier_head, &smt_notifier);
> > +
> >   	return 0;
> >   }
> >
> > @@ -1122,6 +1136,8 @@ static int smu_sw_fini(void *handle)
> >
> >   	smu_fini_microcode(smu);
> >
> > +	raw_notifier_chain_unregister(&smt_notifier_head, &smt_notifier);
> > +
> >   	return 0;
> >   }
> >
> > @@ -3241,3 +3257,28 @@ int smu_send_hbm_bad_channel_flag(struct
> > smu_context *smu, uint32_t size)
> >
> >   	return ret;
> >   }
> > +
> > +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool
> > +enable) {
> > +	int ret = -EINVAL;
> > +
> > +	if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
> > +		ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
> > +
> > +	return ret;
> > +}
> > +
> > +static int smt_notifier_callback(struct notifier_block *nb,
> > +				 unsigned long action, void *data) {
> > +	struct smu_context *smu = current_smu;
> > +	int ret = NOTIFY_OK;
> 
> This initialization is pointless, it's clobbered in the next line.

Yes, accept.
> 
> > +
> > +	ret = (action == SMT_ENABLED) ?
> > +				smu_set_cpu_smt_enable(smu, true) :
> > +				smu_set_cpu_smt_enable(smu, false);
> 
> How about this instead, it should be more readable:
> 
> 	ret = smu_set_cpu_smt_enable(smu, action == SMT_ENABLED);

Accept.
> 
> > +	if (ret)
> > +		ret = NOTIFY_BAD;
> > +
> > +	return ret;
> 
> How about instead:
> 
> 	dev_dbg(adev->dev, "failed to %sable SMT: %d\n", action ==
> SMT_ENABLED ? "en" : "dis", ret);
> 
> 	return ret ? NOTIFY_BAD : NOTIFY_OK;
> 

Accept.

Thanks 
Wenyou

> > +}
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > index 09469c750a96..7c6594bba796 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > @@ -1354,6 +1354,11 @@ struct pptable_funcs {
> >   	 * @init_pptable_microcode: Prepare the pptable microcode to
> upload via PSP
> >   	 */
> >   	int (*init_pptable_microcode)(struct smu_context *smu);
> > +
> > +	/**
> > +	 * @set_cpu_smt_enable: Set the CPU SMT status
> > +	 */
> > +	int (*set_cpu_smt_enable)(struct smu_context *smu, bool enable);
> >   };
> >
> >   typedef enum {

[PATCH v1 3/3] drm/amd/pm: vangogh: support to send SMT enable message

[Wenyou Yang]

Add the support to PPSMC_MSG_SetCClkSMTEnable(0x58) message to pmfw
for vangogh.

Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
---
 .../pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h    |  3 ++-
 drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h  |  3 ++-
 .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  | 19 +++++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
index 7471e2df2828..2b182dbc6f9c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
@@ -111,7 +111,8 @@
 #define PPSMC_MSG_GetGfxOffStatus		       0x50
 #define PPSMC_MSG_GetGfxOffEntryCount		       0x51
 #define PPSMC_MSG_LogGfxOffResidency		       0x52
-#define PPSMC_Message_Count                            0x53
+#define PPSMC_MSG_SetCClkSMTEnable		       0x58
+#define PPSMC_Message_Count                            0x54
 
 //Argument for PPSMC_MSG_GfxDeviceDriverReset
 enum {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
index 297b70b9388f..820812d910bf 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
@@ -245,7 +245,8 @@
 	__SMU_DUMMY_MAP(AllowGpo),	\
 	__SMU_DUMMY_MAP(Mode2Reset),	\
 	__SMU_DUMMY_MAP(RequestI2cTransaction), \
-	__SMU_DUMMY_MAP(GetMetricsTable),
+	__SMU_DUMMY_MAP(GetMetricsTable), \
+	__SMU_DUMMY_MAP(SetCClkSMTEnable),
 
 #undef __SMU_DUMMY_MAP
 #define __SMU_DUMMY_MAP(type)	SMU_MSG_##type
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
index 7433dcaa16e0..f0eeb42df96b 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
@@ -141,6 +141,7 @@ static struct cmn2asic_msg_mapping vangogh_message_map[SMU_MSG_MAX_COUNT] = {
 	MSG_MAP(GetGfxOffStatus,		    PPSMC_MSG_GetGfxOffStatus,						0),
 	MSG_MAP(GetGfxOffEntryCount,		    PPSMC_MSG_GetGfxOffEntryCount,					0),
 	MSG_MAP(LogGfxOffResidency,		    PPSMC_MSG_LogGfxOffResidency,					0),
+	MSG_MAP(SetCClkSMTEnable,		    PPSMC_MSG_SetCClkSMTEnable,						0),
 };
 
 static struct cmn2asic_mapping vangogh_feature_mask_map[SMU_FEATURE_COUNT] = {
@@ -2428,6 +2429,23 @@ static u32 vangogh_get_gfxoff_entrycount(struct smu_context *smu, uint64_t *entr
 	return ret;
 }
 
+static int vangogh_set_cpu_smt_enable(struct smu_context *smu, bool enable)
+{
+	int ret = 0;
+
+	if (enable) {
+		ret = smu_cmn_send_smc_msg_with_param(smu,
+						      SMU_MSG_SetCClkSMTEnable,
+						      1, NULL);
+	} else {
+		ret = smu_cmn_send_smc_msg_with_param(smu,
+						      SMU_MSG_SetCClkSMTEnable,
+						      0, NULL);
+	}
+
+	return ret;
+}
+
 static const struct pptable_funcs vangogh_ppt_funcs = {
 
 	.check_fw_status = smu_v11_0_check_fw_status,
@@ -2474,6 +2492,7 @@ static const struct pptable_funcs vangogh_ppt_funcs = {
 	.get_power_limit = vangogh_get_power_limit,
 	.set_power_limit = vangogh_set_power_limit,
 	.get_vbios_bootup_values = smu_v11_0_get_vbios_bootup_values,
+	.set_cpu_smt_enable = vangogh_set_cpu_smt_enable,
 };
 
 void vangogh_set_ppt_funcs(struct smu_context *smu)
-- 
2.39.2

Re: [v1,3/3] drm/amd/pm: vangogh: support to send SMT enable message

[Limonciello, Mario]

On 3/22/2023 00:48, Wenyou Yang wrote:
> Add the support to PPSMC_MSG_SetCClkSMTEnable(0x58) message to pmfw
> for vangogh.
> 
> Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> ---
>   .../pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h    |  3 ++-
>   drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h  |  3 ++-
>   .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  | 19 +++++++++++++++++++
>   3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> index 7471e2df2828..2b182dbc6f9c 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> @@ -111,7 +111,8 @@
>   #define PPSMC_MSG_GetGfxOffStatus		       0x50
>   #define PPSMC_MSG_GetGfxOffEntryCount		       0x51
>   #define PPSMC_MSG_LogGfxOffResidency		       0x52
> -#define PPSMC_Message_Count                            0x53
> +#define PPSMC_MSG_SetCClkSMTEnable		       0x58
> +#define PPSMC_Message_Count                            0x54

This doesn't make sense that the PPSMC_Message_Count would be smaller 
than the biggest message.  This should be:

#define PPSMC_Message_Count 0x59

>   
>   //Argument for PPSMC_MSG_GfxDeviceDriverReset
>   enum {
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> index 297b70b9388f..820812d910bf 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> @@ -245,7 +245,8 @@
>   	__SMU_DUMMY_MAP(AllowGpo),	\
>   	__SMU_DUMMY_MAP(Mode2Reset),	\
>   	__SMU_DUMMY_MAP(RequestI2cTransaction), \
> -	__SMU_DUMMY_MAP(GetMetricsTable),
> +	__SMU_DUMMY_MAP(GetMetricsTable), \
> +	__SMU_DUMMY_MAP(SetCClkSMTEnable),
>   
>   #undef __SMU_DUMMY_MAP
>   #define __SMU_DUMMY_MAP(type)	SMU_MSG_##type
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index 7433dcaa16e0..f0eeb42df96b 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -141,6 +141,7 @@ static struct cmn2asic_msg_mapping vangogh_message_map[SMU_MSG_MAX_COUNT] = {
>   	MSG_MAP(GetGfxOffStatus,		    PPSMC_MSG_GetGfxOffStatus,						0),
>   	MSG_MAP(GetGfxOffEntryCount,		    PPSMC_MSG_GetGfxOffEntryCount,					0),
>   	MSG_MAP(LogGfxOffResidency,		    PPSMC_MSG_LogGfxOffResidency,					0),
> +	MSG_MAP(SetCClkSMTEnable,		    PPSMC_MSG_SetCClkSMTEnable,						0),
>   };
>   
>   static struct cmn2asic_mapping vangogh_feature_mask_map[SMU_FEATURE_COUNT] = {
> @@ -2428,6 +2429,23 @@ static u32 vangogh_get_gfxoff_entrycount(struct smu_context *smu, uint64_t *entr
>   	return ret;
>   }
>   
> +static int vangogh_set_cpu_smt_enable(struct smu_context *smu, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (enable) {
> +		ret = smu_cmn_send_smc_msg_with_param(smu,
> +						      SMU_MSG_SetCClkSMTEnable,
> +						      1, NULL);
> +	} else {
> +		ret = smu_cmn_send_smc_msg_with_param(smu,
> +						      SMU_MSG_SetCClkSMTEnable,
> +						      0, NULL);
> +	}
> +
> +	return ret;
> +}
> +
>   static const struct pptable_funcs vangogh_ppt_funcs = {
>   
>   	.check_fw_status = smu_v11_0_check_fw_status,
> @@ -2474,6 +2492,7 @@ static const struct pptable_funcs vangogh_ppt_funcs = {
>   	.get_power_limit = vangogh_get_power_limit,
>   	.set_power_limit = vangogh_set_power_limit,
>   	.get_vbios_bootup_values = smu_v11_0_get_vbios_bootup_values,
> +	.set_cpu_smt_enable = vangogh_set_cpu_smt_enable,
>   };
>   
>   void vangogh_set_ppt_funcs(struct smu_context *smu)

RE: [v1,3/3] drm/amd/pm: vangogh: support to send SMT enable message

[Yang, WenYou]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, March 24, 2023 1:15 AM
> To: Yang, WenYou <WenYou.Yang@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: Li, Ying <YING.LI@amd.com>; Liu, Kun <Kun.Liu2@amd.com>; amd-
> gfx@lists.freedesktop.org; Liang, Richard qi <Richardqi.Liang@amd.com>
> Subject: Re: [v1,3/3] drm/amd/pm: vangogh: support to send SMT enable
> message
> 
> On 3/22/2023 00:48, Wenyou Yang wrote:
> > Add the support to PPSMC_MSG_SetCClkSMTEnable(0x58) message to
> pmfw
> > for vangogh.
> >
> > Signed-off-by: Wenyou Yang <WenYou.Yang@amd.com>
> > ---
> >   .../pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h    |  3 ++-
> >   drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h  |  3 ++-
> >   .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  | 19
> +++++++++++++++++++
> >   3 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > index 7471e2df2828..2b182dbc6f9c 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > +++
> b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > @@ -111,7 +111,8 @@
> >   #define PPSMC_MSG_GetGfxOffStatus		       0x50
> >   #define PPSMC_MSG_GetGfxOffEntryCount		       0x51
> >   #define PPSMC_MSG_LogGfxOffResidency		       0x52
> > -#define PPSMC_Message_Count                            0x53
> > +#define PPSMC_MSG_SetCClkSMTEnable		       0x58
> > +#define PPSMC_Message_Count                            0x54
> 
> This doesn't make sense that the PPSMC_Message_Count would be smaller
> than the biggest message.  This should be:
> 
> #define PPSMC_Message_Count 0x59
> 
Accepted.

Thanks
Wenyou
> >
> >   //Argument for PPSMC_MSG_GfxDeviceDriverReset
> >   enum {
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > index 297b70b9388f..820812d910bf 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > @@ -245,7 +245,8 @@
> >   	__SMU_DUMMY_MAP(AllowGpo),	\
> >   	__SMU_DUMMY_MAP(Mode2Reset),	\
> >   	__SMU_DUMMY_MAP(RequestI2cTransaction), \
> > -	__SMU_DUMMY_MAP(GetMetricsTable),
> > +	__SMU_DUMMY_MAP(GetMetricsTable), \
> > +	__SMU_DUMMY_MAP(SetCClkSMTEnable),
> >
> >   #undef __SMU_DUMMY_MAP
> >   #define __SMU_DUMMY_MAP(type)	SMU_MSG_##type
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > index 7433dcaa16e0..f0eeb42df96b 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > @@ -141,6 +141,7 @@ static struct cmn2asic_msg_mapping
> vangogh_message_map[SMU_MSG_MAX_COUNT] = {
> >   	MSG_MAP(GetGfxOffStatus,
> PPSMC_MSG_GetGfxOffStatus,
> 	0),
> >   	MSG_MAP(GetGfxOffEntryCount,
> PPSMC_MSG_GetGfxOffEntryCount,					0),
> >   	MSG_MAP(LogGfxOffResidency,
> PPSMC_MSG_LogGfxOffResidency,					0),
> > +	MSG_MAP(SetCClkSMTEnable,
> PPSMC_MSG_SetCClkSMTEnable,
> 	0),
> >   };
> >
> >   static struct cmn2asic_mapping
> > vangogh_feature_mask_map[SMU_FEATURE_COUNT] = { @@ -2428,6
> +2429,23 @@ static u32 vangogh_get_gfxoff_entrycount(struct
> smu_context *smu, uint64_t *entr
> >   	return ret;
> >   }
> >
> > +static int vangogh_set_cpu_smt_enable(struct smu_context *smu, bool
> > +enable) {
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		ret = smu_cmn_send_smc_msg_with_param(smu,
> > +
> SMU_MSG_SetCClkSMTEnable,
> > +						      1, NULL);
> > +	} else {
> > +		ret = smu_cmn_send_smc_msg_with_param(smu,
> > +
> SMU_MSG_SetCClkSMTEnable,
> > +						      0, NULL);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >   static const struct pptable_funcs vangogh_ppt_funcs = {
> >
> >   	.check_fw_status = smu_v11_0_check_fw_status, @@ -2474,6
> +2492,7 @@
> > static const struct pptable_funcs vangogh_ppt_funcs = {
> >   	.get_power_limit = vangogh_get_power_limit,
> >   	.set_power_limit = vangogh_set_power_limit,
> >   	.get_vbios_bootup_values = smu_v11_0_get_vbios_bootup_values,
> > +	.set_cpu_smt_enable = vangogh_set_cpu_smt_enable,
> >   };
> >
> >   void vangogh_set_ppt_funcs(struct smu_context *smu)