[Intel-gfx] [PATCH v4] drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence

[Intel-gfx] [PATCH v4] drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence

[Jani Nikula]

Starting from ICL, the default for MIPI GPIO sequences seems to be using
native GPIOs i.e. GPIOs available in the GPU. These native GPIOs reuse
many pins that quite frankly seem scary to poke based on the VBT
sequences. We pretty much have to trust that the board is configured
such that the relevant HPD, PP_CONTROL and GPIO bits aren't used for
anything else.

MIPI sequence v4 also adds a flag to fall back to non-native sequences.

v4:
- Wrap SHOTPLUG_CTL_DDI modification in spin_lock_irq() (Ville)

v3:
- Fix -Wbitwise-conditional-parentheses (kernel test robot <lkp@intel.com>)

v2:
- Fix HPD pin output set (impacts GPIOs 0 and 5)
- Fix GPIO data output direction set (impacts GPIOs 4 and 9)
- Reduce register accesses to single intel_de_rwm()

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6131
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 94 +++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h              |  1 +
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index fce69fa446d5..41f025f089d9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -41,9 +41,11 @@
 
 #include "i915_drv.h"
 #include "i915_reg.h"
+#include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dsi.h"
 #include "intel_dsi_vbt.h"
+#include "intel_gmbus_regs.h"
 #include "vlv_dsi.h"
 #include "vlv_dsi_regs.h"
 #include "vlv_sideband.h"
@@ -377,6 +379,85 @@ static void icl_exec_gpio(struct intel_connector *connector,
 	drm_dbg_kms(&dev_priv->drm, "Skipping ICL GPIO element execution\n");
 }
 
+enum {
+	MIPI_RESET_1 = 0,
+	MIPI_AVDD_EN_1,
+	MIPI_BKLT_EN_1,
+	MIPI_AVEE_EN_1,
+	MIPI_VIO_EN_1,
+	MIPI_RESET_2,
+	MIPI_AVDD_EN_2,
+	MIPI_BKLT_EN_2,
+	MIPI_AVEE_EN_2,
+	MIPI_VIO_EN_2,
+};
+
+static void icl_native_gpio_set_value(struct drm_i915_private *dev_priv,
+				      int gpio, bool value)
+{
+	int index;
+
+	if (drm_WARN_ON(&dev_priv->drm, DISPLAY_VER(dev_priv) == 11 && gpio >= MIPI_RESET_2))
+		return;
+
+	switch (gpio) {
+	case MIPI_RESET_1:
+	case MIPI_RESET_2:
+		index = gpio == MIPI_RESET_1 ? HPD_PORT_A : HPD_PORT_B;
+
+		/*
+		 * Disable HPD to set the pin to output, and set output
+		 * value. The HPD pin should not be enabled for DSI anyway,
+		 * assuming the board design and VBT are sane, and the pin isn't
+		 * used by a non-DSI encoder.
+		 *
+		 * The locking protects against concurrent SHOTPLUG_CTL_DDI
+		 * modifications in irq setup and handling.
+		 */
+		spin_lock_irq(&dev_priv->irq_lock);
+		intel_de_rmw(dev_priv, SHOTPLUG_CTL_DDI,
+			     SHOTPLUG_CTL_DDI_HPD_ENABLE(index) |
+			     SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index),
+			     value ? SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index) : 0);
+		spin_unlock_irq(&dev_priv->irq_lock);
+		break;
+	case MIPI_AVDD_EN_1:
+	case MIPI_AVDD_EN_2:
+		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
+
+		intel_de_rmw(dev_priv, PP_CONTROL(index), PANEL_POWER_ON,
+			     value ? PANEL_POWER_ON : 0);
+		break;
+	case MIPI_BKLT_EN_1:
+	case MIPI_BKLT_EN_2:
+		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
+
+		intel_de_rmw(dev_priv, PP_CONTROL(index), EDP_BLC_ENABLE,
+			     value ? EDP_BLC_ENABLE : 0);
+		break;
+	case MIPI_AVEE_EN_1:
+	case MIPI_AVEE_EN_2:
+		index = gpio == MIPI_AVEE_EN_1 ? 1 : 2;
+
+		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
+			     GPIO_CLOCK_VAL_OUT,
+			     GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_DIR_OUT |
+			     GPIO_CLOCK_VAL_MASK | (value ? GPIO_CLOCK_VAL_OUT : 0));
+		break;
+	case MIPI_VIO_EN_1:
+	case MIPI_VIO_EN_2:
+		index = gpio == MIPI_VIO_EN_1 ? 1 : 2;
+
+		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
+			     GPIO_DATA_VAL_OUT,
+			     GPIO_DATA_DIR_MASK | GPIO_DATA_DIR_OUT |
+			     GPIO_DATA_VAL_MASK | (value ? GPIO_DATA_VAL_OUT : 0));
+		break;
+	default:
+		MISSING_CASE(gpio);
+	}
+}
+
 static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 {
 	struct drm_device *dev = intel_dsi->base.base.dev;
@@ -384,8 +465,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 	struct intel_connector *connector = intel_dsi->attached_connector;
 	u8 gpio_source, gpio_index = 0, gpio_number;
 	bool value;
-
-	drm_dbg_kms(&dev_priv->drm, "\n");
+	bool native = DISPLAY_VER(dev_priv) >= 11;
 
 	if (connector->panel.vbt.dsi.seq_version >= 3)
 		gpio_index = *data++;
@@ -398,10 +478,18 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 	else
 		gpio_source = 0;
 
+	if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
+		native = false;
+
 	/* pull up/down */
 	value = *data++ & 1;
 
-	if (DISPLAY_VER(dev_priv) >= 11)
+	drm_dbg_kms(&dev_priv->drm, "GPIO index %u, number %u, source %u, native %s, set to %s\n",
+		    gpio_index, gpio_number, gpio_source, str_yes_no(native), str_on_off(value));
+
+	if (native)
+		icl_native_gpio_set_value(dev_priv, gpio_number, value);
+	else if (DISPLAY_VER(dev_priv) >= 11)
 		icl_exec_gpio(connector, gpio_source, gpio_index, value);
 	else if (IS_VALLEYVIEW(dev_priv))
 		vlv_exec_gpio(connector, gpio_source, gpio_number, value);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cef9418beec0..8b2cf980f323 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5966,6 +5966,7 @@
 
 #define SHOTPLUG_CTL_DDI				_MMIO(0xc4030)
 #define   SHOTPLUG_CTL_DDI_HPD_ENABLE(hpd_pin)			(0x8 << (_HPD_PIN_DDI(hpd_pin) * 4))
+#define   SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(hpd_pin)		(0x4 << (_HPD_PIN_DDI(hpd_pin) * 4))
 #define   SHOTPLUG_CTL_DDI_HPD_STATUS_MASK(hpd_pin)		(0x3 << (_HPD_PIN_DDI(hpd_pin) * 4))
 #define   SHOTPLUG_CTL_DDI_HPD_NO_DETECT(hpd_pin)		(0x0 << (_HPD_PIN_DDI(hpd_pin) * 4))
 #define   SHOTPLUG_CTL_DDI_HPD_SHORT_DETECT(hpd_pin)		(0x1 << (_HPD_PIN_DDI(hpd_pin) * 4))
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence (rev4)
URL   : https://patchwork.freedesktop.org/series/111850/
State : warning

== Summary ==

Error: make htmldocs had i915 warnings
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Excess function parameter 'crtc_state' description in 'intel_dsb_reg_write'
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Function parameter or member 'dsb' not described in 'intel_dsb_reg_write'
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Excess function parameter 'crtc_state' description in 'intel_dsb_reg_write'

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

== Series Details ==

Series: drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence (rev4)
URL   : https://patchwork.freedesktop.org/series/111850/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12501 -> Patchwork_111850v4
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_111850v4 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_111850v4, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/index.html

Participating hosts (38 -> 19)
------------------------------

  ERROR: It appears as if the changes made in Patchwork_111850v4 prevented too many machines from booting.

  Additional (1): fi-skl-guc 
  Missing    (20): fi-kbl-soraka bat-dg1-6 bat-dg1-5 bat-adlp-6 fi-skl-6600u fi-bsw-n3050 bat-dg2-8 bat-adlm-1 bat-dg2-9 fi-bwr-2160 bat-adln-1 bat-atsm-1 bat-jsl-3 bat-rplp-1 bat-dg2-11 fi-bsw-nick bat-dg1-7 bat-kbl-2 bat-adlp-9 bat-adlp-4 

Known issues
------------

  Here are the changes found in Patchwork_111850v4 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@basic:
    - fi-skl-guc:         NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#4613]) +3 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-skl-guc/igt@gem_lmem_swapping@basic.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [PASS][2] -> [INCOMPLETE][3] ([i915#4817])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12501/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-skl-guc:         NOTRUN -> [SKIP][4] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-skl-guc/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [PASS][5] -> [FAIL][6] ([i915#6298])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12501/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-skl-guc:         NOTRUN -> [SKIP][7] ([fdo#109271]) +9 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-skl-guc/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [FAIL][8] ([i915#7229]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12501/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229


Build changes
-------------

  * Linux: CI_DRM_12501 -> Patchwork_111850v4

  CI-20190529: 20190529
  CI_DRM_12501: 1b38b5a419ab3d838b6ac95d22f1fe057fc8889d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7091: b8015f920c9f469d3733854263cb878373c1df51 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_111850v4: 1b38b5a419ab3d838b6ac95d22f1fe057fc8889d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

7de3009ac12d drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111850v4/index.html

[-- Attachment #2: Type: text/html, Size: 5165 bytes --]

Re: [Intel-gfx] [PATCH v4] drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence

[Ville Syrjälä]

On Tue, Dec 13, 2022 at 11:59:24AM +0200, Jani Nikula wrote:
> Starting from ICL, the default for MIPI GPIO sequences seems to be using
> native GPIOs i.e. GPIOs available in the GPU. These native GPIOs reuse
> many pins that quite frankly seem scary to poke based on the VBT
> sequences. We pretty much have to trust that the board is configured
> such that the relevant HPD, PP_CONTROL and GPIO bits aren't used for
> anything else.
> 
> MIPI sequence v4 also adds a flag to fall back to non-native sequences.
> 
> v4:
> - Wrap SHOTPLUG_CTL_DDI modification in spin_lock_irq() (Ville)
> 
> v3:
> - Fix -Wbitwise-conditional-parentheses (kernel test robot <lkp@intel.com>)
> 
> v2:
> - Fix HPD pin output set (impacts GPIOs 0 and 5)
> - Fix GPIO data output direction set (impacts GPIOs 4 and 9)
> - Reduce register accesses to single intel_de_rwm()
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6131
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 94 +++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index fce69fa446d5..41f025f089d9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -41,9 +41,11 @@
>  
>  #include "i915_drv.h"
>  #include "i915_reg.h"
> +#include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_dsi.h"
>  #include "intel_dsi_vbt.h"
> +#include "intel_gmbus_regs.h"
>  #include "vlv_dsi.h"
>  #include "vlv_dsi_regs.h"
>  #include "vlv_sideband.h"
> @@ -377,6 +379,85 @@ static void icl_exec_gpio(struct intel_connector *connector,
>  	drm_dbg_kms(&dev_priv->drm, "Skipping ICL GPIO element execution\n");
>  }
>  
> +enum {
> +	MIPI_RESET_1 = 0,
> +	MIPI_AVDD_EN_1,
> +	MIPI_BKLT_EN_1,
> +	MIPI_AVEE_EN_1,
> +	MIPI_VIO_EN_1,
> +	MIPI_RESET_2,
> +	MIPI_AVDD_EN_2,
> +	MIPI_BKLT_EN_2,
> +	MIPI_AVEE_EN_2,
> +	MIPI_VIO_EN_2,
> +};
> +
> +static void icl_native_gpio_set_value(struct drm_i915_private *dev_priv,
> +				      int gpio, bool value)
> +{
> +	int index;
> +
> +	if (drm_WARN_ON(&dev_priv->drm, DISPLAY_VER(dev_priv) == 11 && gpio >= MIPI_RESET_2))
> +		return;
> +
> +	switch (gpio) {
> +	case MIPI_RESET_1:
> +	case MIPI_RESET_2:
> +		index = gpio == MIPI_RESET_1 ? HPD_PORT_A : HPD_PORT_B;
> +
> +		/*
> +		 * Disable HPD to set the pin to output, and set output
> +		 * value. The HPD pin should not be enabled for DSI anyway,
> +		 * assuming the board design and VBT are sane, and the pin isn't
> +		 * used by a non-DSI encoder.
> +		 *
> +		 * The locking protects against concurrent SHOTPLUG_CTL_DDI
> +		 * modifications in irq setup and handling.
> +		 */

The rmw in icp_irq_handler() doesn't seem to have the required locking.

> +		spin_lock_irq(&dev_priv->irq_lock);
> +		intel_de_rmw(dev_priv, SHOTPLUG_CTL_DDI,
> +			     SHOTPLUG_CTL_DDI_HPD_ENABLE(index) |
> +			     SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index),
> +			     value ? SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index) : 0);
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		break;
> +	case MIPI_AVDD_EN_1:
> +	case MIPI_AVDD_EN_2:
> +		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
> +
> +		intel_de_rmw(dev_priv, PP_CONTROL(index), PANEL_POWER_ON,
> +			     value ? PANEL_POWER_ON : 0);
> +		break;
> +	case MIPI_BKLT_EN_1:
> +	case MIPI_BKLT_EN_2:
> +		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
> +
> +		intel_de_rmw(dev_priv, PP_CONTROL(index), EDP_BLC_ENABLE,
> +			     value ? EDP_BLC_ENABLE : 0);
> +		break;
> +	case MIPI_AVEE_EN_1:
> +	case MIPI_AVEE_EN_2:
> +		index = gpio == MIPI_AVEE_EN_1 ? 1 : 2;
> +
> +		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
> +			     GPIO_CLOCK_VAL_OUT,
> +			     GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_DIR_OUT |
> +			     GPIO_CLOCK_VAL_MASK | (value ? GPIO_CLOCK_VAL_OUT : 0));
> +		break;
> +	case MIPI_VIO_EN_1:
> +	case MIPI_VIO_EN_2:
> +		index = gpio == MIPI_VIO_EN_1 ? 1 : 2;
> +
> +		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
> +			     GPIO_DATA_VAL_OUT,
> +			     GPIO_DATA_DIR_MASK | GPIO_DATA_DIR_OUT |
> +			     GPIO_DATA_VAL_MASK | (value ? GPIO_DATA_VAL_OUT : 0));
> +		break;
> +	default:
> +		MISSING_CASE(gpio);
> +	}
> +}
> +
>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  {
>  	struct drm_device *dev = intel_dsi->base.base.dev;
> @@ -384,8 +465,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	struct intel_connector *connector = intel_dsi->attached_connector;
>  	u8 gpio_source, gpio_index = 0, gpio_number;
>  	bool value;
> -
> -	drm_dbg_kms(&dev_priv->drm, "\n");
> +	bool native = DISPLAY_VER(dev_priv) >= 11;
>  
>  	if (connector->panel.vbt.dsi.seq_version >= 3)
>  		gpio_index = *data++;
> @@ -398,10 +478,18 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	else
>  		gpio_source = 0;
>  
> +	if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
> +		native = false;
> +
>  	/* pull up/down */
>  	value = *data++ & 1;
>  
> -	if (DISPLAY_VER(dev_priv) >= 11)
> +	drm_dbg_kms(&dev_priv->drm, "GPIO index %u, number %u, source %u, native %s, set to %s\n",
> +		    gpio_index, gpio_number, gpio_source, str_yes_no(native), str_on_off(value));
> +
> +	if (native)
> +		icl_native_gpio_set_value(dev_priv, gpio_number, value);
> +	else if (DISPLAY_VER(dev_priv) >= 11)
>  		icl_exec_gpio(connector, gpio_source, gpio_index, value);
>  	else if (IS_VALLEYVIEW(dev_priv))
>  		vlv_exec_gpio(connector, gpio_source, gpio_number, value);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cef9418beec0..8b2cf980f323 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5966,6 +5966,7 @@
>  
>  #define SHOTPLUG_CTL_DDI				_MMIO(0xc4030)
>  #define   SHOTPLUG_CTL_DDI_HPD_ENABLE(hpd_pin)			(0x8 << (_HPD_PIN_DDI(hpd_pin) * 4))
> +#define   SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(hpd_pin)		(0x4 << (_HPD_PIN_DDI(hpd_pin) * 4))
>  #define   SHOTPLUG_CTL_DDI_HPD_STATUS_MASK(hpd_pin)		(0x3 << (_HPD_PIN_DDI(hpd_pin) * 4))
>  #define   SHOTPLUG_CTL_DDI_HPD_NO_DETECT(hpd_pin)		(0x0 << (_HPD_PIN_DDI(hpd_pin) * 4))
>  #define   SHOTPLUG_CTL_DDI_HPD_SHORT_DETECT(hpd_pin)		(0x1 << (_HPD_PIN_DDI(hpd_pin) * 4))
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v4] drm/i915/dsi: add support for ICL+ native MIPI GPIO sequence

[Jani Nikula]

On Wed, 14 Dec 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Dec 13, 2022 at 11:59:24AM +0200, Jani Nikula wrote:
>> Starting from ICL, the default for MIPI GPIO sequences seems to be using
>> native GPIOs i.e. GPIOs available in the GPU. These native GPIOs reuse
>> many pins that quite frankly seem scary to poke based on the VBT
>> sequences. We pretty much have to trust that the board is configured
>> such that the relevant HPD, PP_CONTROL and GPIO bits aren't used for
>> anything else.
>> 
>> MIPI sequence v4 also adds a flag to fall back to non-native sequences.
>> 
>> v4:
>> - Wrap SHOTPLUG_CTL_DDI modification in spin_lock_irq() (Ville)
>> 
>> v3:
>> - Fix -Wbitwise-conditional-parentheses (kernel test robot <lkp@intel.com>)
>> 
>> v2:
>> - Fix HPD pin output set (impacts GPIOs 0 and 5)
>> - Fix GPIO data output direction set (impacts GPIOs 4 and 9)
>> - Reduce register accesses to single intel_de_rwm()
>> 
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6131
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 94 +++++++++++++++++++-
>>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>>  2 files changed, 92 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> index fce69fa446d5..41f025f089d9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -41,9 +41,11 @@
>>  
>>  #include "i915_drv.h"
>>  #include "i915_reg.h"
>> +#include "intel_de.h"
>>  #include "intel_display_types.h"
>>  #include "intel_dsi.h"
>>  #include "intel_dsi_vbt.h"
>> +#include "intel_gmbus_regs.h"
>>  #include "vlv_dsi.h"
>>  #include "vlv_dsi_regs.h"
>>  #include "vlv_sideband.h"
>> @@ -377,6 +379,85 @@ static void icl_exec_gpio(struct intel_connector *connector,
>>  	drm_dbg_kms(&dev_priv->drm, "Skipping ICL GPIO element execution\n");
>>  }
>>  
>> +enum {
>> +	MIPI_RESET_1 = 0,
>> +	MIPI_AVDD_EN_1,
>> +	MIPI_BKLT_EN_1,
>> +	MIPI_AVEE_EN_1,
>> +	MIPI_VIO_EN_1,
>> +	MIPI_RESET_2,
>> +	MIPI_AVDD_EN_2,
>> +	MIPI_BKLT_EN_2,
>> +	MIPI_AVEE_EN_2,
>> +	MIPI_VIO_EN_2,
>> +};
>> +
>> +static void icl_native_gpio_set_value(struct drm_i915_private *dev_priv,
>> +				      int gpio, bool value)
>> +{
>> +	int index;
>> +
>> +	if (drm_WARN_ON(&dev_priv->drm, DISPLAY_VER(dev_priv) == 11 && gpio >= MIPI_RESET_2))
>> +		return;
>> +
>> +	switch (gpio) {
>> +	case MIPI_RESET_1:
>> +	case MIPI_RESET_2:
>> +		index = gpio == MIPI_RESET_1 ? HPD_PORT_A : HPD_PORT_B;
>> +
>> +		/*
>> +		 * Disable HPD to set the pin to output, and set output
>> +		 * value. The HPD pin should not be enabled for DSI anyway,
>> +		 * assuming the board design and VBT are sane, and the pin isn't
>> +		 * used by a non-DSI encoder.
>> +		 *
>> +		 * The locking protects against concurrent SHOTPLUG_CTL_DDI
>> +		 * modifications in irq setup and handling.
>> +		 */
>
> The rmw in icp_irq_handler() doesn't seem to have the required locking.

Drat! I seem to have thought that's not necessary in the irq handler for
some reason.

spin_lock() there enough?

I'm wondering if the hpd stuff should be switched to a separate spin
lock. The irq lock seems like a too big hammer for this. But that's for
another series, another time.

BR,
Jani.


>
>> +		spin_lock_irq(&dev_priv->irq_lock);
>> +		intel_de_rmw(dev_priv, SHOTPLUG_CTL_DDI,
>> +			     SHOTPLUG_CTL_DDI_HPD_ENABLE(index) |
>> +			     SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index),
>> +			     value ? SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(index) : 0);
>> +		spin_unlock_irq(&dev_priv->irq_lock);
>> +		break;
>> +	case MIPI_AVDD_EN_1:
>> +	case MIPI_AVDD_EN_2:
>> +		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
>> +
>> +		intel_de_rmw(dev_priv, PP_CONTROL(index), PANEL_POWER_ON,
>> +			     value ? PANEL_POWER_ON : 0);
>> +		break;
>> +	case MIPI_BKLT_EN_1:
>> +	case MIPI_BKLT_EN_2:
>> +		index = gpio == MIPI_AVDD_EN_1 ? 0 : 1;
>> +
>> +		intel_de_rmw(dev_priv, PP_CONTROL(index), EDP_BLC_ENABLE,
>> +			     value ? EDP_BLC_ENABLE : 0);
>> +		break;
>> +	case MIPI_AVEE_EN_1:
>> +	case MIPI_AVEE_EN_2:
>> +		index = gpio == MIPI_AVEE_EN_1 ? 1 : 2;
>> +
>> +		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
>> +			     GPIO_CLOCK_VAL_OUT,
>> +			     GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_DIR_OUT |
>> +			     GPIO_CLOCK_VAL_MASK | (value ? GPIO_CLOCK_VAL_OUT : 0));
>> +		break;
>> +	case MIPI_VIO_EN_1:
>> +	case MIPI_VIO_EN_2:
>> +		index = gpio == MIPI_VIO_EN_1 ? 1 : 2;
>> +
>> +		intel_de_rmw(dev_priv, GPIO(dev_priv, index),
>> +			     GPIO_DATA_VAL_OUT,
>> +			     GPIO_DATA_DIR_MASK | GPIO_DATA_DIR_OUT |
>> +			     GPIO_DATA_VAL_MASK | (value ? GPIO_DATA_VAL_OUT : 0));
>> +		break;
>> +	default:
>> +		MISSING_CASE(gpio);
>> +	}
>> +}
>> +
>>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  {
>>  	struct drm_device *dev = intel_dsi->base.base.dev;
>> @@ -384,8 +465,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  	struct intel_connector *connector = intel_dsi->attached_connector;
>>  	u8 gpio_source, gpio_index = 0, gpio_number;
>>  	bool value;
>> -
>> -	drm_dbg_kms(&dev_priv->drm, "\n");
>> +	bool native = DISPLAY_VER(dev_priv) >= 11;
>>  
>>  	if (connector->panel.vbt.dsi.seq_version >= 3)
>>  		gpio_index = *data++;
>> @@ -398,10 +478,18 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>>  	else
>>  		gpio_source = 0;
>>  
>> +	if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
>> +		native = false;
>> +
>>  	/* pull up/down */
>>  	value = *data++ & 1;
>>  
>> -	if (DISPLAY_VER(dev_priv) >= 11)
>> +	drm_dbg_kms(&dev_priv->drm, "GPIO index %u, number %u, source %u, native %s, set to %s\n",
>> +		    gpio_index, gpio_number, gpio_source, str_yes_no(native), str_on_off(value));
>> +
>> +	if (native)
>> +		icl_native_gpio_set_value(dev_priv, gpio_number, value);
>> +	else if (DISPLAY_VER(dev_priv) >= 11)
>>  		icl_exec_gpio(connector, gpio_source, gpio_index, value);
>>  	else if (IS_VALLEYVIEW(dev_priv))
>>  		vlv_exec_gpio(connector, gpio_source, gpio_number, value);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index cef9418beec0..8b2cf980f323 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5966,6 +5966,7 @@
>>  
>>  #define SHOTPLUG_CTL_DDI				_MMIO(0xc4030)
>>  #define   SHOTPLUG_CTL_DDI_HPD_ENABLE(hpd_pin)			(0x8 << (_HPD_PIN_DDI(hpd_pin) * 4))
>> +#define   SHOTPLUG_CTL_DDI_HPD_OUTPUT_DATA(hpd_pin)		(0x4 << (_HPD_PIN_DDI(hpd_pin) * 4))
>>  #define   SHOTPLUG_CTL_DDI_HPD_STATUS_MASK(hpd_pin)		(0x3 << (_HPD_PIN_DDI(hpd_pin) * 4))
>>  #define   SHOTPLUG_CTL_DDI_HPD_NO_DETECT(hpd_pin)		(0x0 << (_HPD_PIN_DDI(hpd_pin) * 4))
>>  #define   SHOTPLUG_CTL_DDI_HPD_SHORT_DETECT(hpd_pin)		(0x1 << (_HPD_PIN_DDI(hpd_pin) * 4))
>> -- 
>> 2.34.1

-- 
Jani Nikula, Intel Open Source Graphics Center