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

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

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