it's already been pushed and reviewed On Fri, Oct 2, 2026, 8:45 AM Persmule <[email protected]> wrote:
> Do you have any plan to upload this patch to https://review.coreboot.org/ > ? At least I can vote +1 for it as I have confirmed it working. > > > > On Thursday, October 1st, 2026 at 12:24 PM, Sebastian Müller < > [email protected]> wrote: > > > Hi, > > I think I found the actual problem, and it does not seem to be in > > libgfxinit. That would also fit with the X301 test, where changing the > > libgfxinit revision did not make any difference. > > > > In src/lib/hw-time-timer.adb the monotonic timer is imported like this: > > > > procedure Timer_Monotonic_Get (MT : out Interfaces.C.long); > > pragma Import (C, Timer_Monotonic_Get, "timer_monotonic_get"); > > > > The C side now uses: > > > > struct mono_time { > > uint64_t microseconds; > > }; > > > > and timer_monotonic_get() eventually does: > > > > *mt = mono_counter.time; > > > > So the C function writes 8 bytes. You can also see that directly in the > > disassembly: > > > > mov %eax,(%ebx) > > mov %edx,0x4(%ebx) > > > > The problem is that on a 32-bit ramstage, Interfaces.C.long is only 4 > > bytes. GNAT allocates a 4-byte temporary, but the C function writes 8 > > bytes into it. > > > > As far as I can tell, the Ada declaration was correct when it was > > originally added. The mismatch appeared later with commit b11f9f7e16 > > (timer: Switch mono_time to uint64_t, 2022-08-13), which changed the C > > side to 64 bit without changing the Ada import. > > > > Without LTO this seems to work mostly by accident. Raw_Value_Min stays as > > a separate function, has a fairly large stack frame, and the extra four > > bytes happen to overwrite unused stack space. > > > > With LTO and GCC 15.2.0 the situation changes. The timer code gets > > inlined, and in HW.Time.Ms_From_Now, HW.Time.Us_From_Now and > > GMA.Panel.Wait_On the temporary ends up at 0xc(%esp) in a 16-byte stack > > frame. > > > > That means the upper four bytes written by timer_monotonic_get() > overwrite > > the saved %ebx. > > > > When the function returns and restores %ebx, it restores the upper half > of > > the timer value instead of the original register value. > > > > That seems to be exactly what breaks the display probe. > > > > In the inlined Probe_Port code inside gma_gfxinit: > > > > movzbl %al,%ebx > > cmpb $0x0,0x4036320(%ebx) > > mov %ebx,%eax > > call to_panel > > movzbl %al,%eax > > call panel__wait_on.part.0 > > mov %ebx,%edx > > call display_probing__read_edid > > > > %ebx holds the port number. > > > > After Panel.Wait_On, %ebx is no longer the port. During early boot the > > upper 32 bits of the microsecond counter are still zero, so %ebx > > effectively becomes 0, which is Disabled. > > > > Read_EDID then subtracts one from the port, giving 0xff. The range check > > fails, To_Display_Type falls through to its default case and returns DP, > > and the DP path ends up using DP_Port'First, which is AUX channel A. > > > > That explains why it tries DDI_AUX_CTL_A instead of GMBUS pin 3, then > > times out and eventually calls Panel.Off. > > > > It also explains why only the eighth probe is affected. > > > > For DP1-3, HDMI1-3 and VGA, To_Panel returns No_Panel, so Panel.Wait_On > > exits before it reaches the timer code. The internal panel is the only > one > > that actually goes through the broken path. > > > > The fact that the X301 behaves the same way also makes sense then, > because > > this is shared timer code rather than something specific to one graphics > > generation. > > > > The fix is simply to make the Ada declaration match the C type: > > > > -- C declares this as `struct mono_time { uint64_t microseconds; > }'. > > procedure Timer_Monotonic_Get (MT : out Word64); > > > > With that change, the generated code looks sane again. Raw_Value_Min is > > called out of line, the 64-bit value comes back in eax:edx, and there is > > no 4-byte temporary on the stack for the C function to overwrite. > > > > It also builds cleanly here for the hp/2170p with GCC 15.2.0 and LTO > > enabled. > > > > As a small side effect, ramstage is about 500 bytes smaller with LTO and > > around 240 bytes smaller without LTO. > > > > I cannot test the actual boot because I do not have a 2170p or X301 here. > > The generated code makes the corruption pretty clear though. > > > > Could you test the patch on both machines with LTO enabled? That should > > tell us whether this is indeed the bug you are seeing. > > _______________________________________________ > > coreboot mailing list -- [email protected] > > To unsubscribe send an email to [email protected] > > >
_______________________________________________ coreboot mailing list -- [email protected] To unsubscribe send an email to [email protected]
