On Wed, Aug 06, 2025 at 04:57:53PM +0300, Jani Nikula wrote:
> On Tue, 05 Aug 2025, Rodrigo Vivi <rodrigo.v...@intel.com> wrote:
> > On Thu, Jul 31, 2025 at 05:21:25PM +0300, Jani Nikula wrote:
> >> Store fsb_freq and mem_freq in dram info the same way we do for other
> >> memory info on later platforms for a slightly more unified approach.
> >>
> >> This allows us to remove fsb_freq, mem_freq and is_ddr3 members from
> >> struct drm_i915_private and struct xe_device.
> >>
> >> Signed-off-by: Jani Nikula <jani.nik...@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/display/i9xx_wm.c | 13 +++++----
> >> drivers/gpu/drm/i915/i915_drv.h | 2 --
> >> drivers/gpu/drm/i915/soc/intel_dram.c | 38 +++++++++++---------------
> >> drivers/gpu/drm/i915/soc/intel_dram.h | 2 ++
> >> drivers/gpu/drm/xe/xe_device_types.h | 1 -
> >> 5 files changed, 26 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c
> >> b/drivers/gpu/drm/i915/display/i9xx_wm.c
> >> index 1f9db5118777..591acce2a4b1 100644
> >> --- a/drivers/gpu/drm/i915/display/i9xx_wm.c
> >> +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c
> >> @@ -3,6 +3,8 @@
> >> * Copyright © 2023 Intel Corporation
> >> */
> >>
> >> +#include "soc/intel_dram.h"
> >> +
> >> #include "i915_drv.h"
> >> #include "i915_reg.h"
> >> #include "i9xx_wm.h"
> >> @@ -85,7 +87,8 @@ static const struct cxsr_latency cxsr_latency_table[] = {
> >>
> >> static const struct cxsr_latency *pnv_get_cxsr_latency(struct
> >> intel_display *display)
> >> {
> >> - struct drm_i915_private *i915 = to_i915(display->drm);
> >> + const struct dram_info *dram_info = intel_dram_info(display->drm);
> >> + bool is_ddr3 = dram_info->type == INTEL_DRAM_DDR3;
> >
> > does this deserves a separate patch? I'm not sure if I followed here...
>
> The current check in the loop below is
>
> i915->is_ddr3 == latency->is_ddr3
>
> and with i915->is_ddr3 being replaced by dram_info->type, I thought it's
> simpler to have that variable.
oh, it makes sense now... what I was missing was the part where
that was getting set in the pnv block.
>
> The alternative is to convert the cxsr_latency_table to use enum
> intel_dram_type and INTEL_DRAM_DDR3, but I felt that's a bit much.
no need for that indeed.
Thanks for the explanation and patience
Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
>
> >
> >> int i;
> >>
> >> for (i = 0; i < ARRAY_SIZE(cxsr_latency_table); i++) {
> >> @@ -93,15 +96,15 @@ static const struct cxsr_latency
> >> *pnv_get_cxsr_latency(struct intel_display *dis
> >> bool is_desktop = !display->platform.mobile;
> >>
> >> if (is_desktop == latency->is_desktop &&
> >> - i915->is_ddr3 == latency->is_ddr3 &&
> >> - DIV_ROUND_CLOSEST(i915->fsb_freq, 1000) ==
> >> latency->fsb_freq &&
> >> - DIV_ROUND_CLOSEST(i915->mem_freq, 1000) ==
> >> latency->mem_freq)
> >> + is_ddr3 == latency->is_ddr3 &&
> >> + DIV_ROUND_CLOSEST(dram_info->fsb_freq, 1000) ==
> >> latency->fsb_freq &&
> >> + DIV_ROUND_CLOSEST(dram_info->mem_freq, 1000) ==
> >> latency->mem_freq)
> >> return latency;
> >> }
> >>
> >> drm_dbg_kms(display->drm,
> >> "Could not find CxSR latency for DDR%s, FSB %u kHz, MEM %u
> >> kHz\n",
> >> - i915->is_ddr3 ? "3" : "2", i915->fsb_freq, i915->mem_freq);
> >> + is_ddr3 ? "3" : "2", dram_info->fsb_freq,
> >> dram_info->mem_freq);
> >>
> >> return NULL;
> >> }
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h
> >> index 4e4e89746aa6..2f3965feada1 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -237,8 +237,6 @@ struct drm_i915_private {
> >>
> >> bool preserve_bios_swizzle;
> >>
> >> - unsigned int fsb_freq, mem_freq, is_ddr3;
> >> -
> >> unsigned int hpll_freq;
> >> unsigned int czclk_freq;
> >>
> >> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c
> >> b/drivers/gpu/drm/i915/soc/intel_dram.c
> >> index d896fb67270f..6405a3d0b930 100644
> >> --- a/drivers/gpu/drm/i915/soc/intel_dram.c
> >> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
> >> @@ -149,17 +149,6 @@ unsigned int intel_mem_freq(struct drm_i915_private
> >> *i915)
> >> return 0;
> >> }
> >>
> >> -static void detect_mem_freq(struct drm_i915_private *i915)
> >> -{
> >> - i915->mem_freq = intel_mem_freq(i915);
> >> -
> >> - if (IS_PINEVIEW(i915))
> >> - i915->is_ddr3 = pnv_is_ddr3(i915);
> >> -
> >> - if (i915->mem_freq)
> >> - drm_dbg(&i915->drm, "DDR speed: %d kHz\n", i915->mem_freq);
> >> -}
> >> -
> >> static unsigned int i9xx_fsb_freq(struct drm_i915_private *i915)
> >> {
> >> u32 fsb;
> >> @@ -252,11 +241,20 @@ unsigned int intel_fsb_freq(struct drm_i915_private
> >> *i915)
> >> return 0;
> >> }
> >>
> >> -static void detect_fsb_freq(struct drm_i915_private *i915)
> >> +static int i915_get_dram_info(struct drm_i915_private *i915, struct
> >> dram_info *dram_info)
> >> {
> >> - i915->fsb_freq = intel_fsb_freq(i915);
> >> - if (i915->fsb_freq)
> >> - drm_dbg(&i915->drm, "FSB frequency: %d kHz\n", i915->fsb_freq);
> >> + dram_info->fsb_freq = intel_fsb_freq(i915);
> >> + if (dram_info->fsb_freq)
> >> + drm_dbg(&i915->drm, "FSB frequency: %d kHz\n",
> >> dram_info->fsb_freq);
> >> +
> >> + dram_info->mem_freq = intel_mem_freq(i915);
> >> + if (dram_info->mem_freq)
> >> + drm_dbg(&i915->drm, "DDR speed: %d kHz\n", dram_info->mem_freq);
> >> +
> >> + if (IS_PINEVIEW(i915) && pnv_is_ddr3(i915))
> >> + dram_info->type = INTEL_DRAM_DDR3;
> >> +
> >> + return 0;
> >> }
> >>
> >> static int intel_dimm_num_devices(const struct dram_dimm_info *dimm)
> >> @@ -728,12 +726,6 @@ int intel_dram_detect(struct drm_i915_private *i915)
> >> if (IS_DG2(i915) || !HAS_DISPLAY(i915))
> >> return 0;
> >>
> >> - detect_fsb_freq(i915);
> >> - detect_mem_freq(i915);
> >> -
> >> - if (GRAPHICS_VER(i915) < 9)
> >> - return 0;
> >
> > oh! this responds my last question in the previous patch...
>
> Yeah, I could've referred to later changes there!
>
> >
> >> -
> >> dram_info = drmm_kzalloc(&i915->drm, sizeof(*dram_info), GFP_KERNEL);
> >> if (!dram_info)
> >> return -ENOMEM;
> >> @@ -754,8 +746,10 @@ int intel_dram_detect(struct drm_i915_private *i915)
> >> ret = gen11_get_dram_info(i915, dram_info);
> >> else if (IS_BROXTON(i915) || IS_GEMINILAKE(i915))
> >> ret = bxt_get_dram_info(i915, dram_info);
> >> - else
> >> + else if (GRAPHICS_VER(i915) >= 9)
> >> ret = skl_get_dram_info(i915, dram_info);
> >> + else
> >> + ret = i915_get_dram_info(i915, dram_info);
> >>
> >> drm_dbg_kms(&i915->drm, "DRAM type: %s\n",
> >> intel_dram_type_str(dram_info->type));
> >> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.h
> >> b/drivers/gpu/drm/i915/soc/intel_dram.h
> >> index 5ba75e279e84..97d21894abdc 100644
> >> --- a/drivers/gpu/drm/i915/soc/intel_dram.h
> >> +++ b/drivers/gpu/drm/i915/soc/intel_dram.h
> >> @@ -29,6 +29,8 @@ struct dram_info {
> >> } type;
> >> u8 num_qgv_points;
> >> u8 num_psf_gv_points;
> >> + unsigned int fsb_freq;
> >> + unsigned int mem_freq;
> >> };
> >>
> >> void intel_dram_edram_detect(struct drm_i915_private *i915);
> >> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> >> b/drivers/gpu/drm/xe/xe_device_types.h
> >> index 38c8329b4d2c..e2206e867b33 100644
> >> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >> @@ -609,7 +609,6 @@ struct xe_device {
> >> struct {
> >> unsigned int hpll_freq;
> >> unsigned int czclk_freq;
> >> - unsigned int fsb_freq, mem_freq, is_ddr3;
> >> };
> >> #endif
> >> };
> >> --
> >> 2.39.5
> >>
>
> --
> Jani Nikula, Intel
