[PATCH 0/5] drm/i915: CHV regressions fixes

[PATCH 0/5] drm/i915: CHV regressions fixes

[ville.syrjala]

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

CHV display side got broken on multiple fronts. This series undoes the
damage a bit and I tossed in a few cleanupish things at the end for fun.

Ville Syrjälä (5):
  Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."
  drm/i915: Pass the correct crtc state to .update_plane()
  drm/i915: Fix watermarks for VLV/CHV
  drm/i915: Wait for vblank after cxsr disable in pre_plane_update
  drm/i915: s/crtc_state/old_crtc_state/ in intel_atomic_commit()

 drivers/gpu/drm/i915/intel_atomic.c       |   3 +-
 drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
 drivers/gpu/drm/i915/intel_display.c      | 108 ++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h          |   2 +-
 drivers/gpu/drm/i915/intel_psr.c          |   3 +-
 5 files changed, 71 insertions(+), 49 deletions(-)

-- 
2.4.10

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

[PATCH 1/5] Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."

[ville.syrjala]

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

This reverts commit a38c274faad0ec6aba692e294ec751d04dbba803.

PSR causes all sorts of vblank wait timeouts and whanot on CHV. Disable
it again.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: a38c274faad0 ("drm/i915: Enable PSR by default on Valleyview and Cherryview.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b1413beb00d1..38e95185d9c6 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -781,8 +781,7 @@ void intel_psr_init(struct drm_device *dev)
 
 	/* Per platform default */
 	if (i915.enable_psr == -1) {
-		if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
-		    IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 			i915.enable_psr = 1;
 		else
 			i915.enable_psr = 0;
-- 
2.4.10

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

Re: [PATCH 1/5] Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."

[Vivi, Rodrigo]

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I confirm this is hitting BAT hard on CHV.

I'm working here to handle this PSR exit on VBlanks on CHV already, but
the vblanks spinlocks are giving me headaches... 

So better to revert.

thanks for the finding, ideas and this revert...

On Wed, 2016-03-09 at 19:07 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit a38c274faad0ec6aba692e294ec751d04dbba803.
> 
> PSR causes all sorts of vblank wait timeouts and whanot on CHV.
> Disable
> it again.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: a38c274faad0 ("drm/i915: Enable PSR by default on Valleyview
> and Cherryview.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index b1413beb00d1..38e95185d9c6 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -781,8 +781,7 @@ void intel_psr_init(struct drm_device *dev)
>  
>  	/* Per platform default */
>  	if (i915.enable_psr == -1) {
> -		if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
> -		    IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  			i915.enable_psr = 1;
>  		else
>  			i915.enable_psr = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."

[Maarten Lankhorst]

Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit a38c274faad0ec6aba692e294ec751d04dbba803.
>
> PSR causes all sorts of vblank wait timeouts and whanot on CHV. Disable
> it again.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: a38c274faad0 ("drm/i915: Enable PSR by default on Valleyview and Cherryview.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Considering those issues, is there a bugzilla linked to this then so we could track when to enable it again?

If this commit has more specifics, or at least some dmesg snippets:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Pass the correct crtc state to .update_plane()

[ville.syrjala]

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

Pass the current crtc state, not the old crtc state, to the
.update_plane() hook.

Noticed on BSW when PRIMSIZE was getting programmed to a stale value
which produced utter garbage on screen eg. wwhen going from 1920x1080
to 1024x768.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: a758e6845825 ("drm/i915: Do not use commit_plane for sprite planes.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index e0b851a0004a..7de7721f65bc 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -195,12 +195,10 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 	struct intel_plane_state *intel_state =
 		to_intel_plane_state(plane->state);
 	struct drm_crtc *crtc = plane->state->crtc ?: old_state->crtc;
-	struct drm_crtc_state *crtc_state =
-		drm_atomic_get_existing_crtc_state(old_state->state, crtc);
 
 	if (intel_state->visible)
 		intel_plane->update_plane(plane,
-					  to_intel_crtc_state(crtc_state),
+					  to_intel_crtc_state(crtc->state),
 					  intel_state);
 	else
 		intel_plane->disable_plane(plane, crtc);
-- 
2.4.10

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

Re: [PATCH 2/5] drm/i915: Pass the correct crtc state to .update_plane()

[Maarten Lankhorst]

Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pass the current crtc state, not the old crtc state, to the
> .update_plane() hook.
>
> Noticed on BSW when PRIMSIZE was getting programmed to a stale value
> which produced utter garbage on screen eg. wwhen going from 1920x1080
> to 1024x768.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: a758e6845825 ("drm/i915: Do not use commit_plane for sprite planes.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

For patch 2, 4 and 5:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Fix watermarks for VLV/CHV

[ville.syrjala]

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

commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
broke thigns by removing the pre vs. post wm update distinction. We also
lost the pre plane wm update entirely for VLV/CHV from the crtc enable
path.

This caused underruns on modeset and plane enable/disable on CHV,
and often those can lead to a dead pipe.

So let's bring back the pre vs. post thing, and let's toss in an
explicit wm update to valleyview_crtc_enable() to avoid having to
put it into the common code.

This is more or less a partial revert of the offending commit.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  3 ++-
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 6a661e796328..79448f1c8b8d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -96,7 +96,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
-	crtc_state->wm_changed = false;
+	crtc_state->update_wm_pre = false;
+	crtc_state->update_wm_post = false;
 	crtc_state->fb_changed = false;
 	crtc_state->wm.need_postvbl_update = false;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 62d36a7b3398..8b44e0652740 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4919,7 +4919,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	crtc->wm.cxsr_allowed = true;
 
-	if (pipe_config->wm_changed && pipe_config->base.active)
+	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)
@@ -5001,7 +5001,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	 */
 	if (dev_priv->display.initial_watermarks != NULL)
 		dev_priv->display.initial_watermarks(pipe_config);
-	else if (pipe_config->wm_changed)
+	else if (pipe_config->update_wm_pre)
 		intel_update_watermarks(&crtc->base);
 }
 
@@ -6372,6 +6372,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
 
 	assert_vblank_disabled(crtc);
@@ -11994,19 +11995,27 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on || turn_off) {
-		pipe_config->wm_changed = true;
+	if (turn_on) {
+		pipe_config->update_wm_pre = true;
+
+		/* must disable cxsr around plane enable/disable */
+		if (plane->type != DRM_PLANE_TYPE_CURSOR)
+			pipe_config->disable_cxsr = true;
+	} else if (turn_off) {
+		pipe_config->update_wm_post = true;
 
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR)
 			pipe_config->disable_cxsr = true;
 	} else if (intel_wm_need_update(plane, plane_state)) {
-		pipe_config->wm_changed = true;
+		/* FIXME bollocks */
+		pipe_config->update_wm_pre = true;
+		pipe_config->update_wm_post = true;
 	}
 
 	/* Pre-gen9 platforms need two-step watermark updates */
-	if (pipe_config->wm_changed && INTEL_INFO(dev)->gen < 9 &&
-	    dev_priv->display.optimize_watermarks)
+	if ((pipe_config->update_wm_pre || pipe_config->update_wm_post) &&
+	    INTEL_INFO(dev)->gen < 9 && dev_priv->display.optimize_watermarks)
 		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
 
 	if (visible || was_visible)
@@ -12106,7 +12115,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	if (mode_changed && !crtc_state->active)
-		pipe_config->wm_changed = true;
+		pipe_config->update_wm_post = true;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -13645,12 +13654,12 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
 		return true;
 
 	/* wm changes, need vblank before final wm's */
-	if (crtc_state->wm_changed)
+	if (crtc_state->update_wm_post)
 		return true;
 
 	/*
 	 * cxsr is re-enabled after vblank.
-	 * This is already handled by crtc_state->wm_changed,
+	 * This is already handled by crtc_state->update_wm_post,
 	 * but added for clarity.
 	 */
 	if (crtc_state->disable_cxsr)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b2d66d8dd7f..c33e9690df51 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -420,7 +420,7 @@ struct intel_crtc_state {
 
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
-	bool wm_changed; /* watermarks are updated */
+	bool update_wm_pre, update_wm_post; /* watermarks are updated */
 	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
-- 
2.4.10

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

Re: [PATCH 3/5] drm/i915: Fix watermarks for VLV/CHV

[Maarten Lankhorst]

Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> broke thigns by removing the pre vs. post wm update distinction. We also
> lost the pre plane wm update entirely for VLV/CHV from the crtc enable
> path.
>
> This caused underruns on modeset and plane enable/disable on CHV,
> and often those can lead to a dead pipe.
>
> So let's bring back the pre vs. post thing, and let's toss in an
> explicit wm update to valleyview_crtc_enable() to avoid having to
> put it into the common code.
>
> This is more or less a partial revert of the offending commit.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  3 ++-
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 6a661e796328..79448f1c8b8d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -96,7 +96,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->disable_cxsr = false;
> -	crtc_state->wm_changed = false;
> +	crtc_state->update_wm_pre = false;
> +	crtc_state->update_wm_post = false;
>  	crtc_state->fb_changed = false;
>  	crtc_state->wm.need_postvbl_update = false;
There's already a wm.need_postvbl_update.
could we use it for non-atomic platforms, and add a wm.need_intermediate_update ?
This could probably be used for ILK too.

I believe that if a driver provides the atomic wm functions those functions should also set the wm.*_update members themselves,
while for the watermarks that aren't converted yet it should be provided by the core.

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

Re: [PATCH 3/5] drm/i915: Fix watermarks for VLV/CHV

[Ville Syrjälä]

On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote:
> Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> > broke thigns by removing the pre vs. post wm update distinction. We also
> > lost the pre plane wm update entirely for VLV/CHV from the crtc enable
> > path.
> >
> > This caused underruns on modeset and plane enable/disable on CHV,
> > and often those can lead to a dead pipe.
> >
> > So let's bring back the pre vs. post thing, and let's toss in an
> > explicit wm update to valleyview_crtc_enable() to avoid having to
> > put it into the common code.
> >
> > This is more or less a partial revert of the offending commit.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c  |  3 ++-
> >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> >  3 files changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 6a661e796328..79448f1c8b8d 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -96,7 +96,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >  	crtc_state->update_pipe = false;
> >  	crtc_state->disable_lp_wm = false;
> >  	crtc_state->disable_cxsr = false;
> > -	crtc_state->wm_changed = false;
> > +	crtc_state->update_wm_pre = false;
> > +	crtc_state->update_wm_post = false;
> >  	crtc_state->fb_changed = false;
> >  	crtc_state->wm.need_postvbl_update = false;
> There's already a wm.need_postvbl_update.
> could we use it for non-atomic platforms, and add a wm.need_intermediate_update ?
> This could probably be used for ILK too.

Dunno. All I know is the regression is now on it's way 4.5 and needs to
be dealt with ASAP. So no major overhaul at this point.

> 
> I believe that if a driver provides the atomic wm functions those functions should also set the wm.*_update members themselves,
> while for the watermarks that aren't converted yet it should be provided by the core.
> 
> ~Maarten

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

Re: [PATCH 3/5] drm/i915: Fix watermarks for VLV/CHV

[Maarten Lankhorst]

Op 10-03-16 om 11:33 schreef Ville Syrjälä:
> On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote:
>> Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
>>> broke thigns by removing the pre vs. post wm update distinction. We also
>>> lost the pre plane wm update entirely for VLV/CHV from the crtc enable
>>> path.
>>>
>>> This caused underruns on modeset and plane enable/disable on CHV,
>>> and often those can lead to a dead pipe.
>>>
>>> So let's bring back the pre vs. post thing, and let's toss in an
>>> explicit wm update to valleyview_crtc_enable() to avoid having to
>>> put it into the common code.
>>>
>>> This is more or less a partial revert of the offending commit.
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: drm-intel-fixes@lists.freedesktop.org
>>> Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_atomic.c  |  3 ++-
>>>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>>  3 files changed, 22 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>>> index 6a661e796328..79448f1c8b8d 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>> @@ -96,7 +96,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>>  	crtc_state->update_pipe = false;
>>>  	crtc_state->disable_lp_wm = false;
>>>  	crtc_state->disable_cxsr = false;
>>> -	crtc_state->wm_changed = false;
>>> +	crtc_state->update_wm_pre = false;
>>> +	crtc_state->update_wm_post = false;
>>>  	crtc_state->fb_changed = false;
>>>  	crtc_state->wm.need_postvbl_update = false;
>> There's already a wm.need_postvbl_update.
>> could we use it for non-atomic platforms, and add a wm.need_intermediate_update ?
>> This could probably be used for ILK too.
> Dunno. All I know is the regression is now on it's way 4.5 and needs to
> be dealt with ASAP. So no major overhaul at this point.
>
Ok, since I lack the hardware to test for now this will have to do.. I'd like this to be revisited in the future.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH 3/5] drm/i915: Fix watermarks for VLV/CHV

[Ville Syrjälä]

On Thu, Mar 10, 2016 at 12:53:37PM +0100, Maarten Lankhorst wrote:
> Op 10-03-16 om 11:33 schreef Ville Syrjälä:
> > On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote:
> >> Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> >>> broke thigns by removing the pre vs. post wm update distinction. We also
> >>> lost the pre plane wm update entirely for VLV/CHV from the crtc enable
> >>> path.
> >>>
> >>> This caused underruns on modeset and plane enable/disable on CHV,
> >>> and often those can lead to a dead pipe.
> >>>
> >>> So let's bring back the pre vs. post thing, and let's toss in an
> >>> explicit wm update to valleyview_crtc_enable() to avoid having to
> >>> put it into the common code.
> >>>
> >>> This is more or less a partial revert of the offending commit.
> >>>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: drm-intel-fixes@lists.freedesktop.org
> >>> Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.")
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_atomic.c  |  3 ++-
> >>>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
> >>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> >>>  3 files changed, 22 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>> index 6a661e796328..79448f1c8b8d 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>> @@ -96,7 +96,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >>>  	crtc_state->update_pipe = false;
> >>>  	crtc_state->disable_lp_wm = false;
> >>>  	crtc_state->disable_cxsr = false;
> >>> -	crtc_state->wm_changed = false;
> >>> +	crtc_state->update_wm_pre = false;
> >>> +	crtc_state->update_wm_post = false;
> >>>  	crtc_state->fb_changed = false;
> >>>  	crtc_state->wm.need_postvbl_update = false;
> >> There's already a wm.need_postvbl_update.
> >> could we use it for non-atomic platforms, and add a wm.need_intermediate_update ?
> >> This could probably be used for ILK too.
> > Dunno. All I know is the regression is now on it's way 4.5 and needs to
> > be dealt with ASAP. So no major overhaul at this point.
> >
> Ok, since I lack the hardware to test for now this will have to do.. I'd like this to be revisited in the future.
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks for the reviews everyone. Series pushed to dinq.

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

[PATCH 4/5] drm/i915: Wait for vblank after cxsr disable in pre_plane_update

[ville.syrjala]

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

We must wait for the hardware to exit cxsr before doing the plane
update, so add the missing vblank wait to pre_plane_update after
disabling cxsr.

We have the wait for vblank in the pre_disable_primary hook, but not in
the pre_plane_update hook. Just move the code from (and comment) from
pre_disable_primary into pre_plane_update. Well, we still have to keep
it in pre_disable_primary for these strange _noatomic codepaths, so
let's do another version of pre_disable_primary for those. Also toss
in some FIXMEs in the hope that someone will eventually clean up this
pre_disable_primary mess.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8b44e0652740..a76ba22ee711 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -116,7 +116,7 @@ static void skylake_pfit_enable(struct intel_crtc *crtc);
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
 static void ironlake_pfit_enable(struct intel_crtc *crtc);
 static void intel_modeset_setup_hw_state(struct drm_device *dev);
-static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
 
 typedef struct {
 	int	min, max;
@@ -2755,7 +2755,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	 */
 	to_intel_plane_state(plane_state)->visible = false;
 	crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
-	intel_pre_disable_primary(&intel_crtc->base);
+	intel_pre_disable_primary_noatomic(&intel_crtc->base);
 	intel_plane->disable_plane(primary, &intel_crtc->base);
 
 	return;
@@ -4857,16 +4857,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/**
- * intel_pre_disable_primary - Perform operations before disabling primary plane
- * @crtc: the CRTC whose primary plane is to be disabled
- *
- * Performs potentially sleeping operations that must be done before the
- * primary plane is disabled, such as updating FBC and IPS.  Note that this may
- * be called due to an explicit primary plane update, or due to an implicit
- * disable that is caused when a sprite plane completely hides the primary
- * plane.
- */
+/* FIXME move all this to pre_plane_update() with proper state tracking */
 static void
 intel_pre_disable_primary(struct drm_crtc *crtc)
 {
@@ -4885,6 +4876,26 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
 	/*
+	 * FIXME IPS should be fine as long as one plane is
+	 * enabled, but in practice it seems to have problems
+	 * when going from primary only to sprite only and vice
+	 * versa.
+	 */
+	hsw_disable_ips(intel_crtc);
+}
+
+/* FIXME get rid of this and use pre_plane_update */
+static void
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+
+	intel_pre_disable_primary(crtc);
+
+	/*
 	 * Vblank time updates from the shadow to live plane control register
 	 * are blocked if the memory self-refresh mode is active at that
 	 * moment. So to make sure the plane gets truly disabled, disable
@@ -4898,14 +4909,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 		dev_priv->wm.vlv.cxsr = false;
 		intel_wait_for_vblank(dev, pipe);
 	}
-
-	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
-	 */
-	hsw_disable_ips(intel_crtc);
 }
 
 static void intel_post_plane_update(struct intel_crtc *crtc)
@@ -4962,8 +4965,20 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->disable_cxsr) {
 		crtc->wm.cxsr_allowed = false;
 
-		if (old_crtc_state->base.active)
+		/*
+		 * Vblank time updates from the shadow to live plane control register
+		 * are blocked if the memory self-refresh mode is active at that
+		 * moment. So to make sure the plane gets truly disabled, disable
+		 * first the self-refresh mode. The self-refresh enable bit in turn
+		 * will be checked/applied by the HW only at the next frame start
+		 * event which is after the vblank start event, so we need to have a
+		 * wait-for-vblank between disabling the plane and the pipe.
+		 */
+		if (old_crtc_state->base.active) {
 			intel_set_memory_cxsr(dev_priv, false);
+			dev_priv->wm.vlv.cxsr = false;
+			intel_wait_for_vblank(dev, crtc->pipe);
+		}
 	}
 
 	/*
@@ -6511,7 +6526,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	if (to_intel_plane_state(crtc->primary->state)->visible) {
 		WARN_ON(intel_crtc->unpin_work);
 
-		intel_pre_disable_primary(crtc);
+		intel_pre_disable_primary_noatomic(crtc);
 
 		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
 		to_intel_plane_state(crtc->primary->state)->visible = false;
-- 
2.4.10

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

[PATCH 5/5] drm/i915: s/crtc_state/old_crtc_state/ in intel_atomic_commit()

[ville.syrjala]

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

Avoid some head spinning by renaming the crtc_state variable to
old_crtc_state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a76ba22ee711..c68964243b99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13705,7 +13705,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc *crtc;
 	struct intel_crtc_state *intel_cstate;
 	int ret = 0, i;
@@ -13731,7 +13731,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		if (needs_modeset(crtc->state) ||
@@ -13746,10 +13746,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		intel_pre_plane_update(to_intel_crtc_state(crtc_state));
+		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
 
-		if (crtc_state->active) {
-			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
+		if (old_crtc_state->active) {
+			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
@@ -13782,7 +13782,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
 		struct intel_crtc_state *pipe_config =
@@ -13795,14 +13795,14 @@ static int intel_atomic_commit(struct drm_device *dev,
 		}
 
 		if (!modeset)
-			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
+			intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
 
 		if (crtc->state->active && intel_crtc->atomic.update_fbc)
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&
 		    (crtc->state->planes_changed || update_pipe))
-			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
 
 		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
 			crtc_vblank_mask |= 1 << i;
@@ -13813,7 +13813,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	if (!state->legacy_cursor_update)
 		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 		intel_post_plane_update(to_intel_crtc(crtc));
 
 		if (put_domains[i])
@@ -13830,7 +13830,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	 *
 	 * TODO: Move this (and other cleanup) to an async worker eventually.
 	 */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 		intel_cstate = to_intel_crtc_state(crtc->state);
 
 		if (dev_priv->display.optimize_watermarks)
-- 
2.4.10

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

✗ Fi.CI.BAT: failure for drm/i915: CHV regressions fixes

[Patchwork]

== Series Details ==

Series: drm/i915: CHV regressions fixes
URL   : https://patchwork.freedesktop.org/series/4283/
State : failure

== Summary ==

Series 4283v1 drm/i915: CHV regressions fixes
http://patchwork.freedesktop.org/api/1.0/series/4283/revisions/1/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (bdw-ultra)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (byt-nuc)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (hsw-gt2)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (hsw-brixbox)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (bdw-nuci7)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (hsw-gt2)

bdw-nuci7        total:193  pass:180  dwarn:1   dfail:0   fail:0   skip:12 
bdw-ultra        total:193  pass:172  dwarn:0   dfail:0   fail:0   skip:21 
byt-nuc          total:193  pass:157  dwarn:0   dfail:0   fail:1   skip:35 
hsw-brixbox      total:193  pass:171  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:193  pass:175  dwarn:1   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:193  pass:130  dwarn:0   dfail:0   fail:0   skip:63 
skl-i5k-2        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:193  pass:158  dwarn:1   dfail:0   fail:0   skip:34 

Results at /archive/results/CI_IGT_test/Patchwork_1554/

ab403b26610034afe0e0c97d960782bad98b97d0 drm-intel-nightly: 2016y-03m-09d-09h-25m-31s UTC integration manifest
0434f7d5769d4ae80fde59476b129fcf8653d655 drm/i915: s/crtc_state/old_crtc_state/ in intel_atomic_commit()
9265370d045306f483b6af65948ef8f5d17e5dc8 drm/i915: Wait for vblank after cxsr disable in pre_plane_update
cbd2f01aa943825becbe92fb126e3b3a67bf49a2 drm/i915: Fix watermarks for VLV/CHV
a4713d57b76d75fb527f3b802ec88e950f3cbac6 drm/i915: Pass the correct crtc state to .update_plane()
fe2c89bbb4a7bffcf8f046c396b132075ab96693 Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: CHV regressions fixes

[Ville Syrjälä]

On Wed, Mar 09, 2016 at 05:31:34PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: CHV regressions fixes
> URL   : https://patchwork.freedesktop.org/series/4283/
> State : failure
> 
> == Summary ==
> 
> Series 4283v1 drm/i915: CHV regressions fixes
> http://patchwork.freedesktop.org/api/1.0/series/4283/revisions/1/mbox/
> 
> Test drv_module_reload_basic:
>                 dmesg-warn -> PASS       (ilk-hp8440p)
> Test kms_flip:
>         Subgroup basic-flip-vs-modeset:
>                 dmesg-warn -> PASS       (bdw-ultra)
>         Subgroup basic-flip-vs-wf_vblank:
>                 pass       -> FAIL       (byt-nuc)

(kms_flip:6823) DEBUG: name = flip
last_ts = 282.573849 usec
last_received_ts = 282.573600 usec
last_seq = 6777
current_ts = 282.757264 usec
current_received_ts = 282.757213 usec
current_seq = 6788
count = 77
seq_step = 1
(kms_flip:6823) CRITICAL: Test assertion failure function check_state, file kms_flip.c:692:
(kms_flip:6823) CRITICAL: Failed assertion: fabs((((double) diff.tv_usec) - usec_interflip) / usec_interflip) <= 0.005
(kms_flip:6823) CRITICAL: Last errno: 25, Inappropriate ioctl for device
(kms_flip:6823) CRITICAL: inter-flip ts jitter: 0s, 183415usec

The more mysterious seq_step==1 variant this time:
https://bugs.freedesktop.org/show_bug.cgi?id=94294

>         Subgroup basic-plain-flip:
>                 dmesg-warn -> PASS       (hsw-gt2)
> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 dmesg-warn -> PASS       (hsw-brixbox)
>         Subgroup read-crc-pipe-a-frame-sequence:
>                 pass       -> DMESG-WARN (bdw-nuci7)

[  335.334105]  [<ffffffffa023be8c>] gen8_write32+0x26c/0x300 [i915]
[  335.334136]  [<ffffffffa01eab88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
[  335.334170]  [<ffffffffa01f01cd>] ilk_program_watermarks+0x4bd/0x980 [i915]

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

>         Subgroup read-crc-pipe-c-frame-sequence:
>                 pass       -> DMESG-WARN (hsw-gt2)

[  569.813901]  [<ffffffffa035a86b>] hsw_write32+0x21b/0x270 [i915]
[  569.813912]  [<ffffffffa0303b88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
[  569.813923]  [<ffffffffa03091cd>] ilk_program_watermarks+0x4bd/0x980 [i915]

same

> 
> bdw-nuci7        total:193  pass:180  dwarn:1   dfail:0   fail:0   skip:12 
> bdw-ultra        total:193  pass:172  dwarn:0   dfail:0   fail:0   skip:21 
> byt-nuc          total:193  pass:157  dwarn:0   dfail:0   fail:1   skip:35 
> hsw-brixbox      total:193  pass:171  dwarn:0   dfail:0   fail:0   skip:22 
> hsw-gt2          total:193  pass:175  dwarn:1   dfail:0   fail:0   skip:17 
> ilk-hp8440p      total:193  pass:130  dwarn:0   dfail:0   fail:0   skip:63 
> skl-i5k-2        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
> skl-i7k-2        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
> snb-dellxps      total:193  pass:158  dwarn:1   dfail:0   fail:0   skip:34 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1554/
> 
> ab403b26610034afe0e0c97d960782bad98b97d0 drm-intel-nightly: 2016y-03m-09d-09h-25m-31s UTC integration manifest
> 0434f7d5769d4ae80fde59476b129fcf8653d655 drm/i915: s/crtc_state/old_crtc_state/ in intel_atomic_commit()
> 9265370d045306f483b6af65948ef8f5d17e5dc8 drm/i915: Wait for vblank after cxsr disable in pre_plane_update
> cbd2f01aa943825becbe92fb126e3b3a67bf49a2 drm/i915: Fix watermarks for VLV/CHV
> a4713d57b76d75fb527f3b802ec88e950f3cbac6 drm/i915: Pass the correct crtc state to .update_plane()
> fe2c89bbb4a7bffcf8f046c396b132075ab96693 Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."

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