[Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Chris Wilson]

Switch off the scanout during driver unregister, so we can shutdown the
HW immediately for unbind.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 320856b665a1..62d188e5cb8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	 * events.
 	 */
 	drm_kms_helper_poll_fini(&dev_priv->drm);
+	drm_atomic_helper_shutdown(&dev_priv->drm);
 
 	intel_gt_driver_unregister(&dev_priv->gt);
 	acpi_video_unregister();
-- 
2.20.1

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Disable outputs during unregister

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 6807 bytes --]

== Series Details ==

Series: drm/i915: Disable outputs during unregister
URL   : https://patchwork.freedesktop.org/series/84371/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9401 -> Patchwork_19008
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@core_hotunplug@unbind-rebind:
    - fi-kbl-7500u:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-kbl-7500u/igt@core_hotunplug@unbind-rebind.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-kbl-7500u/igt@core_hotunplug@unbind-rebind.html

  
New tests
---------

  New tests have been introduced between CI_DRM_9401 and Patchwork_19008:

### New CI tests (1) ###

  * boot:
    - Statuses : 41 pass(s)
    - Exec time: [0.0] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@core_hotunplug@unbind-rebind:
    - fi-tgl-u2:          [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-tgl-u2/igt@core_hotunplug@unbind-rebind.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-tgl-u2/igt@core_hotunplug@unbind-rebind.html

  * igt@i915_module_load@reload:
    - fi-bxt-dsi:         [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-bxt-dsi/igt@i915_module_load@reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-bxt-dsi/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@module-reload:
    - fi-byt-j1900:       [PASS][7] -> [DMESG-WARN][8] ([i915#1982])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@execlists:
    - fi-icl-y:           [PASS][9] -> [INCOMPLETE][10] ([i915#1037] / [i915#2276])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-icl-y/igt@i915_selftest@live@execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-icl-y/igt@i915_selftest@live@execlists.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [PASS][11] -> [DMESG-WARN][12] ([i915#1982])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  * igt@prime_self_import@basic-with_one_bo_two_files:
    - fi-tgl-y:           [PASS][13] -> [DMESG-WARN][14] ([i915#402]) +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html

  
#### Possible fixes ####

  * igt@core_hotunplug@unbind-rebind:
    - fi-blb-e6850:       [INCOMPLETE][15] ([i915#2540]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-blb-e6850/igt@core_hotunplug@unbind-rebind.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-blb-e6850/igt@core_hotunplug@unbind-rebind.html

  * igt@i915_module_load@reload:
    - fi-icl-u2:          [DMESG-WARN][17] ([i915#1982]) -> [PASS][18] +3 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-icl-u2/igt@i915_module_load@reload.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-icl-u2/igt@i915_module_load@reload.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [FAIL][19] ([i915#1161] / [i915#262]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-bsw-kefka:       [DMESG-WARN][21] ([i915#1982]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-tgl-y:           [DMESG-WARN][23] ([i915#402]) -> [PASS][24] +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9401/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19008/fi-tgl-y/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).

  [i915#1037]: https://gitlab.freedesktop.org/drm/intel/issues/1037
  [i915#1161]: https://gitlab.freedesktop.org/drm/intel/issues/1161
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2276]: https://gitlab.freedesktop.org/drm/intel/issues/2276
  [i915#2540]: https://gitlab.freedesktop.org/drm/intel/issues/2540
  [i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402


Participating hosts (46 -> 41)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


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

  * Linux: CI_DRM_9401 -> Patchwork_19008

  CI-20190529: 20190529
  CI_DRM_9401: a88f54b9c003dd8ee5442caa8212cf507e0172c1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5873: b6321b58dcaa41ba1d28aced42d6b15dc3d49ca2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19008: c34cd85729335eba9eb5e4195bfcb30e2c5a3bdc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c34cd8572933 drm/i915: Disable outputs during unregister

== Logs ==

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

[-- Attachment #1.2: Type: text/html, Size: 8158 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Chris Wilson]

Quoting Chris Wilson (2020-11-27 22:05:48)
> Switch off the scanout during driver unregister, so we can shutdown the
> HW immediately for unbind.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ping? Pretty vital for module unbind and unload.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Ville Syrjälä]

On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote:
> Switch off the scanout during driver unregister, so we can shutdown the
> HW immediately for unbind.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 320856b665a1..62d188e5cb8d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  	 * events.
>  	 */
>  	drm_kms_helper_poll_fini(&dev_priv->drm);
> +	drm_atomic_helper_shutdown(&dev_priv->drm);

Looks like we already have this in remove(). Is that too late?

>  
>  	intel_gt_driver_unregister(&dev_priv->gt);
>  	acpi_video_unregister();
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Chris Wilson]

Quoting Ville Syrjälä (2020-12-01 16:05:17)
> On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote:
> > Switch off the scanout during driver unregister, so we can shutdown the
> > HW immediately for unbind.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 320856b665a1..62d188e5cb8d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> >        * events.
> >        */
> >       drm_kms_helper_poll_fini(&dev_priv->drm);
> > +     drm_atomic_helper_shutdown(&dev_priv->drm);
> 
> Looks like we already have this in remove(). Is that too late?

For the operations we do during unbind, yes.

For the core_hotplug/rebind dance, we have to reset the GPU while we
still have runtime-pm operational and have pushed the reset to
unregister (from experimentation that's as late as we can put it where
the GPU works after rebinding and we don't corrupt the system on unbind,
with the current hooks). You can guess how well gen3 likes that.

But I don't think the right answer is to skip the reset for gen3.
Suppose we enable context support for gen3, then the reset would be
required as well, and so we would still need the whole display
shenanigans to turn it off. Moving the modeset to turn the display off
to the end of userspace seems reasonable.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Ville Syrjälä]

On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2020-12-01 16:05:17)
> > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote:
> > > Switch off the scanout during driver unregister, so we can shutdown the
> > > HW immediately for unbind.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 320856b665a1..62d188e5cb8d 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > >        * events.
> > >        */
> > >       drm_kms_helper_poll_fini(&dev_priv->drm);
> > > +     drm_atomic_helper_shutdown(&dev_priv->drm);
> > 
> > Looks like we already have this in remove(). Is that too late?
> 
> For the operations we do during unbind, yes.
> 
> For the core_hotplug/rebind dance, we have to reset the GPU while we
> still have runtime-pm operational and have pushed the reset to
> unregister (from experimentation that's as late as we can put it where
> the GPU works after rebinding and we don't corrupt the system on unbind,
> with the current hooks). You can guess how well gen3 likes that.
> 
> But I don't think the right answer is to skip the reset for gen3.
> Suppose we enable context support for gen3, then the reset would be
> required as well, and so we would still need the whole display
> shenanigans to turn it off. Moving the modeset to turn the display off
> to the end of userspace seems reasonable.

Yeah, just a bit odd to have the same call twice in the
sequence. Can we remove the second call at least?

Also a bit annoying the unload sequence no longer matches the
suspend sequence. Well, I guess it was never 100% anyway but
I think it was a bit closer before this patch. But the whole
thing is rather messy anyway so I guess t's not significantly
worse after this.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Chris Wilson]

Quoting Ville Syrjälä (2020-12-04 16:01:11)
> On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2020-12-01 16:05:17)
> > > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote:
> > > > Switch off the scanout during driver unregister, so we can shutdown the
> > > > HW immediately for unbind.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 320856b665a1..62d188e5cb8d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > > >        * events.
> > > >        */
> > > >       drm_kms_helper_poll_fini(&dev_priv->drm);
> > > > +     drm_atomic_helper_shutdown(&dev_priv->drm);
> > > 
> > > Looks like we already have this in remove(). Is that too late?
> > 
> > For the operations we do during unbind, yes.
> > 
> > For the core_hotplug/rebind dance, we have to reset the GPU while we
> > still have runtime-pm operational and have pushed the reset to
> > unregister (from experimentation that's as late as we can put it where
> > the GPU works after rebinding and we don't corrupt the system on unbind,
> > with the current hooks). You can guess how well gen3 likes that.
> > 
> > But I don't think the right answer is to skip the reset for gen3.
> > Suppose we enable context support for gen3, then the reset would be
> > required as well, and so we would still need the whole display
> > shenanigans to turn it off. Moving the modeset to turn the display off
> > to the end of userspace seems reasonable.
> 
> Yeah, just a bit odd to have the same call twice in the
> sequence. Can we remove the second call at least?

I think we can, but I am sufficiently paranoid to leave it.
I presume if it is a no-op, it will return without touching HW?
 
> Also a bit annoying the unload sequence no longer matches the
> suspend sequence. Well, I guess it was never 100% anyway but
> I think it was a bit closer before this patch. But the whole
> thing is rather messy anyway so I guess t's not significantly
> worse after this.

Yes, I feel things have been thrown into a bit of disarray by
haphazardly fixing unbind.

The last* remaining fly in the ointment is rebinding iommu. Once we have
that solid (and the system stops randomly eating itself 1-10 minutes
after the test passes), we should be in a much better spot to safely
remove duplication and refine the flow.

* that I am aware of.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Disable outputs during unregister

[Ville Syrjälä]

On Fri, Dec 04, 2020 at 04:14:05PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2020-12-04 16:01:11)
> > On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote:
> > > Quoting Ville Syrjälä (2020-12-01 16:05:17)
> > > > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote:
> > > > > Switch off the scanout during driver unregister, so we can shutdown the
> > > > > HW immediately for unbind.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index 320856b665a1..62d188e5cb8d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > > > >        * events.
> > > > >        */
> > > > >       drm_kms_helper_poll_fini(&dev_priv->drm);
> > > > > +     drm_atomic_helper_shutdown(&dev_priv->drm);
> > > > 
> > > > Looks like we already have this in remove(). Is that too late?
> > > 
> > > For the operations we do during unbind, yes.
> > > 
> > > For the core_hotplug/rebind dance, we have to reset the GPU while we
> > > still have runtime-pm operational and have pushed the reset to
> > > unregister (from experimentation that's as late as we can put it where
> > > the GPU works after rebinding and we don't corrupt the system on unbind,
> > > with the current hooks). You can guess how well gen3 likes that.
> > > 
> > > But I don't think the right answer is to skip the reset for gen3.
> > > Suppose we enable context support for gen3, then the reset would be
> > > required as well, and so we would still need the whole display
> > > shenanigans to turn it off. Moving the modeset to turn the display off
> > > to the end of userspace seems reasonable.
> > 
> > Yeah, just a bit odd to have the same call twice in the
> > sequence. Can we remove the second call at least?
> 
> I think we can, but I am sufficiently paranoid to leave it.
> I presume if it is a no-op, it will return without touching HW?

One can hope at least.

>  
> > Also a bit annoying the unload sequence no longer matches the
> > suspend sequence. Well, I guess it was never 100% anyway but
> > I think it was a bit closer before this patch. But the whole
> > thing is rather messy anyway so I guess t's not significantly
> > worse after this.
> 
> Yes, I feel things have been thrown into a bit of disarray by
> haphazardly fixing unbind.
> 
> The last* remaining fly in the ointment is rebinding iommu. Once we have
> that solid (and the system stops randomly eating itself 1-10 minutes
> after the test passes), we should be in a much better spot to safely
> remove duplication and refine the flow.
> 
> * that I am aware of.
> -Chris

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