Message ID | 20170127144427.3040-1-arthur@aheymans.xyz (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Jan 27, 2017 at 03:44:27PM +0100, Arthur Heymans wrote: > This is according to Mobile Intel® 945 Express Chipset > Family datasheet. > > Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 +- > drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++-- > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 70d96162def6..c3141e40d938 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -114,7 +114,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > #define GCFGC 0xf0 /* 915+ only */ > #define GC_LOW_FREQUENCY_ENABLE (1 << 7) > #define GC_DISPLAY_CLOCK_190_200_MHZ (0 << 4) > -#define GC_DISPLAY_CLOCK_333_MHZ (4 << 4) > +#define GC_DISPLAY_CLOCK_333_320_MHZ (4 << 4) > #define GC_DISPLAY_CLOCK_267_MHZ_PNV (0 << 4) > #define GC_DISPLAY_CLOCK_333_MHZ_PNV (1 << 4) > #define GC_DISPLAY_CLOCK_444_MHZ_PNV (2 << 4) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 81c11499bcf0..307fc62e7c70 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7389,6 +7389,26 @@ static int i945_get_display_clock_speed(struct drm_device *dev) > return 400000; > } > > +static int 945gm_get_display_clock_speed(struct drm_device *dev) > +{ > + struct pci_dev *pdev = dev->pdev; > + u16 gcfgc = 0; > + > + pci_read_config_word(pdev, GCFGC, &gcfgc); > + > + if (gcfgc & GC_LOW_FREQUENCY_ENABLE) > + return 133333; > + else { > + switch (gcfgc & GC_DISPLAY_CLOCK_MASK) { > + case GC_DISPLAY_CLOCK_333_320_MHZ: > + return 320000; That indeed is what the docs say. The code is tantalizingly close to the 915gm code now, so maybe we could share it with a simple if (IS_915GM(dev_priv)) return 320000; else return 333333; ? Now if someone could figure out where to dig up the DDR and FSB clocks we could also fix up the 190 vs. 200 MHz case... > + default: > + case GC_DISPLAY_CLOCK_190_200_MHZ: > + return 200000; > + } > + } > +} > + > static int i915_get_display_clock_speed(struct drm_device *dev) > { > return 333333; > @@ -7435,7 +7455,7 @@ static int i915gm_get_display_clock_speed(struct drm_device *dev) > return 133333; > else { > switch (gcfgc & GC_DISPLAY_CLOCK_MASK) { > - case GC_DISPLAY_CLOCK_333_MHZ: > + case GC_DISPLAY_CLOCK_333_320_MHZ: > return 333333; > default: > case GC_DISPLAY_CLOCK_190_200_MHZ: > @@ -15991,9 +16011,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv) > else if (IS_I915G(dev_priv)) > dev_priv->display.get_display_clock_speed = > i915_get_display_clock_speed; > - else if (IS_I945GM(dev_priv) || IS_845G(dev_priv)) > + else if (IS_845G(dev_priv)) > dev_priv->display.get_display_clock_speed = > i9xx_misc_get_display_clock_speed; > + else if (IS_I945GM(dev_priv)) > + dev_priv->display.get_display_clock_speed = > + 954gm_get_display_clock_speed; > else if (IS_I915GM(dev_priv)) > dev_priv->display.get_display_clock_speed = > i915gm_get_display_clock_speed; > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Fri, Jan 27, 2017 at 03:44:27PM +0100, Arthur Heymans wrote: > > That indeed is what the docs say. > > The code is tantalizingly close to the 915gm code now, so maybe > we could share it with a simple > > if (IS_915GM(dev_priv)) > return 320000; > else > return 333333; > Agreed but it's the other way around ;) > Now if someone could figure out where to dig up the DDR and FSB clocks > we could also fix up the 190 vs. 200 MHz case... > >> + default: >> + case GC_DISPLAY_CLOCK_190_200_MHZ: >> + return 200000; >> + } >> + } >> +} >> + Hmm that seems to be 915gm specific (always 200 on 945gm). According to "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported combos have 200Mhz set by that configuration.
On Fri, Jan 27, 2017 at 05:45:06PM +0100, Arthur Heymans wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > On Fri, Jan 27, 2017 at 03:44:27PM +0100, Arthur Heymans wrote: > > > > That indeed is what the docs say. > > > > The code is tantalizingly close to the 915gm code now, so maybe > > we could share it with a simple > > > > if (IS_915GM(dev_priv)) > > return 320000; > > else > > return 333333; > > > > Agreed but it's the other way around ;) Indeed. Apparently my brain can't reconcile the fact that the older part is faster. > > > Now if someone could figure out where to dig up the DDR and FSB clocks > > we could also fix up the 190 vs. 200 MHz case... > > > >> + default: > >> + case GC_DISPLAY_CLOCK_190_200_MHZ: > >> + return 200000; > >> + } > >> + } > >> +} > >> + > > Hmm that seems to be 915gm specific (always 200 on 945gm). According to > "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram > combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported > combos have 200Mhz set by that configuration. Yeah. Unfortunately I couldn't find a solid source of FSB/DDR clock information in the spec, apart from the actual strap pins but I can't see any register that would reflect those. So I guess we'll just leave it the way it is.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: >> >> > Now if someone could figure out where to dig up the DDR and FSB clocks >> > we could also fix up the 190 vs. 200 MHz case... >> > >> >> + default: >> >> + case GC_DISPLAY_CLOCK_190_200_MHZ: >> >> + return 200000; >> >> + } >> >> + } >> >> +} >> >> + >> >> Hmm that seems to be 915gm specific (always 200 on 945gm). According to >> "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram >> combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported >> combos have 200Mhz set by that configuration. > > Yeah. Unfortunately I couldn't find a solid source of FSB/DDR clock > information in the spec, apart from the actual strap pins but I can't > see any register that would reflect those. So I guess we'll just leave > it the way it is. Hmm "Mobile Intel® 915GM/PM/GME/GMS and 910GML/GMLE Express Chipset Specification Update" seems to sketch an even more complicated scenario, where it even depends on the GMCH subtype... FSB and also DRAM freq can be probably be fetched from respectively [2:0] and [6:4] of CLKCFG, MCHBAR(0xC00) but not documented. On 945GM I think the voltage strap is on DFT_STRAP1 bit20, MCHBAR(0xE08) (also not really documented but this is how it is done in Coreboot). It might be the same on 915GM... Given that getting the 915GM display clock correctly probably ends up being quite different from 945GM it might make sense to have 2 separate functions for that? Kind regards
On Fri, Jan 27, 2017 at 06:24:25PM +0100, Arthur Heymans wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > >> > >> > Now if someone could figure out where to dig up the DDR and FSB clocks > >> > we could also fix up the 190 vs. 200 MHz case... > >> > > >> >> + default: > >> >> + case GC_DISPLAY_CLOCK_190_200_MHZ: > >> >> + return 200000; > >> >> + } > >> >> + } > >> >> +} > >> >> + > >> > >> Hmm that seems to be 915gm specific (always 200 on 945gm). According to > >> "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram > >> combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported > >> combos have 200Mhz set by that configuration. > > > > Yeah. Unfortunately I couldn't find a solid source of FSB/DDR clock > > information in the spec, apart from the actual strap pins but I can't > > see any register that would reflect those. So I guess we'll just leave > > it the way it is. > > Hmm "Mobile Intel® 915GM/PM/GME/GMS and 910GML/GMLE Express Chipset > Specification Update" seems to sketch an even more complicated scenario, > where it even depends on the GMCH subtype... Oh, I didn't even have that particular document here. Have it now, and indeed it shows even more variation. > > FSB and also DRAM freq can be probably be fetched from respectively > [2:0] and [6:4] of CLKCFG, MCHBAR(0xC00) but not documented. I managed to dig up one more doc where the 6:4 bits are listed as 1 = 333 2 = 400 3 = 533 so those seem to agree with the 945gm version, with the 333 case added. The FSB speed I didn't manage to find in any 915 doc unfortunately. I suppose we could assume it matches 945gm with the extra 0=400 case added. Apparently there's also CLKCFG bit 16 on 915gm which seems to be telling me something about the ddr speed and voltage: 0 = 333,400 (533 @ 1.5v) 1 = 533 @ 1.05V so that could potentially solve part the voltage detection. Sadly only for DDR 533, if it's even correct. So I see a couple of options we could do here: a) ignore all of it and just go with your original change of for the 320 vs. 333 case b) ignore most of these details and just pick either the 190/213 or 200/222 as our presumed clocks c) just use the fsb/ddr info and again just assume one or the other for the cases where the voltage matters d) just assume the 945 DFT_STRAP1 detection of the voltage works on 915 and deal with every case. We wouldn't that far off the mark even if we got it wrong so I don't see a huge problem with just guessing e) something I didn't think of? I'll let you figure it out ;) > On 945GM I think the voltage strap is on DFT_STRAP1 bit20, MCHBAR(0xE08) > (also not really documented but this is how it is done in Coreboot). > It might be the same on 915GM... > > Given that getting the 915GM display clock correctly probably ends up > being quite different from 945GM it might make sense to have 2 separate > functions for that? > > Kind regards > -- > Arthur Heymans
Hi Arthur, [auto build test ERROR on v4.9-rc8] [cannot apply to drm-intel/for-linux-next next-20170125] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Arthur-Heymans/drm-i915-Get-correct-display-clock-on-945gm/20170128-022541 config: i386-randconfig-x004-201704 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): >> drivers/gpu/drm/i915/intel_display.c:7392:12: error: invalid suffix "gm_get_display_clock_speed" on integer constant static int 945gm_get_display_clock_speed(struct drm_device *dev) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/gpu/drm/i915/intel_display.c:7392:12: error: expected identifier or '(' before numeric constant drivers/gpu/drm/i915/intel_display.c: In function 'intel_init_display_hooks': drivers/gpu/drm/i915/intel_display.c:16020:4: error: invalid suffix "gm_get_display_clock_speed" on integer constant 954gm_get_display_clock_speed; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/gm_get_display_clock_speed +7392 drivers/gpu/drm/i915/intel_display.c 7386 7387 static int i945_get_display_clock_speed(struct drm_device *dev) 7388 { 7389 return 400000; 7390 } 7391 > 7392 static int 945gm_get_display_clock_speed(struct drm_device *dev) 7393 { 7394 struct pci_dev *pdev = dev->pdev; 7395 u16 gcfgc = 0; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Jan 27, 2017 at 09:51:50PM +0200, Ville Syrjälä wrote: > On Fri, Jan 27, 2017 at 06:24:25PM +0100, Arthur Heymans wrote: > > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > > >> > > >> > Now if someone could figure out where to dig up the DDR and FSB clocks > > >> > we could also fix up the 190 vs. 200 MHz case... > > >> > > > >> >> + default: > > >> >> + case GC_DISPLAY_CLOCK_190_200_MHZ: > > >> >> + return 200000; > > >> >> + } > > >> >> + } > > >> >> +} > > >> >> + > > >> > > >> Hmm that seems to be 915gm specific (always 200 on 945gm). According to > > >> "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram > > >> combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported > > >> combos have 200Mhz set by that configuration. > > > > > > Yeah. Unfortunately I couldn't find a solid source of FSB/DDR clock > > > information in the spec, apart from the actual strap pins but I can't > > > see any register that would reflect those. So I guess we'll just leave > > > it the way it is. > > > > Hmm "Mobile Intel® 915GM/PM/GME/GMS and 910GML/GMLE Express Chipset > > Specification Update" seems to sketch an even more complicated scenario, > > where it even depends on the GMCH subtype... > > Oh, I didn't even have that particular document here. Have it now, and > indeed it shows even more variation. > > > > > FSB and also DRAM freq can be probably be fetched from respectively > > [2:0] and [6:4] of CLKCFG, MCHBAR(0xC00) but not documented. > > I managed to dig up one more doc where the 6:4 bits are listed as > 1 = 333 > 2 = 400 > 3 = 533 > > so those seem to agree with the 945gm version, with the 333 case > added. The FSB speed I didn't manage to find in any 915 doc > unfortunately. I suppose we could assume it matches 945gm with > the extra 0=400 case added. > > Apparently there's also CLKCFG bit 16 on 915gm which seems to be > telling me something about the ddr speed and voltage: > 0 = 333,400 (533 @ 1.5v) > 1 = 533 @ 1.05V > > so that could potentially solve part the voltage detection. Sadly only > for DDR 533, if it's even correct. > > So I see a couple of options we could do here: > a) ignore all of it and just go with your original change of for the > 320 vs. 333 case > b) ignore most of these details and just pick either the 190/213 or > 200/222 as our presumed clocks > c) just use the fsb/ddr info and again just assume one or the other > for the cases where the voltage matters > d) just assume the 945 DFT_STRAP1 detection of the voltage works on 915 > and deal with every case. We wouldn't that far off the mark even if > we got it wrong so I don't see a huge problem with just guessing > e) something I didn't think of? > > I'll let you figure it out ;) Isn't the defensive thing to go with the lower clock? We might get a regression report, but chances are slim, and if we do get it we'll have a guinea-pig to test stuff on :-) So option a) -Daniel > > > On 945GM I think the voltage strap is on DFT_STRAP1 bit20, MCHBAR(0xE08) > > (also not really documented but this is how it is done in Coreboot). > > It might be the same on 915GM... > > > > Given that getting the 915GM display clock correctly probably ends up > > being quite different from 945GM it might make sense to have 2 separate > > functions for that? > > > > Kind regards > > -- > > Arthur Heymans > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 70d96162def6..c3141e40d938 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -114,7 +114,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define GCFGC 0xf0 /* 915+ only */ #define GC_LOW_FREQUENCY_ENABLE (1 << 7) #define GC_DISPLAY_CLOCK_190_200_MHZ (0 << 4) -#define GC_DISPLAY_CLOCK_333_MHZ (4 << 4) +#define GC_DISPLAY_CLOCK_333_320_MHZ (4 << 4) #define GC_DISPLAY_CLOCK_267_MHZ_PNV (0 << 4) #define GC_DISPLAY_CLOCK_333_MHZ_PNV (1 << 4) #define GC_DISPLAY_CLOCK_444_MHZ_PNV (2 << 4) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 81c11499bcf0..307fc62e7c70 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7389,6 +7389,26 @@ static int i945_get_display_clock_speed(struct drm_device *dev) return 400000; } +static int 945gm_get_display_clock_speed(struct drm_device *dev) +{ + struct pci_dev *pdev = dev->pdev; + u16 gcfgc = 0; + + pci_read_config_word(pdev, GCFGC, &gcfgc); + + if (gcfgc & GC_LOW_FREQUENCY_ENABLE) + return 133333; + else { + switch (gcfgc & GC_DISPLAY_CLOCK_MASK) { + case GC_DISPLAY_CLOCK_333_320_MHZ: + return 320000; + default: + case GC_DISPLAY_CLOCK_190_200_MHZ: + return 200000; + } + } +} + static int i915_get_display_clock_speed(struct drm_device *dev) { return 333333; @@ -7435,7 +7455,7 @@ static int i915gm_get_display_clock_speed(struct drm_device *dev) return 133333; else { switch (gcfgc & GC_DISPLAY_CLOCK_MASK) { - case GC_DISPLAY_CLOCK_333_MHZ: + case GC_DISPLAY_CLOCK_333_320_MHZ: return 333333; default: case GC_DISPLAY_CLOCK_190_200_MHZ: @@ -15991,9 +16011,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv) else if (IS_I915G(dev_priv)) dev_priv->display.get_display_clock_speed = i915_get_display_clock_speed; - else if (IS_I945GM(dev_priv) || IS_845G(dev_priv)) + else if (IS_845G(dev_priv)) dev_priv->display.get_display_clock_speed = i9xx_misc_get_display_clock_speed; + else if (IS_I945GM(dev_priv)) + dev_priv->display.get_display_clock_speed = + 954gm_get_display_clock_speed; else if (IS_I915GM(dev_priv)) dev_priv->display.get_display_clock_speed = i915gm_get_display_clock_speed;
This is according to Mobile Intel® 945 Express Chipset Family datasheet. Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- drivers/gpu/drm/i915/i915_reg.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-)