From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [RFC PATCH 3/3] drm/i915: add support for per-pipe power sequencing on vlv
Date: Thu, 5 Sep 2013 15:12:11 +0300 [thread overview]
Message-ID: <20130905121211.GA11428@intel.com> (raw)
In-Reply-To: <af79e14e81bcda7e8169c46ba844f5cb91b43e9a.1378208439.git.jani.nikula@intel.com>
On Tue, Sep 03, 2013 at 02:43:39PM +0300, Jani Nikula wrote:
> VLV has per-pipe PP registers. Set up power sequencing on mode set. The
> connector init time setup is problematic, since we don't have a pipe at
> that time. Cook up something.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 139 +++++++++++++++++++++++++++------------
> 1 file changed, 97 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0ba72f1..95d67fc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -237,24 +237,47 @@ intel_hrawclk(struct drm_device *dev)
> }
> }
>
> +static void
> +intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct edp_power_seq *out);
> +static void
> +intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> + struct intel_dp *intel_dp,
> + struct edp_power_seq *out);
> +
> +static u32 _pp_ctrl_reg(struct intel_dp *intel_dp)
> +{
> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + int pipe = to_intel_crtc(intel_dig_port->base.base.crtc)->pipe;
> +
> + return HAS_PCH_SPLIT(dev) ? PCH_PP_CONTROL : VLV_PIPE_PP_CONTROL(pipe);
> +}
> +
> +static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> +{
> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + int pipe = to_intel_crtc(intel_dig_port->base.base.crtc)->pipe;
> +
> + return HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : VLV_PIPE_PP_STATUS(pipe);
> +}
> +
> static bool ironlake_edp_have_panel_power(struct intel_dp *intel_dp)
> {
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 pp_stat_reg;
>
> - pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> - return (I915_READ(pp_stat_reg) & PP_ON) != 0;
> + return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> }
>
> static bool ironlake_edp_have_panel_vdd(struct intel_dp *intel_dp)
> {
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 pp_ctrl_reg;
>
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> - return (I915_READ(pp_ctrl_reg) & EDP_FORCE_VDD) != 0;
> + return (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;
> }
>
> static void
> @@ -262,19 +285,15 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
> {
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 pp_stat_reg, pp_ctrl_reg;
>
> if (!is_edp(intel_dp))
> return;
>
> - pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> -
> if (!ironlake_edp_have_panel_power(intel_dp) && !ironlake_edp_have_panel_vdd(intel_dp)) {
> WARN(1, "eDP powered off while attempting aux channel communication.\n");
> DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
> - I915_READ(pp_stat_reg),
> - I915_READ(pp_ctrl_reg));
> + I915_READ(_pp_stat_reg(intel_dp)),
> + I915_READ(_pp_ctrl_reg(intel_dp)));
> }
> }
>
> @@ -948,8 +967,8 @@ static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 pp_stat_reg, pp_ctrl_reg;
>
> - pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_stat_reg = _pp_stat_reg(intel_dp);
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>
> DRM_DEBUG_KMS("mask %08x value %08x status %08x control %08x\n",
> mask, value,
> @@ -991,11 +1010,8 @@ static u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 control;
> - u32 pp_ctrl_reg;
> -
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> - control = I915_READ(pp_ctrl_reg);
>
> + control = I915_READ(_pp_ctrl_reg(intel_dp));
> control &= ~PANEL_UNLOCK_MASK;
> control |= PANEL_UNLOCK_REGS;
> return control;
> @@ -1028,8 +1044,8 @@ void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
> pp = ironlake_get_pp_control(intel_dp);
> pp |= EDP_FORCE_VDD;
>
> - pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_stat_reg = _pp_stat_reg(intel_dp);
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
> @@ -1057,8 +1073,8 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
> pp = ironlake_get_pp_control(intel_dp);
> pp &= ~EDP_FORCE_VDD;
>
> - pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_stat_reg = _pp_ctrl_reg(intel_dp);
> + pp_ctrl_reg = _pp_stat_reg(intel_dp);
>
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
> @@ -1123,20 +1139,19 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
>
> ironlake_wait_panel_power_cycle(intel_dp);
>
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
> pp = ironlake_get_pp_control(intel_dp);
> if (IS_GEN5(dev)) {
> /* ILK workaround: disable reset around power sequence */
> pp &= ~PANEL_POWER_RESET;
> - I915_WRITE(PCH_PP_CONTROL, pp);
> - POSTING_READ(PCH_PP_CONTROL);
> + I915_WRITE(pp_ctrl_reg, pp);
> + POSTING_READ(pp_ctrl_reg);
> }
>
> pp |= POWER_TARGET_ON;
> if (!IS_GEN5(dev))
> pp |= PANEL_POWER_RESET;
>
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> -
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
>
> @@ -1144,8 +1159,8 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
>
> if (IS_GEN5(dev)) {
> pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> - I915_WRITE(PCH_PP_CONTROL, pp);
> - POSTING_READ(PCH_PP_CONTROL);
> + I915_WRITE(pp_ctrl_reg, pp);
> + POSTING_READ(pp_ctrl_reg);
> }
> }
>
> @@ -1168,7 +1183,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
> * panels get very unhappy and cease to work. */
> pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
>
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
> @@ -1201,7 +1216,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
> pp = ironlake_get_pp_control(intel_dp);
> pp |= EDP_BLC_ENABLE;
>
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
> @@ -1225,7 +1240,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
> pp = ironlake_get_pp_control(intel_dp);
> pp &= ~EDP_BLC_ENABLE;
>
> - pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> + pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>
> I915_WRITE(pp_ctrl_reg, pp);
> POSTING_READ(pp_ctrl_reg);
> @@ -1748,6 +1763,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> int port = vlv_dport_to_channel(dport);
> int pipe = intel_crtc->pipe;
> + struct edp_power_seq power_seq;
> u32 val;
>
> mutex_lock(&dev_priv->dpio_lock);
> @@ -1765,6 +1781,10 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>
> mutex_unlock(&dev_priv->dpio_lock);
>
> + /* init power sequencer on this pipe and port */
> + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, &power_seq);
> +
> intel_enable_dp(encoder);
>
> vlv_wait_port_ready(dev_priv, port);
> @@ -3146,6 +3166,34 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> }
> }
>
> +static enum pipe
> +vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> + struct drm_device *dev = intel_dig_port->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + enum port port = intel_dig_port->port;
> + enum pipe pipe;
> +
> + /* modeset should have pipe */
> + if (crtc)
> + return to_intel_crtc(crtc)->pipe;
> +
> + /* init time, try to find a pipe with this port selected */
> + for (pipe = PIPE_A; pipe <= PIPE_B; pipe++) {
> + u32 port_sel = I915_READ(VLV_PIPE_PP_ON_DELAYS(pipe)) &
> + PANEL_PORT_SELECT_MASK;
> + if (port_sel == PANEL_PORT_SELECT_DPB_VLV && port == PORT_B)
> + return pipe;
> + else if (port_sel == PANEL_PORT_SELECT_DPC && port == PORT_C)
> + return pipe;
> + }
> +
> + /* shrug */
> + return PIPE_A;
Yeah, looks reasonable enough to me. If the BIOS already set up
something keep using it, otherwise just pick A.
The only concern here is whether the power sequencer requires something
else to be set up? Maybe we'd need the DP port register to have the
matching pipe configured too? Would need to test that on real hardware I
suppose.
But to me this seems at least better than the current use pipe A
always situation:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> +}
> +
> static void
> intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> struct intel_dp *intel_dp,
> @@ -3154,24 +3202,26 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct edp_power_seq cur, vbt, spec, final;
> u32 pp_on, pp_off, pp_div, pp;
> - int pp_control_reg, pp_on_reg, pp_off_reg, pp_div_reg;
> + int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
>
> if (HAS_PCH_SPLIT(dev)) {
> - pp_control_reg = PCH_PP_CONTROL;
> + pp_ctrl_reg = PCH_PP_CONTROL;
> pp_on_reg = PCH_PP_ON_DELAYS;
> pp_off_reg = PCH_PP_OFF_DELAYS;
> pp_div_reg = PCH_PP_DIVISOR;
> } else {
> - pp_control_reg = PIPEA_PP_CONTROL;
> - pp_on_reg = PIPEA_PP_ON_DELAYS;
> - pp_off_reg = PIPEA_PP_OFF_DELAYS;
> - pp_div_reg = PIPEA_PP_DIVISOR;
> + enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> +
> + pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> + pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
> + pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
> + pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> }
>
> /* Workaround: Need to write PP_CONTROL with the unlock key as
> * the very first thing. */
> pp = ironlake_get_pp_control(intel_dp);
> - I915_WRITE(pp_control_reg, pp);
> + I915_WRITE(pp_ctrl_reg, pp);
>
> pp_on = I915_READ(pp_on_reg);
> pp_off = I915_READ(pp_off_reg);
> @@ -3259,9 +3309,11 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> pp_off_reg = PCH_PP_OFF_DELAYS;
> pp_div_reg = PCH_PP_DIVISOR;
> } else {
> - pp_on_reg = PIPEA_PP_ON_DELAYS;
> - pp_off_reg = PIPEA_PP_OFF_DELAYS;
> - pp_div_reg = PIPEA_PP_DIVISOR;
> + enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> +
> + pp_on_reg = VLV_PIPE_PP_ON_DELAYS(pipe);
> + pp_off_reg = VLV_PIPE_PP_OFF_DELAYS(pipe);
> + pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> }
>
> /* And finally store the new values in the power sequencer. */
> @@ -3278,7 +3330,10 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> /* Haswell doesn't have any port selection bits for the panel
> * power sequencer any more. */
> if (IS_VALLEYVIEW(dev)) {
> - port_sel = I915_READ(pp_on_reg) & 0xc0000000;
> + if (dp_to_dig_port(intel_dp)->port == PORT_B)
> + port_sel = PANEL_PORT_SELECT_DPB_VLV;
> + else
> + port_sel = PANEL_PORT_SELECT_DPC;
> } else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> if (dp_to_dig_port(intel_dp)->port == PORT_A)
> port_sel = PANEL_PORT_SELECT_DPA;
> --
> 1.7.9.5
--
Ville Syrjälä
Intel OTC
prev parent reply other threads:[~2013-09-05 12:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-03 11:43 [RFC PATCH 0/3] drm/i915: vlv backlight, per-pipe power sequencing fixes Jani Nikula
2013-09-03 11:43 ` [RFC PATCH 1/3] drm/i915: move backlight enable later in vlv enable sequence Jani Nikula
2013-09-04 18:30 ` Ville Syrjälä
2013-09-03 11:43 ` [RFC PATCH 2/3] drm/i915: clean up power sequencing register port select definitions Jani Nikula
2013-09-05 11:55 ` Ville Syrjälä
2013-09-03 11:43 ` [RFC PATCH 3/3] drm/i915: add support for per-pipe power sequencing on vlv Jani Nikula
2013-09-05 12:12 ` Ville Syrjälä [this message]
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=20130905121211.GA11428@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox