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

[Vladimir 'phcoder' Serbinenko]

>
> 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 
> ?
When designing this part I thought of accelerated graphics. Before
loading the OS they would switch to standard framebuffer mode this may
also affect the formats used so the info may be different from
get_info
>
> 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.
race condition? We don't even have threads
>
> 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.
>
Other than addition of double buffering changes will not result in any
visible effect.
>> -  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?
>
I wanted to keep normal grub_printf as long as possible and after
get_mode_and_fini grub_printf may be unfunctional.
>> +#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).
>
It's not about renaming but to inform includes that
grub_video_render_target is in fact grub_video_fbrender_target and so
avoid warnings and casts.
>> +/* 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.
>
Ok, I will try to split while keeping patch functional.
> --
> 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
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git