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

[Vladimir 'phcoder' Serbinenko]

On Fri, Aug 14, 2009 at 3:23 PM, Vladimir 'phcoder'
Serbinenko<address@hidden> wrote:
> 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
And please consider this thread closed and post your patches in a new thread
>> 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
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

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