[RFC,i-g-t,v2,5/8] tests/core_hotunplug: Add 'GEM address space' variant
diff mbox series

Message ID 20200622164415.30352-6-janusz.krzysztofik@linux.intel.com
State New
Headers show
Series
  • tests/core_hotunplug: New subtests and enhancements
Related show

Commit Message

Janusz Krzysztofik June 22, 2020, 4:44 p.m. UTC
Verify if an additional address space associated with an open device
file descriptor is cleaned up correctly on device hotunplug.

v2: rebase on upstream, update includes order

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 tests/core_hotunplug.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Michał Winiarski June 25, 2020, 7:42 p.m. UTC | #1
Quoting Janusz Krzysztofik (2020-06-22 18:44:12)
> Verify if an additional address space associated with an open device
> file descriptor is cleaned up correctly on device hotunplug.
> 
> v2: rebase on upstream, update includes order
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  tests/core_hotunplug.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> index 0892e1927..18a963564 100644
> --- a/tests/core_hotunplug.c
> +++ b/tests/core_hotunplug.c
> @@ -30,6 +30,7 @@
>  #include <unistd.h>
>  
>  #include "i915/gem.h"
> +#include "i915/gem_vm.h"
>  #include "igt.h"
>  #include "igt_device_scan.h"
>  #include "igt_kmod.h"
> @@ -332,6 +333,29 @@ static void hotreplug_lateclose(void)
>         healthcheck();
>  }
>  
> +static void vm_hotunplug_lateclose(void)
> +{
> +       struct hotunplug priv;
> +
> +       prepare_for_rescan(&priv);
> +
> +       gem_require_vm(priv.fd.drm);
> +
> +       local_debug("creating additional GEM user address space");
> +       igt_ignore_warn(gem_vm_create(priv.fd.drm));

Why the "ignore_warn"?
This deserves a comment. And perhaps a word of information about why we picked
this partucular operation in the commit message (vm_create).
This is a regression test, right?

LGTM otherwise (but again - see previous patches):

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał
Janusz Krzysztofik June 26, 2020, 8:04 a.m. UTC | #2
Hi Michał,

thanks for review.

On Thu, 2020-06-25 at 21:42 +0200, Michał Winiarski wrote:
> Quoting Janusz Krzysztofik (2020-06-22 18:44:12)
> > Verify if an additional address space associated with an open device
> > file descriptor is cleaned up correctly on device hotunplug.
> > 
> > v2: rebase on upstream, update includes order
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >  tests/core_hotunplug.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> > index 0892e1927..18a963564 100644
> > --- a/tests/core_hotunplug.c
> > +++ b/tests/core_hotunplug.c
> > @@ -30,6 +30,7 @@
> >  #include <unistd.h>
> >  
> >  #include "i915/gem.h"
> > +#include "i915/gem_vm.h"
> >  #include "igt.h"
> >  #include "igt_device_scan.h"
> >  #include "igt_kmod.h"
> > @@ -332,6 +333,29 @@ static void hotreplug_lateclose(void)
> >         healthcheck();
> >  }
> >  
> > +static void vm_hotunplug_lateclose(void)
> > +{
> > +       struct hotunplug priv;
> > +
> > +       prepare_for_rescan(&priv);
> > +
> > +       gem_require_vm(priv.fd.drm);
> > +
> > +       local_debug("creating additional GEM user address space");
> > +       igt_ignore_warn(gem_vm_create(priv.fd.drm));
> 
> Why the "ignore_warn"?  This deserves a comment. 
> And perhaps a word of information about why we picked
> this partucular operation in the commit message (vm_create).
> This is a regression test, right?

Hmm, I didn't intend it to be a regression test.  The purpose was
generally the same as of other hotunplug-lateclose subtests - exercise
the driver behaviour on hotunplug and lateclose.  This subtest was
intended to perform still the same exercise, only under different
conditions, or different use case of the driver.  In particular,
hotunplug is performed here with an additional address space associated
with an open file descriptor.  We are not interested in exercising that
address space (that's why igt_ignore_warn), only in checking if it is
cleaned up on time so hotunplug-lateclose operations are still safe
from late dma_unmap issues.

Let me try to reword the commit description so it better reflects this
idea.

Thanks,
Janusz

> 
> LGTM otherwise (but again - see previous patches):
> 
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> -Michał

Patch
diff mbox series

diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 0892e1927..18a963564 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -30,6 +30,7 @@ 
 #include <unistd.h>
 
 #include "i915/gem.h"
+#include "i915/gem_vm.h"
 #include "igt.h"
 #include "igt_device_scan.h"
 #include "igt_kmod.h"
@@ -332,6 +333,29 @@  static void hotreplug_lateclose(void)
 	healthcheck();
 }
 
+static void vm_hotunplug_lateclose(void)
+{
+	struct hotunplug priv;
+
+	prepare_for_rescan(&priv);
+
+	gem_require_vm(priv.fd.drm);
+
+	local_debug("creating additional GEM user address space");
+	igt_ignore_warn(gem_vm_create(priv.fd.drm));
+
+	local_debug("hot unplugging the device");
+	device_unplug(priv.fd.sysfs_dev);
+
+	local_debug("late closing the removed device instance");
+	close(priv.fd.drm);
+
+	local_debug("recovering the device");
+	bus_rescan(priv.fd.sysfs_bus);
+
+	healthcheck();
+}
+
 /* Main */
 
 igt_main
@@ -404,4 +428,11 @@  igt_main
 
 	igt_fixture
 		igt_abort_on_f(failure, "%s\n", failure);
+
+	igt_describe("Check if a still open device with extra GEM address space can be cleanly unplugged, then released and recovered");
+	igt_subtest("vm-hotunplug-lateclose")
+		vm_hotunplug_lateclose();
+
+	igt_fixture
+		igt_abort_on_f(failure, "%s\n", failure);
 }