Re: [PATCH v5 2/2] drm/xe: remove GuC reload in D3Hot path

[Riana Tauro <riana.tauro@intel.com>]

Hi Matthew, Michal

Thank you for the review comments

On 8/2/2024 2:32 AM, Matthew Brost wrote:
> On Thu, Aug 01, 2024 at 10:55:07PM +0200, Michal Wajdeczko wrote:
>>
>>
>> On 01.08.2024 22:42, Matthew Brost wrote:
>>> On Thu, Aug 01, 2024 at 11:29:41AM +0530, Riana Tauro wrote:
>>>> Currently GuC is reloaded for both runtime resume and system resume.
>>>> For D3hot <-> D0 transitions no power is lost during suspend so GuC reload
>>>> is not necessary.
>>>>
>>>> Remove GuC reload from D3Hot path and only enable/disable CTB
>>>> communication.
>>>>
>>>> v2: rebase
>>>>
>>>> v3: fix commit message
>>>>      add kernel-doc for gt suspend and resume methods
>>>>      fix comment
>>>>      do not split register and enable calls of CT (Michal)
>>>>
>>>> v4: fix commit message
>>>>      fix comment (Karthik)
>>>>      split patches
>>>>      correct kernel-doc (Rodrigo)
>>>>
>>>> v5: do not expose internal function of CT layer (Michal)
>>>>      remove wait for outstanding g2h as it will be always zero,
>>>>      use assert instead (Matthew Brost)
>>>>      use runtime suspend and runtime resume pair for CT layer
>>>>      (Michal / Matthew Brost)
>>>>
>>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_gt.c     | 33 +++++++++++++++++++++++++++++----
>>>>   drivers/gpu/drm/xe/xe_gt.h     |  4 ++--
>>>>   drivers/gpu/drm/xe/xe_guc.c    | 30 ++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/xe/xe_guc.h    |  2 ++
>>>>   drivers/gpu/drm/xe/xe_guc_ct.c | 31 +++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/xe/xe_guc_ct.h |  3 +++
>>>>   drivers/gpu/drm/xe/xe_pm.c     |  8 ++++----
>>>>   drivers/gpu/drm/xe/xe_uc.c     | 28 ++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/xe/xe_uc.h     |  2 ++
>>>>   9 files changed, 131 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>>> index 58895ed22f6e..e0b13dc7663b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>>> @@ -831,8 +831,16 @@ void xe_gt_suspend_prepare(struct xe_gt *gt)
>>>>   	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>>>>   }
>>>>   
>>>> -int xe_gt_suspend(struct xe_gt *gt)
>>>> +/**
>>>> + * xe_gt_suspend - GT suspend helper
>>>> + * @gt: GT object
>>>> + * @runtime: true if this is from runtime suspend
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise.
>>>> + */
>>>> +int xe_gt_suspend(struct xe_gt *gt, bool runtime)
>>>>   {
>>>> +	struct xe_device *xe = gt_to_xe(gt);
>>>>   	int err;
>>>>   
>>>>   	xe_gt_dbg(gt, "suspending\n");
>>>> @@ -842,7 +850,11 @@ int xe_gt_suspend(struct xe_gt *gt)
>>>>   	if (err)
>>>>   		goto err_msg;
>>>>   
>>>> -	err = xe_uc_suspend(&gt->uc);
>>>> +	if (runtime && !xe->d3cold.allowed)
>>>> +		err = xe_uc_runtime_suspend(&gt->uc);
>>>> +	else
>>>> +		err = xe_uc_suspend(&gt->uc);
>>>> +
>>>>   	if (err)
>>>>   		goto err_force_wake;
>>>>   
>>>> @@ -881,8 +893,16 @@ int xe_gt_sanitize_freq(struct xe_gt *gt)
>>>>   	return ret;
>>>>   }
>>>>   
>>>> -int xe_gt_resume(struct xe_gt *gt)
>>>> +/**
>>>> + * xe_gt_resume - GT resume helper
>>>> + * @gt: GT object
>>>> + * @runtime: true if called on runtime resume
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise.
>>>> + */
>>>> +int xe_gt_resume(struct xe_gt *gt, bool runtime)
>>>>   {
>>>> +	struct xe_device *xe = gt_to_xe(gt);
>>>>   	int err;
>>>>   
>>>>   	xe_gt_dbg(gt, "resuming\n");
>>>> @@ -890,7 +910,12 @@ int xe_gt_resume(struct xe_gt *gt)
>>>>   	if (err)
>>>>   		goto err_msg;
>>>>   
>>>> -	err = do_gt_restart(gt);
>>>> +	/* GuC is still alive at D3hot, no need to reload it */
>>>> +	if (runtime && !xe->d3cold.allowed)
>>>> +		xe_uc_runtime_resume(&gt->uc);
>>>> +	else
>>>> +		err = do_gt_restart(gt);
>>>> +
>>>>   	if (err)
>>>>   		goto err_force_wake;
>>>>   
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>>>> index 8b1a5027dcf2..21f27ca23b67 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt.h
>>>> +++ b/drivers/gpu/drm/xe/xe_gt.h
>>>> @@ -53,8 +53,8 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt);
>>>>   void xe_gt_record_user_engines(struct xe_gt *gt);
>>>>   
>>>>   void xe_gt_suspend_prepare(struct xe_gt *gt);
>>>> -int xe_gt_suspend(struct xe_gt *gt);
>>>> -int xe_gt_resume(struct xe_gt *gt);
>>>> +int xe_gt_suspend(struct xe_gt *gt, bool runtime);
>>>> +int xe_gt_resume(struct xe_gt *gt, bool runtime);
>>>>   void xe_gt_reset_async(struct xe_gt *gt);
>>>>   void xe_gt_sanitize(struct xe_gt *gt);
>>>>   int xe_gt_sanitize_freq(struct xe_gt *gt);
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>>>> index 1ff49318f001..e1923a00a9f6 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>>> @@ -881,6 +881,8 @@ int xe_guc_enable_communication(struct xe_guc *guc, bool register_ctb)
>>>>   
>>>>   	if (register_ctb)
>>>>   		err = xe_guc_ct_enable(&guc->ct);
>>>> +	else
>>>> +		xe_guc_ct_runtime_resume(&guc->ct);
>>>>   
>>>>   	if (err)
>>>>   		return err;
>>>> @@ -1112,6 +1114,34 @@ void xe_guc_sanitize(struct xe_guc *guc)
>>>>   	guc->submission_state.enabled = false;
>>>>   }
>>>>   
>>>> +/**
>>>> + * xe_guc_runtime_suspend - GuC runtime suspend
>>>> + * @guc: GuC object
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise.
>>>> + */
>>>> +int xe_guc_runtime_suspend(struct xe_guc *guc)
>>>> +{
>>>> +	return xe_guc_ct_runtime_suspend(&guc->ct);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_guc_runtime_resume - GuC runtime resume
>>>> + * @guc: GuC object
>>>> + *
>>>> + * This function enables GuC CTB communication
>>>> + */
>>>> +void xe_guc_runtime_resume(struct xe_guc *guc)
>>>> +{
>>>> +	/*
>>>> +	 * Power is not lost when in D3Hot state,
>>>> +	 * hence it is not necessary to reload GuC
>>>> +	 * everytime. Only enable interrupts and
>>>> +	 * CTB communication during resume
>>>> +	 */
>>>> +	xe_guc_enable_communication(guc, false);
>>>> +}
>>>> +
>>>>   int xe_guc_reset_prepare(struct xe_guc *guc)
>>>>   {
>>>>   	return xe_guc_submit_reset_prepare(guc);
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
>>>> index 13e75573f7d0..ddc87798e176 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc.h
>>>> +++ b/drivers/gpu/drm/xe/xe_guc.h
>>>> @@ -22,6 +22,8 @@ int xe_guc_upload(struct xe_guc *guc);
>>>>   int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
>>>>   int xe_guc_enable_communication(struct xe_guc *guc, bool register_ctb);
>>>>   int xe_guc_suspend(struct xe_guc *guc);
>>>> +int xe_guc_runtime_suspend(struct xe_guc *guc);
>>>> +void xe_guc_runtime_resume(struct xe_guc *guc);
>>>>   void xe_guc_notify(struct xe_guc *guc);
>>>>   int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
>>>>   int xe_guc_mmio_send(struct xe_guc *guc, const u32 *request, u32 len);
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> index beeeb120d1fc..9e42c5d29971 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> @@ -419,6 +419,37 @@ int xe_guc_ct_enable(struct xe_guc_ct *ct)
>>>>   	return err;
>>>>   }
>>>>   
>>>> +/**
>>>> + * xe_guc_ct_runtime_resume- GuC CT runtime resume
>>>> + * @ct: the &xe_guc_ct
>>>> + *
>>>> + * Mark GuC CT as enabled on runtime resume
>>>> + */
>>>> +void xe_guc_ct_runtime_resume(struct xe_guc_ct *ct)
>>>> +{
>>>
>>> I'd assert here g2h head and tail are equal in memory to ensure we know
>>> we are doing with the PM as we shouldn't have any g2h on runtime resume
>>> if we have the PM correct. If they are unequal, this isn't fatal just
>>> kick the g2h handler.
>>>
> 
> 
> Based on Michals feedback below, I guess we xe_gt_WARN_ON here too for
> g2h head / tail check + fallback to kick the g2h handler.
> 
Sure will add this
>>>> +	xe_guc_ct_set_state(ct, XE_GUC_CT_STATE_ENABLED);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_guc_ct_runtime_suspend- GuC CT runtime suspend
>>>> + * @ct: the &xe_guc_ct
>>>> + *
>>>> + * Mark GuC CT as disabled on runtime suspend
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise
>>>> + */
>>>> +int xe_guc_ct_runtime_suspend(struct xe_guc_ct *ct)
>>>> +{
>>>
>>> Nit, I'd write it like this:
>>>
>>> xe_gt_assert(ct_to_gt(ct), !ct->g2h_outstanding);
>>> if (ct->g2h_outstanding)
>>> 	return -EBUSY;
>>
>> IMO neither option is good as xe_assert() is not to be used where you
>> plan to use fallback path [1] you may want to use xe_gt_WARN_ON instead
>>
> 
> So like:
> 
> if (xe_gt_WARN_ON(ct_to_gt(ct), ct->g2h_outstanding))
> 	return -EBUSY;
> 
> Yea, that is probably better.
> 
Will make this change

Thanks,
Riana
> Matt
> 
>> Michal
>>
>> [1] https://docs.kernel.org/gpu/xe/xe_debugging.html
>>
>>>
>>> Matt
>>>
>>>> +	if (ct->g2h_outstanding) {
>>>> +		xe_gt_assert(ct_to_gt(ct), !ct->g2h_outstanding);
>>>> +		return -EBUSY;
>>>> +	}
>>>> +
>>>> +	xe_guc_ct_disable(ct);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>   static void stop_g2h_handler(struct xe_guc_ct *ct)
>>>>   {
>>>>   	cancel_work_sync(&ct->g2h_worker);
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
>>>> index 190202fce2d0..0cf9d77feb35 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
>>>> @@ -16,6 +16,9 @@ void xe_guc_ct_disable(struct xe_guc_ct *ct);
>>>>   void xe_guc_ct_stop(struct xe_guc_ct *ct);
>>>>   void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
>>>>   
>>>> +void xe_guc_ct_runtime_resume(struct xe_guc_ct *ct);
>>>> +int xe_guc_ct_runtime_suspend(struct xe_guc_ct *ct);
>>>> +
>>>>   struct xe_guc_ct_snapshot *
>>>>   xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct, bool atomic);
>>>>   void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
>>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>>> index 9f3c14fd9f33..c73a728a7450 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>>> @@ -101,7 +101,7 @@ int xe_pm_suspend(struct xe_device *xe)
>>>>   	xe_display_pm_suspend(xe, false);
>>>>   
>>>>   	for_each_gt(gt, xe, id) {
>>>> -		err = xe_gt_suspend(gt);
>>>> +		err = xe_gt_suspend(gt, false);
>>>>   		if (err) {
>>>>   			xe_display_pm_resume(xe, false);
>>>>   			goto err;
>>>> @@ -157,7 +157,7 @@ int xe_pm_resume(struct xe_device *xe)
>>>>   	xe_display_pm_resume(xe, false);
>>>>   
>>>>   	for_each_gt(gt, xe, id)
>>>> -		xe_gt_resume(gt);
>>>> +		xe_gt_resume(gt, false);
>>>>   
>>>>   	err = xe_bo_restore_user(xe);
>>>>   	if (err)
>>>> @@ -374,7 +374,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>>>   	}
>>>>   
>>>>   	for_each_gt(gt, xe, id) {
>>>> -		err = xe_gt_suspend(gt);
>>>> +		err = xe_gt_suspend(gt, true);
>>>>   		if (err)
>>>>   			goto out;
>>>>   	}
>>>> @@ -428,7 +428,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>>>   	xe_irq_resume(xe);
>>>>   
>>>>   	for_each_gt(gt, xe, id)
>>>> -		xe_gt_resume(gt);
>>>> +		xe_gt_resume(gt, true);
>>>>   
>>>>   	if (xe->d3cold.allowed) {
>>>>   		xe_display_pm_resume(xe, true);
>>>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>>>> index fa98e9f22631..8e535153cc62 100644
>>>> --- a/drivers/gpu/drm/xe/xe_uc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>>>> @@ -288,6 +288,34 @@ int xe_uc_suspend(struct xe_uc *uc)
>>>>   	return xe_guc_suspend(&uc->guc);
>>>>   }
>>>>   
>>>> +/**
>>>> + * xe_uc_runtime_suspend - uC runtime suspend
>>>> + * @uc: uC object
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise
>>>> + */
>>>> +int xe_uc_runtime_suspend(struct xe_uc *uc)
>>>> +{
>>>> +	if (!xe_device_uc_enabled(uc_to_xe(uc)))
>>>> +		return 0;
>>>> +
>>>> +	return xe_guc_runtime_suspend(&uc->guc);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_uc_runtime_resume - uC runtime resume
>>>> + * @uc: uC object
>>>> + *
>>>> + * Called while resuming from D3Hot
>>>> + */
>>>> +void xe_uc_runtime_resume(struct xe_uc *uc)
>>>> +{
>>>> +	if (!xe_device_uc_enabled(uc_to_xe(uc)))
>>>> +		return;
>>>> +
>>>> +	xe_guc_runtime_resume(&uc->guc);
>>>> +}
>>>> +
>>>>   /**
>>>>    * xe_uc_remove() - Clean up the UC structures before driver removal
>>>>    * @uc: the UC object
>>>> diff --git a/drivers/gpu/drm/xe/xe_uc.h b/drivers/gpu/drm/xe/xe_uc.h
>>>> index 506517c11333..1e223d67086a 100644
>>>> --- a/drivers/gpu/drm/xe/xe_uc.h
>>>> +++ b/drivers/gpu/drm/xe/xe_uc.h
>>>> @@ -15,6 +15,8 @@ int xe_uc_init_hw(struct xe_uc *uc);
>>>>   int xe_uc_fini_hw(struct xe_uc *uc);
>>>>   void xe_uc_gucrc_disable(struct xe_uc *uc);
>>>>   int xe_uc_reset_prepare(struct xe_uc *uc);
>>>> +int xe_uc_runtime_suspend(struct xe_uc *uc);
>>>> +void xe_uc_runtime_resume(struct xe_uc *uc);
>>>>   void xe_uc_stop_prepare(struct xe_uc *uc);
>>>>   void xe_uc_stop(struct xe_uc *uc);
>>>>   int xe_uc_start(struct xe_uc *uc);
>>>> -- 
>>>> 2.40.0
>>>>