Re: [PATCH 1/5] drm/xe/guc: Add GuC Relay ABI version 1.0 definitions

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Piotr Piórkowski <piotr.piorkowski@intel.com> wrote on pon [2024-kwi-22 11:25:37 +0200]:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on czw [2024-kwi-18 17:27:58 +0200]:
> > This initial GuC Relay ABI specification includes messages for ABI
> > version negotiation and to query values of runtime/fuse registers.
> > 
> > We will start handling those messages on the PF driver soon.
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >  .../gpu/drm/xe/abi/guc_relay_actions_abi.h    | 147 +++++++++++++++++-
> >  1 file changed, 146 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/abi/guc_relay_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_relay_actions_abi.h
> > index 747e428de421..05a2066ef7f7 100644
> > --- a/drivers/gpu/drm/xe/abi/guc_relay_actions_abi.h
> > +++ b/drivers/gpu/drm/xe/abi/guc_relay_actions_abi.h
> > @@ -1,11 +1,156 @@
> >  /* SPDX-License-Identifier: MIT */
> >  /*
> > - * Copyright © 2023 Intel Corporation
> > + * Copyright © 2023-2024 Intel Corporation
> >   */
> >  
> >  #ifndef _ABI_GUC_RELAY_ACTIONS_ABI_H_
> >  #define _ABI_GUC_RELAY_ACTIONS_ABI_H_
> >  
> > +#include "abi/guc_relay_communication_abi.h"
> > +
> > +/**
> > + * DOC: GuC Relay VF/PF ABI Version
> > + *
> > + * The _`GUC_RELAY_VERSION_BASE` defines minimum VF/PF ABI version that
> > + * drivers must support. Currently this is version 1.0.
> > + *
> > + * The _`GUC_RELAY_VERSION_LATEST` defines latest VF/PF ABI version that
> > + * drivers may use. Currently this is version 1.0.
> > + *
> > + * Some platforms may require different base VF/PF ABI version.
> > + * No supported VF/PF ABI version can be 0.0.
> > + */
> > +
> > +#define GUC_RELAY_VERSION_BASE_MAJOR			1
> > +#define GUC_RELAY_VERSION_BASE_MINOR			0
> > +
> > +#define GUC_RELAY_VERSION_LATEST_MAJOR			1
> > +#define GUC_RELAY_VERSION_LATEST_MINOR			0
> > +
> > +/**
> > + * DOC: GuC Relay Actions
> > + *
> > + * The following actions are supported from VF/PF ABI version 1.0:
> > + *
> > + *  * `VF2PF_HANDSHAKE`_
> > + *  * `VF2PF_QUERY_RUNTIME`_
> > + */
> > +
> > +/**
> > + * DOC: VF2PF_HANDSHAKE
> > + *
> > + * This `Relay Message`_ is used by the VF to establish ABI version with the PF.
> > + *
> > + * This message definition shall be supported by all future ABI versions.
> > + * This message definition shall not be changed by future ABI versions.
> > + *
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |   | Bits  | Description                                                  |
> > + *  +===+=======+==============================================================+
> > + *  | 0 |    31 | ORIGIN = GUC_HXG_ORIGIN_HOST_                                |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 30:28 | TYPE = GUC_HXG_TYPE_REQUEST_                                 |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 27:16 | DATA0 = MBZ                                                  |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  15:0 | ACTION = _`GUC_RELAY_ACTION_VF2PF_HANDSHAKE` = 0x0001        |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 1 | 31:16 | **MAJOR** - requested major version of the VFPF interface    |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  15:0 | **MINOR** - requested minor version of the VFPF interface    |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |   | Bits  | Description                                                  |
> > + *  +===+=======+==============================================================+
> > + *  | 0 |    31 | ORIGIN = GUC_HXG_ORIGIN_HOST_                                |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  27:0 | DATA0 = MBZ                                                  |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 1 | 31:16 | **MAJOR** - agreed major version of the VFPF interface       |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  15:0 | **MINOR** - agreed minor version of the VFPF interface       |
> > + *  +---+-------+--------------------------------------------------------------+
> > + */
> > +#define GUC_RELAY_ACTION_VF2PF_HANDSHAKE		0x0001u
> > +
> > +#define VF2PF_HANDSHAKE_REQUEST_MSG_LEN			2u
> > +#define VF2PF_HANDSHAKE_REQUEST_MSG_0_MBZ		GUC_HXG_REQUEST_MSG_0_DATA0
> > +#define VF2PF_HANDSHAKE_REQUEST_MSG_1_MAJOR		(0xffffu << 16)
> > +#define   VF2PF_HANDSHAKE_MAJOR_ANY			0
> > +#define VF2PF_HANDSHAKE_REQUEST_MSG_1_MINOR		(0xffffu << 0)
> > +#define   VF2PF_HANDSHAKE_MINOR_ANY			0
> 
> NIT: Maybe you should add a description of MAJOR_ANY and MINOR_ANY to the documentation
> of this action.
> 
> > +
> > +#define VF2PF_HANDSHAKE_RESPONSE_MSG_LEN		2u
> > +#define VF2PF_HANDSHAKE_RESPONSE_MSG_0_MBZ		GUC_HXG_RESPONSE_MSG_0_DATA0
> > +#define VF2PF_HANDSHAKE_RESPONSE_MSG_1_MAJOR		(0xffffu << 16)
> > +#define VF2PF_HANDSHAKE_RESPONSE_MSG_1_MINOR		(0xffffu << 0)
> > +
> > +/**
> > + * DOC: VF2PF_QUERY_RUNTIME
> > + *
> > + * This `Relay Message`_ is used by the VF to query values of runtime registers.
> > + *
> 
> NIT: I would describe more what runtime registers are (as a struture itself)
> Because here you have a parameter "START" whose description indicates that it is some
> kind of index. But someone who has the first contact with this has to check the code
> to find out what this index is.
> You could describe that runtime registers is a predefined list of registers depending
> on the platform, and the index indicates the positions in the list.

In addition, from the implementation in https://patchwork.freedesktop.org/patch/590250/?series=132612&rev=2
it looks like 0 is an allowed value and doesn't mean we don't want any registers...
also worth adding here

Piotr
> 
> 
> > + * This message definition is supported from ABI versions 1.0.
> > + *
> > + * VF provides @START index to the requested register entry.
> > + * VF can use @LIMIT to limit number of returned register entries.
> > + *
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |   | Bits  | Description                                                  |
> > + *  +===+=======+==============================================================+
> > + *  | 0 |    31 | ORIGIN = GUC_HXG_ORIGIN_HOST_                                |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 30:28 | TYPE = GUC_HXG_TYPE_REQUEST_                                 |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 27:16 | DATA0 = **LIMIT** - limit number of returned entries         |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  15:0 | ACTION = _`GUC_RELAY_ACTION_VF2PF_QUERY_RUNTIME` = 0x0101    |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 1 |  31:0 | DATA1 = **START** - index of the first requested entry       |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |   | Bits  | Description                                                  |
> > + *  +===+=======+==============================================================+
> > + *  | 0 |    31 | ORIGIN = GUC_HXG_ORIGIN_HOST_                                |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |
> > + *  |   +-------+--------------------------------------------------------------+
> > + *  |   |  27:0 | DATA0 = **COUNT** - number of entries included in response   |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 1 |  31:0 | DATA1 = **REMAINING** - number of remaining entries          |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 2 |  31:0 | DATA2 = **REG_OFFSET** - offset of register[START]           |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | 3 |  31:0 | DATA3 = **REG_VALUE** - value of register[START]             |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |   |       |                                                              |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  |n-1|  31:0 | REG_OFFSET - offset of register[START + x]                   |
> > + *  +---+-------+--------------------------------------------------------------+
> > + *  | n |  31:0 | REG_VALUE - value of register[START + x]                     |
> > + *  +---+-------+--------------------------------------------------------------+
> > + */
> > +#define GUC_RELAY_ACTION_VF2PF_QUERY_RUNTIME		0x0101u
> > +
> > +#define VF2PF_QUERY_RUNTIME_REQUEST_MSG_LEN		2u
> > +#define VF2PF_QUERY_RUNTIME_REQUEST_MSG_0_LIMIT		GUC_HXG_REQUEST_MSG_0_DATA0
> > +#define VF2PF_QUERY_RUNTIME_REQUEST_MSG_1_START		GUC_HXG_REQUEST_MSG_n_DATAn
> > +
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_MSG_MIN_LEN	(GUC_HXG_MSG_MIN_LEN + 1u)
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_MSG_MAX_LEN	\
> > +		(VF2PF_QUERY_RUNTIME_RESPONSE_MSG_MIN_LEN + VF2PF_QUERY_RUNTIME_MAX_COUNT * 2)
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_MSG_0_COUNT	GUC_HXG_RESPONSE_MSG_0_DATA0
> > +#define   VF2PF_QUERY_RUNTIME_MIN_COUNT			0
> > +#define   VF2PF_QUERY_RUNTIME_MAX_COUNT			\
> > +		((GUC_RELAY_MSG_MAX_LEN - VF2PF_QUERY_RUNTIME_RESPONSE_MSG_MIN_LEN) / 2)
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_MSG_1_REMAINING	GUC_HXG_RESPONSE_MSG_n_DATAn
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_DATAn_REG_OFFSETx	GUC_HXG_RESPONSE_MSG_n_DATAn
> > +#define VF2PF_QUERY_RUNTIME_RESPONSE_DATAn_REG_VALUEx	GUC_HXG_RESPONSE_MSG_n_DATAn
> > +
> >  /**
> >   * DOC: GuC Relay Debug Actions
> >   *
> 
> 
> Personally, I would expand the action descriptions a bit ( comments
> above), but still:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> > -- 
> > 2.43.0
> > 
> 
> -- 

--