Re: [PATCH 1/5] drm/nouveau/nvkm: factor out r535_gsp_rpc_handle_reply()

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

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 <zhiw@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?