Re: [PATCH 2/4] drm/xe: Remove runtime argument from display s/r functions

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

Hi Lucas,

On Thu, 2024-09-05 at 11:44 -0500, Lucas De Marchi wrote:
> On Thu, Sep 05, 2024 at 05:00:50PM GMT, Maarten Lankhorst wrote:
> > The previous change ensures that pm_suspend is only called when
> > suspending or resuming. This ensures no further bugs like those
> > in the previous commit.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/xe/display/xe_display.c | 53 +++++++++++++++----------
> > drivers/gpu/drm/xe/display/xe_display.h |  8 ++--
> > drivers/gpu/drm/xe/xe_pm.c              |  6 +--
> > 3 files changed, 39 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > index c0e9aa7a274f1..33071ac3bc12d 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -310,18 +310,7 @@ static void xe_display_flush_cleanup_work(struct xe_device *xe)
> > }
> > 
> > /* TODO: System and runtime suspend/resume sequences will be sanitized as a follow-up. */
> 
> not sure what the TODO means... nuke? or at least remove "as a
> follow-up" and explain what exactly is missing/broken.
> 
> Anyway, unrelated to your patch.

"Todo" was added as part of https://patchwork.freedesktop.org/patch/610494/ based on input from
Imre. And commit log has..

"While xe_display_pm_suspend/resume() performs steps during runtime
suspend/resume that shouldn't happen, like suspending MST and they
are missing other steps like enabling DC9, this patchset is meant
to keep the current behavior wrt. these, leaving the corresponding
updates for a follow-up"

There is a task planned for refactoring these VLK-63235

BR
vinod

> 
> > -void xe_display_pm_runtime_suspend(struct xe_device *xe)
> > -{
> > -       if (!xe->info.probe_display)
> > -               return;
> > -
> > -       if (xe->d3cold.allowed)
> > -               xe_display_pm_suspend(xe, true);
> > -
> > -       intel_hpd_poll_enable(xe);
> > -}
> > -
> > -void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> > +static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> > {
> >         struct intel_display *display = &xe->display;
> >         bool s2idle = suspend_to_idle();
> > @@ -356,26 +345,31 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> >         intel_dmc_suspend(xe);
> > }
> > 
> > -void xe_display_pm_suspend_late(struct xe_device *xe)
> > +void xe_display_pm_suspend(struct xe_device *xe)
> > +{
> > +       __xe_display_pm_suspend(xe, false);
> > +}
> > +
> > +void xe_display_pm_runtime_suspend(struct xe_device *xe)
> > {
> > -       bool s2idle = suspend_to_idle();
> >         if (!xe->info.probe_display)
> >                 return;
> > 
> > -       intel_power_domains_suspend(xe, s2idle);
> > +       if (xe->d3cold.allowed)
> > +               __xe_display_pm_suspend(xe, true);
> > 
> > -       intel_display_power_suspend_late(xe);
> > +       intel_hpd_poll_enable(xe);
> > }
> > 
> > -void xe_display_pm_runtime_resume(struct xe_device *xe)
> > +void xe_display_pm_suspend_late(struct xe_device *xe)
> > {
> > +       bool s2idle = suspend_to_idle();
> >         if (!xe->info.probe_display)
> >                 return;
> > 
> > -       intel_hpd_poll_disable(xe);
> > +       intel_power_domains_suspend(xe, s2idle);
> > 
> > -       if (xe->d3cold.allowed)
> > -               xe_display_pm_resume(xe, true);
> > +       intel_display_power_suspend_late(xe);
> > }
> > 
> > void xe_display_pm_resume_early(struct xe_device *xe)
> > @@ -388,7 +382,7 @@ void xe_display_pm_resume_early(struct xe_device *xe)
> >         intel_power_domains_resume(xe);
> > }
> > 
> > -void xe_display_pm_resume(struct xe_device *xe, bool runtime)
> > +static void __xe_display_pm_resume(struct xe_device *xe, bool runtime)
> > {
> >         struct intel_display *display = &xe->display;
> > 
> > @@ -422,6 +416,23 @@ void xe_display_pm_resume(struct xe_device *xe, bool runtime)
> >         intel_power_domains_enable(xe);
> > }
> > 
> > +void xe_display_pm_resume(struct xe_device *xe)
> > +{
> > +       __xe_display_pm_resume(xe, false);
> > +}
> > +
> > +void xe_display_pm_runtime_resume(struct xe_device *xe)
> > +{
> > +       if (!xe->info.probe_display)
> > +               return;
> > +
> > +       intel_hpd_poll_disable(xe);
> > +
> > +       if (xe->d3cold.allowed)
> > +               __xe_display_pm_resume(xe, true);
> > +}
> > +
> > +
> > static void display_device_remove(struct drm_device *dev, void *arg)
> > {
> >         struct xe_device *xe = arg;
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> > index 53d727fd792b4..bed55fd26f304 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.h
> > +++ b/drivers/gpu/drm/xe/display/xe_display.h
> > @@ -34,10 +34,10 @@ void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir);
> > void xe_display_irq_reset(struct xe_device *xe);
> > void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt);
> > 
> > -void xe_display_pm_suspend(struct xe_device *xe, bool runtime);
> > +void xe_display_pm_suspend(struct xe_device *xe);
> > void xe_display_pm_suspend_late(struct xe_device *xe);
> > void xe_display_pm_resume_early(struct xe_device *xe);
> > -void xe_display_pm_resume(struct xe_device *xe, bool runtime);
> > +void xe_display_pm_resume(struct xe_device *xe);
> > void xe_display_pm_runtime_suspend(struct xe_device *xe);
> > void xe_display_pm_runtime_resume(struct xe_device *xe);
> > 
> > @@ -65,10 +65,10 @@ static inline void xe_display_irq_enable(struct xe_device *xe, u32
> > gu_misc_iir)
> > static inline void xe_display_irq_reset(struct xe_device *xe) {}
> > static inline void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt) {}
> > 
> > -static inline void xe_display_pm_suspend(struct xe_device *xe, bool runtime) {}
> > +static inline void xe_display_pm_suspend(struct xe_device *xe) {}
> > static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
> > static inline void xe_display_pm_resume_early(struct xe_device *xe) {}
> > -static inline void xe_display_pm_resume(struct xe_device *xe, bool runtime) {}
> > +static inline void xe_display_pm_resume(struct xe_device *xe) {}
> > static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
> > static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
> 
> Awesome! Let me offer you a virtual medal of "making our internal APIs
> suck less". Even if we still have that bool being passed around it's
> well hidden in drivers/gpu/drm/xe/display/xe_display.c.
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> thanks
> Lucas De Marchi
> 
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index 9b1204db12c3d..960ed50e3a41b 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -123,7 +123,7 @@ int xe_pm_suspend(struct xe_device *xe)
> >         for_each_gt(gt, xe, id)
> >                 xe_gt_suspend_prepare(gt);
> > 
> > -       xe_display_pm_suspend(xe, false);
> > +       xe_display_pm_suspend(xe);
> > 
> >         /* FIXME: Super racey... */
> >         err = xe_bo_evict_all(xe);
> > @@ -133,7 +133,7 @@ int xe_pm_suspend(struct xe_device *xe)
> >         for_each_gt(gt, xe, id) {
> >                 err = xe_gt_suspend(gt);
> >                 if (err) {
> > -                       xe_display_pm_resume(xe, false);
> > +                       xe_display_pm_resume(xe);
> >                         goto err;
> >                 }
> >         }
> > @@ -187,7 +187,7 @@ int xe_pm_resume(struct xe_device *xe)
> >         for_each_gt(gt, xe, id)
> >                 xe_gt_resume(gt);
> > 
> > -       xe_display_pm_resume(xe, false);
> > +       xe_display_pm_resume(xe);
> > 
> >         err = xe_bo_restore_user(xe);
> >         if (err)
> > -- 
> > 2.45.2
> >