Re: efiboot: Restore GOP mode on SetMode() failure

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

Andrew Daugherity
From:       Klemens Nanni <kl3 () posteo ! org>
Date:       2017-09-16 13:58:21
Message-ID: <[hidden email]>
>Updated diff introducing efi_gop_setmode() as helper to reduce duplicate
>code and enhance readability.
>
>This code continues to work on three different UEFI implementations
>without any regression.
>
>Has anyone tested this on their machine by any chance?

I tested this diff in combination with your "Implement machine gop command"
diff on a Dell PowerEdge R230 and a VirtualBox VM (EFI enabled).  No
regressions, however 'machine gop' doesn't work quite how I expected it to
-- it seems the video mode set with it only applies to the bootloader, and
once the kernel loads, it sets a different mode (possibly a bad one).  I
was hoping the kernel would use the mode I just set.

Sometimes the 800x600 and 640x480 modes either have parts of the screen cut
off, or show a blank screen (but the resolution does change).  Not a huge
deal, since the default 1024x768 mode works fine; also, the FreeBSD loader
has the same issue.

It seems that the 'machine video' setting (e.g. 80x25, 100x31, etc.) is
independent of 'machine gop', and they can trample each other: changing the
"gop" mode does not resize the "video" mode, but changing the "video" mode
always resets the "gop" mode to 1024x768.

Here are the available modes in VirtualBox:
====
>> OpenBSD/amd64 BOOTX64 3.35
boot> machine video
Mode 0: 80 x 25
Mode 2: 100 x 31
Mode 3: 128 x 40

Current Mode = 2
boot> machine gop
Mode 0: 640 x 480 (stride = 640)
Mode 1: 800 x 600 (stride = 800)
Mode 2: 1024 x 768 (stride = 1024)
Mode 3: 1280 x 1024 (stride = 1280)
Mode 4: 1440 x 900 (stride = 1440)
Mode 5: 1920 x 1200 (stride = 1920)

Current Mode = 2
====
All of them work properly, with the caveats described above.  Once the
kernel loads, it switches to 1920x1200 with a windowboxed 100x31 console.


The Dell R230 only has video mode 0 (80x25); the gop modes listed vary
depending on whether the BIOS setting "Load Legacy Video Option ROM" is
enabled.  With it enabled, it only lists modes 0-2, defaulting to 2
(1024x768).  With it disabled, there are additional modes for 1280x960 and
1280x1024 (our rackmount KVM monitor is 1280x1024 native, but specifies a
preferred mode of 1024x768 in its EDID).

All these modes work properly in the bootloader (640x480 & 800x600 have the
same issues as VirtualBox, but everything else is fine), but once the
OpenBSD kernel loads, it sets the resolution to 1024x768, and scrambles the
video output (regardless of gop mode) as mentioned at [1][2] if the legacy
video ROM is disabled. Since that posting, the spontaneous reboot has been
fixed, and serial console support was added to the EFI bootloader, so I was
able to install OpenBSD in EFI mode, and recently randomly discovered that
toggling this BIOS setting "fixes" the OpenBSD console.

This setting is described in the manual [3, p. 44] as:
Load Legacy Video Option ROM
-----------------------------
“Enables you to determine whether the system BIOS loads the legacy video
(INT 10H) option ROM from the video controller. Selecting[sic] Enabled [if]
the operating system does not support UEFI video output standards. This
field is available only for UEFI boot mode. You cannot set the option to
Enabled if UEFI Secure Boot mode is enabled."

With it enabled, the kernel sets a 1024x768 mode (100x31 letterboxed) and
displays mostly correctly (some stray garbage on the leftmost 2 pixels).
With it disabled (the default), video is scrambled to a thin purple line
and efifb(4) is listed as not configured; X still works in either case.
FreeBSD & Linux consoles are fine even with the legacy video ROM disabled.
I've included the dmesgs for both cases.


Thanks,

Andrew

[1] https://marc.info/?l=openbsd-misc&m=146343624320665&w=2
[2] https://marc.info/?l=openbsd-misc&m=148723613906289&w=2
[3] http://topics-cdn.dell.com/pdf/poweredge-r230_owner's%20manual_en-us.pdf

dmesg.legacy-vbios (11K) Download Attachment
dmesg.no-vbios (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

YASUOKA Masahiko-3
Hi,

On Tue, 3 Oct 2017 18:15:33 -0500
Andrew Daugherity <[hidden email]> wrote:

> From:       Klemens Nanni <kl3 () posteo ! org>
> Date:       2017-09-16 13:58:21
> Message-ID: <[hidden email]>
>>Updated diff introducing efi_gop_setmode() as helper to reduce duplicate
>>code and enhance readability.
>>
>>This code continues to work on three different UEFI implementations
>>without any regression.
>>
>>Has anyone tested this on their machine by any chance?
>
> I tested this diff in combination with your "Implement machine gop command"
> diff on a Dell PowerEdge R230 and a VirtualBox VM (EFI enabled).  No
> regressions, however 'machine gop' doesn't work quite how I expected it to
> -- it seems the video mode set with it only applies to the bootloader, and
> once the kernel loads, it sets a different mode (possibly a bad one).  I
> was hoping the kernel would use the mode I just set.

I also expected that the kernel uses the mode to which the gop command
set.  I think it's more useful.

The diff is updated.

comments?

Index: sys/arch/amd64/stand/efiboot/efiboot.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
retrieving revision 1.24
diff -u -p -r1.24 efiboot.c
--- sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 04:52:22 -0000 1.24
+++ sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 05:09:29 -0000
@@ -51,6 +51,7 @@ static EFI_GUID imgp_guid = LOADED_IMA
 static EFI_GUID blkio_guid = BLOCK_IO_PROTOCOL;
 static EFI_GUID devp_guid = DEVICE_PATH_PROTOCOL;
 u_long efi_loadaddr;
+static int efi_gopmode = -1;
 
 static int efi_device_path_depth(EFI_DEVICE_PATH *dp, int);
 static int efi_device_path_ncmp(EFI_DEVICE_PATH *, EFI_DEVICE_PATH *,
@@ -722,7 +723,7 @@ efi_makebootargs(void)
  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
  *gopi;
  bios_efiinfo_t ei;
- int curmode, bestmode = -1;
+ int curmode;
  UINTN sz, gopsiz, bestsiz = 0;
  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
  *info;
@@ -748,20 +749,23 @@ efi_makebootargs(void)
  status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
     (void **)&gop);
  if (!EFI_ERROR(status)) {
- for (i = 0; i < gop->Mode->MaxMode; i++) {
- status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
- if (EFI_ERROR(status))
- continue;
- gopsiz = info->HorizontalResolution *
-    info->VerticalResolution;
- if (gopsiz > bestsiz) {
- bestmode = i;
- bestsiz = gopsiz;
+ if (efi_gopmode < 0) {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop, i, &sz,
+    &info);
+ if (EFI_ERROR(status))
+ continue;
+ gopsiz = info->HorizontalResolution *
+    info->VerticalResolution;
+ if (gopsiz > bestsiz) {
+ efi_gopmode = i;
+ bestsiz = gopsiz;
+ }
  }
  }
- if (bestmode >= 0) {
+ if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
  curmode = gop->Mode->Mode;
- if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
+ if (efi_gop_setmode(gop, efi_gopmode) != EFI_SUCCESS)
  (void)efi_gop_setmode(gop, curmode);
  }
 
@@ -882,5 +886,49 @@ int
 Xpoweroff_efi(void)
 {
  EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
+ return (0);
+}
+
+int
+Xgop_efi(void)
+{
+ EFI_STATUS status;
+ EFI_GRAPHICS_OUTPUT *gop;
+ EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
+ *info;
+ int i, mode = -1;
+ UINTN sz;
+
+ status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
+    (void **)&gop);
+ if (EFI_ERROR(status))
+ return (0);
+
+ if (cmd.argc >= 2) {
+ mode = strtol(cmd.argv[1], NULL, 10);
+ if (0 <= mode && mode < gop->Mode->MaxMode) {
+ status = EFI_CALL(gop->QueryMode, gop, mode,
+    &sz, &info);
+ if (!EFI_ERROR(status)) {
+ if (efi_gop_setmode(gop, mode)
+    == EFI_SUCCESS)
+ efi_gopmode = mode;
+ }
+ }
+ } else {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop, i, &sz,
+    &info);
+ if (EFI_ERROR(status))
+ continue;
+ printf("Mode %d: %d x %d (stride = %d)\n", i,
+    info->HorizontalResolution,
+    info->VerticalResolution,
+    info->PixelsPerScanLine);
+ }
+ printf("\n");
+ }
+ printf("Current Mode = %d\n", gop->Mode->Mode);
+
  return (0);
 }
Index: sys/arch/amd64/stand/efiboot/efiboot.h
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.h,v
retrieving revision 1.2
diff -u -p -r1.2 efiboot.h
--- sys/arch/amd64/stand/efiboot/efiboot.h 31 May 2017 08:40:32 -0000 1.2
+++ sys/arch/amd64/stand/efiboot/efiboot.h 6 Oct 2017 05:09:29 -0000
@@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
 int efi_com_getc(dev_t);
 void efi_com_putc(dev_t, int);
 int Xvideo_efi(void);
+int Xgop_efi(void);
 int Xexit_efi(void);
 void efi_makebootargs(void);
 
Index: sys/arch/amd64/stand/libsa/cmd_i386.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
retrieving revision 1.11
diff -u -p -r1.11 cmd_i386.c
--- sys/arch/amd64/stand/libsa/cmd_i386.c 31 May 2017 08:23:33 -0000 1.11
+++ sys/arch/amd64/stand/libsa/cmd_i386.c 6 Oct 2017 05:09:29 -0000
@@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
  { "memory", CMDT_CMD, Xmemory },
 #ifdef EFIBOOT
  { "video", CMDT_CMD, Xvideo_efi },
+ { "gop", CMDT_CMD, Xgop_efi },
  { "exit", CMDT_CMD, Xexit_efi },
  { "poweroff", CMDT_CMD, Xpoweroff_efi },
 #endif


Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

Klemens Nanni
In reply to this post by Andrew Daugherity
On Tue, Oct 03, 2017 at 11:15:33PM +0000, Andrew Daugherity wrote:
> I tested this diff in combination with your "Implement machine gop command"
> diff on a Dell PowerEdge R230 and a VirtualBox VM (EFI enabled).  No
> regressions, however 'machine gop' doesn't work quite how I expected it to
> -- it seems the video mode set with it only applies to the bootloader, and
> once the kernel loads, it sets a different mode (possibly a bad one).  I
> was hoping the kernel would use the mode I just set.
Thanks a lot for testing! I'm working on the kernel part.

> Sometimes the 800x600 and 640x480 modes either have parts of the screen cut
> off, or show a blank screen (but the resolution does change).  Not a huge
> deal, since the default 1024x768 mode works fine; also, the FreeBSD loader
> has the same issue.
This is probably due to buggy firmware implementations. I've seen such
behaviour on many (all?) boards.

> It seems that the 'machine video' setting (e.g. 80x25, 100x31, etc.) is
> independent of 'machine gop', and they can trample each other: changing the
> "gop" mode does not resize the "video" mode, but changing the "video" mode
> always resets the "gop" mode to 1024x768.
I cannot reproduce this on a DELL Latitiude E5550, setting modes for
both protocols works fine without one breaking or resetting the other.
I'll test other boards when possible.

        boot> machine video
        Mode 0: 80 x 25
        Mode 1: 80 x 50
        Mode 2: 100 x 31
        Mode 3: 240 x 56
       
        Current Mode = 2
        boot> machine gop
        Mode 0: 1280 x 1024 (stride = 1280)
        Mode 1: 1024 x 768 (stride = 1024)
        Mode 2: 640 x 480 (stride = 640)
        Mode 3: 800 x 600 (stride = 800)
        Mode 4: 1920 x 1080 (stride = 1920)
       
        Current Mode = 3
        boot> machine video 3
        Current Mode = 3
        boot> machine gop 4
        Current Mode = 4


> Here are the available modes in VirtualBox:
> ====
> >> OpenBSD/amd64 BOOTX64 3.35
> boot> machine video
> Mode 0: 80 x 25
> Mode 2: 100 x 31
> Mode 3: 128 x 40
>
> Current Mode = 2
> boot> machine gop
> Mode 0: 640 x 480 (stride = 640)
> Mode 1: 800 x 600 (stride = 800)
> Mode 2: 1024 x 768 (stride = 1024)
> Mode 3: 1280 x 1024 (stride = 1280)
> Mode 4: 1440 x 900 (stride = 1440)
> Mode 5: 1920 x 1200 (stride = 1920)
>
> Current Mode = 2
> ====
> All of them work properly, with the caveats described above.  Once the
> kernel loads, it switches to 1920x1200 with a windowboxed 100x31 console.
Same windowboxed behaviour here on all tested machines.

> The Dell R230 only has video mode 0 (80x25); the gop modes listed vary
> depending on whether the BIOS setting "Load Legacy Video Option ROM" is
> enabled.  With it enabled, it only lists modes 0-2, defaulting to 2
> (1024x768).  With it disabled, there are additional modes for 1280x960 and
> 1280x1024 (our rackmount KVM monitor is 1280x1024 native, but specifies a
> preferred mode of 1024x768 in its EDID).
>
> All these modes work properly in the bootloader (640x480 & 800x600 have the
> same issues as VirtualBox, but everything else is fine), but once the
> OpenBSD kernel loads, it sets the resolution to 1024x768, and scrambles the
> video output (regardless of gop mode) as mentioned at [1][2] if the legacy
> video ROM is disabled. Since that posting, the spontaneous reboot has been
> fixed, and serial console support was added to the EFI bootloader, so I was
> able to install OpenBSD in EFI mode, and recently randomly discovered that
> toggling this BIOS setting "fixes" the OpenBSD console.
>
> This setting is described in the manual [3, p. 44] as:
> Load Legacy Video Option ROM
> -----------------------------
> “Enables you to determine whether the system BIOS loads the legacy video
> (INT 10H) option ROM from the video controller. Selecting[sic] Enabled [if]
> the operating system does not support UEFI video output standards. This
> field is available only for UEFI boot mode. You cannot set the option to
> Enabled if UEFI Secure Boot mode is enabled."
>
> With it enabled, the kernel sets a 1024x768 mode (100x31 letterboxed) and
> displays mostly correctly (some stray garbage on the leftmost 2 pixels).
> With it disabled (the default), video is scrambled to a thin purple line
> and efifb(4) is listed as not configured; X still works in either case.
> FreeBSD & Linux consoles are fine even with the legacy video ROM disabled.
> I've included the dmesgs for both cases.
Sounds like FreeBSD and Linux fall back to the old Universal Graphics
Adapter Protocal, which still relies on VGA. We only support GOP, which
is the hardware independent successor of UGA.

> Thanks,
>
> Andrew
>
> [1] https://marc.info/?l=openbsd-misc&m=146343624320665&w=2
> [2] https://marc.info/?l=openbsd-misc&m=148723613906289&w=2
> [3] http://topics-cdn.dell.com/pdf/poweredge-r230_owner's%20manual_en-us.pdf

Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

Klemens Nanni
In reply to this post by YASUOKA Masahiko-3
On Fri, Oct 06, 2017 at 05:15:20AM +0000, YASUOKA Masahiko wrote:

> On Tue, 3 Oct 2017 18:15:33 -0500
> Andrew Daugherity <[hidden email]> wrote:
> > I tested this diff in combination with your "Implement machine gop command"
> > diff on a Dell PowerEdge R230 and a VirtualBox VM (EFI enabled).  No
> > regressions, however 'machine gop' doesn't work quite how I expected it to
> > -- it seems the video mode set with it only applies to the bootloader, and
> > once the kernel loads, it sets a different mode (possibly a bad one).  I
> > was hoping the kernel would use the mode I just set.
>
> I also expected that the kernel uses the mode to which the gop command
> set.  I think it's more useful.
>
> The diff is updated.
>
> comments?
See my updated diff for reusing the gopi struct, please.

Searching for and setting a better mode only if the user didn't pick one
manually before is definitely a good idea, thanks.

Udated diff implementing `machine gop [n]' attached.

> Index: sys/arch/amd64/stand/efiboot/efiboot.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 efiboot.c
> --- sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 04:52:22 -0000 1.24
> +++ sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 05:09:29 -0000
> @@ -51,6 +51,7 @@ static EFI_GUID imgp_guid = LOADED_IMA
>  static EFI_GUID blkio_guid = BLOCK_IO_PROTOCOL;
>  static EFI_GUID devp_guid = DEVICE_PATH_PROTOCOL;
>  u_long efi_loadaddr;
> +static int efi_gopmode = -1;
>  
>  static int efi_device_path_depth(EFI_DEVICE_PATH *dp, int);
>  static int efi_device_path_ncmp(EFI_DEVICE_PATH *, EFI_DEVICE_PATH *,
> @@ -722,7 +723,7 @@ efi_makebootargs(void)
>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>   *gopi;
>   bios_efiinfo_t ei;
> - int curmode, bestmode = -1;
> + int curmode;
>   UINTN sz, gopsiz, bestsiz = 0;
>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>   *info;
> @@ -748,20 +749,23 @@ efi_makebootargs(void)
>   status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
>      (void **)&gop);
>   if (!EFI_ERROR(status)) {
> - for (i = 0; i < gop->Mode->MaxMode; i++) {
> - status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
> - if (EFI_ERROR(status))
> - continue;
> - gopsiz = info->HorizontalResolution *
> -    info->VerticalResolution;
> - if (gopsiz > bestsiz) {
> - bestmode = i;
> - bestsiz = gopsiz;
> + if (efi_gopmode < 0) {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop, i, &sz,
> +    &info);
> + if (EFI_ERROR(status))
> + continue;
> + gopsiz = info->HorizontalResolution *
> +    info->VerticalResolution;
> + if (gopsiz > bestsiz) {
> + efi_gopmode = i;
> + bestsiz = gopsiz;
> + }
>   }
>   }
> - if (bestmode >= 0) {
> + if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
>   curmode = gop->Mode->Mode;
> - if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
> + if (efi_gop_setmode(gop, efi_gopmode) != EFI_SUCCESS)
>   (void)efi_gop_setmode(gop, curmode);
>   }
>  
> @@ -882,5 +886,49 @@ int
>  Xpoweroff_efi(void)
>  {
>   EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
> + return (0);
> +}
> +
> +int
> +Xgop_efi(void)
> +{
> + EFI_STATUS status;
> + EFI_GRAPHICS_OUTPUT *gop;
> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
> + *info;
> + int i, mode = -1;
> + UINTN sz;
> +
> + status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
> +    (void **)&gop);
> + if (EFI_ERROR(status))
> + return (0);
> +
> + if (cmd.argc >= 2) {
> + mode = strtol(cmd.argv[1], NULL, 10);
> + if (0 <= mode && mode < gop->Mode->MaxMode) {
> + status = EFI_CALL(gop->QueryMode, gop, mode,
> +    &sz, &info);
> + if (!EFI_ERROR(status)) {
> + if (efi_gop_setmode(gop, mode)
> +    == EFI_SUCCESS)
> + efi_gopmode = mode;
> + }
> + }
> + } else {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop, i, &sz,
> +    &info);
> + if (EFI_ERROR(status))
> + continue;
> + printf("Mode %d: %d x %d (stride = %d)\n", i,
> +    info->HorizontalResolution,
> +    info->VerticalResolution,
> +    info->PixelsPerScanLine);
> + }
> + printf("\n");
> + }
> + printf("Current Mode = %d\n", gop->Mode->Mode);
> +
>   return (0);
>  }
> Index: sys/arch/amd64/stand/efiboot/efiboot.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 efiboot.h
> --- sys/arch/amd64/stand/efiboot/efiboot.h 31 May 2017 08:40:32 -0000 1.2
> +++ sys/arch/amd64/stand/efiboot/efiboot.h 6 Oct 2017 05:09:29 -0000
> @@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
>  int efi_com_getc(dev_t);
>  void efi_com_putc(dev_t, int);
>  int Xvideo_efi(void);
> +int Xgop_efi(void);
>  int Xexit_efi(void);
>  void efi_makebootargs(void);
>  
> Index: sys/arch/amd64/stand/libsa/cmd_i386.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 cmd_i386.c
> --- sys/arch/amd64/stand/libsa/cmd_i386.c 31 May 2017 08:23:33 -0000 1.11
> +++ sys/arch/amd64/stand/libsa/cmd_i386.c 6 Oct 2017 05:09:29 -0000
> @@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
>   { "memory", CMDT_CMD, Xmemory },
>  #ifdef EFIBOOT
>   { "video", CMDT_CMD, Xvideo_efi },
> + { "gop", CMDT_CMD, Xgop_efi },
>   { "exit", CMDT_CMD, Xexit_efi },
>   { "poweroff", CMDT_CMD, Xpoweroff_efi },
>  #endif
>
>


Implement `machine gop [n]' completely analogue to `machine video [n]'
in the bootloader so users can list and set available frame buffer modes.
---
 sys/arch/amd64/stand/efiboot/efiboot.c | 68 ++++++++++++++++++++++++++++------
 sys/arch/amd64/stand/efiboot/efiboot.h |  1 +
 sys/arch/amd64/stand/libsa/cmd_i386.c  |  1 +
 3 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/sys/arch/amd64/stand/efiboot/efiboot.c b/sys/arch/amd64/stand/efiboot/efiboot.c
index 283f9ab356e..cc04b07dd52 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.c
+++ b/sys/arch/amd64/stand/efiboot/efiboot.c
@@ -708,6 +708,7 @@ static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
 static EFI_GRAPHICS_OUTPUT *gop;
 static EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
  *gopi;
+static int efi_gopmode = -1;
 
 #define efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
 
@@ -729,7 +730,7 @@ efi_makebootargs(void)
  int i;
  EFI_STATUS status;
  bios_efiinfo_t ei;
- int curmode, bestmode = -1;
+ int curmode;
  UINTN sz, gopsiz, bestsiz = 0;
 
  memset(&ei, 0, sizeof(ei));
@@ -753,20 +754,23 @@ efi_makebootargs(void)
  status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
     (void **)&gop);
  if (!EFI_ERROR(status)) {
- for (i = 0; i < gop->Mode->MaxMode; i++) {
- status = EFI_CALL(gop->QueryMode, gop, i, &sz, &gopi);
- if (EFI_ERROR(status))
- continue;
- gopsiz = gopi->HorizontalResolution *
-    gopi->VerticalResolution;
- if (gopsiz > bestsiz) {
- bestmode = i;
- bestsiz = gopsiz;
+ if (efi_gopmode < 0) {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop,
+    i, &sz, &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ gopsiz = gopi->HorizontalResolution *
+    gopi->VerticalResolution;
+ if (gopsiz > bestsiz) {
+ efi_gopmode = i;
+ bestsiz = gopsiz;
+ }
  }
  }
- if (bestmode >= 0) {
+ if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
  curmode = gop->Mode->Mode;
- if (efi_gop_setmode(bestmode) != EFI_SUCCESS)
+ if (efi_gop_setmode(efi_gopmode) != EFI_SUCCESS)
  (void)efi_gop_setmode(curmode);
  }
 
@@ -892,3 +896,43 @@ Xpoweroff_efi(void)
  EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
  return (0);
 }
+
+int
+Xgop_efi(void)
+{
+ EFI_STATUS status;
+ int i, mode = -1;
+ UINTN sz;
+
+ status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
+    (void **)&gop);
+ if (EFI_ERROR(status))
+ return (0);
+
+ if (cmd.argc >= 2) {
+ mode = strtol(cmd.argv[1], NULL, 10);
+ if (0 <= mode && mode < gop->Mode->MaxMode) {
+ status = EFI_CALL(gop->QueryMode, gop, mode,
+    &sz, &gopi);
+ if (!EFI_ERROR(status)) {
+ if (efi_gop_setmode(mode) == EFI_SUCCESS)
+ efi_gopmode = mode;
+ }
+ }
+ } else {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop, i, &sz,
+    &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ printf("Mode %d: %d x %d (stride = %d)\n", i,
+    gopi->HorizontalResolution,
+    gopi->VerticalResolution,
+    gopi->PixelsPerScanLine);
+ }
+ printf("\n");
+ }
+ printf("Current Mode = %d\n", gop->Mode->Mode);
+
+ return (0);
+}
diff --git a/sys/arch/amd64/stand/efiboot/efiboot.h b/sys/arch/amd64/stand/efiboot/efiboot.h
index 8a07d5b46b3..3f5f5b3352c 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.h
+++ b/sys/arch/amd64/stand/efiboot/efiboot.h
@@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
 int efi_com_getc(dev_t);
 void efi_com_putc(dev_t, int);
 int Xvideo_efi(void);
+int Xgop_efi(void);
 int Xexit_efi(void);
 void efi_makebootargs(void);
 
diff --git a/sys/arch/amd64/stand/libsa/cmd_i386.c b/sys/arch/amd64/stand/libsa/cmd_i386.c
index 592cca49547..ee3b7f3cd63 100644
--- a/sys/arch/amd64/stand/libsa/cmd_i386.c
+++ b/sys/arch/amd64/stand/libsa/cmd_i386.c
@@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
  { "memory", CMDT_CMD, Xmemory },
 #ifdef EFIBOOT
  { "video", CMDT_CMD, Xvideo_efi },
+ { "gop", CMDT_CMD, Xgop_efi },
  { "exit", CMDT_CMD, Xexit_efi },
  { "poweroff", CMDT_CMD, Xpoweroff_efi },
 #endif
--
2.14.2


Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

YASUOKA Masahiko-4
Hi,

> See my updated diff for reusing the gopi struct, please.

ok, but the diff seems to be against wrong revision.  You seems to
have other diffs, moving gop and gopi to global at least.

Can you send it entirely?

On Sat, 7 Oct 2017 02:09:29 +0200
Klemens Nanni <[hidden email]> wrote:

> On Fri, Oct 06, 2017 at 05:15:20AM +0000, YASUOKA Masahiko wrote:
>> On Tue, 3 Oct 2017 18:15:33 -0500
>> Andrew Daugherity <[hidden email]> wrote:
>> > I tested this diff in combination with your "Implement machine gop command"
>> > diff on a Dell PowerEdge R230 and a VirtualBox VM (EFI enabled).  No
>> > regressions, however 'machine gop' doesn't work quite how I expected it to
>> > -- it seems the video mode set with it only applies to the bootloader, and
>> > once the kernel loads, it sets a different mode (possibly a bad one).  I
>> > was hoping the kernel would use the mode I just set.
>>
>> I also expected that the kernel uses the mode to which the gop command
>> set.  I think it's more useful.
>>
>> The diff is updated.
>>
>> comments?
> See my updated diff for reusing the gopi struct, please.
>
> Searching for and setting a better mode only if the user didn't pick one
> manually before is definitely a good idea, thanks.
>
> Udated diff implementing `machine gop [n]' attached.
>
>> Index: sys/arch/amd64/stand/efiboot/efiboot.c
>> ===================================================================
>> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
>> retrieving revision 1.24
>> diff -u -p -r1.24 efiboot.c
>> --- sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 04:52:22 -0000 1.24
>> +++ sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 05:09:29 -0000
>> @@ -51,6 +51,7 @@ static EFI_GUID imgp_guid = LOADED_IMA
>>  static EFI_GUID blkio_guid = BLOCK_IO_PROTOCOL;
>>  static EFI_GUID devp_guid = DEVICE_PATH_PROTOCOL;
>>  u_long efi_loadaddr;
>> +static int efi_gopmode = -1;
>>  
>>  static int efi_device_path_depth(EFI_DEVICE_PATH *dp, int);
>>  static int efi_device_path_ncmp(EFI_DEVICE_PATH *, EFI_DEVICE_PATH *,
>> @@ -722,7 +723,7 @@ efi_makebootargs(void)
>>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>>   *gopi;
>>   bios_efiinfo_t ei;
>> - int curmode, bestmode = -1;
>> + int curmode;
>>   UINTN sz, gopsiz, bestsiz = 0;
>>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>>   *info;
>> @@ -748,20 +749,23 @@ efi_makebootargs(void)
>>   status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
>>      (void **)&gop);
>>   if (!EFI_ERROR(status)) {
>> - for (i = 0; i < gop->Mode->MaxMode; i++) {
>> - status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
>> - if (EFI_ERROR(status))
>> - continue;
>> - gopsiz = info->HorizontalResolution *
>> -    info->VerticalResolution;
>> - if (gopsiz > bestsiz) {
>> - bestmode = i;
>> - bestsiz = gopsiz;
>> + if (efi_gopmode < 0) {
>> + for (i = 0; i < gop->Mode->MaxMode; i++) {
>> + status = EFI_CALL(gop->QueryMode, gop, i, &sz,
>> +    &info);
>> + if (EFI_ERROR(status))
>> + continue;
>> + gopsiz = info->HorizontalResolution *
>> +    info->VerticalResolution;
>> + if (gopsiz > bestsiz) {
>> + efi_gopmode = i;
>> + bestsiz = gopsiz;
>> + }
>>   }
>>   }
>> - if (bestmode >= 0) {
>> + if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
>>   curmode = gop->Mode->Mode;
>> - if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
>> + if (efi_gop_setmode(gop, efi_gopmode) != EFI_SUCCESS)
>>   (void)efi_gop_setmode(gop, curmode);
>>   }
>>  
>> @@ -882,5 +886,49 @@ int
>>  Xpoweroff_efi(void)
>>  {
>>   EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
>> + return (0);
>> +}
>> +
>> +int
>> +Xgop_efi(void)
>> +{
>> + EFI_STATUS status;
>> + EFI_GRAPHICS_OUTPUT *gop;
>> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>> + *info;
>> + int i, mode = -1;
>> + UINTN sz;
>> +
>> + status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
>> +    (void **)&gop);
>> + if (EFI_ERROR(status))
>> + return (0);
>> +
>> + if (cmd.argc >= 2) {
>> + mode = strtol(cmd.argv[1], NULL, 10);
>> + if (0 <= mode && mode < gop->Mode->MaxMode) {
>> + status = EFI_CALL(gop->QueryMode, gop, mode,
>> +    &sz, &info);
>> + if (!EFI_ERROR(status)) {
>> + if (efi_gop_setmode(gop, mode)
>> +    == EFI_SUCCESS)
>> + efi_gopmode = mode;
>> + }
>> + }
>> + } else {
>> + for (i = 0; i < gop->Mode->MaxMode; i++) {
>> + status = EFI_CALL(gop->QueryMode, gop, i, &sz,
>> +    &info);
>> + if (EFI_ERROR(status))
>> + continue;
>> + printf("Mode %d: %d x %d (stride = %d)\n", i,
>> +    info->HorizontalResolution,
>> +    info->VerticalResolution,
>> +    info->PixelsPerScanLine);
>> + }
>> + printf("\n");
>> + }
>> + printf("Current Mode = %d\n", gop->Mode->Mode);
>> +
>>   return (0);
>>  }
>> Index: sys/arch/amd64/stand/efiboot/efiboot.h
>> ===================================================================
>> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.h,v
>> retrieving revision 1.2
>> diff -u -p -r1.2 efiboot.h
>> --- sys/arch/amd64/stand/efiboot/efiboot.h 31 May 2017 08:40:32 -0000 1.2
>> +++ sys/arch/amd64/stand/efiboot/efiboot.h 6 Oct 2017 05:09:29 -0000
>> @@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
>>  int efi_com_getc(dev_t);
>>  void efi_com_putc(dev_t, int);
>>  int Xvideo_efi(void);
>> +int Xgop_efi(void);
>>  int Xexit_efi(void);
>>  void efi_makebootargs(void);
>>  
>> Index: sys/arch/amd64/stand/libsa/cmd_i386.c
>> ===================================================================
>> RCS file: /cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 cmd_i386.c
>> --- sys/arch/amd64/stand/libsa/cmd_i386.c 31 May 2017 08:23:33 -0000 1.11
>> +++ sys/arch/amd64/stand/libsa/cmd_i386.c 6 Oct 2017 05:09:29 -0000
>> @@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
>>   { "memory", CMDT_CMD, Xmemory },
>>  #ifdef EFIBOOT
>>   { "video", CMDT_CMD, Xvideo_efi },
>> + { "gop", CMDT_CMD, Xgop_efi },
>>   { "exit", CMDT_CMD, Xexit_efi },
>>   { "poweroff", CMDT_CMD, Xpoweroff_efi },
>>  #endif
>>
>>
>
>
> Implement `machine gop [n]' completely analogue to `machine video [n]'
> in the bootloader so users can list and set available frame buffer modes.
> ---
>  sys/arch/amd64/stand/efiboot/efiboot.c | 68 ++++++++++++++++++++++++++++------
>  sys/arch/amd64/stand/efiboot/efiboot.h |  1 +
>  sys/arch/amd64/stand/libsa/cmd_i386.c  |  1 +
>  3 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/sys/arch/amd64/stand/efiboot/efiboot.c b/sys/arch/amd64/stand/efiboot/efiboot.c
> index 283f9ab356e..cc04b07dd52 100644
> --- a/sys/arch/amd64/stand/efiboot/efiboot.c
> +++ b/sys/arch/amd64/stand/efiboot/efiboot.c
> @@ -708,6 +708,7 @@ static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
>  static EFI_GRAPHICS_OUTPUT *gop;
>  static EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>   *gopi;
> +static int efi_gopmode = -1;
>  
>  #define efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
>  
> @@ -729,7 +730,7 @@ efi_makebootargs(void)
>   int i;
>   EFI_STATUS status;
>   bios_efiinfo_t ei;
> - int curmode, bestmode = -1;
> + int curmode;
>   UINTN sz, gopsiz, bestsiz = 0;
>  
>   memset(&ei, 0, sizeof(ei));
> @@ -753,20 +754,23 @@ efi_makebootargs(void)
>   status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
>      (void **)&gop);
>   if (!EFI_ERROR(status)) {
> - for (i = 0; i < gop->Mode->MaxMode; i++) {
> - status = EFI_CALL(gop->QueryMode, gop, i, &sz, &gopi);
> - if (EFI_ERROR(status))
> - continue;
> - gopsiz = gopi->HorizontalResolution *
> -    gopi->VerticalResolution;
> - if (gopsiz > bestsiz) {
> - bestmode = i;
> - bestsiz = gopsiz;
> + if (efi_gopmode < 0) {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop,
> +    i, &sz, &gopi);
> + if (EFI_ERROR(status))
> + continue;
> + gopsiz = gopi->HorizontalResolution *
> +    gopi->VerticalResolution;
> + if (gopsiz > bestsiz) {
> + efi_gopmode = i;
> + bestsiz = gopsiz;
> + }
>   }
>   }
> - if (bestmode >= 0) {
> + if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
>   curmode = gop->Mode->Mode;
> - if (efi_gop_setmode(bestmode) != EFI_SUCCESS)
> + if (efi_gop_setmode(efi_gopmode) != EFI_SUCCESS)
>   (void)efi_gop_setmode(curmode);
>   }
>  
> @@ -892,3 +896,43 @@ Xpoweroff_efi(void)
>   EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
>   return (0);
>  }
> +
> +int
> +Xgop_efi(void)
> +{
> + EFI_STATUS status;
> + int i, mode = -1;
> + UINTN sz;
> +
> + status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
> +    (void **)&gop);
> + if (EFI_ERROR(status))
> + return (0);
> +
> + if (cmd.argc >= 2) {
> + mode = strtol(cmd.argv[1], NULL, 10);
> + if (0 <= mode && mode < gop->Mode->MaxMode) {
> + status = EFI_CALL(gop->QueryMode, gop, mode,
> +    &sz, &gopi);
> + if (!EFI_ERROR(status)) {
> + if (efi_gop_setmode(mode) == EFI_SUCCESS)
> + efi_gopmode = mode;
> + }
> + }
> + } else {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop, i, &sz,
> +    &gopi);
> + if (EFI_ERROR(status))
> + continue;
> + printf("Mode %d: %d x %d (stride = %d)\n", i,
> +    gopi->HorizontalResolution,
> +    gopi->VerticalResolution,
> +    gopi->PixelsPerScanLine);
> + }
> + printf("\n");
> + }
> + printf("Current Mode = %d\n", gop->Mode->Mode);
> +
> + return (0);
> +}
> diff --git a/sys/arch/amd64/stand/efiboot/efiboot.h b/sys/arch/amd64/stand/efiboot/efiboot.h
> index 8a07d5b46b3..3f5f5b3352c 100644
> --- a/sys/arch/amd64/stand/efiboot/efiboot.h
> +++ b/sys/arch/amd64/stand/efiboot/efiboot.h
> @@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
>  int efi_com_getc(dev_t);
>  void efi_com_putc(dev_t, int);
>  int Xvideo_efi(void);
> +int Xgop_efi(void);
>  int Xexit_efi(void);
>  void efi_makebootargs(void);
>  
> diff --git a/sys/arch/amd64/stand/libsa/cmd_i386.c b/sys/arch/amd64/stand/libsa/cmd_i386.c
> index 592cca49547..ee3b7f3cd63 100644
> --- a/sys/arch/amd64/stand/libsa/cmd_i386.c
> +++ b/sys/arch/amd64/stand/libsa/cmd_i386.c
> @@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
>   { "memory", CMDT_CMD, Xmemory },
>  #ifdef EFIBOOT
>   { "video", CMDT_CMD, Xvideo_efi },
> + { "gop", CMDT_CMD, Xgop_efi },
>   { "exit", CMDT_CMD, Xexit_efi },
>   { "poweroff", CMDT_CMD, Xpoweroff_efi },
>  #endif
> --
> 2.14.2
>
>

Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

Klemens Nanni
On Sat, Oct 07, 2017 at 01:15:40AM +0000, YASUOKA Masahiko wrote:
> Hi,
>
> > See my updated diff for reusing the gopi struct, please.
>
> ok, but the diff seems to be against wrong revision.  You seems to
> have other diffs, moving gop and gopi to global at least.
>
> Can you send it entirely?
My bad, those were applied on top of other (unsubmitted) diffs such as
https://marc.info/?l=openbsd-tech&m=150437080106164

The following diff against the latest revision only includes the
`machine gop' bits as well as reusing/declaring gop(i) globally.

diff --git a/sys/arch/amd64/stand/efiboot/efiboot.c b/sys/arch/amd64/stand/efiboot/efiboot.c
index 7151e22a16a..ebf441b647e 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.c
+++ b/sys/arch/amd64/stand/efiboot/efiboot.c
@@ -60,7 +60,7 @@ static void efi_memprobe_internal(void);
 static void efi_video_init(void);
 static void efi_video_reset(void);
 static EFI_STATUS
- efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode);
+ efi_gop_setmode(int mode);
 EFI_STATUS efi_main(EFI_HANDLE, EFI_SYSTEM_TABLE *);
 
 void (*run_i386)(u_long, u_long, int, int, int, int, int, int, int, int)
@@ -696,13 +696,17 @@ efi_com_putc(dev_t dev, int c)
  * {EFI_,}_ACPI_20_TABLE_GUID or EFI_ACPI_TABLE_GUID means
  * ACPI 2.0 or above.
  */
-static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
-static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
+static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
+static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
+static EFI_GRAPHICS_OUTPUT *gop;
+static EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
+ *gopi;
+static int efi_gopmode = -1;
 
 #define efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
 
 static EFI_STATUS
-efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode)
+efi_gop_setmode(int mode)
 {
  EFI_STATUS status;
 
@@ -718,14 +722,9 @@ efi_makebootargs(void)
 {
  int i;
  EFI_STATUS status;
- EFI_GRAPHICS_OUTPUT *gop;
- EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
- *gopi;
  bios_efiinfo_t ei;
- int curmode, bestmode = -1;
+ int curmode;
  UINTN sz, gopsiz, bestsiz = 0;
- EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
- *info;
 
  memset(&ei, 0, sizeof(ei));
  /*
@@ -748,21 +747,24 @@ efi_makebootargs(void)
  status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
     (void **)&gop);
  if (!EFI_ERROR(status)) {
- for (i = 0; i < gop->Mode->MaxMode; i++) {
- status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
- if (EFI_ERROR(status))
- continue;
- gopsiz = info->HorizontalResolution *
-    info->VerticalResolution;
- if (gopsiz > bestsiz) {
- bestmode = i;
- bestsiz = gopsiz;
+ if (efi_gopmode < 0) {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop,
+    i, &sz, &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ gopsiz = gopi->HorizontalResolution *
+    gopi->VerticalResolution;
+ if (gopsiz > bestsiz) {
+ efi_gopmode = i;
+ bestsiz = gopsiz;
+ }
  }
  }
- if (bestmode >= 0) {
+ if (efi_gopmode >= 0 && efi_gopmode != gop->Mode->Mode) {
  curmode = gop->Mode->Mode;
- if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
- (void)efi_gop_setmode(gop, curmode);
+ if (efi_gop_setmode(efi_gopmode) != EFI_SUCCESS)
+ (void)efi_gop_setmode(curmode);
  }
 
  gopi = gop->Mode->Info;
@@ -884,3 +886,43 @@ Xpoweroff_efi(void)
  EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
  return (0);
 }
+
+int
+Xgop_efi(void)
+{
+ EFI_STATUS status;
+ int i, mode = -1;
+ UINTN sz;
+
+ status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
+    (void **)&gop);
+ if (EFI_ERROR(status))
+ return (0);
+
+ if (cmd.argc >= 2) {
+ mode = strtol(cmd.argv[1], NULL, 10);
+ if (0 <= mode && mode < gop->Mode->MaxMode) {
+ status = EFI_CALL(gop->QueryMode, gop, mode,
+    &sz, &gopi);
+ if (!EFI_ERROR(status)) {
+ if (efi_gop_setmode(mode) == EFI_SUCCESS)
+ efi_gopmode = mode;
+ }
+ }
+ } else {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop, i, &sz,
+    &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ printf("Mode %d: %d x %d (stride = %d)\n", i,
+    gopi->HorizontalResolution,
+    gopi->VerticalResolution,
+    gopi->PixelsPerScanLine);
+ }
+ printf("\n");
+ }
+ printf("Current Mode = %d\n", gop->Mode->Mode);
+
+ return (0);
+}
diff --git a/sys/arch/amd64/stand/efiboot/efiboot.h b/sys/arch/amd64/stand/efiboot/efiboot.h
index 8a07d5b46b3..3f5f5b3352c 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.h
+++ b/sys/arch/amd64/stand/efiboot/efiboot.h
@@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
 int efi_com_getc(dev_t);
 void efi_com_putc(dev_t, int);
 int Xvideo_efi(void);
+int Xgop_efi(void);
 int Xexit_efi(void);
 void efi_makebootargs(void);
 
diff --git a/sys/arch/amd64/stand/libsa/cmd_i386.c b/sys/arch/amd64/stand/libsa/cmd_i386.c
index 592cca49547..ee3b7f3cd63 100644
--- a/sys/arch/amd64/stand/libsa/cmd_i386.c
+++ b/sys/arch/amd64/stand/libsa/cmd_i386.c
@@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
  { "memory", CMDT_CMD, Xmemory },
 #ifdef EFIBOOT
  { "video", CMDT_CMD, Xvideo_efi },
+ { "gop", CMDT_CMD, Xgop_efi },
  { "exit", CMDT_CMD, Xexit_efi },
  { "poweroff", CMDT_CMD, Xpoweroff_efi },
 #endif

Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

YASUOKA Masahiko-3
On Sat, 7 Oct 2017 09:24:20 +0200
Klemens Nanni <[hidden email]> wrote:
> On Sat, Oct 07, 2017 at 01:15:40AM +0000, YASUOKA Masahiko wrote:
>> > See my updated diff for reusing the gopi struct, please.
>>
>> ok, but the diff seems to be against wrong revision.  You seems to
>> have other diffs, moving gop and gopi to global at least.
>>
>> Can you send it entirely?
> My bad, those were applied on top of other (unsubmitted) diffs such as
> https://marc.info/?l=openbsd-tech&m=150437080106164

Ah, sorry I missed that mail.

On Fri, 6 Oct 2017 20:11:24 +0200
Klemens Nanni <[hidden email]> wrote:
> Declaring the gop and gopi strucutures globally makes things easier in
> preparation for the next commit.
>
> Both changes also improve consistency with regard to other structures
> like ei, di and conout as well.

As for gopi, I'd like to keep it be a local since it is used to refer
various different modes.

also s/efi_gopmode/gopmode/

comments?

Index: sys/arch/amd64/stand/efiboot/efiboot.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
retrieving revision 1.24
diff -u -p -r1.24 efiboot.c
--- sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 04:52:22 -0000 1.24
+++ sys/arch/amd64/stand/efiboot/efiboot.c 10 Oct 2017 02:52:46 -0000
@@ -60,7 +60,7 @@ static void efi_memprobe_internal(void)
 static void efi_video_init(void);
 static void efi_video_reset(void);
 static EFI_STATUS
- efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode);
+ efi_gop_setmode(int mode);
 EFI_STATUS efi_main(EFI_HANDLE, EFI_SYSTEM_TABLE *);
 
 void (*run_i386)(u_long, u_long, int, int, int, int, int, int, int, int)
@@ -696,13 +696,15 @@ efi_com_putc(dev_t dev, int c)
  * {EFI_,}_ACPI_20_TABLE_GUID or EFI_ACPI_TABLE_GUID means
  * ACPI 2.0 or above.
  */
-static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
-static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
+static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
+static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
+static EFI_GRAPHICS_OUTPUT *gop;
+static int gopmode = -1;
 
 #define efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
 
 static EFI_STATUS
-efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode)
+efi_gop_setmode(int mode)
 {
  EFI_STATUS status;
 
@@ -718,14 +720,11 @@ efi_makebootargs(void)
 {
  int i;
  EFI_STATUS status;
- EFI_GRAPHICS_OUTPUT *gop;
  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
  *gopi;
  bios_efiinfo_t ei;
- int curmode, bestmode = -1;
+ int curmode;
  UINTN sz, gopsiz, bestsiz = 0;
- EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
- *info;
 
  memset(&ei, 0, sizeof(ei));
  /*
@@ -748,21 +747,24 @@ efi_makebootargs(void)
  status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
     (void **)&gop);
  if (!EFI_ERROR(status)) {
- for (i = 0; i < gop->Mode->MaxMode; i++) {
- status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
- if (EFI_ERROR(status))
- continue;
- gopsiz = info->HorizontalResolution *
-    info->VerticalResolution;
- if (gopsiz > bestsiz) {
- bestmode = i;
- bestsiz = gopsiz;
+ if (gopmode < 0) {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop,
+    i, &sz, &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ gopsiz = gopi->HorizontalResolution *
+    gopi->VerticalResolution;
+ if (gopsiz > bestsiz) {
+ gopmode = i;
+ bestsiz = gopsiz;
+ }
  }
  }
- if (bestmode >= 0) {
+ if (gopmode >= 0 && gopmode != gop->Mode->Mode) {
  curmode = gop->Mode->Mode;
- if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
- (void)efi_gop_setmode(gop, curmode);
+ if (efi_gop_setmode(gopmode) != EFI_SUCCESS)
+ (void)efi_gop_setmode(curmode);
  }
 
  gopi = gop->Mode->Info;
@@ -882,5 +884,46 @@ int
 Xpoweroff_efi(void)
 {
  EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
+ return (0);
+}
+
+int
+Xgop_efi(void)
+{
+ EFI_STATUS status;
+ int i, mode = -1;
+ UINTN sz;
+ EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
+ *gopi;
+
+ status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
+    (void **)&gop);
+ if (EFI_ERROR(status))
+ return (0);
+
+ if (cmd.argc >= 2) {
+ mode = strtol(cmd.argv[1], NULL, 10);
+ if (0 <= mode && mode < gop->Mode->MaxMode) {
+ status = EFI_CALL(gop->QueryMode, gop, mode,
+    &sz, &gopi);
+ if (!EFI_ERROR(status)) {
+ if (efi_gop_setmode(mode) == EFI_SUCCESS)
+ gopmode = mode;
+ }
+ }
+ } else {
+ for (i = 0; i < gop->Mode->MaxMode; i++) {
+ status = EFI_CALL(gop->QueryMode, gop, i, &sz, &gopi);
+ if (EFI_ERROR(status))
+ continue;
+ printf("Mode %d: %d x %d (stride = %d)\n", i,
+    gopi->HorizontalResolution,
+    gopi->VerticalResolution,
+    gopi->PixelsPerScanLine);
+ }
+ printf("\n");
+ }
+ printf("Current Mode = %d\n", gop->Mode->Mode);
+
  return (0);
 }
Index: sys/arch/amd64/stand/efiboot/efiboot.h
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.h,v
retrieving revision 1.2
diff -u -p -r1.2 efiboot.h
--- sys/arch/amd64/stand/efiboot/efiboot.h 31 May 2017 08:40:32 -0000 1.2
+++ sys/arch/amd64/stand/efiboot/efiboot.h 10 Oct 2017 02:52:47 -0000
@@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
 int efi_com_getc(dev_t);
 void efi_com_putc(dev_t, int);
 int Xvideo_efi(void);
+int Xgop_efi(void);
 int Xexit_efi(void);
 void efi_makebootargs(void);
 
Index: sys/arch/amd64/stand/libsa/cmd_i386.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
retrieving revision 1.11
diff -u -p -r1.11 cmd_i386.c
--- sys/arch/amd64/stand/libsa/cmd_i386.c 31 May 2017 08:23:33 -0000 1.11
+++ sys/arch/amd64/stand/libsa/cmd_i386.c 10 Oct 2017 02:52:47 -0000
@@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
  { "memory", CMDT_CMD, Xmemory },
 #ifdef EFIBOOT
  { "video", CMDT_CMD, Xvideo_efi },
+ { "gop", CMDT_CMD, Xgop_efi },
  { "exit", CMDT_CMD, Xexit_efi },
  { "poweroff", CMDT_CMD, Xpoweroff_efi },
 #endif



Reply | Threaded
Open this post in threaded view
|

Re: efiboot: Restore GOP mode on SetMode() failure

Klemens Nanni
On Tue, Oct 10, 2017 at 02:56:26AM +0000, YASUOKA Masahiko wrote:

> On Sat, 7 Oct 2017 09:24:20 +0200
> Klemens Nanni <[hidden email]> wrote:
> > On Sat, Oct 07, 2017 at 01:15:40AM +0000, YASUOKA Masahiko wrote:
> >> > See my updated diff for reusing the gopi struct, please.
> >>
> >> ok, but the diff seems to be against wrong revision.  You seems to
> >> have other diffs, moving gop and gopi to global at least.
> >>
> >> Can you send it entirely?
> > My bad, those were applied on top of other (unsubmitted) diffs such as
> > https://marc.info/?l=openbsd-tech&m=150437080106164
>
> Ah, sorry I missed that mail.
>
> On Fri, 6 Oct 2017 20:11:24 +0200
> Klemens Nanni <[hidden email]> wrote:
> > Declaring the gop and gopi strucutures globally makes things easier in
> > preparation for the next commit.
> >
> > Both changes also improve consistency with regard to other structures
> > like ei, di and conout as well.
>
> As for gopi, I'd like to keep it be a local since it is used to refer
> various different modes.
That's fine.

> also s/efi_gopmode/gopmode/
I adapted efi_gopmode from your previous patch.

> comments?
>
> Index: sys/arch/amd64/stand/efiboot/efiboot.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 efiboot.c
> --- sys/arch/amd64/stand/efiboot/efiboot.c 6 Oct 2017 04:52:22 -0000 1.24
> +++ sys/arch/amd64/stand/efiboot/efiboot.c 10 Oct 2017 02:52:46 -0000
> @@ -60,7 +60,7 @@ static void efi_memprobe_internal(void)
>  static void efi_video_init(void);
>  static void efi_video_reset(void);
>  static EFI_STATUS
> - efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode);
> + efi_gop_setmode(int mode);
>  EFI_STATUS efi_main(EFI_HANDLE, EFI_SYSTEM_TABLE *);
>  
>  void (*run_i386)(u_long, u_long, int, int, int, int, int, int, int, int)
> @@ -696,13 +696,15 @@ efi_com_putc(dev_t dev, int c)
>   * {EFI_,}_ACPI_20_TABLE_GUID or EFI_ACPI_TABLE_GUID means
>   * ACPI 2.0 or above.
>   */
> -static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
> -static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
> +static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
> +static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
> +static EFI_GRAPHICS_OUTPUT *gop;
> +static int gopmode = -1;
>  
>  #define efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
>  
>  static EFI_STATUS
> -efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode)
> +efi_gop_setmode(int mode)
>  {
>   EFI_STATUS status;
>  
> @@ -718,14 +720,11 @@ efi_makebootargs(void)
>  {
>   int i;
>   EFI_STATUS status;
> - EFI_GRAPHICS_OUTPUT *gop;
>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>   *gopi;
>   bios_efiinfo_t ei;
> - int curmode, bestmode = -1;
> + int curmode;
>   UINTN sz, gopsiz, bestsiz = 0;
> - EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
> - *info;
>  
>   memset(&ei, 0, sizeof(ei));
>   /*
> @@ -748,21 +747,24 @@ efi_makebootargs(void)
>   status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
>      (void **)&gop);
>   if (!EFI_ERROR(status)) {
> - for (i = 0; i < gop->Mode->MaxMode; i++) {
> - status = EFI_CALL(gop->QueryMode, gop, i, &sz, &info);
> - if (EFI_ERROR(status))
> - continue;
> - gopsiz = info->HorizontalResolution *
> -    info->VerticalResolution;
> - if (gopsiz > bestsiz) {
> - bestmode = i;
> - bestsiz = gopsiz;
> + if (gopmode < 0) {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop,
> +    i, &sz, &gopi);
> + if (EFI_ERROR(status))
> + continue;
> + gopsiz = gopi->HorizontalResolution *
> +    gopi->VerticalResolution;
> + if (gopsiz > bestsiz) {
> + gopmode = i;
> + bestsiz = gopsiz;
> + }
>   }
>   }
> - if (bestmode >= 0) {
> + if (gopmode >= 0 && gopmode != gop->Mode->Mode) {
>   curmode = gop->Mode->Mode;
> - if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
> - (void)efi_gop_setmode(gop, curmode);
> + if (efi_gop_setmode(gopmode) != EFI_SUCCESS)
> + (void)efi_gop_setmode(curmode);
>   }
>  
>   gopi = gop->Mode->Info;
> @@ -882,5 +884,46 @@ int
>  Xpoweroff_efi(void)
>  {
>   EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL);
> + return (0);
> +}
> +
> +int
> +Xgop_efi(void)
> +{
> + EFI_STATUS status;
> + int i, mode = -1;
> + UINTN sz;
> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
> + *gopi;
> +
> + status = EFI_CALL(BS->LocateProtocol, &gop_guid, NULL,
> +    (void **)&gop);
> + if (EFI_ERROR(status))
> + return (0);
> +
> + if (cmd.argc >= 2) {
> + mode = strtol(cmd.argv[1], NULL, 10);
> + if (0 <= mode && mode < gop->Mode->MaxMode) {
> + status = EFI_CALL(gop->QueryMode, gop, mode,
> +    &sz, &gopi);
> + if (!EFI_ERROR(status)) {
> + if (efi_gop_setmode(mode) == EFI_SUCCESS)
> + gopmode = mode;
> + }
> + }
> + } else {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop, i, &sz, &gopi);
> + if (EFI_ERROR(status))
> + continue;
> + printf("Mode %d: %d x %d (stride = %d)\n", i,
> +    gopi->HorizontalResolution,
> +    gopi->VerticalResolution,
> +    gopi->PixelsPerScanLine);
> + }
> + printf("\n");
> + }
> + printf("Current Mode = %d\n", gop->Mode->Mode);
> +
>   return (0);
>  }
> Index: sys/arch/amd64/stand/efiboot/efiboot.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 efiboot.h
> --- sys/arch/amd64/stand/efiboot/efiboot.h 31 May 2017 08:40:32 -0000 1.2
> +++ sys/arch/amd64/stand/efiboot/efiboot.h 10 Oct 2017 02:52:47 -0000
> @@ -30,6 +30,7 @@ void efi_com_init(struct consdev *);
>  int efi_com_getc(dev_t);
>  void efi_com_putc(dev_t, int);
>  int Xvideo_efi(void);
> +int Xgop_efi(void);
>  int Xexit_efi(void);
>  void efi_makebootargs(void);
>  
> Index: sys/arch/amd64/stand/libsa/cmd_i386.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 cmd_i386.c
> --- sys/arch/amd64/stand/libsa/cmd_i386.c 31 May 2017 08:23:33 -0000 1.11
> +++ sys/arch/amd64/stand/libsa/cmd_i386.c 10 Oct 2017 02:52:47 -0000
> @@ -62,6 +62,7 @@ const struct cmd_table cmd_machine[] = {
>   { "memory", CMDT_CMD, Xmemory },
>  #ifdef EFIBOOT
>   { "video", CMDT_CMD, Xvideo_efi },
> + { "gop", CMDT_CMD, Xgop_efi },
>   { "exit", CMDT_CMD, Xexit_efi },
>   { "poweroff", CMDT_CMD, Xpoweroff_efi },
>  #endif
>
>
>
Good :)