[Intel-gfx] [PATCH v2 14/24] drm/i915: Finish gen6/7 dynamic page table allocation
Michel Thierry
michel.thierry at intel.com
Tue Jan 13 03:53:22 PST 2015
On 1/5/2015 2:45 PM, Daniel Vetter wrote:
> On Tue, Dec 23, 2014 at 05:16:17PM +0000, Michel Thierry wrote:
>> From: Ben Widawsky <benjamin.widawsky at intel.com>
>>
>> This patch continues on the idea from the previous patch. From here on,
>> in the steady state, PDEs are all pointing to the scratch page table (as
>> recommended in the spec). When an object is allocated in the VA range,
>> the code will determine if we need to allocate a page for the page
>> table. Similarly when the object is destroyed, we will remove, and free
>> the page table pointing the PDE back to the scratch page.
>>
>> Following patches will work to unify the code a bit as we bring in GEN8
>> support. GEN6 and GEN8 are different enough that I had a hard time to
>> get to this point with as much common code as I do.
>>
>> The aliasing PPGTT must pre-allocate all of the page tables. There are a
>> few reasons for this. Two trivial ones: aliasing ppgtt goes through the
>> ggtt paths, so it's hard to maintain, we currently do not restore the
>> default context (assuming the previous force reload is indeed
>> necessary). Most importantly though, the only way (it seems from
>> empirical evidence) to invalidate the CS TLBs on non-render ring is to
>> either use ring sync (which requires actually stopping the rings in
>> order to synchronize when the sync completes vs. where you are in
>> execution), or to reload DCLV. Since without full PPGTT we do not ever
>> reload the DCLV register, there is no good way to achieve this. The
>> simplest solution is just to not support dynamic page table
>> creation/destruction in the aliasing PPGTT.
>>
>> We could always reload DCLV, but this seems like quite a bit of excess
>> overhead only to save at most 2MB-4k of memory for the aliasing PPGTT
>> page tables.
>>
>> v2: Make the page table bitmap declared inside the function (Chris)
>> Simplify the way scratching address space works.
>> Move the alloc/teardown tracepoints up a level in the call stack so that
>> both all implementations get the trace.
>>
>> v3: Updated trace event to spit out a name
>>
>> v4: Aliasing ppgtt is now initialized differently (in setup global gtt)
>>
>> v5: Rebase to latest code. Also removed unnecessary aliasing ppgtt check for
>> trace, as it is no longer possible after the PPGTT cleanup patch series
>> of a couple of months ago (Daniel).
>>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> Signed-off-by: Michel Thierry <michel.thierry at intel.com> (v4+)
> The tracepoints should be split into a separate patch. Although the
> teardown stuff will likely disappear I guess ...
>
> Two more comments below.
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
>> drivers/gpu/drm/i915/i915_gem.c | 2 +
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 128 ++++++++++++++++++++++++++++++++----
>> drivers/gpu/drm/i915/i915_trace.h | 115 ++++++++++++++++++++++++++++++++
>> 4 files changed, 236 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 60f91bc..0f63076 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2149,6 +2149,8 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
>> seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n", I915_READ(RING_PP_DIR_BASE_READ(ring)));
>> seq_printf(m, "PP_DIR_DCLV: 0x%08x\n", I915_READ(RING_PP_DIR_DCLV(ring)));
>> }
>> + seq_printf(m, "ECOCHK: 0x%08x\n\n", I915_READ(GAM_ECOCHK));
>> +
>> if (dev_priv->mm.aliasing_ppgtt) {
>> struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
>>
>> @@ -2165,7 +2167,6 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
>> get_pid_task(file->pid, PIDTYPE_PID)->comm);
>> idr_for_each(&file_priv->context_idr, per_file_ctx, m);
>> }
>> - seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
>> }
>>
>> static int i915_ppgtt_info(struct seq_file *m, void *data)
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 5d52990..1649fb2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3599,6 +3599,8 @@ search_free:
>>
>> /* allocate before insert / bind */
>> if (vma->vm->allocate_va_range) {
>> + trace_i915_va_alloc(vma->vm, vma->node.start, vma->node.size,
>> + VM_TO_TRACE_NAME(vma->vm));
>> ret = vma->vm->allocate_va_range(vma->vm,
>> vma->node.start,
>> vma->node.size);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 54c7ca7..32a355a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -1138,10 +1138,47 @@ static void gen6_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>> static int gen6_alloc_va_range(struct i915_address_space *vm,
>> uint64_t start, uint64_t length)
>> {
>> + DECLARE_BITMAP(new_page_tables, GEN6_PPGTT_PD_ENTRIES);
>> + struct drm_device *dev = vm->dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> struct i915_hw_ppgtt *ppgtt =
>> container_of(vm, struct i915_hw_ppgtt, base);
>> struct i915_pagetab *pt;
>> + const uint32_t start_save = start, length_save = length;
>> uint32_t pde, temp;
>> + int ret;
>> +
>> + BUG_ON(upper_32_bits(start));
>> +
>> + bitmap_zero(new_page_tables, GEN6_PPGTT_PD_ENTRIES);
>> +
>> + /* The allocation is done in two stages so that we can bail out with
>> + * minimal amount of pain. The first stage finds new page tables that
>> + * need allocation. The second stage marks use ptes within the page
>> + * tables.
>> + */
> If we drop the bitmask tracking we could massively simplify this -
> checking just the various pt pointers should be enough?
>
>> + gen6_for_each_pde(pt, &ppgtt->pd, start, length, temp, pde) {
>> + if (pt != ppgtt->scratch_pt) {
>> + WARN_ON(bitmap_empty(pt->used_ptes, I915_PPGTT_PT_ENTRIES));
>> + continue;
>> + }
>> +
>> + /* We've already allocated a page table */
>> + WARN_ON(!bitmap_empty(pt->used_ptes, I915_PPGTT_PT_ENTRIES));
>> +
>> + pt = alloc_pt_single(dev);
>> + if (IS_ERR(pt)) {
>> + ret = PTR_ERR(pt);
>> + goto unwind_out;
>> + }
>> +
>> + ppgtt->pd.page_tables[pde] = pt;
>> + set_bit(pde, new_page_tables);
>> + trace_i915_pagetable_alloc(vm, pde, start, GEN6_PDE_SHIFT);
>> + }
>> +
>> + start = start_save;
>> + length = length_save;
>>
>> gen6_for_each_pde(pt, &ppgtt->pd, start, length, temp, pde) {
>> int j;
>> @@ -1159,12 +1196,35 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
>> }
>> }
>>
>> - bitmap_or(pt->used_ptes, pt->used_ptes, tmp_bitmap,
>> + if (test_and_clear_bit(pde, new_page_tables))
>> + gen6_write_pdes(&ppgtt->pd, pde, pt);
>> +
>> + trace_i915_pagetable_map(vm, pde, pt,
>> + gen6_pte_index(start),
>> + gen6_pte_count(start, length),
>> + I915_PPGTT_PT_ENTRIES);
>> + bitmap_or(pt->used_ptes, tmp_bitmap, pt->used_ptes,
>> I915_PPGTT_PT_ENTRIES);
>> }
>>
>> + WARN_ON(!bitmap_empty(new_page_tables, GEN6_PPGTT_PD_ENTRIES));
>> +
>> + /* Make sure write is complete before other code can use this page
>> + * table. Also require for WC mapped PTEs */
>> + readl(dev_priv->gtt.gsm);
>> +
>> ppgtt_invalidate_tlbs(vm);
>> return 0;
>> +
>> +unwind_out:
>> + for_each_set_bit(pde, new_page_tables, GEN6_PPGTT_PD_ENTRIES) {
>> + struct i915_pagetab *pt = ppgtt->pd.page_tables[pde];
>> + ppgtt->pd.page_tables[pde] = NULL;
>> + free_pt_single(pt, vm->dev);
>> + }
>> +
>> + ppgtt_invalidate_tlbs(vm);
>> + return ret;
>> }
>>
>> static void gen6_teardown_va_range(struct i915_address_space *vm,
>> @@ -1176,8 +1236,27 @@ static void gen6_teardown_va_range(struct i915_address_space *vm,
>> uint32_t pde, temp;
>>
>> gen6_for_each_pde(pt, &ppgtt->pd, start, length, temp, pde) {
>> +
>> + if (WARN(pt == ppgtt->scratch_pt,
>> + "Tried to teardown scratch page vm %p. pde %u: %llx-%llx\n",
>> + vm, pde, start, start + length))
>> + continue;
>> +
>> + trace_i915_pagetable_unmap(vm, pde, pt,
>> + gen6_pte_index(start),
>> + gen6_pte_count(start, length),
>> + I915_PPGTT_PT_ENTRIES);
>> +
>> bitmap_clear(pt->used_ptes, gen6_pte_index(start),
>> gen6_pte_count(start, length));
>> +
>> + if (bitmap_empty(pt->used_ptes, I915_PPGTT_PT_ENTRIES)) {
>> + trace_i915_pagetable_destroy(vm, pde,
>> + start & GENMASK_ULL(63, GEN6_PDE_SHIFT),
>> + GEN6_PDE_SHIFT);
>> + gen6_write_pdes(&ppgtt->pd, pde, ppgtt->scratch_pt);
>> + ppgtt->pd.page_tables[pde] = ppgtt->scratch_pt;
>> + }
>> }
>>
>> ppgtt_invalidate_tlbs(vm);
>> @@ -1187,9 +1266,13 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>> {
>> int i;
>>
>> - for (i = 0; i < ppgtt->num_pd_entries; i++)
>> - free_pt_single(ppgtt->pd.page_tables[i], ppgtt->base.dev);
>> + for (i = 0; i < ppgtt->num_pd_entries; i++) {
>> + struct i915_pagetab *pt = ppgtt->pd.page_tables[i];
>> + if (pt != ppgtt->scratch_pt)
>> + free_pt_single(ppgtt->pd.page_tables[i], ppgtt->base.dev);
>> + }
>>
>> + /* Consider putting this as part of pd free. */
>> free_pt_scratch(ppgtt->scratch_pt, ppgtt->base.dev);
>> free_pd_single(&ppgtt->pd);
>> }
>> @@ -1254,7 +1337,7 @@ err_out:
>> return ret;
>> }
>>
>> -static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
>> +static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt, bool preallocate_pt)
> Imo it would be clearer to move the pt preallocation for alising ppgtt
> into the ppgtt_init function. Makes for a bit a bigger diff, but will
> result in less convoluted control flow since we should end up in a nice
>
> if (alising)
> /* create all pts */
> else
> /* allocate&use scratch_pt */
>
> Aside: Should we only allocate the scratch_pt for !aliasing?
The next patch version will have the changes.
About the scratch_pt, I'm not sure if it's a requirement in gen6/7 (to
point unused page tables to the scratch, e.g. if there's less than 2GB).
We know in gen8 that's the case, and systems with less than 4GB must
have the remaining PDPs set to scratch page.
>
>> {
>> int ret;
>>
>> @@ -1262,10 +1345,14 @@ static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
>> if (ret)
>> return ret;
>>
>> + if (!preallocate_pt)
>> + return 0;
>> +
>> ret = alloc_pt_range(&ppgtt->pd, 0, ppgtt->num_pd_entries,
>> ppgtt->base.dev);
>>
>> if (ret) {
>> + free_pt_scratch(ppgtt->scratch_pt, ppgtt->base.dev);
>> drm_mm_remove_node(&ppgtt->node);
>> return ret;
>> }
>> @@ -1273,7 +1360,17 @@ static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
>> return 0;
>> }
>>
>> -static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>> +static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt,
>> + uint64_t start, uint64_t length)
>> +{
>> + struct i915_pagetab *unused;
>> + uint32_t pde, temp;
>> +
>> + gen6_for_each_pde(unused, &ppgtt->pd, start, length, temp, pde)
>> + ppgtt->pd.page_tables[pde] = ppgtt->scratch_pt;
>> +}
>> +
>> +static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>> {
>> struct drm_device *dev = ppgtt->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -1289,7 +1386,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>> } else
>> BUG();
>>
>> - ret = gen6_ppgtt_alloc(ppgtt);
>> + ret = gen6_ppgtt_alloc(ppgtt, aliasing);
>> if (ret)
>> return ret;
>>
>> @@ -1308,6 +1405,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>> ppgtt->pd_addr = (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm +
>> ppgtt->pd.pd_offset / sizeof(gen6_gtt_pte_t);
>>
>> + if (!aliasing)
>> + gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
>> +
>> gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total);
>>
>> DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n",
>> @@ -1320,7 +1420,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>> return 0;
>> }
>>
>> -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>> +static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
>> + bool aliasing)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> @@ -1328,7 +1429,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>> ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>>
>> if (INTEL_INFO(dev)->gen < 8)
>> - return gen6_ppgtt_init(ppgtt);
>> + return gen6_ppgtt_init(ppgtt, aliasing);
>> else
>> return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
>> }
>> @@ -1337,7 +1438,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> int ret = 0;
>>
>> - ret = __hw_ppgtt_init(dev, ppgtt);
>> + ret = __hw_ppgtt_init(dev, ppgtt, false);
>> if (ret == 0) {
>> kref_init(&ppgtt->ref);
>> drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
>> @@ -1445,9 +1546,14 @@ static void ppgtt_unbind_vma(struct i915_vma *vma)
>> vma->node.start,
>> vma->obj->base.size,
>> true);
>> - if (vma->vm->teardown_va_range)
>> + if (vma->vm->teardown_va_range) {
>> + trace_i915_va_teardown(vma->vm,
>> + vma->node.start, vma->node.size,
>> + VM_TO_TRACE_NAME(vma->vm));
>> +
>> vma->vm->teardown_va_range(vma->vm,
>> vma->node.start, vma->node.size);
>> + }
>> }
>>
>> extern int intel_iommu_gfx_mapped;
>> @@ -1963,7 +2069,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>> if (!ppgtt)
>> return -ENOMEM;
>>
>> - ret = __hw_ppgtt_init(dev, ppgtt);
>> + ret = __hw_ppgtt_init(dev, ppgtt, true);
>> if (ret != 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
>> index f004d3d..0b617c9 100644
>> --- a/drivers/gpu/drm/i915/i915_trace.h
>> +++ b/drivers/gpu/drm/i915/i915_trace.h
>> @@ -156,6 +156,121 @@ TRACE_EVENT(i915_vma_unbind,
>> __entry->obj, __entry->offset, __entry->size, __entry->vm)
>> );
>>
>> +#define VM_TO_TRACE_NAME(vm) \
>> + (i915_is_ggtt(vm) ? "GGTT" : \
>> + "Private VM")
>> +
>> +DECLARE_EVENT_CLASS(i915_va,
>> + TP_PROTO(struct i915_address_space *vm, u64 start, u64 length, const char *name),
>> + TP_ARGS(vm, start, length, name),
>> +
>> + TP_STRUCT__entry(
>> + __field(struct i915_address_space *, vm)
>> + __field(u64, start)
>> + __field(u64, end)
>> + __string(name, name)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->vm = vm;
>> + __entry->start = start;
>> + __entry->end = start + length;
>> + __assign_str(name, name);
>> + ),
>> +
>> + TP_printk("vm=%p (%s), 0x%llx-0x%llx",
>> + __entry->vm, __get_str(name), __entry->start, __entry->end)
>> +);
>> +
>> +DEFINE_EVENT(i915_va, i915_va_alloc,
>> + TP_PROTO(struct i915_address_space *vm, u64 start, u64 length, const char *name),
>> + TP_ARGS(vm, start, length, name)
>> +);
>> +
>> +DEFINE_EVENT(i915_va, i915_va_teardown,
>> + TP_PROTO(struct i915_address_space *vm, u64 start, u64 length, const char *name),
>> + TP_ARGS(vm, start, length, name)
>> +);
>> +
>> +DECLARE_EVENT_CLASS(i915_pagetable,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde, u64 start, u64 pde_shift),
>> + TP_ARGS(vm, pde, start, pde_shift),
>> +
>> + TP_STRUCT__entry(
>> + __field(struct i915_address_space *, vm)
>> + __field(u32, pde)
>> + __field(u64, start)
>> + __field(u64, end)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->vm = vm;
>> + __entry->pde = pde;
>> + __entry->start = start;
>> + __entry->end = (start + (1ULL << pde_shift)) & ~((1ULL << pde_shift)-1);
>> + ),
>> +
>> + TP_printk("vm=%p, pde=%d (0x%llx-0x%llx)",
>> + __entry->vm, __entry->pde, __entry->start, __entry->end)
>> +);
>> +
>> +DEFINE_EVENT(i915_pagetable, i915_pagetable_alloc,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde, u64 start, u64 pde_shift),
>> + TP_ARGS(vm, pde, start, pde_shift)
>> +);
>> +
>> +DEFINE_EVENT(i915_pagetable, i915_pagetable_destroy,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde, u64 start, u64 pde_shift),
>> + TP_ARGS(vm, pde, start, pde_shift)
>> +);
>> +
>> +/* Avoid extra math because we only support two sizes. The format is defined by
>> + * bitmap_scnprintf. Each 32 bits is 8 HEX digits followed by comma */
>> +#define TRACE_PT_SIZE(bits) \
>> + ((((bits) == 1024) ? 288 : 144) + 1)
>> +
>> +DECLARE_EVENT_CLASS(i915_pagetable_update,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde,
>> + struct i915_pagetab *pt, u32 first, u32 len, size_t bits),
>> + TP_ARGS(vm, pde, pt, first, len, bits),
>> +
>> + TP_STRUCT__entry(
>> + __field(struct i915_address_space *, vm)
>> + __field(u32, pde)
>> + __field(u32, first)
>> + __field(u32, last)
>> + __dynamic_array(char, cur_ptes, TRACE_PT_SIZE(bits))
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->vm = vm;
>> + __entry->pde = pde;
>> + __entry->first = first;
>> + __entry->last = first + len;
>> +
>> + bitmap_scnprintf(__get_str(cur_ptes),
>> + TRACE_PT_SIZE(bits),
>> + pt->used_ptes,
>> + bits);
>> + ),
>> +
>> + TP_printk("vm=%p, pde=%d, updating %u:%u\t%s",
>> + __entry->vm, __entry->pde, __entry->last, __entry->first,
>> + __get_str(cur_ptes))
>> +);
>> +
>> +DEFINE_EVENT(i915_pagetable_update, i915_pagetable_map,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde,
>> + struct i915_pagetab *pt, u32 first, u32 len, size_t bits),
>> + TP_ARGS(vm, pde, pt, first, len, bits)
>> +);
>> +
>> +DEFINE_EVENT(i915_pagetable_update, i915_pagetable_unmap,
>> + TP_PROTO(struct i915_address_space *vm, u32 pde,
>> + struct i915_pagetab *pt, u32 first, u32 len, size_t bits),
>> + TP_ARGS(vm, pde, pt, first, len, bits)
>> +);
>> +
>> TRACE_EVENT(i915_gem_object_change_domain,
>> TP_PROTO(struct drm_i915_gem_object *obj, u32 old_read, u32 old_write),
>> TP_ARGS(obj, old_read, old_write),
>> --
>> 2.1.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5510 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150113/6149114a/attachment.bin>
More information about the Intel-gfx
mailing list