[Intel-xe] [PATCH v2 2/2] drm/xe: Toggle GuC CT communication for D3Hot Transition
Riana Tauro
riana.tauro at intel.com
Thu Oct 12 11:25:34 UTC 2023
Hi Michal
On 10/12/2023 4:14 PM, Michal Wajdeczko wrote:
>
>
> 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?
suspend will fail and i don't see err message being used for
forcewake in xe driver. I think error message would be unncessary
>
>> +
>> + xe_pmu_suspend(gt);
>> +
>> + ret = xe_uc_toggle_communication(>->uc, false);
>> +
>> + XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>
> isn't XE_WARN_ON deprecated ?
There are many occurences of XE_WARN_ON in the same file as well
as the driver. Could you point me to the patchwork where it mentions
about the deprecation?
>
>> +
>> + 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
I'll fix this. Returning err is not handled in pm_runtime_resume.
>
>> +
>> + ret = xe_uc_toggle_communication(>->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
Will fix this
>
> 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
will add true/false
>
>> + *
>> + * 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()
The first one seems okay. Why is there a inconsistency?
> guc enable|disable_communication -> ct_enable(reg) | ct_toggle(off > guc_ct enable|disable|toggle -> ct_control_toggle(on) | ct_toggle(off)
Agree toggle and enable/disable communication does seem similar.
But i couldn't think of another name for only toggling ct->enable
Thanks
Riana
>
>> +{
>> + /* 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
More information about the Intel-xe
mailing list