Re: Minor framebuffer cleanup patches

[Vladimir 'phcoder' Serbinenko]

On Sat, Aug 15, 2009 at 12:44 AM, Michal Suchanek<address@hidden> wrote:
> 2009/8/14 Vladimir 'phcoder' Serbinenko <address@hidden>:
>> On Fri, Aug 14, 2009 at 4:04 PM, Michal Suchanek<address@hidden> wrote:
>>> Hello
>>>
>>> I am sending the rest of framebuffer patches that were not included in
>>> the split by Vladimir.
>>>
>>> remove-dup-function.patch
>>>
>>> * video_fb.c: remove grub_video_fb_get_video_ptr which is duplicated in 
>>> fbutil.c
>>> * fbutil.c: copy the note from fbfill.c
>> I think the comment should go to video_fb.h to instruct future video
>> driver writers.
>> Could you create entries in the same format as Changelog
>
> * video/fb/video_fb.c (grub_video_fb_get_video_ptr): remove function
> (duplicated in fbutil.c)
> * video/fb/fbutil.c: copy the note from fbfill.c
>
> Attaching additional patch that moves the note about memory
> architecture in video_fb.h
>
> * video_fb.h: move the note about memory architecture from fb*.c here
>
>>>
>>> rename-var.patch
>>>
>>> vbe.c: grub_video_vbe_setup rename local variable
>>>  - this should make the distinction between grub_vbe_mode_info_block
>>> and grub_video_mode_info clearer in this function
>>>
>> Perhaps best_mode_info should be renamed too, for consistency.
>> Other than this I'm ok with both patches.
>
> I meant this only as a reminder that the mode_info that is used all
> the time really refers to a vbe mode (as opposed to framebuffer mode)
> bu it is certainly possible to rename
>
> Attaching an alternate patch that renames all the vbe mode related locals.
>
>
>    * video/i386/pc/vbe.c: rename local variables to make clearer
>      distinction between vbe modes and framebuffer modes
>
>
> While making this patch I noticed that
>
> - grub_vbe_set_video_mode sets the mode in framebuffer.active_mode
> and grub_video_vbe_setup in vbe_mode_in_use. Either this is redundant
> or any call to grub_vbe_set_video_mode without going through
> grub_video_vbe_setup makes the mode_in_use value obsolete - invalid.
>
> - grub_vbe_set_video_mode changes active_mode_info before setting the
> mode (and before checking it can be used)
>
> - grub_vbe_setup calls grub_video_set_video_mode with pointer to
> active_mode_info as argument which causes call to memcpy with this
> pointer as both src and dest
>
> This is resolved with additional patch:
>
> * video/i386/pc/vbe.c: remove vbe_mode_in_use (duplicated in
> framebuffer.active_vbe_mode)
> * video/i386/pc/vbe.c (grub_vbe_set_video_mode): save active mode info
> only after setting the mode
I comitted three patches to branches michal[123] on my git. If
maintainer confirms it's ok for inclusion (legal reasons) I'll commit
to mainstream. You can see on my git how Changelog entries for you
changes should look like
Patch move-memarch-comment.patch is garbled


-- 
Regards
Vladimir 'phcoder' Serbinenko

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