Re: [RFC 9/9] {fwctl,drm}/xe/pcode: Introduce xe_pcode_fwctl

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 29, 2025 at 09:39:56PM +0530, Badal Nilawar wrote:

> diff --git a/drivers/gpu/drm/xe/xe_pcode_fwctl.c b/drivers/gpu/drm/xe/xe_pcode_fwctl.c
> new file mode 100644

I really do prefer it if you can find a way to put the code in
drivers/fwctl instead of in DRM subsystem.
> +static int xe_pcode_fwctl_uctx_open(struct fwctl_uctx *uctx)
> +{
> +	struct xe_pcode_fwctl_dev *fwctl_dev =
> +		container_of(uctx->fwctl, struct xe_pcode_fwctl_dev, fwctl);
> +	struct xe_device *xe = fwctl_dev->xe;
> +
> +	xe_pm_runtime_get(xe);

Shouldn't this be in the RPC function? Why keep the device awake as
long as a the FD is open?

> +static void *xe_pcode_fwctl_rpc(struct fwctl_uctx *uctx,
> +				enum fwctl_rpc_scope scope,
> +				void *in, size_t in_len, size_t *out_len)
> +{
> +	struct xe_pcode_fwctl_dev *fwctl_dev =
> +		container_of(uctx->fwctl, struct xe_pcode_fwctl_dev, fwctl);
> +	struct xe_tile *root_tile = xe_device_get_root_tile(fwctl_dev->xe);
> +	struct fwctl_rpc_xe_pcode *rpc = in;
> +	int err;
> +
> +	if (in_len != sizeof(struct fwctl_rpc_xe_pcode) ||
> +	    *out_len != sizeof(struct fwctl_rpc_xe_pcode))
> +		return ERR_PTR(-EMSGSIZE);
> +
> +	if (!xe_pcode_fwctl_rpc_validate(rpc, scope))
> +		return ERR_PTR(-EBADMSG);

There should be an EPERM here if the scope is not good enough..
> +/**
> + * struct fwctl_rpc_xe_pcode - FWCTL Remote Procedure Calls for Xe PCODE
> + */
> +struct fwctl_rpc_xe_pcode {
> +	/** @command: The main Mailbox command */
> +	__u8 command;
> +	/** @param1: A subcommand or a parameter of the main command */
> +	__u16 param1;
> +	/** @param2: A parameter of a subcommand or a subsubcommand */
> +	__u16 param2;
> +	/** @data0: The first 32 bits of data. In general data-in as param */
> +	__u32 data0;
> +	/** @data1: The other 32 bits of data. In general data-out */
> +	__u32 data1;
> +	/** @pad: Padding the uAPI struct - Must be 0. Not sent to firmware */
> +	__u8 pad[3];
> +};

This has implicit padding? Make the padding explicit or use packed..
> +/**
> + * DOC: Late Binding Commands
> + *
> + * FWCTL info.uctx_caps: FWCTL_XE_PCODE_LATEBINDING
> + * FWCTL rpc.scope: FWCTL_RPC_CONFIGURATION
> + *
> + * Command	0x5C - LATE_BINDING
> + * Param1	0x0 - GET_CAPABILITY_STATUS
> + * Param2	0
> + * Data in	None
> + * Data out:
> + *
> + *  - Bit0: ate binding for V1 Fan Tables is supported.

"ate" is a typo?

This seems fine, though very simple in what it can do. Do you imagine
more commands down the road?

Jason