Re: [Intel-gfx] [PATCH v2 9/9] drm/i915/rpm: d3cold Policy

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Thursday, June 16, 2022 7:58 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v2 9/9] drm/i915/rpm: d3cold Policy
> 
> On Thu, 16 Jun 2022, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off
> > zero watt and d3cold-VRAM Self Refresh.
> > i915 requires to evict the lmem objects to smem in order to support
> > d3cold-Off.
> >
> > If gfx root port is not capable of sending PME from d3cold then i915
> > don't need to program d3cold-off/d3cold-vram_sr sequence.
> >
> > FIXME: Eviction of lmem objects in case of D3Cold off is wip.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 27 ++++++++++++++++++++++++---
> > drivers/gpu/drm/i915/i915_params.c |  4 ++++
> > drivers/gpu/drm/i915/i915_params.h |  3 ++-
> >  3 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index fcff5f3fe05e..aef4b17efdbe 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1560,15 +1560,36 @@ static int i915_pm_restore(struct device
> > *kdev)  static int intel_runtime_idle(struct device *kdev)  {
> >  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> > +	u64 lmem_total = to_gt(dev_priv)->lmem->total;
> > +	u64 lmem_avail = to_gt(dev_priv)->lmem->avail;
> > +	u64 lmem_used = lmem_total - lmem_avail;
> > +	struct pci_dev *root_pdev;
> >  	int ret = 1;
> >
> > -	if (!HAS_LMEM_SR(dev_priv)) {
> > -		/*TODO: Prepare for D3Cold-Off */
> > +	root_pdev = pcie_find_root_port(pdev);
> > +	if (!root_pdev)
> > +		goto out;
> > +
> > +	if (!pci_pme_capable(root_pdev, PCI_D3cold))
> >  		goto out;
> > -	}
> >
> >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> >
> > +	if (lmem_used < dev_priv->params.d3cold_sr_lmem_threshold  * 1024 *
> 1024) {
> > +		drm_dbg(&dev_priv->drm, "Prepare for D3Cold off\n");
> > +		pci_d3cold_enable(root_pdev);
> > +		/* FIXME: Eviction of lmem objects and guc reset is wip */
> > +		intel_pm_vram_sr(dev_priv, false);
> > +		enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > +		goto out;
> > +	} else if (!HAS_LMEM_SR(dev_priv)) {
> > +		/* Disable D3Cold to reduce the eviction latency */
> > +		pci_d3cold_disable(root_pdev);
> > +		enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > +		goto out;
> > +	}
> 
> This is *way* too low level code for such high level function. This needs to be
> abstracted better.
> 
> > +
> >  	ret = intel_pm_vram_sr(dev_priv, true);
> >  	if (!ret)
> >  		drm_dbg(&dev_priv->drm, "VRAM Self Refresh enabled\n"); diff
> --git
> > a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 701fbc98afa0..6c6b3c372d4d 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -197,6 +197,10 @@ i915_param_named(enable_gvt, bool, 0400,
> >  	"Enable support for Intel GVT-g graphics virtualization host
> > support(default:false)");  #endif
> >
> > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0400,
> > +	"Enable Vidoe RAM Self refresh when size of lmem is greater to this
> threshold. "
> > +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> > +
> >  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
> >  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
> >  			"Default request/fence/batch buffer expiration
> timeout."); diff
> > --git a/drivers/gpu/drm/i915/i915_params.h
> > b/drivers/gpu/drm/i915/i915_params.h
> > index b5e7ea45d191..28f20ebaf41f 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -83,7 +83,8 @@ struct drm_printer;
> >  	param(bool, verbose_state_checks, true, 0) \
> >  	param(bool, nuclear_pageflip, false, 0400) \
> >  	param(bool, enable_dp_mst, true, 0600) \
> > -	param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ?
> 0400 : 0)
> > +	param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ?
> 0400 : 0) \
> > +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
> 
> What's the point of the parameter?
We want a configurable option to choose an optimum lmem usages threshold on which, i915
can choose lmem self-refresh. This threshold value would require some profiling as well. 
That is the reason this threshold kept as module param.  
> 
> Also, please read the comment /* leave bools at the end to not create holes */
> above.
Thanks for review comment , I will re order the module param.
Regards ,
Anshuman Gupta.
> 
> 
> BR,
> Jani.
> 
> 
> >
> >  #define MEMBER(T, member, ...) T member;  struct i915_params {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center