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

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 03-09-2025 19:46, Nilawar, Badal wrote:
>
> 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".
Okay
>
> 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>
>>