[PATCH 0/2] drm/i915: Fix for resume regression in ilk-bdw due to watermarks

[PATCH 0/2] drm/i915: Fix for resume regression in ilk-bdw due to watermarks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

My ILK was very unhappy runnin BAT tests. Caused by watermark fail during resume.
Here's the smallest fix I thought of.

Ville Syrjälä (2):
  drm/i915: Don't leave old junk in ilk active watermarks on readout
  drm/i915: Ignore stale wm register values on resume on ilk-bdw

 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 drivers/gpu/drm/i915/intel_pm.c      | 2 ++
 2 files changed, 8 insertions(+)

Cc: Matt Roper <matthew.d.roper@intel.com>
-- 
2.7.4

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

[PATCH 1/2] drm/i915: Don't leave old junk in ilk active watermarks on readout

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we read out the watermark state from the hardware we're supposed to
transfer that into the active watermarks, but currently we fail to any
part of the active watermarks that isn't explicitly written. Let's clear
it all upfront.

Looks like this has been like this since the beginning, when I added the
readout. No idea why I didn't clear it up.

Cc: Matt Roper <matthew.d.roper@intel.com>
Fixes: 243e6a44b9ca ("drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1bb0f9ac104c..17c9f9865a18 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3904,6 +3904,8 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
 
+	memset(active, 0, sizeof(*active));
+
 	active->pipe_enabled = intel_crtc->active;
 
 	if (active->pipe_enabled) {
-- 
2.7.4

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

[PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we resume the watermark register may contain some BIOS leftovers,
or just the hardware reset values. We should ignore those as the
pipes will be off anyway, and so frobbing around with intermediate
watermarks doesn't make much sense.

In fact I think we should just throw the skip_intermediate_wm flag
out, and instead properly sanitize the "active" watermarks to match
the current plane and pipe states. The actual wm state readout might
also need a bit of work. But for now, let's continue with the
skip_intermediate_wm to keep the fix more minimal.

Fixes this sort of errors on resume
[drm:ilk_validate_pipe_wm] LP0 watermark invalid
[drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
[drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
and a boatload of subsequent modeset BAT fails on my ILK.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a08c4a45b8d3..938653063f15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
 			return ret;
 		}
+	} else if (dev_priv->display.compute_intermediate_wm) {
+		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
+			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
@@ -15991,6 +15994,9 @@ retry:
 
 		state->acquire_ctx = &ctx;
 
+		/* ignore any reset values/BIOS leftovers in the WM registers */
+		to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			/*
 			 * Force recalculation even if we restore
-- 
2.7.4

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

✗ Ro.CI.BAT: failure for drm/i915: Fix for resume regression in ilk-bdw due to watermarks

[Patchwork]

== Series Details ==

Series: drm/i915: Fix for resume regression in ilk-bdw due to watermarks
URL   : https://patchwork.freedesktop.org/series/7155/
State : failure

== Summary ==

  CC      drivers/usb/host/xhci-trace.o
  CC [M]  drivers/video/fbdev/core/sysfillrect.o
  CC [M]  drivers/video/fbdev/core/syscopyarea.o
  CC      drivers/usb/host/xhci-pci.o
  CC [M]  drivers/video/fbdev/core/sysimgblt.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/usb/core/built-in.o
  CC [M]  drivers/video/fbdev/core/fb_sys_fops.o
  CC      drivers/tty/serial/8250/8250_pci.o
  LD [M]  drivers/usb/serial/usbserial.o
  LD      drivers/video/console/built-in.o
  CC      drivers/tty/serial/8250/8250_early.o
  LD      drivers/video/fbdev/core/fb.o
  LD      drivers/tty/serial/8250/8250.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      drivers/video/fbdev/built-in.o
  LD      drivers/video/built-in.o
  LD [M]  drivers/net/ethernet/intel/igbvf/igbvf.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD      drivers/tty/vt/built-in.o
  LD      drivers/tty/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
make: *** [drivers] Error 2

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

Re: [PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

[Matt Roper]

On Fri, May 13, 2016 at 05:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When we resume the watermark register may contain some BIOS leftovers,
> or just the hardware reset values. We should ignore those as the
> pipes will be off anyway, and so frobbing around with intermediate
> watermarks doesn't make much sense.
> 
> In fact I think we should just throw the skip_intermediate_wm flag
> out, and instead properly sanitize the "active" watermarks to match
> the current plane and pipe states.

That's what the flag was originally added for.  The easiest way to
sanitize the active watermarks is to run the current atomic state
through our whole atomic check process to calculate all derived state
plus the watermark values, and then just push the optimal values into
the hardware.  The flag tells us to just skip calculating the
intermediate values (which might be bogus and/or fail anyway) since all
we care about are the final/optimal values.

Anyway, this looks pretty similar to the fix I wrote right before I went
OOO, but never got a chance to send out; I set the skip flag when the
atomic state was created in the suspend function and you do it in the
resume function, but I think the result should be the same either way,
so overall lgtm.

For both patches:
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> The actual wm state readout might
> also need a bit of work. But for now, let's continue with the
> skip_intermediate_wm to keep the fix more minimal.
> 
> Fixes this sort of errors on resume
> [drm:ilk_validate_pipe_wm] LP0 watermark invalid
> [drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
> [drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
> and a boatload of subsequent modeset BAT fails on my ILK.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a08c4a45b8d3..938653063f15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
>  			return ret;
>  		}
> +	} else if (dev_priv->display.compute_intermediate_wm) {
> +		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> +			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> @@ -15991,6 +15994,9 @@ retry:
>  
>  		state->acquire_ctx = &ctx;
>  
> +		/* ignore any reset values/BIOS leftovers in the WM registers */
> +		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> +
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			/*
>  			 * Force recalculation even if we restore
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw (v2)

[Matt Roper]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we resume the watermark register may contain some BIOS leftovers,
or just the hardware reset values. We should ignore those as the
pipes will be off anyway, and so frobbing around with intermediate
watermarks doesn't make much sense.

In fact I think we should just throw the skip_intermediate_wm flag
out, and instead properly sanitize the "active" watermarks to match
the current plane and pipe states. The actual wm state readout might
also need a bit of work. But for now, let's continue with the
skip_intermediate_wm to keep the fix more minimal.

Fixes this sort of errors on resume
[drm:ilk_validate_pipe_wm] LP0 watermark invalid
[drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
[drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
and a boatload of subsequent modeset BAT fails on my ILK.

v2:
 - Rebase; the SKL atomic WM patches that just landed changed the WM
   structure fields in intel_crtc_state slightly.  (Matt)

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Ville sent his patch just as the SKL atomic wm code was being merged to dinq,
so pipe_config->wm.intermediate becomes pipe_config->wm.ilk.intermediate now.
The logic remains the same, so just re-sending a rebased copy of his patch
so CI knows how to build it.

 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0194390..e6f763e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
 			return ret;
 		}
+	} else if (dev_priv->display.compute_intermediate_wm) {
+		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
+			pipe_config->wm.ilk.intermediate = pipe_config->wm.ilk.optimal;
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
@@ -15970,6 +15973,9 @@ retry:
 
 		state->acquire_ctx = &ctx;
 
+		/* ignore any reset values/BIOS leftovers in the WM registers */
+		to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			/*
 			 * Force recalculation even if we restore
-- 
2.1.4

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

Re: [PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

[Ville Syrjälä]

On Fri, May 13, 2016 at 09:58:55AM -0700, Matt Roper wrote:
> On Fri, May 13, 2016 at 05:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When we resume the watermark register may contain some BIOS leftovers,
> > or just the hardware reset values. We should ignore those as the
> > pipes will be off anyway, and so frobbing around with intermediate
> > watermarks doesn't make much sense.
> > 
> > In fact I think we should just throw the skip_intermediate_wm flag
> > out, and instead properly sanitize the "active" watermarks to match
> > the current plane and pipe states.
> 
> That's what the flag was originally added for.  The easiest way to
> sanitize the active watermarks is to run the current atomic state
> through our whole atomic check process to calculate all derived state
> plus the watermark values, and then just push the optimal values into
> the hardware.  The flag tells us to just skip calculating the
> intermediate values (which might be bogus and/or fail anyway) since all
> we care about are the final/optimal values.

We should still care about the transition to avoid any underruns,
so we should rather have proper intermediate watermarks here. I
think the problem we're working around here is basically poor hw state
readout. That is, we should more or less just discard the results of
the hw readout for any plane/pipe that is disabled.

The end result should be one less special case to worry about. The less
we have of those the better.

If we would ever run into a genuine case where we couldn't do the
intermediate watermarks properly, then we could always just shut
everything down first, and then bring it back up from scratch.

> 
> Anyway, this looks pretty similar to the fix I wrote right before I went
> OOO, but never got a chance to send out; I set the skip flag when the
> atomic state was created in the suspend function and you do it in the
> resume function, but I think the result should be the same either way,
> so overall lgtm.
> 
> For both patches:
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> > The actual wm state readout might
> > also need a bit of work. But for now, let's continue with the
> > skip_intermediate_wm to keep the fix more minimal.
> > 
> > Fixes this sort of errors on resume
> > [drm:ilk_validate_pipe_wm] LP0 watermark invalid
> > [drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
> > [drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
> > and a boatload of subsequent modeset BAT fails on my ILK.
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a08c4a45b8d3..938653063f15 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> >  			return ret;
> >  		}
> > +	} else if (dev_priv->display.compute_intermediate_wm) {
> > +		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> > +			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> > @@ -15991,6 +15994,9 @@ retry:
> >  
> >  		state->acquire_ctx = &ctx;
> >  
> > +		/* ignore any reset values/BIOS leftovers in the WM registers */
> > +		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > +
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  			/*
> >  			 * Force recalculation even if we restore
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

[Matt Roper]

On Fri, May 13, 2016 at 08:18:18PM +0300, Ville Syrjälä wrote:
> On Fri, May 13, 2016 at 09:58:55AM -0700, Matt Roper wrote:
> > On Fri, May 13, 2016 at 05:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > When we resume the watermark register may contain some BIOS leftovers,
> > > or just the hardware reset values. We should ignore those as the
> > > pipes will be off anyway, and so frobbing around with intermediate
> > > watermarks doesn't make much sense.
> > > 
> > > In fact I think we should just throw the skip_intermediate_wm flag
> > > out, and instead properly sanitize the "active" watermarks to match
> > > the current plane and pipe states.
> > 
> > That's what the flag was originally added for.  The easiest way to
> > sanitize the active watermarks is to run the current atomic state
> > through our whole atomic check process to calculate all derived state
> > plus the watermark values, and then just push the optimal values into
> > the hardware.  The flag tells us to just skip calculating the
> > intermediate values (which might be bogus and/or fail anyway) since all
> > we care about are the final/optimal values.
> 
> We should still care about the transition to avoid any underruns,
> so we should rather have proper intermediate watermarks here. I
> think the problem we're working around here is basically poor hw state
> readout. That is, we should more or less just discard the results of
> the hw readout for any plane/pipe that is disabled.

What I mean is that during initial boot, the sanitization we do is only
programming the watermarks and not changing the configuration at all.

For resume that's a bit different at the moment since we read out the
hardware state and then immediately perform an atomic commit (with the
saved state) without having sanitized the watermarks first.  So I think
another potential fix might be to just call sanitize_watermarks() after
reading out the hardware state, but before committing the saved state.
I think that approach is what you were referring to above by "sanitize
the 'active' watermarks to match the current plane and pipe states," but
unless we also changed our sanitization logic, we'd still have the
skip_intermediate_wm flag since it gets used inside the sanitization
code.

> 
> The end result should be one less special case to worry about. The less
> we have of those the better.
> 
> If we would ever run into a genuine case where we couldn't do the
> intermediate watermarks properly, then we could always just shut
> everything down first, and then bring it back up from scratch.

Yeah, that's a good point.


Matt

> 
> > 
> > Anyway, this looks pretty similar to the fix I wrote right before I went
> > OOO, but never got a chance to send out; I set the skip flag when the
> > atomic state was created in the suspend function and you do it in the
> > resume function, but I think the result should be the same either way,
> > so overall lgtm.
> > 
> > For both patches:
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > 
> > > The actual wm state readout might
> > > also need a bit of work. But for now, let's continue with the
> > > skip_intermediate_wm to keep the fix more minimal.
> > > 
> > > Fixes this sort of errors on resume
> > > [drm:ilk_validate_pipe_wm] LP0 watermark invalid
> > > [drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
> > > [drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
> > > and a boatload of subsequent modeset BAT fails on my ILK.
> > > 
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a08c4a45b8d3..938653063f15 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > >  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> > >  			return ret;
> > >  		}
> > > +	} else if (dev_priv->display.compute_intermediate_wm) {
> > > +		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> > > +			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
> > >  	}
> > >  
> > >  	if (INTEL_INFO(dev)->gen >= 9) {
> > > @@ -15991,6 +15994,9 @@ retry:
> > >  
> > >  		state->acquire_ctx = &ctx;
> > >  
> > > +		/* ignore any reset values/BIOS leftovers in the WM registers */
> > > +		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > > +
> > >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > >  			/*
> > >  			 * Force recalculation even if we restore
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

[Ville Syrjälä]

On Fri, May 13, 2016 at 10:35:16AM -0700, Matt Roper wrote:
> On Fri, May 13, 2016 at 08:18:18PM +0300, Ville Syrjälä wrote:
> > On Fri, May 13, 2016 at 09:58:55AM -0700, Matt Roper wrote:
> > > On Fri, May 13, 2016 at 05:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > When we resume the watermark register may contain some BIOS leftovers,
> > > > or just the hardware reset values. We should ignore those as the
> > > > pipes will be off anyway, and so frobbing around with intermediate
> > > > watermarks doesn't make much sense.
> > > > 
> > > > In fact I think we should just throw the skip_intermediate_wm flag
> > > > out, and instead properly sanitize the "active" watermarks to match
> > > > the current plane and pipe states.
> > > 
> > > That's what the flag was originally added for.  The easiest way to
> > > sanitize the active watermarks is to run the current atomic state
> > > through our whole atomic check process to calculate all derived state
> > > plus the watermark values, and then just push the optimal values into
> > > the hardware.  The flag tells us to just skip calculating the
> > > intermediate values (which might be bogus and/or fail anyway) since all
> > > we care about are the final/optimal values.
> > 
> > We should still care about the transition to avoid any underruns,
> > so we should rather have proper intermediate watermarks here. I
> > think the problem we're working around here is basically poor hw state
> > readout. That is, we should more or less just discard the results of
> > the hw readout for any plane/pipe that is disabled.
> 
> What I mean is that during initial boot, the sanitization we do is only
> programming the watermarks and not changing the configuration at all.
> 
> For resume that's a bit different at the moment since we read out the
> hardware state and then immediately perform an atomic commit (with the
> saved state) without having sanitized the watermarks first.  So I think
> another potential fix might be to just call sanitize_watermarks() after
> reading out the hardware state, but before committing the saved state.
> I think that approach is what you were referring to above by "sanitize
> the 'active' watermarks to match the current plane and pipe states," but
> unless we also changed our sanitization logic, we'd still have the
> skip_intermediate_wm flag since it gets used inside the sanitization
> code.

Invoking the entire machinery for this is pretty much overkill, I think.
And it requires these special cases in the normal modeset paths. If we
just did the readout in a better way, it would all be dealt with by the
readout code basically. We might still want to do a manual
active->hw->program hardware step right after the read hardware->hw->active
step to make sure both the cached hw and active values, and the actual
hardware are all in sync. This explicit progamming step would basically
be the sanitize part, with the first part being just the more proper
hardware readout.

> 
> > 
> > The end result should be one less special case to worry about. The less
> > we have of those the better.
> > 
> > If we would ever run into a genuine case where we couldn't do the
> > intermediate watermarks properly, then we could always just shut
> > everything down first, and then bring it back up from scratch.
> 
> Yeah, that's a good point.
> 
> 
> Matt
> 
> > 
> > > 
> > > Anyway, this looks pretty similar to the fix I wrote right before I went
> > > OOO, but never got a chance to send out; I set the skip flag when the
> > > atomic state was created in the suspend function and you do it in the
> > > resume function, but I think the result should be the same either way,
> > > so overall lgtm.
> > > 
> > > For both patches:
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > 
> > > > The actual wm state readout might
> > > > also need a bit of work. But for now, let's continue with the
> > > > skip_intermediate_wm to keep the fix more minimal.
> > > > 
> > > > Fixes this sort of errors on resume
> > > > [drm:ilk_validate_pipe_wm] LP0 watermark invalid
> > > > [drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
> > > > [drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
> > > > and a boatload of subsequent modeset BAT fails on my ILK.
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index a08c4a45b8d3..938653063f15 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > >  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> > > >  			return ret;
> > > >  		}
> > > > +	} else if (dev_priv->display.compute_intermediate_wm) {
> > > > +		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> > > > +			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
> > > >  	}
> > > >  
> > > >  	if (INTEL_INFO(dev)->gen >= 9) {
> > > > @@ -15991,6 +15994,9 @@ retry:
> > > >  
> > > >  		state->acquire_ctx = &ctx;
> > > >  
> > > > +		/* ignore any reset values/BIOS leftovers in the WM registers */
> > > > +		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > > > +
> > > >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > >  			/*
> > > >  			 * Force recalculation even if we restore
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

✗ Ro.CI.BAT: failure for drm/i915: Fix for resume regression in ilk-bdw due to watermarks (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix for resume regression in ilk-bdw due to watermarks (rev2)
URL   : https://patchwork.freedesktop.org/series/7155/
State : failure

== Summary ==

Series 7155v2 drm/i915: Fix for resume regression in ilk-bdw due to watermarks
http://patchwork.freedesktop.org/api/1.0/series/7155/revisions/2/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ro-ilk1-i5-650)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)

ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-bsw-n3050     total:219  pass:175  dwarn:0   dfail:0   fail:2   skip:42 
ro-byt-n2820     total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
ro-hsw-i3-4010u  total:218  pass:193  dwarn:0   dfail:0   fail:0   skip:25 
ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
ro-ilk-i7-620lm  total:219  pass:151  dwarn:0   dfail:0   fail:1   skip:67 
ro-ilk1-i5-650   total:214  pass:152  dwarn:0   dfail:0   fail:1   skip:61 
ro-ivb-i7-3770   total:219  pass:183  dwarn:0   dfail:0   fail:0   skip:36 
ro-ivb2-i7-3770  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-skl-i7-6700hq total:214  pass:189  dwarn:0   dfail:0   fail:0   skip:25 
ro-snb-i7-2620M  total:219  pass:177  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/RO_Patchwork_897/

accd824 drm-intel-nightly: 2016y-05m-13d-14h-38m-15s UTC integration manifest
4fc355d drm/i915: Ignore stale wm register values on resume on ilk-bdw (v2)
03bf66b drm/i915: Don't leave old junk in ilk active watermarks on readout

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

Re: ✗ Ro.CI.BAT: failure for drm/i915: Fix for resume regression in ilk-bdw due to watermarks (rev2)

[Matt Roper]

On Fri, May 13, 2016 at 07:04:07PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix for resume regression in ilk-bdw due to watermarks (rev2)
> URL   : https://patchwork.freedesktop.org/series/7155/
> State : failure
> 
> == Summary ==
> 
> Series 7155v2 drm/i915: Fix for resume regression in ilk-bdw due to watermarks
> http://patchwork.freedesktop.org/api/1.0/series/7155/revisions/2/mbox
> 
> Test drv_hangman:
>         Subgroup error-state-basic:
>                 fail       -> PASS       (ro-ilk1-i5-650)
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-cmd:
>                 pass       -> FAIL       (ro-byt-n2820)

https://bugs.freedesktop.org/show_bug.cgi?id=95372

Pushed to dinq; thanks for the patches.


Matt

> 
> ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
> ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
> ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
> ro-bsw-n3050     total:219  pass:175  dwarn:0   dfail:0   fail:2   skip:42 
> ro-byt-n2820     total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
> ro-hsw-i3-4010u  total:218  pass:193  dwarn:0   dfail:0   fail:0   skip:25 
> ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
> ro-ilk-i7-620lm  total:219  pass:151  dwarn:0   dfail:0   fail:1   skip:67 
> ro-ilk1-i5-650   total:214  pass:152  dwarn:0   dfail:0   fail:1   skip:61 
> ro-ivb-i7-3770   total:219  pass:183  dwarn:0   dfail:0   fail:0   skip:36 
> ro-ivb2-i7-3770  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
> ro-skl-i7-6700hq total:214  pass:189  dwarn:0   dfail:0   fail:0   skip:25 
> ro-snb-i7-2620M  total:219  pass:177  dwarn:0   dfail:0   fail:1   skip:41 
> 
> Results at /archive/results/CI_IGT_test/RO_Patchwork_897/
> 
> accd824 drm-intel-nightly: 2016y-05m-13d-14h-38m-15s UTC integration manifest
> 4fc355d drm/i915: Ignore stale wm register values on resume on ilk-bdw (v2)
> 03bf66b drm/i915: Don't leave old junk in ilk active watermarks on readout
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx