Re: [PATCH V4 19/19] accel/amdxdna: Implement runtime suspend and resume support

[yidong Zhang <yidong.zhang@amd.com>]

On 10/14/26 11:36, Lizhi Hou wrote:
>
> On 10/9/26 15:53, David Zhang wrote:
>> Add runtime suspend/resume for the AIE4 classic device by using its
>> system suspend/resume handlers.
>>
>> Runtime suspend returns -EBUSY for a device without runtime_suspend,
>> so the PM core keeps it active instead of entering an error state.
>>
>> A context suspended by runtime PM makes the command wait return
>> -EAGAIN. Resume the device in the wait ioctl and wait again, so a
>> command wait does not fail because of power management.
>>
>> Wrap .driver.pm and the AIE4 PM callbacks with pm_ptr(), so they are
>> discarded when CONFIG_PM is disabled.
>>
>> Signed-off-by: David Zhang <yidong.zhang@amd.com>
>> ---
>>   drivers/accel/amdxdna/aie2_pci.c        |  2 ++
>>   drivers/accel/amdxdna/aie4_ctx.c        | 10 +++++-
>>   drivers/accel/amdxdna/aie4_pci.c        |  4 +++
>>   drivers/accel/amdxdna/amdxdna_ctx.c     | 47 ++++++++++++++++++-------
>>   drivers/accel/amdxdna/amdxdna_pci_drv.c |  4 +--
>>   drivers/accel/amdxdna/amdxdna_pci_drv.h |  2 ++
>>   drivers/accel/amdxdna/amdxdna_pm.c      | 34 ++++++++++++++++++
>>   drivers/accel/amdxdna/amdxdna_pm.h      |  2 ++
>>   8 files changed, 90 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
>> index 690ad035463c..a3866e8933e6 100644
>> --- a/drivers/accel/amdxdna/aie2_pci.c
>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>> @@ -1221,6 +1221,8 @@ const struct amdxdna_dev_ops aie2_ops = {
>>       .fini = aie2_fini,
>>       .resume = aie2_hw_resume,
>>       .suspend = aie2_hw_suspend,
>> +    .runtime_resume = aie2_hw_resume,
>> +    .runtime_suspend = aie2_hw_suspend,
>>       .get_aie_info = aie2_get_info,
>>       .set_aie_state = aie2_set_state,
>>       .hwctx_init = aie2_hwctx_init,
>> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
>> index a2b6b0fa2934..a3f0a69b93b7 100644
>> --- a/drivers/accel/amdxdna/aie4_ctx.c
>> +++ b/drivers/accel/amdxdna/aie4_ctx.c
>> @@ -22,6 +22,7 @@
>>   #include "amdxdna_mailbox.h"
>>   #include "amdxdna_mailbox_helper.h"
>>   #include "amdxdna_pci_drv.h"
>> +#include "amdxdna_pm.h"
>>     #define CTX_INVALID_ID            (~0U)
>>   #define CTX_INVALID_DOORBELL        AMDXDNA_INVALID_DOORBELL_OFFSET
>> @@ -377,10 +378,14 @@ int aie4_hwctx_init(struct amdxdna_hwctx *hwctx)
>>       if (ret)
>>           goto destroy_lock;
>>   -    ret = aie4_hwctx_create(hwctx);
>> +    ret = amdxdna_pm_resume_get_locked(xdna);
>>       if (ret)
>>           goto umq_fini;
>>   +    ret = aie4_hwctx_create(hwctx);
>> +    if (ret)
>> +        goto suspend_put;
>> +
>>       priv->job_work_q = alloc_ordered_workqueue("aie4_job_%d_%d", 0,
>>                              client->pid, hwctx->fw_ctx_id);
>>       if (!priv->job_work_q) {
>> @@ -388,12 +393,15 @@ int aie4_hwctx_init(struct amdxdna_hwctx *hwctx)
>>           ret = -ENOMEM;
>>           goto destroy_ctx;
>>       }
>> +    amdxdna_pm_suspend_put(xdna);
>>         XDNA_DBG(xdna, "hwctx %d.%d init completed", client->pid, hwctx->fw_ctx_id);
>>       return 0;
>>     destroy_ctx:
>>       aie4_hwctx_destroy(hwctx, AIE4_HWCTX_NORMAL);
>> +suspend_put:
>> +    amdxdna_pm_suspend_put(xdna);
>>   umq_fini:
>>       aie4_hwctx_umq_fini(hwctx);
>>   destroy_lock:
>> diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
>> index 70a052ab5f77..60140c3927a2 100644
>> --- a/drivers/accel/amdxdna/aie4_pci.c
>> +++ b/drivers/accel/amdxdna/aie4_pci.c
>> @@ -1041,6 +1041,7 @@ static int aie4_classic_init(struct amdxdna_dev *xdna)
>>       if (ret)
>>           goto free_work_buf;
>>   +    amdxdna_pm_init(xdna);
>>       return 0;
>>     free_work_buf:
>> @@ -1064,6 +1065,7 @@ static void aie4_vf_fini(struct amdxdna_dev *xdna)
>>     static void aie4_classic_fini(struct amdxdna_dev *xdna)
>>   {
>> +    amdxdna_pm_fini(xdna);
>>       aie4_classic_hw_stop(xdna->dev_handle);
>>       aie4_free_fw_log_buffer(xdna->dev_handle);
>>       aie4_free_work_buffer(xdna->dev_handle);
>> @@ -1408,4 +1410,6 @@ const struct amdxdna_dev_ops aie4_classic_ops = {
>>       .set_aie_state        = aie4_set_state,
>>       .resume            = pm_ptr(aie4_classic_resume),
>>       .suspend        = pm_ptr(aie4_classic_suspend),
>> +    .runtime_resume        = pm_ptr(aie4_classic_resume),
>> +    .runtime_suspend    = pm_ptr(aie4_classic_suspend),
>>   };
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
>> index 608b86fe8f6b..f4004630dc1b 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>> @@ -761,13 +761,32 @@ int amdxdna_drm_submit_cmd_ioctl(struct drm_device *dev, void *data, struct drm_
>>       return -EINVAL;
>>   }
>>   +static int amdxdna_hwctx_wait_cmd(struct amdxdna_client *client,
>> +                  struct amdxdna_drm_wait_cmd *args)
>> +{
>> +    struct amdxdna_dev *xdna = client->xdna;
>> +    struct amdxdna_hwctx *hwctx;
>> +    int ret, idx;
>> +
>> +    idx = srcu_read_lock(&client->hwctx_srcu);
>> +    hwctx = xa_load(&client->hwctx_xa, args->hwctx);
>> +    if (!hwctx) {
>> +        XDNA_DBG(xdna, "PID %d failed to get ctx %d", client->pid, args->hwctx);
>> +        ret = -EINVAL;
>> +    } else {
>> +        ret = xdna->dev_info->ops->cmd_wait(hwctx, args->seq, args->timeout);
>> +    }
>> +    srcu_read_unlock(&client->hwctx_srcu, idx);
>> +
>> +    return ret;
>> +}
>> +
>>   int amdxdna_drm_wait_cmd_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>   {
>>       struct amdxdna_client *client = filp->driver_priv;
>>       struct amdxdna_dev *xdna = to_xdna_dev(dev);
>>       struct amdxdna_drm_wait_cmd *args = data;
>> -    struct amdxdna_hwctx *hwctx;
>> -    int ret, idx;
>> +    int ret;
>>         XDNA_DBG(xdna, "PID %d ctx %d timeout set %d ms for cmd %llu",
>>            client->pid, args->hwctx, args->timeout, args->seq);
>> @@ -775,22 +794,26 @@ int amdxdna_drm_wait_cmd_ioctl(struct drm_device *dev, void *data, struct drm_fi
>>       if (!xdna->dev_info->ops->cmd_wait)
>>           return -EOPNOTSUPP;
>>   -    idx = srcu_read_lock(&client->hwctx_srcu);
>> -    hwctx = xa_load(&client->hwctx_xa, args->hwctx);
>> -    if (!hwctx) {
>> -        XDNA_DBG(xdna, "PID %d failed to get ctx %d", client->pid, args->hwctx);
>> -        ret = -EINVAL;
>> -        goto unlock_ctx_srcu;
>> +    ret = amdxdna_hwctx_wait_cmd(client, args);
>> +    if (ret == -EAGAIN) {
>> +        /*
>> +         * Suspended by power management before the command completed.
>> +         * Resume the device, which reconnects the context, and wait
>> +         * again. Take the PM reference outside hwctx_srcu: runtime
>> +         * resume takes dev_lock, which hwctx destroy holds across
>> +         * synchronize_srcu().
>> +         */
>> +        ret = amdxdna_pm_resume_get(xdna);
>> +        if (ret)
>> +            return ret;
>> +        ret = amdxdna_hwctx_wait_cmd(client, args);
>> +        amdxdna_pm_suspend_put(xdna);
>>       }
>>   -    ret = xdna->dev_info->ops->cmd_wait(hwctx, args->seq, args->timeout);
>> -
>>       XDNA_DBG(xdna, "PID %d ctx %d cmd %lld wait finished, ret %d",
>>            client->pid, args->hwctx, args->seq, ret);
>>         trace_amdxdna_debug_point(current->comm, args->seq, "job returned to user");
>>   -unlock_ctx_srcu:
>> -    srcu_read_unlock(&client->hwctx_srcu, idx);
>>       return ret;
>>   }
>> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> index f5f7831c4e80..cb78f145ffa1 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> @@ -465,7 +465,7 @@ static void amdxdna_remove(struct pci_dev *pdev)
>>     static const struct dev_pm_ops amdxdna_pm_ops = {
>>       SYSTEM_SLEEP_PM_OPS(amdxdna_pm_suspend, amdxdna_pm_resume)
>> -    RUNTIME_PM_OPS(amdxdna_pm_suspend, amdxdna_pm_resume, NULL)
>> +    RUNTIME_PM_OPS(amdxdna_pm_runtime_suspend, amdxdna_pm_runtime_resume, NULL)
>>   };
>>     static int amdxdna_sriov_configure(struct pci_dev *pdev, int num_vfs)
>> @@ -484,7 +484,7 @@ static struct pci_driver amdxdna_pci_driver = {
>>       .id_table = pci_ids,
>>       .probe = amdxdna_probe,
>>       .remove = amdxdna_remove,
>> -    .driver.pm = &amdxdna_pm_ops,
>> +    .driver.pm = pm_ptr(&amdxdna_pm_ops),
>>       .sriov_configure = amdxdna_sriov_configure,
>>   };
>>   diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h b/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> index 505477b4a292..4d047cf3d981 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> @@ -57,6 +57,8 @@ struct amdxdna_dev_ops {
>>       void (*debugfs_init)(struct amdxdna_dev *xdna);
>>       int (*resume)(struct amdxdna_dev *xdna);
>>       int (*suspend)(struct amdxdna_dev *xdna);
>> +    int (*runtime_resume)(struct amdxdna_dev *xdna);
>> +    int (*runtime_suspend)(struct amdxdna_dev *xdna);
>>       int (*sriov_configure)(struct amdxdna_dev *xdna, int num_vfs);
>>       int (*hwctx_init)(struct amdxdna_hwctx *hwctx);
>>       void (*hwctx_stop)(struct amdxdna_hwctx *hwctx);
>> diff --git a/drivers/accel/amdxdna/amdxdna_pm.c b/drivers/accel/amdxdna/amdxdna_pm.c
>> index b1fafddd7ad5..7d7bc965342d 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pm.c
>> +++ b/drivers/accel/amdxdna/amdxdna_pm.c
>> @@ -37,11 +37,40 @@ int amdxdna_pm_resume(struct device *dev)
>>       return ret;
>>   }
>>   +int amdxdna_pm_runtime_suspend(struct device *dev)
>> +{
>> +    struct amdxdna_dev *xdna = to_xdna_dev(dev_get_drvdata(dev));
>> +    int ret = -EBUSY;
>> +
>> +    guard(mutex)(&xdna->dev_lock);
>> +    if (xdna->dev_info->ops->runtime_suspend)
>> +        ret = xdna->dev_info->ops->runtime_suspend(xdna);
>> +
>> +    XDNA_DBG(xdna, "Runtime suspend done ret %d", ret);
>> +    return ret;
>> +}
>> +
>> +int amdxdna_pm_runtime_resume(struct device *dev)
>> +{
>> +    struct amdxdna_dev *xdna = to_xdna_dev(dev_get_drvdata(dev));
>> +    int ret = -EOPNOTSUPP;
>> +
>> +    guard(mutex)(&xdna->dev_lock);
>> +    if (xdna->dev_info->ops->runtime_resume)
>> +        ret = xdna->dev_info->ops->runtime_resume(xdna);
>> +
>> +    XDNA_DBG(xdna, "Runtime resume done ret %d", ret);
>> +    return ret;
>> +}
>> +
>>   int amdxdna_pm_resume_get(struct amdxdna_dev *xdna)
>>   {
>>       struct device *dev = xdna->ddev.dev;
>>       int ret;
>>   +    if (!pm_runtime_enabled(dev))
>> +        return 0;
>> +
>>       ret = pm_runtime_resume_and_get(dev);
>>       if (ret) {
>>           XDNA_ERR(xdna, "Resume failed: %d", ret);
>> @@ -55,6 +84,10 @@ void amdxdna_pm_suspend_put(struct amdxdna_dev *xdna)
>>   {
>>       struct device *dev = xdna->ddev.dev;
>>   +    if (!pm_runtime_enabled(dev))
>> +        return;
>> +
>> +    pm_runtime_mark_last_busy(dev);
> This should be removed. mark_last_busy is called by put_autosuspend.
>>       pm_runtime_put_autosuspend(dev);
>>   }
>>   @@ -66,6 +99,7 @@ void amdxdna_pm_init(struct amdxdna_dev *xdna)
>>       pm_runtime_set_autosuspend_delay(dev, AMDXDNA_AUTOSUSPEND_DELAY);
>>       pm_runtime_use_autosuspend(dev);
>>       pm_runtime_allow(dev);
>> +    pm_runtime_mark_last_busy(dev);
>
> This should be removed. 

Thank you. I will remove these 2 line of code in next version of the patch.

/David

>
> Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>
>>       pm_runtime_put_autosuspend(dev);
>>   }
>>   diff --git a/drivers/accel/amdxdna/amdxdna_pm.h b/drivers/accel/amdxdna/amdxdna_pm.h
>> index 3d26b973e0e3..02beef6448f1 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pm.h
>> +++ b/drivers/accel/amdxdna/amdxdna_pm.h
>> @@ -10,6 +10,8 @@
>>     int amdxdna_pm_suspend(struct device *dev);
>>   int amdxdna_pm_resume(struct device  *dev);
>> +int amdxdna_pm_runtime_suspend(struct device *dev);
>> +int amdxdna_pm_runtime_resume(struct device *dev);
>>   int amdxdna_pm_resume_get(struct amdxdna_dev *xdna);
>>   void amdxdna_pm_suspend_put(struct amdxdna_dev *xdna);
>>   void amdxdna_pm_init(struct amdxdna_dev *xdna);