[PATCH] drm/i915: simplify bind_to_vm init code

[PATCH] drm/i915: simplify bind_to_vm init code

[Matthew Auld]

No functional change, just makes the code easier to follow.

v2:
    - Remove local fence_size variable
(Tvrtko Ursulin)
    - Remove redundant NULL ggtt_view check
    - Reuse size variable
v3:
(Chris Wilson)
    - Keep start, end computation separate
    - Simplify alignment

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 52 +++++++++++------------------------------
 1 file changed, 13 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f45856d..ffb802e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3465,48 +3465,23 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 fence_alignment, unfenced_alignment;
+	u32 min_alignment;
 	u32 search_flag, alloc_flag;
 	u64 start, end;
-	u64 size, fence_size;
+	u64 size;
 	struct i915_vma *vma;
 	int ret;
 
-	if (i915_is_ggtt(vm)) {
-		u32 view_size;
-
-		if (WARN_ON(!ggtt_view))
-			return ERR_PTR(-EINVAL);
-
-		view_size = i915_ggtt_view_size(obj, ggtt_view);
-
-		fence_size = i915_gem_get_gtt_size(dev,
-						   view_size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     view_size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
-								view_size,
-								obj->tiling_mode,
-								false);
-		size = flags & PIN_MAPPABLE ? fence_size : view_size;
-	} else {
-		fence_size = i915_gem_get_gtt_size(dev,
-						   obj->base.size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     obj->base.size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment =
-			i915_gem_get_gtt_alignment(dev,
-						   obj->base.size,
+	if (i915_is_ggtt(vm))
+		size = i915_ggtt_view_size(obj, ggtt_view);
+	else
+		size = obj->base.size;
+
+	min_alignment = i915_gem_get_gtt_alignment(dev, size,
 						   obj->tiling_mode,
-						   false);
-		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
-	}
+						   flags & PIN_MAPPABLE);
+	if (flags & PIN_MAPPABLE)
+		size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);
 
 	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
 	end = vm->total;
@@ -3516,9 +3491,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 		end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
 
 	if (alignment == 0)
-		alignment = flags & PIN_MAPPABLE ? fence_alignment :
-						unfenced_alignment;
-	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
+		alignment = min_alignment;
+	if (flags & PIN_MAPPABLE && alignment & (min_alignment - 1)) {
 		DRM_DEBUG("Invalid object (view type=%u) alignment requested %u\n",
 			  ggtt_view ? ggtt_view->type : 0,
 			  alignment);
-- 
2.4.3

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

✗ Fi.CI.BAT: failure for drm/i915: simplify bind_to_vm init code (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: simplify bind_to_vm init code (rev3)
URL   : https://patchwork.freedesktop.org/series/4531/
State : failure

== Summary ==

Series 4531v3 drm/i915: simplify bind_to_vm init code
http://patchwork.freedesktop.org/api/1.0/series/4531/revisions/3/mbox/

Test gem_ringfill:
        Subgroup basic-default-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (snb-x220t)
                fail       -> PASS       (byt-nuc)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (hsw-brixbox)
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> DMESG-FAIL (snb-x220t)
                dmesg-warn -> PASS       (snb-dellxps)
                pass       -> DMESG-WARN (byt-nuc)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (snb-dellxps)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:194  pass:157  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:194  pass:158  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:194  pass:170  dwarn:2   dfail:0   fail:0   skip:22 
hsw-gt2          total:194  pass:175  dwarn:2   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:194  pass:130  dwarn:1   dfail:0   fail:0   skip:63 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34 
snb-x220t        total:194  pass:159  dwarn:0   dfail:1   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1645/

e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
e812ad29ac14754f16e94d4c470fa416af4fda4d drm/i915: simplify bind_to_vm init code

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Matthew Auld]

Hi Chris,

Are the changes acceptable?

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

[PATCH] drm/i915: simplify bind_to_vm init code

[Matthew Auld]

No functional change, just makes the code easier to follow.

v2:
    - Remove local fence_size variable
(Tvrtko Ursulin)
    - Remove redundant NULL ggtt_view check
    - Reuse size variable

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f45856d..e5d9d0b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	u32 fence_alignment, unfenced_alignment;
 	u32 search_flag, alloc_flag;
 	u64 start, end;
-	u64 size, fence_size;
+	u64 size;
 	struct i915_vma *vma;
 	int ret;
 
-	if (i915_is_ggtt(vm)) {
-		u32 view_size;
-
-		if (WARN_ON(!ggtt_view))
-			return ERR_PTR(-EINVAL);
-
-		view_size = i915_ggtt_view_size(obj, ggtt_view);
-
-		fence_size = i915_gem_get_gtt_size(dev,
-						   view_size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     view_size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
-								view_size,
-								obj->tiling_mode,
-								false);
-		size = flags & PIN_MAPPABLE ? fence_size : view_size;
-	} else {
-		fence_size = i915_gem_get_gtt_size(dev,
-						   obj->base.size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     obj->base.size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment =
-			i915_gem_get_gtt_alignment(dev,
-						   obj->base.size,
-						   obj->tiling_mode,
-						   false);
-		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
-	}
+	if (i915_is_ggtt(vm))
+		size = i915_ggtt_view_size(obj, ggtt_view);
+	else
+		size = obj->base.size;
+
+	fence_alignment = i915_gem_get_gtt_alignment(dev, size,
+						     obj->tiling_mode, true);
+	unfenced_alignment = i915_gem_get_gtt_alignment(dev, size,
+							obj->tiling_mode,
+							false);
 
 	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
 	end = vm->total;
-	if (flags & PIN_MAPPABLE)
+	if (flags & PIN_MAPPABLE) {
 		end = min_t(u64, end, dev_priv->gtt.mappable_end);
+		size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);
+	}
 	if (flags & PIN_ZONE_4G)
 		end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
 
-- 
2.4.3

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Tvrtko Ursulin]

On 18/03/16 10:46, Matthew Auld wrote:
> No functional change, just makes the code easier to follow.
>
> v2:
>      - Remove local fence_size variable
> (Tvrtko Ursulin)
>      - Remove redundant NULL ggtt_view check
>      - Reuse size variable
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------
>   1 file changed, 14 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f45856d..e5d9d0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   	u32 fence_alignment, unfenced_alignment;
>   	u32 search_flag, alloc_flag;
>   	u64 start, end;
> -	u64 size, fence_size;
> +	u64 size;
>   	struct i915_vma *vma;
>   	int ret;
>
> -	if (i915_is_ggtt(vm)) {
> -		u32 view_size;
> -
> -		if (WARN_ON(!ggtt_view))
> -			return ERR_PTR(-EINVAL);
> -
> -		view_size = i915_ggtt_view_size(obj, ggtt_view);
> -
> -		fence_size = i915_gem_get_gtt_size(dev,
> -						   view_size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(dev,
> -							     view_size,
> -							     obj->tiling_mode,
> -							     true);
> -		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> -								view_size,
> -								obj->tiling_mode,
> -								false);
> -		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> -	} else {
> -		fence_size = i915_gem_get_gtt_size(dev,
> -						   obj->base.size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(dev,
> -							     obj->base.size,
> -							     obj->tiling_mode,
> -							     true);
> -		unfenced_alignment =
> -			i915_gem_get_gtt_alignment(dev,
> -						   obj->base.size,
> -						   obj->tiling_mode,
> -						   false);
> -		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> -	}
> +	if (i915_is_ggtt(vm))
> +		size = i915_ggtt_view_size(obj, ggtt_view);
> +	else
> +		size = obj->base.size;
> +
> +	fence_alignment = i915_gem_get_gtt_alignment(dev, size,
> +						     obj->tiling_mode, true);
> +	unfenced_alignment = i915_gem_get_gtt_alignment(dev, size,
> +							obj->tiling_mode,
> +							false);
>
>   	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>   	end = vm->total;
> -	if (flags & PIN_MAPPABLE)
> +	if (flags & PIN_MAPPABLE) {
>   		end = min_t(u64, end, dev_priv->gtt.mappable_end);
> +		size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);
> +	}
>   	if (flags & PIN_ZONE_4G)
>   		end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
>
>

Looks good to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Chris Wilson]

On Fri, Mar 18, 2016 at 11:10:50AM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 18/03/16 10:46, Matthew Auld wrote:
> >No functional change, just makes the code easier to follow.
> >
> >v2:
> >     - Remove local fence_size variable
> >(Tvrtko Ursulin)
> >     - Remove redundant NULL ggtt_view check
> >     - Reuse size variable
> >
> >Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------
> >  1 file changed, 14 insertions(+), 37 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index f45856d..e5d9d0b 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  	u32 fence_alignment, unfenced_alignment;
> >  	u32 search_flag, alloc_flag;
> >  	u64 start, end;
> >-	u64 size, fence_size;
> >+	u64 size;
> >  	struct i915_vma *vma;
> >  	int ret;
> >
> >-	if (i915_is_ggtt(vm)) {
> >-		u32 view_size;
> >-
> >-		if (WARN_ON(!ggtt_view))
> >-			return ERR_PTR(-EINVAL);
> >-
> >-		view_size = i915_ggtt_view_size(obj, ggtt_view);
> >-
> >-		fence_size = i915_gem_get_gtt_size(dev,
> >-						   view_size,
> >-						   obj->tiling_mode);
> >-		fence_alignment = i915_gem_get_gtt_alignment(dev,
> >-							     view_size,
> >-							     obj->tiling_mode,
> >-							     true);
> >-		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> >-								view_size,
> >-								obj->tiling_mode,
> >-								false);
> >-		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> >-	} else {
> >-		fence_size = i915_gem_get_gtt_size(dev,
> >-						   obj->base.size,
> >-						   obj->tiling_mode);
> >-		fence_alignment = i915_gem_get_gtt_alignment(dev,
> >-							     obj->base.size,
> >-							     obj->tiling_mode,
> >-							     true);
> >-		unfenced_alignment =
> >-			i915_gem_get_gtt_alignment(dev,
> >-						   obj->base.size,
> >-						   obj->tiling_mode,
> >-						   false);
> >-		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> >-	}
> >+	if (i915_is_ggtt(vm))
> >+		size = i915_ggtt_view_size(obj, ggtt_view);
> >+	else
> >+		size = obj->base.size;
> >+
> >+	fence_alignment = i915_gem_get_gtt_alignment(dev, size,
> >+						     obj->tiling_mode, true);
> >+	unfenced_alignment = i915_gem_get_gtt_alignment(dev, size,
> >+							obj->tiling_mode,
> >+							false);
> >
> >  	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> >  	end = vm->total;
> >-	if (flags & PIN_MAPPABLE)
> >+	if (flags & PIN_MAPPABLE) {
> >  		end = min_t(u64, end, dev_priv->gtt.mappable_end);
> >+		size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);

No. Keep the start, end computation separate.

For example the above size needs only be done when inspecting the
i915_is_ggtt().

If you simplified the alignement as well, it becomes clearer. If you
reviewed the patches to handle vma, it would help. The key question is
what igt did you run to verify the changes?
-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

[PATCH] drm/i915: simplify bind_to_vm init code

[Matthew Auld]

No functional change, just makes the code easier to follow.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b854af2..5a8d69d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3470,46 +3470,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	u32 fence_alignment, unfenced_alignment;
 	u32 search_flag, alloc_flag;
 	u64 start, end;
-	u64 size, fence_size;
+	u64 size, obj_size, fence_size;
 	struct i915_vma *vma;
 	int ret;
 
 	if (i915_is_ggtt(vm)) {
-		u32 view_size;
-
 		if (WARN_ON(!ggtt_view))
 			return ERR_PTR(-EINVAL);
 
-		view_size = i915_ggtt_view_size(obj, ggtt_view);
-
-		fence_size = i915_gem_get_gtt_size(dev,
-						   view_size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     view_size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
-								view_size,
-								obj->tiling_mode,
-								false);
-		size = flags & PIN_MAPPABLE ? fence_size : view_size;
+		obj_size = i915_ggtt_view_size(obj, ggtt_view);
 	} else {
-		fence_size = i915_gem_get_gtt_size(dev,
-						   obj->base.size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(dev,
-							     obj->base.size,
-							     obj->tiling_mode,
-							     true);
-		unfenced_alignment =
-			i915_gem_get_gtt_alignment(dev,
-						   obj->base.size,
-						   obj->tiling_mode,
-						   false);
-		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
+		obj_size = obj->base.size;
 	}
 
+	fence_size = i915_gem_get_gtt_size(dev, obj_size, obj->tiling_mode);
+	fence_alignment = i915_gem_get_gtt_alignment(dev, obj_size,
+						     obj->tiling_mode, true);
+	unfenced_alignment = i915_gem_get_gtt_alignment(dev, obj_size,
+							obj->tiling_mode,
+							false);
+	size = flags & PIN_MAPPABLE ? fence_size : obj_size;
+
 	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
 	end = vm->total;
 	if (flags & PIN_MAPPABLE)
-- 
2.4.3

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Tvrtko Ursulin]

Hi,

On 16/03/16 17:07, Matthew Auld wrote:
> No functional change, just makes the code easier to follow.
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 41 +++++++++++------------------------------
>   1 file changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b854af2..5a8d69d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3470,46 +3470,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   	u32 fence_alignment, unfenced_alignment;
>   	u32 search_flag, alloc_flag;
>   	u64 start, end;
> -	u64 size, fence_size;
> +	u64 size, obj_size, fence_size;
>   	struct i915_vma *vma;
>   	int ret;
>
>   	if (i915_is_ggtt(vm)) {
> -		u32 view_size;
> -
>   		if (WARN_ON(!ggtt_view))
>   			return ERR_PTR(-EINVAL);
>
> -		view_size = i915_ggtt_view_size(obj, ggtt_view);
> -
> -		fence_size = i915_gem_get_gtt_size(dev,
> -						   view_size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(dev,
> -							     view_size,
> -							     obj->tiling_mode,
> -							     true);
> -		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> -								view_size,
> -								obj->tiling_mode,
> -								false);
> -		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> +		obj_size = i915_ggtt_view_size(obj, ggtt_view);
>   	} else {
> -		fence_size = i915_gem_get_gtt_size(dev,
> -						   obj->base.size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(dev,
> -							     obj->base.size,
> -							     obj->tiling_mode,
> -							     true);
> -		unfenced_alignment =
> -			i915_gem_get_gtt_alignment(dev,
> -						   obj->base.size,
> -						   obj->tiling_mode,
> -						   false);
> -		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> +		obj_size = obj->base.size;
>   	}

PIN_MAPPABLE mandates PIN_GLOBAL, and PIN_GLOBAL mandates GGTT (see 
i915_gem_object_do_pin), so I think the cleanup should be just not to do 
any of the fence business on if !ggtt branch.

if (i915_is_ggtt(vm)) {
	... existing code ...
} else {
	size = obj->base.size;
}

Check for NULL ggtt_view is also done on the higher level so perhaps 
that can go as well to consolidate the checks at one place.

> +	fence_size = i915_gem_get_gtt_size(dev, obj_size, obj->tiling_mode);
> +	fence_alignment = i915_gem_get_gtt_alignment(dev, obj_size,
> +						     obj->tiling_mode, true);
> +	unfenced_alignment = i915_gem_get_gtt_alignment(dev, obj_size,
> +							obj->tiling_mode,
> +							false);
> +	size = flags & PIN_MAPPABLE ? fence_size : obj_size;
> +
>   	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>   	end = vm->total;
>   	if (flags & PIN_MAPPABLE)
>

Regards,

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Matthew Auld]

Hi,

If we don't do any of the fence business for !i915_is_gtt, then will
this not change the following code:

if (alignment == 0)
    alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment;

Or am I missing something?

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

Re: [PATCH] drm/i915: simplify bind_to_vm init code

[Chris Wilson]

On Thu, Mar 17, 2016 at 01:41:32PM +0000, Matthew Auld wrote:
> Hi,
> 
> If we don't do any of the fence business for !i915_is_gtt, then will
> this not change the following code:
> 
> if (alignment == 0)
>     alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment;
> 
> Or am I missing something?

Fwiw, the patches I have sent previously change it to:

size = max(size, vma->size);
if (flags & PIN_MAPPABLE)
	size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);

alignment =
	max_t(u64, max(alignment, vma->display_alignment),
      	      i915_gem_get_gtt_alignment(dev, size, obj->tiling_mode, flags & PIN_MAPPABLE));
if (alignment == 4096)
	alignment = 0;

start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;

end = vma->vm->total;
if (flags & PIN_MAPPABLE)
	end = min_t(u64, end, dev_priv->gtt.mappable_end);
if (flags & PIN_ZONE_4G)
	end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
-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] drm/i915: simplify bind_to_vm init code

[Tvrtko Ursulin]

On 17/03/16 13:41, Matthew Auld wrote:
> Hi,
>
> If we don't do any of the fence business for !i915_is_gtt, then will
> this not change the following code:
>
> if (alignment == 0)
>      alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment;
>
> Or am I missing something?

No I missed that bit. In which case your cleanup looks OK to me in 
principle. I would just not use obj_size since it is not that. Perhaps 
just reuse the existing size variable to hold the intermediate result?

Regards,

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