From: Thomas P. <tp...@pc...> - 2008-01-28 10:03:56
|
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 <tp...@pc...> --- 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; |
From: Andrew M. <ak...@li...> - 2008-01-31 22:07:46
|
On Mon, 28 Jan 2008 11:03:38 +0100 (CET) Thomas Pfaff <tp...@pc...> 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. |
From: Andrew M. <ak...@li...> - 2008-01-31 22:19:08
|
On Thu, 31 Jan 2008 14:07:01 -0800 Andrew Morton <ak...@li...> 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. |
From: Geert U. <ge...@li...> - 2008-02-01 08:02:26
|
On Thu, 31 Jan 2008, Andrew Morton wrote: > On Mon, 28 Jan 2008 11:03:38 +0100 (CET) Thomas Pfaff <tp...@pc...> 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 -- ge...@li... 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 |
From: Thomas P. <tp...@pc...> - 2008-02-01 11:59:11
|
On Thu, 31 Jan 2008, Andrew Morton wrote: > On Mon, 28 Jan 2008 11:03:38 +0100 (CET) > Thomas Pfaff <tp...@pc...> 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 |
From: Geert U. <ge...@li...> - 2008-02-01 12:25:08
|
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 -- ge...@li... 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 |
From: Thomas P. <tp...@pc...> - 2008-02-01 13:13:45
|
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. |
From: Geert U. <ge...@li...> - 2008-02-01 13:38:22
|
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 -- ge...@li... 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 |
From: Thomas P. <tp...@pc...> - 2008-02-01 13:55:49
|
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. > |