Re: [PATCH] 2/5 VBE double buffering

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> This patch adds double buffering support support to the VBE
> video driver including page flipping and blitting from an offscreen back
> buffer.
> 
> The patch is against GRUB trunk revision 1964.

Unless we come up with better idea we can go with this time being...
Check the comments below and act accordingly...

> === modified file 'include/grub/video.h'
> --- include/grub/video.h      2009-01-02 15:26:06 +0000
> +++ include/grub/video.h      2009-01-31 17:48:10 +0000
> @@ -47,10 +47,10 @@
>  #define GRUB_VIDEO_MODE_TYPE_DEPTH_MASK              0x0000ff00
>  #define GRUB_VIDEO_MODE_TYPE_DEPTH_POS               8
>  
> -/* Defined predefined render targets.  */
> -#define GRUB_VIDEO_RENDER_TARGET_DISPLAY     ((struct 
> grub_video_render_target *) 0)
> -#define GRUB_VIDEO_RENDER_TARGET_FRONT_BUFFER        ((struct 
> grub_video_render_target *) 0)
> -#define GRUB_VIDEO_RENDER_TARGET_BACK_BUFFER ((struct 
> grub_video_render_target *) 1)
> +/* The basic render target representing the whole display.  This always
> +   renders to the back buffer when double-buffering is in use.  */
> +#define GRUB_VIDEO_RENDER_TARGET_DISPLAY \
> +  ((struct grub_video_render_target *) 0)
>  
>  /* Defined blitting formats.  */
>  enum grub_video_blit_format
> 
> === modified file 'video/i386/pc/vbe.c'
> --- video/i386/pc/vbe.c       2009-01-02 15:26:06 +0000
> +++ video/i386/pc/vbe.c       2009-01-31 17:48:10 +0000
> @@ -62,6 +62,7 @@
>  static struct
>  {
>    struct grub_video_render_target render_target;

We do not use comments at end of line. Please move it to line before
actual variable it is describing.

> +  int is_double_buffered;       /* Is the video mode double buffered? */
>  
>    unsigned int bytes_per_scan_line;
>    unsigned int bytes_per_pixel;
> @@ -76,6 +77,24 @@
>  static grub_uint32_t mode_in_use = 0x55aa;
>  static grub_uint16_t *mode_list;
>  

Ditto for below. And after '.' in comment there should be two spaces.

> +static struct 
> +{
> +  grub_size_t page_size;        /* The size of a page in bytes. */
> +
> +  /* For page flipping strategy. */
> +  int displayed_page;           /* The page # that is the front buffer. */
> +  int render_page;              /* The page # that is the back buffer. */
> +
> +  /* For blit strategy. */
> +  grub_uint8_t *offscreen_buffer;
> +
> +  /* Virtual functions. */
> +  int (*update_screen) (void);
> +  int (*destroy) (void);
> +} doublebuf_state;
> +
> +static void double_buffering_init (void);
> +
>  static void *
>  real2pm (grub_vbe_farptr_t ptr)
>  {
> @@ -375,6 +394,7 @@
>    /* Reset frame buffer and render target variables.  */
>    grub_memset (&framebuffer, 0, sizeof(framebuffer));
>    render_target = &framebuffer.render_target;
> +  grub_memset (&doublebuf_state, 0, sizeof(doublebuf_state));
>  
>    return GRUB_ERR_NONE;
>  }
> @@ -390,6 +410,9 @@
>      /* TODO: Decide, is this something we want to do.  */
>      return grub_errno;
>  
> +  if (doublebuf_state.destroy)
> +    doublebuf_state.destroy();
> +
>    /* TODO: Free any resources allocated by driver.  */
>    grub_free (mode_list);
>    mode_list = 0;
> @@ -532,9 +555,12 @@
>        render_target->viewport.width = active_mode_info.x_resolution;
>        render_target->viewport.height = active_mode_info.y_resolution;
>  
> -      /* Set framebuffer pointer and mark it as non allocated.  */
> +      /* Mark framebuffer memory as non allocated.  */
>        render_target->is_allocated = 0;
> -      render_target->data = framebuffer.ptr;

Please add empty line here to group it a bit nicer.

> +      /* Set up double buffering information. */
> +      framebuffer.is_double_buffered =
> +        ((mode_type & GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED) != 0);
> +      double_buffering_init ();
>  
>        /* Copy default palette to initialize emulated palette.  */
>        for (i = 0;
> @@ -555,6 +581,166 @@
>    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no matching mode found.");
>  }
>  
> +/* 
> +   Set framebuffer render target page and display the proper page, based on
> +   `doublebuf_state.render_page' and `doublebuf_state.displayed_page',
> +   respectively. 
> +
> +   Returns 0 upon success, nonzero upon failure.
> + */
> +static int
> +doublebuf_pageflipping_commit (void)
> +{

Dot spaces..

> +  /* Set the render target's data pointer to the start of the render_page. */
> +  framebuffer.render_target.data =
> +    ((char *) framebuffer.ptr) +
> +    doublebuf_state.page_size * doublebuf_state.render_page;
> +
> +  /* Tell the video adapter to display the new front page. */
> +  int display_start_line =
> +    framebuffer.render_target.mode_info.height 
> +    * doublebuf_state.displayed_page;
> + 
> +  grub_vbe_status_t vbe_err = 
> +    grub_vbe_bios_set_display_start (0, display_start_line);
> +  if (vbe_err != GRUB_VBE_STATUS_OK)
> +    return 1;
> +
> +  return 0;
> +}
> +
> +static int
> +doublebuf_pageflipping_update_screen (void)
> +{

Same here...

> +  /* Swap the page numbers in the framebuffer struct. */
> +  int new_displayed_page = doublebuf_state.render_page;
> +  doublebuf_state.render_page = doublebuf_state.displayed_page;
> +  doublebuf_state.displayed_page = new_displayed_page;
> +
> +  return doublebuf_pageflipping_commit ();
> +}
> +
> +static int
> +doublebuf_pageflipping_destroy (void)
> +{
> +  doublebuf_state.update_screen = 0;
> +  doublebuf_state.destroy = 0;
> +  return 0;
> +}
> +
> +static int
> +doublebuf_pageflipping_init (void)
> +{
> +  doublebuf_state.page_size =
> +    framebuffer.bytes_per_scan_line * render_target->mode_info.height;
> +  
> +  /* Get video RAM size in bytes.  */
> +  grub_size_t vram_size = controller_info.total_memory << 16;
> +

Create new line for comment...

> +  if (2 * doublebuf_state.page_size > vram_size)
> +    return 1;   /* Not enough video memory for 2 pages. */
> +
> +  doublebuf_state.displayed_page = 0;
> +  doublebuf_state.render_page = 1;
> +
> +  doublebuf_state.update_screen = doublebuf_pageflipping_update_screen;
> +  doublebuf_state.destroy = doublebuf_pageflipping_destroy;
> +

Same *2 below...

> +  /* Set the framebuffer memory data pointer and display the right page. */
> +  if (doublebuf_pageflipping_commit () != GRUB_ERR_NONE)
> +    return 1;   /* Unable to set the display start.  */
> +
> +  framebuffer.render_target.mode_info.mode_type
> +    |= GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED;
> +  return 0;
> +}
> +
> +static int
> +doublebuf_blit_update_screen (void)
> +{
> +  grub_memcpy (framebuffer.ptr,
> +               doublebuf_state.offscreen_buffer, 
> +               doublebuf_state.page_size);
> +  return 0;
> +}
> +
> +static int
> +doublebuf_blit_destroy (void)
> +{
> +  grub_free (doublebuf_state.offscreen_buffer);
> +  doublebuf_state.offscreen_buffer = 0;
> +
> +  doublebuf_state.update_screen = 0;
> +  doublebuf_state.destroy = 0;
> +  return 0;
> +}
> +
> +static int
> +doublebuf_blit_init (void)
> +{
> +  doublebuf_state.page_size =
> +    framebuffer.bytes_per_scan_line * render_target->mode_info.height;
> +
> +  doublebuf_state.offscreen_buffer = (grub_uint8_t *)
> +    grub_malloc (doublebuf_state.page_size);

Here too... Ok... and the rest in the file ;)

> +  if (doublebuf_state.offscreen_buffer == 0)
> +    return 1;   /* Error.  */
> +
> +  framebuffer.render_target.data = doublebuf_state.offscreen_buffer;
> +  doublebuf_state.update_screen = doublebuf_blit_update_screen;
> +  doublebuf_state.destroy = doublebuf_blit_destroy;
> +
> +  framebuffer.render_target.mode_info.mode_type
> +    |= GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED;
> +  return 0;
> +}
> +
> +static int
> +doublebuf_null_update_screen (void)
> +{
> +  return 0;
> +}
> +
> +static int
> +doublebuf_null_destroy (void)
> +{
> +  doublebuf_state.update_screen = 0;
> +  doublebuf_state.destroy = 0;
> +  return 0;
> +}
> +
> +static int
> +doublebuf_null_init (void)
> +{
> +  framebuffer.render_target.data = framebuffer.ptr;
> +  doublebuf_state.update_screen = doublebuf_null_update_screen;
> +  doublebuf_state.destroy = doublebuf_null_destroy;
> +
> +  framebuffer.render_target.mode_info.mode_type
> +    &= ~GRUB_VIDEO_MODE_TYPE_DOUBLE_BUFFERED;
> +  return 0;
> +}
> +
> +/* Select the best double buffering mode available.  */
> +static void
> +double_buffering_init (void)
> +{
> +  if (doublebuf_state.destroy)
> +    doublebuf_state.destroy();
> +
> +  if (framebuffer.is_double_buffered)
> +    {
> +      if (doublebuf_pageflipping_init () == 0)
> +        return;
> +
> +      if (doublebuf_blit_init () == 0)
> +        return;
> +    }
> +
> +  /* Fall back to no double buffering. */
> +  doublebuf_null_init ();
> +}
> +
>  static grub_err_t
>  grub_video_vbe_get_info (struct grub_video_mode_info *mode_info)
>  {
> @@ -1443,7 +1629,10 @@
>  static grub_err_t
>  grub_video_vbe_swap_buffers (void)
>  {
> -  /* TODO: Implement buffer swapping.  */
> +  if (doublebuf_state.update_screen () != 0)
> +    return grub_error (GRUB_ERR_INVALID_COMMAND,
> +                       "Double buffer update failed");
> +
>    return GRUB_ERR_NONE;
>  }
>  
> @@ -1542,17 +1731,13 @@
>  static grub_err_t
>  grub_video_vbe_set_active_render_target (struct grub_video_render_target 
> *target)
>  {
> -  if (target == GRUB_VIDEO_RENDER_TARGET_FRONT_BUFFER)
> +  if (target == GRUB_VIDEO_RENDER_TARGET_DISPLAY)
>      {
>        render_target = &framebuffer.render_target;
>  
>        return GRUB_ERR_NONE;
>      }
>  
> -  if (target == GRUB_VIDEO_RENDER_TARGET_BACK_BUFFER)
> -    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -                       "double buffering not implemented yet.");
> -
>    if (! target->data)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT,
>                         "invalid render target given.");