On Wed, 12 Feb 2025 15:55:08 +0900
"Alexandre Courbot" <acourbot@xxxxxxxxxx> 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@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 :))
>
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.
>
