diff mbox series

[03/15] drm/i915: Introduce .{enable, disable}_clock() encoder vfuncs

Message ID 20210201183343.15292-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Clean up the DDI clock routing mess | expand

Commit Message

Ville Syrjälä Feb. 1, 2021, 6:33 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current code dealing with the clock routing for DDI encoders
is a maintenance nightmare. Let's start cleaning it up by allowing
the encoder to provide vfuncs for enablign/disabling the clock.

We leave them initially unimplemented, falling back to the old
if-else approach.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c      | 29 +++++++++++++++----
 .../drm/i915/display/intel_display_types.h    |  6 ++++
 2 files changed, 29 insertions(+), 6 deletions(-)

Comments

Lucas De Marchi Feb. 1, 2021, 7:04 p.m. UTC | #1 
On Mon, Feb 01, 2021 at 08:33:31PM +0200, Ville Syrjälä wrote:
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The current code dealing with the clock routing for DDI encoders
>is a maintenance nightmare. Let's start cleaning it up by allowing
>the encoder to provide vfuncs for enablign/disabling the clock.
>
>We leave them initially unimplemented, falling back to the old
>if-else approach.
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/display/intel_ddi.c      | 29 +++++++++++++++----
> .../drm/i915/display/intel_display_types.h    |  6 ++++
> 2 files changed, 29 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>index 23fbb9013e09..da8bb9a2de0b 100644
>--- a/drivers/gpu/drm/i915/display/intel_ddi.c
>+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>@@ -3464,6 +3464,23 @@ static void intel_ddi_clk_disable(struct intel_encoder *encoder)
> 	}
> }
>
>+static void intel_ddi_enable_clock(struct intel_encoder *encoder,
>+				   const struct intel_crtc_state *crtc_state)
>+{
>+	if (encoder->enable_clock)
>+		encoder->enable_clock(encoder, crtc_state);
>+	else
>+		intel_ddi_clk_select(encoder, crtc_state);
>+}
>+
>+static void intel_ddi_disable_clock(struct intel_encoder *encoder)
>+{
>+	if (encoder->disable_clock)
>+		encoder->disable_clock(encoder);
>+	else
>+		intel_ddi_clk_disable(encoder);

intel_ddi_disable_clock  vs  intel_ddi_clk_disable

I think the names here is very prone to mistake. Maybe we should come
back with better names?  We could have renamed intel_ddi_clk_disable to
__ddi_clk_disable in this patch, since we are going to remove the
function once everything is converted.

Lucas De Marchi


>+}
>+
> static void
> icl_program_mg_dp_mode(struct intel_digital_port *dig_port,
> 		       const struct intel_crtc_state *crtc_state)
>@@ -3708,7 +3725,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> 	 * hsw_crtc_enable()->intel_enable_shared_dpll().  We need only
> 	 * configure the PLL to port mapping here.
> 	 */
>-	intel_ddi_clk_select(encoder, crtc_state);
>+	intel_ddi_enable_clock(encoder, crtc_state);
>
> 	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
> 	if (!intel_phy_is_tc(dev_priv, phy) ||
>@@ -3829,7 +3846,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>
> 	intel_pps_on(intel_dp);
>
>-	intel_ddi_clk_select(encoder, crtc_state);
>+	intel_ddi_enable_clock(encoder, crtc_state);
>
> 	if (!intel_phy_is_tc(dev_priv, phy) ||
> 	    dig_port->tc_mode != TC_PORT_TBT_ALT) {
>@@ -3904,7 +3921,7 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
> 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>
> 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
>-	intel_ddi_clk_select(encoder, crtc_state);
>+	intel_ddi_enable_clock(encoder, crtc_state);
>
> 	drm_WARN_ON(&dev_priv->drm, dig_port->ddi_io_wakeref);
> 	dig_port->ddi_io_wakeref = intel_display_power_get(dev_priv,
>@@ -4056,7 +4073,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
> 					dig_port->ddi_io_power_domain,
> 					fetch_and_zero(&dig_port->ddi_io_wakeref));
>
>-	intel_ddi_clk_disable(encoder);
>+	intel_ddi_disable_clock(encoder);
> }
>
> static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
>@@ -4079,7 +4096,7 @@ static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
> 				dig_port->ddi_io_power_domain,
> 				fetch_and_zero(&dig_port->ddi_io_wakeref));
>
>-	intel_ddi_clk_disable(encoder);
>+	intel_ddi_disable_clock(encoder);
>
> 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
> }
>@@ -4179,7 +4196,7 @@ void intel_ddi_fdi_post_disable(struct intel_atomic_state *state,
> 	intel_de_write(dev_priv, FDI_RX_CTL(PIPE_A), val);
>
> 	intel_disable_ddi_buf(encoder, old_crtc_state);
>-	intel_ddi_clk_disable(encoder);
>+	intel_ddi_disable_clock(encoder);
>
> 	val = intel_de_read(dev_priv, FDI_RX_MISC(PIPE_A));
> 	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>index 39397748b4b0..085162616112 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_types.h
>+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>@@ -219,6 +219,12 @@ struct intel_encoder {
> 	 * encoders have been disabled and suspended.
> 	 */
> 	void (*shutdown)(struct intel_encoder *encoder);
>+	/*
>+	 * Enable/disable the clock to the port.
>+	 */
>+	void (*enable_clock)(struct intel_encoder *encoder,
>+			     const struct intel_crtc_state *crtc_state);
>+	void (*disable_clock)(struct intel_encoder *encoder);
> 	enum hpd_pin hpd_pin;
> 	enum intel_display_power_domain power_domain;
> 	/* for communication with audio component; protected by av_mutex */
>-- 
>2.26.2
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Feb. 1, 2021, 7:09 p.m. UTC | #2 
On Mon, Feb 01, 2021 at 11:04:40AM -0800, Lucas De Marchi wrote:
> On Mon, Feb 01, 2021 at 08:33:31PM +0200, Ville Syrjälä wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The current code dealing with the clock routing for DDI encoders
> >is a maintenance nightmare. Let's start cleaning it up by allowing
> >the encoder to provide vfuncs for enablign/disabling the clock.
> >
> >We leave them initially unimplemented, falling back to the old
> >if-else approach.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_ddi.c      | 29 +++++++++++++++----
> > .../drm/i915/display/intel_display_types.h    |  6 ++++
> > 2 files changed, 29 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >index 23fbb9013e09..da8bb9a2de0b 100644
> >--- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >@@ -3464,6 +3464,23 @@ static void intel_ddi_clk_disable(struct intel_encoder *encoder)
> > 	}
> > }
> >
> >+static void intel_ddi_enable_clock(struct intel_encoder *encoder,
> >+				   const struct intel_crtc_state *crtc_state)
> >+{
> >+	if (encoder->enable_clock)
> >+		encoder->enable_clock(encoder, crtc_state);
> >+	else
> >+		intel_ddi_clk_select(encoder, crtc_state);
> >+}
> >+
> >+static void intel_ddi_disable_clock(struct intel_encoder *encoder)
> >+{
> >+	if (encoder->disable_clock)
> >+		encoder->disable_clock(encoder);
> >+	else
> >+		intel_ddi_clk_disable(encoder);
> 
> intel_ddi_disable_clock  vs  intel_ddi_clk_disable
> 
> I think the names here is very prone to mistake. Maybe we should come
> back with better names?  We could have renamed intel_ddi_clk_disable to
> __ddi_clk_disable in this patch, since we are going to remove the
> function once everything is converted.

It's a dead man walking anyway. Don't see much point in renaming it.

> 
> Lucas De Marchi
> 
> 
> >+}
> >+
> > static void
> > icl_program_mg_dp_mode(struct intel_digital_port *dig_port,
> > 		       const struct intel_crtc_state *crtc_state)
> >@@ -3708,7 +3725,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> > 	 * hsw_crtc_enable()->intel_enable_shared_dpll().  We need only
> > 	 * configure the PLL to port mapping here.
> > 	 */
> >-	intel_ddi_clk_select(encoder, crtc_state);
> >+	intel_ddi_enable_clock(encoder, crtc_state);
> >
> > 	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
> > 	if (!intel_phy_is_tc(dev_priv, phy) ||
> >@@ -3829,7 +3846,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >
> > 	intel_pps_on(intel_dp);
> >
> >-	intel_ddi_clk_select(encoder, crtc_state);
> >+	intel_ddi_enable_clock(encoder, crtc_state);
> >
> > 	if (!intel_phy_is_tc(dev_priv, phy) ||
> > 	    dig_port->tc_mode != TC_PORT_TBT_ALT) {
> >@@ -3904,7 +3921,7 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
> > 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >
> > 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> >-	intel_ddi_clk_select(encoder, crtc_state);
> >+	intel_ddi_enable_clock(encoder, crtc_state);
> >
> > 	drm_WARN_ON(&dev_priv->drm, dig_port->ddi_io_wakeref);
> > 	dig_port->ddi_io_wakeref = intel_display_power_get(dev_priv,
> >@@ -4056,7 +4073,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
> > 					dig_port->ddi_io_power_domain,
> > 					fetch_and_zero(&dig_port->ddi_io_wakeref));
> >
> >-	intel_ddi_clk_disable(encoder);
> >+	intel_ddi_disable_clock(encoder);
> > }
> >
> > static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
> >@@ -4079,7 +4096,7 @@ static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
> > 				dig_port->ddi_io_power_domain,
> > 				fetch_and_zero(&dig_port->ddi_io_wakeref));
> >
> >-	intel_ddi_clk_disable(encoder);
> >+	intel_ddi_disable_clock(encoder);
> >
> > 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
> > }
> >@@ -4179,7 +4196,7 @@ void intel_ddi_fdi_post_disable(struct intel_atomic_state *state,
> > 	intel_de_write(dev_priv, FDI_RX_CTL(PIPE_A), val);
> >
> > 	intel_disable_ddi_buf(encoder, old_crtc_state);
> >-	intel_ddi_clk_disable(encoder);
> >+	intel_ddi_disable_clock(encoder);
> >
> > 	val = intel_de_read(dev_priv, FDI_RX_MISC(PIPE_A));
> > 	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
> >diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> >index 39397748b4b0..085162616112 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_types.h
> >+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> >@@ -219,6 +219,12 @@ struct intel_encoder {
> > 	 * encoders have been disabled and suspended.
> > 	 */
> > 	void (*shutdown)(struct intel_encoder *encoder);
> >+	/*
> >+	 * Enable/disable the clock to the port.
> >+	 */
> >+	void (*enable_clock)(struct intel_encoder *encoder,
> >+			     const struct intel_crtc_state *crtc_state);
> >+	void (*disable_clock)(struct intel_encoder *encoder);
> > 	enum hpd_pin hpd_pin;
> > 	enum intel_display_power_domain power_domain;
> > 	/* for communication with audio component; protected by av_mutex */
> >-- 
> >2.26.2
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 23fbb9013e09..da8bb9a2de0b 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3464,6 +3464,23 @@  static void intel_ddi_clk_disable(struct intel_encoder *encoder)
 	}
 }
 
+static void intel_ddi_enable_clock(struct intel_encoder *encoder,
+				   const struct intel_crtc_state *crtc_state)
+{
+	if (encoder->enable_clock)
+		encoder->enable_clock(encoder, crtc_state);
+	else
+		intel_ddi_clk_select(encoder, crtc_state);
+}
+
+static void intel_ddi_disable_clock(struct intel_encoder *encoder)
+{
+	if (encoder->disable_clock)
+		encoder->disable_clock(encoder);
+	else
+		intel_ddi_clk_disable(encoder);
+}
+
 static void
 icl_program_mg_dp_mode(struct intel_digital_port *dig_port,
 		       const struct intel_crtc_state *crtc_state)
@@ -3708,7 +3725,7 @@  static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	 * hsw_crtc_enable()->intel_enable_shared_dpll().  We need only
 	 * configure the PLL to port mapping here.
 	 */
-	intel_ddi_clk_select(encoder, crtc_state);
+	intel_ddi_enable_clock(encoder, crtc_state);
 
 	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
 	if (!intel_phy_is_tc(dev_priv, phy) ||
@@ -3829,7 +3846,7 @@  static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 
 	intel_pps_on(intel_dp);
 
-	intel_ddi_clk_select(encoder, crtc_state);
+	intel_ddi_enable_clock(encoder, crtc_state);
 
 	if (!intel_phy_is_tc(dev_priv, phy) ||
 	    dig_port->tc_mode != TC_PORT_TBT_ALT) {
@@ -3904,7 +3921,7 @@  static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
-	intel_ddi_clk_select(encoder, crtc_state);
+	intel_ddi_enable_clock(encoder, crtc_state);
 
 	drm_WARN_ON(&dev_priv->drm, dig_port->ddi_io_wakeref);
 	dig_port->ddi_io_wakeref = intel_display_power_get(dev_priv,
@@ -4056,7 +4073,7 @@  static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
 					dig_port->ddi_io_power_domain,
 					fetch_and_zero(&dig_port->ddi_io_wakeref));
 
-	intel_ddi_clk_disable(encoder);
+	intel_ddi_disable_clock(encoder);
 }
 
 static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
@@ -4079,7 +4096,7 @@  static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
 				dig_port->ddi_io_power_domain,
 				fetch_and_zero(&dig_port->ddi_io_wakeref));
 
-	intel_ddi_clk_disable(encoder);
+	intel_ddi_disable_clock(encoder);
 
 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
 }
@@ -4179,7 +4196,7 @@  void intel_ddi_fdi_post_disable(struct intel_atomic_state *state,
 	intel_de_write(dev_priv, FDI_RX_CTL(PIPE_A), val);
 
 	intel_disable_ddi_buf(encoder, old_crtc_state);
-	intel_ddi_clk_disable(encoder);
+	intel_ddi_disable_clock(encoder);
 
 	val = intel_de_read(dev_priv, FDI_RX_MISC(PIPE_A));
 	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 39397748b4b0..085162616112 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -219,6 +219,12 @@  struct intel_encoder {
 	 * encoders have been disabled and suspended.
 	 */
 	void (*shutdown)(struct intel_encoder *encoder);
+	/*
+	 * Enable/disable the clock to the port.
+	 */
+	void (*enable_clock)(struct intel_encoder *encoder,
+			     const struct intel_crtc_state *crtc_state);
+	void (*disable_clock)(struct intel_encoder *encoder);
 	enum hpd_pin hpd_pin;
 	enum intel_display_power_domain power_domain;
 	/* for communication with audio component; protected by av_mutex */