[RFC] drm/i915: Simplify page allocation loop a bit

[RFC] drm/i915: Simplify page allocation loop a bit

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Looks like there are some redundant lines in the main loop of
i915_gem_object_get_pages_gtt.

I haven't tested this so just RFC please.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 00c8361..5af3537 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1912,15 +1912,8 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
 			gfp &= ~(__GFP_IO | __GFP_WAIT);
 		}
-#ifdef CONFIG_SWIOTLB
-		if (swiotlb_nr_tbl()) {
-			st->nents++;
-			sg_set_page(sg, page, PAGE_SIZE, 0);
-			sg = sg_next(sg);
-			continue;
-		}
-#endif
-		if (!i || page_to_pfn(page) != last_pfn + 1) {
+		if (!i || page_to_pfn(page) != last_pfn + 1
+		       || swiotlb_nr_tbl()) {
 			if (i)
 				sg = sg_next(sg);
 			st->nents++;
@@ -1933,10 +1926,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
 	}
-#ifdef CONFIG_SWIOTLB
-	if (!swiotlb_nr_tbl())
-#endif
-		sg_mark_end(sg);
+	sg_mark_end(sg);
 	obj->pages = st;
 
 	if (i915_gem_object_needs_bit17_swizzle(obj))
-- 
1.9.0

Re: [RFC] drm/i915: Simplify page allocation loop a bit

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1743 bytes --]

On Wed, 2014-03-26 at 16:48 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Looks like there are some redundant lines in the main loop of
> i915_gem_object_get_pages_gtt.
> 
> I haven't tested this so just RFC please.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>

Looks good to me. Fwiw:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 00c8361..5af3537 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1912,15 +1912,8 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>  			gfp &= ~(__GFP_IO | __GFP_WAIT);
>  		}
> -#ifdef CONFIG_SWIOTLB
> -		if (swiotlb_nr_tbl()) {
> -			st->nents++;
> -			sg_set_page(sg, page, PAGE_SIZE, 0);
> -			sg = sg_next(sg);
> -			continue;
> -		}
> -#endif
> -		if (!i || page_to_pfn(page) != last_pfn + 1) {
> +		if (!i || page_to_pfn(page) != last_pfn + 1
> +		       || swiotlb_nr_tbl()) {
>  			if (i)
>  				sg = sg_next(sg);
>  			st->nents++;
> @@ -1933,10 +1926,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  		/* Check that the i965g/gm workaround works. */
>  		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>  	}
> -#ifdef CONFIG_SWIOTLB
> -	if (!swiotlb_nr_tbl())
> -#endif
> -		sg_mark_end(sg);
> +	sg_mark_end(sg);
>  	obj->pages = st;
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC] drm/i915: Simplify page allocation loop a bit

[Chris Wilson]

On Wed, Mar 26, 2014 at 04:48:47PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Looks like there are some redundant lines in the main loop of
> i915_gem_object_get_pages_gtt.
> 
> I haven't tested this so just RFC please.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>

I am pretty sure we can't drop CONFIG_SWIOTLB as swiotlb_nr_tbl only
exists when lib/swiotlb.c is built i.e. #if IS_ENABLED(CONFIG_SWIOTLB).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC] drm/i915: Simplify page allocation loop a bit

[Tvrtko Ursulin]

On 03/26/2014 05:11 PM, Chris Wilson wrote:
> On Wed, Mar 26, 2014 at 04:48:47PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Looks like there are some redundant lines in the main loop of
>> i915_gem_object_get_pages_gtt.
>>
>> I haven't tested this so just RFC please.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>
> I am pretty sure we can't drop CONFIG_SWIOTLB as swiotlb_nr_tbl only
> exists when lib/swiotlb.c is built i.e. #if IS_ENABLED(CONFIG_SWIOTLB).

I think you're right. I'll repost. Although it diminishes the 
attractiveness of the whole line reducing enterprise a bit.

Thanks,

Tvrtko

[RFC] drm/i915: Simplify page allocation loop a bit

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Looks like there are some redundant lines in the main loop of
i915_gem_object_get_pages_gtt.

v2: Put CONFIG_SWIOTLB back in.

I haven't tested this so just RFC please. Not sure that
seven lines net less is worth this. It just made it clerer
to a newby to sg building to think of it in terms of
"merge entries always except when...".

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 00c8361..b435db1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1912,15 +1912,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
 			gfp &= ~(__GFP_IO | __GFP_WAIT);
 		}
+		if (!i || page_to_pfn(page) != last_pfn + 1
 #ifdef CONFIG_SWIOTLB
-		if (swiotlb_nr_tbl()) {
-			st->nents++;
-			sg_set_page(sg, page, PAGE_SIZE, 0);
-			sg = sg_next(sg);
-			continue;
-		}
+		       || swiotlb_nr_tbl()
 #endif
-		if (!i || page_to_pfn(page) != last_pfn + 1) {
+		  ) {
 			if (i)
 				sg = sg_next(sg);
 			st->nents++;
@@ -1933,10 +1929,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
 	}
-#ifdef CONFIG_SWIOTLB
-	if (!swiotlb_nr_tbl())
-#endif
-		sg_mark_end(sg);
+	sg_mark_end(sg);
 	obj->pages = st;
 
 	if (i915_gem_object_needs_bit17_swizzle(obj))
-- 
1.9.0

Re: [RFC] drm/i915: Simplify page allocation loop a bit

[Tvrtko Ursulin]

On 03/27/2014 10:18 AM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Looks like there are some redundant lines in the main loop of
> i915_gem_object_get_pages_gtt.
>
> v2: Put CONFIG_SWIOTLB back in.
>
> I haven't tested this so just RFC please. Not sure that
> seven lines net less is worth this. It just made it clerer
> to a newby to sg building to think of it in terms of
> "merge entries always except when...".

For what it is worth, this builds and doesn't blow up on some smoke tests.

Tvrtko