[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir 'phcoder' Serbinenko
Subject: Re: Fwd: [PATCH 1/2] Framebuffer split
Date: Fri, 14 Aug 2009 13:41:14 +0200

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
> I am not sure if this is to hide the structure from other code but it
> logically belongs to video_fb.h.
Again, you're the one who requested encapsulation.
> 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' ?
> 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.
> In create_render_target_from_pointer the is_allocated is set to 0 twice.
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)
> Thanks
> Michal
> _______________________________________________
> Grub-devel mailing list
> address@hidden

Vladimir 'phcoder' Serbinenko

Personal git repository:

reply via email to

[Prev in Thread] Current Thread [Next in Thread]