[PATCH v5 0/8] GEN-9 Arbitrated Bandwidth WM WA's & IPC

[PATCH v5 0/8] GEN-9 Arbitrated Bandwidth WM WA's & IPC

[Mahesh Kumar]

This series implements following set of functionality
    Implement IPC WA's for Broxton/KBL
    Enable IPC in supported platforms
    Convert WM calculation to fixed point calculation
    Calculation of System memory Bandwidth for SKL/KBL/BXT
    Implementation of Arbitrated memory Bandwidth related WM WA's

Mahesh Kumar (8):
  drm/i915/skl: Add variables to check x_tile and y_tile
  drm/i915/bxt: IPC WA for Broxton
  drm/i915/kbl: IPC workaround for kabylake
  drm/i915/bxt: Enable IPC support
  drm/i915/skl+: change WM calc to fixed point 16.16
  drm/i915: Add intel_atomic_get_existing_crtc_state function
  drm/i915: Decode system memory bandwidth
  drm/i915/gen9: WM memory bandwidth related workaround

 drivers/gpu/drm/i915/i915_drv.c  | 176 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h  | 108 ++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h  |  38 ++++++
 drivers/gpu/drm/i915/intel_drv.h |  15 +++
 drivers/gpu/drm/i915/intel_pm.c  | 265 ++++++++++++++++++++++++++++++++-------
 5 files changed, 555 insertions(+), 47 deletions(-)

-- 
2.10.1

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

[PATCH v5 1/8] drm/i915/skl: Add variables to check x_tile and y_tile

[Mahesh Kumar]

This patch adds variable to check for X_tiled & y_tiled planes, instead
of always checking against framebuffer-modifiers.

Changes:
 - Created separate patch as per Paulo's comment
 - Added x_tiled variable as well
Changes since V2:
 - Incorporate Paulo's comments
 - Rebase

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e54708d..8908736 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3592,13 +3592,18 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	struct intel_atomic_state *state =
 		to_intel_atomic_state(cstate->base.state);
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+	bool y_tiled, x_tiled;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
 		return 0;
 	}
 
-	if (apply_memory_bw_wa && fb->modifier == I915_FORMAT_MOD_X_TILED)
+	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
+				fb->modifier == I915_FORMAT_MOD_Yf_TILED;
+	x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
+
+	if (apply_memory_bw_wa && x_tiled)
 		latency += 15;
 
 	width = drm_rect_width(&intel_pstate->base.src) >> 16;
@@ -3637,16 +3642,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		y_min_scanlines *= 2;
 
 	plane_bytes_per_line = width * cpp;
-	if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
-	    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
+	if (y_tiled) {
 		plane_blocks_per_line =
 		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
 		plane_blocks_per_line /= y_min_scanlines;
-	} else if (fb->modifier == DRM_FORMAT_MOD_NONE) {
+	} else if (x_tiled) {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+	} else {
 		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
 					+ 1;
-	} else {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 	}
 
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
@@ -3657,8 +3661,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
 
-	if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
-	    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
+	if (y_tiled) {
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
@@ -3674,8 +3677,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	res_lines = DIV_ROUND_UP(selected_result, plane_blocks_per_line);
 
 	if (level >= 1 && level <= 7) {
-		if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
-		    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
+		if (y_tiled) {
 			res_blocks += y_tile_minimum;
 			res_lines += y_min_scanlines;
 		} else {
-- 
2.10.1

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

[PATCH v5 2/8] drm/i915/bxt: IPC WA for Broxton

[Mahesh Kumar]

If IPC is enabled in BXT, display underruns are observed.
WA: The Line Time programmed in the WM_LINETIME register should be
half of the actual calculated Line Time.

Programmed Line Time = 1/2*Calculated Line Time

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  2 ++
 drivers/gpu/drm/i915/i915_drv.h |  3 +++
 drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 445fec9..1b0a589 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1243,6 +1243,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	intel_runtime_pm_enable(dev_priv);
 
+	dev_priv->ipc_enabled = false;
+
 	/* Everything is in place, we can now relax! */
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
 		 driver.name, driver.major, driver.minor, driver.patchlevel,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 63c0ea0..394d7ea 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2128,6 +2128,9 @@ struct drm_i915_private {
 	/* perform PHY state sanity checks? */
 	bool chv_phy_assert[2];
 
+	/* To apply IPC WA */
+	bool ipc_enabled;
+
 	/* Used to save the pipe-to-encoder mapping for audio */
 	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8908736..7090a7c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3769,7 +3769,10 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 static uint32_t
 skl_compute_linetime_wm(struct intel_crtc_state *cstate)
 {
+	struct drm_atomic_state *state = cstate->base.state;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	uint32_t pixel_rate;
+	uint32_t linetime_wm;
 
 	if (!cstate->base.active)
 		return 0;
@@ -3779,8 +3782,12 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
 	if (WARN_ON(pixel_rate == 0))
 		return 0;
 
-	return DIV_ROUND_UP(8 * cstate->base.adjusted_mode.crtc_htotal * 1000,
-			    pixel_rate);
+	linetime_wm = DIV_ROUND_UP(8 * cstate->base.adjusted_mode.crtc_htotal *
+						1000, pixel_rate);
+	if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
+		linetime_wm = DIV_ROUND_UP(linetime_wm, 2);
+
+	return linetime_wm;
 }
 
 static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
-- 
2.10.1

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

[PATCH v5 3/8] drm/i915/kbl: IPC workaround for kabylake

[Mahesh Kumar]

IPC (Isoch Priority Control) may cause underflows.

KBL WA: When IPC is enabled, watermark latency values must be increased
by 4us across all levels. This brings level 0 up to 6us.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7090a7c..df39b50 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3599,6 +3599,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		return 0;
 	}
 
+	/* IPC WA for kabylake */
+	if (IS_KABYLAKE(dev_priv) && dev_priv->ipc_enabled)
+		latency += 4;
+
 	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
 				fb->modifier == I915_FORMAT_MOD_Yf_TILED;
 	x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
-- 
2.10.1

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

[PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Mahesh Kumar]

This patch adds IPC support for platforms. This patch enables IPC
only for BXT/KBL platform as for SKL recommendation is to keep is disabled.
IPC (Isochronous Priority Control) is the hardware feature, which
dynamically controles the memory read priority of Display.

When IPC is enabled, plane read requests are sent at high priority until
filling above the transition watermark, then the requests are sent at
lower priority until dropping below the level 0 watermark.
The lower priority requests allow other memory clients to have better
memory access. When IPC is disabled, all plane read requests are sent at
high priority.

Changes since V1:
 - Remove commandline parameter to disable ipc
 - Address Paulo's comments
Changes since V2:
 - Address review comments
 - Set ipc_enabled flag

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  |  1 +
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
 4 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1b0a589..4074601 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	intel_runtime_pm_enable(dev_priv);
 
 	dev_priv->ipc_enabled = false;
+	intel_enable_ipc(dev_priv);
 
 	/* Everything is in place, we can now relax! */
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c70c07a..300418a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6076,6 +6076,7 @@ enum {
 #define  DISP_FBC_WM_DIS		(1<<15)
 #define DISP_ARB_CTL2	_MMIO(0x45004)
 #define  DISP_DATA_PARTITION_5_6	(1<<6)
+#define  DISP_IPC_ENABLE		(1<<3)
 #define DBUF_CTL	_MMIO(0x45008)
 #define  DBUF_POWER_REQUEST		(1<<31)
 #define  DBUF_POWER_STATE		(1<<30)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cd132c2..ad542a2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
+void intel_enable_ipc(struct drm_i915_private *dev_priv);
 static inline int intel_enable_rc6(void)
 {
 	return i915.enable_rc6;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index df39b50..d8090aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct intel_crtc *crtc)
 		dev_priv->display.update_wm(crtc);
 }
 
+void intel_enable_ipc(struct drm_i915_private *dev_priv)
+{
+	u32 val;
+
+	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
+		return;
+
+	val = I915_READ(DISP_ARB_CTL2);
+
+	val |= DISP_IPC_ENABLE;
+
+	I915_WRITE(DISP_ARB_CTL2, val);
+	dev_priv->ipc_enabled = true;
+}
+
 /*
  * Lock protecting IPS related data structures
  */
-- 
2.10.1

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

[PATCH v5 5/8] drm/i915/skl+: change WM calc to fixed point 16.16

[Mahesh Kumar]

This patch changes Watermak calculation to fixed point calculation.
Problem with current calculation is during plane_blocks_per_line
calculation we divide intermediate blocks with min_scanlines and
takes floor of the result because of integer operation.
hence we end-up assigning less blocks than required. Which leads to
flickers.

Changes since V1:
 - Add fixed point data type as per Paulo's review

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 84 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++--------------
 2 files changed, 125 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 394d7ea..20691e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -119,6 +119,90 @@ bool __i915_inject_load_failure(const char *func, int line);
 #define i915_inject_load_failure() \
 	__i915_inject_load_failure(__func__, __LINE__)
 
+typedef struct {
+	uint32_t val;
+} uint_fixed_16_16_t;
+
+#define FP_16_16_MAX ({ \
+		uint_fixed_16_16_t fp; \
+		fp.val = UINT_MAX; \
+		fp; \
+})
+
+static inline uint_fixed_16_16_t u32_to_fp_16_16(uint32_t val)
+{
+	uint_fixed_16_16_t fp;
+
+	WARN_ON(val >> 16);
+
+	fp.val = val << 16;
+	return fp;
+}
+
+static inline uint32_t fp_16_16_to_u32_round_up(uint_fixed_16_16_t fp)
+{
+	return DIV_ROUND_UP(fp.val, 1 << 16);
+}
+
+static inline uint32_t fp_16_16_to_u32(uint_fixed_16_16_t fp)
+{
+	return (fp.val / 1 << 16);
+}
+
+static inline uint_fixed_16_16_t min_fp_16_16(uint_fixed_16_16_t min1,
+						uint_fixed_16_16_t min2)
+{
+	uint_fixed_16_16_t min;
+
+	min.val = min(min1.val, min2.val);
+	return min;
+}
+
+static inline uint_fixed_16_16_t max_fp_16_16(uint_fixed_16_16_t max1,
+						uint_fixed_16_16_t max2)
+{
+	uint_fixed_16_16_t max;
+
+	max.val = max(max1.val, max2.val);
+	return max;
+}
+
+static inline uint_fixed_16_16_t fp_16_16_div_round_up(uint32_t val, uint32_t d)
+{
+	uint_fixed_16_16_t fp, res;
+
+	fp = u32_to_fp_16_16(val);
+	res.val = DIV_ROUND_UP(fp.val, d);
+	return res;
+}
+
+static inline uint_fixed_16_16_t fp_16_16_div_round_up_ull(uint64_t val,
+								uint32_t d)
+{
+	uint_fixed_16_16_t res;
+	uint64_t interm_val;
+
+	WARN_ON(val >> 48);
+	val <<= 16;
+	interm_val = DIV_ROUND_UP_ULL(val, d);
+	WARN_ON(interm_val >> 32);
+	res.val = (uint32_t) interm_val;
+
+	return res;
+}
+
+static inline uint_fixed_16_16_t mul_fp_16_16_u32(uint32_t val,
+						uint_fixed_16_16_t mul)
+{
+	uint64_t intermediate_val;
+	uint_fixed_16_16_t fp;
+
+	intermediate_val = (uint64_t) val * mul.val;
+	WARN_ON(intermediate_val >> 32);
+	fp.val = (uint32_t) intermediate_val;
+	return fp;
+}
+
 static inline const char *yesno(bool v)
 {
 	return v ? "yes" : "no";
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d8090aa..7c788ac 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3515,33 +3515,37 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
  * for the read latency) and cpp should always be <= 8, so that
  * should allow pixel_rate up to ~2 GHz which seems sufficient since max
  * 2xcdclk is 1350 MHz and the pixel rate should never exceed that.
+ * Both Method1 & Method2 returns fixedpoint 16.16 output
 */
-static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latency)
+static uint_fixed_16_16_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp,
+					 uint32_t latency)
 {
-	uint32_t wm_intermediate_val, ret;
+	uint64_t wm_intermediate_val;
+	uint_fixed_16_16_t ret;
 
 	if (latency == 0)
-		return UINT_MAX;
-
-	wm_intermediate_val = latency * pixel_rate * cpp / 512;
-	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
+		return FP_16_16_MAX;
 
+	wm_intermediate_val = latency * pixel_rate * cpp;
+	ret = fp_16_16_div_round_up_ull(wm_intermediate_val, 1000 * 512);
 	return ret;
 }
 
-static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
-			       uint32_t latency, uint32_t plane_blocks_per_line)
+static uint_fixed_16_16_t skl_wm_method2(uint32_t pixel_rate,
+			uint32_t pipe_htotal,
+			uint32_t latency,
+			uint_fixed_16_16_t plane_blocks_per_line)
 {
-	uint32_t ret;
 	uint32_t wm_intermediate_val;
+	uint_fixed_16_16_t ret;
 
 	if (latency == 0)
-		return UINT_MAX;
+		return FP_16_16_MAX;
 
 	wm_intermediate_val = latency * pixel_rate;
-	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
-				plane_blocks_per_line;
-
+	wm_intermediate_val = DIV_ROUND_UP(wm_intermediate_val,
+							pipe_htotal * 1000);
+	ret = mul_fp_16_16_u32(wm_intermediate_val, plane_blocks_per_line);
 	return ret;
 }
 
@@ -3581,14 +3585,17 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	struct drm_plane_state *pstate = &intel_pstate->base;
 	struct drm_framebuffer *fb = pstate->fb;
 	uint32_t latency = dev_priv->wm.skl_latency[level];
-	uint32_t method1, method2;
-	uint32_t plane_bytes_per_line, plane_blocks_per_line;
+	uint_fixed_16_16_t method1, method2;
+	uint_fixed_16_16_t plane_blocks_per_line;
+	uint_fixed_16_16_t selected_result;
+	uint32_t interm_pbpl;
+	uint32_t plane_bytes_per_line;
 	uint32_t res_blocks, res_lines;
-	uint32_t selected_result;
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
-	uint32_t y_tile_minimum, y_min_scanlines;
+	uint_fixed_16_16_t y_tile_minimum;
+	uint32_t y_min_scanlines;
 	struct intel_atomic_state *state =
 		to_intel_atomic_state(cstate->base.state);
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
@@ -3647,14 +3654,16 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	plane_bytes_per_line = width * cpp;
 	if (y_tiled) {
+		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line *
+						y_min_scanlines, 512);
 		plane_blocks_per_line =
-		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
-		plane_blocks_per_line /= y_min_scanlines;
+		      fp_16_16_div_round_up(interm_pbpl, y_min_scanlines);
 	} else if (x_tiled) {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line, 512);
+		plane_blocks_per_line = u32_to_fp_16_16(interm_pbpl);
 	} else {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
-					+ 1;
+		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
+		plane_blocks_per_line = u32_to_fp_16_16(interm_pbpl);
 	}
 
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
@@ -3663,26 +3672,29 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				 latency,
 				 plane_blocks_per_line);
 
-	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
+	y_tile_minimum = mul_fp_16_16_u32(y_min_scanlines,
+						plane_blocks_per_line);
 
 	if (y_tiled) {
-		selected_result = max(method2, y_tile_minimum);
+		selected_result = max_fp_16_16(method2, y_tile_minimum);
 	} else {
 		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
 		    (plane_bytes_per_line / 512 < 1))
 			selected_result = method2;
-		else if ((ddb_allocation / plane_blocks_per_line) >= 1)
-			selected_result = min(method1, method2);
+		else if ((ddb_allocation /
+			fp_16_16_to_u32_round_up(plane_blocks_per_line)) >= 1)
+			selected_result = min_fp_16_16(method1, method2);
 		else
 			selected_result = method1;
 	}
 
-	res_blocks = selected_result + 1;
-	res_lines = DIV_ROUND_UP(selected_result, plane_blocks_per_line);
+	res_blocks = fp_16_16_to_u32_round_up(selected_result) + 1;
+	res_lines = DIV_ROUND_UP(selected_result.val,
+					plane_blocks_per_line.val);
 
 	if (level >= 1 && level <= 7) {
 		if (y_tiled) {
-			res_blocks += y_tile_minimum;
+			res_blocks += fp_16_16_to_u32_round_up(y_tile_minimum);
 			res_lines += y_min_scanlines;
 		} else {
 			res_blocks++;
-- 
2.10.1

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

[PATCH v5 6/8] drm/i915: Add intel_atomic_get_existing_crtc_state function

[Mahesh Kumar]

This patch Adds a function to extract intel_crtc_state from the
atomic_state, if not available it returns NULL.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ad542a2..db75773 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1794,6 +1794,20 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 	return to_intel_crtc_state(crtc_state);
 }
 
+static inline struct intel_crtc_state *
+intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
+				      struct intel_crtc *crtc)
+{
+	struct drm_crtc_state *crtc_state;
+
+	crtc_state = drm_atomic_get_existing_crtc_state(state, &crtc->base);
+
+	if (crtc_state)
+		return to_intel_crtc_state(crtc_state);
+	else
+		return NULL;
+}
+
 static inline struct intel_plane_state *
 intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
 				      struct intel_plane *plane)
-- 
2.10.1

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

[PATCH v5 7/8] drm/i915: Decode system memory bandwidth

[Mahesh Kumar]

This patch adds support to decode system memory bandwidth
which will be used for arbitrated display memory percentage
calculation in GEN9 based system.

Changes from v1:
 - Address comments from Paulo
 - implement decode function for SKL/KBL also
Changes from v2:
 - Rewrite the code as per HW team inputs
 - Addresses review comments

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 173 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  12 +++
 drivers/gpu/drm/i915/i915_reg.h |  37 +++++++++
 3 files changed, 222 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4074601..f79bf92 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -976,6 +976,173 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
 }
 
+static inline void skl_memdev_get_channel_rank(uint32_t val, enum rank *rank)
+{
+	uint8_t l_rank, s_rank;
+	uint8_t l_size, s_size;
+	enum rank ch_rank = DRAM_RANK_SINGLE;
+
+	l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & SKL_DRAM_SIZE_MASK;
+	s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & SKL_DRAM_SIZE_MASK;
+	l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & SKL_DRAM_RANK_MASK;
+	s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & SKL_DRAM_RANK_MASK;
+
+	/*
+	 * If any of the slot has dual rank memory consider
+	 * dual rank memory channel
+	 */
+	if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL)
+		ch_rank = DRAM_RANK_DUAL;
+
+	/*
+	 * If both the slot has single rank memory then configuration
+	 * is dual rank memory
+	 */
+	if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) &&
+		(s_size && s_rank == SKL_DRAM_RANK_SINGLE))
+		ch_rank = DRAM_RANK_DUAL;
+	*rank = ch_rank;
+}
+
+static int
+skl_get_memdev_info(struct drm_i915_private *dev_priv)
+{
+	struct memdev_info *memdev_info = &dev_priv->memdev_info;
+	uint32_t mem_freq_khz;
+	uint32_t val;
+	enum rank ch0_rank, ch1_rank;
+
+	val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
+	mem_freq_khz = (val & SKL_REQ_DATA_MASK) *
+				SKL_MEMORY_FREQ_MULTIPLIER_KHZ;
+
+	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
+	if (val != 0x0) {
+		memdev_info->num_channels++;
+		skl_memdev_get_channel_rank(val, &ch0_rank);
+	}
+
+	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
+	if (val != 0x0) {
+		memdev_info->num_channels++;
+		skl_memdev_get_channel_rank(val, &ch1_rank);
+	}
+
+	if (memdev_info->num_channels == 0) {
+		DRM_ERROR("Number of mem channels are zero\n");
+		return -EINVAL;
+	}
+
+	memdev_info->bandwidth_kbps = (memdev_info->num_channels *
+							mem_freq_khz * 8);
+
+	if (memdev_info->bandwidth_kbps == 0) {
+		DRM_ERROR("Couldn't get system memory bandwidth\n");
+		return -EINVAL;
+	}
+	memdev_info->valid = true;
+
+	/*
+	 * If any of channel is single rank channel,
+	 * consider single rank memory
+	 */
+	if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == DRAM_RANK_SINGLE)
+		memdev_info->rank = DRAM_RANK_SINGLE;
+	else
+		memdev_info->rank = max(ch0_rank, ch1_rank);
+
+	return 0;
+}
+
+static int
+bxt_get_memdev_info(struct drm_i915_private *dev_priv)
+{
+	struct memdev_info *memdev_info = &dev_priv->memdev_info;
+	uint32_t dram_channels;
+	uint32_t mem_freq_khz, val;
+	uint8_t num_active_channels;
+	int i;
+
+	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
+	mem_freq_khz = ((val & BXT_REQ_DATA_MASK) *
+				BXT_MEMORY_FREQ_MULTIPLIER_KHZ);
+
+	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
+					BXT_DRAM_CHANNEL_ACTIVE_MASK;
+	num_active_channels = hweight32(dram_channels);
+
+	memdev_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
+
+	if (memdev_info->bandwidth_kbps == 0) {
+		DRM_ERROR("Couldn't get system memory bandwidth\n");
+		return -EINVAL;
+	}
+	memdev_info->valid = true;
+
+	/*
+	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
+	 */
+	for (i = 0; i < BXT_D_CR_DRP0_DUNIT_MAX; i++) {
+		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
+		if (val != 0xFFFFFFFF) {
+			uint8_t rank;
+			enum rank ch_rank;
+
+			memdev_info->num_channels++;
+			rank = val & BXT_DRAM_RANK_MASK;
+			if (rank == BXT_DRAM_RANK_SINGLE)
+				ch_rank = DRAM_RANK_SINGLE;
+			else if (rank == BXT_DRAM_RANK_DUAL)
+				ch_rank = DRAM_RANK_DUAL;
+			else
+				ch_rank = DRAM_RANK_INVALID;
+
+			/*
+			 * If any of channel is having single rank memory
+			 * consider memory as single rank
+			 */
+			if (memdev_info->rank == DRAM_RANK_INVALID)
+				memdev_info->rank = ch_rank;
+			else if (ch_rank == DRAM_RANK_SINGLE)
+				memdev_info->rank = DRAM_RANK_SINGLE;
+		}
+	}
+	return 0;
+}
+
+static void
+intel_get_memdev_info(struct drm_i915_private *dev_priv)
+{
+	struct memdev_info *memdev_info = &dev_priv->memdev_info;
+	int ret;
+
+	memdev_info->valid = false;
+	memdev_info->rank = DRAM_RANK_INVALID;
+	memdev_info->num_channels = 0;
+
+	if (!IS_GEN9(dev_priv))
+		return;
+
+	if (IS_BROXTON(dev_priv))
+		ret = bxt_get_memdev_info(dev_priv);
+	else
+		ret = skl_get_memdev_info(dev_priv);
+	if (ret)
+		return;
+
+	DRM_DEBUG_DRIVER("DRAM bandwidth: %d KBps total-channels: %d\n",
+				memdev_info->bandwidth_kbps,
+				memdev_info->num_channels);
+	if (memdev_info->rank == DRAM_RANK_INVALID)
+		DRM_INFO("Counld not get memory rank info\n");
+	else {
+		DRM_DEBUG_DRIVER("DRAM rank: %s\n",
+				(memdev_info->rank == DRAM_RANK_DUAL) ?
+						"dual" : "single");
+	}
+}
+
+
 /**
  * i915_driver_init_hw - setup state requiring device access
  * @dev_priv: device private
@@ -1078,6 +1245,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 			DRM_DEBUG_DRIVER("can't enable MSI");
 	}
 
+	/*
+	 * Fill the memdev structure to get the system raw bandwidth
+	 * This will be used by WM algorithm, to implement GEN9 based WA
+	 */
+	intel_get_memdev_info(dev_priv);
+
 	return 0;
 
 out_ggtt:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20691e9..171163c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2168,6 +2168,18 @@ struct drm_i915_private {
 		bool distrust_bios_wm;
 	} wm;
 
+	struct memdev_info {
+		bool valid;
+		uint32_t bandwidth_kbps;
+		uint8_t num_channels;
+		enum rank {
+			DRAM_RANK_INVALID = 0,
+			DRAM_RANK_SINGLE,
+			DRAM_RANK_DUAL
+		} rank;
+	} memdev_info;
+
+
 	struct i915_runtime_pm pm;
 
 	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 300418a..9a06810 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7672,6 +7672,43 @@ enum {
 #define  DC_STATE_DEBUG_MASK_CORES	(1<<0)
 #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1<<1)
 
+#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
+#define BXT_REQ_DATA_MASK			0x3F
+#define BXT_DRAM_ACTIVE_CHANNEL_SHIFT		12
+#define BXT_DRAM_ACTIVE_CHANNEL_MASK		0xF
+/*
+ * BIOS programs this field of REQ_DATA [5:0] in integer
+ * multiple of 133333 KHz (133.33MHz)
+ */
+#define	BXT_MEMORY_FREQ_MULTIPLIER_KHZ		133333
+#define BXT_D_CR_DRP0_DUNIT8			0x1000
+#define BXT_D_CR_DRP0_DUNIT9			0x1200
+#define BXT_D_CR_DRP0_DUNIT_MAX			4
+#define _MMIO_MCHBAR_DUNIT(x, a, b) _MMIO(MCHBAR_MIRROR_BASE_SNB + (a) + (x)*((b)-(a)))
+#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO_MCHBAR_DUNIT(x, BXT_D_CR_DRP0_DUNIT8, BXT_D_CR_DRP0_DUNIT9)
+#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
+#define BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
+#define BXT_DRAM_RANK_MASK			0x3
+#define BXT_DRAM_RANK_SINGLE			0x1
+#define BXT_DRAM_RANK_DUAL			0x3
+
+/*
+ * SKL memory frequeny multiplier is 266667 KHz (266.67 MHz)
+ */
+#define	SKL_MEMORY_FREQ_MULTIPLIER_KHZ		266667
+#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
+#define SKL_REQ_DATA_MASK			(0xF << 0)
+#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
+#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
+#define SKL_DRAM_SIZE_MASK			0x1F
+#define SKL_DRAM_SIZE_L_SHIFT			0
+#define SKL_DRAM_SIZE_S_SHIFT			16
+#define SKL_DRAM_RANK_MASK			0x1
+#define SKL_DRAM_RANK_L_SHIFT			10
+#define SKL_DRAM_RANK_S_SHIFT			26
+#define SKL_DRAM_RANK_SINGLE			0x0
+#define SKL_DRAM_RANK_DUAL			0x1
+
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
  * since on HSW we can't write to it using I915_WRITE. */
 #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
-- 
2.10.1

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

[PATCH v5 8/8] drm/i915/gen9: WM memory bandwidth related workaround

[Mahesh Kumar]

This patch implemnets Workariunds related to display arbitrated memory
bandwidth. These WA are applicabe for all gen-9 based platforms.

Changes since v1:
 - Rebase on top of Paulo's patch series
Changes since v2:
 - Address review comments
 - Rebase/rework as per other patch changes in series

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   9 +++
 drivers/gpu/drm/i915/intel_pm.c | 149 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 171163c..f8700a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1193,6 +1193,13 @@ enum intel_sbi_destination {
 	SBI_MPHY,
 };
 
+/* SKL+ Watermark arbitrated display bandwidth Workarounds */
+enum watermark_memory_wa {
+	WATERMARK_WA_NONE,
+	WATERMARK_WA_X_TILED,
+	WATERMARK_WA_Y_TILED,
+};
+
 #define QUIRK_PIPEA_FORCE (1<<0)
 #define QUIRK_LVDS_SSC_DISABLE (1<<1)
 #define QUIRK_INVERT_BRIGHTNESS (1<<2)
@@ -1764,6 +1771,8 @@ struct skl_ddb_allocation {
 
 struct skl_wm_values {
 	unsigned dirty_pipes;
+	/* any WaterMark memory workaround Required */
+	enum watermark_memory_wa mem_wa;
 	struct skl_ddb_allocation ddb;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7c788ac..f42b190 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2889,11 +2889,7 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
-/*
- * FIXME: We still don't have the proper code detect if we need to apply the WA,
- * so assume we'll always need it in order to avoid underruns.
- */
-static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
+static bool intel_needs_memory_bw_wa(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 
@@ -3067,7 +3063,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 		latency = dev_priv->wm.skl_latency[level];
 
-		if (skl_needs_memory_bw_wa(intel_state) &&
+		if (intel_needs_memory_bw_wa(intel_state) &&
 		    plane->base.state->fb->modifier ==
 		    I915_FORMAT_MOD_X_TILED)
 			latency += 15;
@@ -3598,7 +3594,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint32_t y_min_scanlines;
 	struct intel_atomic_state *state =
 		to_intel_atomic_state(cstate->base.state);
-	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+	enum watermark_memory_wa mem_wa;
 	bool y_tiled, x_tiled;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
@@ -3614,7 +3610,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				fb->modifier == I915_FORMAT_MOD_Yf_TILED;
 	x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
 
-	if (apply_memory_bw_wa && x_tiled)
+	mem_wa = state->wm_results.mem_wa;
+	if (mem_wa != WATERMARK_WA_NONE && x_tiled)
 		latency += 15;
 
 	width = drm_rect_width(&intel_pstate->base.src) >> 16;
@@ -3649,7 +3646,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		y_min_scanlines = 4;
 	}
 
-	if (apply_memory_bw_wa)
+	if (mem_wa == WATERMARK_WA_Y_TILED)
 		y_min_scanlines *= 2;
 
 	plane_bytes_per_line = width * cpp;
@@ -4078,6 +4075,15 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	}
 
 	/*
+	 * If Watermark workaround is changed we need to recalculate
+	 * watermark values for all active pipes
+	 */
+	if (intel_state->wm_results.mem_wa != dev_priv->wm.skl_hw.mem_wa) {
+		realloc_pipes = ~0;
+		intel_state->wm_results.dirty_pipes = ~0;
+	}
+
+	/*
 	 * We're not recomputing for the pipes not included in the commit, so
 	 * make sure we start with the current state.
 	 */
@@ -4103,6 +4109,129 @@ skl_compute_ddb(struct drm_atomic_state *state)
 }
 
 static void
+skl_compute_memory_bandwidth_wm_wa(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *intel_crtc;
+	struct intel_plane_state *intel_pstate;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct memdev_info *memdev_info = &dev_priv->memdev_info;
+	int num_active_pipes;
+	uint32_t max_pipe_bw_kbps, total_pipe_bw_kbps;
+	int display_bw_percentage;
+	bool y_tile_enabled = false;
+
+	if (!intel_needs_memory_bw_wa(intel_state)) {
+		intel_state->wm_results.mem_wa = WATERMARK_WA_NONE;
+		return;
+	}
+
+	if (!memdev_info->valid)
+		goto exit;
+
+	max_pipe_bw_kbps = 0;
+	num_active_pipes = 0;
+	for_each_intel_crtc(dev, intel_crtc) {
+		struct intel_crtc_state *cstate;
+		struct intel_plane *plane;
+		int num_active_planes;
+		uint32_t  max_plane_bw_kbps, pipe_bw_kbps;
+
+		/*
+		 * If CRTC is part of current atomic commit, get crtc state from
+		 * existing CRTC state. else take the cached CRTC state
+		 */
+		cstate = NULL;
+		if (state)
+			cstate = intel_atomic_get_existing_crtc_state(state,
+					intel_crtc);
+		if (!cstate)
+			cstate = to_intel_crtc_state(intel_crtc->base.state);
+
+		if (!cstate->base.active)
+			continue;
+
+		num_active_planes = 0;
+		max_plane_bw_kbps = 0;
+		for_each_intel_plane_mask(dev, plane, cstate->base.plane_mask) {
+			struct drm_framebuffer *fb;
+			uint32_t plane_bw_kbps;
+			uint32_t id = skl_wm_plane_id(plane);
+
+			intel_pstate = NULL;
+			intel_pstate =
+				intel_atomic_get_existing_plane_state(state,
+									plane);
+			if (!intel_pstate)
+				intel_pstate =
+					to_intel_plane_state(plane->base.state);
+
+			if (!intel_pstate->base.visible)
+				continue;
+
+			if (WARN_ON(!intel_pstate->base.fb))
+				continue;
+
+			if (id == PLANE_CURSOR)
+				continue;
+
+			fb = intel_pstate->base.fb;
+			if ((fb->modifier == I915_FORMAT_MOD_Y_TILED ||
+				fb->modifier == I915_FORMAT_MOD_Yf_TILED))
+				y_tile_enabled = true;
+
+			plane_bw_kbps = skl_adjusted_plane_pixel_rate(cstate,
+								intel_pstate);
+			max_plane_bw_kbps = max(plane_bw_kbps,
+							max_plane_bw_kbps);
+			num_active_planes++;
+		}
+		pipe_bw_kbps = max_plane_bw_kbps * num_active_planes;
+		max_pipe_bw_kbps = max(pipe_bw_kbps, max_pipe_bw_kbps);
+		num_active_pipes++;
+	}
+
+	total_pipe_bw_kbps = max_pipe_bw_kbps * num_active_pipes;
+
+	display_bw_percentage = DIV_ROUND_UP_ULL(total_pipe_bw_kbps * 100,
+						memdev_info->bandwidth_kbps);
+
+	/*
+	 * If there is any Ytile plane enabled and arbitrated display
+	 * bandwidth > 20% of raw system memory bandwidth
+	 * Enale Y-tile related WA
+	 *
+	 * If memory is dual channel single rank, Xtile limit = 35%, else Xtile
+	 * limit = 60%
+	 * If there is no Ytile plane enabled and
+	 * arbitrated display bandwidth > Xtile limit
+	 * Enable X-tile realated WA
+	 */
+	if (y_tile_enabled && (display_bw_percentage > 20))
+		intel_state->wm_results.mem_wa = WATERMARK_WA_Y_TILED;
+	else {
+		int x_tile_percentage = 60;
+		enum rank rank = DRAM_RANK_SINGLE;
+
+		if (memdev_info->rank != DRAM_RANK_INVALID)
+			rank = memdev_info->rank;
+
+		if ((rank == DRAM_RANK_SINGLE) &&
+					(memdev_info->num_channels == 2))
+			x_tile_percentage = 35;
+
+		if (display_bw_percentage > x_tile_percentage)
+			intel_state->wm_results.mem_wa = WATERMARK_WA_X_TILED;
+	}
+	return;
+
+exit:
+	intel_state->wm_results.mem_wa = WATERMARK_WA_Y_TILED;
+}
+
+
+static void
 skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 		     struct skl_wm_values *src,
 		     enum pipe pipe)
@@ -4178,6 +4307,8 @@ skl_compute_wm(struct drm_atomic_state *state)
 	/* Clear all dirty flags */
 	results->dirty_pipes = 0;
 
+	skl_compute_memory_bandwidth_wm_wa(state);
+
 	ret = skl_compute_ddb(state);
 	if (ret)
 		return ret;
-- 
2.10.1

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

✗ Fi.CI.BAT: failure for GEN-9 Arbitrated Bandwidth WM WA's & IPC

[Patchwork]

== Series Details ==

Series: GEN-9 Arbitrated Bandwidth WM WA's & IPC
URL   : https://patchwork.freedesktop.org/series/15562/
State : failure

== Summary ==

  CC [M]  drivers/gpu/drm/i915/gvt/sched_policy.o
  CC [M]  drivers/gpu/drm/i915/gvt/cmd_parser.o
  LD      sound/built-in.o
  LD      drivers/watchdog/built-in.o
  LD      drivers/rtc/built-in.o
  LD      drivers/pci/pcie/pcieportdrv.o
drivers/gpu/drm/i915/i915_drv.c: In function ‘i915_driver_load’:
drivers/gpu/drm/i915/i915_drv.c:1049:35: error: ‘ch1_rank’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == DRAM_RANK_SINGLE)
                                   ^
drivers/gpu/drm/i915/i915_drv.c:1013:22: note: ‘ch1_rank’ was declared here
  enum rank ch0_rank, ch1_rank;
                      ^
drivers/gpu/drm/i915/i915_drv.c:1049:5: error: ‘ch0_rank’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == DRAM_RANK_SINGLE)
     ^
drivers/gpu/drm/i915/i915_drv.c:1013:12: note: ‘ch0_rank’ was declared here
  enum rank ch0_rank, ch1_rank;
            ^
  LD [M]  drivers/ssb/ssb.o
  LD      drivers/pnp/pnpacpi/pnp.o
  LD      drivers/gpu/drm/drm.o
  LD      drivers/pnp/pnpacpi/built-in.o
  LD      drivers/pnp/built-in.o
  LD      drivers/pci/pcie/aer/aerdriver.o
  LD      drivers/pci/pcie/aer/built-in.o
  LD      drivers/pci/pcie/built-in.o
  LD [M]  drivers/mmc/core/mmc_core.o
  LD      drivers/mmc/built-in.o
  LD      drivers/video/fbdev/core/fb.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      drivers/tty/serial/8250/8250.o
cc1: all warnings being treated as errors
  LD      drivers/usb/storage/usb-storage.o
scripts/Makefile.build:290: recipe for target 'drivers/gpu/drm/i915/i915_drv.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_drv.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  AR      lib/lib.a
  LD      drivers/usb/storage/built-in.o
  EXPORTS lib/lib-ksyms.o
  LD      drivers/scsi/scsi_mod.o
  LD      lib/built-in.o
  LD [M]  drivers/usb/serial/usbserial.o
  LD [M]  drivers/misc/mei/mei-me.o
  LD      drivers/misc/built-in.o
  LD      drivers/video/fbdev/built-in.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      drivers/usb/gadget/libcomposite.o
  LD      drivers/usb/gadget/built-in.o
  LD [M]  drivers/net/ethernet/intel/igbvf/igbvf.o
  LD      drivers/thermal/thermal_sys.o
  LD      drivers/thermal/built-in.o
  LD [M]  drivers/net/ethernet/broadcom/genet/genet.o
  LD      net/ipv4/built-in.o
  LD      drivers/pci/built-in.o
  LD      net/ipv6/ipv6.o
  LD      drivers/spi/built-in.o
  LD      drivers/iommu/built-in.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      net/ipv6/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/video/console/built-in.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/video/built-in.o
  LD      drivers/scsi/built-in.o
  LD      drivers/tty/vt/built-in.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      drivers/tty/built-in.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      fs/btrfs/btrfs.o
  LD      arch/x86/kernel/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      fs/btrfs/built-in.o
  LD      arch/x86/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  LD      drivers/usb/core/built-in.o
  LD      net/core/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      net/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
  LD      fs/ext4/ext4.o
  LD      fs/ext4/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      fs/built-in.o
  LD      drivers/usb/built-in.o
  LD      drivers/md/md-mod.o
  LD      drivers/md/built-in.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
scripts/Makefile.build:475: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:475: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:475: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:985: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH v5 1/8] drm/i915/skl: Add variables to check x_tile and y_tile

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> This patch adds variable to check for X_tiled & y_tiled planes,
> instead
> of always checking against framebuffer-modifiers.
> 
> Changes:
>  - Created separate patch as per Paulo's comment
>  - Added x_tiled variable as well
> Changes since V2:
>  - Incorporate Paulo's comments
>  - Rebase
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index e54708d..8908736 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3592,13 +3592,18 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	struct intel_atomic_state *state =
>  		to_intel_atomic_state(cstate->base.state);
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> +	bool y_tiled, x_tiled;
>  
>  	if (latency == 0 || !cstate->base.active || !intel_pstate-
> >base.visible) {
>  		*enabled = false;
>  		return 0;
>  	}
>  
> -	if (apply_memory_bw_wa && fb->modifier ==
> I915_FORMAT_MOD_X_TILED)
> +	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> +				fb->modifier ==
> I915_FORMAT_MOD_Yf_TILED;

Unusual indentation here. I can fix this while applying the patch.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> +	x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
> +
> +	if (apply_memory_bw_wa && x_tiled)
>  		latency += 15;
>  
>  	width = drm_rect_width(&intel_pstate->base.src) >> 16;
> @@ -3637,16 +3642,15 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  		y_min_scanlines *= 2;
>  
>  	plane_bytes_per_line = width * cpp;
> -	if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> -	    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> +	if (y_tiled) {
>  		plane_blocks_per_line =
>  		      DIV_ROUND_UP(plane_bytes_per_line *
> y_min_scanlines, 512);
>  		plane_blocks_per_line /= y_min_scanlines;
> -	} else if (fb->modifier == DRM_FORMAT_MOD_NONE) {
> +	} else if (x_tiled) {
> +		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512);
> +	} else {
>  		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512)
>  					+ 1;
> -	} else {
> -		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512);
>  	}
>  
>  	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
> @@ -3657,8 +3661,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  
>  	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
>  
> -	if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> -	    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> +	if (y_tiled) {
>  		selected_result = max(method2, y_tile_minimum);
>  	} else {
>  		if ((cpp * cstate->base.adjusted_mode.crtc_htotal /
> 512 < 1) &&
> @@ -3674,8 +3677,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	res_lines = DIV_ROUND_UP(selected_result,
> plane_blocks_per_line);
>  
>  	if (level >= 1 && level <= 7) {
> -		if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> -		    fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> +		if (y_tiled) {
>  			res_blocks += y_tile_minimum;
>  			res_lines += y_min_scanlines;
>  		} else {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/8] drm/i915/bxt: IPC WA for Broxton

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> If IPC is enabled in BXT, display underruns are observed.
> WA: The Line Time programmed in the WM_LINETIME register should be
> half of the actual calculated Line Time.
> 
> Programmed Line Time = 1/2*Calculated Line Time
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++--
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 445fec9..1b0a589 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1243,6 +1243,8 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	dev_priv->ipc_enabled = false;
> +
>  	/* Everything is in place, we can now relax! */
>  	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>  		 driver.name, driver.major, driver.minor,
> driver.patchlevel,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 63c0ea0..394d7ea 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2128,6 +2128,9 @@ struct drm_i915_private {
>  	/* perform PHY state sanity checks? */
>  	bool chv_phy_assert[2];
>  
> +	/* To apply IPC WA */

The comment is not exactly necessary since we can just look where
ipc_enabled is used to find out why it's here. As a bonus, whenever we
add more ipc_enabled readers we won't need to update the comment.


> +	bool ipc_enabled;
> +
>  	/* Used to save the pipe-to-encoder mapping for audio */
>  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 8908736..7090a7c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3769,7 +3769,10 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  static uint32_t
>  skl_compute_linetime_wm(struct intel_crtc_state *cstate)
>  {
> +	struct drm_atomic_state *state = cstate->base.state;
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	uint32_t pixel_rate;
> +	uint32_t linetime_wm;
>  
>  	if (!cstate->base.active)
>  		return 0;
> @@ -3779,8 +3782,12 @@ skl_compute_linetime_wm(struct
> intel_crtc_state *cstate)
>  	if (WARN_ON(pixel_rate == 0))
>  		return 0;
>  
> -	return DIV_ROUND_UP(8 * cstate-
> >base.adjusted_mode.crtc_htotal * 1000,
> -			    pixel_rate);
> +	linetime_wm = DIV_ROUND_UP(8 * cstate-
> >base.adjusted_mode.crtc_htotal *
> +						1000, pixel_rate);

Weird indentation here again. Is your editor configured to show tabs as
4 spaces?

> +	if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)

Once we merge Gemnilake, we'll need to write a patch checking for it
here.


Anyway, my two suggestions can be fixed while merging, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +		linetime_wm = DIV_ROUND_UP(linetime_wm, 2);
> +
> +	return linetime_wm;
>  }
>  
>  static void skl_compute_transition_wm(struct intel_crtc_state
> *cstate,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 3/8] drm/i915/kbl: IPC workaround for kabylake

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> IPC (Isoch Priority Control) may cause underflows.
> 
> KBL WA: When IPC is enabled, watermark latency values must be
> increased
> by 4us across all levels. This brings level 0 up to 6us.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 7090a7c..df39b50 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3599,6 +3599,10 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  		return 0;
>  	}
>  
> +	/* IPC WA for kabylake */
> +	if (IS_KABYLAKE(dev_priv) && dev_priv->ipc_enabled)
> +		latency += 4;
> +

Nitpick: put this a little below so it's next to the other code that
changes the latency.

With or without this:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
>  				fb->modifier ==
> I915_FORMAT_MOD_Yf_TILED;
>  	x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> This patch adds IPC support for platforms. This patch enables IPC
> only for BXT/KBL platform as for SKL recommendation is to keep is
> disabled.
> IPC (Isochronous Priority Control) is the hardware feature, which
> dynamically controles the memory read priority of Display.
> 
> When IPC is enabled, plane read requests are sent at high priority
> until
> filling above the transition watermark, then the requests are sent at
> lower priority until dropping below the level 0 watermark.
> The lower priority requests allow other memory clients to have better
> memory access. When IPC is disabled, all plane read requests are sent
> at
> high priority.
> 
> Changes since V1:
>  - Remove commandline parameter to disable ipc
>  - Address Paulo's comments
> Changes since V2:
>  - Address review comments
>  - Set ipc_enabled flag
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  1 +
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
>  4 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 1b0a589..4074601 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  	intel_runtime_pm_enable(dev_priv);
>  
>  	dev_priv->ipc_enabled = false;
> +	intel_enable_ipc(dev_priv);

So now we have to places that touch dev_priv->ipc_enabled. This one and
intel_enable_ipc(). Please move that "dev_priv->ipc_enabled = false"
line to inside intel_enable_ipc(). It's much easier to read the code
when there's a single function responsible for setting the appropriate
value to a variable.


Besides, my understanding of your discussion with Maarten in the last
revision of this patch was that we needed to change where
intel_enable_ipc() is called in order to make sure the bit stays
enabled after suspend/resume. If that's not needed, why is it not
needed?

>  
>  	/* Everything is in place, we can now relax! */
>  	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index c70c07a..300418a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6076,6 +6076,7 @@ enum {
>  #define  DISP_FBC_WM_DIS		(1<<15)
>  #define DISP_ARB_CTL2	_MMIO(0x45004)
>  #define  DISP_DATA_PARTITION_5_6	(1<<6)
> +#define  DISP_IPC_ENABLE		(1<<3)
>  #define DBUF_CTL	_MMIO(0x45008)
>  #define  DBUF_POWER_REQUEST		(1<<31)
>  #define  DBUF_POWER_STATE		(1<<30)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index cd132c2..ad542a2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const struct
> skl_ddb_entry **entries,
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>  static inline int intel_enable_rc6(void)
>  {
>  	return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index df39b50..d8090aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct intel_crtc
> *crtc)
>  		dev_priv->display.update_wm(crtc);
>  }
>  
> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
> +		return;
> +
> +	val = I915_READ(DISP_ARB_CTL2);
> +
> +	val |= DISP_IPC_ENABLE;
> +
> +	I915_WRITE(DISP_ARB_CTL2, val);
> +	dev_priv->ipc_enabled = true;
> +}
> +
>  /*
>   * Lock protecting IPS related data structures
>   */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 6/8] drm/i915: Add intel_atomic_get_existing_crtc_state function

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> This patch Adds a function to extract intel_crtc_state from the
> atomic_state, if not available it returns NULL.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index ad542a2..db75773 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1794,6 +1794,20 @@ intel_atomic_get_crtc_state(struct
> drm_atomic_state *state,
>  	return to_intel_crtc_state(crtc_state);
>  }
>  
> +static inline struct intel_crtc_state *
> +intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
> +				      struct intel_crtc *crtc)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> &crtc->base);
> +
> +	if (crtc_state)
> +		return to_intel_crtc_state(crtc_state);
> +	else
> +		return NULL;
> +}
> +
>  static inline struct intel_plane_state *
>  intel_atomic_get_existing_plane_state(struct drm_atomic_state
> *state,
>  				      struct intel_plane *plane)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 5/8] drm/i915/skl+: change WM calc to fixed point 16.16

[Paulo Zanoni]

Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> This patch changes Watermak calculation to fixed point calculation.
> Problem with current calculation is during plane_blocks_per_line
> calculation we divide intermediate blocks with min_scanlines and
> takes floor of the result because of integer operation.
> hence we end-up assigning less blocks than required. Which leads to
> flickers.
> 
> Changes since V1:
>  - Add fixed point data type as per Paulo's review
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 84
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++-----------
> ---
>  2 files changed, 125 insertions(+), 29 deletions(-)

First of all, there's non-standard indentation all over the code. When
a statement is broken into multiple lines, sometimes there's way too
many tabs, sometimes tabs/spaces are missing.


> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 394d7ea..20691e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -119,6 +119,90 @@ bool __i915_inject_load_failure(const char
> *func, int line);
>  #define i915_inject_load_failure() \
>  	__i915_inject_load_failure(__func__, __LINE__)
>  
> +typedef struct {
> +	uint32_t val;
> +} uint_fixed_16_16_t;
> +
> +#define FP_16_16_MAX ({ \
> +		uint_fixed_16_16_t fp; \
> +		fp.val = UINT_MAX; \
> +		fp; \
> +})
> +
> +static inline uint_fixed_16_16_t u32_to_fp_16_16(uint32_t val)

In the type name the patch uses "fixed", but in some functions it uses
"fp". I know that fp is supposed to help function names shorter, but
IMHO we should stick with the same word all the time. My suggestion is
to keep "fixed" everywhere, because the definition of "fp" is not
obvious to everybody (fixed point? floating point? frame pointer?
functional programming?).


> +{
> +	uint_fixed_16_16_t fp;
> +
> +	WARN_ON(val >> 16);
> +
> +	fp.val = val << 16;
> +	return fp;
> +}
> +
> +static inline uint32_t fp_16_16_to_u32_round_up(uint_fixed_16_16_t
> fp)
> +{
> +	return DIV_ROUND_UP(fp.val, 1 << 16);
> +}
> +
> +static inline uint32_t fp_16_16_to_u32(uint_fixed_16_16_t fp)
> +{
> +	return (fp.val / 1 << 16);

This is dividing by 1 and then shifting. Doesn't look correct.


> +}
> +
> +static inline uint_fixed_16_16_t min_fp_16_16(uint_fixed_16_16_t
> min1,
> +						uint_fixed_16_16_t
> min2)
> +{
> +	uint_fixed_16_16_t min;
> +
> +	min.val = min(min1.val, min2.val);
> +	return min;
> +}
> +
> +static inline uint_fixed_16_16_t max_fp_16_16(uint_fixed_16_16_t
> max1,
> +						uint_fixed_16_16_t
> max2)
> +{
> +	uint_fixed_16_16_t max;
> +
> +	max.val = max(max1.val, max2.val);
> +	return max;
> +}
> +
> +static inline uint_fixed_16_16_t fp_16_16_div_round_up(uint32_t val,
> uint32_t d)
> +{
> +	uint_fixed_16_16_t fp, res;
> +
> +	fp = u32_to_fp_16_16(val);
> +	res.val = DIV_ROUND_UP(fp.val, d);
> +	return res;
> +}
> +
> +static inline uint_fixed_16_16_t fp_16_16_div_round_up_ull(uint64_t
> val,
> +								uint
> 32_t d)

I'd end the name with "u64" instead of "ull" since it's an uint64_t
type, not unsigned long long.


> +{
> +	uint_fixed_16_16_t res;
> +	uint64_t interm_val;
> +
> +	WARN_ON(val >> 48);
> +	val <<= 16;
> +	interm_val = DIV_ROUND_UP_ULL(val, d);
> +	WARN_ON(interm_val >> 32);
> +	res.val = (uint32_t) interm_val;
> +
> +	return res;
> +}
> +
> +static inline uint_fixed_16_16_t mul_fp_16_16_u32(uint32_t val,
> +						uint_fixed_16_16_t
> mul)
> +{
> +	uint64_t intermediate_val;
> +	uint_fixed_16_16_t fp;
> +
> +	intermediate_val = (uint64_t) val * mul.val;
> +	WARN_ON(intermediate_val >> 32);
> +	fp.val = (uint32_t) intermediate_val;
> +	return fp;
> +}
> +
>  static inline const char *yesno(bool v)
>  {
>  	return v ? "yes" : "no";
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index d8090aa..7c788ac 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3515,33 +3515,37 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>   * for the read latency) and cpp should always be <= 8, so that
>   * should allow pixel_rate up to ~2 GHz which seems sufficient since
> max
>   * 2xcdclk is 1350 MHz and the pixel rate should never exceed that.
> + * Both Method1 & Method2 returns fixedpoint 16.16 output

This line is not needed anymore: function signatures are saying it now.


>  */
> -static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp,
> uint32_t latency)
> +static uint_fixed_16_16_t skl_wm_method1(uint32_t pixel_rate,
> uint8_t cpp,
> +					 uint32_t latency)
>  {
> -	uint32_t wm_intermediate_val, ret;
> +	uint64_t wm_intermediate_val;

Why are we using uint64_t now? If we weren't overflowing before, we
shouldn't be overflowing now, should we? Is this a separate bug fix?

Everything else looks good. Thanks for the improvements!


> +	uint_fixed_16_16_t ret;
>  
>  	if (latency == 0)
> -		return UINT_MAX;
> -
> -	wm_intermediate_val = latency * pixel_rate * cpp / 512;
> -	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
> +		return FP_16_16_MAX;
>  
> +	wm_intermediate_val = latency * pixel_rate * cpp;
> +	ret = fp_16_16_div_round_up_ull(wm_intermediate_val, 1000 *
> 512);
>  	return ret;
>  }
>  
> -static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t
> pipe_htotal,
> -			       uint32_t latency, uint32_t
> plane_blocks_per_line)
> +static uint_fixed_16_16_t skl_wm_method2(uint32_t pixel_rate,
> +			uint32_t pipe_htotal,
> +			uint32_t latency,
> +			uint_fixed_16_16_t plane_blocks_per_line)
>  {
> -	uint32_t ret;
>  	uint32_t wm_intermediate_val;
> +	uint_fixed_16_16_t ret;
>  
>  	if (latency == 0)
> -		return UINT_MAX;
> +		return FP_16_16_MAX;
>  
>  	wm_intermediate_val = latency * pixel_rate;
> -	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000)
> *
> -				plane_blocks_per_line;
> -
> +	wm_intermediate_val = DIV_ROUND_UP(wm_intermediate_val,
> +							pipe_htotal
> * 1000);
> +	ret = mul_fp_16_16_u32(wm_intermediate_val,
> plane_blocks_per_line);
>  	return ret;
>  }
>  
> @@ -3581,14 +3585,17 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	struct drm_plane_state *pstate = &intel_pstate->base;
>  	struct drm_framebuffer *fb = pstate->fb;
>  	uint32_t latency = dev_priv->wm.skl_latency[level];
> -	uint32_t method1, method2;
> -	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> +	uint_fixed_16_16_t method1, method2;
> +	uint_fixed_16_16_t plane_blocks_per_line;
> +	uint_fixed_16_16_t selected_result;
> +	uint32_t interm_pbpl;
> +	uint32_t plane_bytes_per_line;
>  	uint32_t res_blocks, res_lines;
> -	uint32_t selected_result;
>  	uint8_t cpp;
>  	uint32_t width = 0, height = 0;
>  	uint32_t plane_pixel_rate;
> -	uint32_t y_tile_minimum, y_min_scanlines;
> +	uint_fixed_16_16_t y_tile_minimum;
> +	uint32_t y_min_scanlines;
>  	struct intel_atomic_state *state =
>  		to_intel_atomic_state(cstate->base.state);
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> @@ -3647,14 +3654,16 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  
>  	plane_bytes_per_line = width * cpp;
>  	if (y_tiled) {
> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line *
> +						y_min_scanlines,
> 512);
>  		plane_blocks_per_line =
> -		      DIV_ROUND_UP(plane_bytes_per_line *
> y_min_scanlines, 512);
> -		plane_blocks_per_line /= y_min_scanlines;
> +		      fp_16_16_div_round_up(interm_pbpl,
> y_min_scanlines);
>  	} else if (x_tiled) {
> -		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512);
> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line,
> 512);
> +		plane_blocks_per_line =
> u32_to_fp_16_16(interm_pbpl);
>  	} else {
> -		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512)
> -					+ 1;
> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line,
> 512) + 1;
> +		plane_blocks_per_line =
> u32_to_fp_16_16(interm_pbpl);
>  	}
>  
>  	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
> @@ -3663,26 +3672,29 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  				 latency,
>  				 plane_blocks_per_line);
>  
> -	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
> +	y_tile_minimum = mul_fp_16_16_u32(y_min_scanlines,
> +						plane_blocks_per_lin
> e);
>  
>  	if (y_tiled) {
> -		selected_result = max(method2, y_tile_minimum);
> +		selected_result = max_fp_16_16(method2,
> y_tile_minimum);
>  	} else {
>  		if ((cpp * cstate->base.adjusted_mode.crtc_htotal /
> 512 < 1) &&
>  		    (plane_bytes_per_line / 512 < 1))
>  			selected_result = method2;
> -		else if ((ddb_allocation / plane_blocks_per_line) >=
> 1)
> -			selected_result = min(method1, method2);
> +		else if ((ddb_allocation /
> +			fp_16_16_to_u32_round_up(plane_blocks_per_li
> ne)) >= 1)
> +			selected_result = min_fp_16_16(method1,
> method2);
>		else
>  			selected_result = method1;
>  	}
>  
> -	res_blocks = selected_result + 1;
> -	res_lines = DIV_ROUND_UP(selected_result,
> plane_blocks_per_line);
> +	res_blocks = fp_16_16_to_u32_round_up(selected_result) + 1;
> +	res_lines = DIV_ROUND_UP(selected_result.val,
> +					plane_blocks_per_line.val);
>  
>  	if (level >= 1 && level <= 7) {
>  		if (y_tiled) {
> -			res_blocks += y_tile_minimum;
> +			res_blocks +=
> fp_16_16_to_u32_round_up(y_tile_minimum);
>  			res_lines += y_min_scanlines;
>  		} else {
>  			res_blocks++;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Mahesh Kumar]

Hi,


On Tuesday 22 November 2016 12:16 AM, Paulo Zanoni wrote:
> Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
>> This patch adds IPC support for platforms. This patch enables IPC
>> only for BXT/KBL platform as for SKL recommendation is to keep is
>> disabled.
>> IPC (Isochronous Priority Control) is the hardware feature, which
>> dynamically controles the memory read priority of Display.
>>
>> When IPC is enabled, plane read requests are sent at high priority
>> until
>> filling above the transition watermark, then the requests are sent at
>> lower priority until dropping below the level 0 watermark.
>> The lower priority requests allow other memory clients to have better
>> memory access. When IPC is disabled, all plane read requests are sent
>> at
>> high priority.
>>
>> Changes since V1:
>>   - Remove commandline parameter to disable ipc
>>   - Address Paulo's comments
>> Changes since V2:
>>   - Address review comments
>>   - Set ipc_enabled flag
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c  |  1 +
>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
>>   4 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 1b0a589..4074601 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>   	intel_runtime_pm_enable(dev_priv);
>>   
>>   	dev_priv->ipc_enabled = false;
>> +	intel_enable_ipc(dev_priv);
> So now we have to places that touch dev_priv->ipc_enabled. This one and
> intel_enable_ipc(). Please move that "dev_priv->ipc_enabled = false"
> line to inside intel_enable_ipc(). It's much easier to read the code
> when there's a single function responsible for setting the appropriate
> value to a variable.
>
>
> Besides, my understanding of your discussion with Maarten in the last
> revision of this patch was that we needed to change where
> intel_enable_ipc() is called in order to make sure the bit stays
> enabled after suspend/resume. If that's not needed, why is it not
> needed?
We don't overwrite DISP_ARB_CTL2 register during suspend/resume
So there will not be any impact of it & handling during suspend/resume 
is not needed.
thanks for review

Regards,
-Mahesh
>>   
>>   	/* Everything is in place, we can now relax! */
>>   	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index c70c07a..300418a 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6076,6 +6076,7 @@ enum {
>>   #define  DISP_FBC_WM_DIS		(1<<15)
>>   #define DISP_ARB_CTL2	_MMIO(0x45004)
>>   #define  DISP_DATA_PARTITION_5_6	(1<<6)
>> +#define  DISP_IPC_ENABLE		(1<<3)
>>   #define DBUF_CTL	_MMIO(0x45008)
>>   #define  DBUF_POWER_REQUEST		(1<<31)
>>   #define  DBUF_POWER_STATE		(1<<30)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index cd132c2..ad542a2 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const struct
>> skl_ddb_entry **entries,
>>   uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
>> *pipe_config);
>>   bool ilk_disable_lp_wm(struct drm_device *dev);
>>   int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
>> enable_rc6);
>> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>>   static inline int intel_enable_rc6(void)
>>   {
>>   	return i915.enable_rc6;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index df39b50..d8090aa 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct intel_crtc
>> *crtc)
>>   		dev_priv->display.update_wm(crtc);
>>   }
>>   
>> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
>> +{
>> +	u32 val;
>> +
>> +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
>> +		return;
>> +
>> +	val = I915_READ(DISP_ARB_CTL2);
>> +
>> +	val |= DISP_IPC_ENABLE;
>> +
>> +	I915_WRITE(DISP_ARB_CTL2, val);
>> +	dev_priv->ipc_enabled = true;
>> +}
>> +
>>   /*
>>    * Lock protecting IPS related data structures
>>    */

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

Re: [PATCH v5 5/8] drm/i915/skl+: change WM calc to fixed point 16.16

[Mahesh Kumar]

On Tuesday 22 November 2016 06:12 PM, Paulo Zanoni wrote:
> Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
>> This patch changes Watermak calculation to fixed point calculation.
>> Problem with current calculation is during plane_blocks_per_line
>> calculation we divide intermediate blocks with min_scanlines and
>> takes floor of the result because of integer operation.
>> hence we end-up assigning less blocks than required. Which leads to
>> flickers.
>>
>> Changes since V1:
>>   - Add fixed point data type as per Paulo's review
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h | 84
>> +++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++-----------
>> ---
>>   2 files changed, 125 insertions(+), 29 deletions(-)
> First of all, there's non-standard indentation all over the code. When
> a statement is broken into multiple lines, sometimes there's way too
> many tabs, sometimes tabs/spaces are missing.
looks like some editor setting went wrong, will fix it.
>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 394d7ea..20691e9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -119,6 +119,90 @@ bool __i915_inject_load_failure(const char
>> *func, int line);
>>   #define i915_inject_load_failure() \
>>   	__i915_inject_load_failure(__func__, __LINE__)
>>   
>> +typedef struct {
>> +	uint32_t val;
>> +} uint_fixed_16_16_t;
>> +
>> +#define FP_16_16_MAX ({ \
>> +		uint_fixed_16_16_t fp; \
>> +		fp.val = UINT_MAX; \
>> +		fp; \
>> +})
>> +
>> +static inline uint_fixed_16_16_t u32_to_fp_16_16(uint32_t val)
> In the type name the patch uses "fixed", but in some functions it uses
> "fp". I know that fp is supposed to help function names shorter, but
> IMHO we should stick with the same word all the time. My suggestion is
> to keep "fixed" everywhere, because the definition of "fp" is not
> obvious to everybody (fixed point? floating point? frame pointer?
> functional programming?).
>
>
>> +{
>> +	uint_fixed_16_16_t fp;
>> +
>> +	WARN_ON(val >> 16);
>> +
>> +	fp.val = val << 16;
>> +	return fp;
>> +}
>> +
>> +static inline uint32_t fp_16_16_to_u32_round_up(uint_fixed_16_16_t
>> fp)
>> +{
>> +	return DIV_ROUND_UP(fp.val, 1 << 16);
>> +}
>> +
>> +static inline uint32_t fp_16_16_to_u32(uint_fixed_16_16_t fp)
>> +{
>> +	return (fp.val / 1 << 16);
> This is dividing by 1 and then shifting. Doesn't look correct.

	It should have been (fp.val / (1 << 16))

>
>> +}
>> +
>> +static inline uint_fixed_16_16_t min_fp_16_16(uint_fixed_16_16_t
>> min1,
>> +						uint_fixed_16_16_t
>> min2)
>> +{
>> +	uint_fixed_16_16_t min;
>> +
>> +	min.val = min(min1.val, min2.val);
>> +	return min;
>> +}
>> +
>> +static inline uint_fixed_16_16_t max_fp_16_16(uint_fixed_16_16_t
>> max1,
>> +						uint_fixed_16_16_t
>> max2)
>> +{
>> +	uint_fixed_16_16_t max;
>> +
>> +	max.val = max(max1.val, max2.val);
>> +	return max;
>> +}
>> +
>> +static inline uint_fixed_16_16_t fp_16_16_div_round_up(uint32_t val,
>> uint32_t d)
>> +{
>> +	uint_fixed_16_16_t fp, res;
>> +
>> +	fp = u32_to_fp_16_16(val);
>> +	res.val = DIV_ROUND_UP(fp.val, d);
>> +	return res;
>> +}
>> +
>> +static inline uint_fixed_16_16_t fp_16_16_div_round_up_ull(uint64_t
>> val,
>> +								uint
>> 32_t d)
> I'd end the name with "u64" instead of "ull" since it's an uint64_t
> type, not unsigned long long.
>
>
>> +{
>> +	uint_fixed_16_16_t res;
>> +	uint64_t interm_val;
>> +
>> +	WARN_ON(val >> 48);
>> +	val <<= 16;
>> +	interm_val = DIV_ROUND_UP_ULL(val, d);
>> +	WARN_ON(interm_val >> 32);
>> +	res.val = (uint32_t) interm_val;
>> +
>> +	return res;
>> +}
>> +
>> +static inline uint_fixed_16_16_t mul_fp_16_16_u32(uint32_t val,
>> +						uint_fixed_16_16_t
>> mul)
>> +{
>> +	uint64_t intermediate_val;
>> +	uint_fixed_16_16_t fp;
>> +
>> +	intermediate_val = (uint64_t) val * mul.val;
>> +	WARN_ON(intermediate_val >> 32);
>> +	fp.val = (uint32_t) intermediate_val;
>> +	return fp;
>> +}
>> +
>>   static inline const char *yesno(bool v)
>>   {
>>   	return v ? "yes" : "no";
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index d8090aa..7c788ac 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3515,33 +3515,37 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>> *cstate,
>>    * for the read latency) and cpp should always be <= 8, so that
>>    * should allow pixel_rate up to ~2 GHz which seems sufficient since
>> max
>>    * 2xcdclk is 1350 MHz and the pixel rate should never exceed that.
>> + * Both Method1 & Method2 returns fixedpoint 16.16 output
> This line is not needed anymore: function signatures are saying it now.
>
>
>>   */
>> -static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp,
>> uint32_t latency)
>> +static uint_fixed_16_16_t skl_wm_method1(uint32_t pixel_rate,
>> uint8_t cpp,
>> +					 uint32_t latency)
>>   {
>> -	uint32_t wm_intermediate_val, ret;
>> +	uint64_t wm_intermediate_val;
> Why are we using uint64_t now? If we weren't overflowing before, we
> shouldn't be overflowing now, should we? Is this a separate bug fix?
no, it should still not overflow, it was just a precaution, will address 
this
thanks,

Regards,
-Mahesh
>
> Everything else looks good. Thanks for the improvements!
>
>
>> +	uint_fixed_16_16_t ret;
>>   
>>   	if (latency == 0)
>> -		return UINT_MAX;
>> -
>> -	wm_intermediate_val = latency * pixel_rate * cpp / 512;
>> -	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
>> +		return FP_16_16_MAX;
>>   
>> +	wm_intermediate_val = latency * pixel_rate * cpp;
>> +	ret = fp_16_16_div_round_up_ull(wm_intermediate_val, 1000 *
>> 512);
>>   	return ret;
>>   }
>>   
>> -static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t
>> pipe_htotal,
>> -			       uint32_t latency, uint32_t
>> plane_blocks_per_line)
>> +static uint_fixed_16_16_t skl_wm_method2(uint32_t pixel_rate,
>> +			uint32_t pipe_htotal,
>> +			uint32_t latency,
>> +			uint_fixed_16_16_t plane_blocks_per_line)
>>   {
>> -	uint32_t ret;
>>   	uint32_t wm_intermediate_val;
>> +	uint_fixed_16_16_t ret;
>>   
>>   	if (latency == 0)
>> -		return UINT_MAX;
>> +		return FP_16_16_MAX;
>>   
>>   	wm_intermediate_val = latency * pixel_rate;
>> -	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000)
>> *
>> -				plane_blocks_per_line;
>> -
>> +	wm_intermediate_val = DIV_ROUND_UP(wm_intermediate_val,
>> +							pipe_htotal
>> * 1000);
>> +	ret = mul_fp_16_16_u32(wm_intermediate_val,
>> plane_blocks_per_line);
>>   	return ret;
>>   }
>>   
>> @@ -3581,14 +3585,17 @@ static int skl_compute_plane_wm(const struct
>> drm_i915_private *dev_priv,
>>   	struct drm_plane_state *pstate = &intel_pstate->base;
>>   	struct drm_framebuffer *fb = pstate->fb;
>>   	uint32_t latency = dev_priv->wm.skl_latency[level];
>> -	uint32_t method1, method2;
>> -	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> +	uint_fixed_16_16_t method1, method2;
>> +	uint_fixed_16_16_t plane_blocks_per_line;
>> +	uint_fixed_16_16_t selected_result;
>> +	uint32_t interm_pbpl;
>> +	uint32_t plane_bytes_per_line;
>>   	uint32_t res_blocks, res_lines;
>> -	uint32_t selected_result;
>>   	uint8_t cpp;
>>   	uint32_t width = 0, height = 0;
>>   	uint32_t plane_pixel_rate;
>> -	uint32_t y_tile_minimum, y_min_scanlines;
>> +	uint_fixed_16_16_t y_tile_minimum;
>> +	uint32_t y_min_scanlines;
>>   	struct intel_atomic_state *state =
>>   		to_intel_atomic_state(cstate->base.state);
>>   	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
>> @@ -3647,14 +3654,16 @@ static int skl_compute_plane_wm(const struct
>> drm_i915_private *dev_priv,
>>   
>>   	plane_bytes_per_line = width * cpp;
>>   	if (y_tiled) {
>> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line *
>> +						y_min_scanlines,
>> 512);
>>   		plane_blocks_per_line =
>> -		      DIV_ROUND_UP(plane_bytes_per_line *
>> y_min_scanlines, 512);
>> -		plane_blocks_per_line /= y_min_scanlines;
>> +		      fp_16_16_div_round_up(interm_pbpl,
>> y_min_scanlines);
>>   	} else if (x_tiled) {
>> -		plane_blocks_per_line =
>> DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line,
>> 512);
>> +		plane_blocks_per_line =
>> u32_to_fp_16_16(interm_pbpl);
>>   	} else {
>> -		plane_blocks_per_line =
>> DIV_ROUND_UP(plane_bytes_per_line, 512)
>> -					+ 1;
>> +		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line,
>> 512) + 1;
>> +		plane_blocks_per_line =
>> u32_to_fp_16_16(interm_pbpl);
>>   	}
>>   
>>   	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
>> @@ -3663,26 +3672,29 @@ static int skl_compute_plane_wm(const struct
>> drm_i915_private *dev_priv,
>>   				 latency,
>>   				 plane_blocks_per_line);
>>   
>> -	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
>> +	y_tile_minimum = mul_fp_16_16_u32(y_min_scanlines,
>> +						plane_blocks_per_lin
>> e);
>>   
>>   	if (y_tiled) {
>> -		selected_result = max(method2, y_tile_minimum);
>> +		selected_result = max_fp_16_16(method2,
>> y_tile_minimum);
>>   	} else {
>>   		if ((cpp * cstate->base.adjusted_mode.crtc_htotal /
>> 512 < 1) &&
>>   		    (plane_bytes_per_line / 512 < 1))
>>   			selected_result = method2;
>> -		else if ((ddb_allocation / plane_blocks_per_line) >=
>> 1)
>> -			selected_result = min(method1, method2);
>> +		else if ((ddb_allocation /
>> +			fp_16_16_to_u32_round_up(plane_blocks_per_li
>> ne)) >= 1)
>> +			selected_result = min_fp_16_16(method1,
>> method2);
>> 		else
>>   			selected_result = method1;
>>   	}
>>   
>> -	res_blocks = selected_result + 1;
>> -	res_lines = DIV_ROUND_UP(selected_result,
>> plane_blocks_per_line);
>> +	res_blocks = fp_16_16_to_u32_round_up(selected_result) + 1;
>> +	res_lines = DIV_ROUND_UP(selected_result.val,
>> +					plane_blocks_per_line.val);
>>   
>>   	if (level >= 1 && level <= 7) {
>>   		if (y_tiled) {
>> -			res_blocks += y_tile_minimum;
>> +			res_blocks +=
>> fp_16_16_to_u32_round_up(y_tile_minimum);
>>   			res_lines += y_min_scanlines;
>>   		} else {
>>   			res_blocks++;

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

Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Paulo Zanoni]

Em Ter, 2016-11-22 às 19:05 +0530, Mahesh Kumar escreveu:
> Hi,
> 
> 
> On Tuesday 22 November 2016 12:16 AM, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
> > > 
> > > This patch adds IPC support for platforms. This patch enables IPC
> > > only for BXT/KBL platform as for SKL recommendation is to keep is
> > > disabled.
> > > IPC (Isochronous Priority Control) is the hardware feature, which
> > > dynamically controles the memory read priority of Display.
> > > 
> > > When IPC is enabled, plane read requests are sent at high
> > > priority
> > > until
> > > filling above the transition watermark, then the requests are
> > > sent at
> > > lower priority until dropping below the level 0 watermark.
> > > The lower priority requests allow other memory clients to have
> > > better
> > > memory access. When IPC is disabled, all plane read requests are
> > > sent
> > > at
> > > high priority.
> > > 
> > > Changes since V1:
> > >   - Remove commandline parameter to disable ipc
> > >   - Address Paulo's comments
> > > Changes since V2:
> > >   - Address review comments
> > >   - Set ipc_enabled flag
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.c  |  1 +
> > >   drivers/gpu/drm/i915/i915_reg.h  |  1 +
> > >   drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >   drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
> > >   4 files changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 1b0a589..4074601 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
> > > const struct pci_device_id *ent)
> > >   	intel_runtime_pm_enable(dev_priv);
> > >   
> > >   	dev_priv->ipc_enabled = false;
> > > +	intel_enable_ipc(dev_priv);
> > So now we have to places that touch dev_priv->ipc_enabled. This one
> > and
> > intel_enable_ipc(). Please move that "dev_priv->ipc_enabled =
> > false"
> > line to inside intel_enable_ipc(). It's much easier to read the
> > code
> > when there's a single function responsible for setting the
> > appropriate
> > value to a variable.
> > 
> > 
> > Besides, my understanding of your discussion with Maarten in the
> > last
> > revision of this patch was that we needed to change where
> > intel_enable_ipc() is called in order to make sure the bit stays
> > enabled after suspend/resume. If that's not needed, why is it not
> > needed?
> We don't overwrite DISP_ARB_CTL2 register during suspend/resume
> So there will not be any impact of it & handling during
> suspend/resume 
> is not needed.

Just to make sure I understood: any value that our driver sets to this
register will persist after a suspend/resume cycle? That's not what I
was expecting. Usually after a suspend/resume cycle a lot of registers
are reset to their default value, so our driver needs to write them all
again.

Can you also confirm whether this is true during runtime PM?

Thanks,
Paulo

> thanks for review
> 
> Regards,
> -Mahesh
> > 
> > > 
> > >   
> > >   	/* Everything is in place, we can now relax! */
> > >   	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor
> > > %d\n",
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index c70c07a..300418a 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6076,6 +6076,7 @@ enum {
> > >   #define  DISP_FBC_WM_DIS		(1<<15)
> > >   #define DISP_ARB_CTL2	_MMIO(0x45004)
> > >   #define  DISP_DATA_PARTITION_5_6	(1<<6)
> > > +#define  DISP_IPC_ENABLE		(1<<3)
> > >   #define DBUF_CTL	_MMIO(0x45008)
> > >   #define  DBUF_POWER_REQUEST		(1<<31)
> > >   #define  DBUF_POWER_STATE		(1<<30)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index cd132c2..ad542a2 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const
> > > struct
> > > skl_ddb_entry **entries,
> > >   uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > > *pipe_config);
> > >   bool ilk_disable_lp_wm(struct drm_device *dev);
> > >   int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> > > enable_rc6);
> > > +void intel_enable_ipc(struct drm_i915_private *dev_priv);
> > >   static inline int intel_enable_rc6(void)
> > >   {
> > >   	return i915.enable_rc6;
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index df39b50..d8090aa 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct
> > > intel_crtc
> > > *crtc)
> > >   		dev_priv->display.update_wm(crtc);
> > >   }
> > >   
> > > +void intel_enable_ipc(struct drm_i915_private *dev_priv)
> > > +{
> > > +	u32 val;
> > > +
> > > +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
> > > +		return;
> > > +
> > > +	val = I915_READ(DISP_ARB_CTL2);
> > > +
> > > +	val |= DISP_IPC_ENABLE;
> > > +
> > > +	I915_WRITE(DISP_ARB_CTL2, val);
> > > +	dev_priv->ipc_enabled = true;
> > > +}
> > > +
> > >   /*
> > >    * Lock protecting IPS related data structures
> > >    */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Mahesh Kumar]

Hi,


On Tuesday 22 November 2016 08:15 PM, Paulo Zanoni wrote:
> Em Ter, 2016-11-22 às 19:05 +0530, Mahesh Kumar escreveu:
>> Hi,
>>
>>
>> On Tuesday 22 November 2016 12:16 AM, Paulo Zanoni wrote:
>>> Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
>>>> This patch adds IPC support for platforms. This patch enables IPC
>>>> only for BXT/KBL platform as for SKL recommendation is to keep is
>>>> disabled.
>>>> IPC (Isochronous Priority Control) is the hardware feature, which
>>>> dynamically controles the memory read priority of Display.
>>>>
>>>> When IPC is enabled, plane read requests are sent at high
>>>> priority
>>>> until
>>>> filling above the transition watermark, then the requests are
>>>> sent at
>>>> lower priority until dropping below the level 0 watermark.
>>>> The lower priority requests allow other memory clients to have
>>>> better
>>>> memory access. When IPC is disabled, all plane read requests are
>>>> sent
>>>> at
>>>> high priority.
>>>>
>>>> Changes since V1:
>>>>    - Remove commandline parameter to disable ipc
>>>>    - Address Paulo's comments
>>>> Changes since V2:
>>>>    - Address review comments
>>>>    - Set ipc_enabled flag
>>>>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.c  |  1 +
>>>>    drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>>>    drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>>    drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
>>>>    4 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 1b0a589..4074601 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
>>>> const struct pci_device_id *ent)
>>>>    	intel_runtime_pm_enable(dev_priv);
>>>>    
>>>>    	dev_priv->ipc_enabled = false;
>>>> +	intel_enable_ipc(dev_priv);
>>> So now we have to places that touch dev_priv->ipc_enabled. This one
>>> and
>>> intel_enable_ipc(). Please move that "dev_priv->ipc_enabled =
>>> false"
>>> line to inside intel_enable_ipc(). It's much easier to read the
>>> code
>>> when there's a single function responsible for setting the
>>> appropriate
>>> value to a variable.
>>>
>>>
>>> Besides, my understanding of your discussion with Maarten in the
>>> last
>>> revision of this patch was that we needed to change where
>>> intel_enable_ipc() is called in order to make sure the bit stays
>>> enabled after suspend/resume. If that's not needed, why is it not
>>> needed?
>> We don't overwrite DISP_ARB_CTL2 register during suspend/resume
>> So there will not be any impact of it & handling during
>> suspend/resume
>> is not needed.
> Just to make sure I understood: any value that our driver sets to this
> register will persist after a suspend/resume cycle? That's not what I
> was expecting. Usually after a suspend/resume cycle a lot of registers
> are reset to their default value, so our driver needs to write them all
> again.
>
> Can you also confirm whether this is true during runtime PM?
My understanding is HW takes care of register save/restore during deeper 
power states(DC5/DC6 or DC9)
Will confirm this & comeback.

Regards,
-Mahesh
> Thanks,
> Paulo
>
>> thanks for review
>>
>> Regards,
>> -Mahesh
>>>>    
>>>>    	/* Everything is in place, we can now relax! */
>>>>    	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor
>>>> %d\n",
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>> b/drivers/gpu/drm/i915/i915_reg.h
>>>> index c70c07a..300418a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -6076,6 +6076,7 @@ enum {
>>>>    #define  DISP_FBC_WM_DIS		(1<<15)
>>>>    #define DISP_ARB_CTL2	_MMIO(0x45004)
>>>>    #define  DISP_DATA_PARTITION_5_6	(1<<6)
>>>> +#define  DISP_IPC_ENABLE		(1<<3)
>>>>    #define DBUF_CTL	_MMIO(0x45008)
>>>>    #define  DBUF_POWER_REQUEST		(1<<31)
>>>>    #define  DBUF_POWER_STATE		(1<<30)
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index cd132c2..ad542a2 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const
>>>> struct
>>>> skl_ddb_entry **entries,
>>>>    uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
>>>> *pipe_config);
>>>>    bool ilk_disable_lp_wm(struct drm_device *dev);
>>>>    int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
>>>> enable_rc6);
>>>> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>>>>    static inline int intel_enable_rc6(void)
>>>>    {
>>>>    	return i915.enable_rc6;
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>> index df39b50..d8090aa 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct
>>>> intel_crtc
>>>> *crtc)
>>>>    		dev_priv->display.update_wm(crtc);
>>>>    }
>>>>    
>>>> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
>>>> +		return;
>>>> +
>>>> +	val = I915_READ(DISP_ARB_CTL2);
>>>> +
>>>> +	val |= DISP_IPC_ENABLE;
>>>> +
>>>> +	I915_WRITE(DISP_ARB_CTL2, val);
>>>> +	dev_priv->ipc_enabled = true;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Lock protecting IPS related data structures
>>>>     */

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

Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Mahesh Kumar]

Hi,


On Tuesday 22 November 2016 08:15 PM, Paulo Zanoni wrote:
> Em Ter, 2016-11-22 às 19:05 +0530, Mahesh Kumar escreveu:
>> Hi,
>>
>>
>> On Tuesday 22 November 2016 12:16 AM, Paulo Zanoni wrote:
>>> Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
>>>> This patch adds IPC support for platforms. This patch enables IPC
>>>> only for BXT/KBL platform as for SKL recommendation is to keep is
>>>> disabled.
>>>> IPC (Isochronous Priority Control) is the hardware feature, which
>>>> dynamically controles the memory read priority of Display.
>>>>
>>>> When IPC is enabled, plane read requests are sent at high
>>>> priority
>>>> until
>>>> filling above the transition watermark, then the requests are
>>>> sent at
>>>> lower priority until dropping below the level 0 watermark.
>>>> The lower priority requests allow other memory clients to have
>>>> better
>>>> memory access. When IPC is disabled, all plane read requests are
>>>> sent
>>>> at
>>>> high priority.
>>>>
>>>> Changes since V1:
>>>>    - Remove commandline parameter to disable ipc
>>>>    - Address Paulo's comments
>>>> Changes since V2:
>>>>    - Address review comments
>>>>    - Set ipc_enabled flag
>>>>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.c  |  1 +
>>>>    drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>>>    drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>>    drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
>>>>    4 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 1b0a589..4074601 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
>>>> const struct pci_device_id *ent)
>>>>    	intel_runtime_pm_enable(dev_priv);
>>>>    
>>>>    	dev_priv->ipc_enabled = false;
>>>> +	intel_enable_ipc(dev_priv);
>>> So now we have to places that touch dev_priv->ipc_enabled. This one
>>> and
>>> intel_enable_ipc(). Please move that "dev_priv->ipc_enabled =
>>> false"
>>> line to inside intel_enable_ipc(). It's much easier to read the
>>> code
>>> when there's a single function responsible for setting the
>>> appropriate
>>> value to a variable.
>>>
>>>
>>> Besides, my understanding of your discussion with Maarten in the
>>> last
>>> revision of this patch was that we needed to change where
>>> intel_enable_ipc() is called in order to make sure the bit stays
>>> enabled after suspend/resume. If that's not needed, why is it not
>>> needed?
>> We don't overwrite DISP_ARB_CTL2 register during suspend/resume
>> So there will not be any impact of it & handling during
>> suspend/resume
>> is not needed.
> Just to make sure I understood: any value that our driver sets to this
> register will persist after a suspend/resume cycle? That's not what I
> was expecting. Usually after a suspend/resume cycle a lot of registers
> are reset to their default value, so our driver needs to write them all
> again.
>
> Can you also confirm whether this is true during runtime PM?
>
> Thanks,
> Paulo
>
After discussing with HW team, My understanding was not fully correct. 
HW/(DMC fw) does the restoring of registers only in case of DC5/6,
In case of DC9 sw need to take-care of restoring. Will add restore part 
in intel_display_resume & intel_runtime_resume.
above two places are correct places for restoring?

Regards,
-Mahesh

>> thanks for review
>>
>> Regards,
>> -Mahesh
>>>>    
>>>>    	/* Everything is in place, we can now relax! */
>>>>    	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor
>>>> %d\n",
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>> b/drivers/gpu/drm/i915/i915_reg.h
>>>> index c70c07a..300418a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -6076,6 +6076,7 @@ enum {
>>>>    #define  DISP_FBC_WM_DIS		(1<<15)
>>>>    #define DISP_ARB_CTL2	_MMIO(0x45004)
>>>>    #define  DISP_DATA_PARTITION_5_6	(1<<6)
>>>> +#define  DISP_IPC_ENABLE		(1<<3)
>>>>    #define DBUF_CTL	_MMIO(0x45008)
>>>>    #define  DBUF_POWER_REQUEST		(1<<31)
>>>>    #define  DBUF_POWER_STATE		(1<<30)
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index cd132c2..ad542a2 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const
>>>> struct
>>>> skl_ddb_entry **entries,
>>>>    uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
>>>> *pipe_config);
>>>>    bool ilk_disable_lp_wm(struct drm_device *dev);
>>>>    int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
>>>> enable_rc6);
>>>> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>>>>    static inline int intel_enable_rc6(void)
>>>>    {
>>>>    	return i915.enable_rc6;
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>> index df39b50..d8090aa 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct
>>>> intel_crtc
>>>> *crtc)
>>>>    		dev_priv->display.update_wm(crtc);
>>>>    }
>>>>    
>>>> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
>>>> +		return;
>>>> +
>>>> +	val = I915_READ(DISP_ARB_CTL2);
>>>> +
>>>> +	val |= DISP_IPC_ENABLE;
>>>> +
>>>> +	I915_WRITE(DISP_ARB_CTL2, val);
>>>> +	dev_priv->ipc_enabled = true;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Lock protecting IPS related data structures
>>>>     */

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