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

[Robert Millan <rmh@aybabtu.com>]

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."