[PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Thomas Pfaff]

Patch recreated for 2.6.24.

The current attr_fgcol_ec / attr_bgcol_ec macros do a simple shift of bits to 
get the color from vc_video_erase_char. For a monochrome display however the 
attribute does not contain any color, only attribute bits. Furthermore the 
reverse bit is lost because it is shifted out, the resulting color is always 0.

This can bee seen on a monochrome console either directly or by setting it to 
inverse mode via "setterm -inversescreen on" . Text is written with correct 
color, fb_fillrects from a bit_clear / bit_clear_margins will get wrong colors.

Kind Regards,
Thomas Pfaff

Signed-off-by: Thomas Pfaff <tpfaff@pcs.com> 
---
diff -urp a/drivers/video/console/bitblit.c b/drivers/video/console/bitblit.c
--- a/drivers/video/console/bitblit.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/bitblit.c	2008-01-28 10:39:44.000000000 +0100
@@ -63,7 +63,7 @@ static void bit_clear(struct vc_data *vc
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
  	struct fb_fillrect region;

-	region.color = attr_bgcol_ec(bgshift, vc);
+	region.color = attr_bgcol_ec(bgshift, vc, info);
  	region.dx = sx * vc->vc_font.width;
  	region.dy = sy * vc->vc_font.height;
  	region.width = width * vc->vc_font.width;
@@ -213,7 +213,7 @@ static void bit_clear_margins(struct vc_
  	unsigned int bs = info->var.yres - bh;
  	struct fb_fillrect region;

-	region.color = attr_bgcol_ec(bgshift, vc);
+	region.color = attr_bgcol_ec(bgshift, vc, info);
  	region.rop = ROP_COPY;

  	if (rw && !bottom_only) {
diff -urp a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
--- a/drivers/video/console/fbcon.c	2008-01-28 10:24:50.000000000 +0100
+++ b/drivers/video/console/fbcon.c	2008-01-28 10:39:44.000000000 +0100
@@ -334,10 +334,7 @@ static inline int get_color(struct vc_da
  	switch (depth) {
  	case 1:
  	{
-		int col = ~(0xfff << (max(info->var.green.length,
-					  max(info->var.red.length,
-					      info->var.blue.length)))) & 0xff;
-
+		int col = mono_col(info);
  		/* 0 or 1 */
  		int fg = (info->fix.visual != FB_VISUAL_MONO01) ? col : 0;
  		int bg = (info->fix.visual != FB_VISUAL_MONO01) ? 0 : col;
diff -urp a/drivers/video/console/fbcon_ccw.c b/drivers/video/console/fbcon_ccw.c
--- a/drivers/video/console/fbcon_ccw.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/fbcon_ccw.c	2008-01-28 10:39:44.000000000 +0100
@@ -84,7 +84,7 @@ static void ccw_clear(struct vc_data *vc
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
  	u32 vyres = GETVYRES(ops->p->scrollmode, info);

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.dx = sy * vc->vc_font.height;
  	region.dy = vyres - ((sx + width) * vc->vc_font.width);
  	region.height = width * vc->vc_font.width;
@@ -198,7 +198,7 @@ static void ccw_clear_margins(struct vc_
  	struct fb_fillrect region;
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.rop = ROP_COPY;

  	if (rw && !bottom_only) {
diff -urp a/drivers/video/console/fbcon_cw.c b/drivers/video/console/fbcon_cw.c
--- a/drivers/video/console/fbcon_cw.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/fbcon_cw.c	2008-01-28 10:39:44.000000000 +0100
@@ -70,7 +70,7 @@ static void cw_clear(struct vc_data *vc,
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
  	u32 vxres = GETVXRES(ops->p->scrollmode, info);

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.dx = vxres - ((sy + height) * vc->vc_font.height);
  	region.dy = sx *  vc->vc_font.width;
  	region.height = width * vc->vc_font.width;
@@ -182,7 +182,7 @@ static void cw_clear_margins(struct vc_d
  	struct fb_fillrect region;
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.rop = ROP_COPY;

  	if (rw && !bottom_only) {
diff -urp a/drivers/video/console/fbcon.h b/drivers/video/console/fbcon.h
--- a/drivers/video/console/fbcon.h	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/fbcon.h	2008-01-28 10:39:44.000000000 +0100
@@ -93,10 +93,6 @@ struct fbcon_ops {
  	(((s) >> (fgshift)) & 0x0f)
  #define attr_bgcol(bgshift,s)    \
  	(((s) >> (bgshift)) & 0x0f)
-#define	attr_bgcol_ec(bgshift,vc) \
-	((vc) ? (((vc)->vc_video_erase_char >> (bgshift)) & 0x0f) : 0)
-#define attr_fgcol_ec(fgshift,vc) \
-	((vc) ? (((vc)->vc_video_erase_char >> (fgshift)) & 0x0f) : 0)

  /* Monochrome */
  #define attr_bold(s) \
@@ -108,6 +104,49 @@ struct fbcon_ops {
  #define attr_blink(s) \
  	((s) & 0x8000)

+#define mono_col(info)							\
+	(~(0xfff << (max((info)->var.green.length,			\
+			 max((info)->var.red.length,			\
+			     (info)->var.blue.length)))) & 0xff) 
+
+static inline int attr_col_ec(int shift, struct vc_data *vc,
+			      struct fb_info *info, int is_fg)
+{
+	int is_mono01;
+	int col;
+	int fg;
+	int bg;
+
+	if (!vc)
+		return 0;
+
+	if (vc->vc_can_do_color)
+		return is_fg ? attr_fgcol(shift,vc->vc_video_erase_char)
+			: attr_bgcol(shift,vc->vc_video_erase_char);
+
+	if (!info)
+		return 0;
+
+	col = mono_col(info);
+	is_mono01 = info->fix.visual == FB_VISUAL_MONO01;
+
+	if (attr_reverse(vc->vc_video_erase_char)) {
+		fg = is_mono01 ? col : 0;
+		bg = is_mono01 ? 0 : col;
+	}
+	else {
+		fg = is_mono01 ? 0 : col;
+		bg = is_mono01 ? col : 0;
+	}
+
+	return is_fg ? fg : bg;
+}
+
+#define attr_bgcol_ec(bgshift,vc,info)		\
+	attr_col_ec(bgshift,vc,info,0);
+#define attr_fgcol_ec(fgshift,vc,info)		\
+	attr_col_ec(fgshift,vc,info,1);
+
  /* Font */
  #define REFCOUNT(fd)	(((int *)(fd))[-1])
  #define FNTSIZE(fd)	(((int *)(fd))[-2])
diff -urp a/drivers/video/console/fbcon_ud.c b/drivers/video/console/fbcon_ud.c
--- a/drivers/video/console/fbcon_ud.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/fbcon_ud.c	2008-01-28 10:39:44.000000000 +0100
@@ -71,7 +71,7 @@ static void ud_clear(struct vc_data *vc,
  	u32 vyres = GETVYRES(ops->p->scrollmode, info);
  	u32 vxres = GETVXRES(ops->p->scrollmode, info);

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.dy = vyres - ((sy + height) * vc->vc_font.height);
  	region.dx = vxres - ((sx + width) *  vc->vc_font.width);
  	region.width = width * vc->vc_font.width;
@@ -228,7 +228,7 @@ static void ud_clear_margins(struct vc_d
  	struct fb_fillrect region;
  	int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;

-	region.color = attr_bgcol_ec(bgshift,vc);
+	region.color = attr_bgcol_ec(bgshift,vc,info);
  	region.rop = ROP_COPY;

  	if (rw && !bottom_only) {
diff -urp a/drivers/video/console/tileblit.c b/drivers/video/console/tileblit.c
--- a/drivers/video/console/tileblit.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/console/tileblit.c	2008-01-28 10:39:44.000000000 +0100
@@ -40,8 +40,8 @@ static void tile_clear(struct vc_data *v

  	rect.index = vc->vc_video_erase_char &
  		((vc->vc_hi_font_mask) ? 0x1ff : 0xff);
-	rect.fg = attr_fgcol_ec(fgshift, vc);
-	rect.bg = attr_bgcol_ec(bgshift, vc);
+	rect.fg = attr_fgcol_ec(fgshift, vc, info);
+	rect.bg = attr_bgcol_ec(bgshift, vc, info);
  	rect.sx = sx;
  	rect.sy = sy;
  	rect.width = width;
diff -urp a/drivers/video/pmag-aa-fb.c b/drivers/video/pmag-aa-fb.c
--- a/drivers/video/pmag-aa-fb.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/video/pmag-aa-fb.c	2008-01-28 10:39:44.000000000 +0100
@@ -150,7 +150,7 @@ static int aafbcon_set_font(struct displ
  {
  	struct aafb_info *info = (struct aafb_info *)disp->fb_info;
  	struct aafb_cursor *c = &info->cursor;
-	u8 fgc = ~attr_bgcol_ec(disp, disp->conp);
+	u8 fgc = ~attr_bgcol_ec(disp, disp->conp, &info->info);

  	if (width > 64 || height > 64 || width < 0 || height < 0)
  		return -EINVAL;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Andrew Morton]

On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
Thomas Pfaff <tpfaff@pcs.com> wrote:

> Patch recreated for 2.6.24.
> 
> The current attr_fgcol_ec / attr_bgcol_ec macros do a simple shift of bits to 
> get the color from vc_video_erase_char. For a monochrome display however the 
> attribute does not contain any color, only attribute bits. Furthermore the 
> reverse bit is lost because it is shifted out, the resulting color is always 0.
> 
> This can bee seen on a monochrome console either directly or by setting it to 
> inverse mode via "setterm -inversescreen on" . Text is written with correct 
> color, fb_fillrects from a bit_clear / bit_clear_margins will get wrong colors.
> 

I'm struggling to understand the seriousness of this bug.  Do you think
this patch needs to be backported to 2.6.24.x?

> +#define mono_col(info)							\
> +	(~(0xfff << (max((info)->var.green.length,			\
> +			 max((info)->var.red.length,			\
> +			     (info)->var.blue.length)))) & 0xff) 

This macro is dangerous because it references its arg more than a single
time.  If someone does mono_col(*foo++) they will get a surprise.

So please follow the general rule here: do not implement in a macro that
which could have been implemented in a C function.

Plus C fundtions have better type checking and for some reason people are
more likely to document C functions.

> +static inline int attr_col_ec(int shift, struct vc_data *vc,
> +			      struct fb_info *info, int is_fg)
> +{
> +	int is_mono01;
> +	int col;
> +	int fg;
> +	int bg;
> +
> +	if (!vc)
> +		return 0;
> +
> +	if (vc->vc_can_do_color)
> +		return is_fg ? attr_fgcol(shift,vc->vc_video_erase_char)
> +			: attr_bgcol(shift,vc->vc_video_erase_char);
> +
> +	if (!info)
> +		return 0;
> +
> +	col = mono_col(info);
> +	is_mono01 = info->fix.visual == FB_VISUAL_MONO01;
> +
> +	if (attr_reverse(vc->vc_video_erase_char)) {
> +		fg = is_mono01 ? col : 0;
> +		bg = is_mono01 ? 0 : col;
> +	}
> +	else {
> +		fg = is_mono01 ? 0 : col;
> +		bg = is_mono01 ? col : 0;
> +	}
> +
> +	return is_fg ? fg : bg;
> +}

This function is vastly too large to be inlined.  Even mono_col() is too
large to be inlined.

Please implement both as regular C functions, then take a look at the
before and after output from size(1).

> +#define attr_bgcol_ec(bgshift,vc,info)		\
> +	attr_col_ec(bgshift,vc,info,0);
> +#define attr_fgcol_ec(fgshift,vc,info)		\
> +	attr_col_ec(fgshift,vc,info,1);
> +

Could be implemented as C functions.

I'll queue the pastch up for testing as-is, but would like a de-macroed,
de-inlined version please.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Andrew Morton]

On Thu, 31 Jan 2008 14:07:01 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> I'll queue the pastch up for testing as-is, but would like a de-macroed,
> de-inlined version please.

Please also pass the resulting patch through scripts/checkpatch.pl.  This
version generated 23 checkpatch errors.

Also, for some reason the patch didn't apply with `patch -p1': 100%
rejects.  It applied OK with patch's "-l" option.  I didn't investigate why
this happened, but I guess it's your email client playing games.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Geert Uytterhoeven]

On Thu, 31 Jan 2008, Andrew Morton wrote:
> On Mon, 28 Jan 2008 11:03:38 +0100 (CET) Thomas Pfaff <tpfaff@pcs.com> wrote:
> > +#define attr_bgcol_ec(bgshift,vc,info)		\
> > +	attr_col_ec(bgshift,vc,info,0);
> > +#define attr_fgcol_ec(fgshift,vc,info)		\
> > +	attr_col_ec(fgshift,vc,info,1);
> > +
> 
> Could be implemented as C functions.

I don't see a real advantage in using a function instead. It will just
become longer. There won't be any additional type checking.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Thomas Pfaff]

On Thu, 31 Jan 2008, Andrew Morton wrote:

> On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
> Thomas Pfaff <tpfaff@pcs.com> wrote:
> 
> > Patch recreated for 2.6.24.
> > 
> > The current attr_fgcol_ec / attr_bgcol_ec macros do a simple shift of bits to 
> > get the color from vc_video_erase_char. For a monochrome display however the 
> > attribute does not contain any color, only attribute bits. Furthermore the 
> > reverse bit is lost because it is shifted out, the resulting color is always 0.
> > 
> > This can bee seen on a monochrome console either directly or by setting it to 
> > inverse mode via "setterm -inversescreen on" . Text is written with correct 
> > color, fb_fillrects from a bit_clear / bit_clear_margins will get wrong colors.
> > 
> 
> I'm struggling to understand the seriousness of this bug.  Do you think
> this patch needs to be backported to 2.6.24.x?

I think that only a few people would ever notice it and the bug is far too 
harmless to be in a bugfix release. 

> > +#define mono_col(info)							\
> > +	(~(0xfff << (max((info)->var.green.length,			\
> > +			 max((info)->var.red.length,			\
> > +			     (info)->var.blue.length)))) & 0xff) 
> 
> This macro is dangerous because it references its arg more than a single
> time.  If someone does mono_col(*foo++) they will get a surprise.
> 
> So please follow the general rule here: do not implement in a macro that
> which could have been implemented in a C function.
> 
> Plus C fundtions have better type checking and for some reason people are
> more likely to document C functions.
> 

I agree. This was a quick copy from the fbcon.c source.

> > +static inline int attr_col_ec(int shift, struct vc_data *vc,
> > +			      struct fb_info *info, int is_fg)
> > +{
> > +	int is_mono01;
> > +	int col;
> > +	int fg;
> > +	int bg;
> > +
> > +	if (!vc)
> > +		return 0;
> > +
> > +	if (vc->vc_can_do_color)
> > +		return is_fg ? attr_fgcol(shift,vc->vc_video_erase_char)
> > +			: attr_bgcol(shift,vc->vc_video_erase_char);
> > +
> > +	if (!info)
> > +		return 0;
> > +
> > +	col = mono_col(info);
> > +	is_mono01 = info->fix.visual == FB_VISUAL_MONO01;
> > +
> > +	if (attr_reverse(vc->vc_video_erase_char)) {
> > +		fg = is_mono01 ? col : 0;
> > +		bg = is_mono01 ? 0 : col;
> > +	}
> > +	else {
> > +		fg = is_mono01 ? 0 : col;
> > +		bg = is_mono01 ? col : 0;
> > +	}
> > +
> > +	return is_fg ? fg : bg;
> > +}
> 
> This function is vastly too large to be inlined.  Even mono_col() is too
> large to be inlined.
> 
> Please implement both as regular C functions, then take a look at the
> before and after output from size(1).
> 
> > +#define attr_bgcol_ec(bgshift,vc,info)		\
> > +	attr_col_ec(bgshift,vc,info,0);
> > +#define attr_fgcol_ec(fgshift,vc,info)		\
> > +	attr_col_ec(fgshift,vc,info,1);
> > +
> 
> Could be implemented as C functions.
> 
> I'll queue the pastch up for testing as-is, but would like a de-macroed,
> de-inlined version please.
> 
> 

If the functions should be deinlined someone should decide where to put them. 
Since they must be exported i would avoid to put them into fbcon.c, this would 
lead to cross dependencies.
I would suggest to put the prototype into <linux/fb.h> and the definition into 
fbcmap.c instead, but i am unsure whether they really belong there.

Regarding the checkpatch problem the errors are all coding style related. I have 
decided to keep the coding style that was used in the source file, i think the 
style should be consistent in a source file.

And i forgot a "quell-flowed-text" feature in pine that i did not need in older 
pine versions, therefore the patch was garbled. Sorry.

I will be on vacation next week, answers will take a while.

Greetings,
Thomas

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Geert Uytterhoeven]

On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> On Thu, 31 Jan 2008, Andrew Morton wrote:
> > On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
> If the functions should be deinlined someone should decide where to put them. 
> Since they must be exported i would avoid to put them into fbcon.c, this would 
> lead to cross dependencies.
> I would suggest to put the prototype into <linux/fb.h> and the definition into 
> fbcmap.c instead, but i am unsure whether they really belong there.

General note: functions in header files must be inline, else you get
multiple definitions of the same symbol.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Thomas Pfaff]

On Fri, 1 Feb 2008, Geert Uytterhoeven wrote:
> On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> > On Thu, 31 Jan 2008, Andrew Morton wrote:
> > > On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
> > If the functions should be deinlined someone should decide where to put them. 
> > Since they must be exported i would avoid to put them into fbcon.c, this would 
> > lead to cross dependencies.
> > I would suggest to put the prototype into <linux/fb.h> and the definition into 
> > fbcmap.c instead, but i am unsure whether they really belong there.
> 
> General note: functions in header files must be inline, else you get
> multiple definitions of the same symbol.
> 

Of course you are right but this does not answer my question:
Where should i put a mono_col / attr_col_ec if they must be deinlined ?

My suggestion is a fb_mono_col / fb_attr_col_ec in fbcmap.c.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Geert Uytterhoeven]

On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> On Fri, 1 Feb 2008, Geert Uytterhoeven wrote:
> > On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> > > On Thu, 31 Jan 2008, Andrew Morton wrote:
> > > > On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
> > > If the functions should be deinlined someone should decide where to put them. 
> > > Since they must be exported i would avoid to put them into fbcon.c, this would 
> > > lead to cross dependencies.
> > > I would suggest to put the prototype into <linux/fb.h> and the definition into 
> > > fbcmap.c instead, but i am unsure whether they really belong there.
> > 
> > General note: functions in header files must be inline, else you get
> > multiple definitions of the same symbol.
> 
> Of course you are right but this does not answer my question:
> Where should i put a mono_col / attr_col_ec if they must be deinlined ?

Sorry, I misunderstood. I thought you meant the macros that Andrew
wanted to be turned into functions.

> My suggestion is a fb_mono_col / fb_attr_col_ec in fbcmap.c.

As they're used by fbcon only, not by fbdev, I'd suggest fbcon.c.

pmag-aa-fb.c is the only other user, but it's broken anyway because it
still uses the 2.4.x fbdev API.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] [Resend] fbcon : fix color generation for monochrome framebuffer

[Thomas Pfaff]

On Fri, 1 Feb 2008, Geert Uytterhoeven wrote:
> On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> > On Fri, 1 Feb 2008, Geert Uytterhoeven wrote:
> > > On Fri, 1 Feb 2008, Thomas Pfaff wrote:
> > > > On Thu, 31 Jan 2008, Andrew Morton wrote:
> > > > > On Mon, 28 Jan 2008 11:03:38 +0100 (CET)
> > > > If the functions should be deinlined someone should decide where to put them. 
> > > > Since they must be exported i would avoid to put them into fbcon.c, this would 
> > > > lead to cross dependencies.
> > > > I would suggest to put the prototype into <linux/fb.h> and the definition into 
> > > > fbcmap.c instead, but i am unsure whether they really belong there.
> > > 
> > > General note: functions in header files must be inline, else you get
> > > multiple definitions of the same symbol.
> > 
> > Of course you are right but this does not answer my question:
> > Where should i put a mono_col / attr_col_ec if they must be deinlined ?
> 
> Sorry, I misunderstood. I thought you meant the macros that Andrew
> wanted to be turned into functions.
> 
> > My suggestion is a fb_mono_col / fb_attr_col_ec in fbcmap.c.
> 
> As they're used by fbcon only, not by fbdev, I'd suggest fbcon.c.
> 

IMHO putting them in fbcon.c does not work because the functions are called in 
tileblit / bitblit . Both modules are used by fbcon (bitblit directly, tileblit 
when CONFIG_FB_TILEBLITTING is defined).

When the functions are defined in fbcon.c you would get a
bitblit depends on fbcon depends on bitblit
situation.

> pmag-aa-fb.c is the only other user, but it's broken anyway because it
> still uses the 2.4.x fbdev API.
> 


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/