[PATCH 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[PATCH 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Michał Winiarski]

It will be initialized just moments later by i915_init_vm. Global and
aliasing tables are going through different path anyways.

Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8786281..7ff7239 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2129,8 +2129,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 	ret = __hw_ppgtt_init(dev, ppgtt);
 	if (ret == 0) {
 		kref_init(&ppgtt->ref);
-		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
-			    ppgtt->base.total);
 		i915_init_vm(dev_priv, &ppgtt->base);
 	}
 
-- 
2.4.3

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

[PATCH 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Michał Winiarski]

According to bspec, some parts of HW expect the addresses to be in
a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
converting the address prior to allocating/clearing the entries in page
tables.

Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 13 +++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7ff7239..f25a33f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -766,6 +766,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
 	} else {
 		uint64_t templ4, pml4e;
 		struct i915_page_directory_pointer *pdp;
+		start = gen8_canonical_addr(start);
 
 		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
 			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
@@ -1227,15 +1228,10 @@ 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.
-	 */
+	/* Wrap is never okay */
 	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;
@@ -1333,6 +1329,8 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 	uint64_t temp, pml4e;
 	int ret = 0;
 
+	start = gen8_canonical_addr(start);
+
 	/* Do the pml4 allocations first, so we don't need to track the newly
 	 * allocated tables below the pdp */
 	bitmap_zero(new_pdps, GEN8_PML4ES_PER_PML4);
@@ -1377,6 +1375,9 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 
+	if (WARN_ON(start + length > vm->total))
+		return -ENODEV;
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev))
 		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
 	else
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8275007..9397387 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -503,6 +503,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
 }
 
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return ((int64_t)address << 16) >> 16;
+}
+
 static inline dma_addr_t
 i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 {
-- 
2.4.3

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

Re: [PATCH 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Chris Wilson]

On Tue, Sep 15, 2015 at 03:30:15PM +0200, Michał Winiarski wrote:
> It will be initialized just moments later by i915_init_vm. Global and
> aliasing tables are going through different path anyways.
> 
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8786281..7ff7239 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2129,8 +2129,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
> -		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> -			    ppgtt->base.total);
>  		i915_init_vm(dev_priv, &ppgtt->base);

And please move i915_init_vm() to i915_gem_gtt.c and rename it
correctly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Chris Wilson]

On Tue, Sep 15, 2015 at 03:30:16PM +0200, Michał Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
> converting the address prior to allocating/clearing the entries in page
> tables.

You should be a little more specific as to why this is only required for
the ppgtt paths.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Michel Thierry]

On 9/15/2015 2:30 PM, Michał Winiarski wrote:
> It will be initialized just moments later by i915_init_vm. Global and
> aliasing tables are going through different path anyways.
>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8786281..7ff7239 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2129,8 +2129,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>   	ret = __hw_ppgtt_init(dev, ppgtt);
>   	if (ret == 0) {
>   		kref_init(&ppgtt->ref);
> -		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> -			    ppgtt->base.total);
>   		i915_init_vm(dev_priv, &ppgtt->base);
>   	}
>
>

Indeed,

Reviewed-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Michel Thierry]

On 9/15/2015 2:30 PM, Michał Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
> converting the address prior to allocating/clearing the entries in page
> tables.

Thanks for sending this. A couple of comments below.
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7ff7239..f25a33f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -766,6 +766,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>   	} else {
>   		uint64_t templ4, pml4e;
>   		struct i915_page_directory_pointer *pdp;
> +		start = gen8_canonical_addr(start);
>
>   		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
>   			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
> @@ -1227,15 +1228,10 @@ 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.
> -	 */
> +	/* Wrap is never okay */
>   	if (WARN_ON(start + length < start))
>   		return -ENODEV;

start can be in canonical form here (alloc_va_range_4lvl calls 
alloc_va_range_3lvl), and then this check could fail, e.g: first alloc 
when using top-down.
Just move it to gen8_alloc_va_range like you did to the check below.

>
> -	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;
> @@ -1333,6 +1329,8 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
>   	uint64_t temp, pml4e;
>   	int ret = 0;
>
> +	start = gen8_canonical_addr(start);
> +

Can you move this to gen8_alloc_va_range (48bit path) instead?
That will keep it in sync to _clear_range.

Also, I know it isn't really required in gen8_ppgtt_insert_entries (as 
it only relies in pdp/pdp/pt indexes), but it will be nice to have 
alloc/insert/clear functions in sync.

>   	/* Do the pml4 allocations first, so we don't need to track the newly
>   	 * allocated tables below the pdp */
>   	bitmap_zero(new_pdps, GEN8_PML4ES_PER_PML4);
> @@ -1377,6 +1375,9 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
>
> +	if (WARN_ON(start + length > vm->total))
> +		return -ENODEV;
> +
>   	if (USES_FULL_48BIT_PPGTT(vm->dev))
>   		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
>   	else
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 8275007..9397387 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -503,6 +503,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
>   	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
>   }
>
> +static inline uint64_t gen8_canonical_addr(uint64_t address)

Feel free to ignore this one, but this isn't gen8 per se, so I vote for 
to_canonical_addr().

> +{
> +	return ((int64_t)address << 16) >> 16;
> +}
> +
>   static inline dma_addr_t
>   i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
>   {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Michał Winiarski]

It would be initialized just moments later by i915_init_vm.

v2: Commit msg update,
    s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
    init address_space during i915_gem_setup_global_gtt for ggtt.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

to be squashed
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 --
 drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
 drivers/gpu/drm/i915/i915_gem_gtt.c | 30 +++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  3 +++
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b..039227d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2804,8 +2804,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size);
 struct drm_i915_gem_object *i915_gem_object_create_from_data(
 		struct drm_device *dev, const void *data, size_t size);
-void i915_init_vm(struct drm_i915_private *dev_priv,
-		  struct i915_address_space *vm);
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb0df7e..4811f8a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs *ring)
 	INIT_LIST_HEAD(&ring->request_list);
 }
 
-void i915_init_vm(struct drm_i915_private *dev_priv,
-		  struct i915_address_space *vm)
-{
-	if (!i915_is_ggtt(vm))
-		drm_mm_init(&vm->mm, vm->start, vm->total);
-	vm->dev = dev_priv->dev;
-	INIT_LIST_HEAD(&vm->active_list);
-	INIT_LIST_HEAD(&vm->inactive_list);
-	INIT_LIST_HEAD(&vm->global_link);
-	list_add_tail(&vm->global_link, &dev_priv->vm_list);
-}
-
 void
 i915_gem_load(struct drm_device *dev)
 {
@@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
 				  NULL);
 
 	INIT_LIST_HEAD(&dev_priv->vm_list);
-	i915_init_vm(dev_priv, &dev_priv->gtt.base);
-
 	INIT_LIST_HEAD(&dev_priv->context_list);
 	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
 	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8786281..97c27da5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2123,15 +2123,12 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 
 int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret = 0;
 
 	ret = __hw_ppgtt_init(dev, ppgtt);
 	if (ret == 0) {
 		kref_init(&ppgtt->ref);
-		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
-			    ppgtt->base.total);
-		i915_init_vm(dev_priv, &ppgtt->base);
+		i915_address_space_init(dev, &ppgtt->base);
 	}
 
 	return ret;
@@ -2618,11 +2615,13 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 
 	BUG_ON(mappable_end > end);
 
-	/* Subtract the guard page ... */
-	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+	ggtt_vm->start = start;
 
-	dev_priv->gtt.base.start = start;
-	dev_priv->gtt.base.total = end - start;
+	/* Subtract the guard page before address space initialization to
+	 * shrink the range used by drm_mm */
+	ggtt_vm->total = end - start - PAGE_SIZE;
+	i915_address_space_init(dev, ggtt_vm);
+	ggtt_vm->total += PAGE_SIZE;
 
 	if (intel_vgpu_active(dev)) {
 		ret = intel_vgt_balloon(dev);
@@ -2631,7 +2630,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 	}
 
 	if (!HAS_LLC(dev))
-		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
+		ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
 
 	/* Mark any preallocated objects as occupied */
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
@@ -3051,6 +3050,19 @@ static void i915_gmch_remove(struct i915_address_space *vm)
 	intel_gmch_remove();
 }
 
+void i915_address_space_init(struct drm_device *dev,
+		  struct i915_address_space *vm)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	drm_mm_init(&vm->mm, vm->start, vm->total);
+	vm->dev = dev;
+	INIT_LIST_HEAD(&vm->active_list);
+	INIT_LIST_HEAD(&vm->inactive_list);
+	INIT_LIST_HEAD(&vm->global_link);
+	list_add_tail(&vm->global_link, &dev_priv->vm_list);
+}
+
 int i915_gem_gtt_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8275007..2114d4e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -511,6 +511,9 @@ i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 		px_dma(ppgtt->base.scratch_pd);
 }
 
+void i915_address_space_init(struct drm_device *dev,
+			     struct i915_address_space *vm);
+
 int i915_gem_gtt_init(struct drm_device *dev);
 void i915_gem_init_global_gtt(struct drm_device *dev);
 void i915_global_gtt_cleanup(struct drm_device *dev);
-- 
2.4.3

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

[PATCH v2 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Michał Winiarski]

According to bspec, some parts of HW expect the addresses to be in
a canonical form where bits [63:48] == [47].
If we're using 32b addressing, we never need to handle such high
addresses, but since we've recently added 48b address space support,
lets satisfy the HW by converting the address prior to
alloc/insert/clear.

v2: Commit msg update,
    move WARN to gen8_alloc_va_range,
    also convert to canonical in insert_entries.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 21 ++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 97c27da5..be66f75 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -766,6 +766,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
 	} else {
 		uint64_t templ4, pml4e;
 		struct i915_page_directory_pointer *pdp;
+		start = gen8_canonical_addr(start);
 
 		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
 			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
@@ -835,6 +836,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 		struct i915_page_directory_pointer *pdp;
 		uint64_t templ4, pml4e;
 		uint64_t length = (uint64_t)pages->orig_nents << PAGE_SHIFT;
+		start = gen8_canonical_addr(start);
 
 		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
 			gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter,
@@ -1227,14 +1229,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)
@@ -1377,8 +1371,17 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 
+	/* Wrap is never okay */
+	if (WARN_ON(start + length < start))
+		return -ENODEV;
+
+	if (WARN_ON(start + length > vm->total))
+		return -ENODEV;
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev))
-		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
+		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4,
+						gen8_canonical_addr(start),
+						length);
 	else
 		return gen8_alloc_va_range_3lvl(vm, &ppgtt->pdp, start, length);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 2114d4e..0172590 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -503,6 +503,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
 }
 
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return ((int64_t)address << 16) >> 16;
+}
+
 static inline dma_addr_t
 i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 {
-- 
2.4.3

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

Re: [PATCH v2 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Chris Wilson]

On Tue, Sep 15, 2015 at 08:04:27PM +0200, Michał Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47].
> If we're using 32b addressing, we never need to handle such high
> addresses, but since we've recently added 48b address space support,
> lets satisfy the HW by converting the address prior to
> alloc/insert/clear.

Ah, that makes sense - it is only the PDE (et al) that you are setting,
right?
 
>  	if (USES_FULL_48BIT_PPGTT(vm->dev))
> -		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
> +		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4,
> +						gen8_canonical_addr(start),
> +						length);
>  	else
>  		return gen8_alloc_va_range_3lvl(vm, &ppgtt->pdp, start, length);

Whilst we cannot have the high bit set if !48b, if we just
	start = gen8_canonical_addr(start);
and used the same start for both paths here, that feels more organized -
or have I misunderstand where the high bits needs to be set? In which
case please add a comment.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Chris Wilson]

On Tue, Sep 15, 2015 at 08:01:14PM +0200, Michał Winiarski wrote:
> +void i915_address_space_init(struct drm_device *dev,
> +		  struct i915_address_space *vm)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	drm_mm_init(&vm->mm, vm->start, vm->total);
> +	vm->dev = dev;
> +	INIT_LIST_HEAD(&vm->active_list);
> +	INIT_LIST_HEAD(&vm->inactive_list);
> +	INIT_LIST_HEAD(&vm->global_link);
> +	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> +}

Having done all that work, it can now be static. It is an internal
function so pass around drm_i915_private, and if we follow our oop then
the object (struct i915_address_space) should be the first parameter.
Also on my hit list is s/vm->dev/vm->i915/ so we can drop lots of
pointless pointer dancing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Chris Wilson]

On Tue, Sep 15, 2015 at 08:01:14PM +0200, Michał Winiarski wrote:
> +void i915_address_space_init(struct drm_device *dev,
> +		  struct i915_address_space *vm)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	drm_mm_init(&vm->mm, vm->start, vm->total);
> +	vm->dev = dev;
> +	INIT_LIST_HEAD(&vm->active_list);
> +	INIT_LIST_HEAD(&vm->inactive_list);
> +	INIT_LIST_HEAD(&vm->global_link);
> +	list_add_tail(&vm->global_link, &dev_priv->vm_list);

Oh, that INIT_LIST_HEAD(global_link) is redundant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Michał Winiarski]

It would be initialized just moments later by i915_init_vm.

v2: Commit msg update,
    s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
    init address_space during i915_gem_setup_global_gtt for ggtt.
v3: Do not init global_link - we are adding it to vm_list moments later,
    make i915_address_space_init static, use OOP style parameter order.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 --
 drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
 drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++--------
 3 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b..039227d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2804,8 +2804,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size);
 struct drm_i915_gem_object *i915_gem_object_create_from_data(
 		struct drm_device *dev, const void *data, size_t size);
-void i915_init_vm(struct drm_i915_private *dev_priv,
-		  struct i915_address_space *vm);
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb0df7e..4811f8a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs *ring)
 	INIT_LIST_HEAD(&ring->request_list);
 }
 
-void i915_init_vm(struct drm_i915_private *dev_priv,
-		  struct i915_address_space *vm)
-{
-	if (!i915_is_ggtt(vm))
-		drm_mm_init(&vm->mm, vm->start, vm->total);
-	vm->dev = dev_priv->dev;
-	INIT_LIST_HEAD(&vm->active_list);
-	INIT_LIST_HEAD(&vm->inactive_list);
-	INIT_LIST_HEAD(&vm->global_link);
-	list_add_tail(&vm->global_link, &dev_priv->vm_list);
-}
-
 void
 i915_gem_load(struct drm_device *dev)
 {
@@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
 				  NULL);
 
 	INIT_LIST_HEAD(&dev_priv->vm_list);
-	i915_init_vm(dev_priv, &dev_priv->gtt.base);
-
 	INIT_LIST_HEAD(&dev_priv->context_list);
 	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
 	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8786281..01f3521 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2121,6 +2121,16 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 		return gen8_ppgtt_init(ppgtt);
 }
 
+static void i915_address_space_init(struct i915_address_space *vm,
+				    struct drm_i915_private *dev_priv)
+{
+	drm_mm_init(&vm->mm, vm->start, vm->total);
+	vm->dev = dev_priv->dev;
+	INIT_LIST_HEAD(&vm->active_list);
+	INIT_LIST_HEAD(&vm->inactive_list);
+	list_add_tail(&vm->global_link, &dev_priv->vm_list);
+}
+
 int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2129,9 +2139,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 	ret = __hw_ppgtt_init(dev, ppgtt);
 	if (ret == 0) {
 		kref_init(&ppgtt->ref);
-		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
-			    ppgtt->base.total);
-		i915_init_vm(dev_priv, &ppgtt->base);
+		i915_address_space_init(&ppgtt->base, dev_priv);
 	}
 
 	return ret;
@@ -2618,11 +2626,13 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 
 	BUG_ON(mappable_end > end);
 
-	/* Subtract the guard page ... */
-	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+	ggtt_vm->start = start;
 
-	dev_priv->gtt.base.start = start;
-	dev_priv->gtt.base.total = end - start;
+	/* Subtract the guard page before address space initialization to
+	 * shrink the range used by drm_mm */
+	ggtt_vm->total = end - start - PAGE_SIZE;
+	i915_address_space_init(ggtt_vm, dev_priv);
+	ggtt_vm->total += PAGE_SIZE;
 
 	if (intel_vgpu_active(dev)) {
 		ret = intel_vgt_balloon(dev);
@@ -2631,7 +2641,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 	}
 
 	if (!HAS_LLC(dev))
-		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
+		ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
 
 	/* Mark any preallocated objects as occupied */
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-- 
2.4.3

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

[PATCH v3 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Michał Winiarski]

According to bspec, some parts of HW expect the addresses to be in
a canonical form where bits [63:48] == [47].
If we're using 32b addressing, we never need to handle such high
addresses, but since we've recently added 48b address space support,
lets satisfy the HW by converting the address prior to
alloc/insert/clear.

v2: Commit msg update,
    move WARN to gen8_alloc_va_range,
    also convert to canonical in insert_entries.
v3: Also convert for !48b (causes no harm, improves code stucture),
    update wrap comment.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 20 ++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 01f3521..feb499d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -759,6 +759,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
 		container_of(vm, struct i915_hw_ppgtt, base);
 	gen8_pte_t scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
 						 I915_CACHE_LLC, use_scratch);
+	start = gen8_canonical_addr(start);
 
 	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
 		gen8_ppgtt_clear_pte_range(vm, &ppgtt->pdp, start, length,
@@ -827,6 +828,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 	struct sg_page_iter sg_iter;
 
 	__sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl), 0);
+	start = gen8_canonical_addr(start);
 
 	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
 		gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
@@ -1227,14 +1229,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)
@@ -1377,6 +1371,16 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 
+	/* Wrap is not okay since we're dealing with addresses that have not
+	 * been converted to canonical form yet */
+	if (WARN_ON(start + length < start))
+		return -ENODEV;
+
+	if (WARN_ON(start + length > vm->total))
+		return -ENODEV;
+
+	start = gen8_canonical_addr(start);
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev))
 		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
 	else
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8275007..9397387 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -503,6 +503,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
 }
 
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return ((int64_t)address << 16) >> 16;
+}
+
 static inline dma_addr_t
 i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 {
-- 
2.4.3

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

Re: [PATCH v3 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Chris Wilson]

On Wed, Sep 16, 2015 at 11:50:50AM +0200, Michał Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47].
> If we're using 32b addressing, we never need to handle such high
> addresses, but since we've recently added 48b address space support,
> lets satisfy the HW by converting the address prior to
> alloc/insert/clear.
> 
> v2: Commit msg update,
>     move WARN to gen8_alloc_va_range,
>     also convert to canonical in insert_entries.
> v3: Also convert for !48b (causes no harm, improves code stucture),
>     update wrap comment.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

I didn't even complain about the surplus brackets :)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Chris Wilson]

On Wed, Sep 16, 2015 at 11:49:00AM +0200, Michał Winiarski wrote:

Rearrange the code such that i915_init_vm() is next to its callers
inside i915_gem_gtt (and so we can make it static). After removing the
dance around the files, it is clear that we are repeating some work
inside the initializers (such as calling drm_mm_init() multiple times),
so take advantage of the refactor to also remove some redundant code and
clean up the interface.

> v2: Commit msg update,
>     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
>     init address_space during i915_gem_setup_global_gtt for ggtt.
> v3: Do not init global_link - we are adding it to vm_list moments later,
>     make i915_address_space_init static, use OOP style parameter order.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Winiarski, Michal]

On śro, 2015-09-16 at 11:50 +0200, Michał Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47].
> If we're using 32b addressing, we never need to handle such high
> addresses, but since we've recently added 48b address space support,
> lets satisfy the HW by converting the address prior to
> alloc/insert/clear.
> 
> v2: Commit msg update,
>     move WARN to gen8_alloc_va_range,
>     also convert to canonical in insert_entries.
> v3: Also convert for !48b (causes no harm, improves code stucture),
>     update wrap comment.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Actually... Please ignore this patch.
It's harmless, unfortunately it's also useless, and I need to address
this in a different way.
Sorry for the noise.

-Michał

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

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Daniel Vetter]

On Thu, Sep 17, 2015 at 11:59:19AM +0100, Chris Wilson wrote:
> On Wed, Sep 16, 2015 at 11:49:00AM +0200, Michał Winiarski wrote:
> 
> Rearrange the code such that i915_init_vm() is next to its callers
> inside i915_gem_gtt (and so we can make it static). After removing the
> dance around the files, it is clear that we are repeating some work
> inside the initializers (such as calling drm_mm_init() multiple times),
> so take advantage of the refactor to also remove some redundant code and
> clean up the interface.

Added to the commit message.

> > v2: Commit msg update,
> >     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
> >     init address_space during i915_gem_setup_global_gtt for ggtt.
> > v3: Do not init global_link - we are adding it to vm_list moments later,
> >     make i915_address_space_init static, use OOP style parameter order.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

[Daniel Vetter]

On Thu, Sep 17, 2015 at 05:17:24PM +0000, Winiarski, Michal wrote:
> On śro, 2015-09-16 at 11:50 +0200, Michał Winiarski wrote:
> > According to bspec, some parts of HW expect the addresses to be in
> > a canonical form where bits [63:48] == [47].
> > If we're using 32b addressing, we never need to handle such high
> > addresses, but since we've recently added 48b address space support,
> > lets satisfy the HW by converting the address prior to
> > alloc/insert/clear.
> > 
> > v2: Commit msg update,
> >     move WARN to gen8_alloc_va_range,
> >     also convert to canonical in insert_entries.
> > v3: Also convert for !48b (causes no harm, improves code stucture),
> >     update wrap comment.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> Actually... Please ignore this patch.
> It's harmless, unfortunately it's also useless, and I need to address
> this in a different way.

Please explain in more detail why this patch is void - it seems to do
exactly what it says on the tin?

> Sorry for the noise.

Explaining why something doesn't work or not is never noise, but a great
opportunity to learn something.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Imre Deak]

On ke, 2015-09-16 at 11:49 +0200, Michał Winiarski wrote:
> It would be initialized just moments later by i915_init_vm.
> 
> v2: Commit msg update,
>     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
>     init address_space during i915_gem_setup_global_gtt for ggtt.
> v3: Do not init global_link - we are adding it to vm_list moments later,
>     make i915_address_space_init static, use OOP style parameter order.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

With this patch I get the oops below when booting. Looking at it it's
caused by dev_priv->gtt being initialized now too late for
i915_gem_object_create_stolen_for_preallocated().

[   13.547091] BUG: unable to handle kernel NULL pointer dereference at           (null)
[   13.555857] IP: [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
[   13.566380] PGD 0 
[   13.568634] Oops: 0002 [#1] PREEMPT SMP 
[   13.573035] Modules linked in: snd_hda_codec_hdmi snd_hda_intel i915(O+) snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_seq_dummy snd_seq_oss kvm_intel i2c_algo_bit snd_seq_midi drm_kms_helper kvm snd_rawmidi syscopyarea sysfillrect sysimgblt snd_seq_midi_event crc32c_intel fb_sys_fops microcode snd_seq drm snd_seq_device snd_timer efivars intel_gtt snd agpgart fuse
[   13.609814] CPU: 1 PID: 329 Comm: systemd-udevd Tainted: G          IO    4.3.0-rc2-bxt+ #177
[   13.619401] Hardware name: Intel Corp. BROXTON A1 PLATFORM/TABLET, BIOS BXTM_IFWI_X64_R_2015_36_4_00 09/02/2015
[   13.630681] task: ffff88007674cc80 ti: ffff880075c1c000 task.ti: ffff880075c1c000
[   13.639049] RIP: 0010:[<ffffffffa0243f09>]  [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
[   13.652219] RSP: 0018:ffff880075c1f810  EFLAGS: 00010286
[   13.658154] RAX: 0000000000000000 RBX: 00000000008ca000 RCX: ffff880072e61ca8
[   13.666129] RDX: ffff880072e61c00 RSI: ffff88006de4a0c8 RDI: ffff88007674d4c8
[   13.674103] RBP: ffff880075c1f848 R08: 00000000000196f0 R09: ffff880072e61c00
[   13.682078] R10: 0000000000000001 R11: 0000000000000001 R12: ffff88006de49f18
[   13.690053] R13: ffff88006df00000 R14: ffff8800754154e0 R15: 0000000000000000
[   13.698034] FS:  00007f4d61c0f880(0000) GS:ffff880079480000(0000) knlGS:0000000000000000
[   13.707089] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   13.713513] CR2: 0000000000000000 CR3: 0000000073304000 CR4: 00000000003406e0
[   13.721494] Stack:
[   13.723740]  ffff88006de4a048 ffff88006de49e18 ffff880075c1f928 ffff880075c1f858
[   13.732044]  ffff880072cba540 ffff880076061000 ffff880076061000 ffff880075c1f8e8
[   13.740345]  ffffffffa027bdbc 0000000000000000 0000000000000000
[   13.745411] sdhci-pci 0000:00:1c.0: power state changed by ACPI to D3cold
[   13.754373]  0000000000000000

[   13.757903] Call Trace:
[   13.760709]  [<ffffffffa027bdbc>] intel_alloc_initial_plane_obj.isra.56+0x7c/0x190 [i915]
[   13.769930]  [<ffffffffa0287654>] intel_modeset_init+0xbb4/0x1a10 [i915]
[   13.777495]  [<ffffffffa02c03fc>] i915_driver_load+0xeec/0x14e0 [i915]
[   13.784831]  [<ffffffffa0069569>] drm_dev_register+0xa9/0xc0 [drm]
[   13.791762]  [<ffffffffa006c37d>] drm_get_pci_dev+0x8d/0x1e0 [drm]
[   13.798682]  [<ffffffff818c7e42>] ? _raw_spin_unlock_irqrestore+0x42/0x70
[   13.806321]  [<ffffffffa0201234>] i915_pci_probe+0x34/0x50 [i915]
[   13.813144]  [<ffffffff81452cbc>] pci_device_probe+0x8c/0x100
[   13.819577]  [<ffffffff8152a5a9>] driver_probe_device+0x169/0x4a0
[   13.826393]  [<ffffffff8152a968>] __driver_attach+0x88/0x90
[   13.832628]  [<ffffffff8152a8e0>] ? driver_probe_device+0x4a0/0x4a0
[   13.839643]  [<ffffffff81528126>] bus_for_each_dev+0x66/0xa0
[   13.845976]  [<ffffffff81529f5e>] driver_attach+0x1e/0x20
[   13.852012]  [<ffffffff815299b4>] bus_add_driver+0x1f4/0x290
[   13.858343]  [<ffffffff8152b540>] driver_register+0x60/0xe0
[   13.864577]  [<ffffffff814513b0>] __pci_register_driver+0x60/0x70
[   13.871410]  [<ffffffffa006c5b0>] drm_pci_init+0xe0/0x110 [drm]
[   13.878034]  [<ffffffff810d024d>] ? trace_hardirqs_on+0xd/0x10
[   13.884560]  [<ffffffffa032b000>] ? 0xffffffffa032b000
[   13.890350]  [<ffffffffa032b094>] i915_init+0x94/0x9b [i915]
[   13.896685]  [<ffffffff810002f3>] do_one_initcall+0xb3/0x1d0
[   13.903016]  [<ffffffff810ed246>] ? rcu_read_lock_sched_held+0x86/0x90
[   13.910323]  [<ffffffff811e21f1>] ? kmem_cache_alloc_trace+0x1c1/0x270
[   13.917626]  [<ffffffff81192893>] ? do_init_module+0x27/0x1ea
[   13.924052]  [<ffffffff811928cc>] do_init_module+0x60/0x1ea
[   13.930288]  [<ffffffff81114b02>] load_module+0x2012/0x2580
[   13.936521]  [<ffffffff811109a0>] ? store_uevent+0x40/0x40
[   13.942659]  [<ffffffff81111246>] ? copy_module_from_fd.isra.36+0xf6/0x140
[   13.950350]  [<ffffffff81115270>] SyS_finit_module+0x80/0xb0
[   13.956681]  [<ffffffff818c86db>] entry_SYSCALL_64_fastpath+0x16/0x73


> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 --
>  drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++--------
>  3 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b..039227d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2804,8 +2804,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  						  size_t size);
>  struct drm_i915_gem_object *i915_gem_object_create_from_data(
>  		struct drm_device *dev, const void *data, size_t size);
> -void i915_init_vm(struct drm_i915_private *dev_priv,
> -		  struct i915_address_space *vm);
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index cb0df7e..4811f8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs *ring)
>  	INIT_LIST_HEAD(&ring->request_list);
>  }
>  
> -void i915_init_vm(struct drm_i915_private *dev_priv,
> -		  struct i915_address_space *vm)
> -{
> -	if (!i915_is_ggtt(vm))
> -		drm_mm_init(&vm->mm, vm->start, vm->total);
> -	vm->dev = dev_priv->dev;
> -	INIT_LIST_HEAD(&vm->active_list);
> -	INIT_LIST_HEAD(&vm->inactive_list);
> -	INIT_LIST_HEAD(&vm->global_link);
> -	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> -}
> -
>  void
>  i915_gem_load(struct drm_device *dev)
>  {
> @@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
>  				  NULL);
>  
>  	INIT_LIST_HEAD(&dev_priv->vm_list);
> -	i915_init_vm(dev_priv, &dev_priv->gtt.base);
> -
>  	INIT_LIST_HEAD(&dev_priv->context_list);
>  	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
>  	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8786281..01f3521 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2121,6 +2121,16 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  		return gen8_ppgtt_init(ppgtt);
>  }
>  
> +static void i915_address_space_init(struct i915_address_space *vm,
> +				    struct drm_i915_private *dev_priv)
> +{
> +	drm_mm_init(&vm->mm, vm->start, vm->total);
> +	vm->dev = dev_priv->dev;
> +	INIT_LIST_HEAD(&vm->active_list);
> +	INIT_LIST_HEAD(&vm->inactive_list);
> +	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> +}
> +
>  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2129,9 +2139,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
> -		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> -			    ppgtt->base.total);
> -		i915_init_vm(dev_priv, &ppgtt->base);
> +		i915_address_space_init(&ppgtt->base, dev_priv);
>  	}
>  
>  	return ret;
> @@ -2618,11 +2626,13 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  
>  	BUG_ON(mappable_end > end);
>  
> -	/* Subtract the guard page ... */
> -	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> +	ggtt_vm->start = start;
>  
> -	dev_priv->gtt.base.start = start;
> -	dev_priv->gtt.base.total = end - start;
> +	/* Subtract the guard page before address space initialization to
> +	 * shrink the range used by drm_mm */
> +	ggtt_vm->total = end - start - PAGE_SIZE;
> +	i915_address_space_init(ggtt_vm, dev_priv);
> +	ggtt_vm->total += PAGE_SIZE;
>  
>  	if (intel_vgpu_active(dev)) {
>  		ret = intel_vgt_balloon(dev);
> @@ -2631,7 +2641,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	}
>  
>  	if (!HAS_LLC(dev))
> -		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
> +		ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
>  
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {


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

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Paulo Zanoni]

2015-09-23 13:48 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On ke, 2015-09-16 at 11:49 +0200, Michał Winiarski wrote:
>> It would be initialized just moments later by i915_init_vm.
>>
>> v2: Commit msg update,
>>     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
>>     init address_space during i915_gem_setup_global_gtt for ggtt.
>> v3: Do not init global_link - we are adding it to vm_list moments later,
>>     make i915_address_space_init static, use OOP style parameter order.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>
> With this patch I get the oops below when booting. Looking at it it's
> caused by dev_priv->gtt being initialized now too late for
> i915_gem_object_create_stolen_for_preallocated().

My BDW machine stopped booting, and git bisect tells me this patch is
the cause. I didn't setup anything to help me grab the logs before the
crash, so I can't confirm the backtrace below.

Something interesting is that if I boot with i915.modeset=0, then
later "rmmod i915; modprobe i915 modeset=1", the driver loads
successfully.

>
> [   13.547091] BUG: unable to handle kernel NULL pointer dereference at           (null)
> [   13.555857] IP: [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
> [   13.566380] PGD 0
> [   13.568634] Oops: 0002 [#1] PREEMPT SMP
> [   13.573035] Modules linked in: snd_hda_codec_hdmi snd_hda_intel i915(O+) snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_seq_dummy snd_seq_oss kvm_intel i2c_algo_bit snd_seq_midi drm_kms_helper kvm snd_rawmidi syscopyarea sysfillrect sysimgblt snd_seq_midi_event crc32c_intel fb_sys_fops microcode snd_seq drm snd_seq_device snd_timer efivars intel_gtt snd agpgart fuse
> [   13.609814] CPU: 1 PID: 329 Comm: systemd-udevd Tainted: G          IO    4.3.0-rc2-bxt+ #177
> [   13.619401] Hardware name: Intel Corp. BROXTON A1 PLATFORM/TABLET, BIOS BXTM_IFWI_X64_R_2015_36_4_00 09/02/2015
> [   13.630681] task: ffff88007674cc80 ti: ffff880075c1c000 task.ti: ffff880075c1c000
> [   13.639049] RIP: 0010:[<ffffffffa0243f09>]  [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
> [   13.652219] RSP: 0018:ffff880075c1f810  EFLAGS: 00010286
> [   13.658154] RAX: 0000000000000000 RBX: 00000000008ca000 RCX: ffff880072e61ca8
> [   13.666129] RDX: ffff880072e61c00 RSI: ffff88006de4a0c8 RDI: ffff88007674d4c8
> [   13.674103] RBP: ffff880075c1f848 R08: 00000000000196f0 R09: ffff880072e61c00
> [   13.682078] R10: 0000000000000001 R11: 0000000000000001 R12: ffff88006de49f18
> [   13.690053] R13: ffff88006df00000 R14: ffff8800754154e0 R15: 0000000000000000
> [   13.698034] FS:  00007f4d61c0f880(0000) GS:ffff880079480000(0000) knlGS:0000000000000000
> [   13.707089] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   13.713513] CR2: 0000000000000000 CR3: 0000000073304000 CR4: 00000000003406e0
> [   13.721494] Stack:
> [   13.723740]  ffff88006de4a048 ffff88006de49e18 ffff880075c1f928 ffff880075c1f858
> [   13.732044]  ffff880072cba540 ffff880076061000 ffff880076061000 ffff880075c1f8e8
> [   13.740345]  ffffffffa027bdbc 0000000000000000 0000000000000000
> [   13.745411] sdhci-pci 0000:00:1c.0: power state changed by ACPI to D3cold
> [   13.754373]  0000000000000000
>
> [   13.757903] Call Trace:
> [   13.760709]  [<ffffffffa027bdbc>] intel_alloc_initial_plane_obj.isra.56+0x7c/0x190 [i915]
> [   13.769930]  [<ffffffffa0287654>] intel_modeset_init+0xbb4/0x1a10 [i915]
> [   13.777495]  [<ffffffffa02c03fc>] i915_driver_load+0xeec/0x14e0 [i915]
> [   13.784831]  [<ffffffffa0069569>] drm_dev_register+0xa9/0xc0 [drm]
> [   13.791762]  [<ffffffffa006c37d>] drm_get_pci_dev+0x8d/0x1e0 [drm]
> [   13.798682]  [<ffffffff818c7e42>] ? _raw_spin_unlock_irqrestore+0x42/0x70
> [   13.806321]  [<ffffffffa0201234>] i915_pci_probe+0x34/0x50 [i915]
> [   13.813144]  [<ffffffff81452cbc>] pci_device_probe+0x8c/0x100
> [   13.819577]  [<ffffffff8152a5a9>] driver_probe_device+0x169/0x4a0
> [   13.826393]  [<ffffffff8152a968>] __driver_attach+0x88/0x90
> [   13.832628]  [<ffffffff8152a8e0>] ? driver_probe_device+0x4a0/0x4a0
> [   13.839643]  [<ffffffff81528126>] bus_for_each_dev+0x66/0xa0
> [   13.845976]  [<ffffffff81529f5e>] driver_attach+0x1e/0x20
> [   13.852012]  [<ffffffff815299b4>] bus_add_driver+0x1f4/0x290
> [   13.858343]  [<ffffffff8152b540>] driver_register+0x60/0xe0
> [   13.864577]  [<ffffffff814513b0>] __pci_register_driver+0x60/0x70
> [   13.871410]  [<ffffffffa006c5b0>] drm_pci_init+0xe0/0x110 [drm]
> [   13.878034]  [<ffffffff810d024d>] ? trace_hardirqs_on+0xd/0x10
> [   13.884560]  [<ffffffffa032b000>] ? 0xffffffffa032b000
> [   13.890350]  [<ffffffffa032b094>] i915_init+0x94/0x9b [i915]
> [   13.896685]  [<ffffffff810002f3>] do_one_initcall+0xb3/0x1d0
> [   13.903016]  [<ffffffff810ed246>] ? rcu_read_lock_sched_held+0x86/0x90
> [   13.910323]  [<ffffffff811e21f1>] ? kmem_cache_alloc_trace+0x1c1/0x270
> [   13.917626]  [<ffffffff81192893>] ? do_init_module+0x27/0x1ea
> [   13.924052]  [<ffffffff811928cc>] do_init_module+0x60/0x1ea
> [   13.930288]  [<ffffffff81114b02>] load_module+0x2012/0x2580
> [   13.936521]  [<ffffffff811109a0>] ? store_uevent+0x40/0x40
> [   13.942659]  [<ffffffff81111246>] ? copy_module_from_fd.isra.36+0xf6/0x140
> [   13.950350]  [<ffffffff81115270>] SyS_finit_module+0x80/0xb0
> [   13.956681]  [<ffffffff818c86db>] entry_SYSCALL_64_fastpath+0x16/0x73
>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     |  2 --
>>  drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++--------
>>  3 files changed, 18 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 3bf8a9b..039227d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2804,8 +2804,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>>                                                 size_t size);
>>  struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>               struct drm_device *dev, const void *data, size_t size);
>> -void i915_init_vm(struct drm_i915_private *dev_priv,
>> -               struct i915_address_space *vm);
>>  void i915_gem_free_object(struct drm_gem_object *obj);
>>  void i915_gem_vma_destroy(struct i915_vma *vma);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index cb0df7e..4811f8a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs *ring)
>>       INIT_LIST_HEAD(&ring->request_list);
>>  }
>>
>> -void i915_init_vm(struct drm_i915_private *dev_priv,
>> -               struct i915_address_space *vm)
>> -{
>> -     if (!i915_is_ggtt(vm))
>> -             drm_mm_init(&vm->mm, vm->start, vm->total);
>> -     vm->dev = dev_priv->dev;
>> -     INIT_LIST_HEAD(&vm->active_list);
>> -     INIT_LIST_HEAD(&vm->inactive_list);
>> -     INIT_LIST_HEAD(&vm->global_link);
>> -     list_add_tail(&vm->global_link, &dev_priv->vm_list);
>> -}
>> -
>>  void
>>  i915_gem_load(struct drm_device *dev)
>>  {
>> @@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
>>                                 NULL);
>>
>>       INIT_LIST_HEAD(&dev_priv->vm_list);
>> -     i915_init_vm(dev_priv, &dev_priv->gtt.base);
>> -
>>       INIT_LIST_HEAD(&dev_priv->context_list);
>>       INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
>>       INIT_LIST_HEAD(&dev_priv->mm.bound_list);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 8786281..01f3521 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2121,6 +2121,16 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>>               return gen8_ppgtt_init(ppgtt);
>>  }
>>
>> +static void i915_address_space_init(struct i915_address_space *vm,
>> +                                 struct drm_i915_private *dev_priv)
>> +{
>> +     drm_mm_init(&vm->mm, vm->start, vm->total);
>> +     vm->dev = dev_priv->dev;
>> +     INIT_LIST_HEAD(&vm->active_list);
>> +     INIT_LIST_HEAD(&vm->inactive_list);
>> +     list_add_tail(&vm->global_link, &dev_priv->vm_list);
>> +}
>> +
>>  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -2129,9 +2139,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>>       ret = __hw_ppgtt_init(dev, ppgtt);
>>       if (ret == 0) {
>>               kref_init(&ppgtt->ref);
>> -             drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
>> -                         ppgtt->base.total);
>> -             i915_init_vm(dev_priv, &ppgtt->base);
>> +             i915_address_space_init(&ppgtt->base, dev_priv);
>>       }
>>
>>       return ret;
>> @@ -2618,11 +2626,13 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>>
>>       BUG_ON(mappable_end > end);
>>
>> -     /* Subtract the guard page ... */
>> -     drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
>> +     ggtt_vm->start = start;
>>
>> -     dev_priv->gtt.base.start = start;
>> -     dev_priv->gtt.base.total = end - start;
>> +     /* Subtract the guard page before address space initialization to
>> +      * shrink the range used by drm_mm */
>> +     ggtt_vm->total = end - start - PAGE_SIZE;
>> +     i915_address_space_init(ggtt_vm, dev_priv);
>> +     ggtt_vm->total += PAGE_SIZE;
>>
>>       if (intel_vgpu_active(dev)) {
>>               ret = intel_vgt_balloon(dev);
>> @@ -2631,7 +2641,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>>       }
>>
>>       if (!HAS_LLC(dev))
>> -             dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
>> +             ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
>>
>>       /* Mark any preallocated objects as occupied */
>>       list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Jesse Barnes]

On 09/23/2015 11:55 AM, Paulo Zanoni wrote:
> 2015-09-23 13:48 GMT-03:00 Imre Deak <imre.deak@intel.com>:
>> On ke, 2015-09-16 at 11:49 +0200, Michał Winiarski wrote:
>>> It would be initialized just moments later by i915_init_vm.
>>>
>>> v2: Commit msg update,
>>>     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
>>>     init address_space during i915_gem_setup_global_gtt for ggtt.
>>> v3: Do not init global_link - we are adding it to vm_list moments later,
>>>     make i915_address_space_init static, use OOP style parameter order.
>>>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Michel Thierry <michel.thierry@intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>
>> With this patch I get the oops below when booting. Looking at it it's
>> caused by dev_priv->gtt being initialized now too late for
>> i915_gem_object_create_stolen_for_preallocated().
> 
> My BDW machine stopped booting, and git bisect tells me this patch is
> the cause. I didn't setup anything to help me grab the logs before the
> crash, so I can't confirm the backtrace below.
> 
> Something interesting is that if I boot with i915.modeset=0, then
> later "rmmod i915; modprobe i915 modeset=1", the driver loads
> successfully.
> 

I see this too on BYT.  Can we get this reverted or get a fix?

Michal?

Thanks,
Jesse

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

Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Winiarski, Michal]

On Wed, 2015-09-23 at 15:55 -0300, Paulo Zanoni wrote:
> 2015-09-23 13:48 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > On ke, 2015-09-16 at 11:49 +0200, Michał Winiarski wrote:
> > > It would be initialized just moments later by i915_init_vm.
> > > 
> > > v2: Commit msg update,
> > >     s/i915_init_vm/i915_address_space_init, move to
> > > i915_gem_gtt.c,
> > >     init address_space during i915_gem_setup_global_gtt for ggtt.
> > > v3: Do not init global_link - we are adding it to vm_list moments
> > > later,
> > >     make i915_address_space_init static, use OOP style parameter
> > > order.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michel Thierry <michel.thierry@intel.com>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > With this patch I get the oops below when booting. Looking at it
> > it's
> > caused by dev_priv->gtt being initialized now too late for
> > i915_gem_object_create_stolen_for_preallocated().
> 
> My BDW machine stopped booting, and git bisect tells me this patch is
> the cause. I didn't setup anything to help me grab the logs before
> the
> crash, so I can't confirm the backtrace below.
> 
> Something interesting is that if I boot with i915.modeset=0, then
> later "rmmod i915; modprobe i915 modeset=1", the driver loads
> successfully.

drm/i915: Defer adding preallocated stolen objects to the VM list
from Chris fixes this. I missed it because I usually just reload i915
module after the machine boots - sorry for breaking your setups :(

-Michał
 
> > 
> > [   13.547091] BUG: unable to handle kernel NULL pointer
> > dereference at           (null)
> > [   13.555857] IP: [<ffffffffa0243f09>]
> > i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
> > [   13.566380] PGD 0
> > [   13.568634] Oops: 0002 [#1] PREEMPT SMP
> > [   13.573035] Modules linked in: snd_hda_codec_hdmi snd_hda_intel
> > i915(O+) snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_seq_dummy
> > snd_seq_oss kvm_intel i2c_algo_bit snd_seq_midi drm_kms_helper kvm
> > snd_rawmidi syscopyarea sysfillrect sysimgblt snd_seq_midi_event
> > crc32c_intel fb_sys_fops microcode snd_seq drm snd_seq_device
> > snd_timer efivars intel_gtt snd agpgart fuse
> > [   13.609814] CPU: 1 PID: 329 Comm: systemd-udevd Tainted: G      
> >     IO    4.3.0-rc2-bxt+ #177
> > [   13.619401] Hardware name: Intel Corp. BROXTON A1
> > PLATFORM/TABLET, BIOS BXTM_IFWI_X64_R_2015_36_4_00 09/02/2015
> > [   13.630681] task: ffff88007674cc80 ti: ffff880075c1c000 task.ti:
> > ffff880075c1c000
> > [   13.639049] RIP: 0010:[<ffffffffa0243f09>]  [<ffffffffa0243f09>]
> > i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
> > [   13.652219] RSP: 0018:ffff880075c1f810  EFLAGS: 00010286
> > [   13.658154] RAX: 0000000000000000 RBX: 00000000008ca000 RCX:
> > ffff880072e61ca8
> > [   13.666129] RDX: ffff880072e61c00 RSI: ffff88006de4a0c8 RDI:
> > ffff88007674d4c8
> > [   13.674103] RBP: ffff880075c1f848 R08: 00000000000196f0 R09:
> > ffff880072e61c00
> > [   13.682078] R10: 0000000000000001 R11: 0000000000000001 R12:
> > ffff88006de49f18
> > [   13.690053] R13: ffff88006df00000 R14: ffff8800754154e0 R15:
> > 0000000000000000
> > [   13.698034] FS:  00007f4d61c0f880(0000)
> > GS:ffff880079480000(0000) knlGS:0000000000000000
> > [   13.707089] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   13.713513] CR2: 0000000000000000 CR3: 0000000073304000 CR4:
> > 00000000003406e0
> > [   13.721494] Stack:
> > [   13.723740]  ffff88006de4a048 ffff88006de49e18 ffff880075c1f928
> > ffff880075c1f858
> > [   13.732044]  ffff880072cba540 ffff880076061000 ffff880076061000
> > ffff880075c1f8e8
> > [   13.740345]  ffffffffa027bdbc 0000000000000000 0000000000000000
> > [   13.745411] sdhci-pci 0000:00:1c.0: power state changed by ACPI
> > to D3cold
> > [   13.754373]  0000000000000000
> > 
> > [   13.757903] Call Trace:
> > [   13.760709]  [<ffffffffa027bdbc>]
> > intel_alloc_initial_plane_obj.isra.56+0x7c/0x190 [i915]
> > [   13.769930]  [<ffffffffa0287654>]
> > intel_modeset_init+0xbb4/0x1a10 [i915]
> > [   13.777495]  [<ffffffffa02c03fc>] i915_driver_load+0xeec/0x14e0
> > [i915]
> > [   13.784831]  [<ffffffffa0069569>] drm_dev_register+0xa9/0xc0
> > [drm]
> > [   13.791762]  [<ffffffffa006c37d>] drm_get_pci_dev+0x8d/0x1e0
> > [drm]
> > [   13.798682]  [<ffffffff818c7e42>] ?
> > _raw_spin_unlock_irqrestore+0x42/0x70
> > [   13.806321]  [<ffffffffa0201234>] i915_pci_probe+0x34/0x50
> > [i915]
> > [   13.813144]  [<ffffffff81452cbc>] pci_device_probe+0x8c/0x100
> > [   13.819577]  [<ffffffff8152a5a9>]
> > driver_probe_device+0x169/0x4a0
> > [   13.826393]  [<ffffffff8152a968>] __driver_attach+0x88/0x90
> > [   13.832628]  [<ffffffff8152a8e0>] ?
> > driver_probe_device+0x4a0/0x4a0
> > [   13.839643]  [<ffffffff81528126>] bus_for_each_dev+0x66/0xa0
> > [   13.845976]  [<ffffffff81529f5e>] driver_attach+0x1e/0x20
> > [   13.852012]  [<ffffffff815299b4>] bus_add_driver+0x1f4/0x290
> > [   13.858343]  [<ffffffff8152b540>] driver_register+0x60/0xe0
> > [   13.864577]  [<ffffffff814513b0>]
> > __pci_register_driver+0x60/0x70
> > [   13.871410]  [<ffffffffa006c5b0>] drm_pci_init+0xe0/0x110 [drm]
> > [   13.878034]  [<ffffffff810d024d>] ? trace_hardirqs_on+0xd/0x10
> > [   13.884560]  [<ffffffffa032b000>] ? 0xffffffffa032b000
> > [   13.890350]  [<ffffffffa032b094>] i915_init+0x94/0x9b [i915]
> > [   13.896685]  [<ffffffff810002f3>] do_one_initcall+0xb3/0x1d0
> > [   13.903016]  [<ffffffff810ed246>] ?
> > rcu_read_lock_sched_held+0x86/0x90
> > [   13.910323]  [<ffffffff811e21f1>] ?
> > kmem_cache_alloc_trace+0x1c1/0x270
> > [   13.917626]  [<ffffffff81192893>] ? do_init_module+0x27/0x1ea
> > [   13.924052]  [<ffffffff811928cc>] do_init_module+0x60/0x1ea
> > [   13.930288]  [<ffffffff81114b02>] load_module+0x2012/0x2580
> > [   13.936521]  [<ffffffff811109a0>] ? store_uevent+0x40/0x40
> > [   13.942659]  [<ffffffff81111246>] ?
> > copy_module_from_fd.isra.36+0xf6/0x140
> > [   13.950350]  [<ffffffff81115270>] SyS_finit_module+0x80/0xb0
> > [   13.956681]  [<ffffffff818c86db>]
> > entry_SYSCALL_64_fastpath+0x16/0x73
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h     |  2 --
> > >  drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++-----
> > > ---
> > >  3 files changed, 18 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 3bf8a9b..039227d 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2804,8 +2804,6 @@ struct drm_i915_gem_object
> > > *i915_gem_alloc_object(struct drm_device *dev,
> > >                                                 size_t size);
> > >  struct drm_i915_gem_object *i915_gem_object_create_from_data(
> > >               struct drm_device *dev, const void *data, size_t
> > > size);
> > > -void i915_init_vm(struct drm_i915_private *dev_priv,
> > > -               struct i915_address_space *vm);
> > >  void i915_gem_free_object(struct drm_gem_object *obj);
> > >  void i915_gem_vma_destroy(struct i915_vma *vma);
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index cb0df7e..4811f8a 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs
> > > *ring)
> > >       INIT_LIST_HEAD(&ring->request_list);
> > >  }
> > > 
> > > -void i915_init_vm(struct drm_i915_private *dev_priv,
> > > -               struct i915_address_space *vm)
> > > -{
> > > -     if (!i915_is_ggtt(vm))
> > > -             drm_mm_init(&vm->mm, vm->start, vm->total);
> > > -     vm->dev = dev_priv->dev;
> > > -     INIT_LIST_HEAD(&vm->active_list);
> > > -     INIT_LIST_HEAD(&vm->inactive_list);
> > > -     INIT_LIST_HEAD(&vm->global_link);
> > > -     list_add_tail(&vm->global_link, &dev_priv->vm_list);
> > > -}
> > > -
> > >  void
> > >  i915_gem_load(struct drm_device *dev)
> > >  {
> > > @@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
> > >                                 NULL);
> > > 
> > >       INIT_LIST_HEAD(&dev_priv->vm_list);
> > > -     i915_init_vm(dev_priv, &dev_priv->gtt.base);
> > > -
> > >       INIT_LIST_HEAD(&dev_priv->context_list);
> > >       INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
> > >       INIT_LIST_HEAD(&dev_priv->mm.bound_list);
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 8786281..01f3521 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -2121,6 +2121,16 @@ static int __hw_ppgtt_init(struct
> > > drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > >               return gen8_ppgtt_init(ppgtt);
> > >  }
> > > 
> > > +static void i915_address_space_init(struct i915_address_space
> > > *vm,
> > > +                                 struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +     drm_mm_init(&vm->mm, vm->start, vm->total);
> > > +     vm->dev = dev_priv->dev;
> > > +     INIT_LIST_HEAD(&vm->active_list);
> > > +     INIT_LIST_HEAD(&vm->inactive_list);
> > > +     list_add_tail(&vm->global_link, &dev_priv->vm_list);
> > > +}
> > > +
> > >  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt
> > > *ppgtt)
> > >  {
> > >       struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -2129,9 +2139,7 @@ int i915_ppgtt_init(struct drm_device *dev,
> > > struct i915_hw_ppgtt *ppgtt)
> > >       ret = __hw_ppgtt_init(dev, ppgtt);
> > >       if (ret == 0) {
> > >               kref_init(&ppgtt->ref);
> > > -             drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> > > -                         ppgtt->base.total);
> > > -             i915_init_vm(dev_priv, &ppgtt->base);
> > > +             i915_address_space_init(&ppgtt->base, dev_priv);
> > >       }
> > > 
> > >       return ret;
> > > @@ -2618,11 +2626,13 @@ static int
> > > i915_gem_setup_global_gtt(struct drm_device *dev,
> > > 
> > >       BUG_ON(mappable_end > end);
> > > 
> > > -     /* Subtract the guard page ... */
> > > -     drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> > > +     ggtt_vm->start = start;
> > > 
> > > -     dev_priv->gtt.base.start = start;
> > > -     dev_priv->gtt.base.total = end - start;
> > > +     /* Subtract the guard page before address space
> > > initialization to
> > > +      * shrink the range used by drm_mm */
> > > +     ggtt_vm->total = end - start - PAGE_SIZE;
> > > +     i915_address_space_init(ggtt_vm, dev_priv);
> > > +     ggtt_vm->total += PAGE_SIZE;
> > > 
> > >       if (intel_vgpu_active(dev)) {
> > >               ret = intel_vgt_balloon(dev);
> > > @@ -2631,7 +2641,7 @@ static int i915_gem_setup_global_gtt(struct
> > > drm_device *dev,
> > >       }
> > > 
> > >       if (!HAS_LLC(dev))
> > > -             dev_priv->gtt.base.mm.color_adjust =
> > > i915_gtt_color_adjust;
> > > +             ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
> > > 
> > >       /* Mark any preallocated objects as occupied */
> > >       list_for_each_entry(obj, &dev_priv->mm.bound_list,
> > > global_list) {
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx