[PATCH v1] Fix the possible watermark miswriting

[PATCH v1] Fix the possible watermark miswriting

[Stanislav Lisovskiy]

Currently whenever we attempt to recalculate
watermarks, we assign dirty_pipes to zero,
then compare current wm results to the recalculated
one and if they changed we set correspondent dirty_pipes
bit again.
This can lead to situation, when we same clearing dirty_pipes,
same wm results twice and not setting dirty_pipes => so that
watermarks are not actually updated, which then might
lead to fifo underruns, crc mismatch and other issues.

Instead, whenever we detect that wm results are changed,
need to set correspondent dirty_pipes bit and clear it
only once the change is written, but not clear it everytime
we attempt to recalculate those in skl_compute_wm.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dc034617febb..f7fbc4bc0d43 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
 	bool changed = false;
 	int ret, i;
 
-	/* Clear all dirty flags */
-	results->dirty_pipes = 0;
-
 	ret = skl_ddb_add_affected_pipes(state, &changed);
 	if (ret || !changed)
 		return ret;
@@ -5496,6 +5493,7 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
+	struct skl_ddb_values *results = &state->wm_results;
 	enum pipe pipe = crtc->pipe;
 	enum plane_id plane_id;
 
@@ -5512,6 +5510,10 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 			skl_write_cursor_wm(crtc, &pipe_wm->planes[plane_id],
 					    ddb);
 	}
+
+	/* Clear correspondent dirty bit */
+	results->dirty_pipes &= ~drm_crtc_mask(&crtc->base);
+
 }
 
 static void skl_initial_wm(struct intel_atomic_state *state,
-- 
2.17.1

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

Re: [Intel-gfx] [PATCH v1] Fix the possible watermark miswriting

[Ville Syrjälä]

On Tue, Nov 13, 2018 at 04:27:28PM +0200, Stanislav Lisovskiy wrote:
> Currently whenever we attempt to recalculate
> watermarks, we assign dirty_pipes to zero,
> then compare current wm results to the recalculated
> one and if they changed we set correspondent dirty_pipes
> bit again.
> This can lead to situation, when we same clearing dirty_pipes,
> same wm results twice and not setting dirty_pipes => so that
> watermarks are not actually updated, which then might
> lead to fifo underruns, crc mismatch and other issues.
> 
> Instead, whenever we detect that wm results are changed,
> need to set correspondent dirty_pipes bit and clear it
> only once the change is written, but not clear it everytime
> we attempt to recalculate those in skl_compute_wm.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index dc034617febb..f7fbc4bc0d43 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	bool changed = false;
>  	int ret, i;
>  
> -	/* Clear all dirty flags */
> -	results->dirty_pipes = 0;
> -
>  	ret = skl_ddb_add_affected_pipes(state, &changed);
>  	if (ret || !changed)
>  		return ret;
> @@ -5496,6 +5493,7 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
> +	struct skl_ddb_values *results = &state->wm_results;
>  	enum pipe pipe = crtc->pipe;
>  	enum plane_id plane_id;
>  
> @@ -5512,6 +5510,10 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
>  			skl_write_cursor_wm(crtc, &pipe_wm->planes[plane_id],
>  					    ddb);
>  	}
> +
> +	/* Clear correspondent dirty bit */
> +	results->dirty_pipes &= ~drm_crtc_mask(&crtc->base);
> +
>  }

Hmm. I can't figure out what the problem really is here. Yes, it does
look like we'd end up writing the watermarks twice for pipes that
are getting enabled. But that should be safeish (apart from the whole
"need PLANE_SURF write to atually arm the update" mess).

Note that this whole code is getting nuked soon (I hope):
https://patchwork.freedesktop.org/series/51878/
but in the meantime if there is a clear bug we probably want the fix in
first so that it can be backported. So I think we need a clear analysis
of the actual problem.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel