diff mbox series

[1/4] drm/i915/fdi: move fdi modeset asserts to intel_fdi.c

Message ID 427d27eb4e5daca208d496d6c2ffc91ed90ba714.1632992608.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: move modeset asserts out of intel_display.c | expand

Commit Message

Jani Nikula Sept. 30, 2021, 9:22 a.m. UTC 
Keep the functionality and the assert code together.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 72 ----------------
 drivers/gpu/drm/i915/display/intel_display.h |  4 -
 drivers/gpu/drm/i915/display/intel_fdi.c     | 91 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_fdi.h     | 10 +++
 drivers/gpu/drm/i915/display/intel_lvds.c    |  1 +
 5 files changed, 102 insertions(+), 76 deletions(-)

Comments

Ville Syrjälä Sept. 30, 2021, 9:44 a.m. UTC | #1 
On Thu, Sep 30, 2021 at 12:22:58PM +0300, Jani Nikula wrote:
<snip>
> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
> index af01d1fa761e..02d3294bad7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_fdi.c
> +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
> @@ -10,6 +10,97 @@
>  #include "intel_fdi.h"
>  #include "intel_sideband.h"
>  
> +static void assert_fdi_tx(struct drm_i915_private *dev_priv,
> +			  enum pipe pipe, bool state)
> +{
> +	bool cur_state;
> +
> +	if (HAS_DDI(dev_priv)) {
> +		/*
> +		 * DDI does not have a specific FDI_TX register.
> +		 *
> +		 * FDI is never fed from EDP transcoder
> +		 * so pipe->transcoder cast is fine here.
> +		 */
> +		enum transcoder cpu_transcoder = (enum transcoder)pipe;
> +		cur_state = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder)) & TRANS_DDI_FUNC_ENABLE;
> +	} else {
> +		cur_state = intel_de_read(dev_priv, FDI_TX_CTL(pipe)) & FDI_TX_ENABLE;
> +	}
> +	I915_STATE_WARN(cur_state != state,
> +			"FDI TX state assertion failure (expected %s, current %s)\n",
> +			onoff(state), onoff(cur_state));
> +}
> +
> +void assert_fdi_tx_enabled(struct drm_i915_private *i915, enum pipe pipe)
> +{
> +	assert_fdi_tx(i915, pipe, true);
> +}
> +
> +void assert_fdi_tx_disabled(struct drm_i915_private *i915, enum pipe pipe)
> +{
> +	assert_fdi_tx(i915, pipe, false);
> +}

For these wrappers I could argue that static inlines would be less
loc overall, while still wouldn't need any extra struct definitions/etc.
in the header. But not performance sensitive so from that pov static
inline is pointless.

Anyways, this approach seems fine to me. For the series
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Jani Nikula Sept. 30, 2021, 10:24 a.m. UTC | #2 
On Thu, 30 Sep 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Sep 30, 2021 at 12:22:58PM +0300, Jani Nikula wrote:
> <snip>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
>> index af01d1fa761e..02d3294bad7b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fdi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
>> @@ -10,6 +10,97 @@
>>  #include "intel_fdi.h"
>>  #include "intel_sideband.h"
>>  
>> +static void assert_fdi_tx(struct drm_i915_private *dev_priv,
>> +			  enum pipe pipe, bool state)
>> +{
>> +	bool cur_state;
>> +
>> +	if (HAS_DDI(dev_priv)) {
>> +		/*
>> +		 * DDI does not have a specific FDI_TX register.
>> +		 *
>> +		 * FDI is never fed from EDP transcoder
>> +		 * so pipe->transcoder cast is fine here.
>> +		 */
>> +		enum transcoder cpu_transcoder = (enum transcoder)pipe;
>> +		cur_state = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder)) & TRANS_DDI_FUNC_ENABLE;
>> +	} else {
>> +		cur_state = intel_de_read(dev_priv, FDI_TX_CTL(pipe)) & FDI_TX_ENABLE;
>> +	}
>> +	I915_STATE_WARN(cur_state != state,
>> +			"FDI TX state assertion failure (expected %s, current %s)\n",
>> +			onoff(state), onoff(cur_state));
>> +}
>> +
>> +void assert_fdi_tx_enabled(struct drm_i915_private *i915, enum pipe pipe)
>> +{
>> +	assert_fdi_tx(i915, pipe, true);
>> +}
>> +
>> +void assert_fdi_tx_disabled(struct drm_i915_private *i915, enum pipe pipe)
>> +{
>> +	assert_fdi_tx(i915, pipe, false);
>> +}
>
> For these wrappers I could argue that static inlines would be less
> loc overall, while still wouldn't need any extra struct definitions/etc.
> in the header. But not performance sensitive so from that pov static
> inline is pointless.

I didn't actually check the compiler output, but I think even
performance wise it'll probably end up being just one function call
either way. It's just a question which side of the call the logic
is. But agreed, doesn't really matter.

Anyway, the main argument I have for avoiding static inlines is to not
set an example to cargo cult from. They should be the exception, not the
rule. I think both the driver and the team have grown big enough to
require a style that promotes better structure. Because let's face it,
people look at what's there, copy the style, and not think of all the
subtleties.

> Anyways, this approach seems fine to me. For the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks,
Jani.
Jani Nikula Oct. 1, 2021, 8:49 a.m. UTC | #3 
On Thu, 30 Sep 2021, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 30 Sep 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Thu, Sep 30, 2021 at 12:22:58PM +0300, Jani Nikula wrote:
>> <snip>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
>>> index af01d1fa761e..02d3294bad7b 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fdi.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
>>> @@ -10,6 +10,97 @@
>>>  #include "intel_fdi.h"
>>>  #include "intel_sideband.h"
>>>  
>>> +static void assert_fdi_tx(struct drm_i915_private *dev_priv,
>>> +			  enum pipe pipe, bool state)
>>> +{
>>> +	bool cur_state;
>>> +
>>> +	if (HAS_DDI(dev_priv)) {
>>> +		/*
>>> +		 * DDI does not have a specific FDI_TX register.
>>> +		 *
>>> +		 * FDI is never fed from EDP transcoder
>>> +		 * so pipe->transcoder cast is fine here.
>>> +		 */
>>> +		enum transcoder cpu_transcoder = (enum transcoder)pipe;
>>> +		cur_state = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder)) & TRANS_DDI_FUNC_ENABLE;
>>> +	} else {
>>> +		cur_state = intel_de_read(dev_priv, FDI_TX_CTL(pipe)) & FDI_TX_ENABLE;
>>> +	}
>>> +	I915_STATE_WARN(cur_state != state,
>>> +			"FDI TX state assertion failure (expected %s, current %s)\n",
>>> +			onoff(state), onoff(cur_state));
>>> +}
>>> +
>>> +void assert_fdi_tx_enabled(struct drm_i915_private *i915, enum pipe pipe)
>>> +{
>>> +	assert_fdi_tx(i915, pipe, true);
>>> +}
>>> +
>>> +void assert_fdi_tx_disabled(struct drm_i915_private *i915, enum pipe pipe)
>>> +{
>>> +	assert_fdi_tx(i915, pipe, false);
>>> +}
>>
>> For these wrappers I could argue that static inlines would be less
>> loc overall, while still wouldn't need any extra struct definitions/etc.
>> in the header. But not performance sensitive so from that pov static
>> inline is pointless.
>
> I didn't actually check the compiler output, but I think even
> performance wise it'll probably end up being just one function call
> either way. It's just a question which side of the call the logic
> is. But agreed, doesn't really matter.
>
> Anyway, the main argument I have for avoiding static inlines is to not
> set an example to cargo cult from. They should be the exception, not the
> rule. I think both the driver and the team have grown big enough to
> require a style that promotes better structure. Because let's face it,
> people look at what's there, copy the style, and not think of all the
> subtleties.
>
>> Anyways, this approach seems fine to me. For the series
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Thanks,
> Jani.

And pushed.

BR,
Jani.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4ce80ca7751f..02400e5f518d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -428,78 +428,6 @@  void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state)
 			onoff(state), onoff(cur_state));
 }
 
-static void assert_fdi_tx(struct drm_i915_private *dev_priv,
-			  enum pipe pipe, bool state)
-{
-	bool cur_state;
-
-	if (HAS_DDI(dev_priv)) {
-		/*
-		 * DDI does not have a specific FDI_TX register.
-		 *
-		 * FDI is never fed from EDP transcoder
-		 * so pipe->transcoder cast is fine here.
-		 */
-		enum transcoder cpu_transcoder = (enum transcoder)pipe;
-		u32 val = intel_de_read(dev_priv,
-					TRANS_DDI_FUNC_CTL(cpu_transcoder));
-		cur_state = !!(val & TRANS_DDI_FUNC_ENABLE);
-	} else {
-		u32 val = intel_de_read(dev_priv, FDI_TX_CTL(pipe));
-		cur_state = !!(val & FDI_TX_ENABLE);
-	}
-	I915_STATE_WARN(cur_state != state,
-	     "FDI TX state assertion failure (expected %s, current %s)\n",
-			onoff(state), onoff(cur_state));
-}
-#define assert_fdi_tx_enabled(d, p) assert_fdi_tx(d, p, true)
-#define assert_fdi_tx_disabled(d, p) assert_fdi_tx(d, p, false)
-
-static void assert_fdi_rx(struct drm_i915_private *dev_priv,
-			  enum pipe pipe, bool state)
-{
-	u32 val;
-	bool cur_state;
-
-	val = intel_de_read(dev_priv, FDI_RX_CTL(pipe));
-	cur_state = !!(val & FDI_RX_ENABLE);
-	I915_STATE_WARN(cur_state != state,
-	     "FDI RX state assertion failure (expected %s, current %s)\n",
-			onoff(state), onoff(cur_state));
-}
-#define assert_fdi_rx_enabled(d, p) assert_fdi_rx(d, p, true)
-#define assert_fdi_rx_disabled(d, p) assert_fdi_rx(d, p, false)
-
-static void assert_fdi_tx_pll_enabled(struct drm_i915_private *dev_priv,
-				      enum pipe pipe)
-{
-	u32 val;
-
-	/* ILK FDI PLL is always enabled */
-	if (IS_IRONLAKE(dev_priv))
-		return;
-
-	/* On Haswell, DDI ports are responsible for the FDI PLL setup */
-	if (HAS_DDI(dev_priv))
-		return;
-
-	val = intel_de_read(dev_priv, FDI_TX_CTL(pipe));
-	I915_STATE_WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n");
-}
-
-void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
-		       enum pipe pipe, bool state)
-{
-	u32 val;
-	bool cur_state;
-
-	val = intel_de_read(dev_priv, FDI_RX_CTL(pipe));
-	cur_state = !!(val & FDI_RX_PLL_ENABLE);
-	I915_STATE_WARN(cur_state != state,
-	     "FDI RX PLL assertion failure (expected %s, current %s)\n",
-			onoff(state), onoff(cur_state));
-}
-
 void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
 	i915_reg_t pp_reg;
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 87b96fed5e0b..8cb3bb33cbe0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -654,10 +654,6 @@  void assert_pll(struct drm_i915_private *dev_priv,
 void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state);
 #define assert_dsi_pll_enabled(d) assert_dsi_pll(d, true)
 #define assert_dsi_pll_disabled(d) assert_dsi_pll(d, false)
-void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
-		       enum pipe pipe, bool state);
-#define assert_fdi_rx_pll_enabled(d, p) assert_fdi_rx_pll(d, p, true)
-#define assert_fdi_rx_pll_disabled(d, p) assert_fdi_rx_pll(d, p, false)
 void assert_transcoder(struct drm_i915_private *dev_priv,
 		       enum transcoder cpu_transcoder, bool state);
 #define assert_transcoder_enabled(d, t) assert_transcoder(d, t, true)
diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
index af01d1fa761e..02d3294bad7b 100644
--- a/drivers/gpu/drm/i915/display/intel_fdi.c
+++ b/drivers/gpu/drm/i915/display/intel_fdi.c
@@ -10,6 +10,97 @@ 
 #include "intel_fdi.h"
 #include "intel_sideband.h"
 
+static void assert_fdi_tx(struct drm_i915_private *dev_priv,
+			  enum pipe pipe, bool state)
+{
+	bool cur_state;
+
+	if (HAS_DDI(dev_priv)) {
+		/*
+		 * DDI does not have a specific FDI_TX register.
+		 *
+		 * FDI is never fed from EDP transcoder
+		 * so pipe->transcoder cast is fine here.
+		 */
+		enum transcoder cpu_transcoder = (enum transcoder)pipe;
+		cur_state = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder)) & TRANS_DDI_FUNC_ENABLE;
+	} else {
+		cur_state = intel_de_read(dev_priv, FDI_TX_CTL(pipe)) & FDI_TX_ENABLE;
+	}
+	I915_STATE_WARN(cur_state != state,
+			"FDI TX state assertion failure (expected %s, current %s)\n",
+			onoff(state), onoff(cur_state));
+}
+
+void assert_fdi_tx_enabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_tx(i915, pipe, true);
+}
+
+void assert_fdi_tx_disabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_tx(i915, pipe, false);
+}
+
+static void assert_fdi_rx(struct drm_i915_private *dev_priv,
+			  enum pipe pipe, bool state)
+{
+	bool cur_state;
+
+	cur_state = intel_de_read(dev_priv, FDI_RX_CTL(pipe)) & FDI_RX_ENABLE;
+	I915_STATE_WARN(cur_state != state,
+			"FDI RX state assertion failure (expected %s, current %s)\n",
+			onoff(state), onoff(cur_state));
+}
+
+void assert_fdi_rx_enabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_rx(i915, pipe, true);
+}
+
+void assert_fdi_rx_disabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_rx(i915, pipe, false);
+}
+
+void assert_fdi_tx_pll_enabled(struct drm_i915_private *i915,
+			       enum pipe pipe)
+{
+	bool cur_state;
+
+	/* ILK FDI PLL is always enabled */
+	if (IS_IRONLAKE(i915))
+		return;
+
+	/* On Haswell, DDI ports are responsible for the FDI PLL setup */
+	if (HAS_DDI(i915))
+		return;
+
+	cur_state = intel_de_read(i915, FDI_TX_CTL(pipe)) & FDI_TX_PLL_ENABLE;
+	I915_STATE_WARN(!cur_state, "FDI TX PLL assertion failure, should be active but is disabled\n");
+}
+
+static void assert_fdi_rx_pll(struct drm_i915_private *i915,
+			      enum pipe pipe, bool state)
+{
+	bool cur_state;
+
+	cur_state = intel_de_read(i915, FDI_RX_CTL(pipe)) & FDI_RX_PLL_ENABLE;
+	I915_STATE_WARN(cur_state != state,
+			"FDI RX PLL assertion failure (expected %s, current %s)\n",
+			onoff(state), onoff(cur_state));
+}
+
+void assert_fdi_rx_pll_enabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_rx_pll(i915, pipe, true);
+}
+
+void assert_fdi_rx_pll_disabled(struct drm_i915_private *i915, enum pipe pipe)
+{
+	assert_fdi_rx_pll(i915, pipe, false);
+}
+
 void intel_fdi_link_train(struct intel_crtc *crtc,
 			  const struct intel_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_fdi.h b/drivers/gpu/drm/i915/display/intel_fdi.h
index 61cb216a09f5..d557fc603d7d 100644
--- a/drivers/gpu/drm/i915/display/intel_fdi.h
+++ b/drivers/gpu/drm/i915/display/intel_fdi.h
@@ -6,6 +6,7 @@ 
 #ifndef _INTEL_FDI_H_
 #define _INTEL_FDI_H_
 
+enum pipe;
 struct drm_i915_private;
 struct intel_crtc;
 struct intel_crtc_state;
@@ -28,4 +29,13 @@  void lpt_fdi_program_mphy(struct drm_i915_private *i915);
 
 void intel_fdi_link_train(struct intel_crtc *crtc,
 			  const struct intel_crtc_state *crtc_state);
+
+void assert_fdi_tx_enabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_tx_disabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_rx_enabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_rx_disabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_tx_pll_enabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_rx_pll_enabled(struct drm_i915_private *i915, enum pipe pipe);
+void assert_fdi_rx_pll_disabled(struct drm_i915_private *i915, enum pipe pipe);
+
 #endif
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index e9fb402708a7..6520a8984651 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -44,6 +44,7 @@ 
 #include "intel_connector.h"
 #include "intel_de.h"
 #include "intel_display_types.h"
+#include "intel_fdi.h"
 #include "intel_gmbus.h"
 #include "intel_lvds.h"
 #include "intel_panel.h"