On 08.06.2021 03:23, Daniele Ceraolo Spurio wrote:
>
>
> On 6/7/2021 11:03 AM, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>>
>> Definition of the CTB registration action has changed.
>> Add some ABI documentation and implement required changes.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
>> Cc: Piotr Piórkowski <piotr.piorkowski@xxxxxxxxx> #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);
>> }
>> }
>>
>
