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

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

Hi,

Thanks for the review..

On Thu, 2024-08-15 at 14:27 -0400, Rodrigo Vivi 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.
> 

Okay! Thanks for pointing out about the d3hot case! I will work on having a separate runtime
function. 

The intention was to have minimal change. 
For LNL, d3cold.allowed is alwayas false -> Then we will execute "goto part" - the HPD poll.
For BMG, execute the main part only if  d3cold.allowed = true. But missed the d3hot.

But as you suggested having a seprate runtime function might be much simpler and minimal. 

But are we expected to call other suspend routines than this HPD poll from the display runtime
suspend. The i915 part for runtime suspend is pretty different from the xe counterpart. So is there
are any specific document I should follow?


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

Just to clarify, the below part is expected only if xe->d3cold.allowed = false based on the goto
above. For initial try outs I  wanted to have minimum changes to this function - to test on both LNL
and BMG.

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

Ack.

BR
Vinod

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