[igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Niranjana Vishwanathapura]

Do not check for supports_faults in xe_device_get() as
it creates a VM in fault mode which prohibits creation
of any other VM in non-fault mode until this fault mode
VM is closed. This leads to test failures in multi threaded
cases.

Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
---
 lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
 lib/xe/xe_query.h |  3 ---
 2 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
index 183523280..dc91d59bc 100644
--- a/lib/xe/xe_query.c
+++ b/lib/xe/xe_query.c
@@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
 	return alignment;
 }
 
-static bool xe_check_supports_faults(int fd)
-{
-	bool supports_faults;
-
-	struct drm_xe_vm_create create = {
-		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
-			 DRM_XE_VM_CREATE_FAULT_MODE,
-	};
-
-	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
-
-	if (supports_faults)
-		xe_vm_destroy(fd, create.vm_id);
-
-	return supports_faults;
-}
-
 /**
  * xe_engine_class_string:
  * @engine_class: engine class
@@ -258,7 +241,6 @@ struct xe_device *xe_device_get(int fd)
 						     xe_dev->gts, gt);
 	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_usage);
 	xe_dev->has_vram = __mem_has_vram(xe_dev->mem_usage);
-	xe_dev->supports_faults = xe_check_supports_faults(fd);
 
 	igt_map_insert(cache.map, &xe_dev->fd, xe_dev);
 
@@ -294,6 +276,30 @@ void xe_device_put(int fd)
 	pthread_mutex_unlock(&cache.cache_mutex);
 }
 
+/**
+ * xe_supports_faults:
+ * @fd: xe device fd
+ *
+ * Returns true if xe device @fd allows creating vm in fault mode otherwise
+ * false.
+ */
+bool xe_supports_faults(int fd)
+{
+	bool supports_faults;
+
+	struct drm_xe_vm_create create = {
+		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
+			 DRM_XE_VM_CREATE_FAULT_MODE,
+	};
+
+	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
+
+	if (supports_faults)
+		xe_vm_destroy(fd, create.vm_id);
+
+	return supports_faults;
+}
+
 static void xe_device_destroy_cache(void)
 {
 	pthread_mutex_lock(&cache.cache_mutex);
@@ -449,15 +455,6 @@ uint64_t xe_vram_size(int fd, int gt)
  */
 xe_dev_FN(xe_get_default_alignment, default_alignment, uint32_t);
 
-/**
- * xe_supports_faults:
- * @fd: xe device fd
- *
- * Returns true if xe device @fd allows creating vm in fault mode otherwise
- * false.
- */
-xe_dev_FN(xe_supports_faults, supports_faults, bool);
-
 /**
  * xe_va_bits:
  * @fd: xe device fd
diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
index 70de183cc..3a00ecd1c 100644
--- a/lib/xe/xe_query.h
+++ b/lib/xe/xe_query.h
@@ -53,9 +53,6 @@ struct xe_device {
 	/** @has_vram: true if gpu has vram, false if system memory only */
 	bool has_vram;
 
-	/** @supports_faults: true if gpu supports faults, otherwise false */
-	bool supports_faults;
-
 	/** @va_bits: va length in bits */
 	uint32_t va_bits;
 
-- 
2.21.0.rc0.32.g243a4c7e27

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Mauro Carvalho Chehab]

On Thu, 23 Mar 2023 22:02:53 -0700
Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:

> Do not check for supports_faults in xe_device_get() as
> it creates a VM in fault mode which prohibits creation
> of any other VM in non-fault mode until this fault mode
> VM is closed. This leads to test failures in multi threaded
> cases.

Hmm...

> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
>  lib/xe/xe_query.h |  3 ---
>  2 files changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 183523280..dc91d59bc 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
>  	return alignment;
>  }
>  
> -static bool xe_check_supports_faults(int fd)
> -{
> -	bool supports_faults;
> -
> -	struct drm_xe_vm_create create = {
> -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> -			 DRM_XE_VM_CREATE_FAULT_MODE,
> -	};
> -
> -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> -
> -	if (supports_faults)
> -		xe_vm_destroy(fd, create.vm_id);

Weren't the VM supposed to be closed here?


Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Niranjana Vishwanathapura]

On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
>On Thu, 23 Mar 2023 22:02:53 -0700
>Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
>
>> Do not check for supports_faults in xe_device_get() as
>> it creates a VM in fault mode which prohibits creation
>> of any other VM in non-fault mode until this fault mode
>> VM is closed. This leads to test failures in multi threaded
>> cases.
>
>Hmm...
>
>>
>> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
>> ---
>>  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
>>  lib/xe/xe_query.h |  3 ---
>>  2 files changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
>> index 183523280..dc91d59bc 100644
>> --- a/lib/xe/xe_query.c
>> +++ b/lib/xe/xe_query.c
>> @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
>>  	return alignment;
>>  }
>>
>> -static bool xe_check_supports_faults(int fd)
>> -{
>> -	bool supports_faults;
>> -
>> -	struct drm_xe_vm_create create = {
>> -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
>> -			 DRM_XE_VM_CREATE_FAULT_MODE,
>> -	};
>> -
>> -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
>> -
>> -	if (supports_faults)
>> -		xe_vm_destroy(fd, create.vm_id);
>
>Weren't the VM supposed to be closed here?
>

Yes, but before it does destroy the VM, some other thread can try
to create a VM in non-fault mode and fail because we have created
a fault mode VM here. This happens in multi-threaded test like
xe_exec_threads.

Niranjana


>
>Regards,
>Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Matthew Brost]

On Thu, Mar 23, 2023 at 10:02:53PM -0700, Niranjana Vishwanathapura wrote:
> Do not check for supports_faults in xe_device_get() as
> it creates a VM in fault mode which prohibits creation
> of any other VM in non-fault mode until this fault mode
> VM is closed. This leads to test failures in multi threaded
> cases.
> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

Just a general comment, we probably should report if the device supports
faults via the device query IOCTL. Let me look into that (and a few
other query issues) and post a RFC.

For now this fix LGTM. With that:

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
>  lib/xe/xe_query.h |  3 ---
>  2 files changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 183523280..dc91d59bc 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
>  	return alignment;
>  }
>  
> -static bool xe_check_supports_faults(int fd)
> -{
> -	bool supports_faults;
> -
> -	struct drm_xe_vm_create create = {
> -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> -			 DRM_XE_VM_CREATE_FAULT_MODE,
> -	};
> -
> -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> -
> -	if (supports_faults)
> -		xe_vm_destroy(fd, create.vm_id);
> -
> -	return supports_faults;
> -}
> -
>  /**
>   * xe_engine_class_string:
>   * @engine_class: engine class
> @@ -258,7 +241,6 @@ struct xe_device *xe_device_get(int fd)
>  						     xe_dev->gts, gt);
>  	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_usage);
>  	xe_dev->has_vram = __mem_has_vram(xe_dev->mem_usage);
> -	xe_dev->supports_faults = xe_check_supports_faults(fd);
>  
>  	igt_map_insert(cache.map, &xe_dev->fd, xe_dev);
>  
> @@ -294,6 +276,30 @@ void xe_device_put(int fd)
>  	pthread_mutex_unlock(&cache.cache_mutex);
>  }
>  
> +/**
> + * xe_supports_faults:
> + * @fd: xe device fd
> + *
> + * Returns true if xe device @fd allows creating vm in fault mode otherwise
> + * false.
> + */
> +bool xe_supports_faults(int fd)
> +{
> +	bool supports_faults;
> +
> +	struct drm_xe_vm_create create = {
> +		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> +			 DRM_XE_VM_CREATE_FAULT_MODE,
> +	};
> +
> +	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> +
> +	if (supports_faults)
> +		xe_vm_destroy(fd, create.vm_id);
> +
> +	return supports_faults;
> +}
> +
>  static void xe_device_destroy_cache(void)
>  {
>  	pthread_mutex_lock(&cache.cache_mutex);
> @@ -449,15 +455,6 @@ uint64_t xe_vram_size(int fd, int gt)
>   */
>  xe_dev_FN(xe_get_default_alignment, default_alignment, uint32_t);
>  
> -/**
> - * xe_supports_faults:
> - * @fd: xe device fd
> - *
> - * Returns true if xe device @fd allows creating vm in fault mode otherwise
> - * false.
> - */
> -xe_dev_FN(xe_supports_faults, supports_faults, bool);
> -
>  /**
>   * xe_va_bits:
>   * @fd: xe device fd
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index 70de183cc..3a00ecd1c 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -53,9 +53,6 @@ struct xe_device {
>  	/** @has_vram: true if gpu has vram, false if system memory only */
>  	bool has_vram;
>  
> -	/** @supports_faults: true if gpu supports faults, otherwise false */
> -	bool supports_faults;
> -
>  	/** @va_bits: va length in bits */
>  	uint32_t va_bits;
>  
> -- 
> 2.21.0.rc0.32.g243a4c7e27
> 

[igt-dev] ✗ Fi.CI.BAT: failure for lib/xe/xe_query: Extern xe_supports_faults()

[Patchwork]

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

== Series Details ==

Series: lib/xe/xe_query: Extern xe_supports_faults()
URL   : https://patchwork.freedesktop.org/series/115579/
State : failure

== Summary ==

CI Bug Log - changes from IGT_7216 -> IGTPW_8675
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8675 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8675, 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/IGTPW_8675/index.html

Participating hosts (37 -> 35)
------------------------------

  Missing    (2): bat-rpls-2 fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - bat-adlp-9:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-adlp-9/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-adlp-9/igt@i915_selftest@live@hangcheck.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         NOTRUN -> [ABORT][3] ([i915#6687] / [i915#7978])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-dg2-11:         NOTRUN -> [SKIP][4] ([i915#5354])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - fi-ivb-3770:        NOTRUN -> [SKIP][5] ([fdo#109271])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/fi-ivb-3770/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][6] ([i915#7699]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-dg2-11/igt@i915_selftest@live@migrate.html
    - bat-atsm-1:         [DMESG-FAIL][8] ([i915#7699]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-atsm-1/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][10] ([i915#4983]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-rpls-1/igt@i915_selftest@live@reset.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/bat-rpls-1/igt@i915_selftest@live@reset.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978


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

  * CI: CI-20190529 -> None
  * IGT: IGT_7216 -> IGTPW_8675

  CI-20190529: 20190529
  CI_DRM_12907: 3e6be7c63e438996c88d6ba51a7d3025c56086d0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8675: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/index.html
  IGT_7216: 0682c2b07c7eab2bf384899c3da3cc7f08083847 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8675/index.html

[-- Attachment #2: Type: text/html, Size: 4770 bytes --]

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Mauro Carvalho Chehab]

On Thu, 23 Mar 2023 23:23:35 -0700
Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:

> On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
> >On Thu, 23 Mar 2023 22:02:53 -0700
> >Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
> >
> >> Do not check for supports_faults in xe_device_get() as
> >> it creates a VM in fault mode which prohibits creation
> >> of any other VM in non-fault mode until this fault mode
> >> VM is closed. This leads to test failures in multi threaded
> >> cases.
> >
> >Hmm...
> >
> >>
> >> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> >> ---
> >>  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
> >>  lib/xe/xe_query.h |  3 ---
> >>  2 files changed, 24 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> >> index 183523280..dc91d59bc 100644
> >> --- a/lib/xe/xe_query.c
> >> +++ b/lib/xe/xe_query.c
> >> @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
> >>  	return alignment;
> >>  }
> >>
> >> -static bool xe_check_supports_faults(int fd)
> >> -{
> >> -	bool supports_faults;
> >> -
> >> -	struct drm_xe_vm_create create = {
> >> -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> >> -			 DRM_XE_VM_CREATE_FAULT_MODE,
> >> -	};
> >> -
> >> -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> >> -
> >> -	if (supports_faults)
> >> -		xe_vm_destroy(fd, create.vm_id);
> >
> >Weren't the VM supposed to be closed here?
> >
> 
> Yes, but before it does destroy the VM, some other thread can try
> to create a VM in non-fault mode and fail because we have created
> a fault mode VM here. This happens in multi-threaded test like
> xe_exec_threads.

Ok. Please add a note at the kernel-doc. Something like:

	/**
	 * xe_supports_faults:
	 * @fd: xe device fd
	 *
	 * Returns true if xe device @fd allows creating vm in fault mode otherwise
	 * false.
	 *
	 * Please note that, while this function is executed, no
	 * non-fault mode VMs can be created.  
	 */
	bool xe_supports_faults(int fd)

On Fri, 24 Mar 2023 06:53:52 +0000
Matthew Brost <matthew.brost@intel.com> wrote:

> On Thu, Mar 23, 2023 at 10:02:53PM -0700, Niranjana Vishwanathapura wrote:
>
> Just a general comment, we probably should report if the device supports
> faults via the device query IOCTL. Let me look into that (and a few
> other query issues) and post a RFC.

Agreed. Specially for ioctls like this that, which checking if a feature
is supported will change the driver's state machine, placing the
data into the query ioctl is a need.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Zbigniew Kempczyński]

On Thu, Mar 23, 2023 at 11:23:35PM -0700, Niranjana Vishwanathapura wrote:
> On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
> > On Thu, 23 Mar 2023 22:02:53 -0700
> > Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
> > 
> > > Do not check for supports_faults in xe_device_get() as
> > > it creates a VM in fault mode which prohibits creation
> > > of any other VM in non-fault mode until this fault mode
> > > VM is closed. This leads to test failures in multi threaded
> > > cases.
> > 
> > Hmm...
> > 
> > > 
> > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > > ---
> > >  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
> > >  lib/xe/xe_query.h |  3 ---
> > >  2 files changed, 24 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > > index 183523280..dc91d59bc 100644
> > > --- a/lib/xe/xe_query.c
> > > +++ b/lib/xe/xe_query.c
> > > @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
> > >  	return alignment;
> > >  }
> > > 
> > > -static bool xe_check_supports_faults(int fd)
> > > -{
> > > -	bool supports_faults;
> > > -
> > > -	struct drm_xe_vm_create create = {
> > > -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> > > -			 DRM_XE_VM_CREATE_FAULT_MODE,
> > > -	};
> > > -
> > > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > > -
> > > -	if (supports_faults)
> > > -		xe_vm_destroy(fd, create.vm_id);
> > 
> > Weren't the VM supposed to be closed here?
> > 
> 
> Yes, but before it does destroy the VM, some other thread can try
> to create a VM in non-fault mode and fail because we have created
> a fault mode VM here. This happens in multi-threaded test like
> xe_exec_threads.

I've question about it - why separately created vm in fault mode
influences on creating another vm in non-fault mode? Those vm's
are separate entities so why they collide?

I've examined the code and at the moment I see two scenarios
- threads are reusing same xe_device instantiated on opening
fixture and are opening their own fd, what means each cached
xe_device entry reside on separate fd. I see there's lack
of proper locking during insertion to igt_map (I'm going to
send a fix in a minute). I bet this might be reason of problems
- multiple threads adding to hashmap (which might resize in
this moment).

I see there's risk of executing xe_check_supports_faults()
twice on same fd from two competing threads and this is not
mutexed. But create/destroy vm is on local stack and even with
this it shouldn't influence on other thread execution.

--
Zbigniew

> 
> Niranjana
> 
> 
> > 
> > Regards,
> > Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Zbigniew Kempczyński]

On Fri, Mar 24, 2023 at 06:53:52AM +0000, Matthew Brost wrote:
> On Thu, Mar 23, 2023 at 10:02:53PM -0700, Niranjana Vishwanathapura wrote:
> > Do not check for supports_faults in xe_device_get() as
> > it creates a VM in fault mode which prohibits creation
> > of any other VM in non-fault mode until this fault mode
> > VM is closed. This leads to test failures in multi threaded
> > cases.
> > 
> > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> 
> Just a general comment, we probably should report if the device supports
> faults via the device query IOCTL. Let me look into that (and a few
> other query issues) and post a RFC.
> 
> For now this fix LGTM. With that:
> 
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 

I'm sorry but I would like to get more about this before we merge.

Does it mean that if one thread will create fault-mode VM until it is
closed another thread is not able to create non-fault-mode?
This imo breaks vm separation and even if query ioctl for this exists
still doesn't fix the real issue in the driver.

--
Zbigniew

> > ---
> >  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
> >  lib/xe/xe_query.h |  3 ---
> >  2 files changed, 24 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > index 183523280..dc91d59bc 100644
> > --- a/lib/xe/xe_query.c
> > +++ b/lib/xe/xe_query.c
> > @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
> >  	return alignment;
> >  }
> >  
> > -static bool xe_check_supports_faults(int fd)
> > -{
> > -	bool supports_faults;
> > -
> > -	struct drm_xe_vm_create create = {
> > -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> > -			 DRM_XE_VM_CREATE_FAULT_MODE,
> > -	};
> > -
> > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > -
> > -	if (supports_faults)
> > -		xe_vm_destroy(fd, create.vm_id);
> > -
> > -	return supports_faults;
> > -}
> > -
> >  /**
> >   * xe_engine_class_string:
> >   * @engine_class: engine class
> > @@ -258,7 +241,6 @@ struct xe_device *xe_device_get(int fd)
> >  						     xe_dev->gts, gt);
> >  	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_usage);
> >  	xe_dev->has_vram = __mem_has_vram(xe_dev->mem_usage);
> > -	xe_dev->supports_faults = xe_check_supports_faults(fd);
> >  
> >  	igt_map_insert(cache.map, &xe_dev->fd, xe_dev);
> >  
> > @@ -294,6 +276,30 @@ void xe_device_put(int fd)
> >  	pthread_mutex_unlock(&cache.cache_mutex);
> >  }
> >  
> > +/**
> > + * xe_supports_faults:
> > + * @fd: xe device fd
> > + *
> > + * Returns true if xe device @fd allows creating vm in fault mode otherwise
> > + * false.
> > + */
> > +bool xe_supports_faults(int fd)
> > +{
> > +	bool supports_faults;
> > +
> > +	struct drm_xe_vm_create create = {
> > +		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> > +			 DRM_XE_VM_CREATE_FAULT_MODE,
> > +	};
> > +
> > +	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > +
> > +	if (supports_faults)
> > +		xe_vm_destroy(fd, create.vm_id);
> > +
> > +	return supports_faults;
> > +}
> > +
> >  static void xe_device_destroy_cache(void)
> >  {
> >  	pthread_mutex_lock(&cache.cache_mutex);
> > @@ -449,15 +455,6 @@ uint64_t xe_vram_size(int fd, int gt)
> >   */
> >  xe_dev_FN(xe_get_default_alignment, default_alignment, uint32_t);
> >  
> > -/**
> > - * xe_supports_faults:
> > - * @fd: xe device fd
> > - *
> > - * Returns true if xe device @fd allows creating vm in fault mode otherwise
> > - * false.
> > - */
> > -xe_dev_FN(xe_supports_faults, supports_faults, bool);
> > -
> >  /**
> >   * xe_va_bits:
> >   * @fd: xe device fd
> > diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> > index 70de183cc..3a00ecd1c 100644
> > --- a/lib/xe/xe_query.h
> > +++ b/lib/xe/xe_query.h
> > @@ -53,9 +53,6 @@ struct xe_device {
> >  	/** @has_vram: true if gpu has vram, false if system memory only */
> >  	bool has_vram;
> >  
> > -	/** @supports_faults: true if gpu supports faults, otherwise false */
> > -	bool supports_faults;
> > -
> >  	/** @va_bits: va length in bits */
> >  	uint32_t va_bits;
> >  
> > -- 
> > 2.21.0.rc0.32.g243a4c7e27
> > 

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Niranjana Vishwanathapura]

On Fri, Mar 24, 2023 at 10:21:51AM +0100, Zbigniew Kempczyński wrote:
>On Thu, Mar 23, 2023 at 11:23:35PM -0700, Niranjana Vishwanathapura wrote:
>> On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
>> > On Thu, 23 Mar 2023 22:02:53 -0700
>> > Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
>> >
>> > > Do not check for supports_faults in xe_device_get() as
>> > > it creates a VM in fault mode which prohibits creation
>> > > of any other VM in non-fault mode until this fault mode
>> > > VM is closed. This leads to test failures in multi threaded
>> > > cases.
>> >
>> > Hmm...
>> >
>> > >
>> > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
>> > > ---
>> > >  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
>> > >  lib/xe/xe_query.h |  3 ---
>> > >  2 files changed, 24 insertions(+), 30 deletions(-)
>> > >
>> > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
>> > > index 183523280..dc91d59bc 100644
>> > > --- a/lib/xe/xe_query.c
>> > > +++ b/lib/xe/xe_query.c
>> > > @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
>> > >  	return alignment;
>> > >  }
>> > >
>> > > -static bool xe_check_supports_faults(int fd)
>> > > -{
>> > > -	bool supports_faults;
>> > > -
>> > > -	struct drm_xe_vm_create create = {
>> > > -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
>> > > -			 DRM_XE_VM_CREATE_FAULT_MODE,
>> > > -	};
>> > > -
>> > > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
>> > > -
>> > > -	if (supports_faults)
>> > > -		xe_vm_destroy(fd, create.vm_id);
>> >
>> > Weren't the VM supposed to be closed here?
>> >
>>
>> Yes, but before it does destroy the VM, some other thread can try
>> to create a VM in non-fault mode and fail because we have created
>> a fault mode VM here. This happens in multi-threaded test like
>> xe_exec_threads.
>
>I've question about it - why separately created vm in fault mode
>influences on creating another vm in non-fault mode? Those vm's
>are separate entities so why they collide?
>
>I've examined the code and at the moment I see two scenarios
>- threads are reusing same xe_device instantiated on opening
>fixture and are opening their own fd, what means each cached
>xe_device entry reside on separate fd. I see there's lack
>of proper locking during insertion to igt_map (I'm going to
>send a fix in a minute). I bet this might be reason of problems
>- multiple threads adding to hashmap (which might resize in
>this moment).
>
>I see there's risk of executing xe_check_supports_faults()
>twice on same fd from two competing threads and this is not
>mutexed. But create/destroy vm is on local stack and even with
>this it shouldn't influence on other thread execution.
>

Zbigniew,

Looks like KMD Xe driver fault/non-fault mode is at device level
(check xe_device_in_fault_mode(), it is used in couple places).

Probably Matt Brost can answer your question precisely.

I wonder, whether we need this runtime switching between fault
and non-fault modes. I would assume, we always use fault mode
if device supports it, We can perhaps have a module load time
parameter to force non-fault mode.

Matt, any thoughts?

In any case, this patch seems good to me. It checks whether
xe supports faults only for those tests that need it and also
solves the problem we currently have. So, I suggest we go
ahead with this patch. What do you think?

Niranjana

>--
>Zbigniew
>
>>
>> Niranjana
>>
>>
>> >
>> > Regards,
>> > Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Matthew Brost]

On Mon, Mar 27, 2023 at 04:54:42PM -0700, Niranjana Vishwanathapura wrote:
> On Fri, Mar 24, 2023 at 10:21:51AM +0100, Zbigniew Kempczyński wrote:
> > On Thu, Mar 23, 2023 at 11:23:35PM -0700, Niranjana Vishwanathapura wrote:
> > > On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
> > > > On Thu, 23 Mar 2023 22:02:53 -0700
> > > > Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
> > > >
> > > > > Do not check for supports_faults in xe_device_get() as
> > > > > it creates a VM in fault mode which prohibits creation
> > > > > of any other VM in non-fault mode until this fault mode
> > > > > VM is closed. This leads to test failures in multi threaded
> > > > > cases.
> > > >
> > > > Hmm...
> > > >
> > > > >
> > > > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > > > > ---
> > > > >  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
> > > > >  lib/xe/xe_query.h |  3 ---
> > > > >  2 files changed, 24 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > > > > index 183523280..dc91d59bc 100644
> > > > > --- a/lib/xe/xe_query.c
> > > > > +++ b/lib/xe/xe_query.c
> > > > > @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
> > > > >  	return alignment;
> > > > >  }
> > > > >
> > > > > -static bool xe_check_supports_faults(int fd)
> > > > > -{
> > > > > -	bool supports_faults;
> > > > > -
> > > > > -	struct drm_xe_vm_create create = {
> > > > > -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> > > > > -			 DRM_XE_VM_CREATE_FAULT_MODE,
> > > > > -	};
> > > > > -
> > > > > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > > > > -
> > > > > -	if (supports_faults)
> > > > > -		xe_vm_destroy(fd, create.vm_id);
> > > >
> > > > Weren't the VM supposed to be closed here?
> > > >
> > > 
> > > Yes, but before it does destroy the VM, some other thread can try
> > > to create a VM in non-fault mode and fail because we have created
> > > a fault mode VM here. This happens in multi-threaded test like
> > > xe_exec_threads.
> > 
> > I've question about it - why separately created vm in fault mode
> > influences on creating another vm in non-fault mode? Those vm's
> > are separate entities so why they collide?
> > 
> > I've examined the code and at the moment I see two scenarios
> > - threads are reusing same xe_device instantiated on opening
> > fixture and are opening their own fd, what means each cached
> > xe_device entry reside on separate fd. I see there's lack
> > of proper locking during insertion to igt_map (I'm going to
> > send a fix in a minute). I bet this might be reason of problems
> > - multiple threads adding to hashmap (which might resize in
> > this moment).
> > 
> > I see there's risk of executing xe_check_supports_faults()
> > twice on same fd from two competing threads and this is not
> > mutexed. But create/destroy vm is on local stack and even with
> > this it shouldn't influence on other thread execution.
> > 
> 
> Zbigniew,
> 
> Looks like KMD Xe driver fault/non-fault mode is at device level
> (check xe_device_in_fault_mode(), it is used in couple places).
> 
> Probably Matt Brost can answer your question precisely.
>

Yes, the KMD check is at a device level, basically you can't use any
dma-fences if faults are used on another VM without the risk of
deadlocking, this is why we have these checks in the KMD,
 
> I wonder, whether we need this runtime switching between fault
> and non-fault modes. I would assume, we always use fault mode
> if device supports it, We can perhaps have a module load time
> parameter to force non-fault mode.
> 
> Matt, any thoughts?
>

I prefer to have this dynamic as we shouldn't force a paradigm on the
user.

> In any case, this patch seems good to me. It checks whether
> xe supports faults only for those tests that need it and also
> solves the problem we currently have. So, I suggest we go
> ahead with this patch. What do you think?
>

I agree this patch is good for now.

Matt

> Niranjana
> 
> > --
> > Zbigniew
> > 
> > > 
> > > Niranjana
> > > 
> > > 
> > > >
> > > > Regards,
> > > > Mauro

Re: [igt-dev] [PATCH i-g-t] lib/xe/xe_query: Extern xe_supports_faults()

[Zbigniew Kempczyński]

On Mon, Mar 27, 2023 at 04:54:42PM -0700, Niranjana Vishwanathapura wrote:
> On Fri, Mar 24, 2023 at 10:21:51AM +0100, Zbigniew Kempczyński wrote:
> > On Thu, Mar 23, 2023 at 11:23:35PM -0700, Niranjana Vishwanathapura wrote:
> > > On Fri, Mar 24, 2023 at 07:12:46AM +0100, Mauro Carvalho Chehab wrote:
> > > > On Thu, 23 Mar 2023 22:02:53 -0700
> > > > Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> wrote:
> > > >
> > > > > Do not check for supports_faults in xe_device_get() as
> > > > > it creates a VM in fault mode which prohibits creation
> > > > > of any other VM in non-fault mode until this fault mode
> > > > > VM is closed. This leads to test failures in multi threaded
> > > > > cases.
> > > >
> > > > Hmm...
> > > >
> > > > >
> > > > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > > > > ---
> > > > >  lib/xe/xe_query.c | 51 ++++++++++++++++++++++-------------------------
> > > > >  lib/xe/xe_query.h |  3 ---
> > > > >  2 files changed, 24 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > > > > index 183523280..dc91d59bc 100644
> > > > > --- a/lib/xe/xe_query.c
> > > > > +++ b/lib/xe/xe_query.c
> > > > > @@ -160,23 +160,6 @@ static uint32_t __mem_default_alignment(struct drm_xe_query_mem_usage *mem_usage
> > > > >  	return alignment;
> > > > >  }
> > > > >
> > > > > -static bool xe_check_supports_faults(int fd)
> > > > > -{
> > > > > -	bool supports_faults;
> > > > > -
> > > > > -	struct drm_xe_vm_create create = {
> > > > > -		.flags = DRM_XE_VM_CREATE_ASYNC_BIND_OPS |
> > > > > -			 DRM_XE_VM_CREATE_FAULT_MODE,
> > > > > -	};
> > > > > -
> > > > > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > > > > -
> > > > > -	if (supports_faults)
> > > > > -		xe_vm_destroy(fd, create.vm_id);
> > > >
> > > > Weren't the VM supposed to be closed here?
> > > >
> > > 
> > > Yes, but before it does destroy the VM, some other thread can try
> > > to create a VM in non-fault mode and fail because we have created
> > > a fault mode VM here. This happens in multi-threaded test like
> > > xe_exec_threads.
> > 
> > I've question about it - why separately created vm in fault mode
> > influences on creating another vm in non-fault mode? Those vm's
> > are separate entities so why they collide?
> > 
> > I've examined the code and at the moment I see two scenarios
> > - threads are reusing same xe_device instantiated on opening
> > fixture and are opening their own fd, what means each cached
> > xe_device entry reside on separate fd. I see there's lack
> > of proper locking during insertion to igt_map (I'm going to
> > send a fix in a minute). I bet this might be reason of problems
> > - multiple threads adding to hashmap (which might resize in
> > this moment).
> > 
> > I see there's risk of executing xe_check_supports_faults()
> > twice on same fd from two competing threads and this is not
> > mutexed. But create/destroy vm is on local stack and even with
> > this it shouldn't influence on other thread execution.
> > 
> 
> Zbigniew,
> 
> Looks like KMD Xe driver fault/non-fault mode is at device level
> (check xe_device_in_fault_mode(), it is used in couple places).

Structure "usm" (not union) xe_device keeping usm state contains two fields:

		/** @num_vm_in_fault_mode: number of VM in fault mode */
		u32 num_vm_in_fault_mode;
		/** @num_vm_in_non_fault_mode: number of VM in non-fault mode */
		u32 num_vm_in_non_fault_mode;

So I presume supporting fault mode shouldn't influence on non-fault
mode vm's on same opened fd.

Failure in creating non-fault mode vm while another fault vm mode
is created looks like a bug imo.

--
Zbigniew

> 
> Probably Matt Brost can answer your question precisely.
> 
> I wonder, whether we need this runtime switching between fault
> and non-fault modes. I would assume, we always use fault mode
> if device supports it, We can perhaps have a module load time
> parameter to force non-fault mode.
> 
> Matt, any thoughts?
> 
> In any case, this patch seems good to me. It checks whether
> xe supports faults only for those tests that need it and also
> solves the problem we currently have. So, I suggest we go
> ahead with this patch. What do you think?
> 
> Niranjana
> 
> > --
> > Zbigniew
> > 
> > > 
> > > Niranjana
> > > 
> > > 
> > > >
> > > > Regards,
> > > > Mauro