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

[Paulo Zanoni]

Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar <mahesh1.kumar at intel.com>
> 
> 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.

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?


> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> ---
>  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)