[PATCH 1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

[PATCH 1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

[Matthew Auld]

Remove dev local and use to_i915() in gen8_ppgtt_notify_vgt.

v2: use dev_priv directly for QUESTION_MACROS (Joonas Lahtinen)

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c5cb049..5ed713d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -905,11 +905,10 @@ static int gen8_init_scratch(struct i915_address_space *vm)
 static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
 {
 	enum vgt_g2v_type msg;
-	struct drm_device *dev = ppgtt->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
 	int i;
 
-	if (USES_FULL_48BIT_PPGTT(dev)) {
+	if (USES_FULL_48BIT_PPGTT(dev_priv)) {
 		u64 daddr = px_dma(&ppgtt->pml4);
 
 		I915_WRITE(vgtif_reg(pdp[0].lo), lower_32_bits(daddr));
-- 
2.4.11

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

[PATCH 2/5] drm/i915: harden allocation paths in allocate_va_range

[Matthew Auld]

For the gen6/gen8_allocate_va_range functions remove the WARN_ON range
sanity checks in favour of simply hardening the allocation paths. This
also fixes the inconsistency in which we don't always handle the
potential overflow in our checks.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5ed713d..fa583d5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1220,15 +1220,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev);
 	int ret;
 
-	/* Wrap is never okay since we can only represent 48b, and we don't
-	 * actually use the other side of the canonical address space.
-	 */
-	if (WARN_ON(start + length < start))
-		return -ENODEV;
-
-	if (WARN_ON(start + length > vm->total))
-		return -ENODEV;
-
 	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
 	if (ret)
 		return ret;
@@ -1370,6 +1361,8 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 
+	length = min_t(uint64_t, length, vm->total);
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev))
 		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
 	else
@@ -1860,11 +1853,9 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 	uint32_t pde, temp;
 	int ret;
 
-	if (WARN_ON(start_in + length_in > ppgtt->base.total))
-		return -ENODEV;
-
 	start = start_save = start_in;
-	length = length_save = length_in;
+	length = length_save = length_in = min_t(uint64_t, length_in,
+						 ppgtt->base.total);
 
 	bitmap_zero(new_page_tables, I915_PDES);
 
-- 
2.4.11

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

[PATCH 3/5] drm/i915: bail in alloc_pdp when !FULL_48BIT_PPGTT

[Matthew Auld]

If we are not in FULL_48BIT_PPGTT mode then we really shouldn't
continue on with our allocations, given that the call to free_dpd would
bail early without freeing everything, thus leaking memory.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index fa583d5..6601b11 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -569,7 +569,8 @@ i915_page_directory_pointer *alloc_pdp(struct drm_device *dev)
 	struct i915_page_directory_pointer *pdp;
 	int ret = -ENOMEM;
 
-	WARN_ON(!USES_FULL_48BIT_PPGTT(dev));
+	if (WARN_ON(!USES_FULL_48BIT_PPGTT(dev)))
+		return ERR_PTR(-EINVAL);
 
 	pdp = kzalloc(sizeof(*pdp), GFP_KERNEL);
 	if (!pdp)
-- 
2.4.11

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

[PATCH 4/5] drm/i915: call kunmap_px on pt_vaddr

[Matthew Auld]

We need to kunmap pt_vaddr and not pt itself, otherwise we end up
mapping a bunch of pages without ever unmapping them.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6601b11..7aef7bf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -746,7 +746,7 @@ static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
 			num_entries--;
 		}
 
-		kunmap_px(ppgtt, pt);
+		kunmap_px(ppgtt, pt_vaddr);
 
 		pte = 0;
 		if (++pde == I915_PDES) {
-- 
2.4.11

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

[PATCH 5/5] drm/i915: Propagate error from drm_gem_object_init()

[Matthew Auld]

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

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

Propagate the real error from drm_gem_object_init(). Note this also
fixes some confusion in the error return from i915_gem_alloc_object...

v2:
(Matthew Auld)
  - updated new users of gem_alloc_object from latest drm-nightly
  - replaced occurrences of IS_ERR_OR_NULL() with IS_ERR()

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c              | 14 ++++++++------
 drivers/gpu/drm/i915/i915_gem_batch_pool.c   |  4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c      |  4 ++--
 drivers/gpu/drm/i915/i915_gem_render_state.c |  7 +++++--
 drivers/gpu/drm/i915/i915_guc_submission.c   |  2 +-
 drivers/gpu/drm/i915/intel_display.c         |  4 ++--
 drivers/gpu/drm/i915/intel_fbdev.c           |  4 ++--
 drivers/gpu/drm/i915/intel_lrc.c             | 10 ++++++----
 drivers/gpu/drm/i915/intel_overlay.c         |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c      | 19 ++++++++++---------
 10 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b37ffea..6a6998c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -387,8 +387,8 @@ i915_gem_create(struct drm_file *file,
 
 	/* Allocate the new object */
 	obj = i915_gem_alloc_object(dev, size);
-	if (obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	ret = drm_gem_handle_create(file, &obj->base, &handle);
 	/* drop reference from allocate - handle holds it now */
@@ -4527,14 +4527,16 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
+	int ret;
 
 	obj = i915_gem_object_alloc(dev);
 	if (obj == NULL)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
-	if (drm_gem_object_init(dev, &obj->base, size) != 0) {
+	ret = drm_gem_object_init(dev, &obj->base, size);
+	if (ret) {
 		i915_gem_object_free(obj);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
@@ -5390,7 +5392,7 @@ i915_gem_object_create_from_data(struct drm_device *dev,
 	int ret;
 
 	obj = i915_gem_alloc_object(dev, round_up(size, PAGE_SIZE));
-	if (IS_ERR_OR_NULL(obj))
+	if (IS_ERR(obj))
 		return obj;
 
 	ret = i915_gem_object_set_to_cpu_domain(obj, true);
diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
index 7bf2f3f..d79caa2 100644
--- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
+++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
@@ -135,8 +135,8 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
 		int ret;
 
 		obj = i915_gem_alloc_object(pool->dev, size);
-		if (obj == NULL)
-			return ERR_PTR(-ENOMEM);
+		if (IS_ERR(obj))
+			return obj;
 
 		ret = i915_gem_object_get_pages(obj);
 		if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index fe580cb..a5adc66 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -179,8 +179,8 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 	int ret;
 
 	obj = i915_gem_alloc_object(dev, size);
-	if (obj == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(obj))
+		return obj;
 
 	/*
 	 * Try to make the context utilize L3 as well as LLC.
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 71611bf..071c11e 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -58,8 +58,11 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
 		return -EINVAL;
 
 	so->obj = i915_gem_alloc_object(dev, 4096);
-	if (so->obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(so->obj)) {
+		ret = PTR_ERR(so->obj);
+		so->obj = NULL;
+		return ret;
+	}
 
 	ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da86bdb..004f8f7 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -617,7 +617,7 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
 	struct drm_i915_gem_object *obj;
 
 	obj = i915_gem_alloc_object(dev, size);
-	if (!obj)
+	if (IS_ERR(obj))
 		return NULL;
 
 	if (i915_gem_object_get_pages(obj)) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 551541b303..2247bdb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10283,8 +10283,8 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 
 	obj = i915_gem_alloc_object(dev,
 				    intel_framebuffer_size_for_mode(mode, bpp));
-	if (obj == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
 
 	mode_cmd.width = mode->hdisplay;
 	mode_cmd.height = mode->vdisplay;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202..7047b38 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -151,9 +151,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 		obj = i915_gem_object_create_stolen(dev, size);
 	if (obj == NULL)
 		obj = i915_gem_alloc_object(dev, size);
-	if (!obj) {
+	if (IS_ERR(obj)) {
 		DRM_ERROR("failed to allocate framebuffer\n");
-		ret = -ENOMEM;
+		ret = PTR_ERR(obj);
 		goto out;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0d6dc5e..c0543bb 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1483,9 +1483,11 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *engine, u32 size)
 
 	engine->wa_ctx.obj = i915_gem_alloc_object(engine->dev,
 						   PAGE_ALIGN(size));
-	if (!engine->wa_ctx.obj) {
+	if (IS_ERR(engine->wa_ctx.obj)) {
 		DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
-		return -ENOMEM;
+		ret = PTR_ERR(engine->wa_ctx.obj);
+		engine->wa_ctx.obj = NULL;
+		return ret;
 	}
 
 	ret = i915_gem_obj_ggtt_pin(engine->wa_ctx.obj, PAGE_SIZE, 0);
@@ -2638,9 +2640,9 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	context_size += PAGE_SIZE * LRC_PPHWSP_PN;
 
 	ctx_obj = i915_gem_alloc_object(dev, context_size);
-	if (!ctx_obj) {
+	if (IS_ERR(ctx_obj)) {
 		DRM_DEBUG_DRIVER("Alloc LRC backing obj failed.\n");
-		return -ENOMEM;
+		return PTR_ERR(ctx_obj);
 	}
 
 	ringbuf = intel_engine_create_ringbuffer(engine, 4 * PAGE_SIZE);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6694e92..77ee12f 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1397,7 +1397,7 @@ void intel_setup_overlay(struct drm_device *dev)
 		reg_bo = i915_gem_object_create_stolen(dev, PAGE_SIZE);
 	if (reg_bo == NULL)
 		reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);
-	if (reg_bo == NULL)
+	if (IS_ERR(reg_bo))
 		goto out_free;
 	overlay->reg_bo = reg_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 41b604e..b20cf91 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -672,9 +672,10 @@ intel_init_pipe_control(struct intel_engine_cs *engine)
 	WARN_ON(engine->scratch.obj);
 
 	engine->scratch.obj = i915_gem_alloc_object(engine->dev, 4096);
-	if (engine->scratch.obj == NULL) {
+	if (IS_ERR(engine->scratch.obj)) {
 		DRM_ERROR("Failed to allocate seqno page\n");
-		ret = -ENOMEM;
+		ret = PTR_ERR(engine->scratch.obj);
+		engine->scratch.obj = NULL;
 		goto err;
 	}
 
@@ -2021,9 +2022,9 @@ static int init_status_page(struct intel_engine_cs *engine)
 		int ret;
 
 		obj = i915_gem_alloc_object(engine->dev, 4096);
-		if (obj == NULL) {
+		if (IS_ERR(obj)) {
 			DRM_ERROR("Failed to allocate status page\n");
-			return -ENOMEM;
+			return PTR_ERR(obj);
 		}
 
 		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
@@ -2156,8 +2157,8 @@ static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 		obj = i915_gem_object_create_stolen(dev, ringbuf->size);
 	if (obj == NULL)
 		obj = i915_gem_alloc_object(dev, ringbuf->size);
-	if (obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	/* mark ring buffers as read-only from GPU side by default */
 	obj->gt_ro = 1;
@@ -2780,7 +2781,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 8) {
 		if (i915_semaphore_is_enabled(dev)) {
 			obj = i915_gem_alloc_object(dev, 4096);
-			if (obj == NULL) {
+			if (IS_ERR(obj)) {
 				DRM_ERROR("Failed to allocate semaphore bo. Disabling semaphores\n");
 				i915.semaphores = 0;
 			} else {
@@ -2889,9 +2890,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	/* Workaround batchbuffer to combat CS tlb bug. */
 	if (HAS_BROKEN_CS_TLB(dev)) {
 		obj = i915_gem_alloc_object(dev, I830_WA_SIZE);
-		if (obj == NULL) {
+		if (IS_ERR(obj)) {
 			DRM_ERROR("Failed to allocate batch bo\n");
-			return -ENOMEM;
+			return PTR_ERR(obj);
 		}
 
 		ret = i915_gem_obj_ggtt_pin(obj, 0, 0);
-- 
2.4.11

_______________________________________________
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: use dev_priv directly in gen8_ppgtt_notify_vgt

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt
URL   : https://patchwork.freedesktop.org/series/5601/
State : failure

== Summary ==

Series 5601v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5601/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (hsw-gt2)
Test gem_sync:
        Subgroup basic-vebox:
                pass       -> INCOMPLETE (hsw-gt2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (bsw-nuc-2)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:162  dwarn:0   dfail:0   fail:1   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:142  pass:128  dwarn:0   dfail:0   fail:0   skip:13 
ilk-hp8440p      total:203  pass:135  dwarn:0   dfail:0   fail:0   skip:68 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:165  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1878/

631ffd2f45bb43964f729e8661532fb115f5eeec drm-intel-nightly: 2016y-04m-13d-13h-00m-18s UTC integration manifest
9a1e64a drm/i915: Propagate error from drm_gem_object_init()
b2d1dcb drm/i915: call kunmap_px on pt_vaddr
4c75324 drm/i915: bail in alloc_pdp when !FULL_48BIT_PPGTT
b6ba9cf drm/i915: harden allocation paths in allocate_va_range
472e351 drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

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

Re: [PATCH 1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

[Joonas Lahtinen]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> Remove dev local and use to_i915() in gen8_ppgtt_notify_vgt.
> 
> v2: use dev_priv directly for QUESTION_MACROS (Joonas Lahtinen)
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c5cb049..5ed713d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -905,11 +905,10 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>  static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
>  {
>  	enum vgt_g2v_type msg;
> -	struct drm_device *dev = ppgtt->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
>  	int i;
>  
> -	if (USES_FULL_48BIT_PPGTT(dev)) {
> +	if (USES_FULL_48BIT_PPGTT(dev_priv)) {
>  		u64 daddr = px_dma(&ppgtt->pml4);
>  
>  		I915_WRITE(vgtif_reg(pdp[0].lo), lower_32_bits(daddr));
-- 
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

Re: [PATCH 1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

[Chris Wilson]

On Mon, Apr 18, 2016 at 03:51:20PM +0300, Joonas Lahtinen wrote:
> On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> > Remove dev local and use to_i915() in gen8_ppgtt_notify_vgt.
> > 
> > v2: use dev_priv directly for QUESTION_MACROS (Joonas Lahtinen)
> > 
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index c5cb049..5ed713d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -905,11 +905,10 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> >  static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
> >  {
> >  	enum vgt_g2v_type msg;
> > -	struct drm_device *dev = ppgtt->base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
> >  	int i;
> >  
> > -	if (USES_FULL_48BIT_PPGTT(dev)) {
> > +	if (USES_FULL_48BIT_PPGTT(dev_priv)) {

The irony of this macro, is that it doesn't use its argument, and should
it ever do so, it will use information stored in the ppgtt->vm.
-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

Re: [PATCH 2/5] drm/i915: harden allocation paths in allocate_va_range

[Joonas Lahtinen]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> For the gen6/gen8_allocate_va_range functions remove the WARN_ON range
> sanity checks in favour of simply hardening the allocation paths. This
> also fixes the inconsistency in which we don't always handle the
> potential overflow in our checks.
> 

Not sure this is the right thing to do. I think I do prefer erroring
out on invalid arguments as we're talking of internal programmer
interface.

Regards, Joonas

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 5ed713d..fa583d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1220,15 +1220,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
>  	uint32_t pdpes = I915_PDPES_PER_PDP(dev);
>  	int ret;
>  
> -	/* Wrap is never okay since we can only represent 48b, and we don't
> -	 * actually use the other side of the canonical address space.
> -	 */
> -	if (WARN_ON(start + length < start))
> -		return -ENODEV;
> -
> -	if (WARN_ON(start + length > vm->total))
> -		return -ENODEV;
> -
>  	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
>  	if (ret)
>  		return ret;
> @@ -1370,6 +1361,8 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>  {
>  	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
>  
> +	length = min_t(uint64_t, length, vm->total);
> +
>  	if (USES_FULL_48BIT_PPGTT(vm->dev))
>  		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
>  	else
> @@ -1860,11 +1853,9 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
>  	uint32_t pde, temp;
>  	int ret;
>  
> -	if (WARN_ON(start_in + length_in > ppgtt->base.total))
> -		return -ENODEV;
> -
>  	start = start_save = start_in;
> -	length = length_save = length_in;
> +	length = length_save = length_in = min_t(uint64_t, length_in,
> +						 ppgtt->base.total);
>  
>  	bitmap_zero(new_page_tables, I915_PDES);
>  
-- 
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

Re: [PATCH 3/5] drm/i915: bail in alloc_pdp when !FULL_48BIT_PPGTT

[Joonas Lahtinen]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> If we are not in FULL_48BIT_PPGTT mode then we really shouldn't
> continue on with our allocations, given that the call to free_dpd would
> bail early without freeing everything, thus leaking memory.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index fa583d5..6601b11 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -569,7 +569,8 @@ i915_page_directory_pointer *alloc_pdp(struct drm_device *dev)
>  	struct i915_page_directory_pointer *pdp;
>  	int ret = -ENOMEM;

This default value is unused.

While touching the function, I'd rather make it ret = -EINVAL and add
goto fail; to clean it up further.

> -	WARN_ON(!USES_FULL_48BIT_PPGTT(dev));
> +	if (WARN_ON(!USES_FULL_48BIT_PPGTT(dev)))
> +		return ERR_PTR(-EINVAL);

goto fail here.

>  
>  	pdp = kzalloc(sizeof(*pdp), GFP_KERNEL);
>  	if (!pdp)

ret = -ENOMEM and goto fail here?

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

Re: [PATCH 4/5] drm/i915: call kunmap_px on pt_vaddr

[Joonas Lahtinen]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> We need to kunmap pt_vaddr and not pt itself, otherwise we end up
> mapping a bunch of pages without ever unmapping them.
> 

It's always a good idea to use 'git blame' to figure out the bug
inducing commit, and include that information + CC'ing the author (Mika
here):

Fixes: d1c54acd67dc ("drm/i915/gtt: Introduce kmap|kunmap for dma page")

With that added,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Mika to comment on too, looks like  fairly straightforward error when
doing a mass replace.

Regards, Joonas

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6601b11..7aef7bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -746,7 +746,7 @@ static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
>  			num_entries--;
>  		}
>  
> -		kunmap_px(ppgtt, pt);
> +		kunmap_px(ppgtt, pt_vaddr);
>  
>  		pte = 0;
>  		if (++pde == I915_PDES) {
-- 
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

Re: [PATCH 1/5] drm/i915: use dev_priv directly in gen8_ppgtt_notify_vgt

[Joonas Lahtinen]

On ma, 2016-04-18 at 14:11 +0100, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 03:51:20PM +0300, Joonas Lahtinen wrote:
> > 
> > On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> > > 
> > > Remove dev local and use to_i915() in gen8_ppgtt_notify_vgt.
> > > 
> > > v2: use dev_priv directly for QUESTION_MACROS (Joonas Lahtinen)
> > > 
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > > 
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index c5cb049..5ed713d 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -905,11 +905,10 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> > >  static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
> > >  {
> > >  	enum vgt_g2v_type msg;
> > > -	struct drm_device *dev = ppgtt->base.dev;
> > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
> > >  	int i;
> > >  
> > > -	if (USES_FULL_48BIT_PPGTT(dev)) {
> > > +	if (USES_FULL_48BIT_PPGTT(dev_priv)) {
> The irony of this macro, is that it doesn't use its argument, and should
> it ever do so, it will use information stored in the ppgtt->vm.

Oh dear, only now looked at it. Pretty horrible macro :P

Regards, Joonas

> -Chris
> 
-- 
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

Re: [PATCH 5/5] drm/i915: Propagate error from drm_gem_object_init()

[Joonas Lahtinen]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> From: Chris Wilson <chris@chris-wilson.co.uk>

Double "From: ", one should be enough.

> 
> Propagate the real error from drm_gem_object_init(). Note this also
> fixes some confusion in the error return from i915_gem_alloc_object...
> 
> v2:
> (Matthew Auld)
>   - updated new users of gem_alloc_object from latest drm-nightly
>   - replaced occurrences of IS_ERR_OR_NULL() with IS_ERR()
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c              | 14 ++++++++------
>  drivers/gpu/drm/i915/i915_gem_batch_pool.c   |  4 ++--
>  drivers/gpu/drm/i915/i915_gem_context.c      |  4 ++--
>  drivers/gpu/drm/i915/i915_gem_render_state.c |  7 +++++--
>  drivers/gpu/drm/i915/i915_guc_submission.c   |  2 +-
>  drivers/gpu/drm/i915/intel_display.c         |  4 ++--
>  drivers/gpu/drm/i915/intel_fbdev.c           |  4 ++--
>  drivers/gpu/drm/i915/intel_lrc.c             | 10 ++++++----
>  drivers/gpu/drm/i915/intel_overlay.c         |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c      | 19 ++++++++++---------
>  10 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b37ffea..6a6998c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -387,8 +387,8 @@ i915_gem_create(struct drm_file *file,
>  
>  	/* Allocate the new object */
>  	obj = i915_gem_alloc_object(dev, size);
> -	if (obj == NULL)
> -		return -ENOMEM;
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
>  
>  	ret = drm_gem_handle_create(file, &obj->base, &handle);
>  	/* drop reference from allocate - handle holds it now */
> @@ -4527,14 +4527,16 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  	struct drm_i915_gem_object *obj;
>  	struct address_space *mapping;
>  	gfp_t mask;
> +	int ret;
>  
>  	obj = i915_gem_object_alloc(dev);
>  	if (obj == NULL)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
> -	if (drm_gem_object_init(dev, &obj->base, size) != 0) {
> +	ret = drm_gem_object_init(dev, &obj->base, size);
> +	if (ret) {
>  		i915_gem_object_free(obj);
> -		return NULL;
> +		return ERR_PTR(ret);

While at it, I'd make this have a goto teardown path.

>  	}
>  
>  	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> @@ -5390,7 +5392,7 @@ i915_gem_object_create_from_data(struct drm_device *dev,
>  	int ret;
>  
>  	obj = i915_gem_alloc_object(dev, round_up(size, PAGE_SIZE));
> -	if (IS_ERR_OR_NULL(obj))
> +	if (IS_ERR(obj))
>  		return obj;
>  
>  	ret = i915_gem_object_set_to_cpu_domain(obj, true);
> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> index 7bf2f3f..d79caa2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> @@ -135,8 +135,8 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
>  		int ret;
>  
>  		obj = i915_gem_alloc_object(pool->dev, size);
> -		if (obj == NULL)
> -			return ERR_PTR(-ENOMEM);
> +		if (IS_ERR(obj))
> +			return obj;
>  
>  		ret = i915_gem_object_get_pages(obj);
>  		if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index fe580cb..a5adc66 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -179,8 +179,8 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
>  	int ret;
>  
>  	obj = i915_gem_alloc_object(dev, size);
> -	if (obj == NULL)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(obj))
> +		return obj;
>  
>  	/*
>  	 * Try to make the context utilize L3 as well as LLC.
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 71611bf..071c11e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -58,8 +58,11 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
>  		return -EINVAL;
>  
>  	so->obj = i915_gem_alloc_object(dev, 4096);
> -	if (so->obj == NULL)
> -		return -ENOMEM;
> +	if (IS_ERR(so->obj)) {
> +		ret = PTR_ERR(so->obj);
> +		so->obj = NULL;
> +		return ret;
> +	}

The teardown path in this function leaves so->obj != NULL in later
failure path. Also i915_gem_alloc_object has only
drm_gem_object_unreference as a counterpart so it'll leak too. Should
be fixed in a follow-up patch.

>  
>  	ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index da86bdb..004f8f7 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -617,7 +617,7 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
>  	struct drm_i915_gem_object *obj;
>  
>  	obj = i915_gem_alloc_object(dev, size);
> -	if (!obj)
> +	if (IS_ERR(obj))
>  		return NULL;
>  
>  	if (i915_gem_object_get_pages(obj)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 551541b303..2247bdb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10283,8 +10283,8 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  
>  	obj = i915_gem_alloc_object(dev,
>  				    intel_framebuffer_size_for_mode(mode, bpp));
> -	if (obj == NULL)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(obj))
> +		return ERR_CAST(obj);
>  
>  	mode_cmd.width = mode->hdisplay;
>  	mode_cmd.height = mode->vdisplay;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202..7047b38 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -151,9 +151,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  		obj = i915_gem_object_create_stolen(dev, size);
>  	if (obj == NULL)
>  		obj = i915_gem_alloc_object(dev, size);
> -	if (!obj) {
> +	if (IS_ERR(obj)) {
>  		DRM_ERROR("failed to allocate framebuffer\n");
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(obj);
>  		goto out;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0d6dc5e..c0543bb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1483,9 +1483,11 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *engine, u32 size)
>  
>  	engine->wa_ctx.obj = i915_gem_alloc_object(engine->dev,
>  						   PAGE_ALIGN(size));
> -	if (!engine->wa_ctx.obj) {
> +	if (IS_ERR(engine->wa_ctx.obj)) {
>  		DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
> -		return -ENOMEM;
> +		ret = PTR_ERR(engine->wa_ctx.obj);
> +		engine->wa_ctx.obj = NULL;
> +		return ret;
>  	}
>  
>  	ret = i915_gem_obj_ggtt_pin(engine->wa_ctx.obj, PAGE_SIZE, 0);
> @@ -2638,9 +2640,9 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
>  	context_size += PAGE_SIZE * LRC_PPHWSP_PN;
>  
>  	ctx_obj = i915_gem_alloc_object(dev, context_size);
> -	if (!ctx_obj) {
> +	if (IS_ERR(ctx_obj)) {
>  		DRM_DEBUG_DRIVER("Alloc LRC backing obj failed.\n");
> -		return -ENOMEM;
> +		return PTR_ERR(ctx_obj);
>  	}
>  
>  	ringbuf = intel_engine_create_ringbuffer(engine, 4 * PAGE_SIZE);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 6694e92..77ee12f 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1397,7 +1397,7 @@ void intel_setup_overlay(struct drm_device *dev)
>  		reg_bo = i915_gem_object_create_stolen(dev, PAGE_SIZE);
>  	if (reg_bo == NULL)
>  		reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);
> -	if (reg_bo == NULL)
> +	if (IS_ERR(reg_bo))
>  		goto out_free;
>  	overlay->reg_bo = reg_bo;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 41b604e..b20cf91 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -672,9 +672,10 @@ intel_init_pipe_control(struct intel_engine_cs *engine)
>  	WARN_ON(engine->scratch.obj);
>  
>  	engine->scratch.obj = i915_gem_alloc_object(engine->dev, 4096);
> -	if (engine->scratch.obj == NULL) {
> +	if (IS_ERR(engine->scratch.obj)) {
>  		DRM_ERROR("Failed to allocate seqno page\n");
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(engine->scratch.obj);
> +		engine->scratch.obj = NULL;
>  		goto err;
>  	}
>  
> @@ -2021,9 +2022,9 @@ static int init_status_page(struct intel_engine_cs *engine)
>  		int ret;
>  
>  		obj = i915_gem_alloc_object(engine->dev, 4096);
> -		if (obj == NULL) {
> +		if (IS_ERR(obj)) {
>  			DRM_ERROR("Failed to allocate status page\n");
> -			return -ENOMEM;
> +			return PTR_ERR(obj);
>  		}
>  
>  		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> @@ -2156,8 +2157,8 @@ static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>  		obj = i915_gem_object_create_stolen(dev, ringbuf->size);
>  	if (obj == NULL)
>  		obj = i915_gem_alloc_object(dev, ringbuf->size);
> -	if (obj == NULL)
> -		return -ENOMEM;
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
>  
>  	/* mark ring buffers as read-only from GPU side by default */
>  	obj->gt_ro = 1;
> @@ -2780,7 +2781,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->gen >= 8) {
>  		if (i915_semaphore_is_enabled(dev)) {
>  			obj = i915_gem_alloc_object(dev, 4096);
> -			if (obj == NULL) {
> +			if (IS_ERR(obj)) {
>  				DRM_ERROR("Failed to allocate semaphore bo. Disabling semaphores\n");
>  				i915.semaphores = 0;
>  			} else {
> @@ -2889,9 +2890,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  	/* Workaround batchbuffer to combat CS tlb bug. */
>  	if (HAS_BROKEN_CS_TLB(dev)) {
>  		obj = i915_gem_alloc_object(dev, I830_WA_SIZE);
> -		if (obj == NULL) {
> +		if (IS_ERR(obj)) {
>  			DRM_ERROR("Failed to allocate batch bo\n");
> -			return -ENOMEM;
> +			return PTR_ERR(obj);
>  		}
>  
>  		ret = i915_gem_obj_ggtt_pin(obj, 0, 0);

Some comments above.

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

Re: [PATCH 4/5] drm/i915: call kunmap_px on pt_vaddr

[Mika Kuoppala]

Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:

> [ text/plain ]
> On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
>> We need to kunmap pt_vaddr and not pt itself, otherwise we end up
>> mapping a bunch of pages without ever unmapping them.
>> 
>
> It's always a good idea to use 'git blame' to figure out the bug
> inducing commit, and include that information + CC'ing the author (Mika
> here):
>
> Fixes: d1c54acd67dc ("drm/i915/gtt: Introduce kmap|kunmap for dma page")
>
> With that added,
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Mika to comment on too, looks like  fairly straightforward error when
> doing a mass replace.
>

Fixes tag added and pushed to dinq. Thanks for patch and review.
-Mika

> Regards, Joonas
>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 6601b11..7aef7bf 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -746,7 +746,7 @@ static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
>>  			num_entries--;
>>  		}
>>  
>> -		kunmap_px(ppgtt, pt);
>> +		kunmap_px(ppgtt, pt_vaddr);
>>  
>>  		pte = 0;
>>  		if (++pde == I915_PDES) {
> -- 
> 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