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
