[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.
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, ¶m, 1,
> read_arg);
> + else
> + ret = smu_cmn_send_msg_internal(smu, msg, 1, ¶m, 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 */
