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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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.

>  
>  	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
>