From: "Piotr Piórkowski" <piotr.piorkowski@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: <intel-xe@lists.freedesktop.org>
Subject: Re: [PATCH 1/5] drm/xe/guc: Add GuC Relay ABI version 1.0 definitions
Date: Mon, 22 Apr 2024 12:26:18 +0200 [thread overview]
Message-ID: <20240422102618.vtaapag6sskxqju6@intel.com> (raw)
In-Reply-To: <20240422092537.lbe5asu3gofps2ib@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
> >
>
> --
--
next prev parent reply other threads:[~2024-04-22 10:26 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 15:27 [PATCH 0/5] Add SR-IOV GuC Relay PF services Michal Wajdeczko
2024-04-18 15:27 ` [PATCH 1/5] drm/xe/guc: Add GuC Relay ABI version 1.0 definitions Michal Wajdeczko
2024-04-22 9:25 ` Piotr Piórkowski
2024-04-22 10:26 ` Piotr Piórkowski [this message]
2024-04-18 15:27 ` [PATCH 2/5] drm/xe: Add helper to calculate adjusted register offset Michal Wajdeczko
2024-04-22 10:29 ` Piotr Piórkowski
2024-04-18 15:28 ` [PATCH 3/5] drm/xe: Add few more GT register definitions Michal Wajdeczko
2024-04-22 9:38 ` Piotr Piórkowski
2024-04-18 15:28 ` [PATCH 4/5] drm/xe/pf: Add SR-IOV GuC Relay PF services Michal Wajdeczko
2024-04-22 10:48 ` Piotr Piórkowski
2024-04-18 15:28 ` [PATCH 5/5] drm/xe/kunit: Add PF service tests Michal Wajdeczko
2024-04-22 12:01 ` Piotr Piórkowski
2024-04-18 19:48 ` ✓ CI.Patch_applied: success for Add SR-IOV GuC Relay PF services Patchwork
2024-04-18 19:48 ` ✗ CI.checkpatch: warning " Patchwork
2024-04-18 19:49 ` ✓ CI.KUnit: success " Patchwork
2024-04-18 20:01 ` ✓ CI.Build: " Patchwork
2024-04-18 20:03 ` ✗ CI.Hooks: failure " Patchwork
2024-04-18 20:14 ` ✓ CI.checksparse: success " Patchwork
2024-04-18 20:38 ` ✗ CI.BAT: failure " Patchwork
2024-04-20 2:04 ` ✓ CI.Patch_applied: success for Add SR-IOV GuC Relay PF services (rev2) Patchwork
2024-04-20 2:04 ` ✗ CI.checkpatch: warning " Patchwork
2024-04-20 2:05 ` ✓ CI.KUnit: success " Patchwork
2024-04-20 2:23 ` ✓ CI.Build: " Patchwork
2024-04-20 2:27 ` ✗ CI.Hooks: failure " Patchwork
2024-04-20 2:28 ` ✓ CI.checksparse: success " Patchwork
2024-04-20 11:30 ` ✗ CI.FULL: failure for Add SR-IOV GuC Relay PF services Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240422102618.vtaapag6sskxqju6@intel.com \
--to=piotr.piorkowski@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=michal.wajdeczko@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.