Re: [PATCH 06/13] drm/i915/guc: New definition of the CTB descriptor

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

 




On 08.06.2021 02:59, Daniele Ceraolo Spurio wrote:
> 
> 
> On 6/7/2021 11:03 AM, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>>
>> Definition of the CTB descriptor has changed, leaving only
>> minimal shared fields like HEAD/TAIL/STATUS.
>>
>> Both HEAD and TAIL are now in dwords.
>>
>> Add some ABI documentation and implement required changes.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
>> ---
>>   .../gt/uc/abi/guc_communication_ctb_abi.h     | 70 ++++++++++++++-----
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 70 +++++++++----------
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h     |  2 +-
>>   3 files changed, 85 insertions(+), 57 deletions(-)
>>
>> 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 d38935f47ecf..c2a069a78e01 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
>> @@ -7,6 +7,58 @@
>>   #define _ABI_GUC_COMMUNICATION_CTB_ABI_H
>>     #include <linux/types.h>
>> +#include <linux/build_bug.h>
>> +
>> +#include "guc_messages_abi.h"
>> +
>> +/**
>> + * DOC: CT Buffer
>> + *
>> + * TBD
> 
> What's the plan with this TBD here?

Plan was to add some updated text based on old "DOC: CTB based
communication" section

> 
>> + */
>> +
>> +/**
>> + * DOC: CTB Descriptor
>> + *
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + *  |   | Bits  |
>> Description                                                  |
>> + * 
>> +===+=======+==============================================================+
>>
>> + *  | 0 |  31:0 | **HEAD** - offset (in dwords) to the last dword
>> that was     |
>> + *  |   |       | read from the `CT
>> Buffer`_.                                  |
>> + *  |   |       | It can only be updated by the
>> receiver.                      |
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + *  | 1 |  31:0 | **TAIL** - offset (in dwords) to the last dword
>> that was     |
>> + *  |   |       | written to the `CT
>> Buffer`_.                                 |
>> + *  |   |       | It can only be updated by the
>> sender.                        |
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + *  | 2 |  31:0 | **STATUS** - status of the
>> CTB                               |
>> + *  |   |      
>> |                                                              |
>> + *  |   |       |   - _`GUC_CTB_STATUS_NO_ERROR` = 0 (normal
>> operation)        |
>> + *  |   |       |   - _`GUC_CTB_STATUS_OVERFLOW` = 1 (head/tail too
>> large)     |
>> + *  |   |       |   - _`GUC_CTB_STATUS_UNDERFLOW` = 2 (truncated
>> message)      |
>> + *  |   |       |   - _`GUC_CTB_STATUS_MISMATCH` = 4 (head/tail
>> modified)      |
>> + *  |   |       |   - _`GUC_CTB_STATUS_NO_BACKCHANNEL` =
>> 8                     |
>> + *  |   |       |   - _`GUC_CTB_STATUS_MALFORMED_MSG` =
>> 16                     |
> 
> I don't see the last 2 error (8 & 16) in the 62.0.0 specs. Where is the
> reference for them?

both were discussed on various meetings but likely didn't make into
final spec 62, so for now we can drop them both

> 
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + *  |...|       | RESERVED =
>> MBZ                                               |
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + *  | 15|  31:0 | RESERVED =
>> MBZ                                               |
>> + * 
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +struct guc_ct_buffer_desc {
>> +    u32 head;
>> +    u32 tail;
>> +    u32 status;
>> +#define GUC_CTB_STATUS_NO_ERROR                0
>> +#define GUC_CTB_STATUS_OVERFLOW                (1 << 0)
>> +#define GUC_CTB_STATUS_UNDERFLOW            (1 << 1)
>> +#define GUC_CTB_STATUS_MISMATCH                (1 << 2)
>> +#define GUC_CTB_STATUS_NO_BACKCHANNEL            (1 << 3)
>> +#define GUC_CTB_STATUS_MALFORMED_MSG            (1 << 4)
> 
> use BIT() ?

as explained before, on ABI headers we didn't want any dependency and
just use plain C

> 
>> +    u32 reserved[13];
>> +} __packed;
>> +static_assert(sizeof(struct guc_ct_buffer_desc) == 64);
>>     /**
>>    * DOC: CTB based communication
>> @@ -60,24 +112,6 @@
>>    * - **flags**, holds various bits to control message handling
>>    */
>>   -/*
>> - * Describes single command transport buffer.
>> - * Used by both guc-master and clients.
>> - */
>> -struct guc_ct_buffer_desc {
>> -    u32 addr;        /* gfx address */
>> -    u64 host_private;    /* host private data */
>> -    u32 size;        /* size in bytes */
>> -    u32 head;        /* offset updated by GuC*/
>> -    u32 tail;        /* offset updated by owner */
>> -    u32 is_in_error;    /* error indicator */
>> -    u32 reserved1;
>> -    u32 reserved2;
>> -    u32 owner;        /* id of the channel owner */
>> -    u32 owner_sub_id;    /* owner-defined field for extra tracking */
>> -    u32 reserved[5];
>> -} __packed;
>> -
>>   /* Type of command transport buffer */
>>   #define INTEL_GUC_CT_BUFFER_TYPE_SEND    0x0u
>>   #define INTEL_GUC_CT_BUFFER_TYPE_RECV    0x1u
>> 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 63056ea0631e..3241a477196f 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> @@ -112,32 +112,28 @@ static inline const char
>> *guc_ct_buffer_type_to_str(u32 type)
>>       }
>>   }
>>   -static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
>> -                    u32 cmds_addr, u32 size)
>> +static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc)
> 
> this function is called from only 1 place and only does a memset now, so
> IMO we can just drop it and inline the memset.

ok, but without enthusiasm ;)

> 
> The logic below matches the specs.
> 
> Daniele
> 
>>   {
>>       memset(desc, 0, sizeof(*desc));
>> -    desc->addr = cmds_addr;
>> -    desc->size = size;
>> -    desc->owner = CTB_OWNER_HOST;
>>   }
>>   -static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb,
>> u32 cmds_addr)
>> +static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb)
>>   {
>>       ctb->broken = false;
>> -    guc_ct_buffer_desc_init(ctb->desc, cmds_addr, ctb->size);
>> +    guc_ct_buffer_desc_init(ctb->desc);
>>   }
>>     static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,
>>                      struct guc_ct_buffer_desc *desc,
>> -                   u32 *cmds, u32 size)
>> +                   u32 *cmds, u32 size_in_bytes)
>>   {
>> -    GEM_BUG_ON(size % 4);
>> +    GEM_BUG_ON(size_in_bytes % 4);
>>         ctb->desc = desc;
>>       ctb->cmds = cmds;
>> -    ctb->size = size;
>> +    ctb->size = size_in_bytes / 4;
>>   -    guc_ct_buffer_reset(ctb, 0);
>> +    guc_ct_buffer_reset(ctb);
>>   }
>>     static int guc_action_register_ct_buffer(struct intel_guc *guc,
>> @@ -279,10 +275,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>>         /* (re)initialize descriptors */
>>       cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
>> -    guc_ct_buffer_reset(&ct->ctbs.send, cmds);
>> +    guc_ct_buffer_reset(&ct->ctbs.send);
>>         cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);
>> -    guc_ct_buffer_reset(&ct->ctbs.recv, cmds);
>> +    guc_ct_buffer_reset(&ct->ctbs.recv);
>>         /*
>>        * Register both CT buffers starting with RECV buffer.
>> @@ -391,17 +387,15 @@ static int ct_write(struct intel_guc_ct *ct,
>>       if (unlikely(ctb->broken))
>>           return -EPIPE;
>>   -    if (unlikely(desc->is_in_error))
>> +    if (unlikely(desc->status))
>>           goto corrupted;
>>   -    if (unlikely(!IS_ALIGNED(head | tail, 4) ||
>> -             (tail | head) >= size))
>> +    if (unlikely((tail | head) >= size)) {
>> +        CT_ERROR(ct, "Invalid offsets head=%u tail=%u (size=%u)\n",
>> +             head, tail, size);
>> +        desc->status |= GUC_CTB_STATUS_OVERFLOW;
>>           goto corrupted;
>> -
>> -    /* later calculations will be done in dwords */
>> -    head /= 4;
>> -    tail /= 4;
>> -    size /= 4;
>> +    }
>>         /*
>>        * tail == head condition indicates empty. GuC FW does not support
>> @@ -447,14 +441,14 @@ static int ct_write(struct intel_guc_ct *ct,
>>        */
>>       write_barrier(ct);
>>   -    /* now update desc tail (back in bytes) */
>> -    desc->tail = tail * 4;
>> +    /* now update descriptor */
>> +    WRITE_ONCE(desc->tail, tail);
>> +
>>       return 0;
>>     corrupted:
>> -    CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u
>> size=%u\n",
>> -         desc->addr, desc->head, desc->tail, desc->size);
>> -    desc->is_in_error = 1;
>> +    CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n",
>> +         desc->head, desc->tail, desc->status);
>>       ctb->broken = true;
>>       return -EPIPE;
>>   }
>> @@ -640,17 +634,15 @@ static int ct_read(struct intel_guc_ct *ct,
>> struct ct_incoming_msg **msg)
>>       if (unlikely(ctb->broken))
>>           return -EPIPE;
>>   -    if (unlikely(desc->is_in_error))
>> +    if (unlikely(desc->status))
>>           goto corrupted;
>>   -    if (unlikely(!IS_ALIGNED(head | tail, 4) ||
>> -             (tail | head) >= size))
>> +    if (unlikely((tail | head) >= size)) {
>> +        CT_ERROR(ct, "Invalid offsets head=%u tail=%u (size=%u)\n",
>> +             head, tail, size);
>> +        desc->status |= GUC_CTB_STATUS_OVERFLOW;
>>           goto corrupted;
>> -
>> -    /* later calculations will be done in dwords */
>> -    head /= 4;
>> -    tail /= 4;
>> -    size /= 4;
>> +    }
>>         /* tail == head condition indicates empty */
>>       available = tail - head;
>> @@ -677,6 +669,7 @@ static int ct_read(struct intel_guc_ct *ct, struct
>> ct_incoming_msg **msg)
>>                     size - head : available - 1), &cmds[head],
>>                4 * (head + available - 1 > size ?
>>                     available - 1 - size + head : 0), &cmds[0]);
>> +        desc->status |= GUC_CTB_STATUS_UNDERFLOW;
>>           goto corrupted;
>>       }
>>   @@ -699,13 +692,14 @@ static int ct_read(struct intel_guc_ct *ct,
>> struct ct_incoming_msg **msg)
>>       }
>>       CT_DEBUG(ct, "received %*ph\n", 4 * len, (*msg)->msg);
>>   -    desc->head = head * 4;
>> +    /* now update descriptor */
>> +    WRITE_ONCE(desc->head, head);
>> +
>>       return available - len;
>>     corrupted:
>> -    CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u
>> size=%u\n",
>> -         desc->addr, desc->head, desc->tail, desc->size);
>> -    desc->is_in_error = 1;
>> +    CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n",
>> +         desc->head, desc->tail, desc->status);
>>       ctb->broken = true;
>>       return -EPIPE;
>>   }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> index 7d3cd375d6a7..905202caaad3 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> @@ -31,7 +31,7 @@ struct intel_guc;
>>    * @lock: protects access to the commands buffer and buffer descriptor
>>    * @desc: pointer to the buffer descriptor
>>    * @cmds: pointer to the commands buffer
>> - * @size: size of the commands buffer
>> + * @size: size of the commands buffer in dwords
>>    * @broken: flag to indicate if descriptor data is broken
>>    */
>>   struct intel_guc_ct_buffer {
> 



[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