Re: [PATCH V1 1/2] accel/amdxdna: Update message DMA buffer allocation

[Lizhi Hou <lizhi.hou@amd.com>]

On 1/7/26 14:07, Mario Limonciello wrote:
> On 1/7/26 4:05 PM, Lizhi Hou wrote:
>>
>> On 1/7/26 13:19, Mario Limonciello wrote:
>>> On 12/18/25 7:43 PM, Lizhi Hou wrote:
>>>> The latest firmware requires the message DMA buffer to
>>>>    - have a minimum size of 8K
>>>>    - use a power-of-two size
>>>>    - be aligned to the buffer size
>>>>    - not cross 64M boundary
>>>>
>>>> Update the buffer allocation logic to meet these requirements and 
>>>> support
>>>> the latest firmware.
>>>
>>> We can't guarantee that kernel and firmware are moving at the same 
>>> time.
>>> What happens if you run old firmware with these changes?
>>
>> The old firmware runs fine this these changes. The new firmware adds 
>> more alignment and size requirements which the old one does not need.
>
> Ah OK - so patch 2 won't reject current firmware right?

Correct. It will not reject current firmware.


Lizhi

>
>>
>>
>> Lizhi
>>
>>>
>>> If the old firmware can't run with these changes then it would be 
>>> better to instead add a fallback system.
>>>
>>> IE:
>>> 1) kernel tries to load new firmware name and use new behavior
>>> 2) if firmware is missing, kernel tries to load old firmware name 
>>> and use old behavior.
>>> 3) if firmware is missing in old name then fail probe
>>>
>>>
>>>>
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>>>> ---
>>>>   drivers/accel/amdxdna/aie2_error.c   | 10 ++++-----
>>>>   drivers/accel/amdxdna/aie2_message.c | 33 
>>>> ++++++++++++++++++++--------
>>>>   drivers/accel/amdxdna/aie2_pci.h     |  5 +++++
>>>>   3 files changed, 33 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/accel/amdxdna/aie2_error.c b/drivers/accel/ 
>>>> amdxdna/aie2_error.c
>>>> index d452008ec4f4..5e82df2b7cf6 100644
>>>> --- a/drivers/accel/amdxdna/aie2_error.c
>>>> +++ b/drivers/accel/amdxdna/aie2_error.c
>>>> @@ -338,8 +338,7 @@ void aie2_error_async_events_free(struct 
>>>> amdxdna_dev_hdl *ndev)
>>>>       destroy_workqueue(events->wq);
>>>>       mutex_lock(&xdna->dev_lock);
>>>>   -    dma_free_noncoherent(xdna->ddev.dev, events->size, events->buf,
>>>> -                 events->addr, DMA_FROM_DEVICE);
>>>> +    aie2_free_msg_buffer(ndev, events->size, events->buf, events- 
>>>> >addr);
>>>>       kfree(events);
>>>>   }
>>>>   @@ -355,8 +354,8 @@ int aie2_error_async_events_alloc(struct 
>>>> amdxdna_dev_hdl *ndev)
>>>>       if (!events)
>>>>           return -ENOMEM;
>>>>   -    events->buf = dma_alloc_noncoherent(xdna->ddev.dev, 
>>>> total_size, &events->addr,
>>>> -                        DMA_FROM_DEVICE, GFP_KERNEL);
>>>> +    events->buf = aie2_alloc_msg_buffer(ndev, &total_size, 
>>>> &events- >addr);
>>>> +
>>>>       if (!events->buf) {
>>>>           ret = -ENOMEM;
>>>>           goto free_events;
>>>> @@ -396,8 +395,7 @@ int aie2_error_async_events_alloc(struct 
>>>> amdxdna_dev_hdl *ndev)
>>>>   free_wq:
>>>>       destroy_workqueue(events->wq);
>>>>   free_buf:
>>>> -    dma_free_noncoherent(xdna->ddev.dev, events->size, events->buf,
>>>> -                 events->addr, DMA_FROM_DEVICE);
>>>> +    aie2_free_msg_buffer(ndev, events->size, events->buf, events- 
>>>> >addr);
>>>>   free_events:
>>>>       kfree(events);
>>>>       return ret;
>>>> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/ 
>>>> amdxdna/aie2_message.c
>>>> index 051f4ceaabae..99215328505e 100644
>>>> --- a/drivers/accel/amdxdna/aie2_message.c
>>>> +++ b/drivers/accel/amdxdna/aie2_message.c
>>>> @@ -55,6 +55,22 @@ static int aie2_send_mgmt_msg_wait(struct 
>>>> amdxdna_dev_hdl *ndev,
>>>>       return ret;
>>>>   }
>>>>   +void *aie2_alloc_msg_buffer(struct amdxdna_dev_hdl *ndev, u32 
>>>> *size,
>>>> +                dma_addr_t *dma_addr)
>>>> +{
>>>> +    struct amdxdna_dev *xdna = ndev->xdna;
>>>> +    int order;
>>>> +
>>>> +    *size = max(*size, SZ_8K);
>>>> +    order = get_order(*size);
>>>> +    if (order > MAX_PAGE_ORDER)
>>>> +        return NULL;
>>>> +    *size = PAGE_SIZE << order;
>>>> +
>>>> +    return dma_alloc_noncoherent(xdna->ddev.dev, *size, dma_addr,
>>>> +                     DMA_FROM_DEVICE, GFP_KERNEL);
>>>> +}
>>>> +
>>>>   int aie2_suspend_fw(struct amdxdna_dev_hdl *ndev)
>>>>   {
>>>>       DECLARE_AIE2_MSG(suspend, MSG_OP_SUSPEND);
>>>> @@ -346,14 +362,13 @@ int aie2_query_status(struct amdxdna_dev_hdl 
>>>> *ndev, char __user *buf,
>>>>   {
>>>>       DECLARE_AIE2_MSG(aie_column_info, MSG_OP_QUERY_COL_STATUS);
>>>>       struct amdxdna_dev *xdna = ndev->xdna;
>>>> +    u32 buf_sz = size, aie_bitmap = 0;
>>>>       struct amdxdna_client *client;
>>>>       dma_addr_t dma_addr;
>>>> -    u32 aie_bitmap = 0;
>>>>       u8 *buff_addr;
>>>>       int ret;
>>>>   -    buff_addr = dma_alloc_noncoherent(xdna->ddev.dev, size, 
>>>> &dma_addr,
>>>> -                      DMA_FROM_DEVICE, GFP_KERNEL);
>>>> +    buff_addr = aie2_alloc_msg_buffer(ndev, &buf_sz, &dma_addr);
>>>>       if (!buff_addr)
>>>>           return -ENOMEM;
>>>>   @@ -363,7 +378,7 @@ int aie2_query_status(struct amdxdna_dev_hdl 
>>>> *ndev, char __user *buf,
>>>>         *cols_filled = 0;
>>>>       req.dump_buff_addr = dma_addr;
>>>> -    req.dump_buff_size = size;
>>>> +    req.dump_buff_size = buf_sz;
>>>>       req.num_cols = hweight32(aie_bitmap);
>>>>       req.aie_bitmap = aie_bitmap;
>>>>   @@ -391,7 +406,7 @@ int aie2_query_status(struct amdxdna_dev_hdl 
>>>> *ndev, char __user *buf,
>>>>       *cols_filled = aie_bitmap;
>>>>     fail:
>>>> -    dma_free_noncoherent(xdna->ddev.dev, size, buff_addr, 
>>>> dma_addr, DMA_FROM_DEVICE);
>>>> +    aie2_free_msg_buffer(ndev, buf_sz, buff_addr, dma_addr);
>>>>       return ret;
>>>>   }
>>>>   @@ -402,19 +417,19 @@ int aie2_query_telemetry(struct 
>>>> amdxdna_dev_hdl *ndev,
>>>>       DECLARE_AIE2_MSG(get_telemetry, MSG_OP_GET_TELEMETRY);
>>>>       struct amdxdna_dev *xdna = ndev->xdna;
>>>>       dma_addr_t dma_addr;
>>>> +    u32 buf_sz = size;
>>>>       u8 *addr;
>>>>       int ret;
>>>>         if (header->type >= MAX_TELEMETRY_TYPE)
>>>>           return -EINVAL;
>>>>   -    addr = dma_alloc_noncoherent(xdna->ddev.dev, size, &dma_addr,
>>>> -                     DMA_FROM_DEVICE, GFP_KERNEL);
>>>> +    addr = aie2_alloc_msg_buffer(ndev, &buf_sz, &dma_addr);
>>>>       if (!addr)
>>>>           return -ENOMEM;
>>>>         req.buf_addr = dma_addr;
>>>> -    req.buf_size = size;
>>>> +    req.buf_size = buf_sz;
>>>>       req.type = header->type;
>>>>         drm_clflush_virt_range(addr, size); /* device can access */
>>>> @@ -440,7 +455,7 @@ int aie2_query_telemetry(struct amdxdna_dev_hdl 
>>>> *ndev,
>>>>       header->minor = resp.minor;
>>>>     free_buf:
>>>> -    dma_free_noncoherent(xdna->ddev.dev, size, addr, dma_addr, 
>>>> DMA_FROM_DEVICE);
>>>> +    aie2_free_msg_buffer(ndev, buf_sz, addr, dma_addr);
>>>>       return ret;
>>>>   }
>>>>   diff --git a/drivers/accel/amdxdna/aie2_pci.h b/drivers/accel/ 
>>>> amdxdna/aie2_pci.h
>>>> index a929fa98a121..e1745f07b268 100644
>>>> --- a/drivers/accel/amdxdna/aie2_pci.h
>>>> +++ b/drivers/accel/amdxdna/aie2_pci.h
>>>> @@ -336,6 +336,11 @@ int aie2_sync_bo(struct amdxdna_hwctx *hwctx, 
>>>> struct amdxdna_sched_job *job,
>>>>            int (*notify_cb)(void *, void __iomem *, size_t));
>>>>   int aie2_config_debug_bo(struct amdxdna_hwctx *hwctx, struct 
>>>> amdxdna_sched_job *job,
>>>>                int (*notify_cb)(void *, void __iomem *, size_t));
>>>> +void *aie2_alloc_msg_buffer(struct amdxdna_dev_hdl *ndev, u32 *size,
>>>> +                dma_addr_t *dma_addr);
>>>> +#define aie2_free_msg_buffer(ndev, size, buff_addr, 
>>>> dma_addr)        \
>>>> +    dma_free_noncoherent((ndev)->xdna->ddev.dev, size, 
>>>> buff_addr,    \
>>>> +                 dma_addr, DMA_FROM_DEVICE)
>>>>     /* aie2_hwctx.c */
>>>>   int aie2_hwctx_init(struct amdxdna_hwctx *hwctx);
>>>
>