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.
>
> The current supported reply policies are "callers don't care" or "receive
> the entire message" according to the requirement of the caller. For
> introducing a new policy, factor out the current RPC command reply
> polices.
>
> Factor out NVKM_GSP_RPC_REPLY_RECV as "receive the entire message". If
> none is specified, the default is "callers don't care".
>
> No functional change is intended.
>
> Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>
> ---
> .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 16 +++++---
> .../gpu/drm/nouveau/nvkm/subdev/bar/r535.c | 2 +-
> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 41 +++++++++++--------
> .../drm/nouveau/nvkm/subdev/instmem/r535.c | 2 +-
> 4 files changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> index 746e126c3ecf..c467e44cab47 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -31,6 +31,10 @@ typedef int (*nvkm_gsp_msg_ntfy_func)(void *priv, u32 fn, void *repv, u32 repc);
> struct nvkm_gsp_event;
> typedef void (*nvkm_gsp_event_func)(struct nvkm_gsp_event *, void *repv, u32 repc);
>
> +enum {
> + NVKM_GSP_RPC_REPLY_RECV = 1,
> +};
Let's turn this into a named type and add a variant for the 0 value, e.g.
enum nvkm_gsp_rpc_reply_type {
NVKM_GSP_RPC_DONT_CARE = 0,
NVKM_GSP_RPC_REPLY_RECV = 1,
}
and use this type instead of an integer in the client code. This will
make the compiler warn us if we try to pass an unexpected value.
(DONT_CARE needs to be defined to something less ambiguous, I left it
as-is because I don't really understand the intent behind this name :))
> +
> struct nvkm_gsp {
> const struct nvkm_gsp_func *func;
> struct nvkm_subdev subdev;
> @@ -188,7 +192,7 @@ struct nvkm_gsp {
>
> const struct nvkm_gsp_rm {
> void *(*rpc_get)(struct nvkm_gsp *, u32 fn, u32 argc);
> - void *(*rpc_push)(struct nvkm_gsp *, void *argv, bool wait, u32 repc);
> + void *(*rpc_push)(struct nvkm_gsp *gsp, void *argv, int reply, u32 repc);
> void (*rpc_done)(struct nvkm_gsp *gsp, void *repv);
>
> void *(*rm_ctrl_get)(struct nvkm_gsp_object *, u32 cmd, u32 argc);
> @@ -255,9 +259,9 @@ nvkm_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 argc)
> }
>
> static inline void *
> -nvkm_gsp_rpc_push(struct nvkm_gsp *gsp, void *argv, bool wait, u32 repc)
> +nvkm_gsp_rpc_push(struct nvkm_gsp *gsp, void *argv, int reply, u32 repc)
> {
> - return gsp->rm->rpc_push(gsp, argv, wait, repc);
> + return gsp->rm->rpc_push(gsp, argv, reply, repc);
> }
>
> static inline void *
> @@ -268,13 +272,13 @@ nvkm_gsp_rpc_rd(struct nvkm_gsp *gsp, u32 fn, u32 argc)
> if (IS_ERR_OR_NULL(argv))
> return argv;
>
> - return nvkm_gsp_rpc_push(gsp, argv, true, argc);
> + return nvkm_gsp_rpc_push(gsp, argv, NVKM_GSP_RPC_REPLY_RECV, argc);
> }
>
> static inline int
> -nvkm_gsp_rpc_wr(struct nvkm_gsp *gsp, void *argv, bool wait)
> +nvkm_gsp_rpc_wr(struct nvkm_gsp *gsp, void *argv, int reply)
> {
> - void *repv = nvkm_gsp_rpc_push(gsp, argv, wait, 0);
> + void *repv = nvkm_gsp_rpc_push(gsp, argv, reply, 0);
>
> if (IS_ERR(repv))
> return PTR_ERR(repv);
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
> index 3a30bea30e36..90186f98065c 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
> @@ -56,7 +56,7 @@ r535_bar_bar2_update_pde(struct nvkm_gsp *gsp, u64 addr)
> rpc->info.entryValue = addr ? ((addr >> 4) | 2) : 0; /* PD3 entry format! */
> rpc->info.entryLevelShift = 47; //XXX: probably fetch this from mmu!
>
> - return nvkm_gsp_rpc_wr(gsp, rpc, true);
> + return nvkm_gsp_rpc_wr(gsp, rpc, NVKM_GSP_RPC_REPLY_RECV);
> }
>
> static void
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> index 1ed7d5624a56..bc8eb9a3cb28 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> @@ -584,25 +584,32 @@ r535_gsp_rpc_poll(struct nvkm_gsp *gsp, u32 fn)
> }
>
> static void *
> -r535_gsp_rpc_handle_reply(struct nvkm_gsp *gsp, u32 fn, bool wait,
> +r535_gsp_rpc_handle_reply(struct nvkm_gsp *gsp, u32 fn, int reply,
> u32 gsp_rpc_len)
So here 'int reply' would become 'enum nvkm_gsp_rpc_reply_type reply'
(and propagate to other callers).
> {
> struct nvfw_gsp_rpc *msg;
> void *repv = NULL;
>
> - if (wait) {
> + if (!reply)
> + return NULL;
> +
> + switch (reply) {
> + case NVKM_GSP_RPC_REPLY_RECV:
> msg = r535_gsp_msg_recv(gsp, fn, gsp_rpc_len);
> if (!IS_ERR_OR_NULL(msg))
> repv = msg->data;
> else
> repv = msg;
> + break;
> + default:
> + repv = ERR_PTR(-EINVAL);
> + break;
> }
With the introduced type, this would become:
switch (reply) {
case NVKM_GSP_RPC_DONT_CARE:
/* Works as repv is NULL already */
break;
case NVKM_GSP_RPC_REPLY_RECV:
msg = r535_gsp_msg_recv(gsp, fn, gsp_rpc_len);
if (!IS_ERR_OR_NULL(msg))
repv = msg->data;
else
repv = msg;
break;
}
I'm not sure whether you still need a 'default' arm? The compiler is
happy without it, and since you control all the call sites nothing funny
can happen without a dirty cast.
> -
No need to remove this separator line IMHO.
