* [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
@ 2016-10-31 15:24 ` Mika Kuoppala
2016-10-31 15:49 ` Chris Wilson
2016-10-31 15:42 ` [PATCH 1/2] drm/i915/gtt: Fix pte clear range Chris Wilson
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Mika Kuoppala @ 2016-10-31 15:24 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear
2016-10-31 15:24 ` [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear Mika Kuoppala
@ 2016-10-31 15:49 ` Chris Wilson
2016-10-31 15:58 ` Mika Kuoppala
0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2016-10-31 15:49 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear
2016-10-31 15:49 ` Chris Wilson
@ 2016-10-31 15:58 ` Mika Kuoppala
2016-10-31 17:14 ` Chris Wilson
0 siblings, 1 reply; 16+ messages in thread
From: Mika Kuoppala @ 2016-10-31 15:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear
2016-10-31 15:58 ` Mika Kuoppala
@ 2016-10-31 17:14 ` Chris Wilson
0 siblings, 0 replies; 16+ messages in thread
From: Chris Wilson @ 2016-10-31 17:14 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
2016-10-31 15:24 ` [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear Mika Kuoppala
@ 2016-10-31 15:42 ` Chris Wilson
2016-10-31 15:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] " Patchwork
` (4 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Chris Wilson @ 2016-10-31 15:42 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
2016-10-31 15:24 ` [PATCH 2/2] drm/i915/gtt: Mark tlbs dirty on clear Mika Kuoppala
2016-10-31 15:42 ` [PATCH 1/2] drm/i915/gtt: Fix pte clear range Chris Wilson
@ 2016-10-31 15:46 ` Patchwork
2016-10-31 15:55 ` [PATCH 1/2] " Mika Kuoppala
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2016-10-31 15:46 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
` (2 preceding siblings ...)
2016-10-31 15:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] " Patchwork
@ 2016-10-31 15:55 ` Mika Kuoppala
2016-11-01 8:19 ` Joonas Lahtinen
2016-10-31 16:16 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev2) Patchwork
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Mika Kuoppala @ 2016-10-31 15:55 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-10-31 15:55 ` [PATCH 1/2] " Mika Kuoppala
@ 2016-11-01 8:19 ` Joonas Lahtinen
2016-11-01 10:22 ` Mika Kuoppala
0 siblings, 1 reply; 16+ messages in thread
From: Joonas Lahtinen @ 2016-11-01 8:19 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-11-01 8:19 ` Joonas Lahtinen
@ 2016-11-01 10:22 ` Mika Kuoppala
2016-11-01 10:28 ` Chris Wilson
0 siblings, 1 reply; 16+ messages in thread
From: Mika Kuoppala @ 2016-11-01 10:22 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-11-01 10:22 ` Mika Kuoppala
@ 2016-11-01 10:28 ` Chris Wilson
0 siblings, 0 replies; 16+ messages in thread
From: Chris Wilson @ 2016-11-01 10:28 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev2)
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
` (3 preceding siblings ...)
2016-10-31 15:55 ` [PATCH 1/2] " Mika Kuoppala
@ 2016-10-31 16:16 ` Patchwork
2016-11-01 13:27 ` [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
2016-11-01 15:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3) Patchwork
6 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2016-10-31 16:16 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
` (4 preceding siblings ...)
2016-10-31 16:16 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev2) Patchwork
@ 2016-11-01 13:27 ` Mika Kuoppala
2016-11-02 9:29 ` Joonas Lahtinen
2016-11-01 15:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3) Patchwork
6 siblings, 1 reply; 16+ messages in thread
From: Mika Kuoppala @ 2016-11-01 13:27 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] drm/i915/gtt: Fix pte clear range
2016-11-01 13:27 ` [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
@ 2016-11-02 9:29 ` Joonas Lahtinen
0 siblings, 0 replies; 16+ messages in thread
From: Joonas Lahtinen @ 2016-11-02 9:29 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3)
2016-10-31 15:24 [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
` (5 preceding siblings ...)
2016-11-01 13:27 ` [PATCH 1/2] drm/i915/gtt: Fix pte clear range Mika Kuoppala
@ 2016-11-01 15:46 ` Patchwork
2016-11-01 16:28 ` Mika Kuoppala
6 siblings, 1 reply; 16+ messages in thread
From: Patchwork @ 2016-11-01 15:46 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3)
2016-11-01 15:46 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/gtt: Fix pte clear range (rev3) Patchwork
@ 2016-11-01 16:28 ` Mika Kuoppala
0 siblings, 0 replies; 16+ messages in thread
From: Mika Kuoppala @ 2016-11-01 16:28 UTC (permalink / raw)
To: Patchwork; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread