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

[Michal Suchanek]

2009/8/8 Michal Suchanek <address@hidden>:
> Hello
>
> I cannot get any sense of these patches "with moving code around
> omitted" so I tried the git repository in Vladimir's signature.
>
> Maybe I am missing something but the function of this code escapes me:
>
> static grub_err_t
> grub_video_vbe_set_viewport (unsigned int x, unsigned int y,
>                            unsigned int width, unsigned int height)
> {
>  /* Make sure viewport is withing screen dimensions.  If viewport was set
>     to be out of the region, mark its size as zero.  */
>  if (x > active_mode_info.x_resolution)
>    {
>      x = 0;
>      width = 0;
>    }
>
>  if (y > active_mode_info.y_resolution)
>    {
>      y = 0;
>      height = 0;
>    }
>
>  if (x + width > active_mode_info.x_resolution)
>    width = active_mode_info.x_resolution - x;
>
>  if (y + height > active_mode_info.y_resolution)
>    height = active_mode_info.y_resolution - y;
>  return grub_video_fb_set_viewport (x, y, width, height);
> }
>
> 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.
>

It seems that this check should be done in grub_video_fb_set_viewport
instead, and this function completely eliminated until viewport on
non-active target can be set. The assumption about bounds being
checked beforehand could break for the blit* functions in fbblit.c
because the video_fb blit dispatcher only checks viewport on the
target, and viewport is not checked against mode when set.

There is get_data_ptr in fbutil.c and grub_video_fb_get_video_ptr in
video_fb.c that seem to do the same thing. They do not have the
warning abut bounds checking which they do not do. This warning is
also missing from get/set_pixel.

Perhaps there should also be get/set_pixel method for video adapters?
The functions in fbutil.c probably should not be used from outside fb
this library but there are no user counterparts in video_fb.c

Thanks

Michal