From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 02/14] drm/i915: Clean up LVDS pipe select bits
Date: Tue, 20 Mar 2018 08:39:21 +0200 [thread overview]
Message-ID: <87fu4vz1wm.fsf@intel.com> (raw)
In-Reply-To: <20180302095512.19329-3-ville.syrjala@linux.intel.com>
On Fri, 02 Mar 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Clean up the LVDS pipe select bits. To make the whole situation a bit
> less ugly we'll start to share the same code between .get_hw_state()
> and the port state asserts.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Couple of nitpicks below, but regardless,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9 ++++--
> drivers/gpu/drm/i915/intel_display.c | 35 ++++++-----------------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_lvds.c | 54 +++++++++++++++++++-----------------
> 4 files changed, 44 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f573095d60c2..e993eec97c98 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4458,9 +4458,12 @@ enum {
> */
> #define LVDS_PORT_EN (1 << 31)
> /* Selects pipe B for LVDS data. Must be set on pre-965. */
> -#define LVDS_PIPEB_SELECT (1 << 30)
> -#define LVDS_PIPE_MASK (1 << 30)
> -#define LVDS_PIPE(pipe) ((pipe) << 30)
> +#define LVDS_PIPE_SEL(pipe) ((pipe) << 30)
> +#define LVDS_PIPE_SEL_MASK (1 << 30)
> +#define LVDS_PIPE_SEL_SHIFT 30
> +#define LVDS_PIPE_SEL_CPT(pipe) ((pipe) << 29)
> +#define LVDS_PIPE_SEL_MASK_CPT (3 << 30)
> +#define LVDS_PIPE_SEL_SHIFT_CPT 29
I'd prefer masks and shifts before values.
> /* LVDS dithering flag on 965/g4x platform */
> #define LVDS_ENABLE_DITHER (1 << 25)
> /* LVDS sync polarity flags. Set to invert (i.e. negative) */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 545d89152e9b..f1f164a20b3f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1171,9 +1171,9 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
> pp_reg = PP_CONTROL(0);
> port_sel = I915_READ(PP_ON_DELAYS(0)) & PANEL_PORT_SELECT_MASK;
>
> - if (port_sel == PANEL_PORT_SELECT_LVDS &&
> - I915_READ(PCH_LVDS) & LVDS_PIPEB_SELECT)
> - panel_pipe = PIPE_B;
> + if (port_sel == PANEL_PORT_SELECT_LVDS) {
> + intel_lvds_port_enabled(dev_priv, PCH_LVDS, &panel_pipe);
> + }
Superfluous braces.
> /* XXX: else fix for eDP */
> } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> /* presumably write lock depends on pipe, not port select */
> @@ -1181,8 +1181,7 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
> panel_pipe = pipe;
> } else {
> pp_reg = PP_CONTROL(0);
> - if (I915_READ(LVDS) & LVDS_PIPEB_SELECT)
> - panel_pipe = PIPE_B;
> + intel_lvds_port_enabled(dev_priv, LVDS, &panel_pipe);
> }
>
> val = I915_READ(pp_reg);
> @@ -1301,22 +1300,6 @@ static bool hdmi_pipe_enabled(struct drm_i915_private *dev_priv,
> return true;
> }
>
> -static bool lvds_pipe_enabled(struct drm_i915_private *dev_priv,
> - enum pipe pipe, u32 val)
> -{
> - if ((val & LVDS_PORT_EN) == 0)
> - return false;
> -
> - if (HAS_PCH_CPT(dev_priv)) {
> - if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
> - return false;
> - } else {
> - if ((val & LVDS_PIPE_MASK) != LVDS_PIPE(pipe))
> - return false;
> - }
> - return true;
> -}
> -
> static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> enum pipe pipe, i915_reg_t reg,
> u32 port_sel)
> @@ -1348,7 +1331,6 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
> enum pipe pipe)
> {
> enum pipe port_pipe;
> - u32 val;
>
> assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL_B);
> assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL_C);
> @@ -1359,10 +1341,10 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
> "PCH VGA enabled on transcoder %c, should be disabled\n",
> pipe_name(pipe));
>
> - val = I915_READ(PCH_LVDS);
> - I915_STATE_WARN(lvds_pipe_enabled(dev_priv, pipe, val),
> - "PCH LVDS enabled on transcoder %c, should be disabled\n",
> - pipe_name(pipe));
> + I915_STATE_WARN(intel_lvds_port_enabled(dev_priv, PCH_LVDS, &port_pipe) &&
> + port_pipe == pipe,
> + "PCH LVDS enabled on transcoder %c, should be disabled\n",
> + pipe_name(pipe));
>
> assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIB);
> assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIC);
> @@ -1405,7 +1387,6 @@ static void vlv_enable_pll(struct intel_crtc *crtc,
> POSTING_READ(DPLL_MD(pipe));
> }
>
> -
Superfluous whitespace change.
> static void _chv_enable_pll(struct intel_crtc *crtc,
> const struct intel_crtc_state *pipe_config)
> {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 79e741845e16..16b7adc7b0c9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1789,6 +1789,8 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
>
>
> /* intel_lvds.c */
> +bool intel_lvds_port_enabled(struct drm_i915_private *dev_priv,
> + i915_reg_t lvds_reg, enum pipe *pipe);
> void intel_lvds_init(struct drm_i915_private *dev_priv);
> struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
> bool intel_is_dual_link_lvds(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index d35d2d50f595..571b69f79eda 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -85,34 +85,35 @@ static struct intel_lvds_connector *to_lvds_connector(struct drm_connector *conn
> return container_of(connector, struct intel_lvds_connector, base.base);
> }
>
> +bool intel_lvds_port_enabled(struct drm_i915_private *dev_priv,
> + i915_reg_t lvds_reg, enum pipe *pipe)
> +{
> + u32 val;
> +
> + val = I915_READ(lvds_reg);
> +
> + /* asserts want to know the pipe even if the port is disabled */
> + if (HAS_PCH_CPT(dev_priv))
> + *pipe = (val & LVDS_PIPE_SEL_MASK_CPT) >> LVDS_PIPE_SEL_SHIFT_CPT;
> + else
> + *pipe = (val & LVDS_PIPE_SEL_MASK) >> LVDS_PIPE_SEL_SHIFT;
> +
> + return val & LVDS_PORT_EN;
> +}
> +
> static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
> enum pipe *pipe)
> {
> - struct drm_device *dev = encoder->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> - u32 tmp;
> bool ret;
>
> if (!intel_display_power_get_if_enabled(dev_priv,
> encoder->power_domain))
> return false;
>
> - ret = false;
> + ret = intel_lvds_port_enabled(dev_priv, lvds_encoder->reg, pipe);
>
> - tmp = I915_READ(lvds_encoder->reg);
> -
> - if (!(tmp & LVDS_PORT_EN))
> - goto out;
> -
> - if (HAS_PCH_CPT(dev_priv))
> - *pipe = PORT_TO_PIPE_CPT(tmp);
> - else
> - *pipe = PORT_TO_PIPE(tmp);
> -
> - ret = true;
> -
> -out:
> intel_display_power_put(dev_priv, encoder->power_domain);
>
> return ret;
> @@ -255,14 +256,11 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder,
> temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
>
> if (HAS_PCH_CPT(dev_priv)) {
> - temp &= ~PORT_TRANS_SEL_MASK;
> - temp |= PORT_TRANS_SEL_CPT(pipe);
> + temp &= ~LVDS_PIPE_SEL_MASK_CPT;
> + temp |= LVDS_PIPE_SEL_CPT(pipe);
> } else {
> - if (pipe == 1) {
> - temp |= LVDS_PIPEB_SELECT;
> - } else {
> - temp &= ~LVDS_PIPEB_SELECT;
> - }
> + temp &= ~LVDS_PIPE_SEL_MASK;
> + temp |= LVDS_PIPE_SEL(pipe);
> }
>
> /* set the corresponsding LVDS_BORDER bit */
> @@ -906,8 +904,12 @@ static bool compute_is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
> * we need to check "the value to be set" in VBT when LVDS
> * register is uninitialized.
> */
> - val = I915_READ(lvds_encoder->reg);
> - if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> + val = I915_READ(lvds_encoder->reg) & ~LVDS_DETECTED;
> + if (HAS_PCH_CPT(dev_priv))
> + val &= ~LVDS_PIPE_SEL_MASK_CPT;
> + else
> + val &= ~LVDS_PIPE_SEL_MASK;
Matter of taste, I'd duplicate the LVDS_DETECTED in the masking instead
of combining it in the register read line.
> + if (val == 0)
> val = dev_priv->vbt.bios_lvds_val;
>
> return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-03-20 6:39 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-02 9:54 [PATCH 00/14] drm/i915: Clean up the port pipe select bits Ville Syrjala
2018-03-02 9:54 ` [PATCH 01/14] drm/i915: Clean up ADPA " Ville Syrjala
2018-03-20 6:27 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 02/14] drm/i915: Clean up LVDS " Ville Syrjala
2018-03-20 6:39 ` Jani Nikula [this message]
2018-03-02 9:55 ` [PATCH 03/14] drm/i915: Clean up SDVO " Ville Syrjala
2018-03-20 6:50 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 04/14] drm/i915: Clean up TV " Ville Syrjala
2018-03-20 6:55 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 05/14] drm/i915: Clean up DVO " Ville Syrjala
2018-03-20 7:00 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 06/14] drm/i915: Use intel_ddi_dp_voltage_max() for HSW/BDW too Ville Syrjala
2018-03-02 9:55 ` [PATCH 07/14] drm/i915: Use the same vswing->max_preemph mapping on HSW/BDW as on SKL+ Ville Syrjala
2018-03-02 9:55 ` [PATCH 08/14] drm/i915: Check for IVB instead of gen7 when we think about IVB CPU eDP Ville Syrjala
2018-03-20 7:11 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 09/14] drm/i915: Move intel_ddi_get_crtc_new_encoder() out from ddi code Ville Syrjala
2018-03-20 7:19 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 10/14] drm/i915: Parametrize TRANS_DP_PORT_SEL Ville Syrjala
2018-03-20 8:36 ` Jani Nikula
2018-03-02 9:55 ` [PATCH 11/14] drm/i915: Nuke intel_trans_dp_port_sel() Ville Syrjala
2018-03-02 9:55 ` [PATCH 12/14] drm/i915: Clean up DP pipe select bits Ville Syrjala
2018-03-02 18:08 ` [PATCH v2 " Ville Syrjala
2018-03-02 9:55 ` [PATCH 13/14] drm/i915: Allow eDP on port C in theory Ville Syrjala
2018-03-02 9:55 ` [PATCH 14/14] drm/i915: Implement the missing bits of assert_panel_unlocked() Ville Syrjala
2018-03-02 11:02 ` ✗ Fi.CI.BAT: failure for drm/i915: Clean up the port pipe select bits Patchwork
2018-03-02 13:09 ` Patchwork
2018-03-02 18:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Clean up the port pipe select bits (rev2) 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=87fu4vz1wm.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.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 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.