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

[Zhi Wang <zhiw@nvidia.com>]

On Wed, 12 Feb 2025 15:55:08 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> 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 :))
> 

Thanks for the idea.

I was struggling to come up with a perfect name as well. DONT_CARE was on
the list, but it seems so heavy when considering that DONT_CARE is the
most common case and not looking ideal when it spreads to the call sites. :)

Probably I will go with NOWAIT.

> > +
> >  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.
>