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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, May 06, 2025 at 03:13:53PM -0300, Jason Gunthorpe wrote:

Hi Jason,

first of all, thank you so much for creating this and for the review here.

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

I was really in doubt about this and decided to go more towards the CXL way,
but if there's a stronger preference it would be okay by me if it is okay by
Sima and Dave as well.

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

In general I prefer the runtime on the outer bounds to avoid funny deadlock
cases. But right, in this case here it could be inside the xe_pcode calls
itself, that is when the mmio/mailboxes accesses will really happen.

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

EPERM seems better indeed

> > +/**
> > + * 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?

yes, for 'Late'

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

Indeed, and this first RFC was to test the waters and the overall acceptance,
reception. PCODE has more mailboxes that we might start using FWCTL more
instead of growing per platform spread sysfs files.

One last thing since I have your attention here. Was any time in the previous
fwctl discussions talked about the possibility of some extra usages for like
FW flashing or in-field-repair/tests where big data needs to filled bypassing
lockdown mode?

> 
> Jason