[SeaBIOS] Re: [PATCH v2 2/2] Add basic MXM 3.0 interrupt support

[Paul Menzel]

Dear Riku,

Thank you very much for your patch. Great work. Just some small nits.

Am 23.02.24 um 21:55 schrieb Riku Viitanen via SeaBIOS:
> VGAROMs on MXM graphics cards need certain int15h functions present,
> to function properly.

I think the comma is superfluous.
> On HP EliteBook 8560w with coreboot and Quadro 2000M, this warning
> is displayed for 30 seconds, making every boot extremely slow:
>
> ERROR: Valid MXM Structure not found
>    POST halted for 30 seconds, P-state limited to P10...
>
> This patch implements the minimum functionality to get rid of it: the
> functions 0 and 1, version 3.0 of the specification [1]. Older version
> 2.1 is not implemented.
>
> Functions are enabled only if romfile "mxm-30-sis" exists. These functios

functio*n*s
> are specific to the MXM revision, but not the board or GPU. Some boards
> might require more of the optional functions to be implemented, however.
>
> Function 0 returns information about supported functions and revision.
>
> Function 1 returns a pointer to a MXM configuration structure in low
>
> memory. This is copied from romfile "mxm-30-sis".

Again. Great work.

How is the mxm-30-sis file created?
> TEST: HP EliteBook 8560w boots without error and delay, graphics work.
>
> [1]: MXM Graphics Module Software Specification Version 3.0, Revision 1.1
> [2]: https://codeberg.org/Riku_V/mxmdump/

The second URL is not referenced anywhere.
> Signed-off-by: Riku Viitanen
>
> src/optionroms.c |  8 ++++++
>   src/vgahooks.c   | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>   src/vgahooks.h   |  8 ++++++
>   3 files changed, 86 insertions(+)
>   create mode 100644 src/vgahooks.h
>
> diff --git a/src/optionroms.c b/src/optionroms.c
> index e906ab97..4e6a1ad1 100644
> --- a/src/optionroms.c
> +++ b/src/optionroms.c
> @@ -22,6 +22,7 @@
>   #include "string.h" // memset
>   #include "util.h" // get_pnp_offset
>   #include "tcgbios.h" // tpm_*
> +#include "vgahooks.h" // MXM30SIS
>
> static int EnforceChecksum, S3ResumeVga, RunPCIroms;
>
> @@ -463,6 +464,13 @@ vgarom_setup(void)
>       RunPCIroms = romfile_loadint("etc/pci-optionrom-exec", 2);
>       ScreenAndDebug = romfile_loadint("etc/screen-and-debug", 1);
> // Load MXM 3.0 System Information Structure, if it exists
> void *mxm_sis = romfile_loadfile_low("mxm-30-sis", NULL);
> if (mxm_sis)
>    MXM30SIS = (u32)mxm_sis;
> else
>    MXM30SIS = 0;
> // Clear option rom memory
> memset((void*)BUILD_ROM_START, 0, rom_get_max() - BUILD_ROM_START);
>
> diff --git a/src/vgahooks.c b/src/vgahooks.c
> index 1f149532..25e91f20 100644
> --- a/src/vgahooks.c
> +++ b/src/vgahooks.c
> @@ -18,8 +18,10 @@
>   #define VH_VIA 1
>   #define VH_INTEL 2
>   #define VH_SMI 3
> +#define VH_MXM 4
>
> int VGAHookHandlerType VARFSEG;
> +u32 MXM30SIS VARFSEG;
>
> static void
>   handle_155fXX(struct bregs *regs)
> @@ -296,6 +298,71 @@ winent_mb6047_setup(struct pci_device *pci)
>       SmiBootDisplay = 0x02;
>   }
>
> +/****************************************************************
> MXM VGA hooks
> ****************************************************************/
>
> +// Function 0: Return Specification Support Level
> +static void
> +mxm_V30_F00(struct bregs *regs)
> +{
> regs->ax = 0x005f; // Success
> regs->bl = 0x30; // MXM 3.0
> regs->cx = 0x0003; // Supported Functions
> set_success(regs);
>
> +}
>
> +// Function 1: Return a Pointer to the MXM Structure
> +static void
> +mxm_V30_F01(struct bregs *regs)
> +{
> switch (regs->cx) {
> case 0x0030:
>    regs->ax = 0x005f; // Success
>    regs->es = GET_GLOBAL(MXM30SIS)/16;
>    regs->di = GET_GLOBAL(MXM30SIS)%16;
>    set_success(regs);
>    break;
> default:
>    handle_155fXX(regs);
>    break;
> }
>
> +}
>
> +static void
> +mxm_V30(struct bregs *regs)
> +{
> switch (regs->bx) {
> case 0xff00: mxm_V30_F00(regs); break;
> case 0xff01: mxm_V30_F01(regs); break;
> default:   handle_155fXX(regs); break;
> }
>
> +}
>
> +static void
> +mxm_155f80(struct bregs *regs)
> +{
> // TODO: implement other versions, like 2.1
> mxm_V30(regs);
>
> +}
>
> +static void
> +mxm_155f(struct bregs *regs)
> +{
> switch (regs->al) {
> case 0x80: mxm_155f80(regs); break;
> default:   handle_155fXX(regs); break;
> }
>
> +}
>
> +void
> +mxm_setup(void)
> +{
> VGAHookHandlerType = VH_MXM;
>
> +}
> /****************************************************************
> Entry and setup
> ****************************************************************/
>
> @@ -313,6 +380,7 @@ handle_155f(struct bregs *regs)
>       switch (htype) {
>       case VH_VIA:   via_155f(regs); break;
>       case VH_INTEL: intel_155f(regs); break;
> case VH_MXM:   mxm_155f(regs); break;
>  default:       handle_155fXX(regs); break;
>  }
> }
>
> @@ -352,4 +420,6 @@ vgahook_setup(struct pci_device *pci)
>           via_setup(pci);
>       else if (pci->vendor == PCI_VENDOR_ID_INTEL)
>           intel_setup(pci);
> else if (GET_GLOBAL(MXM30SIS))
>    mxm_setup();
> }
>
> diff --git a/src/vgahooks.h b/src/vgahooks.h
> new file mode 100644
> index 00000000..c9113714
> --- /dev/null
> +++ b/src/vgahooks.h
> @@ -0,0 +1,8 @@
> +#ifndef __VGAHOOKS_H
> +#define __VGAHOOKS_H
>
> +#include "types.h" // u32
>
> +extern u32 MXM30SIS;
>
> +#endif // vgahooks.h

Should the Kconfig help text for VGAHOOKS be extended?

Kind regards,

Paul