[RFC PATCH 0/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[RFC PATCH 0/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Janusz Krzysztofik]

Hi,

I do realize more work needs to be done to get a clean hotunplug
solution, however I need your comments to make sure that I'm going in
the right direction.

So far I have no good idea how to resolve pm_runtime_get_sync()
failures on outstanding device file close after successfull driver
unbind.

Thanks,
Janusz


Janusz Krzysztofik (1):
  drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

 drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.21.0

[RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Janusz Krzysztofik]

In order to support driver hot unbind, some cleanup operations, now
performed on PCI driver remove, must be called later, after all device
file descriptors are closed.

Split out those operations from the tail of pci_driver.remove()
callback and put them into drm_driver.release() which is called as soon
as all references to the driver are put.  As a result, those cleanups
will be now run on last drm_dev_put(), either still called from
pci_driver.remove() if all device file descriptors are already closed,
or on last drm_release() file operation.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 83d2eb9e74cb..8be69f84eb6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_gem:
 	i915_gem_suspend(dev_priv);
+	i915_gem_fini_hw(dev_priv);
 	i915_gem_fini(dev_priv);
 cleanup_modeset:
 	intel_modeset_cleanup(dev);
@@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct drm_i915_private *dev_priv)
 		pci_disable_msi(pdev);
 
 	pm_qos_remove_request(&dev_priv->pm_qos);
-	i915_ggtt_cleanup_hw(dev_priv);
 }
 
 /**
@@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 out_cleanup_hw:
 	i915_driver_cleanup_hw(dev_priv);
+	i915_ggtt_cleanup_hw(dev_priv);
 out_cleanup_mmio:
 	i915_driver_cleanup_mmio(dev_priv);
 out_runtime_pm_put:
@@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	i915_reset_error_state(dev_priv);
 
-	i915_gem_fini(dev_priv);
+	i915_gem_fini_hw(dev_priv);
 
 	intel_power_domains_fini_hw(dev_priv);
 
 	i915_driver_cleanup_hw(dev_priv);
-	i915_driver_cleanup_mmio(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
-	intel_runtime_pm_cleanup(dev_priv);
 }
 
 static void i915_driver_release(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
+	i915_gem_fini(dev_priv);
+
+	i915_ggtt_cleanup_hw(dev_priv);
+	i915_driver_cleanup_mmio(dev_priv);
+
+	enable_rpm_wakeref_asserts(dev_priv);
+	intel_runtime_pm_cleanup(dev_priv);
+
 	i915_driver_cleanup_early(dev_priv);
 	i915_driver_destroy(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2664ea1395b..d08e7bd83544 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private *i915);
 int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
 int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
+void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
 void i915_gem_fini(struct drm_i915_private *dev_priv);
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags, long timeout);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7cafd5612f71..c6a8e665a6ba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void i915_gem_fini(struct drm_i915_private *dev_priv)
+void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
 {
 	GEM_BUG_ON(dev_priv->gt.awake);
 
@@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
 	intel_uc_fini_hw(dev_priv);
 	intel_uc_fini(dev_priv);
 	intel_engines_cleanup(dev_priv);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	i915_gem_drain_freed_objects(dev_priv);
+}
+
+void i915_gem_fini(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->drm.struct_mutex);
 	i915_gem_contexts_fini(dev_priv);
 	i915_gem_fini_scratch(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
-- 
2.21.0

Re: [RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-05-30 10:24:26)
> In order to support driver hot unbind, some cleanup operations, now
> performed on PCI driver remove, must be called later, after all device
> file descriptors are closed.
> 
> Split out those operations from the tail of pci_driver.remove()
> callback and put them into drm_driver.release() which is called as soon
> as all references to the driver are put.  As a result, those cleanups
> will be now run on last drm_dev_put(), either still called from
> pci_driver.remove() if all device file descriptors are already closed,
> or on last drm_release() file operation.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 83d2eb9e74cb..8be69f84eb6d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_gem:
>         i915_gem_suspend(dev_priv);
> +       i915_gem_fini_hw(dev_priv);
>         i915_gem_fini(dev_priv);
>  cleanup_modeset:
>         intel_modeset_cleanup(dev);
> @@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct drm_i915_private *dev_priv)
>                 pci_disable_msi(pdev);
>  
>         pm_qos_remove_request(&dev_priv->pm_qos);
> -       i915_ggtt_cleanup_hw(dev_priv);
>  }
>  
>  /**
> @@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)

Would it make sense to rename load/unload from the legacy drm stubs over
to match the pci entry points?

>  out_cleanup_hw:
>         i915_driver_cleanup_hw(dev_priv);
> +       i915_ggtt_cleanup_hw(dev_priv);
>  out_cleanup_mmio:
>         i915_driver_cleanup_mmio(dev_priv);
>  out_runtime_pm_put:
> @@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
>         cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>         i915_reset_error_state(dev_priv);
>  
> -       i915_gem_fini(dev_priv);
> +       i915_gem_fini_hw(dev_priv);
>  
>         intel_power_domains_fini_hw(dev_priv);
>  
>         i915_driver_cleanup_hw(dev_priv);
> -       i915_driver_cleanup_mmio(dev_priv);
>  
>         enable_rpm_wakeref_asserts(dev_priv);
> -       intel_runtime_pm_cleanup(dev_priv);
>  }
>  
>  static void i915_driver_release(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
>  
> +       disable_rpm_wakeref_asserts(dev_priv);
> +
> +       i915_gem_fini(dev_priv);
> +
> +       i915_ggtt_cleanup_hw(dev_priv);
> +       i915_driver_cleanup_mmio(dev_priv);
> +
> +       enable_rpm_wakeref_asserts(dev_priv);
> +       intel_runtime_pm_cleanup(dev_priv);

We should really propagate the release nomenclature down and replace our
mixed fini/cleanup. Consistency is helpful when trying to work out which
phase the code is in.

>         i915_driver_cleanup_early(dev_priv);
>         i915_driver_destroy(dev_priv);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2664ea1395b..d08e7bd83544 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private *i915);
>  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
>  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
>  void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
>  void i915_gem_fini(struct drm_i915_private *dev_priv);
>  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>                            unsigned int flags, long timeout);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7cafd5612f71..c6a8e665a6ba 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>         return ret;
>  }
>  
> -void i915_gem_fini(struct drm_i915_private *dev_priv)
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
>  {
>         GEM_BUG_ON(dev_priv->gt.awake);
>  
> @@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>         intel_uc_fini_hw(dev_priv);
>         intel_uc_fini(dev_priv);

>         intel_engines_cleanup(dev_priv);

intel_engines_cleanup -> i915_gem_fini -- that is in principle just
freeing structs. One side effect it does have is to make all engines
unavailable (but it doesn't update the engine_mask so the inconsistency
might catch us out if it is not one of the last cleanup actions).

intel_uc_fini() is a bit of a mixed bag. It looks like it flushes
runtime state, so preferrably that flush should be moved to the 
_fini_hw so that _fini is pure cleanup. So for the time being, best to
leave intel_uc_fini() here.

> +       mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +       i915_gem_drain_freed_objects(dev_priv);
> +}
> +
> +void i915_gem_fini(struct drm_i915_private *dev_priv)
> +{
> +       mutex_lock(&dev_priv->drm.struct_mutex);
>         i915_gem_contexts_fini(dev_priv);
>         i915_gem_fini_scratch(dev_priv);
>         mutex_unlock(&dev_priv->drm.struct_mutex);

That split looks sensible to me, with the consideration as to whether
defer intel_engines_cleanup() as well,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-05-30 10:24:26)
> In order to support driver hot unbind, some cleanup operations, now
> performed on PCI driver remove, must be called later, after all device
> file descriptors are closed.
> 
> Split out those operations from the tail of pci_driver.remove()
> callback and put them into drm_driver.release() which is called as soon
> as all references to the driver are put.  As a result, those cleanups
> will be now run on last drm_dev_put(), either still called from
> pci_driver.remove() if all device file descriptors are already closed,
> or on last drm_release() file operation.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 83d2eb9e74cb..8be69f84eb6d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_gem:
>         i915_gem_suspend(dev_priv);
> +       i915_gem_fini_hw(dev_priv);
>         i915_gem_fini(dev_priv);
>  cleanup_modeset:
>         intel_modeset_cleanup(dev);
> @@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct drm_i915_private *dev_priv)
>                 pci_disable_msi(pdev);
>  
>         pm_qos_remove_request(&dev_priv->pm_qos);
> -       i915_ggtt_cleanup_hw(dev_priv);
>  }
>  
>  /**
> @@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)

Would it make sense to rename load/unload from the legacy drm stubs over
to match the pci entry points?

>  out_cleanup_hw:
>         i915_driver_cleanup_hw(dev_priv);
> +       i915_ggtt_cleanup_hw(dev_priv);
>  out_cleanup_mmio:
>         i915_driver_cleanup_mmio(dev_priv);
>  out_runtime_pm_put:
> @@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
>         cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>         i915_reset_error_state(dev_priv);
>  
> -       i915_gem_fini(dev_priv);
> +       i915_gem_fini_hw(dev_priv);
>  
>         intel_power_domains_fini_hw(dev_priv);
>  
>         i915_driver_cleanup_hw(dev_priv);
> -       i915_driver_cleanup_mmio(dev_priv);
>  
>         enable_rpm_wakeref_asserts(dev_priv);
> -       intel_runtime_pm_cleanup(dev_priv);
>  }
>  
>  static void i915_driver_release(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
>  
> +       disable_rpm_wakeref_asserts(dev_priv);
> +
> +       i915_gem_fini(dev_priv);
> +
> +       i915_ggtt_cleanup_hw(dev_priv);
> +       i915_driver_cleanup_mmio(dev_priv);
> +
> +       enable_rpm_wakeref_asserts(dev_priv);
> +       intel_runtime_pm_cleanup(dev_priv);

We should really propagate the release nomenclature down and replace our
mixed fini/cleanup. Consistency is helpful when trying to work out which
phase the code is in.

>         i915_driver_cleanup_early(dev_priv);
>         i915_driver_destroy(dev_priv);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2664ea1395b..d08e7bd83544 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private *i915);
>  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
>  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
>  void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
>  void i915_gem_fini(struct drm_i915_private *dev_priv);
>  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>                            unsigned int flags, long timeout);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7cafd5612f71..c6a8e665a6ba 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>         return ret;
>  }
>  
> -void i915_gem_fini(struct drm_i915_private *dev_priv)
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
>  {
>         GEM_BUG_ON(dev_priv->gt.awake);
>  
> @@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>         intel_uc_fini_hw(dev_priv);
>         intel_uc_fini(dev_priv);

>         intel_engines_cleanup(dev_priv);

intel_engines_cleanup -> i915_gem_fini -- that is in principle just
freeing structs. One side effect it does have is to make all engines
unavailable (but it doesn't update the engine_mask so the inconsistency
might catch us out if it is not one of the last cleanup actions).

intel_uc_fini() is a bit of a mixed bag. It looks like it flushes
runtime state, so preferrably that flush should be moved to the 
_fini_hw so that _fini is pure cleanup. So for the time being, best to
leave intel_uc_fini() here.

> +       mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +       i915_gem_drain_freed_objects(dev_priv);
> +}
> +
> +void i915_gem_fini(struct drm_i915_private *dev_priv)
> +{
> +       mutex_lock(&dev_priv->drm.struct_mutex);
>         i915_gem_contexts_fini(dev_priv);
>         i915_gem_fini_scratch(dev_priv);
>         mutex_unlock(&dev_priv->drm.struct_mutex);

That split looks sensible to me, with the consideration as to whether
defer intel_engines_cleanup() as well,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

✗ Fi.CI.BAT: failure for drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Patchwork]

== Series Details ==

Series: drm/i915: Split off pci_driver.remove() tail to drm_driver.release()
URL   : https://patchwork.freedesktop.org/series/61376/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6168 -> Patchwork_13137
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_execlists:
    - fi-icl-u3:          NOTRUN -> [DMESG-WARN][1] +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-u3/igt@i915_selftest@live_execlists.html

  
#### Warnings ####

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          [INCOMPLETE][2] ([fdo#107713] / [fdo#108569]) -> [DMESG-WARN][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [PASS][4] -> [INCOMPLETE][5] ([fdo#107718])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-cml-u2:          [PASS][6] -> [FAIL][7] ([fdo#110627])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][8] -> [DMESG-WARN][9] ([fdo#102614])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-icl-u3:          [DMESG-WARN][10] ([fdo#107724]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-icl-u3/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-u3/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [DMESG-WARN][12] ([fdo#106387]) -> [PASS][13] +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110627]: https://bugs.freedesktop.org/show_bug.cgi?id=110627


Participating hosts (50 -> 45)
------------------------------

  Missing    (5): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6168 -> Patchwork_13137

  CI_DRM_6168: ef1f9911a52a7d00cb52cc9447f411340b653bcc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5024: f414756be2ac57e194919973da7b86644ba61241 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13137: a8438c2c0735c8654275bcfa4d08943c4ef15595 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a8438c2c0735 drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

== Logs ==

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

✓ Fi.CI.BAT: success for drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Patchwork]

== Series Details ==

Series: drm/i915: Split off pci_driver.remove() tail to drm_driver.release()
URL   : https://patchwork.freedesktop.org/series/61376/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6168 -> Patchwork_13137
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-dsi:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#109100])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-dsi/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-cml-u2:          [PASS][5] -> [FAIL][6] ([fdo#110627])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][7] -> [DMESG-WARN][8] ([fdo#102614])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-icl-u3/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-u3/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [DMESG-WARN][11] ([fdo#106387]) -> [PASS][12] +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
#### Warnings ####

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          [INCOMPLETE][13] ([fdo#107713] / [fdo#108569]) -> [DMESG-WARN][14] ([fdo#110801])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6168/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13137/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110627]: https://bugs.freedesktop.org/show_bug.cgi?id=110627
  [fdo#110801]: https://bugs.freedesktop.org/show_bug.cgi?id=110801


Participating hosts (50 -> 45)
------------------------------

  Missing    (5): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6168 -> Patchwork_13137

  CI_DRM_6168: ef1f9911a52a7d00cb52cc9447f411340b653bcc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5024: f414756be2ac57e194919973da7b86644ba61241 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13137: a8438c2c0735c8654275bcfa4d08943c4ef15595 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a8438c2c0735 drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

== Logs ==

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

Re: [RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Daniel Vetter]

On Thu, May 30, 2019 at 10:40:09AM +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-05-30 10:24:26)
> > In order to support driver hot unbind, some cleanup operations, now
> > performed on PCI driver remove, must be called later, after all device
> > file descriptors are closed.
> > 
> > Split out those operations from the tail of pci_driver.remove()
> > callback and put them into drm_driver.release() which is called as soon
> > as all references to the driver are put.  As a result, those cleanups
> > will be now run on last drm_dev_put(), either still called from
> > pci_driver.remove() if all device file descriptors are already closed,
> > or on last drm_release() file operation.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
> >  3 files changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 83d2eb9e74cb..8be69f84eb6d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  
> >  cleanup_gem:
> >         i915_gem_suspend(dev_priv);
> > +       i915_gem_fini_hw(dev_priv);
> >         i915_gem_fini(dev_priv);
> >  cleanup_modeset:
> >         intel_modeset_cleanup(dev);
> > @@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct drm_i915_private *dev_priv)
> >                 pci_disable_msi(pdev);
> >  
> >         pm_qos_remove_request(&dev_priv->pm_qos);
> > -       i915_ggtt_cleanup_hw(dev_priv);
> >  }
> >  
> >  /**
> > @@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> 
> Would it make sense to rename load/unload from the legacy drm stubs over
> to match the pci entry points?

+1 on that rename, load/unload is really terribly confusing and has
horrible semantics in the dri1 shadow attach world ...
-Daniel

> 
> >  out_cleanup_hw:
> >         i915_driver_cleanup_hw(dev_priv);
> > +       i915_ggtt_cleanup_hw(dev_priv);
> >  out_cleanup_mmio:
> >         i915_driver_cleanup_mmio(dev_priv);
> >  out_runtime_pm_put:
> > @@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
> >         cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> >         i915_reset_error_state(dev_priv);
> >  
> > -       i915_gem_fini(dev_priv);
> > +       i915_gem_fini_hw(dev_priv);
> >  
> >         intel_power_domains_fini_hw(dev_priv);
> >  
> >         i915_driver_cleanup_hw(dev_priv);
> > -       i915_driver_cleanup_mmio(dev_priv);
> >  
> >         enable_rpm_wakeref_asserts(dev_priv);
> > -       intel_runtime_pm_cleanup(dev_priv);
> >  }
> >  
> >  static void i915_driver_release(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > +       disable_rpm_wakeref_asserts(dev_priv);
> > +
> > +       i915_gem_fini(dev_priv);
> > +
> > +       i915_ggtt_cleanup_hw(dev_priv);
> > +       i915_driver_cleanup_mmio(dev_priv);
> > +
> > +       enable_rpm_wakeref_asserts(dev_priv);
> > +       intel_runtime_pm_cleanup(dev_priv);
> 
> We should really propagate the release nomenclature down and replace our
> mixed fini/cleanup. Consistency is helpful when trying to work out which
> phase the code is in.
> 
> >         i915_driver_cleanup_early(dev_priv);
> >         i915_driver_destroy(dev_priv);
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a2664ea1395b..d08e7bd83544 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private *i915);
> >  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
> >  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
> >  void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
> > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
> >  void i915_gem_fini(struct drm_i915_private *dev_priv);
> >  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
> >                            unsigned int flags, long timeout);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 7cafd5612f71..c6a8e665a6ba 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> >         return ret;
> >  }
> >  
> > -void i915_gem_fini(struct drm_i915_private *dev_priv)
> > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
> >  {
> >         GEM_BUG_ON(dev_priv->gt.awake);
> >  
> > @@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
> >         intel_uc_fini_hw(dev_priv);
> >         intel_uc_fini(dev_priv);
> 
> >         intel_engines_cleanup(dev_priv);
> 
> intel_engines_cleanup -> i915_gem_fini -- that is in principle just
> freeing structs. One side effect it does have is to make all engines
> unavailable (but it doesn't update the engine_mask so the inconsistency
> might catch us out if it is not one of the last cleanup actions).
> 
> intel_uc_fini() is a bit of a mixed bag. It looks like it flushes
> runtime state, so preferrably that flush should be moved to the 
> _fini_hw so that _fini is pure cleanup. So for the time being, best to
> leave intel_uc_fini() here.
> 
> > +       mutex_unlock(&dev_priv->drm.struct_mutex);
> > +
> > +       i915_gem_drain_freed_objects(dev_priv);
> > +}
> > +
> > +void i915_gem_fini(struct drm_i915_private *dev_priv)
> > +{
> > +       mutex_lock(&dev_priv->drm.struct_mutex);
> >         i915_gem_contexts_fini(dev_priv);
> >         i915_gem_fini_scratch(dev_priv);
> >         mutex_unlock(&dev_priv->drm.struct_mutex);
> 
> That split looks sensible to me, with the consideration as to whether
> defer intel_engines_cleanup() as well,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Daniel Vetter]

On Thu, May 30, 2019 at 10:40:09AM +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-05-30 10:24:26)
> > In order to support driver hot unbind, some cleanup operations, now
> > performed on PCI driver remove, must be called later, after all device
> > file descriptors are closed.
> > 
> > Split out those operations from the tail of pci_driver.remove()
> > callback and put them into drm_driver.release() which is called as soon
> > as all references to the driver are put.  As a result, those cleanups
> > will be now run on last drm_dev_put(), either still called from
> > pci_driver.remove() if all device file descriptors are already closed,
> > or on last drm_release() file operation.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
> >  3 files changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 83d2eb9e74cb..8be69f84eb6d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  
> >  cleanup_gem:
> >         i915_gem_suspend(dev_priv);
> > +       i915_gem_fini_hw(dev_priv);
> >         i915_gem_fini(dev_priv);
> >  cleanup_modeset:
> >         intel_modeset_cleanup(dev);
> > @@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct drm_i915_private *dev_priv)
> >                 pci_disable_msi(pdev);
> >  
> >         pm_qos_remove_request(&dev_priv->pm_qos);
> > -       i915_ggtt_cleanup_hw(dev_priv);
> >  }
> >  
> >  /**
> > @@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> 
> Would it make sense to rename load/unload from the legacy drm stubs over
> to match the pci entry points?

+1 on that rename, load/unload is really terribly confusing and has
horrible semantics in the dri1 shadow attach world ...
-Daniel

> 
> >  out_cleanup_hw:
> >         i915_driver_cleanup_hw(dev_priv);
> > +       i915_ggtt_cleanup_hw(dev_priv);
> >  out_cleanup_mmio:
> >         i915_driver_cleanup_mmio(dev_priv);
> >  out_runtime_pm_put:
> > @@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
> >         cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> >         i915_reset_error_state(dev_priv);
> >  
> > -       i915_gem_fini(dev_priv);
> > +       i915_gem_fini_hw(dev_priv);
> >  
> >         intel_power_domains_fini_hw(dev_priv);
> >  
> >         i915_driver_cleanup_hw(dev_priv);
> > -       i915_driver_cleanup_mmio(dev_priv);
> >  
> >         enable_rpm_wakeref_asserts(dev_priv);
> > -       intel_runtime_pm_cleanup(dev_priv);
> >  }
> >  
> >  static void i915_driver_release(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > +       disable_rpm_wakeref_asserts(dev_priv);
> > +
> > +       i915_gem_fini(dev_priv);
> > +
> > +       i915_ggtt_cleanup_hw(dev_priv);
> > +       i915_driver_cleanup_mmio(dev_priv);
> > +
> > +       enable_rpm_wakeref_asserts(dev_priv);
> > +       intel_runtime_pm_cleanup(dev_priv);
> 
> We should really propagate the release nomenclature down and replace our
> mixed fini/cleanup. Consistency is helpful when trying to work out which
> phase the code is in.
> 
> >         i915_driver_cleanup_early(dev_priv);
> >         i915_driver_destroy(dev_priv);
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a2664ea1395b..d08e7bd83544 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private *i915);
> >  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
> >  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
> >  void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
> > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
> >  void i915_gem_fini(struct drm_i915_private *dev_priv);
> >  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
> >                            unsigned int flags, long timeout);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 7cafd5612f71..c6a8e665a6ba 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> >         return ret;
> >  }
> >  
> > -void i915_gem_fini(struct drm_i915_private *dev_priv)
> > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
> >  {
> >         GEM_BUG_ON(dev_priv->gt.awake);
> >  
> > @@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
> >         intel_uc_fini_hw(dev_priv);
> >         intel_uc_fini(dev_priv);
> 
> >         intel_engines_cleanup(dev_priv);
> 
> intel_engines_cleanup -> i915_gem_fini -- that is in principle just
> freeing structs. One side effect it does have is to make all engines
> unavailable (but it doesn't update the engine_mask so the inconsistency
> might catch us out if it is not one of the last cleanup actions).
> 
> intel_uc_fini() is a bit of a mixed bag. It looks like it flushes
> runtime state, so preferrably that flush should be moved to the 
> _fini_hw so that _fini is pure cleanup. So for the time being, best to
> leave intel_uc_fini() here.
> 
> > +       mutex_unlock(&dev_priv->drm.struct_mutex);
> > +
> > +       i915_gem_drain_freed_objects(dev_priv);
> > +}
> > +
> > +void i915_gem_fini(struct drm_i915_private *dev_priv)
> > +{
> > +       mutex_lock(&dev_priv->drm.struct_mutex);
> >         i915_gem_contexts_fini(dev_priv);
> >         i915_gem_fini_scratch(dev_priv);
> >         mutex_unlock(&dev_priv->drm.struct_mutex);
> 
> That split looks sensible to me, with the consideration as to whether
> defer intel_engines_cleanup() as well,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

-- 
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

Re: [RFC PATCH 1/1] drm/i915: Split off pci_driver.remove() tail to drm_driver.release()

[Janusz Krzysztofik]

On Monday, June 3, 2019 9:28:18 AM CEST Daniel Vetter wrote:
> On Thu, May 30, 2019 at 10:40:09AM +0100, Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-05-30 10:24:26)
> > > In order to support driver hot unbind, some cleanup operations, now
> > > performed on PCI driver remove, must be called later, after all device
> > > file descriptors are closed.
> > > 
> > > Split out those operations from the tail of pci_driver.remove()
> > > callback and put them into drm_driver.release() which is called as soon
> > > as all references to the driver are put.  As a result, those cleanups
> > > will be now run on last drm_dev_put(), either still called from
> > > pci_driver.remove() if all device file descriptors are already closed,
> > > or on last drm_release() file operation.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++++++----
> > >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> > >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
> > >  3 files changed, 23 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/
i915_drv.c
> > > index 83d2eb9e74cb..8be69f84eb6d 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -738,6 +738,7 @@ static int i915_load_modeset_init(struct drm_device 
*dev)
> > >  
> > >  cleanup_gem:
> > >         i915_gem_suspend(dev_priv);
> > > +       i915_gem_fini_hw(dev_priv);
> > >         i915_gem_fini(dev_priv);
> > >  cleanup_modeset:
> > >         intel_modeset_cleanup(dev);
> > > @@ -1685,7 +1686,6 @@ static void i915_driver_cleanup_hw(struct 
drm_i915_private *dev_priv)
> > >                 pci_disable_msi(pdev);
> > >  
> > >         pm_qos_remove_request(&dev_priv->pm_qos);
> > > -       i915_ggtt_cleanup_hw(dev_priv);
> > >  }
> > >  
> > >  /**
> > > @@ -1909,6 +1909,7 @@ int i915_driver_load(struct pci_dev *pdev, const 
struct pci_device_id *ent)
> > 
> > Would it make sense to rename load/unload from the legacy drm stubs over
> > to match the pci entry points?
> 
> +1 on that rename, load/unload is really terribly confusing and has
> horrible semantics in the dri1 shadow attach world ...
> -Daniel

I've not responded to that comment, sorry, but I agree too.  I've assumed 
that's a candidate for a separate patch or series.  I'm willing to work on 
that as time permits.

Thanks,
Janusz

> > 
> > >  out_cleanup_hw:
> > >         i915_driver_cleanup_hw(dev_priv);
> > > +       i915_ggtt_cleanup_hw(dev_priv);
> > >  out_cleanup_mmio:
> > >         i915_driver_cleanup_mmio(dev_priv);
> > >  out_runtime_pm_put:
> > > @@ -1960,21 +1961,29 @@ void i915_driver_unload(struct drm_device *dev)
> > >         cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> > >         i915_reset_error_state(dev_priv);
> > >  
> > > -       i915_gem_fini(dev_priv);
> > > +       i915_gem_fini_hw(dev_priv);
> > >  
> > >         intel_power_domains_fini_hw(dev_priv);
> > >  
> > >         i915_driver_cleanup_hw(dev_priv);
> > > -       i915_driver_cleanup_mmio(dev_priv);
> > >  
> > >         enable_rpm_wakeref_asserts(dev_priv);
> > > -       intel_runtime_pm_cleanup(dev_priv);
> > >  }
> > >  
> > >  static void i915_driver_release(struct drm_device *dev)
> > >  {
> > >         struct drm_i915_private *dev_priv = to_i915(dev);
> > >  
> > > +       disable_rpm_wakeref_asserts(dev_priv);
> > > +
> > > +       i915_gem_fini(dev_priv);
> > > +
> > > +       i915_ggtt_cleanup_hw(dev_priv);
> > > +       i915_driver_cleanup_mmio(dev_priv);
> > > +
> > > +       enable_rpm_wakeref_asserts(dev_priv);
> > > +       intel_runtime_pm_cleanup(dev_priv);
> > 
> > We should really propagate the release nomenclature down and replace our
> > mixed fini/cleanup. Consistency is helpful when trying to work out which
> > phase the code is in.
> > 
> > >         i915_driver_cleanup_early(dev_priv);
> > >         i915_driver_destroy(dev_priv);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/
i915_drv.h
> > > index a2664ea1395b..d08e7bd83544 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3047,6 +3047,7 @@ void i915_gem_init_mmio(struct drm_i915_private 
*i915);
> > >  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
> > >  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
> > >  void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
> > > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
> > >  void i915_gem_fini(struct drm_i915_private *dev_priv);
> > >  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
> > >                            unsigned int flags, long timeout);
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/
i915_gem.c
> > > index 7cafd5612f71..c6a8e665a6ba 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -4667,7 +4667,7 @@ int i915_gem_init(struct drm_i915_private 
*dev_priv)
> > >         return ret;
> > >  }
> > >  
> > > -void i915_gem_fini(struct drm_i915_private *dev_priv)
> > > +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
> > >  {
> > >         GEM_BUG_ON(dev_priv->gt.awake);
> > >  
> > > @@ -4681,6 +4681,14 @@ void i915_gem_fini(struct drm_i915_private 
*dev_priv)
> > >         intel_uc_fini_hw(dev_priv);
> > >         intel_uc_fini(dev_priv);
> > 
> > >         intel_engines_cleanup(dev_priv);
> > 
> > intel_engines_cleanup -> i915_gem_fini -- that is in principle just
> > freeing structs. One side effect it does have is to make all engines
> > unavailable (but it doesn't update the engine_mask so the inconsistency
> > might catch us out if it is not one of the last cleanup actions).
> > 
> > intel_uc_fini() is a bit of a mixed bag. It looks like it flushes
> > runtime state, so preferrably that flush should be moved to the 
> > _fini_hw so that _fini is pure cleanup. So for the time being, best to
> > leave intel_uc_fini() here.
> > 
> > > +       mutex_unlock(&dev_priv->drm.struct_mutex);
> > > +
> > > +       i915_gem_drain_freed_objects(dev_priv);
> > > +}
> > > +
> > > +void i915_gem_fini(struct drm_i915_private *dev_priv)
> > > +{
> > > +       mutex_lock(&dev_priv->drm.struct_mutex);
> > >         i915_gem_contexts_fini(dev_priv);
> > >         i915_gem_fini_scratch(dev_priv);
> > >         mutex_unlock(&dev_priv->drm.struct_mutex);
> > 
> > That split looks sensible to me, with the consideration as to whether
> > defer intel_engines_cleanup() as well,
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> 
>