On 23/07/2019 19:38, Chris Wilson wrote:
The aliasing_ppgtt provides a PIN_USER alias for the global gtt, so move
it under the i915_ggtt to simplify later transformations to enable
intel_context.vm.
Can you pin point what exactly it makes easier in the following patch?
Just the __context_pin_ppgtt change? I have reservations about ggtt
embedding aliasing ppgtt. But I guess it is handy for some usages.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 +-
.../drm/i915/gem/selftests/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 69 ++++++++++++-------
drivers/gpu/drm/i915/i915_drv.h | 3 -
drivers/gpu/drm/i915/i915_gem_gtt.c | 36 +++++-----
drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +
drivers/gpu/drm/i915/i915_vma.c | 2 +-
7 files changed, 71 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ffb59d96d4d8..0f6b0678f548 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -459,8 +459,7 @@ __create_context(struct drm_i915_private *i915)
i915_gem_context_set_recoverable(ctx);
ctx->ring_size = 4 * PAGE_SIZE;
- ctx->desc_template =
- default_desc_template(i915, &i915->mm.aliasing_ppgtt->vm);
+ ctx->desc_template = default_desc_template(i915, NULL);
for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
@@ -2258,8 +2257,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
args->size = 0;
if (ctx->vm)
args->value = ctx->vm->total;
- else if (to_i915(dev)->mm.aliasing_ppgtt)
- args->value = to_i915(dev)->mm.aliasing_ppgtt->vm.total;
+ else if (to_i915(dev)->ggtt.alias)
+ args->value = to_i915(dev)->ggtt.alias->vm.total;
else
args->value = to_i915(dev)->ggtt.vm.total;
break;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index db7856f0f31e..bbd17d4b8ffd 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -1190,7 +1190,7 @@ static int igt_ctx_readonly(void *arg)
goto out_unlock;
}
- vm = ctx->vm ?: &i915->mm.aliasing_ppgtt->vm;
+ vm = ctx->vm ?: &i915->ggtt.alias->vm;
if (!vm || !vm->has_read_only) {
err = 0;
goto out_unlock;
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index 1de19dac4a14..b056f25c66f2 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1392,30 +1392,41 @@ static void ring_context_destroy(struct kref *ref)
intel_context_free(ce);
}
-static int __context_pin_ppgtt(struct i915_gem_context *ctx)
+static struct i915_address_space *vm_alias(struct intel_context *ce)
+{
+ struct i915_address_space *vm;
+
+ vm = ce->gem_context->vm;
+ if (!vm)
+ vm = &ce->engine->gt->ggtt->alias->vm;
+
+ return vm;
vm_or_alias? Still not good.. get_vm might pass since it is local?
+}
+
+static int __context_pin_ppgtt(struct intel_context *ce)
{
struct i915_address_space *vm;
int err = 0;
- vm = ctx->vm ?: &ctx->i915->mm.aliasing_ppgtt->vm;
+ vm = vm_alias(ce);
if (vm)
Can't return NULL it seems. (Same below.)
err = gen6_ppgtt_pin(i915_vm_to_ppgtt((vm)));
return err;
}
-static void __context_unpin_ppgtt(struct i915_gem_context *ctx)
+static void __context_unpin_ppgtt(struct intel_context *ce)
{
struct i915_address_space *vm;
- vm = ctx->vm ?: &ctx->i915->mm.aliasing_ppgtt->vm;
+ vm = vm_alias(ce);
if (vm)
gen6_ppgtt_unpin(i915_vm_to_ppgtt(vm));
}
static void ring_context_unpin(struct intel_context *ce)
{
- __context_unpin_ppgtt(ce->gem_context);
+ __context_unpin_ppgtt(ce);
}
static struct i915_vma *
@@ -1509,7 +1520,7 @@ static int ring_context_pin(struct intel_context *ce)
if (err)
return err;
- err = __context_pin_ppgtt(ce->gem_context);
+ err = __context_pin_ppgtt(ce);
if (err)
goto err_active;
@@ -1701,7 +1712,7 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
return 0;
}
-static int remap_l3(struct i915_request *rq, int slice)
+static int remap_l3_slice(struct i915_request *rq, int slice)
{
u32 *cs, *remap_info = rq->i915->l3_parity.remap_info[slice];
int i;
@@ -1729,15 +1740,34 @@ static int remap_l3(struct i915_request *rq, int slice)
return 0;
}
+static int remap_l3(struct i915_request *rq)
+{
+ struct i915_gem_context *ctx = rq->gem_context;
+ int i, err;
+
+ if (!ctx->remap_slice)
+ return 0;
+
+ for (i = 0; i < MAX_L3_SLICES; i++) {
err declaration could go here but meh..
+ if (!(ctx->remap_slice & BIT(i)))
+ continue;
+
+ err = remap_l3_slice(rq, i);
+ if (err)
+ return err;
... or could stay at top and here you break and return err at the end.
More meh. Depending whether it is important or not to clear
ctx->remap_slice on error.
+ }
+
+ ctx->remap_slice = 0;
+ return 0;
+}
+
static int switch_context(struct i915_request *rq)
{
struct intel_engine_cs *engine = rq->engine;
- struct i915_gem_context *ctx = rq->gem_context;
- struct i915_address_space *vm =
- ctx->vm ?: &rq->i915->mm.aliasing_ppgtt->vm;
+ struct i915_address_space *vm = vm_alias(rq->hw_context);
unsigned int unwind_mm = 0;
u32 hw_flags = 0;
- int ret, i;
+ int ret;
GEM_BUG_ON(HAS_EXECLISTS(rq->i915));
@@ -1781,7 +1811,7 @@ static int switch_context(struct i915_request *rq)
* as nothing actually executes using the kernel context; it
* is purely used for flushing user contexts.
*/
- if (i915_gem_context_is_kernel(ctx))
+ if (i915_gem_context_is_kernel(rq->gem_context))
hw_flags = MI_RESTORE_INHIBIT;
ret = mi_set_context(rq, hw_flags);
@@ -1815,18 +1845,9 @@ static int switch_context(struct i915_request *rq)
goto err_mm;
}
- if (ctx->remap_slice) {
- for (i = 0; i < MAX_L3_SLICES; i++) {
- if (!(ctx->remap_slice & BIT(i)))
- continue;
-
- ret = remap_l3(rq, i);
- if (ret)
- goto err_mm;
- }
-
- ctx->remap_slice = 0;
- }
+ ret = remap_l3(rq);
+ if (ret)
+ goto err_mm;
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0e44cc4b2ca1..269a1b32b48b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -784,9 +784,6 @@ struct i915_gem_mm {
*/
struct vfsmount *gemfs;
- /** PPGTT used for aliasing the PPGTT with the GTT */
- struct i915_ppgtt *aliasing_ppgtt;
-
struct notifier_block oom_notifier;
struct notifier_block vmap_notifier;
struct shrinker shrinker;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4dd1fa956143..8304b98b0bf8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2446,18 +2446,18 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
pte_flags |= PTE_READ_ONLY;
if (flags & I915_VMA_LOCAL_BIND) {
- struct i915_ppgtt *appgtt = i915->mm.aliasing_ppgtt;
+ struct i915_ppgtt *alias = i915_vm_to_ggtt(vma->vm)->alias;
Keeping the variable name would have reduced the churn.
if (!(vma->flags & I915_VMA_LOCAL_BIND)) {
- ret = appgtt->vm.allocate_va_range(&appgtt->vm,
- vma->node.start,
- vma->size);
+ ret = alias->vm.allocate_va_range(&alias->vm,
+ vma->node.start,
+ vma->size);
if (ret)
return ret;
}
- appgtt->vm.insert_entries(&appgtt->vm, vma, cache_level,
- pte_flags);
+ alias->vm.insert_entries(&alias->vm, vma,
+ cache_level, pte_flags);
}
if (flags & I915_VMA_GLOBAL_BIND) {
@@ -2485,7 +2485,8 @@ static void aliasing_gtt_unbind_vma(struct i915_vma *vma)
}
if (vma->flags & I915_VMA_LOCAL_BIND) {
- struct i915_address_space *vm = &i915->mm.aliasing_ppgtt->vm;
+ struct i915_address_space *vm =
+ &i915_vm_to_ggtt(vma->vm)->alias->vm;
vm->clear_range(vm, vma->node.start, vma->size);
}
@@ -2542,13 +2543,12 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
*end -= I915_GTT_PAGE_SIZE;
}
-static int init_aliasing_ppgtt(struct drm_i915_private *i915)
+static int init_aliasing_ppgtt(struct i915_ggtt *ggtt)
{
- struct i915_ggtt *ggtt = &i915->ggtt;
struct i915_ppgtt *ppgtt;
int err;
- ppgtt = i915_ppgtt_create(i915);
+ ppgtt = i915_ppgtt_create(ggtt->vm.i915);
if (IS_ERR(ppgtt))
return PTR_ERR(ppgtt);
@@ -2567,7 +2567,7 @@ static int init_aliasing_ppgtt(struct drm_i915_private *i915)
if (err)
goto err_ppgtt;
- i915->mm.aliasing_ppgtt = ppgtt;
+ ggtt->alias = ppgtt;
GEM_BUG_ON(ggtt->vm.vma_ops.bind_vma != ggtt_bind_vma);
ggtt->vm.vma_ops.bind_vma = aliasing_gtt_bind_vma;
@@ -2582,14 +2582,14 @@ static int init_aliasing_ppgtt(struct drm_i915_private *i915)
return err;
}
-static void fini_aliasing_ppgtt(struct drm_i915_private *i915)
+static void fini_aliasing_ppgtt(struct i915_ggtt *ggtt)
{
- struct i915_ggtt *ggtt = &i915->ggtt;
+ struct drm_i915_private *i915 = ggtt->vm.i915;
struct i915_ppgtt *ppgtt;
mutex_lock(&i915->drm.struct_mutex);
- ppgtt = fetch_and_zero(&i915->mm.aliasing_ppgtt);
+ ppgtt = fetch_and_zero(&ggtt->alias);
if (!ppgtt)
goto out;
@@ -2706,7 +2706,7 @@ int i915_init_ggtt(struct drm_i915_private *i915)
return ret;
if (INTEL_PPGTT(i915) == INTEL_PPGTT_ALIASING) {
- ret = init_aliasing_ppgtt(i915);
+ ret = init_aliasing_ppgtt(&i915->ggtt);
if (ret)
cleanup_init_ggtt(&i915->ggtt);
}
@@ -2752,7 +2752,7 @@ void i915_ggtt_driver_release(struct drm_i915_private *i915)
{
struct pagevec *pvec;
- fini_aliasing_ppgtt(i915);
+ fini_aliasing_ppgtt(&i915->ggtt);
ggtt_cleanup_hw(&i915->ggtt);
@@ -3585,7 +3585,7 @@ int i915_gem_gtt_reserve(struct i915_address_space *vm,
GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_PAGE_SIZE));
GEM_BUG_ON(!IS_ALIGNED(offset, I915_GTT_MIN_ALIGNMENT));
GEM_BUG_ON(range_overflows(offset, size, vm->total));
- GEM_BUG_ON(vm == &vm->i915->mm.aliasing_ppgtt->vm);
+ GEM_BUG_ON(vm == &vm->i915->ggtt.alias->vm);
GEM_BUG_ON(drm_mm_node_allocated(node));
node->size = size;
@@ -3682,7 +3682,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
GEM_BUG_ON(start >= end);
GEM_BUG_ON(start > 0 && !IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
GEM_BUG_ON(end < U64_MAX && !IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
- GEM_BUG_ON(vm == &vm->i915->mm.aliasing_ppgtt->vm);
+ GEM_BUG_ON(vm == &vm->i915->ggtt.alias->vm);
GEM_BUG_ON(drm_mm_node_allocated(node));
if (unlikely(range_overflows(start, size, end)))
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index cea59ef1a365..51274483502e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -394,6 +394,9 @@ struct i915_ggtt {
void __iomem *gsm;
void (*invalidate)(struct i915_ggtt *ggtt);
+ /** PPGTT used for aliasing the PPGTT with the GTT */
+ struct i915_ppgtt *alias;
+
bool do_idle_maps;
int mtrr;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index ee73baf29415..eb16a1a93bbc 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -104,7 +104,7 @@ vma_create(struct drm_i915_gem_object *obj,
struct rb_node *rb, **p;
/* The aliasing_ppgtt should never be used directly! */
- GEM_BUG_ON(vm == &vm->i915->mm.aliasing_ppgtt->vm);
+ GEM_BUG_ON(vm == &vm->i915->ggtt.alias->vm);
vma = i915_vma_alloc();
if (vma == NULL)
Nothing but nitpicks, looks okay in principle.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
