Re: [PATCH v3 8/9] drm/i915/bxt: set chicken bit as IPC y-tile WA

Mahesh Kumar <mahesh1.kumar@xxxxxxxxx> · Thu, 22 Sep 2016 15:13:00 +0530

Hi,


On Thursday 22 September 2016 01:53 AM, Paulo Zanoni wrote:
Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
From: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>

It implements the WA to enable IDLE_WAKEMEM bit of CHICKEN_DCPR_1
register for Broxton platform. When IPC is enabled & Y-tile is
enabled in any of the enabled plane, above bit should be set.
Without this WA system observes random hang.
The previous patch enabled the feature, and now this patch implements a
missing workaround for the feature. This is not the appropriate order,
since it can mean that the previous patch introduced a bug that was
fixed by this patch, and this potentially breaks bisectability. We only
enable the feature once we know all its workarounds are enabled and the
feature will Just Work*. So, normally, my suggestion would be to either
invert the patch order, or just make patches 7 and 8 be a single patch.
I can invert the order of patches, If below point seems OK.

But we have another problem, please see
commit 590e8ff04bc0182dce97228e5e352d6413d80456. What's the problem
with the current implementation? Why can't we just keep the bit as 1
forever?
mentioned commit is making IDLE_WAKEMEM bit always 1b (masking), but it 
is require only in case of y/yf tiling.
need to check for consequences in keeping this bit always Masked.
what is your opinion on this? Should not we implement it only in case of 
y/yf tiling, when we can do it in our framework?

Regards,
-Mahesh


Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
  drivers/gpu/drm/i915/i915_reg.h      |  3 +++
  drivers/gpu/drm/i915/intel_display.c | 50
++++++++++++++++++++++++++++++++++++
  3 files changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 4737a0e..79b9305 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1632,6 +1632,8 @@ struct skl_wm_values {
  	unsigned dirty_pipes;
  	/* any WaterMark memory workaround Required */
  	enum watermark_memory_wa mem_wa;
+	/* IPC Y-tiled WA related member */
+	u32 y_plane_mask;
  	struct skl_ddb_allocation ddb;
  	uint32_t wm_linetime[I915_MAX_PIPES];
  	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 75b5b52..210d9b0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5658,6 +5658,9 @@ enum {
  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
  	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe),
_PLANE_NV12_BUF_CFG_2(pipe))
  
+#define CHICKEN_DCPR_1             _MMIO(0x46430)
+#define IDLE_WAKEMEM_MASK          (1 << 13)
+
  /* SKL new cursor registers */
  #define _CUR_BUF_CFG_A				0x7017c
  #define _CUR_BUF_CFG_B				0x7117c
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 5f50f53..ee7f88e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12415,6 +12415,8 @@ int intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,
  	bool is_crtc_enabled = crtc_state->active;
  	bool turn_off, turn_on, visible, was_visible;
  	struct drm_framebuffer *fb = plane_state->fb;
+	struct intel_atomic_state *intel_state =
+				to_intel_atomic_state(plane_state-
state);
  	int ret;
  
  	if (INTEL_GEN(dev) >= 9 && plane->type !=
DRM_PLANE_TYPE_CURSOR) {
@@ -12501,6 +12503,15 @@ int intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,
  	    !needs_scaling(old_plane_state))
  		pipe_config->disable_lp_wm = true;
  
+	if (fb && (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+			fb->modifier[0] ==
I915_FORMAT_MOD_Yf_TILED)) {
+		intel_state->wm_results.y_plane_mask |=
+						(1 <<
drm_plane_index(plane));
+	} else {
+		intel_state->wm_results.y_plane_mask &=
+						~(1 <<
drm_plane_index(plane));
+	}
+
  	return 0;
  }
  
@@ -13963,6 +13974,10 @@ static int intel_atomic_check(struct
drm_device *dev,
  	if (ret)
  		return ret;
  
+	/* Copy the Y-tile WA related states */
+	intel_state->wm_results.y_plane_mask =
+				dev_priv-
wm.skl_results.y_plane_mask;
+
  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
  		struct intel_crtc_state *pipe_config =
  			to_intel_crtc_state(crtc_state);
@@ -14204,6 +14219,32 @@ static void intel_update_crtcs(struct
drm_atomic_state *state,
  	}
  }
  
+/*
+ * GEN9 IPC WA for Y-tiled
+ */
+void bxt_set_ipc_wa(struct drm_i915_private *dev_priv, bool enable)
+{
+	u32 val;
+
+	if (!IS_BROXTON(dev_priv) || !i915.enable_ipc)
+		return;
+
+	val = I915_READ(CHICKEN_DCPR_1);
+	/*
+	 * If WA is already enabled or disabled
+	 * no need to re-enable or disable it.
+	 */
+	if ((enable && (val & IDLE_WAKEMEM_MASK)) ||
+		(!enable && !(val & IDLE_WAKEMEM_MASK)))
+		return;
+
+	if (enable)
+		val |= IDLE_WAKEMEM_MASK;
+	else
+		val &= ~IDLE_WAKEMEM_MASK;
+	I915_WRITE(CHICKEN_DCPR_1, val);
+}
+
  static void skl_update_crtcs(struct drm_atomic_state *state,
  			     unsigned int *crtc_vblank_mask)
  {
@@ -14219,6 +14260,12 @@ static void skl_update_crtcs(struct
drm_atomic_state *state,
  	enum pipe pipe;
  
  	/*
+	 * If IPC WA need to be enabled, enable it now
+	 */
+	if (intel_state->wm_results.y_plane_mask)
+		bxt_set_ipc_wa(dev_priv, true);
+
+	/*
  	 * Whenever the number of active pipes changes, we need to
make sure we
  	 * update the pipes in the right order so that their ddb
allocations
  	 * never overlap with eachother inbetween CRTC updates.
Otherwise we'll
@@ -14261,6 +14308,9 @@ static void skl_update_crtcs(struct
drm_atomic_state *state,
  			progress = true;
  		}
  	} while (progress);
+
+	if (!intel_state->wm_results.y_plane_mask)
+		bxt_set_ipc_wa(dev_priv, false);
  }
  
  static void intel_atomic_commit_tail(struct drm_atomic_state *state)

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







