Re: [BK FBDEV] A few more updates.

James Simmons
2003-03-25T19:37:06Z

On 25 Mar 2003, Benjamin Herrenschmidt wrote:

> On Tue, 2003-03-25 at 19:28, Benjamin Herrenschmidt wrote:
>
> > You "fixed" it by using GFP_ATOMIC but didn't test the result of
> > kmalloc. That is very bad. GFP_ATOMIC can fail (return NULL), thus
> > you will crash the kernel under high memory pressure.
> >
> > I think the proper fix is, as you asked me, using a workqueue,
> > that way, you can both use GFP_KERNEL allocations, and avoid
> > the spinlock you added to fbmem.c, thus letting the fb_sync()
> > ops on fbdev's be able to block.
>
> Well, actually, creating a workqueue would be overhead since
> it involves one kernel thread per CPU. After more thinking &
> discussion, I beleive you shall rather use keventd existing
> workqueue (schedule_work() will do that)

Done. Can you look over this patch and test it. I tested it and it worked
fine.

diff -urN -X /home/jsimmons/dontdiff linus-2.5/drivers/video/console/fbcon.c fbdev-2.5/drivers/video/console/fbcon.c
--- linus-2.5/drivers/video/console/fbcon.c Sat Mar 22 21:45:23 2003
+++ fbdev-2.5/drivers/video/console/fbcon.c Tue Mar 25 12:03:56 2003
@@ -172,8 +172,9 @@
* 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,
+ struct fb_cursor *cursor, int yy);
static __inline__ int real_y(struct display *p, int ypos);
-static void fb_vbl_handler(int irq, void *dummy, struct pt_regs *fp);
static __inline__ void updatescrollmode(struct display *p, struct vc_data *vc);
static __inline__ void ywrap_up(struct vc_data *vc, int count);
static __inline__ void ywrap_down(struct vc_data *vc, int count);
@@ -194,6 +195,34 @@
}
#endif

+static void fb_callback(void *private)
+{
+ struct fb_info *info = (struct fb_info *) private;
+ struct display *p = &fb_display[fg_console];
+ struct vc_data *vc = vc_cons[fg_console].d;
+ struct fb_cursor cursor;
+
+ if (!info || !cursor_on)
+ return;
+
+ if (vbl_cursor_cnt && --vbl_cursor_cnt == 0) {
+ cursor.set = 0;
+
+ 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;
+ }
+}
+
+static void fb_vbl_handler(int irq, void *dev_id, struct pt_regs *fp)
+{
+ struct fb_info *info = dev_id;
+
+ schedule_work(&info->queue);
+}
+
static void cursor_timer_handler(unsigned long dev_addr);

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

- fb_vbl_handler(0, info, NULL);
+ schedule_work(&info->queue);
cursor_timer.expires = jiffies + HZ / 50;
add_timer(&cursor_timer);
}
@@ -290,14 +319,14 @@
const unsigned short *s)
{
unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
- unsigned int width = (vc->vc_font.width + 7)/8;
+ unsigned int width = (vc->vc_font.width + 7) >> 3;
unsigned int cellsize = vc->vc_font.height * width;
unsigned int maxcnt = info->pixmap.size/cellsize;
unsigned int shift_low = 0, mod = vc->vc_font.width % 8;
unsigned int shift_high = 8, size, pitch, cnt, k;
unsigned int buf_align = info->pixmap.buf_align - 1;
unsigned int scan_align = info->pixmap.scan_align - 1;
- unsigned int idx = vc->vc_font.width/8;
+ unsigned int idx = vc->vc_font.width >> 3;
u8 mask, *src, *dst, *dst0;

while (count) {
@@ -307,7 +336,7 @@
cnt = k = count;

image->width = vc->vc_font.width * cnt;
- pitch = (image->width + 7)/8 + scan_align;
+ pitch = ((image->width + 7) >> 3) + scan_align;
pitch &= ~scan_align;
size = pitch * vc->vc_font.height + buf_align;
size &= ~buf_align;
@@ -338,7 +367,7 @@
const unsigned short *s)
{
unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
- unsigned int width = vc->vc_font.width/8;
+ unsigned int width = vc->vc_font.width >> 3;
unsigned int cellsize = vc->vc_font.height * width;
unsigned int maxcnt = info->pixmap.size/cellsize;
unsigned int scan_align = info->pixmap.scan_align - 1;
@@ -411,7 +440,7 @@
int c, int ypos, int xpos)
{
unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
- unsigned int width = (vc->vc_font.width + 7)/8;
+ unsigned int width = (vc->vc_font.width + 7) >> 3;
unsigned int scan_align = info->pixmap.scan_align - 1;
unsigned int buf_align = info->pixmap.buf_align - 1;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
@@ -559,6 +588,15 @@

vc = (struct vc_data *) kmalloc(sizeof(struct vc_data), GFP_ATOMIC);

+ if (!vc) {
+ if (softback_buf)
+ kfree((void *) softback_buf);
+ 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;
@@ -956,8 +994,8 @@
accel_putcs(vc, info, s, count, real_y(p, ypos), xpos);
}

-void accel_cursor(struct vc_data *vc, struct fb_info *info, struct fb_cursor *cursor,
- int yy)
+static void accel_cursor(struct vc_data *vc, struct fb_info *info,
+ struct fb_cursor *cursor, int yy)
{
unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
@@ -986,7 +1024,15 @@
size = ((width + 7) >> 3) * height;

data = kmalloc(size, GFP_KERNEL);
+
+ if (!data) return;
+
mask = kmalloc(size, GFP_KERNEL);
+
+ if (!mask) {
+ kfree(data);
+ return;
+ }

if (cursor->set & FB_CUR_SETSIZE) {
memset(data, 0xff, size);
@@ -1101,27 +1147,6 @@
}
}

-static void fb_vbl_handler(int irq, void *dev_id, struct pt_regs *fp)
-{
- struct fb_info *info = dev_id;
- struct display *p = &fb_display[fg_console];
- struct vc_data *vc = vc_cons[fg_console].d;
- struct fb_cursor cursor;
-
- if (!cursor_on)
- return;
-
- if (vbl_cursor_cnt && --vbl_cursor_cnt == 0) {
- cursor.set = 0;
-
- 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;
- }
-}
-
static int scrollback_phys_max = 0;
static int scrollback_max = 0;
static int scrollback_current = 0;
diff -urN -X /home/jsimmons/dontdiff linus-2.5/drivers/video/softcursor.c fbdev-2.5/drivers/video/softcursor.c
--- linus-2.5/drivers/video/softcursor.c Sat Mar 22 21:45:22 2003
+++ fbdev-2.5/drivers/video/softcursor.c Tue Mar 25 11:41:28 2003
@@ -44,6 +44,7 @@
if (info->cursor.mask)
kfree(info->cursor.mask);
info->cursor.mask = kmalloc(dsize, GFP_KERNEL);
+ if (!info->cursor.mask) return -ENOMEM;
if (cursor->mask)
memcpy(info->cursor.mask, cursor->mask, dsize);
else
diff -urN -X /home/jsimmons/dontdiff linus-2.5/include/linux/fb.h fbdev-2.5/include/linux/fb.h
--- linus-2.5/include/linux/fb.h Sat Mar 22 21:45:25 2003
+++ fbdev-2.5/include/linux/fb.h Tue Mar 25 12:00:20 2003
@@ -2,6 +2,7 @@
#define _LINUX_FB_H

#include <linux/tty.h>
+#include <linux/workqueue.h>
#include <asm/types.h>
#include <asm/io.h>

@@ -406,8 +407,9 @@
struct fb_fix_screeninfo fix; /* Current fix */
struct fb_monspecs monspecs; /* Current Monitor specs */
struct fb_cursor cursor; /* Current cursor */
- struct fb_cmap cmap; /* Current cmap */
+ struct work_struct queue; /* Framebuffer event queue */
struct fb_pixmap pixmap; /* Current pixmap */
+ struct fb_cmap cmap; /* Current cmap */
struct fb_ops *fbops;
char *screen_base; /* Virtual address */
struct vc_data *display_fg; /* Console visible on this display */