diff mbox series

[1/2] drm/i915: Fix pre-skl DP AUX precharge length

Message ID 20210318181039.17260-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Fix pre-skl DP AUX precharge length | expand

Commit Message

Ville Syrjälä March 18, 2021, 6:10 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

DP v1.1+ says:
"The DisplayPort transmitter, which is the driving end for a request
 transaction, pre-charges the AUX-CH+ and AUX-CH- to a common mode
 voltage by transmitting 10 to 16 consecutive 0’s in Manchester II code.
 After the active pre-charge, the transmitter sends an AUX Sync pattern.
 The AUX Sync pattern must be as follows:
 Start with 16 consecutive 0s in Manchester-II code, which results in
 a transition from low to high in the middle of each bit period.
 Including active pre-charge pulses, there shall be 26 to 32
 consecutive 0s before the end of the AUX_SYNC pattern."

BDW bspec says:
"Used to determine the precharge time for the Aux Channel. During this
 time the Aux Channel will drive the SYNC pattern. Every microsecond
 gives one additional SYNC pulse beyond the hard coded 26 SYNC pulses.
 The value is the number of microseconds times 2. Default is 3 decimal
 which gives 6us of precharge which is 6 extra SYN pulses for a total
 of 32."

CPT bspec says the same thing apart from:
"... Default is 5 decimal which gives 10us of precharge which is 10
 extra SYNC pulses for a total of 36."

So it looks like to match the max of 32 of the DP spec we should just
always program this extra precharge time to 3.

Unfortunately g4x/ibx bspec doesn't have this clarification, but
since the cpt default was still the same 5 as for g4x/ibx let's
assume the behaviour was always the same.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_aux.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Ville Syrjälä March 18, 2021, 8:52 p.m. UTC | #1
On Thu, Mar 18, 2021 at 08:09:43PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915: Fix pre-skl DP AUX precharge length
> URL   : https://patchwork.freedesktop.org/series/88132/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_9870 -> Patchwork_19807
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_19807 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_19807, 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_19807/index.html
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_19807:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@i915_selftest@live@execlists:
>     - fi-kbl-soraka:      [PASS][1] -> [DMESG-WARN][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9870/fi-kbl-soraka/igt@i915_selftest@live@execlists.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/fi-kbl-soraka/igt@i915_selftest@live@execlists.html

<3> [1019.090846] i915 0000:00:02.0: [drm] *ERROR* AUX B/DDI B/PHY B: did not complete or timeout within 10ms (status 0xad4003ff)

Unrelated to the patch since it doesn't touch the skl+ AUX code
and this machine is a kbl. So seems to be just some random fail.

> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_19807 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@core_hotunplug@unbind-rebind:
>     - fi-bwr-2160:        [PASS][3] -> [FAIL][4] ([i915#3194])
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9870/fi-bwr-2160/igt@core_hotunplug@unbind-rebind.html
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/fi-bwr-2160/igt@core_hotunplug@unbind-rebind.html
> 
>   
>   [i915#3194]: https://gitlab.freedesktop.org/drm/intel/issues/3194
> 
> 
> Participating hosts (44 -> 40)
> ------------------------------
> 
>   Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 
> 
> 
> Build changes
> -------------
> 
>   * Linux: CI_DRM_9870 -> Patchwork_19807
> 
>   CI-20190529: 20190529
>   CI_DRM_9870: a9a5ed8d2432e5335e6c26118cefb2cfff28ae37 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_6036: 5b535494abcdf5ce2b9be99b7bb5df8ab4733083 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_19807: 89c9ce9bcfbf7286c2ec25ffaa26041909ec84d6 @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 89c9ce9bcfbf drm/i915: Remove stray newlines
> 6b3f9e7ed7f6 drm/i915: Fix pre-skl DP AUX precharge length
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/index.html
Vudum, Lakshminarayana March 18, 2021, 9:22 p.m. UTC | #2
Re-reported.

-----Original Message-----
From: Ville Syrjälä <ville.syrjala@linux.intel.com> 
Sent: Thursday, March 18, 2021 1:53 PM
To: intel-gfx@lists.freedesktop.org
Cc: Vudum, Lakshminarayana <lakshminarayana.vudum@intel.com>
Subject: Re: ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Fix pre-skl DP AUX precharge length

On Thu, Mar 18, 2021 at 08:09:43PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915: Fix pre-skl DP AUX precharge length
> URL   : https://patchwork.freedesktop.org/series/88132/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_9870 -> Patchwork_19807 
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_19807 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_19807, 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_19807/index.html
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_19807:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@i915_selftest@live@execlists:
>     - fi-kbl-soraka:      [PASS][1] -> [DMESG-WARN][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9870/fi-kbl-soraka/igt@i915_selftest@live@execlists.html
>    [2]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/fi-kbl-soraka
> /igt@i915_selftest@live@execlists.html

<3> [1019.090846] i915 0000:00:02.0: [drm] *ERROR* AUX B/DDI B/PHY B: did not complete or timeout within 10ms (status 0xad4003ff)

Unrelated to the patch since it doesn't touch the skl+ AUX code and this machine is a kbl. So seems to be just some random fail.

> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_19807 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@core_hotunplug@unbind-rebind:
>     - fi-bwr-2160:        [PASS][3] -> [FAIL][4] ([i915#3194])
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9870/fi-bwr-2160/igt@core_hotunplug@unbind-rebind.html
>    [4]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/fi-bwr-2160/i
> gt@core_hotunplug@unbind-rebind.html
> 
>   
>   [i915#3194]: https://gitlab.freedesktop.org/drm/intel/issues/3194
> 
> 
> Participating hosts (44 -> 40)
> ------------------------------
> 
>   Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 
> 
> 
> Build changes
> -------------
> 
>   * Linux: CI_DRM_9870 -> Patchwork_19807
> 
>   CI-20190529: 20190529
>   CI_DRM_9870: a9a5ed8d2432e5335e6c26118cefb2cfff28ae37 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_6036: 5b535494abcdf5ce2b9be99b7bb5df8ab4733083 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_19807: 89c9ce9bcfbf7286c2ec25ffaa26041909ec84d6 @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 89c9ce9bcfbf drm/i915: Remove stray newlines
> 6b3f9e7ed7f6 drm/i915: Fix pre-skl DP AUX precharge length
> 
> == Logs ==
> 
> For more details see: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19807/index.html

--
Ville Syrjälä
Intel
Ville Syrjälä March 30, 2021, 6:06 p.m. UTC | #3
On Thu, Mar 18, 2021 at 08:10:38PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP v1.1+ says:
> "The DisplayPort transmitter, which is the driving end for a request
>  transaction, pre-charges the AUX-CH+ and AUX-CH- to a common mode
>  voltage by transmitting 10 to 16 consecutive 0’s in Manchester II code.
>  After the active pre-charge, the transmitter sends an AUX Sync pattern.
>  The AUX Sync pattern must be as follows:
>  Start with 16 consecutive 0s in Manchester-II code, which results in
>  a transition from low to high in the middle of each bit period.
>  Including active pre-charge pulses, there shall be 26 to 32
>  consecutive 0s before the end of the AUX_SYNC pattern."
> 
> BDW bspec says:
> "Used to determine the precharge time for the Aux Channel. During this
>  time the Aux Channel will drive the SYNC pattern. Every microsecond
>  gives one additional SYNC pulse beyond the hard coded 26 SYNC pulses.
>  The value is the number of microseconds times 2. Default is 3 decimal
>  which gives 6us of precharge which is 6 extra SYN pulses for a total
>  of 32."
> 
> CPT bspec says the same thing apart from:
> "... Default is 5 decimal which gives 10us of precharge which is 10
>  extra SYNC pulses for a total of 36."
> 
> So it looks like to match the max of 32 of the DP spec we should just
> always program this extra precharge time to 3.
> 
> Unfortunately g4x/ibx bspec doesn't have this clarification, but
> since the cpt default was still the same 5 as for g4x/ibx let's
> assume the behaviour was always the same.

I did a bit more archaeology and found the following:

commit e3421a189447 ("drm/i915: enable DP/eDP for Sandybridge/Cougarpoint")
added the precharge==3 for snb.

commit 092945e11c5b ("drm/i915/dp: Use auxch precharge value of 5 everywhere")
tried to change it to be 5 for snb.

commit 6b4e0a93ff6e ("Revert "drm/i915/dp: Use auxch precharge value of 5 everywhere"")
went back to 3 for snb due to a regression.

So I think the value of 5 was just always wrong, but I guess very
few display actually get upset if we do too many SYNCs. Also DP 1.0
did not specify any max value for this, whereas DP 1.1+ added the
max==32 wording.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index eaebf123310a..d5443d6d6123 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -126,12 +126,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv =
>  			to_i915(dig_port->base.base.dev);
> -	u32 precharge, timeout;
> -
> -	if (IS_GEN(dev_priv, 6))
> -		precharge = 3;
> -	else
> -		precharge = 5;
> +	u32 timeout;
>  
>  	if (IS_BROADWELL(dev_priv))
>  		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> @@ -145,7 +140,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	       timeout |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
>  	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> -	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +	       (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
>  	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
>  }
>  
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak April 26, 2021, 3:54 p.m. UTC | #4
On Thu, Mar 18, 2021 at 08:10:38PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP v1.1+ says:
> "The DisplayPort transmitter, which is the driving end for a request
>  transaction, pre-charges the AUX-CH+ and AUX-CH- to a common mode
>  voltage by transmitting 10 to 16 consecutive 0’s in Manchester II code.
>  After the active pre-charge, the transmitter sends an AUX Sync pattern.
>  The AUX Sync pattern must be as follows:
>  Start with 16 consecutive 0s in Manchester-II code, which results in
>  a transition from low to high in the middle of each bit period.
>  Including active pre-charge pulses, there shall be 26 to 32
>  consecutive 0s before the end of the AUX_SYNC pattern."
> 
> BDW bspec says:
> "Used to determine the precharge time for the Aux Channel. During this
>  time the Aux Channel will drive the SYNC pattern. Every microsecond
>  gives one additional SYNC pulse beyond the hard coded 26 SYNC pulses.
>  The value is the number of microseconds times 2. Default is 3 decimal
>  which gives 6us of precharge which is 6 extra SYN pulses for a total
>  of 32."
> 
> CPT bspec says the same thing apart from:
> "... Default is 5 decimal which gives 10us of precharge which is 10
>  extra SYNC pulses for a total of 36."
> 
> So it looks like to match the max of 32 of the DP spec we should just
> always program this extra precharge time to 3.
> 
> Unfortunately g4x/ibx bspec doesn't have this clarification, but
> since the cpt default was still the same 5 as for g4x/ibx let's
> assume the behaviour was always the same.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index eaebf123310a..d5443d6d6123 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -126,12 +126,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv =
>  			to_i915(dig_port->base.base.dev);
> -	u32 precharge, timeout;
> -
> -	if (IS_GEN(dev_priv, 6))
> -		precharge = 3;
> -	else
> -		precharge = 5;
> +	u32 timeout;
>  
>  	if (IS_BROADWELL(dev_priv))
>  		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> @@ -145,7 +140,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	       timeout |
>  	       DP_AUX_CH_CTL_RECEIVE_ERROR |
>  	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> -	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +	       (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
>  	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
>  }
>  
> -- 
> 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_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index eaebf123310a..d5443d6d6123 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -126,12 +126,7 @@  static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv =
 			to_i915(dig_port->base.base.dev);
-	u32 precharge, timeout;
-
-	if (IS_GEN(dev_priv, 6))
-		precharge = 3;
-	else
-		precharge = 5;
+	u32 timeout;
 
 	if (IS_BROADWELL(dev_priv))
 		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
@@ -145,7 +140,7 @@  static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 	       timeout |
 	       DP_AUX_CH_CTL_RECEIVE_ERROR |
 	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
-	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
+	       (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
 	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
 }