[PATCH 1/2] drm/i915/gtt: Fix pte clear range

[PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Mika Kuoppala]

Comparing pte index to a number of entries is wrong
when clearing a range of pte entries. Use end marker
of 'one past' to correctly point adequate number of
ptes to the scratch page.

Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
References: https://bugs.freedesktop.org/show_bug.cgi?id=98282
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e7afad5..cda263c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
  */
 static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 				struct i915_page_table *pt,
-				uint64_t start,
-				uint64_t length)
+				const uint64_t start,
+				const uint64_t length)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	unsigned int pte_start = gen8_pte_index(start);
-	unsigned int num_entries = gen8_pte_count(start, length);
-	uint64_t pte;
+	const unsigned int num_entries = gen8_pte_count(start, length);
+	unsigned int pte = gen8_pte_index(start);
+	unsigned int pte_end = pte + num_entries;
 	gen8_pte_t *pt_vaddr;
 	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
 						 I915_CACHE_LLC);
@@ -726,17 +726,20 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	if (WARN_ON(!px_page(pt)))
 		return false;
 
-	bitmap_clear(pt->used_ptes, pte_start, num_entries);
+	bitmap_clear(pt->used_ptes, pte, num_entries);
 
 	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
 		free_pt(vm->dev, pt);
 		return true;
 	}
 
+	if (WARN_ON_ONCE(pte_end > GEN8_PTES))
+		pte_end = GEN8_PTES;
+
 	pt_vaddr = kmap_px(pt);
 
-	for (pte = pte_start; pte < num_entries; pte++)
-		pt_vaddr[pte] = scratch_pte;
+	while (pte < pte_end)
+		pt_vaddr[pte++] = scratch_pte;
 
 	kunmap_px(ppgtt, pt_vaddr);
 
-- 
2.7.4

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

[PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear

[Mika Kuoppala]

Now when clearing ptes can modify upper level pdp's,
we need to mark them dirty so that they will be flushed
correctly.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index cda263c..cbca332 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -707,6 +707,16 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	return gen8_write_pdp(req, 0, px_dma(&ppgtt->pml4));
 }
 
+/* PDE TLBs are a pain to invalidate on GEN8+. When we modify
+ * the page table structures, we mark them dirty so that
+ * context switching/execlist queuing code takes extra steps
+ * to ensure that tlbs are flushed.
+ */
+static void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt)
+{
+	ppgtt->pd_dirty_rings = INTEL_INFO(ppgtt->base.dev)->ring_mask;
+}
+
 /* Removes entries from a single page table, releasing it if it's empty.
  * Caller can use the return value to update higher-level entries.
  */
@@ -810,6 +820,8 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 		}
 	}
 
+	mark_tlbs_dirty(ppgtt);
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev) &&
 	    bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(vm->dev))) {
 		free_pdp(vm->dev, pdp);
@@ -1284,16 +1296,6 @@ int __must_check alloc_gen8_temp_bitmaps(unsigned long **new_pds,
 	return -ENOMEM;
 }
 
-/* PDE TLBs are a pain to invalidate on GEN8+. When we modify
- * the page table structures, we mark them dirty so that
- * context switching/execlist queuing code takes extra steps
- * to ensure that tlbs are flushed.
- */
-static void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt)
-{
-	ppgtt->pd_dirty_rings = INTEL_INFO(ppgtt->base.dev)->ring_mask;
-}
-
 static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 				    struct i915_page_directory_pointer *pdp,
 				    uint64_t start,
-- 
2.7.4

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

Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Chris Wilson]

On Mon, Oct 31, 2016 at 05:24:45PM +0200, Mika Kuoppala wrote:
> Comparing pte index to a number of entries is wrong
> when clearing a range of pte entries. Use end marker
> of 'one past' to correctly point adequate number of
> ptes to the scratch page.
> 
> Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98282
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e7afad5..cda263c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
>   */
>  static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  				struct i915_page_table *pt,
> -				uint64_t start,
> -				uint64_t length)
> +				const uint64_t start,
> +				const uint64_t length)
>  {
>  	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> -	unsigned int pte_start = gen8_pte_index(start);
> -	unsigned int num_entries = gen8_pte_count(start, length);
> -	uint64_t pte;
> +	const unsigned int num_entries = gen8_pte_count(start, length);
> +	unsigned int pte = gen8_pte_index(start);
> +	unsigned int pte_end = pte + num_entries;
>  	gen8_pte_t *pt_vaddr;
>  	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
>  						 I915_CACHE_LLC);
> @@ -726,17 +726,20 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  	if (WARN_ON(!px_page(pt)))
>  		return false;
>  
> -	bitmap_clear(pt->used_ptes, pte_start, num_entries);
> +	bitmap_clear(pt->used_ptes, pte, num_entries);
>  
>  	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
>  		free_pt(vm->dev, pt);
>  		return true;
>  	}
>  
> +	if (WARN_ON_ONCE(pte_end > GEN8_PTES))
> +		pte_end = GEN8_PTES;

Internal programming error, if you hit all the upper layers are dead,
i.e. bug on. GEM_BUG_ON.

And the assert should be earlier since you have already used the invalid
values (i.e. pte+num_entries).

> +
>  	pt_vaddr = kmap_px(pt);
>  
> -	for (pte = pte_start; pte < num_entries; pte++)

Nice catch.
R-b on this part, less enamoured with the reset.
-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: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/gtt: Fix pte clear range
URL   : https://patchwork.freedesktop.org/series/14620/
State : warning

== Summary ==

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

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-kbl-7200u)
                dmesg-warn -> PASS       (fi-ilk-650)
                dmesg-warn -> PASS       (fi-bdw-5557u)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-ilk-650)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (fi-snb-2520m)
        Subgroup force-edid:
                dmesg-warn -> PASS       (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-3:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup bad-source:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-ilk-650)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-ilk-650       total:241  pass:185  dwarn:2   dfail:0   fail:0   skip:54 
fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-kbl-7200u     total:241  pass:218  dwarn:1   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:207  dwarn:1   dfail:0   fail:0   skip:33 
fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 

6a1197bcb5cc18a56ad4ae8e6d706a212bc3db7d drm-intel-nightly: 2016y-10m-31d-14h-58m-16s UTC integration manifest
d5c25c9 drm/i915/gtt: Mark tlbs dirty on clear
a6d3d38 drm/i915/gtt: Fix pte clear range

== Logs ==

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

Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear

[Chris Wilson]

On Mon, Oct 31, 2016 at 05:24:46PM +0200, Mika Kuoppala wrote:
> Now when clearing ptes can modify upper level pdp's,
> we need to mark them dirty so that they will be flushed
> correctly.

I suppose so. I think we could push this into the cleanup_px() (and
alloc) but that is probably a fair chunk of work for immeasurable gain.
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

[PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Mika Kuoppala]

Comparing pte index to a number of entries is wrong
when clearing a range of pte entries. Use end marker
of 'one past' to correctly point adequate number of
ptes to the scratch page.

v2: assert early instead of warning late (Chris)

Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
References: https://bugs.freedesktop.org/show_bug.cgi?id=98282
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e7afad5..5f5008f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
  */
 static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 				struct i915_page_table *pt,
-				uint64_t start,
-				uint64_t length)
+				const uint64_t start,
+				const uint64_t length)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	unsigned int pte_start = gen8_pte_index(start);
-	unsigned int num_entries = gen8_pte_count(start, length);
-	uint64_t pte;
+	const unsigned int num_entries = gen8_pte_count(start, length);
+	unsigned int pte = gen8_pte_index(start);
+	unsigned int pte_end = pte + num_entries;
 	gen8_pte_t *pt_vaddr;
 	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
 						 I915_CACHE_LLC);
@@ -726,7 +726,9 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	if (WARN_ON(!px_page(pt)))
 		return false;
 
-	bitmap_clear(pt->used_ptes, pte_start, num_entries);
+	GEM_BUG_ON(pte_end > GEN8_PTES);
+
+	bitmap_clear(pt->used_ptes, pte, num_entries);
 
 	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
 		free_pt(vm->dev, pt);
@@ -735,8 +737,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 
 	pt_vaddr = kmap_px(pt);
 
-	for (pte = pte_start; pte < num_entries; pte++)
-		pt_vaddr[pte] = scratch_pte;
+	while (pte < pte_end)
+		pt_vaddr[pte++] = scratch_pte;
 
 	kunmap_px(ppgtt, pt_vaddr);
 
-- 
2.7.4

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

Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear

[Mika Kuoppala]

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

> On Mon, Oct 31, 2016 at 05:24:46PM +0200, Mika Kuoppala wrote:
>> Now when clearing ptes can modify upper level pdp's,
>> we need to mark them dirty so that they will be flushed
>> correctly.
>
> I suppose so.

It is a bit iffy if we really do, but this way we gain symmetry
with the alloc side.

I pondered why this is missing from 4l side, but in there the
topmost pointer is static.

-Mika

> I think we could push this into the cleanup_px() (and
> alloc) but that is probably a fair chunk of work for immeasurable gain.
> 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

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev2)
URL   : https://patchwork.freedesktop.org/series/14620/
State : warning

== Summary ==

Series 14620v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14620/revisions/2/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-bdw-5557u)
                pass       -> SKIP       (fi-skl-6770hq)
                dmesg-warn -> PASS       (fi-ilk-650)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-ilk-650)
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-ilk-650)
Test kms_force_connector_basic:
        Subgroup force-edid:
                dmesg-warn -> PASS       (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-3:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup bad-source:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-ilk-650)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-ilk-650       total:241  pass:181  dwarn:6   dfail:0   fail:0   skip:54 
fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-snb-2520m     total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 

6a1197bcb5cc18a56ad4ae8e6d706a212bc3db7d drm-intel-nightly: 2016y-10m-31d-14h-58m-16s UTC integration manifest
e857e29 drm/i915/gtt: Mark tlbs dirty on clear
c6d35e9 drm/i915/gtt: Fix pte clear range

== Logs ==

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

Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear

[Chris Wilson]

On Mon, Oct 31, 2016 at 05:58:15PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Mon, Oct 31, 2016 at 05:24:46PM +0200, Mika Kuoppala wrote:
> >> Now when clearing ptes can modify upper level pdp's,
> >> we need to mark them dirty so that they will be flushed
> >> correctly.
> >
> > I suppose so.
> 
> It is a bit iffy if we really do, but this way we gain symmetry
> with the alloc side.

If we assume correct behaviour on the client, no. They shouldn't be
accessing any of the removed PTE, PDE, PDPE and so now follow the stale
TLB to the old pages (now reused by the system and containing garbage ->
GPU hang or carefully crafted redirection). However, on the same basis
we fill the client address space with scratch pages, we also should
prepare for random stray access which means we need to invalidate the
TLB -- however, given that this is undefined behaviour a ncurrently
executing batch may be accessing this page illegally and see the stale
value long before a newly submitted batch flushes the TLB.

So based on that we fix predictable TLB errors (such as the prefetcher
crossing the page boundary, if it still does!) for correctly behaving
batches.
-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 1/2] drm/i915/gtt: Fix pte clear range

[Joonas Lahtinen]

On ma, 2016-10-31 at 17:55 +0200, Mika Kuoppala wrote:
> @@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
>   */
>  static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  				struct i915_page_table *pt,
> -				uint64_t start,
> -				uint64_t length)
> +				const uint64_t start,
> +				const uint64_t length)
>  {

I think const for integers is bit much, with that logic we should make
the pointers const too (not the pointer destination).

> @@ -735,8 +737,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  
>  	pt_vaddr = kmap_px(pt);
>  
> -	for (pte = pte_start; pte < num_entries; pte++)
> -		pt_vaddr[pte] = scratch_pte;
> +	while (pte < pte_end)
> +		pt_vaddr[pte++] = scratch_pte;

I'd prefer the for loop still. Just fix "pte < pte_end".

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Mika Kuoppala]

Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:

> On ma, 2016-10-31 at 17:55 +0200, Mika Kuoppala wrote:
>> @@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
>>   */
>>  static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>>  				struct i915_page_table *pt,
>> -				uint64_t start,
>> -				uint64_t length)
>> +				const uint64_t start,
>> +				const uint64_t length)
>>  {
>
> I think const for integers is bit much, with that logic we should make
> the pointers const too (not the pointer destination).
>

It is more of a compiler guard of accidentally changing these
in the body. Separation of variants and invariants. But
if this not preferred, I can change them back.

>> @@ -735,8 +737,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>>  
>>  	pt_vaddr = kmap_px(pt);
>>  
>> -	for (pte = pte_start; pte < num_entries; pte++)
>> -		pt_vaddr[pte] = scratch_pte;
>> +	while (pte < pte_end)
>> +		pt_vaddr[pte++] = scratch_pte;
>
> I'd prefer the for loop still. Just fix "pte < pte_end".
>

I changed to this due to pte being already set and
used before the loop, to get rid of superfluous pte_start.
-Mika


> Regards, Joonas
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Chris Wilson]

On Tue, Nov 01, 2016 at 12:22:45PM +0200, Mika Kuoppala wrote:
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:
> >> @@ -735,8 +737,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
> >>  
> >>  	pt_vaddr = kmap_px(pt);
> >>  
> >> -	for (pte = pte_start; pte < num_entries; pte++)
> >> -		pt_vaddr[pte] = scratch_pte;
> >> +	while (pte < pte_end)
> >> +		pt_vaddr[pte++] = scratch_pte;
> >
> > I'd prefer the for loop still. Just fix "pte < pte_end".
> >
> 
> I changed to this due to pte being already set and
> used before the loop, to get rid of superfluous pte_start.

Yes, I think the change to while() made sense with the new flow of pte
being used a few times before we set the scratch_pte.
-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

[PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Mika Kuoppala]

Comparing pte index to a number of entries is wrong
when clearing a range of pte entries. Use end marker
of 'one past' to correctly point adequate number of
ptes to the scratch page.

v2: assert early instead of warning late (Chris)
v3: removed consts (Joonas)

Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98282
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reported-by: Mike Lothian <mike@fireburn.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Mike Lothian <mike@fireburn.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e7afad5..3b968ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -716,9 +716,9 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 				uint64_t length)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	unsigned int pte_start = gen8_pte_index(start);
 	unsigned int num_entries = gen8_pte_count(start, length);
-	uint64_t pte;
+	unsigned int pte = gen8_pte_index(start);
+	unsigned int pte_end = pte + num_entries;
 	gen8_pte_t *pt_vaddr;
 	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
 						 I915_CACHE_LLC);
@@ -726,7 +726,9 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	if (WARN_ON(!px_page(pt)))
 		return false;
 
-	bitmap_clear(pt->used_ptes, pte_start, num_entries);
+	GEM_BUG_ON(pte_end > GEN8_PTES);
+
+	bitmap_clear(pt->used_ptes, pte, num_entries);
 
 	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
 		free_pt(vm->dev, pt);
@@ -735,8 +737,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 
 	pt_vaddr = kmap_px(pt);
 
-	for (pte = pte_start; pte < num_entries; pte++)
-		pt_vaddr[pte] = scratch_pte;
+	while (pte < pte_end)
+		pt_vaddr[pte++] = scratch_pte;
 
 	kunmap_px(ppgtt, pt_vaddr);
 
-- 
2.7.4

_______________________________________________
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 [1/2] drm/i915/gtt: Fix pte clear range (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3)
URL   : https://patchwork.freedesktop.org/series/14620/
State : warning

== Summary ==

Series 14620v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14620/revisions/3/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)
                dmesg-warn -> PASS       (fi-ilk-650)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-ilk-650       total:241  pass:184  dwarn:3   dfail:0   fail:0   skip:54 
fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 

659f3942757fa5e85f37a2cab9da8c94b8f16f7a drm-intel-nightly: 2016y-11m-01d-14h-42m-02s UTC integration manifest
b44a28b drm/i915/gtt: Mark tlbs dirty on clear
de61fdd drm/i915/gtt: Fix pte clear range

== Logs ==

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

Re: ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3)

[Mika Kuoppala]

Patchwork <patchwork@emeril.freedesktop.org> writes:

> == Series Details ==
>
> Series: series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3)
> URL   : https://patchwork.freedesktop.org/series/14620/
> State : warning
>
> == Summary ==
>
> Series 14620v3 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/14620/revisions/3/mbox/
>
> Test drv_module_reload_basic:
>                 dmesg-warn -> PASS       (fi-skl-6770hq)
>                 dmesg-warn -> PASS       (fi-ilk-650)
> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                 pass       -> DMESG-WARN (fi-ilk-650)
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> DMESG-WARN (fi-ilk-650)
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 pass       -> DMESG-WARN (fi-ilk-650)

https://bugs.freedesktop.org/show_bug.cgi?id=98531

>
> fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
> fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
> fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
> fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
> fi-ilk-650       total:241  pass:184  dwarn:3   dfail:0   fail:0   skip:54 
> fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
> fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
> fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
> fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
> fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
> fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-2520m     total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
> fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 
>
> 659f3942757fa5e85f37a2cab9da8c94b8f16f7a drm-intel-nightly: 2016y-11m-01d-14h-42m-02s UTC integration manifest
> b44a28b drm/i915/gtt: Mark tlbs dirty on clear
> de61fdd drm/i915/gtt: Fix pte clear range
>
> == Logs ==
>
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2876/
^^

This is awesome!

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

Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Joonas Lahtinen]

On ti, 2016-11-01 at 15:27 +0200, Mika Kuoppala wrote:
> Comparing pte index to a number of entries is wrong
> when clearing a range of pte entries. Use end marker
> of 'one past' to correctly point adequate number of
> ptes to the scratch page.
> 
> v2: assert early instead of warning late (Chris)
> v3: removed consts (Joonas)
> 
> Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98282
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reported-by: Mike Lothian <mike@fireburn.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Mike Lothian <mike@fireburn.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx