* [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
@ 2016-06-11 1:18 Paulo Zanoni
2016-06-11 6:56 ` ✗ Ro.CI.BAT: warning for " Patchwork
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Paulo Zanoni @ 2016-06-11 1:18 UTC (permalink / raw)
To: intel-gfx; +Cc: Stefan Richter, Paulo Zanoni, Steven Honeyman
Ever since I started working on FBC I was already aware that FBC can
really amplify the FIFO underrun symptoms. On systems where FIFO
underruns were harmless error messages, enabling FBC would cause the
underruns to give black screens.
We recently tried to enable FBC on Haswell and got reports of a system
that would hang after some hours of uptime, and the first bad commit
was the one that enabled FBC. We also observed that this system had
FIFO underrun error messages on its dmesg. Although we don't have any
evidence that fixing the underruns would solve the bug and make FBC
work properly on this machine, IMHO it's better if we minimize the
amount of possible problems by just giving up FBC whenever we detect
an underrun.
v2: new version, different implementation and commit message.
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Lyude <cpaul@redhat.com>
Cc: Steven Honeyman <stevenhoneyman@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 3 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_fbc.c | 53 ++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 ++
4 files changed, 59 insertions(+)
Since my test machines don't produce FIFO underrun errors, I tested this by
creating a debugfs file that just calls intel_fbc_handle_fifo_underrun(). I'd
appreciate some Tested-by tags, if possible.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20a676d..18b4257 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -908,6 +908,9 @@ struct intel_fbc {
bool enabled;
bool active;
+ bool underrun_detected;
+ struct work_struct underrun_work;
+
struct intel_fbc_state_cache {
struct {
unsigned int mode_flags;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ebe7b34..7bf97b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1436,6 +1436,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
void intel_fbc_flush(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits, enum fb_op_origin origin);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
+void intel_fbc_handle_fifo_underrun(struct drm_i915_private *dev_priv);
/* intel_hdmi.c */
void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d268f76..2363bff 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -755,6 +755,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
struct intel_fbc *fbc = &dev_priv->fbc;
struct intel_fbc_state_cache *cache = &fbc->state_cache;
+ /* We don't need to use a state cache here since this information is
+ * global for every CRTC. */
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (!cache->plane.visible) {
fbc->no_fbc_reason = "primary plane not visible";
return false;
@@ -1195,6 +1202,51 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
cancel_work_sync(&fbc->work.work);
}
+static void intel_fbc_underrun_work_fn(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, struct drm_i915_private, fbc.underrun_work);
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ mutex_lock(&fbc->lock);
+
+ /* Maybe we were scheduled twice. */
+ if (fbc->underrun_detected)
+ goto out;
+
+ DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
+ fbc->underrun_detected = true;
+
+ intel_fbc_deactivate(dev_priv);
+out:
+ mutex_unlock(&fbc->lock);
+}
+
+/**
+ * intel_fbc_handle_fifo_underrun - disable FBC when we get a FIFO underrun
+ * @dev_priv: i915 device instance
+ *
+ * Without FBC, most underruns are harmless and don't really cause too many
+ * problems, except for an annoying message on dmesg. With FBC, underruns can
+ * become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable FBC
+ * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
+ * already suggests that watermarks may be bad, so try to be as safe as
+ * possible.
+ */
+void intel_fbc_handle_fifo_underrun(struct drm_i915_private *dev_priv)
+{
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ if (!fbc_supported(dev_priv))
+ return;
+
+ if (fbc->underrun_detected)
+ return;
+
+ schedule_work(&fbc->underrun_work);
+}
+
/**
* intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
* @dev_priv: i915 device instance
@@ -1250,6 +1302,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
enum pipe pipe;
INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
+ INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
mutex_init(&fbc->lock);
fbc->enabled = false;
fbc->active = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 9be839a..9d43cbd 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -370,6 +370,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
DRM_ERROR("CPU pipe %c FIFO underrun\n",
pipe_name(pipe));
+
+ intel_fbc_handle_fifo_underrun(dev_priv);
}
/**
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* ✗ Ro.CI.BAT: warning for drm/i915/fbc: disable FBC on FIFO underruns
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
@ 2016-06-11 6:56 ` Patchwork
2016-06-13 11:47 ` [PATCH] " Stefan Richter
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2016-06-11 6:56 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/fbc: disable FBC on FIFO underruns
URL : https://patchwork.freedesktop.org/series/8575/
State : warning
== Summary ==
Series 8575v1 drm/i915/fbc: disable FBC on FIFO underruns
http://patchwork.freedesktop.org/api/1.0/series/8575/revisions/1/mbox
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (ro-hsw-i3-4010u)
Subgroup nonblocking-crc-pipe-b-frame-sequence:
skip -> PASS (fi-skl-i5-6260u)
Subgroup suspend-read-crc-pipe-a:
skip -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup suspend-read-crc-pipe-c:
skip -> PASS (fi-skl-i5-6260u)
fi-bdw-i7-5557u total:213 pass:201 dwarn:0 dfail:0 fail:0 skip:12
fi-skl-i5-6260u total:213 pass:202 dwarn:0 dfail:0 fail:0 skip:11
fi-skl-i7-6700k total:213 pass:188 dwarn:0 dfail:0 fail:0 skip:25
fi-snb-i7-2600 total:213 pass:174 dwarn:0 dfail:0 fail:0 skip:39
ro-bdw-i5-5250u total:213 pass:197 dwarn:3 dfail:0 fail:0 skip:13
ro-bdw-i7-5557U total:213 pass:198 dwarn:0 dfail:0 fail:0 skip:15
ro-bdw-i7-5600u total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28
ro-bsw-n3050 total:213 pass:172 dwarn:0 dfail:0 fail:2 skip:39
ro-byt-n2820 total:213 pass:173 dwarn:0 dfail:0 fail:3 skip:37
ro-hsw-i3-4010u total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23
ro-hsw-i7-4770r total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23
ro-ilk-i7-620lm total:213 pass:150 dwarn:0 dfail:0 fail:1 skip:62
ro-ilk1-i5-650 total:208 pass:150 dwarn:0 dfail:0 fail:1 skip:57
ro-ivb-i7-3770 total:213 pass:181 dwarn:0 dfail:0 fail:0 skip:32
ro-ivb2-i7-3770 total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28
ro-skl3-i5-6260u total:213 pass:201 dwarn:1 dfail:0 fail:0 skip:11
ro-snb-i7-2620M total:213 pass:174 dwarn:0 dfail:0 fail:1 skip:38
fi-hsw-i7-4770k failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1165/
94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration manifest
04a81df drm/i915/fbc: disable FBC on FIFO underruns
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
2016-06-11 6:56 ` ✗ Ro.CI.BAT: warning for " Patchwork
@ 2016-06-13 11:47 ` Stefan Richter
2016-06-13 14:33 ` Zanoni, Paulo R
2016-08-12 23:24 ` Pandiyan, Dhinakaran
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Stefan Richter @ 2016-06-13 11:47 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Steven Honeyman
On Jun 10 Paulo Zanoni wrote:
> Since my test machines don't produce FIFO underrun errors, I tested this by
> creating a debugfs file that just calls intel_fbc_handle_fifo_underrun(). I'd
> appreciate some Tested-by tags, if possible.
Since May 8 I have been using 4.6.0-rc6 patched with
drm-intel-nightly_2016y-05m-06d-14h-29m-58s (from what I understand,
something close to what is now in mainline), and fbc disabled on the
kernel commandline. I have not rebooted since May 8.
For that kernel, i915.enable_fbc=0 is both required and sufficient to
prevent freezes at random points in time (as reported). The
drm-intel-nightly_2016y-05m-06d-14h-29m-58s patch on the other hand has the
effect that there are never any FIFO underruns logged anymore.
If there are no FIFO underruns reported in dmesg, your underrun
detection routine would never hit, would it?
If you nevertheless think it's worth trying, should I test it on top of
4.7-rc3 or on current drm-intel-nightly?
--
Stefan Richter
-======----- -==- -==-=
http://arcgraph.de/sr/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-06-13 11:47 ` [PATCH] " Stefan Richter
@ 2016-06-13 14:33 ` Zanoni, Paulo R
2016-06-15 12:19 ` Stefan Richter
0 siblings, 1 reply; 20+ messages in thread
From: Zanoni, Paulo R @ 2016-06-13 14:33 UTC (permalink / raw)
To: stefanr; +Cc: intel-gfx, stevenhoneyman
Em Seg, 2016-06-13 às 13:47 +0200, Stefan Richter escreveu:
> On Jun 10 Paulo Zanoni wrote:
> >
> > Since my test machines don't produce FIFO underrun errors, I tested
> > this by
> > creating a debugfs file that just calls
> > intel_fbc_handle_fifo_underrun(). I'd
> > appreciate some Tested-by tags, if possible.
> Since May 8 I have been using 4.6.0-rc6 patched with
> drm-intel-nightly_2016y-05m-06d-14h-29m-58s (from what I understand,
> something close to what is now in mainline), and fbc disabled on the
> kernel commandline. I have not rebooted since May 8.
Ok, so you're saying that if you boot this Kernel with i915.fbc=1, you
won't get any FIFO underrun messages, but the system is still going to
freeze somehow?
If that's the case, then this patch won't help.
>
> For that kernel, i915.enable_fbc=0 is both required and sufficient to
> prevent freezes at random points in time (as reported). The
> drm-intel-nightly_2016y-05m-06d-14h-29m-58s patch on the other hand
> has the
> effect that there are never any FIFO underruns logged anymore.
>
> If there are no FIFO underruns reported in dmesg, your underrun
> detection routine would never hit, would it?
You're right, there's no need to test on this case.
>
> If you nevertheless think it's worth trying, should I test it on top
> of
> 4.7-rc3 or on current drm-intel-nightly?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-06-13 14:33 ` Zanoni, Paulo R
@ 2016-06-15 12:19 ` Stefan Richter
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Richter @ 2016-06-15 12:19 UTC (permalink / raw)
To: Zanoni, Paulo R; +Cc: intel-gfx, stevenhoneyman
On Jun 13 Zanoni, Paulo R wrote:
> Em Seg, 2016-06-13 às 13:47 +0200, Stefan Richter escreveu:
> > On Jun 10 Paulo Zanoni wrote:
> > >
> > > Since my test machines don't produce FIFO underrun errors, I tested
> > > this by
> > > creating a debugfs file that just calls
> > > intel_fbc_handle_fifo_underrun(). I'd
> > > appreciate some Tested-by tags, if possible.
> > Since May 8 I have been using 4.6.0-rc6 patched with
> > drm-intel-nightly_2016y-05m-06d-14h-29m-58s (from what I understand,
> > something close to what is now in mainline), and fbc disabled on the
> > kernel commandline. I have not rebooted since May 8.
>
> Ok, so you're saying that if you boot this Kernel with i915.fbc=1, you
> won't get any FIFO underrun messages, but the system is still going to
> freeze somehow?
Yes.
> If that's the case, then this patch won't help.
Nevertheless I will try to test current mainline + your patch this week or
next week, just to be sure and to have another point of reference.
--
Stefan Richter
-======----- -==- -====
http://arcgraph.de/sr/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: drm/i915/fbc: disable FBC on FIFO underruns
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
2016-06-11 6:56 ` ✗ Ro.CI.BAT: warning for " Patchwork
2016-06-13 11:47 ` [PATCH] " Stefan Richter
@ 2016-08-12 23:24 ` Pandiyan, Dhinakaran
2016-08-13 9:05 ` Chris Wilson
2016-08-16 5:57 ` ✗ Ro.CI.BAT: failure for drm/i915/fbc: disable FBC on FIFO underruns (rev2) Patchwork
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2016-08-12 23:24 UTC (permalink / raw)
To: Zanoni, Paulo R; +Cc: intel-gfx, stefanr, stevenhoneyman
On Fri, 2016-06-10 at 22:18 -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
>
Do we know why we get black screens in this scenario?
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
>
> v2: new version, different implementation and commit message.
>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Lyude <cpaul@redhat.com>
> Cc: Steven Honeyman <stevenhoneyman@gmail.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_fbc.c | 53 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 ++
> 4 files changed, 59 insertions(+)
>
>
> Since my test machines don't produce FIFO underrun errors, I tested this by
> creating a debugfs file that just calls intel_fbc_handle_fifo_underrun(). I'd
> appreciate some Tested-by tags, if possible.
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20a676d..18b4257 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -908,6 +908,9 @@ struct intel_fbc {
> bool enabled;
> bool active;
>
> + bool underrun_detected;
> + struct work_struct underrun_work;
> +
> struct intel_fbc_state_cache {
> struct {
> unsigned int mode_flags;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ebe7b34..7bf97b1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1436,6 +1436,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun(struct drm_i915_private *dev_priv);
>
> /* intel_hdmi.c */
> void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d268f76..2363bff 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -755,6 +755,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> struct intel_fbc *fbc = &dev_priv->fbc;
> struct intel_fbc_state_cache *cache = &fbc->state_cache;
>
> + /* We don't need to use a state cache here since this information is
> + * global for every CRTC. */
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (!cache->plane.visible) {
> fbc->no_fbc_reason = "primary plane not visible";
> return false;
> @@ -1195,6 +1202,51 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
> cancel_work_sync(&fbc->work.work);
> }
>
> +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private, fbc.underrun_work);
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + mutex_lock(&fbc->lock);
> +
> + /* Maybe we were scheduled twice. */
> + if (fbc->underrun_detected)
> + goto out;
> +
> + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> + fbc->underrun_detected = true;
> +
> + intel_fbc_deactivate(dev_priv);
> +out:
> + mutex_unlock(&fbc->lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun - disable FBC when we get a FIFO underrun
> + * @dev_priv: i915 device instance
> + *
> + * Without FBC, most underruns are harmless and don't really cause too many
> + * problems, except for an annoying message on dmesg. With FBC, underruns can
> + * become black screens or even worse, especially when paired with bad
> + * watermarks. So in order for us to be on the safe side, completely disable FBC
> + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> + * already suggests that watermarks may be bad, so try to be as safe as
> + * possible.
> + */
> +void intel_fbc_handle_fifo_underrun(struct drm_i915_private *dev_priv)
> +{
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + if (!fbc_supported(dev_priv))
> + return;
> +
Should we bail out if fbc is not enabled?
Also, can we just disable fbc if we see an underrun instead of using a
new flag to prevent activation?
> + if (fbc->underrun_detected)
> + return;
> +
> + schedule_work(&fbc->underrun_work);
> +}
> +
> /**
> * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
> * @dev_priv: i915 device instance
> @@ -1250,6 +1302,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> enum pipe pipe;
>
> INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> + INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> mutex_init(&fbc->lock);
> fbc->enabled = false;
> fbc->active = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 9be839a..9d43cbd 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -370,6 +370,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> DRM_ERROR("CPU pipe %c FIFO underrun\n",
> pipe_name(pipe));
> +
> + intel_fbc_handle_fifo_underrun(dev_priv);
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: drm/i915/fbc: disable FBC on FIFO underruns
2016-08-12 23:24 ` Pandiyan, Dhinakaran
@ 2016-08-13 9:05 ` Chris Wilson
2016-08-15 20:49 ` Zanoni, Paulo R
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2016-08-13 9:05 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, stefanr, Zanoni, Paulo R, stevenhoneyman
On Fri, Aug 12, 2016 at 11:24:59PM +0000, Pandiyan, Dhinakaran wrote:
> > +/**
> > + * intel_fbc_handle_fifo_underrun - disable FBC when we get a FIFO underrun
> > + * @dev_priv: i915 device instance
> > + *
> > + * Without FBC, most underruns are harmless and don't really cause too many
> > + * problems, except for an annoying message on dmesg. With FBC, underruns can
> > + * become black screens or even worse, especially when paired with bad
> > + * watermarks. So in order for us to be on the safe side, completely disable FBC
> > + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> > + * already suggests that watermarks may be bad, so try to be as safe as
> > + * possible.
> > + */
> > +void intel_fbc_handle_fifo_underrun(struct drm_i915_private *dev_priv)
> > +{
> > + struct intel_fbc *fbc = &dev_priv->fbc;
> > +
> > + if (!fbc_supported(dev_priv))
> > + return;
> > +
>
> Should we bail out if fbc is not enabled?
> Also, can we just disable fbc if we see an underrun instead of using a
> new flag to prevent activation?
The information that is crucially absent in the function name, and its
kerneldoc, is that this function is run from hardirq context. There isn't
much you are allowed to do here but schedule work.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: drm/i915/fbc: disable FBC on FIFO underruns
2016-08-13 9:05 ` Chris Wilson
@ 2016-08-15 20:49 ` Zanoni, Paulo R
2016-08-15 22:36 ` [PATCH] " Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Zanoni, Paulo R @ 2016-08-15 20:49 UTC (permalink / raw)
To: Pandiyan, Dhinakaran, chris; +Cc: intel-gfx, stefanr, stevenhoneyman
Em Sáb, 2016-08-13 às 10:05 +0100, Chris Wilson escreveu:
> On Fri, Aug 12, 2016 at 11:24:59PM +0000, Pandiyan, Dhinakaran wrote:
> Do we know why we get black screens in this scenario?
I don't know exactly, but I can speculate that it's probably because
the display engine fails to decompress the framebuffer and gets
completely lost on when the data ends, so it gives up and shows black.
> >
> > >
> > > +/**
> > > + * intel_fbc_handle_fifo_underrun - disable FBC when we get a
> > > FIFO underrun
> > > + * @dev_priv: i915 device instance
> > > + *
> > > + * Without FBC, most underruns are harmless and don't really
> > > cause too many
> > > + * problems, except for an annoying message on dmesg. With FBC,
> > > underruns can
> > > + * become black screens or even worse, especially when paired
> > > with bad
> > > + * watermarks. So in order for us to be on the safe side,
> > > completely disable FBC
> > > + * in case we ever detect a FIFO underrun on any pipe. An
> > > underrun on any pipe
> > > + * already suggests that watermarks may be bad, so try to be as
> > > safe as
> > > + * possible.
> > > + */
> > > +void intel_fbc_handle_fifo_underrun(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > + struct intel_fbc *fbc = &dev_priv->fbc;
> > > +
> > > + if (!fbc_supported(dev_priv))
> > > + return;
> > > +
> >
> > Should we bail out if fbc is not enabled?
No. Even if FBC is disabled or deactivated we need to make sure the
code doesn't decide to enable/activate FBC later, and simply disabling
it now won't prevent it from being reactivated on the next modesets.
> > Also, can we just disable fbc if we see an underrun instead of
> > using a
> > new flag to prevent activation?
No, for the same reason as above: we don't want the code to decide to
enable/activate FBC later.
>
> The information that is crucially absent in the function name, and
> its
> kerneldoc, is that this function is run from hardirq context. There
> isn't
> much you are allowed to do here but schedule work.
You're right. I'll rename the function and improve the doc.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-08-15 20:49 ` Zanoni, Paulo R
@ 2016-08-15 22:36 ` Paulo Zanoni
2016-09-02 5:58 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2016-08-15 22:36 UTC (permalink / raw)
To: intel-gfx; +Cc: Stefan Richter, Paulo Zanoni, stevenhoneyman @ gmail . com
Ever since I started working on FBC I was already aware that FBC can
really amplify the FIFO underrun symptoms. On systems where FIFO
underruns were harmless error messages, enabling FBC would cause the
underruns to give black screens.
We recently tried to enable FBC on Haswell and got reports of a system
that would hang after some hours of uptime, and the first bad commit
was the one that enabled FBC. We also observed that this system had
FIFO underrun error messages on its dmesg. Although we don't have any
evidence that fixing the underruns would solve the bug and make FBC
work properly on this machine, IMHO it's better if we minimize the
amount of possible problems by just giving up FBC whenever we detect
an underrun.
v2: New version, different implementation and commit message.
v3: Clarify the fact that we run from an IRQ handler (Chris).
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Lyude <cpaul@redhat.com>
Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 3 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_fbc.c | 61 ++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
4 files changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 35caa9b..baa9c66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -941,6 +941,9 @@ struct intel_fbc {
bool enabled;
bool active;
+ bool underrun_detected;
+ struct work_struct underrun_work;
+
struct intel_fbc_state_cache {
struct {
unsigned int mode_flags;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1c700b0..50c1eb0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1494,6 +1494,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
void intel_fbc_flush(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits, enum fb_op_origin origin);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
/* intel_hdmi.c */
void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index e122052..950aed5 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -750,6 +750,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
struct intel_fbc *fbc = &dev_priv->fbc;
struct intel_fbc_state_cache *cache = &fbc->state_cache;
+ /* We don't need to use a state cache here since this information is
+ * global for every CRTC. */
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (!cache->plane.visible) {
fbc->no_fbc_reason = "primary plane not visible";
return false;
@@ -1193,6 +1200,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
cancel_work_sync(&fbc->work.work);
}
+static void intel_fbc_underrun_work_fn(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, struct drm_i915_private, fbc.underrun_work);
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ mutex_lock(&fbc->lock);
+
+ /* Maybe we were scheduled twice. */
+ if (fbc->underrun_detected)
+ goto out;
+
+ DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
+ fbc->underrun_detected = true;
+
+ intel_fbc_deactivate(dev_priv);
+out:
+ mutex_unlock(&fbc->lock);
+}
+
+/**
+ * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
+ * @dev_priv: i915 device instance
+ *
+ * Without FBC, most underruns are harmless and don't really cause too many
+ * problems, except for an annoying message on dmesg. With FBC, underruns can
+ * become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable FBC
+ * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
+ * already suggests that watermarks may be bad, so try to be as safe as
+ * possible.
+ *
+ * This function is called from the IRQ handler.
+ */
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
+{
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ if (!fbc_supported(dev_priv))
+ return;
+
+ /* There's no guarantee that underrun_detected won't be set to true
+ * right after this check and before the work is scheduled, but that's
+ * not a problem since we'll check it again under the work function
+ * while FBC is locked. This check here is just to prevent us from
+ * unnecessarily scheduling the work, and it relies on the fact that we
+ * never switch underrun_detect back to false after it's true. */
+ if (fbc->underrun_detected)
+ return;
+
+ schedule_work(&fbc->underrun_work);
+}
+
/**
* intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
* @dev_priv: i915 device instance
@@ -1264,6 +1324,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
enum pipe pipe;
INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
+ INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
mutex_init(&fbc->lock);
fbc->enabled = false;
fbc->active = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 2aa7440..ebb4fed 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
DRM_ERROR("CPU pipe %c FIFO underrun\n",
pipe_name(pipe));
+
+ intel_fbc_handle_fifo_underrun_irq(dev_priv);
}
/**
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* ✗ Ro.CI.BAT: failure for drm/i915/fbc: disable FBC on FIFO underruns (rev2)
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
` (2 preceding siblings ...)
2016-08-12 23:24 ` Pandiyan, Dhinakaran
@ 2016-08-16 5:57 ` Patchwork
2016-09-12 20:54 ` ✓ Fi.CI.BAT: success for drm/i915/fbc: disable FBC on FIFO underruns (rev3) Patchwork
2016-09-13 14:19 ` ✗ Fi.CI.BAT: warning for drm/i915/fbc: disable FBC on FIFO underruns (rev4) Patchwork
5 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2016-08-16 5:57 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/fbc: disable FBC on FIFO underruns (rev2)
URL : https://patchwork.freedesktop.org/series/8575/
State : failure
== Summary ==
Series 8575v2 drm/i915/fbc: disable FBC on FIFO underruns
http://patchwork.freedesktop.org/api/1.0/series/8575/revisions/2/mbox
Test drv_module_reload_basic:
pass -> SKIP (ro-hsw-i3-4010u)
Test gem_exec_gttfill:
Subgroup basic:
skip -> PASS (fi-snb-i7-2600)
Test kms_cursor_legacy:
Subgroup basic-flip-vs-cursor-legacy:
fail -> PASS (ro-bdw-i5-5250u)
Subgroup basic-flip-vs-cursor-varying-size:
pass -> FAIL (ro-skl3-i5-6260u)
pass -> FAIL (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (ro-bdw-i7-5600u)
dmesg-warn -> SKIP (ro-bdw-i5-5250u)
fi-kbl-qkkr total:244 pass:185 dwarn:29 dfail:0 fail:3 skip:27
fi-skl-i7-6700k total:244 pass:208 dwarn:4 dfail:2 fail:2 skip:28
fi-snb-i7-2600 total:244 pass:202 dwarn:0 dfail:0 fail:0 skip:42
ro-bdw-i5-5250u total:240 pass:219 dwarn:2 dfail:0 fail:1 skip:18
ro-bdw-i7-5600u total:240 pass:207 dwarn:0 dfail:0 fail:1 skip:32
ro-bsw-n3050 total:240 pass:193 dwarn:0 dfail:0 fail:5 skip:42
ro-byt-n2820 total:240 pass:197 dwarn:0 dfail:0 fail:3 skip:40
ro-hsw-i3-4010u total:240 pass:213 dwarn:0 dfail:0 fail:0 skip:27
ro-hsw-i7-4770r total:240 pass:185 dwarn:0 dfail:0 fail:0 skip:55
ro-ilk1-i5-650 total:235 pass:174 dwarn:0 dfail:0 fail:1 skip:60
ro-ivb-i7-3770 total:240 pass:205 dwarn:0 dfail:0 fail:0 skip:35
ro-ivb2-i7-3770 total:240 pass:209 dwarn:0 dfail:0 fail:0 skip:31
ro-skl3-i5-6260u total:240 pass:222 dwarn:0 dfail:0 fail:4 skip:14
Results at /archive/results/CI_IGT_test/RO_Patchwork_1880/
bc5705c drm-intel-nightly: 2016y-08m-15d-15h-16m-01s UTC integration manifest
0e8d83a drm/i915/fbc: disable FBC on FIFO underruns
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-08-15 22:36 ` [PATCH] " Paulo Zanoni
@ 2016-09-02 5:58 ` Pandiyan, Dhinakaran
2016-09-02 14:45 ` Zanoni, Paulo R
0 siblings, 1 reply; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2016-09-02 5:58 UTC (permalink / raw)
To: Zanoni, Paulo R; +Cc: intel-gfx, stefanr, stevenhoneyman
On Mon, 2016-08-15 at 19:36 -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
>
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
>
> v2: New version, different implementation and commit message.
> v3: Clarify the fact that we run from an IRQ handler (Chris).
>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Lyude <cpaul@redhat.com>
> Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_fbc.c | 61 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
> 4 files changed, 67 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 35caa9b..baa9c66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -941,6 +941,9 @@ struct intel_fbc {
> bool enabled;
> bool active;
>
> + bool underrun_detected;
> + struct work_struct underrun_work;
> +
> struct intel_fbc_state_cache {
> struct {
> unsigned int mode_flags;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1c700b0..50c1eb0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1494,6 +1494,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
>
> /* intel_hdmi.c */
> void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index e122052..950aed5 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -750,6 +750,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> struct intel_fbc *fbc = &dev_priv->fbc;
> struct intel_fbc_state_cache *cache = &fbc->state_cache;
>
> + /* We don't need to use a state cache here since this information is
> + * global for every CRTC. */
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (!cache->plane.visible) {
> fbc->no_fbc_reason = "primary plane not visible";
> return false;
> @@ -1193,6 +1200,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
> cancel_work_sync(&fbc->work.work);
> }
>
> +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private, fbc.underrun_work);
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + mutex_lock(&fbc->lock);
> +
> + /* Maybe we were scheduled twice. */
> + if (fbc->underrun_detected)
> + goto out;
If the underrun happens very early after boot (likely during modeset),
the user might end up seeing just FBC enable and disable messages in
dmesg without knowing it (or why it) was deactivated.
For e.g.,
[ 1855.552489] [drm:intel_fbc_enable] Enabling FBC on pipe A
[ 1856.854239] [drm:__intel_fbc_disable] Disabling FBC on pipe A
[ 1857.753383] [drm:intel_fbc_alloc_cfb] Reducing the compressed
framebuffer size. This may lead to less power savings than a
non-reduced-size. Try to increase stolen memory size if available in
BIOS.
[ 1857.753384] [drm:intel_fbc_alloc_cfb] reserved 10368000 bytes of
contiguous stolen space for FBC, threshold: 4
[ 1857.753384] [drm:intel_fbc_enable] Enabling FBC on pipe A
I find this noise misleading because FBC is not going to be activated at
all.
> +
> + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
Print out the pipe here? Both the enabling and disabling debug messages
clarify the pipe. It may be useful to provide this information here too.
> + fbc->underrun_detected = true;
> +
> + intel_fbc_deactivate(dev_priv);
> +out:
> + mutex_unlock(&fbc->lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
> + * @dev_priv: i915 device instance
> + *
> + * Without FBC, most underruns are harmless and don't really cause too many
> + * problems, except for an annoying message on dmesg. With FBC, underruns can
> + * become black screens or even worse, especially when paired with bad
> + * watermarks. So in order for us to be on the safe side, completely disable FBC
> + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> + * already suggests that watermarks may be bad, so try to be as safe as
> + * possible.
> + *
> + * This function is called from the IRQ handler.
> + */
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> +{
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + if (!fbc_supported(dev_priv))
> + return;
> +
> + /* There's no guarantee that underrun_detected won't be set to true
> + * right after this check and before the work is scheduled, but that's
> + * not a problem since we'll check it again under the work function
> + * while FBC is locked. This check here is just to prevent us from
> + * unnecessarily scheduling the work, and it relies on the fact that we
> + * never switch underrun_detect back to false after it's true. */
> + if (fbc->underrun_detected)
> + return;
> +
> + schedule_work(&fbc->underrun_work);
> +}
> +
> /**
> * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
> * @dev_priv: i915 device instance
> @@ -1264,6 +1324,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> enum pipe pipe;
>
> INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> + INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> mutex_init(&fbc->lock);
> fbc->enabled = false;
> fbc->active = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 2aa7440..ebb4fed 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> DRM_ERROR("CPU pipe %c FIFO underrun\n",
> pipe_name(pipe));
> +
> + intel_fbc_handle_fifo_underrun_irq(dev_priv);
> }
>
> /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-02 5:58 ` Pandiyan, Dhinakaran
@ 2016-09-02 14:45 ` Zanoni, Paulo R
2016-09-12 20:02 ` Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Zanoni, Paulo R @ 2016-09-02 14:45 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, stefanr, stevenhoneyman
Em Sex, 2016-09-02 às 05:58 +0000, Pandiyan, Dhinakaran escreveu:
> On Mon, 2016-08-15 at 19:36 -0300, Paulo Zanoni wrote:
> >
> > Ever since I started working on FBC I was already aware that FBC
> > can
> > really amplify the FIFO underrun symptoms. On systems where FIFO
> > underruns were harmless error messages, enabling FBC would cause
> > the
> > underruns to give black screens.
> >
> > We recently tried to enable FBC on Haswell and got reports of a
> > system
> > that would hang after some hours of uptime, and the first bad
> > commit
> > was the one that enabled FBC. We also observed that this system had
> > FIFO underrun error messages on its dmesg. Although we don't have
> > any
> > evidence that fixing the underruns would solve the bug and make FBC
> > work properly on this machine, IMHO it's better if we minimize the
> > amount of possible problems by just giving up FBC whenever we
> > detect
> > an underrun.
> >
> > v2: New version, different implementation and commit message.
> > v3: Clarify the fact that we run from an IRQ handler (Chris).
> >
> > Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> > Cc: Lyude <cpaul@redhat.com>
> > Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_fbc.c | 61
> > ++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
> > 4 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 35caa9b..baa9c66 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -941,6 +941,9 @@ struct intel_fbc {
> > bool enabled;
> > bool active;
> >
> > + bool underrun_detected;
> > + struct work_struct underrun_work;
> > +
> > struct intel_fbc_state_cache {
> > struct {
> > unsigned int mode_flags;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 1c700b0..50c1eb0 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1494,6 +1494,7 @@ void intel_fbc_invalidate(struct
> > drm_i915_private *dev_priv,
> > void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > unsigned int frontbuffer_bits, enum
> > fb_op_origin origin);
> > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> > +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private
> > *dev_priv);
> >
> > /* intel_hdmi.c */
> > void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg,
> > enum port port);
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index e122052..950aed5 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -750,6 +750,13 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> > struct intel_fbc *fbc = &dev_priv->fbc;
> > struct intel_fbc_state_cache *cache = &fbc->state_cache;
> >
> > + /* We don't need to use a state cache here since this
> > information is
> > + * global for every CRTC. */
> > + if (fbc->underrun_detected) {
> > + fbc->no_fbc_reason = "underrun detected";
> > + return false;
> > + }
> > +
> > if (!cache->plane.visible) {
> > fbc->no_fbc_reason = "primary plane not visible";
> > return false;
> > @@ -1193,6 +1200,59 @@ void intel_fbc_global_disable(struct
> > drm_i915_private *dev_priv)
> > cancel_work_sync(&fbc->work.work);
> > }
> >
> > +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(work, struct drm_i915_private,
> > fbc.underrun_work);
> > + struct intel_fbc *fbc = &dev_priv->fbc;
> > +
> > + mutex_lock(&fbc->lock);
> > +
> > + /* Maybe we were scheduled twice. */
> > + if (fbc->underrun_detected)
> > + goto out;
>
> If the underrun happens very early after boot (likely during
> modeset),
> the user might end up seeing just FBC enable and disable messages in
> dmesg without knowing it (or why it) was deactivated.
>
> For e.g.,
>
> [ 1855.552489] [drm:intel_fbc_enable] Enabling FBC on pipe A
> [ 1856.854239] [drm:__intel_fbc_disable] Disabling FBC on pipe A
> [ 1857.753383] [drm:intel_fbc_alloc_cfb] Reducing the compressed
> framebuffer size. This may lead to less power savings than a
> non-reduced-size. Try to increase stolen memory size if available in
> BIOS.
> [ 1857.753384] [drm:intel_fbc_alloc_cfb] reserved 10368000 bytes of
> contiguous stolen space for FBC, threshold: 4
> [ 1857.753384] [drm:intel_fbc_enable] Enabling FBC on pipe A
>
> I find this noise misleading because FBC is not going to be activated
> at
> all.
You have a point here, good catch. I was trying to avoid having two
checks (one for enabling, one for activation) and just kept the
activation check since it was the one we needed for sure. But it seems
having the extra check during enabling will prevent these extra
messages, so I'll update the patch.
>
>
> >
> > +
> > + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
>
> Print out the pipe here? Both the enabling and disabling debug
> messages
> clarify the pipe. It may be useful to provide this information here
> too.
I think this could be misleading because we're disabling FBC forever on
all pipes whenever a FIFO underrun happens on any pipe. Maybe FBC is
not enabled so there's no FBC pipe to print. Also, the DRM_ERRORs
provided by the FIFO underrun detection code actually print the pipe
that caused the underrun just a few lines above on dmesg, so we know
that too.
>
> >
> > + fbc->underrun_detected = true;
> > +
> > + intel_fbc_deactivate(dev_priv);
> > +out:
> > + mutex_unlock(&fbc->lock);
> > +}
> > +
> > +/**
> > + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a
> > FIFO underrun
> > + * @dev_priv: i915 device instance
> > + *
> > + * Without FBC, most underruns are harmless and don't really cause
> > too many
> > + * problems, except for an annoying message on dmesg. With FBC,
> > underruns can
> > + * become black screens or even worse, especially when paired with
> > bad
> > + * watermarks. So in order for us to be on the safe side,
> > completely disable FBC
> > + * in case we ever detect a FIFO underrun on any pipe. An underrun
> > on any pipe
> > + * already suggests that watermarks may be bad, so try to be as
> > safe as
> > + * possible.
> > + *
> > + * This function is called from the IRQ handler.
> > + */
> > +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private
> > *dev_priv)
> > +{
> > + struct intel_fbc *fbc = &dev_priv->fbc;
> > +
> > + if (!fbc_supported(dev_priv))
> > + return;
> > +
> > + /* There's no guarantee that underrun_detected won't be
> > set to true
> > + * right after this check and before the work is
> > scheduled, but that's
> > + * not a problem since we'll check it again under the work
> > function
> > + * while FBC is locked. This check here is just to prevent
> > us from
> > + * unnecessarily scheduling the work, and it relies on the
> > fact that we
> > + * never switch underrun_detect back to false after it's
> > true. */
> > + if (fbc->underrun_detected)
> > + return;
> > +
> > + schedule_work(&fbc->underrun_work);
> > +}
> > +
> > /**
> > * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility
> > tracking
> > * @dev_priv: i915 device instance
> > @@ -1264,6 +1324,7 @@ void intel_fbc_init(struct drm_i915_private
> > *dev_priv)
> > enum pipe pipe;
> >
> > INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> > + INIT_WORK(&fbc->underrun_work,
> > intel_fbc_underrun_work_fn);
> > mutex_init(&fbc->lock);
> > fbc->enabled = false;
> > fbc->active = false;
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index 2aa7440..ebb4fed 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct
> > drm_i915_private *dev_priv,
> > if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe,
> > false))
> > DRM_ERROR("CPU pipe %c FIFO underrun\n",
> > pipe_name(pipe));
> > +
> > + intel_fbc_handle_fifo_underrun_irq(dev_priv);
> > }
> >
> > /**
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-02 14:45 ` Zanoni, Paulo R
@ 2016-09-12 20:02 ` Paulo Zanoni
2016-09-12 20:25 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Paulo Zanoni @ 2016-09-12 20:02 UTC (permalink / raw)
To: intel-gfx
Cc: Dhinakaran Pandiyan, Stefan Richter, Paulo Zanoni,
stevenhoneyman @ gmail . com
Ever since I started working on FBC I was already aware that FBC can
really amplify the FIFO underrun symptoms. On systems where FIFO
underruns were harmless error messages, enabling FBC would cause the
underruns to give black screens.
We recently tried to enable FBC on Haswell and got reports of a system
that would hang after some hours of uptime, and the first bad commit
was the one that enabled FBC. We also observed that this system had
FIFO underrun error messages on its dmesg. Although we don't have any
evidence that fixing the underruns would solve the bug and make FBC
work properly on this machine, IMHO it's better if we minimize the
amount of possible problems by just giving up FBC whenever we detect
an underrun.
v2: New version, different implementation and commit message.
v3: Clarify the fact that we run from an IRQ handler (Chris).
v4: Also add the underrun_detected check at can_choose() to avoid
misleading dmesg messages (DK).
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Lyude <cpaul@redhat.com>
Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 3 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_fbc.c | 66 ++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
4 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e2dda8..2025f42 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -971,6 +971,9 @@ struct intel_fbc {
bool enabled;
bool active;
+ bool underrun_detected;
+ struct work_struct underrun_work;
+
struct intel_fbc_state_cache {
struct {
unsigned int mode_flags;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index abe7a4d..0d05712 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1508,6 +1508,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
void intel_fbc_flush(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits, enum fb_op_origin origin);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
/* intel_hdmi.c */
void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5dcb81a..4b91af1 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -774,6 +774,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
struct intel_fbc *fbc = &dev_priv->fbc;
struct intel_fbc_state_cache *cache = &fbc->state_cache;
+ /* We don't need to use a state cache here since this information is
+ * global for every CRTC. */
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (!cache->plane.visible) {
fbc->no_fbc_reason = "primary plane not visible";
return false;
@@ -859,6 +866,11 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
return false;
}
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
fbc->no_fbc_reason = "no enabled pipes can have FBC";
return false;
@@ -1221,6 +1233,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
cancel_work_sync(&fbc->work.work);
}
+static void intel_fbc_underrun_work_fn(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, struct drm_i915_private, fbc.underrun_work);
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ mutex_lock(&fbc->lock);
+
+ /* Maybe we were scheduled twice. */
+ if (fbc->underrun_detected)
+ goto out;
+
+ DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
+ fbc->underrun_detected = true;
+
+ intel_fbc_deactivate(dev_priv);
+out:
+ mutex_unlock(&fbc->lock);
+}
+
+/**
+ * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
+ * @dev_priv: i915 device instance
+ *
+ * Without FBC, most underruns are harmless and don't really cause too many
+ * problems, except for an annoying message on dmesg. With FBC, underruns can
+ * become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable FBC
+ * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
+ * already suggests that watermarks may be bad, so try to be as safe as
+ * possible.
+ *
+ * This function is called from the IRQ handler.
+ */
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
+{
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ if (!fbc_supported(dev_priv))
+ return;
+
+ /* There's no guarantee that underrun_detected won't be set to true
+ * right after this check and before the work is scheduled, but that's
+ * not a problem since we'll check it again under the work function
+ * while FBC is locked. This check here is just to prevent us from
+ * unnecessarily scheduling the work, and it relies on the fact that we
+ * never switch underrun_detect back to false after it's true. */
+ if (fbc->underrun_detected)
+ return;
+
+ schedule_work(&fbc->underrun_work);
+}
+
/**
* intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
* @dev_priv: i915 device instance
@@ -1292,6 +1357,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
enum pipe pipe;
INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
+ INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
mutex_init(&fbc->lock);
fbc->enabled = false;
fbc->active = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 2aa7440..ebb4fed 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
DRM_ERROR("CPU pipe %c FIFO underrun\n",
pipe_name(pipe));
+
+ intel_fbc_handle_fifo_underrun_irq(dev_priv);
}
/**
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-12 20:02 ` Paulo Zanoni
@ 2016-09-12 20:25 ` Chris Wilson
2016-09-12 21:25 ` Zanoni, Paulo R
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2016-09-12 20:25 UTC (permalink / raw)
To: Paulo Zanoni
Cc: intel-gfx, Stefan Richter, Dhinakaran Pandiyan,
stevenhoneyman @ gmail . com
On Mon, Sep 12, 2016 at 05:02:56PM -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
>
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
>
> v2: New version, different implementation and commit message.
> v3: Clarify the fact that we run from an IRQ handler (Chris).
> v4: Also add the underrun_detected check at can_choose() to avoid
> misleading dmesg messages (DK).
>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Lyude <cpaul@redhat.com>
> Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_fbc.c | 66 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
> 4 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e2dda8..2025f42 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -971,6 +971,9 @@ struct intel_fbc {
> bool enabled;
> bool active;
>
> + bool underrun_detected;
> + struct work_struct underrun_work;
> +
> struct intel_fbc_state_cache {
> struct {
> unsigned int mode_flags;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index abe7a4d..0d05712 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1508,6 +1508,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
>
> /* intel_hdmi.c */
> void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 5dcb81a..4b91af1 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -774,6 +774,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> struct intel_fbc *fbc = &dev_priv->fbc;
> struct intel_fbc_state_cache *cache = &fbc->state_cache;
>
> + /* We don't need to use a state cache here since this information is
> + * global for every CRTC. */
* global for all CRTC.
*/
"global for every CRTC" is confusing, as it says to me that is a
separate global variable for each CRTC. But what you mean is that there
is one variable that reflects all CRTC.
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (!cache->plane.visible) {
> fbc->no_fbc_reason = "primary plane not visible";
> return false;
> @@ -859,6 +866,11 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
> return false;
> }
>
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
> fbc->no_fbc_reason = "no enabled pipes can have FBC";
> return false;
> @@ -1221,6 +1233,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
> cancel_work_sync(&fbc->work.work);
> }
>
> +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private, fbc.underrun_work);
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + mutex_lock(&fbc->lock);
> +
> + /* Maybe we were scheduled twice. */
> + if (fbc->underrun_detected)
> + goto out;
> +
> + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> + fbc->underrun_detected = true;
Could juggle to reduce the coverage of the mutex, but irrelevant.
A comment as to why we permanently disable the fbc would be useful. At
least to disuade people from trying to make helpful remarks...
> + intel_fbc_deactivate(dev_priv);
> +out:
> + mutex_unlock(&fbc->lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
> + * @dev_priv: i915 device instance
> + *
> + * Without FBC, most underruns are harmless and don't really cause too many
> + * problems, except for an annoying message on dmesg. With FBC, underruns can
> + * become black screens or even worse, especially when paired with bad
> + * watermarks. So in order for us to be on the safe side, completely disable FBC
> + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> + * already suggests that watermarks may be bad, so try to be as safe as
> + * possible.
> + *
> + * This function is called from the IRQ handler.
> + */
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> +{
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + if (!fbc_supported(dev_priv))
> + return;
> +
> + /* There's no guarantee that underrun_detected won't be set to true
> + * right after this check and before the work is scheduled, but that's
> + * not a problem since we'll check it again under the work function
> + * while FBC is locked. This check here is just to prevent us from
> + * unnecessarily scheduling the work, and it relies on the fact that we
> + * never switch underrun_detect back to false after it's true. */
*/
> + if (fbc->underrun_detected)
if (READ_ONCE(fbc->underrun_detected))
> + return;
> +
> + schedule_work(&fbc->underrun_work);
> +}
> +
> /**
> * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
> * @dev_priv: i915 device instance
> @@ -1292,6 +1357,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> enum pipe pipe;
>
> INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> + INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> mutex_init(&fbc->lock);
> fbc->enabled = false;
> fbc->active = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 2aa7440..ebb4fed 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> DRM_ERROR("CPU pipe %c FIFO underrun\n",
> pipe_name(pipe));
> +
> + intel_fbc_handle_fifo_underrun_irq(dev_priv);
> }
>
> /**
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/fbc: disable FBC on FIFO underruns (rev3)
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
` (3 preceding siblings ...)
2016-08-16 5:57 ` ✗ Ro.CI.BAT: failure for drm/i915/fbc: disable FBC on FIFO underruns (rev2) Patchwork
@ 2016-09-12 20:54 ` Patchwork
2016-09-13 14:19 ` ✗ Fi.CI.BAT: warning for drm/i915/fbc: disable FBC on FIFO underruns (rev4) Patchwork
5 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2016-09-12 20:54 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/fbc: disable FBC on FIFO underruns (rev3)
URL : https://patchwork.freedesktop.org/series/8575/
State : success
== Summary ==
Series 8575v3 drm/i915/fbc: disable FBC on FIFO underruns
https://patchwork.freedesktop.org/api/1.0/series/8575/revisions/3/mbox/
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-legacy:
fail -> PASS (fi-bsw-n3050)
Subgroup basic-cursor-vs-flip-varying-size:
fail -> PASS (fi-ilk-650)
fi-bdw-5557u total:254 pass:239 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:254 pass:208 dwarn:0 dfail:0 fail:0 skip:46
fi-byt-n2820 total:254 pass:212 dwarn:0 dfail:0 fail:1 skip:41
fi-hsw-4770k total:254 pass:232 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:254 pass:228 dwarn:0 dfail:0 fail:0 skip:26
fi-ilk-650 total:254 pass:185 dwarn:0 dfail:0 fail:1 skip:68
fi-ivb-3520m total:254 pass:223 dwarn:0 dfail:0 fail:0 skip:31
fi-ivb-3770 total:254 pass:223 dwarn:0 dfail:0 fail:0 skip:31
fi-skl-6260u total:254 pass:240 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:254 pass:227 dwarn:0 dfail:0 fail:1 skip:26
fi-skl-6700k total:254 pass:225 dwarn:1 dfail:0 fail:0 skip:28
fi-snb-2520m total:254 pass:209 dwarn:0 dfail:0 fail:0 skip:45
fi-snb-2600 total:254 pass:209 dwarn:0 dfail:0 fail:0 skip:45
Results at /archive/results/CI_IGT_test/Patchwork_2514/
93d9d9dd030c6eb360b500bf9872663b30b5ebe5 drm-intel-nightly: 2016y-09m-12d-14h-34m-11s UTC integration manifest
cf76cad drm/i915/fbc: disable FBC on FIFO underruns
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-12 20:25 ` Chris Wilson
@ 2016-09-12 21:25 ` Zanoni, Paulo R
2016-09-13 13:38 ` Paulo Zanoni
0 siblings, 1 reply; 20+ messages in thread
From: Zanoni, Paulo R @ 2016-09-12 21:25 UTC (permalink / raw)
To: chris; +Cc: intel-gfx, stefanr, stevenhoneyman, Pandiyan, Dhinakaran
Em Seg, 2016-09-12 às 21:25 +0100, Chris Wilson escreveu:
> On Mon, Sep 12, 2016 at 05:02:56PM -0300, Paulo Zanoni wrote:
> >
> > Ever since I started working on FBC I was already aware that FBC
> > can
> > really amplify the FIFO underrun symptoms. On systems where FIFO
> > underruns were harmless error messages, enabling FBC would cause
> > the
> > underruns to give black screens.
> >
> > We recently tried to enable FBC on Haswell and got reports of a
> > system
> > that would hang after some hours of uptime, and the first bad
> > commit
> > was the one that enabled FBC. We also observed that this system had
> > FIFO underrun error messages on its dmesg. Although we don't have
> > any
> > evidence that fixing the underruns would solve the bug and make FBC
> > work properly on this machine, IMHO it's better if we minimize the
> > amount of possible problems by just giving up FBC whenever we
> > detect
> > an underrun.
> >
> > v2: New version, different implementation and commit message.
> > v3: Clarify the fact that we run from an IRQ handler (Chris).
> > v4: Also add the underrun_detected check at can_choose() to avoid
> > misleading dmesg messages (DK).
> >
> > Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> > Cc: Lyude <cpaul@redhat.com>
> > Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_fbc.c | 66
> > ++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
> > 4 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 1e2dda8..2025f42 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -971,6 +971,9 @@ struct intel_fbc {
> > bool enabled;
> > bool active;
> >
> > + bool underrun_detected;
> > + struct work_struct underrun_work;
> > +
> > struct intel_fbc_state_cache {
> > struct {
> > unsigned int mode_flags;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index abe7a4d..0d05712 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1508,6 +1508,7 @@ void intel_fbc_invalidate(struct
> > drm_i915_private *dev_priv,
> > void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > unsigned int frontbuffer_bits, enum
> > fb_op_origin origin);
> > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> > +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private
> > *dev_priv);
> >
> > /* intel_hdmi.c */
> > void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg,
> > enum port port);
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 5dcb81a..4b91af1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -774,6 +774,13 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> > struct intel_fbc *fbc = &dev_priv->fbc;
> > struct intel_fbc_state_cache *cache = &fbc->state_cache;
> >
> > + /* We don't need to use a state cache here since this
> > information is
> > + * global for every CRTC. */
>
> * global for all CRTC.
> */
>
> "global for every CRTC" is confusing, as it says to me that is a
> separate global variable for each CRTC. But what you mean is that
> there
> is one variable that reflects all CRTC.
Makes sense.
>
> >
> > + if (fbc->underrun_detected) {
> > + fbc->no_fbc_reason = "underrun detected";
> > + return false;
> > + }
> > +
> > if (!cache->plane.visible) {
> > fbc->no_fbc_reason = "primary plane not visible";
> > return false;
> > @@ -859,6 +866,11 @@ static bool intel_fbc_can_choose(struct
> > intel_crtc *crtc)
> > return false;
> > }
> >
> > + if (fbc->underrun_detected) {
> > + fbc->no_fbc_reason = "underrun detected";
> > + return false;
> > + }
> > +
> > if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> > {
> > fbc->no_fbc_reason = "no enabled pipes can have
> > FBC";
> > return false;
> > @@ -1221,6 +1233,59 @@ void intel_fbc_global_disable(struct
> > drm_i915_private *dev_priv)
> > cancel_work_sync(&fbc->work.work);
> > }
> >
> > +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(work, struct drm_i915_private,
> > fbc.underrun_work);
> > + struct intel_fbc *fbc = &dev_priv->fbc;
> > +
> > + mutex_lock(&fbc->lock);
> > +
> > + /* Maybe we were scheduled twice. */
> > + if (fbc->underrun_detected)
> > + goto out;
> > +
> > + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> > + fbc->underrun_detected = true;
>
> Could juggle to reduce the coverage of the mutex, but irrelevant.
> A comment as to why we permanently disable the fbc would be useful.
> At
> least to disuade people from trying to make helpful remarks...
There's already one right below, at the
intel_fbc_handle_fifo_underrun_irq documentation, and also on the
commit message. If those are not enough, can you please clarify what
should be done?
Thanks,
Paulo
>
>
> >
> > + intel_fbc_deactivate(dev_priv);
> > +out:
> > + mutex_unlock(&fbc->lock);
> > +}
> > +
> > +/**
> > + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a
> > FIFO underrun
> > + * @dev_priv: i915 device instance
> > + *
> > + * Without FBC, most underruns are harmless and don't really cause
> > too many
> > + * problems, except for an annoying message on dmesg. With FBC,
> > underruns can
> > + * become black screens or even worse, especially when paired with
> > bad
> > + * watermarks. So in order for us to be on the safe side,
> > completely disable FBC
> > + * in case we ever detect a FIFO underrun on any pipe. An underrun
> > on any pipe
> > + * already suggests that watermarks may be bad, so try to be as
> > safe as
> > + * possible.
> > + *
> > + * This function is called from the IRQ handler.
> > + */
> > +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private
> > *dev_priv)
> > +{
> > + struct intel_fbc *fbc = &dev_priv->fbc;
> > +
> > + if (!fbc_supported(dev_priv))
> > + return;
> > +
> > + /* There's no guarantee that underrun_detected won't be
> > set to true
> > + * right after this check and before the work is
> > scheduled, but that's
> > + * not a problem since we'll check it again under the work
> > function
> > + * while FBC is locked. This check here is just to prevent
> > us from
> > + * unnecessarily scheduling the work, and it relies on the
> > fact that we
> > + * never switch underrun_detect back to false after it's
> > true. */
> */
> >
> > + if (fbc->underrun_detected)
> if (READ_ONCE(fbc->underrun_detected))
> >
> > + return;
> > +
> > + schedule_work(&fbc->underrun_work);
> > +}
> > +
> > /**
> > * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility
> > tracking
> > * @dev_priv: i915 device instance
> > @@ -1292,6 +1357,7 @@ void intel_fbc_init(struct drm_i915_private
> > *dev_priv)
> > enum pipe pipe;
> >
> > INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> > + INIT_WORK(&fbc->underrun_work,
> > intel_fbc_underrun_work_fn);
> > mutex_init(&fbc->lock);
> > fbc->enabled = false;
> > fbc->active = false;
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index 2aa7440..ebb4fed 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct
> > drm_i915_private *dev_priv,
> > if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe,
> > false))
> > DRM_ERROR("CPU pipe %c FIFO underrun\n",
> > pipe_name(pipe));
> > +
> > + intel_fbc_handle_fifo_underrun_irq(dev_priv);
> > }
> >
> > /**
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-12 21:25 ` Zanoni, Paulo R
@ 2016-09-13 13:38 ` Paulo Zanoni
2016-09-13 13:56 ` Chris Wilson
2016-09-13 17:07 ` Pandiyan, Dhinakaran
0 siblings, 2 replies; 20+ messages in thread
From: Paulo Zanoni @ 2016-09-13 13:38 UTC (permalink / raw)
To: intel-gfx
Cc: Paulo Zanoni, Stefan Richter, Dhinakaran Pandiyan,
stevenhoneyman @ gmail . com
Ever since I started working on FBC I was already aware that FBC can
really amplify the FIFO underrun symptoms. On systems where FIFO
underruns were harmless error messages, enabling FBC would cause the
underruns to give black screens.
We recently tried to enable FBC on Haswell and got reports of a system
that would hang after some hours of uptime, and the first bad commit
was the one that enabled FBC. We also observed that this system had
FIFO underrun error messages on its dmesg. Although we don't have any
evidence that fixing the underruns would solve the bug and make FBC
work properly on this machine, IMHO it's better if we minimize the
amount of possible problems by just giving up FBC whenever we detect
an underrun.
v2: New version, different implementation and commit message.
v3: Clarify the fact that we run from an IRQ handler (Chris).
v4: Also add the underrun_detected check at can_choose() to avoid
misleading dmesg messages (DK).
v5: Fix Engrish, use READ_ONCE on the unlocked read (Chris).
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Lyude <cpaul@redhat.com>
Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 3 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_fbc.c | 67 ++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
4 files changed, 73 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20b7743..5b1c241 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -971,6 +971,9 @@ struct intel_fbc {
bool enabled;
bool active;
+ bool underrun_detected;
+ struct work_struct underrun_work;
+
struct intel_fbc_state_cache {
struct {
unsigned int mode_flags;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index abe7a4d..0d05712 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1508,6 +1508,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
void intel_fbc_flush(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits, enum fb_op_origin origin);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
/* intel_hdmi.c */
void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5dcb81a..10ca8cd 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -774,6 +774,14 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
struct intel_fbc *fbc = &dev_priv->fbc;
struct intel_fbc_state_cache *cache = &fbc->state_cache;
+ /* We don't need to use a state cache here since this information is
+ * global for all CRTC.
+ */
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (!cache->plane.visible) {
fbc->no_fbc_reason = "primary plane not visible";
return false;
@@ -859,6 +867,11 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
return false;
}
+ if (fbc->underrun_detected) {
+ fbc->no_fbc_reason = "underrun detected";
+ return false;
+ }
+
if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
fbc->no_fbc_reason = "no enabled pipes can have FBC";
return false;
@@ -1221,6 +1234,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
cancel_work_sync(&fbc->work.work);
}
+static void intel_fbc_underrun_work_fn(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, struct drm_i915_private, fbc.underrun_work);
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ mutex_lock(&fbc->lock);
+
+ /* Maybe we were scheduled twice. */
+ if (fbc->underrun_detected)
+ goto out;
+
+ DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
+ fbc->underrun_detected = true;
+
+ intel_fbc_deactivate(dev_priv);
+out:
+ mutex_unlock(&fbc->lock);
+}
+
+/**
+ * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
+ * @dev_priv: i915 device instance
+ *
+ * Without FBC, most underruns are harmless and don't really cause too many
+ * problems, except for an annoying message on dmesg. With FBC, underruns can
+ * become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable FBC
+ * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
+ * already suggests that watermarks may be bad, so try to be as safe as
+ * possible.
+ *
+ * This function is called from the IRQ handler.
+ */
+void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
+{
+ struct intel_fbc *fbc = &dev_priv->fbc;
+
+ if (!fbc_supported(dev_priv))
+ return;
+
+ /* There's no guarantee that underrun_detected won't be set to true
+ * right after this check and before the work is scheduled, but that's
+ * not a problem since we'll check it again under the work function
+ * while FBC is locked. This check here is just to prevent us from
+ * unnecessarily scheduling the work, and it relies on the fact that we
+ * never switch underrun_detect back to false after it's true. */
+ if (READ_ONCE(fbc->underrun_detected))
+ return;
+
+ schedule_work(&fbc->underrun_work);
+}
+
/**
* intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
* @dev_priv: i915 device instance
@@ -1292,6 +1358,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
enum pipe pipe;
INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
+ INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
mutex_init(&fbc->lock);
fbc->enabled = false;
fbc->active = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 2aa7440..ebb4fed 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
DRM_ERROR("CPU pipe %c FIFO underrun\n",
pipe_name(pipe));
+
+ intel_fbc_handle_fifo_underrun_irq(dev_priv);
}
/**
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-13 13:38 ` Paulo Zanoni
@ 2016-09-13 13:56 ` Chris Wilson
2016-09-13 17:07 ` Pandiyan, Dhinakaran
1 sibling, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2016-09-13 13:56 UTC (permalink / raw)
To: Paulo Zanoni
Cc: Dhinakaran Pandiyan, intel-gfx, Stefan Richter,
stevenhoneyman @ gmail . com
On Tue, Sep 13, 2016 at 10:38:57AM -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
>
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
>
> v2: New version, different implementation and commit message.
> v3: Clarify the fact that we run from an IRQ handler (Chris).
> v4: Also add the underrun_detected check at can_choose() to avoid
> misleading dmesg messages (DK).
> v5: Fix Engrish, use READ_ONCE on the unlocked read (Chris).
>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Lyude <cpaul@redhat.com>
> Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Looks like it does what you describe, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/fbc: disable FBC on FIFO underruns (rev4)
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
` (4 preceding siblings ...)
2016-09-12 20:54 ` ✓ Fi.CI.BAT: success for drm/i915/fbc: disable FBC on FIFO underruns (rev3) Patchwork
@ 2016-09-13 14:19 ` Patchwork
5 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2016-09-13 14:19 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/fbc: disable FBC on FIFO underruns (rev4)
URL : https://patchwork.freedesktop.org/series/8575/
State : warning
== Summary ==
Series 8575v4 drm/i915/fbc: disable FBC on FIFO underruns
https://patchwork.freedesktop.org/api/1.0/series/8575/revisions/4/mbox/
Test drv_module_reload_basic:
pass -> DMESG-WARN (fi-ilk-650)
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (fi-ilk-650)
Test kms_busy:
Subgroup basic-flip-default-a:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup basic-flip-default-b:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup basic-flip-default-c:
pass -> SKIP (fi-ivb-3770)
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-legacy:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup basic-cursor-vs-flip-varying-size:
fail -> DMESG-FAIL (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup basic-flip-vs-cursor-legacy:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup basic-flip-vs-cursor-varying-size:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Test kms_frontbuffer_tracking:
Subgroup basic:
pass -> SKIP (fi-ivb-3770)
Test kms_pipe_crc_basic:
Subgroup bad-nb-words-1:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-nb-words-3:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-pipe:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup bad-source:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup hang-read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup nonblocking-crc-pipe-c-frame-sequence:
pass -> SKIP (fi-hsw-4770r)
Subgroup read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-a-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup read-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (fi-ilk-650)
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-ilk-650)
pass -> SKIP (fi-ivb-3770)
Subgroup suspend-read-crc-pipe-c:
pass -> SKIP (fi-ivb-3770)
Test prime_vgem:
Subgroup basic-fence-flip:
pass -> SKIP (fi-ivb-3770)
skip -> PASS (fi-snb-2520m)
fi-bdw-5557u total:254 pass:239 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:254 pass:208 dwarn:0 dfail:0 fail:0 skip:46
fi-byt-n2820 total:254 pass:212 dwarn:0 dfail:0 fail:1 skip:41
fi-hsw-4770k total:254 pass:232 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:254 pass:227 dwarn:0 dfail:0 fail:0 skip:27
fi-ilk-650 total:254 pass:161 dwarn:23 dfail:1 fail:1 skip:68
fi-ivb-3520m total:254 pass:223 dwarn:0 dfail:0 fail:0 skip:31
fi-ivb-3770 total:254 pass:211 dwarn:0 dfail:0 fail:0 skip:43
fi-skl-6260u total:254 pass:240 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:254 pass:227 dwarn:0 dfail:0 fail:1 skip:26
fi-skl-6700k total:254 pass:225 dwarn:1 dfail:0 fail:0 skip:28
fi-snb-2520m total:254 pass:210 dwarn:0 dfail:0 fail:0 skip:44
fi-snb-2600 total:254 pass:209 dwarn:0 dfail:0 fail:0 skip:45
Results at /archive/results/CI_IGT_test/Patchwork_2521/
b2c7ef9208902fdd352655b3a45a98714c051c0e drm-intel-nightly: 2016y-09m-13d-09h-04m-50s UTC integration manifest
8a3cf05 drm/i915/fbc: disable FBC on FIFO underruns
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/fbc: disable FBC on FIFO underruns
2016-09-13 13:38 ` Paulo Zanoni
2016-09-13 13:56 ` Chris Wilson
@ 2016-09-13 17:07 ` Pandiyan, Dhinakaran
1 sibling, 0 replies; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2016-09-13 17:07 UTC (permalink / raw)
To: Zanoni, Paulo R; +Cc: intel-gfx, stefanr, stevenhoneyman
Looks good to me.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
On Tue, 2016-09-13 at 10:38 -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
>
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
>
> v2: New version, different implementation and commit message.
> v3: Clarify the fact that we run from an IRQ handler (Chris).
> v4: Also add the underrun_detected check at can_choose() to avoid
> misleading dmesg messages (DK).
> v5: Fix Engrish, use READ_ONCE on the unlocked read (Chris).
>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Lyude <cpaul@redhat.com>
> Cc: stevenhoneyman@gmail.com <stevenhoneyman@gmail.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_fbc.c | 67 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +
> 4 files changed, 73 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20b7743..5b1c241 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -971,6 +971,9 @@ struct intel_fbc {
> bool enabled;
> bool active;
>
> + bool underrun_detected;
> + struct work_struct underrun_work;
> +
> struct intel_fbc_state_cache {
> struct {
> unsigned int mode_flags;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index abe7a4d..0d05712 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1508,6 +1508,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
>
> /* intel_hdmi.c */
> void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 5dcb81a..10ca8cd 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -774,6 +774,14 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> struct intel_fbc *fbc = &dev_priv->fbc;
> struct intel_fbc_state_cache *cache = &fbc->state_cache;
>
> + /* We don't need to use a state cache here since this information is
> + * global for all CRTC.
> + */
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (!cache->plane.visible) {
> fbc->no_fbc_reason = "primary plane not visible";
> return false;
> @@ -859,6 +867,11 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
> return false;
> }
>
> + if (fbc->underrun_detected) {
> + fbc->no_fbc_reason = "underrun detected";
> + return false;
> + }
> +
> if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
> fbc->no_fbc_reason = "no enabled pipes can have FBC";
> return false;
> @@ -1221,6 +1234,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
> cancel_work_sync(&fbc->work.work);
> }
>
> +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private, fbc.underrun_work);
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + mutex_lock(&fbc->lock);
> +
> + /* Maybe we were scheduled twice. */
> + if (fbc->underrun_detected)
> + goto out;
> +
> + DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> + fbc->underrun_detected = true;
> +
> + intel_fbc_deactivate(dev_priv);
> +out:
> + mutex_unlock(&fbc->lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
> + * @dev_priv: i915 device instance
> + *
> + * Without FBC, most underruns are harmless and don't really cause too many
> + * problems, except for an annoying message on dmesg. With FBC, underruns can
> + * become black screens or even worse, especially when paired with bad
> + * watermarks. So in order for us to be on the safe side, completely disable FBC
> + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> + * already suggests that watermarks may be bad, so try to be as safe as
> + * possible.
> + *
> + * This function is called from the IRQ handler.
> + */
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> +{
> + struct intel_fbc *fbc = &dev_priv->fbc;
> +
> + if (!fbc_supported(dev_priv))
> + return;
> +
> + /* There's no guarantee that underrun_detected won't be set to true
> + * right after this check and before the work is scheduled, but that's
> + * not a problem since we'll check it again under the work function
> + * while FBC is locked. This check here is just to prevent us from
> + * unnecessarily scheduling the work, and it relies on the fact that we
> + * never switch underrun_detect back to false after it's true. */
> + if (READ_ONCE(fbc->underrun_detected))
> + return;
> +
> + schedule_work(&fbc->underrun_work);
> +}
> +
> /**
> * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
> * @dev_priv: i915 device instance
> @@ -1292,6 +1358,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> enum pipe pipe;
>
> INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> + INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> mutex_init(&fbc->lock);
> fbc->enabled = false;
> fbc->active = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 2aa7440..ebb4fed 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> DRM_ERROR("CPU pipe %c FIFO underrun\n",
> pipe_name(pipe));
> +
> + intel_fbc_handle_fifo_underrun_irq(dev_priv);
> }
>
> /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-09-13 17:07 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-11 1:18 [PATCH] drm/i915/fbc: disable FBC on FIFO underruns Paulo Zanoni
2016-06-11 6:56 ` ✗ Ro.CI.BAT: warning for " Patchwork
2016-06-13 11:47 ` [PATCH] " Stefan Richter
2016-06-13 14:33 ` Zanoni, Paulo R
2016-06-15 12:19 ` Stefan Richter
2016-08-12 23:24 ` Pandiyan, Dhinakaran
2016-08-13 9:05 ` Chris Wilson
2016-08-15 20:49 ` Zanoni, Paulo R
2016-08-15 22:36 ` [PATCH] " Paulo Zanoni
2016-09-02 5:58 ` Pandiyan, Dhinakaran
2016-09-02 14:45 ` Zanoni, Paulo R
2016-09-12 20:02 ` Paulo Zanoni
2016-09-12 20:25 ` Chris Wilson
2016-09-12 21:25 ` Zanoni, Paulo R
2016-09-13 13:38 ` Paulo Zanoni
2016-09-13 13:56 ` Chris Wilson
2016-09-13 17:07 ` Pandiyan, Dhinakaran
2016-08-16 5:57 ` ✗ Ro.CI.BAT: failure for drm/i915/fbc: disable FBC on FIFO underruns (rev2) Patchwork
2016-09-12 20:54 ` ✓ Fi.CI.BAT: success for drm/i915/fbc: disable FBC on FIFO underruns (rev3) Patchwork
2016-09-13 14:19 ` ✗ Fi.CI.BAT: warning for drm/i915/fbc: disable FBC on FIFO underruns (rev4) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.