Re: [v4,10/11] drm/xe/vrsr: Enable VRSR

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 24-06-2025 17:15, Poosa, Karthik wrote:
>
> On 29-05-2025 16:46, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Enabling VRSR in runtime suspend and also in System wide suspend.
>
> This can be rephrased to -
>
> Integrate VRSR into both system-wide and runtime suspend-resume flows.

I will drop gerund.  So it will be "Enable VRSR  in runtime suspend and 
in System wide suspend".

Thanks,
Badal

>
>> Also fix couple of typo in xe_pm.c.
>>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pci.c |  4 ++--
>>   drivers/gpu/drm/xe/xe_pm.c  | 46 +++++++++++++++++++++++++++----------
>>   2 files changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 5ae1df345416..fdf878594fb0 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -926,7 +926,7 @@ static int xe_pci_suspend(struct device *dev)
>>         /*
>>        * Enabling D3Cold is needed for S2Idle/S0ix.
>> -     * It is save to allow here since xe_pm_suspend has evicted
>> +     * It is safe to allow here since xe_pm_suspend has evicted
>>        * the local memory and the direct complete optimization is 
>> disabled.
>>        */
>>       d3cold_toggle(pdev, D3COLD_ENABLE);
>> @@ -943,7 +943,7 @@ static int xe_pci_resume(struct device *dev)
>>       struct pci_dev *pdev = to_pci_dev(dev);
>>       int err;
>>   -    /* Give back the D3Cold decision to the runtime P M*/
>> +    /* Give back the D3Cold decision to the runtime PM */
>>       d3cold_toggle(pdev, D3COLD_DISABLE);
>>         err = pci_set_power_state(pdev, PCI_D0);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index 1e061bfc3e52..19596d467298 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -147,10 +147,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>         xe_display_pm_suspend(xe);
>>   -    /* FIXME: Super racey... */
>> -    err = xe_bo_evict_all(xe);
>> -    if (err)
>> -        goto err_pxp;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        /* FIXME: Super racey... */
>> +        err = xe_bo_evict_all(xe);
>> +        if (err)
>> +            goto err_pxp;
>> +    }
>>         for_each_gt(gt, xe, id) {
>>           err = xe_gt_suspend(gt);
>> @@ -162,6 +164,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>         xe_display_pm_suspend_late(xe);
>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>> +        err = xe_pm_vrsr_enable(xe, true);
>> +            if (err)
>> +                goto err_display;
>> +    }
>> +
>>       drm_dbg(&xe->drm, "Device suspended\n");
>>       return 0;
>>   @@ -203,9 +211,11 @@ int xe_pm_resume(struct xe_device *xe)
>>        * This only restores pinned memory which is the memory 
>> required for the
>>        * GT(s) to resume.
>>        */
>> -    err = xe_bo_restore_early(xe);
>> -    if (err)
>> -        goto err;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        err = xe_bo_restore_early(xe);
>> +        if (err)
>> +            goto err;
>> +    }
>>         xe_irq_resume(xe);
>>   @@ -214,9 +224,11 @@ int xe_pm_resume(struct xe_device *xe)
>>         xe_display_pm_resume(xe);
>>   -    err = xe_bo_restore_late(xe);
>> -    if (err)
>> -        goto err;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        err = xe_bo_restore_late(xe);
>> +        if (err)
>> +            goto err;
>> +    }
>>         xe_pxp_pm_resume(xe->pxp);
>>   @@ -616,7 +628,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>         xe_display_pm_runtime_suspend(xe);
>>   -    if (xe->d3cold.target_state) {
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           err = xe_bo_evict_all(xe);
>>           if (err)
>>               goto out_resume;
>> @@ -632,6 +644,14 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>         xe_display_pm_runtime_suspend_late(xe);
>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>> +        err = xe_pm_vrsr_enable(xe, true);
>> +            if (err) {
>> +                drm_err(&xe->drm, "Failed to enable VRSR: %d\n", err);
>> +                goto out_resume;
>> +            }
>> +    }
>> +
>>       xe_rpm_lockmap_release(xe);
>>       xe_pm_write_callback_task(xe, NULL);
>>       return 0;
>> @@ -669,7 +689,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>               goto out;
>>             xe_display_pm_resume_early(xe);
>> +    }
>>   +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           /*
>>            * This only restores pinned memory which is the memory
>>            * required for the GT(s) to resume.
>> @@ -686,7 +708,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>         xe_display_pm_runtime_resume(xe);
>>   -    if (xe->d3cold.target_state) {
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           err = xe_bo_restore_late(xe);
>>           if (err)
>>               goto out;
>
> Please rephrase commit message. Other than that, code code flow looks 
> fine.
>
> Acked-by: Karthik Poosa <karthik.poosa@intel.com>
>