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

[Vladimir 'phcoder' Serbinenko]

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