On Mon, Jun 03, 2013 at 01:54:40PM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > ... not the port clock. This allows us to kill the funny semantics
> > around pixel_target_clock.
> >
> > Since the dpll code still needs the real port clock, add a new
> > port_clock field to the pipe configuration. Handling the default case
> > for that one is a bit tricky, since encoders might not consistently
> > overwrite it when retrying the crtc/encoder bw arbitrage step in the
> > compute config stage. Hence we need to always clear port_clock and
> > update it again if the encoder hasn't put in something more specific.
> > This can't be done in one step since the encoder might want to adjust
> > the mode first.
> >
> > I was a bit on the fence whether I should subsume the pixel multiplier
> > handling into the port_clock, too. But then I decided against this
> > since it's on an abstract level still the dotclock of the adjusted
> > mode, and only our hw makes it a bit special due to the separate pixel
> > mulitplier setting (which requires that the dpll runs at the
> > non-multiplied dotclock).
> >
> > So after this patch the adjusted_mode accurately describes the mode we
> > feed into the port, after the panel fitter and pixel multiplier (or
> > line doubling, if we ever bother with that) have done their job.
> > Since the fdi link is between the pfit and the pixel multiplier steps
> > we need to be careful with calculating the fdi link config.
> >
> > v2: Fix up ilk cpu pll handling.
> >
> > v3: Introduce an fdi_dotclock variable in ironlake_fdi_compute_config
> > to make it clearer that we transmit the adjusted_mode without the
> > pixel multiplier taken into account. The old code multiplied the the
> > available link bw with the pixel multiplier, which results in the same
> > fdi configuration, but is much more confusing.
> >
> > v4: Rebase on top of Imre's is_cpu_edp removal.
> >
> > v5: Rebase on top of Paulo's haswell watermark fixes, which introduce
> > a new place which looked at the pixel_clock and so needed conversion.
> >
> > v6: Split out prep patches as requested by Paulo Zanoni. Also rebase
> > on top of the fdi dotclock handling fix in the fdi lanes/bw
> > computation code.
> >
> > Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org> (v3)
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> Due to my comments on patch 3, there's a possibility you may need to
> rebase this patch.
>
> My only optional bikeshed is to print port_clock inside
> intel_dump_pipe_config. If you rebase, you may consider doing it :)
Yeah, I think I'll follow up with another pipe config dump patch to add
lots more stuff. Already screamed around because it wasn't as complete as
I've wished for ;-)
-Daniel
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com> (v6)
>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
> > drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++---------------
> > drivers/gpu/drm/i915/intel_dp.c | 18 +++++++-----------
> > drivers/gpu/drm/i915/intel_drv.h | 13 +++++++------
> > drivers/gpu/drm/i915/intel_hdmi.c | 4 +---
> > drivers/gpu/drm/i915/intel_pm.c | 5 +----
> > 6 files changed, 35 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 9649df8..486c46b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -624,7 +624,7 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
> > clock, *p_out, *n2_out, *r2_out);
> > }
> >
> > -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> > +bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
> > {
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> > @@ -634,6 +634,7 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> > int type = intel_encoder->type;
> > enum pipe pipe = intel_crtc->pipe;
> > uint32_t reg, val;
> > + int clock = intel_crtc->config.port_clock;
> >
> > /* TODO: reuse PLLs when possible (compare values) */
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 761254d..103f4e9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > {
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> > - int target_clock, lane, link_bw, fdi_dotclock;
> > + int lane, link_bw, fdi_dotclock;
> > bool setup_ok, needs_recompute = false;
> >
> > retry:
> > @@ -4005,12 +4005,7 @@ retry:
> > */
> > link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> >
> > - if (pipe_config->pixel_target_clock)
> > - target_clock = pipe_config->pixel_target_clock;
> > - else
> > - target_clock = adjusted_mode->clock;
> > -
> > - fdi_dotclock = target_clock;
> > + fdi_dotclock = adjusted_mode->clock;
>
> In the patch you sent Friday, you used adjusted_mode->clock instead of
> fdi_dotclock as the parameter to ironlake_get_lanes_required, you only
> initialized fdi_dotclock before calling intel_link_compute_m_n. This
> doesn't change the code behavior, I'm just pointing in case you
> changed without noticing and prefer the older version.
>
>
> > if (pipe_config->pixel_multiplier > 1)
> > fdi_dotclock /= pipe_config->pixel_multiplier;
> >
> > @@ -4360,8 +4355,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> > {
> > struct drm_device *dev = crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct drm_display_mode *adjusted_mode =
> > - &crtc->config.adjusted_mode;
> > struct intel_encoder *encoder;
> > int pipe = crtc->pipe;
> > u32 dpll, mdiv;
> > @@ -4414,7 +4407,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> > vlv_dpio_write(dev_priv, DPIO_DIV(pipe), mdiv);
> >
> > /* Set HBR and RBR LPF coefficients */
> > - if (adjusted_mode->clock == 162000 ||
> > + if (crtc->config.port_clock == 162000 ||
> > intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
> > vlv_dpio_write(dev_priv, DPIO_LFP_COEFF(pipe),
> > 0x005f0021);
> > @@ -4859,7 +4852,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + ok = dev_priv->display.find_dpll(limit, crtc,
> > + intel_crtc->config.port_clock,
> > refclk, NULL, &clock);
> > if (!ok) {
> > DRM_ERROR("Couldn't find PLL settings for mode!\n");
> > @@ -5467,7 +5461,6 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
> > }
> >
> > static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > - struct drm_display_mode *adjusted_mode,
> > intel_clock_t *clock,
> > bool *has_reduced_clock,
> > intel_clock_t *reduced_clock)
> > @@ -5495,7 +5488,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + ret = dev_priv->display.find_dpll(limit, crtc,
> > + to_intel_crtc(crtc)->config.port_clock,
> > refclk, NULL, clock);
> > if (!ret)
> > return false;
> > @@ -5695,7 +5689,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> > "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
> >
> > - ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> > + ok = ironlake_compute_clocks(crtc, &clock,
> > &has_reduced_clock, &reduced_clock);
> > if (!ok) {
> > DRM_ERROR("Couldn't find PLL settings for mode!\n");
> > @@ -5898,7 +5892,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> > WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
> > num_connectors, pipe_name(pipe));
> >
> > - if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
> > + if (!intel_ddi_pll_mode_set(crtc))
> > return -EINVAL;
> >
> > /* Ensure that the cursor is valid for the new mode before changing... */
> > @@ -7789,6 +7783,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > goto fail;
> >
> > encoder_retry:
> > + /* Ensure the port clock default is reset when retrying. */
> > + pipe_config->port_clock = 0;
> > +
> > /* Pass our mode to the connectors and the CRTC to give them a chance to
> > * adjust it according to limitations or connector properties, and also
> > * a chance to reject the mode entirely.
> > @@ -7817,6 +7814,11 @@ encoder_retry:
> > }
> > }
> >
> > + /* Set default port clock if not overwritten by the encoder. Needs to be
> > + * done afterwards in case the encoder adjusts the mode. */
> > + if (!pipe_config->port_clock)
> > + pipe_config->port_clock = pipe_config->adjusted_mode.clock;
> > +
> > ret = intel_crtc_compute_config(crtc, pipe_config);
> > if (ret < 0) {
> > DRM_DEBUG_KMS("CRTC fixup failed\n");
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 647cc2b..c92eeeb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -677,7 +677,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
> > int bpp, mode_rate;
> > static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
> > - int target_clock, link_avail, link_clock;
> > + int link_avail, link_clock;
> >
> > if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
> > pipe_config->has_pch_encoder = true;
> > @@ -694,8 +694,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > intel_pch_panel_fitting(intel_crtc, pipe_config,
> > intel_connector->panel.fitting_mode);
> > }
> > - /* We need to take the panel's fixed mode into account. */
> > - target_clock = adjusted_mode->clock;
> >
> > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > return false;
> > @@ -711,7 +709,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
> >
> > for (; bpp >= 6*3; bpp -= 2*3) {
> > - mode_rate = intel_dp_link_required(target_clock, bpp);
> > + mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
> >
> > for (clock = 0; clock <= max_clock; clock++) {
> > for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
> > @@ -746,18 +744,17 @@ found:
> >
> > intel_dp->link_bw = bws[clock];
> > intel_dp->lane_count = lane_count;
> > - adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> > pipe_config->pipe_bpp = bpp;
> > - pipe_config->pixel_target_clock = target_clock;
> > + pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> >
> > DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
> > intel_dp->link_bw, intel_dp->lane_count,
> > - adjusted_mode->clock, bpp);
> > + pipe_config->port_clock, bpp);
> > DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> > mode_rate, link_avail);
> >
> > intel_link_compute_m_n(bpp, lane_count,
> > - target_clock, adjusted_mode->clock,
> > + adjusted_mode->clock, pipe_config->port_clock,
> > &pipe_config->dp_m_n);
> >
> > intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
> > @@ -788,12 +785,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 dpa_ctl;
> >
> > - DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> > - crtc->config.adjusted_mode.clock);
> > + DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", crtc->config.port_clock);
> > dpa_ctl = I915_READ(DP_A);
> > dpa_ctl &= ~DP_PLL_FREQ_MASK;
> >
> > - if (crtc->config.adjusted_mode.clock == 162000) {
> > + if (crtc->config.port_clock == 162000) {
> > /* For a long time we've carried around a ILK-DevA w/a for the
> > * 160MHz clock. If we're really unlucky, it's still required.
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fdf6303..afda71f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -243,12 +243,13 @@ struct intel_crtc_config {
> >
> > int pipe_bpp;
> > struct intel_link_m_n dp_m_n;
> > - /**
> > - * This is currently used by DP and HDMI encoders since those can have a
> > - * target pixel clock != the port link clock (which is currently stored
> > - * in adjusted_mode->clock).
> > +
> > + /*
> > + * Frequence the dpll for the port should run at. Differs from the
> > + * adjusted dotclock e.g. for DP or 12bpc hdmi mode.
> > */
> > - int pixel_target_clock;
> > + int port_clock;
> > +
> > /* Used by SDVO (and if we ever fix it, HDMI). */
> > unsigned pixel_multiplier;
> >
> > @@ -786,7 +787,7 @@ extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> > extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
> > extern void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
> > extern void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
> > -extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock);
> > +extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
> > extern void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
> > extern void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
> > extern void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 8062a92..bc12518 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -835,9 +835,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > desired_bpp = 12*3;
> >
> > /* Need to adjust the port link by 1.5x for 12bpc. */
> > - adjusted_mode->clock = clock_12bpc;
> > - pipe_config->pixel_target_clock =
> > - pipe_config->requested_mode.clock;
> > + pipe_config->port_clock = clock_12bpc;
> > } else {
> > DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> > desired_bpp = 8*3;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 49a1887..4126fb1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2078,10 +2078,7 @@ static uint32_t hsw_wm_get_pixel_rate(struct drm_device *dev,
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > uint32_t pixel_rate, pfit_size;
> >
> > - if (intel_crtc->config.pixel_target_clock)
> > - pixel_rate = intel_crtc->config.pixel_target_clock;
> > - else
> > - pixel_rate = intel_crtc->config.adjusted_mode.clock;
> > + pixel_rate = intel_crtc->config.adjusted_mode.clock;
> >
> > /* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
> > * adjust the pixel_rate here. */
> > --
> > 1.7.11.7
> >
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
