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

[Michal Suchanek]

2009/8/14 Vladimir 'phcoder' Serbinenko <address@hidden>:
> On Fri, Aug 14, 2009 at 2:13 AM, Michal Suchanek<address@hidden> wrote:
>> 2009/8/13 Vladimir 'phcoder' Serbinenko <address@hidden>:
>>>> Considering that vbe.c and sdl.c currently aren't affected a lot
>>>> whether there is or there isn't encapsulation in place, I'm ok tih
>>>> encapsulating it but if any driver needs the breach of encapsulation
>>>> it will be broken and I'll post no opposition to it.
>>>> I'll do the encapsulation tomorrow.
>>> Patch attached. One incremental version (for review) and one complete
>>> (for patching)
>>
>> The patch as presented here works for me.
>>
>> However, I wonder some of the changes:
>>
>> In the latest round of patches struct grub_video_fbrender_target
>> declaration was needlessly and somewhat illogically moved from
>> video_fb.h to fbfill.h.
> fbfill.h is a private header whereas video_fb.h is public one. You're
> the one who requested encapsulation

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.

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

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

>>
>> In create_render_target_from_pointer the is_allocated is set to 0 twice.
> Fixed
>>
> I'm running the series of tests now and when I'm finished I'll commit
> it. After that you're welcome to submit patches for the problems you
> spot (you already talked about few)

This is a patch against the current (as far as git knows) framebuf branch:

    * move video_fb_fender_target structure declaration to fbutil.h
    * remove duplicate assignment in create_render_target_from_pointer
       + quiet warning
    * rename local variable in grub_video_vbe_setup
    * remove grub_video_fb_get_video_ptr from video_fb.c (dup in fbutil.c)

Take which you want

Thanks

Michal

fbsplit_cleanup.patch
Description: Text Data