[Intel-gfx] [PATCH 3/3] drm/i915: Implement 16GB dimm wa for latency level-0
Kumar, Mahesh
mahesh1.kumar at intel.com
Sat Jul 14 14:10:43 UTC 2018
Hi,
Thanks for review.
On 7/13/2018 8:27 PM, Ville Syrjälä wrote:
> On Fri, Jul 13, 2018 at 07:41:24PM +0530, Mahesh Kumar wrote:
>> Memory with 16GB dimms require an increase of 1us in level-0 latency.
>> This patch implements the same.
>> Bspec: 4381
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 35 +++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++++
>> 3 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index b93194cbd820..c6d30653d70c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1063,6 +1063,29 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>> intel_gvt_sanitize_options(dev_priv);
>> }
>>
>> +static void
>> +intel_memdev_is_16gb_dimm(struct memdev_info *memdev_info,
>> + u8 rank, u8 size, u8 width)
>> +{
>> + bool found_16gb_dimm = false;
>> +
>> + if (size == 16 && width == SKL_DRAM_WIDTH_X8 &&
>> + rank == SKL_DRAM_RANK_SINGLE)
>> + found_16gb_dimm = true;
>> + else if (size == 32 && width == SKL_DRAM_WIDTH_X8 &&
>> + rank == SKL_DRAM_RANK_DUAL)
>> + found_16gb_dimm = true;
>> + else if (size == 8 && width == SKL_DRAM_WIDTH_X16 &&
>> + rank == SKL_DRAM_RANK_SINGLE)
>> + found_16gb_dimm = true;
>> + else if (size == 16 && width == SKL_DRAM_WIDTH_X16 &&
>> + rank == SKL_DRAM_RANK_DUAL)
>> + found_16gb_dimm = true;
>> +
>> + if (!memdev_info->is_16gb_dimm)
>> + memdev_info->is_16gb_dimm = found_16gb_dimm;
>> +}
> Pure functions are generally more fun. Would also make the function name
> match the implementation.
Are you suggesting to change the name to only "is_16gb_dimm" ?
>> +
>> static enum memdev_rank
>> skl_memdev_get_channel_rank(struct memdev_info *memdev_info, u32 val)
>> {
>> @@ -1084,6 +1107,8 @@ skl_memdev_get_channel_rank(struct memdev_info *memdev_info, u32 val)
>> if (l_size == 0 && s_size == 0)
>> return I915_DRAM_RANK_INVALID;
>>
>> + memdev_info->valid_dimm = true;
>> +
>> DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
>> l_size, (1 << l_width) * 8, l_rank ? "dual" : "single",
>> s_size, (1 << s_width) * 8, s_rank ? "dual" : "single");
>> @@ -1096,6 +1121,9 @@ skl_memdev_get_channel_rank(struct memdev_info *memdev_info, u32 val)
>> else
>> rank = I915_DRAM_RANK_SINGLE;
>>
>> + intel_memdev_is_16gb_dimm(memdev_info, l_rank, l_size, l_width);
>> + intel_memdev_is_16gb_dimm(memdev_info, s_rank, s_size, s_width);
>> +
>> return rank;
>> }
>>
>> @@ -1247,6 +1275,7 @@ bxt_get_memdev_info(struct drm_i915_private *dev_priv)
>> return -EINVAL;
>> }
>>
>> + memdev_info->valid_dimm = true;
>> memdev_info->valid = true;
>> return 0;
>> }
>> @@ -1259,6 +1288,8 @@ intel_get_memdev_info(struct drm_i915_private *dev_priv)
>> int ret;
>>
>> memdev_info->valid = false;
>> + memdev_info->valid_dimm = false;
>> + memdev_info->is_16gb_dimm = false;
>> memdev_info->rank = I915_DRAM_RANK_INVALID;
>> memdev_info->bandwidth_kbps = 0;
>> memdev_info->num_channels = 0;
>> @@ -1282,9 +1313,9 @@ intel_get_memdev_info(struct drm_i915_private *dev_priv)
>> sprintf(bandwidth_str, "unknown");
>> DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>> bandwidth_str, memdev_info->num_channels);
>> - DRM_DEBUG_KMS("DRAM rank: %s rank\n",
>> + DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
>> (memdev_info->rank == I915_DRAM_RANK_DUAL) ?
>> - "dual" : "single");
>> + "dual" : "single", yesno(memdev_info->is_16gb_dimm));
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 845447d3806a..244adf8a30f1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1914,6 +1914,8 @@ struct drm_i915_private {
>>
>> struct memdev_info {
>> bool valid;
>> + bool valid_dimm;
>> + bool is_16gb_dimm;
>> u8 num_channels;
>> enum memdev_rank {
>> I915_DRAM_RANK_INVALID = 0,
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 53aaaa3e6886..f20f2f9118df 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2874,6 +2874,19 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
>> }
>> }
>>
>> + /*
>> + * WA Level-0 adjustment for 16GB DIMMs: SKL+
>> + * If we could not get dimm info enable this WA to prevent from
>> + * any underrun. If not able to get Dimm info assume 16GB dimm
>> + * to avoid any underrun.
>> + */
>> + if (dev_priv->memdev_info.valid_dimm) {
>> + if (dev_priv->memdev_info.is_16gb_dimm)
> So we only need these two bools in the end? Is there any point in storing
> all the intermediate stuff under dev_priv?
Gen-9 Watermark calculations require other parameters "system bandwidth,
memory rank and number of channels" for memory-bandwidth workaround
calculation. I'm working on patches to enable workaround only if it's
required or may be required. (calculate only during modeset to avoid
taking modeset lock during each flip)
Currently we enabling those workaround unconditionally because of which
we are facing multiple issues i.e. bug similar to following
https://bugs.freedesktop.org/show_bug.cgi?id=107113
-Mahesh
>> + wm[0] += 1;
>> + } else {
>> + wm[0] += 1;
>> + }
>> +
>> } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>> uint64_t sskpd = I915_READ64(MCH_SSKPD);
>>
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list