[PATCH] drm/i915: Delegate our irq handler to a thread

[PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Moving our primary irq handler to a RT thread incurs an extra 1us delay
in process interrupts. This is most notice in waking up client threads,
where it adds about 20% of extra latency. It also imposes a delay in
feeding the GPU, an extra 1us before signaling secondary engines and
extra latency in resubmitting work to keep the GPU busy. The latter case
is insignificant as the latency hidden by the active GPU, and
preempt-to-busy ensures that no extra latency is incurred for
preemption.

The benefit is that we reduced the impact on the rest of the system, the
cycletest shows a reduction from 5us mean latency to 2us, with the
maximum observed latency (in a 2 minute window) reduced by over 160us.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Clark Williams <williams@redhat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Note this should need the fixes in
20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
test vehicle to exercise that patch!
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc83f094065a..f3df7714a3f3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
 
 	intel_irq_reset(dev_priv);
 
-	ret = request_irq(irq, intel_irq_handler(dev_priv),
-			  IRQF_SHARED, DRIVER_NAME, dev_priv);
+	ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
+				   IRQF_SHARED, DRIVER_NAME, dev_priv);
 	if (ret < 0) {
 		dev_priv->drm.irq_enabled = false;
 		return ret;
-- 
2.23.0

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Tvrtko Ursulin]

On 26/09/2019 15:25, Chris Wilson wrote:
> Moving our primary irq handler to a RT thread incurs an extra 1us delay
> in process interrupts. This is most notice in waking up client threads,
> where it adds about 20% of extra latency. It also imposes a delay in
> feeding the GPU, an extra 1us before signaling secondary engines and
> extra latency in resubmitting work to keep the GPU busy. The latter case
> is insignificant as the latency hidden by the active GPU, and
> preempt-to-busy ensures that no extra latency is incurred for
> preemption.
> 
> The benefit is that we reduced the impact on the rest of the system, the
> cycletest shows a reduction from 5us mean latency to 2us, with the
> maximum observed latency (in a 2 minute window) reduced by over 160us.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Note this should need the fixes in
> 20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
> test vehicle to exercise that patch!
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc83f094065a..f3df7714a3f3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
>   
>   	intel_irq_reset(dev_priv);
>   
> -	ret = request_irq(irq, intel_irq_handler(dev_priv),
> -			  IRQF_SHARED, DRIVER_NAME, dev_priv);
> +	ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> +				   IRQF_SHARED, DRIVER_NAME, dev_priv);
>   	if (ret < 0) {
>   		dev_priv->drm.irq_enabled = false;
>   		return ret;
> 

Two questions:

1. Should we split out the master_ctl handling into the fast handler? 
Although can we pass anything from the fast to threaded handler? If not 
we'd have to re-read the master_ctl from the threaded handler.

2. What about our tasklets - with threaded irqs we don't need them any 
more, right? So in this case they just add additional latency.

Regards,

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-26 15:57:07)
> 
> On 26/09/2019 15:25, Chris Wilson wrote:
> > Moving our primary irq handler to a RT thread incurs an extra 1us delay
> > in process interrupts. This is most notice in waking up client threads,
> > where it adds about 20% of extra latency. It also imposes a delay in
> > feeding the GPU, an extra 1us before signaling secondary engines and
> > extra latency in resubmitting work to keep the GPU busy. The latter case
> > is insignificant as the latency hidden by the active GPU, and
> > preempt-to-busy ensures that no extra latency is incurred for
> > preemption.
> > 
> > The benefit is that we reduced the impact on the rest of the system, the
> > cycletest shows a reduction from 5us mean latency to 2us, with the
> > maximum observed latency (in a 2 minute window) reduced by over 160us.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Clark Williams <williams@redhat.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > Note this should need the fixes in
> > 20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
> > test vehicle to exercise that patch!
> > ---
> >   drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc83f094065a..f3df7714a3f3 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> >   
> >       intel_irq_reset(dev_priv);
> >   
> > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> >       if (ret < 0) {
> >               dev_priv->drm.irq_enabled = false;
> >               return ret;
> > 
> 
> Two questions:
> 
> 1. Should we split out the master_ctl handling into the fast handler? 
> Although can we pass anything from the fast to threaded handler? If not 
> we'd have to re-read the master_ctl from the threaded handler.

I did look at using the primary/threaded irq handler to do some
fast/slow handling (but RT is probably going to force the primary into a
thread, so long term prospect there looked bleak ;)

In our pre-gen11 model we could certainly do the ack in the fast
primary handler, pushing the processing into the secondary thread. Post
gen11 is more annoying and we probably only do the master_ctl
immediately. (It's very tempting to put GT handler into fast, but that
defeats the PREEMPT_RT objections ;)

We can accumulate into i915 anything we want to pass from primary to
secondary. Or we can use per-cpu handlers and data structures.

> 2. What about our tasklets - with threaded irqs we don't need them any 
> more, right? So in this case they just add additional latency.

Our tasklets run immediately within the RT thread. That appears to be a
positive outcome of this. We keep our direct submission so our initial
latency is not impacted at all, and the impact of the thread latency is
minimised by keeping the GPU busy. So the only (easily?) measurable
impact is client wakeup. (I have a modicum of concern that there is a
priority inversion here for an RT client waiting on the GPU.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Sebastian Andrzej Siewior]

On 2019-09-26 15:25:38 [+0100], Chris Wilson wrote:
> Moving our primary irq handler to a RT thread incurs an extra 1us delay
> in process interrupts. This is most notice in waking up client threads,
> where it adds about 20% of extra latency. It also imposes a delay in
> feeding the GPU, an extra 1us before signaling secondary engines and
> extra latency in resubmitting work to keep the GPU busy. The latter case
> is insignificant as the latency hidden by the active GPU, and
> preempt-to-busy ensures that no extra latency is incurred for
> preemption.
> 
> The benefit is that we reduced the impact on the rest of the system, the
> cycletest shows a reduction from 5us mean latency to 2us, with the
> maximum observed latency (in a 2 minute window) reduced by over 160us.

cycletest is the name of the tool I don't know about, right? It is not
cyclictest with a typo?
I don't know how many interrupts you have in a system but if this is the
only one for the card then you could remove _irqsave() from locking if
the lock is never observed in IRQ context (but then you have irqwork
which requires this irqsave).

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Note this should need the fixes in
> 20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
> test vehicle to exercise that patch!
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc83f094065a..f3df7714a3f3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
>  
>  	intel_irq_reset(dev_priv);
>  
> -	ret = request_irq(irq, intel_irq_handler(dev_priv),
> -			  IRQF_SHARED, DRIVER_NAME, dev_priv);
> +	ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> +				   IRQF_SHARED, DRIVER_NAME, dev_priv);

I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
keep interrupting the CPU and you never manage to switch to the thread.

>  	if (ret < 0) {
>  		dev_priv->drm.irq_enabled = false;
>  		return ret;

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Sebastian Andrzej Siewior]

On 2019-09-26 15:57:07 [+0100], Tvrtko Ursulin wrote:
> 2. What about our tasklets - with threaded irqs we don't need them any more,
> right? So in this case they just add additional latency.

If you enqueue / schedule tasklets from your threaded handler then this
will wake up ksoftirqd and perform the work there (based on my memory of
the code).

> Regards,
> 
> Tvrtko

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-09-26 16:13:08)
> On 2019-09-26 15:25:38 [+0100], Chris Wilson wrote:
> > Moving our primary irq handler to a RT thread incurs an extra 1us delay
> > in process interrupts. This is most notice in waking up client threads,
> > where it adds about 20% of extra latency. It also imposes a delay in
> > feeding the GPU, an extra 1us before signaling secondary engines and
> > extra latency in resubmitting work to keep the GPU busy. The latter case
> > is insignificant as the latency hidden by the active GPU, and
> > preempt-to-busy ensures that no extra latency is incurred for
> > preemption.
> > 
> > The benefit is that we reduced the impact on the rest of the system, the
> > cycletest shows a reduction from 5us mean latency to 2us, with the
> > maximum observed latency (in a 2 minute window) reduced by over 160us.
> 
> cycletest is the name of the tool I don't know about, right? It is not
> cyclictest with a typo?

It's a tool like cyclictest that measures the timer latency in one
thread while flooding the system with GPU work, filling the page cache,
grabbing as many THP as possible etc. (It's actually called
gem_syslatency.)

> I don't know how many interrupts you have in a system but if this is the
> only one for the card then you could remove _irqsave() from locking if
> the lock is never observed in IRQ context (but then you have irqwork
> which requires this irqsave).

True, pushing the irq into a thread should allow us to re-evaluate a lot
of our spin_lock_irq.

> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Clark Williams <williams@redhat.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > Note this should need the fixes in
> > 20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
> > test vehicle to exercise that patch!
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc83f094065a..f3df7714a3f3 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> >  
> >       intel_irq_reset(dev_priv);
> >  
> > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> 
> I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
> keep interrupting the CPU and you never manage to switch to the thread.

The interrupts only keep coming if we feed the GPU, and we only feed the
GPU if we service the interrupt. That should provide a natural
quiescence :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Delegate our irq handler to a thread

[Patchwork]

== Series Details ==

Series: drm/i915: Delegate our irq handler to a thread
URL   : https://patchwork.freedesktop.org/series/67294/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6963 -> Patchwork_14557
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_busy@busy-all:
    - fi-kbl-guc:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-kbl-guc/igt@gem_busy@busy-all.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-guc/igt@gem_busy@busy-all.html
    - fi-icl-u2:          [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-icl-u2/igt@gem_busy@busy-all.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-icl-u2/igt@gem_busy@busy-all.html
    - fi-cfl-8700k:       [PASS][5] -> [DMESG-WARN][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-cfl-8700k/igt@gem_busy@busy-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-8700k/igt@gem_busy@busy-all.html
    - fi-skl-guc:         [PASS][7] -> [DMESG-WARN][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-guc/igt@gem_busy@busy-all.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-guc/igt@gem_busy@busy-all.html
    - fi-cfl-guc:         [PASS][9] -> [DMESG-WARN][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-cfl-guc/igt@gem_busy@busy-all.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-guc/igt@gem_busy@busy-all.html
    - fi-icl-u3:          NOTRUN -> [DMESG-WARN][11]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-icl-u3/igt@gem_busy@busy-all.html
    - fi-kbl-r:           [PASS][12] -> [DMESG-WARN][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-kbl-r/igt@gem_busy@busy-all.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-r/igt@gem_busy@busy-all.html
    - fi-kbl-x1275:       [PASS][14] -> [DMESG-WARN][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-kbl-x1275/igt@gem_busy@busy-all.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-x1275/igt@gem_busy@busy-all.html
    - fi-skl-6600u:       [PASS][16] -> [DMESG-WARN][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-6600u/igt@gem_busy@busy-all.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-6600u/igt@gem_busy@busy-all.html
    - fi-whl-u:           [PASS][18] -> [DMESG-WARN][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-whl-u/igt@gem_busy@busy-all.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-whl-u/igt@gem_busy@busy-all.html
    - fi-byt-j1900:       [PASS][20] -> [DMESG-WARN][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-byt-j1900/igt@gem_busy@busy-all.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-byt-j1900/igt@gem_busy@busy-all.html
    - fi-cfl-8109u:       [PASS][22] -> [DMESG-WARN][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-cfl-8109u/igt@gem_busy@busy-all.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-8109u/igt@gem_busy@busy-all.html

  * igt@gem_close_race@basic-process:
    - fi-ivb-3770:        [PASS][24] -> [DMESG-WARN][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-ivb-3770/igt@gem_close_race@basic-process.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-ivb-3770/igt@gem_close_race@basic-process.html
    - fi-cml-u2:          [PASS][26] -> [DMESG-WARN][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-cml-u2/igt@gem_close_race@basic-process.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cml-u2/igt@gem_close_race@basic-process.html
    - fi-hsw-4770:        [PASS][28] -> [DMESG-WARN][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-hsw-4770/igt@gem_close_race@basic-process.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-hsw-4770/igt@gem_close_race@basic-process.html
    - fi-skl-6700k2:      [PASS][30] -> [DMESG-WARN][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-6700k2/igt@gem_close_race@basic-process.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-6700k2/igt@gem_close_race@basic-process.html
    - fi-snb-2600:        [PASS][32] -> [DMESG-WARN][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-snb-2600/igt@gem_close_race@basic-process.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-snb-2600/igt@gem_close_race@basic-process.html
    - fi-byt-n2820:       [PASS][34] -> [DMESG-WARN][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-byt-n2820/igt@gem_close_race@basic-process.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-byt-n2820/igt@gem_close_race@basic-process.html
    - fi-hsw-4770r:       NOTRUN -> [DMESG-WARN][36]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-hsw-4770r/igt@gem_close_race@basic-process.html
    - fi-snb-2520m:       [PASS][37] -> [DMESG-WARN][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-snb-2520m/igt@gem_close_race@basic-process.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-snb-2520m/igt@gem_close_race@basic-process.html

  * igt@gem_close_race@basic-threads:
    - fi-bsw-kefka:       [PASS][39] -> [DMESG-WARN][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bsw-kefka/igt@gem_close_race@basic-threads.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bsw-kefka/igt@gem_close_race@basic-threads.html

  * igt@gem_cpu_reloc@basic:
    - fi-skl-lmem:        [PASS][41] -> [DMESG-WARN][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-lmem/igt@gem_cpu_reloc@basic.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-lmem/igt@gem_cpu_reloc@basic.html
    - fi-skl-6770hq:      [PASS][43] -> [DMESG-WARN][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-6770hq/igt@gem_cpu_reloc@basic.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-6770hq/igt@gem_cpu_reloc@basic.html
    - fi-bxt-dsi:         [PASS][45] -> [DMESG-WARN][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bxt-dsi/igt@gem_cpu_reloc@basic.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bxt-dsi/igt@gem_cpu_reloc@basic.html
    - fi-kbl-8809g:       [PASS][47] -> [DMESG-WARN][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-kbl-8809g/igt@gem_cpu_reloc@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-8809g/igt@gem_cpu_reloc@basic.html
    - fi-kbl-7500u:       [PASS][49] -> [DMESG-WARN][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-kbl-7500u/igt@gem_cpu_reloc@basic.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-7500u/igt@gem_cpu_reloc@basic.html
    - fi-glk-dsi:         [PASS][51] -> [DMESG-WARN][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-glk-dsi/igt@gem_cpu_reloc@basic.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-glk-dsi/igt@gem_cpu_reloc@basic.html
    - fi-skl-iommu:       [PASS][53] -> [DMESG-WARN][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-iommu/igt@gem_cpu_reloc@basic.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-iommu/igt@gem_cpu_reloc@basic.html

  * igt@gem_ctx_create@basic-files:
    - fi-bsw-n3050:       [PASS][55] -> [DMESG-WARN][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bsw-n3050/igt@gem_ctx_create@basic-files.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bsw-n3050/igt@gem_ctx_create@basic-files.html
    - fi-bdw-5557u:       [PASS][57] -> [DMESG-WARN][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bdw-5557u/igt@gem_ctx_create@basic-files.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bdw-5557u/igt@gem_ctx_create@basic-files.html

  * igt@gem_ctx_exec@basic:
    - fi-apl-guc:         [PASS][59] -> [DMESG-WARN][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-apl-guc/igt@gem_ctx_exec@basic.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-apl-guc/igt@gem_ctx_exec@basic.html

  * igt@gem_ctx_switch@rcs0:
    - fi-elk-e7500:       [PASS][61] -> [TIMEOUT][62] +9 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-elk-e7500/igt@gem_ctx_switch@rcs0.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-elk-e7500/igt@gem_ctx_switch@rcs0.html
    - fi-skl-6260u:       [PASS][63] -> [DMESG-WARN][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-6260u/igt@gem_ctx_switch@rcs0.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-6260u/igt@gem_ctx_switch@rcs0.html

  * igt@gem_exec_basic@basic-all:
    - fi-bdw-gvtdvm:      [PASS][65] -> [DMESG-WARN][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bdw-gvtdvm/igt@gem_exec_basic@basic-all.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bdw-gvtdvm/igt@gem_exec_basic@basic-all.html

  * igt@gem_exec_fence@basic-busy-default:
    - fi-ilk-650:         [PASS][67] -> [DMESG-WARN][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-ilk-650/igt@gem_exec_fence@basic-busy-default.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-ilk-650/igt@gem_exec_fence@basic-busy-default.html

  * igt@gem_exec_store@basic-all:
    - fi-pnv-d510:        [PASS][69] -> [TIMEOUT][70] +4 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-pnv-d510/igt@gem_exec_store@basic-all.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-pnv-d510/igt@gem_exec_store@basic-all.html

  * igt@gem_wait@basic-busy-all:
    - fi-blb-e6850:       [PASS][71] -> [TIMEOUT][72] +7 similar issues
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-blb-e6850/igt@gem_wait@basic-busy-all.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-blb-e6850/igt@gem_wait@basic-busy-all.html
    - fi-bwr-2160:        [PASS][73] -> [TIMEOUT][74] +5 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bwr-2160/igt@gem_wait@basic-busy-all.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bwr-2160/igt@gem_wait@basic-busy-all.html

  * igt@gem_wait@basic-wait-all:
    - fi-gdg-551:         [PASS][75] -> [TIMEOUT][76] +4 similar issues
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-gdg-551/igt@gem_wait@basic-wait-all.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-gdg-551/igt@gem_wait@basic-wait-all.html

  * igt@i915_selftest@live_requests:
    - fi-skl-6260u:       [PASS][77] -> [INCOMPLETE][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-skl-6260u/igt@i915_selftest@live_requests.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-skl-6260u/igt@i915_selftest@live_requests.html

  * igt@runner@aborted:
    - fi-bdw-gvtdvm:      NOTRUN -> [FAIL][79]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bdw-gvtdvm/igt@runner@aborted.html
    - fi-cfl-8109u:       NOTRUN -> [FAIL][80]
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-8109u/igt@runner@aborted.html
    - fi-kbl-7500u:       NOTRUN -> [FAIL][81]
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-7500u/igt@runner@aborted.html
    - fi-whl-u:           NOTRUN -> [FAIL][82]
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-whl-u/igt@runner@aborted.html
    - fi-cml-u2:          NOTRUN -> [FAIL][83]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cml-u2/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][84]
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bxt-dsi/igt@runner@aborted.html
    - fi-cfl-guc:         NOTRUN -> [FAIL][85]
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-guc/igt@runner@aborted.html
    - fi-kbl-x1275:       NOTRUN -> [FAIL][86]
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-x1275/igt@runner@aborted.html
    - fi-cfl-8700k:       NOTRUN -> [FAIL][87]
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cfl-8700k/igt@runner@aborted.html
    - fi-kbl-8809g:       NOTRUN -> [FAIL][88]
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-8809g/igt@runner@aborted.html
    - fi-apl-guc:         NOTRUN -> [FAIL][89]
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-apl-guc/igt@runner@aborted.html
    - fi-kbl-r:           NOTRUN -> [FAIL][90]
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-r/igt@runner@aborted.html
    - fi-bdw-5557u:       NOTRUN -> [FAIL][91]
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bdw-5557u/igt@runner@aborted.html
    - fi-kbl-guc:         NOTRUN -> [FAIL][92]
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-guc/igt@runner@aborted.html

  
#### Warnings ####

  * igt@gem_ctx_switch@rcs0:
    - fi-blb-e6850:       [SKIP][93] ([fdo#109271]) -> [TIMEOUT][94] +1 similar issue
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-blb-e6850/igt@gem_ctx_switch@rcs0.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-blb-e6850/igt@gem_ctx_switch@rcs0.html
    - fi-pnv-d510:        [SKIP][95] ([fdo#109271]) -> [TIMEOUT][96] +1 similar issue
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-pnv-d510/igt@gem_ctx_switch@rcs0.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-pnv-d510/igt@gem_ctx_switch@rcs0.html
    - fi-gdg-551:         [SKIP][97] ([fdo#109271]) -> [TIMEOUT][98] +4 similar issues
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-gdg-551/igt@gem_ctx_switch@rcs0.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-gdg-551/igt@gem_ctx_switch@rcs0.html

  * igt@gem_exec_parallel@basic:
    - fi-bwr-2160:        [SKIP][99] ([fdo#109271]) -> [TIMEOUT][100] +2 similar issues
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bwr-2160/igt@gem_exec_parallel@basic.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bwr-2160/igt@gem_exec_parallel@basic.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_busy@busy-all:
    - {fi-icl-guc}:       [PASS][101] -> [DMESG-WARN][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-icl-guc/igt@gem_busy@busy-all.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-icl-guc/igt@gem_busy@busy-all.html
    - {fi-icl-u4}:        [PASS][103] -> [DMESG-WARN][104]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-icl-u4/igt@gem_busy@busy-all.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-icl-u4/igt@gem_busy@busy-all.html
    - {fi-icl-dsi}:       [PASS][105] -> [DMESG-WARN][106]
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-icl-dsi/igt@gem_busy@busy-all.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-icl-dsi/igt@gem_busy@busy-all.html
    - {fi-tgl-u2}:        [PASS][107] -> [DMESG-WARN][108]
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-tgl-u2/igt@gem_busy@busy-all.html
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-tgl-u2/igt@gem_busy@busy-all.html

  * igt@gem_cpu_reloc@basic:
    - {fi-cml-s}:         [PASS][109] -> [DMESG-WARN][110]
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-cml-s/igt@gem_cpu_reloc@basic.html
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cml-s/igt@gem_cpu_reloc@basic.html

  * igt@runner@aborted:
    - {fi-kbl-soraka}:    NOTRUN -> [FAIL][111]
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-kbl-soraka/igt@runner@aborted.html
    - {fi-cml-s}:         NOTRUN -> [FAIL][112]
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-cml-s/igt@runner@aborted.html
    - {fi-tgl-u2}:        NOTRUN -> [FAIL][113]
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-tgl-u2/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_cpu_reloc@basic:
    - fi-elk-e7500:       [PASS][114] -> [SKIP][115] ([fdo#109271]) +89 similar issues
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-elk-e7500/igt@gem_cpu_reloc@basic.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-elk-e7500/igt@gem_cpu_reloc@basic.html

  * igt@gem_ctx_switch@legacy-render:
    - fi-elk-e7500:       [PASS][116] -> [TIMEOUT][117] ([fdo#111381])
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-elk-e7500/igt@gem_ctx_switch@legacy-render.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-elk-e7500/igt@gem_ctx_switch@legacy-render.html
    - fi-bwr-2160:        [PASS][118] -> [TIMEOUT][119] ([fdo#111381])
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bwr-2160/igt@gem_ctx_switch@legacy-render.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bwr-2160/igt@gem_ctx_switch@legacy-render.html

  * igt@gem_exec_reloc@basic-cpu:
    - fi-pnv-d510:        [PASS][120] -> [SKIP][121] ([fdo#109271]) +84 similar issues
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-pnv-d510/igt@gem_exec_reloc@basic-cpu.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-pnv-d510/igt@gem_exec_reloc@basic-cpu.html

  * igt@gem_flink_basic@basic:
    - fi-gdg-551:         [PASS][122] -> [SKIP][123] ([fdo#109271]) +69 similar issues
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-gdg-551/igt@gem_flink_basic@basic.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-gdg-551/igt@gem_flink_basic@basic.html

  * igt@gem_mmap_gtt@basic-write-gtt:
    - fi-blb-e6850:       [PASS][124] -> [SKIP][125] ([fdo#109271]) +86 similar issues
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-blb-e6850/igt@gem_mmap_gtt@basic-write-gtt.html
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-blb-e6850/igt@gem_mmap_gtt@basic-write-gtt.html

  * igt@gem_mmap_gtt@basic-write-read:
    - fi-bwr-2160:        [PASS][126] -> [SKIP][127] ([fdo#109271]) +74 similar issues
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6963/fi-bwr-2160/igt@gem_mmap_gtt@basic-write-read.html
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14557/fi-bwr-2160/igt@gem_mmap_gtt@basic-write-read.html

  
#### Warnings ####

  * igt@gem_ctx_switch@legacy-render:
    - fi-pnv-d510:        [SKIP][128] ([fdo#109271] / [fdo#111381]) -> [TIMEOUT][129] ([fdo#111381])
   [128]:

== Logs ==

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Sebastian Andrzej Siewior]

On 2019-09-26 16:24:59 [+0100], Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index bc83f094065a..f3df7714a3f3 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> > >  
> > >       intel_irq_reset(dev_priv);
> > >  
> > > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> > 
> > I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
> > keep interrupting the CPU and you never manage to switch to the thread.
> 
> The interrupts only keep coming if we feed the GPU, and we only feed the
> GPU if we service the interrupt. That should provide a natural
> quiescence :)

In IRQ-context your primary handler gets invoked which wakes the thread
(what ever intel_irq_handler() returns). Then you leave the IRQ context
and should switch to your IRQ-handler. This will never happen because
the IRQ line remains asserted and CPU ends up in the primary handler
again.
An EDGE typed IRQ wouldn't notice a difference. But a LINE typed IRQ
will remain asserted until the hardware de-asserts the interrupt again.
Since you never reach your thread handler, I don't see how this can
happen.

> -Chris

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-09-26 16:32:52)
> On 2019-09-26 16:24:59 [+0100], Chris Wilson wrote:
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > index bc83f094065a..f3df7714a3f3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> > > >  
> > > >       intel_irq_reset(dev_priv);
> > > >  
> > > > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > > > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > > > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > > > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> > > 
> > > I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
> > > keep interrupting the CPU and you never manage to switch to the thread.
> > 
> > The interrupts only keep coming if we feed the GPU, and we only feed the
> > GPU if we service the interrupt. That should provide a natural
> > quiescence :)
> 
> In IRQ-context your primary handler gets invoked which wakes the thread
> (what ever intel_irq_handler() returns). Then you leave the IRQ context
> and should switch to your IRQ-handler. This will never happen because
> the IRQ line remains asserted and CPU ends up in the primary handler
> again.
> An EDGE typed IRQ wouldn't notice a difference. But a LINE typed IRQ
> will remain asserted until the hardware de-asserts the interrupt again.
> Since you never reach your thread handler, I don't see how this can
> happen.

It's all edge interrupts -- although for gen2/3 my memory is hazy. But
the GPU (circa gen6) can generate more than enough interrupts to saturate
a CPU.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Chris Wilson (2019-09-26 16:40:34)
> Quoting Sebastian Andrzej Siewior (2019-09-26 16:32:52)
> > On 2019-09-26 16:24:59 [+0100], Chris Wilson wrote:
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index bc83f094065a..f3df7714a3f3 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> > > > >  
> > > > >       intel_irq_reset(dev_priv);
> > > > >  
> > > > > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > > > > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > > > > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > > > > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> > > > 
> > > > I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
> > > > keep interrupting the CPU and you never manage to switch to the thread.
> > > 
> > > The interrupts only keep coming if we feed the GPU, and we only feed the
> > > GPU if we service the interrupt. That should provide a natural
> > > quiescence :)
> > 
> > In IRQ-context your primary handler gets invoked which wakes the thread
> > (what ever intel_irq_handler() returns). Then you leave the IRQ context
> > and should switch to your IRQ-handler. This will never happen because
> > the IRQ line remains asserted and CPU ends up in the primary handler
> > again.
> > An EDGE typed IRQ wouldn't notice a difference. But a LINE typed IRQ
> > will remain asserted until the hardware de-asserts the interrupt again.
> > Since you never reach your thread handler, I don't see how this can
> > happen.
> 
> It's all edge interrupts -- although for gen2/3 my memory is hazy. But
> the GPU (circa gen6) can generate more than enough interrupts to saturate
> a CPU.

So everything older than gen5 has MSI disabled it appears and needs
ONESHOT.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Sebastian Andrzej Siewior]

On 2019-09-26 16:40:34 [+0100], Chris Wilson wrote:
> 
> It's all edge interrupts -- although for gen2/3 my memory is hazy. But
> the GPU (circa gen6) can generate more than enough interrupts to saturate
> a CPU.

:)

> -Chris

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

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Brian Welty]

On 9/26/2019 7:25 AM, Chris Wilson wrote:
> Moving our primary irq handler to a RT thread incurs an extra 1us delay
> in process interrupts. This is most notice in waking up client threads,
> where it adds about 20% of extra latency. It also imposes a delay in
> feeding the GPU, an extra 1us before signaling secondary engines and
> extra latency in resubmitting work to keep the GPU busy. The latter case
> is insignificant as the latency hidden by the active GPU, and
> preempt-to-busy ensures that no extra latency is incurred for
> preemption.
> 
> The benefit is that we reduced the impact on the rest of the system, the
> cycletest shows a reduction from 5us mean latency to 2us, with the
> maximum observed latency (in a 2 minute window) reduced by over 160us.

Hi Chris,

I'm curious to understand this a little better.
If only benefit of this patch is improving overall system performance, then
can you say why i915 interrupt handling doesn't fit into what I thought was
the common usage of threaded interrupts...

Usually with request_threaded_irq(), I thought typically both handlers
are specified and so you'd only fallback to the threaded handler when the
interrupt context handler is overwhelmed?
Like so:
	while (HW events need some action) {
		.... do something ...
		if (too overwhelmed)  /* ie. reduce load on system */
			return IRQ_WAKE_THREAD;
	}
	return IRQ_HANDLED;

Likely you considered something like above.
I'm just looking to understand why using only threaded interrupt handler is best in this case.
(Also maybe discuss this a little bit further in commit message...?)

Sorry if this was perhaps discussed in response to Tvrtko's question.
I didn't quite follow the fast/slow separation mentioned in that thread.

Thanks,
-Brian


> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Note this should need the fixes in
> 20190926105644.16703-2-bigeasy@linutronix.de, by itself this should be a
> test vehicle to exercise that patch!
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc83f094065a..f3df7714a3f3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
>  
>  	intel_irq_reset(dev_priv);
>  
> -	ret = request_irq(irq, intel_irq_handler(dev_priv),
> -			  IRQF_SHARED, DRIVER_NAME, dev_priv);
> +	ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> +				   IRQF_SHARED, DRIVER_NAME, dev_priv);
>  	if (ret < 0) {
>  		dev_priv->drm.irq_enabled = false;
>  		return ret;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Brian Welty (2019-09-26 19:57:17)
> 
> On 9/26/2019 7:25 AM, Chris Wilson wrote:
> > Moving our primary irq handler to a RT thread incurs an extra 1us delay
> > in process interrupts. This is most notice in waking up client threads,
> > where it adds about 20% of extra latency. It also imposes a delay in
> > feeding the GPU, an extra 1us before signaling secondary engines and
> > extra latency in resubmitting work to keep the GPU busy. The latter case
> > is insignificant as the latency hidden by the active GPU, and
> > preempt-to-busy ensures that no extra latency is incurred for
> > preemption.
> > 
> > The benefit is that we reduced the impact on the rest of the system, the
> > cycletest shows a reduction from 5us mean latency to 2us, with the
> > maximum observed latency (in a 2 minute window) reduced by over 160us.
> 
> Hi Chris,
> 
> I'm curious to understand this a little better.
> If only benefit of this patch is improving overall system performance, then
> can you say why i915 interrupt handling doesn't fit into what I thought was
> the common usage of threaded interrupts...

The way cyclictest determines system impact is via latency in delivery
of a timer to userspace; it observes irq-off time. We use that as a
convenient metric to determine how long we block the system while
servicing the gpu.
 
> Usually with request_threaded_irq(), I thought typically both handlers
> are specified and so you'd only fallback to the threaded handler when the
> interrupt context handler is overwhelmed?
> Like so:
>         while (HW events need some action) {
>                 .... do something ...
>                 if (too overwhelmed)  /* ie. reduce load on system */
>                         return IRQ_WAKE_THREAD;
>         }
>         return IRQ_HANDLED;
> 
> Likely you considered something like above.
> I'm just looking to understand why using only threaded interrupt handler is best in this case.
> (Also maybe discuss this a little bit further in commit message...?)

Convenience. The primary motivation for the patch was to test
Sebastian's patches in our CI which are targeting PREEMPT_RT issues and
he noted that would also be reproduced in a threaded irq handler.

The measurements I did were to assuage my own gut response that a
threaded irq handler would be devastating to our latency metrics
(although a 1us extra latency is terrible if our goal happens to be
sub-1us gpu->client latency!). (I was contemplating adding IRQF_NO_THREAD
to avoid the issue.)

The problem I feel with the overflow of primary to thread is what value
do you decide is acceptable latency for the rest of the system; and why
bother to kick off a second thread if all it does is service a tasklet,
would we be better served by a dedicated thread per-submission-thread?
(Which might sound familiar -- it what's we used before tasklets.)

It's definitely a tradeoff; our latency versus the rest of the system.
Though there were a few advantages to having an RT thread spawned for
the softirq (which I guess is was because RT threads are granted a
frequency boost), and if we can reduce the number of irq-off spinlocks
that may have much greater advantages across the driver that outweigh
the cost of the extra context switch in client wakeup.
 
> Sorry if this was perhaps discussed in response to Tvrtko's question.
> I didn't quite follow the fast/slow separation mentioned in that thread.

Our interrupt handlers are effectively split into 2 phases. First we read
the master control and various IIR, and ack the interrupt, after which
the GPU is free to raise an interrupt again. Then we process the
interrupt, handling the various bits asserted in the IIR (signaling
clients, feeding the GPU its next context, completing vblanks etc).
The first ack should be fast -- you can demonstrate the impact of not
doing the ack/handler split, by measuring the throughput on multiple
engines (a context-switch event on the second engine will be noticeably
blocked by the interrupt handler on the first, if you are not careful).
(gen11+ breaks this split atm as we haven't worked out how we are meant
to ack the banked GT IIR yet, or at least no one has seen Icelake to
measure the impact).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Chris Wilson (2019-09-26 15:25:38)
> Moving our primary irq handler to a RT thread incurs an extra 1us delay
> in process interrupts. This is most notice in waking up client threads,
> where it adds about 20% of extra latency. It also imposes a delay in
> feeding the GPU, an extra 1us before signaling secondary engines and
> extra latency in resubmitting work to keep the GPU busy. The latter case
> is insignificant as the latency hidden by the active GPU, and
> preempt-to-busy ensures that no extra latency is incurred for
> preemption.

Fwiw, gem_wsim says the impact is around 1.25% for transcode (just
looking at the first hour or so of runs), which is enough to put the idea
on the back burner for now.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Delegate our irq handler to a thread

[Sebastian Andrzej Siewior]

On 2019-09-26 16:44:33 [+0100], Chris Wilson wrote:
> > It's all edge interrupts -- although for gen2/3 my memory is hazy. But
> > the GPU (circa gen6) can generate more than enough interrupts to saturate
> > a CPU.
> 
> So everything older than gen5 has MSI disabled it appears and needs
> ONESHOT.

Also ACPI/PCI-quirks may decide that MSI is broken on the system and
disable it.

If you end up with a shared handler, you can't mix ONESHOT among the
handlers. So either all have that flag set or none of them.
In that case you need to provide a tiny primary handler which just
disables the IRQ (in the HW) and the threaded handler has to enable it
again (at the end of its routine).

> -Chris

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