[PATCH 1/5] drm/i915/execlists: Set priority hint prior to submission

[PATCH 1/5] drm/i915/execlists: Set priority hint prior to submission

[Chris Wilson]

Since we now run process_csb() outside of the engine->active.lock, we
can process a CS-event immediately upon our ELSP write. As we currently
inspect the pending queue *after* the ELSP write, there is an
opportunity for a CS-event to update the pending queue before we can
read it, making ourselves chases an invalid pointer.

Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 44780e7fafec..d42584439f51 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1335,9 +1335,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	if (submit) {
 		*port = execlists_schedule_in(last, port - execlists->pending);
 		memset(port + 1, 0, (last_port - port) * sizeof(*port));
-		execlists_submit_ports(engine);
 		execlists->switch_priority_hint =
 			switch_prio(engine, *execlists->pending);
+		execlists_submit_ports(engine);
 	} else {
 		ring_set_paused(engine, 0);
 	}
-- 
2.23.0.rc1

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

[PATCH 2/5] drm/i915/gtt: Add some range asserts

[Chris Wilson]

These should have been validated in the upper layers, but for sanity's
sake, repeat them.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b06d1d9054ba..0b81e0b64393 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -965,8 +965,10 @@ static u64 __gen8_ppgtt_clear(struct i915_address_space * const vm,
 	const struct i915_page_scratch * const scratch = &vm->scratch[lvl];
 	unsigned int idx, len;
 
+	GEM_BUG_ON(end > vm->total >> GEN8_PTE_SHIFT);
+
 	len = gen8_pd_range(start, end, lvl--, &idx);
-	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d}\n",
+	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d }\n",
 	    __func__, vm, lvl + 1, start, end,
 	    idx, len, atomic_read(px_used(pd)));
 	GEM_BUG_ON(!len || len >= atomic_read(px_used(pd)));
@@ -992,7 +994,7 @@ static u64 __gen8_ppgtt_clear(struct i915_address_space * const vm,
 			u64 *vaddr;
 
 			count = gen8_pt_count(start, end);
-			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d} removing pte\n",
+			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d } removing pte\n",
 			    __func__, vm, lvl, start, end,
 			    gen8_pd_index(start, 0), count,
 			    atomic_read(&pt->used));
@@ -1020,6 +1022,7 @@ static void gen8_ppgtt_clear(struct i915_address_space *vm,
 {
 	GEM_BUG_ON(!IS_ALIGNED(start, BIT_ULL(GEN8_PTE_SHIFT)));
 	GEM_BUG_ON(!IS_ALIGNED(length, BIT_ULL(GEN8_PTE_SHIFT)));
+	GEM_BUG_ON(range_overflows(start, length, vm->total));
 
 	start >>= GEN8_PTE_SHIFT;
 	length >>= GEN8_PTE_SHIFT;
@@ -1031,15 +1034,17 @@ static void gen8_ppgtt_clear(struct i915_address_space *vm,
 
 static int __gen8_ppgtt_alloc(struct i915_address_space * const vm,
 			      struct i915_page_directory * const pd,
-			      u64 * const start, u64 end, int lvl)
+			      u64 * const start, const u64 end, int lvl)
 {
 	const struct i915_page_scratch * const scratch = &vm->scratch[lvl];
 	struct i915_page_table *alloc = NULL;
 	unsigned int idx, len;
 	int ret = 0;
 
+	GEM_BUG_ON(end > vm->total >> GEN8_PTE_SHIFT);
+
 	len = gen8_pd_range(*start, end, lvl--, &idx);
-	DBG("%s(%p):{lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d}\n",
+	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d }\n",
 	    __func__, vm, lvl + 1, *start, end,
 	    idx, len, atomic_read(px_used(pd)));
 	GEM_BUG_ON(!len || (idx + len - 1) >> gen8_pd_shift(1));
@@ -1105,7 +1110,7 @@ static int __gen8_ppgtt_alloc(struct i915_address_space * const vm,
 		} else {
 			unsigned int count = gen8_pt_count(*start, end);
 
-			DBG("%s(%p):{lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d} inserting pte\n",
+			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d } inserting pte\n",
 			    __func__, vm, lvl, *start, end,
 			    gen8_pd_index(*start, 0), count,
 			    atomic_read(&pt->used));
@@ -1131,6 +1136,7 @@ static int gen8_ppgtt_alloc(struct i915_address_space *vm,
 
 	GEM_BUG_ON(!IS_ALIGNED(start, BIT_ULL(GEN8_PTE_SHIFT)));
 	GEM_BUG_ON(!IS_ALIGNED(length, BIT_ULL(GEN8_PTE_SHIFT)));
+	GEM_BUG_ON(range_overflows(start, length, vm->total));
 
 	start >>= GEN8_PTE_SHIFT;
 	length >>= GEN8_PTE_SHIFT;
-- 
2.23.0.rc1

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

Re: [PATCH 2/5] drm/i915/gtt: Add some range asserts

[Mika Kuoppala]

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

> These should have been validated in the upper layers, but for sanity's
> sake, repeat them.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b06d1d9054ba..0b81e0b64393 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -965,8 +965,10 @@ static u64 __gen8_ppgtt_clear(struct i915_address_space * const vm,
>  	const struct i915_page_scratch * const scratch = &vm->scratch[lvl];
>  	unsigned int idx, len;
>  
> +	GEM_BUG_ON(end > vm->total >> GEN8_PTE_SHIFT);

I am having thoughts to clean all underlying handling to
use page indexes consistently...

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +
>  	len = gen8_pd_range(start, end, lvl--, &idx);
> -	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d}\n",
> +	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d }\n",
>  	    __func__, vm, lvl + 1, start, end,
>  	    idx, len, atomic_read(px_used(pd)));
>  	GEM_BUG_ON(!len || len >= atomic_read(px_used(pd)));
> @@ -992,7 +994,7 @@ static u64 __gen8_ppgtt_clear(struct i915_address_space * const vm,
>  			u64 *vaddr;
>  
>  			count = gen8_pt_count(start, end);
> -			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d} removing pte\n",
> +			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d } removing pte\n",
>  			    __func__, vm, lvl, start, end,
>  			    gen8_pd_index(start, 0), count,
>  			    atomic_read(&pt->used));
> @@ -1020,6 +1022,7 @@ static void gen8_ppgtt_clear(struct i915_address_space *vm,
>  {
>  	GEM_BUG_ON(!IS_ALIGNED(start, BIT_ULL(GEN8_PTE_SHIFT)));
>  	GEM_BUG_ON(!IS_ALIGNED(length, BIT_ULL(GEN8_PTE_SHIFT)));
> +	GEM_BUG_ON(range_overflows(start, length, vm->total));
>  
>  	start >>= GEN8_PTE_SHIFT;
>  	length >>= GEN8_PTE_SHIFT;
> @@ -1031,15 +1034,17 @@ static void gen8_ppgtt_clear(struct i915_address_space *vm,
>  
>  static int __gen8_ppgtt_alloc(struct i915_address_space * const vm,
>  			      struct i915_page_directory * const pd,
> -			      u64 * const start, u64 end, int lvl)
> +			      u64 * const start, const u64 end, int lvl)
>  {
>  	const struct i915_page_scratch * const scratch = &vm->scratch[lvl];
>  	struct i915_page_table *alloc = NULL;
>  	unsigned int idx, len;
>  	int ret = 0;
>  
> +	GEM_BUG_ON(end > vm->total >> GEN8_PTE_SHIFT);
> +
>  	len = gen8_pd_range(*start, end, lvl--, &idx);
> -	DBG("%s(%p):{lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d}\n",
> +	DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d }\n",
>  	    __func__, vm, lvl + 1, *start, end,
>  	    idx, len, atomic_read(px_used(pd)));
>  	GEM_BUG_ON(!len || (idx + len - 1) >> gen8_pd_shift(1));
> @@ -1105,7 +1110,7 @@ static int __gen8_ppgtt_alloc(struct i915_address_space * const vm,
>  		} else {
>  			unsigned int count = gen8_pt_count(*start, end);
>  
> -			DBG("%s(%p):{lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d} inserting pte\n",
> +			DBG("%s(%p):{ lvl:%d, start:%llx, end:%llx, idx:%d, len:%d, used:%d } inserting pte\n",
>  			    __func__, vm, lvl, *start, end,
>  			    gen8_pd_index(*start, 0), count,
>  			    atomic_read(&pt->used));
> @@ -1131,6 +1136,7 @@ static int gen8_ppgtt_alloc(struct i915_address_space *vm,
>  
>  	GEM_BUG_ON(!IS_ALIGNED(start, BIT_ULL(GEN8_PTE_SHIFT)));
>  	GEM_BUG_ON(!IS_ALIGNED(length, BIT_ULL(GEN8_PTE_SHIFT)));
> +	GEM_BUG_ON(range_overflows(start, length, vm->total));
>  
>  	start >>= GEN8_PTE_SHIFT;
>  	length >>= GEN8_PTE_SHIFT;
> -- 
> 2.23.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/gtt: Add some range asserts

[Chris Wilson]

Quoting Mika Kuoppala (2019-08-21 17:24:05)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > These should have been validated in the upper layers, but for sanity's
> > sake, repeat them.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index b06d1d9054ba..0b81e0b64393 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -965,8 +965,10 @@ static u64 __gen8_ppgtt_clear(struct i915_address_space * const vm,
> >       const struct i915_page_scratch * const scratch = &vm->scratch[lvl];
> >       unsigned int idx, len;
> >  
> > +     GEM_BUG_ON(end > vm->total >> GEN8_PTE_SHIFT);
> 
> I am having thoughts to clean all underlying handling to
> use page indexes consistently...

I would agree, pfn throughout does make a certain sense for the page
directory trees.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Track ggtt fence reservations under its own mutex

[Chris Wilson]

We can reduce the locking for fence registers from the dev->struct_mutex
to a local mutex. We could introduce a mutex for the sole purpose of
tracking the fence acquisition, except there is a little bit of overlap
with the fault tracking, so use the i915_ggtt.mutex as it covers both.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c |   7 ++
 drivers/gpu/drm/i915/gvt/aperture_gm.c       |  10 +-
 drivers/gpu/drm/i915/i915_debugfs.c          |   5 +-
 drivers/gpu/drm/i915/i915_gem_fence_reg.c    | 108 ++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_fence_reg.h    |   2 +-
 drivers/gpu/drm/i915/i915_vma.h              |   4 +-
 6 files changed, 87 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 298c4d191439..a0098fc35921 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -1157,7 +1157,14 @@ static int evict_fence(void *data)
 		goto out_unlock;
 	}
 
+	err = i915_vma_pin(arg->vma, 0, 0, PIN_GLOBAL | PIN_MAPPABLE);
+	if (err) {
+		pr_err("Unable to pin vma for Y-tiled fence; err:%d\n", err);
+		goto out_unlock;
+	}
+
 	err = i915_vma_pin_fence(arg->vma);
+	i915_vma_unpin(arg->vma);
 	if (err) {
 		pr_err("Unable to pin Y-tiled fence; err:%d\n", err);
 		goto out_unlock;
diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c b/drivers/gpu/drm/i915/gvt/aperture_gm.c
index c3d19d88da40..5ff2437b2998 100644
--- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
+++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
@@ -172,14 +172,14 @@ static void free_vgpu_fence(struct intel_vgpu *vgpu)
 
 	intel_runtime_pm_get(&dev_priv->runtime_pm);
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
+	mutex_lock(&dev_priv->ggtt.vm.mutex);
 	_clear_vgpu_fence(vgpu);
 	for (i = 0; i < vgpu_fence_sz(vgpu); i++) {
 		reg = vgpu->fence.regs[i];
 		i915_unreserve_fence(reg);
 		vgpu->fence.regs[i] = NULL;
 	}
-	mutex_unlock(&dev_priv->drm.struct_mutex);
+	mutex_unlock(&dev_priv->ggtt.vm.mutex);
 
 	intel_runtime_pm_put_unchecked(&dev_priv->runtime_pm);
 }
@@ -195,7 +195,7 @@ static int alloc_vgpu_fence(struct intel_vgpu *vgpu)
 	intel_runtime_pm_get(rpm);
 
 	/* Request fences from host */
-	mutex_lock(&dev_priv->drm.struct_mutex);
+	mutex_lock(&dev_priv->ggtt.vm.mutex);
 
 	for (i = 0; i < vgpu_fence_sz(vgpu); i++) {
 		reg = i915_reserve_fence(dev_priv);
@@ -207,7 +207,7 @@ static int alloc_vgpu_fence(struct intel_vgpu *vgpu)
 
 	_clear_vgpu_fence(vgpu);
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
+	mutex_unlock(&dev_priv->ggtt.vm.mutex);
 	intel_runtime_pm_put_unchecked(rpm);
 	return 0;
 out_free_fence:
@@ -220,7 +220,7 @@ static int alloc_vgpu_fence(struct intel_vgpu *vgpu)
 		i915_unreserve_fence(reg);
 		vgpu->fence.regs[i] = NULL;
 	}
-	mutex_unlock(&dev_priv->drm.struct_mutex);
+	mutex_unlock(&dev_priv->ggtt.vm.mutex);
 	intel_runtime_pm_put_unchecked(rpm);
 	return -ENOSPC;
 }
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b39226d7f8d2..f5d6702ec7df 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -652,10 +652,11 @@ static int i915_gem_fence_regs_info(struct seq_file *m, void *data)
 
 	rcu_read_lock();
 	for (i = 0; i < i915->ggtt.num_fences; i++) {
-		struct i915_vma *vma = i915->ggtt.fence_regs[i].vma;
+		struct i915_fence_reg *reg = &i915->ggtt.fence_regs[i];
+		struct i915_vma *vma = reg->vma;
 
 		seq_printf(m, "Fence %d, pin count = %d, object = ",
-			   i, i915->ggtt.fence_regs[i].pin_count);
+			   i, atomic_read(&reg->pin_count));
 		if (!vma)
 			seq_puts(m, "unused");
 		else
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index c9654f1a468f..6a33a0bb97a9 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -299,15 +299,24 @@ static int fence_update(struct i915_fence_reg *fence,
  */
 int i915_vma_put_fence(struct i915_vma *vma)
 {
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vma->vm);
 	struct i915_fence_reg *fence = vma->fence;
+	int err;
 
 	if (!fence)
 		return 0;
 
-	if (fence->pin_count)
+	if (atomic_read(&fence->pin_count))
 		return -EBUSY;
 
-	return fence_update(fence, NULL);
+	err = mutex_lock_interruptible(&ggtt->vm.mutex);
+	if (err)
+		return err;
+
+	err = fence_update(fence, NULL);
+	mutex_unlock(&ggtt->vm.mutex);
+
+	return err;
 }
 
 static struct i915_fence_reg *fence_find(struct drm_i915_private *i915)
@@ -317,7 +326,7 @@ static struct i915_fence_reg *fence_find(struct drm_i915_private *i915)
 	list_for_each_entry(fence, &i915->ggtt.fence_list, link) {
 		GEM_BUG_ON(fence->vma && fence->vma->fence != fence);
 
-		if (fence->pin_count)
+		if (atomic_read(&fence->pin_count))
 			continue;
 
 		return fence;
@@ -330,6 +339,48 @@ static struct i915_fence_reg *fence_find(struct drm_i915_private *i915)
 	return ERR_PTR(-EDEADLK);
 }
 
+static int __i915_vma_pin_fence(struct i915_vma *vma)
+{
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vma->vm);
+	struct i915_fence_reg *fence;
+	struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
+	int err;
+
+	/* Just update our place in the LRU if our fence is getting reused. */
+	if (vma->fence) {
+		fence = vma->fence;
+		GEM_BUG_ON(fence->vma != vma);
+		atomic_inc(&fence->pin_count);
+		if (!fence->dirty) {
+			list_move_tail(&fence->link, &ggtt->fence_list);
+			return 0;
+		}
+	} else if (set) {
+		fence = fence_find(vma->vm->i915);
+		if (IS_ERR(fence))
+			return PTR_ERR(fence);
+
+		GEM_BUG_ON(atomic_read(&fence->pin_count));
+		atomic_inc(&fence->pin_count);
+	} else {
+		return 0;
+	}
+
+	err = fence_update(fence, set);
+	if (err)
+		goto out_unpin;
+
+	GEM_BUG_ON(fence->vma != set);
+	GEM_BUG_ON(vma->fence != (set ? fence : NULL));
+
+	if (set)
+		return 0;
+
+out_unpin:
+	atomic_dec(&fence->pin_count);
+	return err;
+}
+
 /**
  * i915_vma_pin_fence - set up fencing for a vma
  * @vma: vma to map through a fence reg
@@ -350,8 +401,6 @@ static struct i915_fence_reg *fence_find(struct drm_i915_private *i915)
  */
 int i915_vma_pin_fence(struct i915_vma *vma)
 {
-	struct i915_fence_reg *fence;
-	struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
 	int err;
 
 	/*
@@ -359,39 +408,16 @@ int i915_vma_pin_fence(struct i915_vma *vma)
 	 * must keep the device awake whilst using the fence.
 	 */
 	assert_rpm_wakelock_held(&vma->vm->i915->runtime_pm);
+	GEM_BUG_ON(!i915_vma_is_pinned(vma));
+	GEM_BUG_ON(!i915_vma_is_ggtt(vma));
 
-	/* Just update our place in the LRU if our fence is getting reused. */
-	if (vma->fence) {
-		fence = vma->fence;
-		GEM_BUG_ON(fence->vma != vma);
-		fence->pin_count++;
-		if (!fence->dirty) {
-			list_move_tail(&fence->link,
-				       &fence->i915->ggtt.fence_list);
-			return 0;
-		}
-	} else if (set) {
-		fence = fence_find(vma->vm->i915);
-		if (IS_ERR(fence))
-			return PTR_ERR(fence);
-
-		GEM_BUG_ON(fence->pin_count);
-		fence->pin_count++;
-	} else
-		return 0;
-
-	err = fence_update(fence, set);
+	err = mutex_lock_interruptible(&vma->vm->mutex);
 	if (err)
-		goto out_unpin;
+		return err;
 
-	GEM_BUG_ON(fence->vma != set);
-	GEM_BUG_ON(vma->fence != (set ? fence : NULL));
-
-	if (set)
-		return 0;
+	err = __i915_vma_pin_fence(vma);
+	mutex_unlock(&vma->vm->mutex);
 
-out_unpin:
-	fence->pin_count--;
 	return err;
 }
 
@@ -404,16 +430,17 @@ int i915_vma_pin_fence(struct i915_vma *vma)
  */
 struct i915_fence_reg *i915_reserve_fence(struct drm_i915_private *i915)
 {
+	struct i915_ggtt *ggtt = &i915->ggtt;
 	struct i915_fence_reg *fence;
 	int count;
 	int ret;
 
-	lockdep_assert_held(&i915->drm.struct_mutex);
+	lockdep_assert_held(&ggtt->vm.mutex);
 
 	/* Keep at least one fence available for the display engine. */
 	count = 0;
-	list_for_each_entry(fence, &i915->ggtt.fence_list, link)
-		count += !fence->pin_count;
+	list_for_each_entry(fence, &ggtt->fence_list, link)
+		count += !atomic_read(&fence->pin_count);
 	if (count <= 1)
 		return ERR_PTR(-ENOSPC);
 
@@ -429,6 +456,7 @@ struct i915_fence_reg *i915_reserve_fence(struct drm_i915_private *i915)
 	}
 
 	list_del(&fence->link);
+
 	return fence;
 }
 
@@ -440,9 +468,11 @@ struct i915_fence_reg *i915_reserve_fence(struct drm_i915_private *i915)
  */
 void i915_unreserve_fence(struct i915_fence_reg *fence)
 {
-	lockdep_assert_held(&fence->i915->drm.struct_mutex);
+	struct i915_ggtt *ggtt = &fence->i915->ggtt;
+
+	lockdep_assert_held(&ggtt->vm.mutex);
 
-	list_add(&fence->link, &fence->i915->ggtt.fence_list);
+	list_add(&fence->link, &ggtt->fence_list);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.h b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
index 37e4f104f7c0..99866fb9d94f 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.h
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.h
@@ -41,7 +41,7 @@ struct i915_fence_reg {
 	struct list_head link;
 	struct drm_i915_private *i915;
 	struct i915_vma *vma;
-	int pin_count;
+	atomic_t pin_count;
 	int id;
 	/**
 	 * Whether the tiling parameters for the currently
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 9e7d8f4154b2..cf6c0437091d 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -426,8 +426,8 @@ int __must_check i915_vma_put_fence(struct i915_vma *vma);
 
 static inline void __i915_vma_unpin_fence(struct i915_vma *vma)
 {
-	GEM_BUG_ON(vma->fence->pin_count <= 0);
-	vma->fence->pin_count--;
+	GEM_BUG_ON(atomic_read(&vma->fence->pin_count) <= 0);
+	atomic_dec(&vma->fence->pin_count);
 }
 
 /**
-- 
2.23.0.rc1

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

Re: [PATCH 3/5] drm/i915: Track ggtt fence reservations under its own mutex

[Matthew Auld]

On Wed, 21 Aug 2019 at 16:58, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> We can reduce the locking for fence registers from the dev->struct_mutex
> to a local mutex. We could introduce a mutex for the sole purpose of
> tracking the fence acquisition, except there is a little bit of overlap
> with the fault tracking, so use the i915_ggtt.mutex as it covers both.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Pull obj->userfault tracking under the ggtt->mutex

[Chris Wilson]

Since we want to revoke the ggtt vma from only under the ggtt->mutex, we
need to move protection of the userfault tracking from the struct_mutex
to the ggtt->mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 10 +++++++---
 drivers/gpu/drm/i915/i915_debugfs.c      |  2 +-
 drivers/gpu/drm/i915/i915_vma.c          |  4 +++-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index dba5dd779149..595539a09e38 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -306,14 +306,17 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 	if (ret)
 		goto err_fence;
 
-	/* Mark as being mmapped into userspace for later revocation */
 	assert_rpm_wakelock_held(rpm);
+
+	/* Mark as being mmapped into userspace for later revocation */
+	mutex_lock(&i915->ggtt.vm.mutex);
 	if (!i915_vma_set_userfault(vma) && !obj->userfault_count++)
 		list_add(&obj->userfault_link, &i915->ggtt.userfault_list);
+	mutex_unlock(&i915->ggtt.vm.mutex);
+
 	if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)
 		intel_wakeref_auto(&i915->ggtt.userfault_wakeref,
 				   msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
-	GEM_BUG_ON(!obj->userfault_count);
 
 	i915_vma_set_ggtt_write(vma);
 
@@ -408,8 +411,8 @@ void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
 	 * requirement that operations to the GGTT be made holding the RPM
 	 * wakeref.
 	 */
-	lockdep_assert_held(&i915->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+	mutex_lock(&i915->ggtt.vm.mutex);
 
 	if (!obj->userfault_count)
 		goto out;
@@ -426,6 +429,7 @@ void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
 	wmb();
 
 out:
+	mutex_unlock(&i915->ggtt.vm.mutex);
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f5d6702ec7df..b0f51591f2e4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -94,7 +94,7 @@ static char get_tiling_flag(struct drm_i915_gem_object *obj)
 
 static char get_global_flag(struct drm_i915_gem_object *obj)
 {
-	return obj->userfault_count ? 'g' : ' ';
+	return READ_ONCE(obj->userfault_count) ? 'g' : ' ';
 }
 
 static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 79f9d1fb7611..9840cb2f70b9 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -864,7 +864,7 @@ void i915_vma_revoke_mmap(struct i915_vma *vma)
 	struct drm_vma_offset_node *node = &vma->obj->base.vma_node;
 	u64 vma_offset;
 
-	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
+	lockdep_assert_held(&vma->vm->mutex);
 
 	if (!i915_vma_has_userfault(vma))
 		return;
@@ -987,7 +987,9 @@ int i915_vma_unbind(struct i915_vma *vma)
 			return ret;
 
 		/* Force a pagefault for domain tracking on next user access */
+		mutex_lock(&vma->vm->mutex);
 		i915_vma_revoke_mmap(vma);
+		mutex_unlock(&vma->vm->mutex);
 
 		__i915_vma_iounmap(vma);
 		vma->flags &= ~I915_VMA_CAN_FENCE;
-- 
2.23.0.rc1

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

Re: [PATCH 4/5] drm/i915: Pull obj->userfault tracking under the ggtt->mutex

[Matthew Auld]

On Wed, 21 Aug 2019 at 16:59, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Since we want to revoke the ggtt vma from only under the ggtt->mutex, we
> need to move protection of the userfault tracking from the struct_mutex
> to the ggtt->mutex.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Replace i915_vma_put_fence()

[Chris Wilson]

Avoid calling i915_vma_put_fence() by using our alternate paths that
bind a secondary vma avoiding the original fenced vma. For the few
instances where we need to release the fence (i.e. on binding when the
GGTT range becomes invalid), replace the put_fence with a revoke_fence.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/display/intel_overlay.c  |  4 ---
 drivers/gpu/drm/i915/gem/i915_gem_domain.c    | 16 ++++++---
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  9 ++---
 drivers/gpu/drm/i915/i915_gem.c               | 36 ++++++++-----------
 drivers/gpu/drm/i915/i915_gem_fence_reg.c     | 16 +++------
 drivers/gpu/drm/i915/i915_vma.c               |  4 ++-
 drivers/gpu/drm/i915/i915_vma.h               |  4 +--
 7 files changed, 38 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
index eca41c4a5aa6..29edfc343716 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -770,10 +770,6 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	}
 	intel_frontbuffer_flush(new_bo->frontbuffer, ORIGIN_DIRTYFB);
 
-	ret = i915_vma_put_fence(vma);
-	if (ret)
-		goto out_unpin;
-
 	if (!overlay->active) {
 		u32 oconfig;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index a1afc2690e9e..60134c5aefa7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -221,6 +221,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	 * state and so involves less work.
 	 */
 	if (atomic_read(&obj->bind_count)) {
+		struct drm_i915_private *i915 = to_i915(obj->base.dev);
+
 		/* Before we change the PTE, the GPU must not be accessing it.
 		 * If we wait upon the object, we know that all the bound
 		 * VMA are no longer active.
@@ -232,8 +234,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		if (ret)
 			return ret;
 
-		if (!HAS_LLC(to_i915(obj->base.dev)) &&
-		    cache_level != I915_CACHE_NONE) {
+		if (!HAS_LLC(i915) && cache_level != I915_CACHE_NONE) {
 			/* Access to snoopable pages through the GTT is
 			 * incoherent and on some machines causes a hard
 			 * lockup. Relinquish the CPU mmaping to force
@@ -241,6 +242,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 			 * then double check if the GTT mapping is still
 			 * valid for that pointer access.
 			 */
+			ret = mutex_lock_interruptible(&i915->ggtt.vm.mutex);
+			if (ret)
+				return ret;
+
 			i915_gem_object_release_mmap(obj);
 
 			/* As we no longer need a fence for GTT access,
@@ -251,10 +256,13 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 			 * supposed to be linear.
 			 */
 			for_each_ggtt_vma(vma, obj) {
-				ret = i915_vma_put_fence(vma);
+				ret = i915_vma_revoke_fence(vma);
 				if (ret)
-					return ret;
+					break;
 			}
+			mutex_unlock(&i915->ggtt.vm.mutex);
+			if (ret)
+				return ret;
 		} else {
 			/* We either have incoherent backing store and
 			 * so no GTT access or the architecture is fully
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 2dca2962c73a..b5f6937369ea 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1024,6 +1024,9 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
 		struct i915_vma *vma;
 		int err;
 
+		if (i915_gem_object_is_tiled(obj))
+			return ERR_PTR(-EINVAL);
+
 		if (use_cpu_reloc(cache, obj))
 			return NULL;
 
@@ -1047,12 +1050,6 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
 			if (err) /* no inactive aperture space, use cpu reloc */
 				return NULL;
 		} else {
-			err = i915_vma_put_fence(vma);
-			if (err) {
-				i915_vma_unpin(vma);
-				return ERR_PTR(err);
-			}
-
 			cache->node.start = vma->node.start;
 			cache->node.mm = (void *)vma;
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 68976689d569..bef404fa7556 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -343,20 +343,16 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
 		return ret;
 
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
-	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
-				       PIN_MAPPABLE |
-				       PIN_NONBLOCK /* NOWARN */ |
-				       PIN_NOEVICT);
+	vma = ERR_PTR(-ENODEV);
+	if (!i915_gem_object_is_tiled(obj))
+		vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
+					       PIN_MAPPABLE |
+					       PIN_NONBLOCK /* NOWARN */ |
+					       PIN_NOEVICT);
 	if (!IS_ERR(vma)) {
 		node.start = i915_ggtt_offset(vma);
 		node.allocated = false;
-		ret = i915_vma_put_fence(vma);
-		if (ret) {
-			i915_vma_unpin(vma);
-			vma = ERR_PTR(ret);
-		}
-	}
-	if (IS_ERR(vma)) {
+	} else {
 		ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
 		if (ret)
 			goto out_unlock;
@@ -557,20 +553,16 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 		wakeref = intel_runtime_pm_get(rpm);
 	}
 
-	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
-				       PIN_MAPPABLE |
-				       PIN_NONBLOCK /* NOWARN */ |
-				       PIN_NOEVICT);
+	vma = ERR_PTR(-ENODEV);
+	if (i915_gem_object_is_tiled(obj))
+		vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
+					       PIN_MAPPABLE |
+					       PIN_NONBLOCK /* NOWARN */ |
+					       PIN_NOEVICT);
 	if (!IS_ERR(vma)) {
 		node.start = i915_ggtt_offset(vma);
 		node.allocated = false;
-		ret = i915_vma_put_fence(vma);
-		if (ret) {
-			i915_vma_unpin(vma);
-			vma = ERR_PTR(ret);
-		}
-	}
-	if (IS_ERR(vma)) {
+	} else {
 		ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
 		if (ret)
 			goto out_rpm;
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 6a33a0bb97a9..615a9f4ef30c 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -287,7 +287,7 @@ static int fence_update(struct i915_fence_reg *fence,
 }
 
 /**
- * i915_vma_put_fence - force-remove fence for a VMA
+ * i915_vma_revoke_fence - force-remove fence for a VMA
  * @vma: vma to map linearly (not through a fence reg)
  *
  * This function force-removes any fence from the given object, which is useful
@@ -297,26 +297,18 @@ static int fence_update(struct i915_fence_reg *fence,
  *
  * 0 on success, negative error code on failure.
  */
-int i915_vma_put_fence(struct i915_vma *vma)
+int i915_vma_revoke_fence(struct i915_vma *vma)
 {
-	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vma->vm);
 	struct i915_fence_reg *fence = vma->fence;
-	int err;
 
+	lockdep_assert_held(&vma->vm->mutex);
 	if (!fence)
 		return 0;
 
 	if (atomic_read(&fence->pin_count))
 		return -EBUSY;
 
-	err = mutex_lock_interruptible(&ggtt->vm.mutex);
-	if (err)
-		return err;
-
-	err = fence_update(fence, NULL);
-	mutex_unlock(&ggtt->vm.mutex);
-
-	return err;
+	return fence_update(fence, NULL);
 }
 
 static struct i915_fence_reg *fence_find(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 9840cb2f70b9..e0e677b2a3a9 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -982,7 +982,9 @@ int i915_vma_unbind(struct i915_vma *vma)
 		GEM_BUG_ON(i915_vma_has_ggtt_write(vma));
 
 		/* release the fence reg _after_ flushing */
-		ret = i915_vma_put_fence(vma);
+		mutex_lock(&vma->vm->mutex);
+		ret = i915_vma_revoke_fence(vma);
+		mutex_unlock(&vma->vm->mutex);
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index cf6c0437091d..889fc7cb910a 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -421,8 +421,8 @@ static inline struct page *i915_vma_first_page(struct i915_vma *vma)
  *
  * True if the vma has a fence, false otherwise.
  */
-int i915_vma_pin_fence(struct i915_vma *vma);
-int __must_check i915_vma_put_fence(struct i915_vma *vma);
+int __must_check i915_vma_pin_fence(struct i915_vma *vma);
+int __must_check i915_vma_revoke_fence(struct i915_vma *vma);
 
 static inline void __i915_vma_unpin_fence(struct i915_vma *vma)
 {
-- 
2.23.0.rc1

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

Re: [PATCH 5/5] drm/i915: Replace i915_vma_put_fence()

[Matthew Auld]

On Wed, 21 Aug 2019 at 16:59, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Avoid calling i915_vma_put_fence() by using our alternate paths that
> bind a secondary vma avoiding the original fenced vma. For the few
> instances where we need to release the fence (i.e. on binding when the
> GGTT range becomes invalid), replace the put_fence with a revoke_fence.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<snip>

> @@ -557,20 +553,16 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>                 wakeref = intel_runtime_pm_get(rpm);
>         }
>
> -       vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> -                                      PIN_MAPPABLE |
> -                                      PIN_NONBLOCK /* NOWARN */ |
> -                                      PIN_NOEVICT);
> +       vma = ERR_PTR(-ENODEV);
> +       if (i915_gem_object_is_tiled(obj))
> +               vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,

Hmm?

Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Replace i915_vma_put_fence()

[Chris Wilson]

Quoting Matthew Auld (2019-08-21 22:05:55)
> On Wed, 21 Aug 2019 at 16:59, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Avoid calling i915_vma_put_fence() by using our alternate paths that
> > bind a secondary vma avoiding the original fenced vma. For the few
> > instances where we need to release the fence (i.e. on binding when the
> > GGTT range becomes invalid), replace the put_fence with a revoke_fence.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <snip>
> 
> > @@ -557,20 +553,16 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
> >                 wakeref = intel_runtime_pm_get(rpm);
> >         }
> >
> > -       vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> > -                                      PIN_MAPPABLE |
> > -                                      PIN_NONBLOCK /* NOWARN */ |
> > -                                      PIN_NOEVICT);
> > +       vma = ERR_PTR(-ENODEV);
> > +       if (i915_gem_object_is_tiled(obj))
> > +               vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> 
> Hmm?

Well spotted. Just shows how often we hit this path!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/execlists: Set priority hint prior to submission

[Mika Kuoppala]

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

> Since we now run process_csb() outside of the engine->active.lock, we
> can process a CS-event immediately upon our ELSP write. As we currently
> inspect the pending queue *after* the ELSP write, there is an
> opportunity for a CS-event to update the pending queue before we can
> read it, making ourselves chases an invalid pointer.
>
> Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 44780e7fafec..d42584439f51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1335,9 +1335,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	if (submit) {
>  		*port = execlists_schedule_in(last, port - execlists->pending);
>  		memset(port + 1, 0, (last_port - port) * sizeof(*port));
> -		execlists_submit_ports(engine);
>  		execlists->switch_priority_hint =
>  			switch_prio(engine, *execlists->pending);
> +		execlists_submit_ports(engine);
>  	} else {
>  		ring_set_paused(engine, 0);
>  	}
> -- 
> 2.23.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915/execlists: Set priority hint prior to submission

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/execlists: Set priority hint prior to submission
URL   : https://patchwork.freedesktop.org/series/65553/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6757 -> Patchwork_14125
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14125 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14125, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/

Known issues
------------

  Here are the changes found in Patchwork_14125 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6757/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-bsw-n3050:       [PASS][3] -> [FAIL][4] ([fdo#103167])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6757/fi-bsw-n3050/igt@kms_frontbuffer_tracking@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/fi-bsw-n3050/igt@kms_frontbuffer_tracking@basic.html
    - fi-bsw-kefka:       [PASS][5] -> [FAIL][6] ([fdo#103167])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6757/fi-bsw-kefka/igt@kms_frontbuffer_tracking@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/fi-bsw-kefka/igt@kms_frontbuffer_tracking@basic.html
    - fi-byt-n2820:       [PASS][7] -> [FAIL][8] ([fdo#103167])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6757/fi-byt-n2820/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/fi-byt-n2820/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@gem_ctx_switch@legacy-render:
    - fi-apl-guc:         [INCOMPLETE][9] ([fdo#103927]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6757/fi-apl-guc/igt@gem_ctx_switch@legacy-render.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/fi-apl-guc/igt@gem_ctx_switch@legacy-render.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718


Participating hosts (56 -> 20)
------------------------------

  ERROR: It appears as if the changes made in Patchwork_14125 prevented too many machines from booting.

  Missing    (36): fi-kbl-soraka fi-skl-6770hq fi-bdw-gvtdvm fi-icl-u2 fi-snb-2520m fi-icl-u3 fi-pnv-d510 fi-icl-y fi-skl-lmem fi-icl-guc fi-skl-6600u fi-cml-u2 fi-icl-u4 fi-bxt-dsi fi-tgl-u fi-byt-j1900 fi-glk-dsi fi-bwr-2160 fi-ilk-650 fi-kbl-7500u fi-hsw-4770 fi-gdg-551 fi-kbl-r fi-ilk-m540 fi-skl-gvtdvm fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-cfl-guc fi-kbl-guc fi-whl-u fi-kbl-x1275 fi-cfl-8109u fi-kbl-8809g fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6757 -> Patchwork_14125

  CI-20190529: 20190529
  CI_DRM_6757: be31a083b02b1b37be1a8c67bab6d0b57422d3b7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5146: 357dbe1869d88a2f08bcee4eebceff4ee9014424 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14125: 394eca9840e4303b7b149082a5b746a8d60b0862 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

394eca9840e4 drm/i915: Replace i915_vma_put_fence()
332e6e957e98 drm/i915: Pull obj->userfault tracking under the ggtt->mutex
f9abd7a99ab6 drm/i915: Track ggtt fence reservations under its own mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14125/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx