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
> address@hidden
> 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."