Hi Zhi,
On Sat Feb 8, 2025 at 2:58 AM JST, Zhi Wang wrote:
> There can be multiple cases of handling the GSP RPC messages, which are
> the reply of GSP RPC commands according to the requirement of the
> callers and the nature of the GSP RPC commands.
>
> To support all cases, first, centralize the handling of the reply in a
> single function. Factor out r535_gsp_rpc_handle_reply().
>
> No functional change is intended for small GSP RPC commands. For large GSP
> commands, the caller decides the policy of how to handle the returned GSP
> RPC message.
>
> Signed-off-by: Zhi Wang <z...@nvidia.com>
> ---
> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 48 +++++++++----------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> index 2075cad63805..1ed7d5624a56 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> @@ -583,14 +583,30 @@ r535_gsp_rpc_poll(struct nvkm_gsp *gsp, u32 fn)
> return 0;
> }
>
> +static void *
> +r535_gsp_rpc_handle_reply(struct nvkm_gsp *gsp, u32 fn, bool wait,
> + u32 gsp_rpc_len)
> +{
> + struct nvfw_gsp_rpc *msg;
> + void *repv = NULL;
> +
> + if (wait) {
> + msg = r535_gsp_msg_recv(gsp, fn, gsp_rpc_len);
> + if (!IS_ERR_OR_NULL(msg))
> + repv = msg->data;
> + else
> + repv = msg;
> + }
> +
> + return repv;
> +}
> +
> static void *
> r535_gsp_rpc_send(struct nvkm_gsp *gsp, void *payload, bool wait,
> u32 gsp_rpc_len)
> {
> struct nvfw_gsp_rpc *rpc = to_gsp_hdr(payload, rpc);
> - struct nvfw_gsp_rpc *msg;
> - u32 fn = rpc->function;
> - void *repv = NULL;
> + u32 function = rpc->function;
Is it necessary to rename 'fn' here? You don't do it in
r535_gsp_rpc_push()...
> int ret;
>
> if (gsp->subdev.debug >= NV_DBG_TRACE) {
> @@ -604,15 +620,7 @@ r535_gsp_rpc_send(struct nvkm_gsp *gsp, void *payload,
> bool wait,
> if (ret)
> return ERR_PTR(ret);
>
> - if (wait) {
> - msg = r535_gsp_msg_recv(gsp, fn, gsp_rpc_len);
> - if (!IS_ERR_OR_NULL(msg))
> - repv = msg->data;
> - else
> - repv = msg;
> - }
> -
> - return repv;
> + return r535_gsp_rpc_handle_reply(gsp, function, wait, gsp_rpc_len);
> }
>
> static void
> @@ -996,18 +1004,10 @@ r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload,
> bool wait,
> }
>
> /* Wait for reply. */
> - rpc = r535_gsp_msg_recv(gsp, fn, payload_size +
> - sizeof(*rpc));
> - if (!IS_ERR_OR_NULL(rpc)) {
> - if (wait) {
> - repv = rpc->data;
> - } else {
> - nvkm_gsp_rpc_done(gsp, rpc);
> - repv = NULL;
> - }
> - } else {
> - repv = wait ? rpc : NULL;
> - }
> + repv = r535_gsp_rpc_handle_reply(gsp, fn, wait, payload_size +
> + sizeof(*rpc));
> + if (IS_ERR_OR_NULL(repv))
> + repv = wait ? repv : NULL;
I'm not familiar with this code, so maybe that's nothing, but is it ok
to drop the call to nvkm_gsp_rpc_done() that was done in the original
code if wait == false?
