[PATCH 00/12] Polish DRAM information readout code

[PATCH 00/12] Polish DRAM information readout code

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Try to pimp up the DRAM information readut code a bit. This
is prep work for some DRAM bandwidth limit checks we'll be
needing.

Ville Syrjälä (12):
  drm/i915: Store DIMM rank information as a number
  drm/i915: Extract functions to derive SKL+ DIMM info
  drm/i915: Polish skl_is_16gb_dimm()
  drm/i915: Extract BXT DIMM helpers
  drm/i915: Fix DRAM size reporting for BXT
  drm/i915: Extract DIMM info on GLK too
  drm/i915: Use dram_dimm_info more
  drm/i915: Generalize intel_is_dram_symmetric()
  drm/i914: s/l_info/dimm_l/ etc.
  drm/i915: Clean up intel_get_dram_info() a bit
  drm/i915: Extract DIMM info on cnl+
  drm/i915: Read out memory type

 drivers/gpu/drm/i915/i915_drv.c | 408 +++++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h |  24 +-
 drivers/gpu/drm/i915/i915_reg.h |  30 ++-
 3 files changed, 312 insertions(+), 150 deletions(-)

-- 
2.19.2

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

[PATCH 01/12] drm/i915: Store DIMM rank information as a number

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Life will be easier later if we have the ranks stored
as a bare number.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 92 +++++++++++++++------------------
 drivers/gpu/drm/i915/i915_drv.h | 11 ++--
 2 files changed, 45 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c6354f6cdbdb..48c6bc44072d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1068,28 +1068,28 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
-static enum dram_rank skl_get_dimm_rank(u8 size, u32 rank)
+static int skl_get_dimm_ranks(u8 size, u32 rank)
 {
 	if (size == 0)
-		return I915_DRAM_RANK_INVALID;
+		return 0;
 	if (rank == SKL_DRAM_RANK_SINGLE)
-		return I915_DRAM_RANK_SINGLE;
+		return 1;
 	else if (rank == SKL_DRAM_RANK_DUAL)
-		return I915_DRAM_RANK_DUAL;
+		return 2;
 
-	return I915_DRAM_RANK_INVALID;
+	return 0;
 }
 
 static bool
-skl_is_16gb_dimm(enum dram_rank rank, u8 size, u8 width)
+skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
 {
-	if (rank == I915_DRAM_RANK_SINGLE && width == 8 && size == 16)
+	if (ranks == 1 && width == 8 && size == 16)
 		return true;
-	else if (rank == I915_DRAM_RANK_DUAL && width == 8 && size == 32)
+	else if (ranks == 2 && width == 8 && size == 32)
 		return true;
-	else if (rank == SKL_DRAM_RANK_SINGLE && width == 16 && size == 8)
+	else if (ranks == 1 && width == 16 && size == 8)
 		return true;
-	else if (rank == SKL_DRAM_RANK_DUAL && width == 16 && size == 16)
+	else if (ranks == 2 && width == 16 && size == 16)
 		return true;
 
 	return false;
@@ -1120,28 +1120,24 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 
 	tmp_l = val & SKL_DRAM_RANK_MASK;
 	tmp_s = s_val & SKL_DRAM_RANK_MASK;
-	ch->l_info.rank = skl_get_dimm_rank(ch->l_info.size, tmp_l);
-	ch->s_info.rank = skl_get_dimm_rank(ch->s_info.size, tmp_s);
-
-	if (ch->l_info.rank == I915_DRAM_RANK_DUAL ||
-	    ch->s_info.rank == I915_DRAM_RANK_DUAL)
-		ch->rank = I915_DRAM_RANK_DUAL;
-	else if (ch->l_info.rank == I915_DRAM_RANK_SINGLE &&
-		 ch->s_info.rank == I915_DRAM_RANK_SINGLE)
-		ch->rank = I915_DRAM_RANK_DUAL;
+	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
+	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
+
+	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
+		ch->ranks = 2;
+	else if (ch->l_info.ranks == 1 && ch->s_info.ranks == 1)
+		ch->ranks = 2;
 	else
-		ch->rank = I915_DRAM_RANK_SINGLE;
+		ch->ranks = 1;
 
-	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.rank, ch->l_info.size,
+	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
 					    ch->l_info.width) ||
-			   skl_is_16gb_dimm(ch->s_info.rank, ch->s_info.size,
+			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
 					    ch->s_info.width);
 
-	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
-		      ch->l_info.size, ch->l_info.width,
-		      ch->l_info.rank ? "dual" : "single",
-		      ch->s_info.size, ch->s_info.width,
-		      ch->s_info.rank ? "dual" : "single");
+	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
+		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
+		      ch->s_info.size, ch->s_info.width, ch->s_info.ranks);
 
 	return 0;
 }
@@ -1154,7 +1150,7 @@ intel_is_dram_symmetric(u32 val_ch0, u32 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)));
+		  ch0->l_info.ranks == ch0->s_info.ranks)));
 }
 
 static int
@@ -1185,13 +1181,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 	 * will be same as if single rank memory, so consider single rank
 	 * memory.
 	 */
-	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
-	    ch1.rank == I915_DRAM_RANK_SINGLE)
-		dram_info->rank = I915_DRAM_RANK_SINGLE;
+	if (ch0.ranks == 1 || ch1.ranks == 1)
+		dram_info->ranks = 1;
 	else
-		dram_info->rank = max(ch0.rank, ch1.rank);
+		dram_info->ranks = max(ch0.ranks, ch1.ranks);
 
-	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
+	if (dram_info->ranks == 0) {
 		DRM_INFO("couldn't get memory rank information\n");
 		return -EINVAL;
 	}
@@ -1262,8 +1257,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
 	 */
 	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
-		u8 size, width;
-		enum dram_rank rank;
+		u8 size, width, ranks;
 		u32 tmp;
 
 		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
@@ -1274,11 +1268,11 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		tmp = val & BXT_DRAM_RANK_MASK;
 
 		if (tmp == BXT_DRAM_RANK_SINGLE)
-			rank = I915_DRAM_RANK_SINGLE;
+			ranks = 1;
 		else if (tmp == BXT_DRAM_RANK_DUAL)
-			rank = I915_DRAM_RANK_DUAL;
+			ranks = 2;
 		else
-			rank = I915_DRAM_RANK_INVALID;
+			ranks = 0;
 
 		tmp = val & BXT_DRAM_SIZE_MASK;
 		if (tmp == BXT_DRAM_SIZE_4GB)
@@ -1296,22 +1290,21 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 
 		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
 		width = (1 << tmp) * 8;
-		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
-			      width, rank == I915_DRAM_RANK_SINGLE ? "single" :
-			      rank == I915_DRAM_RANK_DUAL ? "dual" : "unknown");
+		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
+			      size, width, ranks);
 
 		/*
 		 * If any of the channel is single rank channel,
 		 * worst case output will be same as if single rank
 		 * memory, so consider single rank memory.
 		 */
-		if (dram_info->rank == I915_DRAM_RANK_INVALID)
-			dram_info->rank = rank;
-		else if (rank == I915_DRAM_RANK_SINGLE)
-			dram_info->rank = I915_DRAM_RANK_SINGLE;
+		if (dram_info->ranks == 0)
+			dram_info->ranks = ranks;
+		else if (ranks == 1)
+			dram_info->ranks = 1;
 	}
 
-	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
+	if (dram_info->ranks == 0) {
 		DRM_INFO("couldn't get memory rank information\n");
 		return -EINVAL;
 	}
@@ -1328,7 +1321,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	int ret;
 
 	dram_info->valid = false;
-	dram_info->rank = I915_DRAM_RANK_INVALID;
+	dram_info->ranks = 0;
 	dram_info->bandwidth_kbps = 0;
 	dram_info->num_channels = 0;
 
@@ -1358,9 +1351,8 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 		sprintf(bandwidth_str, "unknown");
 	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
 		      bandwidth_str, dram_info->num_channels);
-	DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
-		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
-		      "dual" : "single", yesno(dram_info->is_16gb_dimm));
+	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
+		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cc09caf3870e..c9cb13a6edaf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1833,11 +1833,7 @@ struct drm_i915_private {
 		bool valid;
 		bool is_16gb_dimm;
 		u8 num_channels;
-		enum dram_rank {
-			I915_DRAM_RANK_INVALID = 0,
-			I915_DRAM_RANK_SINGLE,
-			I915_DRAM_RANK_DUAL
-		} rank;
+		u8 ranks;
 		u32 bandwidth_kbps;
 		bool symmetric_memory;
 	} dram_info;
@@ -2071,10 +2067,9 @@ struct drm_i915_private {
 
 struct dram_channel_info {
 	struct info {
-		u8 size, width;
-		enum dram_rank rank;
+		u8 size, width, ranks;
 	} l_info, s_info;
-	enum dram_rank rank;
+	u8 ranks;
 	bool is_16gb_dimm;
 };
 
-- 
2.19.2

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

Re: [PATCH 01/12] drm/i915: Store DIMM rank information as a number

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Life will be easier later if we have the ranks stored
> as a bare number.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 92 +++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_drv.h | 11 ++--
>  2 files changed, 45 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c6354f6cdbdb..48c6bc44072d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,28 +1068,28 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> -static enum dram_rank skl_get_dimm_rank(u8 size, u32 rank)
> +static int skl_get_dimm_ranks(u8 size, u32 rank)
>  {
>  	if (size == 0)
> -		return I915_DRAM_RANK_INVALID;
> +		return 0;
>  	if (rank == SKL_DRAM_RANK_SINGLE)
> -		return I915_DRAM_RANK_SINGLE;
> +		return 1;
>  	else if (rank == SKL_DRAM_RANK_DUAL)
> -		return I915_DRAM_RANK_DUAL;
> +		return 2;
>  
> -	return I915_DRAM_RANK_INVALID;
> +	return 0;
>  }
>  
>  static bool
> -skl_is_16gb_dimm(enum dram_rank rank, u8 size, u8 width)
> +skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
>  {
> -	if (rank == I915_DRAM_RANK_SINGLE && width == 8 && size == 16)
> +	if (ranks == 1 && width == 8 && size == 16)
>  		return true;
> -	else if (rank == I915_DRAM_RANK_DUAL && width == 8 && size == 32)
> +	else if (ranks == 2 && width == 8 && size == 32)
>  		return true;
> -	else if (rank == SKL_DRAM_RANK_SINGLE && width == 16 && size == 8)
> +	else if (ranks == 1 && width == 16 && size == 8)
>  		return true;
> -	else if (rank == SKL_DRAM_RANK_DUAL && width == 16 && size == 16)
> +	else if (ranks == 2 && width == 16 && size == 16)
>  		return true;
>  
>  	return false;
> @@ -1120,28 +1120,24 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  
>  	tmp_l = val & SKL_DRAM_RANK_MASK;
>  	tmp_s = s_val & SKL_DRAM_RANK_MASK;
> -	ch->l_info.rank = skl_get_dimm_rank(ch->l_info.size, tmp_l);
> -	ch->s_info.rank = skl_get_dimm_rank(ch->s_info.size, tmp_s);
> -
> -	if (ch->l_info.rank == I915_DRAM_RANK_DUAL ||
> -	    ch->s_info.rank == I915_DRAM_RANK_DUAL)
> -		ch->rank = I915_DRAM_RANK_DUAL;
> -	else if (ch->l_info.rank == I915_DRAM_RANK_SINGLE &&
> -		 ch->s_info.rank == I915_DRAM_RANK_SINGLE)
> -		ch->rank = I915_DRAM_RANK_DUAL;
> +	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> +	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> +
> +	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> +		ch->ranks = 2;
> +	else if (ch->l_info.ranks == 1 && ch->s_info.ranks == 1)
> +		ch->ranks = 2;
>  	else
> -		ch->rank = I915_DRAM_RANK_SINGLE;
> +		ch->ranks = 1;
>  
> -	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.rank, ch->l_info.size,
> +	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
>  					    ch->l_info.width) ||
> -			   skl_is_16gb_dimm(ch->s_info.rank, ch->s_info.size,
> +			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
>  					    ch->s_info.width);
>  
> -	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
> -		      ch->l_info.size, ch->l_info.width,
> -		      ch->l_info.rank ? "dual" : "single",
> -		      ch->s_info.size, ch->s_info.width,
> -		      ch->s_info.rank ? "dual" : "single");

I don't understand how the above ternary operators could ever have
produced the right results before.

> +	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
> +		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
> +		      ch->s_info.size, ch->s_info.width, ch->s_info.ranks);

%u instead of %d? Ditto for all debug prints.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  
>  	return 0;
>  }
> @@ -1154,7 +1150,7 @@ intel_is_dram_symmetric(u32 val_ch0, u32 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)));
> +		  ch0->l_info.ranks == ch0->s_info.ranks)));
>  }
>  
>  static int
> @@ -1185,13 +1181,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  	 * will be same as if single rank memory, so consider single rank
>  	 * memory.
>  	 */
> -	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
> -	    ch1.rank == I915_DRAM_RANK_SINGLE)
> -		dram_info->rank = I915_DRAM_RANK_SINGLE;
> +	if (ch0.ranks == 1 || ch1.ranks == 1)
> +		dram_info->ranks = 1;
>  	else
> -		dram_info->rank = max(ch0.rank, ch1.rank);
> +		dram_info->ranks = max(ch0.ranks, ch1.ranks);
>  
> -	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> +	if (dram_info->ranks == 0) {
>  		DRM_INFO("couldn't get memory rank information\n");
>  		return -EINVAL;
>  	}
> @@ -1262,8 +1257,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
>  	 */
>  	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> -		u8 size, width;
> -		enum dram_rank rank;
> +		u8 size, width, ranks;
>  		u32 tmp;
>  
>  		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> @@ -1274,11 +1268,11 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  		tmp = val & BXT_DRAM_RANK_MASK;
>  
>  		if (tmp == BXT_DRAM_RANK_SINGLE)
> -			rank = I915_DRAM_RANK_SINGLE;
> +			ranks = 1;
>  		else if (tmp == BXT_DRAM_RANK_DUAL)
> -			rank = I915_DRAM_RANK_DUAL;
> +			ranks = 2;
>  		else
> -			rank = I915_DRAM_RANK_INVALID;
> +			ranks = 0;
>  
>  		tmp = val & BXT_DRAM_SIZE_MASK;
>  		if (tmp == BXT_DRAM_SIZE_4GB)
> @@ -1296,22 +1290,21 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  
>  		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
>  		width = (1 << tmp) * 8;
> -		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> -			      width, rank == I915_DRAM_RANK_SINGLE ? "single" :
> -			      rank == I915_DRAM_RANK_DUAL ? "dual" : "unknown");
> +		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
> +			      size, width, ranks);
>  
>  		/*
>  		 * If any of the channel is single rank channel,
>  		 * worst case output will be same as if single rank
>  		 * memory, so consider single rank memory.
>  		 */
> -		if (dram_info->rank == I915_DRAM_RANK_INVALID)
> -			dram_info->rank = rank;
> -		else if (rank == I915_DRAM_RANK_SINGLE)
> -			dram_info->rank = I915_DRAM_RANK_SINGLE;
> +		if (dram_info->ranks == 0)
> +			dram_info->ranks = ranks;
> +		else if (ranks == 1)
> +			dram_info->ranks = 1;
>  	}
>  
> -	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> +	if (dram_info->ranks == 0) {
>  		DRM_INFO("couldn't get memory rank information\n");
>  		return -EINVAL;
>  	}
> @@ -1328,7 +1321,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	int ret;
>  
>  	dram_info->valid = false;
> -	dram_info->rank = I915_DRAM_RANK_INVALID;
> +	dram_info->ranks = 0;
>  	dram_info->bandwidth_kbps = 0;
>  	dram_info->num_channels = 0;
>  
> @@ -1358,9 +1351,8 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  		sprintf(bandwidth_str, "unknown");
>  	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>  		      bandwidth_str, dram_info->num_channels);
> -	DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
> -		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> -		      "dual" : "single", yesno(dram_info->is_16gb_dimm));
> +	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
> +		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc09caf3870e..c9cb13a6edaf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1833,11 +1833,7 @@ struct drm_i915_private {
>  		bool valid;
>  		bool is_16gb_dimm;
>  		u8 num_channels;
> -		enum dram_rank {
> -			I915_DRAM_RANK_INVALID = 0,
> -			I915_DRAM_RANK_SINGLE,
> -			I915_DRAM_RANK_DUAL
> -		} rank;
> +		u8 ranks;
>  		u32 bandwidth_kbps;
>  		bool symmetric_memory;
>  	} dram_info;
> @@ -2071,10 +2067,9 @@ struct drm_i915_private {
>  
>  struct dram_channel_info {
>  	struct info {
> -		u8 size, width;
> -		enum dram_rank rank;
> +		u8 size, width, ranks;
>  	} l_info, s_info;
> -	enum dram_rank rank;
> +	u8 ranks;
>  	bool is_16gb_dimm;
>  };

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Store DIMM rank information as a number

[Ville Syrjälä]

On Mon, Mar 04, 2019 at 06:17:50PM +0200, Jani Nikula wrote:
> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Life will be easier later if we have the ranks stored
> > as a bare number.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 92 +++++++++++++++------------------
> >  drivers/gpu/drm/i915/i915_drv.h | 11 ++--
> >  2 files changed, 45 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index c6354f6cdbdb..48c6bc44072d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1068,28 +1068,28 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
> >  	intel_gvt_sanitize_options(dev_priv);
> >  }
> >  
> > -static enum dram_rank skl_get_dimm_rank(u8 size, u32 rank)
> > +static int skl_get_dimm_ranks(u8 size, u32 rank)
> >  {
> >  	if (size == 0)
> > -		return I915_DRAM_RANK_INVALID;
> > +		return 0;
> >  	if (rank == SKL_DRAM_RANK_SINGLE)
> > -		return I915_DRAM_RANK_SINGLE;
> > +		return 1;
> >  	else if (rank == SKL_DRAM_RANK_DUAL)
> > -		return I915_DRAM_RANK_DUAL;
> > +		return 2;
> >  
> > -	return I915_DRAM_RANK_INVALID;
> > +	return 0;
> >  }
> >  
> >  static bool
> > -skl_is_16gb_dimm(enum dram_rank rank, u8 size, u8 width)
> > +skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
> >  {
> > -	if (rank == I915_DRAM_RANK_SINGLE && width == 8 && size == 16)
> > +	if (ranks == 1 && width == 8 && size == 16)
> >  		return true;
> > -	else if (rank == I915_DRAM_RANK_DUAL && width == 8 && size == 32)
> > +	else if (ranks == 2 && width == 8 && size == 32)
> >  		return true;
> > -	else if (rank == SKL_DRAM_RANK_SINGLE && width == 16 && size == 8)
> > +	else if (ranks == 1 && width == 16 && size == 8)
> >  		return true;
> > -	else if (rank == SKL_DRAM_RANK_DUAL && width == 16 && size == 16)
> > +	else if (ranks == 2 && width == 16 && size == 16)
> >  		return true;
> >  
> >  	return false;
> > @@ -1120,28 +1120,24 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> >  
> >  	tmp_l = val & SKL_DRAM_RANK_MASK;
> >  	tmp_s = s_val & SKL_DRAM_RANK_MASK;
> > -	ch->l_info.rank = skl_get_dimm_rank(ch->l_info.size, tmp_l);
> > -	ch->s_info.rank = skl_get_dimm_rank(ch->s_info.size, tmp_s);
> > -
> > -	if (ch->l_info.rank == I915_DRAM_RANK_DUAL ||
> > -	    ch->s_info.rank == I915_DRAM_RANK_DUAL)
> > -		ch->rank = I915_DRAM_RANK_DUAL;
> > -	else if (ch->l_info.rank == I915_DRAM_RANK_SINGLE &&
> > -		 ch->s_info.rank == I915_DRAM_RANK_SINGLE)
> > -		ch->rank = I915_DRAM_RANK_DUAL;
> > +	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> > +	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> > +
> > +	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> > +		ch->ranks = 2;
> > +	else if (ch->l_info.ranks == 1 && ch->s_info.ranks == 1)
> > +		ch->ranks = 2;
> >  	else
> > -		ch->rank = I915_DRAM_RANK_SINGLE;
> > +		ch->ranks = 1;
> >  
> > -	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.rank, ch->l_info.size,
> > +	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
> >  					    ch->l_info.width) ||
> > -			   skl_is_16gb_dimm(ch->s_info.rank, ch->s_info.size,
> > +			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
> >  					    ch->s_info.width);
> >  
> > -	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
> > -		      ch->l_info.size, ch->l_info.width,
> > -		      ch->l_info.rank ? "dual" : "single",
> > -		      ch->s_info.size, ch->s_info.width,
> > -		      ch->s_info.rank ? "dual" : "single");
> 
> I don't understand how the above ternary operators could ever have
> produced the right results before.

Good point. I didn't even notice :)

> 
> > +	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
> > +		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
> > +		      ch->s_info.size, ch->s_info.width, ch->s_info.ranks);
> 
> %u instead of %d? Ditto for all debug prints.

Doesn't really matter for these small values. But no harm in %u either
so might as well I suppose.

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  
> >  	return 0;
> >  }
> > @@ -1154,7 +1150,7 @@ intel_is_dram_symmetric(u32 val_ch0, u32 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)));
> > +		  ch0->l_info.ranks == ch0->s_info.ranks)));
> >  }
> >  
> >  static int
> > @@ -1185,13 +1181,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> >  	 * will be same as if single rank memory, so consider single rank
> >  	 * memory.
> >  	 */
> > -	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
> > -	    ch1.rank == I915_DRAM_RANK_SINGLE)
> > -		dram_info->rank = I915_DRAM_RANK_SINGLE;
> > +	if (ch0.ranks == 1 || ch1.ranks == 1)
> > +		dram_info->ranks = 1;
> >  	else
> > -		dram_info->rank = max(ch0.rank, ch1.rank);
> > +		dram_info->ranks = max(ch0.ranks, ch1.ranks);
> >  
> > -	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> > +	if (dram_info->ranks == 0) {
> >  		DRM_INFO("couldn't get memory rank information\n");
> >  		return -EINVAL;
> >  	}
> > @@ -1262,8 +1257,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> >  	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> >  	 */
> >  	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> > -		u8 size, width;
> > -		enum dram_rank rank;
> > +		u8 size, width, ranks;
> >  		u32 tmp;
> >  
> >  		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> > @@ -1274,11 +1268,11 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> >  		tmp = val & BXT_DRAM_RANK_MASK;
> >  
> >  		if (tmp == BXT_DRAM_RANK_SINGLE)
> > -			rank = I915_DRAM_RANK_SINGLE;
> > +			ranks = 1;
> >  		else if (tmp == BXT_DRAM_RANK_DUAL)
> > -			rank = I915_DRAM_RANK_DUAL;
> > +			ranks = 2;
> >  		else
> > -			rank = I915_DRAM_RANK_INVALID;
> > +			ranks = 0;
> >  
> >  		tmp = val & BXT_DRAM_SIZE_MASK;
> >  		if (tmp == BXT_DRAM_SIZE_4GB)
> > @@ -1296,22 +1290,21 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> >  
> >  		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
> >  		width = (1 << tmp) * 8;
> > -		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> > -			      width, rank == I915_DRAM_RANK_SINGLE ? "single" :
> > -			      rank == I915_DRAM_RANK_DUAL ? "dual" : "unknown");
> > +		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
> > +			      size, width, ranks);
> >  
> >  		/*
> >  		 * If any of the channel is single rank channel,
> >  		 * worst case output will be same as if single rank
> >  		 * memory, so consider single rank memory.
> >  		 */
> > -		if (dram_info->rank == I915_DRAM_RANK_INVALID)
> > -			dram_info->rank = rank;
> > -		else if (rank == I915_DRAM_RANK_SINGLE)
> > -			dram_info->rank = I915_DRAM_RANK_SINGLE;
> > +		if (dram_info->ranks == 0)
> > +			dram_info->ranks = ranks;
> > +		else if (ranks == 1)
> > +			dram_info->ranks = 1;
> >  	}
> >  
> > -	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> > +	if (dram_info->ranks == 0) {
> >  		DRM_INFO("couldn't get memory rank information\n");
> >  		return -EINVAL;
> >  	}
> > @@ -1328,7 +1321,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
> >  	int ret;
> >  
> >  	dram_info->valid = false;
> > -	dram_info->rank = I915_DRAM_RANK_INVALID;
> > +	dram_info->ranks = 0;
> >  	dram_info->bandwidth_kbps = 0;
> >  	dram_info->num_channels = 0;
> >  
> > @@ -1358,9 +1351,8 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
> >  		sprintf(bandwidth_str, "unknown");
> >  	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
> >  		      bandwidth_str, dram_info->num_channels);
> > -	DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
> > -		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> > -		      "dual" : "single", yesno(dram_info->is_16gb_dimm));
> > +	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
> > +		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index cc09caf3870e..c9cb13a6edaf 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1833,11 +1833,7 @@ struct drm_i915_private {
> >  		bool valid;
> >  		bool is_16gb_dimm;
> >  		u8 num_channels;
> > -		enum dram_rank {
> > -			I915_DRAM_RANK_INVALID = 0,
> > -			I915_DRAM_RANK_SINGLE,
> > -			I915_DRAM_RANK_DUAL
> > -		} rank;
> > +		u8 ranks;
> >  		u32 bandwidth_kbps;
> >  		bool symmetric_memory;
> >  	} dram_info;
> > @@ -2071,10 +2067,9 @@ struct drm_i915_private {
> >  
> >  struct dram_channel_info {
> >  	struct info {
> > -		u8 size, width;
> > -		enum dram_rank rank;
> > +		u8 size, width, ranks;
> >  	} l_info, s_info;
> > -	enum dram_rank rank;
> > +	u8 ranks;
> >  	bool is_16gb_dimm;
> >  };
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

[PATCH 02/12] drm/i915: Extract functions to derive SKL+ DIMM info

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make the code less repetitive by extracting a few small helpers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 48c6bc44072d..b94bf475b04c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
-static int skl_get_dimm_ranks(u8 size, u32 rank)
+static int skl_get_dimm_size(u16 val)
 {
-	if (size == 0)
+	return val & SKL_DRAM_SIZE_MASK;
+}
+
+static int skl_get_dimm_width(u16 val)
+{
+	if (skl_get_dimm_size(val) == 0)
 		return 0;
-	if (rank == SKL_DRAM_RANK_SINGLE)
-		return 1;
-	else if (rank == SKL_DRAM_RANK_DUAL)
-		return 2;
 
-	return 0;
+	switch (val & SKL_DRAM_WIDTH_MASK) {
+	case SKL_DRAM_WIDTH_X8:
+	case SKL_DRAM_WIDTH_X16:
+	case SKL_DRAM_WIDTH_X32:
+		val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
+		return 8 << val;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
+}
+
+static int skl_get_dimm_ranks(u16 val)
+{
+	if (skl_get_dimm_size(val) == 0)
+		return 0;
+
+	switch (val & SKL_DRAM_RANK_MASK) {
+	case SKL_DRAM_RANK_SINGLE:
+	case SKL_DRAM_RANK_DUAL:
+		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
+		return val + 1;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
 }
 
 static bool
@@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
 static int
 skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 {
-	u32 tmp_l, tmp_s;
-	u32 s_val = val >> SKL_DRAM_S_SHIFT;
+	u16 tmp_l, tmp_s;
 
-	if (!val)
-		return -EINVAL;
+	tmp_l = val & 0xffff;
+	tmp_s = val >> 16;
 
-	tmp_l = val & SKL_DRAM_SIZE_MASK;
-	tmp_s = s_val & SKL_DRAM_SIZE_MASK;
+	ch->l_info.size = skl_get_dimm_size(tmp_l);
+	ch->s_info.size = skl_get_dimm_size(tmp_s);
 
-	if (tmp_l == 0 && tmp_s == 0)
+	if (ch->l_info.size == 0 && ch->s_info.size == 0)
 		return -EINVAL;
 
-	ch->l_info.size = tmp_l;
-	ch->s_info.size = tmp_s;
-
-	tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
-	tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
-	ch->l_info.width = (1 << tmp_l) * 8;
-	ch->s_info.width = (1 << tmp_s) * 8;
+	ch->l_info.width = skl_get_dimm_width(tmp_l);
+	ch->s_info.width = skl_get_dimm_width(tmp_s);
 
-	tmp_l = val & SKL_DRAM_RANK_MASK;
-	tmp_s = s_val & SKL_DRAM_RANK_MASK;
-	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
-	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
+	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
+	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
 
 	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
 		ch->ranks = 2;
-- 
2.19.2

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

Re: [PATCH 02/12] drm/i915: Extract functions to derive SKL+ DIMM info

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the code less repetitive by extracting a few small helpers.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 48c6bc44072d..b94bf475b04c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> -static int skl_get_dimm_ranks(u8 size, u32 rank)
> +static int skl_get_dimm_size(u16 val)
>  {
> -	if (size == 0)
> +	return val & SKL_DRAM_SIZE_MASK;
> +}
> +
> +static int skl_get_dimm_width(u16 val)
> +{
> +	if (skl_get_dimm_size(val) == 0)
>  		return 0;
> -	if (rank == SKL_DRAM_RANK_SINGLE)
> -		return 1;
> -	else if (rank == SKL_DRAM_RANK_DUAL)
> -		return 2;
>  
> -	return 0;
> +	switch (val & SKL_DRAM_WIDTH_MASK) {
> +	case SKL_DRAM_WIDTH_X8:
> +	case SKL_DRAM_WIDTH_X16:
> +	case SKL_DRAM_WIDTH_X32:
> +		val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> +		return 8 << val;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}
> +}
> +
> +static int skl_get_dimm_ranks(u16 val)
> +{
> +	if (skl_get_dimm_size(val) == 0)
> +		return 0;
> +
> +	switch (val & SKL_DRAM_RANK_MASK) {
> +	case SKL_DRAM_RANK_SINGLE:
> +	case SKL_DRAM_RANK_DUAL:
> +		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> +		return val + 1;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}

I don't much care for this dual use of both the macro and then the
calculation. I'd either just calculate, or return pre-calculated values
from the cases, not both. The missing cases can also never happen.

But it all checks out, so

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  }
>  
>  static bool
> @@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
>  static int
>  skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  {
> -	u32 tmp_l, tmp_s;
> -	u32 s_val = val >> SKL_DRAM_S_SHIFT;
> +	u16 tmp_l, tmp_s;
>  
> -	if (!val)
> -		return -EINVAL;
> +	tmp_l = val & 0xffff;
> +	tmp_s = val >> 16;
>  
> -	tmp_l = val & SKL_DRAM_SIZE_MASK;
> -	tmp_s = s_val & SKL_DRAM_SIZE_MASK;
> +	ch->l_info.size = skl_get_dimm_size(tmp_l);
> +	ch->s_info.size = skl_get_dimm_size(tmp_s);
>  
> -	if (tmp_l == 0 && tmp_s == 0)
> +	if (ch->l_info.size == 0 && ch->s_info.size == 0)
>  		return -EINVAL;
>  
> -	ch->l_info.size = tmp_l;
> -	ch->s_info.size = tmp_s;
> -
> -	tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> -	tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> -	ch->l_info.width = (1 << tmp_l) * 8;
> -	ch->s_info.width = (1 << tmp_s) * 8;
> +	ch->l_info.width = skl_get_dimm_width(tmp_l);
> +	ch->s_info.width = skl_get_dimm_width(tmp_s);
>  
> -	tmp_l = val & SKL_DRAM_RANK_MASK;
> -	tmp_s = s_val & SKL_DRAM_RANK_MASK;
> -	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> -	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> +	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> +	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
>  
>  	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
>  		ch->ranks = 2;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Extract functions to derive SKL+ DIMM info

[Ville Syrjälä]

On Mon, Mar 04, 2019 at 06:32:25PM +0200, Jani Nikula wrote:
> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make the code less repetitive by extracting a few small helpers.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
> >  1 file changed, 43 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 48c6bc44072d..b94bf475b04c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
> >  	intel_gvt_sanitize_options(dev_priv);
> >  }
> >  
> > -static int skl_get_dimm_ranks(u8 size, u32 rank)
> > +static int skl_get_dimm_size(u16 val)
> >  {
> > -	if (size == 0)
> > +	return val & SKL_DRAM_SIZE_MASK;
> > +}
> > +
> > +static int skl_get_dimm_width(u16 val)
> > +{
> > +	if (skl_get_dimm_size(val) == 0)
> >  		return 0;
> > -	if (rank == SKL_DRAM_RANK_SINGLE)
> > -		return 1;
> > -	else if (rank == SKL_DRAM_RANK_DUAL)
> > -		return 2;
> >  
> > -	return 0;
> > +	switch (val & SKL_DRAM_WIDTH_MASK) {
> > +	case SKL_DRAM_WIDTH_X8:
> > +	case SKL_DRAM_WIDTH_X16:
> > +	case SKL_DRAM_WIDTH_X32:
> > +		val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > +		return 8 << val;
> > +	default:
> > +		MISSING_CASE(val);
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int skl_get_dimm_ranks(u16 val)
> > +{
> > +	if (skl_get_dimm_size(val) == 0)
> > +		return 0;
> > +
> > +	switch (val & SKL_DRAM_RANK_MASK) {
> > +	case SKL_DRAM_RANK_SINGLE:
> > +	case SKL_DRAM_RANK_DUAL:
> > +		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> > +		return val + 1;
> > +	default:
> > +		MISSING_CASE(val);
> > +		return 0;
> > +	}
> 
> I don't much care for this dual use of both the macro and then the
> calculation. I'd either just calculate, or return pre-calculated values
> from the cases, not both. The missing cases can also never happen.

I generally lean toward the arithmetic option myself, and I did
consider it here as well. I suppose I ended up being swayed
slightly towards the other end of the spectrum by the potential
documentation value of the case labels. And so I ended up
somewhere in the middle.

> 
> But it all checks out, so
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  }
> >  
> >  static bool
> > @@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
> >  static int
> >  skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> >  {
> > -	u32 tmp_l, tmp_s;
> > -	u32 s_val = val >> SKL_DRAM_S_SHIFT;
> > +	u16 tmp_l, tmp_s;
> >  
> > -	if (!val)
> > -		return -EINVAL;
> > +	tmp_l = val & 0xffff;
> > +	tmp_s = val >> 16;
> >  
> > -	tmp_l = val & SKL_DRAM_SIZE_MASK;
> > -	tmp_s = s_val & SKL_DRAM_SIZE_MASK;
> > +	ch->l_info.size = skl_get_dimm_size(tmp_l);
> > +	ch->s_info.size = skl_get_dimm_size(tmp_s);
> >  
> > -	if (tmp_l == 0 && tmp_s == 0)
> > +	if (ch->l_info.size == 0 && ch->s_info.size == 0)
> >  		return -EINVAL;
> >  
> > -	ch->l_info.size = tmp_l;
> > -	ch->s_info.size = tmp_s;
> > -
> > -	tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > -	tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > -	ch->l_info.width = (1 << tmp_l) * 8;
> > -	ch->s_info.width = (1 << tmp_s) * 8;
> > +	ch->l_info.width = skl_get_dimm_width(tmp_l);
> > +	ch->s_info.width = skl_get_dimm_width(tmp_s);
> >  
> > -	tmp_l = val & SKL_DRAM_RANK_MASK;
> > -	tmp_s = s_val & SKL_DRAM_RANK_MASK;
> > -	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> > -	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> > +	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> > +	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
> >  
> >  	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> >  		ch->ranks = 2;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

[PATCH 03/12] drm/i915: Polish skl_is_16gb_dimm()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pass the dimm struct to skl_is_16gb_dimm() rather than passing each
value separately. And let's replace the hardcoded set of values with
some simple arithmetic.

Also fix the byte vs. bit inconsistency in the debug message,
and polish the wording otherwise as well.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 22 ++++++----------------
 drivers/gpu/drm/i915/i915_drv.h |  8 +++++---
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b94bf475b04c..9d7fc2bc6593 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1107,18 +1107,9 @@ static int skl_get_dimm_ranks(u16 val)
 }
 
 static bool
-skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
+skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
 {
-	if (ranks == 1 && width == 8 && size == 16)
-		return true;
-	else if (ranks == 2 && width == 8 && size == 32)
-		return true;
-	else if (ranks == 1 && width == 16 && size == 8)
-		return true;
-	else if (ranks == 2 && width == 16 && size == 16)
-		return true;
-
-	return false;
+	return dimm->size * dimm->width / (8 * dimm->ranks ?: 1) == 16;
 }
 
 static int
@@ -1148,10 +1139,9 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 	else
 		ch->ranks = 1;
 
-	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
-					    ch->l_info.width) ||
-			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
-					    ch->s_info.width);
+	ch->is_16gb_dimm =
+		skl_is_16gb_dimm(&ch->l_info) ||
+		skl_is_16gb_dimm(&ch->s_info);
 
 	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
 		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
@@ -1369,7 +1359,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 		sprintf(bandwidth_str, "unknown");
 	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
 		      bandwidth_str, dram_info->num_channels);
-	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
+	DRM_DEBUG_KMS("DRAM ranks: %d, 16Gb DIMMs: %s\n",
 		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9cb13a6edaf..fcde09934bb5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2065,10 +2065,12 @@ struct drm_i915_private {
 	 */
 };
 
+struct dram_dimm_info {
+	u8 size, width, ranks;
+};
+
 struct dram_channel_info {
-	struct info {
-		u8 size, width, ranks;
-	} l_info, s_info;
+	struct dram_dimm_info l_info, s_info;
 	u8 ranks;
 	bool is_16gb_dimm;
 };
-- 
2.19.2

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

[PATCH v2 03/12] drm/i915: Polish skl_is_16gb_dimm()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pass the dimm struct to skl_is_16gb_dimm() rather than passing each
value separately. And let's replace the hardcoded set of values with
some simple arithmetic.

Also fix the byte vs. bit inconsistency in the debug message,
and polish the wording otherwise as well.

v2: Deobfuscate the math (Chris)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++----------------
 drivers/gpu/drm/i915/i915_drv.h |  8 +++++---
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b94bf475b04c..d84f3485e775 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1068,6 +1068,11 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+static int intel_dimm_num_devices(const struct dram_dimm_info *dimm)
+{
+	return dimm->ranks * 64 / (dimm->width ?: 1);
+}
+
 static int skl_get_dimm_size(u16 val)
 {
 	return val & SKL_DRAM_SIZE_MASK;
@@ -1107,18 +1112,10 @@ static int skl_get_dimm_ranks(u16 val)
 }
 
 static bool
-skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
+skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
 {
-	if (ranks == 1 && width == 8 && size == 16)
-		return true;
-	else if (ranks == 2 && width == 8 && size == 32)
-		return true;
-	else if (ranks == 1 && width == 16 && size == 8)
-		return true;
-	else if (ranks == 2 && width == 16 && size == 16)
-		return true;
-
-	return false;
+	/* Convert total GB to Gb per DRAM device */
+	return 8 * dimm->size / (intel_dimm_num_devices(dimm) ?: 1) == 16;
 }
 
 static int
@@ -1148,10 +1145,9 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 	else
 		ch->ranks = 1;
 
-	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
-					    ch->l_info.width) ||
-			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
-					    ch->s_info.width);
+	ch->is_16gb_dimm =
+		skl_is_16gb_dimm(&ch->l_info) ||
+		skl_is_16gb_dimm(&ch->s_info);
 
 	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
 		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
@@ -1369,7 +1365,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 		sprintf(bandwidth_str, "unknown");
 	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
 		      bandwidth_str, dram_info->num_channels);
-	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
+	DRM_DEBUG_KMS("DRAM ranks: %d, 16Gb DIMMs: %s\n",
 		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9cb13a6edaf..fcde09934bb5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2065,10 +2065,12 @@ struct drm_i915_private {
 	 */
 };
 
+struct dram_dimm_info {
+	u8 size, width, ranks;
+};
+
 struct dram_channel_info {
-	struct info {
-		u8 size, width, ranks;
-	} l_info, s_info;
+	struct dram_dimm_info l_info, s_info;
 	u8 ranks;
 	bool is_16gb_dimm;
 };
-- 
2.19.2

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

Re: [PATCH v2 03/12] drm/i915: Polish skl_is_16gb_dimm()

[Jani Nikula]

On Tue, 26 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pass the dimm struct to skl_is_16gb_dimm() rather than passing each
> value separately. And let's replace the hardcoded set of values with
> some simple arithmetic.
>
> Also fix the byte vs. bit inconsistency in the debug message,
> and polish the wording otherwise as well.
>
> v2: Deobfuscate the math (Chris)

Took me longer than I'd like to wrap my head around this, but looks
good.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++----------------
>  drivers/gpu/drm/i915/i915_drv.h |  8 +++++---
>  2 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b94bf475b04c..d84f3485e775 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,6 +1068,11 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> +static int intel_dimm_num_devices(const struct dram_dimm_info *dimm)
> +{
> +	return dimm->ranks * 64 / (dimm->width ?: 1);
> +}
> +
>  static int skl_get_dimm_size(u16 val)
>  {
>  	return val & SKL_DRAM_SIZE_MASK;
> @@ -1107,18 +1112,10 @@ static int skl_get_dimm_ranks(u16 val)
>  }
>  
>  static bool
> -skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
> +skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>  {
> -	if (ranks == 1 && width == 8 && size == 16)
> -		return true;
> -	else if (ranks == 2 && width == 8 && size == 32)
> -		return true;
> -	else if (ranks == 1 && width == 16 && size == 8)
> -		return true;
> -	else if (ranks == 2 && width == 16 && size == 16)
> -		return true;
> -
> -	return false;
> +	/* Convert total GB to Gb per DRAM device */
> +	return 8 * dimm->size / (intel_dimm_num_devices(dimm) ?: 1) == 16;
>  }
>  
>  static int
> @@ -1148,10 +1145,9 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  	else
>  		ch->ranks = 1;
>  
> -	ch->is_16gb_dimm = skl_is_16gb_dimm(ch->l_info.ranks, ch->l_info.size,
> -					    ch->l_info.width) ||
> -			   skl_is_16gb_dimm(ch->s_info.ranks, ch->s_info.size,
> -					    ch->s_info.width);
> +	ch->is_16gb_dimm =
> +		skl_is_16gb_dimm(&ch->l_info) ||
> +		skl_is_16gb_dimm(&ch->s_info);
>  
>  	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
>  		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
> @@ -1369,7 +1365,7 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  		sprintf(bandwidth_str, "unknown");
>  	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>  		      bandwidth_str, dram_info->num_channels);
> -	DRM_DEBUG_KMS("DRAM ranks: %d, 16GB-dimm:%s\n",
> +	DRM_DEBUG_KMS("DRAM ranks: %d, 16Gb DIMMs: %s\n",
>  		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9cb13a6edaf..fcde09934bb5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2065,10 +2065,12 @@ struct drm_i915_private {
>  	 */
>  };
>  
> +struct dram_dimm_info {
> +	u8 size, width, ranks;
> +};
> +
>  struct dram_channel_info {
> -	struct info {
> -		u8 size, width, ranks;
> -	} l_info, s_info;
> +	struct dram_dimm_info l_info, s_info;
>  	u8 ranks;
>  	bool is_16gb_dimm;
>  };

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/12] drm/i915: Extract BXT DIMM helpers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Polish the bxt DIMM parsing by extracting a few small helpers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 79 ++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9d7fc2bc6593..1f4a966a9727 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1237,6 +1237,51 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static int bxt_get_dimm_size(u32 val)
+{
+	switch (val & BXT_DRAM_SIZE_MASK) {
+	case BXT_DRAM_SIZE_4GB:
+		return 4;
+	case BXT_DRAM_SIZE_6GB:
+		return 6;
+	case BXT_DRAM_SIZE_8GB:
+		return 8;
+	case BXT_DRAM_SIZE_12GB:
+		return 12;
+	case BXT_DRAM_SIZE_16GB:
+		return 16;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
+}
+
+static int bxt_get_dimm_width(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return 0;
+
+	val = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
+
+	return 8 << val;
+}
+
+static int bxt_get_dimm_ranks(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return 0;
+
+	switch (val & BXT_DRAM_RANK_MASK) {
+	case BXT_DRAM_RANK_SINGLE:
+		return 1;
+	case BXT_DRAM_RANK_DUAL:
+		return 2;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
+}
+
 static int
 bxt_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1266,39 +1311,19 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 	 */
 	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
 		u8 size, width, ranks;
-		u32 tmp;
 
 		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
 		if (val == 0xFFFFFFFF)
 			continue;
 
 		dram_info->num_channels++;
-		tmp = val & BXT_DRAM_RANK_MASK;
-
-		if (tmp == BXT_DRAM_RANK_SINGLE)
-			ranks = 1;
-		else if (tmp == BXT_DRAM_RANK_DUAL)
-			ranks = 2;
-		else
-			ranks = 0;
-
-		tmp = val & BXT_DRAM_SIZE_MASK;
-		if (tmp == BXT_DRAM_SIZE_4GB)
-			size = 4;
-		else if (tmp == BXT_DRAM_SIZE_6GB)
-			size = 6;
-		else if (tmp == BXT_DRAM_SIZE_8GB)
-			size = 8;
-		else if (tmp == BXT_DRAM_SIZE_12GB)
-			size = 12;
-		else if (tmp == BXT_DRAM_SIZE_16GB)
-			size = 16;
-		else
-			size = 0;
-
-		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
-		width = (1 << tmp) * 8;
-		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
+
+		size = bxt_get_dimm_size(val);
+		width = bxt_get_dimm_width(val);
+		ranks = bxt_get_dimm_ranks(val);
+
+		DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
+			      i - BXT_D_CR_DRP0_DUNIT_START,
 			      size, width, ranks);
 
 		/*
-- 
2.19.2

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

[PATCH v2 04/12] drm/i915: Extract BXT DIMM helpers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Polish the bxt DIMM parsing by extracting a few small helpers.

v2: Use struct dram_dimm_info

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 93 ++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d84f3485e775..f948d475bdf4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1243,6 +1243,59 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static int bxt_get_dimm_size(u32 val)
+{
+	switch (val & BXT_DRAM_SIZE_MASK) {
+	case BXT_DRAM_SIZE_4GB:
+		return 4;
+	case BXT_DRAM_SIZE_6GB:
+		return 6;
+	case BXT_DRAM_SIZE_8GB:
+		return 8;
+	case BXT_DRAM_SIZE_12GB:
+		return 12;
+	case BXT_DRAM_SIZE_16GB:
+		return 16;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
+}
+
+static int bxt_get_dimm_width(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return 0;
+
+	val = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
+
+	return 8 << val;
+}
+
+static int bxt_get_dimm_ranks(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return 0;
+
+	switch (val & BXT_DRAM_RANK_MASK) {
+	case BXT_DRAM_RANK_SINGLE:
+		return 1;
+	case BXT_DRAM_RANK_DUAL:
+		return 2;
+	default:
+		MISSING_CASE(val);
+		return 0;
+	}
+}
+
+static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
+			      u32 val)
+{
+	dimm->size = bxt_get_dimm_size(val);
+	dimm->width = bxt_get_dimm_width(val);
+	dimm->ranks = bxt_get_dimm_ranks(val);
+}
+
 static int
 bxt_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1271,41 +1324,19 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
 	 */
 	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
-		u8 size, width, ranks;
-		u32 tmp;
+		struct dram_dimm_info dimm;
 
 		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
 		if (val == 0xFFFFFFFF)
 			continue;
 
 		dram_info->num_channels++;
-		tmp = val & BXT_DRAM_RANK_MASK;
-
-		if (tmp == BXT_DRAM_RANK_SINGLE)
-			ranks = 1;
-		else if (tmp == BXT_DRAM_RANK_DUAL)
-			ranks = 2;
-		else
-			ranks = 0;
-
-		tmp = val & BXT_DRAM_SIZE_MASK;
-		if (tmp == BXT_DRAM_SIZE_4GB)
-			size = 4;
-		else if (tmp == BXT_DRAM_SIZE_6GB)
-			size = 6;
-		else if (tmp == BXT_DRAM_SIZE_8GB)
-			size = 8;
-		else if (tmp == BXT_DRAM_SIZE_12GB)
-			size = 12;
-		else if (tmp == BXT_DRAM_SIZE_16GB)
-			size = 16;
-		else
-			size = 0;
-
-		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
-		width = (1 << tmp) * 8;
-		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
-			      size, width, ranks);
+
+		bxt_get_dimm_info(&dimm, val);
+
+		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
+			      i - BXT_D_CR_DRP0_DUNIT_START,
+			      dimm.size, dimm.width, dimm.ranks);
 
 		/*
 		 * If any of the channel is single rank channel,
@@ -1313,8 +1344,8 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		 * memory, so consider single rank memory.
 		 */
 		if (dram_info->ranks == 0)
-			dram_info->ranks = ranks;
-		else if (ranks == 1)
+			dram_info->ranks = dimm.ranks;
+		else if (dimm.ranks == 1)
 			dram_info->ranks = 1;
 	}
 
-- 
2.19.2

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

Re: [PATCH v2 04/12] drm/i915: Extract BXT DIMM helpers

[Jani Nikula]

On Tue, 26 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Polish the bxt DIMM parsing by extracting a few small helpers.
>
> v2: Use struct dram_dimm_info
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 93 ++++++++++++++++++++++-----------
>  1 file changed, 62 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d84f3485e775..f948d475bdf4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1243,6 +1243,59 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +static int bxt_get_dimm_size(u32 val)
> +{
> +	switch (val & BXT_DRAM_SIZE_MASK) {
> +	case BXT_DRAM_SIZE_4GB:
> +		return 4;
> +	case BXT_DRAM_SIZE_6GB:
> +		return 6;
> +	case BXT_DRAM_SIZE_8GB:
> +		return 8;
> +	case BXT_DRAM_SIZE_12GB:
> +		return 12;
> +	case BXT_DRAM_SIZE_16GB:
> +		return 16;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}
> +}
> +
> +static int bxt_get_dimm_width(u32 val)
> +{
> +	if (!bxt_get_dimm_size(val))
> +		return 0;
> +
> +	val = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
> +
> +	return 8 << val;
> +}
> +
> +static int bxt_get_dimm_ranks(u32 val)
> +{
> +	if (!bxt_get_dimm_size(val))
> +		return 0;
> +
> +	switch (val & BXT_DRAM_RANK_MASK) {
> +	case BXT_DRAM_RANK_SINGLE:
> +		return 1;
> +	case BXT_DRAM_RANK_DUAL:
> +		return 2;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}
> +}
> +
> +static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
> +			      u32 val)
> +{
> +	dimm->size = bxt_get_dimm_size(val);
> +	dimm->width = bxt_get_dimm_width(val);
> +	dimm->ranks = bxt_get_dimm_ranks(val);
> +}
> +
>  static int
>  bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  {
> @@ -1271,41 +1324,19 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
>  	 */
>  	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> -		u8 size, width, ranks;
> -		u32 tmp;
> +		struct dram_dimm_info dimm;
>  
>  		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
>  		if (val == 0xFFFFFFFF)
>  			continue;
>  
>  		dram_info->num_channels++;
> -		tmp = val & BXT_DRAM_RANK_MASK;
> -
> -		if (tmp == BXT_DRAM_RANK_SINGLE)
> -			ranks = 1;
> -		else if (tmp == BXT_DRAM_RANK_DUAL)
> -			ranks = 2;
> -		else
> -			ranks = 0;
> -
> -		tmp = val & BXT_DRAM_SIZE_MASK;
> -		if (tmp == BXT_DRAM_SIZE_4GB)
> -			size = 4;
> -		else if (tmp == BXT_DRAM_SIZE_6GB)
> -			size = 6;
> -		else if (tmp == BXT_DRAM_SIZE_8GB)
> -			size = 8;
> -		else if (tmp == BXT_DRAM_SIZE_12GB)
> -			size = 12;
> -		else if (tmp == BXT_DRAM_SIZE_16GB)
> -			size = 16;
> -		else
> -			size = 0;
> -
> -		tmp = (val & BXT_DRAM_WIDTH_MASK) >> BXT_DRAM_WIDTH_SHIFT;
> -		width = (1 << tmp) * 8;
> -		DRM_DEBUG_KMS("dram size:%dGB width:X%d ranks:%d\n",
> -			      size, width, ranks);
> +
> +		bxt_get_dimm_info(&dimm, val);
> +
> +		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
> +			      i - BXT_D_CR_DRP0_DUNIT_START,
> +			      dimm.size, dimm.width, dimm.ranks);
>  
>  		/*
>  		 * If any of the channel is single rank channel,
> @@ -1313,8 +1344,8 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  		 * memory, so consider single rank memory.
>  		 */
>  		if (dram_info->ranks == 0)
> -			dram_info->ranks = ranks;
> -		else if (ranks == 1)
> +			dram_info->ranks = dimm.ranks;
> +		else if (dimm.ranks == 1)
>  			dram_info->ranks = 1;
>  	}

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/12] drm/i915: Fix DRAM size reporting for BXT

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The BXT DUNIT register tells us the size of each DRAM device
in Gb. We want to report the size of the whole DIMM in GB, so
that it matches how we report it for non-LP platforms.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1f4a966a9727..c40a738dabd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1322,7 +1322,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		width = bxt_get_dimm_width(val);
 		ranks = bxt_get_dimm_ranks(val);
 
-		DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
+		/*
+		 * Size in register is Gb per DRAM device.
+		 * Convert to total GB to match the way
+		 * we report this for non-LP platforms.
+		 */
+		size = size * ranks * 8 / (width ?: 1);
+
+		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
 			      i - BXT_D_CR_DRP0_DUNIT_START,
 			      size, width, ranks);
 
-- 
2.19.2

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

Re: [PATCH 05/12] drm/i915: Fix DRAM size reporting for BXT

[Chris Wilson]

Quoting Ville Syrjala (2019-02-25 20:29:00)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The BXT DUNIT register tells us the size of each DRAM device
> in Gb. We want to report the size of the whole DIMM in GB, so
> that it matches how we report it for non-LP platforms.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1f4a966a9727..c40a738dabd3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1322,7 +1322,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>                 width = bxt_get_dimm_width(val);
>                 ranks = bxt_get_dimm_ranks(val);
>  
> -               DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
> +               /*
> +                * Size in register is Gb per DRAM device.
> +                * Convert to total GB to match the way
> +                * we report this for non-LP platforms.
> +                */
> +               size = size * ranks * 8 / (width ?: 1);

Should it be /8 for Gbits to GBytes?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: Fix DRAM size reporting for BXT

[Ville Syrjälä]

On Mon, Feb 25, 2019 at 08:35:08PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-02-25 20:29:00)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The BXT DUNIT register tells us the size of each DRAM device
> > in Gb. We want to report the size of the whole DIMM in GB, so
> > that it matches how we report it for non-LP platforms.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1f4a966a9727..c40a738dabd3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1322,7 +1322,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> >                 width = bxt_get_dimm_width(val);
> >                 ranks = bxt_get_dimm_ranks(val);
> >  
> > -               DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
> > +               /*
> > +                * Size in register is Gb per DRAM device.
> > +                * Convert to total GB to match the way
> > +                * we report this for non-LP platforms.
> > +                */
> > +               size = size * ranks * 8 / (width ?: 1);
> 
> Should it be /8 for Gbits to GBytes?

It's a hand optimized version of

size*ranks*64 / width / 8

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

Re: [PATCH 05/12] drm/i915: Fix DRAM size reporting for BXT

[Chris Wilson]

Quoting Ville Syrjälä (2019-02-25 20:48:10)
> On Mon, Feb 25, 2019 at 08:35:08PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2019-02-25 20:29:00)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The BXT DUNIT register tells us the size of each DRAM device
> > > in Gb. We want to report the size of the whole DIMM in GB, so
> > > that it matches how we report it for non-LP platforms.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 1f4a966a9727..c40a738dabd3 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1322,7 +1322,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> > >                 width = bxt_get_dimm_width(val);
> > >                 ranks = bxt_get_dimm_ranks(val);
> > >  
> > > -               DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
> > > +               /*
> > > +                * Size in register is Gb per DRAM device.
> > > +                * Convert to total GB to match the way
> > > +                * we report this for non-LP platforms.
> > > +                */
> > > +               size = size * ranks * 8 / (width ?: 1);
> > 
> > Should it be /8 for Gbits to GBytes?
> 
> It's a hand optimized version of
> 
> size*ranks*64 / width / 8

Maybe let the compiler handle the constants, otherwise every time I see
this I'll think it's backwards ;)

Maybe be even
size *= 64 * ranks / (width ?: 1);

/*
 * Size in register is Gb per DRAM device.
 * Convert to total GB to match the way
 * we report this for non-LP platforms.
 */
size /= 8;
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: Fix DRAM size reporting for BXT

[Ville Syrjälä]

On Mon, Feb 25, 2019 at 08:57:45PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2019-02-25 20:48:10)
> > On Mon, Feb 25, 2019 at 08:35:08PM +0000, Chris Wilson wrote:
> > > Quoting Ville Syrjala (2019-02-25 20:29:00)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > The BXT DUNIT register tells us the size of each DRAM device
> > > > in Gb. We want to report the size of the whole DIMM in GB, so
> > > > that it matches how we report it for non-LP platforms.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 1f4a966a9727..c40a738dabd3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1322,7 +1322,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
> > > >                 width = bxt_get_dimm_width(val);
> > > >                 ranks = bxt_get_dimm_ranks(val);
> > > >  
> > > > -               DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks:%d\n",
> > > > +               /*
> > > > +                * Size in register is Gb per DRAM device.
> > > > +                * Convert to total GB to match the way
> > > > +                * we report this for non-LP platforms.
> > > > +                */
> > > > +               size = size * ranks * 8 / (width ?: 1);
> > > 
> > > Should it be /8 for Gbits to GBytes?
> > 
> > It's a hand optimized version of
> > 
> > size*ranks*64 / width / 8
> 
> Maybe let the compiler handle the constants, otherwise every time I see
> this I'll think it's backwards ;)
> 
> Maybe be even
> size *= 64 * ranks / (width ?: 1);
> 
> /*
>  * Size in register is Gb per DRAM device.
>  * Convert to total GB to match the way
>  * we report this for non-LP platforms.
>  */
> size /= 8;

I suppose clarity is better here. I'll have to double check
the types though.

There's the opposite calculation in is_16gb_dimm(). I suppose
that should get the same treatment.

Hmm. Maybe I should even extract something like:
intel_dimm_num_devices() {
	return 64 * ranks / width;
}
and use that everywhere.

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

[PATCH v2 05/12] drm/i915: Fix DRAM size reporting for BXT

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The BXT DUNIT register tells us the size of each DRAM device
in Gb. We want to report the size of the whole DIMM in GB, so
that it matches how we report it for non-LP platforms.

v2: Deobfuscate the math (Chris)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f948d475bdf4..08fb1b1502a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1291,9 +1291,14 @@ static int bxt_get_dimm_ranks(u32 val)
 static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
 			      u32 val)
 {
-	dimm->size = bxt_get_dimm_size(val);
 	dimm->width = bxt_get_dimm_width(val);
 	dimm->ranks = bxt_get_dimm_ranks(val);
+
+	/*
+	 * Size in register is Gb per DRAM device. Convert to total
+	 * GB to match the way we report this for non-LP platforms.
+	 */
+	dimm->size = bxt_get_dimm_size(val) * intel_dimm_num_devices(dimm) / 8;
 }
 
 static int
-- 
2.19.2

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

Re: [PATCH v2 05/12] drm/i915: Fix DRAM size reporting for BXT

[Jani Nikula]

On Tue, 26 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The BXT DUNIT register tells us the size of each DRAM device
> in Gb. We want to report the size of the whole DIMM in GB, so
> that it matches how we report it for non-LP platforms.
>
> v2: Deobfuscate the math (Chris)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f948d475bdf4..08fb1b1502a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1291,9 +1291,14 @@ static int bxt_get_dimm_ranks(u32 val)
>  static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
>  			      u32 val)
>  {
> -	dimm->size = bxt_get_dimm_size(val);
>  	dimm->width = bxt_get_dimm_width(val);
>  	dimm->ranks = bxt_get_dimm_ranks(val);
> +
> +	/*
> +	 * Size in register is Gb per DRAM device. Convert to total
> +	 * GB to match the way we report this for non-LP platforms.
> +	 */
> +	dimm->size = bxt_get_dimm_size(val) * intel_dimm_num_devices(dimm) / 8;

I wouldn't object to {bxt,skl}_get_dimm_size() having a comment about
the unit. Also wouldn't object to renaming the BXT_DRAM_SIZE_<N>GB
macros to GBIT. Even Gb vs. GB seems too subtle at times.

Anyway,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



>  }
>  
>  static int

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] drm/i915: Extract DIMM info on GLK too

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The BXT code for parsing DIMM info works for GLK too. Let's
dig it out even if we might not need it immediately.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c40a738dabd3..9c1ff3eb5775 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1372,11 +1372,11 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	 */
 	dram_info->is_16gb_dimm = !IS_GEN9_LP(dev_priv);
 
-	if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv))
+	if (INTEL_GEN(dev_priv) < 9)
 		return;
 
 	/* Need to calculate bandwidth only for Gen9 */
-	if (IS_BROXTON(dev_priv))
+	if (IS_GEN9_LP(dev_priv))
 		ret = bxt_get_dram_info(dev_priv);
 	else if (IS_GEN(dev_priv, 9))
 		ret = skl_get_dram_info(dev_priv);
-- 
2.19.2

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

Re: [PATCH 06/12] drm/i915: Extract DIMM info on GLK too

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The BXT code for parsing DIMM info works for GLK too. Let's
> dig it out even if we might not need it immediately.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c40a738dabd3..9c1ff3eb5775 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1372,11 +1372,11 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	 */
>  	dram_info->is_16gb_dimm = !IS_GEN9_LP(dev_priv);
>  
> -	if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv))
> +	if (INTEL_GEN(dev_priv) < 9)
>  		return;
>  
>  	/* Need to calculate bandwidth only for Gen9 */
> -	if (IS_BROXTON(dev_priv))
> +	if (IS_GEN9_LP(dev_priv))
>  		ret = bxt_get_dram_info(dev_priv);
>  	else if (IS_GEN(dev_priv, 9))
>  		ret = skl_get_dram_info(dev_priv);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/12] drm/i915: Use dram_dimm_info more

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reduce the code duplication a bit by sharing the same
code for parsing both DIMMs on a channel.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 44 ++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9c1ff3eb5775..3d6a08e907e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1112,25 +1112,30 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
 	return dimm->size * dimm->width / (8 * dimm->ranks ?: 1) == 16;
 }
 
-static int
-skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
+static void
+skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
+		       int channel, char dimm_name, u16 val)
 {
-	u16 tmp_l, tmp_s;
+	dimm->size = skl_get_dimm_size(val);
+	dimm->width = skl_get_dimm_width(val);
+	dimm->ranks = skl_get_dimm_ranks(val);
 
-	tmp_l = val & 0xffff;
-	tmp_s = val >> 16;
+	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
+		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
+		      yesno(skl_is_16gb_dimm(dimm)));
+}
 
-	ch->l_info.size = skl_get_dimm_size(tmp_l);
-	ch->s_info.size = skl_get_dimm_size(tmp_s);
+static int
+skl_dram_get_channel_info(struct dram_channel_info *ch,
+			  int channel, u32 val)
+{
+	skl_dram_get_dimm_info(&ch->l_info, channel, 'L', val & 0xffff);
+	skl_dram_get_dimm_info(&ch->s_info, channel, 'S', val >> 16);
 
-	if (ch->l_info.size == 0 && ch->s_info.size == 0)
+	if (ch->l_info.size == 0 && ch->s_info.size == 0) {
+		DRM_DEBUG_KMS("CH%d not populated\n", channel);
 		return -EINVAL;
-
-	ch->l_info.width = skl_get_dimm_width(tmp_l);
-	ch->s_info.width = skl_get_dimm_width(tmp_s);
-
-	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
-	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
+	}
 
 	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
 		ch->ranks = 2;
@@ -1143,9 +1148,8 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 		skl_is_16gb_dimm(&ch->l_info) ||
 		skl_is_16gb_dimm(&ch->s_info);
 
-	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
-		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
-		      ch->s_info.size, ch->s_info.width, ch->s_info.ranks);
+	DRM_DEBUG_KMS("CH%d ranks: %d, 16Gb DIMMs: %s\n",
+		      channel, ch->ranks, yesno(ch->is_16gb_dimm));
 
 	return 0;
 }
@@ -1165,17 +1169,17 @@ 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;
+	struct dram_channel_info ch0 = {}, ch1 = {};
 	u32 val_ch0, val_ch1;
 	int ret;
 
 	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch0, val_ch0);
+	ret = skl_dram_get_channel_info(&ch0, 0, val_ch0);
 	if (ret == 0)
 		dram_info->num_channels++;
 
 	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch1, val_ch1);
+	ret = skl_dram_get_channel_info(&ch1, 1, val_ch1);
 	if (ret == 0)
 		dram_info->num_channels++;
 
-- 
2.19.2

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

Re: [PATCH 07/12] drm/i915: Use dram_dimm_info more

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reduce the code duplication a bit by sharing the same
> code for parsing both DIMMs on a channel.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 44 ++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9c1ff3eb5775..3d6a08e907e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1112,25 +1112,30 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>  	return dimm->size * dimm->width / (8 * dimm->ranks ?: 1) == 16;
>  }
>  
> -static int
> -skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> +static void
> +skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
> +		       int channel, char dimm_name, u16 val)

Personally I think using chars like this for dimm_name is an unnecessary
micro-optimization for const char * of length 1. But *shrug*.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

PS. What a horrible, horrible diff to review this change produced!


>  {
> -	u16 tmp_l, tmp_s;
> +	dimm->size = skl_get_dimm_size(val);
> +	dimm->width = skl_get_dimm_width(val);
> +	dimm->ranks = skl_get_dimm_ranks(val);
>  
> -	tmp_l = val & 0xffff;
> -	tmp_s = val >> 16;
> +	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
> +		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
> +		      yesno(skl_is_16gb_dimm(dimm)));
> +}
>  
> -	ch->l_info.size = skl_get_dimm_size(tmp_l);
> -	ch->s_info.size = skl_get_dimm_size(tmp_s);
> +static int
> +skl_dram_get_channel_info(struct dram_channel_info *ch,
> +			  int channel, u32 val)
> +{
> +	skl_dram_get_dimm_info(&ch->l_info, channel, 'L', val & 0xffff);
> +	skl_dram_get_dimm_info(&ch->s_info, channel, 'S', val >> 16);
>  
> -	if (ch->l_info.size == 0 && ch->s_info.size == 0)
> +	if (ch->l_info.size == 0 && ch->s_info.size == 0) {
> +		DRM_DEBUG_KMS("CH%d not populated\n", channel);
>  		return -EINVAL;
> -
> -	ch->l_info.width = skl_get_dimm_width(tmp_l);
> -	ch->s_info.width = skl_get_dimm_width(tmp_s);
> -
> -	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> -	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
> +	}
>  
>  	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
>  		ch->ranks = 2;
> @@ -1143,9 +1148,8 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  		skl_is_16gb_dimm(&ch->l_info) ||
>  		skl_is_16gb_dimm(&ch->s_info);
>  
> -	DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
> -		      ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
> -		      ch->s_info.size, ch->s_info.width, ch->s_info.ranks);
> +	DRM_DEBUG_KMS("CH%d ranks: %d, 16Gb DIMMs: %s\n",
> +		      channel, ch->ranks, yesno(ch->is_16gb_dimm));
>  
>  	return 0;
>  }
> @@ -1165,17 +1169,17 @@ 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;
> +	struct dram_channel_info ch0 = {}, ch1 = {};
>  	u32 val_ch0, val_ch1;
>  	int ret;
>  
>  	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch0, val_ch0);
> +	ret = skl_dram_get_channel_info(&ch0, 0, val_ch0);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>  
>  	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch1, val_ch1);
> +	ret = skl_dram_get_channel_info(&ch1, 1, val_ch1);
>  	if (ret == 0)
>  		dram_info->num_channels++;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/12] drm/i915: Generalize intel_is_dram_symmetric()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Decouple intel_is_dram_symmetric() from the raw register values
by comparing just the dram_channel_info structs.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3d6a08e907e3..9261bd0dccd6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1155,14 +1155,12 @@ skl_dram_get_channel_info(struct dram_channel_info *ch,
 }
 
 static bool
-intel_is_dram_symmetric(u32 val_ch0, u32 val_ch1,
-			struct dram_channel_info *ch0)
+intel_is_dram_symmetric(const struct dram_channel_info *ch0,
+			const struct dram_channel_info *ch1)
 {
-	return (val_ch0 == val_ch1 &&
+	return !memcmp(ch0, ch1, sizeof(*ch0)) &&
 		(ch0->s_info.size == 0 ||
-		 (ch0->l_info.size == ch0->s_info.size &&
-		  ch0->l_info.width == ch0->s_info.width &&
-		  ch0->l_info.ranks == ch0->s_info.ranks)));
+		 !memcmp(&ch0->l_info, &ch0->s_info, sizeof(ch0->l_info)));
 }
 
 static int
@@ -1170,16 +1168,16 @@ 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_ch0, val_ch1;
+	u32 val;
 	int ret;
 
-	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch0, 0, val_ch0);
+	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch0, 0, val);
 	if (ret == 0)
 		dram_info->num_channels++;
 
-	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch1, 1, val_ch1);
+	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch1, 1, val);
 	if (ret == 0)
 		dram_info->num_channels++;
 
@@ -1205,12 +1203,10 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 
 	dram_info->is_16gb_dimm = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
 
-	dev_priv->dram_info.symmetric_memory = intel_is_dram_symmetric(val_ch0,
-								       val_ch1,
-								       &ch0);
+	dram_info->symmetric_memory = intel_is_dram_symmetric(&ch0, &ch1);
 
-	DRM_DEBUG_KMS("memory configuration is %sSymmetric memory\n",
-		      dev_priv->dram_info.symmetric_memory ? "" : "not ");
+	DRM_DEBUG_KMS("Memory configuration is symmetric? %s\n",
+		      yesno(dram_info->symmetric_memory));
 	return 0;
 }
 
-- 
2.19.2

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

Re: [PATCH 08/12] drm/i915: Generalize intel_is_dram_symmetric()

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Decouple intel_is_dram_symmetric() from the raw register values
> by comparing just the dram_channel_info structs.

The idea is sound.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3d6a08e907e3..9261bd0dccd6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1155,14 +1155,12 @@ skl_dram_get_channel_info(struct dram_channel_info *ch,
>  }
>  
>  static bool
> -intel_is_dram_symmetric(u32 val_ch0, u32 val_ch1,
> -			struct dram_channel_info *ch0)
> +intel_is_dram_symmetric(const struct dram_channel_info *ch0,
> +			const struct dram_channel_info *ch1)
>  {
> -	return (val_ch0 == val_ch1 &&
> +	return !memcmp(ch0, ch1, sizeof(*ch0)) &&
>  		(ch0->s_info.size == 0 ||
> -		 (ch0->l_info.size == ch0->s_info.size &&
> -		  ch0->l_info.width == ch0->s_info.width &&
> -		  ch0->l_info.ranks == ch0->s_info.ranks)));
> +		 !memcmp(&ch0->l_info, &ch0->s_info, sizeof(ch0->l_info)));

However using memcmp like this gives me the creeps. It's probably going
to work just fine. It's just the knowledge that it's not guaranteed to
work that bugs me.

Oh well.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  }
>  
>  static int
> @@ -1170,16 +1168,16 @@ 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_ch0, val_ch1;
> +	u32 val;
>  	int ret;
>  
> -	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch0, 0, val_ch0);
> +	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch0, 0, val);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>  
> -	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch1, 1, val_ch1);
> +	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch1, 1, val);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>  
> @@ -1205,12 +1203,10 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  
>  	dram_info->is_16gb_dimm = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
>  
> -	dev_priv->dram_info.symmetric_memory = intel_is_dram_symmetric(val_ch0,
> -								       val_ch1,
> -								       &ch0);
> +	dram_info->symmetric_memory = intel_is_dram_symmetric(&ch0, &ch1);
>  
> -	DRM_DEBUG_KMS("memory configuration is %sSymmetric memory\n",
> -		      dev_priv->dram_info.symmetric_memory ? "" : "not ");
> +	DRM_DEBUG_KMS("Memory configuration is symmetric? %s\n",
> +		      yesno(dram_info->symmetric_memory));
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/12] drm/i914: s/l_info/dimm_l/ etc.

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rename the dimm info structs for clarity.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 18 +++++++++---------
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9261bd0dccd6..21413069a480 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1129,24 +1129,24 @@ static int
 skl_dram_get_channel_info(struct dram_channel_info *ch,
 			  int channel, u32 val)
 {
-	skl_dram_get_dimm_info(&ch->l_info, channel, 'L', val & 0xffff);
-	skl_dram_get_dimm_info(&ch->s_info, channel, 'S', val >> 16);
+	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
+	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
 
-	if (ch->l_info.size == 0 && ch->s_info.size == 0) {
+	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
 		DRM_DEBUG_KMS("CH%d not populated\n", channel);
 		return -EINVAL;
 	}
 
-	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
+	if (ch->dimm_l.ranks == 2 || ch->dimm_s.ranks == 2)
 		ch->ranks = 2;
-	else if (ch->l_info.ranks == 1 && ch->s_info.ranks == 1)
+	else if (ch->dimm_l.ranks == 1 && ch->dimm_s.ranks == 1)
 		ch->ranks = 2;
 	else
 		ch->ranks = 1;
 
 	ch->is_16gb_dimm =
-		skl_is_16gb_dimm(&ch->l_info) ||
-		skl_is_16gb_dimm(&ch->s_info);
+		skl_is_16gb_dimm(&ch->dimm_l) ||
+		skl_is_16gb_dimm(&ch->dimm_s);
 
 	DRM_DEBUG_KMS("CH%d ranks: %d, 16Gb DIMMs: %s\n",
 		      channel, ch->ranks, yesno(ch->is_16gb_dimm));
@@ -1159,8 +1159,8 @@ intel_is_dram_symmetric(const struct dram_channel_info *ch0,
 			const struct dram_channel_info *ch1)
 {
 	return !memcmp(ch0, ch1, sizeof(*ch0)) &&
-		(ch0->s_info.size == 0 ||
-		 !memcmp(&ch0->l_info, &ch0->s_info, sizeof(ch0->l_info)));
+		(ch0->dimm_s.size == 0 ||
+		 !memcmp(&ch0->dimm_l, &ch0->dimm_s, sizeof(ch0->dimm_l)));
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fcde09934bb5..89881b68dcb4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2070,7 +2070,7 @@ struct dram_dimm_info {
 };
 
 struct dram_channel_info {
-	struct dram_dimm_info l_info, s_info;
+	struct dram_dimm_info dimm_l, dimm_s;
 	u8 ranks;
 	bool is_16gb_dimm;
 };
-- 
2.19.2

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

Re: [PATCH 09/12] drm/i914: s/l_info/dimm_l/ etc.

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Rename the dimm info structs for clarity.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 18 +++++++++---------
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9261bd0dccd6..21413069a480 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1129,24 +1129,24 @@ static int
>  skl_dram_get_channel_info(struct dram_channel_info *ch,
>  			  int channel, u32 val)
>  {
> -	skl_dram_get_dimm_info(&ch->l_info, channel, 'L', val & 0xffff);
> -	skl_dram_get_dimm_info(&ch->s_info, channel, 'S', val >> 16);
> +	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
> +	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
>  
> -	if (ch->l_info.size == 0 && ch->s_info.size == 0) {
> +	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
>  		DRM_DEBUG_KMS("CH%d not populated\n", channel);
>  		return -EINVAL;
>  	}
>  
> -	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> +	if (ch->dimm_l.ranks == 2 || ch->dimm_s.ranks == 2)
>  		ch->ranks = 2;
> -	else if (ch->l_info.ranks == 1 && ch->s_info.ranks == 1)
> +	else if (ch->dimm_l.ranks == 1 && ch->dimm_s.ranks == 1)
>  		ch->ranks = 2;
>  	else
>  		ch->ranks = 1;
>  
>  	ch->is_16gb_dimm =
> -		skl_is_16gb_dimm(&ch->l_info) ||
> -		skl_is_16gb_dimm(&ch->s_info);
> +		skl_is_16gb_dimm(&ch->dimm_l) ||
> +		skl_is_16gb_dimm(&ch->dimm_s);
>  
>  	DRM_DEBUG_KMS("CH%d ranks: %d, 16Gb DIMMs: %s\n",
>  		      channel, ch->ranks, yesno(ch->is_16gb_dimm));
> @@ -1159,8 +1159,8 @@ intel_is_dram_symmetric(const struct dram_channel_info *ch0,
>  			const struct dram_channel_info *ch1)
>  {
>  	return !memcmp(ch0, ch1, sizeof(*ch0)) &&
> -		(ch0->s_info.size == 0 ||
> -		 !memcmp(&ch0->l_info, &ch0->s_info, sizeof(ch0->l_info)));
> +		(ch0->dimm_s.size == 0 ||
> +		 !memcmp(&ch0->dimm_l, &ch0->dimm_s, sizeof(ch0->dimm_l)));
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fcde09934bb5..89881b68dcb4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2070,7 +2070,7 @@ struct dram_dimm_info {
>  };
>  
>  struct dram_channel_info {
> -	struct dram_dimm_info l_info, s_info;
> +	struct dram_dimm_info dimm_l, dimm_s;
>  	u8 ranks;
>  	bool is_16gb_dimm;
>  };

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/12] drm/i915: Clean up intel_get_dram_info() a bit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Remove the pointless zero initialization of bunch of things
(the thing is kzalloc()ed).

Also throw out the mostly useless on-stack string. I think
it'll be clear enough from the logs that 0 means unknown.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 21413069a480..e3aafe2bf3b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1357,14 +1357,8 @@ static void
 intel_get_dram_info(struct drm_i915_private *dev_priv)
 {
 	struct dram_info *dram_info = &dev_priv->dram_info;
-	char bandwidth_str[32];
 	int ret;
 
-	dram_info->valid = false;
-	dram_info->ranks = 0;
-	dram_info->bandwidth_kbps = 0;
-	dram_info->num_channels = 0;
-
 	/*
 	 * Assume 16Gb DIMMs are present until proven otherwise.
 	 * This is only used for the level 0 watermark latency
@@ -1385,12 +1379,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	if (ret)
 		return;
 
-	if (dram_info->bandwidth_kbps)
-		sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
-	else
-		sprintf(bandwidth_str, "unknown");
-	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
-		      bandwidth_str, dram_info->num_channels);
+	DRM_DEBUG_KMS("DRAM bandwidth: %u kBps, channels: %u\n",
+		      dram_info->bandwidth_kbps,
+		      dram_info->num_channels);
+
 	DRM_DEBUG_KMS("DRAM ranks: %d, 16Gb DIMMs: %s\n",
 		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
 }
-- 
2.19.2

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

Re: [PATCH 10/12] drm/i915: Clean up intel_get_dram_info() a bit

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Remove the pointless zero initialization of bunch of things
> (the thing is kzalloc()ed).
>
> Also throw out the mostly useless on-stack string. I think
> it'll be clear enough from the logs that 0 means unknown.

Yeah. Alternatively you could just do DRM_DEBUG_KMS in both if branches
instead of using a buffer.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 21413069a480..e3aafe2bf3b7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1357,14 +1357,8 @@ static void
>  intel_get_dram_info(struct drm_i915_private *dev_priv)
>  {
>  	struct dram_info *dram_info = &dev_priv->dram_info;
> -	char bandwidth_str[32];
>  	int ret;
>  
> -	dram_info->valid = false;
> -	dram_info->ranks = 0;
> -	dram_info->bandwidth_kbps = 0;
> -	dram_info->num_channels = 0;
> -
>  	/*
>  	 * Assume 16Gb DIMMs are present until proven otherwise.
>  	 * This is only used for the level 0 watermark latency
> @@ -1385,12 +1379,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	if (ret)
>  		return;
>  
> -	if (dram_info->bandwidth_kbps)
> -		sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
> -	else
> -		sprintf(bandwidth_str, "unknown");
> -	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
> -		      bandwidth_str, dram_info->num_channels);
> +	DRM_DEBUG_KMS("DRAM bandwidth: %u kBps, channels: %u\n",
> +		      dram_info->bandwidth_kbps,
> +		      dram_info->num_channels);
> +
>  	DRM_DEBUG_KMS("DRAM ranks: %d, 16Gb DIMMs: %s\n",
>  		      dram_info->ranks, yesno(dram_info->is_16gb_dimm));
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] drm/i915: Extract DIMM info on cnl+

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We'll need information about the memory configuration on cnl+ too.
Extend the code to parse the slightly changed register layout.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 69 ++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_reg.h | 17 +++++++-
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e3aafe2bf3b7..95361814b531 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1095,17 +1095,43 @@ static int skl_get_dimm_ranks(u16 val)
 	if (skl_get_dimm_size(val) == 0)
 		return 0;
 
-	switch (val & SKL_DRAM_RANK_MASK) {
-	case SKL_DRAM_RANK_SINGLE:
-	case SKL_DRAM_RANK_DUAL:
-		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
-		return val + 1;
+	val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
+
+	return val + 1;
+}
+
+static int cnl_get_dimm_size(u16 val)
+{
+	return (val & CNL_DRAM_SIZE_MASK) / 2;
+}
+
+static int cnl_get_dimm_width(u16 val)
+{
+	if (cnl_get_dimm_size(val) == 0)
+		return 0;
+
+	switch (val & CNL_DRAM_WIDTH_MASK) {
+	case CNL_DRAM_WIDTH_X8:
+	case CNL_DRAM_WIDTH_X16:
+	case CNL_DRAM_WIDTH_X32:
+		val = (val & CNL_DRAM_WIDTH_MASK) >> CNL_DRAM_WIDTH_SHIFT;
+		return 8 << val;
 	default:
 		MISSING_CASE(val);
 		return 0;
 	}
 }
 
+static int cnl_get_dimm_ranks(u16 val)
+{
+	if (cnl_get_dimm_size(val) == 0)
+		return 0;
+
+	val = (val & CNL_DRAM_RANK_MASK) >> CNL_DRAM_RANK_SHIFT;
+
+	return val + 1;
+}
+
 static bool
 skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
 {
@@ -1113,12 +1139,19 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
 }
 
 static void
-skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
+skl_dram_get_dimm_info(struct drm_i915_private *dev_priv,
+		       struct dram_dimm_info *dimm,
 		       int channel, char dimm_name, u16 val)
 {
-	dimm->size = skl_get_dimm_size(val);
-	dimm->width = skl_get_dimm_width(val);
-	dimm->ranks = skl_get_dimm_ranks(val);
+	if (INTEL_GEN(dev_priv) >= 10) {
+		dimm->size = cnl_get_dimm_size(val);
+		dimm->width = cnl_get_dimm_width(val);
+		dimm->ranks = cnl_get_dimm_ranks(val);
+	} else {
+		dimm->size = skl_get_dimm_size(val);
+		dimm->width = skl_get_dimm_width(val);
+		dimm->ranks = skl_get_dimm_ranks(val);
+	}
 
 	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
 		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
@@ -1126,11 +1159,14 @@ skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
 }
 
 static int
-skl_dram_get_channel_info(struct dram_channel_info *ch,
+skl_dram_get_channel_info(struct drm_i915_private *dev_priv,
+			  struct dram_channel_info *ch,
 			  int channel, u32 val)
 {
-	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
-	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
+	skl_dram_get_dimm_info(dev_priv, &ch->dimm_l,
+			       channel, 'L', val & 0xffff);
+	skl_dram_get_dimm_info(dev_priv, &ch->dimm_s,
+			       channel, 'S', val >> 16);
 
 	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
 		DRM_DEBUG_KMS("CH%d not populated\n", channel);
@@ -1172,12 +1208,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 	int ret;
 
 	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch0, 0, val);
+	ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
 	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, 1, val);
+	ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
 	if (ret == 0)
 		dram_info->num_channels++;
 
@@ -1369,13 +1405,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) < 9)
 		return;
 
-	/* Need to calculate bandwidth only for Gen9 */
 	if (IS_GEN9_LP(dev_priv))
 		ret = bxt_get_dram_info(dev_priv);
-	else if (IS_GEN(dev_priv, 9))
-		ret = skl_get_dram_info(dev_priv);
 	else
-		ret = skl_dram_get_channels_info(dev_priv);
+		ret = skl_get_dram_info(dev_priv);
 	if (ret)
 		return;
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 730bb1917fd1..b35b0220764f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9875,8 +9875,21 @@ enum skl_power_gate {
 #define  SKL_DRAM_WIDTH_X32			(0x2 << 8)
 #define  SKL_DRAM_RANK_MASK			(0x1 << 10)
 #define  SKL_DRAM_RANK_SHIFT			10
-#define  SKL_DRAM_RANK_SINGLE			(0x0 << 10)
-#define  SKL_DRAM_RANK_DUAL			(0x1 << 10)
+#define  SKL_DRAM_RANK_1			(0x0 << 10)
+#define  SKL_DRAM_RANK_2			(0x1 << 10)
+#define  SKL_DRAM_RANK_MASK			(0x1 << 10)
+#define  CNL_DRAM_SIZE_MASK			0x7F
+#define  CNL_DRAM_WIDTH_MASK			(0x3 << 7)
+#define  CNL_DRAM_WIDTH_SHIFT			7
+#define  CNL_DRAM_WIDTH_X8			(0x0 << 7)
+#define  CNL_DRAM_WIDTH_X16			(0x1 << 7)
+#define  CNL_DRAM_WIDTH_X32			(0x2 << 7)
+#define  CNL_DRAM_RANK_MASK			(0x3 << 9)
+#define  CNL_DRAM_RANK_SHIFT			9
+#define  CNL_DRAM_RANK_1			(0x0 << 9)
+#define  CNL_DRAM_RANK_2			(0x1 << 9)
+#define  CNL_DRAM_RANK_3			(0x2 << 9)
+#define  CNL_DRAM_RANK_4			(0x3 << 9)
 
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
  * since on HSW we can't write to it using I915_WRITE. */
-- 
2.19.2

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

Re: [PATCH 11/12] drm/i915: Extract DIMM info on cnl+

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We'll need information about the memory configuration on cnl+ too.
> Extend the code to parse the slightly changed register layout.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 69 ++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_reg.h | 17 +++++++-
>  2 files changed, 66 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e3aafe2bf3b7..95361814b531 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1095,17 +1095,43 @@ static int skl_get_dimm_ranks(u16 val)
>  	if (skl_get_dimm_size(val) == 0)
>  		return 0;
>  
> -	switch (val & SKL_DRAM_RANK_MASK) {
> -	case SKL_DRAM_RANK_SINGLE:
> -	case SKL_DRAM_RANK_DUAL:
> -		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> -		return val + 1;
> +	val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> +
> +	return val + 1;

This part is a bit out of place. Rebase fail?

> +}
> +
> +static int cnl_get_dimm_size(u16 val)
> +{
> +	return (val & CNL_DRAM_SIZE_MASK) / 2;

Multiples of 0.5 GB... what an odd unit. What if there's an odd value?

> +}
> +
> +static int cnl_get_dimm_width(u16 val)
> +{
> +	if (cnl_get_dimm_size(val) == 0)
> +		return 0;
> +
> +	switch (val & CNL_DRAM_WIDTH_MASK) {
> +	case CNL_DRAM_WIDTH_X8:
> +	case CNL_DRAM_WIDTH_X16:
> +	case CNL_DRAM_WIDTH_X32:
> +		val = (val & CNL_DRAM_WIDTH_MASK) >> CNL_DRAM_WIDTH_SHIFT;
> +		return 8 << val;
>  	default:
>  		MISSING_CASE(val);
>  		return 0;
>  	}
>  }
>  
> +static int cnl_get_dimm_ranks(u16 val)
> +{
> +	if (cnl_get_dimm_size(val) == 0)
> +		return 0;
> +
> +	val = (val & CNL_DRAM_RANK_MASK) >> CNL_DRAM_RANK_SHIFT;
> +
> +	return val + 1;
> +}
> +
>  static bool
>  skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>  {
> @@ -1113,12 +1139,19 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>  }
>  
>  static void
> -skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
> +skl_dram_get_dimm_info(struct drm_i915_private *dev_priv,
> +		       struct dram_dimm_info *dimm,
>  		       int channel, char dimm_name, u16 val)
>  {
> -	dimm->size = skl_get_dimm_size(val);
> -	dimm->width = skl_get_dimm_width(val);
> -	dimm->ranks = skl_get_dimm_ranks(val);
> +	if (INTEL_GEN(dev_priv) >= 10) {
> +		dimm->size = cnl_get_dimm_size(val);
> +		dimm->width = cnl_get_dimm_width(val);
> +		dimm->ranks = cnl_get_dimm_ranks(val);
> +	} else {
> +		dimm->size = skl_get_dimm_size(val);
> +		dimm->width = skl_get_dimm_width(val);
> +		dimm->ranks = skl_get_dimm_ranks(val);
> +	}
>  
>  	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
>  		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
> @@ -1126,11 +1159,14 @@ skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
>  }
>  
>  static int
> -skl_dram_get_channel_info(struct dram_channel_info *ch,
> +skl_dram_get_channel_info(struct drm_i915_private *dev_priv,
> +			  struct dram_channel_info *ch,
>  			  int channel, u32 val)
>  {
> -	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
> -	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
> +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_l,
> +			       channel, 'L', val & 0xffff);
> +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_s,
> +			       channel, 'S', val >> 16);
>  
>  	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
>  		DRM_DEBUG_KMS("CH%d not populated\n", channel);
> @@ -1172,12 +1208,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  	int ret;
>  
>  	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch0, 0, val);
> +	ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
>  	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, 1, val);
> +	ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>  
> @@ -1369,13 +1405,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) < 9)
>  		return;
>  
> -	/* Need to calculate bandwidth only for Gen9 */
>  	if (IS_GEN9_LP(dev_priv))
>  		ret = bxt_get_dram_info(dev_priv);
> -	else if (IS_GEN(dev_priv, 9))
> -		ret = skl_get_dram_info(dev_priv);
>  	else
> -		ret = skl_dram_get_channels_info(dev_priv);
> +		ret = skl_get_dram_info(dev_priv);

The part that's hidden here is the use of the common parts in
skl_get_dram_info() and in particular
SKL_MEMORY_FREQ_MULTIPLIER_HZ. Spec says "The value is given in units of
133.33 MHz". But it says the same for SKL too. What am I missing?

Tentative Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  	if (ret)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 730bb1917fd1..b35b0220764f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9875,8 +9875,21 @@ enum skl_power_gate {
>  #define  SKL_DRAM_WIDTH_X32			(0x2 << 8)
>  #define  SKL_DRAM_RANK_MASK			(0x1 << 10)
>  #define  SKL_DRAM_RANK_SHIFT			10
> -#define  SKL_DRAM_RANK_SINGLE			(0x0 << 10)
> -#define  SKL_DRAM_RANK_DUAL			(0x1 << 10)
> +#define  SKL_DRAM_RANK_1			(0x0 << 10)
> +#define  SKL_DRAM_RANK_2			(0x1 << 10)
> +#define  SKL_DRAM_RANK_MASK			(0x1 << 10)
> +#define  CNL_DRAM_SIZE_MASK			0x7F
> +#define  CNL_DRAM_WIDTH_MASK			(0x3 << 7)
> +#define  CNL_DRAM_WIDTH_SHIFT			7
> +#define  CNL_DRAM_WIDTH_X8			(0x0 << 7)
> +#define  CNL_DRAM_WIDTH_X16			(0x1 << 7)
> +#define  CNL_DRAM_WIDTH_X32			(0x2 << 7)
> +#define  CNL_DRAM_RANK_MASK			(0x3 << 9)
> +#define  CNL_DRAM_RANK_SHIFT			9
> +#define  CNL_DRAM_RANK_1			(0x0 << 9)
> +#define  CNL_DRAM_RANK_2			(0x1 << 9)
> +#define  CNL_DRAM_RANK_3			(0x2 << 9)
> +#define  CNL_DRAM_RANK_4			(0x3 << 9)
>  
>  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>   * since on HSW we can't write to it using I915_WRITE. */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extract DIMM info on cnl+

[Ville Syrjälä]

On Tue, Mar 05, 2019 at 06:16:57PM +0200, Jani Nikula wrote:
> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We'll need information about the memory configuration on cnl+ too.
> > Extend the code to parse the slightly changed register layout.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 69 ++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/i915_reg.h | 17 +++++++-
> >  2 files changed, 66 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e3aafe2bf3b7..95361814b531 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1095,17 +1095,43 @@ static int skl_get_dimm_ranks(u16 val)
> >  	if (skl_get_dimm_size(val) == 0)
> >  		return 0;
> >  
> > -	switch (val & SKL_DRAM_RANK_MASK) {
> > -	case SKL_DRAM_RANK_SINGLE:
> > -	case SKL_DRAM_RANK_DUAL:
> > -		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> > -		return val + 1;
> > +	val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> > +
> > +	return val + 1;
> 
> This part is a bit out of place. Rebase fail?

Aye. Moved to patch 02 where it belongs.

> 
> > +}
> > +
> > +static int cnl_get_dimm_size(u16 val)
> > +{
> > +	return (val & CNL_DRAM_SIZE_MASK) / 2;
> 
> Multiples of 0.5 GB... what an odd unit. What if there's an odd value?

The worst thing that could happen is that the 16 Gb detection
gives us the wrong answer. The other thing is that we'd print
out the wrong size.

I'm not sure if there is any chance of having such oddly sized
DRAM chips on any modern DIMM that we'd hit this. I didn't
really want to change everything just for this at this time.
We can always revisit it later if necesary.

> 
> > +}
> > +
> > +static int cnl_get_dimm_width(u16 val)
> > +{
> > +	if (cnl_get_dimm_size(val) == 0)
> > +		return 0;
> > +
> > +	switch (val & CNL_DRAM_WIDTH_MASK) {
> > +	case CNL_DRAM_WIDTH_X8:
> > +	case CNL_DRAM_WIDTH_X16:
> > +	case CNL_DRAM_WIDTH_X32:
> > +		val = (val & CNL_DRAM_WIDTH_MASK) >> CNL_DRAM_WIDTH_SHIFT;
> > +		return 8 << val;
> >  	default:
> >  		MISSING_CASE(val);
> >  		return 0;
> >  	}
> >  }
> >  
> > +static int cnl_get_dimm_ranks(u16 val)
> > +{
> > +	if (cnl_get_dimm_size(val) == 0)
> > +		return 0;
> > +
> > +	val = (val & CNL_DRAM_RANK_MASK) >> CNL_DRAM_RANK_SHIFT;
> > +
> > +	return val + 1;
> > +}
> > +
> >  static bool
> >  skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
> >  {
> > @@ -1113,12 +1139,19 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
> >  }
> >  
> >  static void
> > -skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
> > +skl_dram_get_dimm_info(struct drm_i915_private *dev_priv,
> > +		       struct dram_dimm_info *dimm,
> >  		       int channel, char dimm_name, u16 val)
> >  {
> > -	dimm->size = skl_get_dimm_size(val);
> > -	dimm->width = skl_get_dimm_width(val);
> > -	dimm->ranks = skl_get_dimm_ranks(val);
> > +	if (INTEL_GEN(dev_priv) >= 10) {
> > +		dimm->size = cnl_get_dimm_size(val);
> > +		dimm->width = cnl_get_dimm_width(val);
> > +		dimm->ranks = cnl_get_dimm_ranks(val);
> > +	} else {
> > +		dimm->size = skl_get_dimm_size(val);
> > +		dimm->width = skl_get_dimm_width(val);
> > +		dimm->ranks = skl_get_dimm_ranks(val);
> > +	}
> >  
> >  	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
> >  		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
> > @@ -1126,11 +1159,14 @@ skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
> >  }
> >  
> >  static int
> > -skl_dram_get_channel_info(struct dram_channel_info *ch,
> > +skl_dram_get_channel_info(struct drm_i915_private *dev_priv,
> > +			  struct dram_channel_info *ch,
> >  			  int channel, u32 val)
> >  {
> > -	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
> > -	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
> > +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_l,
> > +			       channel, 'L', val & 0xffff);
> > +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_s,
> > +			       channel, 'S', val >> 16);
> >  
> >  	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
> >  		DRM_DEBUG_KMS("CH%d not populated\n", channel);
> > @@ -1172,12 +1208,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> >  	int ret;
> >  
> >  	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > -	ret = skl_dram_get_channel_info(&ch0, 0, val);
> > +	ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
> >  	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, 1, val);
> > +	ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
> >  	if (ret == 0)
> >  		dram_info->num_channels++;
> >  
> > @@ -1369,13 +1405,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
> >  	if (INTEL_GEN(dev_priv) < 9)
> >  		return;
> >  
> > -	/* Need to calculate bandwidth only for Gen9 */
> >  	if (IS_GEN9_LP(dev_priv))
> >  		ret = bxt_get_dram_info(dev_priv);
> > -	else if (IS_GEN(dev_priv, 9))
> > -		ret = skl_get_dram_info(dev_priv);
> >  	else
> > -		ret = skl_dram_get_channels_info(dev_priv);
> > +		ret = skl_get_dram_info(dev_priv);
> 
> The part that's hidden here is the use of the common parts in
> skl_get_dram_info() and in particular
> SKL_MEMORY_FREQ_MULTIPLIER_HZ. Spec says "The value is given in units of
> 133.33 MHz". But it says the same for SKL too. What am I missing?

The whole DRAM clocking is a bit of a mystery to me. So far I didn't
find any good docs on the subject. The registers talk about QCLK (which
is the data clock IIUC) but bunch of stuff is interested in the DCLK
(the command clock) though. I guess there's 1:1 relationship, and I
suppose the factor of two here is maybe just due to the DDR.

> 
> Tentative Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> >  	if (ret)
> >  		return;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 730bb1917fd1..b35b0220764f 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9875,8 +9875,21 @@ enum skl_power_gate {
> >  #define  SKL_DRAM_WIDTH_X32			(0x2 << 8)
> >  #define  SKL_DRAM_RANK_MASK			(0x1 << 10)
> >  #define  SKL_DRAM_RANK_SHIFT			10
> > -#define  SKL_DRAM_RANK_SINGLE			(0x0 << 10)
> > -#define  SKL_DRAM_RANK_DUAL			(0x1 << 10)
> > +#define  SKL_DRAM_RANK_1			(0x0 << 10)
> > +#define  SKL_DRAM_RANK_2			(0x1 << 10)
> > +#define  SKL_DRAM_RANK_MASK			(0x1 << 10)
> > +#define  CNL_DRAM_SIZE_MASK			0x7F
> > +#define  CNL_DRAM_WIDTH_MASK			(0x3 << 7)
> > +#define  CNL_DRAM_WIDTH_SHIFT			7
> > +#define  CNL_DRAM_WIDTH_X8			(0x0 << 7)
> > +#define  CNL_DRAM_WIDTH_X16			(0x1 << 7)
> > +#define  CNL_DRAM_WIDTH_X32			(0x2 << 7)
> > +#define  CNL_DRAM_RANK_MASK			(0x3 << 9)
> > +#define  CNL_DRAM_RANK_SHIFT			9
> > +#define  CNL_DRAM_RANK_1			(0x0 << 9)
> > +#define  CNL_DRAM_RANK_2			(0x1 << 9)
> > +#define  CNL_DRAM_RANK_3			(0x2 << 9)
> > +#define  CNL_DRAM_RANK_4			(0x3 << 9)
> >  
> >  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
> >   * since on HSW we can't write to it using I915_WRITE. */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

Re: [PATCH 11/12] drm/i915: Extract DIMM info on cnl+

[Jani Nikula]

On Wed, 06 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Mar 05, 2019 at 06:16:57PM +0200, Jani Nikula wrote:
>> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We'll need information about the memory configuration on cnl+ too.
>> > Extend the code to parse the slightly changed register layout.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c | 69 ++++++++++++++++++++++++---------
>> >  drivers/gpu/drm/i915/i915_reg.h | 17 +++++++-
>> >  2 files changed, 66 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index e3aafe2bf3b7..95361814b531 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -1095,17 +1095,43 @@ static int skl_get_dimm_ranks(u16 val)
>> >  	if (skl_get_dimm_size(val) == 0)
>> >  		return 0;
>> >  
>> > -	switch (val & SKL_DRAM_RANK_MASK) {
>> > -	case SKL_DRAM_RANK_SINGLE:
>> > -	case SKL_DRAM_RANK_DUAL:
>> > -		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
>> > -		return val + 1;
>> > +	val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
>> > +
>> > +	return val + 1;
>> 
>> This part is a bit out of place. Rebase fail?
>
> Aye. Moved to patch 02 where it belongs.
>
>> 
>> > +}
>> > +
>> > +static int cnl_get_dimm_size(u16 val)
>> > +{
>> > +	return (val & CNL_DRAM_SIZE_MASK) / 2;
>> 
>> Multiples of 0.5 GB... what an odd unit. What if there's an odd value?
>
> The worst thing that could happen is that the 16 Gb detection
> gives us the wrong answer. The other thing is that we'd print
> out the wrong size.
>
> I'm not sure if there is any chance of having such oddly sized
> DRAM chips on any modern DIMM that we'd hit this. I didn't
> really want to change everything just for this at this time.
> We can always revisit it later if necesary.

Ack.

>
>> 
>> > +}
>> > +
>> > +static int cnl_get_dimm_width(u16 val)
>> > +{
>> > +	if (cnl_get_dimm_size(val) == 0)
>> > +		return 0;
>> > +
>> > +	switch (val & CNL_DRAM_WIDTH_MASK) {
>> > +	case CNL_DRAM_WIDTH_X8:
>> > +	case CNL_DRAM_WIDTH_X16:
>> > +	case CNL_DRAM_WIDTH_X32:
>> > +		val = (val & CNL_DRAM_WIDTH_MASK) >> CNL_DRAM_WIDTH_SHIFT;
>> > +		return 8 << val;
>> >  	default:
>> >  		MISSING_CASE(val);
>> >  		return 0;
>> >  	}
>> >  }
>> >  
>> > +static int cnl_get_dimm_ranks(u16 val)
>> > +{
>> > +	if (cnl_get_dimm_size(val) == 0)
>> > +		return 0;
>> > +
>> > +	val = (val & CNL_DRAM_RANK_MASK) >> CNL_DRAM_RANK_SHIFT;
>> > +
>> > +	return val + 1;
>> > +}
>> > +
>> >  static bool
>> >  skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>> >  {
>> > @@ -1113,12 +1139,19 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
>> >  }
>> >  
>> >  static void
>> > -skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
>> > +skl_dram_get_dimm_info(struct drm_i915_private *dev_priv,
>> > +		       struct dram_dimm_info *dimm,
>> >  		       int channel, char dimm_name, u16 val)
>> >  {
>> > -	dimm->size = skl_get_dimm_size(val);
>> > -	dimm->width = skl_get_dimm_width(val);
>> > -	dimm->ranks = skl_get_dimm_ranks(val);
>> > +	if (INTEL_GEN(dev_priv) >= 10) {
>> > +		dimm->size = cnl_get_dimm_size(val);
>> > +		dimm->width = cnl_get_dimm_width(val);
>> > +		dimm->ranks = cnl_get_dimm_ranks(val);
>> > +	} else {
>> > +		dimm->size = skl_get_dimm_size(val);
>> > +		dimm->width = skl_get_dimm_width(val);
>> > +		dimm->ranks = skl_get_dimm_ranks(val);
>> > +	}
>> >  
>> >  	DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
>> >  		      channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
>> > @@ -1126,11 +1159,14 @@ skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
>> >  }
>> >  
>> >  static int
>> > -skl_dram_get_channel_info(struct dram_channel_info *ch,
>> > +skl_dram_get_channel_info(struct drm_i915_private *dev_priv,
>> > +			  struct dram_channel_info *ch,
>> >  			  int channel, u32 val)
>> >  {
>> > -	skl_dram_get_dimm_info(&ch->dimm_l, channel, 'L', val & 0xffff);
>> > -	skl_dram_get_dimm_info(&ch->dimm_s, channel, 'S', val >> 16);
>> > +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_l,
>> > +			       channel, 'L', val & 0xffff);
>> > +	skl_dram_get_dimm_info(dev_priv, &ch->dimm_s,
>> > +			       channel, 'S', val >> 16);
>> >  
>> >  	if (ch->dimm_l.size == 0 && ch->dimm_s.size == 0) {
>> >  		DRM_DEBUG_KMS("CH%d not populated\n", channel);
>> > @@ -1172,12 +1208,12 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>> >  	int ret;
>> >  
>> >  	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>> > -	ret = skl_dram_get_channel_info(&ch0, 0, val);
>> > +	ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
>> >  	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, 1, val);
>> > +	ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
>> >  	if (ret == 0)
>> >  		dram_info->num_channels++;
>> >  
>> > @@ -1369,13 +1405,10 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>> >  	if (INTEL_GEN(dev_priv) < 9)
>> >  		return;
>> >  
>> > -	/* Need to calculate bandwidth only for Gen9 */
>> >  	if (IS_GEN9_LP(dev_priv))
>> >  		ret = bxt_get_dram_info(dev_priv);
>> > -	else if (IS_GEN(dev_priv, 9))
>> > -		ret = skl_get_dram_info(dev_priv);
>> >  	else
>> > -		ret = skl_dram_get_channels_info(dev_priv);
>> > +		ret = skl_get_dram_info(dev_priv);
>> 
>> The part that's hidden here is the use of the common parts in
>> skl_get_dram_info() and in particular
>> SKL_MEMORY_FREQ_MULTIPLIER_HZ. Spec says "The value is given in units of
>> 133.33 MHz". But it says the same for SKL too. What am I missing?
>
> The whole DRAM clocking is a bit of a mystery to me. So far I didn't
> find any good docs on the subject. The registers talk about QCLK (which
> is the data clock IIUC) but bunch of stuff is interested in the DCLK
> (the command clock) though. I guess there's 1:1 relationship, and I
> suppose the factor of two here is maybe just due to the DDR.

Okay. Let's go with this then. Strike the tentative below.

BR,
Jani.

>
>> 
>> Tentative Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> >  	if (ret)
>> >  		return;
>> >  
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index 730bb1917fd1..b35b0220764f 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -9875,8 +9875,21 @@ enum skl_power_gate {
>> >  #define  SKL_DRAM_WIDTH_X32			(0x2 << 8)
>> >  #define  SKL_DRAM_RANK_MASK			(0x1 << 10)
>> >  #define  SKL_DRAM_RANK_SHIFT			10
>> > -#define  SKL_DRAM_RANK_SINGLE			(0x0 << 10)
>> > -#define  SKL_DRAM_RANK_DUAL			(0x1 << 10)
>> > +#define  SKL_DRAM_RANK_1			(0x0 << 10)
>> > +#define  SKL_DRAM_RANK_2			(0x1 << 10)
>> > +#define  SKL_DRAM_RANK_MASK			(0x1 << 10)
>> > +#define  CNL_DRAM_SIZE_MASK			0x7F
>> > +#define  CNL_DRAM_WIDTH_MASK			(0x3 << 7)
>> > +#define  CNL_DRAM_WIDTH_SHIFT			7
>> > +#define  CNL_DRAM_WIDTH_X8			(0x0 << 7)
>> > +#define  CNL_DRAM_WIDTH_X16			(0x1 << 7)
>> > +#define  CNL_DRAM_WIDTH_X32			(0x2 << 7)
>> > +#define  CNL_DRAM_RANK_MASK			(0x3 << 9)
>> > +#define  CNL_DRAM_RANK_SHIFT			9
>> > +#define  CNL_DRAM_RANK_1			(0x0 << 9)
>> > +#define  CNL_DRAM_RANK_2			(0x1 << 9)
>> > +#define  CNL_DRAM_RANK_3			(0x2 << 9)
>> > +#define  CNL_DRAM_RANK_4			(0x3 << 9)
>> >  
>> >  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>> >   * since on HSW we can't write to it using I915_WRITE. */
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/12] drm/i915: Read out memory type

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We'll need to know the memory type in the system for some
bandwidth limitations and whatnot. Let's read that out on
gen9+.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 83 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h |  7 +++
 drivers/gpu/drm/i915/i915_reg.h | 13 ++++++
 3 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 95361814b531..5f7e868b44f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1068,6 +1068,26 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+#define DRAM_TYPE_STR(type) [INTEL_DRAM_ ## type] = #type
+
+static const char *intel_dram_type_str(enum intel_dram_type type)
+{
+	static const char * const str[] = {
+		DRAM_TYPE_STR(UNKNOWN),
+		DRAM_TYPE_STR(DDR3),
+		DRAM_TYPE_STR(DDR4),
+		DRAM_TYPE_STR(LPDDR3),
+		DRAM_TYPE_STR(LPDDR4),
+	};
+
+	if (type >= ARRAY_SIZE(str))
+		type = INTEL_DRAM_UNKNOWN;
+
+	return str[type];
+}
+
+#undef DRAM_TYPE_STR
+
 static int skl_get_dimm_size(u16 val)
 {
 	return val & SKL_DRAM_SIZE_MASK;
@@ -1246,6 +1266,28 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static enum intel_dram_type
+skl_get_dram_type(struct drm_i915_private *dev_priv)
+{
+	u32 val;
+
+	val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
+
+	switch (val & SKL_DRAM_DDR_TYPE_MASK) {
+	case SKL_DRAM_DDR_TYPE_DDR3:
+		return INTEL_DRAM_DDR3;
+	case SKL_DRAM_DDR_TYPE_DDR4:
+		return INTEL_DRAM_DDR4;
+	case SKL_DRAM_DDR_TYPE_LPDDR3:
+		return INTEL_DRAM_LPDDR3;
+	case SKL_DRAM_DDR_TYPE_LPDDR4:
+		return INTEL_DRAM_LPDDR4;
+	default:
+		MISSING_CASE(val);
+		return INTEL_DRAM_UNKNOWN;
+	}
+}
+
 static int
 skl_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1253,6 +1295,9 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
 	u32 mem_freq_khz, val;
 	int ret;
 
+	dram_info->type = skl_get_dram_type(dev_priv);
+	DRM_DEBUG_KMS("DRAM type: %s\n", intel_dram_type_str(dram_info->type));
+
 	ret = skl_dram_get_channels_info(dev_priv);
 	if (ret)
 		return ret;
@@ -1318,6 +1363,26 @@ static int bxt_get_dimm_ranks(u32 val)
 	}
 }
 
+static enum intel_dram_type bxt_get_dimm_type(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return INTEL_DRAM_UNKNOWN;
+
+	switch (val & BXT_DRAM_TYPE_MASK) {
+	case BXT_DRAM_TYPE_DDR3:
+		return INTEL_DRAM_DDR3;
+	case BXT_DRAM_TYPE_LPDDR3:
+		return INTEL_DRAM_LPDDR3;
+	case BXT_DRAM_TYPE_DDR4:
+		return INTEL_DRAM_DDR4;
+	case BXT_DRAM_TYPE_LPDDR4:
+		return INTEL_DRAM_LPDDR4;
+	default:
+		MISSING_CASE(val);
+		return INTEL_DRAM_UNKNOWN;
+	}
+}
+
 static int
 bxt_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1346,6 +1411,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
 	 */
 	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
+		enum intel_dram_type type;
 		u8 size, width, ranks;
 
 		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
@@ -1357,6 +1423,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		size = bxt_get_dimm_size(val);
 		width = bxt_get_dimm_width(val);
 		ranks = bxt_get_dimm_ranks(val);
+		type = bxt_get_dimm_type(val);
 
 		/*
 		 * Size in register is Gb per DRAM device.
@@ -1365,9 +1432,13 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		 */
 		size = size * ranks * 8 / (width ?: 1);
 
-		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
+		WARN_ON(type != INTEL_DRAM_UNKNOWN &&
+			dram_info->type != INTEL_DRAM_UNKNOWN &&
+			dram_info->type != type);
+
+		DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks: %d, type: %s\n",
 			      i - BXT_D_CR_DRP0_DUNIT_START,
-			      size, width, ranks);
+			      size, width, ranks, intel_dram_type_str(type));
 
 		/*
 		 * If any of the channel is single rank channel,
@@ -1378,10 +1449,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 			dram_info->ranks = ranks;
 		else if (ranks == 1)
 			dram_info->ranks = 1;
+
+		if (type != INTEL_DRAM_UNKNOWN)
+			dram_info->type = type;
 	}
 
-	if (dram_info->ranks == 0) {
-		DRM_INFO("couldn't get memory rank information\n");
+	if (dram_info->type == INTEL_DRAM_UNKNOWN ||
+	    dram_info->ranks == 0) {
+		DRM_INFO("couldn't get memory information\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 89881b68dcb4..67a283ad54b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1836,6 +1836,13 @@ struct drm_i915_private {
 		u8 ranks;
 		u32 bandwidth_kbps;
 		bool symmetric_memory;
+		enum intel_dram_type {
+			INTEL_DRAM_UNKNOWN,
+			INTEL_DRAM_DDR3,
+			INTEL_DRAM_DDR4,
+			INTEL_DRAM_LPDDR3,
+			INTEL_DRAM_LPDDR4
+		} type;
 	} dram_info;
 
 	struct i915_runtime_pm runtime_pm;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b35b0220764f..1a6904176fc6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9859,11 +9859,24 @@ enum skl_power_gate {
 #define  BXT_DRAM_SIZE_8GB			(0x2 << 6)
 #define  BXT_DRAM_SIZE_12GB			(0x3 << 6)
 #define  BXT_DRAM_SIZE_16GB			(0x4 << 6)
+#define  BXT_DRAM_TYPE_MASK			(0x7 << 22)
+#define  BXT_DRAM_TYPE_SHIFT			22
+#define  BXT_DRAM_TYPE_DDR3			(0x0 << 6)
+#define  BXT_DRAM_TYPE_LPDDR3			(0x1 << 6)
+#define  BXT_DRAM_TYPE_LPDDR4			(0x2 << 6)
+#define  BXT_DRAM_TYPE_DDR4			(0x4 << 6)
 
 #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
 #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
 #define  SKL_REQ_DATA_MASK			(0xF << 0)
 
+#define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
+#define  SKL_DRAM_DDR_TYPE_MASK			(0x3 << 0)
+#define  SKL_DRAM_DDR_TYPE_DDR4			(0 << 0)
+#define  SKL_DRAM_DDR_TYPE_DDR3			(1 << 0)
+#define  SKL_DRAM_DDR_TYPE_LPDDR3		(2 << 0)
+#define  SKL_DRAM_DDR_TYPE_LPDDR4		(3 << 0)
+
 #define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
 #define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
 #define  SKL_DRAM_S_SHIFT			16
-- 
2.19.2

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

[PATCH v2 12/12] drm/i915: Read out memory type

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We'll need to know the memory type in the system for some
bandwidth limitations and whatnot. Let's read that out on
gen9+.

v2: Rebase

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 84 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h |  7 +++
 drivers/gpu/drm/i915/i915_reg.h | 13 +++++
 3 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 47d1a8734f2d..03ad9a8e32f4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1068,6 +1068,26 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+#define DRAM_TYPE_STR(type) [INTEL_DRAM_ ## type] = #type
+
+static const char *intel_dram_type_str(enum intel_dram_type type)
+{
+	static const char * const str[] = {
+		DRAM_TYPE_STR(UNKNOWN),
+		DRAM_TYPE_STR(DDR3),
+		DRAM_TYPE_STR(DDR4),
+		DRAM_TYPE_STR(LPDDR3),
+		DRAM_TYPE_STR(LPDDR4),
+	};
+
+	if (type >= ARRAY_SIZE(str))
+		type = INTEL_DRAM_UNKNOWN;
+
+	return str[type];
+}
+
+#undef DRAM_TYPE_STR
+
 static int intel_dimm_num_devices(const struct dram_dimm_info *dimm)
 {
 	return dimm->ranks * 64 / (dimm->width ?: 1);
@@ -1252,6 +1272,28 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static enum intel_dram_type
+skl_get_dram_type(struct drm_i915_private *dev_priv)
+{
+	u32 val;
+
+	val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
+
+	switch (val & SKL_DRAM_DDR_TYPE_MASK) {
+	case SKL_DRAM_DDR_TYPE_DDR3:
+		return INTEL_DRAM_DDR3;
+	case SKL_DRAM_DDR_TYPE_DDR4:
+		return INTEL_DRAM_DDR4;
+	case SKL_DRAM_DDR_TYPE_LPDDR3:
+		return INTEL_DRAM_LPDDR3;
+	case SKL_DRAM_DDR_TYPE_LPDDR4:
+		return INTEL_DRAM_LPDDR4;
+	default:
+		MISSING_CASE(val);
+		return INTEL_DRAM_UNKNOWN;
+	}
+}
+
 static int
 skl_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1259,6 +1301,9 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
 	u32 mem_freq_khz, val;
 	int ret;
 
+	dram_info->type = skl_get_dram_type(dev_priv);
+	DRM_DEBUG_KMS("DRAM type: %s\n", intel_dram_type_str(dram_info->type));
+
 	ret = skl_dram_get_channels_info(dev_priv);
 	if (ret)
 		return ret;
@@ -1324,6 +1369,26 @@ static int bxt_get_dimm_ranks(u32 val)
 	}
 }
 
+static enum intel_dram_type bxt_get_dimm_type(u32 val)
+{
+	if (!bxt_get_dimm_size(val))
+		return INTEL_DRAM_UNKNOWN;
+
+	switch (val & BXT_DRAM_TYPE_MASK) {
+	case BXT_DRAM_TYPE_DDR3:
+		return INTEL_DRAM_DDR3;
+	case BXT_DRAM_TYPE_LPDDR3:
+		return INTEL_DRAM_LPDDR3;
+	case BXT_DRAM_TYPE_DDR4:
+		return INTEL_DRAM_DDR4;
+	case BXT_DRAM_TYPE_LPDDR4:
+		return INTEL_DRAM_LPDDR4;
+	default:
+		MISSING_CASE(val);
+		return INTEL_DRAM_UNKNOWN;
+	}
+}
+
 static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
 			      u32 val)
 {
@@ -1366,6 +1431,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 	 */
 	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
 		struct dram_dimm_info dimm;
+		enum intel_dram_type type;
 
 		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
 		if (val == 0xFFFFFFFF)
@@ -1374,10 +1440,16 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		dram_info->num_channels++;
 
 		bxt_get_dimm_info(&dimm, val);
+		type = bxt_get_dimm_type(val);
+
+		WARN_ON(type != INTEL_DRAM_UNKNOWN &&
+			dram_info->type != INTEL_DRAM_UNKNOWN &&
+			dram_info->type != type);
 
-		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
+		DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks: %d, type: %s\n",
 			      i - BXT_D_CR_DRP0_DUNIT_START,
-			      dimm.size, dimm.width, dimm.ranks);
+			      dimm.size, dimm.width, dimm.ranks,
+			      intel_dram_type_str(type));
 
 		/*
 		 * If any of the channel is single rank channel,
@@ -1388,10 +1460,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 			dram_info->ranks = dimm.ranks;
 		else if (dimm.ranks == 1)
 			dram_info->ranks = 1;
+
+		if (type != INTEL_DRAM_UNKNOWN)
+			dram_info->type = type;
 	}
 
-	if (dram_info->ranks == 0) {
-		DRM_INFO("couldn't get memory rank information\n");
+	if (dram_info->type == INTEL_DRAM_UNKNOWN ||
+	    dram_info->ranks == 0) {
+		DRM_INFO("couldn't get memory information\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 89881b68dcb4..67a283ad54b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1836,6 +1836,13 @@ struct drm_i915_private {
 		u8 ranks;
 		u32 bandwidth_kbps;
 		bool symmetric_memory;
+		enum intel_dram_type {
+			INTEL_DRAM_UNKNOWN,
+			INTEL_DRAM_DDR3,
+			INTEL_DRAM_DDR4,
+			INTEL_DRAM_LPDDR3,
+			INTEL_DRAM_LPDDR4
+		} type;
 	} dram_info;
 
 	struct i915_runtime_pm runtime_pm;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b35b0220764f..1a6904176fc6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9859,11 +9859,24 @@ enum skl_power_gate {
 #define  BXT_DRAM_SIZE_8GB			(0x2 << 6)
 #define  BXT_DRAM_SIZE_12GB			(0x3 << 6)
 #define  BXT_DRAM_SIZE_16GB			(0x4 << 6)
+#define  BXT_DRAM_TYPE_MASK			(0x7 << 22)
+#define  BXT_DRAM_TYPE_SHIFT			22
+#define  BXT_DRAM_TYPE_DDR3			(0x0 << 6)
+#define  BXT_DRAM_TYPE_LPDDR3			(0x1 << 6)
+#define  BXT_DRAM_TYPE_LPDDR4			(0x2 << 6)
+#define  BXT_DRAM_TYPE_DDR4			(0x4 << 6)
 
 #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
 #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
 #define  SKL_REQ_DATA_MASK			(0xF << 0)
 
+#define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
+#define  SKL_DRAM_DDR_TYPE_MASK			(0x3 << 0)
+#define  SKL_DRAM_DDR_TYPE_DDR4			(0 << 0)
+#define  SKL_DRAM_DDR_TYPE_DDR3			(1 << 0)
+#define  SKL_DRAM_DDR_TYPE_LPDDR3		(2 << 0)
+#define  SKL_DRAM_DDR_TYPE_LPDDR4		(3 << 0)
+
 #define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
 #define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
 #define  SKL_DRAM_S_SHIFT			16
-- 
2.19.2

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

Re: [PATCH v2 12/12] drm/i915: Read out memory type

[Jani Nikula]

On Tue, 26 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We'll need to know the memory type in the system for some
> bandwidth limitations and whatnot. Let's read that out on
> gen9+.
>
> v2: Rebase
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 84 +++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_drv.h |  7 +++
>  drivers/gpu/drm/i915/i915_reg.h | 13 +++++
>  3 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 47d1a8734f2d..03ad9a8e32f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,6 +1068,26 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> +#define DRAM_TYPE_STR(type) [INTEL_DRAM_ ## type] = #type
> +
> +static const char *intel_dram_type_str(enum intel_dram_type type)
> +{
> +	static const char * const str[] = {
> +		DRAM_TYPE_STR(UNKNOWN),
> +		DRAM_TYPE_STR(DDR3),
> +		DRAM_TYPE_STR(DDR4),
> +		DRAM_TYPE_STR(LPDDR3),
> +		DRAM_TYPE_STR(LPDDR4),
> +	};
> +
> +	if (type >= ARRAY_SIZE(str))
> +		type = INTEL_DRAM_UNKNOWN;
> +
> +	return str[type];
> +}
> +
> +#undef DRAM_TYPE_STR
> +
>  static int intel_dimm_num_devices(const struct dram_dimm_info *dimm)
>  {
>  	return dimm->ranks * 64 / (dimm->width ?: 1);
> @@ -1252,6 +1272,28 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +static enum intel_dram_type
> +skl_get_dram_type(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
> +
> +	switch (val & SKL_DRAM_DDR_TYPE_MASK) {
> +	case SKL_DRAM_DDR_TYPE_DDR3:
> +		return INTEL_DRAM_DDR3;
> +	case SKL_DRAM_DDR_TYPE_DDR4:
> +		return INTEL_DRAM_DDR4;
> +	case SKL_DRAM_DDR_TYPE_LPDDR3:
> +		return INTEL_DRAM_LPDDR3;
> +	case SKL_DRAM_DDR_TYPE_LPDDR4:
> +		return INTEL_DRAM_LPDDR4;
> +	default:
> +		MISSING_CASE(val);
> +		return INTEL_DRAM_UNKNOWN;
> +	}
> +}
> +
>  static int
>  skl_get_dram_info(struct drm_i915_private *dev_priv)
>  {
> @@ -1259,6 +1301,9 @@ skl_get_dram_info(struct drm_i915_private *dev_priv)
>  	u32 mem_freq_khz, val;
>  	int ret;
>  
> +	dram_info->type = skl_get_dram_type(dev_priv);
> +	DRM_DEBUG_KMS("DRAM type: %s\n", intel_dram_type_str(dram_info->type));
> +
>  	ret = skl_dram_get_channels_info(dev_priv);
>  	if (ret)
>  		return ret;
> @@ -1324,6 +1369,26 @@ static int bxt_get_dimm_ranks(u32 val)
>  	}
>  }
>  
> +static enum intel_dram_type bxt_get_dimm_type(u32 val)
> +{
> +	if (!bxt_get_dimm_size(val))
> +		return INTEL_DRAM_UNKNOWN;
> +
> +	switch (val & BXT_DRAM_TYPE_MASK) {
> +	case BXT_DRAM_TYPE_DDR3:
> +		return INTEL_DRAM_DDR3;
> +	case BXT_DRAM_TYPE_LPDDR3:
> +		return INTEL_DRAM_LPDDR3;
> +	case BXT_DRAM_TYPE_DDR4:
> +		return INTEL_DRAM_DDR4;
> +	case BXT_DRAM_TYPE_LPDDR4:
> +		return INTEL_DRAM_LPDDR4;
> +	default:
> +		MISSING_CASE(val);
> +		return INTEL_DRAM_UNKNOWN;
> +	}
> +}
> +
>  static void bxt_get_dimm_info(struct dram_dimm_info *dimm,
>  			      u32 val)
>  {
> @@ -1366,6 +1431,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  	 */
>  	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
>  		struct dram_dimm_info dimm;
> +		enum intel_dram_type type;
>  
>  		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
>  		if (val == 0xFFFFFFFF)
> @@ -1374,10 +1440,16 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  		dram_info->num_channels++;
>  
>  		bxt_get_dimm_info(&dimm, val);
> +		type = bxt_get_dimm_type(val);
> +
> +		WARN_ON(type != INTEL_DRAM_UNKNOWN &&
> +			dram_info->type != INTEL_DRAM_UNKNOWN &&
> +			dram_info->type != type);
>  
> -		DRM_DEBUG_KMS("CH%d DIMM size: %d GB, width: X%d, ranks: %d\n",
> +		DRM_DEBUG_KMS("CH%d DIMM size: % dGB, width: X%d, ranks: %d, type: %s\n",
>  			      i - BXT_D_CR_DRP0_DUNIT_START,
> -			      dimm.size, dimm.width, dimm.ranks);
> +			      dimm.size, dimm.width, dimm.ranks,
> +			      intel_dram_type_str(type));
>  
>  		/*
>  		 * If any of the channel is single rank channel,
> @@ -1388,10 +1460,14 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  			dram_info->ranks = dimm.ranks;
>  		else if (dimm.ranks == 1)
>  			dram_info->ranks = 1;
> +
> +		if (type != INTEL_DRAM_UNKNOWN)
> +			dram_info->type = type;
>  	}
>  
> -	if (dram_info->ranks == 0) {
> -		DRM_INFO("couldn't get memory rank information\n");
> +	if (dram_info->type == INTEL_DRAM_UNKNOWN ||
> +	    dram_info->ranks == 0) {
> +		DRM_INFO("couldn't get memory information\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 89881b68dcb4..67a283ad54b1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1836,6 +1836,13 @@ struct drm_i915_private {
>  		u8 ranks;
>  		u32 bandwidth_kbps;
>  		bool symmetric_memory;
> +		enum intel_dram_type {
> +			INTEL_DRAM_UNKNOWN,
> +			INTEL_DRAM_DDR3,
> +			INTEL_DRAM_DDR4,
> +			INTEL_DRAM_LPDDR3,
> +			INTEL_DRAM_LPDDR4
> +		} type;
>  	} dram_info;
>  
>  	struct i915_runtime_pm runtime_pm;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b35b0220764f..1a6904176fc6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9859,11 +9859,24 @@ enum skl_power_gate {
>  #define  BXT_DRAM_SIZE_8GB			(0x2 << 6)
>  #define  BXT_DRAM_SIZE_12GB			(0x3 << 6)
>  #define  BXT_DRAM_SIZE_16GB			(0x4 << 6)
> +#define  BXT_DRAM_TYPE_MASK			(0x7 << 22)
> +#define  BXT_DRAM_TYPE_SHIFT			22
> +#define  BXT_DRAM_TYPE_DDR3			(0x0 << 6)
> +#define  BXT_DRAM_TYPE_LPDDR3			(0x1 << 6)
> +#define  BXT_DRAM_TYPE_LPDDR4			(0x2 << 6)
> +#define  BXT_DRAM_TYPE_DDR4			(0x4 << 6)

Copy-paste fail with the shifts.

With those fixed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  
>  #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
>  #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
>  #define  SKL_REQ_DATA_MASK			(0xF << 0)
>  
> +#define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
> +#define  SKL_DRAM_DDR_TYPE_MASK			(0x3 << 0)
> +#define  SKL_DRAM_DDR_TYPE_DDR4			(0 << 0)
> +#define  SKL_DRAM_DDR_TYPE_DDR3			(1 << 0)
> +#define  SKL_DRAM_DDR_TYPE_LPDDR3		(2 << 0)
> +#define  SKL_DRAM_DDR_TYPE_LPDDR4		(3 << 0)
> +
>  #define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
>  #define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
>  #define  SKL_DRAM_S_SHIFT			16

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Polish DRAM information readout code

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code
URL   : https://patchwork.freedesktop.org/series/57213/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
00f769077034 drm/i915: Store DIMM rank information as a number
6000a48eb1cf drm/i915: Extract functions to derive SKL+ DIMM info
bafd36480e66 drm/i915: Polish skl_is_16gb_dimm()
22d9a7332e2c drm/i915: Extract BXT DIMM helpers
b7ce42c317b8 drm/i915: Fix DRAM size reporting for BXT
3472026fb73d drm/i915: Extract DIMM info on GLK too
b73f574462db drm/i915: Use dram_dimm_info more
922fbda7b9af drm/i915: Generalize intel_is_dram_symmetric()
24b522fc37d6 drm/i914: s/l_info/dimm_l/ etc.
c01f3a66c0bb drm/i915: Clean up intel_get_dram_info() a bit
e72b3d3ce155 drm/i915: Extract DIMM info on cnl+
51c5b331412d drm/i915: Read out memory type
-:23: ERROR:BRACKET_SPACE: space prohibited before open square bracket '['
#23: FILE: drivers/gpu/drm/i915/i915_drv.c:1071:
+#define DRAM_TYPE_STR(type) [INTEL_DRAM_ ## type] = #type

total: 1 errors, 0 warnings, 0 checks, 171 lines checked

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

✗ Fi.CI.SPARSE: warning for Polish DRAM information readout code

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code
URL   : https://patchwork.freedesktop.org/series/57213/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Store DIMM rank information as a number
-O:drivers/gpu/drm/i915/i915_drv.c:1192:35: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_drv.c:1192:35: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_drv.c:1187:36: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_drv.c:1187:36: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3581:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3576:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract functions to derive SKL+ DIMM info
Okay!

Commit: drm/i915: Polish skl_is_16gb_dimm()
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3576:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3578:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract BXT DIMM helpers
Okay!

Commit: drm/i915: Fix DRAM size reporting for BXT
Okay!

Commit: drm/i915: Extract DIMM info on GLK too
Okay!

Commit: drm/i915: Use dram_dimm_info more
Okay!

Commit: drm/i915: Generalize intel_is_dram_symmetric()
Okay!

Commit: drm/i914: s/l_info/dimm_l/ etc.
Okay!

Commit: drm/i915: Clean up intel_get_dram_info() a bit
Okay!

Commit: drm/i915: Extract DIMM info on cnl+
Okay!

Commit: drm/i915: Read out memory type
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3578:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3585:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for Polish DRAM information readout code

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code
URL   : https://patchwork.freedesktop.org/series/57213/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5659 -> Patchwork_12301
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57213/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12301 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       PASS -> SKIP [fdo#109271]

  * igt@i915_pm_rpm@basic-rte:
    - fi-bsw-kefka:       PASS -> FAIL [fdo#108800]

  * igt@kms_busy@basic-flip-b:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (44 -> 39)
------------------------------

  Missing    (5): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u3 


Build changes
-------------

    * Linux: CI_DRM_5659 -> Patchwork_12301

  CI_DRM_5659: bffea990c63087245e8501df82fd45f24ce6ad1f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4854: 06b0830fb948b9b632342cd26100342aa01cbc79 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12301: 51c5b331412de5a26f602d5bbc2936b5480398c3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

51c5b331412d drm/i915: Read out memory type
e72b3d3ce155 drm/i915: Extract DIMM info on cnl+
c01f3a66c0bb drm/i915: Clean up intel_get_dram_info() a bit
24b522fc37d6 drm/i914: s/l_info/dimm_l/ etc.
922fbda7b9af drm/i915: Generalize intel_is_dram_symmetric()
b73f574462db drm/i915: Use dram_dimm_info more
3472026fb73d drm/i915: Extract DIMM info on GLK too
b7ce42c317b8 drm/i915: Fix DRAM size reporting for BXT
22d9a7332e2c drm/i915: Extract BXT DIMM helpers
bafd36480e66 drm/i915: Polish skl_is_16gb_dimm()
6000a48eb1cf drm/i915: Extract functions to derive SKL+ DIMM info
00f769077034 drm/i915: Store DIMM rank information as a number

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12301/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Polish DRAM information readout code

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code
URL   : https://patchwork.freedesktop.org/series/57213/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5659_full -> Patchwork_12301_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12301_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mocs_settings@mocs-reset-bsd2:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +103

  * igt@gem_mocs_settings@mocs-settings-vebox:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109287] +2

  * igt@gem_softpin@evict-snoop-interruptible:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109312]

  * igt@i915_pm_rpm@basic-rte:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +5

  * igt@i915_pm_rpm@dpms-mode-unset-lpsp:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#107713] / [fdo#108840]

  * igt@kms_atomic_transition@3x-modeset-transitions-nonblocking-fencing:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +9

  * igt@kms_atomic_transition@5x-modeset-transitions-nonblocking:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_atomic_transition@6x-modeset-transitions-nonblocking-fencing:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
    - shard-glk:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_chamelium@hdmi-crc-single:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109284]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x256-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_fbcon_fbt@fbc:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109593]

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +5

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-onoff:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +8

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-indfb-pgflip-blt:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +2

  * igt@kms_invalid_dotclock:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109310]

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-glk:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-none:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-iclb:         PASS -> FAIL [fdo#103166]

  * igt@kms_plane_scaling@pipe-a-scaler-with-rotation:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724]

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109441]

  * igt@kms_vblank@pipe-a-ts-continuation-modeset-hang:
    - shard-apl:          PASS -> FAIL [fdo#104894]

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +5

  * igt@prime_vgem@fence-write-hang:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +11

  * igt@runner@aborted:
    - shard-iclb:         NOTRUN -> FAIL [fdo#109593]

  * igt@v3d_get_param@get-bad-param:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109315]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@gem-idle:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +6

  * igt@i915_pm_rpm@legacy-planes-dpms:
    - shard-iclb:         INCOMPLETE [fdo#108840] / [fdo#109369] -> PASS

  * igt@i915_suspend@sysfs-reader:
    - shard-iclb:         INCOMPLETE [fdo#107713] -> PASS

  * igt@kms_busy@extended-pageflip-hang-newfb-render-a:
    - shard-glk:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS

  * igt@kms_color@pipe-a-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          FAIL [fdo#103232] -> PASS +3

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-iclb:         FAIL [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS +3
    - shard-iclb:         FAIL [fdo#103167] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-apl:          FAIL [fdo#103167] / [fdo#105682] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-apl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-glk:          INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane@plane-position-hole-dpms-pipe-b-planes:
    - shard-snb:          SKIP [fdo#109271] -> PASS +1

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-iclb:         FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          DMESG-FAIL [fdo#105763] -> PASS

  * igt@kms_setmode@basic:
    - shard-hsw:          FAIL [fdo#99912] -> PASS
    - shard-kbl:          FAIL [fdo#99912] -> PASS

  * igt@prime_busy@hang-vebox:
    - shard-hsw:          FAIL [fdo#108807] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@system-suspend-modeset:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> INCOMPLETE [fdo#107713]

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108807]: https://bugs.freedesktop.org/show_bug.cgi?id=108807
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
  [fdo#109310]: https://bugs.freedesktop.org/show_bug.cgi?id=109310
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109369]: https://bugs.freedesktop.org/show_bug.cgi?id=109369
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

    * Linux: CI_DRM_5659 -> Patchwork_12301

  CI_DRM_5659: bffea990c63087245e8501df82fd45f24ce6ad1f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4854: 06b0830fb948b9b632342cd26100342aa01cbc79 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12301: 51c5b331412de5a26f602d5bbc2936b5480398c3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12301/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Polish DRAM information readout code (rev4)

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code (rev4)
URL   : https://patchwork.freedesktop.org/series/57213/
State : failure

== Summary ==

Applying: drm/i915: Store DIMM rank information as a number
Applying: drm/i915: Extract functions to derive SKL+ DIMM info
Applying: drm/i915: Polish skl_is_16gb_dimm()
Applying: drm/i915: Extract BXT DIMM helpers
Applying: drm/i915: Fix DRAM size reporting for BXT
Applying: drm/i915: Extract DIMM info on GLK too
Applying: drm/i915: Use dram_dimm_info more
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.c
Applying: drm/i915: Generalize intel_is_dram_symmetric()
Applying: drm/i914: s/l_info/dimm_l/ etc.
Applying: drm/i915: Clean up intel_get_dram_info() a bit
Applying: drm/i915: Extract DIMM info on cnl+
Applying: drm/i915: Read out memory type
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_drv.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0012 drm/i915: Read out memory type
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Fi.CI.CHECKPATCH: warning for Polish DRAM information readout code (rev5)

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code (rev5)
URL   : https://patchwork.freedesktop.org/series/57213/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
011c8a3291c3 drm/i915: Store DIMM rank information as a number
00c9d4441d97 drm/i915: Extract functions to derive SKL+ DIMM info
7037185e1bfe drm/i915: Polish skl_is_16gb_dimm()
09497aef64c1 drm/i915: Extract BXT DIMM helpers
26478c23b10c drm/i915: Fix DRAM size reporting for BXT
1b42808bab0c drm/i915: Extract DIMM info on GLK too
fb64928b7757 drm/i915: Use dram_dimm_info more
ddb21bce2745 drm/i915: Generalize intel_is_dram_symmetric()
c3619070b3e2 drm/i914: s/l_info/dimm_l/ etc.
20981e507547 drm/i915: Clean up intel_get_dram_info() a bit
3b0ad2d82a0c drm/i915: Extract DIMM info on cnl+
72c9e18ef286 drm/i915: Read out memory type
-:25: ERROR:BRACKET_SPACE: space prohibited before open square bracket '['
#25: FILE: drivers/gpu/drm/i915/i915_drv.c:1071:
+#define DRAM_TYPE_STR(type) [INTEL_DRAM_ ## type] = #type

total: 1 errors, 0 warnings, 0 checks, 167 lines checked

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

✗ Fi.CI.SPARSE: warning for Polish DRAM information readout code (rev5)

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code (rev5)
URL   : https://patchwork.freedesktop.org/series/57213/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Store DIMM rank information as a number
-O:drivers/gpu/drm/i915/i915_drv.c:1192:35: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_drv.c:1192:35: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_drv.c:1187:36: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_drv.c:1187:36: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3581:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3576:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract functions to derive SKL+ DIMM info
Okay!

Commit: drm/i915: Polish skl_is_16gb_dimm()
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3576:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3578:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract BXT DIMM helpers
Okay!

Commit: drm/i915: Fix DRAM size reporting for BXT
Okay!

Commit: drm/i915: Extract DIMM info on GLK too
Okay!

Commit: drm/i915: Use dram_dimm_info more
Okay!

Commit: drm/i915: Generalize intel_is_dram_symmetric()
Okay!

Commit: drm/i914: s/l_info/dimm_l/ etc.
Okay!

Commit: drm/i915: Clean up intel_get_dram_info() a bit
Okay!

Commit: drm/i915: Extract DIMM info on cnl+
Okay!

Commit: drm/i915: Read out memory type
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3578:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3585:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for Polish DRAM information readout code (rev5)

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code (rev5)
URL   : https://patchwork.freedesktop.org/series/57213/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5663 -> Patchwork_12312
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57213/revisions/5/

Known issues
------------

  Here are the changes found in Patchwork_12312 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_busy@basic-flip-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@i915_pm_rpm@module-reload:
    - {fi-icl-y}:         INCOMPLETE [fdo#108840] -> PASS

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       WARN [fdo#109380] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       SKIP [fdo#109271] -> PASS +33

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380


Participating hosts (44 -> 37)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-byt-n2820 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5663 -> Patchwork_12312

  CI_DRM_5663: b3edf5fc71aad143ed214a72901a2c75da5535d2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4859: 1d8f3320cbc06fa73ad1487453a63993f17b9d57 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12312: 72c9e18ef286c47016fe0f17dd9e32ff709a5b6c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

72c9e18ef286 drm/i915: Read out memory type
3b0ad2d82a0c drm/i915: Extract DIMM info on cnl+
20981e507547 drm/i915: Clean up intel_get_dram_info() a bit
c3619070b3e2 drm/i914: s/l_info/dimm_l/ etc.
ddb21bce2745 drm/i915: Generalize intel_is_dram_symmetric()
fb64928b7757 drm/i915: Use dram_dimm_info more
1b42808bab0c drm/i915: Extract DIMM info on GLK too
26478c23b10c drm/i915: Fix DRAM size reporting for BXT
09497aef64c1 drm/i915: Extract BXT DIMM helpers
7037185e1bfe drm/i915: Polish skl_is_16gb_dimm()
00c9d4441d97 drm/i915: Extract functions to derive SKL+ DIMM info
011c8a3291c3 drm/i915: Store DIMM rank information as a number

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12312/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Polish DRAM information readout code (rev5)

[Patchwork]

== Series Details ==

Series: Polish DRAM information readout code (rev5)
URL   : https://patchwork.freedesktop.org/series/57213/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5663_full -> Patchwork_12312_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12312_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_workarounds@suspend-resume:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_busy@basic-modeset-d:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-glk:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-kbl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_color@pipe-b-ctm-max:
    - shard-apl:          PASS -> FAIL [fdo#108147]

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-glk:          PASS -> FAIL [fdo#102887] / [fdo#105363] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-mmap-cpu:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +1

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-wc:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +44

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_vblank@pipe-c-ts-continuation-modeset-hang:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  
#### Possible fixes ####

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          SKIP [fdo#109271] -> PASS

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-hsw:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_color@pipe-c-ctm-red-to-blue:
    - shard-skl:          FAIL [fdo#107201] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-dpms:
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-onscreen:
    - shard-skl:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip_tiling@flip-changes-tiling-yf:
    - shard-skl:          FAIL [fdo#108228] / [fdo#108303] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-none:
    - shard-glk:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_setmode@basic:
    - shard-hsw:          FAIL [fdo#99912] -> PASS

  
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#107201]: https://bugs.freedesktop.org/show_bug.cgi?id=107201
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108228]: https://bugs.freedesktop.org/show_bug.cgi?id=108228
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (6 -> 6)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5663 -> Patchwork_12312

  CI_DRM_5663: b3edf5fc71aad143ed214a72901a2c75da5535d2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4859: 1d8f3320cbc06fa73ad1487453a63993f17b9d57 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12312: 72c9e18ef286c47016fe0f17dd9e32ff709a5b6c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12312/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx