Re: [PATCH] drm/xe/display: handle HPD polling in display runtime suspend/resume

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 15 Aug 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Aug 15, 2024 at 08:25:00PM +0300, Vinod Govindapillai wrote:
>> In XE, display runtime suspend / resume routines are called only
>> if d3cold is allowed. This makes the driver unable to detect any
>> HPDs once the device goes into runtime suspend state in platforms
>> like LNL. Update the display runtime suspend / resume routines
>> to include HPD polling regardless of d3cold status.
>> 
>> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
>> ---
>>  drivers/gpu/drm/xe/display/xe_display.c | 20 ++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_pm.c              |  5 +++--
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>> index 982b9c5b440f..0cddf55351c8 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.c
>> +++ b/drivers/gpu/drm/xe/display/xe_display.c
>> @@ -294,6 +294,9 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
>>  	if (!xe->info.probe_display)
>>  		return;
>>  
>> +	if (!xe->d3cold.allowed)
>> +		goto enable_hpd_poll;
>> +
>>  	/*
>>  	 * We do a lot of poking in a lot of registers, make sure they work
>>  	 * properly.
>> @@ -308,6 +311,8 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
>>  	intel_dp_mst_suspend(xe);
>>  
>>  	intel_hpd_cancel_work(xe);
>> +	if (runtime)
>> +		intel_hpd_poll_enable(xe);
>
> If we need to do this, please do not use this function.
>
> Let's first spin out the runtime function to a separate function and then
> add the hpd poll only there. But also I don't believe we need anything
> of this call in d3hot case anyway. So we need a better refactor with
> minimal change.

Well, I'll keep preaching: We need to move all of this to the shared
display code, and axe it out of xe and i915 cores.

Maybe it doesn't belong in the bugfix at hand, but we need to do it
sooner rather than later.

BR,
Jani.



>
>>  
>>  	intel_encoder_suspend_all(&xe->display);
>>  
>> @@ -316,6 +321,12 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
>>  	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
>>  
>>  	intel_dmc_suspend(xe);
>> +
>> +	return;
>
> with return here your code below is not executed.
>
>> +
>> +enable_hpd_poll:
>> +	if (runtime)
>> +		intel_hpd_poll_enable(xe);
>>  }
>>  
>>  void xe_display_pm_suspend_late(struct xe_device *xe)
>> @@ -346,6 +357,9 @@ void xe_display_pm_resume(struct xe_device *xe, bool runtime)
>>  	if (!xe->info.probe_display)
>>  		return;
>>  
>> +	if (!xe->d3cold.allowed)
>> +		goto disable_hpd_poll;
>> +
>>  	intel_dmc_resume(xe);
>>  
>>  	if (has_display(xe))
>> @@ -368,6 +382,12 @@ void xe_display_pm_resume(struct xe_device *xe, bool runtime)
>>  	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false);
>>  
>>  	intel_power_domains_enable(xe);
>> +
>> +	return;
>
> another bogus place:
>
>> +
>> +disable_hpd_poll:
>> +	intel_hpd_init(xe);
>> +	intel_hpd_poll_disable(xe);
>>  }
>>  
>>  static void display_device_remove(struct drm_device *dev, void *arg)
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index 9f3c14fd9f33..2abfe70d2697 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -370,8 +370,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>  		err = xe_bo_evict_all(xe);
>>  		if (err)
>>  			goto out;
>> -		xe_display_pm_suspend(xe, true);
>>  	}
>> +	xe_display_pm_suspend(xe, true);
>
> ... please, let's not add extra unecessary calls to the d3hot path.
> If hpd_poll is the only missing thing we need to create a function
> only for that.
>
>>  
>>  	for_each_gt(gt, xe, id) {
>>  		err = xe_gt_suspend(gt);
>> @@ -431,11 +431,12 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>  		xe_gt_resume(gt);
>>  
>>  	if (xe->d3cold.allowed) {
>> -		xe_display_pm_resume(xe, true);
>>  		err = xe_bo_restore_user(xe);
>>  		if (err)
>>  			goto out;
>>  	}
>> +	xe_display_pm_resume(xe, true);
>> +
>>  out:
>>  	lock_map_release(&xe_pm_runtime_lockdep_map);
>>  	xe_pm_write_callback_task(xe, NULL);
>> -- 
>> 2.34.1
>> 

-- 
Jani Nikula, Intel