Re: [Intel-xe] [PATCH 2/2] RFC drm/xe: Disable GuC CT communication for D3Hot Transition

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

Hi Bala

Thanks for the review

On 8/23/2023 12:07 PM, Balasubramani Vivekanandan wrote:
> On 23.08.2023 10:39, Riana Tauro wrote:
>> During Runtime suspend, GuC is reset for both D0->D3hot/D3Cold
>> transistions. It is not necessary for GuC to reset for D0 -> D3hot,
>> only enable/disable ctb communication.
>>
>> Add a function that enables/disables CT communication when d3cold
>> is not allowed.
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt.c | 56 ++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt.h |  2 ++
>>   drivers/gpu/drm/xe/xe_pm.c | 10 +++++--
>>   drivers/gpu/drm/xe/xe_uc.c | 18 ++++++++++++
>>   drivers/gpu/drm/xe/xe_uc.h |  2 ++
>>   5 files changed, 85 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 13320af4ddd3..0d52621ce64d 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -676,6 +676,62 @@ int xe_gt_resume(struct xe_gt *gt)
>>   	return err;
>>   }
>>   
>> +int xe_gt_runtime_suspend(struct xe_gt *gt)
>> +{
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	int err;
>> +
>> +	if (xe->d3cold.allowed)
>> +		return xe_gt_suspend(gt);
>> +
>> +	err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>> +	if (err)
>> +		return err;
>> +
>> +	err = xe_uc_disable_communication(&gt->uc);
>> +	if (err)
>> +		goto err_force_wake;
>> +
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +	xe_gt_info(gt, "suspended\n");
>> +
>> +	return 0;
>> +
>> +err_force_wake:
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +	xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(err));
>> +
>> +	return err;
> 
> Can this be simplified to
> 
> =====
> 	err = xe_uc_disable_communication(&gt->uc);
> 	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
> 	if (err)
> 		xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(err));
> 	else
> 		xe_gt_info(gt, "suspended\n");
> 
> 	return err;
> ====
Followed the pattern in the file.
Can be simplified
Will make changes in the next rev
>> +}
>> +
>> +int xe_gt_runtime_resume(struct xe_gt *gt)
>> +{
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	int err;
>> +
>> +	if (xe->d3cold.allowed)
>> +		return xe_gt_resume(gt);
>> +
>> +	err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>> +	if (err)
>> +		return err;
>> +
>> +	err = xe_uc_resume(&gt->uc);
>> +	if (err)
>> +		goto err_force_wake;
>> +
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +	xe_gt_info(gt, "resumed\n");
>> +
>> +	return 0;
>> +
>> +err_force_wake:
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +	xe_gt_err(gt, "resume failed (%pe)\n", ERR_PTR(err));
>> +
>> +	return err;
> 
> Same comment as above.
>> +}
>> +
>>   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 0f06d8304e17..6bc01bb45fc2 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -245,7 +245,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;
>>   	}
>> @@ -294,14 +294,18 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>   
>>   	xe_irq_resume(xe);
>>   
>> -	for_each_gt(gt, xe, id)
>> -		xe_gt_resume(gt);
>> +	for_each_gt(gt, xe, id) {
>> +		err = xe_gt_runtime_resume(gt);
>> +		if (err)
>> +			goto out;
>> +	}
>>   
>>   	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
>>   		err = xe_bo_restore_user(xe);
>>   		if (err)
>>   			goto out;
>>   	}
>> +
>>   out:
>>   	lock_map_release(&xe_device_mem_access_lockdep_map);
>>   	xe_pm_write_callback_task(xe, NULL);
>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index addd6f2681b9..b5b53c8c3edb 100644
>> --- a/drivers/gpu/drm/xe/xe_uc.c
>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>> @@ -216,6 +216,15 @@ static void uc_reset_wait(struct xe_uc *uc)
>>   		goto again;
>>   }
>>   
>> +int xe_uc_disable_communication(struct xe_uc *uc)
>> +{
>> +	/* GuC submission not enabled, nothing to do */
>> +	if (!xe_device_guc_submission_enabled(uc_to_xe(uc)))
>> +		return 0;
>> +
>> +	return xe_guc_disable_communication(&uc->guc);
>> +}
>> +
>>   int xe_uc_suspend(struct xe_uc *uc)
>>   {
>>   	int ret;
>> @@ -232,3 +241,12 @@ int xe_uc_suspend(struct xe_uc *uc)
>>   
>>   	return xe_guc_suspend(&uc->guc);
>>   }
>> +
>> +int xe_uc_resume(struct xe_uc *uc)
> 
> why not xe_uc_enable_communication?
Added a generic name to allow adding any other functions.


Thanks
Riana
> 
> Regards,
> Bala
> 
>> +{
>> +	/* GuC submission not enabled, nothing to do */
>> +	if (!xe_device_guc_submission_enabled(uc_to_xe(uc)))
>> +		return 0;
>> +
>> +	return xe_guc_enable_communication(&uc->guc);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_uc.h b/drivers/gpu/drm/xe/xe_uc.h
>> index 42219b361df5..29bd692d8800 100644
>> --- a/drivers/gpu/drm/xe/xe_uc.h
>> +++ b/drivers/gpu/drm/xe/xe_uc.h
>> @@ -12,8 +12,10 @@ int xe_uc_init(struct xe_uc *uc);
>>   int xe_uc_init_hwconfig(struct xe_uc *uc);
>>   int xe_uc_init_post_hwconfig(struct xe_uc *uc);
>>   int xe_uc_init_hw(struct xe_uc *uc);
>> +int xe_uc_disable_communication(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_resume(struct xe_uc *uc);
>>   void xe_uc_stop_prepare(struct xe_uc *uc);
>>   int xe_uc_stop(struct xe_uc *uc);
>>   int xe_uc_start(struct xe_uc *uc);
>> -- 
>> 2.40.0
>>