Re: [PATCH V3 03/11] accel/amdxdna: Support hardware mailbox

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 10/6/2024 10:15 PM, Lizhi Hou wrote:
> 
> On 10/4/24 10:34, Jeffrey Hugo wrote:
>> On 9/11/2024 12:05 PM, Lizhi Hou wrote:
>>> +struct create_ctx_req {
>>> +    __u32    aie_type;
>>> +    __u8    start_col;
>>> +    __u8    num_col;
>>> +    __u16    reserved;
>>> +    __u8    num_cq_pairs_requested;
>>> +    __u8    reserved1;
>>> +    __u16    pasid;
>>> +    __u32    pad[2];
>>> +    __u32    sec_comm_target_type;
>>> +    __u32     context_priority;
>>
>> Alignment
> Will fix it.
>>
>>> +} __packed;
>>> +
>>> +struct create_ctx_resp {
>>> +    enum aie2_msg_status    status;
>>> +    __u32            context_id;
>>> +    __u16            msix_id;
>>> +    __u8            num_cq_pairs_allocated;
>>> +    __u8            reserved;
>>> +    struct cq_pair        cq_pair[MAX_CQ_PAIRS];
>>> +} __packed;
>>> +
>>> +struct destroy_ctx_req {
>>> +    __u32    context_id;
>>> +} __packed;
>>> +
>>> +struct destroy_ctx_resp {
>>> +    enum aie2_msg_status    status;
>>> +} __packed;
>>> +
>>> +struct execute_buffer_req {
>>> +    __u32    cu_idx;
>>> +    __u32    payload[19];
>>> +} __packed;
>>> +
>>> +struct exec_dpu_req {
>>> +    __u64    inst_buf_addr;
>>> +    __u32     inst_size;
>>> +    __u32     inst_prop_cnt;
>>> +    __u32     cu_idx;
>>
>> Alignment
> will fix it.
>>
>>> +    __u32    payload[35];
>>> +} __packed;
>>> +
>>> diff --git a/drivers/accel/amdxdna/aie2_psp.c 
>>> b/drivers/accel/amdxdna/aie2_psp.c
>>> index c87ca322e206..ac5296f4f2ae 100644
>>> --- a/drivers/accel/amdxdna/aie2_psp.c
>>> +++ b/drivers/accel/amdxdna/aie2_psp.c
>>> @@ -8,6 +8,8 @@
>>>   #include <drm/drm_print.h>
>>>   #include <linux/iopoll.h>
>>>   +#include "amdxdna_mailbox.h"
>>> +#include "amdxdna_pci_drv.h"
>>>   #include "aie2_pci.h"
>>
>> Doesn't look like alphabetical order to me.  Also similar instances in 
>> the *_regs.c files below
> 
> aie2 is one type of amdxdna hw platform. I would put amdxdna_*.h ahead 
> of aie_*.h. Otherwise I probably need to add include in aie2_*.h.  Is it 
> acceptable reason? Or I must use alphabetical for #include?

Sorting the includes is part of the Linux coding style, and the current 
style document mentions using clang-format for ordering them (in 
alphabetical order).  Requesting ordered includes is common feedback per 
Lore.

Relying on include ordering seems very fragile.

-Jeff