[PATCH v2 1/3] drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE

[PATCH v2 1/3] drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE

[Chris Wilson]

I incorrectly converted the exclusion of the last 4096 bytes (that avoids
any potential prefetching past the end of the GTT) to PAGE_SIZE and not
to I915_GTT_PAGE_SIZE as it should be.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 68ecfc193747..9bed5e9ee31b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3241,9 +3241,9 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	 * shrink the range used by drm_mm.
 	 */
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	ggtt->base.total -= PAGE_SIZE;
+	ggtt->base.total -= I915_GTT_PAGE_SIZE;
 	i915_address_space_init(&ggtt->base, dev_priv, "[global]");
-	ggtt->base.total += PAGE_SIZE;
+	ggtt->base.total += I915_GTT_PAGE_SIZE;
 	if (!HAS_LLC(dev_priv))
 		ggtt->base.mm.color_adjust = i915_gtt_color_adjust;
 	mutex_unlock(&dev_priv->drm.struct_mutex);
-- 
2.11.0

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

[PATCH v2 2/3] drm/i915: Assign I915_COLOR_UNEVICTABLE to the address space head_node

[Chris Wilson]

The drm_mm range manager (within i915_address_space) uses a special
drm_mm_node that excludes the unavailable range (beyond the end of the
drm_mm). However, we play games with the global GTT to use the head_node
to exclude the tail page but tell ourselves that the whole range is
available. This causes an issue when we try to evict using the full
range of the global GTT which is wider than the drm_mm, resulting in
complete confusion and castrophe. One way to resolve this would be to
use a reserved node to exclude the guard page, or we can treat the
drm_mm's head_node as our guard page and assign it the appropriate
colour.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9bed5e9ee31b..75cd2a56b8b4 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2160,10 +2160,14 @@ static void i915_address_space_init(struct i915_address_space *vm,
 				    const char *name)
 {
 	i915_gem_timeline_init(dev_priv, &vm->timeline, name);
+
 	drm_mm_init(&vm->mm, vm->start, vm->total);
+	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
+
 	INIT_LIST_HEAD(&vm->active_list);
 	INIT_LIST_HEAD(&vm->inactive_list);
 	INIT_LIST_HEAD(&vm->unbound_list);
+
 	list_add_tail(&vm->global_link, &dev_priv->vm_list);
 }
 
-- 
2.11.0

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

[PATCH v2 3/3] drm/i915: Use page coloring to provide the guard page at the end of the GTT

[Chris Wilson]

As we now mark the reserved hole (drm_mm.head_node) with the special
UNEVICTABLE color, we can use the page coloring to avoid prefetching of
the CS beyond the end of the GTT.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_evict.c | 10 ++++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.c   | 19 ++++++++++++-------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index c181b1bb3d2c..776c508cb9ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -258,6 +258,9 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
 	int ret = 0;
 
 	lockdep_assert_held(&vm->i915->drm.struct_mutex);
+	GEM_BUG_ON(!IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
+	GEM_BUG_ON(!IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
+
 	trace_i915_gem_evict_node(vm, target, flags);
 
 	/* Retire before we search the active list. Although we have
@@ -273,9 +276,11 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
 		/* Expand search to cover neighbouring guard pages (or lack!) */
 		if (start > vm->start)
 			start -= I915_GTT_PAGE_SIZE;
-		if (end < vm->start + vm->total)
-			end += I915_GTT_PAGE_SIZE;
+
+		/* Always look at the page afterwards to avoid the end-of-GTT */
+		end += I915_GTT_PAGE_SIZE;
 	}
+	GEM_BUG_ON(start >= end);
 
 	drm_mm_for_each_node_in_range(node, &vm->mm, start, end) {
 		/* If we find any non-objects (!vma), we cannot evict them */
@@ -284,6 +289,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
 			break;
 		}
 
+		GEM_BUG_ON(!node->allocated);
 		vma = container_of(node, typeof(*vma), node);
 
 		/* If we are using coloring to insert guard pages between
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 75cd2a56b8b4..959764394718 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2718,11 +2718,16 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
 				  u64 *start,
 				  u64 *end)
 {
-	if (node->color != color)
+	if (node->allocated && node->color != color)
 		*start += I915_GTT_PAGE_SIZE;
 
+	/* Also leave a space between the unallocated reserved node after the
+	 * GTT and any objects within the GTT, i.e. we use the color adjustment
+	 * to insert a guard page to prevent prefetches crossing over the
+	 * GTT boundary.
+	 */
 	node = list_next_entry(node, node_list);
-	if (node->allocated && node->color != color)
+	if (node->color != color)
 		*end -= I915_GTT_PAGE_SIZE;
 }
 
@@ -3241,14 +3246,14 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 
 	INIT_LIST_HEAD(&dev_priv->vm_list);
 
-	/* Subtract the guard page before address space initialization to
-	 * shrink the range used by drm_mm.
+	/* Note that we use page colouring to enforce a guard page at the
+	 * end of the address space. This is required as the CS may prefetch
+	 * beyond the end of the batch buffer, across the page boundary,
+	 * and beyond the end of the GTT if we do provide a guard.
 	 */
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	ggtt->base.total -= I915_GTT_PAGE_SIZE;
 	i915_address_space_init(&ggtt->base, dev_priv, "[global]");
-	ggtt->base.total += I915_GTT_PAGE_SIZE;
-	if (!HAS_LLC(dev_priv))
+	if (!HAS_LLC(dev_priv) && !USES_PPGTT(dev_priv))
 		ggtt->base.mm.color_adjust = i915_gtt_color_adjust;
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-- 
2.11.0

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

Re: [PATCH v2 1/3] drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE

[Matthew Auld]

On 6 February 2017 at 08:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> I incorrectly converted the exclusion of the last 4096 bytes (that avoids
> any potential prefetching past the end of the GTT) to PAGE_SIZE and not
> to I915_GTT_PAGE_SIZE as it should be.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Assign I915_COLOR_UNEVICTABLE to the address space head_node

[Matthew Auld]

On 6 February 2017 at 08:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The drm_mm range manager (within i915_address_space) uses a special
> drm_mm_node that excludes the unavailable range (beyond the end of the
> drm_mm). However, we play games with the global GTT to use the head_node
> to exclude the tail page but tell ourselves that the whole range is
> available. This causes an issue when we try to evict using the full
> range of the global GTT which is wider than the drm_mm, resulting in
> complete confusion and castrophe. One way to resolve this would be to
catastrophe

> use a reserved node to exclude the guard page, or we can treat the
> drm_mm's head_node as our guard page and assign it the appropriate
> colour.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [v2,1/3] drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE
URL   : https://patchwork.freedesktop.org/series/19148/
State : warning

== Summary ==

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

Test gem_cs_tlb:
        Subgroup basic-default:
                incomplete -> PASS       (fi-ilk-650)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:252  pass:214  dwarn:0   dfail:0   fail:0   skip:38 
fi-bsw-n3050     total:252  pass:192  dwarn:0   dfail:0   fail:0   skip:60 
fi-bxt-j4205     total:252  pass:208  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:209  pass:167  dwarn:0   dfail:0   fail:0   skip:41 
fi-byt-j1900     total:252  pass:204  dwarn:0   dfail:0   fail:0   skip:48 
fi-byt-n2820     total:252  pass:200  dwarn:0   dfail:0   fail:0   skip:52 
fi-hsw-4770      total:252  pass:211  dwarn:0   dfail:0   fail:0   skip:41 
fi-hsw-4770r     total:252  pass:211  dwarn:0   dfail:0   fail:0   skip:41 
fi-ilk-650       total:252  pass:180  dwarn:0   dfail:0   fail:0   skip:72 
fi-ivb-3520m     total:252  pass:210  dwarn:0   dfail:0   fail:0   skip:42 
fi-ivb-3770      total:252  pass:210  dwarn:0   dfail:0   fail:0   skip:42 
fi-kbl-7500u     total:252  pass:207  dwarn:0   dfail:0   fail:2   skip:43 
fi-skl-6260u     total:252  pass:215  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6700hq    total:252  pass:210  dwarn:0   dfail:0   fail:0   skip:42 
fi-skl-6700k     total:252  pass:205  dwarn:4   dfail:0   fail:0   skip:43 
fi-skl-6770hq    total:252  pass:215  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2520m     total:252  pass:200  dwarn:1   dfail:0   fail:0   skip:51 
fi-snb-2600      total:252  pass:200  dwarn:0   dfail:0   fail:0   skip:52 

e7d4ec79b24f82431f34d070d16772538422fb13 drm-tip: 2017y-02m-06d-11h-49m-42s UTC integration manifest
23a2da1 drm/i915: Use page coloring to provide the guard page at the end of the GTT
0452ed9 drm/i915: Assign I915_COLOR_UNEVICTABLE to the address space head_node
4214689 drm/i915: Manipulate the Global GTT size using I915_GTT_PAGE_SIZE

== Logs ==

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

Re: [PATCH v2 3/3] drm/i915: Use page coloring to provide the guard page at the end of the GTT

[Matthew Auld]

On 6 February 2017 at 08:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> As we now mark the reserved hole (drm_mm.head_node) with the special
> UNEVICTABLE color, we can use the page coloring to avoid prefetching of
> the CS beyond the end of the GTT.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_evict.c | 10 ++++++++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c   | 19 ++++++++++++-------
>  2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index c181b1bb3d2c..776c508cb9ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -258,6 +258,9 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>         int ret = 0;
>
>         lockdep_assert_held(&vm->i915->drm.struct_mutex);
> +       GEM_BUG_ON(!IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
> +       GEM_BUG_ON(!IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
> +
>         trace_i915_gem_evict_node(vm, target, flags);
>
>         /* Retire before we search the active list. Although we have
> @@ -273,9 +276,11 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>                 /* Expand search to cover neighbouring guard pages (or lack!) */
>                 if (start > vm->start)
>                         start -= I915_GTT_PAGE_SIZE;
> -               if (end < vm->start + vm->total)
> -                       end += I915_GTT_PAGE_SIZE;
> +
> +               /* Always look at the page afterwards to avoid the end-of-GTT */
> +               end += I915_GTT_PAGE_SIZE;
>         }
> +       GEM_BUG_ON(start >= end);
>
>         drm_mm_for_each_node_in_range(node, &vm->mm, start, end) {
>                 /* If we find any non-objects (!vma), we cannot evict them */
> @@ -284,6 +289,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>                         break;
>                 }
>
> +               GEM_BUG_ON(!node->allocated);
>                 vma = container_of(node, typeof(*vma), node);
>
>                 /* If we are using coloring to insert guard pages between
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 75cd2a56b8b4..959764394718 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2718,11 +2718,16 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
>                                   u64 *start,
>                                   u64 *end)
>  {
> -       if (node->color != color)
> +       if (node->allocated && node->color != color)
>                 *start += I915_GTT_PAGE_SIZE;
>
> +       /* Also leave a space between the unallocated reserved node after the
> +        * GTT and any objects within the GTT, i.e. we use the color adjustment
> +        * to insert a guard page to prevent prefetches crossing over the
> +        * GTT boundary.
> +        */
>         node = list_next_entry(node, node_list);
> -       if (node->allocated && node->color != color)
> +       if (node->color != color)
>                 *end -= I915_GTT_PAGE_SIZE;
>  }
>
> @@ -3241,14 +3246,14 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
>
>         INIT_LIST_HEAD(&dev_priv->vm_list);
>
> -       /* Subtract the guard page before address space initialization to
> -        * shrink the range used by drm_mm.
> +       /* Note that we use page colouring to enforce a guard page at the
> +        * end of the address space. This is required as the CS may prefetch
> +        * beyond the end of the batch buffer, across the page boundary,
> +        * and beyond the end of the GTT if we do provide a guard.
if we do not provide a guard?

Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Use page coloring to provide the guard page at the end of the GTT

[Chris Wilson]

On Mon, Feb 06, 2017 at 12:54:39PM +0000, Matthew Auld wrote:
> On 6 February 2017 at 08:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > As we now mark the reserved hole (drm_mm.head_node) with the special
> > UNEVICTABLE color, we can use the page coloring to avoid prefetching of
> > the CS beyond the end of the GTT.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > @@ -3241,14 +3246,14 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
> >
> >         INIT_LIST_HEAD(&dev_priv->vm_list);
> >
> > -       /* Subtract the guard page before address space initialization to
> > -        * shrink the range used by drm_mm.
> > +       /* Note that we use page colouring to enforce a guard page at the
> > +        * end of the address space. This is required as the CS may prefetch
> > +        * beyond the end of the batch buffer, across the page boundary,
> > +        * and beyond the end of the GTT if we do provide a guard.
> if we do not provide a guard?
> 
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Thanks for the review, and now back to the usual breaking of CI.
-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