On Fri, Jul 21, 2023 at 02:00:52AM -0400, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx> > > Sent: Friday, July 21, 2023 2:34 AM > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Gupta, Anshuman > > <anshuman.gupta@xxxxxxxxx> > > 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@xxxxxxxxx> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > --- > > 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 >
