[PATCH] Cleanup pixmap locking

Hi James !

As we discussed, the current pixmap locking scheme doesn't work and
is actually rendered useless by the fact that we are going to be
entirely protected by the console semaphore, at least in 2.6.

This patch removes it, and re-implements fb_get_buffer_offset()
based on these assumption:

 - a FB_PIXMAP_IO buffer is considered as residing in VRAM, we
sync before letting the common code access it, we do no sharing

 - a buffer in memory is allowed to be shared. We do not race on
buffer usage, but we can have a pending DMA from the buffer to the
card, which is what I call sharing. If we run out of buffer space,
and the pixmap has FB_PIXMAP_SYNC set, we assume the pixmap can be
the source of a DMA, thus we sync before resetting the offset.

A card that does DMA, but synchronously, doesn't need to set FB_PIXMAP_SYNC.
Note that in most cases (non-DMA or synchronous DMA), the actual offset thing
is useless, we could just use the beginning of the pixmap each time. I find
the current FB_PIXMAP_* semantics confusing anyway, so you are welcome to do
even more cleanup in there, I just
did the minimum I feel is necessary.

I added the console semaphore around the whole fb_ioctl as well and
around the fbcon cursor operations.

The patch is against your fbdev-2.5 tree and is untested at this
point (no test config at hand until tomorrow). Please consider applying
if it works for you though.

Once that's in, I'll send you my fixes to the fbcon resize stuf, though
those require the FB_ACTIVATE_FIND to work really properly, at least
without that, it's still better than what we have now. I'm still chasing
a crasher in the stty code path though.

Ben.

1 more lines

--- 1.102/drivers/video/fbmem.c	Sat Nov  1 08:10:46 2003
+++ edited/drivers/video/fbmem.c	Sun Jan 11 18:16:51 2004

@@ -466,22 +466,30 @@
 	u32 offset, count = 1000;
 	char *addr = buf->addr;
 
-	spin_lock(&buf->lock);
-	if (!(buf->flags & FB_PIXMAP_IO)) { 
-		offset = buf->offset + align;
-		offset &= ~align;
-		if (offset + size > buf->size) {
-			while (atomic_read(&buf->count) && count--);
-			if (info->fbops->fb_sync && (buf->flags & FB_PIXMAP_SYNC))
-				info->fbops->fb_sync(info);
-			offset = 0;
-		}
-		buf->offset = offset + size;
-		addr += offset;	
+	/* If IO mapped, we need to sync before access, no sharing of
+	 * the pixmap is done
+	 */
+	if (buf->flags & FB_PIXMAP_IO) {
+		if (info->fbops->fb_sync && (buf->flags & FB_PIXMAP_SYNC))
+			info->fbops->fb_sync(info);
+		return addr;
 	}
-	atomic_inc(&buf->count);
-	smp_mb__after_atomic_inc();
-	spin_unlock(&buf->lock);
+
+	/* See if we fit in the remaining pixmap space */
+	offset = buf->offset + align;
+	offset &= ~align;
+	if (offset + size > buf->size) {
+		/* We do not fit. In order to be able to re-use the buffer,
+		 * we must ensure no asynchronous DMA'ing or whatever operation
+		 * is in progress, we sync for that.
+		 */
+		if (info->fbops->fb_sync && (buf->flags & FB_PIXMAP_SYNC))
+			info->fbops->fb_sync(info);
+		offset = 0;
+	}
+	buf->offset = offset + size;
+	addr += offset;	
+
 	return addr;
 }
 

===== drivers/video/console/fbcon.c 1.124 vs edited =====

--- 1.124/drivers/video/console/fbcon.c	Wed Jan  7 13:42:58 2004
+++ edited/drivers/video/console/fbcon.c	Sun Jan 11 18:18:34 2004

@@ -197,8 +197,10 @@
 
 	if (!info)
 		return;
-	info->cursor.enable ^= 1;
+	acquire_console_sem();
+	info->cursor.enable ^= 1;	
 	info->fbops->fb_cursor(info, &info->cursor);
+	release_console_sem();
 }
 
 #if (defined(__arm__) && defined(IRQ_VSYNCPULSE)) || defined(CONFIG_ATARI) || defined(CONFIG_MAC)

@@ -398,8 +400,6 @@
 		info->fbops->fb_imageblit(info, &image);
 		image.dx += cnt * vc->vc_font.width;
 		count -= cnt;
-		atomic_dec(&info->pixmap.count);
-		smp_mb__after_atomic_dec();
 	}
 }
 

@@ -913,8 +913,6 @@
 	move_buf_aligned(info, &info->pixmap, dst, pitch, src, width, image.height);
 
 	info->fbops->fb_imageblit(info, &image);
-	atomic_dec(&info->pixmap.count);
-	smp_mb__after_atomic_dec();
 }
 
 static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,

@@ -1064,8 +1062,6 @@
 		}
 		info->fbops->fb_cursor(info, &cursor);
 		info->cursor.enable = 1;
-		atomic_dec(&info->sprite.count);
-		smp_mb__after_atomic_dec();
 		mod_timer(&cursor_timer, jiffies + HZ/5);
 		vbl_cursor_cnt = CURSOR_DRAW_DELAY;
 	}

===== include/linux/fb.h 1.62 vs edited =====

--- 1.62/include/linux/fb.h	Thu Oct 30 17:20:52 2003
+++ edited/include/linux/fb.h	Sun Jan 11 18:15:50 2004

@@ -375,8 +375,6 @@
 	u32 scan_align;		/* alignment per scanline 		*/
 	u32 access_align;	/* alignment per read/write		*/
 	u32 flags;		/* see FB_PIXMAP_* 			*/
-	spinlock_t lock;	/* spinlock 				*/
-	atomic_t count;
 	/* access methods */
 	void (*outbuf)(struct fb_info *info, u8 *addr, u8 *src, unsigned int size); 
 	u8   (*inbuf) (u8 *addr);




-------------------------------------------------------

This SF.net email is sponsored by: Perforce Software.
Perforce is the Fast Software Configuration Management System offering
advanced branching capabilities and atomic changes on 50+ platforms.
Free Eval! http://www.perforce.com/perforce/loadprog.html

Applied. I just tested it and it works for vesafb. I will test it with a 
few other cards.


On Sun, 11 Jan 2004, Benjamin Herrenschmidt wrote:

Hi James !

As we discussed, the current pixmap locking scheme doesn't work and
is actually rendered useless by the fact that we are going to be
entirely protected by the console semaphore, at least in 2.6.

This patch removes it, and re-implements fb_get_buffer_offset()
based on these assumption:

 - a FB_PIXMAP_IO buffer is considered as residing in VRAM, we
sync before letting the common code access it, we do no sharing

 - a buffer in memory is allowed to be shared. We do not race on
buffer usage, but we can have a pending DMA from the buffer to the
card, which is what I call sharing. If we run out of buffer space,
and the pixmap has FB_PIXMAP_SYNC set, we assume the pixmap can be
the source of a DMA, thus we sync before resetting the offset.

A card that does DMA, but synchronously, doesn't need to set FB_PIXMAP_SYNC.
Note that in most cases (non-DMA or synchronous DMA), the actual offset thing
is useless, we could just use the beginning of the pixmap each time. I find
the current FB_PIXMAP_* semantics confusing anyway, so you are welcome to do
even more cleanup in there, I just
did the minimum I feel is necessary.

I added the console semaphore around the whole fb_ioctl as well and
around the fbcon cursor operations.

The patch is against your fbdev-2.5 tree and is untested at this
point (no test config at hand until tomorrow). Please consider applying
if it works for you though.

Once that's in, I'll send you my fixes to the fbcon resize stuf, though
those require the FB_ACTIVATE_FIND to work really properly, at least
without that, it's still better than what we have now. I'm still chasing
a crasher in the stty code path though.

Ben.

106 more quoted lines

-------------------------------------------------------
This SF.net email is sponsored by: Perforce Software.
Perforce is the Fast Software Configuration Management System offering
advanced branching capabilities and atomic changes on 50+ platforms.
Free Eval! http://www.perforce.com/perforce/loadprog.html