Re: Fwd: [PATCH 1/2] Framebuffer split

[Robert Millan <rmh@aybabtu.com>]

On Mon, Jul 27, 2009 at 12:06:17AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>    grub_err_t (*get_info) (struct grub_video_mode_info *mode_info);
>  
> +  grub_err_t (*get_info_and_fini) (struct grub_video_mode_info *mode_info,
> +				   void **framebuffer);
> +
> [...]
> +/* Framebuffer address may change as a part of normal operation
> +   (e.g. double buffering). That's why you need to stop video subsystem to be
> +   sure that framebuffer address doesn't change. To ensure this abstraction
> +   grub_video_get_info_and_fini is the only function supplying framebuffer
> +   address. */
> +grub_err_t grub_video_get_info_and_fini (struct grub_video_mode_info *mode_info,
> +					 void **framebuffer);
> +

I see that returning framebuffer address and finishing the video subsystem
must be together, but is there a reason to couple this with getting mode_info ?

If mode_info is also affected by this problem, or if getting mode info only
makes sense in a situation in which we'd also want to obtain framebuffer
address or finishing the subsystem, then the existing get_info() function
is no longer necessary.

Otherwise, users who want both things can just invoke get_info() first and then
the new function to obtain FB address.

Btw, if I understand correctly, we have a race condition right now.  As a
bugfix it'd be better to merge this separately from the interface redesign if
possible.

Also, does finishing the video subsystem only affect GRUB internally, or
result in any effect at the display level?  I want to avoid having visual
glitches when the payload is loaded without switching video mode.

> -  modevar = grub_env_get ("gfxpayload");
> -
> -  /* Now all graphical modes are acceptable.
> -     May change in future if we have modes without framebuffer.  */
> [...]
> -    {
> -      params->have_vga = GRUB_VIDEO_TYPE_TEXT;
> -      params->video_width = 80;
> -      params->video_height = 25;
> -    }
> -

Why is this chunk of code moved down?  AFAICS, this change only involves
adding an additional layer between it and the video backend.  Does this
make it conflict with something else?

> +#define grub_video_render_target grub_video_fbrender_target

If we want to rename this function, I'd rather do it all the way than
keeping a compatibility macro.  But then, I'd also prefer if this is
done separately from the rest (either before or after).

> +/* Select the best double buffering mode available.  */
> +static void
> +double_buffering_init (int enable)

This patch also seems to add double buffering support, which is a feature
that wasn't yet implemented.  I suppose it's good to have (I'm not very
clued about graphics), but I'd prefer if we can merge this separately
from the redesign changes.

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