[RFC 1/3] drm/i915: Use natural width type for VMA pin count

[RFC 1/3] drm/i915: Use natural width type for VMA pin count

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Using four bits for the pin count in the middle of the data
structure just makes the compiler generate verbose code.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..b4766c4c4bac 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -213,9 +213,9 @@ struct i915_vma {
 	 * and the framebuffer code. When switching/pageflipping, the
 	 * framebuffer code has at most two buffers pinned per crtc.
 	 *
-	 * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
-	 * bits with absolutely no headroom. So use 4 bits. */
-	unsigned int pin_count:4;
+	 * In the worst case this is 1 + 1 + 1 + 2*2 = 7.
+	 */
+	unsigned int pin_count;
 #define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
 };
 
-- 
1.9.1

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

[RFC 2/3] drm/i915: Track aggregate per-object VMA pin count

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This allows trivial (non-iterating) i915_gem_obj_is_pinned
implementation which in turns prevents i915_gem_madvise_ioctl
showing up in profiles in benchmarks like SynMark2/OglDrvCtx.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  9 ++++++++-
 drivers/gpu/drm/i915/i915_gem.c            | 15 +++++----------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +++-
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6f1e0f127c0a..589256e2ebe9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2104,6 +2104,9 @@ struct drm_i915_gem_object {
 	/** List of VMAs backed by this object */
 	struct list_head vma_list;
 
+	/** Aggregate pin count of all VMAs backed by this object. */
+	unsigned int vma_pin_count;
+
 	/** Stolen memory for this object, instead of being backed by shmem. */
 	struct drm_mm_node *stolen;
 	struct list_head global_list;
@@ -3236,7 +3239,11 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 {
 	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
 }
-bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
+
+static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
+{
+	return obj->vma_pin_count > 0;
+}
 
 /* Some GGTT VM helpers */
 static inline struct i915_hw_ppgtt *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 261a3ef72828..0549dea683e1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4311,6 +4311,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 	}
 
 	vma->pin_count++;
+	obj->vma_pin_count++;
+
 	return 0;
 }
 
@@ -4351,6 +4353,7 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
 	WARN_ON(!i915_gem_obj_ggtt_bound_view(obj, view));
 
 	--vma->pin_count;
+	--obj->vma_pin_count;
 }
 
 int
@@ -4583,6 +4586,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) {
 		int ret;
 
+		GEM_BUG_ON(obj->vma_pin_count < vma->pin_count);
+		obj->vma_pin_count -= vma->pin_count;
 		vma->pin_count = 0;
 		ret = i915_vma_unbind(vma);
 		if (WARN_ON(ret == -ERESTARTSYS)) {
@@ -5326,16 +5331,6 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 	return 0;
 }
 
-bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, obj_link)
-		if (vma->pin_count > 0)
-			return true;
-
-	return false;
-}
-
 /* Like i915_gem_object_get_page(), but mark the returned page dirty */
 struct page *
 i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6f4f2a6cdf93..b4c787763955 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -218,8 +218,10 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
 	if (entry->flags & __EXEC_OBJECT_HAS_FENCE)
 		i915_gem_object_unpin_fence(obj);
 
-	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
+	if (entry->flags & __EXEC_OBJECT_HAS_PIN) {
 		vma->pin_count--;
+		obj->vma_pin_count--;
+	}
 
 	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
 }
-- 
1.9.1

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

[RFC 3/3] drm/i915: Micro-optimize i915_gem_obj_to_vma

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

i915_gem_obj_to_vma is one of the most expensive functions in
our profiles. Could avoiding some branching by replacing it
with arithmetic be beneficial? Some benchmarks suggest it
slightly might.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0549dea683e1..243bfb922eb3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4642,11 +4642,21 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm)
 {
 	struct i915_vma *vma;
+
+	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL != 0);
+
 	list_for_each_entry(vma, &obj->vma_list, obj_link) {
-		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
-		    vma->vm == vm)
+		/*
+		 * Below is just a branching avoiding way of saying:
+		 * vma_ggtt_view.type == I915_GGTT_VIEW_NORMAL && vma->vm == vm,
+		 * which relies on the fact I915_GGTT_VIEW_NORMAL has to be
+		 * zero.
+		 */
+		if (!((unsigned long)vma->ggtt_view.type |
+		    ((unsigned long)vma->vm ^ (unsigned long)vm)))
 			return vma;
 	}
+
 	return NULL;
 }
 
-- 
1.9.1

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

Re: [RFC 2/3] drm/i915: Track aggregate per-object VMA pin count

[Chris Wilson]

On Thu, Apr 21, 2016 at 01:05:52PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This allows trivial (non-iterating) i915_gem_obj_is_pinned
> implementation which in turns prevents i915_gem_madvise_ioctl
> showing up in profiles in benchmarks like SynMark2/OglDrvCtx.

madvise doesn't need to check is-pinned at all.

And I prefer obj->bind_count.
-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: [RFC 1/3] drm/i915: Use natural width type for VMA pin count

[Chris Wilson]

On Thu, Apr 21, 2016 at 01:05:51PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Using four bits for the pin count in the middle of the data
> structure just makes the compiler generate verbose code.

Or rather by opening coding this we can do much better and
simultaneously checking pin and binding inside the hot i915_vma_pin
function.
-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: [RFC 3/3] drm/i915: Micro-optimize i915_gem_obj_to_vma

[Chris Wilson]

On Thu, Apr 21, 2016 at 01:05:53PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> i915_gem_obj_to_vma is one of the most expensive functions in
> our profiles. Could avoiding some branching by replacing it
> with arithmetic be beneficial? Some benchmarks suggest it
> slightly might.

We can do much better by changing the algorithm here (here and higher
up the call chain).
-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: [RFC 2/3] drm/i915: Track aggregate per-object VMA pin count

[Chris Wilson]

On Thu, Apr 21, 2016 at 01:15:57PM +0100, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 01:05:52PM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > This allows trivial (non-iterating) i915_gem_obj_is_pinned
> > implementation which in turns prevents i915_gem_madvise_ioctl
> > showing up in profiles in benchmarks like SynMark2/OglDrvCtx.
> 
> madvise doesn't need to check is-pinned at all.

And if you are keen on solving the madvise contention, I've some patches
to migrate it from struct_mutex to its own obj->mm.mutex. The patch is
in very early (functional) state:
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=239350fdd06f5aca4112c3d046979b281b7adf60
-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

✓ Fi.CI.BAT: success for series starting with [RFC,1/3] drm/i915: Use natural width type for VMA pin count

[Patchwork]

== Series Details ==

Series: series starting with [RFC,1/3] drm/i915: Use natural width type for VMA pin count
URL   : https://patchwork.freedesktop.org/series/6061/
State : success

== Summary ==

Series 6061v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6061/revisions/1/mbox/


bdw-nuci7        total:193  pass:181  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:192  pass:153  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
ilk-hp8440p      total:193  pass:136  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:193  pass:165  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:193  pass:168  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:193  pass:182  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:193  pass:155  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t failed to connect after reboot

Results at /archive/results/CI_IGT_test/Patchwork_2022/

340c485ad98d0ec0369a3b18d4a09938f3f5537d drm-intel-nightly: 2016y-04m-22d-17h-32m-25s UTC integration manifest
88b2604 drm/i915: Micro-optimize i915_gem_obj_to_vma
1e27bbf drm/i915: Track aggregate per-object VMA pin count
8a4b065 drm/i915: Use natural width type for VMA pin count

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

Re: [RFC 3/3] drm/i915: Micro-optimize i915_gem_obj_to_vma

[Dave Gordon]

On 21/04/16 13:05, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> i915_gem_obj_to_vma is one of the most expensive functions in
> our profiles. Could avoiding some branching by replacing it
> with arithmetic be beneficial? Some benchmarks suggest it
> slightly might.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0549dea683e1..243bfb922eb3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4642,11 +4642,21 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>   				     struct i915_address_space *vm)
>   {
>   	struct i915_vma *vma;
> +
> +	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL != 0);
> +
>   	list_for_each_entry(vma, &obj->vma_list, obj_link) {
> -		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
> -		    vma->vm == vm)
> +		/*
> +		 * Below is just a branching avoiding way of saying:
> +		 * vma_ggtt_view.type == I915_GGTT_VIEW_NORMAL && vma->vm == vm,
> +		 * which relies on the fact I915_GGTT_VIEW_NORMAL has to be
> +		 * zero.
> +		 */
> +		if (!((unsigned long)vma->ggtt_view.type |
> +		    ((unsigned long)vma->vm ^ (unsigned long)vm)))
>   			return vma;
>   	}
> +
>   	return NULL;
>   }

Other alternatives might include splitting the vma_list, so that we have 
one list for the most-frequently searched-for entries (GGTT view NORMAL) 
and for everything else, so the above would just need a single test for 
equality.

Or, slightly less effectively, add GGTT/NORMAL entries at the head of 
the list and others at the tail (and search backwards if you *don't* 
want a GGTT/NORMAL entry). That would still need the comparisons, but 
would likely hit an early match.

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

Re: [RFC 3/3] drm/i915: Micro-optimize i915_gem_obj_to_vma

[Chris Wilson]

On Tue, Apr 26, 2016 at 11:35:53AM +0100, Dave Gordon wrote:
> On 21/04/16 13:05, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >i915_gem_obj_to_vma is one of the most expensive functions in
> >our profiles. Could avoiding some branching by replacing it
> >with arithmetic be beneficial? Some benchmarks suggest it
> >slightly might.
> >
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 0549dea683e1..243bfb922eb3 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4642,11 +4642,21 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> >  				     struct i915_address_space *vm)
> >  {
> >  	struct i915_vma *vma;
> >+
> >+	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL != 0);
> >+
> >  	list_for_each_entry(vma, &obj->vma_list, obj_link) {
> >-		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
> >-		    vma->vm == vm)
> >+		/*
> >+		 * Below is just a branching avoiding way of saying:
> >+		 * vma_ggtt_view.type == I915_GGTT_VIEW_NORMAL && vma->vm == vm,
> >+		 * which relies on the fact I915_GGTT_VIEW_NORMAL has to be
> >+		 * zero.
> >+		 */
> >+		if (!((unsigned long)vma->ggtt_view.type |
> >+		    ((unsigned long)vma->vm ^ (unsigned long)vm)))
> >  			return vma;
> >  	}
> >+
> >  	return NULL;
> >  }
> 
> Other alternatives might include splitting the vma_list, so that we
> have one list for the most-frequently searched-for entries (GGTT
> view NORMAL) and for everything else, so the above would just need a
> single test for equality.
> 
> Or, slightly less effectively, add GGTT/NORMAL entries at the head
> of the list and others at the tail (and search backwards if you
> *don't* want a GGTT/NORMAL entry). That would still need the
> comparisons, but would likely hit an early match.

We want one list for convenience elsewhere, but can keep a rht in
parallel. This is not as effective/important as keeping a hashtable to
translate from handle to vma, but is still useful for some stress cases.
-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