From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/14] drm/i915: Clean up SDVO pipe select bits
Date: Tue, 20 Mar 2018 08:50:14 +0200 [thread overview]
Message-ID: <87d0zzz1eh.fsf@intel.com> (raw)
In-Reply-To: <20180302095512.19329-4-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 SDVO 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>
Good stuff. Same nitpicks about masks and shifts before values, but it's
already there before this patch,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 4 +++-
> drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++-----------------------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_hdmi.c | 25 ++++----------------
> drivers/gpu/drm/i915/intel_sdvo.c | 38 ++++++++++++++++++-----------
> 5 files changed, 49 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e993eec97c98..0917fcbd618d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4365,7 +4365,7 @@ enum {
> #define SDVO_ENABLE (1 << 31)
> #define SDVO_PIPE_SEL(pipe) ((pipe) << 30)
> #define SDVO_PIPE_SEL_MASK (1 << 30)
> -#define SDVO_PIPE_B_SELECT (1 << 30)
> +#define SDVO_PIPE_SEL_SHIFT 30
> #define SDVO_STALL_SELECT (1 << 29)
> #define SDVO_INTERRUPT_ENABLE (1 << 26)
> /*
> @@ -4407,10 +4407,12 @@ enum {
> /* Gen 6 (CPT) SDVO/HDMI bits: */
> #define SDVO_PIPE_SEL_CPT(pipe) ((pipe) << 29)
> #define SDVO_PIPE_SEL_MASK_CPT (3 << 29)
> +#define SDVO_PIPE_SEL_SHIFT_CPT 29
>
> /* CHV SDVO/HDMI bits: */
> #define SDVO_PIPE_SEL_CHV(pipe) ((pipe) << 24)
> #define SDVO_PIPE_SEL_MASK_CHV (3 << 24)
> +#define SDVO_PIPE_SEL_SHIFT_CHV 24
>
>
> /* DVO port control */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f1f164a20b3f..26f8f1e741f7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1281,25 +1281,6 @@ static bool dp_pipe_enabled(struct drm_i915_private *dev_priv,
> return true;
> }
>
> -static bool hdmi_pipe_enabled(struct drm_i915_private *dev_priv,
> - enum pipe pipe, u32 val)
> -{
> - if ((val & SDVO_ENABLE) == 0)
> - return false;
> -
> - if (HAS_PCH_CPT(dev_priv)) {
> - if ((val & SDVO_PIPE_SEL_MASK_CPT) != SDVO_PIPE_SEL_CPT(pipe))
> - return false;
> - } else if (IS_CHERRYVIEW(dev_priv)) {
> - if ((val & SDVO_PIPE_SEL_MASK_CHV) != SDVO_PIPE_SEL_CHV(pipe))
> - return false;
> - } else {
> - if ((val & SDVO_PIPE_SEL_MASK) != SDVO_PIPE_SEL(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)
> @@ -1315,16 +1296,21 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> }
>
> static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
> - enum pipe pipe, i915_reg_t reg)
> + enum pipe pipe, enum port port,
> + i915_reg_t hdmi_reg)
> {
> - u32 val = I915_READ(reg);
> - I915_STATE_WARN(hdmi_pipe_enabled(dev_priv, pipe, val),
> - "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
> - i915_mmio_reg_offset(reg), pipe_name(pipe));
> + enum pipe port_pipe;
> + bool state;
> +
> + state = intel_sdvo_port_enabled(dev_priv, hdmi_reg, &port_pipe);
> +
> + I915_STATE_WARN(state && port_pipe == pipe,
> + "PCH HDMI %c enabled on transcoder %c, should be disabled\n",
> + port_name(port), pipe_name(pipe));
>
> - I915_STATE_WARN(HAS_PCH_IBX(dev_priv) && (val & SDVO_ENABLE) == 0
> - && (val & SDVO_PIPE_B_SELECT),
> - "IBX PCH hdmi port still using transcoder B\n");
> + I915_STATE_WARN(HAS_PCH_IBX(dev_priv) && !state && port_pipe == PIPE_B,
> + "IBX PCH HDMI %c still using transcoder B\n",
> + port_name(port));
> }
>
> static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
> @@ -1346,9 +1332,9 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
> "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);
> - assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMID);
> + assert_pch_hdmi_disabled(dev_priv, pipe, PORT_B, PCH_HDMIB);
> + assert_pch_hdmi_disabled(dev_priv, pipe, PORT_C, PCH_HDMIC);
> + assert_pch_hdmi_disabled(dev_priv, pipe, PORT_D, PCH_HDMID);
> }
>
> static void _vlv_enable_pll(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 16b7adc7b0c9..09a1b968ac9c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2024,6 +2024,8 @@ void intel_init_ipc(struct drm_i915_private *dev_priv);
> void intel_enable_ipc(struct drm_i915_private *dev_priv);
>
> /* intel_sdvo.c */
> +bool intel_sdvo_port_enabled(struct drm_i915_private *dev_priv,
> + i915_reg_t sdvo_reg, enum pipe *pipe);
> bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> i915_reg_t reg, enum port port);
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index f5d7bfb43006..bf6facec4efb 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1161,33 +1161,16 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder,
> static bool intel_hdmi_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_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> - u32 tmp;
> bool ret;
>
> if (!intel_display_power_get_if_enabled(dev_priv,
> encoder->power_domain))
> return false;
>
> - ret = false;
> -
> - tmp = I915_READ(intel_hdmi->hdmi_reg);
> -
> - if (!(tmp & SDVO_ENABLE))
> - goto out;
> -
> - if (HAS_PCH_CPT(dev_priv))
> - *pipe = PORT_TO_PIPE_CPT(tmp);
> - else if (IS_CHERRYVIEW(dev_priv))
> - *pipe = SDVO_PORT_TO_PIPE_CHV(tmp);
> - else
> - *pipe = PORT_TO_PIPE(tmp);
> -
> - ret = true;
> + ret = intel_sdvo_port_enabled(dev_priv, intel_hdmi->hdmi_reg, pipe);
>
> -out:
> intel_display_power_put(dev_priv, encoder->power_domain);
>
> return ret;
> @@ -1421,8 +1404,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder,
> intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
>
> - temp &= ~SDVO_PIPE_B_SELECT;
> - temp |= SDVO_ENABLE;
> + temp &= ~SDVO_PIPE_SEL_MASK;
> + temp |= SDVO_ENABLE | SDVO_PIPE_SEL(PIPE_A);
> /*
> * HW workaround, need to write this twice for issue
> * that may result in first write getting masked.
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 0c14d1c04cbd..3a90bcfbbedb 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1403,27 +1403,37 @@ static bool intel_sdvo_connector_get_hw_state(struct intel_connector *connector)
> return false;
> }
>
> +bool intel_sdvo_port_enabled(struct drm_i915_private *dev_priv,
> + i915_reg_t sdvo_reg, enum pipe *pipe)
> +{
> + u32 val;
> +
> + val = I915_READ(sdvo_reg);
> +
> + /* asserts want to know the pipe even if the port is disabled */
> + if (HAS_PCH_CPT(dev_priv))
> + *pipe = (val & SDVO_PIPE_SEL_MASK_CPT) >> SDVO_PIPE_SEL_SHIFT_CPT;
> + else if (IS_CHERRYVIEW(dev_priv))
> + *pipe = (val & SDVO_PIPE_SEL_MASK_CHV) >> SDVO_PIPE_SEL_SHIFT_CHV;
> + else
> + *pipe = (val & SDVO_PIPE_SEL_MASK) >> SDVO_PIPE_SEL_SHIFT;
> +
> + return val & SDVO_ENABLE;
> +}
> +
> static bool intel_sdvo_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_sdvo *intel_sdvo = to_sdvo(encoder);
> u16 active_outputs = 0;
> - u32 tmp;
> + bool ret;
>
> - tmp = I915_READ(intel_sdvo->sdvo_reg);
> intel_sdvo_get_active_outputs(intel_sdvo, &active_outputs);
>
> - if (!(tmp & SDVO_ENABLE) && (active_outputs == 0))
> - return false;
> -
> - if (HAS_PCH_CPT(dev_priv))
> - *pipe = PORT_TO_PIPE_CPT(tmp);
> - else
> - *pipe = PORT_TO_PIPE(tmp);
> + ret = intel_sdvo_port_enabled(dev_priv, intel_sdvo->sdvo_reg, pipe);
>
> - return true;
> + return ret || active_outputs;
> }
>
> static void intel_sdvo_get_config(struct intel_encoder *encoder,
> @@ -1550,8 +1560,8 @@ static void intel_disable_sdvo(struct intel_encoder *encoder,
> intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
>
> - temp &= ~SDVO_PIPE_B_SELECT;
> - temp |= SDVO_ENABLE;
> + temp &= ~SDVO_PIPE_SEL_MASK;
> + temp |= SDVO_ENABLE | SDVO_PIPE_SEL(PIPE_A);
> intel_sdvo_write_sdvox(intel_sdvo, temp);
>
> temp &= ~SDVO_ENABLE;
--
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:50 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
2018-03-02 9:55 ` [PATCH 03/14] drm/i915: Clean up SDVO " Ville Syrjala
2018-03-20 6:50 ` Jani Nikula [this message]
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=87d0zzz1eh.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.