[PATCH v2 0/3] Resume time optimisation

[PATCH v2 0/3] Resume time optimisation

[David Weinehall]

This patch series aims to reduce the time we spend in
i915_gem_restore_gtt_mappings() on resume.

Now that we're getting the resume times down a bit this function
started to stand out.

David Weinehall (3):
  drm/i915: Cleanup i915_gem_restore_gtt_mappings()
  drm/i915: Take runtime pm in i915_gem_resume()
  drm/i915: optimise intel_runtime_pm_{get,put}

 drivers/gpu/drm/i915/i915_gem.c         |  2 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 17 +++++++++++------
 drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
 3 files changed, 25 insertions(+), 7 deletions(-)

-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm/i915: Cleanup i915_gem_restore_gtt_mappings()

[David Weinehall]

On resume we unbind+bind our VMA-mappings. This patch simplifies
this a bit by introducing a restore_vma() helper.  As a nice side-effect
this also makes the resume callgraph self-documenting.

v2: move the helper to i915_gem_gtt.c since it's only used by
    i915_gem_restore_gtt_mappings(), rename to restore_vma(),
    and make static (Chris)

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b4bde1452f2a..2711044e3bf2 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3277,6 +3277,16 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static bool restore_vma(struct i915_vma *vma)
+{
+	if (i915_vma_is_pinned(vma)) {
+		WARN_ON(i915_vma_bind(vma, vma->obj->cache_level, PIN_UPDATE));
+		return true;
+	} else {
+		WARN_ON(i915_vma_unbind(vma));
+		return false;
+	}
+}
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
@@ -3299,12 +3309,7 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
 			if (vma->vm != &ggtt->base)
 				continue;
 
-			if (!i915_vma_unbind(vma))
-				continue;
-
-			WARN_ON(i915_vma_bind(vma, obj->cache_level,
-					      PIN_UPDATE));
-			ggtt_bound = true;
+			ggtt_bound |= restore_vma(vma);
 		}
 
 		if (ggtt_bound)
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/3] drm/i915: Take runtime pm in i915_gem_resume()

[David Weinehall]

In i915_gem_resume(), before calling i915_gem_restore_gtt_mappings(),
we want to take the runtime PM wakelocks for the entire duration
instead of many short lived wakelocks around each access to the GGTT.
We will take it in i915_gem_resume() as any other GEM operation
is likely to require the gT powerwell as well.

v2: Better explanation of the patch (Chris)

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
CC: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d5b7723bb028..da6e949906ca 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4240,6 +4240,7 @@ void i915_gem_resume(struct drm_device *dev)
 
 	WARN_ON(dev_priv->gt.awake);
 
+	intel_runtime_pm_get(dev_priv);
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev_priv);
 
@@ -4250,6 +4251,7 @@ void i915_gem_resume(struct drm_device *dev)
 	dev_priv->gt.resume(dev_priv);
 
 	mutex_unlock(&dev->struct_mutex);
+	intel_runtime_pm_put(dev_priv);
 }
 
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: optimise intel_runtime_pm_{get, put}

[David Weinehall]

Benchmarking shows that on resume we spend quite a bit of time
just taking and dropping these references, leaving us two options;
either rewriting the code not to take these references more than
once, which would be a rather invasive change since the involved
functions are used from other places, or to optimise
intel_runtime_pm_{get,put}().  This patch does the latter.
Initial benchmarking indicate improvements of a couple
of milliseconds on resume.

Original patch by Chris, with slight fixes by me.

v2: Fix missing return value (Patchwork)
    Remove extra atomic_dec() (Chris)

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 356c662ad453..831fde343d10 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2632,6 +2632,9 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct device *kdev = &pdev->dev;
 
+	if (atomic_inc_not_zero(&dev_priv->pm.wakeref_count))
+		return;
+
 	pm_runtime_get_sync(kdev);
 
 	atomic_inc(&dev_priv->pm.wakeref_count);
@@ -2653,6 +2656,9 @@ bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct device *kdev = &pdev->dev;
 
+	if (atomic_inc_not_zero(&dev_priv->pm.wakeref_count))
+		return true;
+
 	if (IS_ENABLED(CONFIG_PM)) {
 		int ret = pm_runtime_get_if_in_use(kdev);
 
@@ -2695,6 +2701,9 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct device *kdev = &pdev->dev;
 
+	if (atomic_inc_not_zero(&dev_priv->pm.wakeref_count))
+		return;
+
 	assert_rpm_wakelock_held(dev_priv);
 	pm_runtime_get_noresume(kdev);
 
@@ -2714,8 +2723,10 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct device *kdev = &pdev->dev;
 
+	if (!atomic_dec_and_test(&dev_priv->pm.wakeref_count))
+		return;
+
 	assert_rpm_wakelock_held(dev_priv);
-	atomic_dec(&dev_priv->pm.wakeref_count);
 
 	pm_runtime_mark_last_busy(kdev);
 	pm_runtime_put_autosuspend(kdev);
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Cleanup i915_gem_restore_gtt_mappings()

[Chris Wilson]

On Fri, Nov 18, 2016 at 03:36:45PM +0200, David Weinehall wrote:
> On resume we unbind+bind our VMA-mappings. This patch simplifies
> this a bit by introducing a restore_vma() helper.  As a nice side-effect
> this also makes the resume callgraph self-documenting.
> 
> v2: move the helper to i915_gem_gtt.c since it's only used by
>     i915_gem_restore_gtt_mappings(), rename to restore_vma(),
>     and make static (Chris)
> 
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b4bde1452f2a..2711044e3bf2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3277,6 +3277,16 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +static bool restore_vma(struct i915_vma *vma)
> +{
> +	if (i915_vma_is_pinned(vma)) {
> +		WARN_ON(i915_vma_bind(vma, vma->obj->cache_level, PIN_UPDATE));
> +		return true;
> +	} else {
> +		WARN_ON(i915_vma_unbind(vma));
> +		return false;
> +	}
> +}

Missed a blank line [checkpatch]

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: optimise intel_runtime_pm_{get, put}

[Chris Wilson]

On Fri, Nov 18, 2016 at 03:36:47PM +0200, David Weinehall wrote:
> Benchmarking shows that on resume we spend quite a bit of time
> just taking and dropping these references, leaving us two options;
> either rewriting the code not to take these references more than
> once, which would be a rather invasive change since the involved
> functions are used from other places, or to optimise
> intel_runtime_pm_{get,put}().  This patch does the latter.
> Initial benchmarking indicate improvements of a couple
> of milliseconds on resume.
> 
> Original patch by Chris, with slight fixes by me.
> 
> v2: Fix missing return value (Patchwork)
>     Remove extra atomic_dec() (Chris)
> 
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@linux.intel.com>

I'm happy with this. Not amused that it apparently saves quite a bit of
overhead with frequent pm_runtime calls.

Imre?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: optimise intel_runtime_pm_{get, put}

[Ville Syrjälä]

On Fri, Nov 18, 2016 at 02:00:40PM +0000, Chris Wilson wrote:
> On Fri, Nov 18, 2016 at 03:36:47PM +0200, David Weinehall wrote:
> > Benchmarking shows that on resume we spend quite a bit of time
> > just taking and dropping these references, leaving us two options;
> > either rewriting the code not to take these references more than
> > once, which would be a rather invasive change since the involved
> > functions are used from other places, or to optimise
> > intel_runtime_pm_{get,put}().  This patch does the latter.
> > Initial benchmarking indicate improvements of a couple
> > of milliseconds on resume.
> > 
> > Original patch by Chris, with slight fixes by me.
> > 
> > v2: Fix missing return value (Patchwork)
> >     Remove extra atomic_dec() (Chris)
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@linux.intel.com>
> 
> I'm happy with this. Not amused that it apparently saves quite a bit of
> overhead with frequent pm_runtime calls.

We could eliminate some of those calls entirely by moving them from
intel_display_power_{get,put}() into the always on well enable/disable
hooks. But I'm not sure how much this overhead originates from the power
well code as opposed to some gem/etc. stuff.

> 
> Imre?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Resume time optimisation (rev2)

[Patchwork]

== Series Details ==

Series: Resume time optimisation (rev2)
URL   : https://patchwork.freedesktop.org/series/15545/
State : failure

== Summary ==

Series 15545v2 Resume time optimisation
https://patchwork.freedesktop.org/api/1.0/series/15545/revisions/2/mbox/

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> INCOMPLETE (fi-bxt-t5700)
                pass       -> INCOMPLETE (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-hsw-4770)
        Subgroup basic-rte:
                pass       -> INCOMPLETE (fi-bsw-n3050)
                pass       -> INCOMPLETE (fi-hsw-4770)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:213  pass:173  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-t5700     total:212  pass:183  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:212  pass:183  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:212  pass:179  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:213  pass:191  dwarn:1   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

05104adb7446f310fad4e36f22d4c84ffafa31fc drm-intel-nightly: 2016y-11m-18d-13h-10m-16s UTC integration manifest
db3405c drm/i915: optimise intel_runtime_pm_{get, put}
1526407 drm/i915: Take runtime pm in i915_gem_resume()
c3ff9f5 drm/i915: Cleanup i915_gem_restore_gtt_mappings()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3055/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: optimise intel_runtime_pm_{get, put}

[Imre Deak]

On pe, 2016-11-18 at 14:00 +0000, Chris Wilson wrote:
> On Fri, Nov 18, 2016 at 03:36:47PM +0200, David Weinehall wrote:
> > Benchmarking shows that on resume we spend quite a bit of time
> > just taking and dropping these references, leaving us two options;
> > either rewriting the code not to take these references more than
> > once, which would be a rather invasive change since the involved
> > functions are used from other places, or to optimise
> > intel_runtime_pm_{get,put}().  This patch does the latter.
> > Initial benchmarking indicate improvements of a couple
> > of milliseconds on resume.
> > 
> > Original patch by Chris, with slight fixes by me.
> > 
> > v2: Fix missing return value (Patchwork)
> >     Remove extra atomic_dec() (Chris)
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@linux.intel.com>
> 
> I'm happy with this. Not amused that it apparently saves quite a bit
> of
> overhead with frequent pm_runtime calls.
> 
> Imre?

I think the overhead is because the RPM core takes a lock and checks if
the device needs to be woken up even if the runtime_usage count is 0.
But for us the device is awake whenever wakeref_count > 0, so yes we
can optimize things.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Cleanup i915_gem_restore_gtt_mappings()

[David Weinehall]

On Fri, Nov 18, 2016 at 01:58:35PM +0000, Chris Wilson wrote:
> On Fri, Nov 18, 2016 at 03:36:45PM +0200, David Weinehall wrote:
> > On resume we unbind+bind our VMA-mappings. This patch simplifies
> > this a bit by introducing a restore_vma() helper.  As a nice side-effect
> > this also makes the resume callgraph self-documenting.
> > 
> > v2: move the helper to i915_gem_gtt.c since it's only used by
> >     i915_gem_restore_gtt_mappings(), rename to restore_vma(),
> >     and make static (Chris)
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index b4bde1452f2a..2711044e3bf2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -3277,6 +3277,16 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> >  
> > +static bool restore_vma(struct i915_vma *vma)
> > +{
> > +	if (i915_vma_is_pinned(vma)) {
> > +		WARN_ON(i915_vma_bind(vma, vma->obj->cache_level, PIN_UPDATE));
> > +		return true;
> > +	} else {
> > +		WARN_ON(i915_vma_unbind(vma));
> > +		return false;
> > +	}
> > +}
> 
> Missed a blank line [checkpatch]

Dang.

Will you fix this manually for the merge, or do you want a fixed patch?


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Cleanup i915_gem_restore_gtt_mappings()

[Chris Wilson]

On Mon, Nov 21, 2016 at 02:21:39PM +0200, David Weinehall wrote:
> On Fri, Nov 18, 2016 at 01:58:35PM +0000, Chris Wilson wrote:
> > On Fri, Nov 18, 2016 at 03:36:45PM +0200, David Weinehall wrote:
> > > On resume we unbind+bind our VMA-mappings. This patch simplifies
> > > this a bit by introducing a restore_vma() helper.  As a nice side-effect
> > > this also makes the resume callgraph self-documenting.
> > > 
> > > v2: move the helper to i915_gem_gtt.c since it's only used by
> > >     i915_gem_restore_gtt_mappings(), rename to restore_vma(),
> > >     and make static (Chris)
> > > 
> > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index b4bde1452f2a..2711044e3bf2 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -3277,6 +3277,16 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
> > >  	return 0;
> > >  }
> > >  
> > > +static bool restore_vma(struct i915_vma *vma)
> > > +{
> > > +	if (i915_vma_is_pinned(vma)) {
> > > +		WARN_ON(i915_vma_bind(vma, vma->obj->cache_level, PIN_UPDATE));
> > > +		return true;
> > > +	} else {
> > > +		WARN_ON(i915_vma_unbind(vma));
> > > +		return false;
> > > +	}
> > > +}
> > 
> > Missed a blank line [checkpatch]
> 
> Dang.
> 
> Will you fix this manually for the merge, or do you want a fixed patch?

I was looking at how we get out of the pm_runtime mess. In part, we hurt
ourselves because we are using wakeref_count to disable asserts, but it
also seems that pushing the optimisation to pm_runtime is the right
thing to do.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Cleanup i915_gem_restore_gtt_mappings()

[David Weinehall]

On Mon, Nov 21, 2016 at 12:30:01PM +0000, Chris Wilson wrote:
> 
> I was looking at how we get out of the pm_runtime mess. In part, we hurt
> ourselves because we are using wakeref_count to disable asserts, but it
> also seems that pushing the optimisation to pm_runtime is the right
> thing to do.

Any more thoughts on this?


Kind regards, David Weinehall
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx