Re: [PATCH v9 3/6] drm/xe/sysctrl: Add mailbox communication headers

[Riana Tauro <riana.tauro@intel.com>]

Hi Anoop

On 3/10/2026 11:53 PM, Anoop, Vijay wrote:
> From: Anoop Vijay <anoop.c.vijay@intel.com>
> 
> Add ABI definitions, mailbox API, and command structures for
> System Controller communication.
> 
> No functional code yet. Only protocol layer definitions are added.
> 
> Signed-off-by: Anoop Vijay <anoop.c.vijay@intel.com>
> ---
> v8: (Matt, Michal)
> - Reordered patches for logical flow
> - Moved ABI definitions to dedicated header
> 
> v9: (Badal)
> - Renamed MKHI to SCHI (System Controller Host Interface)
> ---
> 
>   drivers/gpu/drm/xe/abi/xe_sysctrl_abi.h       | 31 ++++++++++++++++
>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.h       | 31 ++++++++++++++++
>   drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h | 35 +++++++++++++++++++
>   3 files changed, 97 insertions(+)
>   create mode 100644 drivers/gpu/drm/xe/abi/xe_sysctrl_abi.h
>   create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>   create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> 
> diff --git a/drivers/gpu/drm/xe/abi/xe_sysctrl_abi.h b/drivers/gpu/drm/xe/abi/xe_sysctrl_abi.h
> new file mode 100644
> index 000000000000..bc4793fb21e5
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/abi/xe_sysctrl_abi.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_SYSCTRL_ABI_H_
> +#define _XE_SYSCTRL_ABI_H_
> +
> +#include <linux/types.h>
> +

Add documentation. Anyone not seeing the version history will not know 
what SCHI is.

What is the reason of using SCHI? why not just mailbox_msg_hdr?

> +struct xe_sysctrl_mailbox_schi_msg_hdr {
> +	__le32 data;
> +} __packed;
> +

I don't see this getting used outside sysctrl file. Will there be any
users for this else move it to .c

> +struct xe_sysctrl_app_msg_hdr {
> +	__le32 data;
> +} __packed;
> +

This should be u32. Any conversions should be done in sc_send_command
not by upper layer.

> +#define SCHI_HDR_GROUP_ID_MASK		GENMASK(7, 0)
> +#define SCHI_HDR_COMMAND_MASK		GENMASK(14, 8)
> +#define SCHI_HDR_COMMAND_MAX		0x7f
> +#define SCHI_HDR_IS_RESPONSE		BIT(15)
> +#define SCHI_HDR_RESERVED_MASK		GENMASK(23, 16)
> +#define SCHI_HDR_RESULT_MASK		GENMASK(31, 24)
> +
> +#define APP_HDR_GROUP_ID_MASK		GENMASK(7, 0)
> +#define APP_HDR_COMMAND_MASK		GENMASK(15, 8)
> +#define APP_HDR_VERSION_MASK		GENMASK(23, 16)
> +#define APP_HDR_RESERVED_MASK		GENMASK(31, 24)
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> new file mode 100644
> index 000000000000..91460be9e22c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_SYSCTRL_MAILBOX_H_
> +#define _XE_SYSCTRL_MAILBOX_H_
> +
> +#include <linux/bitfield.h>
> +#include <linux/types.h>
> +
> +#include "abi/xe_sysctrl_abi.h"
> +
> +struct xe_sysctrl;
> +struct xe_sysctrl_mailbox_command;
> +
> +#define XE_SYSCTRL_APP_HDR_GROUP_ID(hdr) \
> +	FIELD_GET(APP_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
> +
> +#define XE_SYSCTRL_APP_HDR_COMMAND(hdr) \
> +	FIELD_GET(APP_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
> +
> +#define XE_SYSCTRL_APP_HDR_VERSION(hdr) \
> +	FIELD_GET(APP_HDR_VERSION_MASK, le32_to_cpu((hdr)->data))
> +
> +void xe_sysctrl_mailbox_init(struct xe_sysctrl *sc);
> +int xe_sysctrl_send_command(struct xe_sysctrl *sc,
> +			    struct xe_sysctrl_mailbox_command *cmd,
> +			    size_t *rdata_len);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> new file mode 100644
> index 000000000000..fdf8d1d4e3cd
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_SYSCTRL_MAILBOX_TYPES_H_
> +#define _XE_SYSCTRL_MAILBOX_TYPES_H_
> +
> +#include <linux/types.h>
> +
> +#include "abi/xe_sysctrl_abi.h"
> +
> +/**
> + * struct xe_sysctrl_mailbox_command - System Controller mailbox command
> + */
> +struct xe_sysctrl_mailbox_command {
> +	/** @header: Application message header containing command information */
> +	struct xe_sysctrl_app_msg_hdr header;
> +	/** @data_in: Pointer to input payload data (can be NULL if no input data) */
> +	void *data_in;
> +	/** @data_in_len: Size of input payload in bytes (0 if no input data) */
> +	size_t data_in_len;
> +	/** @data_out: Pointer to output buffer for response data (can be NULL if no response) */
> +	void *data_out;
> +	/** @data_out_len: Size of output buffer in bytes (0 if no response expected) */
> +	size_t data_out_len;
> +};
> +
> +#define XE_SYSCTRL_MB_FRAME_SIZE	16
> +#define XE_SYSCTRL_MB_MAX_FRAMES	64
> +#define XE_SYSCTRL_MB_MAX_MESSAGE_SIZE	(XE_SYSCTRL_MB_FRAME_SIZE * XE_SYSCTRL_MB_MAX_FRAMES)

Maybe use another tab and keep the macros aligned
Also if these are unused by other layers, define it in .c

Thanks
Riana

> +
> +#define XE_SYSCTRL_MB_DEFAULT_TIMEOUT_MS	500
> +
> +#endif