Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
> IPC may cause underflows if not used with dual channel symmetric
> memory configuration. Disable IPC for non symmetric configurations in
> affected platforms.
> Display WA #1141
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 43 ++++++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c |  2 +-
>  3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 86bc2e685522..2273664166bc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  	return 0;
>  }
>
> +static bool
> +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
> +			  u32 val_ch0, u32 val_ch1,
> +			  struct dram_channel_info *ch0)

what about
intel_is_dram_symmetric() ?

> +{
> +	/* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */

move this to the wa call, not the the function check...

> +	if (INTEL_GEN(dev_priv) > 9 &&
> +	    !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))

please don't add CNL pre-prod wa

> +		return true;
> +
> +	if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
> +		return true;

actually remove all platforms checks here...

> +
> +	if (val_ch0 != val_ch1)
> +		return false;
> +
> +	if (ch0->s_info.size == 0)
> +		return true;
> +	if (ch0->l_info.size == ch0->s_info.size &&
> +	    ch0->l_info.width == ch0->s_info.width &&
> +	    ch0->l_info.rank == ch0->s_info.rank)
> +		return true;
> +
> +	return false;

return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
       (ch0->l_info.size == ch0->s_info.size &&
         ch0->l_info.width == ch0->s_info.width &&
         ch0->l_info.rank == ch0->s_info.rank)))

> +}

> +
>  static int
>  skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  {
>  	struct dram_info *dram_info = &dev_priv->dram_info;
>  	struct dram_channel_info ch0, ch1;
> -	u32 val;
> +	u32 val_ch0, val_ch1;
>  	int ret;
>
> -	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch0, val);
> +	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch0, val_ch0);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>
> -	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch1, val);
> +	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch1, val_ch1);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>
> @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
>  		dram_info->is_16gb_dimm = true;
>
> +	if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
> +		dev_priv->ipc_capable_mem = true;
> +	else
> +		dev_priv->ipc_capable_mem = false;
> +
> +	DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
> +		      dev_priv->ipc_capable_mem ? "" : "not ");
>  	return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 854f3c828e01..036d6554c017 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2112,6 +2112,7 @@ struct drm_i915_private {
>  	bool chv_phy_assert[2];
>
>  	bool ipc_enabled;
> +	bool ipc_capable_mem;

I don't think we need to stage this...

>
>  	/* 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 2446f53adf21..39e400d5f555 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
>  	u32 val;
>
>  	/* Display WA #0477 WaDisableIPC: skl */
> -	if (IS_SKYLAKE(dev_priv)) {
> +	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
>  		dev_priv->ipc_enabled = false;

This is not the WA 1141 for other platforms than SKL. Please only keep skl here.

For the other WA add 4us across all watermark levels

/* Display WA #1141: skl,kbl,cfl */
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
   intel_is_dram_symmetric())
   levels += 4;

>  		return;
>  	}
> --
> 2.16.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx