[PATCH] gfxpayload

[PATCH] gfxpayload

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

Hello. Here is a long-discussed gfxpayload patch. To start linux in
1024x768 mode do:
insmod vbe
gfxmode=1024x768
linux ...
...
boot
Multiple modes can be separated by either comma or semicolon. First
mode can be "keep" which will make the system keep the current mode if
accepted by kernel. Another special value is "text" which will put in
text mode.
Can someone check that vga= backward compatibility code interprets
values correctly?
With linux for a reason I don't know it results in black screen in
qemu. But perhaps it's a problem with my kernel
Now it's the same syntax for xnu too. Just remember that current xnu
loader support 32-bit modes only

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: gfxpayload.diff --]
[-- Type: text/x-patch, Size: 27838 bytes --]

diff --git a/commands/videotest.c b/commands/videotest.c
index db7f704..6fe4b9b 100644
--- a/commands/videotest.c
+++ b/commands/videotest.c
@@ -30,8 +30,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ ((unused)),
                     int argc __attribute__ ((unused)),
                     char **args __attribute__ ((unused)))
 {
-  if (grub_video_setup (1024, 768,
-                        GRUB_VIDEO_MODE_TYPE_INDEX_COLOR) != GRUB_ERR_NONE)
+  if (grub_video_set_mode ("1024x768;800x600;640x480", 0) != GRUB_ERR_NONE)
     return grub_errno;
 
   grub_video_color_t color;
diff --git a/include/grub/video.h b/include/grub/video.h
index cb73dae..3b27655 100644
--- a/include/grub/video.h
+++ b/include/grub/video.h
@@ -34,6 +34,7 @@ struct grub_video_render_target;
 struct grub_video_bitmap;
 
 /* Defines used to describe video mode or rendering target.  */
+#define GRUB_VIDEO_MODE_TYPE_PURE_TEXT		0x00000040
 #define GRUB_VIDEO_MODE_TYPE_ALPHA		0x00000020
 #define GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED	0x00000010
 #define GRUB_VIDEO_MODE_TYPE_1BIT_BITMAP	0x00000004
@@ -236,9 +237,6 @@ void grub_video_register (grub_video_adapter_t adapter);
 void grub_video_unregister (grub_video_adapter_t adapter);
 void grub_video_iterate (int (*hook) (grub_video_adapter_t adapter));
 
-grub_err_t grub_video_setup (unsigned int width, unsigned int height,
-                             unsigned int mode_type);
-
 grub_err_t grub_video_restore (void);
 
 grub_err_t grub_video_get_info (struct grub_video_mode_info *mode_info);
@@ -299,4 +297,8 @@ grub_err_t grub_video_set_active_render_target (struct grub_video_render_target
 
 grub_err_t grub_video_get_active_render_target (struct grub_video_render_target **target);
 
+grub_err_t grub_video_set_mode (char *modestring,
+				int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+							      struct grub_video_mode_info *mode_info));
+
 #endif /* ! GRUB_VIDEO_HEADER */
diff --git a/loader/i386/linux.c b/loader/i386/linux.c
index 8a7288e..a40690b 100644
--- a/loader/i386/linux.c
+++ b/loader/i386/linux.c
@@ -44,7 +44,9 @@
    GRUB and Linux (saving boot time and visual glitches).  Official GRUB, OTOH,
    needs to be conservative.  */
 #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
-#define GRUB_ASSUME_LINUX_HAS_FB_SUPPORT 0
+#define DEFAULT_VIDEO_MODE "text"
+#else
+#define DEFAULT_VIDEO_MODE "keep"
 #endif
 
 static grub_dl_t my_mod;
@@ -94,8 +96,7 @@ static struct idt_descriptor idt_desc =
     0
   };
 
-static grub_uint16_t vid_mode;
-
+#ifdef GRUB_MACHINE_PCBIOS
 struct linux_vesafb_res
 {
   grub_uint16_t width;
@@ -262,6 +263,7 @@ struct linux_vesafb_mode linux_vesafb_modes[] =
     { VGA_800_500, 24 },	/* 0x372 */
     { VGA_800_500, 32 },	/* 0x373 */
   };
+#endif
 
 static inline grub_size_t
 page_align (grub_size_t size)
@@ -445,48 +447,32 @@ grub_linux_boot (void)
 {
   struct linux_kernel_params *params;
   int e820_num;
-  
+  grub_err_t err;
+  char *modevar, *tmp;
+
   params = real_mode_mem;
 
-  if (vid_mode == GRUB_LINUX_VID_MODE_NORMAL || vid_mode == GRUB_LINUX_VID_MODE_EXTENDED)
-    grub_video_restore ();
-  else if (vid_mode)
+  modevar = grub_env_get ("gfxpayload");
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0);
+  else
     {
-      struct linux_vesafb_mode *linux_mode;
-      int depth, flags;
-      
-      flags = 0;
-      linux_mode = &linux_vesafb_modes[vid_mode - GRUB_LINUX_VID_MODE_VESA_START];
-      depth = linux_mode->depth;
-      
-      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-      if ((depth <= 8) && (depth != -1))
-	flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-      
-      /* We have more than 8 bits, then assume that it is RGB color mode.  */
-      if (depth > 8)
-	flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-      
-      /* If user requested specific depth, forward that information to driver.  */
-      if (depth != -1)
-	flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-	  & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
-      
-      /* Try to initialize requested mode.  */
-      if (grub_video_setup (linux_vesafb_res[linux_mode->res_index].width,
-			    linux_vesafb_res[linux_mode->res_index].height,
-			    flags) != GRUB_ERR_NONE)
-	{
-	  grub_printf ("Unable to initialize requested video mode (vga=0x%x)\n", vid_mode);
-	  return grub_errno;
-	}
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      if (! tmp)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			   "couldn't allocate temporary storag");
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, 0);
+      grub_free (tmp);
+    }
+
+  if (err)
+    {
+      grub_print_error ();
+      grub_printf ("Booting however\n");
+      grub_errno = GRUB_ERR_NONE;
     }
-#if ! GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
-  else
-    /* If user didn't request a video mode, and we can't assume Linux supports FB,
-       then we go back to text mode.  */
-    grub_video_restore ();
-#endif
 
   if (! grub_linux_setup_video (params))
     params->have_vga = GRUB_VIDEO_TYPE_VLFB;
@@ -713,7 +699,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 	       (unsigned) real_size, (unsigned) prot_size);
 
   /* Look for memory size and video mode specified on the command line.  */
-  vid_mode = 0;
   linux_mem_size = 0;
   for (i = 1; i < argc; i++)
 #ifdef GRUB_MACHINE_PCBIOS
@@ -721,11 +706,15 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       {
 	/* Video mode selection support.  */
 	char *val = argv[i] + 4;
+	unsigned vid_mode = 0;
+	struct linux_vesafb_mode *linux_mode;
+	grub_err_t err;
+	char *buf;
 
 	if (grub_strcmp (val, "normal") == 0)
-	  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
+	  vid_mode = 0;
 	else if (grub_strcmp (val, "ext") == 0)
-	  vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
+	  vid_mode = 1;
 	else if (grub_strcmp (val, "ask") == 0)
 	  {
 	    grub_printf ("Legacy `ask' parameter no longer supported.\n");
@@ -742,21 +731,41 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 	switch (vid_mode)
 	  {
 	  case 0:
-	    vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
+	    grub_env_set ("gfxpayload", "keep");
 	    break;
 	  case 1:
-	    vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
+	    grub_env_set ("gfxpayload", "keep");
 	    break;
 	  default:
 	    /* Ignore invalid values.  */
 	    if (vid_mode < GRUB_LINUX_VID_MODE_VESA_START ||
 		vid_mode >= GRUB_LINUX_VID_MODE_VESA_START +
 		ARRAY_SIZE (linux_vesafb_modes))
-	      vid_mode = 0;
-	  }
+	      {
+		grub_env_set ("gfxpayload", "keep");
+		break;
+	      }
 
-	if (grub_errno)
-	  goto fail;
+	    buf = grub_malloc (20);
+	    if (! buf)
+	      {
+		grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			    "couldn't allocate temporary storage");
+		goto fail;
+	      }
+	    
+	    linux_mode 
+	      = &linux_vesafb_modes[vid_mode - GRUB_LINUX_VID_MODE_VESA_START];
+	    
+	    grub_sprintf (buf, "%dx%dx%d", 
+			  linux_vesafb_res[linux_mode->res_index].width,
+			  linux_vesafb_res[linux_mode->res_index].height,
+			  linux_mode->depth);
+	    err = grub_env_set ("gfxpayload", buf);
+	    grub_free (buf);
+	    if (err)
+	      goto fail;
+	  }
       }
     else
 #endif /* GRUB_MACHINE_PCBIOS */
diff --git a/loader/i386/pc/xnu.c b/loader/i386/pc/xnu.c
index 1cca138..d32f679 100644
--- a/loader/i386/pc/xnu.c
+++ b/loader/i386/pc/xnu.c
@@ -19,6 +19,7 @@
 #include <grub/env.h>
 #include <grub/misc.h>
 #include <grub/xnu.h>
+#include <grub/mm.h>
 #include <grub/cpu/xnu.h>
 #include <grub/machine/vbe.h>
 #include <grub/machine/vga.h>
@@ -26,6 +27,17 @@
 #define min(a,b) (((a) < (b)) ? (a) : (b))
 #define max(a,b) (((a) > (b)) ? (a) : (b))
 
+#define DEFAULT_VIDEO_MODE "1024x768x32,800x600x32,640x480x32"
+
+static int NESTED_FUNC_ATTR video_hook (grub_video_adapter_t p __attribute__ ((unused)),
+					struct grub_video_mode_info *info)
+{
+  if (info->mode_type & GRUB_VIDEO_MODE_TYPE_PURE_TEXT)
+    return 0;
+
+  return 1;
+}
+
 /* Setup video for xnu. */
 grub_err_t
 grub_xnu_set_video (struct grub_xnu_boot_params *params)
@@ -34,8 +46,27 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
   struct grub_video_render_target *render_target;
   int ret;
   int x,y;
+  char *tmp, *modevar;
   grub_err_t err;
 
+  modevar = grub_env_get ("gfxpayload");
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
+  else
+    {
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      if (! tmp)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			   "couldn't allocate temporary storag");
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, video_hook);
+      grub_free (tmp);
+    }
+
+  if (err)
+    return err;
+
   ret = grub_video_get_info (&mode_info);
   if (ret)
     return grub_error (GRUB_ERR_IO, "couldn't retrieve video parameters");
diff --git a/term/gfxterm.c b/term/gfxterm.c
index e6baa15..5da6788 100644
--- a/term/gfxterm.c
+++ b/term/gfxterm.c
@@ -27,10 +27,7 @@
 #include <grub/bitmap.h>
 #include <grub/command.h>
 
-#define DEFAULT_VIDEO_WIDTH	640
-#define DEFAULT_VIDEO_HEIGHT	480
-#define DEFAULT_VIDEO_FLAGS	0
-
+#define DEFAULT_VIDEO_MODE "1024x768,800x600,640x480"
 #define DEFAULT_BORDER_WIDTH	10
 
 #define DEFAULT_STANDARD_COLOR  0x07
@@ -231,16 +228,22 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
   return grub_errno;
 }
 
+static int NESTED_FUNC_ATTR video_hook (grub_video_adapter_t p __attribute__ ((unused)),
+					struct grub_video_mode_info *info)
+{
+  return ! (info->mode_type & GRUB_VIDEO_MODE_TYPE_PURE_TEXT);
+}
+
 static grub_err_t
 grub_gfxterm_init (void)
 {
   char *font_name;
   char *modevar;
-  int width = DEFAULT_VIDEO_WIDTH;
-  int height = DEFAULT_VIDEO_HEIGHT;
-  int depth = -1;
-  int flags = DEFAULT_VIDEO_FLAGS;
+  char *tmp;
   grub_video_color_t color;
+  int width;
+  int height;
+  grub_err_t err;
 
   /* Select the font to use. */
   font_name = grub_env_get ("gfxterm_font");
@@ -249,231 +252,24 @@ grub_gfxterm_init (void)
 
   /* Parse gfxmode environment variable if set.  */
   modevar = grub_env_get ("gfxmode");
-  if (modevar)
-    {
-      char *tmp;
-      char *next_mode;
-      char *current_mode;
-      char *param;
-      char *value;
-      int mode_found = 0;
-
-      /* Take copy of env.var. as we don't want to modify that.  */
-      tmp = grub_strdup (modevar);
-      modevar = tmp;
-
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-        
-      /* Initialize next mode.  */
-      next_mode = modevar;
-      
-      /* Loop until all modes has been tested out.  */
-      while (next_mode != NULL)
-        {
-          /* Use last next_mode as current mode.  */
-          tmp = next_mode;
-          
-          /* Reset video mode settings.  */
-          width = DEFAULT_VIDEO_WIDTH;
-          height = DEFAULT_VIDEO_HEIGHT;
-          depth = -1;
-          flags = DEFAULT_VIDEO_FLAGS;
-        
-          /* Save position of next mode and separate modes.  */
-          next_mode = grub_strchr(next_mode, ';');
-          if (next_mode)
-            {
-              *next_mode = 0;
-              next_mode++;
-            }
-
-          /* Skip whitespace.  */
-          while (grub_isspace (*tmp))
-            tmp++;
-
-          /* Initialize token holders.  */
-          current_mode = tmp;
-          param = tmp;
-          value = NULL;
-
-          /* Parse <width>x<height>[x<depth>]*/
-
-          /* Find width value.  */
-          value = param;
-          param = grub_strchr(param, 'x');
-          if (param == NULL)
-            {
-              grub_err_t rc;
-              
-              /* First setup error message.  */
-	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                               "Invalid mode: %s\n",
-                               current_mode);
-              
-              /* Free memory before returning.  */
-              grub_free (modevar);
-              
-              return rc;
-            }
-
-          *param = 0;
-          param++;
-
-          width = grub_strtoul (value, 0, 0);
-          if (grub_errno != GRUB_ERR_NONE)
-            {
-              grub_err_t rc;
-              
-              /* First setup error message.  */
-	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                               "Invalid mode: %s\n",
-                               current_mode);
-              
-              /* Free memory before returning.  */
-              grub_free (modevar);
-              
-              return rc;
-            }
-
-          /* Find height value.  */
-          value = param;
-          param = grub_strchr(param, 'x');
-          if (param == NULL)
-            {
-              height = grub_strtoul (value, 0, 0);
-              if (grub_errno != GRUB_ERR_NONE)
-                {
-                  grub_err_t rc;
-
-                  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                                   "Invalid mode: %s\n",
-                                   current_mode);
-
-                  /* Free memory before returning.  */
-                  grub_free (modevar);
-
-		  return rc;
-		}
-            }
-	  else
-            {
-	      /* We have optional color depth value.  */
-	      *param = 0;
-	      param++;
-
-	      height = grub_strtoul (value, 0, 0);
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_err_t rc;
-
-		  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   "Invalid mode: %s\n",
-				   current_mode);
-
-		  /* Free memory before returning.  */
-		  grub_free (modevar);
-
-		  return rc;
-		}
-
-	      /* Convert color depth value.  */
-	      value = param;
-	      depth = grub_strtoul (value, 0, 0);
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_err_t rc;
-
-		  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   "Invalid mode: %s\n",
-				   current_mode);
-
-		  /* Free memory before returning.  */
-		  grub_free (modevar);
-
-		  return rc;
-		}
-            }
-
-	  /* Try out video mode.  */
-
-	  /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-	  if ((depth <= 8) && (depth != -1))
-	    flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-
-	  /* We have more than 8 bits, then assume that it is RGB color mode.  */
-	  if (depth > 8)
-	    flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-
-	  /* If user requested specific depth, forward that information to driver.  */
-	  if (depth != -1)
-	    flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-		     & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
-
-	  /* Try to initialize requested mode.  Ignore any errors.  */
-	  grub_error_push ();
-	  if (grub_video_setup (width, height, flags) != GRUB_ERR_NONE)
-	    {
-	      grub_error_pop ();
-	      continue;
-	    }
-
-	  /* Figure out what mode we ended up.  */
-	  if (grub_video_get_info (&mode_info) != GRUB_ERR_NONE)
-	    {
-	      /* Couldn't get video mode info, restore old mode and continue to next one.  */
-	      grub_error_pop ();
-
-	      grub_video_restore ();
-	      continue;
-	    }
-          
-          /* Restore state of error stack.  */
-          grub_error_pop ();
-          
-          /* Mode found!  Exit loop.  */
-          mode_found = 1;
-          break;
-        }
-
-      /* Free memory.  */
-      grub_free (modevar);
-      
-      if (!mode_found)
-        return grub_error (GRUB_ERR_BAD_ARGUMENT,
-                           "No suitable mode found.");
-    }
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
   else
     {
-      /* No gfxmode variable set, use defaults.  */
-      
-      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-      if ((depth <= 8) && (depth != -1))
-        flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-
-      /* We have more than 8 bits, then assume that it is RGB color mode.  */
-      if (depth > 8)
-        flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-
-      /* If user requested specific depth, forward that information to driver.  */
-      if (depth != -1)
-        flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-                 & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, video_hook);
+      grub_free (tmp);
+    }
 
-      /* Initialize user requested mode.  */
-      if (grub_video_setup (width, height, flags) != GRUB_ERR_NONE)
-        return grub_errno;
+  if (err)
+    return err;
 
-      /* Figure out what mode we ended up.  */
-      if (grub_video_get_info (&mode_info) != GRUB_ERR_NONE)
-        {
-          grub_video_restore ();
-          return grub_errno;
-        }
-    }
+  err = grub_video_get_info (&mode_info);
+  /* Figure out what mode we ended up.  */
+  if (err)
+    return err;
 
   /* Make sure screen is black.  */
   color = grub_video_map_rgb (0, 0, 0);
diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
index a145277..7d4d17e 100644
--- a/util/grub.d/30_os-prober.in
+++ b/util/grub.d/30_os-prober.in
@@ -90,9 +90,6 @@ EOF
 menuentry "${LONGNAME} (on ${DEVICE})" {
 	set root=${OSXROOT}
         insmod vbe
-        insmod gfxterm
-        gfxmode="1024x768x32;800x600x32"
-        terminal_output gfxterm
         do_resume=0
         if [ /var/vm/sleepimage -nt10 / ]; then
            if xnu_resume /var/vm/sleepimage; then
diff --git a/video/video.c b/video/video.c
index 49e6cb9..2c7f12b 100644
--- a/video/video.c
+++ b/video/video.c
@@ -19,6 +19,8 @@
 #include <grub/video.h>
 #include <grub/types.h>
 #include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
 
 /* The list of video adapters registered to system.  */
 static grub_video_adapter_t grub_video_adapter_list;
@@ -59,59 +61,6 @@ grub_video_iterate (int (*hook) (grub_video_adapter_t adapter))
       break;
 }
 
-/* Setup specified video mode.  */
-grub_err_t
-grub_video_setup (unsigned int width, unsigned int height,
-                  unsigned int mode_type)
-{
-  grub_video_adapter_t p;
-
-  /* De-activate last set video adapter.  */
-  if (grub_video_adapter_active)
-    {
-      /* Finalize adapter.  */
-      grub_video_adapter_active->fini ();
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-
-      /* Mark active adapter as not set.  */
-      grub_video_adapter_active = 0;
-    }
-
-  /* Loop thru all possible video adapter trying to find requested mode.  */
-  for (p = grub_video_adapter_list; p; p = p->next)
-    {
-      /* Try to initialize adapter, if it fails, skip to next adapter.  */
-      p->init ();
-      if (grub_errno != GRUB_ERR_NONE)
-        {
-          grub_errno = GRUB_ERR_NONE;
-          continue;
-        }
-
-      /* Try to initialize video mode.  */
-      p->setup (width, height, mode_type);
-      if (grub_errno == GRUB_ERR_NONE)
-        {
-          /* Valid mode found from adapter, and it has been activated.
-             Specify it as active adapter.  */
-          grub_video_adapter_active = p;
-          return GRUB_ERR_NONE;
-        }
-      else
-        grub_errno = GRUB_ERR_NONE;
-
-      /* No valid mode found in this adapter, finalize adapter.  */
-      p->fini ();
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-    }
-
-  /* We couldn't find suitable adapter for specified mode.  */
-  return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
-                     "Can't locate valid adapter for mode");
-}
-
 /* Restore back to initial mode (where applicable).  */
 grub_err_t
 grub_video_restore (void)
@@ -430,6 +379,319 @@ grub_video_get_active_render_target (struct grub_video_render_target **target)
   return grub_video_adapter_active->get_active_render_target (target);
 }
 
+grub_err_t
+grub_video_set_mode (char *modestring,
+		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+						   struct grub_video_mode_info *mode_info))
+{
+  char *tmp;
+  char *next_mode;
+  char *current_mode;
+  char *param;
+  char *value;
+  char *modevar;
+  int width = -1;
+  int height = -1;
+  int depth = -1;
+  int flags = 0;
+
+  /* Take copy of env.var. as we don't want to modify that.  */
+  modevar = grub_strdup (modestring);
+
+  /* Initialize next mode.  */
+  next_mode = modevar;
+
+  if (! modevar)
+    return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+		       "couldn't allocate space for local modevar copy");
+
+  if (grub_memcmp (next_mode, "keep", sizeof ("keep")) == 0
+      || grub_memcmp (next_mode, "keep,", sizeof ("keep,") - 1) == 0
+      || grub_memcmp (next_mode, "keep;", sizeof ("keep;") - 1) == 0)
+    {
+      struct grub_video_mode_info mode_info;
+      int suitable = 1;
+      grub_err_t err;
+      
+      grub_memset (&mode_info, 0, sizeof (mode_info));
+
+      if (grub_video_adapter_active)
+	{
+	  err = grub_video_get_info (&mode_info);
+	  if (err)
+	    {
+	      suitable = 0;
+	      grub_errno = GRUB_ERR_NONE;
+	    }
+	}
+      else
+	mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
+
+      if (suitable && hook)
+	suitable = hook (grub_video_adapter_active, &mode_info);
+      if (suitable)
+	{
+	  grub_free (modevar);
+	  return GRUB_ERR_NONE;
+	}
+      next_mode += sizeof ("keep") - 1;
+      if (! *next_mode)
+	{
+	  grub_free (modevar);
+  
+	  return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			     "No suitable mode found.");
+	}
+      
+      /* Skip separator. */
+      next_mode++;
+    }
+
+  /* De-activate last set video adapter.  */
+  if (grub_video_adapter_active)
+    {
+      /* Finalize adapter.  */
+      grub_video_adapter_active->fini ();
+      if (grub_errno != GRUB_ERR_NONE)
+	grub_errno = GRUB_ERR_NONE;
+      
+      /* Mark active adapter as not set.  */
+      grub_video_adapter_active = 0;
+    }
+    
+  /* Loop until all modes has been tested out.  */
+  while (next_mode != NULL)
+    {
+      /* Use last next_mode as current mode.  */
+      tmp = next_mode;
+      
+      /* Reset video mode settings.  */
+      width = -1;
+      height = -1;
+      depth = -1;
+      flags = 0;
+      
+      /* Save position of next mode and separate modes.  */
+      for (; *next_mode; next_mode++)
+	if (*next_mode == ',' || *next_mode == ';')
+	  break;
+      if (*next_mode)
+	{
+	  *next_mode = 0;
+	  next_mode++;
+	}
+      else
+	next_mode = 0;
+      
+      /* Skip whitespace.  */
+      while (grub_isspace (*tmp))
+	tmp++;
+      
+      /* Initialize token holders.  */
+      current_mode = tmp;
+      param = tmp;
+      value = NULL;
+
+      /* XXX: we assume that we're in pure text mode if 
+	 no video mode is initialized. Is it always true? */
+      if (grub_strcmp (param, "text") == 0)
+	{
+	  struct grub_video_mode_info mode_info;
+	  
+	  grub_memset (&mode_info, 0, sizeof (mode_info));
+	  mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
+
+	  if (! hook || hook (0, &mode_info))
+	    {
+	      /* Valid mode found from adapter, and it has been activated.
+		 Specify it as active adapter.  */
+	      grub_video_adapter_active = NULL;
+
+	      /* Free memory.  */
+	      grub_free (modevar);
+
+	      return GRUB_ERR_NONE;
+	    }
+	}
+
+      /* Parse <width>x<height>[x<depth>]*/
+      
+      /* Find width value.  */
+      value = param;
+      param = grub_strchr(param, 'x');
+      if (param == NULL)
+	{
+	  grub_err_t rc;
+          
+	  /* First setup error message.  */
+	  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   "Invalid mode: %s\n",
+			   current_mode);
+          
+	  /* Free memory before returning.  */
+	  grub_free (modevar);
+	  
+	  return rc;
+	}
+      
+      *param = 0;
+      param++;
+      
+      width = grub_strtoul (value, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE)
+	{
+	  grub_err_t rc;
+          
+	  /* First setup error message.  */
+	  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   "Invalid mode: %s\n",
+			   current_mode);
+	  
+	  /* Free memory before returning.  */
+	  grub_free (modevar);
+          
+	  return rc;
+	}
+      
+      /* Find height value.  */
+      value = param;
+      param = grub_strchr(param, 'x');
+      if (param == NULL)
+	{
+	  height = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+	      
+	      return rc;
+	    }
+	}
+      else
+	{
+	  /* We have optional color depth value.  */
+	  *param = 0;
+	  param++;
+	  
+	  height = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+	      
+	      return rc;
+	    }
+	  
+	  /* Convert color depth value.  */
+	  value = param;
+	  depth = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+
+	      return rc;
+	    }
+	}
+
+      /* Try out video mode.  */
+      
+      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
+      if ((depth <= 8) && (depth != -1))
+	flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
+
+      /* We have more than 8 bits, then assume that it is RGB color mode.  */
+      if (depth > 8)
+	flags |= GRUB_VIDEO_MODE_TYPE_RGB;
+      
+      /* If user requested specific depth, forward that information to driver.  */
+      if (depth != -1)
+	flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
+	  & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+      
+      /* Try to initialize requested mode.  Ignore any errors.  */
+      grub_video_adapter_t p;
+
+      /* Loop thru all possible video adapter trying to find requested mode.  */
+      for (p = grub_video_adapter_list; p; p = p->next)
+	{
+	  grub_err_t err;
+	  struct grub_video_mode_info mode_info;
+	  
+	  grub_memset (&mode_info, 0, sizeof (mode_info));
+
+	  /* Try to initialize adapter, if it fails, skip to next adapter.  */
+	  err = p->init ();
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  /* Try to initialize video mode.  */
+	  err = p->setup (width, height, flags);
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  err = p->get_info (&mode_info);
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  if (hook && ! hook (p, &mode_info))
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+	    
+	  /* Valid mode found from adapter, and it has been activated.
+	     Specify it as active adapter.  */
+	  grub_video_adapter_active = p;
+	  
+	  /* Free memory.  */
+	  grub_free (modevar);
+	  
+	  return GRUB_ERR_NONE;
+	}
+
+    }
+
+  /* Free memory.  */
+  grub_free (modevar);
+  
+  return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		     "No suitable mode found.");
+}
+
 /* Initialize Video API module.  */
 GRUB_MOD_INIT(video_video)
 {

Re: [PATCH] gfxpayload

[Felix Zielcke]

Am Samstag, den 16.05.2009, 20:01 +0200 schrieb Vladimir 'phcoder'
Serbinenko:
> Hello. Here is a long-discussed gfxpayload patch. To start linux in
> 1024x768 mode do:
> insmod vbe
> gfxmode=1024x768
> linux ...
> ...
> boot
> Multiple modes can be separated by either comma or semicolon. First
> mode can be "keep" which will make the system keep the current mode if
> accepted by kernel. Another special value is "text" which will put in
> text mode.
> Can someone check that vga= backward compatibility code interprets
> values correctly?
> With linux for a reason I don't know it results in black screen in
> qemu. But perhaps it's a problem with my kernel
> Now it's the same syntax for xnu too. Just remember that current xnu
> loader support 32-bit modes only

Just tried it in VMware. 
With set gfxmode=1024x768x24 and set gfxpayload=1024x768x24 and no vga=
option I just get the normal text mode
with set gfxpayload=keep it works
and also vga=0x317 works too.
-- 
Felix Zielcke

Re: [PATCH] gfxpayload

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

> Just tried it in VMware.
> With set gfxmode=1024x768x24 and set gfxpayload=1024x768x24 and no vga=
> option I just get the normal text mode
> with set gfxpayload=keep it works
> and also vga=0x317 works too.
As we figured out on IRC the problem was that vmware has no 16-bit.
New patch with fixed vga=0 and warning for users when using vga=
> --
> Felix Zielcke
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: gfxpayload.diff --]
[-- Type: text/x-diff, Size: 28461 bytes --]

diff --git a/commands/videotest.c b/commands/videotest.c
index db7f704..6fe4b9b 100644
--- a/commands/videotest.c
+++ b/commands/videotest.c
@@ -30,8 +30,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ ((unused)),
                     int argc __attribute__ ((unused)),
                     char **args __attribute__ ((unused)))
 {
-  if (grub_video_setup (1024, 768,
-                        GRUB_VIDEO_MODE_TYPE_INDEX_COLOR) != GRUB_ERR_NONE)
+  if (grub_video_set_mode ("1024x768;800x600;640x480", 0) != GRUB_ERR_NONE)
     return grub_errno;
 
   grub_video_color_t color;
diff --git a/include/grub/video.h b/include/grub/video.h
index cb73dae..3b27655 100644
--- a/include/grub/video.h
+++ b/include/grub/video.h
@@ -34,6 +34,7 @@ struct grub_video_render_target;
 struct grub_video_bitmap;
 
 /* Defines used to describe video mode or rendering target.  */
+#define GRUB_VIDEO_MODE_TYPE_PURE_TEXT		0x00000040
 #define GRUB_VIDEO_MODE_TYPE_ALPHA		0x00000020
 #define GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED	0x00000010
 #define GRUB_VIDEO_MODE_TYPE_1BIT_BITMAP	0x00000004
@@ -236,9 +237,6 @@ void grub_video_register (grub_video_adapter_t adapter);
 void grub_video_unregister (grub_video_adapter_t adapter);
 void grub_video_iterate (int (*hook) (grub_video_adapter_t adapter));
 
-grub_err_t grub_video_setup (unsigned int width, unsigned int height,
-                             unsigned int mode_type);
-
 grub_err_t grub_video_restore (void);
 
 grub_err_t grub_video_get_info (struct grub_video_mode_info *mode_info);
@@ -299,4 +297,8 @@ grub_err_t grub_video_set_active_render_target (struct grub_video_render_target
 
 grub_err_t grub_video_get_active_render_target (struct grub_video_render_target **target);
 
+grub_err_t grub_video_set_mode (char *modestring,
+				int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+							      struct grub_video_mode_info *mode_info));
+
 #endif /* ! GRUB_VIDEO_HEADER */
diff --git a/loader/i386/linux.c b/loader/i386/linux.c
index ca44fb4..72d826c 100644
--- a/loader/i386/linux.c
+++ b/loader/i386/linux.c
@@ -44,7 +44,9 @@
    GRUB and Linux (saving boot time and visual glitches).  Official GRUB, OTOH,
    needs to be conservative.  */
 #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
-#define GRUB_ASSUME_LINUX_HAS_FB_SUPPORT 0
+#define DEFAULT_VIDEO_MODE "text"
+#else
+#define DEFAULT_VIDEO_MODE "keep"
 #endif
 
 static grub_dl_t my_mod;
@@ -94,8 +96,7 @@ static struct idt_descriptor idt_desc =
     0
   };
 
-static grub_uint16_t vid_mode;
-
+#ifdef GRUB_MACHINE_PCBIOS
 struct linux_vesafb_res
 {
   grub_uint16_t width;
@@ -262,6 +263,7 @@ struct linux_vesafb_mode linux_vesafb_modes[] =
     { VGA_800_500, 24 },	/* 0x372 */
     { VGA_800_500, 32 },	/* 0x373 */
   };
+#endif
 
 static inline grub_size_t
 page_align (grub_size_t size)
@@ -442,48 +444,32 @@ grub_linux_boot (void)
 {
   struct linux_kernel_params *params;
   int e820_num;
-  
+  grub_err_t err;
+  char *modevar, *tmp;
+
   params = real_mode_mem;
 
-  if (vid_mode == GRUB_LINUX_VID_MODE_NORMAL || vid_mode == GRUB_LINUX_VID_MODE_EXTENDED)
-    grub_video_restore ();
-  else if (vid_mode)
+  modevar = grub_env_get ("gfxpayload");
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0);
+  else
     {
-      struct linux_vesafb_mode *linux_mode;
-      int depth, flags;
-      
-      flags = 0;
-      linux_mode = &linux_vesafb_modes[vid_mode - GRUB_LINUX_VID_MODE_VESA_START];
-      depth = linux_mode->depth;
-      
-      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-      if ((depth <= 8) && (depth != -1))
-	flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-      
-      /* We have more than 8 bits, then assume that it is RGB color mode.  */
-      if (depth > 8)
-	flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-      
-      /* If user requested specific depth, forward that information to driver.  */
-      if (depth != -1)
-	flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-	  & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
-      
-      /* Try to initialize requested mode.  */
-      if (grub_video_setup (linux_vesafb_res[linux_mode->res_index].width,
-			    linux_vesafb_res[linux_mode->res_index].height,
-			    flags) != GRUB_ERR_NONE)
-	{
-	  grub_printf ("Unable to initialize requested video mode (vga=0x%x)\n", vid_mode);
-	  return grub_errno;
-	}
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      if (! tmp)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			   "couldn't allocate temporary storag");
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, 0);
+      grub_free (tmp);
+    }
+
+  if (err)
+    {
+      grub_print_error ();
+      grub_printf ("Booting however\n");
+      grub_errno = GRUB_ERR_NONE;
     }
-#if ! GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
-  else
-    /* If user didn't request a video mode, and we can't assume Linux supports FB,
-       then we go back to text mode.  */
-    grub_video_restore ();
-#endif
 
   if (! grub_linux_setup_video (params))
     params->have_vga = GRUB_VIDEO_TYPE_VLFB;
@@ -708,7 +694,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 	       (unsigned) real_size, (unsigned) prot_size);
 
   /* Look for memory size and video mode specified on the command line.  */
-  vid_mode = 0;
   linux_mem_size = 0;
   for (i = 1; i < argc; i++)
 #ifdef GRUB_MACHINE_PCBIOS
@@ -716,11 +701,15 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       {
 	/* Video mode selection support.  */
 	char *val = argv[i] + 4;
+	unsigned vid_mode = 0;
+	struct linux_vesafb_mode *linux_mode;
+	grub_err_t err;
+	char *buf;
 
 	if (grub_strcmp (val, "normal") == 0)
-	  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
+	  vid_mode = 0;
 	else if (grub_strcmp (val, "ext") == 0)
-	  vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
+	  vid_mode = 1;
 	else if (grub_strcmp (val, "ask") == 0)
 	  {
 	    grub_printf ("Legacy `ask' parameter no longer supported.\n");
@@ -737,21 +726,58 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 	switch (vid_mode)
 	  {
 	  case 0:
-	    vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
+	    grub_env_set ("gfxpayload", "text");
+	    grub_printf ("%s is depreceated. "
+			 "Use set gfxpayload=text before "
+			 "linux command instead.\n", 
+			 argv[i]);	
 	    break;
 	  case 1:
-	    vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
+	    /* FIXME: support 80x50 text. */
+	    grub_env_set ("gfxpayload", "text");
+	    grub_printf ("%s is depreceated. "
+			 "Use set gfxpayload=text before "
+			 "linux command instead.\n", 
+			 argv[i]);	
 	    break;
 	  default:
 	    /* Ignore invalid values.  */
 	    if (vid_mode < GRUB_LINUX_VID_MODE_VESA_START ||
 		vid_mode >= GRUB_LINUX_VID_MODE_VESA_START +
 		ARRAY_SIZE (linux_vesafb_modes))
-	      vid_mode = 0;
-	  }
+	      {
+		grub_env_set ("gfxpayload", "text");
+		grub_printf ("%s is depreceated. Mode %d isn't recognized. "
+			     "Use set gfxpayload=WIDTHxHEIGHT[xDEPTH] before "
+			     "linux command instead.\n", 
+			     argv[i], vid_mode);	
+		break;
+	      }
 
-	if (grub_errno)
-	  goto fail;
+	    buf = grub_malloc (20);
+	    if (! buf)
+	      {
+		grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			    "couldn't allocate temporary storage");
+		goto fail;
+	      }
+	    
+	    linux_mode 
+	      = &linux_vesafb_modes[vid_mode - GRUB_LINUX_VID_MODE_VESA_START];
+	    
+	    grub_sprintf (buf, "%dx%dx%d", 
+			  linux_vesafb_res[linux_mode->res_index].width,
+			  linux_vesafb_res[linux_mode->res_index].height,
+			  linux_mode->depth);
+	    grub_printf ("%s is depreceated. "
+			 "Use set gfxpayload=%s before "
+			 "linux command instead.\n", 
+			 argv[i], buf);	
+	    err = grub_env_set ("gfxpayload", buf);
+	    grub_free (buf);
+	    if (err)
+	      goto fail;
+	  }
       }
     else
 #endif /* GRUB_MACHINE_PCBIOS */
diff --git a/loader/i386/pc/xnu.c b/loader/i386/pc/xnu.c
index 1cca138..d32f679 100644
--- a/loader/i386/pc/xnu.c
+++ b/loader/i386/pc/xnu.c
@@ -19,6 +19,7 @@
 #include <grub/env.h>
 #include <grub/misc.h>
 #include <grub/xnu.h>
+#include <grub/mm.h>
 #include <grub/cpu/xnu.h>
 #include <grub/machine/vbe.h>
 #include <grub/machine/vga.h>
@@ -26,6 +27,17 @@
 #define min(a,b) (((a) < (b)) ? (a) : (b))
 #define max(a,b) (((a) > (b)) ? (a) : (b))
 
+#define DEFAULT_VIDEO_MODE "1024x768x32,800x600x32,640x480x32"
+
+static int NESTED_FUNC_ATTR video_hook (grub_video_adapter_t p __attribute__ ((unused)),
+					struct grub_video_mode_info *info)
+{
+  if (info->mode_type & GRUB_VIDEO_MODE_TYPE_PURE_TEXT)
+    return 0;
+
+  return 1;
+}
+
 /* Setup video for xnu. */
 grub_err_t
 grub_xnu_set_video (struct grub_xnu_boot_params *params)
@@ -34,8 +46,27 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
   struct grub_video_render_target *render_target;
   int ret;
   int x,y;
+  char *tmp, *modevar;
   grub_err_t err;
 
+  modevar = grub_env_get ("gfxpayload");
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
+  else
+    {
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      if (! tmp)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+			   "couldn't allocate temporary storag");
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, video_hook);
+      grub_free (tmp);
+    }
+
+  if (err)
+    return err;
+
   ret = grub_video_get_info (&mode_info);
   if (ret)
     return grub_error (GRUB_ERR_IO, "couldn't retrieve video parameters");
diff --git a/term/gfxterm.c b/term/gfxterm.c
index e6baa15..5da6788 100644
--- a/term/gfxterm.c
+++ b/term/gfxterm.c
@@ -27,10 +27,7 @@
 #include <grub/bitmap.h>
 #include <grub/command.h>
 
-#define DEFAULT_VIDEO_WIDTH	640
-#define DEFAULT_VIDEO_HEIGHT	480
-#define DEFAULT_VIDEO_FLAGS	0
-
+#define DEFAULT_VIDEO_MODE "1024x768,800x600,640x480"
 #define DEFAULT_BORDER_WIDTH	10
 
 #define DEFAULT_STANDARD_COLOR  0x07
@@ -231,16 +228,22 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
   return grub_errno;
 }
 
+static int NESTED_FUNC_ATTR video_hook (grub_video_adapter_t p __attribute__ ((unused)),
+					struct grub_video_mode_info *info)
+{
+  return ! (info->mode_type & GRUB_VIDEO_MODE_TYPE_PURE_TEXT);
+}
+
 static grub_err_t
 grub_gfxterm_init (void)
 {
   char *font_name;
   char *modevar;
-  int width = DEFAULT_VIDEO_WIDTH;
-  int height = DEFAULT_VIDEO_HEIGHT;
-  int depth = -1;
-  int flags = DEFAULT_VIDEO_FLAGS;
+  char *tmp;
   grub_video_color_t color;
+  int width;
+  int height;
+  grub_err_t err;
 
   /* Select the font to use. */
   font_name = grub_env_get ("gfxterm_font");
@@ -249,231 +252,24 @@ grub_gfxterm_init (void)
 
   /* Parse gfxmode environment variable if set.  */
   modevar = grub_env_get ("gfxmode");
-  if (modevar)
-    {
-      char *tmp;
-      char *next_mode;
-      char *current_mode;
-      char *param;
-      char *value;
-      int mode_found = 0;
-
-      /* Take copy of env.var. as we don't want to modify that.  */
-      tmp = grub_strdup (modevar);
-      modevar = tmp;
-
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-        
-      /* Initialize next mode.  */
-      next_mode = modevar;
-      
-      /* Loop until all modes has been tested out.  */
-      while (next_mode != NULL)
-        {
-          /* Use last next_mode as current mode.  */
-          tmp = next_mode;
-          
-          /* Reset video mode settings.  */
-          width = DEFAULT_VIDEO_WIDTH;
-          height = DEFAULT_VIDEO_HEIGHT;
-          depth = -1;
-          flags = DEFAULT_VIDEO_FLAGS;
-        
-          /* Save position of next mode and separate modes.  */
-          next_mode = grub_strchr(next_mode, ';');
-          if (next_mode)
-            {
-              *next_mode = 0;
-              next_mode++;
-            }
-
-          /* Skip whitespace.  */
-          while (grub_isspace (*tmp))
-            tmp++;
-
-          /* Initialize token holders.  */
-          current_mode = tmp;
-          param = tmp;
-          value = NULL;
-
-          /* Parse <width>x<height>[x<depth>]*/
-
-          /* Find width value.  */
-          value = param;
-          param = grub_strchr(param, 'x');
-          if (param == NULL)
-            {
-              grub_err_t rc;
-              
-              /* First setup error message.  */
-	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                               "Invalid mode: %s\n",
-                               current_mode);
-              
-              /* Free memory before returning.  */
-              grub_free (modevar);
-              
-              return rc;
-            }
-
-          *param = 0;
-          param++;
-
-          width = grub_strtoul (value, 0, 0);
-          if (grub_errno != GRUB_ERR_NONE)
-            {
-              grub_err_t rc;
-              
-              /* First setup error message.  */
-	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                               "Invalid mode: %s\n",
-                               current_mode);
-              
-              /* Free memory before returning.  */
-              grub_free (modevar);
-              
-              return rc;
-            }
-
-          /* Find height value.  */
-          value = param;
-          param = grub_strchr(param, 'x');
-          if (param == NULL)
-            {
-              height = grub_strtoul (value, 0, 0);
-              if (grub_errno != GRUB_ERR_NONE)
-                {
-                  grub_err_t rc;
-
-                  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-                                   "Invalid mode: %s\n",
-                                   current_mode);
-
-                  /* Free memory before returning.  */
-                  grub_free (modevar);
-
-		  return rc;
-		}
-            }
-	  else
-            {
-	      /* We have optional color depth value.  */
-	      *param = 0;
-	      param++;
-
-	      height = grub_strtoul (value, 0, 0);
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_err_t rc;
-
-		  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   "Invalid mode: %s\n",
-				   current_mode);
-
-		  /* Free memory before returning.  */
-		  grub_free (modevar);
-
-		  return rc;
-		}
-
-	      /* Convert color depth value.  */
-	      value = param;
-	      depth = grub_strtoul (value, 0, 0);
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_err_t rc;
-
-		  /* First setup error message.  */
-		  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   "Invalid mode: %s\n",
-				   current_mode);
-
-		  /* Free memory before returning.  */
-		  grub_free (modevar);
-
-		  return rc;
-		}
-            }
-
-	  /* Try out video mode.  */
-
-	  /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-	  if ((depth <= 8) && (depth != -1))
-	    flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-
-	  /* We have more than 8 bits, then assume that it is RGB color mode.  */
-	  if (depth > 8)
-	    flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-
-	  /* If user requested specific depth, forward that information to driver.  */
-	  if (depth != -1)
-	    flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-		     & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
-
-	  /* Try to initialize requested mode.  Ignore any errors.  */
-	  grub_error_push ();
-	  if (grub_video_setup (width, height, flags) != GRUB_ERR_NONE)
-	    {
-	      grub_error_pop ();
-	      continue;
-	    }
-
-	  /* Figure out what mode we ended up.  */
-	  if (grub_video_get_info (&mode_info) != GRUB_ERR_NONE)
-	    {
-	      /* Couldn't get video mode info, restore old mode and continue to next one.  */
-	      grub_error_pop ();
-
-	      grub_video_restore ();
-	      continue;
-	    }
-          
-          /* Restore state of error stack.  */
-          grub_error_pop ();
-          
-          /* Mode found!  Exit loop.  */
-          mode_found = 1;
-          break;
-        }
-
-      /* Free memory.  */
-      grub_free (modevar);
-      
-      if (!mode_found)
-        return grub_error (GRUB_ERR_BAD_ARGUMENT,
-                           "No suitable mode found.");
-    }
+  if (! modevar || *modevar == 0)
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
   else
     {
-      /* No gfxmode variable set, use defaults.  */
-      
-      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
-      if ((depth <= 8) && (depth != -1))
-        flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
-
-      /* We have more than 8 bits, then assume that it is RGB color mode.  */
-      if (depth > 8)
-        flags |= GRUB_VIDEO_MODE_TYPE_RGB;
-
-      /* If user requested specific depth, forward that information to driver.  */
-      if (depth != -1)
-        flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
-                 & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+      tmp = grub_malloc (grub_strlen (modevar) 
+			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
+      grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+      err = grub_video_set_mode (tmp, video_hook);
+      grub_free (tmp);
+    }
 
-      /* Initialize user requested mode.  */
-      if (grub_video_setup (width, height, flags) != GRUB_ERR_NONE)
-        return grub_errno;
+  if (err)
+    return err;
 
-      /* Figure out what mode we ended up.  */
-      if (grub_video_get_info (&mode_info) != GRUB_ERR_NONE)
-        {
-          grub_video_restore ();
-          return grub_errno;
-        }
-    }
+  err = grub_video_get_info (&mode_info);
+  /* Figure out what mode we ended up.  */
+  if (err)
+    return err;
 
   /* Make sure screen is black.  */
   color = grub_video_map_rgb (0, 0, 0);
diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
index a145277..7d4d17e 100644
--- a/util/grub.d/30_os-prober.in
+++ b/util/grub.d/30_os-prober.in
@@ -90,9 +90,6 @@ EOF
 menuentry "${LONGNAME} (on ${DEVICE})" {
 	set root=${OSXROOT}
         insmod vbe
-        insmod gfxterm
-        gfxmode="1024x768x32;800x600x32"
-        terminal_output gfxterm
         do_resume=0
         if [ /var/vm/sleepimage -nt10 / ]; then
            if xnu_resume /var/vm/sleepimage; then
diff --git a/video/video.c b/video/video.c
index 49e6cb9..2c7f12b 100644
--- a/video/video.c
+++ b/video/video.c
@@ -19,6 +19,8 @@
 #include <grub/video.h>
 #include <grub/types.h>
 #include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
 
 /* The list of video adapters registered to system.  */
 static grub_video_adapter_t grub_video_adapter_list;
@@ -59,59 +61,6 @@ grub_video_iterate (int (*hook) (grub_video_adapter_t adapter))
       break;
 }
 
-/* Setup specified video mode.  */
-grub_err_t
-grub_video_setup (unsigned int width, unsigned int height,
-                  unsigned int mode_type)
-{
-  grub_video_adapter_t p;
-
-  /* De-activate last set video adapter.  */
-  if (grub_video_adapter_active)
-    {
-      /* Finalize adapter.  */
-      grub_video_adapter_active->fini ();
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-
-      /* Mark active adapter as not set.  */
-      grub_video_adapter_active = 0;
-    }
-
-  /* Loop thru all possible video adapter trying to find requested mode.  */
-  for (p = grub_video_adapter_list; p; p = p->next)
-    {
-      /* Try to initialize adapter, if it fails, skip to next adapter.  */
-      p->init ();
-      if (grub_errno != GRUB_ERR_NONE)
-        {
-          grub_errno = GRUB_ERR_NONE;
-          continue;
-        }
-
-      /* Try to initialize video mode.  */
-      p->setup (width, height, mode_type);
-      if (grub_errno == GRUB_ERR_NONE)
-        {
-          /* Valid mode found from adapter, and it has been activated.
-             Specify it as active adapter.  */
-          grub_video_adapter_active = p;
-          return GRUB_ERR_NONE;
-        }
-      else
-        grub_errno = GRUB_ERR_NONE;
-
-      /* No valid mode found in this adapter, finalize adapter.  */
-      p->fini ();
-      if (grub_errno != GRUB_ERR_NONE)
-        return grub_errno;
-    }
-
-  /* We couldn't find suitable adapter for specified mode.  */
-  return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
-                     "Can't locate valid adapter for mode");
-}
-
 /* Restore back to initial mode (where applicable).  */
 grub_err_t
 grub_video_restore (void)
@@ -430,6 +379,319 @@ grub_video_get_active_render_target (struct grub_video_render_target **target)
   return grub_video_adapter_active->get_active_render_target (target);
 }
 
+grub_err_t
+grub_video_set_mode (char *modestring,
+		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+						   struct grub_video_mode_info *mode_info))
+{
+  char *tmp;
+  char *next_mode;
+  char *current_mode;
+  char *param;
+  char *value;
+  char *modevar;
+  int width = -1;
+  int height = -1;
+  int depth = -1;
+  int flags = 0;
+
+  /* Take copy of env.var. as we don't want to modify that.  */
+  modevar = grub_strdup (modestring);
+
+  /* Initialize next mode.  */
+  next_mode = modevar;
+
+  if (! modevar)
+    return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
+		       "couldn't allocate space for local modevar copy");
+
+  if (grub_memcmp (next_mode, "keep", sizeof ("keep")) == 0
+      || grub_memcmp (next_mode, "keep,", sizeof ("keep,") - 1) == 0
+      || grub_memcmp (next_mode, "keep;", sizeof ("keep;") - 1) == 0)
+    {
+      struct grub_video_mode_info mode_info;
+      int suitable = 1;
+      grub_err_t err;
+      
+      grub_memset (&mode_info, 0, sizeof (mode_info));
+
+      if (grub_video_adapter_active)
+	{
+	  err = grub_video_get_info (&mode_info);
+	  if (err)
+	    {
+	      suitable = 0;
+	      grub_errno = GRUB_ERR_NONE;
+	    }
+	}
+      else
+	mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
+
+      if (suitable && hook)
+	suitable = hook (grub_video_adapter_active, &mode_info);
+      if (suitable)
+	{
+	  grub_free (modevar);
+	  return GRUB_ERR_NONE;
+	}
+      next_mode += sizeof ("keep") - 1;
+      if (! *next_mode)
+	{
+	  grub_free (modevar);
+  
+	  return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			     "No suitable mode found.");
+	}
+      
+      /* Skip separator. */
+      next_mode++;
+    }
+
+  /* De-activate last set video adapter.  */
+  if (grub_video_adapter_active)
+    {
+      /* Finalize adapter.  */
+      grub_video_adapter_active->fini ();
+      if (grub_errno != GRUB_ERR_NONE)
+	grub_errno = GRUB_ERR_NONE;
+      
+      /* Mark active adapter as not set.  */
+      grub_video_adapter_active = 0;
+    }
+    
+  /* Loop until all modes has been tested out.  */
+  while (next_mode != NULL)
+    {
+      /* Use last next_mode as current mode.  */
+      tmp = next_mode;
+      
+      /* Reset video mode settings.  */
+      width = -1;
+      height = -1;
+      depth = -1;
+      flags = 0;
+      
+      /* Save position of next mode and separate modes.  */
+      for (; *next_mode; next_mode++)
+	if (*next_mode == ',' || *next_mode == ';')
+	  break;
+      if (*next_mode)
+	{
+	  *next_mode = 0;
+	  next_mode++;
+	}
+      else
+	next_mode = 0;
+      
+      /* Skip whitespace.  */
+      while (grub_isspace (*tmp))
+	tmp++;
+      
+      /* Initialize token holders.  */
+      current_mode = tmp;
+      param = tmp;
+      value = NULL;
+
+      /* XXX: we assume that we're in pure text mode if 
+	 no video mode is initialized. Is it always true? */
+      if (grub_strcmp (param, "text") == 0)
+	{
+	  struct grub_video_mode_info mode_info;
+	  
+	  grub_memset (&mode_info, 0, sizeof (mode_info));
+	  mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
+
+	  if (! hook || hook (0, &mode_info))
+	    {
+	      /* Valid mode found from adapter, and it has been activated.
+		 Specify it as active adapter.  */
+	      grub_video_adapter_active = NULL;
+
+	      /* Free memory.  */
+	      grub_free (modevar);
+
+	      return GRUB_ERR_NONE;
+	    }
+	}
+
+      /* Parse <width>x<height>[x<depth>]*/
+      
+      /* Find width value.  */
+      value = param;
+      param = grub_strchr(param, 'x');
+      if (param == NULL)
+	{
+	  grub_err_t rc;
+          
+	  /* First setup error message.  */
+	  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   "Invalid mode: %s\n",
+			   current_mode);
+          
+	  /* Free memory before returning.  */
+	  grub_free (modevar);
+	  
+	  return rc;
+	}
+      
+      *param = 0;
+      param++;
+      
+      width = grub_strtoul (value, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE)
+	{
+	  grub_err_t rc;
+          
+	  /* First setup error message.  */
+	  rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   "Invalid mode: %s\n",
+			   current_mode);
+	  
+	  /* Free memory before returning.  */
+	  grub_free (modevar);
+          
+	  return rc;
+	}
+      
+      /* Find height value.  */
+      value = param;
+      param = grub_strchr(param, 'x');
+      if (param == NULL)
+	{
+	  height = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+	      
+	      return rc;
+	    }
+	}
+      else
+	{
+	  /* We have optional color depth value.  */
+	  *param = 0;
+	  param++;
+	  
+	  height = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+	      
+	      return rc;
+	    }
+	  
+	  /* Convert color depth value.  */
+	  value = param;
+	  depth = grub_strtoul (value, 0, 0);
+	  if (grub_errno != GRUB_ERR_NONE)
+	    {
+	      grub_err_t rc;
+	      
+	      /* First setup error message.  */
+	      rc = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       "Invalid mode: %s\n",
+			       current_mode);
+	      
+	      /* Free memory before returning.  */
+	      grub_free (modevar);
+
+	      return rc;
+	    }
+	}
+
+      /* Try out video mode.  */
+      
+      /* If we have 8 or less bits, then assume that it is indexed color mode.  */
+      if ((depth <= 8) && (depth != -1))
+	flags |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
+
+      /* We have more than 8 bits, then assume that it is RGB color mode.  */
+      if (depth > 8)
+	flags |= GRUB_VIDEO_MODE_TYPE_RGB;
+      
+      /* If user requested specific depth, forward that information to driver.  */
+      if (depth != -1)
+	flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
+	  & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+      
+      /* Try to initialize requested mode.  Ignore any errors.  */
+      grub_video_adapter_t p;
+
+      /* Loop thru all possible video adapter trying to find requested mode.  */
+      for (p = grub_video_adapter_list; p; p = p->next)
+	{
+	  grub_err_t err;
+	  struct grub_video_mode_info mode_info;
+	  
+	  grub_memset (&mode_info, 0, sizeof (mode_info));
+
+	  /* Try to initialize adapter, if it fails, skip to next adapter.  */
+	  err = p->init ();
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  /* Try to initialize video mode.  */
+	  err = p->setup (width, height, flags);
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  err = p->get_info (&mode_info);
+	  if (err != GRUB_ERR_NONE)
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+
+	  if (hook && ! hook (p, &mode_info))
+	    {
+	      p->fini ();
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
+	    
+	  /* Valid mode found from adapter, and it has been activated.
+	     Specify it as active adapter.  */
+	  grub_video_adapter_active = p;
+	  
+	  /* Free memory.  */
+	  grub_free (modevar);
+	  
+	  return GRUB_ERR_NONE;
+	}
+
+    }
+
+  /* Free memory.  */
+  grub_free (modevar);
+  
+  return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		     "No suitable mode found.");
+}
+
 /* Initialize Video API module.  */
 GRUB_MOD_INIT(video_video)
 {

Re: Fwd: [PATCH] gfxpayload

[Robert Millan]

Hi,

On Sun, May 17, 2009 at 04:34:16PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>  #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
> -#define GRUB_ASSUME_LINUX_HAS_FB_SUPPORT 0
> +#define DEFAULT_VIDEO_MODE "text"
> +#else
> +#define DEFAULT_VIDEO_MODE "keep"
>  #endif

As discussed on IRC, this "keep" option seems unnecessary;  it just means
"do nothing" so we may as well #ifdef the code out.

> +	return grub_error (GRUB_ERR_OUT_OF_MEMORY, 
> +			   "couldn't allocate temporary storag");

Typo.

>  	if (grub_strcmp (val, "normal") == 0)
> -	  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
> +	  vid_mode = 0;
>  	else if (grub_strcmp (val, "ext") == 0)
> -	  vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
> +	  vid_mode = 1;
> [...]
>  	switch (vid_mode)
>  	  {
>  	  case 0:
> [...]
>  	  case 1:

Can we avoid encoding this information as numbers?  We had nice macros
before, and plain numbers are confusing to read for those not familiar with
the code.

> +	    grub_env_set ("gfxpayload", "text");
> +	    grub_printf ("%s is depreceated. "

Typo ;-)

> +	    buf = grub_malloc (20);
> +	    if (! buf)
> +	      {
> +		grub_error (GRUB_ERR_OUT_OF_MEMORY, 
> +			    "couldn't allocate temporary storage");
> +		goto fail;
> +	      }

Doesn't grub_malloc set this errno / errmsg already?

> --- a/term/gfxterm.c
> +++ b/term/gfxterm.c
> @@ -27,10 +27,7 @@
>  #include <grub/bitmap.h>
>  #include <grub/command.h>
>  
> -#define DEFAULT_VIDEO_WIDTH	640
> -#define DEFAULT_VIDEO_HEIGHT	480
> -#define DEFAULT_VIDEO_FLAGS	0
> -
> +#define DEFAULT_VIDEO_MODE "1024x768,800x600,640x480"

Our fonts / texts / menus in general are not prepared to cope well with
resolutions higher than 640x480.  It works, but looks different/ugly.
Please leave that as default untill we solve those problems.

Wrt changes in the video subsystem, as discussed on IRC this was discussed
before (me and Vesa, I think) and Vesa said he wanted to think about it.
Would be nice if we can reach consensus on this, and someone who's
familiarised with that area (not me) could approve those changes.

But Vesa is on vacation... maybe Colin?  I don't know.  Try CCing them :-)

Thanks!

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] gfxpayload

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

Robert Millan wrote on Sunday 17 May 2009:
> Hi,
>
> On Sun, May 17, 2009 at 04:34:16PM +0200, Vladimir 'phcoder' Serbinenko 
wrote:
> > --- a/term/gfxterm.c
> > +++ b/term/gfxterm.c
> > @@ -27,10 +27,7 @@
> >  #include <grub/bitmap.h>
> >  #include <grub/command.h>
> >
> > -#define DEFAULT_VIDEO_WIDTH	640
> > -#define DEFAULT_VIDEO_HEIGHT	480
> > -#define DEFAULT_VIDEO_FLAGS	0
> > -
> > +#define DEFAULT_VIDEO_MODE "1024x768,800x600,640x480"
>
> Our fonts / texts / menus in general are not prepared to cope well with
> resolutions higher than 640x480.  It works, but looks different/ugly.
> Please leave that as default untill we solve those problems.

I think the main problems at present are (1) the only provided font is GNU 
Unifont, which is fairly small (but I think it should work ok at sizes around 
1024x768) and (2) my graphical menu themes currently support only absolute 
pixel positioning, so when the screen size is changed, they don't adapt (I 
really want to implement some sort of layout management system to handle 
component layout consistently in the GUI).

> Wrt changes in the video subsystem, as discussed on IRC this was discussed
> before (me and Vesa, I think) and Vesa said he wanted to think about it.
> Would be nice if we can reach consensus on this, and someone who's
> familiarised with that area (not me) could approve those changes.
>
> But Vesa is on vacation... maybe Colin?  I don't know.  Try CCing them :-)

I think your grub_video_set_mode() is a good change.  In fact, I also moved 
that mode selection code out of gfxterm and into the video API in my graphical 
menu branch.

It seems to me that there aren't significant changes to the video subsystem 
except that grub_video_setmode(width, height, mode_type) is removed and 
replaced with a function that takes only a mode list string.  I think this is 
fine since we should (once we get the above-mentioned issues resolved to 
support various resolutions properly) never hard-code video mode info (except 
for the 'videotest' command, perhaps; rather it should always be sourced from 
the user configuration, which means it will be a mode list string anyway.

I had a minor comment regarding the patch:

--- a/video/video.c
+++ b/video/video.c
...
+grub_err_t
+grub_video_set_mode (char *modestring,
+		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
+						   struct grub_video_mode_info *mode_info))
+{
+  char *tmp;
+  char *next_mode;
+  char *current_mode;
+  char *param;
+  char *value;
+  char *modevar;
+  int width = -1;
+  int height = -1;
+  int depth = -1;
+  int flags = 0;
+
+  /* Take copy of env.var. as we don't want to modify that.  */
+  modevar = grub_strdup (modestring);

Since this is called in cases where 'modestring' is not an environment 
variable, maybe it's more correct to just say, "Copy the mode list argument 
because it will be modified."

I would declare modestring as 'const char *' to prevent accidental
modification, especially since there is code that passes string constants for
modestring.

Regards,
Colin

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Fwd: [PATCH] gfxpayload

[Robert Millan]

On Sat, May 30, 2009 at 05:23:18PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sun, May 17, 2009 at 10:36 PM, Robert Millan <rmh@aybabtu.com> wrote:
> >
> > Hi,
> >
> > On Sun, May 17, 2009 at 04:34:16PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >>  #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
> >> -#define GRUB_ASSUME_LINUX_HAS_FB_SUPPORT 0
> >> +#define DEFAULT_VIDEO_MODE "text"
> >> +#else
> >> +#define DEFAULT_VIDEO_MODE "keep"
> >>  #endif
> >
> > As discussed on IRC, this "keep" option seems unnecessary;  it just means
> > "do nothing" so we may as well #ifdef the code out.
> It may be necessary in future for linux loader to be able to check if
> current mode is acceptable for linux (e.g. vnc is not). xnu code does
> this (text mode isn't acceptable). But for now calling with keep can
> be put in ifdef..endif

Maybe, but there's no point in some part of GRUB telling another routine
"please do nothing, leave things as they are" when it could just not call
it instead.  It just wastes execution time.

> >> +             grub_error (GRUB_ERR_OUT_OF_MEMORY,
> >> +                         "couldn't allocate temporary storage");
> >> +             goto fail;
> >> +           }
> >
> > Doesn't grub_malloc set this errno / errmsg already?
> >
> It says "out of memory" but I guess it should be clear enough.

If it's not, it should be fixed in kern/mm.c.  There's nothing specific to
linux.c in this problem.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."