On 08.06.2021 03:23, Daniele Ceraolo Spurio wrote: > > > On 6/7/2021 11:03 AM, Matthew Brost wrote: >> From: Michal Wajdeczko <michal.wajdec...@intel.com> >> >> Definition of the CTB registration action has changed. >> Add some ABI documentation and implement required changes. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdec...@intel.com> >> Signed-off-by: Matthew Brost <matthew.br...@intel.com> >> Cc: Piotr Piórkowski <piotr.piorkow...@intel.com> #4 >> --- >> .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 107 ++++++++++++++++++ >> .../gt/uc/abi/guc_communication_ctb_abi.h | 4 - >> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 76 ++++++++----- >> 3 files changed, 152 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h >> b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h >> index 90efef8a73e4..6426fc183692 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h >> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h >> @@ -6,6 +6,113 @@ >> #ifndef _ABI_GUC_ACTIONS_ABI_H >> #define _ABI_GUC_ACTIONS_ABI_H >> +/** >> + * DOC: HOST2GUC_REGISTER_CTB >> + * >> + * This message is used as part of the `CTB based communication`_ setup. >> + * >> + * This message must be sent as `MMIO HXG Message`_. >> + * >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | | Bits | >> Description | >> + * >> +===+=======+==============================================================+ >> >> + * | 0 | 31 | ORIGIN = >> GUC_HXG_ORIGIN_HOST_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 30:28 | TYPE = >> GUC_HXG_TYPE_REQUEST_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 27:16 | DATA0 = >> MBZ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 15:0 | ACTION = _`GUC_ACTION_HOST2GUC_REGISTER_CTB` = >> 0x5200 | > > Specs says 4505
but draft was saying 5200 ;) > >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | 1 | 31:12 | RESERVED = >> MBZ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 11:8 | **TYPE** - type for the `CT >> Buffer`_ | >> + * | | >> | | >> + * | | | - _`GUC_CTB_TYPE_HOST2GUC` = >> 0 | >> + * | | | - _`GUC_CTB_TYPE_GUC2HOST` = >> 1 | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 7:0 | **SIZE** - size of the `CT Buffer`_ in 4K units >> minus 1 | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | 2 | 31:0 | **DESC_ADDR** - GGTT address of the `CTB >> Descriptor`_ | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | 3 | 31:0 | **BUFF_ADDF** - GGTT address of the `CT >> Buffer`_ | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> +* >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | | Bits | >> Description | >> + * >> +===+=======+==============================================================+ >> >> + * | 0 | 31 | ORIGIN = >> GUC_HXG_ORIGIN_GUC_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 30:28 | TYPE = >> GUC_HXG_TYPE_RESPONSE_SUCCESS_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 27:0 | DATA0 = >> MBZ | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + */ >> +#define GUC_ACTION_HOST2GUC_REGISTER_CTB 0x4505 // FIXME 0x5200 > > Why FIXME? AFAICS the specs still says 4505, even if we plan to update > at some point I don;t think this deserves a FIXME since nothing is > incorrect. patch was prepared based on draft spec and this FIXME was added just as head-up since we were expecting GuC to make this change soon, but since we are going with GuC 62 that uses 4505, agree, we need drop this FIXME > >> + >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN >> (GUC_HXG_REQUEST_MSG_MIN_LEN + 3u) >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_0_MBZ >> GUC_HXG_REQUEST_MSG_0_DATA0 >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_MBZ (0xfffff << 12) >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE (0xf << 8) >> +#define GUC_CTB_TYPE_HOST2GUC 0u >> +#define GUC_CTB_TYPE_GUC2HOST 1u >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE (0xff << 0) >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR >> GUC_HXG_REQUEST_MSG_n_DATAn >> +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR >> GUC_HXG_REQUEST_MSG_n_DATAn > > The full mask still seems like overkill to me and I still think we > should use BIT()/GENMASK() and a _MASK prefix, but not going to block on > it. > >> + >> +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_LEN >> GUC_HXG_RESPONSE_MSG_MIN_LEN >> +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_0_MBZ >> GUC_HXG_RESPONSE_MSG_0_DATA0 >> + >> +/** >> + * DOC: HOST2GUC_DEREGISTER_CTB >> + * >> + * This message is used as part of the `CTB based communication`_ >> teardown. >> + * >> + * This message must be sent as `MMIO HXG Message`_. >> + * >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | | Bits | >> Description | >> + * >> +===+=======+==============================================================+ >> >> + * | 0 | 31 | ORIGIN = >> GUC_HXG_ORIGIN_HOST_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 30:28 | TYPE = >> GUC_HXG_TYPE_REQUEST_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 27:16 | DATA0 = >> MBZ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 15:0 | ACTION = _`GUC_ACTION_HOST2GUC_DEREGISTER_CTB` = >> 0x5201 | > > Specs says 4506 > >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | 1 | 31:12 | RESERVED = >> MBZ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 11:8 | **TYPE** - type of the `CT >> Buffer`_ | >> + * | | >> | | >> + * | | | see >> `GUC_ACTION_HOST2GUC_REGISTER_CTB`_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 7:0 | RESERVED = >> MBZ | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> +* >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + * | | Bits | >> Description | >> + * >> +===+=======+==============================================================+ >> >> + * | 0 | 31 | ORIGIN = >> GUC_HXG_ORIGIN_GUC_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 30:28 | TYPE = >> GUC_HXG_TYPE_RESPONSE_SUCCESS_ | >> + * | >> +-------+--------------------------------------------------------------+ >> + * | | 27:0 | DATA0 = >> MBZ | >> + * >> +---+-------+--------------------------------------------------------------+ >> >> + */ >> +#define GUC_ACTION_HOST2GUC_DEREGISTER_CTB 0x4506 // FIXME 0x5201 > > Same comment for the FIXME as above > >> + >> +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN >> (GUC_HXG_REQUEST_MSG_MIN_LEN + 1u) >> +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_0_MBZ >> GUC_HXG_REQUEST_MSG_0_DATA0 >> +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ (0xfffff << 12) >> +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE (0xf << 8) >> +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ2 (0xff << 0) >> + >> +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_LEN >> GUC_HXG_RESPONSE_MSG_MIN_LEN >> +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_0_MBZ >> GUC_HXG_RESPONSE_MSG_0_DATA0 >> + >> +/* legacy definitions */ >> + >> enum intel_guc_action { >> INTEL_GUC_ACTION_DEFAULT = 0x0, >> INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2, >> diff --git >> a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h >> b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h >> index c2a069a78e01..127b256a662c 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h >> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h >> @@ -112,10 +112,6 @@ static_assert(sizeof(struct guc_ct_buffer_desc) >> == 64); >> * - **flags**, holds various bits to control message handling >> */ >> -/* Type of command transport buffer */ >> -#define INTEL_GUC_CT_BUFFER_TYPE_SEND 0x0u >> -#define INTEL_GUC_CT_BUFFER_TYPE_RECV 0x1u >> - >> /* >> * Definition of the command transport message header (DW0) >> * >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> index 3241a477196f..6a29be779cc9 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> @@ -103,9 +103,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct) >> static inline const char *guc_ct_buffer_type_to_str(u32 type) >> { >> switch (type) { >> - case INTEL_GUC_CT_BUFFER_TYPE_SEND: >> + case GUC_CTB_TYPE_HOST2GUC: >> return "SEND"; >> - case INTEL_GUC_CT_BUFFER_TYPE_RECV: >> + case GUC_CTB_TYPE_GUC2HOST: >> return "RECV"; >> default: >> return "<invalid>"; >> @@ -136,25 +136,33 @@ static void guc_ct_buffer_init(struct >> intel_guc_ct_buffer *ctb, >> guc_ct_buffer_reset(ctb); >> } >> -static int guc_action_register_ct_buffer(struct intel_guc *guc, >> - u32 desc_addr, >> - u32 type) >> +static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 >> type, >> + u32 desc_addr, u32 buff_addr, u32 size) >> { >> - u32 action[] = { >> - INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER, >> - desc_addr, >> - sizeof(struct guc_ct_buffer_desc), >> - type >> + u32 request[HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN] = { >> + FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | >> + FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | >> + FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, >> GUC_ACTION_HOST2GUC_REGISTER_CTB), > > IMO we could use a macro or 2 for the HXG header, to avoid all these > lines, which are hard to read. something like: > > GUC_HXG_HEADER(origin, type, data, action) \ > (FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, origin) | \ > FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) | \ > FIELD_PREP(GUC_HXG_MSG_0_DATA0, data) | \ > FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, action)) > > H2G_HEADER(type, data, action) \ > GUC_HXG_HEADER(GUC_HXG_ORIGIN_HOST, type, data, action) > > and then call > > H2G_HEADER(GUC_HXG_TYPE_REQUEST, 0, GUC_ACTION_HOST2GUC_REGISTER_CTB) note that each message type defines its own bits, so helpers macros will likely be per type, but still doable (as the future improvement) #define H2G_REQUEST(action, data) ... \ GUC_ACTION_##action H2G_REQUEST(HOST2GUC_REGISTER_CTB, 0) > > > Not a blocker. > > Daniele > >> + FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE, size / >> SZ_4K - 1) | >> + FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE, type), >> + FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, >> desc_addr), >> + FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, >> buff_addr), >> }; >> - /* Can't use generic send(), CT registration must go over MMIO */ >> - return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, >> 0); >> + GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != >> GUC_CTB_TYPE_GUC2HOST); >> + GEM_BUG_ON(size % SZ_4K); >> + >> + /* CT registration must go over MMIO */ >> + return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), >> NULL, 0); >> } >> -static int ct_register_buffer(struct intel_guc_ct *ct, u32 >> desc_addr, u32 type) >> +static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, >> + u32 desc_addr, u32 buff_addr, u32 size) >> { >> - int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, >> type); >> + int err; >> + err = guc_action_register_ct_buffer(ct_to_guc(ct), type, >> + desc_addr, buff_addr, size); >> if (unlikely(err)) >> CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n", >> guc_ct_buffer_type_to_str(type), err); >> @@ -163,14 +171,17 @@ static int ct_register_buffer(struct >> intel_guc_ct *ct, u32 desc_addr, u32 type) >> static int guc_action_deregister_ct_buffer(struct intel_guc *guc, >> u32 type) >> { >> - u32 action[] = { >> - INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER, >> - CTB_OWNER_HOST, >> - type >> + u32 request[HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN] = { >> + FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | >> + FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | >> + FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, >> GUC_ACTION_HOST2GUC_DEREGISTER_CTB), >> + FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type), >> }; >> - /* Can't use generic send(), CT deregistration must go over >> MMIO */ >> - return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, >> 0); >> + GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != >> GUC_CTB_TYPE_GUC2HOST); >> + >> + /* CT deregistration must go over MMIO */ >> + return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), >> NULL, 0); >> } >> static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) >> @@ -258,7 +269,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct) >> int intel_guc_ct_enable(struct intel_guc_ct *ct) >> { >> struct intel_guc *guc = ct_to_guc(ct); >> - u32 base, cmds; >> + u32 base, desc, cmds; >> void *blob; >> int err; >> @@ -274,23 +285,26 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) >> GEM_BUG_ON(blob != ct->ctbs.send.desc); >> /* (re)initialize descriptors */ >> - cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); >> guc_ct_buffer_reset(&ct->ctbs.send); >> - >> - cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); >> guc_ct_buffer_reset(&ct->ctbs.recv); >> /* >> * Register both CT buffers starting with RECV buffer. >> * Descriptors are in first half of the blob. >> */ >> - err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, >> blob), >> - INTEL_GUC_CT_BUFFER_TYPE_RECV); >> + desc = base + ptrdiff(ct->ctbs.recv.desc, blob); >> + cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); >> + err = ct_register_buffer(ct, GUC_CTB_TYPE_GUC2HOST, >> + desc, cmds, ct->ctbs.recv.size * 4); >> + >> if (unlikely(err)) >> goto err_out; >> - err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, >> blob), >> - INTEL_GUC_CT_BUFFER_TYPE_SEND); >> + desc = base + ptrdiff(ct->ctbs.send.desc, blob); >> + cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); >> + err = ct_register_buffer(ct, GUC_CTB_TYPE_HOST2GUC, >> + desc, cmds, ct->ctbs.send.size * 4); >> + >> if (unlikely(err)) >> goto err_deregister; >> @@ -299,7 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) >> return 0; >> err_deregister: >> - ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); >> + ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST); >> err_out: >> CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err)); >> return err; >> @@ -318,8 +332,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct) >> ct->enabled = false; >> if (intel_guc_is_fw_running(guc)) { >> - ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND); >> - ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); >> + ct_deregister_buffer(ct, GUC_CTB_TYPE_HOST2GUC); >> + ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST); >> } >> } >> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx