Re: [BK FBDEV] A few more updates.

Re: [BK FBDEV] A few more updates.

[Petr Vandrovec]

On 26 Mar 03 at 17:53, Antonino Daplas wrote:
> On Wed, 2003-03-26 at 13:34, James Simmons wrote:
> > 
> > > 5.  softcursor should not concern itself with memory bookkeeping, and
> > > must be able to function with just the parameter passed to it in order
> > > to keep it as simple as possible.  These tasks are moved to
> > > accel_cursor.
> > 
> > We do if we make a ioctl for cursors. I'm trying to avoid reprogramming 
> > the hardware over and over again if the properties of the cursor don't 
> > change. The idea is similar to passing in var and comparing it to the var 
> > in struct fb_info. 
> 
> Of course, that's what the fb_cursor.set field is for, and drivers have
> the option of ignoring or not ignoring bits in this field. Whoever calls
> fb_cursor has the responsibility of setting any cursor state changes. 
> 
> Unlike fb_set_var(), cursor states change very frequently (ie, each
> blink or movement of the cursor are considered state changes), so just
> forego the memcmp() and call fb_cursor unconditionally.  Let the
> low-level method sort it out by checking bits in fb_cursor.set.

accel_cursor unconditionally sets FB_CUR_SETPOS. Can you write it
down to the TODO list to eliminate this? Cursor position lives 
in different registers than cursor enable/disable on my hardware...

And if we could rename FB_CUR_SETCUR to FB_CUR_SETVISIBILITY and
leave cursor->enable setting on accel_cursor's caller, it would
be even better.

And if we could change enable value to 0: disable; 1: enable;
2: disable due to blink (called from vbl), it would be even better,
as then fbdev which does hardware blinking could just completely
ignore changes which set only FB_CUR_SETVISIBILITY with enable == 2.
                                                Petr Vandrovec
                                                vandrove@vc.cvut.cz
                                                



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Antonino Daplas]

On Wed, 2003-03-26 at 18:42, Petr Vandrovec wrote:
> On 26 Mar 03 at 17:53, Antonino Daplas wrote:
> > On Wed, 2003-03-26 at 13:34, James Simmons wrote:
> > > 
> > > > 5.  softcursor should not concern itself with memory bookkeeping, and
> > > > must be able to function with just the parameter passed to it in order
> > > > to keep it as simple as possible.  These tasks are moved to
> > > > accel_cursor.
> > > 
> > > We do if we make a ioctl for cursors. I'm trying to avoid reprogramming 
> > > the hardware over and over again if the properties of the cursor don't 
> > > change. The idea is similar to passing in var and comparing it to the var 
> > > in struct fb_info. 
> > 
> > Of course, that's what the fb_cursor.set field is for, and drivers have
> > the option of ignoring or not ignoring bits in this field. Whoever calls
> > fb_cursor has the responsibility of setting any cursor state changes. 
> > 
> > Unlike fb_set_var(), cursor states change very frequently (ie, each
> > blink or movement of the cursor are considered state changes), so just
> > forego the memcmp() and call fb_cursor unconditionally.  Let the
> > low-level method sort it out by checking bits in fb_cursor.set.
> 
> accel_cursor unconditionally sets FB_CUR_SETPOS. Can you write it
> down to the TODO list to eliminate this? Cursor position lives 
> in different registers than cursor enable/disable on my hardware...
> 

The patch that I submitted to James does that already.

> And if we could rename FB_CUR_SETCUR to FB_CUR_SETVISIBILITY and
> leave cursor->enable setting on accel_cursor's caller, it would
> be even better.
> 
> And if we could change enable value to 0: disable; 1: enable;
> 2: disable due to blink (called from vbl), it would be even better,
> as then fbdev which does hardware blinking could just completely
> ignore changes which set only FB_CUR_SETVISIBILITY with enable == 2.


Okay.


Tony




-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Petr Vandrovec]

On 26 Mar 03 at 11:42, Petr Vandrovec wrote:
> 
> accel_cursor unconditionally sets FB_CUR_SETPOS. Can you write it
> down to the TODO list to eliminate this? Cursor position lives 
> in different registers than cursor enable/disable on my hardware...
 
> And if we could rename FB_CUR_SETCUR to FB_CUR_SETVISIBILITY and
> leave cursor->enable setting on accel_cursor's caller, it would
> be even better.

I just noticed that softcursor.c contains copy of FB_CUR_SETCUR ->
cursor->enable code from accel_cursor(). Either 'enable' field has 
no bussiness in the fb_cursor structure (as it can be always infered from 
set flags, and if softcursor needs some internal bookkeeping, it should 
use some other variable and not fb_cursor's field), or this conversion 
should disappear from softcursor.c, and FB_CUR_SETCUR semantic should
change.
                                                Petr
                                                



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

[BK FBDEV] A few more updates.

[James Simmons]

Linus, please do a

	bk pull http://fbdev.bkbits.net/fbdev-2.5

This will update the following files:

 drivers/video/aty/aty128fb.c  |   16 +++++++---------
 drivers/video/console/fbcon.c |    4 ++--
 drivers/video/controlfb.c     |   18 +++---------------
 drivers/video/platinumfb.c    |   28 ++++++++--------------------
 drivers/video/radeonfb.c      |   10 ++++++++++
 drivers/video/softcursor.c    |    2 +-
 6 files changed, 31 insertions(+), 47 deletions(-)

through these ChangeSets:

<jsimmons@maxwell.earthlink.net> (03/03/25 1.981)
   [FBCON] Could be called outside of a process context. This fixes that.

<jsimmons@maxwell.earthlink.net> (03/03/25 1.979)
   [RAGE 128/CONTROL/PLATNIUM FBDEV] PPC updates.

   [RADEON FBDEV] PLL fix for specific type of card.




-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Russell King]

On Tue, Mar 25, 2003 at 10:32:11AM -0800, James Simmons wrote:
> Linus, please do a
> 
> 	bk pull http://fbdev.bkbits.net/fbdev-2.5

Patch?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [BK FBDEV] A few more updates.

[Benjamin Herrenschmidt]

On Tue, 2003-03-25 at 19:32, James Simmons wrote:
> Linus, please do a
> 
> 	bk pull http://fbdev.bkbits.net/fbdev-2.5
> 
> This will update the following files:
> 
>  drivers/video/aty/aty128fb.c  |   16 +++++++---------
>  drivers/video/console/fbcon.c |    4 ++--
>  drivers/video/controlfb.c     |   18 +++---------------
>  drivers/video/platinumfb.c    |   28 ++++++++--------------------
>  drivers/video/radeonfb.c      |   10 ++++++++++
>  drivers/video/softcursor.c    |    2 +-
>  6 files changed, 31 insertions(+), 47 deletions(-)
> 
> through these ChangeSets:
> 
> <jsimmons@maxwell.earthlink.net> (03/03/25 1.981)
>    [FBCON] Could be called outside of a process context. This fixes that.

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.

Ben.



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Benjamin Herrenschmidt]

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)

Ben.



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[James Simmons]

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 */




-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Benjamin Herrenschmidt]

On Tue, 2003-03-25 at 20:48, James Simmons wrote:

> > 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.

I don't have a test config at hand right now. The patch looks better,
though you didn't remove the spinlock and replace it with some
"softer" sync. primitives.

Note that if fbcon is ever to be rmmod'ed, you need to properly
remove the timer and make sure all pending work queues have completed
(and make sure the timer won't be re-scheduled by one).

Ben.



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[James Simmons]

> > > 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.
> 
> I don't have a test config at hand right now. The patch looks better,
> though you didn't remove the spinlock and replace it with some
> "softer" sync. primitives.

I didn't get around to removing the spinlock. That is next on the list.
I just wanted to fix the big problem.  

> Note that if fbcon is ever to be rmmod'ed, you need to properly
> remove the timer and make sure all pending work queues have completed
> (and make sure the timer won't be re-scheduled by one).

Note support for rmmod fbcon is incomplete. The function giveup_console is 
really sad but none one ever imagine that the console system would become 
powerful enough to switch from one driver to another. 

Re: [BK FBDEV] A few more updates.

[James Simmons]

> 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.

Ug! The quick fix was a bad idea. I will work on the workqueue idea 
instead. Ignore the pull. 





-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Antonino Daplas]

On Wed, 2003-03-26 at 02:32, James Simmons wrote:
> 
> Linus, please do a
> 
> 	bk pull http://fbdev.bkbits.net/fbdev-2.5
> 
> This will update the following files:
> 
>  drivers/video/aty/aty128fb.c  |   16 +++++++---------
>  drivers/video/console/fbcon.c |    4 ++--
>  drivers/video/controlfb.c     |   18 +++---------------
>  drivers/video/platinumfb.c    |   28 ++++++++--------------------
>  drivers/video/radeonfb.c      |   10 ++++++++++
>  drivers/video/softcursor.c    |    2 +-
>  6 files changed, 31 insertions(+), 47 deletions(-)
> 

James,

Attached is a patch that fixes the following:

1.  The following lines are missing from softcursor.c, but is present in
fb_show_logo() where it shoudn't be:

	atomic_dec(&info->pixmap.count);
	smp_mb__after_atomic_dec();

In both cases, the reference counting will be incorrect which can cause
irregular cursor flashing and unnecessary "syncing".  Only functions
that call fb_get_buffer_offset() need to decrement the reference count
afterwards.

2.  I applied your workqueue patch and changed locking from spinlocks to
semaphores in fb_get_buffer_offset().

3.  BTW, there are too many kmalloc's/kfree's in accel_cursor() and
softcursor().  Personally, I would rather have 2 64-byte buffers for the
mask and the data in the info->cursor structure than allocating/freeing
memory each time the cursor flashes.  However, if you prefer doing it
this way, the patch also includes changes so kmallocs are only done when
necessary.  Still, accel_cursor() has unnecessary work being done, such
as always creating the mask bitmap, when a simple flag to monitor cursor
shape changes could prevent all this.

5.  softcursor should not concern itself with memory bookkeeping, and
must be able to function with just the parameter passed to it in order
to keep it as simple as possible.  These tasks are moved to
accel_cursor.

6.  The patch should also fix the "irregularly dashed" cursor.

Tony

PS:  What happened to the logo?  I just get a white square in the uppper left corner.


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 02:25:01.000000000 +0000
+++ linux-2.5.66/drivers/video/console/fbcon.c	2003-03-26 02:47:58.000000000 +0000
@@ -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)
+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;
@@ -971,25 +1009,39 @@
 	else
 		cursor->enable = 0;
 
-	cursor->set |= FB_CUR_SETPOS;
 
 	height = info->cursor.image.height;
 	width = info->cursor.image.width;
 
 	if (width != vc->vc_font.width || 
 	    height != vc->vc_font.height) {
-		width = vc->vc_font.width;
-		height = vc->vc_font.height;
+		info->cursor.image.width = vc->vc_font.width;
+		info->cursor.image.height = vc->vc_font.height;
+		height = info->cursor.image.height;
+		width = info->cursor.image.width;
 		cursor->set |= FB_CUR_SETSIZE;
 	}	
 
+	if (info->cursor.image.dx != vc->vc_x * width ||
+	    info->cursor.image.dy != yy * height) {
+		info->cursor.image.dx = vc->vc_x * width;
+		info->cursor.image.dy = yy * height;
+		cursor->set |= FB_CUR_SETPOS;
+	}
+
 	size = ((width + 7) >> 3) * height;
 
-	data = kmalloc(size, GFP_KERNEL);
 	mask = kmalloc(size, GFP_KERNEL);
-	
+	if (mask == NULL)
+		return;
 	if (cursor->set & FB_CUR_SETSIZE) {
+		if (info->cursor.image.data)
+			kfree(info->cursor.image.data);
+		data = kmalloc(size, GFP_KERNEL);
+		if (data == NULL)
+			return;
 		memset(data, 0xff, size);
+		info->cursor.image.data = data;
 		cursor->set |= FB_CUR_SETSHAPE;
 	}
 
@@ -997,13 +1049,14 @@
 
 	if (info->cursor.image.fg_color != attr_fgcol(fgshift, c) ||
 	    info->cursor.image.bg_color != attr_bgcol(bgshift, c)) {
-		cursor->image.fg_color = attr_fgcol(fgshift, c);
-		cursor->image.bg_color = attr_bgcol(bgshift, c);
+		info->cursor.image.fg_color = attr_fgcol(fgshift, c);
+		info->cursor.image.bg_color = attr_bgcol(bgshift, c);
+		info->cursor.image.depth = 0;
 		cursor->set |= FB_CUR_SETCMAP;
 	}
 	font = vc->vc_font.data + ((c & charmask) * size);
 	if (font != info->cursor.dest) {
-		cursor->dest = font;
+		info->cursor.dest = font;
 		cursor->set |= FB_CUR_SETDEST;
 	}
 
@@ -1038,21 +1091,18 @@
 	while (size--)
 		mask[i++] = 0xff;
 
-	if (!info->cursor.mask ||  (memcmp(mask, info->cursor.mask, w*height)))
+	if (!(cursor->set & FB_CUR_SETSHAPE) && 
+	    (memcmp(mask, info->cursor.mask, size)))
 		cursor->set |= FB_CUR_SETSHAPE;
-
-	cursor->image.width = width;
-	cursor->image.height = height;
-	cursor->image.dx = vc->vc_x * width;
-	cursor->image.dy = yy * height;
-	cursor->image.depth = 0;
-	cursor->image.data = data;
-	cursor->mask = mask;
+	if (info->cursor.mask)
+		kfree(info->cursor.mask);
+	info->cursor.mask = mask;
+	
+	cursor->image = info->cursor.image;
+	cursor->dest = info->cursor.dest;
+	cursor->mask = info->cursor.mask;
 	cursor->rop = ROP_XOR;
-
 	info->fbops->fb_cursor(info, cursor);
-	kfree(data);
-	kfree(mask);
 }
 	
 static void fbcon_cursor(struct vc_data *vc, int mode)
@@ -1101,26 +1151,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;
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 02:25:01.000000000 +0000
+++ linux-2.5.66/drivers/video/fbmem.c	2003-03-26 02:47:44.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);
+	down(&info->pixmap.sem);
 	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);
+	up(&info->pixmap.sem);
 	return offset;
 }
 
@@ -754,8 +752,6 @@
 	     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)
@@ -1195,6 +1191,7 @@
 register_framebuffer(struct fb_info *fb_info)
 {
 	char name_buf[12];
+	struct semaphore *sem = &fb_info->pixmap.sem;
 	int i;
 
 	if (num_registered_fb == FB_MAX)
@@ -1219,8 +1216,8 @@
 		fb_info->pixmap.outbuf = sys_outbuf;
 	if (fb_info->pixmap.inbuf == NULL)
 		fb_info->pixmap.inbuf = sys_inbuf;
-	spin_lock_init(&fb_info->pixmap.lock);
-	
+
+	*sem = (struct semaphore) __SEMAPHORE_INITIALIZER((*sem), 1);
 	registered_fb[i] = fb_info;
 	sprintf(name_buf, "fb/%d", i);
 	devfs_register(NULL, name_buf, DEVFS_FL_DEFAULT,
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 02:25:02.000000000 +0000
+++ linux-2.5.66/drivers/video/softcursor.c	2003-03-26 02:47:48.000000000 +0000
@@ -19,57 +19,20 @@
 
 int soft_cursor(struct fb_info *info, struct fb_cursor *cursor)
 {
+	struct fb_image image;
 	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];
 
-	info->cursor.enable = (cursor->set & FB_CUR_SETCUR) ? 1 : 0;
-
-	if (cursor->set & FB_CUR_SETSIZE) {
-		info->cursor.image.width = cursor->image.width;
-		info->cursor.image.height = cursor->image.height;
-		cursor->set |= FB_CUR_SETSHAPE;
-	}
-
-	s_pitch = (info->cursor.image.width + 7) >> 3;
-	dsize = s_pitch * info->cursor.image.height;
+	s_pitch = (cursor->image.width + 7) >> 3;
+	dsize = s_pitch * cursor->image.height;
 	d_pitch = (s_pitch + scan_align) & ~scan_align;
-	size = d_pitch * info->cursor.image.height + buf_align;
+	size = d_pitch * cursor->image.height + buf_align;
 	size &= ~buf_align;
 	dst = info->pixmap.addr + fb_get_buffer_offset(info, size);
-	info->cursor.image.data = dst;
-
-	if (cursor->set & FB_CUR_SETSHAPE) {
-		if (info->cursor.mask)
-			kfree(info->cursor.mask);
-		info->cursor.mask = kmalloc(dsize, GFP_KERNEL);
-		if (cursor->mask)
-			memcpy(info->cursor.mask, cursor->mask, dsize);
-		else
-			memset(info->cursor.mask, 0, dsize);
-	}
-
-	if (cursor->set & FB_CUR_SETPOS) {
-		info->cursor.image.dx = cursor->image.dx;
-		info->cursor.image.dy = cursor->image.dy;
-	}
-
-	if (cursor->set & FB_CUR_SETHOT)
-		info->cursor.hot = cursor->hot;
-
-	if (cursor->set & FB_CUR_SETCMAP) {
-		if (cursor->image.depth == 0) {
-			info->cursor.image.bg_color = cursor->image.bg_color;
-			info->cursor.image.fg_color = cursor->image.fg_color;
-		} else {
-			if (cursor->image.cmap.len)
-				fb_copy_cmap(&cursor->image.cmap, &info->cursor.image.cmap, 0);
-		}
-		info->cursor.image.depth = cursor->image.depth;
-	}
 
-	if (info->cursor.enable) {
+	if (cursor->enable) {
 		switch (cursor->rop) {
 		case ROP_XOR:
 			for (i = 0; i < dsize; i++) {
@@ -89,10 +52,15 @@
 		move_buf_aligned(info, dst, src, d_pitch, s_pitch,
 				 cursor->image.height);
 	} else {
-		move_buf_aligned(info, dst, cursor->dest, s_pitch, d_pitch,
+		move_buf_aligned(info, dst, cursor->dest, d_pitch, s_pitch,
 				 cursor->image.height);
 	}
-	info->fbops->fb_imageblit(info, &info->cursor.image);
+	image = cursor->image;
+	image.data = dst;
+	info->fbops->fb_imageblit(info, &image);
+	atomic_dec(&info->pixmap.count);
+	smp_mb__after_atomic_dec();
+	
 	return 0;
 }
 
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 02:25:04.000000000 +0000
+++ linux-2.5.66/include/linux/fb.h	2003-03-26 02:48:10.000000000 +0000
@@ -2,6 +2,7 @@
 #define _LINUX_FB_H
 
 #include <linux/tty.h>
+#include <linux/workqueue.h>
 #include <asm/types.h>
 #include <asm/io.h>
 
@@ -341,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                      */
+	struct semaphore sem;
 	atomic_t count;
 };
 #ifdef __KERNEL__
@@ -407,6 +407,7 @@
    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_ops *fbops;
    char *screen_base;                   /* Virtual address */



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[James Simmons]

> 1.  The following lines are missing from softcursor.c, but is present in
> fb_show_logo() where it shoudn't be:
> 
> 	atomic_dec(&info->pixmap.count);
> 	smp_mb__after_atomic_dec();
> 
> In both cases, the reference counting will be incorrect which can cause
> irregular cursor flashing and unnecessary "syncing".  Only functions
> that call fb_get_buffer_offset() need to decrement the reference count
> afterwards.

Fixed. Applied. I missed it when I was applying your patches by hand. My 
tree was way to different to accept your patches. 
 
> 2.  I applied your workqueue patch and changed locking from spinlocks to
> semaphores in fb_get_buffer_offset().

Cool. Needed really bad. Applied.

> 3.  BTW, there are too many kmalloc's/kfree's in accel_cursor() and
> softcursor().  Personally, I would rather have 2 64-byte buffers for the
> mask and the data in the info->cursor structure than allocating/freeing
> memory each time the cursor flashes.  However, if you prefer doing it
> this way, the patch also includes changes so kmallocs are only done when
> necessary.  Still, accel_cursor() has unnecessary work being done, such
> as always creating the mask bitmap, when a simple flag to monitor cursor
> shape changes could prevent all this.

I agree. The problem is the upper layer of the console system is to brain 
dead. Its either erase the cursor or redraw it again. There is no way to 
just say cursor just moved. There is a CM_MOVE but the upper layer doesn't 
even use it :-( If you look at vgacon and friends you will see they 
recreate the cursor every time the cursor blinks. Yes even vgacon.c does 
this. It is stupid and brain dead but that is the way the upper layers of 
the console work. The correct solution would be to use actually use 
CM_MOVE in the upper layers.

> 5.  softcursor should not concern itself with memory bookkeeping, and
> must be able to function with just the parameter passed to it in order
> to keep it as simple as possible.  These tasks are moved to
> accel_cursor.

We do if we make a ioctl for cursors. I'm trying to avoid reprogramming 
the hardware over and over again if the properties of the cursor don't 
change. The idea is similar to passing in var and comparing it to the var 
in struct fb_info. 

> 6.  The patch should also fix the "irregularly dashed" cursor.

Okay.

> PS:  What happened to the logo?  I just get a white square in the uppper left corner.

Strange. It should work. I reworked the logo code anyways. I did this 
because if we have more than one framebuffer in the system, say vesafb and 
vga16fb. We need to select the 16 color logo and a 224 color logo. The 
right logo needs to be draw on the right screen.



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Antonino Daplas]

On Wed, 2003-03-26 at 13:34, James Simmons wrote:
> 
> > 5.  softcursor should not concern itself with memory bookkeeping, and
> > must be able to function with just the parameter passed to it in order
> > to keep it as simple as possible.  These tasks are moved to
> > accel_cursor.
> 
> We do if we make a ioctl for cursors. I'm trying to avoid reprogramming 
> the hardware over and over again if the properties of the cursor don't 
> change. The idea is similar to passing in var and comparing it to the var 
> in struct fb_info. 

Of course, that's what the fb_cursor.set field is for, and drivers have
the option of ignoring or not ignoring bits in this field. Whoever calls
fb_cursor has the responsibility of setting any cursor state changes. 

Unlike fb_set_var(), cursor states change very frequently (ie, each
blink or movement of the cursor are considered state changes), so just
forego the memcmp() and call fb_cursor unconditionally.  Let the
low-level method sort it out by checking bits in fb_cursor.set.

But what I really meant was since accel_cursor() is already doing the
memory bookkeeping, why let softcursor do it too?  We can all do these
in the upper layer.

This is especially true if you are planning to expose cursor handling to
user space.  For example, softcursor refers to fields in
fb_info.fb_cursor, but this is a structure private to the driver. 
Worse, it refers to both the passed fb_cursor structure and the
fb_info.fb_cursor structure. Why not just make softcursor refer entirely
to the passed fb_cursor structure?  It's saner, less confusing and less
prone to bugs.  

Tony
 




-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en

Re: [BK FBDEV] A few more updates.

[Antonino Daplas]

On Wed, 2003-03-26 at 13:34, James Simmons wrote:
> 
> > 3.  BTW, there are too many kmalloc's/kfree's in accel_cursor() and
> > softcursor().  Personally, I would rather have 2 64-byte buffers for the
> > mask and the data in the info->cursor structure than allocating/freeing
> > memory each time the cursor flashes.  However, if you prefer doing it
> > this way, the patch also includes changes so kmallocs are only done when
> > necessary.  Still, accel_cursor() has unnecessary work being done, such
> > as always creating the mask bitmap, when a simple flag to monitor cursor
> > shape changes could prevent all this.
> 
> I agree. The problem is the upper layer of the console system is to brain 
> dead. Its either erase the cursor or redraw it again. There is no way to 
> just say cursor just moved. There is a CM_MOVE but the upper layer doesn't 
> even use it :-( If you look at vgacon and friends you will see they 
> recreate the cursor every time the cursor blinks. Yes even vgacon.c does 
> this. It is stupid and brain dead but that is the way the upper layers of 
> the console work. The correct solution would be to use actually use 
> CM_MOVE in the upper layers.

Even so, (and I don't really fault the console cursor as it only needs
to show, hide and move the cursor), accel_cursor() can easily monitor
shape changes.  We can use a bitfield somewhere in fb_cursor(perhaps the
high 8 bits of info->fb_cursor.set?) to "remember" the current cursor
shape.

Tony




-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en