[PATCH 0/2] Prevent (sleeping) wm updates under vblank evasion

[PATCH 0/2] Prevent (sleeping) wm updates under vblank evasion

[Matt Roper]

Our low-level plane update functions, which get called while we're under vblank
evasion with IRQ's disabled, currently call intel_update_sprite_watermarks
which is a potentially sleeping operation.  This fumble has been present in the
code ever since we added atomic vblank evasion.  We actually do have a
mechanism to flag the need for wm updates and then perform them once evasion is
finished, but our handling of that flag is a bit busted at the moment (i.e.,
currently only being used when a plane gets disabled).  Let's fix that...

Matt Roper (2):
  drm/i915: Drop parameters to intel_update_sprite_watermarks()
  drm/i915: Update sprite watermarks outside vblank evasion

 drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  6 +-----
 drivers/gpu/drm/i915/intel_pm.c      | 21 +++++++++++++++------
 drivers/gpu/drm/i915/intel_sprite.c  | 21 ---------------------
 4 files changed, 33 insertions(+), 41 deletions(-)

-- 
2.1.4

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

[PATCH 1/2] drm/i915: Drop parameters to intel_update_sprite_watermarks()

[Matt Roper]

The values that ultimately get passed to
intel_update_sprite_watermarks() are pulled out of the plane state
(which has already been swapped into plane->state) as we update the
plane programming.  Drop the function parameters and just pull the
relevant values out of the state structure inside the function.

This change will make it easier for a future patch to pull the sprite
WM programming out of the low-level foo_update_plane() functions (which
are run under vblank evasion and shouldn't be calling potentially
blocking watermark functions).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +--
 drivers/gpu/drm/i915/intel_drv.h     |  6 +-----
 drivers/gpu/drm/i915/intel_pm.c      | 17 ++++++++++++-----
 drivers/gpu/drm/i915/intel_sprite.c  | 21 ++++++---------------
 4 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index de6f8cc..5e8e01c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4719,8 +4719,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 		intel_post_enable_primary(&crtc->base);
 
 	drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
-		intel_update_sprite_watermarks(plane, &crtc->base,
-					       0, 0, 0, false, false);
+		intel_update_sprite_watermarks(plane, &crtc->base);
 
 	memset(atomic, 0, sizeof(*atomic));
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e2174fd..fdfa2c9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1355,11 +1355,7 @@ void intel_suspend_hw(struct drm_device *dev);
 int ilk_wm_max_level(const struct drm_device *dev);
 void intel_update_watermarks(struct drm_crtc *crtc);
 void intel_update_sprite_watermarks(struct drm_plane *plane,
-				    struct drm_crtc *crtc,
-				    uint32_t sprite_width,
-				    uint32_t sprite_height,
-				    int pixel_size,
-				    bool enabled, bool scaled);
+				    struct drm_crtc *crtc);
 void intel_init_pm(struct drm_device *dev);
 void intel_pm_setup(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 32ff034..4d3cb70 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3755,13 +3755,20 @@ void intel_update_watermarks(struct drm_crtc *crtc)
 }
 
 void intel_update_sprite_watermarks(struct drm_plane *plane,
-				    struct drm_crtc *crtc,
-				    uint32_t sprite_width,
-				    uint32_t sprite_height,
-				    int pixel_size,
-				    bool enabled, bool scaled)
+				    struct drm_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = plane->dev->dev_private;
+	struct intel_plane_state *state = to_intel_plane_state(plane->state);
+	struct drm_framebuffer *fb = state->base.fb;
+	uint32_t sprite_width = drm_rect_width(&state->dst);
+	uint32_t sprite_height = drm_rect_height(&state->dst);
+	int pixel_size = fb ? drm_format_plane_cpp(fb->pixel_format, 0) : 0;
+	bool enabled = state->visible;
+	unsigned int src_w = drm_rect_width(&state->src) >> 16;
+	unsigned int src_h = drm_rect_height(&state->src) >> 16;
+	unsigned int dst_w = drm_rect_width(&state->dst);
+	unsigned int dst_h = drm_rect_height(&state->dst);
+	bool scaled = (src_w != dst_w || src_h != dst_h);
 
 	if (dev_priv->display.update_sprite_wm)
 		dev_priv->display.update_sprite_wm(plane, crtc,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e0045aa..b627067 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -181,7 +181,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div, stride;
-	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key =
 		&to_intel_plane_state(drm_plane->state)->ckey;
 	unsigned long surf_addr;
@@ -200,9 +199,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	rotation = drm_plane->state->rotation;
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
-	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
-				       pixel_size, true,
-				       src_w != crtc_w || src_h != crtc_h);
+	intel_update_sprite_watermarks(drm_plane, crtc);
 
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
@@ -286,7 +283,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
 	POSTING_READ(PLANE_SURF(pipe, plane));
 
-	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
+	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 static void
@@ -402,9 +399,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	if (obj->tiling_mode != I915_TILING_NONE)
 		sprctl |= SP_TILED;
 
-	intel_update_sprite_watermarks(dplane, crtc, src_w, src_h,
-				       pixel_size, true,
-				       src_w != crtc_w || src_h != crtc_h);
+	intel_update_sprite_watermarks(dplane, crtc);
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -471,7 +466,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
-	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
+	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 static void
@@ -535,9 +530,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		sprctl |= SPRITE_PIPE_CSC_ENABLE;
 
-	intel_update_sprite_watermarks(plane, crtc, src_w, src_h, pixel_size,
-				       true,
-				       src_w != crtc_w || src_h != crtc_h);
+	intel_update_sprite_watermarks(plane, crtc);
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -672,9 +665,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (IS_GEN6(dev))
 		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
 
-	intel_update_sprite_watermarks(plane, crtc, src_w, src_h,
-				       pixel_size, true,
-				       src_w != crtc_w || src_h != crtc_h);
+	intel_update_sprite_watermarks(plane, crtc);
 
 	/* Sizes are 0 based */
 	src_w--;
-- 
2.1.4

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

[PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

[Matt Roper]

We never removed the sprite watermark updates from our low-level
foo_update_plane() functions; since our hardware updates happen under
vblank evasion, we're not supposed to be calling potentially sleeping
functions there (since interrupts are disabled).  We do already have a
mechanism that's supposed to be used to update sprite watermarks in the
post-evasion function intel_post_plane_update(), but at the moment it's
only being used for updates caused by plane disables.

To fix this oversight we need to make a few changes:

 * When intel_plane_atomic_calc_changes() calls intel_wm_need_update()
   to determine whether watermarks need an update, we need to set the
   'atomic.update_sprite_watermarks' flag rather than the
   'atomic.update_wm' flag when the plane in question is a sprite.  Some
   platforms don't care about this change since the two types of update
   do the same thing, but some platforms (e.g., ILK-style watermarks)
   need to specifically use the sprite update function to update cached
   watermark parameters.

 * intel_wm_need_update() needs to also look for plane size changes
   (previously it only checked tiling & rotation).

With the above changes, the need for sprite watermark updates should be
properly flagged at atomic 'check' time and handled at 'commit' time in
post-evasion, so we can drop the direct calls to
intel_update_sprite_watermarks() from all of the
intel_plane->update_plane() handlers.  We'll also toss a
WARN_ON(irqs_disabled()) into the watermark update functions to avoid
such mistakes in the future.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
 drivers/gpu/drm/i915/intel_pm.c      |  2 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 12 ------------
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e8e01c..181aedd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11575,13 +11575,17 @@ retry:
 static bool intel_wm_need_update(struct drm_plane *plane,
 				 struct drm_plane_state *state)
 {
-	/* Update watermarks on tiling changes. */
+	struct intel_plane_state *new = to_intel_plane_state(state);
+	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
+
+	/* Update watermarks on tiling changes or size changes. */
 	if (!plane->state->fb || !state->fb ||
 	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
-	    plane->state->rotation != state->rotation)
-		return true;
-
-	if (plane->state->crtc_w != state->crtc_w)
+	    plane->state->rotation != state->rotation ||
+	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
+	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
+	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
+	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
 		return true;
 
 	return false;
@@ -11645,8 +11649,13 @@ 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 (intel_wm_need_update(plane, plane_state))
-		intel_crtc->atomic.update_wm = true;
+	if (intel_wm_need_update(plane, plane_state)) {
+		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
+			intel_crtc->atomic.update_sprite_watermarks |=
+				1 << i;
+		else
+			intel_crtc->atomic.update_wm = true;
+	}
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4d3cb70..2470ede 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 
+	WARN_ON(irqs_disabled());
 	if (dev_priv->display.update_wm)
 		dev_priv->display.update_wm(crtc);
 }
@@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
 	unsigned int dst_h = drm_rect_height(&state->dst);
 	bool scaled = (src_w != dst_w || src_h != dst_h);
 
+	WARN_ON(irqs_disabled());
 	if (dev_priv->display.update_sprite_wm)
 		dev_priv->display.update_sprite_wm(plane, crtc,
 						   sprite_width, sprite_height,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b627067..ce2fa92 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	rotation = drm_plane->state->rotation;
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
-	intel_update_sprite_watermarks(drm_plane, crtc);
-
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
 
@@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
 	POSTING_READ(PLANE_SURF(pipe, plane));
-
-	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 static void
@@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	if (obj->tiling_mode != I915_TILING_NONE)
 		sprctl |= SP_TILED;
 
-	intel_update_sprite_watermarks(dplane, crtc);
-
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
-
-	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 static void
@@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		sprctl |= SPRITE_PIPE_CSC_ENABLE;
 
-	intel_update_sprite_watermarks(plane, crtc);
-
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (IS_GEN6(dev))
 		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
 
-	intel_update_sprite_watermarks(plane, crtc);
-
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
-- 
2.1.4

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

Re: [PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

[Daniel Vetter]

On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote:
> We never removed the sprite watermark updates from our low-level
> foo_update_plane() functions; since our hardware updates happen under
> vblank evasion, we're not supposed to be calling potentially sleeping
> functions there (since interrupts are disabled).  We do already have a
> mechanism that's supposed to be used to update sprite watermarks in the
> post-evasion function intel_post_plane_update(), but at the moment it's
> only being used for updates caused by plane disables.
> 
> To fix this oversight we need to make a few changes:
> 
>  * When intel_plane_atomic_calc_changes() calls intel_wm_need_update()
>    to determine whether watermarks need an update, we need to set the
>    'atomic.update_sprite_watermarks' flag rather than the
>    'atomic.update_wm' flag when the plane in question is a sprite.  Some
>    platforms don't care about this change since the two types of update
>    do the same thing, but some platforms (e.g., ILK-style watermarks)
>    need to specifically use the sprite update function to update cached
>    watermark parameters.
> 
>  * intel_wm_need_update() needs to also look for plane size changes
>    (previously it only checked tiling & rotation).
> 
> With the above changes, the need for sprite watermark updates should be
> properly flagged at atomic 'check' time and handled at 'commit' time in
> post-evasion, so we can drop the direct calls to
> intel_update_sprite_watermarks() from all of the
> intel_plane->update_plane() handlers.  We'll also toss a
> WARN_ON(irqs_disabled()) into the watermark update functions to avoid
> such mistakes in the future.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Why do even still need a separate sprite wm update hooks? Aren't we yet
recomputing wm values for all planes if something changes, then diffing
them with the current one and updating if anything changed? That seems to
be what update_sprite_wm essentially does, except it's pre-atomic and
open-codes the updates ...

Ofc there's still that ilk w/a around which needs to be moved to a
suitable place.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_pm.c      |  2 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 12 ------------
>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e8e01c..181aedd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11575,13 +11575,17 @@ retry:
>  static bool intel_wm_need_update(struct drm_plane *plane,
>  				 struct drm_plane_state *state)
>  {
> -	/* Update watermarks on tiling changes. */
> +	struct intel_plane_state *new = to_intel_plane_state(state);
> +	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> +
> +	/* Update watermarks on tiling changes or size changes. */
>  	if (!plane->state->fb || !state->fb ||
>  	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> -	    plane->state->rotation != state->rotation)
> -		return true;
> -
> -	if (plane->state->crtc_w != state->crtc_w)
> +	    plane->state->rotation != state->rotation ||
> +	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
> +	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
> +	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
> +	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
>  		return true;
>  
>  	return false;
> @@ -11645,8 +11649,13 @@ 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 (intel_wm_need_update(plane, plane_state))
> -		intel_crtc->atomic.update_wm = true;
> +	if (intel_wm_need_update(plane, plane_state)) {
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			intel_crtc->atomic.update_sprite_watermarks |=
> +				1 << i;
> +		else
> +			intel_crtc->atomic.update_wm = true;
> +	}
>  
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4d3cb70..2470ede 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  
> +	WARN_ON(irqs_disabled());
>  	if (dev_priv->display.update_wm)
>  		dev_priv->display.update_wm(crtc);
>  }
> @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
>  	unsigned int dst_h = drm_rect_height(&state->dst);
>  	bool scaled = (src_w != dst_w || src_h != dst_h);
>  
> +	WARN_ON(irqs_disabled());
>  	if (dev_priv->display.update_sprite_wm)
>  		dev_priv->display.update_sprite_wm(plane, crtc,
>  						   sprite_width, sprite_height,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b627067..ce2fa92 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	rotation = drm_plane->state->rotation;
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	intel_update_sprite_watermarks(drm_plane, crtc);
> -
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
>  
> @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  static void
> @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	if (obj->tiling_mode != I915_TILING_NONE)
>  		sprctl |= SP_TILED;
>  
> -	intel_update_sprite_watermarks(dplane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  static void
> @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
>  
> -	intel_update_sprite_watermarks(plane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (IS_GEN6(dev))
>  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
>  
> -	intel_update_sprite_watermarks(plane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

[Matt Roper]

On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote:
> On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote:
> > We never removed the sprite watermark updates from our low-level
> > foo_update_plane() functions; since our hardware updates happen under
> > vblank evasion, we're not supposed to be calling potentially sleeping
> > functions there (since interrupts are disabled).  We do already have a
> > mechanism that's supposed to be used to update sprite watermarks in the
> > post-evasion function intel_post_plane_update(), but at the moment it's
> > only being used for updates caused by plane disables.
> > 
> > To fix this oversight we need to make a few changes:
> > 
> >  * When intel_plane_atomic_calc_changes() calls intel_wm_need_update()
> >    to determine whether watermarks need an update, we need to set the
> >    'atomic.update_sprite_watermarks' flag rather than the
> >    'atomic.update_wm' flag when the plane in question is a sprite.  Some
> >    platforms don't care about this change since the two types of update
> >    do the same thing, but some platforms (e.g., ILK-style watermarks)
> >    need to specifically use the sprite update function to update cached
> >    watermark parameters.
> > 
> >  * intel_wm_need_update() needs to also look for plane size changes
> >    (previously it only checked tiling & rotation).
> > 
> > With the above changes, the need for sprite watermark updates should be
> > properly flagged at atomic 'check' time and handled at 'commit' time in
> > post-evasion, so we can drop the direct calls to
> > intel_update_sprite_watermarks() from all of the
> > intel_plane->update_plane() handlers.  We'll also toss a
> > WARN_ON(irqs_disabled()) into the watermark update functions to avoid
> > such mistakes in the future.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Why do even still need a separate sprite wm update hooks? Aren't we yet
> recomputing wm values for all planes if something changes, then diffing
> them with the current one and updating if anything changed? That seems to
> be what update_sprite_wm essentially does, except it's pre-atomic and
> open-codes the updates ...

Some platforms stage a copy of plane/pipe state in 'wm parameter'
structures and only update the pipe wm params with the new plane values
when update_sprite_wm is called.  That's definitely something my main
watermark series kills off, but until we get to that point and remove
the extra state staging in 'params' I think we still need to make sure
the update_sprite_wm is called appropriately to make sure watermarks use
the right values on all platforms.  I figured these two patches were a
quick fix for a known bug while we get the main watermark series
hammered out.


Matt

> 
> Ofc there's still that ilk w/a around which needs to be moved to a
> suitable place.
> -Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_pm.c      |  2 ++
> >  drivers/gpu/drm/i915/intel_sprite.c  | 12 ------------
> >  3 files changed, 18 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5e8e01c..181aedd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11575,13 +11575,17 @@ retry:
> >  static bool intel_wm_need_update(struct drm_plane *plane,
> >  				 struct drm_plane_state *state)
> >  {
> > -	/* Update watermarks on tiling changes. */
> > +	struct intel_plane_state *new = to_intel_plane_state(state);
> > +	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> > +
> > +	/* Update watermarks on tiling changes or size changes. */
> >  	if (!plane->state->fb || !state->fb ||
> >  	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> > -	    plane->state->rotation != state->rotation)
> > -		return true;
> > -
> > -	if (plane->state->crtc_w != state->crtc_w)
> > +	    plane->state->rotation != state->rotation ||
> > +	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
> > +	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
> > +	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
> > +	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
> >  		return true;
> >  
> >  	return false;
> > @@ -11645,8 +11649,13 @@ 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 (intel_wm_need_update(plane, plane_state))
> > -		intel_crtc->atomic.update_wm = true;
> > +	if (intel_wm_need_update(plane, plane_state)) {
> > +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> > +			intel_crtc->atomic.update_sprite_watermarks |=
> > +				1 << i;
> > +		else
> > +			intel_crtc->atomic.update_wm = true;
> > +	}
> >  
> >  	switch (plane->type) {
> >  	case DRM_PLANE_TYPE_PRIMARY:
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 4d3cb70..2470ede 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> >  
> > +	WARN_ON(irqs_disabled());
> >  	if (dev_priv->display.update_wm)
> >  		dev_priv->display.update_wm(crtc);
> >  }
> > @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
> >  	unsigned int dst_h = drm_rect_height(&state->dst);
> >  	bool scaled = (src_w != dst_w || src_h != dst_h);
> >  
> > +	WARN_ON(irqs_disabled());
> >  	if (dev_priv->display.update_sprite_wm)
> >  		dev_priv->display.update_sprite_wm(plane, crtc,
> >  						   sprite_width, sprite_height,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index b627067..ce2fa92 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	rotation = drm_plane->state->rotation;
> >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> >  
> > -	intel_update_sprite_watermarks(drm_plane, crtc);
> > -
> >  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >  					       fb->pixel_format);
> >  
> > @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  
> >  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
> >  	POSTING_READ(PLANE_SURF(pipe, plane));
> > -
> > -	intel_update_sprite_watermarks(dplane, crtc);
> >  }
> >  
> >  static void
> > @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	if (obj->tiling_mode != I915_TILING_NONE)
> >  		sprctl |= SP_TILED;
> >  
> > -	intel_update_sprite_watermarks(dplane, crtc);
> > -
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  
> >  	I915_WRITE(SPSURF(pipe, plane), 0);
> >  	POSTING_READ(SPSURF(pipe, plane));
> > -
> > -	intel_update_sprite_watermarks(dplane, crtc);
> >  }
> >  
> >  static void
> > @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> >  
> > -	intel_update_sprite_watermarks(plane, crtc);
> > -
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	if (IS_GEN6(dev))
> >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
> >  
> > -	intel_update_sprite_watermarks(plane, crtc);
> > -
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

[Daniel Vetter]

On Tue, Jun 23, 2015 at 07:29:58AM -0700, Matt Roper wrote:
> On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote:
> > On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote:
> > > We never removed the sprite watermark updates from our low-level
> > > foo_update_plane() functions; since our hardware updates happen under
> > > vblank evasion, we're not supposed to be calling potentially sleeping
> > > functions there (since interrupts are disabled).  We do already have a
> > > mechanism that's supposed to be used to update sprite watermarks in the
> > > post-evasion function intel_post_plane_update(), but at the moment it's
> > > only being used for updates caused by plane disables.
> > > 
> > > To fix this oversight we need to make a few changes:
> > > 
> > >  * When intel_plane_atomic_calc_changes() calls intel_wm_need_update()
> > >    to determine whether watermarks need an update, we need to set the
> > >    'atomic.update_sprite_watermarks' flag rather than the
> > >    'atomic.update_wm' flag when the plane in question is a sprite.  Some
> > >    platforms don't care about this change since the two types of update
> > >    do the same thing, but some platforms (e.g., ILK-style watermarks)
> > >    need to specifically use the sprite update function to update cached
> > >    watermark parameters.
> > > 
> > >  * intel_wm_need_update() needs to also look for plane size changes
> > >    (previously it only checked tiling & rotation).
> > > 
> > > With the above changes, the need for sprite watermark updates should be
> > > properly flagged at atomic 'check' time and handled at 'commit' time in
> > > post-evasion, so we can drop the direct calls to
> > > intel_update_sprite_watermarks() from all of the
> > > intel_plane->update_plane() handlers.  We'll also toss a
> > > WARN_ON(irqs_disabled()) into the watermark update functions to avoid
> > > such mistakes in the future.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > Why do even still need a separate sprite wm update hooks? Aren't we yet
> > recomputing wm values for all planes if something changes, then diffing
> > them with the current one and updating if anything changed? That seems to
> > be what update_sprite_wm essentially does, except it's pre-atomic and
> > open-codes the updates ...
> 
> Some platforms stage a copy of plane/pipe state in 'wm parameter'
> structures and only update the pipe wm params with the new plane values
> when update_sprite_wm is called.  That's definitely something my main
> watermark series kills off, but until we get to that point and remove
> the extra state staging in 'params' I think we still need to make sure
> the update_sprite_wm is called appropriately to make sure watermarks use
> the right values on all platforms.  I figured these two patches were a
> quick fix for a known bug while we get the main watermark series
> hammered out.

Oh I thought that update_wm hooks already walk over all planes for all
platforms and so if we call that for all (atomic) modeset we should be
covered. I did see that update_sprite_wm do update some of those cached
values before calling intel_update_wm though and so wondered whether we
still need this all. Sounds like we still do :(
-Daniel

> 
> 
> Matt
> 
> > 
> > Ofc there's still that ilk w/a around which needs to be moved to a
> > suitable place.
> > -Daniel
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
> > >  drivers/gpu/drm/i915/intel_pm.c      |  2 ++
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 12 ------------
> > >  3 files changed, 18 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 5e8e01c..181aedd 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11575,13 +11575,17 @@ retry:
> > >  static bool intel_wm_need_update(struct drm_plane *plane,
> > >  				 struct drm_plane_state *state)
> > >  {
> > > -	/* Update watermarks on tiling changes. */
> > > +	struct intel_plane_state *new = to_intel_plane_state(state);
> > > +	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> > > +
> > > +	/* Update watermarks on tiling changes or size changes. */
> > >  	if (!plane->state->fb || !state->fb ||
> > >  	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> > > -	    plane->state->rotation != state->rotation)
> > > -		return true;
> > > -
> > > -	if (plane->state->crtc_w != state->crtc_w)
> > > +	    plane->state->rotation != state->rotation ||
> > > +	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
> > > +	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
> > > +	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
> > > +	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
> > >  		return true;
> > >  
> > >  	return false;
> > > @@ -11645,8 +11649,13 @@ 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 (intel_wm_need_update(plane, plane_state))
> > > -		intel_crtc->atomic.update_wm = true;
> > > +	if (intel_wm_need_update(plane, plane_state)) {
> > > +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> > > +			intel_crtc->atomic.update_sprite_watermarks |=
> > > +				1 << i;
> > > +		else
> > > +			intel_crtc->atomic.update_wm = true;
> > > +	}
> > >  
> > >  	switch (plane->type) {
> > >  	case DRM_PLANE_TYPE_PRIMARY:
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 4d3cb70..2470ede 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc)
> > >  {
> > >  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> > >  
> > > +	WARN_ON(irqs_disabled());
> > >  	if (dev_priv->display.update_wm)
> > >  		dev_priv->display.update_wm(crtc);
> > >  }
> > > @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
> > >  	unsigned int dst_h = drm_rect_height(&state->dst);
> > >  	bool scaled = (src_w != dst_w || src_h != dst_h);
> > >  
> > > +	WARN_ON(irqs_disabled());
> > >  	if (dev_priv->display.update_sprite_wm)
> > >  		dev_priv->display.update_sprite_wm(plane, crtc,
> > >  						   sprite_width, sprite_height,
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index b627067..ce2fa92 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> > >  	rotation = drm_plane->state->rotation;
> > >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> > >  
> > > -	intel_update_sprite_watermarks(drm_plane, crtc);
> > > -
> > >  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > >  					       fb->pixel_format);
> > >  
> > > @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > >  
> > >  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
> > >  	POSTING_READ(PLANE_SURF(pipe, plane));
> > > -
> > > -	intel_update_sprite_watermarks(dplane, crtc);
> > >  }
> > >  
> > >  static void
> > > @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > >  	if (obj->tiling_mode != I915_TILING_NONE)
> > >  		sprctl |= SP_TILED;
> > >  
> > > -	intel_update_sprite_watermarks(dplane, crtc);
> > > -
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > >  
> > >  	I915_WRITE(SPSURF(pipe, plane), 0);
> > >  	POSTING_READ(SPSURF(pipe, plane));
> > > -
> > > -	intel_update_sprite_watermarks(dplane, crtc);
> > >  }
> > >  
> > >  static void
> > > @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > >  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > >  
> > > -	intel_update_sprite_watermarks(plane, crtc);
> > > -
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	if (IS_GEN6(dev))
> > >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
> > >  
> > > -	intel_update_sprite_watermarks(plane, crtc);
> > > -
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx