[PATCH 1/2] Replace hook-based with flag-based approach for choosing video mode

[PATCH 1/2] Replace hook-based with flag-based approach for choosing video mode

[Vladimir 'phcoder' Serbinenko]

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

After discussion on IRC it revealed that hook-based approach is
unpracticable because number of available modes grows exponentially
with every parameter available. Here is the patch to change to
flag-based approach

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: videomask.diff --]
[-- Type: text/plain, Size: 11775 bytes --]

diff --git a/commands/videotest.c b/commands/videotest.c
index 6fe4b9b..07f61bd 100644
--- a/commands/videotest.c
+++ b/commands/videotest.c
@@ -30,7 +30,8 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ ((unused)),
                     int argc __attribute__ ((unused)),
                     char **args __attribute__ ((unused)))
 {
-  if (grub_video_set_mode ("1024x768;800x600;640x480", 0) != GRUB_ERR_NONE)
+  if (grub_video_set_mode ("1024x768;800x600;640x480",
+			   GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 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 4145db4..94b0467 100644
--- a/include/grub/video.h
+++ b/include/grub/video.h
@@ -171,7 +171,7 @@ struct grub_video_adapter
   grub_err_t (*fini) (void);
 
   grub_err_t (*setup) (unsigned int width,  unsigned int height,
-                       unsigned int mode_type);
+                       unsigned int mode_type, unsigned int mode_mask);
 
   grub_err_t (*get_info) (struct grub_video_mode_info *mode_info);
 
@@ -307,7 +307,7 @@ 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 (const char *modestring,
-				int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
-							      struct grub_video_mode_info *mode_info));
+				unsigned int modemask,
+				unsigned int modevalue);
 
 #endif /* ! GRUB_VIDEO_HEADER */
diff --git a/loader/i386/linux.c b/loader/i386/linux.c
index 238e4cd..c5802d8 100644
--- a/loader/i386/linux.c
+++ b/loader/i386/linux.c
@@ -505,12 +505,12 @@ grub_linux_boot (void)
       if (! tmp)
 	return grub_errno;
       grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
-      err = grub_video_set_mode (tmp, 0);
+      err = grub_video_set_mode (tmp, 0, 0);
       grub_free (tmp);
     }
 #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
   else
-    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0);
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0, 0);
 #endif
 
   if (err)
diff --git a/loader/i386/pc/xnu.c b/loader/i386/pc/xnu.c
index 69a9405..adca8c6 100644
--- a/loader/i386/pc/xnu.c
+++ b/loader/i386/pc/xnu.c
@@ -28,15 +28,6 @@
 
 #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)
@@ -49,8 +40,12 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
   grub_err_t err;
 
   modevar = grub_env_get ("gfxpayload");
+  /* Consider only graphical 32-bit deep modes.  */
   if (! modevar || *modevar == 0)
-    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
+			       GRUB_VIDEO_MODE_TYPE_PURE_TEXT
+			       | GRUB_VIDEO_MODE_TYPE_DEPTH_MASK,
+			       32 << GRUB_VIDEO_MODE_TYPE_DEPTH_POS);
   else
     {
       tmp = grub_malloc (grub_strlen (modevar)
@@ -59,7 +54,10 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
 	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);
+      err = grub_video_set_mode (tmp,
+				 GRUB_VIDEO_MODE_TYPE_PURE_TEXT
+				 | GRUB_VIDEO_MODE_TYPE_DEPTH_MASK,
+				 32 << GRUB_VIDEO_MODE_TYPE_DEPTH_POS);
       grub_free (tmp);
     }
 
diff --git a/term/gfxterm.c b/term/gfxterm.c
index f161499..e6c6746 100644
--- a/term/gfxterm.c
+++ b/term/gfxterm.c
@@ -244,12 +244,6 @@ 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)
 {
@@ -269,13 +263,15 @@ grub_gfxterm_init (void)
   /* Parse gfxmode environment variable if set.  */
   modevar = grub_env_get ("gfxmode");
   if (! modevar || *modevar == 0)
-    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
+    err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
+			       GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
   else
     {
       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);
+      err = grub_video_set_mode (tmp,
+				 GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
       grub_free (tmp);
     }
 
diff --git a/video/i386/pc/vbe.c b/video/i386/pc/vbe.c
index 0244b90..c4878f6 100644
--- a/video/i386/pc/vbe.c
+++ b/video/i386/pc/vbe.c
@@ -379,7 +379,7 @@ grub_video_vbe_fini (void)
 
 static grub_err_t
 grub_video_vbe_setup (unsigned int width, unsigned int height,
-                      unsigned int mode_type)
+                      unsigned int mode_type, unsigned int mode_mask)
 {
   grub_uint16_t *p;
   struct grub_vbe_mode_info_block mode_info;
@@ -435,17 +435,22 @@ grub_video_vbe_setup (unsigned int width, unsigned int height,
         continue;
 
       /* Check if user requested RGB or index color mode.  */
-      if ((mode_type & GRUB_VIDEO_MODE_TYPE_COLOR_MASK) != 0)
+      if ((mode_mask & GRUB_VIDEO_MODE_TYPE_COLOR_MASK) != 0)
         {
-          if (((mode_type & GRUB_VIDEO_MODE_TYPE_INDEX_COLOR) != 0)
-              && (mode_info.memory_model != GRUB_VBE_MEMORY_MODEL_PACKED_PIXEL))
-            /* Requested only index color modes.  */
-            continue;
-
-          if (((mode_type & GRUB_VIDEO_MODE_TYPE_RGB) != 0)
-              && (mode_info.memory_model != GRUB_VBE_MEMORY_MODEL_DIRECT_COLOR))
-            /* Requested only RGB modes.  */
-            continue;
+	  unsigned my_mode_type = 0;
+
+	  if (mode_info.memory_model == GRUB_VBE_MEMORY_MODEL_PACKED_PIXEL)
+	    my_mode_type |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
+
+	  if (mode_info.memory_model == GRUB_VBE_MEMORY_MODEL_DIRECT_COLOR)
+	    my_mode_type |= GRUB_VIDEO_MODE_TYPE_RGB;
+
+	  if ((my_mode_type & mode_mask
+	       & (GRUB_VIDEO_MODE_TYPE_RGB | GRUB_VIDEO_MODE_TYPE_INDEX_COLOR))
+	      != (mode_type & mode_mask
+		  & (GRUB_VIDEO_MODE_TYPE_RGB
+		     | GRUB_VIDEO_MODE_TYPE_INDEX_COLOR)))
+	    continue;
         }
 
       /* If there is a request for specific depth, ignore others.  */
diff --git a/video/video.c b/video/video.c
index 36ebfd1..1c5a35d 100644
--- a/video/video.c
+++ b/video/video.c
@@ -399,8 +399,8 @@ grub_video_get_active_render_target (struct grub_video_render_target **target)
 
 grub_err_t
 grub_video_set_mode (const char *modestring,
-		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
-						   struct grub_video_mode_info *mode_info))
+		     unsigned int modemask,
+		     unsigned int modevalue)
 {
   char *tmp;
   char *next_mode;
@@ -408,10 +408,8 @@ grub_video_set_mode (const char *modestring,
   char *param;
   char *value;
   char *modevar;
-  int width = -1;
-  int height = -1;
-  int depth = -1;
-  int flags = 0;
+
+  modevalue &= modemask;
 
   /* Take copy of env.var. as we don't want to modify that.  */
   modevar = grub_strdup (modestring);
@@ -427,26 +425,26 @@ grub_video_set_mode (const char *modestring,
       || 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)
 	{
+	  struct grub_video_mode_info mode_info;
+	  grub_memset (&mode_info, 0, sizeof (mode_info));
 	  err = grub_video_get_info (&mode_info);
 	  if (err)
 	    {
 	      suitable = 0;
 	      grub_errno = GRUB_ERR_NONE;
 	    }
+	  if ((mode_info.mode_type & modemask) != modevalue)
+	    suitable = 0;
 	}
-      else
-	mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
+      else if (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) != 0)
+	       && ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) == 0))
+	suitable = 0;
 
-      if (suitable && hook)
-	suitable = hook (grub_video_adapter_active, &mode_info);
       if (suitable)
 	{
 	  grub_free (modevar);
@@ -480,15 +478,15 @@ grub_video_set_mode (const char *modestring,
   /* Loop until all modes has been tested out.  */
   while (next_mode != NULL)
     {
+      int width = -1;
+      int height = -1;
+      int depth = -1;
+      unsigned int flags = modevalue;
+      unsigned int flagmask = modemask;
+
       /* 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 == ';')
@@ -517,9 +515,8 @@ grub_video_set_mode (const char *modestring,
 	  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))
+	  if (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) == 0)
+	      || ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) != 0))
 	    {
 	      /* Valid mode found from adapter, and it has been activated.
 		 Specify it as active adapter.  */
@@ -635,18 +632,20 @@ grub_video_set_mode (const char *modestring,
 
       /* 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 check if this depth is supported.  */
+      if (depth != -1 && (flagmask & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
+	  &&
+	  (((flags & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
+	    != ((depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
+		& GRUB_VIDEO_MODE_TYPE_DEPTH_MASK))))
+	continue;
 
-      /* 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;
+	{
+	  flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
+	    & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+	  flagmask |= GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+	}
 
       /* Try to initialize requested mode.  Ignore any errors.  */
       grub_video_adapter_t p;
@@ -668,7 +667,7 @@ grub_video_set_mode (const char *modestring,
 	    }
 
 	  /* Try to initialize video mode.  */
-	  err = p->setup (width, height, flags);
+	  err = p->setup (width, height, flags, flagmask);
 	  if (err != GRUB_ERR_NONE)
 	    {
 	      p->fini ();
@@ -684,7 +683,15 @@ grub_video_set_mode (const char *modestring,
 	      continue;
 	    }
 
-	  if (hook && ! hook (p, &mode_info))
+	  flags = mode_info.mode_type & ~GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+	  flags |= (mode_info.bpp << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
+	    & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
+
+	  /* Check that mode is suitable for upper layer.  */
+	  if ((flags & GRUB_VIDEO_MODE_TYPE_PURE_TEXT)
+	      ? (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) != 0)
+		 && ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) == 0))
+	      : ((flags & modemask) != modevalue))
 	    {
 	      p->fini ();
 	      grub_errno = GRUB_ERR_NONE;

Re: [PATCH 1/2] Replace hook-based with flag-based approach for choosing video mode

[Robert Millan]

On Sun, Aug 16, 2009 at 08:51:06PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> After discussion on IRC it revealed that hook-based approach is
> unpracticable because number of available modes grows exponentially
> with every parameter available. Here is the patch to change to
> flag-based approach

Please can you ellaborate on this?  (if you still have a copy of the
discussion, it'd be useful to post it)


> diff --git a/commands/videotest.c b/commands/videotest.c
> index 6fe4b9b..07f61bd 100644
> --- a/commands/videotest.c
> +++ b/commands/videotest.c
> @@ -30,7 +30,8 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ ((unused)),
>                      int argc __attribute__ ((unused)),
>                      char **args __attribute__ ((unused)))
>  {
> -  if (grub_video_set_mode ("1024x768;800x600;640x480", 0) != GRUB_ERR_NONE)
> +  if (grub_video_set_mode ("1024x768;800x600;640x480",
> +			   GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 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 4145db4..94b0467 100644
> --- a/include/grub/video.h
> +++ b/include/grub/video.h
> @@ -171,7 +171,7 @@ struct grub_video_adapter
>    grub_err_t (*fini) (void);
>  
>    grub_err_t (*setup) (unsigned int width,  unsigned int height,
> -                       unsigned int mode_type);
> +                       unsigned int mode_type, unsigned int mode_mask);
>  
>    grub_err_t (*get_info) (struct grub_video_mode_info *mode_info);
>  
> @@ -307,7 +307,7 @@ 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 (const char *modestring,
> -				int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
> -							      struct grub_video_mode_info *mode_info));
> +				unsigned int modemask,
> +				unsigned int modevalue);
>  
>  #endif /* ! GRUB_VIDEO_HEADER */
> diff --git a/loader/i386/linux.c b/loader/i386/linux.c
> index 238e4cd..c5802d8 100644
> --- a/loader/i386/linux.c
> +++ b/loader/i386/linux.c
> @@ -505,12 +505,12 @@ grub_linux_boot (void)
>        if (! tmp)
>  	return grub_errno;
>        grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
> -      err = grub_video_set_mode (tmp, 0);
> +      err = grub_video_set_mode (tmp, 0, 0);
>        grub_free (tmp);
>      }
>  #ifndef GRUB_ASSUME_LINUX_HAS_FB_SUPPORT
>    else
> -    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0);
> +    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0, 0);
>  #endif
>  
>    if (err)
> diff --git a/loader/i386/pc/xnu.c b/loader/i386/pc/xnu.c
> index 69a9405..adca8c6 100644
> --- a/loader/i386/pc/xnu.c
> +++ b/loader/i386/pc/xnu.c
> @@ -28,15 +28,6 @@
>  
>  #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)
> @@ -49,8 +40,12 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
>    grub_err_t err;
>  
>    modevar = grub_env_get ("gfxpayload");
> +  /* Consider only graphical 32-bit deep modes.  */
>    if (! modevar || *modevar == 0)
> -    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
> +    err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
> +			       GRUB_VIDEO_MODE_TYPE_PURE_TEXT
> +			       | GRUB_VIDEO_MODE_TYPE_DEPTH_MASK,
> +			       32 << GRUB_VIDEO_MODE_TYPE_DEPTH_POS);
>    else
>      {
>        tmp = grub_malloc (grub_strlen (modevar)
> @@ -59,7 +54,10 @@ grub_xnu_set_video (struct grub_xnu_boot_params *params)
>  	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);
> +      err = grub_video_set_mode (tmp,
> +				 GRUB_VIDEO_MODE_TYPE_PURE_TEXT
> +				 | GRUB_VIDEO_MODE_TYPE_DEPTH_MASK,
> +				 32 << GRUB_VIDEO_MODE_TYPE_DEPTH_POS);
>        grub_free (tmp);
>      }
>  
> diff --git a/term/gfxterm.c b/term/gfxterm.c
> index f161499..e6c6746 100644
> --- a/term/gfxterm.c
> +++ b/term/gfxterm.c
> @@ -244,12 +244,6 @@ 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)
>  {
> @@ -269,13 +263,15 @@ grub_gfxterm_init (void)
>    /* Parse gfxmode environment variable if set.  */
>    modevar = grub_env_get ("gfxmode");
>    if (! modevar || *modevar == 0)
> -    err = grub_video_set_mode (DEFAULT_VIDEO_MODE, video_hook);
> +    err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
> +			       GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
>    else
>      {
>        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);
> +      err = grub_video_set_mode (tmp,
> +				 GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
>        grub_free (tmp);
>      }
>  
> diff --git a/video/i386/pc/vbe.c b/video/i386/pc/vbe.c
> index 0244b90..c4878f6 100644
> --- a/video/i386/pc/vbe.c
> +++ b/video/i386/pc/vbe.c
> @@ -379,7 +379,7 @@ grub_video_vbe_fini (void)
>  
>  static grub_err_t
>  grub_video_vbe_setup (unsigned int width, unsigned int height,
> -                      unsigned int mode_type)
> +                      unsigned int mode_type, unsigned int mode_mask)
>  {
>    grub_uint16_t *p;
>    struct grub_vbe_mode_info_block mode_info;
> @@ -435,17 +435,22 @@ grub_video_vbe_setup (unsigned int width, unsigned int height,
>          continue;
>  
>        /* Check if user requested RGB or index color mode.  */
> -      if ((mode_type & GRUB_VIDEO_MODE_TYPE_COLOR_MASK) != 0)
> +      if ((mode_mask & GRUB_VIDEO_MODE_TYPE_COLOR_MASK) != 0)
>          {
> -          if (((mode_type & GRUB_VIDEO_MODE_TYPE_INDEX_COLOR) != 0)
> -              && (mode_info.memory_model != GRUB_VBE_MEMORY_MODEL_PACKED_PIXEL))
> -            /* Requested only index color modes.  */
> -            continue;
> -
> -          if (((mode_type & GRUB_VIDEO_MODE_TYPE_RGB) != 0)
> -              && (mode_info.memory_model != GRUB_VBE_MEMORY_MODEL_DIRECT_COLOR))
> -            /* Requested only RGB modes.  */
> -            continue;
> +	  unsigned my_mode_type = 0;
> +
> +	  if (mode_info.memory_model == GRUB_VBE_MEMORY_MODEL_PACKED_PIXEL)
> +	    my_mode_type |= GRUB_VIDEO_MODE_TYPE_INDEX_COLOR;
> +
> +	  if (mode_info.memory_model == GRUB_VBE_MEMORY_MODEL_DIRECT_COLOR)
> +	    my_mode_type |= GRUB_VIDEO_MODE_TYPE_RGB;
> +
> +	  if ((my_mode_type & mode_mask
> +	       & (GRUB_VIDEO_MODE_TYPE_RGB | GRUB_VIDEO_MODE_TYPE_INDEX_COLOR))
> +	      != (mode_type & mode_mask
> +		  & (GRUB_VIDEO_MODE_TYPE_RGB
> +		     | GRUB_VIDEO_MODE_TYPE_INDEX_COLOR)))
> +	    continue;
>          }
>  
>        /* If there is a request for specific depth, ignore others.  */
> diff --git a/video/video.c b/video/video.c
> index 36ebfd1..1c5a35d 100644
> --- a/video/video.c
> +++ b/video/video.c
> @@ -399,8 +399,8 @@ grub_video_get_active_render_target (struct grub_video_render_target **target)
>  
>  grub_err_t
>  grub_video_set_mode (const char *modestring,
> -		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
> -						   struct grub_video_mode_info *mode_info))
> +		     unsigned int modemask,
> +		     unsigned int modevalue)
>  {
>    char *tmp;
>    char *next_mode;
> @@ -408,10 +408,8 @@ grub_video_set_mode (const char *modestring,
>    char *param;
>    char *value;
>    char *modevar;
> -  int width = -1;
> -  int height = -1;
> -  int depth = -1;
> -  int flags = 0;
> +
> +  modevalue &= modemask;
>  
>    /* Take copy of env.var. as we don't want to modify that.  */
>    modevar = grub_strdup (modestring);
> @@ -427,26 +425,26 @@ grub_video_set_mode (const char *modestring,
>        || 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)
>  	{
> +	  struct grub_video_mode_info mode_info;
> +	  grub_memset (&mode_info, 0, sizeof (mode_info));
>  	  err = grub_video_get_info (&mode_info);
>  	  if (err)
>  	    {
>  	      suitable = 0;
>  	      grub_errno = GRUB_ERR_NONE;
>  	    }
> +	  if ((mode_info.mode_type & modemask) != modevalue)
> +	    suitable = 0;
>  	}
> -      else
> -	mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_PURE_TEXT;
> +      else if (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) != 0)
> +	       && ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) == 0))
> +	suitable = 0;
>  
> -      if (suitable && hook)
> -	suitable = hook (grub_video_adapter_active, &mode_info);
>        if (suitable)
>  	{
>  	  grub_free (modevar);
> @@ -480,15 +478,15 @@ grub_video_set_mode (const char *modestring,
>    /* Loop until all modes has been tested out.  */
>    while (next_mode != NULL)
>      {
> +      int width = -1;
> +      int height = -1;
> +      int depth = -1;
> +      unsigned int flags = modevalue;
> +      unsigned int flagmask = modemask;
> +
>        /* 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 == ';')
> @@ -517,9 +515,8 @@ grub_video_set_mode (const char *modestring,
>  	  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))
> +	  if (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) == 0)
> +	      || ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) != 0))
>  	    {
>  	      /* Valid mode found from adapter, and it has been activated.
>  		 Specify it as active adapter.  */
> @@ -635,18 +632,20 @@ grub_video_set_mode (const char *modestring,
>  
>        /* 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 check if this depth is supported.  */
> +      if (depth != -1 && (flagmask & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
> +	  &&
> +	  (((flags & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
> +	    != ((depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
> +		& GRUB_VIDEO_MODE_TYPE_DEPTH_MASK))))
> +	continue;
>  
> -      /* 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;
> +	{
> +	  flags |= (depth << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
> +	    & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
> +	  flagmask |= GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
> +	}
>  
>        /* Try to initialize requested mode.  Ignore any errors.  */
>        grub_video_adapter_t p;
> @@ -668,7 +667,7 @@ grub_video_set_mode (const char *modestring,
>  	    }
>  
>  	  /* Try to initialize video mode.  */
> -	  err = p->setup (width, height, flags);
> +	  err = p->setup (width, height, flags, flagmask);
>  	  if (err != GRUB_ERR_NONE)
>  	    {
>  	      p->fini ();
> @@ -684,7 +683,15 @@ grub_video_set_mode (const char *modestring,
>  	      continue;
>  	    }
>  
> -	  if (hook && ! hook (p, &mode_info))
> +	  flags = mode_info.mode_type & ~GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
> +	  flags |= (mode_info.bpp << GRUB_VIDEO_MODE_TYPE_DEPTH_POS)
> +	    & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK;
> +
> +	  /* Check that mode is suitable for upper layer.  */
> +	  if ((flags & GRUB_VIDEO_MODE_TYPE_PURE_TEXT)
> +	      ? (((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modemask) != 0)
> +		 && ((GRUB_VIDEO_MODE_TYPE_PURE_TEXT & modevalue) == 0))
> +	      : ((flags & modemask) != modevalue))
>  	    {
>  	      p->fini ();
>  	      grub_errno = GRUB_ERR_NONE;

> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
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 1/2] Replace hook-based with flag-based approach for choosing video mode

[Vladimir 'phcoder' Serbinenko]

On Mon, Aug 17, 2009 at 3:51 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Sun, Aug 16, 2009 at 08:51:06PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> After discussion on IRC it revealed that hook-based approach is
>> unpracticable because number of available modes grows exponentially
>> with every parameter available. Here is the patch to change to
>> flag-based approach
>
> Please can you ellaborate on this?  (if you still have a copy of the
> discussion, it'd be useful to post it)
>
There was no discussion on this point. But current design is to call a
hook with every available mode and see if it approves the mode.
Unfortunately it doesn't work as expected because some modes aren't
presented by driver unless explicitly requested. This may cause
appropriate mode to be missed. Making driver iterate through a lot of
modes will slow down mode switching. With flag-based approach driver
knows which mode upper layer wants and so can present appropriate one.
It has a drawback of limiting number of filtering possibilities but
currently no subsystem needs more than is done with this patch. If it
changes in the future interface can be extended to have new flags or
parameters

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH 1/2] Replace hook-based with flag-based approach for choosing video mode

[Robert Millan]

On Fri, Aug 21, 2009 at 02:54:32PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Mon, Aug 17, 2009 at 3:51 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Sun, Aug 16, 2009 at 08:51:06PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> After discussion on IRC it revealed that hook-based approach is
> >> unpracticable because number of available modes grows exponentially
> >> with every parameter available. Here is the patch to change to
> >> flag-based approach
> >
> > Please can you ellaborate on this?  (if you still have a copy of the
> > discussion, it'd be useful to post it)
> >
> There was no discussion on this point. But current design is to call a
> hook with every available mode and see if it approves the mode.
> Unfortunately it doesn't work as expected because some modes aren't
> presented by driver unless explicitly requested. This may cause
> appropriate mode to be missed. Making driver iterate through a lot of
> modes will slow down mode switching. With flag-based approach driver
> knows which mode upper layer wants and so can present appropriate one.
> It has a drawback of limiting number of filtering possibilities but
> currently no subsystem needs more than is done with this patch. If it
> changes in the future interface can be extended to have new flags or
> parameters

Thank you.

-- 
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 1/2] Replace hook-based with flag-based approach for choosing video mode

[Robert Millan]

I've only one comment, the rest looks fine.

Btw, please include a ChangeLog entry next time.

On Sun, Aug 16, 2009 at 08:51:06PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> diff --git a/commands/videotest.c b/commands/videotest.c
> index 6fe4b9b..07f61bd 100644
> --- a/commands/videotest.c
> +++ b/commands/videotest.c
> @@ -30,7 +30,8 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ ((unused)),
>                      int argc __attribute__ ((unused)),
>                      char **args __attribute__ ((unused)))
>  {
> -  if (grub_video_set_mode ("1024x768;800x600;640x480", 0) != GRUB_ERR_NONE)
> +  if (grub_video_set_mode ("1024x768;800x600;640x480",
> +			   GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0) != GRUB_ERR_NONE)
>      return grub_errno;
> [...]
>  grub_err_t grub_video_set_mode (const char *modestring,
> -				int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,
> -							      struct grub_video_mode_info *mode_info));
> +				unsigned int modemask,
> +				unsigned int modevalue);

People usually associate "pure text" mode with a managed mode like 80x25
vga text.  I understand here it means something else, but is there some
way this could be clearer?  Perhaps a different macro name or a comment.

-- 
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 1/2] Replace hook-based with flag-based approach for choosing video mode

[Vladimir 'phcoder' Serbinenko]

On Sun, Aug 23, 2009 at 12:30 PM, Robert Millan<rmh@aybabtu.com> wrote:
>
> I've only one comment, the rest looks fine.
>
> Btw, please include a ChangeLog entry next time.
Ok. I will from now one but it will also mean a delay sending patches ;)
> People usually associate "pure text" mode with a managed mode like 80x25
> vga text.  I understand here it means something else,
It means exactly the modes like 80x25


-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH 1/2] Replace hook-based with flag-based approach for choosing video mode

[Robert Millan]

On Sun, Aug 23, 2009 at 12:58:46PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Btw, please include a ChangeLog entry next time.
> Ok. I will from now one but it will also mean a delay sending patches ;)

I know it's a PITA, but consider that it needs to be done anyway.  When you
think about that, it's not so bad :-)

> > People usually associate "pure text" mode with a managed mode like 80x25
> > vga text.  I understand here it means something else,
> It means exactly the modes like 80x25

Then I don't understand what

  grub_video_set_mode ("1024x768;800x600;640x480", GRUB_VIDEO_MODE_TYPE_PURE_TEXT)

could mean.  Are you requesting a graphical mode or a text one?  If it's a
text one, why the list of resolutions?

-- 
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 1/2] Replace hook-based with flag-based approach for choosing video mode

[Vladimir 'phcoder' Serbinenko]

>
> Then I don't understand what
>
>  grub_video_set_mode ("1024x768;800x600;640x480", GRUB_VIDEO_MODE_TYPE_PURE_TEXT)
>
> could mean.  Are you requesting a graphical mode or a text one?  If it's a
> text one, why the list of resolutions?
Actually it's
grub_video_set_mode ("auto", GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
And it means: "I care whether it's PURE_TEXT and want it not to be one (0)"
>
> --
> 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."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git