Re: [PATCH 3/3] drm/i915: Implement 16GB dimm wa for latency level-0

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

> +
>  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?

> +				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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx