[Intel-xe] [PATCH v2 2/2] drm/xe: Toggle GuC CT communication for D3Hot Transition

[Michal Wajdeczko]

On 12.10.2023 08:32, Riana Tauro wrote:
> During Runtime suspend/resume, GuC is reloaded for both
> D3hot/D3Cold-> D0 transistions. It is not necessary for GuC to be
> loaded everytime for D3hot->D0, only enable/disable ctb communication.
> 
> Add a function that toggles CT communication when d3cold
> is not allowed.
> 
> v2: simplify code (Bala)
>     handle pmu suspend in runtime suspend (Rodrigo)
>     change function names
> 
> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c | 62 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt.h |  2 ++
>  drivers/gpu/drm/xe/xe_pm.c |  4 +--
>  drivers/gpu/drm/xe/xe_uc.c | 21 +++++++++++++
>  drivers/gpu/drm/xe/xe_uc.h |  2 +-
>  5 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index c63e2e4750b1..2c507d67291b 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -709,6 +709,68 @@ int xe_gt_resume(struct xe_gt *gt)
>  	return err;
>  }
>  
> +/**
> + * xe_gt_runtime_suspend - Helper for GT related Runtime PM suspend actions
> + * @xe: xe gt instance
> + *
> + * Return: 0 on success, negative error code on error.
> + */
> +int xe_gt_runtime_suspend(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +	int ret = 0;
> +
> +	if (xe->d3cold.allowed)
> +		return xe_gt_suspend(gt);
> +
> +	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (ret)
> +		return ret;

silent exit, without err() message, is it ok?

> +
> +	xe_pmu_suspend(gt);
> +
> +	ret = xe_uc_toggle_communication(&gt->uc, false);
> +
> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));

isn't XE_WARN_ON deprecated ?

> +
> +	if (ret)
> +		xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(ret));
> +	else
> +		xe_gt_info(gt, "suspended\n");
> +
> +	return ret;
> +}
> +
> +/**
> + * xe_gt_runtime_resume - Helper for GT related Runtime PM resume actions
> + * @xe: xe gt instance
> + *
> + * Return: 0 on success, negative error code on error.
> + */
> +int xe_gt_runtime_resume(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +	int ret = 0;
> +
> +	if (xe->d3cold.allowed)
> +		return xe_gt_resume(gt);
> +
> +	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (ret)
> +		return ret;

ditto

> +
> +	ret = xe_uc_toggle_communication(&gt->uc, true);
> +
> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));

ditto

> +
> +	if (ret)
> +		xe_gt_err(gt, "resume failed (%pe)\n", ERR_PTR(ret));
> +	else
> +		xe_gt_info(gt, "resumed\n");
> +
> +	return ret;
> +}
> +
>  struct xe_hw_engine *xe_gt_hw_engine(struct xe_gt *gt,
>  				     enum xe_engine_class class,
>  				     u16 instance, bool logical)
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index caded203a8a0..e6574e51004f 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -37,6 +37,8 @@ int xe_gt_record_default_lrcs(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_runtime_suspend(struct xe_gt *gt);
> +int xe_gt_runtime_resume(struct xe_gt *gt);
>  void xe_gt_reset_async(struct xe_gt *gt);
>  void xe_gt_sanitize(struct xe_gt *gt);
>  
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index e31a91cf311c..1cd46ab804fe 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -254,7 +254,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  	}
>  
>  	for_each_gt(gt, xe, id) {
> -		err = xe_gt_suspend(gt);
> +		err = xe_gt_runtime_suspend(gt);
>  		if (err)
>  			goto out;
>  	}
> @@ -304,7 +304,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_runtime_resume(gt);
>  
>  	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
>  		err = xe_bo_restore_user(xe);
> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index bf75c39d929d..62a5fa6e5362 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -225,6 +225,27 @@ static void uc_reset_wait(struct xe_uc *uc)
>  		goto again;
>  }
>  
> +/**
> + * xe_uc_toggle_communication - enable or disable uc communication
> + * @uc: The UC object

s/uc/uC
s/UC/uC

best if we can link that to some `uC`_ documentation (I hope there is
some already, right ?)

> + * @toggle: 0-disable, 1-enable

maybe @enable

also for bool we use true/false, not 1/0

> + *
> + * Return: 0 on success, negative error code on error.
> + */
> +int xe_uc_toggle_communication(struct xe_uc *uc, bool toggle)

there seems to be some inconsistency in the naming:

level:	function:	next:

uc	toggle_communication -> guc_enable(false) | guc_disable()
guc	enable|disable_communication -> ct_enable(reg) | ct_toggle(off)
guc_ct	enable|disable|toggle -> ct_control_toggle(on) | ct_toggle(off)

> +{
> +	/* GuC submission not enabled, nothing to do */
> +	if (!xe_device_uc_enabled(uc_to_xe(uc)))
> +		return 0;
> +
> +	if (toggle)
> +		return xe_guc_enable_communication(&uc->guc, false);
> +
> +	xe_guc_disable_communication(&uc->guc);
> +
> +	return 0;
> +}
> +
>  int xe_uc_suspend(struct xe_uc *uc)
>  {
>  	int ret;
> diff --git a/drivers/gpu/drm/xe/xe_uc.h b/drivers/gpu/drm/xe/xe_uc.h
> index 4109ae7028af..69d7dff0900e 100644
> --- a/drivers/gpu/drm/xe/xe_uc.h
> +++ b/drivers/gpu/drm/xe/xe_uc.h
> @@ -20,5 +20,5 @@ int xe_uc_stop(struct xe_uc *uc);
>  int xe_uc_start(struct xe_uc *uc);
>  int xe_uc_suspend(struct xe_uc *uc);
>  void xe_uc_sanitize(struct xe_uc *uc);
> -
> +int xe_uc_toggle_communication(struct xe_uc *uc, bool toggle);

please keep empty separation line

>  #endif