| Message ID | 20211013010046.91858-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Remove memory frequency calculation | expand |
On Tue, 2021-10-12 at 18:00 -0700, José Roberto de Souza wrote: > This memory frequency calculated is only used to check if it is zero, > what is not useful as it will never actually be zero. > > Also the calculation is wrong, we should be checking other bit to > select the appropriate frequency multiplier while this code is stuck > with a fixed multiplier. > > So here dropping it as whole. > > v2: > - Also remove memory frequency calculation for gen9 LP platforms > > Cc: Yakui Zhao <yakui.zhao@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") Ops hash should be: 5d0c938ec9cc Will fix in the next version or when applying. > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 8 -------- > drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- > 2 files changed, 2 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a897f4abea0c3..8825f7ac477b6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -11109,12 +11109,6 @@ enum skl_power_gate { > #define DC_STATE_DEBUG_MASK_CORES (1 << 0) > #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) > > -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) > -#define BXT_REQ_DATA_MASK 0x3F > -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 > -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) > -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 > - > #define BXT_D_CR_DRP0_DUNIT8 0x1000 > #define BXT_D_CR_DRP0_DUNIT9 0x1200 > #define BXT_D_CR_DRP0_DUNIT_START 8 > @@ -11145,9 +11139,7 @@ enum skl_power_gate { > #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) > #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) > > -#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 DG1_GEAR_TYPE REG_BIT(16) > > #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > index 30a0cab5eff46..0adadfd9528aa 100644 > --- a/drivers/gpu/drm/i915/intel_dram.c > +++ b/drivers/gpu/drm/i915/intel_dram.c > @@ -244,7 +244,6 @@ static int > skl_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 mem_freq_khz, val; > int ret; > > dram_info->type = skl_get_dram_type(i915); > @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) > if (ret) > return ret; > > - val = intel_uncore_read(&i915->uncore, > - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); > - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * > - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - if (dram_info->num_channels * mem_freq_khz == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > return 0; > } > > @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) > static int bxt_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 dram_channels; > - u32 mem_freq_khz, val; > - u8 num_active_channels, valid_ranks = 0; > + u32 val; > + u8 valid_ranks = 0; > int i; > > - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); > - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * > - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; > - num_active_channels = hweight32(dram_channels); > - > - if (mem_freq_khz * num_active_channels == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > /* > * Now read each DUNIT8/9/10/11 to check the rank of each dimms. > */
On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: > This memory frequency calculated is only used to check if it is zero, > what is not useful as it will never actually be zero. > > Also the calculation is wrong, we should be checking other bit to > select the appropriate frequency multiplier while this code is stuck > with a fixed multiplier. > > So here dropping it as whole. > > v2: > - Also remove memory frequency calculation for gen9 LP platforms > > Cc: Yakui Zhao <yakui.zhao@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 8 -------- > drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- > 2 files changed, 2 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a897f4abea0c3..8825f7ac477b6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -11109,12 +11109,6 @@ enum skl_power_gate { > #define DC_STATE_DEBUG_MASK_CORES (1 << 0) > #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) > > -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) > -#define BXT_REQ_DATA_MASK 0x3F > -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 > -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) > -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 > - > #define BXT_D_CR_DRP0_DUNIT8 0x1000 > #define BXT_D_CR_DRP0_DUNIT9 0x1200 > #define BXT_D_CR_DRP0_DUNIT_START 8 > @@ -11145,9 +11139,7 @@ enum skl_power_gate { > #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) > #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) > > -#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 DG1_GEAR_TYPE REG_BIT(16) > > #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > index 30a0cab5eff46..0adadfd9528aa 100644 > --- a/drivers/gpu/drm/i915/intel_dram.c > +++ b/drivers/gpu/drm/i915/intel_dram.c > @@ -244,7 +244,6 @@ static int > skl_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 mem_freq_khz, val; > int ret; > > dram_info->type = skl_get_dram_type(i915); > @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) > if (ret) > return ret; > > - val = intel_uncore_read(&i915->uncore, > - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); > - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * > - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - if (dram_info->num_channels * mem_freq_khz == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > return 0; > } > > @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) > static int bxt_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 dram_channels; > - u32 mem_freq_khz, val; > - u8 num_active_channels, valid_ranks = 0; > + u32 val; > + u8 valid_ranks = 0; > int i; > > - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); > - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * > - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; > - num_active_channels = hweight32(dram_channels); > - > - if (mem_freq_khz * num_active_channels == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > /* > * Now read each DUNIT8/9/10/11 to check the rank of each dimms. > */ > -- > 2.33.0 >
On 2021/10/13 10:54, Matt Roper wrote: > On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: >> This memory frequency calculated is only used to check if it is zero, >> what is not useful as it will never actually be zero. >> >> Also the calculation is wrong, we should be checking other bit to >> select the appropriate frequency multiplier while this code is stuck >> with a fixed multiplier. >> >> So here dropping it as whole. >> >> v2: >> - Also remove memory frequency calculation for gen9 LP platforms >> >> Cc: Yakui Zhao <yakui.zhao@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") >> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> After removing the check of memory frequency, the EHL SBL can work as expected. Otherwise it will fail some checks in intel_dram_detect because of incorrect memory frequency calculation. Add: Tested-by: Zhao Yakui <yakui.zhao@intel.com> > >> --- >> drivers/gpu/drm/i915/i915_reg.h | 8 -------- >> drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- >> 2 files changed, 2 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index a897f4abea0c3..8825f7ac477b6 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -11109,12 +11109,6 @@ enum skl_power_gate { >> #define DC_STATE_DEBUG_MASK_CORES (1 << 0) >> #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) >> >> -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) >> -#define BXT_REQ_DATA_MASK 0x3F >> -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 >> -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) >> -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 >> - >> #define BXT_D_CR_DRP0_DUNIT8 0x1000 >> #define BXT_D_CR_DRP0_DUNIT9 0x1200 >> #define BXT_D_CR_DRP0_DUNIT_START 8 >> @@ -11145,9 +11139,7 @@ enum skl_power_gate { >> #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) >> #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) >> >> -#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 DG1_GEAR_TYPE REG_BIT(16) >> >> #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) >> diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c >> index 30a0cab5eff46..0adadfd9528aa 100644 >> --- a/drivers/gpu/drm/i915/intel_dram.c >> +++ b/drivers/gpu/drm/i915/intel_dram.c >> @@ -244,7 +244,6 @@ static int >> skl_get_dram_info(struct drm_i915_private *i915) >> { >> struct dram_info *dram_info = &i915->dram_info; >> - u32 mem_freq_khz, val; >> int ret; >> >> dram_info->type = skl_get_dram_type(i915); >> @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) >> if (ret) >> return ret; >> >> - val = intel_uncore_read(&i915->uncore, >> - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); >> - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * >> - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); >> - >> - if (dram_info->num_channels * mem_freq_khz == 0) { >> - drm_info(&i915->drm, >> - "Couldn't get system memory bandwidth\n"); >> - return -EINVAL; >> - } >> - >> return 0; >> } >> >> @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) >> static int bxt_get_dram_info(struct drm_i915_private *i915) >> { >> struct dram_info *dram_info = &i915->dram_info; >> - u32 dram_channels; >> - u32 mem_freq_khz, val; >> - u8 num_active_channels, valid_ranks = 0; >> + u32 val; >> + u8 valid_ranks = 0; >> int i; >> >> - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); >> - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * >> - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); >> - >> - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; >> - num_active_channels = hweight32(dram_channels); >> - >> - if (mem_freq_khz * num_active_channels == 0) { >> - drm_info(&i915->drm, >> - "Couldn't get system memory bandwidth\n"); >> - return -EINVAL; >> - } >> - >> /* >> * Now read each DUNIT8/9/10/11 to check the rank of each dimms. >> */ >> -- >> 2.33.0 >> >
On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: > This memory frequency calculated is only used to check if it is zero, > what is not useful as it will never actually be zero. > > Also the calculation is wrong, we should be checking other bit to > select the appropriate frequency multiplier while this code is stuck > with a fixed multiplier. I don't think the alternate ref clock was ever used. At least I don't recall ever seeing it. The real problem with this is that IIRC this is just the last requested frequency. So on a system with SAGV this will change dynamically. > > So here dropping it as whole. We have a second copy of this in gen6_update_ring_freq(). Rather than removing one and leaving another potentially broken one behind we should probably just consolidate on a single implementation. > > v2: > - Also remove memory frequency calculation for gen9 LP platforms > > Cc: Yakui Zhao <yakui.zhao@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 8 -------- > drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- > 2 files changed, 2 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a897f4abea0c3..8825f7ac477b6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -11109,12 +11109,6 @@ enum skl_power_gate { > #define DC_STATE_DEBUG_MASK_CORES (1 << 0) > #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) > > -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) > -#define BXT_REQ_DATA_MASK 0x3F > -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 > -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) > -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 > - > #define BXT_D_CR_DRP0_DUNIT8 0x1000 > #define BXT_D_CR_DRP0_DUNIT9 0x1200 > #define BXT_D_CR_DRP0_DUNIT_START 8 > @@ -11145,9 +11139,7 @@ enum skl_power_gate { > #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) > #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) > > -#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 DG1_GEAR_TYPE REG_BIT(16) > > #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > index 30a0cab5eff46..0adadfd9528aa 100644 > --- a/drivers/gpu/drm/i915/intel_dram.c > +++ b/drivers/gpu/drm/i915/intel_dram.c > @@ -244,7 +244,6 @@ static int > skl_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 mem_freq_khz, val; > int ret; > > dram_info->type = skl_get_dram_type(i915); > @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) > if (ret) > return ret; > > - val = intel_uncore_read(&i915->uncore, > - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); > - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * > - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - if (dram_info->num_channels * mem_freq_khz == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > return 0; > } > > @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) > static int bxt_get_dram_info(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; > - u32 dram_channels; > - u32 mem_freq_khz, val; > - u8 num_active_channels, valid_ranks = 0; > + u32 val; > + u8 valid_ranks = 0; > int i; > > - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); > - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * > - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > - > - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; > - num_active_channels = hweight32(dram_channels); > - > - if (mem_freq_khz * num_active_channels == 0) { > - drm_info(&i915->drm, > - "Couldn't get system memory bandwidth\n"); > - return -EINVAL; > - } > - > /* > * Now read each DUNIT8/9/10/11 to check the rank of each dimms. > */ > -- > 2.33.0
On Wed, 2021-10-13 at 12:32 +0300, Ville Syrjälä wrote: > On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: > > This memory frequency calculated is only used to check if it is zero, > > what is not useful as it will never actually be zero. > > > > Also the calculation is wrong, we should be checking other bit to > > select the appropriate frequency multiplier while this code is stuck > > with a fixed multiplier. > > I don't think the alternate ref clock was ever used. > At least I don't recall ever seeing it. > > The real problem with this is that IIRC this is just the last > requested frequency. So on a system with SAGV this will > change dynamically. > > > > > So here dropping it as whole. > > We have a second copy of this in gen6_update_ring_freq(). Rather > than removing one and leaving another potentially broken one behind we > should probably just consolidate on a single implementation. gen6_update_ring_freq() is related to GPU frequency not memory, don't look related at all to me. > > > > > v2: > > - Also remove memory frequency calculation for gen9 LP platforms > > > > Cc: Yakui Zhao <yakui.zhao@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 8 -------- > > drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- > > 2 files changed, 2 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index a897f4abea0c3..8825f7ac477b6 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -11109,12 +11109,6 @@ enum skl_power_gate { > > #define DC_STATE_DEBUG_MASK_CORES (1 << 0) > > #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) > > > > -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) > > -#define BXT_REQ_DATA_MASK 0x3F > > -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 > > -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) > > -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 > > - > > #define BXT_D_CR_DRP0_DUNIT8 0x1000 > > #define BXT_D_CR_DRP0_DUNIT9 0x1200 > > #define BXT_D_CR_DRP0_DUNIT_START 8 > > @@ -11145,9 +11139,7 @@ enum skl_power_gate { > > #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) > > #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) > > > > -#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 DG1_GEAR_TYPE REG_BIT(16) > > > > #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > > index 30a0cab5eff46..0adadfd9528aa 100644 > > --- a/drivers/gpu/drm/i915/intel_dram.c > > +++ b/drivers/gpu/drm/i915/intel_dram.c > > @@ -244,7 +244,6 @@ static int > > skl_get_dram_info(struct drm_i915_private *i915) > > { > > struct dram_info *dram_info = &i915->dram_info; > > - u32 mem_freq_khz, val; > > int ret; > > > > dram_info->type = skl_get_dram_type(i915); > > @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) > > if (ret) > > return ret; > > > > - val = intel_uncore_read(&i915->uncore, > > - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); > > - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * > > - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > > - > > - if (dram_info->num_channels * mem_freq_khz == 0) { > > - drm_info(&i915->drm, > > - "Couldn't get system memory bandwidth\n"); > > - return -EINVAL; > > - } > > - > > return 0; > > } > > > > @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) > > static int bxt_get_dram_info(struct drm_i915_private *i915) > > { > > struct dram_info *dram_info = &i915->dram_info; > > - u32 dram_channels; > > - u32 mem_freq_khz, val; > > - u8 num_active_channels, valid_ranks = 0; > > + u32 val; > > + u8 valid_ranks = 0; > > int i; > > > > - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); > > - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * > > - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > > - > > - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; > > - num_active_channels = hweight32(dram_channels); > > - > > - if (mem_freq_khz * num_active_channels == 0) { > > - drm_info(&i915->drm, > > - "Couldn't get system memory bandwidth\n"); > > - return -EINVAL; > > - } > > - > > /* > > * Now read each DUNIT8/9/10/11 to check the rank of each dimms. > > */ > > -- > > 2.33.0 >
On Wed, Oct 13, 2021 at 07:17:14PM +0000, Souza, Jose wrote: > On Wed, 2021-10-13 at 12:32 +0300, Ville Syrjälä wrote: > > On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: > > > This memory frequency calculated is only used to check if it is zero, > > > what is not useful as it will never actually be zero. > > > > > > Also the calculation is wrong, we should be checking other bit to > > > select the appropriate frequency multiplier while this code is stuck > > > with a fixed multiplier. > > > > I don't think the alternate ref clock was ever used. > > At least I don't recall ever seeing it. > > > > The real problem with this is that IIRC this is just the last > > requested frequency. So on a system with SAGV this will > > change dynamically. > > > > > > > > So here dropping it as whole. > > > > We have a second copy of this in gen6_update_ring_freq(). Rather > > than removing one and leaving another potentially broken one behind we > > should probably just consolidate on a single implementation. > > gen6_update_ring_freq() is related to GPU frequency not memory, don't look related at all to me. > GPU, CPU and memory clocks are all needed there, at least on some platforms. I forget which ones did what exactly.
On Wed, 2021-10-13 at 22:31 +0300, Ville Syrjälä wrote: > On Wed, Oct 13, 2021 at 07:17:14PM +0000, Souza, Jose wrote: > > On Wed, 2021-10-13 at 12:32 +0300, Ville Syrjälä wrote: > > > On Tue, Oct 12, 2021 at 06:00:46PM -0700, José Roberto de Souza wrote: > > > > This memory frequency calculated is only used to check if it is zero, > > > > what is not useful as it will never actually be zero. > > > > > > > > Also the calculation is wrong, we should be checking other bit to > > > > select the appropriate frequency multiplier while this code is stuck > > > > with a fixed multiplier. > > > > > > I don't think the alternate ref clock was ever used. > > > At least I don't recall ever seeing it. > > > > > > The real problem with this is that IIRC this is just the last > > > requested frequency. So on a system with SAGV this will > > > change dynamically. > > > > > > > > > > > So here dropping it as whole. > > > > > > We have a second copy of this in gen6_update_ring_freq(). Rather > > > than removing one and leaving another potentially broken one behind we > > > should probably just consolidate on a single implementation. > > > > gen6_update_ring_freq() is related to GPU frequency not memory, don't look related at all to me. > > > > GPU, CPU and memory clocks are all needed there, at least on some > platforms. I forget which ones did what exactly. But is is not relate with removing this memory frequency calculation, so we can drop it without leaving any code behind. >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a897f4abea0c3..8825f7ac477b6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -11109,12 +11109,6 @@ enum skl_power_gate { #define DC_STATE_DEBUG_MASK_CORES (1 << 0) #define DC_STATE_DEBUG_MASK_MEMORY_UP (1 << 1) -#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) -#define BXT_REQ_DATA_MASK 0x3F -#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 -#define BXT_DRAM_CHANNEL_ACTIVE_MASK (0xF << 12) -#define BXT_MEMORY_FREQ_MULTIPLIER_HZ 133333333 - #define BXT_D_CR_DRP0_DUNIT8 0x1000 #define BXT_D_CR_DRP0_DUNIT9 0x1200 #define BXT_D_CR_DRP0_DUNIT_START 8 @@ -11145,9 +11139,7 @@ enum skl_power_gate { #define BXT_DRAM_TYPE_LPDDR4 (0x2 << 22) #define BXT_DRAM_TYPE_DDR4 (0x4 << 22) -#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 DG1_GEAR_TYPE REG_BIT(16) #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000) diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c index 30a0cab5eff46..0adadfd9528aa 100644 --- a/drivers/gpu/drm/i915/intel_dram.c +++ b/drivers/gpu/drm/i915/intel_dram.c @@ -244,7 +244,6 @@ static int skl_get_dram_info(struct drm_i915_private *i915) { struct dram_info *dram_info = &i915->dram_info; - u32 mem_freq_khz, val; int ret; dram_info->type = skl_get_dram_type(i915); @@ -255,17 +254,6 @@ skl_get_dram_info(struct drm_i915_private *i915) if (ret) return ret; - val = intel_uncore_read(&i915->uncore, - SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); - mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * - SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); - - if (dram_info->num_channels * mem_freq_khz == 0) { - drm_info(&i915->drm, - "Couldn't get system memory bandwidth\n"); - return -EINVAL; - } - return 0; } @@ -350,24 +338,10 @@ static void bxt_get_dimm_info(struct dram_dimm_info *dimm, u32 val) static int bxt_get_dram_info(struct drm_i915_private *i915) { struct dram_info *dram_info = &i915->dram_info; - u32 dram_channels; - u32 mem_freq_khz, val; - u8 num_active_channels, valid_ranks = 0; + u32 val; + u8 valid_ranks = 0; int i; - val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); - mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) * - BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000); - - dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; - num_active_channels = hweight32(dram_channels); - - if (mem_freq_khz * num_active_channels == 0) { - drm_info(&i915->drm, - "Couldn't get system memory bandwidth\n"); - return -EINVAL; - } - /* * Now read each DUNIT8/9/10/11 to check the rank of each dimms. */
This memory frequency calculated is only used to check if it is zero, what is not useful as it will never actually be zero. Also the calculation is wrong, we should be checking other bit to select the appropriate frequency multiplier while this code is stuck with a fixed multiplier. So here dropping it as whole. v2: - Also remove memory frequency calculation for gen9 LP platforms Cc: Yakui Zhao <yakui.zhao@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode") Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 8 -------- drivers/gpu/drm/i915/intel_dram.c | 30 ++---------------------------- 2 files changed, 2 insertions(+), 36 deletions(-)