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@xxxxxxxxx> --- 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" ?
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+ 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?
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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
