Re: [PATCH v5] drm/xe: Poll GT for C6 before D3

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 9/16/2026 8:44 AM, Rodrigo Vivi wrote:
> On Wed, Sep 16, 2026 at 08:05:56AM -0700, Belgaumkar, Vinay wrote:
>> On 9/16/2026 5:19 AM, Rodrigo Vivi wrote:
>>> On Tue, Sep 15, 2026 at 02:01:13PM -0700, Vinay Belgaumkar wrote:
>>>> Ensure GT C6 state before D3 transition in runtime suspend flow.
>>>> This avoids the scenario where GT is being accessed after D3 state
>>>> is programmed. If the check for C6 fails, resume the device so it is
>>>> not stuck in an intermediate state.
>>>>
>>>> v2: Force resume when we cancel suspend due to C6 check (Sashiko)
>>>> v3: Abort the suspend properly when idle check fails (Sashiko)
>>>> v4: Only check for C6 during suspend (Rodrigo). Have check in
>>>> xe_pci_runtime_suspend() instead of xe_pci_suspend().
>>>> v5: Don't checkfor PVC (Sashiko), rename function from_device_c6
>>>> to _device_idle
>>>>
>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_gt_idle.c | 36 +++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/xe/xe_gt_idle.h |  1 +
>>>>    drivers/gpu/drm/xe/xe_pci.c     | 13 +++++++++++-
>>>>    drivers/gpu/drm/xe/xe_pm.c      | 24 ++++++++++++++++++++++
>>>>    drivers/gpu/drm/xe/xe_pm.h      |  1 +
>>>>    5 files changed, 74 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c b/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> index c208d7563e36..8242feede1dd 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> @@ -8,10 +8,12 @@
>>>>    #include <drm/drm_managed.h>
>>>>    #include <generated/xe_wa_oob.h>
>>>> +#include <linux/iopoll.h>
>>>>    #include "xe_force_wake.h"
>>>>    #include "xe_device.h"
>>>>    #include "xe_gt.h"
>>>>    #include "xe_gt_idle.h"
>>>> +#include "xe_gt_printk.h"
>>>>    #include "xe_gt_sysfs.h"
>>>>    #include "xe_guc_pc.h"
>>>>    #include "regs/xe_gt_regs.h"
>>>> @@ -451,3 +453,37 @@ int xe_gt_idle_disable_c6(struct xe_gt *gt)
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +static int wait_for_gt_c6_state(struct xe_gt *gt, u32 timeout_ms)
>>>> +{
>>>> +	struct xe_guc_pc *pc = &gt->uc.guc.pc;
>>>> +	enum xe_gt_idle_state state;
>>>> +
>>>> +	return poll_timeout_us(state = gt->gtidle.idle_status(pc),
>>>> +			       state == GT_IDLE_C6,
>>>> +			       20,
>>>> +			       timeout_ms * USEC_PER_MSEC,
>>>> +			       false);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_gt_idle_wait_for_c6 - Poll for GT C6
>>>> + * @gt: GT object
>>>> + * @timeout_ms: wait time in ms
>>>> + *
>>>> + * This function waits for GT to enter C6.
>>>> + *
>>>> + * Return: 0 on success, -EAGAIN otherwise
>>>> + */
>>>> +int xe_gt_idle_wait_for_c6(struct xe_gt *gt, u32 timeout_ms)
>>>> +{
>>>> +	if (IS_SRIOV_VF(gt_to_xe(gt)))
>>>> +		return 0;
>>> Why?
>> We skip GuC RC init for VFs, so cannot check for it.
> Fair enough. To keep consistency.
> In my view you wouldn't get here on rpm flow, but you are right,
> since it is a different scope on this component itself, let's keep it.
>
>>> And why not the PVC check right in here?
>>>
>>>> +
>>>> +	if (wait_for_gt_c6_state(gt, timeout_ms)) {
>>>> +		xe_gt_dbg(gt, "GT is not in C6\n");
>>>> +		return -EAGAIN;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.h b/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> index 9c34a155e102..2c8e9adf4a68 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> @@ -18,5 +18,6 @@ void xe_gt_idle_enable_pg(struct xe_gt *gt);
>>>>    void xe_gt_idle_disable_pg(struct xe_gt *gt);
>>>>    int xe_gt_idle_pg_print(struct xe_gt *gt, struct drm_printer *p);
>>>>    u64 xe_gt_idle_residency_msec(struct xe_gt_idle *gtidle);
>>>> +int xe_gt_idle_wait_for_c6(struct xe_gt *gt, u32 timeout_ms);
>>>>    #endif /* _XE_GT_IDLE_H_ */
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>>> index ab4da1d9a9f1..379795ac5e7b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>> @@ -1381,7 +1381,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
>>>>    {
>>>>    	struct pci_dev *pdev = to_pci_dev(dev);
>>>>    	struct xe_device *xe = pdev_to_xe_device(pdev);
>>>> -	int err;
>>>> +	int err, ret;
>>>>    	/*
>>>>    	 * We hold an additional reference to the runtime PM to keep PF in D0
>>>> @@ -1396,6 +1396,17 @@ static int xe_pci_runtime_suspend(struct device *dev)
>>>>    	if (err)
>>>>    		return err;
>>>> +	err = xe_pm_check_device_idle(xe);
>>> I still don't like _idle prefix in here...
>> I initially had it as _check_device_c6, then realized it is a misleading
>> term.
> I know the pain... I'm bad with naming myself.
> But idleness is not what we are actually checking and _idle is already
> a rpm sufix that has a different meaning in the flow and might be confusing...
>
>>>> +	if (err) {
>>>> +		drm_dbg(&xe->drm, "Resuming - Device idle check failed!");
>>>> +		ret = xe_pm_runtime_resume(xe);
>>>> +		if (ret) {
>>>> +			drm_err(&xe->drm, "Resume failed after suspend was canceled");
>>>> +			return ret;
>>>> +		}
>>>> +		return err;
>>>> +	}
>>>> +
>>>>    	pci_save_state(pdev);
>>>>    	if (xe->d3cold.allowed) {
>>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>>> index f517bf453b54..f03a463d2a05 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>>> @@ -1030,6 +1030,30 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
>>>>    	mutex_unlock(&xe->d3cold.lock);
>>>>    }
>>>> +/**
>>>> + * xe_pm_check_device_idle() - Check if all GTs are idle.
>>>> + * @xe: xe device instance
>>>> + *
>>>> + * One of the ways to do this is to check for GT C6 state. This check
>>>> + * will fail if some component is holding a forcewake, for example.
>>>> + *
>>>> + * Return: 0 on success, -EAGAIN on failure
>>>> + */
>>>> +int xe_pm_check_device_idle(struct xe_device *xe)
>>>> +{
>>>> +	struct xe_gt *gt;
>>>> +	u8 id;
>>>> +
>>>> +	if (xe->info.platform == XE_PVC)
>>>> +		return 0;
>>> PVC check belongs inside the gt_idle component,
>>> where the RC xe decisions are made
>> Sure, I added it there since it is a device level flag.
>>>> +
>>>> +	for_each_gt(gt, xe, id)
>>>> +		if (xe_gt_idle_wait_for_c6(gt, 200))
>>>> +			return -EAGAIN;
>>>> +
>>>> +	return 0;
>>>> +}
>>> But anyway, why this indirection through xe_pm? Why can't the xe_pci call
>>> directly the gt_idle waiting for c6 inside the runtime_suspend function?
>> The only reason was that all gt_idle functions take GT as param, not Xe. In
>> that case, I would have had the for_all_gts loop in xe_pci, which I was
>> trying to avoid.
> it makes sense. Just rename to xe_pm_wait_all_c6 or something like that...

Yes, that seems better.

Thanks,

Vinay.

>
>> Thanks,
>>
>> Vinay.
>>
>>>> +
>>>>    /**
>>>>     * xe_pm_module_init() - Perform xe_pm specific module initialization.
>>>>     *
>>>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>>>> index 6d5ab09cb769..a4fdc9c5cb10 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pm.h
>>>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>>>> @@ -36,6 +36,7 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
>>>>    bool xe_rpm_reclaim_safe(const struct xe_device *xe);
>>>>    struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>>>>    int xe_pm_block_on_suspend(struct xe_device *xe);
>>>> +int xe_pm_check_device_idle(struct xe_device *xe);
>>>>    void xe_pm_might_block_on_suspend(void);
>>>>    int xe_pm_module_init(void);
>>>> -- 
>>>> 2.38.1
>>>>