Re: [Intel-gfx] [PATCH v10] drm/i915/skl: Add support for the SAGV, fix underrun hangs

2016-08-09 Thread Zanoni, Paulo R 
Em Ter, 2016-08-09 às 14:44 +0200, Maarten Lankhorst escreveu:
> Hey,
> 
> Op 08-08-16 om 23:03 schreef Lyude:
> > 
> > Since the watermark calculations for Skylake are still broken,
> > we're apt
> > to hitting underruns very easily under multi-monitor
> > configurations.
> > While it would be lovely if this was fixed, it's not. Another
> > problem
> > that's been coming from this however, is the mysterious issue of
> > underruns causing full system hangs. An easy way to reproduce this
> > with
> > a skylake system:
> > 
> > - Get a laptop with a skylake GPU, and hook up two external
> > monitors to
> >   it
> > - Move the cursor from the built-in LCD to one of the external
> > displays
> >   as quickly as you can
> > - You'll get a few pipe underruns, and eventually the entire system
> > will
> >   just freeze.
> > 
> > After doing a lot of investigation and reading through the bspec, I
> > found the existence of the SAGV, which is responsible for adjusting
> > the
> > system agent voltage and clock frequencies depending on how much
> > power
> > we need. According to the bspec:
> > 
> > "The display engine access to system memory is blocked during the
> >  adjustment time. SAGV defaults to enabled. Software must use the
> >  GT-driver pcode mailbox to disable SAGV when the display engine is
> > not
> >  able to tolerate the blocking time."
> > 
> > The rest of the bspec goes on to explain that software can simply
> > leave
> > the SAGV enabled, and disable it when we use interlaced pipes/have
> > more
> > then one pipe active.
> > 
> > Sure enough, with this patchset the system hangs resulting from
> > pipe
> > underruns on Skylake have completely vanished on my T460s.
> > Additionally,
> > the bspec mentions turning off the SAGV with more then one
> > pipe enabled
> > as a workaround for display underruns. While this patch doesn't
> > entirely
> > fix that, it looks like it does improve the situation a little bit
> > so
> > it's likely this is going to be required to make watermarks on
> > Skylake
> > fully functional.
> 
> I think this patch goes with v9 6/6 and v8 2-5/6. If you're only
> updating a single patch it might be better to send it in reply to the
> original patch.
> 
> I'm testing the whole series on my prerelease skylake, and running
> into this:
> 
> [ 2794.933149] kms_cursor_legacy: starting subtest 2x-flip-vs-cursor-
> legacy
> [ 2795.813970] [drm:skl_disable_sagv [i915]] *ERROR* Request to
> disable SAGV timed out
> 
> Value returned from skl_do_sagv_disable is always 0 for me, even when
> I bump the timeout to 15.

Yesterday I started testing this series, and I also noticed some visual
corruption: while browsing moderately-heavy websites on a maximized
Firefox, I could see the desktop background sort of "blinking" in the
screen (the background was not supposed to be visible). It looks like
the problem was introduced by patch 4, but I can't be 100% sure since
sometimes it's a little harder to reproduce it. Still, this is better
than the current "X doesn't work" state that we have without the
series.

> 
> ~Maarten
> 
> ___
> 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

Re: [Intel-gfx] [PATCH v10] drm/i915/skl: Add support for the SAGV, fix underrun hangs

2016-08-09 Thread Maarten Lankhorst 
Hey,

Op 08-08-16 om 23:03 schreef Lyude:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
>
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
>
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
>
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
>
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
>
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV   with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.

I think this patch goes with v9 6/6 and v8 2-5/6. If you're only updating a 
single patch it might be better to send it in reply to the original patch.

I'm testing the whole series on my prerelease skylake, and running into this:

[ 2794.933149] kms_cursor_legacy: starting subtest 2x-flip-vs-cursor-legacy
[ 2795.813970] [drm:skl_disable_sagv [i915]] *ERROR* Request to disable SAGV 
timed out

Value returned from skl_do_sagv_disable is always 0 for me, even when I bump 
the timeout to 15.

~Maarten

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

[Intel-gfx] [PATCH v10] drm/i915/skl: Add support for the SAGV, fix underrun hangs

2016-08-08 Thread Lyude 
Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:

- Get a laptop with a skylake GPU, and hook up two external monitors to
  it
- Move the cursor from the built-in LCD to one of the external displays
  as quickly as you can
- You'll get a few pipe underruns, and eventually the entire system will
  just freeze.

After doing a lot of investigation and reading through the bspec, I
found the existence of the SAGV, which is responsible for adjusting the
system agent voltage and clock frequencies depending on how much power
we need. According to the bspec:

"The display engine access to system memory is blocked during the
 adjustment time. SAGV defaults to enabled. Software must use the
 GT-driver pcode mailbox to disable SAGV when the display engine is not
 able to tolerate the blocking time."

The rest of the bspec goes on to explain that software can simply leave
the SAGV enabled, and disable it when we use interlaced pipes/have more
then one pipe active.

Sure enough, with this patchset the system hangs resulting from pipe
underruns on Skylake have completely vanished on my T460s. Additionally,
the bspec mentions turning off the SAGV with more then one pipe enabled
as a workaround for display underruns. While this patch doesn't entirely
fix that, it looks like it does improve the situation a little bit so
it's likely this is going to be required to make watermarks on Skylake
fully functional.

Changes since v9:
 - Only enable/disable sagv on Skylake
Changes since v8:
 - Add intel_state->modeset guard to the conditional for
   skl_enable_sagv()
Changes since v7:
 - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
   all we use it for anyway)
 - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
 - Fix a styling error that snuck past me
Changes since v6:
 - Protect skl_enable_sagv() with intel_state->modeset conditional in
   intel_atomic_commit_tail()
Changes since v5:
 - Don't use is_power_of_2. Makes things confusing
 - Don't use the old state to figure out whether or not to
   enable/disable the sagv, use the new one
 - Split the loop in skl_disable_sagv into it's own function
 - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
Changes since v4:
 - Use is_power_of_2 against active_crtcs to check whether we have > 1
   pipe enabled
 - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
   enabled
 - Call skl_sagv_enable/disable() from pre/post-plane updates
Changes since v3:
 - Use time_before() to compare timeout to jiffies
Changes since v2:
 - Really apply minor style nitpicks to patch this time
Changes since v1:
 - Added comments about this probably being one of the requirements to
   fixing Skylake's watermark issues
 - Minor style nitpicks from Matt Roper
 - Disable these functions on Broxton, since it doesn't have an SAGV

Reviewed-by: Matt Roper 
Reviewed-by: Maarten Lankhorst 
Signed-off-by: Lyude 
Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: sta...@vger.kernel.org

squash! drm/i915/skl: Add support for the SAGV, fix underrun hangs

squash! drm/i915/skl: Add support for the SAGV, fix underrun hangs

Signed-off-by: Lyude 
---
 drivers/gpu/drm/i915/i915_drv.h  |   2 +
 drivers/gpu/drm/i915/i915_reg.h  |   4 ++
 drivers/gpu/drm/i915/intel_display.c |  12 
 drivers/gpu/drm/i915/intel_drv.h |   2 +
 drivers/gpu/drm/i915/intel_pm.c  | 112 +++
 5 files changed, 132 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index feec00f..eb449f6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1948,6 +1948,8 @@ struct drm_i915_private {
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
 
+   bool skl_sagv_enabled;
+
struct {
/*
 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f38a5e2..f7e0bc2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7170,6 +7170,10 @@ enum {
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ  0x17
 #define   DISPLAY_IPS_CONTROL  0x19
 #define  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL  0x1A
+#define   GEN9_PCODE_SAGV_CONTROL  0x21
+#define GEN9_SAGV_DISABLE  0x0
+#define GEN9_SAGV_IS_DISABLED  0x1
+#define GEN9_SAGV_DYNAMIC_FREQ  0x3
 #define GEN6_PCODE_DATA

[Intel-gfx] [PATCH v10] drm/i915/skl: Add support for the SAGV, fix underrun hangs

2016-08-08 Thread Lyude 
Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:

- Get a laptop with a skylake GPU, and hook up two external monitors to
  it
- Move the cursor from the built-in LCD to one of the external displays
  as quickly as you can
- You'll get a few pipe underruns, and eventually the entire system will
  just freeze.

After doing a lot of investigation and reading through the bspec, I
found the existence of the SAGV, which is responsible for adjusting the
system agent voltage and clock frequencies depending on how much power
we need. According to the bspec:

"The display engine access to system memory is blocked during the
 adjustment time. SAGV defaults to enabled. Software must use the
 GT-driver pcode mailbox to disable SAGV when the display engine is not
 able to tolerate the blocking time."

The rest of the bspec goes on to explain that software can simply leave
the SAGV enabled, and disable it when we use interlaced pipes/have more
then one pipe active.

Sure enough, with this patchset the system hangs resulting from pipe
underruns on Skylake have completely vanished on my T460s. Additionally,
the bspec mentions turning off the SAGV with more then one pipe enabled
as a workaround for display underruns. While this patch doesn't entirely
fix that, it looks like it does improve the situation a little bit so
it's likely this is going to be required to make watermarks on Skylake
fully functional.

Changes since v9:
 - Only enable/disable sagv on Skylake
Changes since v8:
 - Add intel_state->modeset guard to the conditional for
   skl_enable_sagv()
Changes since v7:
 - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
   all we use it for anyway)
 - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
 - Fix a styling error that snuck past me
Changes since v6:
 - Protect skl_enable_sagv() with intel_state->modeset conditional in
   intel_atomic_commit_tail()
Changes since v5:
 - Don't use is_power_of_2. Makes things confusing
 - Don't use the old state to figure out whether or not to
   enable/disable the sagv, use the new one
 - Split the loop in skl_disable_sagv into it's own function
 - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
Changes since v4:
 - Use is_power_of_2 against active_crtcs to check whether we have > 1
   pipe enabled
 - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
   enabled
 - Call skl_sagv_enable/disable() from pre/post-plane updates
Changes since v3:
 - Use time_before() to compare timeout to jiffies
Changes since v2:
 - Really apply minor style nitpicks to patch this time
Changes since v1:
 - Added comments about this probably being one of the requirements to
   fixing Skylake's watermark issues
 - Minor style nitpicks from Matt Roper
 - Disable these functions on Broxton, since it doesn't have an SAGV

Reviewed-by: Matt Roper 
Reviewed-by: Maarten Lankhorst 
Signed-off-by: Lyude 
Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: sta...@vger.kernel.org

squash! drm/i915/skl: Add support for the SAGV, fix underrun hangs

squash! drm/i915/skl: Add support for the SAGV, fix underrun hangs

Signed-off-by: Lyude 
---
 drivers/gpu/drm/i915/i915_drv.h  |   2 +
 drivers/gpu/drm/i915/i915_reg.h  |   4 ++
 drivers/gpu/drm/i915/intel_display.c |  12 
 drivers/gpu/drm/i915/intel_drv.h |   2 +
 drivers/gpu/drm/i915/intel_pm.c  | 112 +++
 5 files changed, 132 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index feec00f..eb449f6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1948,6 +1948,8 @@ struct drm_i915_private {
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
 
+   bool skl_sagv_enabled;
+
struct {
/*
 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f38a5e2..f7e0bc2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7170,6 +7170,10 @@ enum {
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ  0x17
 #define   DISPLAY_IPS_CONTROL  0x19
 #define  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL  0x1A
+#define   GEN9_PCODE_SAGV_CONTROL  0x21
+#define GEN9_SAGV_DISABLE  0x0
+#define GEN9_SAGV_IS_DISABLED  0x1
+#define GEN9_SAGV_DYNAMIC_FREQ  0x3
 #define GEN6_PCODE_DATA