[CI] drm/i915: Skip waking the device to service pwrite

[CI] drm/i915: Skip waking the device to service pwrite

[Chris Wilson]

If the device is in runtime suspend, resuming takes time and reduces our
powersaving. If this was for a small write into an object, that resume
will take longer than any savings in using the indirect GGTT access to
avoid the cpu cache.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d699ea3ab80b..026cb52ece0b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1240,7 +1240,23 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 	if (ret)
 		return ret;
 
-	intel_runtime_pm_get(i915);
+	if (i915_gem_object_has_struct_page(obj)) {
+		/*
+		 * Avoid waking the device up if we can fallback, as
+		 * waking/resuming is very slow (worst-case 10-100 ms
+		 * depending on PCI sleeps and our own resume time).
+		 * This easily dwarfs any performance advantage from
+		 * using the cache bypass of indirect GGTT access.
+		 */
+		if (!intel_runtime_pm_get_if_in_use(i915)) {
+			ret = -EFAULT;
+			goto out_unlock;
+		}
+	} else {
+		/* No backing pages, no fallback, we must force GGTT access */
+		intel_runtime_pm_get(i915);
+	}
+
 	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
 				       PIN_MAPPABLE |
 				       PIN_NONFAULT |
@@ -1257,7 +1273,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 	if (IS_ERR(vma)) {
 		ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
 		if (ret)
-			goto out_unlock;
+			goto out_rpm;
 		GEM_BUG_ON(!node.allocated);
 	}
 
@@ -1320,8 +1336,9 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 	} else {
 		i915_vma_unpin(vma);
 	}
-out_unlock:
+out_rpm:
 	intel_runtime_pm_put(i915);
+out_unlock:
 	mutex_unlock(&i915->drm.struct_mutex);
 	return ret;
 }
-- 
2.15.0.rc1

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

✓ Fi.CI.BAT: success for drm/i915: Skip waking the device to service pwrite (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Skip waking the device to service pwrite (rev2)
URL   : https://patchwork.freedesktop.org/series/29560/
State : success

== Summary ==

Series 29560v2 drm/i915: Skip waking the device to service pwrite
https://patchwork.freedesktop.org/api/1.0/series/29560/revisions/2/mbox/

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:459s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:376s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:526s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:264s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:501s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:497s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:481s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:424s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:255s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:576s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:448s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:436s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:491s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:492s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:572s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:542s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:455s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:648s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:527s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:566s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:425s

399dd92e2b42475c3d89e7a2f16b8305e105ed11 drm-tip: 2017y-10m-18d-21h-54m-46s UTC integration manifest
793d6adac336 drm/i915: Skip waking the device to service pwrite

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6098/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI] drm/i915: Skip waking the device to service pwrite

[Daniel Vetter]

On Thu, Oct 19, 2017 at 07:37:33AM +0100, Chris Wilson wrote:
> If the device is in runtime suspend, resuming takes time and reduces our
> powersaving. If this was for a small write into an object, that resume
> will take longer than any savings in using the indirect GGTT access to
> avoid the cpu cache.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Still sounds like a reasonable idea.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d699ea3ab80b..026cb52ece0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1240,7 +1240,23 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>  	if (ret)
>  		return ret;
>  
> -	intel_runtime_pm_get(i915);
> +	if (i915_gem_object_has_struct_page(obj)) {
> +		/*
> +		 * Avoid waking the device up if we can fallback, as
> +		 * waking/resuming is very slow (worst-case 10-100 ms
> +		 * depending on PCI sleeps and our own resume time).
> +		 * This easily dwarfs any performance advantage from
> +		 * using the cache bypass of indirect GGTT access.
> +		 */
> +		if (!intel_runtime_pm_get_if_in_use(i915)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +	} else {
> +		/* No backing pages, no fallback, we must force GGTT access */
> +		intel_runtime_pm_get(i915);
> +	}
> +
>  	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
>  				       PIN_MAPPABLE |
>  				       PIN_NONFAULT |
> @@ -1257,7 +1273,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>  	if (IS_ERR(vma)) {
>  		ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
>  		if (ret)
> -			goto out_unlock;
> +			goto out_rpm;
>  		GEM_BUG_ON(!node.allocated);
>  	}
>  
> @@ -1320,8 +1336,9 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>  	} else {
>  		i915_vma_unpin(vma);
>  	}
> -out_unlock:
> +out_rpm:
>  	intel_runtime_pm_put(i915);
> +out_unlock:
>  	mutex_unlock(&i915->drm.struct_mutex);
>  	return ret;
>  }
> -- 
> 2.15.0.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Skip waking the device to service pwrite (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Skip waking the device to service pwrite (rev2)
URL   : https://patchwork.freedesktop.org/series/29560/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
        Subgroup oa-exponents:
                fail       -> PASS       (shard-hsw) fdo#102254
Test kms_cursor_legacy:
        Subgroup cursorA-vs-flipA-legacy:
                skip       -> PASS       (shard-hsw)
Test pm_rpm:
        Subgroup modeset-non-lpsp-stress-no-wait:
                skip       -> PASS       (shard-hsw)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-B:
                skip       -> PASS       (shard-hsw)
Test kms_flip:
        Subgroup wf_vblank-vs-modeset:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614
Test prime_self_import:
        Subgroup export-vs-gem_close-race:
                pass       -> FAIL       (shard-hsw) fdo#102655

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102655 https://bugs.freedesktop.org/show_bug.cgi?id=102655

shard-hsw        total:2540 pass:1427 dwarn:0   dfail:0   fail:12  skip:1101 time:9195s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6098/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI] drm/i915: Skip waking the device to service pwrite

[Tvrtko Ursulin]

On 19/10/2017 07:37, Chris Wilson wrote:
> If the device is in runtime suspend, resuming takes time and reduces our
> powersaving. If this was for a small write into an object, that resume
> will take longer than any savings in using the indirect GGTT access to
> avoid the cpu cache.

Commit talks about small writes but the patch takes no notice to size of 
requested writes. Is that intended?

Regards,

Tvrtko

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d699ea3ab80b..026cb52ece0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1240,7 +1240,23 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>   	if (ret)
>   		return ret;
>   
> -	intel_runtime_pm_get(i915);
> +	if (i915_gem_object_has_struct_page(obj)) {
> +		/*
> +		 * Avoid waking the device up if we can fallback, as
> +		 * waking/resuming is very slow (worst-case 10-100 ms
> +		 * depending on PCI sleeps and our own resume time).
> +		 * This easily dwarfs any performance advantage from
> +		 * using the cache bypass of indirect GGTT access.
> +		 */
> +		if (!intel_runtime_pm_get_if_in_use(i915)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +	} else {
> +		/* No backing pages, no fallback, we must force GGTT access */
> +		intel_runtime_pm_get(i915);
> +	}
> +
>   	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
>   				       PIN_MAPPABLE |
>   				       PIN_NONFAULT |
> @@ -1257,7 +1273,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>   	if (IS_ERR(vma)) {
>   		ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
>   		if (ret)
> -			goto out_unlock;
> +			goto out_rpm;
>   		GEM_BUG_ON(!node.allocated);
>   	}
>   
> @@ -1320,8 +1336,9 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>   	} else {
>   		i915_vma_unpin(vma);
>   	}
> -out_unlock:
> +out_rpm:
>   	intel_runtime_pm_put(i915);
> +out_unlock:
>   	mutex_unlock(&i915->drm.struct_mutex);
>   	return ret;
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI] drm/i915: Skip waking the device to service pwrite

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-19 11:02:05)
> 
> On 19/10/2017 07:37, Chris Wilson wrote:
> > If the device is in runtime suspend, resuming takes time and reduces our
> > powersaving. If this was for a small write into an object, that resume
> > will take longer than any savings in using the indirect GGTT access to
> > avoid the cpu cache.
> 
> Commit talks about small writes but the patch takes no notice to size of 
> requested writes. Is that intended?

We are talking gigabytes before the difference in paths outweigh the
worstcase costs, even before we start measuring power. pwrite is a tricky
one, for anything big and tiled or for anything where you may repeat the
read/write/mmap, you don't want to use pwrite but the direct mmap. pwrite
should only wins for small one-off uses, so that's what I have in mind
as the typical user -- and I encourage them to re-evaluate their use.

So why bother? Because it made a big difference to igt pwrite runtime
when we didn't have a display connector. Is the addition in complexity
worth it for the few real users? Should we be more aware of the wider
implications (both power and performance) of certain operations within
the driver? Do we need more mostly idle and/or headless testing?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI] drm/i915: Skip waking the device to service pwrite

[Tvrtko Ursulin]

On 19/10/2017 11:19, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-10-19 11:02:05)
>>
>> On 19/10/2017 07:37, Chris Wilson wrote:
>>> If the device is in runtime suspend, resuming takes time and reduces our
>>> powersaving. If this was for a small write into an object, that resume
>>> will take longer than any savings in using the indirect GGTT access to
>>> avoid the cpu cache.
>>
>> Commit talks about small writes but the patch takes no notice to size of
>> requested writes. Is that intended?
> 
> We are talking gigabytes before the difference in paths outweigh the
> worstcase costs, even before we start measuring power. pwrite is a tricky

No real complaints, just read the commit and expected to see something 
about the size in the patch itself.

> one, for anything big and tiled or for anything where you may repeat the
> read/write/mmap, you don't want to use pwrite but the direct mmap. pwrite
> should only wins for small one-off uses, so that's what I have in mind
> as the typical user -- and I encourage them to re-evaluate their use.
> 
> So why bother? Because it made a big difference to igt pwrite runtime
> when we didn't have a display connector. Is the addition in complexity
> worth it for the few real users? Should we be more aware of the wider
> implications (both power and performance) of certain operations within
> the driver? Do we need more mostly idle and/or headless testing?

Regards,

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