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

[Michal Suchanek]

2009/8/10 Vladimir 'phcoder' Serbinenko <address@hidden>:
> On Mon, Aug 10, 2009 at 4:38 PM, Michal Suchanek<address@hidden> wrote:
>> Yes, but set_viewport works on the active target. You create an
>> offscreen target and set it as the active target and want to set the
>> viewport on it. Depending on its size this function either needlessly
>> limits you to the current screen mode or allows illegal viewport to be
>> set.
>>
>> It simply compares apples and oranges.
>>
> Fixed in repo. New patch is coming after testing

Thanks

>> I just want the drivers not touch the render target structure so that
>> it is possible to add features to video_fb (such as framebuffer
>> rotation) later without breaking the drivers that do not use them.
>>
>> Because it breaks encapsulation and needlessly duplicates code.
> I'm not sure that encapsulation is good in this particular case.
> Clearly the driver should take the lead and not the framebuffer
> library. Now it's coded with "library of functions" concept and
> changing concept will put strains on drivers. I don't know which
> approach is better in a long run
>>> Both pieces of code check against the current mode. It's just because
>>> vbe does most of job for us that this code is a bunch of assignments
>>> but it may be more complex. I don't see what your problem with filling
>>> render target about screen parameters is
>>
>> Exactly that it may get more complex. You have to change the code in
>> all drivers then.
> Why? Driver just sets fields framebuf? In which cases does it need to
> be modified?
>>> Then you pass video mode info which is just a screen render target
>>
>> Yes, without any needless redundancy.
> Passing framebuffer parameters one way or another you have redundancy
>> For that they should be able to create and use render targets but not
>> manipulate them directly.
>>
> They do. They need to say "this render target is at ... with these parameters"
>>
>> The mode_info is already part of the framebuffer so I only ask that
>> the driver fills in the mode_info and has a render target created from
>> that rather than creating a render target itself.
> mode_info lacks crucial data like framebuffer address.
Yes, it does.

I would like a video_fb function like
grub_video_fb_create_render_target_from_buffer(void * buffer, int
allocated, const grub_video_mode_info_t * mode_info)

>> If features are later added to the render target (such as rotation)
>> this code needs to be aware of them. However, if the structure was
>> filled out in fb_video then these new features are transparent.
>>
> But if a driver partially accelerates rotation it needs to tell which
> buffers are rotated and which aren't. Basically we have 2 possible

Yes, and it can create them rotated or not as appropriate (and specify
that in the mode_info). If the rotation is done in hardware then it
should be transparent and the buffer is effectively non-rotated as far
as video_fb cares.

> benefits but I'm not sure which one is better. What do others think?

I am sure that for doing transparent rotation in video_fb
encapsulation is good. I can do without it or patch it in with the
rotation if I get it into working state.

The only other users of the render_target structure are drivers and
from what I have seen so far they should better be kept at good
distance from it.

However, an opinion of somebody else who looks at the graphics
subsystem would be welcome.

Thanks

Michal