[Intel-gfx] [PATCH] drm/i915: Only enable DDI IO power domains after enabling DPLL
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Tue Feb 28 07:28:41 UTC 2017
On Mon, 2017-02-27 at 22:38 +0200, Ville Syrjälä wrote:
> On Fri, Feb 24, 2017 at 04:19:59PM +0200, Ander Conselvan de Oliveira wrote:
> > According to bspec, the DDI IO power domains should be enabled after
> > enabling the DPLL and mapping it to the DDI. The current order doesn't
> > seem to create problems with Skylake and Kabylake, but causes enable
> > timeouts in Geminilake.
>
> This one seems to kill SKL+MST.
>
> Just load the driver with an MST display plugged in and we get:
> [ 79.389527] Kernel panic - not syncing: Timeout: Not all CPUs entered broadcast exception handler
>
> So presumably we're trying to access something that's not powered/clocked
> and the CPU just gets totally stuck.
Hmm, we need to enable the DDI IO power domain in the MST path too. /o\
I sent the fix as a separate series so CI doesn't get confused, although it
doesn't seem to test MST+SKL.
https://patchwork.freedesktop.org/series/20345/
Ander
>
> >
> > v2: Rebase.
> > - Take power domain references before sanitizing encoders. (Imre)
> > - Add comment to get_encoder_power_domains() defition. (Ander)
> >
> > v3: Don't put the domain if called with HSW/BDW's analog encoder. (CI)
> >
> > v4: Put IO power domain before unmapping DPLL. (Imre)
> > - Change return type of intel_ddi_get_power_domains() to u64. (Imre)
> >
> > Cc: David Weinehall <david.weinehall at linux.intel.com>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> > Reviewed-by: David Weinehall <david.weinehall at linux.intel.com> # v1
> > Reviewed-by: Imre Deak <imre.deak at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 5 +++
> > drivers/gpu/drm/i915/intel_ddi.c | 49 ++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 4 ++
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 68 +++++++++++++++++++--------------
> > 5 files changed, 117 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4673912..5a032ba 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -344,6 +344,11 @@ enum intel_display_power_domain {
> > POWER_DOMAIN_PORT_DDI_C_LANES,
> > POWER_DOMAIN_PORT_DDI_D_LANES,
> > POWER_DOMAIN_PORT_DDI_E_LANES,
> > + POWER_DOMAIN_PORT_DDI_A_IO,
> > + POWER_DOMAIN_PORT_DDI_B_IO,
> > + POWER_DOMAIN_PORT_DDI_C_IO,
> > + POWER_DOMAIN_PORT_DDI_D_IO,
> > + POWER_DOMAIN_PORT_DDI_E_IO,
> > POWER_DOMAIN_PORT_DSI,
> > POWER_DOMAIN_PORT_CRT,
> > POWER_DOMAIN_PORT_OTHER,
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index b0c4d23..e9013f1 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1440,6 +1440,17 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > return ret;
> > }
> >
> > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder)
> > +{
> > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > + enum pipe pipe;
> > +
> > + if (intel_ddi_get_hw_state(encoder, &pipe))
> > + return BIT_ULL(dig_port->ddi_io_power_domain);
> > +
> > + return 0;
> > +}
> > +
> > void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
> > {
> > struct drm_crtc *crtc = &intel_crtc->base;
> > @@ -1682,6 +1693,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > enum port port = intel_ddi_get_encoder_port(encoder);
> > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> >
> > intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> > link_mst);
> > @@ -1689,6 +1701,9 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > intel_edp_panel_on(intel_dp);
> >
> > intel_ddi_clk_select(encoder, pll);
> > +
> > + intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
> > +
> > intel_prepare_dp_ddi_buffers(encoder);
> > intel_ddi_init_dp_buf_reg(encoder);
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > @@ -1708,9 +1723,13 @@ static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
> > struct drm_encoder *drm_encoder = &encoder->base;
> > enum port port = intel_ddi_get_encoder_port(encoder);
> > int level = intel_ddi_hdmi_level(dev_priv, port);
> > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> >
> > intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> > intel_ddi_clk_select(encoder, pll);
> > +
> > + intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
> > +
> > intel_prepare_hdmi_ddi_buffers(encoder);
> > if (IS_GEN9_BC(dev_priv))
> > skl_ddi_set_iboost(encoder, level);
> > @@ -1754,6 +1773,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> > struct drm_encoder *encoder = &intel_encoder->base;
> > struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> > enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > int type = intel_encoder->type;
> > uint32_t val;
> > bool wait = false;
> > @@ -1782,6 +1802,9 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> > intel_edp_panel_off(intel_dp);
> > }
> >
> > + if (dig_port)
> > + intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
> > +
> > if (IS_GEN9_BC(dev_priv))
> > I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
> > DPLL_CTRL2_DDI_CLK_OFF(port)));
> > @@ -2190,12 +2213,38 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> > intel_encoder->get_hw_state = intel_ddi_get_hw_state;
> > intel_encoder->get_config = intel_ddi_get_config;
> > intel_encoder->suspend = intel_dp_encoder_suspend;
> > + intel_encoder->get_power_domains = intel_ddi_get_power_domains;
> >
> > intel_dig_port->port = port;
> > intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> > (DDI_BUF_PORT_REVERSAL |
> > DDI_A_4_LANES);
> >
> > + switch (port) {
> > + case PORT_A:
> > + intel_dig_port->ddi_io_power_domain =
> > + POWER_DOMAIN_PORT_DDI_A_IO;
> > + break;
> > + case PORT_B:
> > + intel_dig_port->ddi_io_power_domain =
> > + POWER_DOMAIN_PORT_DDI_B_IO;
> > + break;
> > + case PORT_C:
> > + intel_dig_port->ddi_io_power_domain =
> > + POWER_DOMAIN_PORT_DDI_C_IO;
> > + break;
> > + case PORT_D:
> > + intel_dig_port->ddi_io_power_domain =
> > + POWER_DOMAIN_PORT_DDI_D_IO;
> > + break;
> > + case PORT_E:
> > + intel_dig_port->ddi_io_power_domain =
> > + POWER_DOMAIN_PORT_DDI_E_IO;
> > + break;
> > + default:
> > + MISSING_CASE(port);
> > + }
> > +
> > /*
> > * Bspec says that DDI_A_4_LANES is the only supported configuration
> > * for Broxton. Yet some BIOS fail to set this bit on port A if eDP
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 010086c..7673d5d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15413,6 +15413,24 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > }
> > }
> >
> > +static void
> > +get_encoder_power_domains(struct drm_i915_private *dev_priv)
> > +{
> > + struct intel_encoder *encoder;
> > +
> > + for_each_intel_encoder(&dev_priv->drm, encoder) {
> > + u64 get_domains;
> > + enum intel_display_power_domain domain;
> > +
> > + if (!encoder->get_power_domains)
> > + continue;
> > +
> > + get_domains = encoder->get_power_domains(encoder);
> > + for_each_power_domain(domain, get_domains)
> > + intel_display_power_get(dev_priv, domain);
> > + }
> > +}
> > +
> > /* Scan out the current hw modeset state,
> > * and sanitizes it to the current state
> > */
> > @@ -15428,6 +15446,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> > intel_modeset_readout_hw_state(dev);
> >
> > /* HW state is read out, now we need to sanitize this mess. */
> > + get_encoder_power_domains(dev_priv);
> > +
> > for_each_intel_encoder(dev, encoder) {
> > intel_sanitize_encoder(encoder);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 97621a1..0425dbe 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -241,6 +241,9 @@ struct intel_encoder {
> > * be set correctly before calling this function. */
> > void (*get_config)(struct intel_encoder *,
> > struct intel_crtc_state *pipe_config);
> > + /* Returns a mask of power domains that need to be referenced as part
> > + * of the hardware state readout code. */
> > + u64 (*get_power_domains)(struct intel_encoder *encoder);
> > /*
> > * Called during system suspend after all pending requests for the
> > * encoder are flushed (for example for DP AUX transactions) and
> > @@ -1027,6 +1030,7 @@ struct intel_digital_port {
> > enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
> > bool release_cl2_override;
> > uint8_t max_lanes;
> > + enum intel_display_power_domain ddi_io_power_domain;
> > };
> >
> > struct intel_dp_mst_encoder {
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 514ef56..012bc35 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -93,6 +93,16 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > return "PORT_DDI_D_LANES";
> > case POWER_DOMAIN_PORT_DDI_E_LANES:
> > return "PORT_DDI_E_LANES";
> > + case POWER_DOMAIN_PORT_DDI_A_IO:
> > + return "PORT_DDI_A_IO";
> > + case POWER_DOMAIN_PORT_DDI_B_IO:
> > + return "PORT_DDI_B_IO";
> > + case POWER_DOMAIN_PORT_DDI_C_IO:
> > + return "PORT_DDI_C_IO";
> > + case POWER_DOMAIN_PORT_DDI_D_IO:
> > + return "PORT_DDI_D_IO";
> > + case POWER_DOMAIN_PORT_DDI_E_IO:
> > + return "PORT_DDI_E_IO";
> > case POWER_DOMAIN_PORT_DSI:
> > return "PORT_DSI";
> > case POWER_DOMAIN_PORT_CRT:
> > @@ -385,18 +395,18 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_AUDIO) | \
> > BIT_ULL(POWER_DOMAIN_VGA) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > -#define SKL_DISPLAY_DDI_A_E_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) | \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_E_LANES) | \
> > +#define SKL_DISPLAY_DDI_IO_A_E_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_A_IO) | \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_E_IO) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > -#define SKL_DISPLAY_DDI_B_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_B_LANES) | \
> > +#define SKL_DISPLAY_DDI_IO_B_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_B_IO) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > -#define SKL_DISPLAY_DDI_C_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_C_LANES) | \
> > +#define SKL_DISPLAY_DDI_IO_C_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_C_IO) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > -#define SKL_DISPLAY_DDI_D_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_D_LANES) | \
> > +#define SKL_DISPLAY_DDI_IO_D_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_D_IO) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > #define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \
> > SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> > @@ -451,12 +461,12 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_AUDIO) | \
> > BIT_ULL(POWER_DOMAIN_VGA) | \
> > BIT_ULL(POWER_DOMAIN_INIT))
> > -#define GLK_DISPLAY_DDI_A_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES))
> > -#define GLK_DISPLAY_DDI_B_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_B_LANES))
> > -#define GLK_DISPLAY_DDI_C_POWER_DOMAINS ( \
> > - BIT_ULL(POWER_DOMAIN_PORT_DDI_C_LANES))
> > +#define GLK_DISPLAY_DDI_IO_A_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_A_IO))
> > +#define GLK_DISPLAY_DDI_IO_B_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_B_IO))
> > +#define GLK_DISPLAY_DDI_IO_C_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_PORT_DDI_C_IO))
> > #define GLK_DPIO_CMN_A_POWER_DOMAINS ( \
> > BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) | \
> > BIT_ULL(POWER_DOMAIN_AUX_A) | \
> > @@ -2114,26 +2124,26 @@ static struct i915_power_well skl_power_wells[] = {
> > .id = SKL_DISP_PW_2,
> > },
> > {
> > - .name = "DDI A/E power well",
> > - .domains = SKL_DISPLAY_DDI_A_E_POWER_DOMAINS,
> > + .name = "DDI A/E IO power well",
> > + .domains = SKL_DISPLAY_DDI_IO_A_E_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_A_E,
> > },
> > {
> > - .name = "DDI B power well",
> > - .domains = SKL_DISPLAY_DDI_B_POWER_DOMAINS,
> > + .name = "DDI B IO power well",
> > + .domains = SKL_DISPLAY_DDI_IO_B_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_B,
> > },
> > {
> > - .name = "DDI C power well",
> > - .domains = SKL_DISPLAY_DDI_C_POWER_DOMAINS,
> > + .name = "DDI C IO power well",
> > + .domains = SKL_DISPLAY_DDI_IO_C_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_C,
> > },
> > {
> > - .name = "DDI D power well",
> > - .domains = SKL_DISPLAY_DDI_D_POWER_DOMAINS,
> > + .name = "DDI D IO power well",
> > + .domains = SKL_DISPLAY_DDI_IO_D_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_D,
> > },
> > @@ -2246,20 +2256,20 @@ static struct i915_power_well glk_power_wells[] = {
> > .id = GLK_DISP_PW_AUX_C,
> > },
> > {
> > - .name = "DDI A power well",
> > - .domains = GLK_DISPLAY_DDI_A_POWER_DOMAINS,
> > + .name = "DDI A IO power well",
> > + .domains = GLK_DISPLAY_DDI_IO_A_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = GLK_DISP_PW_DDI_A,
> > },
> > {
> > - .name = "DDI B power well",
> > - .domains = GLK_DISPLAY_DDI_B_POWER_DOMAINS,
> > + .name = "DDI B IO power well",
> > + .domains = GLK_DISPLAY_DDI_IO_B_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_B,
> > },
> > {
> > - .name = "DDI C power well",
> > - .domains = GLK_DISPLAY_DDI_C_POWER_DOMAINS,
> > + .name = "DDI C IO power well",
> > + .domains = GLK_DISPLAY_DDI_IO_C_POWER_DOMAINS,
> > .ops = &skl_power_well_ops,
> > .id = SKL_DISP_PW_DDI_C,
> > },
> > --
> > 2.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
More information about the Intel-gfx
mailing list