Re: [PATCH 2/5] drm/nouveau/nvkm: factor out the current RPC command reply policies

["Alexandre Courbot" <acourbot@nvidia.com>]

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@nvidia.com>
> ---
>  .../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.