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

[Michal Suchanek]

2009/8/10 Vladimir 'phcoder' Serbinenko <address@hidden>:
>>
>>
>> 2009/8/10 Vladimir 'phcoder' Serbinenko <address@hidden>:
>>>>
>>>> As I understand it the code checks the bounds against the active
>>>> videomode and then sets the viewport on the active render target.
>>>> Since the active render target can be arbitrarily set by the user (to,
>>>> say an offscreen bitmap for rendering the terminal text) I do not see
>>>> how these two parts match.
>>> Well this code was already there before my patch. I just restructured
>>> it. It basically requires viewport to be legal. For offscreen
>>
>> If the code works the way described above than it requires the
>> viewport to be inside the videomode bounds for any render target for
>> which viewport is set, including offscreen targets. This is not what
>> legal veiwport is about.
>>
>> If it was broken to start with then it should be perhaps fixed in a
>> separate patch before the split.
>>
> Nothing is broken here. Just we don't use the things like setting
> viewport outside the real screen. For offscreen rendering use
> grub_video_create_render_target and then blit it later on. I like this

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.

> way. Unlike setting viewport to invisible coordinates it's clean and
> doesn't create any issues if two parts of code decide to use the same
> invisible offset. If you want us to consider to change this way of
> doing you need to provide a useful testcase. Just tearing downing
> existing, working and tested code without benefit isn't good.

I do not want to tear it down - just move the check to video_fb and
check against the video mode info provided with the target  rather
than the active video mode which should do the right thing for the
screen framebuffer as well as offscreen targets.

It might just happen to do the right thing now because nobody uses
render targets larger than the screen and everybody sets the viewport
sane so the check is not needed but it's broken nonetheless.

>
>> I can try to put together a patch that fixes this. It is not unrelated
>> to the fb split, though. The driver code messes with the wrong
>> structure here because it tries to do what should be done at the fb
>> level.
> Retrieving framebuffer address and parameters has to be done in
> driver. Using any special functions to encapsulate this information
> just leads to more calls and troubles for double buffering. The design
> of video_fb.c is that it does only what it's explicitly asked to do
> and doesn't try to be complete driver

That's completely fine.

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.

>>> It isn't. If you read the code you'll see that video_render_target is
>>> anonymous structure except in drivers.
>>
>> Yes, but the driver that uses the fb library to handle rendering need
>> not touch the render target.
> Why not? It just passes parameters to values this way

Because it breaks encapsulation and needlessly duplicates code.

>> That's the point here. There are two places in the driver that work
>> with render targets but the code was not moved to video_fb.
>>
>> One is the filling out of the render target structure, the other is
>> the bounds checking for viewport.
> 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.

>> What I want to do here is the split to be exactly at the natural spot
>> - video_fb works with render targets, everything outside of there uses
>> them as opaque structures.
>>
> Then you pass video mode info which is just a screen render target

Yes, without any needless redundancy.

>>
>> If other drivers used different (accelerated) rendering methods but
>> implemented only some of the rendering functions then sharing of the
>> framebuffer might be possible by using two render targets - one for
>> the accelerated operations, the other for video_fb operations.
> Actually this is another point why driver should be able to manipulate
> framebuffer render targets. If you use acceleration then you may have
> something different than a normal framebuffer. Trying to use
> framebuffer on a screen as whole will cause strange effects. The
> driver may however ask video_fb to do operations on some parts of
> screen which are framebuffers even when the whole screen isn't. In
> this case it's the responsibility of driver to determine which
> accelerated render target corresponds to which framebuffer render
> target

For that they should be able to create and use render targets but not
manipulate them directly.

>>
>>> As for moving data from video info to render target the problem is
>>> that video_fb can't be aware about the structures used by drivers. and
>>> so I see no way around it
>>
>> I do not want to move any data here.
> Well it's exactly what you ask.

Where?

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

Thanks

Michal