Change in ...libgfxinit[master]: dp training: Write correct training data when switching patterns
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/libgfxinit/+/32732
Change subject: dp training: Write correct training data when switching patterns ......................................................................
dp training: Write correct training data when switching patterns
Apparently this was wrong all the time. When switching the training pattern, i.e. writes to DPCD+0x102, we also have to write the current signal levels to subsequent offsets. We always wrote 0s in this case, even if we already negotiated higher values during the clock-recovery phase. Obviously, this results in havoc if the sink takes the 0s serious.
Change-Id: I6ae2f9aaec0b042e8dee6e8b0099ea62c82f611b Signed-off-by: Nico Huber nico.huber@secunet.com --- M common/hw-gfx-dp_training.adb 1 file changed, 11 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/32/32732/1
diff --git a/common/hw-gfx-dp_training.adb b/common/hw-gfx-dp_training.adb index 079d49c..d613d85 100644 --- a/common/hw-gfx-dp_training.adb +++ b/common/hw-gfx-dp_training.adb @@ -113,9 +113,10 @@ end Sink_Init;
procedure Sink_Set_Training_Pattern - (DP : in Aux_T; + (Port : in T; Link : in DP_Link; Pattern : in DP_Info.Training_Pattern; + Train_Set : in DP_Info.Train_Set; Success : out Boolean) is use type DP_Info.Training_Pattern; @@ -124,21 +125,21 @@ TP : constant TP_Array := TP_Array' (DP_Info.TP_1 => 16#21#, DP_Info.TP_2 => 16#22#, DP_Info.TP_3 => 16#23#, DP_Info.TP_Idle => 16#00#, DP_Info.TP_None => 16#00#); + T_Set : constant Word8 := Training_Set (Port, Train_Set);
Data : DP_Defs.Aux_Payload; Length : Positive := 1; begin pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity));
- Data := - (0 => TP (Pattern), - others => 0); -- Don't care - + Data := (TP (Pattern), others => 0); if Pattern < DP_Info.TP_Idle then Length := Length + Lane_Count (Link); + Data (1 .. Lane_Count (Link)) := (others => T_Set); end if; + Aux_Ch.Aux_Write - (Port => DP, + (Port => To_Aux (Port), Address => 16#00102#, -- TRAINING_PATTERN_SET Length => Length, Data => Data, @@ -332,7 +333,8 @@ pragma Warnings (GNATprove, On, """Success"" modified by call, but value overwritten*"); if Success then - Sink_Set_Training_Pattern (DP, Link, DP_Info.TP_1, Success); + Sink_Set_Training_Pattern + (Port, Link, DP_Info.TP_1, Train_Set, Success); end if;
if Success then @@ -365,7 +367,7 @@
if Success then Set_Pattern (Port, Link, EQ_Pattern); - Sink_Set_Training_Pattern (DP, Link, EQ_Pattern, Success); + Sink_Set_Training_Pattern (Port, Link, EQ_Pattern, Train_Set, Success); end if;
if Success then @@ -385,7 +387,7 @@ -- Set_Pattern (TP_None) includes sending the Idle Pattern, -- so tell sink first. Sink_Set_Training_Pattern - (DP, Link, DP_Info.TP_None, Success); + (Port, Link, DP_Info.TP_None, Train_Set, Success); Set_Pattern (Port, Link, DP_Info.TP_None);
Success := Success and then EQ_Done;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
Patch Set 1: Verified+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32732/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32732/1//COMMIT_MSG@14 PS1, Line 14: serious. Could you add the specific sink, taking this serious?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
Patch Set 1: Code-Review+1
patch train tested ok on google/link
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
Patch Set 1: Code-Review+2
Hello Patrick Rudolph, Arthur Heymans, Matt DeVillier,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/libgfxinit/+/32732
to look at the new patch set (#2).
Change subject: dp training: Write correct training data when switching patterns ......................................................................
dp training: Write correct training data when switching patterns
Apparently this was wrong all the time. When switching the training pattern, i.e. writes to DPCD+0x102, we also have to write the current signal levels to subsequent offsets. We always wrote 0s in this case, even if we already negotiated higher values during the clock-recovery phase. Obviously, this results in havoc if the sink takes the 0s serious.
TEST=Run a few hundred training rounds with a Terra 2462W display. This display almost always requested an increase of the voltage swing to level 1. Trainings where it recovered the clock with level 0 always succeeded, while trainings with level 1 almost always lost synchronization at the start of channel equalization. With the patch applied, all trainings succeeded.
Change-Id: I6ae2f9aaec0b042e8dee6e8b0099ea62c82f611b Signed-off-by: Nico Huber nico.huber@secunet.com --- M common/hw-gfx-dp_training.adb 1 file changed, 11 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/32/32732/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32732/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32732/1//COMMIT_MSG@14 PS1, Line 14: serious.
Could you add the specific sink, taking this serious?
Done
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/libgfxinit/+/32732 )
Change subject: dp training: Write correct training data when switching patterns ......................................................................
dp training: Write correct training data when switching patterns
Apparently this was wrong all the time. When switching the training pattern, i.e. writes to DPCD+0x102, we also have to write the current signal levels to subsequent offsets. We always wrote 0s in this case, even if we already negotiated higher values during the clock-recovery phase. Obviously, this results in havoc if the sink takes the 0s serious.
TEST=Run a few hundred training rounds with a Terra 2462W display. This display almost always requested an increase of the voltage swing to level 1. Trainings where it recovered the clock with level 0 always succeeded, while trainings with level 1 almost always lost synchronization at the start of channel equalization. With the patch applied, all trainings succeeded.
Change-Id: I6ae2f9aaec0b042e8dee6e8b0099ea62c82f611b Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/libgfxinit/+/32732 Tested-by: Nico Huber nico.h@gmx.de Reviewed-by: Matt DeVillier matt.devillier@gmail.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M common/hw-gfx-dp_training.adb 1 file changed, 11 insertions(+), 9 deletions(-)
Approvals: Nico Huber: Verified Matt DeVillier: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/common/hw-gfx-dp_training.adb b/common/hw-gfx-dp_training.adb index 079d49c..d613d85 100644 --- a/common/hw-gfx-dp_training.adb +++ b/common/hw-gfx-dp_training.adb @@ -113,9 +113,10 @@ end Sink_Init;
procedure Sink_Set_Training_Pattern - (DP : in Aux_T; + (Port : in T; Link : in DP_Link; Pattern : in DP_Info.Training_Pattern; + Train_Set : in DP_Info.Train_Set; Success : out Boolean) is use type DP_Info.Training_Pattern; @@ -124,21 +125,21 @@ TP : constant TP_Array := TP_Array' (DP_Info.TP_1 => 16#21#, DP_Info.TP_2 => 16#22#, DP_Info.TP_3 => 16#23#, DP_Info.TP_Idle => 16#00#, DP_Info.TP_None => 16#00#); + T_Set : constant Word8 := Training_Set (Port, Train_Set);
Data : DP_Defs.Aux_Payload; Length : Positive := 1; begin pragma Debug (Debug.Put_Line (GNAT.Source_Info.Enclosing_Entity));
- Data := - (0 => TP (Pattern), - others => 0); -- Don't care - + Data := (TP (Pattern), others => 0); if Pattern < DP_Info.TP_Idle then Length := Length + Lane_Count (Link); + Data (1 .. Lane_Count (Link)) := (others => T_Set); end if; + Aux_Ch.Aux_Write - (Port => DP, + (Port => To_Aux (Port), Address => 16#00102#, -- TRAINING_PATTERN_SET Length => Length, Data => Data, @@ -332,7 +333,8 @@ pragma Warnings (GNATprove, On, """Success"" modified by call, but value overwritten*"); if Success then - Sink_Set_Training_Pattern (DP, Link, DP_Info.TP_1, Success); + Sink_Set_Training_Pattern + (Port, Link, DP_Info.TP_1, Train_Set, Success); end if;
if Success then @@ -365,7 +367,7 @@
if Success then Set_Pattern (Port, Link, EQ_Pattern); - Sink_Set_Training_Pattern (DP, Link, EQ_Pattern, Success); + Sink_Set_Training_Pattern (Port, Link, EQ_Pattern, Train_Set, Success); end if;
if Success then @@ -385,7 +387,7 @@ -- Set_Pattern (TP_None) includes sending the Idle Pattern, -- so tell sink first. Sink_Set_Training_Pattern - (DP, Link, DP_Info.TP_None, Success); + (Port, Link, DP_Info.TP_None, Train_Set, Success); Set_Pattern (Port, Link, DP_Info.TP_None);
Success := Success and then EQ_Done;
-
Arthur Heymans (Code Review) -
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)