[Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Manasi Navare]

Modify the helper to add a fixed delay or poll with timeout
based on platform specification to check for either Idle bit
set (DDI_BUF_CTL is idle for disable case)

v3:
* Change the timeout to 16usecs (Ville)
v2:
* Use 2 separate functions or idle and active (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 884b507c5f55..052a74625a61 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
 static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
 				    enum port port)
 {
-	i915_reg_t reg = DDI_BUF_CTL(port);
-	int i;
-
-	for (i = 0; i < 16; i++) {
-		udelay(1);
-		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
-			return;
+	if (IS_BROXTON(dev_priv)) {
+		udelay(16);
+		return;
 	}
-	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
-		port_name(port));
+
+	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
+			 DDI_BUF_IS_IDLE), 16))
+		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
+			port_name(port));
 }
 
 static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 2/2] drm/i915/dp: Helper to check for DDI BUF status to get active

[Manasi Navare]

Based on the platform, Bspec expects us to wait or poll with
timeout for DDI BUF IDLE bit to be set to 0 (non idle) or get active
after enabling DDI_BUF_CTL.

v4:
* Use the timeout for GLK (Ville)
v3:
* Add a new function _active for DDI BUF CTL to be non idle (Ville)
v2:
* Based on platform, fixed delay or poll (Ville)
* Use a helper to do this (Imre, Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 052a74625a61..94d57b57139b 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1195,6 +1195,20 @@ static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
 			port_name(port));
 }
 
+static void intel_wait_ddi_buf_active(struct drm_i915_private *dev_priv,
+				      enum port port)
+{
+	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv) ) {
+		usleep_range(600, 1000);
+		return;
+	}
+
+	if (wait_for_us(!(intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
+			  DDI_BUF_IS_IDLE), 600))
+		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get active\n",
+			port_name(port));
+}
+
 static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
 {
 	switch (pll->info->id) {
@@ -4020,7 +4034,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 	intel_de_write(dev_priv, DDI_BUF_CTL(port), intel_dp->DP);
 	intel_de_posting_read(dev_priv, DDI_BUF_CTL(port));
 
-	udelay(600);
+	intel_wait_ddi_buf_active(dev_priv, port);
 }
 
 static void intel_ddi_set_link_train(struct intel_dp *intel_dp,
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v4,1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status
URL   : https://patchwork.freedesktop.org/series/78867/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e41b98b71f2b drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status
-:38: CHECK:USLEEP_RANGE: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
#38: FILE: drivers/gpu/drm/i915/display/intel_ddi.c:1188:
+		udelay(16);

total: 0 errors, 0 warnings, 1 checks, 24 lines checked
8d63e562ec8b drm/i915/dp: Helper to check for DDI BUF status to get active
-:36: ERROR:SPACING: space prohibited before that close parenthesis ')'
#36: FILE: drivers/gpu/drm/i915/display/intel_ddi.c:1201:
+	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv) ) {

total: 1 errors, 0 warnings, 0 checks, 28 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v4,1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status
URL   : https://patchwork.freedesktop.org/series/78867/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8668 -> Patchwork_18032
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-apl-guc:         [PASS][1] -> [INCOMPLETE][2] ([i915#1242])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-apl-guc/igt@gem_exec_suspend@basic-s0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-apl-guc/igt@gem_exec_suspend@basic-s0.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-byt-j1900/igt@i915_pm_rpm@basic-pci-d3-state.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-byt-j1900/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@module-reload:
    - fi-glk-dsi:         [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-icl-u2:          [PASS][7] -> [DMESG-WARN][8] ([i915#1982])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - fi-tgl-u2:          [PASS][9] -> [DMESG-WARN][10] ([i915#402])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-bsw-nick:        [INCOMPLETE][11] ([i915#1250] / [i915#1436]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-bsw-nick/igt@debugfs_test@read_all_entries.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-bsw-nick/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-tgl-u2:          [FAIL][13] ([i915#1888]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_module_load@reload:
    - fi-bxt-dsi:         [DMESG-WARN][15] ([i915#1982]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-bxt-dsi/igt@i915_module_load@reload.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-bxt-dsi/igt@i915_module_load@reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - {fi-kbl-7560u}:     [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1:
    - fi-icl-u2:          [DMESG-WARN][19] ([i915#1982]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html

  
#### Warnings ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-kbl-x1275:       [DMESG-WARN][21] ([i915#62] / [i915#92]) -> [DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_force_connector_basic@force-edid:
    - fi-kbl-x1275:       [DMESG-WARN][23] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][24] ([i915#62] / [i915#92]) +4 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8668/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1242]: https://gitlab.freedesktop.org/drm/intel/issues/1242
  [i915#1250]: https://gitlab.freedesktop.org/drm/intel/issues/1250
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (45 -> 39)
------------------------------

  Additional (1): fi-tgl-y 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_8668 -> Patchwork_18032

  CI-20190529: 20190529
  CI_DRM_8668: ebcb5923cc316fea9d46629cce83960511da889e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5718: af1ef32bfae90bcdbaf1b5d84c61ff4e04368505 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18032: 8d63e562ec8b0cecedc4569a51b0860494a1d006 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8d63e562ec8b drm/i915/dp: Helper to check for DDI BUF status to get active
e41b98b71f2b drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18032/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Manasi Navare]

@Ville @Imre, addressed your review comments, could you take a look?

Manasi

On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> Modify the helper to add a fixed delay or poll with timeout
> based on platform specification to check for either Idle bit
> set (DDI_BUF_CTL is idle for disable case)
> 
> v3:
> * Change the timeout to 16usecs (Ville)
> v2:
> * Use 2 separate functions or idle and active (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 884b507c5f55..052a74625a61 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
>  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>  				    enum port port)
>  {
> -	i915_reg_t reg = DDI_BUF_CTL(port);
> -	int i;
> -
> -	for (i = 0; i < 16; i++) {
> -		udelay(1);
> -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> -			return;
> +	if (IS_BROXTON(dev_priv)) {
> +		udelay(16);
> +		return;
>  	}
> -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> -		port_name(port));
> +
> +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> +			 DDI_BUF_IS_IDLE), 16))
> +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> +			port_name(port));
>  }
>  
>  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/2] drm/i915/dp: Helper to check for DDI BUF status to get active

[Manasi Navare]

@Ville, @Imre : addressed your review comments, could you please take a look?

Regards
Manasi

On Fri, Jun 26, 2020 at 04:26:41PM -0700, Manasi Navare wrote:
> Based on the platform, Bspec expects us to wait or poll with
> timeout for DDI BUF IDLE bit to be set to 0 (non idle) or get active
> after enabling DDI_BUF_CTL.
> 
> v4:
> * Use the timeout for GLK (Ville)
> v3:
> * Add a new function _active for DDI BUF CTL to be non idle (Ville)
> v2:
> * Based on platform, fixed delay or poll (Ville)
> * Use a helper to do this (Imre, Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 052a74625a61..94d57b57139b 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1195,6 +1195,20 @@ static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>  			port_name(port));
>  }
>  
> +static void intel_wait_ddi_buf_active(struct drm_i915_private *dev_priv,
> +				      enum port port)
> +{
> +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv) ) {
> +		usleep_range(600, 1000);
> +		return;
> +	}
> +
> +	if (wait_for_us(!(intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> +			  DDI_BUF_IS_IDLE), 600))
> +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get active\n",
> +			port_name(port));
> +}
> +
>  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
>  {
>  	switch (pll->info->id) {
> @@ -4020,7 +4034,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  	intel_de_write(dev_priv, DDI_BUF_CTL(port), intel_dp->DP);
>  	intel_de_posting_read(dev_priv, DDI_BUF_CTL(port));
>  
> -	udelay(600);
> +	intel_wait_ddi_buf_active(dev_priv, port);
>  }
>  
>  static void intel_ddi_set_link_train(struct intel_dp *intel_dp,
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> Modify the helper to add a fixed delay or poll with timeout
> based on platform specification to check for either Idle bit
> set (DDI_BUF_CTL is idle for disable case)
> 
> v3:
> * Change the timeout to 16usecs (Ville)
> v2:
> * Use 2 separate functions or idle and active (Ville)

Missing changelog? Did somehting change?

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 884b507c5f55..052a74625a61 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
>  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>  				    enum port port)
>  {
> -	i915_reg_t reg = DDI_BUF_CTL(port);
> -	int i;
> -
> -	for (i = 0; i < 16; i++) {
> -		udelay(1);
> -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> -			return;
> +	if (IS_BROXTON(dev_priv)) {
> +		udelay(16);
> +		return;
>  	}
> -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> -		port_name(port));
> +
> +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> +			 DDI_BUF_IS_IDLE), 16))
> +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> +			port_name(port));
>  }
>  
>  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Manasi Navare]

On Tue, Jun 30, 2020 at 06:30:16PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> > Modify the helper to add a fixed delay or poll with timeout
> > based on platform specification to check for either Idle bit
> > set (DDI_BUF_CTL is idle for disable case)
> > 
> > v3:
> > * Change the timeout to 16usecs (Ville)
> > v2:
> > * Use 2 separate functions or idle and active (Ville)
> 
> Missing changelog? Did somehting change?
>

No its a v3 for thsi patch, but patch 2/2 is a v4 so published
both patches with a v4,

does this now look good with v3, timeout changed to 16 usecs?

Manasi
 
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 884b507c5f55..052a74625a61 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> >  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> >  				    enum port port)
> >  {
> > -	i915_reg_t reg = DDI_BUF_CTL(port);
> > -	int i;
> > -
> > -	for (i = 0; i < 16; i++) {
> > -		udelay(1);
> > -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> > -			return;
> > +	if (IS_BROXTON(dev_priv)) {
> > +		udelay(16);
> > +		return;
> >  	}
> > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > -		port_name(port));
> > +
> > +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > +			 DDI_BUF_IS_IDLE), 16))
> > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> > +			port_name(port));
> >  }
> >  
> >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Ville Syrjälä]

On Tue, Jun 30, 2020 at 01:49:22PM -0700, Manasi Navare wrote:
> On Tue, Jun 30, 2020 at 06:30:16PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> > > Modify the helper to add a fixed delay or poll with timeout
> > > based on platform specification to check for either Idle bit
> > > set (DDI_BUF_CTL is idle for disable case)
> > > 
> > > v3:
> > > * Change the timeout to 16usecs (Ville)
> > > v2:
> > > * Use 2 separate functions or idle and active (Ville)
> > 
> > Missing changelog? Did somehting change?
> >
> 
> No its a v3 for thsi patch, but patch 2/2 is a v4 so published
> both patches with a v4,

Doh. The changelog is backwards so didn't even notice.

> 
> does this now look good with v3, timeout changed to 16 usecs?
> 
> Manasi
>  
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
> > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 884b507c5f55..052a74625a61 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> > >  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> > >  				    enum port port)
> > >  {
> > > -	i915_reg_t reg = DDI_BUF_CTL(port);
> > > -	int i;
> > > -
> > > -	for (i = 0; i < 16; i++) {
> > > -		udelay(1);
> > > -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> > > -			return;
> > > +	if (IS_BROXTON(dev_priv)) {
> > > +		udelay(16);
> > > +		return;
> > >  	}
> > > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > > -		port_name(port));
> > > +
> > > +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > > +			 DDI_BUF_IS_IDLE), 16))
> > > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> > > +			port_name(port));
> > >  }
> > >  
> > >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> Modify the helper to add a fixed delay or poll with timeout
> based on platform specification to check for either Idle bit
> set (DDI_BUF_CTL is idle for disable case)
> 
> v3:
> * Change the timeout to 16usecs (Ville)
> v2:
> * Use 2 separate functions or idle and active (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 884b507c5f55..052a74625a61 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
>  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>  				    enum port port)
>  {
> -	i915_reg_t reg = DDI_BUF_CTL(port);
> -	int i;
> -
> -	for (i = 0; i < 16; i++) {
> -		udelay(1);
> -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> -			return;
> +	if (IS_BROXTON(dev_priv)) {
> +		udelay(16);
> +		return;
>  	}
> -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> -		port_name(port));
> +
> +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> +			 DDI_BUF_IS_IDLE), 16))

16 is the BXT number. IIRC the spec said 8 usec for the other platforms.

> +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> +			port_name(port));
>  }
>  
>  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Manasi Navare]

On Wed, Jul 01, 2020 at 12:03:30AM +0300, Ville Syrjälä wrote:
> On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> > Modify the helper to add a fixed delay or poll with timeout
> > based on platform specification to check for either Idle bit
> > set (DDI_BUF_CTL is idle for disable case)
> > 
> > v3:
> > * Change the timeout to 16usecs (Ville)
> > v2:
> > * Use 2 separate functions or idle and active (Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 884b507c5f55..052a74625a61 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> >  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> >  				    enum port port)
> >  {
> > -	i915_reg_t reg = DDI_BUF_CTL(port);
> > -	int i;
> > -
> > -	for (i = 0; i < 16; i++) {
> > -		udelay(1);
> > -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> > -			return;
> > +	if (IS_BROXTON(dev_priv)) {
> > +		udelay(16);
> > +		return;
> >  	}
> > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > -		port_name(port));
> > +
> > +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > +			 DDI_BUF_IS_IDLE), 16))
> 
> 16 is the BXT number. IIRC the spec said 8 usec for the other platforms.
>

Yes I see for HSW atleast yes it says 8usecs but i left it at 16 since thats
what we always had and the only change was that BXT add a fixed delay
But if you prefer i will change it to 8us timeout?

Manasi
 
> > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> > +			port_name(port));
> >  }
> >  
> >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/2] drm/i915/dp: Helper to check for DDI BUF status to get active

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 04:26:41PM -0700, Manasi Navare wrote:
> Based on the platform, Bspec expects us to wait or poll with
> timeout for DDI BUF IDLE bit to be set to 0 (non idle) or get active
> after enabling DDI_BUF_CTL.
> 
> v4:
> * Use the timeout for GLK (Ville)
> v3:
> * Add a new function _active for DDI BUF CTL to be non idle (Ville)
> v2:
> * Based on platform, fixed delay or poll (Ville)
> * Use a helper to do this (Imre, Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 052a74625a61..94d57b57139b 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1195,6 +1195,20 @@ static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>  			port_name(port));
>  }
>  
> +static void intel_wait_ddi_buf_active(struct drm_i915_private *dev_priv,
> +				      enum port port)
> +{
> +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv) ) {
> +		usleep_range(600, 1000);

I would probably put a spec quote here to make it clear what this is:
"Wait >518 us for buffers to enable..."

Or we could s/600/518/ to make it easier to figure out. But that could
be a followup.

> +		return;
> +	}
> +
> +	if (wait_for_us(!(intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> +			  DDI_BUF_IS_IDLE), 600))

Spec says 500 usec for this. Is there a reason to not go with the spec
value? I guess one argument is that we didn't do it before. But I'd
probably change it, if not in this patch then as a followup.

> +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get active\n",
> +			port_name(port));
> +}
> +
>  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
>  {
>  	switch (pll->info->id) {
> @@ -4020,7 +4034,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  	intel_de_write(dev_priv, DDI_BUF_CTL(port), intel_dp->DP);
>  	intel_de_posting_read(dev_priv, DDI_BUF_CTL(port));
>  
> -	udelay(600);
> +	intel_wait_ddi_buf_active(dev_priv, port);

Still can't see fdi anywhere.

Whatever
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static void intel_ddi_set_link_train(struct intel_dp *intel_dp,
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Ville Syrjälä]

On Tue, Jun 30, 2020 at 02:10:45PM -0700, Manasi Navare wrote:
> On Wed, Jul 01, 2020 at 12:03:30AM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> > > Modify the helper to add a fixed delay or poll with timeout
> > > based on platform specification to check for either Idle bit
> > > set (DDI_BUF_CTL is idle for disable case)
> > > 
> > > v3:
> > > * Change the timeout to 16usecs (Ville)
> > > v2:
> > > * Use 2 separate functions or idle and active (Ville)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++---------
> > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 884b507c5f55..052a74625a61 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> > >  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> > >  				    enum port port)
> > >  {
> > > -	i915_reg_t reg = DDI_BUF_CTL(port);
> > > -	int i;
> > > -
> > > -	for (i = 0; i < 16; i++) {
> > > -		udelay(1);
> > > -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> > > -			return;
> > > +	if (IS_BROXTON(dev_priv)) {
> > > +		udelay(16);
> > > +		return;
> > >  	}
> > > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > > -		port_name(port));
> > > +
> > > +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > > +			 DDI_BUF_IS_IDLE), 16))
> > 
> > 16 is the BXT number. IIRC the spec said 8 usec for the other platforms.
> >
> 
> Yes I see for HSW atleast yes it says 8usecs but i left it at 16 since thats
> what we always had and the only change was that BXT add a fixed delay
> But if you prefer i will change it to 8us timeout?

My usual approach is to a) just use the spec value, b) if there's
a sane reason for not using it then include a comment documenting
the spec value.

Often b) is just for "spec say a few microseconds, let's just wait
a full millisecond to make it simple", or for "old platforms want
timeout N, new ones want M, just go with the larger of the two for
simplicity". Arguably the current code was trying to follow the
latter approach, except if was supposed to since bxt wasn't supposed
to poll at all.

Anyways, since the current code already used 16 usec without any
clarification I guess this is no worse than what we had.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Manasi
>  
> > > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> > > +			port_name(port));
> > >  }
> > >  
> > >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/2] drm/i915/dp: Helper to check for DDI BUF status to get active

[Manasi Navare]

On Wed, Jul 01, 2020 at 12:20:26AM +0300, Ville Syrjälä wrote:
> On Fri, Jun 26, 2020 at 04:26:41PM -0700, Manasi Navare wrote:
> > Based on the platform, Bspec expects us to wait or poll with
> > timeout for DDI BUF IDLE bit to be set to 0 (non idle) or get active
> > after enabling DDI_BUF_CTL.
> > 
> > v4:
> > * Use the timeout for GLK (Ville)
> > v3:
> > * Add a new function _active for DDI BUF CTL to be non idle (Ville)
> > v2:
> > * Based on platform, fixed delay or poll (Ville)
> > * Use a helper to do this (Imre, Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 052a74625a61..94d57b57139b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1195,6 +1195,20 @@ static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> >  			port_name(port));
> >  }
> >  
> > +static void intel_wait_ddi_buf_active(struct drm_i915_private *dev_priv,
> > +				      enum port port)
> > +{
> > +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv) ) {
> > +		usleep_range(600, 1000);
> 
> I would probably put a spec quote here to make it clear what this is:
> "Wait >518 us for buffers to enable..."
> 
> Or we could s/600/518/ to make it easier to figure out. But that could
> be a followup.

Yes I can change this to 518,1000 and add a comment in the next rev

> 
> > +		return;
> > +	}
> > +
> > +	if (wait_for_us(!(intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > +			  DDI_BUF_IS_IDLE), 600))
> 
> Spec says 500 usec for this. Is there a reason to not go with the spec
> value? I guess one argument is that we didn't do it before. But I'd
> probably change it, if not in this patch then as a followup.

Hmm yes probably the HSDs said 600 but since spec says 500usecs, I will change
the timeout to 500

Manasi

> 
> > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get active\n",
> > +			port_name(port));
> > +}
> > +
> >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> >  {
> >  	switch (pll->info->id) {
> > @@ -4020,7 +4034,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> >  	intel_de_write(dev_priv, DDI_BUF_CTL(port), intel_dp->DP);
> >  	intel_de_posting_read(dev_priv, DDI_BUF_CTL(port));
> >  
> > -	udelay(600);
> > +	intel_wait_ddi_buf_active(dev_priv, port);
> 
> Still can't see fdi anywhere.
> 
> Whatever
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >  }
> >  
> >  static void intel_ddi_set_link_train(struct intel_dp *intel_dp,
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx