[PATCH] grub_video_fbrender_target

[PATCH] grub_video_fbrender_target

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]


According to its description, struct grub_video_fbrender_target is a
driver-specific structure.  video_fb.c is generic code and shouldn't be
using this struct to define its function calls, as this makes it impossible
to use any of them from outside a driver.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: fbrender.diff --]
[-- Type: text/x-diff, Size: 8145 bytes --]

=== modified file 'include/grub/video_fb.h'
--- include/grub/video_fb.h	2009-08-17 13:34:24 +0000
+++ include/grub/video_fb.h	2009-11-22 12:45:18 +0000
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2005,2006,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2005,2006,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -28,8 +28,6 @@
 
 struct grub_video_fbblit_info;
 
-struct grub_video_fbrender_target;
-
 #define GRUB_VIDEO_FBSTD_NUMCOLORS 16
 extern struct grub_video_palette_data grub_video_fbstd_colors[GRUB_VIDEO_FBSTD_NUMCOLORS];
 
@@ -88,7 +86,7 @@ grub_video_fb_blit_bitmap (struct grub_v
 			   unsigned int width, unsigned int height);
 
 grub_err_t
-grub_video_fb_blit_render_target (struct grub_video_fbrender_target *source,
+grub_video_fb_blit_render_target (struct grub_video_render_target *source,
                                    enum grub_video_blit_operators oper,
                                    int x, int y, int offset_x, int offset_y,
 				  unsigned int width, unsigned int height);
@@ -97,22 +95,22 @@ grub_err_t
 grub_video_fb_scroll (grub_video_color_t color, int dx, int dy);
 
 grub_err_t
-grub_video_fb_create_render_target (struct grub_video_fbrender_target **result,
+grub_video_fb_create_render_target (struct grub_video_render_target **result,
 				    unsigned int width, unsigned int height,
 				    unsigned int mode_type __attribute__ ((unused)));
 
 grub_err_t
-grub_video_fb_create_render_target_from_pointer (struct grub_video_fbrender_target **result,
+grub_video_fb_create_render_target_from_pointer (struct grub_video_render_target **result,
 						 const struct grub_video_mode_info *mode_info,
 						 void *ptr);
 
 grub_err_t
-grub_video_fb_delete_render_target (struct grub_video_fbrender_target *target);
+grub_video_fb_delete_render_target (struct grub_video_render_target *target);
 
 grub_err_t
-grub_video_fb_get_active_render_target (struct grub_video_fbrender_target **target);
+grub_video_fb_get_active_render_target (struct grub_video_render_target **target);
 
 grub_err_t
-grub_video_fb_set_active_render_target (struct grub_video_fbrender_target *target);
+grub_video_fb_set_active_render_target (struct grub_video_render_target *target);
 
 #endif /* ! GRUB_VIDEO_FB_HEADER */

=== modified file 'video/fb/video_fb.c'
--- video/fb/video_fb.c	2009-08-28 13:54:20 +0000
+++ video/fb/video_fb.c	2009-11-22 12:43:40 +0000
@@ -839,13 +839,14 @@ grub_video_fb_blit_bitmap (struct grub_v
 }
 
 grub_err_t
-grub_video_fb_blit_render_target (struct grub_video_fbrender_target *source,
+grub_video_fb_blit_render_target (struct grub_video_render_target *source,
                                    enum grub_video_blit_operators oper,
                                    int x, int y, int offset_x, int offset_y,
                                    unsigned int width, unsigned int height)
 {
   struct grub_video_fbblit_info source_info;
   struct grub_video_fbblit_info target_info;
+  struct grub_video_fbrender_target *sourcefb = (void *) source;
 
   /* Make sure there is something to do.  */
   if ((width == 0) || (height == 0))
@@ -854,14 +855,14 @@ grub_video_fb_blit_render_target (struct
     return GRUB_ERR_NONE;
   if ((y >= (int)render_target->viewport.height) || (y + (int)height < 0))
     return GRUB_ERR_NONE;
-  if ((x + (int)source->mode_info.width) < 0)
+  if ((x + (int)sourcefb->mode_info.width) < 0)
     return GRUB_ERR_NONE;
-  if ((y + (int)source->mode_info.height) < 0)
+  if ((y + (int)sourcefb->mode_info.height) < 0)
     return GRUB_ERR_NONE;
-  if ((offset_x >= (int)source->mode_info.width)
+  if ((offset_x >= (int)sourcefb->mode_info.width)
       || (offset_x + (int)width < 0))
     return GRUB_ERR_NONE;
-  if ((offset_y >= (int)source->mode_info.height)
+  if ((offset_y >= (int)sourcefb->mode_info.height)
       || (offset_y + (int)height < 0))
     return GRUB_ERR_NONE;
 
@@ -900,25 +901,25 @@ grub_video_fb_blit_render_target (struct
   if ((y + height) > render_target->viewport.height)
     height = render_target->viewport.height - y;
 
-  if ((offset_x + width) > source->mode_info.width)
-    width = source->mode_info.width - offset_x;
-  if ((offset_y + height) > source->mode_info.height)
-    height = source->mode_info.height - offset_y;
+  if ((offset_x + width) > sourcefb->mode_info.width)
+    width = sourcefb->mode_info.width - offset_x;
+  if ((offset_y + height) > sourcefb->mode_info.height)
+    height = sourcefb->mode_info.height - offset_y;
 
   /* Limit drawing to source render target dimensions.  */
-  if (width > source->mode_info.width)
-    width = source->mode_info.width;
+  if (width > sourcefb->mode_info.width)
+    width = sourcefb->mode_info.width;
 
-  if (height > source->mode_info.height)
-    height = source->mode_info.height;
+  if (height > sourcefb->mode_info.height)
+    height = sourcefb->mode_info.height;
 
   /* Add viewport offset.  */
   x += render_target->viewport.x;
   y += render_target->viewport.y;
 
   /* Use fbblit_info to encapsulate rendering.  */
-  source_info.mode_info = &source->mode_info;
-  source_info.data = source->data;
+  source_info.mode_info = &sourcefb->mode_info;
+  source_info.data = sourcefb->data;
   target_info.mode_info = &render_target->mode_info;
   target_info.data = render_target->data;
 
@@ -1036,7 +1037,7 @@ grub_video_fb_scroll (grub_video_color_t
 
 
 grub_err_t
-grub_video_fb_create_render_target (struct grub_video_fbrender_target **result,
+grub_video_fb_create_render_target (struct grub_video_render_target **result,
 				    unsigned int width, unsigned int height,
 				    unsigned int mode_type __attribute__ ((unused)))
 {
@@ -1103,13 +1104,13 @@ grub_video_fb_create_render_target (stru
   /* TODO: Add render target to render target list.  */
 
   /* Save result to caller.  */
-  *result = target;
+  *result = (void *) target;
 
   return GRUB_ERR_NONE;
 }
 
 grub_err_t
-grub_video_fb_create_render_target_from_pointer (struct grub_video_fbrender_target **result,
+grub_video_fb_create_render_target_from_pointer (struct grub_video_render_target **result,
 						 const struct grub_video_mode_info *mode_info,
 						 void *ptr)
 {
@@ -1139,46 +1140,52 @@ grub_video_fb_create_render_target_from_
 		 mode_info->bytes_per_pixel * mode_info->width);
 
   /* Save result to caller.  */
-  *result = target;
+  *result = (void *) target;
 
   return GRUB_ERR_NONE;
 }
 
 grub_err_t
-grub_video_fb_delete_render_target (struct grub_video_fbrender_target *target)
+grub_video_fb_delete_render_target (struct grub_video_render_target *target)
 {
+  struct grub_video_fbrender_target *targetfb = (void *) target;
+
   /* If there is no target, then just return without error.  */
-  if (! target)
+  if (! targetfb)
     return GRUB_ERR_NONE;
 
   /* TODO: Delist render target from render target list.  */
 
   /* If this is software render target, free it's memory.  */
-  if (target->is_allocated)
-    grub_free (target->data);
+  if (targetfb->is_allocated)
+    grub_free (targetfb->data);
 
   /* Free render target.  */
-  grub_free (target);
+  grub_free (targetfb);
 
   return GRUB_ERR_NONE;
 }
 
 grub_err_t
-grub_video_fb_set_active_render_target (struct grub_video_fbrender_target *target)
+grub_video_fb_set_active_render_target (struct grub_video_render_target *target)
 {
-  if (! target->data)
+  struct grub_video_fbrender_target *targetfb = (void *) target;
+
+  if (! targetfb->data)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
                        "invalid render target given.");
 
-  render_target = target;
+  render_target = targetfb;
 
   return GRUB_ERR_NONE;
 }
 
 grub_err_t
-grub_video_fb_get_active_render_target (struct grub_video_fbrender_target **target)
+grub_video_fb_get_active_render_target (struct grub_video_render_target **target)
 {
-  *target = render_target;
+  struct grub_video_fbrender_target **targetfb = (void *) target;
+
+  *targetfb = render_target;
 
   return GRUB_ERR_NONE;
 }

Re: [PATCH] grub_video_fbrender_target

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

Robert Millan wrote:
> According to its description, struct grub_video_fbrender_target is a
> driver-specific structure.  video_fb.c is generic code and shouldn't be
> using this struct to define its function calls, as this makes it impossible
> to use any of them from outside a driver.
>
>   
grub_video_render_target is driver-specific but
grub_video_fbrender_target isn't. Host driver can either put
#define grub_video_render_target grub_video_fbrender_target
In which case driver effectively adopts fbrender_target as its
render_target or add encapsulators for video_fb functions. I think this
patch does more harm than good since if grub_video_render_target isn't
grub_video_fbrender_target then compiler won't complain.
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: [PATCH] grub_video_fbrender_target

[Robert Millan]

On Sun, Nov 22, 2009 at 02:01:00PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > According to its description, struct grub_video_fbrender_target is a
> > driver-specific structure.  video_fb.c is generic code and shouldn't be
> > using this struct to define its function calls, as this makes it impossible
> > to use any of them from outside a driver.
> >
> >   
> grub_video_render_target is driver-specific but
> grub_video_fbrender_target isn't. Host driver can either put
> #define grub_video_render_target grub_video_fbrender_target
> In which case driver effectively adopts fbrender_target as its
> render_target or add encapsulators for video_fb functions. I think this
> patch does more harm than good since if grub_video_render_target isn't
> grub_video_fbrender_target then compiler won't complain.

Ah, I see what you mean.  This is unfortunate, as drivers can't be offloaded
from the obligation to define generic handlers just to assert that they use
the same struct definition.

I was trying to simplify driver registration by making it optional to define
operations, making the fallback to generic code automatic.

But well, it's not so important.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."