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

[Michal Suchanek]

2009/8/10 Vladimir 'phcoder' Serbinenko <address@hidden>:
> On Sun, Aug 9, 2009 at 1:55 PM, Michal Suchanek<address@hidden> wrote:
>> Hello,
>>
>> both in the previous complete patch and Vladimir's git repository
>> introduce a grub_video_rect_t type.
> This is from Collin's double buffering patch.

OK, let's drop rectangles from here.


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.

> rendering use allocate_render_target
> In my patch I tried not to modify the behaviour as far as possible.
> Your comments about improving the design are welcome but could you
> start a separate thread? If an improvement that you propose is small
> in code send a patch together with proposal - it allows to speak about
> something that is here rather something that could be. If it's bigger
> then discuss before implementing, perhaps send a small pseudo-patch
> which shows how it will be implemented. Pseudo patches don't have to
> work but are just for discussion.

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.


2009/8/10 Vladimir 'phcoder' Serbinenko <address@hidden>:
> On Sun, Aug 9, 2009 at 1:55 PM, Michal Suchanek<address@hidden> wrote:

>> The other thing that bothers me is that the video drivers fill in the
>> render target structure during video initialization.
>>
>> In my view the render target structure should not be public.
> 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.

>> The video
>> driver should fill in the video mode structure and then call a
>> function that fills in render target based on video mode and video
>> data (framebuffer memory). This code is currently replicated
>> needlessly in the vbe and sdl drivers.
> I split on the most natural spot I found - render targets. Everything
> that works with render targets is moved to video_fb. Perhaps it's not

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.

> the best spot but cutting at natural function ensures drivers won't
> have to undo something video_fb does. In future if drivers share code
> which can be moved to vide_fb then I'm ok with it but I prefer to see
> more real drivers first before fine-tuning split limit especially away
> fro ma natural spot

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.

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. Making
that work with things like doublebufferenig might be somewhat
challenging, though.

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

Thanks

Michal