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

Reply via email to