From: David Weinehall <david.weinehall@linux.intel.com>
To: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/6] drm/i915: Only enable DDI IO power domains after enabling DPLL
Date: Mon, 13 Feb 2017 18:32:31 +0200 [thread overview]
Message-ID: <20170213163231.wwkvvkpyl2mykkt3@boom> (raw)
In-Reply-To: <20170210132959.16594-7-ander.conselvan.de.oliveira@intel.com>
On Fri, Feb 10, 2017 at 03:29: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.
>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Reviewed-by: David Weinehall <david.weinehall@linux.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 | 12 ++++++
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_runtime_pm.c | 68 +++++++++++++++++++--------------
> 5 files changed, 108 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bfccf9d..27847d4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -343,6 +343,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..72754b9 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1440,6 +1440,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> return ret;
> }
>
> +static unsigned long long
> +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 +1694,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 +1702,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 +1724,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 +1774,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;
> @@ -1793,6 +1814,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>
> intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
> }
> +
> + intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
> }
>
> void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder,
> @@ -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 585d149..0990107 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15487,6 +15487,18 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> else if (HAS_PCH_SPLIT(dev_priv))
> ilk_wm_get_hw_state(dev);
>
> + for_each_intel_encoder(dev, encoder) {
> + unsigned long long 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);
> + }
> +
> for_each_intel_crtc(dev, crtc) {
> u64 put_domains;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c8873c0..03447f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -241,6 +241,8 @@ struct intel_encoder {
> * be set correctly before calling this function. */
> void (*get_config)(struct intel_encoder *,
> struct intel_crtc_state *pipe_config);
> +
> + unsigned long long (*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
> @@ -1026,6 +1028,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 4d2dc31..67782d7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -106,6 +106,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:
> @@ -402,18 +412,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 | \
> @@ -468,12 +478,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) | \
> @@ -2154,26 +2164,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,
> },
> @@ -2305,20 +2315,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 = &glk_ddi_io_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 = &glk_ddi_io_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 = &glk_ddi_io_power_well_ops,
> .id = SKL_DISP_PW_DDI_C,
> },
> --
> 2.9.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-02-13 16:32 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-10 13:29 [PATCH 0/6] Fix Geminilake DDI power well enable timeouts Ander Conselvan de Oliveira
2017-02-10 13:29 ` [PATCH 1/6] drm/i915: Store aux power domain in intel_dp Ander Conselvan de Oliveira
2017-02-16 9:05 ` Imre Deak
2017-02-16 12:23 ` Ander Conselvan De Oliveira
2017-02-16 12:28 ` Jani Nikula
2017-02-10 13:29 ` [PATCH 2/6] drm/i915: Store encoder power domain in struct intel_encoder Ander Conselvan de Oliveira
2017-02-16 9:50 ` Imre Deak
2017-02-10 13:29 ` [PATCH 3/6] drm/i915/glk: Implement WaDDIIOTimeout Ander Conselvan de Oliveira
2017-02-16 10:07 ` Imre Deak
2017-02-10 13:29 ` [PATCH 4/6] drm/i915/glk: Don't enable DDI IO power domains during init Ander Conselvan de Oliveira
2017-02-13 16:31 ` David Weinehall
2017-02-10 13:29 ` [PATCH 5/6] drm/i915/glk: Don't attempt to sync DDI IO power well hw state Ander Conselvan de Oliveira
2017-02-16 10:19 ` Imre Deak
2017-02-10 13:29 ` [PATCH 6/6] drm/i915: Only enable DDI IO power domains after enabling DPLL Ander Conselvan de Oliveira
2017-02-13 16:32 ` David Weinehall [this message]
2017-02-16 10:16 ` Imre Deak
2017-02-10 14:26 ` ✗ Fi.CI.BAT: failure for Fix Geminilake DDI power well enable timeouts Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170213163231.wwkvvkpyl2mykkt3@boom \
--to=david.weinehall@linux.intel.com \
--cc=ander.conselvan.de.oliveira@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.