| Message ID | 1477927486-6549-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)