Re: [PATCH 07/13] drm/i915/guc: New definition of the CTB registration action

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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);
>>       }
>>   }
>>   
> 



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux