[v2,09/11] drm/i915: Turn intel_digital_port_connected() in a vfunc
diff mbox series

Message ID 20200121194403.15066-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • Untitled series #231507
Related show

Commit Message

Ville Syrjälä Jan. 21, 2020, 7:44 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's get rid of the platform if ladders in
intel_digital_port_connected() and make it a vfunc. Now the if
ladders are at the encoder initialization which makes them a bit
less convoluted.

v2: Add forward decl for intel_encoder in intel_tc.h

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c      |  19 ++++
 .../drm/i915/display/intel_display_types.h    |   1 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 107 +++++-------------
 drivers/gpu/drm/i915/display/intel_dp.h       |   6 +
 drivers/gpu/drm/i915/display/intel_tc.c       |   3 +-
 drivers/gpu/drm/i915/display/intel_tc.h       |   3 +-
 6 files changed, 61 insertions(+), 78 deletions(-)

Comments

Jani Nikula Jan. 28, 2020, 9:06 a.m. UTC | #1
On Tue, 21 Jan 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 3da166054788..cf0df6f18734 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -116,6 +116,12 @@ void intel_dp_vsc_enable(struct intel_dp *intel_dp,
>  void intel_dp_hdr_metadata_enable(struct intel_dp *intel_dp,
>  				  const struct intel_crtc_state *crtc_state,
>  				  const struct drm_connector_state *conn_state);
> +bool cpt_digital_port_connected(struct intel_encoder *encoder);
> +bool spt_digital_port_connected(struct intel_encoder *encoder);
> +bool ivb_digital_port_connected(struct intel_encoder *encoder);
> +bool bdw_digital_port_connected(struct intel_encoder *encoder);
> +bool bxt_digital_port_connected(struct intel_encoder *encoder);
> +bool icp_digital_port_connected(struct intel_encoder *encoder);

Please find a way to not expose a plethora of platform specific
functions from files. I want to take things to the completely opposite
direction.

Other than that, I like using the vfuncs.

BR,
Jani.
Ville Syrjälä Feb. 4, 2020, 9:07 p.m. UTC | #2
On Tue, Jan 28, 2020 at 11:06:42AM +0200, Jani Nikula wrote:
> On Tue, 21 Jan 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > index 3da166054788..cf0df6f18734 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -116,6 +116,12 @@ void intel_dp_vsc_enable(struct intel_dp *intel_dp,
> >  void intel_dp_hdr_metadata_enable(struct intel_dp *intel_dp,
> >  				  const struct intel_crtc_state *crtc_state,
> >  				  const struct drm_connector_state *conn_state);
> > +bool cpt_digital_port_connected(struct intel_encoder *encoder);
> > +bool spt_digital_port_connected(struct intel_encoder *encoder);
> > +bool ivb_digital_port_connected(struct intel_encoder *encoder);
> > +bool bdw_digital_port_connected(struct intel_encoder *encoder);
> > +bool bxt_digital_port_connected(struct intel_encoder *encoder);
> > +bool icp_digital_port_connected(struct intel_encoder *encoder);
> 
> Please find a way to not expose a plethora of platform specific
> functions from files. I want to take things to the completely opposite
> direction.

Many of these do disappear in the next patch. Still, I was letting
this idea simmer a bit in case I'd actually come up with a good
approach, alas no inspiration has occurred. I think we have three
options:

1) keep everyting hidden in intel_dp.c and expose some kind of
   intel_dig_port_init() thing that we call from the ddi code.
   The reason I don't really like this is I've been thinking of
   stuffing all the function pointers into some const vtables
   which would probably be mostly populated by stuff from
   intel_ddi.c. So we'd end up in the exact opposite situation
   where ddi code would have to exposes functions to intel_dp.c.

2) Store the hpd register in intel_encoder/dig_port/etc. so we could
   unify the ilk/ivb/bdw/pch functions into a single function. But
   since we still need special sauce for the TC ports we'd still want
   the vfunc. And since we do have the vfunc storing the hpd register
   offset feels rather redundant.

3) Duplicate two of the functions in intel_ddi.c:
   ivb->hsw and pch->lpt (or whatever we call it), and move
   the bdw function over entirely. The duplication seems a
   bit silly, but might actually be the least annoying approach.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index bbf1c0a243a2..d983860560cf 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4890,6 +4890,25 @@  void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 				port_name(port));
 	}
 
+	if (INTEL_GEN(dev_priv) >= 11) {
+		if (intel_phy_is_tc(dev_priv, phy))
+			intel_dig_port->connected = intel_tc_port_connected;
+		else
+			intel_dig_port->connected = icp_digital_port_connected;
+	} else if (IS_GEN9_LP(dev_priv)) {
+		intel_dig_port->connected = bxt_digital_port_connected;
+	} else if (port == PORT_A) {
+		if (INTEL_GEN(dev_priv) >= 8)
+			intel_dig_port->connected = bdw_digital_port_connected;
+		else
+			intel_dig_port->connected = ivb_digital_port_connected;
+	} else {
+		if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
+			intel_dig_port->connected = spt_digital_port_connected;
+		else
+			intel_dig_port->connected = cpt_digital_port_connected;
+	}
+
 	intel_infoframe_init(intel_dig_port);
 
 	return;
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 8c9cfafca195..2b308c612a02 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1371,6 +1371,7 @@  struct intel_digital_port {
 			       const struct drm_connector_state *conn_state);
 	u32 (*infoframes_enabled)(struct intel_encoder *encoder,
 				  const struct intel_crtc_state *pipe_config);
+	bool (*connected)(struct intel_encoder *encoder);
 };
 
 struct intel_dp_mst_encoder {
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 9956367712e9..4e1f73f80951 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5371,7 +5371,7 @@  static bool ibx_digital_port_connected(struct intel_encoder *encoder)
 	return I915_READ(SDEISR) & bit;
 }
 
-static bool cpt_digital_port_connected(struct intel_encoder *encoder)
+bool cpt_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	u32 bit;
@@ -5394,7 +5394,7 @@  static bool cpt_digital_port_connected(struct intel_encoder *encoder)
 	return I915_READ(SDEISR) & bit;
 }
 
-static bool spt_digital_port_connected(struct intel_encoder *encoder)
+bool spt_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	u32 bit;
@@ -5463,43 +5463,24 @@  static bool ilk_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
-	if (encoder->hpd_pin == HPD_PORT_A)
-		return I915_READ(DEISR) & DE_DP_A_HOTPLUG;
-	else
-		return ibx_digital_port_connected(encoder);
+	return I915_READ(DEISR) & DE_DP_A_HOTPLUG;
 }
 
-static bool snb_digital_port_connected(struct intel_encoder *encoder)
+bool ivb_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
-	if (encoder->hpd_pin == HPD_PORT_A)
-		return I915_READ(DEISR) & DE_DP_A_HOTPLUG;
-	else
-		return cpt_digital_port_connected(encoder);
+	return I915_READ(DEISR) & DE_DP_A_HOTPLUG_IVB;
 }
 
-static bool ivb_digital_port_connected(struct intel_encoder *encoder)
+bool bdw_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
-	if (encoder->hpd_pin == HPD_PORT_A)
-		return I915_READ(DEISR) & DE_DP_A_HOTPLUG_IVB;
-	else
-		return cpt_digital_port_connected(encoder);
+	return I915_READ(GEN8_DE_PORT_ISR) & GEN8_PORT_DP_A_HOTPLUG;
 }
 
-static bool bdw_digital_port_connected(struct intel_encoder *encoder)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-
-	if (encoder->hpd_pin == HPD_PORT_A)
-		return I915_READ(GEN8_DE_PORT_ISR) & GEN8_PORT_DP_A_HOTPLUG;
-	else
-		return cpt_digital_port_connected(encoder);
-}
-
-static bool bxt_digital_port_connected(struct intel_encoder *encoder)
+bool bxt_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	u32 bit;
@@ -5522,31 +5503,17 @@  static bool bxt_digital_port_connected(struct intel_encoder *encoder)
 	return I915_READ(GEN8_DE_PORT_ISR) & bit;
 }
 
-static bool intel_combo_phy_connected(struct drm_i915_private *dev_priv,
-				      enum phy phy)
+bool icp_digital_port_connected(struct intel_encoder *encoder)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+
 	if (HAS_PCH_MCC(dev_priv) && phy == PHY_C)
 		return I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(PORT_TC1);
 
 	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(phy);
 }
 
-static bool icp_digital_port_connected(struct intel_encoder *encoder)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
-
-	if (intel_phy_is_combo(dev_priv, phy))
-		return intel_combo_phy_connected(dev_priv, phy);
-	else if (intel_phy_is_tc(dev_priv, phy))
-		return intel_tc_port_connected(dig_port);
-	else
-		MISSING_CASE(encoder->hpd_pin);
-
-	return false;
-}
-
 /*
  * intel_digital_port_connected - is the specified port connected?
  * @encoder: intel_encoder
@@ -5558,44 +5525,15 @@  static bool icp_digital_port_connected(struct intel_encoder *encoder)
  *
  * Return %true if port is connected, %false otherwise.
  */
-static bool __intel_digital_port_connected(struct intel_encoder *encoder)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-
-	if (HAS_GMCH(dev_priv)) {
-		if (IS_GM45(dev_priv))
-			return gm45_digital_port_connected(encoder);
-		else
-			return g4x_digital_port_connected(encoder);
-	}
-
-	if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
-		return icp_digital_port_connected(encoder);
-	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
-		return spt_digital_port_connected(encoder);
-	else if (IS_GEN9_LP(dev_priv))
-		return bxt_digital_port_connected(encoder);
-	else if (IS_GEN(dev_priv, 8))
-		return bdw_digital_port_connected(encoder);
-	else if (IS_GEN(dev_priv, 7))
-		return ivb_digital_port_connected(encoder);
-	else if (IS_GEN(dev_priv, 6))
-		return snb_digital_port_connected(encoder);
-	else if (IS_GEN(dev_priv, 5))
-		return ilk_digital_port_connected(encoder);
-
-	MISSING_CASE(INTEL_GEN(dev_priv));
-	return false;
-}
-
 bool intel_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	bool is_connected = false;
 	intel_wakeref_t wakeref;
 
 	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
-		is_connected = __intel_digital_port_connected(encoder);
+		is_connected = dig_port->connected(encoder);
 
 	return is_connected;
 }
@@ -7634,6 +7572,23 @@  bool intel_dp_init(struct drm_i915_private *dev_priv,
 
 	intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
 
+	if (HAS_GMCH(dev_priv)) {
+		if (IS_GM45(dev_priv))
+			intel_dig_port->connected = gm45_digital_port_connected;
+		else
+			intel_dig_port->connected = g4x_digital_port_connected;
+	} else if (port == PORT_A) {
+		if (IS_IVYBRIDGE(dev_priv))
+			intel_dig_port->connected = ivb_digital_port_connected;
+		else
+			intel_dig_port->connected = ilk_digital_port_connected;
+	} else {
+		if (HAS_PCH_CPT(dev_priv))
+			intel_dig_port->connected = cpt_digital_port_connected;
+		else
+			intel_dig_port->connected = ibx_digital_port_connected;
+	}
+
 	if (port != PORT_A)
 		intel_infoframe_init(intel_dig_port);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 3da166054788..cf0df6f18734 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -116,6 +116,12 @@  void intel_dp_vsc_enable(struct intel_dp *intel_dp,
 void intel_dp_hdr_metadata_enable(struct intel_dp *intel_dp,
 				  const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state);
+bool cpt_digital_port_connected(struct intel_encoder *encoder);
+bool spt_digital_port_connected(struct intel_encoder *encoder);
+bool ivb_digital_port_connected(struct intel_encoder *encoder);
+bool bdw_digital_port_connected(struct intel_encoder *encoder);
+bool bxt_digital_port_connected(struct intel_encoder *encoder);
+bool icp_digital_port_connected(struct intel_encoder *encoder);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
 
 static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 7773169b7331..f498076b132c 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -477,8 +477,9 @@  static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port)
  * connected ports are usable, and avoids exposing to the users objects they
  * can't really use.
  */
-bool intel_tc_port_connected(struct intel_digital_port *dig_port)
+bool intel_tc_port_connected(struct intel_encoder *encoder)
 {
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	bool is_connected;
 
 	intel_tc_port_lock(dig_port);
diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
index 463f1b3c836f..b619e4736f85 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.h
+++ b/drivers/gpu/drm/i915/display/intel_tc.h
@@ -10,8 +10,9 @@ 
 #include <linux/types.h>
 
 struct intel_digital_port;
+struct intel_encoder;
 
-bool intel_tc_port_connected(struct intel_digital_port *dig_port);
+bool intel_tc_port_connected(struct intel_encoder *encoder);
 u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port);
 u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port);
 int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);