On Tue, Mar 03, 2015 at 03:22:15PM +0200, Ander Conselvan de Oliveira wrote:
> Pass a crtc_state to it and find whether the pipe has an encoder of a
> given type by looking at the drm_atomic_state the crtc_state points to.
>
> Note that is possible to reach i9xx_get_refclk() without a proper atomic
> state, since in the function vlv_force_pll_on() a minimally initialized
> crtc_state is allocated in the stack. With the current code, it is not
> possible to end up in a call to intel_pipe_will_have_type() with that
> bogus atomic state. To avoid future problems, a comment is added to warn
> people changing that code.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++++++++--------------
> 2 files changed, 83 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0c6ba2d..aab4421 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -541,7 +541,7 @@ struct drm_i915_display_funcs {
> * Returns true on success, false on failure.
> */
> bool (*find_dpll)(const struct intel_limit *limit,
> - struct intel_crtc *crtc,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk,
> struct dpll *match_clock,
> struct dpll *best_clock);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 518903e..f3652f9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -431,25 +431,37 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
> * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> * encoder->crtc.
> */
> -static bool intel_pipe_will_have_type(struct intel_crtc *crtc, int type)
> +static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
> + int type)
> {
> - struct drm_device *dev = crtc->base.dev;
> + struct drm_atomic_state *state = crtc_state->base.state;
> + struct drm_connector_state *connector_state;
> struct intel_encoder *encoder;
> + int i;
> +
> + for (i = 0; i < state->num_connector; i++) {
> + if (!state->connectors[i])
> + continue;
> +
> + connector_state = state->connector_states[i];
> + if (connector_state->crtc != crtc_state->base.crtc)
> + continue;
>
> - for_each_intel_encoder(dev, encoder)
> - if (encoder->new_crtc == crtc && encoder->type == type)
> + encoder = to_intel_encoder(connector_state->best_encoder);
> + if (encoder->type == type)
> return true;
> + }
>
> return false;
> }
The tricky bit here is that we must have all the connectors added to the
drm_atomic_sate for the given crtc. Otherwise there might be no connector
at all and we'd return a bogus answer. drm_atomic_add_affected_connectors
is the function which does this for you, and I think we need to call it
somewhere in the top-level compute_config code. And I haven't spotted that
call anywhere in your series.
Otherwise the implementation of the new will_have_type looks correct.
-Daniel
>
> -static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
> - int refclk)
> +static const intel_limit_t *
> +intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk)
> {
> - struct drm_device *dev = crtc->base.dev;
> + struct drm_device *dev = crtc_state->base.crtc->dev;
> const intel_limit_t *limit;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> if (intel_is_dual_link_lvds(dev)) {
> if (refclk == 100000)
> limit = &intel_limits_ironlake_dual_lvds_100m;
> @@ -467,20 +479,21 @@ static const intel_limit_t *intel_ironlake_limit(struct intel_crtc *crtc,
> return limit;
> }
>
> -static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
> +static const intel_limit_t *
> +intel_g4x_limit(struct intel_crtc_state *crtc_state)
> {
> - struct drm_device *dev = crtc->base.dev;
> + struct drm_device *dev = crtc_state->base.crtc->dev;
> const intel_limit_t *limit;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> if (intel_is_dual_link_lvds(dev))
> limit = &intel_limits_g4x_dual_channel_lvds;
> else
> limit = &intel_limits_g4x_single_channel_lvds;
> - } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
> - intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
> + } else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI) ||
> + intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_ANALOG)) {
> limit = &intel_limits_g4x_hdmi;
> - } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
> + } else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO)) {
> limit = &intel_limits_g4x_sdvo;
> } else /* The option is for other outputs */
> limit = &intel_limits_i9xx_sdvo;
> @@ -488,17 +501,18 @@ static const intel_limit_t *intel_g4x_limit(struct intel_crtc *crtc)
> return limit;
> }
>
> -static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
> +static const intel_limit_t *
> +intel_limit(struct intel_crtc_state *crtc_state, int refclk)
> {
> - struct drm_device *dev = crtc->base.dev;
> + struct drm_device *dev = crtc_state->base.crtc->dev;
> const intel_limit_t *limit;
>
> if (HAS_PCH_SPLIT(dev))
> - limit = intel_ironlake_limit(crtc, refclk);
> + limit = intel_ironlake_limit(crtc_state, refclk);
> else if (IS_G4X(dev)) {
> - limit = intel_g4x_limit(crtc);
> + limit = intel_g4x_limit(crtc_state);
> } else if (IS_PINEVIEW(dev)) {
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> limit = &intel_limits_pineview_lvds;
> else
> limit = &intel_limits_pineview_sdvo;
> @@ -507,14 +521,14 @@ static const intel_limit_t *intel_limit(struct intel_crtc *crtc, int refclk)
> } else if (IS_VALLEYVIEW(dev)) {
> limit = &intel_limits_vlv;
> } else if (!IS_GEN2(dev)) {
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> limit = &intel_limits_i9xx_lvds;
> else
> limit = &intel_limits_i9xx_sdvo;
> } else {
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> limit = &intel_limits_i8xx_lvds;
> - else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> + else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
> limit = &intel_limits_i8xx_dvo;
> else
> limit = &intel_limits_i8xx_dac;
> @@ -601,15 +615,17 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
> }
>
> static bool
> -i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +i9xx_find_best_dpll(const intel_limit_t *limit,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> intel_clock_t clock;
> int err = target;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> /*
> * For LVDS just rely on its current settings for dual-channel.
> * We haven't figured out how to reliably set up different
> @@ -662,15 +678,17 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> }
>
> static bool
> -pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +pnv_find_best_dpll(const intel_limit_t *limit,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> intel_clock_t clock;
> int err = target;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> /*
> * For LVDS just rely on its current settings for dual-channel.
> * We haven't figured out how to reliably set up different
> @@ -721,10 +739,12 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> }
>
> static bool
> -g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +g4x_find_best_dpll(const intel_limit_t *limit,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> intel_clock_t clock;
> int max_n;
> @@ -733,7 +753,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> int err_most = (target >> 8) + (target >> 9);
> found = false;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> if (intel_is_dual_link_lvds(dev))
> clock.p2 = limit->p2.p2_fast;
> else
> @@ -778,10 +798,12 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> }
>
> static bool
> -vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +vlv_find_best_dpll(const intel_limit_t *limit,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> intel_clock_t clock;
> unsigned int bestppm = 1000000;
> @@ -835,10 +857,12 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> }
>
> static bool
> -chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> +chv_find_best_dpll(const intel_limit_t *limit,
> + struct intel_crtc_state *crtc_state,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> intel_clock_t clock;
> uint64_t m2;
> @@ -5678,7 +5702,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> * - LVDS dual channel mode
> * - Double wide pipe
> */
> - if ((intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> + if ((intel_pipe_will_have_type(pipe_config, INTEL_OUTPUT_LVDS) &&
> intel_is_dual_link_lvds(dev)) || pipe_config->double_wide)
> pipe_config->pipe_src_w &= ~1;
>
> @@ -5861,15 +5885,21 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> && !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
> }
>
> -static int i9xx_get_refclk(struct intel_crtc *crtc, int num_connectors)
> +static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> + int num_connectors)
> {
> - struct drm_device *dev = crtc->base.dev;
> + struct drm_device *dev = crtc_state->base.crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int refclk;
>
> if (IS_VALLEYVIEW(dev)) {
> + /* Note that this path may be reached with a crtc_state that
> + * is not part of an atomic state, due to vlv_force_pll_on().
> + * That means we can't use intel_pipe_will_have_type() or
> + * anything that depends on the whole atomic state here.
> + */
> refclk = 100000;
> - } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> + } else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
> refclk = dev_priv->vbt.lvds_ssc_freq;
> DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
> @@ -5912,7 +5942,7 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
> crtc_state->dpll_hw_state.fp0 = fp;
>
> crtc->lowfreq_avail = false;
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> reduced_clock && i915.powersave) {
> crtc_state->dpll_hw_state.fp1 = fp2;
> crtc->lowfreq_avail = true;
> @@ -6200,7 +6230,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
> (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
>
> /* Loop filter */
> - refclk = i9xx_get_refclk(crtc, 0);
> + refclk = i9xx_get_refclk(pipe_config, 0);
> loopfilter = 5 << DPIO_CHV_PROP_COEFF_SHIFT |
> 2 << DPIO_CHV_GAIN_CTRL_SHIFT;
> if (refclk == 100000)
> @@ -6236,6 +6266,7 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
> struct intel_crtc *crtc =
> to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> struct intel_crtc_state pipe_config = {
> + .base.crtc = &crtc->base,
> .pixel_multiplier = 1,
> .dpll = *dpll,
> };
> @@ -6280,12 +6311,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>
> i9xx_update_pll_dividers(crtc, crtc_state, reduced_clock);
>
> - is_sdvo = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO) ||
> - intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI);
> + is_sdvo = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_SDVO) ||
> + intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_HDMI);
>
> dpll = DPLL_VGA_MODE_DIS;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS))
> dpll |= DPLLB_MODE_LVDS;
> else
> dpll |= DPLLB_MODE_DAC_SERIAL;
> @@ -6328,7 +6359,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>
> if (crtc_state->sdvo_tv_clock)
> dpll |= PLL_REF_INPUT_TVCLKINBC;
> - else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> + else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> else
> @@ -6358,7 +6389,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>
> dpll = DPLL_VGA_MODE_DIS;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) {
> dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> } else {
> if (clock->p1 == 2)
> @@ -6369,10 +6400,10 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
> dpll |= PLL_P2_DIVIDE_BY_4;
> }
>
> - if (!IS_I830(dev) && intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
> + if (!IS_I830(dev) && intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO))
> dpll |= DPLL_DVO_2X_MODE;
>
> - if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
> + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) &&
> intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> else
> @@ -6609,7 +6640,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> return 0;
>
> if (!crtc_state->clock_set) {
> - refclk = i9xx_get_refclk(crtc, num_connectors);
> + refclk = i9xx_get_refclk(crtc_state, num_connectors);
>
> /*
> * Returns a set of divisors for the desired target clock with
> @@ -6617,8 +6648,8 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n +
> * 2) / p1 / p2.
> */
> - limit = intel_limit(crtc, refclk);
> - ok = dev_priv->display.find_dpll(limit, crtc,
> + limit = intel_limit(crtc_state, refclk);
> + ok = dev_priv->display.find_dpll(limit, crtc_state,
> crtc_state->port_clock,
> refclk, NULL, &clock);
> if (!ok) {
> @@ -6634,7 +6665,7 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> * we will disable the LVDS downclock feature.
> */
> has_reduced_clock =
> - dev_priv->display.find_dpll(limit, crtc,
> + dev_priv->display.find_dpll(limit, crtc_state,
> dev_priv->lvds_downclock,
> refclk, &clock,
> &reduced_clock);
> @@ -7462,12 +7493,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int refclk;
> const intel_limit_t *limit;
> bool ret, is_lvds = false;
>
> - is_lvds = intel_pipe_will_have_type(intel_crtc, INTEL_OUTPUT_LVDS);
> + is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
>
> refclk = ironlake_get_refclk(crtc);
>
> @@ -7476,8 +7506,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * refclk, or FALSE. The returned values represent the clock equation:
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> - limit = intel_limit(intel_crtc, refclk);
> - ret = dev_priv->display.find_dpll(limit, intel_crtc,
> + limit = intel_limit(crtc_state, refclk);
> + ret = dev_priv->display.find_dpll(limit, crtc_state,
> crtc_state->port_clock,
> refclk, NULL, clock);
> if (!ret)
> @@ -7491,7 +7521,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * downclock feature.
> */
> *has_reduced_clock =
> - dev_priv->display.find_dpll(limit, intel_crtc,
> + dev_priv->display.find_dpll(limit, crtc_state,
> dev_priv->lvds_downclock,
> refclk, clock,
> reduced_clock);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
