Re: [PATCH 1/4] drm/amd/pm: add variant func smu_cmn_send_msg() to unify msg sending logic

Tue, 24 Mar 2026 02:56:48 -0700 



On 24-Mar-26 3:00 PM, Wang, Yang(Kevin) wrote:

[AMD Official Use Only - AMD Internal Distribution Only]

Your comments rely on subjective assumptions ("I don't think") lacking 
technical justification.
this increases review cost and slows progress without clear technical basis.

my approach stays simple: this patch optimizes common message paths and hides 
unnecessary low-level details.
And the original low‑level APIs remain available, so, developers can still use 
them whenever fine‑grained control is required.


These are the ones in case you missed -


1) This uses same function with multiple variants and it is difficult to 
figure out which form to use.



2) It also becomes reading coding the difficult. Refer back the same 
message to figure out which form is being used.



3) The existing implementation is not complicated, there is nothing 
lowlevel in that. It's just a form of API which takes a struct instead 
of 11 parameters for in/out (at it fullest level it can have 12 
parameters in this form). It's much clearer to read the code in that 
usage and it presents a uniform API rather than multiple forms of usage.


Thanks,
Lijo



Best Regards,
Kevin

-----Original Message-----
From: Lazar, Lijo <[email protected]>
Sent: Tuesday, March 24, 2026 2:12 PM
To: Wang, Yang(Kevin) <[email protected]>; [email protected]
Cc: Deucher, Alexander <[email protected]>; Zhang, Hawking 
<[email protected]>; Feng, Kenneth <[email protected]>
Subject: Re: [PATCH 1/4] drm/amd/pm: add variant func smu_cmn_send_msg() to 
unify msg sending logic



On 24-Mar-26 4:42 AM, Yang Wang wrote:

add variant func smu_cmn_send_msg() to unify smu message sending
logic, and enabling support for newer ASIC interfaces such as SMU v15 and 
upcoming devices.
(support multi-param/multi-response, standardize code across all smu
code layers)

The smu_cmn_send_msg() API will expand to the following prototypes based on the 
number of input parameters.
e.g:
1. r = smu_cmn_send_msg(smu, msg_id);
2. r = smu_cmn_send_msg(smu, msg_id, &read_arg); 3. r =
smu_cmn_send_msg(smu, msg_id, param, &read_arg); 4. r =
smu_cmn_send_msg(smu, msg_id,
                       num_param, [param0, param1, ...],
                       num_response, [arg0, arg1, ...]


As mentioned earlier, I don't think this helps with readability of the code. 
This commit message has to be referred always regarding usage. For
ex: if I want to send a message which has only one output argument and no input 
argument, it takes a while to figure out which form to use.

Instead, it's better to expose args structure directly to user. I don't think 
it's so complicated to use like this.

https://gitlab.freedesktop.org/agd5f/linux/-/blob/drm-next/drivers/gpu/drm/amd/pm/swsmu/smu15/smu_v15_0.c#L627

Maybe I've a biased view, I feel like it's easier to read this way about what 
is being done.

Thanks,
Lijo




Signed-off-by: Yang Wang <[email protected]>
---
   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 79 ++++++++++++++++++++++++++
   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h | 22 +++++++
   2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
index 7bd8c435466a..480d91d88957 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
@@ -193,6 +193,85 @@ int smu_cmn_send_smc_msg(struct smu_context *smu,
                                              read_arg);
   }

+static inline int smu_cmn_send_msg_internal(struct smu_context *smu, enum 
smu_message_type msg,
+                                         int num_in_args, u32 *in_args,
+                                         int num_out_args, u32 *out_args) {
+     struct smu_msg_ctl *ctl = &smu->msg_ctl;
+     struct smu_msg_args args = { 0 };
+     int ret;
+
+     if (msg >= SMU_MSG_MAX_COUNT)
+             return -EINVAL;
+
+     if ((num_in_args >= ARRAY_SIZE(args.args) || num_in_args < 0) ||
+         (num_out_args >= ARRAY_SIZE(args.out_args) || num_out_args < 0))
+             return -EINVAL;
+
+     if ((num_in_args > 0 && !in_args) || (num_out_args > 0 && !out_args))
+             return -EINVAL;
+
+     if (!ctl->ops || !ctl->ops->send_msg)
+             return -EOPNOTSUPP;
+
+     args.msg = msg;
+     args.num_args = num_in_args;
+     args.num_out_args = num_out_args;
+     args.flags = 0;
+     args.timeout = 0;
+
+     if (num_in_args)
+             memcpy(&args.args[0], in_args, num_in_args * sizeof(u32));
+
+     ret = ctl->ops->send_msg(ctl, &args);
+     if (ret)
+             return ret;
+
+     if (num_out_args)
+             memcpy(out_args, &args.out_args[0], num_out_args * sizeof(u32));
+
+     return ret;
+}
+
+/*
+ * NOTE: To ensure compatibility with the behavioral logic of the
+legacy API,
+ * it is required to explicitly set the parameter "param" to 0 when
+invoking
+ * the msg_0 and msg_1 functions.
+ * */
+
+int __smu_cmn_send_msg_0(struct smu_context *smu, enum
+smu_message_type msg) {
+     return __smu_cmn_send_msg_2(smu, msg, 0, NULL); }
+
+int __smu_cmn_send_msg_1(struct smu_context *smu, enum smu_message_type msg,
+                      u32 *read_arg)
+{
+     return __smu_cmn_send_msg_2(smu, msg, 0, read_arg); }
+
+int __smu_cmn_send_msg_2(struct smu_context *smu, enum smu_message_type msg,
+                      u32 param, u32 *read_arg)
+{
+     int ret;
+
+     if (read_arg)
+             ret = smu_cmn_send_msg_internal(smu, msg, 1, &param, 1, read_arg);
+     else
+             ret = smu_cmn_send_msg_internal(smu, msg, 1, &param, 0, NULL);
+
+     return ret;
+}
+
+int __smu_cmn_send_msg_4(struct smu_context *smu, enum smu_message_type msg,
+                      int num_in_args, u32 *in_args,
+                      int num_out_args, u32 *out_args)
+{
+     return smu_cmn_send_msg_internal(smu, msg,
+                                      num_in_args, in_args,
+                                      num_out_args, out_args);
+}
+
   int smu_cmn_send_debug_smc_msg(struct smu_context *smu,
                        uint32_t msg)
   {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
index b76e86df5da7..5c14ed9ed9b4 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
@@ -210,6 +210,28 @@ int smu_cmn_dpm_pcie_gen_idx(int gen);
   int smu_cmn_dpm_pcie_width_idx(int width);
   int smu_cmn_check_fw_version(struct smu_context *smu);

+int __smu_cmn_send_msg_0(struct smu_context *smu, enum
+smu_message_type msg); int __smu_cmn_send_msg_1(struct smu_context *smu, enum 
smu_message_type msg,
+                      u32 *read_arg);
+int __smu_cmn_send_msg_2(struct smu_context *smu, enum smu_message_type msg,
+                      u32 param, u32 *read_arg);
+int __smu_cmn_send_msg_4(struct smu_context *smu, enum smu_message_type msg,
+                      int num_in_args, u32 *in_args,
+                      int num_out_args, u32 *out_args);
+
+/*
+* The smu_cmn_send_msg() API will expand to the following prototypes based on 
the number of input parameters.
+* e.g:
+* 1. r = smu_cmn_send_msg(smu, msg_id);
+* 2. r = smu_cmn_send_msg(smu, msg_id, &read_arg);
+* 3. r = smu_cmn_send_msg(smu, msg_id, param, &read_arg);
+* 4. r = smu_cmn_send_msg(smu, msg_id,
+*                      num_param, [param0, param1, ...],
+*                      num_response, [arg0, arg1, ...]
+*/
+#define smu_cmn_send_msg(smu, msg, ...) \
+     CONCATENATE(__smu_cmn_send_msg_, COUNT_ARGS(__VA_ARGS__))(smu, msg,
+##__VA_ARGS__)
+
   /*SMU gpu metrics */

   /* Attribute ID mapping */


























					Reply via email to