On Tue, Dec 01, 2015 at 05:43:33PM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrj...@linux.intel.com> wrote:
> > On Tue, Dec 01, 2015 at 02:47:41PM +0200, Jani Nikula wrote:
> >> On Mon, 30 Nov 2015, ville.syrj...@linux.intel.com wrote:
> >> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> >
> >> > Generalize rawclk handling by storing it in dev_priv.
> >> >
> >> > Presumably our hrawclk readout works at least for CTG and ELK
> >> > since we've been using it for DP AUX on those platforms. There
> >> > are no real docs anymore after configdb vanished, so the only
> >> > reference is the public CTG GMCH spec. What bits are listed in
> >> > that doc match our code. The ELK GMCH spec have no relevant
> >> > details unfortunately.
> >> >
> >> > The PNV situation is less clear. Starting from
> >> > commit aa17cdb4f836 ("drm/i915: initialize backlight max from VBT")
> >> > we assume that the CTG/ELK hrawclk readout works for PNV as well.
> >> > At least the results *seem* reasonable for one PNV machine (Lenovo
> >> > Ideapad S10-3t). Sadly the PNV GMCH spec doesn't have the goods on
> >> > the relevant register either.
> >> >
> >> > So let's keep assuming it works for PNV,ELK,CTG and read it out on
> >> > those platforms. G33 also has hrawclk according to some notes
> >> > in BSpec, but we don't actually need it for anything, so let's not
> >> > even try to read it out there.
> >>
> >> Looks sensible but moderately scary, did not review properly yet,
> >> however a few nitpicks below.
> >>
> >> >
> >> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> >> > drivers/gpu/drm/i915/intel_display.c | 55
> >> > ++++++++++++++++++++++--------------
> >> > drivers/gpu/drm/i915/intel_dp.c | 16 +++++------
> >> > drivers/gpu/drm/i915/intel_drv.h | 2 --
> >> > drivers/gpu/drm/i915/intel_panel.c | 24 ++++++++--------
> >> > 5 files changed, 53 insertions(+), 45 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 9ab3e25ddf38..64facd410037 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1781,6 +1781,7 @@ struct drm_i915_private {
> >> > unsigned int skl_boot_cdclk;
> >> > unsigned int cdclk_freq, max_cdclk_freq;
> >> > unsigned int max_dotclk_freq;
> >> > + unsigned int rawclk_freq;
> >> > unsigned int hpll_freq;
> >> > unsigned int czclk_freq;
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> > b/drivers/gpu/drm/i915/intel_display.c
> >> > index 84395c8c9dce..84a1359ecba5 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -168,49 +168,61 @@ static int vlv_get_cck_clock_hpll(struct
> >> > drm_i915_private *dev_priv,
> >> > return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> >> > }
> >> >
> >> > -int
> >> > -intel_pch_rawclk(struct drm_device *dev)
> >> > +static int
> >> > +intel_pch_rawclk(struct drm_i915_private *dev_priv)
> >>
> >> I guess intel_ prefix would not be needed anymore. *shrug*.
> >
> > Yeah, lazyness won and I didn't start renaming stuff too much.
>
> No worries.
>
> >
> >>
> >> > {
> >> > - struct drm_i915_private *dev_priv = dev->dev_private;
> >> > -
> >> > - WARN_ON(!HAS_PCH_SPLIT(dev));
> >> > + return (I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK) * 1000;
> >> > +}
> >> >
> >> > - return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> >> > +static int
> >> > +intel_vlv_hrawclk(struct drm_i915_private *dev_priv)
> >> > +{
> >> > + return 200000;
> >> > }
> >> >
> >> > -/* hrawclock is 1/4 the FSB frequency */
> >> > -int intel_hrawclk(struct drm_device *dev)
> >> > +static int
> >> > +intel_g4x_hrawclk(struct drm_i915_private *dev_priv)
> >> > {
> >> > - struct drm_i915_private *dev_priv = dev->dev_private;
> >> > uint32_t clkcfg;
> >> >
> >> > - /* There is no CLKCFG reg in Valleyview. VLV hrawclk is 200 MHz
> >> > */
> >> > - if (IS_VALLEYVIEW(dev))
> >> > - return 200;
> >> > -
> >> > + /* hrawclock is 1/4 the FSB frequency */
> >> > clkcfg = I915_READ(CLKCFG);
> >> > switch (clkcfg & CLKCFG_FSB_MASK) {
> >> > case CLKCFG_FSB_400:
> >> > - return 100;
> >> > + return 100000;
> >> > case CLKCFG_FSB_533:
> >> > - return 133;
> >> > + return 133333;
> >> > case CLKCFG_FSB_667:
> >> > - return 166;
> >> > + return 166667;
> >> > case CLKCFG_FSB_800:
> >> > - return 200;
> >> > + return 200000;
> >> > case CLKCFG_FSB_1067:
> >> > - return 266;
> >> > + return 266667;
> >> > case CLKCFG_FSB_1333:
> >> > - return 333;
> >> > + return 333333;
> >> > /* these two are just a guess; one of them might be right */
> >> > case CLKCFG_FSB_1600:
> >> > case CLKCFG_FSB_1600_ALT:
> >> > - return 400;
> >> > + return 400000;
> >> > default:
> >> > - return 133;
> >> > + return 133333;
> >> > }
> >> > }
> >> >
> >> > +static void intel_update_rawclk(struct drm_i915_private *dev_priv)
> >> > +{
> >> > + if (HAS_PCH_SPLIT(dev_priv))
> >> > + dev_priv->rawclk_freq = intel_pch_rawclk(dev_priv);
> >> > + else if (IS_VALLEYVIEW(dev_priv))
> >> > + dev_priv->rawclk_freq = intel_vlv_hrawclk(dev_priv);
> >> > + else if (IS_G4X(dev_priv) || IS_PINEVIEW(dev_priv))
> >> > + dev_priv->rawclk_freq = intel_g4x_hrawclk(dev_priv);
> >> > + else
> >> > + return; /* no rawclk on other platforms, or no need to
> >> > know it */
> >> > +
> >> > + DRM_DEBUG_DRIVER("rawclk rate: %d kHz\n",
> >> > dev_priv->rawclk_freq);
> >> > +}
> >> > +
> >> > static void intel_update_czclk(struct drm_i915_private *dev_priv)
> >> > {
> >> > if (!IS_VALLEYVIEW(dev_priv))
> >> > @@ -15145,6 +15157,7 @@ void intel_modeset_init(struct drm_device *dev)
> >> > }
> >> >
> >> > intel_update_czclk(dev_priv);
> >> > + intel_update_rawclk(dev_priv);
> >> > intel_update_cdclk(dev);
> >> >
> >> > intel_shared_dpll_init(dev);
> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> > b/drivers/gpu/drm/i915/intel_dp.c
> >> > index f335c92b4fa7..2a9b5710ee83 100644
> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > @@ -675,13 +675,13 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp,
> >> > bool has_aux_irq)
> >> > static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp,
> >> > int index)
> >> > {
> >> > struct intel_digital_port *intel_dig_port =
> >> > dp_to_dig_port(intel_dp);
> >> > - struct drm_device *dev = intel_dig_port->base.base.dev;
> >> > + struct drm_i915_private *dev_priv =
> >> > to_i915(intel_dig_port->base.base.dev);
> >> >
> >> > /*
> >> > * The clock divider is based off the hrawclk, and would like
> >> > to run at
> >> > * 2MHz. So, take the hrawclk value and divide by 2 and use
> >> > that
> >> > */
> >>
> >> The comment needs updating. Possibly elsewhere too outside of patch
> >> context.
> >
> > That's being done in patch 11/11. I assume that's good enough place for
> > you?
>
> Yes.
>
> >
> >>
> >> > - return index ? 0 : DIV_ROUND_CLOSEST(intel_hrawclk(dev), 2);
> >> > + return index ? 0 : DIV_ROUND_CLOSEST(dev_priv->rawclk_freq,
> >> > 2000);
> >> > }
> >> >
> >> > static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp,
> >> > int index)
> >> > @@ -693,12 +693,10 @@ static uint32_t ilk_get_aux_clock_divider(struct
> >> > intel_dp *intel_dp, int index)
> >> > if (index)
> >> > return 0;
> >> >
> >> > - if (intel_dig_port->port == PORT_A) {
> >> > + if (intel_dig_port->port == PORT_A)
> >> > return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
> >> > -
> >> > - } else {
> >> > - return DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> >> > - }
> >> > + else
> >> > + return DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
> >> > }
> >> >
> >> > static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp,
> >> > int index)
> >> > @@ -719,7 +717,7 @@ static uint32_t hsw_get_aux_clock_divider(struct
> >> > intel_dp *intel_dp, int index)
> >> > default: return 0;
> >> > }
> >> > } else {
> >> > - return index ? 0 :
> >> > DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> >> > + return index ? 0 :
> >> > DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
> >> > }
> >> > }
> >> >
> >> > @@ -5237,7 +5235,7 @@
> >> > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >> > {
> >> > struct drm_i915_private *dev_priv = dev->dev_private;
> >> > u32 pp_on, pp_off, pp_div, port_sel = 0;
> >> > - int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) :
> >> > intel_hrawclk(dev);
> >> > + int div = dev_priv->rawclk_freq / 1000;
> >
> > BTW here still round down when calculating the power sequencer divisor.
> > Considering that one is used to enforce the panel power delays, I'm
> > thinking we should really round up to make sure we never end up with a
> > shorter delay than specified. But that disagrees with the examples in
> > the spec, so not really sure what's best.
> >
> >> > i915_reg_t pp_on_reg, pp_off_reg, pp_div_reg, pp_ctrl_reg;
> >> > enum port port = dp_to_dig_port(intel_dp)->port;
> >> > const struct edp_power_seq *seq = &intel_dp->pps_delays;
> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> > b/drivers/gpu/drm/i915/intel_drv.h
> >> > index 1ffd8d5c3235..54b65951622d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > @@ -1050,8 +1050,6 @@ void i915_audio_component_cleanup(struct
> >> > drm_i915_private *dev_priv);
> >> > /* intel_display.c */
> >> > extern const struct drm_plane_funcs intel_plane_funcs;
> >> > bool intel_has_pending_fb_unpin(struct drm_device *dev);
> >> > -int intel_pch_rawclk(struct drm_device *dev);
> >> > -int intel_hrawclk(struct drm_device *dev);
> >> > void intel_mark_busy(struct drm_device *dev);
> >> > void intel_mark_idle(struct drm_device *dev);
> >> > void intel_crtc_restore_mode(struct drm_crtc *crtc);
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> >> > b/drivers/gpu/drm/i915/intel_panel.c
> >> > index 891a587225e2..b8c78d885311 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -1314,8 +1314,8 @@ static u32 lpt_hz_to_pwm(struct intel_connector
> >> > *connector, u32 pwm_freq_hz)
> >> > */
> >> > static u32 pch_hz_to_pwm(struct intel_connector *connector, u32
> >> > pwm_freq_hz)
> >> > {
> >> > - struct drm_device *dev = connector->base.dev;
> >> > - int clock = MHz(intel_pch_rawclk(dev));
> >> > + struct drm_i915_private *dev_priv =
> >> > to_i915(connector->base.dev);
> >> > + int clock = 1000 * dev_priv->rawclk_freq;
> >> >
> >> > return clock / (pwm_freq_hz * 128);
> >>
> >> Could use:
> >>
> >> return KHz(dev_priv->rawclk_freq) / (pwm_freq_hz * 128);
> >>
> >> Same thing all around.
> >
> > I can respin with that.
>
> You can choose to respin, or to wait until I get around to actually
> reviewing the patch instead of just bikeshedding. ;)
Did I wait long enough? ;)
>
> BR,
> Jani.
>
> >
> >>
> >> Side note, maybe these should also round closest (follow-up).
> >
> > Hmm. Yeah, I guess closest would be warranted for PWM.
> >
> >>
> >>
> >> > }
> >> > @@ -1330,12 +1330,11 @@ static u32 pch_hz_to_pwm(struct intel_connector
> >> > *connector, u32 pwm_freq_hz)
> >> > */
> >> > static u32 i9xx_hz_to_pwm(struct intel_connector *connector, u32
> >> > pwm_freq_hz)
> >> > {
> >> > - struct drm_device *dev = connector->base.dev;
> >> > - struct drm_i915_private *dev_priv = dev->dev_private;
> >> > + struct drm_i915_private *dev_priv =
> >> > to_i915(connector->base.dev);
> >> > int clock;
> >> >
> >> > - if (IS_PINEVIEW(dev))
> >> > - clock = MHz(intel_hrawclk(dev));
> >> > + if (IS_PINEVIEW(dev_priv))
> >> > + clock = 1000 * dev_priv->rawclk_freq;
> >> > else
> >> > clock = 1000 * dev_priv->cdclk_freq;
> >> >
> >> > @@ -1354,7 +1353,7 @@ static u32 i965_hz_to_pwm(struct intel_connector
> >> > *connector, u32 pwm_freq_hz)
> >> > int clock;
> >> >
> >> > if (IS_G4X(dev_priv))
> >> > - clock = MHz(intel_hrawclk(dev));
> >> > + clock = 1000 * dev_priv->rawclk_freq;
> >> > else
> >> > clock = 1000 * dev_priv->cdclk_freq;
> >> >
> >> > @@ -1368,18 +1367,17 @@ static u32 i965_hz_to_pwm(struct intel_connector
> >> > *connector, u32 pwm_freq_hz)
> >> > */
> >> > static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32
> >> > pwm_freq_hz)
> >> > {
> >> > - struct drm_device *dev = connector->base.dev;
> >> > - struct drm_i915_private *dev_priv = dev->dev_private;
> >> > - int clock;
> >> > + struct drm_i915_private *dev_priv =
> >> > to_i915(connector->base.dev);
> >> >
> >> > if ((I915_READ(CBR1_VLV) & CBR_PWM_CLOCK_MUX_SELECT) == 0) {
> >> > - if (IS_CHERRYVIEW(dev))
> >> > + if (IS_CHERRYVIEW(dev_priv))
> >> > return KHz(19200) / (pwm_freq_hz * 16);
> >> > else
> >> > return MHz(25) / (pwm_freq_hz * 16);
> >> > } else {
> >> > - clock = intel_hrawclk(dev);
> >> > - return MHz(clock) / (pwm_freq_hz * 128);
> >> > + int clock = 1000 * dev_priv->rawclk_freq;
> >> > +
> >> > + return clock / (pwm_freq_hz * 128);
> >> > }
> >> > }
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
