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

[Vladimir 'phcoder' Serbinenko]

Framebuffer patch comitted and I removed framebuf branch on my repo.
>
> OK, I did not get why fbfill.h of the three private headers.
>
> Perhaps fbutil.h which already declares blit_info would be a better place 
> then.
>
I haven't looked in depth how private headers were organised, I just
moved the same layout to fb*. Perhaps we need only one framebuffer
private header now
>>> In grub_video_vbe_setup function in vbe.c the default palette is
>>> loaded before the created render target is set as active. I guess this
>>> would be a problem if the palette was actually needed/supported.
>>> Changing the order to create, set_active, set_palette makes the code
>>> fail for me,though.
>> Works for me. Have you forgotten about 'return' ?
>
> Yes, perhaps I did something wrong when reordering the calls for the first 
> time.
>
It's already reordered in comitted version
>>>
>>> In this same code struct grub_video_mode_info mode_info is added in
>>> the last round of patches but I cannot find where the changed code
>>> touches the variable. This is somewhat odd.
>> grep is your friend:
>>      framebuffer.mode_info.width = active_mode_info.x_resolution;
>> and follows. Since now vbe.c can't manipulate directly the target.
>> Again, you're the one who requested encapsulation.
>
> Sorry, I just confused the
> grub_video_mode_info mode_info
> with the
> grub_vbe_mode_info_block mode_info
>
> Perhaps a more distinctive name for the latter would be helpful, though.
>
I'm ok with rename.
> This is a patch against the current (as far as git knows) framebuf branch:
This branch doesn't exist anymore since it's upstream ;)
>
>    * move video_fb_fender_target structure declaration to fbutil.h
Requires me to looks at how three headers are organised.
>    * remove duplicate assignment in create_render_target_from_pointer
>       + quiet warning
Is already done in committed version
>    * rename local variable in grub_video_vbe_setup
Good
>    * remove grub_video_fb_get_video_ptr from video_fb.c (dup in fbutil.c)
Good patch too
>
> Take which you want
>
Could you send them as separate patches with changelogs? It's
especially important in your case since you have no copyright
assignment yet and we have to decide which patches can go in without
such. I'm not maintainer so I need explicit consent from them to apply
your patches before you sign copyright assignment
> Thanks
>
> Michal
>
> _______________________________________________
> 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