2003-03-26 19:45:59

by James Simmons

[permalink] [raw]
Subject: Framebuffer fixes.


Okay. Here you go. This patch is against 2.5.66 vanialla. I tested to see
if it applied. It does. Basically I added back in the static buffers in
accel_cursor in fbcon.c. Now the cursor will work just like it did before.
The draw back is that if you have more than one framebuffer then the
cursors will be messed up. So for single headed frmaebuffer systems it
will work perfectly. It is not that big of a deal since the console layer
is broken for multi-head and pre-emptive support anyways. Plus fbcon has
issues as well. The proper fix would require a huge amount of work.
I have a few updated drivers as well. Please test.

http://phoenix.infradead.org/~jsimmons/fbdev.diff.gz

drivers/video/aty/aty128fb.c | 16 +-
drivers/video/cfbimgblt.c | 4
drivers/video/console/fbcon.c | 246 +++++++++++++++++++++-------------------
drivers/video/controlfb.c | 18 --
drivers/video/fbmem.c | 42 ++----
drivers/video/i810/i810.h | 6
drivers/video/i810/i810_accel.c | 140 +++++++++++-----------
drivers/video/i810/i810_dvt.c | 3
drivers/video/i810/i810_gtf.c | 7 -
drivers/video/i810/i810_main.c | 135 +++++++++------------
drivers/video/i810/i810_main.h | 4
drivers/video/logo/logo.c | 69 +++++------
drivers/video/platinumfb.c | 28 +---
drivers/video/radeonfb.c | 10 +
drivers/video/riva/fbdev.c | 2
drivers/video/softcursor.c | 95 ++++-----------
drivers/video/tdfxfb.c | 18 +-
drivers/video/tgafb.c | 2
drivers/video/vga16fb.c | 4
include/linux/fb.h | 4
include/linux/linux_logo.h | 2
21 files changed, 393 insertions(+), 462 deletions(-)



2003-03-27 03:05:06

by Antonino A. Daplas

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Thu, 2003-03-27 at 03:57, James Simmons wrote:
>
> Okay. Here you go. This patch is against 2.5.66 vanialla. I tested to see
> if it applied. It does. Basically I added back in the static buffers in
> accel_cursor in fbcon.c. Now the cursor will work just like it did before.
> The draw back is that if you have more than one framebuffer then the
> cursors will be messed up. So for single headed frmaebuffer systems it
> will work perfectly. It is not that big of a deal since the console layer
> is broken for multi-head and pre-emptive support anyways. Plus fbcon has
> issues as well. The proper fix would require a huge amount of work.
> I have a few updated drivers as well. Please test.

James,

1 The patch still has the atomic_dec(...) in fb_show_logo(). It's not
needed and is actually disruptive.

2. You can still avoid static buffers by just pre-allocating them per
device during fbcon_startup(). We therefore avoid multiple devices
using the same buffers.

3. We can now use a more "lightweight" lock (spin_lock/unlock, instead
of spin_lock_irqsave/unlock_irqrestore) in fb_get_buffer_offset.

4 A new bit definition, FB_CUR_SETFLASH for fb_cursor.set to mean that
the command came from fb_callback/fb_vbl_handler. Drivers can test this
flag and if set, can ignore the entire command if it has support for
hardware cursor blinking.

5. logo fixes
- forgotten case statement for FB_VISUAL_PSEUDOCOLOR.

- image->depth should be representative of the data depth
(currently, either 8 or 1). If image->depth == 1, color expansion can
now be used to draw the logo, thus there's no need to differentiate
between mono logo drawing and monochrome expansion.


The patch is against 2.5.66 + fbdev.diff.gz.

Tony

diff -Naur linux-2.5.66-orig/drivers/video/cfbimgblt.c linux-2.5.66/drivers/video/cfbimgblt.c
--- linux-2.5.66-orig/drivers/video/cfbimgblt.c 2003-03-26 23:46:32.000000000 +0000
+++ linux-2.5.66/drivers/video/cfbimgblt.c 2003-03-27 02:44:35.000000000 +0000
@@ -346,7 +346,7 @@
else
slow_imageblit(image, p, dst1, fgcolor, bgcolor,
start_index, pitch_index);
- } else if (image->depth <= bpp)
+ } else
color_imageblit(image, p, dst1, start_index, pitch_index);
}

diff -Naur linux-2.5.66-orig/drivers/video/console/fbcon.c linux-2.5.66/drivers/video/console/fbcon.c
--- linux-2.5.66-orig/drivers/video/console/fbcon.c 2003-03-26 23:46:32.000000000 +0000
+++ linux-2.5.66/drivers/video/console/fbcon.c 2003-03-27 00:31:27.000000000 +0000
@@ -172,7 +172,7 @@
* Internal routines
*/
static void fbcon_set_display(struct vc_data *vc, int init, int logo);
-static void accel_cursor(struct vc_data *vc, struct fb_info *info,
+static void accel_cursor(struct vc_data *vc, struct fb_info *info,
struct fb_cursor *cursor, int yy);
static __inline__ int real_y(struct display *p, int ypos);
static __inline__ void updatescrollmode(struct display *p, struct vc_data *vc);
@@ -206,13 +206,13 @@
return;

if (vbl_cursor_cnt && --vbl_cursor_cnt == 0) {
- cursor.set = 0;
+ cursor.set = FB_CUR_SETFLASH;

if (!cursor_drawn)
cursor.set = FB_CUR_SETCUR;
accel_cursor(vc, info, &cursor, real_y(p, vc->vc_y));
cursor_drawn ^= 1;
- vbl_cursor_cnt = cursor_blink_rate;
+ vbl_cursor_cnt = cursor_blink_rate;
}
}

@@ -220,9 +220,9 @@
{
struct fb_info *info = dev_id;

- schedule_work(&info->queue);
+ schedule_work(&info->queue);
}
-
+
static void cursor_timer_handler(unsigned long dev_addr);

static struct timer_list cursor_timer =
@@ -232,7 +232,7 @@
{
struct fb_info *info = (struct fb_info *) dev_addr;

- schedule_work(&info->queue);
+ schedule_work(&info->queue);
cursor_timer.expires = jiffies + HZ / 50;
add_timer(&cursor_timer);
}
@@ -530,6 +530,7 @@
* Low Level Operations
*/
/* NOTE: fbcon cannot be __init: it may be called from take_over_console later */
+
static const char *fbcon_startup(void)
{
const char *display_desc = "frame buffer device";
@@ -593,9 +594,16 @@
return NULL;
}

+ /* Allocate private data */
+ info->fbcon_priv = kmalloc(sizeof(struct fbcon_private), GFP_KERNEL);
+ if (info->fbcon_priv == NULL) {
+ kfree(vc);
+ return NULL;
+ }
+
/* Initialize the work queue */
INIT_WORK(&info->queue, fb_callback, info);
-
+
/* Setup default font */
vc->vc_font.data = font->data;
vc->vc_font.width = font->width;
@@ -993,103 +1001,108 @@
accel_putcs(vc, info, s, count, real_y(p, ypos), xpos);
}

-static void accel_cursor(struct vc_data *vc, struct fb_info *info,
- struct fb_cursor *cursor, int yy)
+void accel_cursor(struct vc_data *vc, struct fb_info *info,
+ struct fb_cursor *cursor, int yy)
{
+ struct fbcon_private *priv = (struct fbcon_private *)info->fbcon_priv;
unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
int fgshift = (vc->vc_hi_font_mask) ? 9 : 8;
- static int fgcolor, bgcolor, shape, width, height;
- static char mask[64], image[64], *dest;
- char *font;
- int c;
+ int height, width, size, c;
+ int w, cur_height, i = 0;
+ u8 *font;
+
+ if (cursor->set & FB_CUR_SETCUR)
+ cursor->enable = 1;
+ else
+ cursor->enable = 0;

- if (cursor->set & FB_CUR_SETCUR)
- cursor->enable = 1;
- else
- cursor->enable = 0;

- cursor->set = FB_CUR_SETPOS;
+ height = priv->cursor.image.height;
+ width = priv->cursor.image.width;
+
+ if (width != vc->vc_font.width ||
+ height != vc->vc_font.height) {
+ priv->cursor.image.width = vc->vc_font.width;
+ priv->cursor.image.height = vc->vc_font.height;
+ height = priv->cursor.image.height;
+ width = priv->cursor.image.width;
+ cursor->set |= FB_CUR_SETSIZE;
+ }
+
+ if (priv->cursor.image.dx != vc->vc_x * width ||
+ priv->cursor.image.dy != yy * height) {
+ priv->cursor.image.dx = vc->vc_x * width;
+ priv->cursor.image.dy = yy * height;
+ cursor->set |= FB_CUR_SETPOS;
+ }

- if (width != vc->vc_font.width || height != vc->vc_font.height) {
- width = vc->vc_font.width;
- height = vc->vc_font.height;
- cursor->set |= FB_CUR_SETSIZE;
- }
+ size = ((width + 7) >> 3) * height;

- if ((vc->vc_cursor_type & 0x0f) != shape) {
- shape = vc->vc_cursor_type & 0x0f;
+ if (cursor->set & FB_CUR_SETSIZE) {
+ memset(priv->image, 0xff, size);
cursor->set |= FB_CUR_SETSHAPE;
- }
+ }

- c = scr_readw((u16 *) vc->vc_pos);
+ if (priv->shape != (vc->vc_cursor_type & 0x0f)) {
+ priv->shape = vc->vc_cursor_type & 0x0f;
+ cursor->set |= FB_CUR_SETSHAPE;
+ }

- if (fgcolor != (int) attr_fgcol(fgshift, c) ||
- bgcolor != (int) attr_bgcol(bgshift, c)) {
- fgcolor = (int) attr_fgcol(fgshift, c);
- bgcolor = (int) attr_bgcol(bgshift, c);
- cursor->set |= FB_CUR_SETCMAP;
- }
- c &= charmask;
- font = vc->vc_font.data + (c * ((width + 7) / 8) * height);
- if (font != dest) {
- dest = font;
- cursor->set |= FB_CUR_SETDEST;
- }
-
- if (cursor->set & FB_CUR_SETSIZE) {
- memset(image, 0xff, 64);
- cursor->set |= FB_CUR_SETSHAPE;
- }
+ c = scr_readw((u16 *) vc->vc_pos);

- if (cursor->set & FB_CUR_SETSHAPE) {
- int w, cur_height, size, i = 0;
+ if (priv->cursor.image.fg_color != attr_fgcol(fgshift, c) ||
+ priv->cursor.image.bg_color != attr_bgcol(bgshift, c)) {
+ priv->cursor.image.fg_color = attr_fgcol(fgshift, c);
+ priv->cursor.image.bg_color = attr_bgcol(bgshift, c);
+ priv->cursor.image.depth = 1;
+ cursor->set |= FB_CUR_SETCMAP;
+ }
+ font = vc->vc_font.data + ((c & charmask) * size);
+ if (font != priv->dest) {
+ priv->dest = font;
+ cursor->set |= FB_CUR_SETDEST;
+ }

- w = (width + 7) / 8;
+ w = (width + 7) >> 3;

- switch (shape) {
- case CUR_NONE:
- cur_height = 0;
- break;
- case CUR_UNDERLINE:
- cur_height = (height < 10) ? 1 : 2;
- break;
- case CUR_LOWER_THIRD:
- cur_height = height/3;
- break;
- case CUR_LOWER_HALF:
- cur_height = height/2;
- break;
- case CUR_TWO_THIRDS:
- cur_height = (height * 2)/3;
- break;
- case CUR_BLOCK:
- default:
- cur_height = height;
- break;
- }
- size = (height - cur_height) * w;
- while (size--)
- mask[i++] = 0;
- size = cur_height * w;
- while (size--)
- mask[i++] = 0xff;
- }
-
- cursor->image.width = width;
- cursor->image.height = height;
- cursor->image.dx = vc->vc_x * width;
- cursor->image.dy = yy * height;
- cursor->image.depth = 1;
- cursor->image.data = image;
- cursor->image.bg_color = bgcolor;
- cursor->image.fg_color = fgcolor;
- cursor->mask = mask;
- cursor->dest = dest;
- cursor->rop = ROP_XOR;
+ if (cursor->set & FB_CUR_SETSHAPE) {
+ switch (priv->shape) {
+ case CUR_NONE:
+ cur_height = 0;
+ break;
+ case CUR_UNDERLINE:
+ cur_height = (height < 10) ? 1 : 2;
+ break;
+ case CUR_LOWER_THIRD:
+ cur_height = height/3;
+ break;
+ case CUR_LOWER_HALF:
+ cur_height = height/2;
+ break;
+ case CUR_TWO_THIRDS:
+ cur_height = (height * 2)/3;
+ break;
+ case CUR_BLOCK:
+ default:
+ cur_height = height;
+ break;
+ }

- if (info->fbops->fb_cursor)
- info->fbops->fb_cursor(info, cursor);
+ size = (height - cur_height) * w;
+ while (size--)
+ priv->mask[i++] = 0;
+ size = cur_height * w;
+ while (size--)
+ priv->mask[i++] = 0xff;
+ }
+
+ cursor->image = priv->cursor.image;
+ cursor->image.data = priv->image;
+ cursor->dest = priv->dest;
+ cursor->mask = priv->mask;
+ cursor->rop = ROP_XOR;
+ info->fbops->fb_cursor(info, cursor);
}

static void fbcon_cursor(struct vc_data *vc, int mode)
@@ -1138,6 +1151,7 @@
}
}

+
static int scrollback_phys_max = 0;
static int scrollback_max = 0;
static int scrollback_current = 0;
diff -Naur linux-2.5.66-orig/drivers/video/console/fbcon.h linux-2.5.66/drivers/video/console/fbcon.h
--- linux-2.5.66-orig/drivers/video/console/fbcon.h 2003-03-26 02:25:01.000000000 +0000
+++ linux-2.5.66/drivers/video/console/fbcon.h 2003-03-26 23:49:28.000000000 +0000
@@ -36,6 +36,15 @@
};

/* drivers/video/console/fbcon.c */
+
+struct fbcon_private {
+ struct fb_cursor cursor;
+ __u8 image[64];
+ __u8 mask[64];
+ __u8 *dest;
+ __u32 shape;
+};
+
extern char con2fb_map[MAX_NR_CONSOLES];
extern void set_con2fb_map(int unit, int newidx);

diff -Naur linux-2.5.66-orig/drivers/video/fbmem.c linux-2.5.66/drivers/video/fbmem.c
--- linux-2.5.66-orig/drivers/video/fbmem.c 2003-03-26 23:46:32.000000000 +0000
+++ linux-2.5.66/drivers/video/fbmem.c 2003-03-27 02:51:20.000000000 +0000
@@ -454,14 +454,13 @@
u32 fb_get_buffer_offset(struct fb_info *info, u32 size)
{
u32 align = info->pixmap.buf_align - 1;
- u32 offset, count = 1000;
+ u32 offset;

- spin_lock_irqsave(&info->pixmap.lock,
- info->pixmap.lock_flags);
+ spin_lock(&info->pixmap.lock);
offset = info->pixmap.offset + align;
offset &= ~align;
if (offset + size > info->pixmap.size) {
- while (atomic_read(&info->pixmap.count) && count--);
+ while (atomic_read(&info->pixmap.count));
if (info->fbops->fb_sync &&
info->pixmap.flags & FB_PIXMAP_SYNC)
info->fbops->fb_sync(info);
@@ -472,8 +471,7 @@
atomic_inc(&info->pixmap.count);
smp_mb__after_atomic_inc();

- spin_unlock_irqrestore(&info->pixmap.lock,
- info->pixmap.lock_flags);
+ spin_unlock(&info->pixmap.lock);
return offset;
}

@@ -568,9 +566,8 @@
const struct linux_logo *logo, u8 *dst,
int depth)
{
- int i, j, shift;
+ int i, j;
const u8 *src = logo->data;
- u8 d, xor = 0;

switch (depth) {
case 4:
@@ -584,20 +581,6 @@
}
}
break;
- case ~1:
- xor = 0xff;
- case 1:
- for (i = 0; i < logo->height; i++) {
- shift = 7;
- d = *src++ ^ xor;
- for (j = 0; j < logo->width; j++) {
- *dst++ = (d >> shift) & 1;
- shift = (shift-1) & 7;
- if (shift == 7)
- d = *src++ ^ xor;
- }
- }
- break;
}
}

@@ -667,6 +650,7 @@
case FB_VISUAL_MONO10:
fb_logo.needs_logo = 1;
break;
+ case FB_VISUAL_PSEUDOCOLOR:
case FB_VISUAL_STATIC_PSEUDOCOLOR:
if (info->var.bits_per_pixel >= 8) {
fb_logo.needs_logo = 8;
@@ -701,7 +685,7 @@
if (!info->fbops->fb_imageblit || fb_logo.logo == NULL)
return 0;

- image.depth = fb_logo.depth;
+ image.depth = 8;
image.data = fb_logo.logo->data;

if (fb_logo.needs_cmapreset)
@@ -722,7 +706,7 @@
info->pseudo_palette = palette;
}

- if (fb_logo.needs_logo != 8) {
+ if (fb_logo.needs_logo == 4) {
logo_new = kmalloc(fb_logo.logo->width * fb_logo.logo->height,
GFP_KERNEL);
if (logo_new == NULL) {
@@ -740,12 +724,27 @@
image.height = fb_logo.logo->height;
image.dy = 0;

+ /*
+ * Monochrome expansion and logo drawing functions are the same if
+ * fb_logo.needs_logo == 1.
+ */
+ switch (info->fix.visual) {
+ case FB_VISUAL_MONO10:
+ image.fg_color = (u32) (~(~0UL << fb_logo.depth));
+ image.bg_color = 0;
+ image.depth = 1;
+ break;
+ case FB_VISUAL_MONO01:
+ image.bg_color = (u32) (~(~0UL << fb_logo.depth));
+ image.fg_color = 0;
+ image.depth = 1;
+ break;
+ }
+
for (x = 0; x < num_online_cpus() * (fb_logo.logo->width + 8) &&
x <= info->var.xres-fb_logo.logo->width; x += (fb_logo.logo->width + 8)) {
image.dx = x;
info->fbops->fb_imageblit(info, &image);
- atomic_dec(&info->pixmap.count);
- smp_mb__after_atomic_dec();
}

if (palette != NULL)
diff -Naur linux-2.5.66-orig/drivers/video/softcursor.c linux-2.5.66/drivers/video/softcursor.c
--- linux-2.5.66-orig/drivers/video/softcursor.c 2003-03-26 23:46:32.000000000 +0000
+++ linux-2.5.66/drivers/video/softcursor.c 2003-03-26 23:49:41.000000000 +0000
@@ -19,46 +19,53 @@

int soft_cursor(struct fb_info *info, struct fb_cursor *cursor)
{
- int i, size = ((cursor->image.width + 7) / 8) * cursor->image.height;
struct fb_image image;
- static char data[64];
+ unsigned int scan_align = info->pixmap.scan_align - 1;
+ unsigned int buf_align = info->pixmap.buf_align - 1;
+ unsigned int i, size, dsize, s_pitch, d_pitch;
+ u8 *dst, src[64];
+
+ s_pitch = (cursor->image.width + 7) >> 3;
+ dsize = s_pitch * cursor->image.height;
+ d_pitch = (s_pitch + scan_align) & ~scan_align;
+ size = d_pitch * cursor->image.height + buf_align;
+ size &= ~buf_align;
+ dst = info->pixmap.addr + fb_get_buffer_offset(info, size);

if (cursor->enable) {
switch (cursor->rop) {
case ROP_XOR:
- for (i = 0; i < size; i++)
- data[i] = (cursor->image.data[i] &
- cursor->mask[i]) ^
- cursor->dest[i];
+ for (i = 0; i < dsize; i++) {
+ src[i] = (cursor->image.data[i] &
+ cursor->mask[i]) ^
+ cursor->dest[i];
+ }
break;
case ROP_COPY:
default:
- for (i = 0; i < size; i++)
- data[i] =
- cursor->image.data[i] & cursor->mask[i];
+ for (i = 0; i < dsize; i++) {
+ src[i] = cursor->image.data[i] &
+ cursor->mask[i];
+ }
break;
}
- } else
- memcpy(data, cursor->dest, size);
-
- image.bg_color = cursor->image.bg_color;
- image.fg_color = cursor->image.fg_color;
- image.dx = cursor->image.dx;
- image.dy = cursor->image.dy;
- image.width = cursor->image.width;
- image.height = cursor->image.height;
- image.depth = cursor->image.depth;
- image.data = data;
-
- if (info->fbops->fb_imageblit)
- info->fbops->fb_imageblit(info, &image);
+ move_buf_aligned(info, dst, src, d_pitch, s_pitch,
+ cursor->image.height);
+ } else {
+ move_buf_aligned(info, dst, cursor->dest, d_pitch, s_pitch,
+ cursor->image.height);
+ }
+ image = cursor->image;
+ image.data = dst;
+ info->fbops->fb_imageblit(info, &image);
atomic_dec(&info->pixmap.count);
- smp_mb__after_atomic_dec();
+ smp_mb__after_atomic_dec();
+
return 0;
}

EXPORT_SYMBOL(soft_cursor);
-
+
MODULE_AUTHOR("James Simmons <[email protected]>");
MODULE_DESCRIPTION("Generic software cursor");
MODULE_LICENSE("GPL");
diff -Naur linux-2.5.66-orig/include/linux/fb.h linux-2.5.66/include/linux/fb.h
--- linux-2.5.66-orig/include/linux/fb.h 2003-03-26 23:46:32.000000000 +0000
+++ linux-2.5.66/include/linux/fb.h 2003-03-26 23:50:07.000000000 +0000
@@ -312,8 +312,8 @@
#define FB_CUR_SETSHAPE 0x10
#define FB_CUR_SETSIZE 0x20
#define FB_CUR_SETDEST 0x40
+#define FB_CUR_SETFLASH 0x80
#define FB_CUR_SETALL 0xFF
-
struct fbcurpos {
__u16 x, y;
};
@@ -342,8 +342,7 @@
__u32 flags; /* see FB_PIXMAP_* */
void (*outbuf)(u8 dst, u8 *addr); /* access methods */
u8 (*inbuf) (u8 *addr);
- unsigned long lock_flags; /* flags for locking */
- spinlock_t lock; /* spinlock */
+ spinlock_t lock;
atomic_t count;
};
#ifdef __KERNEL__
@@ -406,15 +405,15 @@
struct fb_var_screeninfo var; /* Current var */
struct fb_fix_screeninfo fix; /* Current fix */
struct fb_monspecs monspecs; /* Current Monitor specs */
- struct fb_cursor cursor; /* Current cursor */
- struct work_struct queue; /* Framebuffer event queue */
- struct fb_pixmap pixmap; /* Current pixmap */
struct fb_cmap cmap; /* Current cmap */
+ struct work_struct queue; /* Framebuffer event queue */
+ struct fb_pixmap pixmap; /* Current pixmap */
struct fb_ops *fbops;
char *screen_base; /* Virtual address */
struct vc_data *display_fg; /* Console visible on this display */
int currcon; /* Current VC. */
void *pseudo_palette; /* Fake palette of 16 colors */
+ void *fbcon_priv; /* console-related private structure */
/* From here on everything is device dependent */
void *par;
};

2003-03-27 08:59:18

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On 27 Mar 2003, Antonino Daplas wrote:
> - image->depth should be representative of the data depth
> (currently, either 8 or 1). If image->depth == 1, color expansion can
> now be used to draw the logo, thus there's no need to differentiate
> between mono logo drawing and monochrome expansion.

> + /*
> + * Monochrome expansion and logo drawing functions are the same if
> + * fb_logo.needs_logo == 1.
> + */
> + switch (info->fix.visual) {
> + case FB_VISUAL_MONO10:
> + image.fg_color = (u32) (~(~0UL << fb_logo.depth));
^^^^^^^^^^^^^
> + image.bg_color = 0;
> + image.depth = 1;
> + break;
> + case FB_VISUAL_MONO01:
> + image.bg_color = (u32) (~(~0UL << fb_logo.depth));
^^^^^^^^^^^^^
> + image.fg_color = 0;
> + image.depth = 1;
> + break;

Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2003-03-27 19:22:58

by Antonino A. Daplas

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Thu, 2003-03-27 at 17:09, Geert Uytterhoeven wrote:
> On 27 Mar 2003, Antonino Daplas wrote:
> > - image->depth should be representative of the data depth
> > (currently, either 8 or 1). If image->depth == 1, color expansion can
> > now be used to draw the logo, thus there's no need to differentiate
> > between mono logo drawing and monochrome expansion.
>
> > + /*
> > + * Monochrome expansion and logo drawing functions are the same if
> > + * fb_logo.needs_logo == 1.
> > + */
> > + switch (info->fix.visual) {
> > + case FB_VISUAL_MONO10:
> > + image.fg_color = (u32) (~(~0UL << fb_logo.depth));
> ^^^^^^^^^^^^^
> > + image.bg_color = 0;
> > + image.depth = 1;
> > + break;
> > + case FB_VISUAL_MONO01:
> > + image.bg_color = (u32) (~(~0UL << fb_logo.depth));
> ^^^^^^^^^^^^^
> > + image.fg_color = 0;
> > + image.depth = 1;
> > + break;
>
> Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
>


Yes, fb_logo.depth == info->var.bits_per_pixel.

Tony

2003-03-27 20:38:34

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On 28 Mar 2003, Antonino Daplas wrote:
> On Thu, 2003-03-27 at 17:09, Geert Uytterhoeven wrote:
> > On 27 Mar 2003, Antonino Daplas wrote:
> > > - image->depth should be representative of the data depth
> > > (currently, either 8 or 1). If image->depth == 1, color expansion can
> > > now be used to draw the logo, thus there's no need to differentiate
> > > between mono logo drawing and monochrome expansion.
> >
> > > + /*
> > > + * Monochrome expansion and logo drawing functions are the same if
> > > + * fb_logo.needs_logo == 1.
> > > + */
> > > + switch (info->fix.visual) {
> > > + case FB_VISUAL_MONO10:
> > > + image.fg_color = (u32) (~(~0UL << fb_logo.depth));
> > ^^^^^^^^^^^^^
> > > + image.bg_color = 0;
> > > + image.depth = 1;
> > > + break;
> > > + case FB_VISUAL_MONO01:
> > > + image.bg_color = (u32) (~(~0UL << fb_logo.depth));
> > ^^^^^^^^^^^^^
> > > + image.fg_color = 0;
> > > + image.depth = 1;
> > > + break;
> >
> > Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
>
> Yes, fb_logo.depth == info->var.bits_per_pixel.

Euh, now I get confused... Do you mean
`Yes, it should be replaced by info->var.bits_per_pixel' or
`No, logo.depth is always equal to info->var.bits_per_pixel'?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2003-03-27 20:51:02

by Antonino A. Daplas

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Thu, 2003-03-27 at 11:01, Antonino Daplas wrote:

> 5. logo fixes
> - forgotten case statement for FB_VISUAL_PSEUDOCOLOR.
>
> - image->depth should be representative of the data depth
> (currently, either 8 or 1). If image->depth == 1, color expansion can
> now be used to draw the logo, thus there's no need to differentiate
> between mono logo drawing and monochrome expansion.
>
>
> The patch is against 2.5.66 + fbdev.diff.gz.
>

Grr. The patch has a few problems...

Tony

diff -Naur linux-2.5.66-orig/drivers/video/console/fbcon.c linux-2.5.66/drivers/video/console/fbcon.c
--- linux-2.5.66-orig/drivers/video/console/fbcon.c 2003-03-27 20:36:06.000000000 +0000
+++ linux-2.5.66/drivers/video/console/fbcon.c 2003-03-27 20:40:53.000000000 +0000
@@ -209,7 +209,7 @@
cursor.set = FB_CUR_SETFLASH;

if (!cursor_drawn)
- cursor.set = FB_CUR_SETCUR;
+ cursor.set |= FB_CUR_SETCUR;
accel_cursor(vc, info, &cursor, real_y(p, vc->vc_y));
cursor_drawn ^= 1;
vbl_cursor_cnt = cursor_blink_rate;
@@ -597,6 +597,8 @@
/* Allocate private data */
info->fbcon_priv = kmalloc(sizeof(struct fbcon_private), GFP_KERNEL);
if (info->fbcon_priv == NULL) {
+ if (softback_buf)
+ kfree(softback_buf);
kfree(vc);
return NULL;
}
diff -Naur linux-2.5.66-orig/drivers/video/fbmem.c linux-2.5.66/drivers/video/fbmem.c
--- linux-2.5.66-orig/drivers/video/fbmem.c 2003-03-27 20:36:06.000000000 +0000
+++ linux-2.5.66/drivers/video/fbmem.c 2003-03-27 20:37:52.000000000 +0000
@@ -728,17 +728,14 @@
* Monochrome expansion and logo drawing functions are the same if
* fb_logo.needs_logo == 1.
*/
- switch (info->fix.visual) {
- case FB_VISUAL_MONO10:
+ if (fb_logo.needs_logo == 1) {
image.fg_color = (u32) (~(~0UL << fb_logo.depth));
image.bg_color = 0;
image.depth = 1;
- break;
- case FB_VISUAL_MONO01:
- image.bg_color = (u32) (~(~0UL << fb_logo.depth));
- image.fg_color = 0;
- image.depth = 1;
- break;
+ if (info->fix.visual == FB_VISUAL_MONO01) {
+ image.bg_color = image.fg_color;
+ image.fg_color = 0;
+ }
}

for (x = 0; x < num_online_cpus() * (fb_logo.logo->width + 8) &&

2003-03-27 20:56:56

by Antonino A. Daplas

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Fri, 2003-03-28 at 04:49, Geert Uytterhoeven wrote:
> > >
> > > Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
> >
> > Yes, fb_logo.depth == info->var.bits_per_pixel.
>
> Euh, now I get confused... Do you mean
> `Yes, it should be replaced by info->var.bits_per_pixel' or
> `No, logo.depth is always equal to info->var.bits_per_pixel'?

:) Sorry about that. I meant:


`No, fb_logo.depth is always equal to info->var.bits_per_pixel'

Tony

2003-03-28 04:37:43

by James Simmons

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.


> > > > Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
> > >
> > > Yes, fb_logo.depth == info->var.bits_per_pixel.
> >
> > Euh, now I get confused... Do you mean
> > `Yes, it should be replaced by info->var.bits_per_pixel' or
> > `No, logo.depth is always equal to info->var.bits_per_pixel'?
>
> :) Sorry about that. I meant:
>
>
> `No, fb_logo.depth is always equal to info->var.bits_per_pixel'

No this is no longer true. For example last night I displayed the 16 color
logo perfectly fine on a 16 bpp display!!!! The mono display still has
bugs tho. The new logo tries to pick the best image to display. Say for
example we have two video cards. One running VESA fbdev at 16 bpp and a
another at vga 4 planar via vga16fb. This way we can have the both the 16
color and 224 color logo compiled in. The correct logo will be displayed
then on the correct display. Now say we only have a mono display but all
the cards support 8 bpp or better. That logo still gets displayed.

2003-03-28 07:49:37

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Fri, 28 Mar 2003, James Simmons wrote:
> > > > > Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
> > > >
> > > > Yes, fb_logo.depth == info->var.bits_per_pixel.
> > >
> > > Euh, now I get confused... Do you mean
> > > `Yes, it should be replaced by info->var.bits_per_pixel' or
> > > `No, logo.depth is always equal to info->var.bits_per_pixel'?
> >
> > :) Sorry about that. I meant:
> >
> >
> > `No, fb_logo.depth is always equal to info->var.bits_per_pixel'
>
> No this is no longer true. For example last night I displayed the 16 color
> logo perfectly fine on a 16 bpp display!!!! The mono display still has
> bugs tho. The new logo tries to pick the best image to display. Say for
> example we have two video cards. One running VESA fbdev at 16 bpp and a
> another at vga 4 planar via vga16fb. This way we can have the both the 16
> color and 224 color logo compiled in. The correct logo will be displayed
> then on the correct display. Now say we only have a mono display but all

Didn't it always work like that? You got the 16 color logo on vga16fb and the
224 color logo on displays with more than 256 colors (except for directcolor).

> the cards support 8 bpp or better. That logo still gets displayed.
^^^^
Which logo do you mean with `that'? On a monochrome display, it should be the
monochrome logo.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2003-03-28 11:10:33

by Antonino A. Daplas

[permalink] [raw]
Subject: Re: [Linux-fbdev-devel] Framebuffer fixes.

On Fri, 2003-03-28 at 16:00, Geert Uytterhoeven wrote:
> On Fri, 28 Mar 2003, James Simmons wrote:
> > > > > > Shouldn't these be info->var.bits_per_pixel instead of fb_logo.depth?
> > > > >
> > > > > Yes, fb_logo.depth == info->var.bits_per_pixel.
> > > >
> > > > Euh, now I get confused... Do you mean
> > > > `Yes, it should be replaced by info->var.bits_per_pixel' or
> > > > `No, logo.depth is always equal to info->var.bits_per_pixel'?
> > >
> > > :) Sorry about that. I meant:
> > >
> > >
> > > `No, fb_logo.depth is always equal to info->var.bits_per_pixel'
> >
> > No this is no longer true. For example last night I displayed the 16 color
> > logo perfectly fine on a 16 bpp display!!!! The mono display still has
> > bugs tho. The new logo tries to pick the best image to display. Say for
> > example we have two video cards. One running VESA fbdev at 16 bpp and a
> > another at vga 4 planar via vga16fb. This way we can have the both the 16
> > color and 224 color logo compiled in. The correct logo will be displayed
> > then on the correct display. Now say we only have a mono display but all
>
> Didn't it always work like that? You got the 16 color logo on vga16fb and the
> 224 color logo on displays with more than 256 colors (except for directcolor).
>

If I'm not mistaken, I think what James meant was that the new code has
the capability of choosing an appropriate logo even if it does not
maximize the color range of the display. Ie if only 4bpp logo is
compiled, but display is set at 8bpp-pseudocolor, it would still
display a 4bpp logo correctly.

Personally, I think it's really a simple matter of choosing the
appropriate logo type for the correct display device, instead of the
code trying to outthink the intention of the user.

However, that was never my point. What I see is a problem with the new
code. What if the display is set at 16-bpp DirectColor? The code will
choose clut224 for it, but that is not correct and may even crash due to
an "out of bounds" error in the pseudo_palette. Directcolor 565, for
instance, will only have 32 entries for red and blue, and 64 entries for
green, greatly exceeding 224. Similarly, Directcolor < 12bpp, will
actually need monochrome, not even 4bpp, and definitely not clut224.
There are other obvious and non-obvious examples that I can enumerate.


> > the cards support 8 bpp or better. That logo still gets displayed.
> ^^^^
> Which logo do you mean with `that'? On a monochrome display, it should be the
> monochrome logo.
>

The patch I submitted was tested by simulating monochrome on an 8-bit
display. It used monochrome logo, drawn using monochrome expansion, and
it works for me.

Tony


2003-03-28 13:06:58

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Why moving driver includes ?

Hi James !

Why did you move the driver includes to include/video ? What is
the reasoning here ?

For example, drivers/video/radeon.h moved to include/video/radeon.h

Is this to be able to share register definitions with the DRM drivers ?
(I doubt this will ever happen as the DRM is rather self contained)

I would have preferred those includes to stay next to their respective
drivers (though renaming radeon.h to radeonfb.h might have made some
sense).

Regards,
Ben.