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

[Kumar, Mahesh]

Hi,


On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
> 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 at 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() ?
sounds good.
>> +{
>> +	/* 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
ok sure
>
>> +		return true;
>> +
>> +	if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
>> +		return true;
> actually remove all platforms checks here...
Agree will remove these checks, function will just return if memory is 
symmetric.
>
>> +
>> +	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...
With above changes storing this info is not needed, But we need to keep 
a flag in dram_info to check if "memory is symmetric", Otherwise we need 
to compute all the memory related info again.

-Mahesh
>>   	/* 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx