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." _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel