| Message ID | 20240806085513.3051107-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe/pm: Change HPD to polling on runtime suspend | expand |
Hi Arun, kernel test robot noticed the following build errors: [auto build test ERROR on drm-xe/drm-xe-next] [also build test ERROR on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.11-rc2 next-20240806] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Arun-R-Murthy/drm-xe-pm-Change-HPD-to-polling-on-runtime-suspend/20240806-175155 base: https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next patch link: https://lore.kernel.org/r/20240806085513.3051107-1-arun.r.murthy%40intel.com patch subject: [PATCH] drm/xe/pm: Change HPD to polling on runtime suspend config: i386-buildonly-randconfig-003-20240806 (https://download.01.org/0day-ci/archive/20240806/202408062332.8Cl1FjhB-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240806/202408062332.8Cl1FjhB-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408062332.8Cl1FjhB-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/gpu/drm/xe/xe_pm.c:25:10: fatal error: intel_hotplug.h: No such file or directory 25 | #include "intel_hotplug.h" | ^~~~~~~~~~~~~~~~~ compilation terminated. vim +25 drivers/gpu/drm/xe/xe_pm.c 12 13 #include "display/xe_display.h" 14 #include "xe_bo.h" 15 #include "xe_bo_evict.h" 16 #include "xe_device.h" 17 #include "xe_device_sysfs.h" 18 #include "xe_ggtt.h" 19 #include "xe_gt.h" 20 #include "xe_guc.h" 21 #include "xe_irq.h" 22 #include "xe_pcode.h" 23 #include "xe_trace.h" 24 #include "xe_wa.h" > 25 #include "intel_hotplug.h" 26
On Tue, Aug 06, 2024 at 02:25:13PM +0530, Arun R Murthy wrote: > HPD is interrupt based and on runtime suspend change it to polling as > HPD is not a wakeup event. A worker thread is scheduled for doing this > polling and it keeps polling for HPD live status on an internval of 10s. > On runtime resume disable polling and fallback to interrupt mechanism. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/xe/xe_pm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > index 9f3c14fd9f33..d952b06ebfb4 100644 > --- a/drivers/gpu/drm/xe/xe_pm.c > +++ b/drivers/gpu/drm/xe/xe_pm.c > @@ -22,6 +22,7 @@ > #include "xe_pcode.h" > #include "xe_trace.h" > #include "xe_wa.h" > +#include "intel_hotplug.h" > > /** > * DOC: Xe Power Management > @@ -378,6 +379,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe) > if (err) > goto out; > } > + /* Enable hpd polling on runtime suspend */ > + intel_hpd_poll_enable(xe); > > xe_irq_suspend(xe); > > @@ -427,6 +430,9 @@ int xe_pm_runtime_resume(struct xe_device *xe) > > xe_irq_resume(xe); > > + /* Disable hpd polling on runtime resume */ > + intel_hpd_poll_disable(xe); This should be handled in xe_display_pm_resume() (likewise enabling polling in xe_display_pm_suspend()), in fact it's already called there. Also this is missing the xe->info.enable_display check and polling shouldn't be enabled when system (vs. runtime) suspending. Not sure why the display is suspended/resumed only if d3cold is allowed, for many of the display s/r steps at least it doesn't make sense to me. For now moving the d3cold check within xe_display_pm_suspend()/resume() and enabling/disabling polling regardless of that flag would be the simplest imo. > + > for_each_gt(gt, xe, id) > xe_gt_resume(gt); > > -- > 2.25.1 >
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Tuesday, August 6, 2024 10:11 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/xe/pm: Change HPD to polling on runtime suspend > > On Tue, Aug 06, 2024 at 02:25:13PM +0530, Arun R Murthy wrote: > > HPD is interrupt based and on runtime suspend change it to polling as > > HPD is not a wakeup event. A worker thread is scheduled for doing this > > polling and it keeps polling for HPD live status on an internval of 10s. > > On runtime resume disable polling and fallback to interrupt mechanism. > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/xe/xe_pm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > > index 9f3c14fd9f33..d952b06ebfb4 100644 > > --- a/drivers/gpu/drm/xe/xe_pm.c > > +++ b/drivers/gpu/drm/xe/xe_pm.c > > @@ -22,6 +22,7 @@ > > #include "xe_pcode.h" > > #include "xe_trace.h" > > #include "xe_wa.h" > > +#include "intel_hotplug.h" > > > > /** > > * DOC: Xe Power Management > > @@ -378,6 +379,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe) > > if (err) > > goto out; > > } > > + /* Enable hpd polling on runtime suspend */ > > + intel_hpd_poll_enable(xe); > > > > xe_irq_suspend(xe); > > > > @@ -427,6 +430,9 @@ int xe_pm_runtime_resume(struct xe_device *xe) > > > > xe_irq_resume(xe); > > > > + /* Disable hpd polling on runtime resume */ > > + intel_hpd_poll_disable(xe); > > This should be handled in xe_display_pm_resume() (likewise enabling polling in > xe_display_pm_suspend()), in fact it's already called there. In xe_display_pm_suspend we have drm_kms_helper_poll_disable and it disables output polling. Moreover we don't have the functions defined for enable/disable_hpd defined in connector_ops which is referenced by drm_kms_helper_poll_enable/disable. What we need is on runtime_suspend the poll work should be enabled so that hotplug can be detected and can come out of runtime_suspend. The same poll work is not required on system suspend. > Also this is missing the xe->info.enable_display check and polling shouldn't be > enabled when system (vs. runtime) suspending. Probably HAS_DISPLAY can be added before calling this intel_hpd_poll_enable. Thanks and Regards, Arun R Murthy -------------------- > > Not sure why the display is suspended/resumed only if d3cold is allowed, for > many of the display s/r steps at least it doesn't make sense to me. > For now moving the d3cold check within > xe_display_pm_suspend()/resume() and enabling/disabling polling regardless of > that flag would be the simplest imo. > > > + > > for_each_gt(gt, xe, id) > > xe_gt_resume(gt); > > > > -- > > 2.25.1 > >
On Wed, Aug 07, 2024 at 12:15:33PM +0300, Murthy, Arun R wrote: > > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Tuesday, August 6, 2024 10:11 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH] drm/xe/pm: Change HPD to polling on runtime suspend > > > > On Tue, Aug 06, 2024 at 02:25:13PM +0530, Arun R Murthy wrote: > > > HPD is interrupt based and on runtime suspend change it to polling as > > > HPD is not a wakeup event. A worker thread is scheduled for doing this > > > polling and it keeps polling for HPD live status on an internval of 10s. > > > On runtime resume disable polling and fallback to interrupt mechanism. > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > > --- > > > drivers/gpu/drm/xe/xe_pm.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > > > index 9f3c14fd9f33..d952b06ebfb4 100644 > > > --- a/drivers/gpu/drm/xe/xe_pm.c > > > +++ b/drivers/gpu/drm/xe/xe_pm.c > > > @@ -22,6 +22,7 @@ > > > #include "xe_pcode.h" > > > #include "xe_trace.h" > > > #include "xe_wa.h" > > > +#include "intel_hotplug.h" > > > > > > /** > > > * DOC: Xe Power Management > > > @@ -378,6 +379,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe) > > > if (err) > > > goto out; > > > } > > > + /* Enable hpd polling on runtime suspend */ > > > + intel_hpd_poll_enable(xe); > > > > > > xe_irq_suspend(xe); > > > > > > @@ -427,6 +430,9 @@ int xe_pm_runtime_resume(struct xe_device *xe) > > > > > > xe_irq_resume(xe); > > > > > > + /* Disable hpd polling on runtime resume */ > > > + intel_hpd_poll_disable(xe); > > > > This should be handled in xe_display_pm_resume() (likewise enabling polling in > > xe_display_pm_suspend()), in fact it's already called there. > > In xe_display_pm_suspend we have drm_kms_helper_poll_disable and it disables > output polling. What I meant is that intel_hpd_poll_disable() is already called in xe_display_pm_resume(), which you would call now twice during runtime resume if d3cold is allowed. drm_kms_helper_poll_disable() in xe_display_pm_suspend() should be called only during system suspend, which is another thing to fix there. > Moreover we don't have the functions defined for enable/disable_hpd > defined in connector_ops which is referenced by > drm_kms_helper_poll_enable/disable. > > What we need is on runtime_suspend the poll work should be > enabled so that hotplug can be detected and can come out of runtime_suspend. > The same poll work is not required on system suspend. All the display related suspend/resume handling is done already in xe_display_pm_suspend()/resume() or in xe_display.c in general, both for runtime and system suspend/resume. The reason for that is to keep the display specific parts separate from the generic/higher-level device suspend/resume handling in xe_pm.c. New display specific things added should keep this separation. > > Also this is missing the xe->info.enable_display check and polling shouldn't be > > enabled when system (vs. runtime) suspending. > Probably HAS_DISPLAY can be added before calling this intel_hpd_poll_enable. > > Thanks and Regards, > Arun R Murthy > -------------------- > > > > Not sure why the display is suspended/resumed only if d3cold is allowed, for > > many of the display s/r steps at least it doesn't make sense to me. > > For now moving the d3cold check within > > xe_display_pm_suspend()/resume() and enabling/disabling polling regardless of > > that flag would be the simplest imo. > > > > > + > > > for_each_gt(gt, xe, id) > > > xe_gt_resume(gt); > > > > > > -- > > > 2.25.1 > > >
On Wed, 07 Aug 2024, Imre Deak <imre.deak@intel.com> wrote: > On Wed, Aug 07, 2024 at 12:15:33PM +0300, Murthy, Arun R wrote: >> >> > -----Original Message----- >> > From: Deak, Imre <imre.deak@intel.com> >> > Sent: Tuesday, August 6, 2024 10:11 PM >> > To: Murthy, Arun R <arun.r.murthy@intel.com> >> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org >> > Subject: Re: [PATCH] drm/xe/pm: Change HPD to polling on runtime suspend >> > >> > On Tue, Aug 06, 2024 at 02:25:13PM +0530, Arun R Murthy wrote: >> > > HPD is interrupt based and on runtime suspend change it to polling as >> > > HPD is not a wakeup event. A worker thread is scheduled for doing this >> > > polling and it keeps polling for HPD live status on an internval of 10s. >> > > On runtime resume disable polling and fallback to interrupt mechanism. >> > > >> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> > > --- >> > > drivers/gpu/drm/xe/xe_pm.c | 6 ++++++ >> > > 1 file changed, 6 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c >> > > index 9f3c14fd9f33..d952b06ebfb4 100644 >> > > --- a/drivers/gpu/drm/xe/xe_pm.c >> > > +++ b/drivers/gpu/drm/xe/xe_pm.c >> > > @@ -22,6 +22,7 @@ >> > > #include "xe_pcode.h" >> > > #include "xe_trace.h" >> > > #include "xe_wa.h" >> > > +#include "intel_hotplug.h" >> > > >> > > /** >> > > * DOC: Xe Power Management >> > > @@ -378,6 +379,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe) >> > > if (err) >> > > goto out; >> > > } >> > > + /* Enable hpd polling on runtime suspend */ >> > > + intel_hpd_poll_enable(xe); >> > > >> > > xe_irq_suspend(xe); >> > > >> > > @@ -427,6 +430,9 @@ int xe_pm_runtime_resume(struct xe_device *xe) >> > > >> > > xe_irq_resume(xe); >> > > >> > > + /* Disable hpd polling on runtime resume */ >> > > + intel_hpd_poll_disable(xe); >> > >> > This should be handled in xe_display_pm_resume() (likewise enabling polling in >> > xe_display_pm_suspend()), in fact it's already called there. >> >> In xe_display_pm_suspend we have drm_kms_helper_poll_disable and it disables >> output polling. > > What I meant is that intel_hpd_poll_disable() is already called in > xe_display_pm_resume(), which you would call now twice during runtime > resume if d3cold is allowed. > > drm_kms_helper_poll_disable() in xe_display_pm_suspend() should be > called only during system suspend, which is another thing to fix there. > >> Moreover we don't have the functions defined for enable/disable_hpd >> defined in connector_ops which is referenced by >> drm_kms_helper_poll_enable/disable. >> >> What we need is on runtime_suspend the poll work should be >> enabled so that hotplug can be detected and can come out of runtime_suspend. >> The same poll work is not required on system suspend. > > All the display related suspend/resume handling is done already in > xe_display_pm_suspend()/resume() or in xe_display.c in general, both for > runtime and system suspend/resume. The reason for that is to keep the > display specific parts separate from the generic/higher-level device > suspend/resume handling in xe_pm.c. New display specific things added > should keep this separation. I'll keep repeating here too: We should aim to move all of this under i915/display, and not duplicate it in i915 and xe specific code. BR, Jani. > >> > Also this is missing the xe->info.enable_display check and polling shouldn't be >> > enabled when system (vs. runtime) suspending. >> Probably HAS_DISPLAY can be added before calling this intel_hpd_poll_enable. >> >> Thanks and Regards, >> Arun R Murthy >> -------------------- >> > >> > Not sure why the display is suspended/resumed only if d3cold is allowed, for >> > many of the display s/r steps at least it doesn't make sense to me. >> > For now moving the d3cold check within >> > xe_display_pm_suspend()/resume() and enabling/disabling polling regardless of >> > that flag would be the simplest imo. >> > >> > > + >> > > for_each_gt(gt, xe, id) >> > > xe_gt_resume(gt); >> > > >> > > -- >> > > 2.25.1 >> > >
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c index 9f3c14fd9f33..d952b06ebfb4 100644 --- a/drivers/gpu/drm/xe/xe_pm.c +++ b/drivers/gpu/drm/xe/xe_pm.c @@ -22,6 +22,7 @@ #include "xe_pcode.h" #include "xe_trace.h" #include "xe_wa.h" +#include "intel_hotplug.h" /** * DOC: Xe Power Management @@ -378,6 +379,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe) if (err) goto out; } + /* Enable hpd polling on runtime suspend */ + intel_hpd_poll_enable(xe); xe_irq_suspend(xe); @@ -427,6 +430,9 @@ int xe_pm_runtime_resume(struct xe_device *xe) xe_irq_resume(xe); + /* Disable hpd polling on runtime resume */ + intel_hpd_poll_disable(xe); + for_each_gt(gt, xe, id) xe_gt_resume(gt);
HPD is interrupt based and on runtime suspend change it to polling as HPD is not a wakeup event. A worker thread is scheduled for doing this polling and it keeps polling for HPD live status on an internval of 10s. On runtime resume disable polling and fallback to interrupt mechanism. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/xe/xe_pm.c | 6 ++++++ 1 file changed, 6 insertions(+)