[RFC 00/15] Atomic watermark updates

[RFC 00/15] Atomic watermark updates

[Matt Roper]

Here's an early RFC series for transitioning watermark updates to an atomic
model.  The general idea is that we want to move calculation of a pipe's
watermark values to the atomic check phase rather than doing it at the commit
stage as we do today, but we actually want to calculate two sets of values: a
"final" (i.e., optimal) set of watermark values, plus an "intermediate"
(transitional) set of values.  The intermediate watermark values are a
combination of the values for the old CRTC state and the new CRTC state so they
should be able to satisfy the needs of both configurations.  That means that
when we get to the atomic commit phase, we can immediately program the
intermediate watermark values without waiting for the vblank to happen.  Later,
when the vblank finally does happen, a workqueue task will be scheduled to
program the watermarks again, this time using the final, optimal values.

Watermark programming details vary wildly between the various platforms we
support.  For this initial RFC, I've only coverted ILK-style platforms to use
this new model (to keep the series simple, and because IVB, which uses
ILK-style watermarks, is the only platform I actually have available to test on
at the moment).  The behavior on platforms I haven't converted should remain
the same.

This series is still very rough and is definitely not ready for merging yet;
I'm just looking to get some feedback that the general approach looks good and
is the way we want to handle this.

One known issue at the moment is that this series has some issues when
disabling a CRTC while sprite planes are active; it seems our modeset code
isn't quite far enough along with the atomic conversion for the calculated
plane states to get properly swapped into plane->state at the commit stage.  I
know Maarten has a bunch of patches that clean up and pretty much finish the
atomic conversion for legacy modeset, so I suspect his series will solve this
issue.



Matt Roper (14):
  drm/i915: Test plane mask for sprite watermark updates properly
  drm/i915: Drop parameters to intel_update_sprite_watermarks()
  drm/i915: Update sprite watermarks outside vblank evasion
  drm/i915: Make atomic use in-flight state for CRTC active value (v2)
  drm/i915: Lookup CRTC for plane directly
  drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM
    code
  drm/i915: Eliminate usage of pipe_wm_parameters from ILK-style WM
  drm/i915: Allow ILK watermark computation to use atomic state
  drm/i915: Move active watermarks into CRTC state
  drm/i915: Calculate pipe watermark values during atomic check
  drm/i915: Actually use pre-computer watermark values (!!SQUASHME)
  drm/i915: Introduce intel_schedule_vblank_job()
  drm/i915: Program atomic watermarks via vblank job
  drm/i915: Add intermediate watermarks

Ville Syrjälä (1):
  drm/i915: Refactor ilk_update_wm (v3)

 drivers/gpu/drm/i915/i915_drv.h           |  16 ++
 drivers/gpu/drm/i915/i915_irq.c           | 117 ++++++++++
 drivers/gpu/drm/i915/intel_atomic.c       |  40 +++-
 drivers/gpu/drm/i915/intel_atomic_plane.c |  23 +-
 drivers/gpu/drm/i915/intel_display.c      | 147 +++++++++++--
 drivers/gpu/drm/i915/intel_drv.h          |  97 ++++++---
 drivers/gpu/drm/i915/intel_pm.c           | 345 +++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_sprite.c       |  40 +---
 8 files changed, 584 insertions(+), 241 deletions(-)

-- 
1.8.5.1

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

[RFC 01/15] drm/i915: Test plane mask for sprite watermark updates properly

[Matt Roper]

Our atomic transaction maintains a bitmask of planes that we need to
update sprite watermarks for once vblank evasion is complete.  When we
actually go to make use of that bitmask, we've been comparing against
the plane index rather than the plane mask; we need to update our
comparison to check '(1 << index)' rather than 'index' directly.

This bug was introduced by

        commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
        Author: Matt Roper <matthew.d.roper@intel.com>
        Date:   Wed Dec 24 07:59:06 2014 -0800

            drm/i915: Refactor work that can sleep out of commit (v7)

However we've been "lucky" and haven't actually run into problems caused
by this yet due to another bug in the code...we forgot to actually
remove the calls to intel_update_sprite_watermarks() from the low-level
plane programming functions that run under evasion.  We'll fix that
problem in a subsequent patch.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c97b496..1d70349 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13238,7 +13238,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 		intel_post_enable_primary(crtc);
 
 	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
-		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
+		if (intel_crtc->atomic.update_sprite_watermarks &
+		    (1 << drm_plane_index(p)))
 			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
 						       false, false);
 
-- 
1.8.5.1

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

[RFC 02/15] 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 us to extract 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 1d70349..0713258 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13240,8 +13240,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
 		if (intel_crtc->atomic.update_sprite_watermarks &
 		    (1 << drm_plane_index(p)))
-			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
-						       false, false);
+			intel_update_sprite_watermarks(p, crtc);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47bc729..fe966ce 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1344,11 +1344,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 ce1d079..2170cc5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3756,13 +3756,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 3f70d59..3a96956 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 = &intel_plane->ckey;
 	unsigned long surf_addr;
 	u32 tile_height, plane_offset, plane_size;
@@ -199,9 +198,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, bool force)
 	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--;
@@ -474,7 +469,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
-	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
+	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 
@@ -539,9 +534,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--;
@@ -678,9 +671,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--;
-- 
1.8.5.1

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

[RFC 03/15] 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).  Ensure that we
properly set the atomic.update_sprite_watermarks flag so that these
updates will happen outside vblank evasion and we can drop the direct
calls from the plane programming code.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++--
 drivers/gpu/drm/i915/intel_sprite.c  | 22 ++++------------------
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0713258..e12b5a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12902,10 +12902,17 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 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)
+	    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;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3a96956..394cf37 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -198,8 +198,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, bool force)
 	/* Activate double buffered register update */
 	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--;
@@ -468,8 +462,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	intel_update_sprite_watermarks(dplane, crtc);
 }
 
 
@@ -534,8 +526,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--;
@@ -671,8 +661,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--;
@@ -921,18 +909,16 @@ finish:
 		intel_crtc->atomic.fb_bits |=
 			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
 
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
+		if (intel_wm_need_update(plane, &state->base) || !state->visible)
+			intel_crtc->atomic.update_sprite_watermarks |=
+				(1 << drm_plane_index(plane));
 
-		if (!state->visible) {
+		if (!state->visible)
 			/*
 			 * Avoid underruns when disabling the sprite.
 			 * FIXME remove once watermark updates are done properly.
 			 */
 			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-- 
1.8.5.1

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

[RFC 04/15] drm/i915: Make atomic use in-flight state for CRTC active value (v2)

[Matt Roper]

Our atomic plane code currently uses intel_crtc->active to determine
how/when to update some derived state values.  This works fine for pure
plane updates at the moment since the CRTC state itself isn't changed as
part of the operation.  However as we convert more of our driver
internals over to atomic modesetting, we need to look at whether the
CRTC will be active at the *end* of the atomic transaction (which may
not match the currently committed state).

The in-flight value we want to use is generally in a crtc_state object
associated with our top-level atomic transaction.  However one exception
to note is that when updating properties of a disabled plane (that is
staying disabled), we'll have a top-level atomic state, but it may not
contain the CRTC state we're looking for.  This means we're not actually
touching any CRTC state so it's safe to use the value from crtc->state
directly.

Note that we're only changing the 'check' side of updates to read out of
in-flight state vs committed state; this takes care of making sure our
derived state is updated as expected.  The 'commit' code responsible for
actually programming the hardware still looks at intel_crtc->active so
that we won't try to write plane registers while the CRTC is disabled.

v2: Rebasing and cleanup

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +------
 drivers/gpu/drm/i915/intel_display.c      | 49 ++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h          |  3 ++
 drivers/gpu/drm/i915/intel_sprite.c       |  5 ++--
 4 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 86ba4b2..714ee24 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -127,16 +127,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	if (!crtc)
 		return 0;
 
-	/* FIXME: temporary hack necessary while we still use the plane update
-	 * helper. */
-	if (state->state) {
-		crtc_state =
-			intel_atomic_get_crtc_state(state->state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	} else {
-		crtc_state = intel_crtc->config;
-	}
+	crtc_state = intel_crtc_state_for_plane(intel_state);
 
 	/*
 	 * The original src/dest coordinates are stored in state->base, but
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e12b5a4..1a7d2a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13034,6 +13034,46 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 	return max_scale;
 }
 
+/**
+ * intel_crtc_state_for_plane - Obtain CRTC state for a plane
+ * @pstate: plane state to lookup corresponding crtc state for
+ *
+ * When working with a top-level atomic transaction (drm_atomic_state),
+ * a CRTC state should be present that corresponds to the provided
+ * plane state; this function provides a quick way to fetch that
+ * CRTC state.  In cases where we have a plane state unassociated with any
+ * top-level atomic transaction (e.g., while using the transitional atomic
+ * helpers), the current CRTC state from crtc->state will be returned
+ * instead.
+ */
+struct intel_crtc_state *
+intel_crtc_state_for_plane(struct intel_plane_state *pstate)
+{
+	struct drm_atomic_state *state = pstate->base.state;
+	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
+	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
+							plane->pipe);
+	struct intel_crtc_state *crtc_state;
+
+	/*
+	 * While using transitional plane helpers, we may not have a top-level
+	 * atomic transaction.  Of course that also means that we're not
+	 * actually touching CRTC state, so just return the currently
+	 * committed state.
+	 *
+	 * FIXME: Once our modeset code stops using transitional helpers
+	 * internally we should add a WARN_ON() to this condition.
+	 */
+	if (!state)
+		return to_intel_crtc_state(crtc->state);
+
+	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
+	if (WARN_ON(IS_ERR(crtc_state)))
+		crtc_state = to_intel_crtc_state(crtc->state);
+
+	return crtc_state;
+}
+
 static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -13054,8 +13094,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
+	crtc_state = intel_crtc_state_for_plane(state);
 
 	if (INTEL_INFO(dev)->gen >= 9) {
 		min_scale = 1;
@@ -13072,7 +13111,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	if (intel_crtc->active) {
+	if (crtc_state->base.active) {
 		struct intel_plane_state *old_state =
 			to_intel_plane_state(plane->state);
 
@@ -13366,6 +13405,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	unsigned stride;
 	int ret;
 
@@ -13404,7 +13444,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 
 finish:
-	if (intel_crtc->active) {
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	if (intel_crtc_state->base.active) {
 		if (plane->state->crtc_w != state->base.crtc_w)
 			intel_crtc->atomic.update_wm = true;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fe966ce..1e22ffe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1072,6 +1072,9 @@ void intel_create_rotation_property(struct drm_device *dev,
 
 bool intel_wm_need_update(struct drm_plane *plane,
 			  struct drm_plane_state *state);
+struct intel_crtc_state *
+intel_crtc_state_for_plane(struct intel_plane_state *pstate);
+
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 394cf37..bfe9213 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -753,8 +753,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int ret;
 
 	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
+	crtc_state = intel_crtc_state_for_plane(state);
 
 	if (!fb) {
 		state->visible = false;
@@ -905,7 +904,7 @@ finish:
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	if (intel_crtc->active) {
+	if (crtc_state->base.active) {
 		intel_crtc->atomic.fb_bits |=
 			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
 
-- 
1.8.5.1

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

[RFC 05/15] drm/i915: Lookup CRTC for plane directly

[Matt Roper]

Various places in the atomic plane code obtain the CRTC via
plane_state->crtc.  But plane_state->crtc is NULL when disabling the
plane, so the code will fall back to looking at the old CRTC value in
plane->crtc in that case.  This isn't going to work when we start
supporting non-blocking flips (since the legacy plane->crtc pointer will
already be updated to its new value before the asynchronous workqueue
task runs the plane commit function).  The code is also fragile in
general (we have to be careful when doing stuff like updating properties
on a disabled plane).  Since Intel hardware has a fixed plane to pipe
mapping, let's just lookup the CRTC via that fixed mapping and avoid
future mistakes.

Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
 drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
 drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 714ee24..21c808d 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -115,16 +115,16 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	/*
-	 * Both crtc and plane->crtc could be NULL if we're updating a
-	 * property while the plane is disabled.  We don't actually have
-	 * anything driver-specific we need to test in that case, so
-	 * just return success.
+	 * Both state->crtc and plane->state->crtc could be NULL if we're
+	 * updating a property while the plane is disabled.  We don't actually
+	 * have anything driver-specific we need to test in that case, so just
+	 * return success.
 	 */
-	if (!crtc)
+	if (!state->crtc && !plane->state->crtc)
 		return 0;
 
 	crtc_state = intel_crtc_state_for_plane(intel_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a7d2a9..f4398a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13092,7 +13092,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 	crtc_state = intel_crtc_state_for_plane(state);
 
@@ -13174,7 +13174,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc;
 	struct drm_rect *src = &state->src;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = fb;
@@ -13409,7 +13409,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	unsigned stride;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
@@ -13482,7 +13482,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
 	uint32_t addr;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = state->base.fb;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1e22ffe..ea67093 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -803,6 +803,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
 	return dev_priv->plane_to_crtc_mapping[plane];
 }
 
+static inline struct drm_crtc *
+intel_get_crtc_for_drm_plane(struct drm_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
+}
+
 struct intel_unpin_work {
 	struct work_struct work;
 	struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index bfe9213..2164e63 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -752,7 +752,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int pixel_size;
 	int ret;
 
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
+	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
 	crtc_state = intel_crtc_state_for_plane(state);
 
 	if (!fb) {
@@ -942,7 +942,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = fb;
-- 
1.8.5.1

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

[RFC 06/15] drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code

[Matt Roper]

Just pull the info out of the plane state structure rather than staging
it in an additional structure.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   4 ++
 drivers/gpu/drm/i915/intel_pm.c | 135 +++++++++++++++++++++-------------------
 2 files changed, 75 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 731b5ce..df5673f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -243,6 +243,10 @@ enum hpd_pin {
 			    &dev->mode_config.plane_list,	\
 			    base.head)
 
+#define for_each_intel_plane_of_crtc(intel_crtc, intel_plane) \
+	for_each_intel_plane(intel_crtc->base.dev, intel_plane) \
+		if (intel_plane->pipe == intel_crtc->pipe)
+
 #define for_each_intel_crtc(dev, intel_crtc) \
 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2170cc5..e4242ff 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1514,9 +1514,6 @@ struct ilk_pipe_wm_parameters {
 	bool active;
 	uint32_t pipe_htotal;
 	uint32_t pixel_rate;
-	struct intel_plane_wm_parameters pri;
-	struct intel_plane_wm_parameters spr;
-	struct intel_plane_wm_parameters cur;
 };
 
 struct ilk_wm_maximums {
@@ -1538,25 +1535,25 @@ struct intel_wm_config {
  * mem_value must be in 0.1us units.
  */
 static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
+				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value,
 				   bool is_lp)
 {
+	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 	uint32_t method1, method2;
 
-	if (!params->active || !params->pri.enabled)
+	if (!params->active || !pstate->visible)
 		return 0;
 
-	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->pri.bytes_per_pixel,
-				 mem_value);
+	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
 
 	if (!is_lp)
 		return method1;
 
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->pri.horiz_pixels,
-				 params->pri.bytes_per_pixel,
+				 drm_rect_width(&pstate->dst),
+				 bpp,
 				 mem_value);
 
 	return min(method1, method2);
@@ -1567,20 +1564,20 @@ static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
  * mem_value must be in 0.1us units.
  */
 static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
+				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value)
 {
+	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 	uint32_t method1, method2;
 
-	if (!params->active || !params->spr.enabled)
+	if (!params->active || !pstate->visible)
 		return 0;
 
-	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->spr.bytes_per_pixel,
-				 mem_value);
+	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->spr.horiz_pixels,
-				 params->spr.bytes_per_pixel,
+				 drm_rect_width(&pstate->dst),
+				 bpp,
 				 mem_value);
 	return min(method1, method2);
 }
@@ -1590,28 +1587,32 @@ static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
  * mem_value must be in 0.1us units.
  */
 static uint32_t ilk_compute_cur_wm(const struct ilk_pipe_wm_parameters *params,
+				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value)
 {
-	if (!params->active || !params->cur.enabled)
+	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
+
+	if (!params->active || !pstate->visible)
 		return 0;
 
 	return ilk_wm_method2(params->pixel_rate,
 			      params->pipe_htotal,
-			      params->cur.horiz_pixels,
-			      params->cur.bytes_per_pixel,
+			      drm_rect_width(&pstate->dst),
+			      bpp,
 			      mem_value);
 }
 
 /* Only for WM_LP. */
 static uint32_t ilk_compute_fbc_wm(const struct ilk_pipe_wm_parameters *params,
+				   const struct intel_plane_state *pstate,
 				   uint32_t pri_val)
 {
-	if (!params->active || !params->pri.enabled)
+	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
+
+	if (!params->active || !pstate->visible)
 		return 0;
 
-	return ilk_wm_fbc(pri_val,
-			  params->pri.horiz_pixels,
-			  params->pri.bytes_per_pixel);
+	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), bpp);
 }
 
 static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
@@ -1776,10 +1777,12 @@ static bool ilk_validate_wm_level(int level,
 }
 
 static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
+				 const struct intel_crtc *intel_crtc,
 				 int level,
 				 const struct ilk_pipe_wm_parameters *p,
 				 struct intel_wm_level *result)
 {
+	struct intel_plane *intel_plane;
 	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
 	uint16_t spr_latency = dev_priv->wm.spr_latency[level];
 	uint16_t cur_latency = dev_priv->wm.cur_latency[level];
@@ -1791,10 +1794,29 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	result->pri_val = ilk_compute_pri_wm(p, pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(p, spr_latency);
-	result->cur_val = ilk_compute_cur_wm(p, cur_latency);
-	result->fbc_val = ilk_compute_fbc_wm(p, result->pri_val);
+	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
+		struct intel_plane_state *pstate =
+			to_intel_plane_state(intel_plane->base.state);
+
+		switch (intel_plane->base.type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			result->pri_val = ilk_compute_pri_wm(p, pstate,
+							     pri_latency,
+							     level);
+			result->fbc_val = ilk_compute_fbc_wm(p, pstate,
+							     result->pri_val);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			result->spr_val = ilk_compute_spr_wm(p, pstate,
+							     spr_latency);
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			result->cur_val = ilk_compute_cur_wm(p, pstate,
+							     cur_latency);
+			break;
+		}
+	}
+
 	result->enable = true;
 }
 
@@ -2056,43 +2078,14 @@ static void skl_setup_wm_latency(struct drm_device *dev)
 static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 				      struct ilk_pipe_wm_parameters *p)
 {
-	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_plane *plane;
 
 	if (!intel_crtc->active)
 		return;
 
 	p->active = true;
 	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
-	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-
-	if (crtc->primary->state->fb)
-		p->pri.bytes_per_pixel =
-			crtc->primary->state->fb->bits_per_pixel / 8;
-	else
-		p->pri.bytes_per_pixel = 4;
-
-	p->cur.bytes_per_pixel = 4;
-	/*
-	 * TODO: for now, assume primary and cursor planes are always enabled.
-	 * Setting them to false makes the screen flicker.
-	 */
-	p->pri.enabled = true;
-	p->cur.enabled = true;
-
-	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
-	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
-
-	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		struct intel_plane *intel_plane = to_intel_plane(plane);
-
-		if (intel_plane->pipe == pipe) {
-			p->spr = intel_plane->wm;
-			break;
-		}
-	}
+	p->pixel_rate = ilk_pipe_pixel_rate(crtc->dev, crtc);
 }
 
 static void ilk_compute_wm_config(struct drm_device *dev,
@@ -2120,28 +2113,42 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane;
+	struct intel_plane_state *sprstate = NULL;
 	int level, max_level = ilk_wm_max_level(dev);
 	/* LP0 watermark maximums depend on this pipe alone */
 	struct intel_wm_config config = {
 		.num_pipes_active = 1,
-		.sprites_enabled = params->spr.enabled,
-		.sprites_scaled = params->spr.scaled,
 	};
 	struct ilk_wm_maximums max;
 
+	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
+		if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
+			sprstate = to_intel_plane_state(intel_plane->base.state);
+			break;
+		}
+	}
+
+	config.sprites_enabled = sprstate->visible;
+	config.sprites_scaled =
+		drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
+		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
+
+
 	pipe_wm->pipe_enabled = params->active;
-	pipe_wm->sprites_enabled = params->spr.enabled;
-	pipe_wm->sprites_scaled = params->spr.scaled;
+	pipe_wm->sprites_enabled = sprstate->visible;
+	pipe_wm->sprites_scaled = config.sprites_scaled;
 
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
-	if (INTEL_INFO(dev)->gen <= 6 && params->spr.enabled)
+	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
 		max_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
-	if (params->spr.scaled)
+	if (config.sprites_scaled)
 		max_level = 0;
 
-	ilk_compute_wm_level(dev_priv, 0, params, &pipe_wm->wm[0]);
+	ilk_compute_wm_level(dev_priv, intel_crtc, 0, params, &pipe_wm->wm[0]);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
@@ -2158,7 +2165,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 	for (level = 1; level <= max_level; level++) {
 		struct intel_wm_level wm = {};
 
-		ilk_compute_wm_level(dev_priv, level, params, &wm);
+		ilk_compute_wm_level(dev_priv, intel_crtc, level, params, &wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
-- 
1.8.5.1

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

[RFC 07/15] drm/i915: Eliminate usage of pipe_wm_parameters from ILK-style WM

[Matt Roper]

Just pull the info out of the CRTC state structure rather than staging
it in an additional structure.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 98 ++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e4242ff..13b1cd3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1434,23 +1434,21 @@ static void i845_update_wm(struct drm_crtc *unused_crtc)
 	I915_WRITE(FW_BLC, fwater_lo);
 }
 
-static uint32_t ilk_pipe_pixel_rate(struct drm_device *dev,
-				    struct drm_crtc *crtc)
+static uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *cstate)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t pixel_rate;
 
-	pixel_rate = intel_crtc->config->base.adjusted_mode.crtc_clock;
+	pixel_rate = cstate->base.adjusted_mode.crtc_clock;
 
 	/* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
 	 * adjust the pixel_rate here. */
 
-	if (intel_crtc->config->pch_pfit.enabled) {
+	if (cstate->pch_pfit.enabled) {
 		uint64_t pipe_w, pipe_h, pfit_w, pfit_h;
-		uint32_t pfit_size = intel_crtc->config->pch_pfit.size;
+		uint32_t pfit_size = cstate->pch_pfit.size;
 
-		pipe_w = intel_crtc->config->pipe_src_w;
-		pipe_h = intel_crtc->config->pipe_src_h;
+		pipe_w = cstate->pipe_src_w;
+		pipe_h = cstate->pipe_src_h;
 		pfit_w = (pfit_size >> 16) & 0xFFFF;
 		pfit_h = pfit_size & 0xFFFF;
 		if (pipe_w < pfit_w)
@@ -1510,12 +1508,6 @@ struct skl_pipe_wm_parameters {
 	struct intel_plane_wm_parameters cursor;
 };
 
-struct ilk_pipe_wm_parameters {
-	bool active;
-	uint32_t pipe_htotal;
-	uint32_t pixel_rate;
-};
-
 struct ilk_wm_maximums {
 	uint16_t pri;
 	uint16_t spr;
@@ -1534,7 +1526,7 @@ struct intel_wm_config {
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
+static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value,
 				   bool is_lp)
@@ -1542,16 +1534,16 @@ static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
 	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 	uint32_t method1, method2;
 
-	if (!params->active || !pstate->visible)
+	if (!cstate->base.active || !pstate->visible)
 		return 0;
 
-	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
+	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
 
 	if (!is_lp)
 		return method1;
 
-	method2 = ilk_wm_method2(params->pixel_rate,
-				 params->pipe_htotal,
+	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
+				 cstate->base.adjusted_mode.crtc_htotal,
 				 drm_rect_width(&pstate->dst),
 				 bpp,
 				 mem_value);
@@ -1563,19 +1555,19 @@ static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
+static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value)
 {
 	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 	uint32_t method1, method2;
 
-	if (!params->active || !pstate->visible)
+	if (!cstate->base.active || !pstate->visible)
 		return 0;
 
-	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
-	method2 = ilk_wm_method2(params->pixel_rate,
-				 params->pipe_htotal,
+	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
+	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
+				 cstate->base.adjusted_mode.crtc_htotal,
 				 drm_rect_width(&pstate->dst),
 				 bpp,
 				 mem_value);
@@ -1586,30 +1578,30 @@ static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_cur_wm(const struct ilk_pipe_wm_parameters *params,
+static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value)
 {
 	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 
-	if (!params->active || !pstate->visible)
+	if (!cstate->base.active || !pstate->visible)
 		return 0;
 
-	return ilk_wm_method2(params->pixel_rate,
-			      params->pipe_htotal,
+	return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
+			      cstate->base.adjusted_mode.crtc_htotal,
 			      drm_rect_width(&pstate->dst),
 			      bpp,
 			      mem_value);
 }
 
 /* Only for WM_LP. */
-static uint32_t ilk_compute_fbc_wm(const struct ilk_pipe_wm_parameters *params,
+static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t pri_val)
 {
 	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 
-	if (!params->active || !pstate->visible)
+	if (!cstate->base.active || !pstate->visible)
 		return 0;
 
 	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), bpp);
@@ -1779,7 +1771,7 @@ static bool ilk_validate_wm_level(int level,
 static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 				 const struct intel_crtc *intel_crtc,
 				 int level,
-				 const struct ilk_pipe_wm_parameters *p,
+				 struct intel_crtc_state *cstate,
 				 struct intel_wm_level *result)
 {
 	struct intel_plane *intel_plane;
@@ -1800,18 +1792,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 
 		switch (intel_plane->base.type) {
 		case DRM_PLANE_TYPE_PRIMARY:
-			result->pri_val = ilk_compute_pri_wm(p, pstate,
+			result->pri_val = ilk_compute_pri_wm(cstate, pstate,
 							     pri_latency,
 							     level);
-			result->fbc_val = ilk_compute_fbc_wm(p, pstate,
+			result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
 							     result->pri_val);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			result->spr_val = ilk_compute_spr_wm(p, pstate,
+			result->spr_val = ilk_compute_spr_wm(cstate, pstate,
 							     spr_latency);
 			break;
 		case DRM_PLANE_TYPE_CURSOR:
-			result->cur_val = ilk_compute_cur_wm(p, pstate,
+			result->cur_val = ilk_compute_cur_wm(cstate, pstate,
 							     cur_latency);
 			break;
 		}
@@ -2075,19 +2067,6 @@ static void skl_setup_wm_latency(struct drm_device *dev)
 	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
 }
 
-static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
-				      struct ilk_pipe_wm_parameters *p)
-{
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	if (!intel_crtc->active)
-		return;
-
-	p->active = true;
-	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
-	p->pixel_rate = ilk_pipe_pixel_rate(crtc->dev, crtc);
-}
-
 static void ilk_compute_wm_config(struct drm_device *dev,
 				  struct intel_wm_config *config)
 {
@@ -2107,10 +2086,10 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 }
 
 /* Compute new watermarks for the pipe */
-static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
-				  const struct ilk_pipe_wm_parameters *params,
+static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 				  struct intel_pipe_wm *pipe_wm)
 {
+	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -2135,8 +2114,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 		drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
 		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
 
-
-	pipe_wm->pipe_enabled = params->active;
+	pipe_wm->pipe_enabled = cstate->base.active;
 	pipe_wm->sprites_enabled = sprstate->visible;
 	pipe_wm->sprites_scaled = config.sprites_scaled;
 
@@ -2148,7 +2126,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 	if (config.sprites_scaled)
 		max_level = 0;
 
-	ilk_compute_wm_level(dev_priv, intel_crtc, 0, params, &pipe_wm->wm[0]);
+	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
@@ -2165,7 +2143,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 	for (level = 1; level <= max_level; level++) {
 		struct intel_wm_level wm = {};
 
-		ilk_compute_wm_level(dev_priv, intel_crtc, level, params, &wm);
+		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
@@ -3467,19 +3445,17 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 static void ilk_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct ilk_wm_maximums max;
-	struct ilk_pipe_wm_parameters params = {};
 	struct ilk_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
 	struct intel_pipe_wm pipe_wm = {};
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct intel_wm_config config = {};
 
-	ilk_compute_wm_parameters(crtc, &params);
-
-	intel_compute_pipe_wm(crtc, &params, &pipe_wm);
+	intel_compute_pipe_wm(cstate, &pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
 		return;
@@ -3519,12 +3495,6 @@ ilk_update_sprite_wm(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 
-	intel_plane->wm.enabled = enabled;
-	intel_plane->wm.scaled = scaled;
-	intel_plane->wm.horiz_pixels = sprite_width;
-	intel_plane->wm.vert_pixels = sprite_width;
-	intel_plane->wm.bytes_per_pixel = pixel_size;
-
 	/*
 	 * IVB workaround: must disable low power watermarks for at least
 	 * one frame before enabling scaling.  LP watermarks can be re-enabled
-- 
1.8.5.1

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

[RFC 08/15] drm/i915: Refactor ilk_update_wm (v3)

[Matt Roper]

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

Split ilk_update_wm() into two parts; one doing the programming
and the other the calculations.

v2: Fix typo in commit message

v3 (by Matt): Heavily rebased for current codebase.

Reviewed-by(v2): Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 13b1cd3..dbff278 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3442,25 +3442,14 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 	skl_update_wm(crtc);
 }
 
-static void ilk_update_wm(struct drm_crtc *crtc)
+static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct ilk_wm_maximums max;
+	struct intel_wm_config config = {};
 	struct ilk_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
-	struct intel_pipe_wm pipe_wm = {};
-	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
-	struct intel_wm_config config = {};
-
-	intel_compute_pipe_wm(cstate, &pipe_wm);
-
-	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
-		return;
-
-	intel_crtc->wm.active = pipe_wm;
 
 	ilk_compute_wm_config(dev, &config);
 
@@ -3486,6 +3475,23 @@ static void ilk_update_wm(struct drm_crtc *crtc)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
+static void ilk_update_wm(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct intel_pipe_wm pipe_wm = {};
+
+	intel_compute_pipe_wm(cstate, &pipe_wm);
+
+	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
+		return;
+
+	intel_crtc->wm.active = pipe_wm;
+
+	ilk_program_watermarks(dev_priv);
+}
+
 static void
 ilk_update_sprite_wm(struct drm_plane *plane,
 		     struct drm_crtc *crtc,
-- 
1.8.5.1

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

[RFC 09/15] drm/i915: Allow ILK watermark computation to use atomic state

[Matt Roper]

In preparation for refactoring watermark computation out to the atomic
'check' phase, we need to allow that computation to operate on the
in-flight state stored in a drm_atomic_state, rather than
already-committed CRTC and plane states (eventually the drm_atomic_state
should be the only source, but we allow both approaches at the moment as
we transition).

While we're at it, s/intel_compute_pipe_wm/ilk_compute_pipe_wm/ since
this function only applies to ILK-style watermarks and we have a
completely different function for SKL-style watermarks.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 102 +++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dbff278..c3b9f99 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1772,9 +1772,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 				 const struct intel_crtc *intel_crtc,
 				 int level,
 				 struct intel_crtc_state *cstate,
+				 struct intel_plane_state *pristate,
+				 struct intel_plane_state *sprstate,
+				 struct intel_plane_state *curstate,
 				 struct intel_wm_level *result)
 {
-	struct intel_plane *intel_plane;
 	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
 	uint16_t spr_latency = dev_priv->wm.spr_latency[level];
 	uint16_t cur_latency = dev_priv->wm.cur_latency[level];
@@ -1786,29 +1788,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
-		struct intel_plane_state *pstate =
-			to_intel_plane_state(intel_plane->base.state);
-
-		switch (intel_plane->base.type) {
-		case DRM_PLANE_TYPE_PRIMARY:
-			result->pri_val = ilk_compute_pri_wm(cstate, pstate,
-							     pri_latency,
-							     level);
-			result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
-							     result->pri_val);
-			break;
-		case DRM_PLANE_TYPE_OVERLAY:
-			result->spr_val = ilk_compute_spr_wm(cstate, pstate,
-							     spr_latency);
-			break;
-		case DRM_PLANE_TYPE_CURSOR:
-			result->cur_val = ilk_compute_cur_wm(cstate, pstate,
-							     cur_latency);
-			break;
-		}
-	}
-
+	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
+					     pri_latency, level);
+	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
+	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
+	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
 	result->enable = true;
 }
 
@@ -2086,15 +2070,20 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 }
 
 /* Compute new watermarks for the pipe */
-static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
-				  struct intel_pipe_wm *pipe_wm)
+static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
+			       struct drm_atomic_state *state,
+			       struct intel_pipe_wm *pipe_wm)
 {
-	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_crtc_state *cs;
+	struct intel_crtc_state *cstate = NULL;
 	struct intel_plane *intel_plane;
+	struct drm_plane_state *ps;
+	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
+	struct intel_plane_state *curstate = NULL;
 	int level, max_level = ilk_wm_max_level(dev);
 	/* LP0 watermark maximums depend on this pipe alone */
 	struct intel_wm_config config = {
@@ -2102,11 +2091,47 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 	};
 	struct ilk_wm_maximums max;
 
-	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
-		if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
-			sprstate = to_intel_plane_state(intel_plane->base.state);
-			break;
+	/*
+	 * FIXME: In an upcoming patch, we'll *only* be calling this from the
+	 * atomic 'check' codepath and thus will always have a top-level atomic
+	 * transaction.  However at the moment we still call this in the legacy
+	 * 'ilk_update_wm' function, which means we need to be able to also
+	 * operate on already-committed state which has been detached from any
+	 * top-level transaction.
+	 *
+	 * Drop the 'else' block here once we only do atomic-style watermarks.
+	 */
+	if (state) {
+		cs = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(cs))
+			return PTR_ERR(cs);
+		else
+			cstate = to_intel_crtc_state(cs);
+
+		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
+			ps = drm_atomic_get_plane_state(state,
+							&intel_plane->base);
+			if (IS_ERR(ps))
+				return PTR_ERR(ps);
+
+			if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+				pristate = to_intel_plane_state(ps);
+			else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
+				sprstate = to_intel_plane_state(ps);
+			else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
+				curstate = to_intel_plane_state(ps);
+		}
+	} else {
+		/* Use already-committed state */
+		cstate = to_intel_crtc_state(crtc->state);
+		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
+			if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
+				sprstate = to_intel_plane_state(intel_plane->base.state);
+				break;
+			}
 		}
+		pristate = to_intel_plane_state(crtc->primary->state);
+		curstate = to_intel_plane_state(crtc->cursor->state);
 	}
 
 	config.sprites_enabled = sprstate->visible;
@@ -2115,7 +2140,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
 
 	pipe_wm->pipe_enabled = cstate->base.active;
-	pipe_wm->sprites_enabled = sprstate->visible;
+	pipe_wm->sprites_enabled = config.sprites_enabled;
 	pipe_wm->sprites_scaled = config.sprites_scaled;
 
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
@@ -2126,7 +2151,8 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 	if (config.sprites_scaled)
 		max_level = 0;
 
-	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
+	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
+			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
@@ -2136,14 +2162,15 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 
 	/* At least LP0 must be valid */
 	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
-		return false;
+		return -EINVAL;
 
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
 		struct intel_wm_level wm = {};
 
-		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
+		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
+				     pristate, sprstate, curstate, &wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
@@ -2156,7 +2183,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
 		pipe_wm->wm[level] = wm;
 	}
 
-	return true;
+	return 0;
 }
 
 /*
@@ -3479,10 +3506,9 @@ static void ilk_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct intel_pipe_wm pipe_wm = {};
 
-	intel_compute_pipe_wm(cstate, &pipe_wm);
+	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
 		return;
-- 
1.8.5.1

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

[RFC 10/15] drm/i915: Move active watermarks into CRTC state

[Matt Roper]

Note that we rename them to 'target' since 'active' isn't really an
accurate term given that we're precomputing these well before they get
committed and become active.

Since we allocate a few CRTC states on the stack, also switch the 'wm'
struct here to be a union so that we're not wasting stack space with
other platforms' watermark values.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 44 ++++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_pm.c  | 33 ++++++++++++++++++------------
 2 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea67093..4ae109c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -319,6 +319,21 @@ struct intel_crtc_scaler_state {
 	int scaler_id;
 };
 
+struct intel_pipe_wm {
+	struct intel_wm_level wm[5];
+	uint32_t linetime;
+	bool fbc_wm_enabled;
+	bool pipe_enabled;
+	bool sprites_enabled;
+	bool sprites_scaled;
+};
+
+struct skl_pipe_wm {
+	struct skl_wm_level wm[8];
+	struct skl_wm_level trans_wm;
+	uint32_t linetime;
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -447,15 +462,14 @@ struct intel_crtc_state {
 	int pbn;
 
 	struct intel_crtc_scaler_state scaler_state;
-};
 
-struct intel_pipe_wm {
-	struct intel_wm_level wm[5];
-	uint32_t linetime;
-	bool fbc_wm_enabled;
-	bool pipe_enabled;
-	bool sprites_enabled;
-	bool sprites_scaled;
+	struct {
+		/* final, target watermarks for state */
+		union {
+			struct intel_pipe_wm ilk;
+			struct skl_pipe_wm skl;
+		} target;
+	} wm;
 };
 
 struct intel_mmio_flip {
@@ -464,12 +478,6 @@ struct intel_mmio_flip {
 	struct intel_crtc *crtc;
 };
 
-struct skl_pipe_wm {
-	struct skl_wm_level wm[8];
-	struct skl_wm_level trans_wm;
-	uint32_t linetime;
-};
-
 /*
  * Tracking of operations that need to be performed at the beginning/end of an
  * atomic commit, outside the atomic section where interrupts are disabled.
@@ -536,14 +544,6 @@ struct intel_crtc {
 	bool cpu_fifo_underrun_disabled;
 	bool pch_fifo_underrun_disabled;
 
-	/* per-pipe watermark state */
-	struct {
-		/* watermarks currently being used  */
-		struct intel_pipe_wm active;
-		/* SKL wm values currently in use */
-		struct skl_pipe_wm skl_active;
-	} wm;
-
 	int scanline_offset;
 
 	struct intel_crtc_atomic_commit atomic;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c3b9f99..789b4fe 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2058,7 +2058,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 
 	/* Compute the currently _active_ config */
 	for_each_intel_crtc(dev, intel_crtc) {
-		const struct intel_pipe_wm *wm = &intel_crtc->wm.active;
+		const struct intel_crtc_state *cstate =
+			to_intel_crtc_state(intel_crtc->base.state);
+		const struct intel_pipe_wm *wm = &cstate->wm.target.ilk;
 
 		if (!wm->pipe_enabled)
 			continue;
@@ -2198,7 +2200,9 @@ static void ilk_merge_wm_level(struct drm_device *dev,
 	ret_wm->enable = true;
 
 	for_each_intel_crtc(dev, intel_crtc) {
-		const struct intel_pipe_wm *active = &intel_crtc->wm.active;
+		const struct intel_crtc_state *cstate =
+			to_intel_crtc_state(intel_crtc->base.state);
+		const struct intel_pipe_wm *active = &cstate->wm.target.ilk;
 		const struct intel_wm_level *wm = &active->wm[level];
 
 		if (!active->pipe_enabled)
@@ -2344,14 +2348,15 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 
 	/* LP0 register values */
 	for_each_intel_crtc(dev, intel_crtc) {
+		const struct intel_crtc_state *cstate =
+			to_intel_crtc_state(intel_crtc->base.state);
 		enum pipe pipe = intel_crtc->pipe;
-		const struct intel_wm_level *r =
-			&intel_crtc->wm.active.wm[0];
+		const struct intel_wm_level *r = &cstate->wm.target.ilk.wm[0];
 
 		if (WARN_ON(!r->enable))
 			continue;
 
-		results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
+		results->wm_linetime[pipe] = cstate->wm.target.ilk.linetime;
 
 		results->wm_pipe[pipe] =
 			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
@@ -3344,16 +3349,16 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 			       struct skl_ddb_allocation *ddb, /* out */
 			       struct skl_pipe_wm *pipe_wm /* out */)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
 	skl_compute_wm_pipe_parameters(crtc, params);
 	skl_allocate_pipe_ddb(crtc, config, params, ddb);
 	skl_compute_pipe_wm(crtc, ddb, params, pipe_wm);
 
-	if (!memcmp(&intel_crtc->wm.skl_active, pipe_wm, sizeof(*pipe_wm)))
+	if (!memcmp(&cstate->wm.target.skl, pipe_wm, sizeof(*pipe_wm)))
 		return false;
 
-	intel_crtc->wm.skl_active = *pipe_wm;
+	cstate->wm.target.skl = *pipe_wm;
 
 	return true;
 }
@@ -3505,15 +3510,15 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 static void ilk_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct intel_pipe_wm pipe_wm = {};
 
 	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
 
-	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
+	if (!memcmp(&cstate->wm.target.ilk, &pipe_wm, sizeof(pipe_wm)))
 		return;
 
-	intel_crtc->wm.active = pipe_wm;
+	cstate->wm.target.ilk = pipe_wm;
 
 	ilk_program_watermarks(dev_priv);
 }
@@ -3590,7 +3595,8 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct skl_pipe_wm *active = &intel_crtc->wm.skl_active;
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct skl_pipe_wm *active = &cstate->wm.target.skl;
 	enum pipe pipe = intel_crtc->pipe;
 	int level, i, max_level;
 	uint32_t temp;
@@ -3653,7 +3659,8 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct ilk_wm_values *hw = &dev_priv->wm.hw;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_pipe_wm *active = &intel_crtc->wm.active;
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct intel_pipe_wm *active = &cstate->wm.target.ilk;
 	enum pipe pipe = intel_crtc->pipe;
 	static const unsigned int wm0_pipe_reg[] = {
 		[PIPE_A] = WM0_PIPEA_ILK,
-- 
1.8.5.1

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

[RFC 11/15] drm/i915: Calculate pipe watermark values during atomic check

[Matt Roper]

Calculate pipe watermarks during atomic calculation phase, based on the
contents of the atomic transaction's state structure.

FIXME:  For now we just dump this in a temporary structure and don't do
anything with it except compare it to the values we re-calculate later.
Ultimately we should squash the next patch into this one once we're sure
this is really working as expected.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 +++
 drivers/gpu/drm/i915/intel_atomic.c  | 29 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  1 +
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_pm.c      |  6 +++++-
 5 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index df5673f..214ce76 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -562,6 +562,9 @@ struct drm_i915_display_funcs {
 			  int target, int refclk,
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
+	int (*compute_pipe_wm)(struct drm_crtc *crtc,
+			       struct drm_atomic_state *state,
+			       void *target);
 	void (*update_wm)(struct drm_crtc *crtc);
 	void (*update_sprite_wm)(struct drm_plane *plane,
 				 struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 7ed8033..db349dd 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -176,8 +176,13 @@ int intel_atomic_commit(struct drm_device *dev,
 			continue;
 		}
 
-		to_intel_crtc(crtc)->config->scaler_state =
-			to_intel_crtc_state(state->crtc_states[i])->scaler_state;
+#define STATE_SWAP(x) to_intel_crtc(crtc)->config->x = \
+	to_intel_crtc_state(state->crtc_states[i])->x;
+
+		STATE_SWAP(scaler_state);
+		STATE_SWAP(wm);
+
+#undef STATE_SWAP
 
 		if (INTEL_INFO(dev)->gen >= 9)
 			skl_detach_scalers(to_intel_crtc(crtc));
@@ -421,3 +426,23 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 	return 0;
 }
+
+/*
+ * intel_check_crtc: Check CRTC state
+ */
+int
+intel_check_crtc(struct drm_crtc *crtc,
+		 struct drm_crtc_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
+	int ret;
+
+	if (!dev_priv->display.compute_pipe_wm)
+		return 0;
+
+	ret = dev_priv->display.compute_pipe_wm(crtc, state->state,
+						&intel_state->wm.tmp);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f4398a7..76affba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11096,6 +11096,7 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.load_lut = intel_crtc_load_lut,
 	.atomic_begin = intel_begin_crtc_commit,
 	.atomic_flush = intel_finish_crtc_commit,
+	.atomic_check = intel_check_crtc,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4ae109c..6ca3c06 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -464,6 +464,9 @@ struct intel_crtc_state {
 	struct intel_crtc_scaler_state scaler_state;
 
 	struct {
+		/* tmp wm */
+		struct intel_pipe_wm tmp;
+
 		/* final, target watermarks for state */
 		union {
 			struct intel_pipe_wm ilk;
@@ -1424,6 +1427,8 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
+int intel_check_crtc(struct drm_crtc *crtc,
+		     struct drm_crtc_state *state);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 789b4fe..017b184 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2074,8 +2074,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 /* Compute new watermarks for the pipe */
 static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
 			       struct drm_atomic_state *state,
-			       struct intel_pipe_wm *pipe_wm)
+			       void *target)
 {
+	struct intel_pipe_wm *pipe_wm = target;
 	struct drm_device *dev = crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -3514,6 +3515,8 @@ static void ilk_update_wm(struct drm_crtc *crtc)
 	struct intel_pipe_wm pipe_wm = {};
 
 	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
+	WARN(memcmp(&cstate->wm.tmp, &pipe_wm, sizeof(pipe_wm)) != 0,
+	     "Pre-computed atomic watermark values do not match final values!");
 
 	if (!memcmp(&cstate->wm.target.ilk, &pipe_wm, sizeof(pipe_wm)))
 		return;
@@ -6655,6 +6658,7 @@ void intel_init_pm(struct drm_device *dev)
 		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
+			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
1.8.5.1

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

[RFC 12/15] drm/i915: Actually use pre-computer watermark values (!!SQUASHME)

[Matt Roper]

Rather that just comparing and then throwing away the pipe watermark
values we precomputed at atomic check time, actually use those values
when we program watermarks.

FIXME:  This should be squashed into the previous patch eventually, once
we're convinced that pre-computed watermarks get the proper values in
all cases.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  3 +-
 drivers/gpu/drm/i915/intel_atomic.c |  4 +-
 drivers/gpu/drm/i915/intel_drv.h    |  3 --
 drivers/gpu/drm/i915/intel_pm.c     | 77 +++++++++++--------------------------
 4 files changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 214ce76..d577eba 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -563,8 +563,7 @@ struct drm_i915_display_funcs {
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct drm_crtc *crtc,
-			       struct drm_atomic_state *state,
-			       void *target);
+			       struct drm_atomic_state *state);
 	void (*update_wm)(struct drm_crtc *crtc);
 	void (*update_sprite_wm)(struct drm_plane *plane,
 				 struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index db349dd..5294840 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -435,14 +435,12 @@ intel_check_crtc(struct drm_crtc *crtc,
 		 struct drm_crtc_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
-	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
 	int ret;
 
 	if (!dev_priv->display.compute_pipe_wm)
 		return 0;
 
-	ret = dev_priv->display.compute_pipe_wm(crtc, state->state,
-						&intel_state->wm.tmp);
+	ret = dev_priv->display.compute_pipe_wm(crtc, state->state);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6ca3c06..627741a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -464,9 +464,6 @@ struct intel_crtc_state {
 	struct intel_crtc_scaler_state scaler_state;
 
 	struct {
-		/* tmp wm */
-		struct intel_pipe_wm tmp;
-
 		/* final, target watermarks for state */
 		union {
 			struct intel_pipe_wm ilk;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 017b184..27337fe 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2073,10 +2073,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 
 /* Compute new watermarks for the pipe */
 static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
-			       struct drm_atomic_state *state,
-			       void *target)
+			       struct drm_atomic_state *state)
 {
-	struct intel_pipe_wm *pipe_wm = target;
+	struct intel_pipe_wm *pipe_wm;
 	struct drm_device *dev = crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -2094,47 +2093,26 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
 	};
 	struct ilk_wm_maximums max;
 
-	/*
-	 * FIXME: In an upcoming patch, we'll *only* be calling this from the
-	 * atomic 'check' codepath and thus will always have a top-level atomic
-	 * transaction.  However at the moment we still call this in the legacy
-	 * 'ilk_update_wm' function, which means we need to be able to also
-	 * operate on already-committed state which has been detached from any
-	 * top-level transaction.
-	 *
-	 * Drop the 'else' block here once we only do atomic-style watermarks.
-	 */
-	if (state) {
-		cs = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(cs))
-			return PTR_ERR(cs);
-		else
-			cstate = to_intel_crtc_state(cs);
-
-		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
-			ps = drm_atomic_get_plane_state(state,
-							&intel_plane->base);
-			if (IS_ERR(ps))
-				return PTR_ERR(ps);
-
-			if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
-				pristate = to_intel_plane_state(ps);
-			else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
-				sprstate = to_intel_plane_state(ps);
-			else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
-				curstate = to_intel_plane_state(ps);
-		}
-	} else {
-		/* Use already-committed state */
-		cstate = to_intel_crtc_state(crtc->state);
-		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
-			if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
-				sprstate = to_intel_plane_state(intel_plane->base.state);
-				break;
-			}
-		}
-		pristate = to_intel_plane_state(crtc->primary->state);
-		curstate = to_intel_plane_state(crtc->cursor->state);
+	cs = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+	else
+		cstate = to_intel_crtc_state(cs);
+
+	pipe_wm = &cstate->wm.target.ilk;
+
+	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
+		ps = drm_atomic_get_plane_state(state,
+						&intel_plane->base);
+		if (IS_ERR(ps))
+			return PTR_ERR(ps);
+
+		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+			pristate = to_intel_plane_state(ps);
+		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
+			sprstate = to_intel_plane_state(ps);
+		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			curstate = to_intel_plane_state(ps);
 	}
 
 	config.sprites_enabled = sprstate->visible;
@@ -3511,17 +3489,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 static void ilk_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct intel_pipe_wm pipe_wm = {};
-
-	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
-	WARN(memcmp(&cstate->wm.tmp, &pipe_wm, sizeof(pipe_wm)) != 0,
-	     "Pre-computed atomic watermark values do not match final values!");
-
-	if (!memcmp(&cstate->wm.target.ilk, &pipe_wm, sizeof(pipe_wm)))
-		return;
-
-	cstate->wm.target.ilk = pipe_wm;
 
 	ilk_program_watermarks(dev_priv);
 }
-- 
1.8.5.1

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

[RFC 13/15] drm/i915: Introduce intel_schedule_vblank_job()

[Matt Roper]

Various places in the driver need the ability to schedule actions to run
on a future vblank (updating watermarks, unpinning buffers, etc.).  The
long term goal is to add such a mechanism in the DRM core that can be
shared by all drivers.  Paulo Zanoni has already written some
preliminary patches to support this, but his work will probably take
some time to polish and get merged since it needs to untangle the DRM
core's vblank infrastructure.  This patch is partially based on Paulo's
work, but takes a simpler approach and just adds an i915-specific
mechanism that can be used in the interim since we have an immediate
need for watermark updates.

Inspired-by: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 117 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   8 +++
 drivers/gpu/drm/i915/intel_drv.h     |  27 ++++++++
 3 files changed, 152 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 557af88..fd5a795 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1649,8 +1649,37 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void process_vblank_jobs(struct drm_device *dev, enum pipe pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct i915_vblank_job *job, *tmp;
+	u32 seq;
+
+	seq = drm_vblank_count(dev, pipe);
+	list_for_each_entry_safe(job, tmp, &intel_crtc->vblank_jobs, link) {
+		if (seq - job->seq > (1<<23))
+			continue;
+
+		list_del(&job->link);
+		drm_vblank_put(dev, pipe);
+
+		if (job->wq) {
+			job->fired_seq = seq;
+			queue_work(job->wq, &job->work);
+		} else {
+			job->callback(intel_crtc, job->callback_data,
+				      false, seq);
+			kfree(job);
+		}
+	}
+}
+
 static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
 {
+	process_vblank_jobs(dev, pipe);
+
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
@@ -4511,3 +4540,91 @@ void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv)
 	dev_priv->dev->driver->irq_preinstall(dev_priv->dev);
 	dev_priv->dev->driver->irq_postinstall(dev_priv->dev);
 }
+
+static void vblank_job_handler(struct work_struct *work)
+{
+	struct i915_vblank_job *job =
+		container_of(work, struct i915_vblank_job, work);
+
+	job->callback(job->crtc, job->callback_data,
+		      job->seq != job->fired_seq, job->fired_seq);
+	kfree(job);
+}
+
+/**
+ * intel_schedule_vblank_job - schedule work to happen on specified vblank
+ * @crtc: CRTC whose vblank should trigger the work
+ * @callback: Code to run on vblank
+ * @callback_data: Data to pass to callback function
+ * @wq: Workqueue to run job on (or NULL to run from interrupt context)
+ * @seq: vblank count relative to current to schedule for
+ *
+ * Schedule code that should be run after the specified vblank fires.  The code
+ * can either run directly in the interrupt context or on a specified
+ * workqueue.
+ */
+int intel_schedule_vblank_job(struct intel_crtc *crtc,
+			      i915_vblank_callback_t callback,
+			      void *callback_data,
+			      struct workqueue_struct *wq,
+			      u32 seq)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct i915_vblank_job *job;
+	unsigned long irqflags;
+	int ret;
+
+	job = kzalloc(sizeof(*job), GFP_ATOMIC);
+	if (!job)
+		return -ENOMEM;
+
+	job->wq = wq;
+	job->crtc = crtc;
+	job->callback = callback;
+	job->callback_data = callback_data;
+	if (job->wq)
+		INIT_WORK(&job->work, vblank_job_handler);
+
+	ret = drm_crtc_vblank_get(&crtc->base);
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to enable interrupts, ret=%d\n", ret);
+		kfree(job);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&dev->event_lock, irqflags);
+	job->seq = seq + drm_vblank_count(dev, crtc->pipe);
+	list_add_tail(&job->link, &crtc->vblank_jobs);
+	spin_unlock_irqrestore(&dev->event_lock, irqflags);
+
+	return 0;
+}
+
+/**
+ * intel_trigger_all_vblank_jobs - immediately trigger vblank jobs for a crtc
+ * @crtc: CRTC to trigger all outstanding jobs for
+ *
+ * Immediately triggers all of the jobs awaiting future vblanks on a CRTC.
+ * This will be called when a CRTC is disabled (because there may never be
+ * another vblank event).  The job callbacks will receive 'true' for the
+ * early parameter, as well as the current vblank count.
+ */
+void trigger_all_vblank_jobs(struct intel_crtc *crtc)
+{
+	struct i915_vblank_job *job, *tmp;
+	u32 seq;
+
+	seq = drm_vblank_count(crtc->base.dev, crtc->pipe);
+	list_for_each_entry_safe(job, tmp, &crtc->vblank_jobs, link) {
+		list_del(&job->link);
+		drm_vblank_put(crtc->base.dev, crtc->pipe);
+
+		if (job->wq) {
+			job->fired_seq = seq;
+			queue_work(job->wq, &job->work);
+		} else {
+			job->callback(crtc, job->callback_data, true, seq);
+			kfree(job);
+		}
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 76affba..9b56d07 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5099,6 +5099,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
+	trigger_all_vblank_jobs(intel_crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
@@ -5161,6 +5163,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
+	trigger_all_vblank_jobs(intel_crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
@@ -6004,6 +6008,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
+	trigger_all_vblank_jobs(intel_crtc);
+
 	/*
 	 * On gen2 planes are double buffered but the pipe isn't, so we must
 	 * wait for planes to fully turn off before disabling the pipe.
@@ -13649,6 +13655,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
+	INIT_LIST_HEAD(&intel_crtc->vblank_jobs);
+
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
 	return;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 627741a..630e7c1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -504,6 +504,24 @@ struct intel_crtc_atomic_commit {
 	unsigned update_sprite_watermarks;
 };
 
+typedef void (*i915_vblank_callback_t)(struct intel_crtc *crtc,
+				       void *data,
+				       bool early,
+				       u32 fired_seq);
+
+/* Task to run (or schedule on a workqueue) on a specific vblank */
+struct i915_vblank_job {
+	u32 seq;
+	u32 fired_seq;                     /* early if crtc gets disabled */
+	struct intel_crtc *crtc;
+	struct workqueue_struct *wq;       /* NULL = run from interrupt */
+	i915_vblank_callback_t callback;
+	void *callback_data;
+
+	struct list_head link;
+	struct work_struct work;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -550,6 +568,9 @@ struct intel_crtc {
 
 	/* scalers available on this crtc */
 	int num_scalers;
+
+	/* Jobs to run/schedule on vblank */
+	struct list_head vblank_jobs;
 };
 
 struct intel_plane_wm_parameters {
@@ -913,6 +934,12 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
 				     unsigned int pipe_mask);
+int intel_schedule_vblank_job(struct intel_crtc *crtc,
+			      i915_vblank_callback_t callback,
+			      void *callback_data,
+			      struct workqueue_struct *wq,
+			      u32 seq);
+void trigger_all_vblank_jobs(struct intel_crtc *crtc);
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
-- 
1.8.5.1

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

[RFC 14/15] drm/i915: Program atomic watermarks via vblank job

[Matt Roper]

Use the new vblank job infrastructure to schedule watermark programming
to happen when the next vblank occurs.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  7 +++++++
 drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_pm.c      |  1 +
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d577eba..5134101 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -569,6 +569,7 @@ struct drm_i915_display_funcs {
 				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, uint32_t sprite_height,
 				 int pixel_size, bool enable, bool scaled);
+	void (*program_watermarks)(struct drm_i915_private *dev_priv);
 	void (*modeset_global_resources)(struct drm_atomic_state *state);
 	/* Returns the active state of the crtc, and if the crtc is active,
 	 * fills out the pipe-config with the hw state. */
@@ -2494,6 +2495,12 @@ struct drm_i915_cmd_table {
 #define HAS_L3_DPF(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
 
+/*
+ * FIXME:  Only some platforms have been transitioned to atomic watermark
+ * updates so far.
+ */
+#define HAS_ATOMIC_WM(dev_priv) (dev_priv->display.program_watermarks != NULL)
+
 #define GT_FREQUENCY_MULTIPLIER 50
 #define GEN9_FREQ_SCALER 3
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b56d07..b2012c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13209,6 +13209,24 @@ intel_disable_primary_plane(struct drm_plane *plane,
 	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
 }
 
+static void
+do_update_watermarks(struct intel_crtc *intel_crtc,
+		     void *data,
+		     bool early,
+		     u32 seq)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+
+	/*
+	 * If we fired early because the CRTC is turning off, there's no
+	 * need to actually program watermarks.
+	 */
+	if (early)
+		return;
+
+	dev_priv->display.program_watermarks(dev_priv);
+}
+
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -13251,7 +13269,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
 
-	if (intel_crtc->atomic.update_wm)
+	if (!HAS_ATOMIC_WM(dev_priv) && intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
 
 	intel_runtime_pm_get(dev_priv);
@@ -13270,6 +13288,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *p;
 
+	/*
+	 * If this platform supports atomic watermarks, schedule a job to
+	 * update watermarks when the next vblank occurs.  Otherwise, just
+	 * update watermarks the old-fashioned way.
+	 */
+	if (HAS_ATOMIC_WM(dev_priv))
+		intel_schedule_vblank_job(intel_crtc, do_update_watermarks,
+					  NULL, dev_priv->wq, 1);
+
 	if (intel_crtc->atomic.evade)
 		intel_pipe_update_end(intel_crtc,
 				      intel_crtc->atomic.start_vbl_count);
@@ -13290,10 +13317,11 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);
 
-	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
-		if (intel_crtc->atomic.update_sprite_watermarks &
-		    (1 << drm_plane_index(p)))
-			intel_update_sprite_watermarks(p, crtc);
+	if (!HAS_ATOMIC_WM(dev_priv))
+		drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
+			if (intel_crtc->atomic.update_sprite_watermarks &
+			    (1 << drm_plane_index(p)))
+				intel_update_sprite_watermarks(p, crtc);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27337fe..7f0a0c1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6626,6 +6626,7 @@ void intel_init_pm(struct drm_device *dev)
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
+			dev_priv->display.program_watermarks = ilk_program_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
1.8.5.1

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

[RFC 15/15] drm/i915: Add intermediate watermarks

[Matt Roper]

From: Matt Roper <matt@mattrope.com>

In addition to calculating final watermarks, let's also pre-calculate a
set of 'intermediate' watermark values.  These intermediate watermarks
merge the watermarks of the old state and the new state such that they
will satisfy the requirements of both states.  This means they can be
programmed immediately (without waiting for a vblank).  Once the vblank
does happen, we can then re-program watermarks to the more optimal
target value.

Note that we add a new 'pending' set of watermark values that the
intermediate values are originally written into.  The watermark
programming routine is updated to use these 'pending' values.  When a
vblank happens, the final 'target' values are copied to the 'pending'
structure so that all subsequent operations will make use of those
values.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/intel_atomic.c  | 15 +++++++-
 drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     | 16 ++++++--
 drivers/gpu/drm/i915/intel_pm.c      | 72 +++++++++++++++++++++++++++++++-----
 5 files changed, 126 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5134101..f5feffc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -564,6 +564,9 @@ struct drm_i915_display_funcs {
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct drm_crtc *crtc,
 			       struct drm_atomic_state *state);
+	void (*compute_intermediate_wm)(struct drm_device *dev,
+					struct intel_crtc_state *newstate,
+					const struct intel_crtc_state *oldstate);
 	void (*update_wm)(struct drm_crtc *crtc);
 	void (*update_sprite_wm)(struct drm_plane *plane,
 				 struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 5294840..92c29b5 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -435,12 +435,25 @@ intel_check_crtc(struct drm_crtc *crtc,
 		 struct drm_crtc_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc_state *new_state = to_intel_crtc_state(state);
+	struct intel_crtc_state *old_state = to_intel_crtc_state(crtc->state);
 	int ret;
 
 	if (!dev_priv->display.compute_pipe_wm)
 		return 0;
+	if (WARN_ON(!dev_priv->display.compute_intermediate_wm))
+		return 0;
 
 	ret = dev_priv->display.compute_pipe_wm(crtc, state->state);
+	if (ret)
+		return ret;
 
-	return ret;
+	/*
+	 * Calculate 'intermediate' watermarks that satisfy both the old state
+	 * and the new state.  We can program these immediately.
+	 */
+	dev_priv->display.compute_intermediate_wm(crtc->dev, new_state,
+						  old_state);
+
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2012c9..45fdbec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13227,6 +13227,17 @@ do_update_watermarks(struct intel_crtc *intel_crtc,
 	dev_priv->display.program_watermarks(dev_priv);
 }
 
+static void
+do_latch_watermarks(struct intel_crtc *crtc,
+		    void *data,
+		    bool early,
+		    u32 seq)
+{
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
+
+	cstate->wm.pending = cstate->wm.target;
+}
+
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -13269,8 +13280,21 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
 
-	if (!HAS_ATOMIC_WM(dev_priv) && intel_crtc->atomic.update_wm)
+	/*
+	 * For platforms that support atomic watermarks, program the
+	 * intermediate watermarks immediately.  When vblank happens, 'pending'
+	 * values will be set to the final 'target' values and we'll do this
+	 * again to get the optimal watermarks.
+	 *
+	 * If a platform hasn't been transitioned to atomic watermarks yet,
+	 * we'll continue to update watermarks the old way, if flags tell
+	 * us to.
+	 */
+	if (HAS_ATOMIC_WM(dev_priv)) {
+		dev_priv->display.program_watermarks(dev_priv);
+	} else if (intel_crtc->atomic.update_wm) {
 		intel_update_watermarks(crtc);
+	}
 
 	intel_runtime_pm_get(dev_priv);
 
@@ -13291,11 +13315,19 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	/*
 	 * If this platform supports atomic watermarks, schedule a job to
 	 * update watermarks when the next vblank occurs.  Otherwise, just
-	 * update watermarks the old-fashioned way.
+	 * update watermarks the old-fashioned way.  Also, schedule a second
+	 * task that runs directly in the interrupt handler (rather than
+	 * in a wq task) that copies the 'target' watermark values to the
+	 * 'pending' watermark structure so that the proper values will be
+	 * used for all subsequent calculations.
+	 *
 	 */
-	if (HAS_ATOMIC_WM(dev_priv))
+	if (HAS_ATOMIC_WM(dev_priv)) {
 		intel_schedule_vblank_job(intel_crtc, do_update_watermarks,
 					  NULL, dev_priv->wq, 1);
+		intel_schedule_vblank_job(intel_crtc, do_latch_watermarks,
+					  NULL, NULL, 1);
+	}
 
 	if (intel_crtc->atomic.evade)
 		intel_pipe_update_end(intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 630e7c1..3f455ce 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -334,6 +334,11 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
+union pipe_wm {
+	struct intel_pipe_wm ilk;
+	struct skl_pipe_wm skl;
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -465,10 +470,13 @@ struct intel_crtc_state {
 
 	struct {
 		/* final, target watermarks for state */
-		union {
-			struct intel_pipe_wm ilk;
-			struct skl_pipe_wm skl;
-		} target;
+		union pipe_wm target;
+
+		/*
+		 * pending watermark (intermediate before vblank, target after
+		 * vblank fires)
+		 */
+		union pipe_wm pending;
 	} wm;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7f0a0c1..887d39a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2060,7 +2060,7 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 	for_each_intel_crtc(dev, intel_crtc) {
 		const struct intel_crtc_state *cstate =
 			to_intel_crtc_state(intel_crtc->base.state);
-		const struct intel_pipe_wm *wm = &cstate->wm.target.ilk;
+		const struct intel_pipe_wm *wm = &cstate->wm.pending.ilk;
 
 		if (!wm->pipe_enabled)
 			continue;
@@ -2071,6 +2071,24 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 	}
 }
 
+static bool ilk_validate_pipe_wm(struct drm_device *dev,
+				 struct intel_wm_config *config,
+				 struct intel_pipe_wm *pipe_wm)
+{
+	struct ilk_wm_maximums max;
+
+	/* LP0 watermarks always use 1/2 DDB partitioning */
+	ilk_compute_wm_maximums(dev, 0, config, INTEL_DDB_PART_1_2, &max);
+
+	/* At least LP0 must be valid */
+	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) {
+		DRM_DEBUG_KMS("LP0 watermark invalid\n");
+		return false;
+	}
+
+	return true;
+}
+
 /* Compute new watermarks for the pipe */
 static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
 			       struct drm_atomic_state *state)
@@ -2138,12 +2156,8 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
 
-	/* LP0 watermarks always use 1/2 DDB partitioning */
-	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
-
-	/* At least LP0 must be valid */
-	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
-		return -EINVAL;
+	if (!ilk_validate_pipe_wm(dev, &config, pipe_wm))
+		return false;
 
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
@@ -2168,6 +2182,40 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
 }
 
 /*
+ * Build a set of 'intermediate' watermark values that satisfy both the old
+ * state and the new state.  These can be programmed to the hardware
+ * immediately.
+ */
+void ilk_compute_intermediate_wm(struct drm_device *dev,
+				 struct intel_crtc_state *newstate,
+				 const struct intel_crtc_state *oldstate)
+{
+	struct intel_pipe_wm *a = &newstate->wm.pending.ilk;
+	const struct intel_pipe_wm *b = &oldstate->wm.pending.ilk;
+	int level, max_level = ilk_wm_max_level(dev);
+
+	/*
+	 * Start with the final, target watermarks, then combine with the
+	 * current state's watermarks.
+	 */
+	*a = newstate->wm.target.ilk;
+	a->pipe_enabled |= b->pipe_enabled;
+	a->sprites_enabled |= b->sprites_enabled;
+	a->sprites_scaled |= b->sprites_scaled;
+
+	for (level = 0; level <= max_level; level++) {
+		struct intel_wm_level *a_wm = &a->wm[level];
+		const struct intel_wm_level *b_wm = &b->wm[level];
+
+		a_wm->enable &= b_wm->enable;
+		a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
+		a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
+		a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
+		a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
+	}
+}
+
+/*
  * Merge the watermarks from all active pipes for a specific level.
  */
 static void ilk_merge_wm_level(struct drm_device *dev,
@@ -2181,7 +2229,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
 	for_each_intel_crtc(dev, intel_crtc) {
 		const struct intel_crtc_state *cstate =
 			to_intel_crtc_state(intel_crtc->base.state);
-		const struct intel_pipe_wm *active = &cstate->wm.target.ilk;
+		const struct intel_pipe_wm *active = &cstate->wm.pending.ilk;
 		const struct intel_wm_level *wm = &active->wm[level];
 
 		if (!active->pipe_enabled)
@@ -2330,12 +2378,12 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 		const struct intel_crtc_state *cstate =
 			to_intel_crtc_state(intel_crtc->base.state);
 		enum pipe pipe = intel_crtc->pipe;
-		const struct intel_wm_level *r = &cstate->wm.target.ilk.wm[0];
+		const struct intel_wm_level *r = &cstate->wm.pending.ilk.wm[0];
 
 		if (WARN_ON(!r->enable))
 			continue;
 
-		results->wm_linetime[pipe] = cstate->wm.target.ilk.linetime;
+		results->wm_linetime[pipe] = cstate->wm.pending.ilk.linetime;
 
 		results->wm_pipe[pipe] =
 			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
@@ -3669,6 +3717,8 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 		for (level = 0; level <= max_level; level++)
 			active->wm[level].enable = true;
 	}
+
+	cstate->wm.pending.ilk = *active;
 }
 
 void ilk_wm_get_hw_state(struct drm_device *dev)
@@ -6626,6 +6676,8 @@ void intel_init_pm(struct drm_device *dev)
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
+			dev_priv->display.compute_intermediate_wm =
+				ilk_compute_intermediate_wm;
 			dev_priv->display.program_watermarks = ilk_program_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
-- 
1.8.5.1

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

Re: [RFC 12/15] drm/i915: Actually use pre-computer watermark values (!!SQUASHME)

[Ville Syrjälä]

On Wed, May 20, 2015 at 07:12:24PM -0700, Matt Roper wrote:
> Rather that just comparing and then throwing away the pipe watermark
> values we precomputed at atomic check time, actually use those values
> when we program watermarks.
> 
> FIXME:  This should be squashed into the previous patch eventually, once
> we're convinced that pre-computed watermarks get the proper values in
> all cases.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +-
>  drivers/gpu/drm/i915/intel_atomic.c |  4 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  3 --
>  drivers/gpu/drm/i915/intel_pm.c     | 77 +++++++++++--------------------------
>  4 files changed, 24 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 214ce76..d577eba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -563,8 +563,7 @@ struct drm_i915_display_funcs {
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct drm_crtc *crtc,
> -			       struct drm_atomic_state *state,
> -			       void *target);
> +			       struct drm_atomic_state *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	void (*update_sprite_wm)(struct drm_plane *plane,
>  				 struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index db349dd..5294840 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -435,14 +435,12 @@ intel_check_crtc(struct drm_crtc *crtc,
>  		 struct drm_crtc_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> -	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>  	int ret;
>  
>  	if (!dev_priv->display.compute_pipe_wm)
>  		return 0;
>  
> -	ret = dev_priv->display.compute_pipe_wm(crtc, state->state,
> -						&intel_state->wm.tmp);
> +	ret = dev_priv->display.compute_pipe_wm(crtc, state->state);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6ca3c06..627741a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -464,9 +464,6 @@ struct intel_crtc_state {
>  	struct intel_crtc_scaler_state scaler_state;
>  
>  	struct {
> -		/* tmp wm */
> -		struct intel_pipe_wm tmp;
> -
>  		/* final, target watermarks for state */
>  		union {
>  			struct intel_pipe_wm ilk;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 017b184..27337fe 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2073,10 +2073,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
>  
>  /* Compute new watermarks for the pipe */
>  static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> -			       struct drm_atomic_state *state,
> -			       void *target)
> +			       struct drm_atomic_state *state)
>  {
> -	struct intel_pipe_wm *pipe_wm = target;
> +	struct intel_pipe_wm *pipe_wm;
>  	struct drm_device *dev = crtc->dev;
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -2094,47 +2093,26 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
>  	};
>  	struct ilk_wm_maximums max;
>  
> -	/*
> -	 * FIXME: In an upcoming patch, we'll *only* be calling this from the
> -	 * atomic 'check' codepath and thus will always have a top-level atomic
> -	 * transaction.  However at the moment we still call this in the legacy
> -	 * 'ilk_update_wm' function, which means we need to be able to also
> -	 * operate on already-committed state which has been detached from any
> -	 * top-level transaction.
> -	 *
> -	 * Drop the 'else' block here once we only do atomic-style watermarks.
> -	 */
> -	if (state) {
> -		cs = drm_atomic_get_crtc_state(state, crtc);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -		else
> -			cstate = to_intel_crtc_state(cs);
> -
> -		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> -			ps = drm_atomic_get_plane_state(state,
> -							&intel_plane->base);
> -			if (IS_ERR(ps))
> -				return PTR_ERR(ps);
> -
> -			if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -				pristate = to_intel_plane_state(ps);
> -			else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> -				sprstate = to_intel_plane_state(ps);
> -			else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> -				curstate = to_intel_plane_state(ps);
> -		}
> -	} else {
> -		/* Use already-committed state */
> -		cstate = to_intel_crtc_state(crtc->state);
> -		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> -			if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
> -				sprstate = to_intel_plane_state(intel_plane->base.state);
> -				break;
> -			}
> -		}
> -		pristate = to_intel_plane_state(crtc->primary->state);
> -		curstate = to_intel_plane_state(crtc->cursor->state);
> +	cs = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +	else
> +		cstate = to_intel_crtc_state(cs);
> +
> +	pipe_wm = &cstate->wm.target.ilk;
> +
> +	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> +		ps = drm_atomic_get_plane_state(state,
> +						&intel_plane->base);
> +		if (IS_ERR(ps))
> +			return PTR_ERR(ps);
> +
> +		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			pristate = to_intel_plane_state(ps);
> +		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> +			sprstate = to_intel_plane_state(ps);
> +		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			curstate = to_intel_plane_state(ps);
>  	}
>  
>  	config.sprites_enabled = sprstate->visible;
> @@ -3511,17 +3489,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  static void ilk_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct intel_pipe_wm pipe_wm = {};
> -
> -	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
> -	WARN(memcmp(&cstate->wm.tmp, &pipe_wm, sizeof(pipe_wm)) != 0,
> -	     "Pre-computed atomic watermark values do not match final values!");
> -
> -	if (!memcmp(&cstate->wm.target.ilk, &pipe_wm, sizeof(pipe_wm)))
> -		return;

The memcmp() is there to avoid reprogromaming needlessly. It should
stay.

> -
> -	cstate->wm.target.ilk = pipe_wm;
>  
>  	ilk_program_watermarks(dev_priv);
>  }
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFC 13/15] drm/i915: Introduce intel_schedule_vblank_job()

[Ville Syrjälä]

On Wed, May 20, 2015 at 07:12:25PM -0700, Matt Roper wrote:
> Various places in the driver need the ability to schedule actions to run
> on a future vblank (updating watermarks, unpinning buffers, etc.).  The
> long term goal is to add such a mechanism in the DRM core that can be
> shared by all drivers.  Paulo Zanoni has already written some
> preliminary patches to support this, but his work will probably take
> some time to polish and get merged since it needs to untangle the DRM
> core's vblank infrastructure.  This patch is partially based on Paulo's
> work, but takes a simpler approach and just adds an i915-specific
> mechanism that can be used in the interim since we have an immediate
> need for watermark updates.
> 
> Inspired-by: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I like what I had better. No needless kmallocs (this IMO is a must
requirement for any solution), no needless workqueues, uses the hw
vblank counter and absolute values so isn't racy.

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 117 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   8 +++
>  drivers/gpu/drm/i915/intel_drv.h     |  27 ++++++++
>  3 files changed, 152 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 557af88..fd5a795 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1649,8 +1649,37 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> +static void process_vblank_jobs(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct i915_vblank_job *job, *tmp;
> +	u32 seq;
> +
> +	seq = drm_vblank_count(dev, pipe);
> +	list_for_each_entry_safe(job, tmp, &intel_crtc->vblank_jobs, link) {
> +		if (seq - job->seq > (1<<23))
> +			continue;
> +
> +		list_del(&job->link);
> +		drm_vblank_put(dev, pipe);
> +
> +		if (job->wq) {
> +			job->fired_seq = seq;
> +			queue_work(job->wq, &job->work);
> +		} else {
> +			job->callback(intel_crtc, job->callback_data,
> +				      false, seq);
> +			kfree(job);
> +		}
> +	}
> +}
> +
>  static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
>  {
> +	process_vblank_jobs(dev, pipe);
> +
>  	if (!drm_handle_vblank(dev, pipe))
>  		return false;
>  
> @@ -4511,3 +4540,91 @@ void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv)
>  	dev_priv->dev->driver->irq_preinstall(dev_priv->dev);
>  	dev_priv->dev->driver->irq_postinstall(dev_priv->dev);
>  }
> +
> +static void vblank_job_handler(struct work_struct *work)
> +{
> +	struct i915_vblank_job *job =
> +		container_of(work, struct i915_vblank_job, work);
> +
> +	job->callback(job->crtc, job->callback_data,
> +		      job->seq != job->fired_seq, job->fired_seq);
> +	kfree(job);
> +}
> +
> +/**
> + * intel_schedule_vblank_job - schedule work to happen on specified vblank
> + * @crtc: CRTC whose vblank should trigger the work
> + * @callback: Code to run on vblank
> + * @callback_data: Data to pass to callback function
> + * @wq: Workqueue to run job on (or NULL to run from interrupt context)
> + * @seq: vblank count relative to current to schedule for
> + *
> + * Schedule code that should be run after the specified vblank fires.  The code
> + * can either run directly in the interrupt context or on a specified
> + * workqueue.
> + */
> +int intel_schedule_vblank_job(struct intel_crtc *crtc,
> +			      i915_vblank_callback_t callback,
> +			      void *callback_data,
> +			      struct workqueue_struct *wq,
> +			      u32 seq)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct i915_vblank_job *job;
> +	unsigned long irqflags;
> +	int ret;
> +
> +	job = kzalloc(sizeof(*job), GFP_ATOMIC);
> +	if (!job)
> +		return -ENOMEM;
> +
> +	job->wq = wq;
> +	job->crtc = crtc;
> +	job->callback = callback;
> +	job->callback_data = callback_data;
> +	if (job->wq)
> +		INIT_WORK(&job->work, vblank_job_handler);
> +
> +	ret = drm_crtc_vblank_get(&crtc->base);
> +	if (ret) {
> +		DRM_DEBUG_KMS("Failed to enable interrupts, ret=%d\n", ret);
> +		kfree(job);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dev->event_lock, irqflags);
> +	job->seq = seq + drm_vblank_count(dev, crtc->pipe);
> +	list_add_tail(&job->link, &crtc->vblank_jobs);
> +	spin_unlock_irqrestore(&dev->event_lock, irqflags);
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_trigger_all_vblank_jobs - immediately trigger vblank jobs for a crtc
> + * @crtc: CRTC to trigger all outstanding jobs for
> + *
> + * Immediately triggers all of the jobs awaiting future vblanks on a CRTC.
> + * This will be called when a CRTC is disabled (because there may never be
> + * another vblank event).  The job callbacks will receive 'true' for the
> + * early parameter, as well as the current vblank count.
> + */
> +void trigger_all_vblank_jobs(struct intel_crtc *crtc)
> +{
> +	struct i915_vblank_job *job, *tmp;
> +	u32 seq;
> +
> +	seq = drm_vblank_count(crtc->base.dev, crtc->pipe);
> +	list_for_each_entry_safe(job, tmp, &crtc->vblank_jobs, link) {
> +		list_del(&job->link);
> +		drm_vblank_put(crtc->base.dev, crtc->pipe);
> +
> +		if (job->wq) {
> +			job->fired_seq = seq;
> +			queue_work(job->wq, &job->work);
> +		} else {
> +			job->callback(crtc, job->callback_data, true, seq);
> +			kfree(job);
> +		}
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 76affba..9b56d07 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5099,6 +5099,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> +	trigger_all_vblank_jobs(intel_crtc);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> @@ -5161,6 +5163,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> +	trigger_all_vblank_jobs(intel_crtc);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		intel_opregion_notify_encoder(encoder, false);
>  		encoder->disable(encoder);
> @@ -6004,6 +6008,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> +	trigger_all_vblank_jobs(intel_crtc);
> +
>  	/*
>  	 * On gen2 planes are double buffered but the pipe isn't, so we must
>  	 * wait for planes to fully turn off before disabling the pipe.
> @@ -13649,6 +13655,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
> +	INIT_LIST_HEAD(&intel_crtc->vblank_jobs);
> +
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
>  	return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 627741a..630e7c1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -504,6 +504,24 @@ struct intel_crtc_atomic_commit {
>  	unsigned update_sprite_watermarks;
>  };
>  
> +typedef void (*i915_vblank_callback_t)(struct intel_crtc *crtc,
> +				       void *data,
> +				       bool early,
> +				       u32 fired_seq);
> +
> +/* Task to run (or schedule on a workqueue) on a specific vblank */
> +struct i915_vblank_job {
> +	u32 seq;
> +	u32 fired_seq;                     /* early if crtc gets disabled */
> +	struct intel_crtc *crtc;
> +	struct workqueue_struct *wq;       /* NULL = run from interrupt */
> +	i915_vblank_callback_t callback;
> +	void *callback_data;
> +
> +	struct list_head link;
> +	struct work_struct work;
> +};
> +
>  struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
> @@ -550,6 +568,9 @@ struct intel_crtc {
>  
>  	/* scalers available on this crtc */
>  	int num_scalers;
> +
> +	/* Jobs to run/schedule on vblank */
> +	struct list_head vblank_jobs;
>  };
>  
>  struct intel_plane_wm_parameters {
> @@ -913,6 +934,12 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>  				     unsigned int pipe_mask);
> +int intel_schedule_vblank_job(struct intel_crtc *crtc,
> +			      i915_vblank_callback_t callback,
> +			      void *callback_data,
> +			      struct workqueue_struct *wq,
> +			      u32 seq);
> +void trigger_all_vblank_jobs(struct intel_crtc *crtc);
>  
>  /* intel_crt.c */
>  void intel_crt_init(struct drm_device *dev);
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFC 03/15] drm/i915: Update sprite watermarks outside vblank evasion

[Ville Syrjälä]

On Wed, May 20, 2015 at 07:12:15PM -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).  Ensure that we
> properly set the atomic.update_sprite_watermarks flag so that these
> updates will happen outside vblank evasion and we can drop the direct
> calls from the plane programming code.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  | 22 ++++------------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0713258..e12b5a4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12902,10 +12902,17 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  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)
> +	    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;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3a96956..394cf37 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -198,8 +198,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, bool force)
>  	/* Activate double buffered register update */
>  	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--;
> @@ -468,8 +462,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  
>  	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  
> @@ -534,8 +526,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--;
> @@ -671,8 +661,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--;
> @@ -921,18 +909,16 @@ finish:
>  		intel_crtc->atomic.fb_bits |=
>  			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>  
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> +		if (intel_wm_need_update(plane, &state->base) || !state->visible)
> +			intel_crtc->atomic.update_sprite_watermarks |=
> +				(1 << drm_plane_index(plane));

I have a local kludge in my CHV WM tree now that splits this into pre
and post wm updates, and chooses one or the other based on whether the
plane is getting enabled or disabled. Of course that kind of kludge
is only needed if the proper two part watermark update isn't being done,
and I didn't want to reimplement it for CHV at this time.

Actually what I have for CHV now is starting to look quite a bit like
the ILK thing, so in the future we might be able to unify more. I
started by putting the wm state into the plane/crtc states, but those
didn't seem sane yet, so had to pull them out again.

Anyway this is all just FYI, but obviously I don't want to block progress
on the ILK stuff with any requirement to unify at this time.

>  
> -		if (!state->visible) {
> +		if (!state->visible)
>  			/*
>  			 * Avoid underruns when disabling the sprite.
>  			 * FIXME remove once watermark updates are done properly.
>  			 */
>  			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_sprite_watermarks |=
> -				(1 << drm_plane_index(plane));
> -		}
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFC 05/15] drm/i915: Lookup CRTC for plane directly

[Ville Syrjälä]

On Wed, May 20, 2015 at 07:12:17PM -0700, Matt Roper wrote:
> Various places in the atomic plane code obtain the CRTC via
> plane_state->crtc.  But plane_state->crtc is NULL when disabling the
> plane, so the code will fall back to looking at the old CRTC value in
> plane->crtc in that case.  This isn't going to work when we start
> supporting non-blocking flips (since the legacy plane->crtc pointer will
> already be updated to its new value before the asynchronous workqueue
> task runs the plane commit function).  The code is also fragile in
> general (we have to be careful when doing stuff like updating properties
> on a disabled plane).  Since Intel hardware has a fixed plane to pipe
> mapping, let's just lookup the CRTC via that fixed mapping and avoid
> future mistakes.

Older gens have non-fixed mapping. So I think we should really be looking
at the state (future state in check, current state in commit). Sadly last
time I tried to do that the state was still all lies and resulted in
explosions. Dunno if things are better now.

> 
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
>  drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
>  drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
>  4 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 714ee24..21c808d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -115,16 +115,16 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	/*
> -	 * Both crtc and plane->crtc could be NULL if we're updating a
> -	 * property while the plane is disabled.  We don't actually have
> -	 * anything driver-specific we need to test in that case, so
> -	 * just return success.
> +	 * Both state->crtc and plane->state->crtc could be NULL if we're
> +	 * updating a property while the plane is disabled.  We don't actually
> +	 * have anything driver-specific we need to test in that case, so just
> +	 * return success.
>  	 */
> -	if (!crtc)
> +	if (!state->crtc && !plane->state->crtc)
>  		return 0;
>  
>  	crtc_state = intel_crtc_state_for_plane(intel_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a7d2a9..f4398a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13092,7 +13092,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  	crtc_state = intel_crtc_state_for_plane(state);
>  
> @@ -13174,7 +13174,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct intel_crtc *intel_crtc;
>  	struct drm_rect *src = &state->src;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = fb;
> @@ -13409,7 +13409,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	unsigned stride;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> @@ -13482,7 +13482,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>  	uint32_t addr;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = state->base.fb;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1e22ffe..ea67093 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -803,6 +803,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
>  	return dev_priv->plane_to_crtc_mapping[plane];
>  }
>  
> +static inline struct drm_crtc *
> +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> +}
> +
>  struct intel_unpin_work {
>  	struct work_struct work;
>  	struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index bfe9213..2164e63 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -752,7 +752,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int pixel_size;
>  	int ret;
>  
> -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
>  	crtc_state = intel_crtc_state_for_plane(state);
>  
>  	if (!fb) {
> @@ -942,7 +942,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = fb;
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFC 03/15] drm/i915: Update sprite watermarks outside vblank evasion

[Damien Lespiau]

On Wed, May 20, 2015 at 07:12:15PM -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).  Ensure that we
> properly set the atomic.update_sprite_watermarks flag so that these
> updates will happen outside vblank evasion and we can drop the direct
> calls from the plane programming code.

Another note is that on SKL/BXT, WM and DDB registers are:

  - double buffered, armed by PLANE_SURF
  - the double buffer update mechanism can additionally be
    disabled/enabled at will ("Go bit")

On top of that, the WM/DDB configuration update with several pipes is a
bit tricky as the DDB is a shared resource between pipes and careful
ordering of pipe updates is needed to avoid planes from different pipes
sharing overlapping DDB allocations.

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

Re: [RFC 05/15] drm/i915: Lookup CRTC for plane directly

[Daniel Vetter]

On Thu, May 21, 2015 at 05:04:15PM +0300, Ville Syrjälä wrote:
> On Wed, May 20, 2015 at 07:12:17PM -0700, Matt Roper wrote:
> > Various places in the atomic plane code obtain the CRTC via
> > plane_state->crtc.  But plane_state->crtc is NULL when disabling the
> > plane, so the code will fall back to looking at the old CRTC value in
> > plane->crtc in that case.  This isn't going to work when we start
> > supporting non-blocking flips (since the legacy plane->crtc pointer will
> > already be updated to its new value before the asynchronous workqueue
> > task runs the plane commit function).  The code is also fragile in
> > general (we have to be careful when doing stuff like updating properties
> > on a disabled plane).  Since Intel hardware has a fixed plane to pipe
> > mapping, let's just lookup the CRTC via that fixed mapping and avoid
> > future mistakes.
> 
> Older gens have non-fixed mapping. So I think we should really be looking
> at the state (future state in check, current state in commit). Sadly last
> time I tried to do that the state was still all lies and resulted in
> explosions. Dunno if things are better now.

Yeah I think for planes that can switch around we'd need to look at
states. But that's only possible once all the reworking has settled down a
bit. Luckily we don't support any such planes (yet).
-Daniel

> 
> > 
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
> >  drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
> >  drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
> >  drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
> >  4 files changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 714ee24..21c808d 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -115,16 +115,16 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	/*
> > -	 * Both crtc and plane->crtc could be NULL if we're updating a
> > -	 * property while the plane is disabled.  We don't actually have
> > -	 * anything driver-specific we need to test in that case, so
> > -	 * just return success.
> > +	 * Both state->crtc and plane->state->crtc could be NULL if we're
> > +	 * updating a property while the plane is disabled.  We don't actually
> > +	 * have anything driver-specific we need to test in that case, so just
> > +	 * return success.
> >  	 */
> > -	if (!crtc)
> > +	if (!state->crtc && !plane->state->crtc)
> >  		return 0;
> >  
> >  	crtc_state = intel_crtc_state_for_plane(intel_state);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a7d2a9..f4398a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13092,7 +13092,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  	crtc_state = intel_crtc_state_for_plane(state);
> >  
> > @@ -13174,7 +13174,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >  	struct intel_crtc *intel_crtc;
> >  	struct drm_rect *src = &state->src;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	plane->fb = fb;
> > @@ -13409,7 +13409,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  	unsigned stride;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > @@ -13482,7 +13482,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> >  	uint32_t addr;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	plane->fb = state->base.fb;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1e22ffe..ea67093 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -803,6 +803,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
> >  	return dev_priv->plane_to_crtc_mapping[plane];
> >  }
> >  
> > +static inline struct drm_crtc *
> > +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > +	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> > +}
> > +
> >  struct intel_unpin_work {
> >  	struct work_struct work;
> >  	struct drm_crtc *crtc;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index bfe9213..2164e63 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -752,7 +752,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  	int pixel_size;
> >  	int ret;
> >  
> > -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> > +	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
> >  	crtc_state = intel_crtc_state_for_plane(state);
> >  
> >  	if (!fb) {
> > @@ -942,7 +942,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> >  	unsigned int crtc_w, crtc_h;
> >  	uint32_t src_x, src_y, src_w, src_h;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	plane->fb = fb;
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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: [RFC 13/15] drm/i915: Introduce intel_schedule_vblank_job()

[Daniel Vetter]

On Thu, May 21, 2015 at 04:20:04PM +0300, Ville Syrjälä wrote:
> On Wed, May 20, 2015 at 07:12:25PM -0700, Matt Roper wrote:
> > Various places in the driver need the ability to schedule actions to run
> > on a future vblank (updating watermarks, unpinning buffers, etc.).  The
> > long term goal is to add such a mechanism in the DRM core that can be
> > shared by all drivers.  Paulo Zanoni has already written some
> > preliminary patches to support this, but his work will probably take
> > some time to polish and get merged since it needs to untangle the DRM
> > core's vblank infrastructure.  This patch is partially based on Paulo's
> > work, but takes a simpler approach and just adds an i915-specific
> > mechanism that can be used in the interim since we have an immediate
> > need for watermark updates.
> > 
> > Inspired-by: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I like what I had better. No needless kmallocs (this IMO is a must
> requirement for any solution), no needless workqueues, uses the hw
> vblank counter and absolute values so isn't racy.

Yeah the kmalloc in Paulo's patches was one of the things I didn't like.
We need to do the same like with timers/work items and make them
embedded into something we already have allocated.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c      | 117 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |   8 +++
> >  drivers/gpu/drm/i915/intel_drv.h     |  27 ++++++++
> >  3 files changed, 152 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 557af88..fd5a795 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1649,8 +1649,37 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> >  	}
> >  }
> >  
> > +static void process_vblank_jobs(struct drm_device *dev, enum pipe pipe)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct i915_vblank_job *job, *tmp;
> > +	u32 seq;
> > +
> > +	seq = drm_vblank_count(dev, pipe);
> > +	list_for_each_entry_safe(job, tmp, &intel_crtc->vblank_jobs, link) {
> > +		if (seq - job->seq > (1<<23))
> > +			continue;
> > +
> > +		list_del(&job->link);
> > +		drm_vblank_put(dev, pipe);
> > +
> > +		if (job->wq) {
> > +			job->fired_seq = seq;
> > +			queue_work(job->wq, &job->work);
> > +		} else {
> > +			job->callback(intel_crtc, job->callback_data,
> > +				      false, seq);
> > +			kfree(job);
> > +		}
> > +	}
> > +}
> > +
> >  static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> >  {
> > +	process_vblank_jobs(dev, pipe);
> > +
> >  	if (!drm_handle_vblank(dev, pipe))
> >  		return false;
> >  
> > @@ -4511,3 +4540,91 @@ void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv)
> >  	dev_priv->dev->driver->irq_preinstall(dev_priv->dev);
> >  	dev_priv->dev->driver->irq_postinstall(dev_priv->dev);
> >  }
> > +
> > +static void vblank_job_handler(struct work_struct *work)
> > +{
> > +	struct i915_vblank_job *job =
> > +		container_of(work, struct i915_vblank_job, work);
> > +
> > +	job->callback(job->crtc, job->callback_data,
> > +		      job->seq != job->fired_seq, job->fired_seq);
> > +	kfree(job);
> > +}
> > +
> > +/**
> > + * intel_schedule_vblank_job - schedule work to happen on specified vblank
> > + * @crtc: CRTC whose vblank should trigger the work
> > + * @callback: Code to run on vblank
> > + * @callback_data: Data to pass to callback function
> > + * @wq: Workqueue to run job on (or NULL to run from interrupt context)
> > + * @seq: vblank count relative to current to schedule for
> > + *
> > + * Schedule code that should be run after the specified vblank fires.  The code
> > + * can either run directly in the interrupt context or on a specified
> > + * workqueue.
> > + */
> > +int intel_schedule_vblank_job(struct intel_crtc *crtc,
> > +			      i915_vblank_callback_t callback,
> > +			      void *callback_data,
> > +			      struct workqueue_struct *wq,
> > +			      u32 seq)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct i915_vblank_job *job;
> > +	unsigned long irqflags;
> > +	int ret;
> > +
> > +	job = kzalloc(sizeof(*job), GFP_ATOMIC);
> > +	if (!job)
> > +		return -ENOMEM;
> > +
> > +	job->wq = wq;
> > +	job->crtc = crtc;
> > +	job->callback = callback;
> > +	job->callback_data = callback_data;
> > +	if (job->wq)
> > +		INIT_WORK(&job->work, vblank_job_handler);
> > +
> > +	ret = drm_crtc_vblank_get(&crtc->base);
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("Failed to enable interrupts, ret=%d\n", ret);
> > +		kfree(job);
> > +		return -EINVAL;
> > +	}
> > +
> > +	spin_lock_irqsave(&dev->event_lock, irqflags);
> > +	job->seq = seq + drm_vblank_count(dev, crtc->pipe);
> > +	list_add_tail(&job->link, &crtc->vblank_jobs);
> > +	spin_unlock_irqrestore(&dev->event_lock, irqflags);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * intel_trigger_all_vblank_jobs - immediately trigger vblank jobs for a crtc
> > + * @crtc: CRTC to trigger all outstanding jobs for
> > + *
> > + * Immediately triggers all of the jobs awaiting future vblanks on a CRTC.
> > + * This will be called when a CRTC is disabled (because there may never be
> > + * another vblank event).  The job callbacks will receive 'true' for the
> > + * early parameter, as well as the current vblank count.
> > + */
> > +void trigger_all_vblank_jobs(struct intel_crtc *crtc)
> > +{
> > +	struct i915_vblank_job *job, *tmp;
> > +	u32 seq;
> > +
> > +	seq = drm_vblank_count(crtc->base.dev, crtc->pipe);
> > +	list_for_each_entry_safe(job, tmp, &crtc->vblank_jobs, link) {
> > +		list_del(&job->link);
> > +		drm_vblank_put(crtc->base.dev, crtc->pipe);
> > +
> > +		if (job->wq) {
> > +			job->fired_seq = seq;
> > +			queue_work(job->wq, &job->work);
> > +		} else {
> > +			job->callback(crtc, job->callback_data, true, seq);
> > +			kfree(job);
> > +		}
> > +	}
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 76affba..9b56d07 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5099,6 +5099,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >  	if (!intel_crtc->active)
> >  		return;
> >  
> > +	trigger_all_vblank_jobs(intel_crtc);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		encoder->disable(encoder);
> >  
> > @@ -5161,6 +5163,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >  	if (!intel_crtc->active)
> >  		return;
> >  
> > +	trigger_all_vblank_jobs(intel_crtc);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder) {
> >  		intel_opregion_notify_encoder(encoder, false);
> >  		encoder->disable(encoder);
> > @@ -6004,6 +6008,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> >  	if (!intel_crtc->active)
> >  		return;
> >  
> > +	trigger_all_vblank_jobs(intel_crtc);
> > +
> >  	/*
> >  	 * On gen2 planes are double buffered but the pipe isn't, so we must
> >  	 * wait for planes to fully turn off before disabling the pipe.
> > @@ -13649,6 +13655,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  
> >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> >  
> > +	INIT_LIST_HEAD(&intel_crtc->vblank_jobs);
> > +
> >  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> >  	return;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 627741a..630e7c1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -504,6 +504,24 @@ struct intel_crtc_atomic_commit {
> >  	unsigned update_sprite_watermarks;
> >  };
> >  
> > +typedef void (*i915_vblank_callback_t)(struct intel_crtc *crtc,
> > +				       void *data,
> > +				       bool early,
> > +				       u32 fired_seq);
> > +
> > +/* Task to run (or schedule on a workqueue) on a specific vblank */
> > +struct i915_vblank_job {
> > +	u32 seq;
> > +	u32 fired_seq;                     /* early if crtc gets disabled */
> > +	struct intel_crtc *crtc;
> > +	struct workqueue_struct *wq;       /* NULL = run from interrupt */
> > +	i915_vblank_callback_t callback;
> > +	void *callback_data;
> > +
> > +	struct list_head link;
> > +	struct work_struct work;
> > +};
> > +
> >  struct intel_crtc {
> >  	struct drm_crtc base;
> >  	enum pipe pipe;
> > @@ -550,6 +568,9 @@ struct intel_crtc {
> >  
> >  	/* scalers available on this crtc */
> >  	int num_scalers;
> > +
> > +	/* Jobs to run/schedule on vblank */
> > +	struct list_head vblank_jobs;
> >  };
> >  
> >  struct intel_plane_wm_parameters {
> > @@ -913,6 +934,12 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
> >  int intel_get_crtc_scanline(struct intel_crtc *crtc);
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> >  				     unsigned int pipe_mask);
> > +int intel_schedule_vblank_job(struct intel_crtc *crtc,
> > +			      i915_vblank_callback_t callback,
> > +			      void *callback_data,
> > +			      struct workqueue_struct *wq,
> > +			      u32 seq);
> > +void trigger_all_vblank_jobs(struct intel_crtc *crtc);
> >  
> >  /* intel_crt.c */
> >  void intel_crt_init(struct drm_device *dev);
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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: [RFC 14/15] drm/i915: Program atomic watermarks via vblank job

[G, Pallavi]

Why don’t we do this wm update as part of the fb unpin what is the need for wq here. May add some latency
Please clarify

Thanks,
Pallavi

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Matt Roper
Sent: Thursday, May 21, 2015 7:42 AM
To: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [RFC 14/15] drm/i915: Program atomic watermarks via vblank job

Use the new vblank job infrastructure to schedule watermark programming to happen when the next vblank occurs.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  7 +++++++
 drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_pm.c      |  1 +
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d577eba..5134101 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -569,6 +569,7 @@ struct drm_i915_display_funcs {
 				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, uint32_t sprite_height,
 				 int pixel_size, bool enable, bool scaled);
+	void (*program_watermarks)(struct drm_i915_private *dev_priv);
 	void (*modeset_global_resources)(struct drm_atomic_state *state);
 	/* Returns the active state of the crtc, and if the crtc is active,
 	 * fills out the pipe-config with the hw state. */ @@ -2494,6 +2495,12 @@ struct drm_i915_cmd_table {  #define HAS_L3_DPF(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))  #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
 
+/*
+ * FIXME:  Only some platforms have been transitioned to atomic 
+watermark
+ * updates so far.
+ */
+#define HAS_ATOMIC_WM(dev_priv) (dev_priv->display.program_watermarks 
+!= NULL)
+
 #define GT_FREQUENCY_MULTIPLIER 50
 #define GEN9_FREQ_SCALER 3
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b56d07..b2012c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13209,6 +13209,24 @@ intel_disable_primary_plane(struct drm_plane *plane,
 	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);  }
 
+static void
+do_update_watermarks(struct intel_crtc *intel_crtc,
+		     void *data,
+		     bool early,
+		     u32 seq)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+
+	/*
+	 * If we fired early because the CRTC is turning off, there's no
+	 * need to actually program watermarks.
+	 */
+	if (early)
+		return;
+
+	dev_priv->display.program_watermarks(dev_priv);
+}
+
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)  {
 	struct drm_device *dev = crtc->dev;
@@ -13251,7 +13269,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
 
-	if (intel_crtc->atomic.update_wm)
+	if (!HAS_ATOMIC_WM(dev_priv) && intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
 
 	intel_runtime_pm_get(dev_priv);
@@ -13270,6 +13288,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *p;
 
+	/*
+	 * If this platform supports atomic watermarks, schedule a job to
+	 * update watermarks when the next vblank occurs.  Otherwise, just
+	 * update watermarks the old-fashioned way.
+	 */
+	if (HAS_ATOMIC_WM(dev_priv))
+		intel_schedule_vblank_job(intel_crtc, do_update_watermarks,
+					  NULL, dev_priv->wq, 1);
+
 	if (intel_crtc->atomic.evade)
 		intel_pipe_update_end(intel_crtc,
 				      intel_crtc->atomic.start_vbl_count);
@@ -13290,10 +13317,11 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);
 
-	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
-		if (intel_crtc->atomic.update_sprite_watermarks &
-		    (1 << drm_plane_index(p)))
-			intel_update_sprite_watermarks(p, crtc);
+	if (!HAS_ATOMIC_WM(dev_priv))
+		drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
+			if (intel_crtc->atomic.update_sprite_watermarks &
+			    (1 << drm_plane_index(p)))
+				intel_update_sprite_watermarks(p, crtc);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));  } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 27337fe..7f0a0c1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6626,6 +6626,7 @@ void intel_init_pm(struct drm_device *dev)
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
+			dev_priv->display.program_watermarks = ilk_program_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
--
1.8.5.1

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