All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: simplify bind_to_vm init code
@ 2016-03-16 17:07 Matthew Auld
  2016-03-17 10:00 ` Tvrtko Ursulin
  2016-03-17 10:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
  0 siblings, 2 replies; 11+ messages in thread
From: Matthew Auld @ 2016-03-16 17:07 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-16 17:07 [PATCH] drm/i915: simplify bind_to_vm init code Matthew Auld
@ 2016-03-17 10:00 ` Tvrtko Ursulin
  2016-03-17 13:41   ` Matthew Auld
  2016-03-17 10:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
  1 sibling, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2016-03-17 10:00 UTC (permalink / raw)
  To: Matthew Auld, intel-gfx


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915: simplify bind_to_vm init code
  2016-03-16 17:07 [PATCH] drm/i915: simplify bind_to_vm init code Matthew Auld
  2016-03-17 10:00 ` Tvrtko Ursulin
@ 2016-03-17 10:36 ` Patchwork
  1 sibling, 0 replies; 11+ messages in thread
From: Patchwork @ 2016-03-17 10:36 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (hsw-gt2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (hsw-gt2)
                dmesg-warn -> PASS       (bdw-ultra)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (hsw-brixbox)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (hsw-brixbox)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                dmesg-warn -> PASS       (hsw-gt2)
                pass       -> DMESG-WARN (hsw-brixbox)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (hsw-brixbox)

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:155  dwarn:2   dfail:0   fail:0   skip:37 
byt-nuc          total:194  pass:155  dwarn:4   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:176  dwarn:1   dfail:0   fail:0   skip:17 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_1623/

8a9a911d489fe160df173580277983dac5952ed0 drm-intel-nightly: 2016y-03m-17d-10h-02m-10s UTC integration manifest
11854942755899b97e78942213f304b118a3a4ef 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-17 10:00 ` Tvrtko Ursulin
@ 2016-03-17 13:41   ` Matthew Auld
  2016-03-17 13:53     ` Chris Wilson
  2016-03-17 17:36     ` Tvrtko Ursulin
  0 siblings, 2 replies; 11+ messages in thread
From: Matthew Auld @ 2016-03-17 13:41 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-17 13:41   ` Matthew Auld
@ 2016-03-17 13:53     ` Chris Wilson
  2016-03-17 17:36     ` Tvrtko Ursulin
  1 sibling, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2016-03-17 13:53 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-17 13:41   ` Matthew Auld
  2016-03-17 13:53     ` Chris Wilson
@ 2016-03-17 17:36     ` Tvrtko Ursulin
  1 sibling, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2016-03-17 17:36 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] drm/i915: simplify bind_to_vm init code
@ 2016-03-18 10:46 Matthew Auld
  2016-03-18 11:10 ` Tvrtko Ursulin
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Auld @ 2016-03-18 10:46 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-18 10:46 [PATCH] " Matthew Auld
@ 2016-03-18 11:10 ` Tvrtko Ursulin
  2016-03-18 11:26   ` Chris Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2016-03-18 11:10 UTC (permalink / raw)
  To: Matthew Auld, intel-gfx



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-18 11:10 ` Tvrtko Ursulin
@ 2016-03-18 11:26   ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2016-03-18 11:26 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, Matthew Auld

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] drm/i915: simplify bind_to_vm init code
@ 2016-03-18 13:54 Matthew Auld
  2016-03-22 10:36 ` Matthew Auld
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Auld @ 2016-03-18 13:54 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: simplify bind_to_vm init code
  2016-03-18 13:54 Matthew Auld
@ 2016-03-22 10:36 ` Matthew Auld
  0 siblings, 0 replies; 11+ messages in thread
From: Matthew Auld @ 2016-03-22 10:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-03-22 10:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-16 17:07 [PATCH] drm/i915: simplify bind_to_vm init code Matthew Auld
2016-03-17 10:00 ` Tvrtko Ursulin
2016-03-17 13:41   ` Matthew Auld
2016-03-17 13:53     ` Chris Wilson
2016-03-17 17:36     ` Tvrtko Ursulin
2016-03-17 10:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2016-03-18 10:46 [PATCH] " Matthew Auld
2016-03-18 11:10 ` Tvrtko Ursulin
2016-03-18 11:26   ` Chris Wilson
2016-03-18 13:54 Matthew Auld
2016-03-22 10:36 ` Matthew Auld

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.