Re: [Intel-gfx] [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed decision.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>