On Wed, 06 Mar 2019, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 05, 2019 at 06:16:57PM +0200, Jani Nikula wrote:
>> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxx>
>>
>> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
