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

[Jani Nikula]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at 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 at 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