[Intel-gfx] [PATCH] RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[Intel-gfx] [PATCH] RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[Chris Wilson]

Normally when we create a new context, and a new ppGTT to go with it, we
point all the unused pages in the ppGTT to a 'safe' scratch page. Any
inadvertent access outside of the declared user's area will result in a
read/write to scratch instead. However, sometimes it is preferrable to
that to cause a fault instead. This does not trap execution of the
faulting batch, but it does record the error:

FAULT_TLB_DATA: 0x00000000 0x00000004
    Address 0x0000000000004000 PPGTT

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jason Ekstrand <jason@jlekstrand.net>
---
The name and value semantics are horrendous. The non-trapping behaviour
is also less than ideal. Worth it?
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 48 +++++++++++++++++++++
 include/uapi/drm/i915_drm.h                 |  2 +
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1c4afa864bfe..f981269e883d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1191,6 +1191,33 @@ static int set_ringsize(struct i915_gem_context *ctx,
 				 __intel_context_ring_size(args->value));
 }
 
+static int set_empty_vm(struct i915_gem_context *ctx,
+			struct drm_i915_gem_context_param *args)
+{
+	struct i915_address_space *vm = ctx->vm;
+	int i;
+
+	if (!vm || INTEL_GEN(ctx->i915) < 8)
+		return -ENODEV;
+
+	if (args->size || args->value)
+		return -EINVAL;
+
+	if (!vm->scratch[0].encode)
+		return 0;
+
+	if (vm->mm.head_node.hole_size != vm->total)
+		return -EBUSY;
+
+	free_scratch(vm);
+
+	fill_page_dma(px_base(i915_vm_to_ppgtt(vm)->pd), 0, 512);
+	for (i = 0; i <= vm->top; i++)
+		vm->scratch[i].encode = 0;
+
+	return 0;
+}
+
 static int __get_ringsize(struct intel_context *ce, void *arg)
 {
 	long sz;
@@ -1220,6 +1247,19 @@ static int get_ringsize(struct i915_gem_context *ctx,
 	return 0;
 }
 
+static int get_empty_vm(struct i915_gem_context *ctx,
+			struct drm_i915_gem_context_param *args)
+{
+	if (!ctx->vm || INTEL_GEN(ctx->i915) < 8)
+		return -ENODEV;
+
+	if (args->size)
+		return -EINVAL;
+
+	args->value = !ctx->vm->scratch[0].encode;
+	return 0;
+}
+
 int
 i915_gem_user_to_context_sseu(struct drm_i915_private *i915,
 			      const struct drm_i915_gem_context_param_sseu *user,
@@ -1896,6 +1936,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
 		ret = set_ringsize(ctx, args);
 		break;
 
+	case I915_CONTEXT_PARAM_EMPTY_VM:
+		ret = set_empty_vm(ctx, args);
+		break;
+
 	case I915_CONTEXT_PARAM_BAN_PERIOD:
 	default:
 		ret = -EINVAL;
@@ -2348,6 +2392,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 		ret = get_ringsize(ctx, args);
 		break;
 
+	case I915_CONTEXT_PARAM_EMPTY_VM:
+		ret = get_empty_vm(ctx, args);
+		break;
+
 	case I915_CONTEXT_PARAM_BAN_PERIOD:
 	default:
 		ret = -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 14b67cd6b54b..b18215a61332 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1640,6 +1640,8 @@ struct drm_i915_gem_context_param {
  * Default is 16 KiB.
  */
 #define I915_CONTEXT_PARAM_RINGSIZE	0xc
+
+#define I915_CONTEXT_PARAM_EMPTY_VM	0xd
 /* Must be kept compact -- no holes and well documented */
 
 	__u64 value;
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH] RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[Chris Wilson]

Quoting Chris Wilson (2020-04-21 17:41:30)
> Normally when we create a new context, and a new ppGTT to go with it, we
> point all the unused pages in the ppGTT to a 'safe' scratch page. Any
> inadvertent access outside of the declared user's area will result in a
> read/write to scratch instead. However, sometimes it is preferrable to
> that to cause a fault instead. This does not trap execution of the
> faulting batch, but it does record the error:
> 
> FAULT_TLB_DATA: 0x00000000 0x00000004
>     Address 0x0000000000004000 PPGTT
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> ---
> The name and value semantics are horrendous. The non-trapping behaviour
> is also less than ideal. Worth it?

Note that we can ask for an interrupt on a page access error, however it
is a 'validation' error and we get an interrupt for every single access
and NOOP fixup (the single bit for all classes of validation errors).
They were quite frequent.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[Patchwork]

== Series Details ==

Series: RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM
URL   : https://patchwork.freedesktop.org/series/76276/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8345 -> Patchwork_17409
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_17409 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_17409, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_17409:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-cml-s:           [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-cml-s/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-cml-s/igt@i915_selftest@live@execlists.html

  
Known issues
------------

  Here are the changes found in Patchwork_17409 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@ring_submission:
    - fi-bwr-2160:        [PASS][3] -> [INCOMPLETE][4] ([i915#489])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-bwr-2160/igt@i915_selftest@live@ring_submission.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-bwr-2160/igt@i915_selftest@live@ring_submission.html

  * igt@i915_selftest@live@workarounds:
    - fi-snb-2600:        [PASS][5] -> [FAIL][6] ([i915#1763])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-snb-2600/igt@i915_selftest@live@workarounds.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-snb-2600/igt@i915_selftest@live@workarounds.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-kbl-soraka:      [DMESG-FAIL][7] ([i915#541]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html

  
#### Warnings ####

  * igt@i915_pm_rpm@basic-rte:
    - fi-kbl-guc:         [FAIL][9] ([i915#579]) -> [SKIP][10] ([fdo#109271])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-x1275:       [FAIL][11] ([i915#62]) -> [SKIP][12] ([fdo#109271])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8345/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17409/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1763]: https://gitlab.freedesktop.org/drm/intel/issues/1763
  [i915#489]: https://gitlab.freedesktop.org/drm/intel/issues/489
  [i915#541]: https://gitlab.freedesktop.org/drm/intel/issues/541
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62


Participating hosts (50 -> 43)
------------------------------

  Missing    (7): fi-cml-u2 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8345 -> Patchwork_17409

  CI-20190529: 20190529
  CI_DRM_8345: e1fa8774e58e663bec8257f678c2f8fd17088292 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5602: a8fcccd15dcc2dd409edd23785a2d6f6e85fb682 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17409: 49e6f5b75a2f7dec45c3aa15c5b2ca489ab3b693 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

49e6f5b75a2f RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

== Logs ==

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

Re: [Intel-gfx] [PATCH] RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[Tvrtko Ursulin]

On 21/04/2020 17:41, Chris Wilson wrote:
> Normally when we create a new context, and a new ppGTT to go with it, we
> point all the unused pages in the ppGTT to a 'safe' scratch page. Any
> inadvertent access outside of the declared user's area will result in a
> read/write to scratch instead. However, sometimes it is preferrable to
> that to cause a fault instead. This does not trap execution of the
> faulting batch, but it does record the error:
> 
> FAULT_TLB_DATA: 0x00000000 0x00000004
>      Address 0x0000000000004000 PPGTT
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> ---
> The name and value semantics are horrendous. The non-trapping behaviour
> is also less than ideal. Worth it?

Empty VM definitely sounds misleading.

Is there any argument to require root for this?

And why not a context create flag? 
I915_CONTEXT_CREATE_NO_VM_OUT_OF_BOUNDS_PROTECTION?

Flag feels simpler for the purpose, plus it can be handled post 
extension processing to remove the question of interleaved set_vm and 
set_empty_vm extension.

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 48 +++++++++++++++++++++
>   include/uapi/drm/i915_drm.h                 |  2 +
>   2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 1c4afa864bfe..f981269e883d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1191,6 +1191,33 @@ static int set_ringsize(struct i915_gem_context *ctx,
>   				 __intel_context_ring_size(args->value));
>   }
>   
> +static int set_empty_vm(struct i915_gem_context *ctx,
> +			struct drm_i915_gem_context_param *args)
> +{
> +	struct i915_address_space *vm = ctx->vm;
> +	int i;
> +
> +	if (!vm || INTEL_GEN(ctx->i915) < 8)
> +		return -ENODEV;
> +
> +	if (args->size || args->value)
> +		return -EINVAL;
> +
> +	if (!vm->scratch[0].encode)
> +		return 0;
> +
> +	if (vm->mm.head_node.hole_size != vm->total)
> +		return -EBUSY;
> +
> +	free_scratch(vm);
> +
> +	fill_page_dma(px_base(i915_vm_to_ppgtt(vm)->pd), 0, 512);
> +	for (i = 0; i <= vm->top; i++)
> +		vm->scratch[i].encode = 0;
> +
> +	return 0;
> +}
> +
>   static int __get_ringsize(struct intel_context *ce, void *arg)
>   {
>   	long sz;
> @@ -1220,6 +1247,19 @@ static int get_ringsize(struct i915_gem_context *ctx,
>   	return 0;
>   }
>   
> +static int get_empty_vm(struct i915_gem_context *ctx,
> +			struct drm_i915_gem_context_param *args)
> +{
> +	if (!ctx->vm || INTEL_GEN(ctx->i915) < 8)
> +		return -ENODEV;
> +
> +	if (args->size)
> +		return -EINVAL;
> +
> +	args->value = !ctx->vm->scratch[0].encode;
> +	return 0;
> +}
> +
>   int
>   i915_gem_user_to_context_sseu(struct drm_i915_private *i915,
>   			      const struct drm_i915_gem_context_param_sseu *user,
> @@ -1896,6 +1936,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
>   		ret = set_ringsize(ctx, args);
>   		break;
>   
> +	case I915_CONTEXT_PARAM_EMPTY_VM:
> +		ret = set_empty_vm(ctx, args);
> +		break;
> +
>   	case I915_CONTEXT_PARAM_BAN_PERIOD:
>   	default:
>   		ret = -EINVAL;
> @@ -2348,6 +2392,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>   		ret = get_ringsize(ctx, args);
>   		break;
>   
> +	case I915_CONTEXT_PARAM_EMPTY_VM:
> +		ret = get_empty_vm(ctx, args);
> +		break;
> +
>   	case I915_CONTEXT_PARAM_BAN_PERIOD:
>   	default:
>   		ret = -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 14b67cd6b54b..b18215a61332 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1640,6 +1640,8 @@ struct drm_i915_gem_context_param {
>    * Default is 16 KiB.
>    */
>   #define I915_CONTEXT_PARAM_RINGSIZE	0xc
> +
> +#define I915_CONTEXT_PARAM_EMPTY_VM	0xd
>   /* Must be kept compact -- no holes and well documented */
>   
>   	__u64 value;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] RFC drm/i915/gem: Allow creation of contexts with an 'empty' VM

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3596 bytes --]

Hi Chris,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20200423]
[cannot apply to v5.7-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/RFC-drm-i915-gem-Allow-creation-of-contexts-with-an-empty-VM/20200423-094753
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/gem/i915_gem_context.c:1377:44: sparse: sparse: incorrect type in initializer (different address spaces) @@    expected struct i915_address_space *vm @@    got struct i915_addresstruct i915_address_space *vm @@
   drivers/gpu/drm/i915/gem/i915_gem_context.c:1377:44: sparse:    expected struct i915_address_space *vm
   drivers/gpu/drm/i915/gem/i915_gem_context.c:1377:44: sparse:    got struct i915_address_space [noderef] <asn:4> *vm
>> drivers/gpu/drm/i915/gem/i915_gem_context.c:1439:40: sparse: sparse: dereference of noderef expression

vim +1377 drivers/gpu/drm/i915/gem/i915_gem_context.c

  1373	
  1374	static int set_empty_vm(struct i915_gem_context *ctx,
  1375				struct drm_i915_gem_context_param *args)
  1376	{
> 1377		struct i915_address_space *vm = ctx->vm;
  1378		int i;
  1379	
  1380		if (!vm || INTEL_GEN(ctx->i915) < 8)
  1381			return -ENODEV;
  1382	
  1383		if (args->size || args->value)
  1384			return -EINVAL;
  1385	
  1386		if (!vm->scratch[0].encode)
  1387			return 0;
  1388	
  1389		if (vm->mm.head_node.hole_size != vm->total)
  1390			return -EBUSY;
  1391	
  1392		free_scratch(vm);
  1393	
  1394		fill_page_dma(px_base(i915_vm_to_ppgtt(vm)->pd), 0, 512);
  1395		for (i = 0; i <= vm->top; i++)
  1396			vm->scratch[i].encode = 0;
  1397	
  1398		return 0;
  1399	}
  1400	
  1401	static int __get_ringsize(struct intel_context *ce, void *arg)
  1402	{
  1403		long sz;
  1404	
  1405		sz = intel_context_get_ring_size(ce);
  1406		GEM_BUG_ON(sz > INT_MAX);
  1407	
  1408		return sz; /* stop on first engine */
  1409	}
  1410	
  1411	static int get_ringsize(struct i915_gem_context *ctx,
  1412				struct drm_i915_gem_context_param *args)
  1413	{
  1414		int sz;
  1415	
  1416		if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
  1417			return -ENODEV;
  1418	
  1419		if (args->size)
  1420			return -EINVAL;
  1421	
  1422		sz = context_apply_all(ctx, __get_ringsize, NULL);
  1423		if (sz < 0)
  1424			return sz;
  1425	
  1426		args->value = sz;
  1427		return 0;
  1428	}
  1429	
  1430	static int get_empty_vm(struct i915_gem_context *ctx,
  1431				struct drm_i915_gem_context_param *args)
  1432	{
  1433		if (!ctx->vm || INTEL_GEN(ctx->i915) < 8)
  1434			return -ENODEV;
  1435	
  1436		if (args->size)
  1437			return -EINVAL;
  1438	
> 1439		args->value = !ctx->vm->scratch[0].encode;
  1440		return 0;
  1441	}
  1442	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org