| Message ID | 20230720210353.214385-3-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing. | expand |
> -----Original Message----- > From: Vivi, Rodrigo <rodrigo.vivi@intel.com> > Sent: Friday, July 21, 2023 2:34 AM > To: intel-gfx@lists.freedesktop.org > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman > <anshuman.gupta@intel.com> > Subject: [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed > decision. > > According to Documentation/power/runtime_pm.txt: I tried to fix runtime idle https://patchwork.freedesktop.org/patch/543024/?series=119467&rev=1 But forgot to CC to you. Anyway some comment below, > > int pm_runtime_put(struct device *dev); > - decrement the device's usage counter; if the result is 0 then run > pm_request_idle(dev) and return its result > > int pm_runtime_put_autosuspend(struct device *dev); > - decrement the device's usage counter; if the result is 0 then run > pm_request_autosuspend(dev) and return its result > > We need to ensure that the idle function is called before suspending so we > take the right d3cold.allowed decision and respect the values set on > vram_d3cold_threshold sysfs. So we need pm_runtime_put() instead of > pm_runtime_put_autosuspend(). > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/xe/xe_pm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > index a6459df2599e..73bcb76c2d42 100644 > --- a/drivers/gpu/drm/xe/xe_pm.c > +++ b/drivers/gpu/drm/xe/xe_pm.c > @@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe) > pm_runtime_set_active(dev); > pm_runtime_allow(dev); > pm_runtime_mark_last_busy(dev); I have not thought of using last_busy() here in _put(). If we mark last_busy during _put then pm core auto-suspend timer will start ticking from _put(). Theoretically that can lead to idle() and runtime_suspend() call to race with each other ? [1] [1] Documentation/power/runtime_pm.txt (1) The callbacks are mutually exclusive (e.g. it is forbidden to execute ->runtime_suspend() in parallel with ->runtime_resume() or with another instance of ->runtime_suspend() for the same device) with the exception that ->runtime_suspend() or ->runtime_resume() can be executed in parallel with ->runtime_idle() (although ->runtime_idle() will not be started while any of the other callbacks is being executed for the same device). Thanks, Anshuman Gupta. > - pm_runtime_put_autosuspend(dev); > + pm_runtime_put(dev); We need to fix this in intel_runtime_pm_put() compat-i915-headers as well. > } > > void xe_pm_init(struct xe_device *xe) > @@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe) int > xe_pm_runtime_put(struct xe_device *xe) { > pm_runtime_mark_last_busy(xe->drm.dev); > - return pm_runtime_put_autosuspend(xe->drm.dev); > + return pm_runtime_put(xe->drm.dev); > } > > int xe_pm_runtime_get_if_active(struct xe_device *xe) > -- > 2.41.0
On Fri, Jul 21, 2023 at 02:00:52AM -0400, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com> > > Sent: Friday, July 21, 2023 2:34 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman > > <anshuman.gupta@intel.com> > > Subject: [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed > > decision. > > > > According to Documentation/power/runtime_pm.txt: > I tried to fix runtime idle https://patchwork.freedesktop.org/patch/543024/?series=119467&rev=1 > But forgot to CC to you. I'm really sorry for having missed that. 2 comments on your version: 1. it forgets to remove the autosuspend from the init, so on the very first entry at driver load it may miss the idle call. 2. I don't like the way other drivers are doing with idle. The rpm infra expects 0 return to then call the suspend. I really don't understand because I few drivers decided to workaround that and return 1 and call the autosuspend themselves from within the idle. > Anyway some comment below, > > > > > int pm_runtime_put(struct device *dev); > > - decrement the device's usage counter; if the result is 0 then run > > pm_request_idle(dev) and return its result > > > > int pm_runtime_put_autosuspend(struct device *dev); > > - decrement the device's usage counter; if the result is 0 then run > > pm_request_autosuspend(dev) and return its result > > > > We need to ensure that the idle function is called before suspending so we > > take the right d3cold.allowed decision and respect the values set on > > vram_d3cold_threshold sysfs. So we need pm_runtime_put() instead of > > pm_runtime_put_autosuspend(). > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/xe/xe_pm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > > index a6459df2599e..73bcb76c2d42 100644 > > --- a/drivers/gpu/drm/xe/xe_pm.c > > +++ b/drivers/gpu/drm/xe/xe_pm.c > > @@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe) > > pm_runtime_set_active(dev); > > pm_runtime_allow(dev); > > pm_runtime_mark_last_busy(dev); > I have not thought of using last_busy() here in _put(). > If we mark last_busy during _put then pm core auto-suspend timer will start ticking from _put(). > Theoretically that can lead to idle() and runtime_suspend() call to race with each other ? [1] that shouldn't happen if you are using the rpm as designed and returning 0 from idle instead of 1 and autosuspend. > [1] Documentation/power/runtime_pm.txt > (1) The callbacks are mutually exclusive (e.g. it is forbidden to execute > ->runtime_suspend() in parallel with ->runtime_resume() or with another > instance of ->runtime_suspend() for the same device) with the exception that > ->runtime_suspend() or ->runtime_resume() can be executed in parallel with > ->runtime_idle() (although ->runtime_idle() will not be started while any > of the other callbacks is being executed for the same device). > Thanks, > Anshuman Gupta. > > - pm_runtime_put_autosuspend(dev); > > + pm_runtime_put(dev); > We need to fix this in intel_runtime_pm_put() compat-i915-headers as well. I can't see that... I see the compat headers calling the xe_ variants so we should be covered from here. what exactly am I missing? > > } > > > > void xe_pm_init(struct xe_device *xe) > > @@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe) int > > xe_pm_runtime_put(struct xe_device *xe) { > > pm_runtime_mark_last_busy(xe->drm.dev); > > - return pm_runtime_put_autosuspend(xe->drm.dev); > > + return pm_runtime_put(xe->drm.dev); > > } > > > > int xe_pm_runtime_get_if_active(struct xe_device *xe) > > -- > > 2.41.0 >
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c index a6459df2599e..73bcb76c2d42 100644 --- a/drivers/gpu/drm/xe/xe_pm.c +++ b/drivers/gpu/drm/xe/xe_pm.c @@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe) pm_runtime_set_active(dev); pm_runtime_allow(dev); pm_runtime_mark_last_busy(dev); - pm_runtime_put_autosuspend(dev); + pm_runtime_put(dev); } void xe_pm_init(struct xe_device *xe) @@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe) int xe_pm_runtime_put(struct xe_device *xe) { pm_runtime_mark_last_busy(xe->drm.dev); - return pm_runtime_put_autosuspend(xe->drm.dev); + return pm_runtime_put(xe->drm.dev); } int xe_pm_runtime_get_if_active(struct xe_device *xe)
According to Documentation/power/runtime_pm.txt: int pm_runtime_put(struct device *dev); - decrement the device's usage counter; if the result is 0 then run pm_request_idle(dev) and return its result int pm_runtime_put_autosuspend(struct device *dev); - decrement the device's usage counter; if the result is 0 then run pm_request_autosuspend(dev) and return its result We need to ensure that the idle function is called before suspending so we take the right d3cold.allowed decision and respect the values set on vram_d3cold_threshold sysfs. So we need pm_runtime_put() instead of pm_runtime_put_autosuspend(). Cc: Anshuman Gupta <anshuman.gupta@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/xe/xe_pm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)