[PATCH] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[PATCH] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

i915 sometimes needs to disable planes in the middle of an atomic
commit, and then reenable them later in the same commit. Because of
this, we can't make the assumption that the state of the plane actually
changed. Since the state of the plane hasn't actually changed, neither
have it's watermarks. And if the watermarks hasn't changed then we
haven't populated skl_results with anything, which means we'll end up
zeroing out a plane's watermarks in the middle of the atomic commit
without restoring them later.

Changes since v1:
 - Fix incorrect use of "it's"

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks atomically
during plane updates")

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
 drivers/gpu/drm/i915/intel_sprite.c  | 9 +++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4e6141..13e47a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3448,7 +3448,12 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 
-	skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
+	/*
+	 * We only populate skl_results on watermark updates, and if the
+	 * plane's visiblity isn't actually changing neither is its watermarks.
+	 */
+	if (!crtc->primary->state->visible)
+		skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0df783a..73a521f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
-	skl_write_plane_wm(to_intel_crtc(crtc), &dev_priv->wm.skl_results,
-			   plane);
+	/*
+	 * We only populate skl_results on watermark updates, and if the
+	 * plane's visiblity isn't actually changing neither is its watermarks.
+	 */
+	if (!dplane->state->visible)
+		skl_write_plane_wm(to_intel_crtc(crtc),
+				   &dev_priv->wm.skl_results, plane);
 
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
-- 
2.7.4

Re: [PATCH] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Jani Nikula]

On Mon, 29 Aug 2016, Lyude <cpaul@redhat.com> wrote:
> i915 sometimes needs to disable planes in the middle of an atomic
> commit, and then reenable them later in the same commit. Because of
> this, we can't make the assumption that the state of the plane actually
> changed. Since the state of the plane hasn't actually changed, neither
> have it's watermarks. And if the watermarks hasn't changed then we
> haven't populated skl_results with anything, which means we'll end up
> zeroing out a plane's watermarks in the middle of the atomic commit
> without restoring them later.

I would appreciate a (brief) description of what the failure mode is in
this case.

BR,
Jani.


>
> Changes since v1:
>  - Fix incorrect use of "it's"
>
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks atomically
> during plane updates")
>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 9 +++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4e6141..13e47a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3448,7 +3448,12 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
>  
> -	skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
> +	/*
> +	 * We only populate skl_results on watermark updates, and if the
> +	 * plane's visiblity isn't actually changing neither is its watermarks.
> +	 */
> +	if (!crtc->primary->state->visible)
> +		skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0df783a..73a521f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	skl_write_plane_wm(to_intel_crtc(crtc), &dev_priv->wm.skl_results,
> -			   plane);
> +	/*
> +	 * We only populate skl_results on watermark updates, and if the
> +	 * plane's visiblity isn't actually changing neither is its watermarks.
> +	 */
> +	if (!dplane->state->visible)
> +		skl_write_plane_wm(to_intel_crtc(crtc),
> +				   &dev_priv->wm.skl_results, plane);
>  
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH v3] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

i915 sometimes needs to disable planes in the middle of an atomic
commit, and then reenable them later in the same commit. Because of
this, we can't make the assumption that the state of the plane actually
changed. Since the state of the plane hasn't actually changed, neither
have it's watermarks. And if the watermarks hasn't changed then we
haven't populated skl_results with anything, which means we'll end up
zeroing out a plane's watermarks in the middle of the atomic commit
without restoring them later.

Simple reproduction recipe:
 - Get a SKL laptop, launch any kind of X session
 - Get two extra monitors
 - Keep hotplugging both displays (so that the display configuration
   jumps from 1 active pipe to 3 active pipes and back)
 - Eventually underrun

Changes since v1:
 - Fix incorrect use of "it's"
Changes since v2:
 - Add reproduction recipe

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks atomically
during plane updates")

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
 drivers/gpu/drm/i915/intel_sprite.c  | 9 +++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4e6141..13e47a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3448,7 +3448,12 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 
-	skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
+	/*
+	 * We only populate skl_results on watermark updates, and if the
+	 * plane's visiblity isn't actually changing neither is its watermarks.
+	 */
+	if (!crtc->primary->state->visible)
+		skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0df783a..73a521f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
-	skl_write_plane_wm(to_intel_crtc(crtc), &dev_priv->wm.skl_results,
-			   plane);
+	/*
+	 * We only populate skl_results on watermark updates, and if the
+	 * plane's visiblity isn't actually changing neither is its watermarks.
+	 */
+	if (!dplane->state->visible)
+		skl_write_plane_wm(to_intel_crtc(crtc),
+				   &dev_priv->wm.skl_results, plane);
 
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
-- 
2.7.4

Re: [PATCH v3] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

Since this patch has been on hold for a little bit, I did a bit of
thinking of how we could this a little more cleanly. Unfortunately I
couldn't think of a way, however I did think of an alternative
solution:

I'm planning on backporting all of the skl wm fixes already, so I'm
going to use this patch for that since it's very small. As for
mainline, I'm going to do a whole reorganization of the skl wm/ddb
structs in i915 like Matt had suggested before. Things might look a
little more like this (taken from my half-complete reorganization):

struct skl_plane_ddb_allocation {
	struct skl_ddb_entry plane;
	struct skl_ddb_entry y_plane;
};

struct skl_plane_wm_values {
	struct skl_plane_ddb_allocation ddb;
	uint32_t wm[8];
	uint32_t trans_wm;
};

struct skl_pipe_wm_values {
	struct skl_ddb_entry ddb;
	uint32_t linetime;
};

struct skl_hw_wm_values { /* (only used for skl_get_hw_ddb and friends) */
	struct skl_pipe_wm_values pipe[I915_MAX_PIPES];
	struct skl_plane_wm_values plane[I915_MAX_PIPES][I915_MAX_PLANES];
};

As well, I'm also just going to completely remove the skl_results and
skl_hw structs from struct drm_i915_private. This makes sense for a lot
of reasons:
 * This completely gets rid of the need for a global watermark lock (on
   Skylake at least) and will make things a lot easier for atomic
   support in the future
 * Skylake doesn't have any actual global watermark hooks anyway, aside
   from skl_update_wm() which is now only used for writing watermarks
   for inactive pipes during haswell_crtc_enable()
 * This makes passing watermarks around way less of a mess
 * Saves a tiny bit of data, and so far being able to grab
   watermarks/ddbs right from the plane states seems to be a lot easier
   then messing with a large array

As for this fix, I'll probably still need someone to review it so I can
get it into 4.7.y.

Let me know what you think.

On Mon, 2016-08-29 at 12:31 -0400, Lyude wrote:
> i915 sometimes needs to disable planes in the middle of an atomic
> commit, and then reenable them later in the same commit. Because of
> this, we can't make the assumption that the state of the plane
> actually
> changed. Since the state of the plane hasn't actually changed,
> neither
> have it's watermarks. And if the watermarks hasn't changed then we
> haven't populated skl_results with anything, which means we'll end up
> zeroing out a plane's watermarks in the middle of the atomic commit
> without restoring them later.
> 
> Simple reproduction recipe:
>  - Get a SKL laptop, launch any kind of X session
>  - Get two extra monitors
>  - Keep hotplugging both displays (so that the display configuration
>    jumps from 1 active pipe to 3 active pipes and back)
>  - Eventually underrun
> 
> Changes since v1:
>  - Fix incorrect use of "it's"
> Changes since v2:
>  - Add reproduction recipe
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks
> atomically
> during plane updates")
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 9 +++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index e4e6141..13e47a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3448,7 +3448,12 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
>  
> -	skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results,
> 0);
> +	/*
> +	 * We only populate skl_results on watermark updates, and if
> the
> +	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> +	 */
> +	if (!crtc->primary->state->visible)
> +		skl_write_plane_wm(intel_crtc, &dev_priv-
> >wm.skl_results, 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 0df783a..73a521f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	skl_write_plane_wm(to_intel_crtc(crtc), &dev_priv-
> >wm.skl_results,
> -			   plane);
> +	/*
> +	 * We only populate skl_results on watermark updates, and if
> the
> +	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> +	 */
> +	if (!dplane->state->visible)
> +		skl_write_plane_wm(to_intel_crtc(crtc),
> +				   &dev_priv->wm.skl_results,
> plane);
>  
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
-- 
Cheers,
	Lyude