[PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[Mika Kuoppala]

When we have bound vma into an address space, the layout
of page table structures is immutable. So we can be absolutely
certain that if vma is already bound, there is no need to
(re)allocate a virtual address range for it.

v2: - add sanity checks and remove superfluous GLOBAL_BIND set
    - we might do update for an unbound vma (Chris)

Testcase: igt/gem_exec_big #bdw
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..9d3852c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1928,8 +1928,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 		vma->vm->insert_entries(vma->vm, pages,
 					vma->node.start,
 					cache_level, pte_flags);
-
-		vma->bound |= GLOBAL_BIND;
 	}
 
 	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
@@ -2804,21 +2802,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
-	int ret = 0;
-	u32 bind_flags = 0;
-
-	if (vma->vm->allocate_va_range) {
-		trace_i915_va_alloc(vma->vm, vma->node.start,
-				    vma->node.size,
-				    VM_TO_TRACE_NAME(vma->vm));
+	int ret;
+	u32 bind_flags;
 
-		ret = vma->vm->allocate_va_range(vma->vm,
-						 vma->node.start,
-						 vma->node.size);
-		if (ret)
-			return ret;
-	}
+	if (WARN_ON(flags == 0))
+		return -EINVAL;
 
+	bind_flags = 0;
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;
 	if (flags & PIN_USER)
@@ -2829,8 +2819,23 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	else
 		bind_flags &= ~vma->bound;
 
-	if (bind_flags)
-		ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
+	if (bind_flags == 0)
+		return 0;
+
+	if (vma->bound == 0 && vma->vm->allocate_va_range) {
+		trace_i915_va_alloc(vma->vm,
+				    vma->node.start,
+				    vma->node.size,
+				    VM_TO_TRACE_NAME(vma->vm));
+
+		ret = vma->vm->allocate_va_range(vma->vm,
+						 vma->node.start,
+						 vma->node.size);
+		if (ret)
+			return ret;
+	}
+
+	ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
 	if (ret)
 		return ret;
 
-- 
1.9.1

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

Re: [PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[Chris Wilson]

On Tue, Apr 28, 2015 at 05:56:17PM +0300, Mika Kuoppala wrote:
> When we have bound vma into an address space, the layout
> of page table structures is immutable. So we can be absolutely
> certain that if vma is already bound, there is no need to
> (re)allocate a virtual address range for it.
> 
> v2: - add sanity checks and remove superfluous GLOBAL_BIND set
>     - we might do update for an unbound vma (Chris)
> 
> Testcase: igt/gem_exec_big #bdw
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6fae6bd..9d3852c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1928,8 +1928,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
>  		vma->vm->insert_entries(vma->vm, pages,
>  					vma->node.start,
>  					cache_level, pte_flags);
> -
> -		vma->bound |= GLOBAL_BIND;
>  	}
>  
>  	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
> @@ -2804,21 +2802,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags)
>  {
> -	int ret = 0;
> -	u32 bind_flags = 0;
> -
> -	if (vma->vm->allocate_va_range) {
> -		trace_i915_va_alloc(vma->vm, vma->node.start,
> -				    vma->node.size,
> -				    VM_TO_TRACE_NAME(vma->vm));
> +	int ret;
> +	u32 bind_flags;

Whilst you are here, can you fix this to be plain unsigned.

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

[PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[Mika Kuoppala]

When we have bound vma into an address space, the layout
of page table structures is immutable. So we can be absolutely
certain that if vma is already bound, there is no need to
(re)allocate a virtual address range for it.

v2: - add sanity checks and remove superfluous GLOBAL_BIND set
    - we might do update for an unbound vma (Chris)

v3: s/u32/unsigned (Chris)

Testcase: igt/gem_exec_big #bdw
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..85f27d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1928,8 +1928,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 		vma->vm->insert_entries(vma->vm, pages,
 					vma->node.start,
 					cache_level, pte_flags);
-
-		vma->bound |= GLOBAL_BIND;
 	}
 
 	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
@@ -2804,21 +2802,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
-	int ret = 0;
-	u32 bind_flags = 0;
-
-	if (vma->vm->allocate_va_range) {
-		trace_i915_va_alloc(vma->vm, vma->node.start,
-				    vma->node.size,
-				    VM_TO_TRACE_NAME(vma->vm));
+	int ret;
+	unsigned bind_flags;
 
-		ret = vma->vm->allocate_va_range(vma->vm,
-						 vma->node.start,
-						 vma->node.size);
-		if (ret)
-			return ret;
-	}
+	if (WARN_ON(flags == 0))
+		return -EINVAL;
 
+	bind_flags = 0;
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;
 	if (flags & PIN_USER)
@@ -2829,8 +2819,23 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	else
 		bind_flags &= ~vma->bound;
 
-	if (bind_flags)
-		ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
+	if (bind_flags == 0)
+		return 0;
+
+	if (vma->bound == 0 && vma->vm->allocate_va_range) {
+		trace_i915_va_alloc(vma->vm,
+				    vma->node.start,
+				    vma->node.size,
+				    VM_TO_TRACE_NAME(vma->vm));
+
+		ret = vma->vm->allocate_va_range(vma->vm,
+						 vma->node.start,
+						 vma->node.size);
+		if (ret)
+			return ret;
+	}
+
+	ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
 	if (ret)
 		return ret;
 
-- 
1.9.1

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

Re: [PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[Chris Wilson]

On Wed, Apr 29, 2015 at 09:49:30AM +0300, Mika Kuoppala wrote:
> When we have bound vma into an address space, the layout
> of page table structures is immutable. So we can be absolutely
> certain that if vma is already bound, there is no need to
> (re)allocate a virtual address range for it.
> 
> v2: - add sanity checks and remove superfluous GLOBAL_BIND set
>     - we might do update for an unbound vma (Chris)
> 
> v3: s/u32/unsigned (Chris)

Go back to v2. Sorry, seems like u32 is being used throughout this
callchain and so needs to be fixed all together.
-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] drm/i915/gtt: Allocate va range only if vma is not bound

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6279
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 BYT  igt@drm_vma_limiter_cached      FAIL(3)PASS(3)      FAIL(2)
*BYT  igt@gem_exec_params@rsvd2-dirt      FAIL(1)PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(2)PASS(3)      TIMEOUT(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[Jani Nikula]

On Wed, 29 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Apr 29, 2015 at 09:49:30AM +0300, Mika Kuoppala wrote:
>> When we have bound vma into an address space, the layout
>> of page table structures is immutable. So we can be absolutely
>> certain that if vma is already bound, there is no need to
>> (re)allocate a virtual address range for it.
>> 
>> v2: - add sanity checks and remove superfluous GLOBAL_BIND set
>>     - we might do update for an unbound vma (Chris)
>> 
>> v3: s/u32/unsigned (Chris)
>
> Go back to v2. Sorry, seems like u32 is being used throughout this
> callchain and so needs to be fixed all together.

v2 pushed to drm-intel-next-queued, with

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90224

Thanks for the patch and review.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Allocate va range only if vma is not bound

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6285
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -4              227/227              223/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 BYT  igt@gem_double_irq_loop      DMESG_WARN(1)PASS(3)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(9)      TIMEOUT(1)PASS(1)
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(4)PASS(6)      TIMEOUT(1)PASS(1)
*BYT  igt@gem_storedw_batches_loop@secure-dispatch      FAIL(1)PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx