framebuffer_{alloc,release}

Hi,
this is a new version of the new framebuffer_alloc API. It is based on the
feedback I had (thanks to Russel King).

framebuffer_alloc  is unchanged,  driver  specific  release callback  is
gone. struct fb_info won't go away until we drop the refcount with a new
function. I think that framebuffer_free can  be misleading as it doesn't
free anything, so I named it framebuffer_release.

Drivers will do something like this:

foo_probe(...) {
        fb_info = framebuffer_alloc(...);
        register_framebuffer(fb_info);
}

foo_remove(...) {
        unregister_framebuffer(fb_info);
        /* Do clean up */
        framebuffer_release(fb_info);
}

As we already have (un)register_framebuffer I'm thinking to change names
to alloc_framebuffer and release_framebuffer.

Comments?

(patch against James Simmons tree)

===== drivers/video/fbmem.c 1.93 vs edited =====

--- 1.93/drivers/video/fbmem.c	Sun Sep 14 16:50:50 2003
+++ edited/drivers/video/fbmem.c	Tue Sep 16 21:20:06 2003

@@ -1215,81 +1215,91 @@
 #endif
 };
 
-struct fb_dev {
-	struct list_head node;
-	dev_t dev;
-	struct class_device class_dev;
-};
-#define to_fb_dev(d) container_of(d, struct fb_dev, class_dev)
+#define to_fb_info(d) container_of(d, struct fb_info, class_dev)
 
-static void release_fb_dev(struct class_device *class_dev)
+static void release_fb_info(struct class_device *class_dev)
 {
-	struct fb_dev *fb_dev = to_fb_dev(class_dev);
-	kfree(fb_dev);
+	struct fb_info *fb_info = to_fb_info(class_dev);
+
+	/* This doesn't harm */
+	fb_dealloc_cmap(&fb_info->cmap);
+	
+	kfree(fb_info);
 }
 
 static struct class fb_class = {
 	.name		= "graphics",
-	.release	= &release_fb_dev,
+	.release	= &release_fb_info,
 };
 
-static LIST_HEAD(fb_dev_list);
-static spinlock_t fb_dev_list_lock = SPIN_LOCK_UNLOCKED;
-
 static ssize_t show_dev(struct class_device *class_dev, char *buf)
 {
-	struct fb_dev *fb_dev = to_fb_dev(class_dev);
-	return print_dev_t(buf, fb_dev->dev);
+	struct fb_info *fb_info = to_fb_info(class_dev);
+	return sprintf(buf, "%u:%u\n", FB_MAJOR, fb_info->node);
 }
 static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
 
-static void fb_add_class_device(int minor, struct device *device)
+static int fb_add_class_device(struct fb_info *fb_info)
 {
-	struct fb_dev *fb_dev = NULL;
 	int retval;
-
-	fb_dev = kmalloc(sizeof(*fb_dev), GFP_KERNEL);
-	if (!fb_dev)
-		return;
-	memset(fb_dev, 0x00, sizeof(*fb_dev));
-
-	fb_dev->dev = MKDEV(FB_MAJOR, minor);
-	fb_dev->class_dev.dev = device;
-	fb_dev->class_dev.class = &fb_class;
-	snprintf(fb_dev->class_dev.class_id, BUS_ID_SIZE, "fb%d", minor);
-	retval = class_device_register(&fb_dev->class_dev);
+	
+	fb_info->class_dev.class = &fb_class;
+	snprintf(fb_info->class_dev.class_id, BUS_ID_SIZE, "fb%d", fb_info->node);
+	retval = class_device_register(&fb_info->class_dev);
 	if (retval)
-		goto error;
-	class_device_create_file(&fb_dev->class_dev, &class_device_attr_dev);
-	spin_lock(&fb_dev_list_lock);
-	list_add(&fb_dev->node, &fb_dev_list);
-	spin_unlock(&fb_dev_list_lock);
-	return;
-error:
-	kfree(fb_dev);
+		return retval;
+	return class_device_create_file(&fb_info->class_dev, &class_device_attr_dev);
 }
 
-void fb_remove_class_device(int minor)
-{
-	struct fb_dev *fb_dev = NULL;
-	struct list_head *tmp;
-	int found = 0;
-
-	spin_lock(&fb_dev_list_lock);
-	list_for_each(tmp, &fb_dev_list) {
-		fb_dev = list_entry(tmp, struct fb_dev, node);
-		if ((MINOR(fb_dev->dev) == minor)) {
-			found = 1;
-			break;
-		}
-	}
-	if (found) {
-		list_del(&fb_dev->node);
-		spin_unlock(&fb_dev_list_lock);
-		class_device_unregister(&fb_dev->class_dev);
-	} else {
-		spin_unlock(&fb_dev_list_lock);
-	}
+/**
+ * framebuffer_alloc - creates a new frame buffer info structure
+ *
+ * @size: size of driver private data, can be zero
+ * @dev: pointer to the device for this fb, can be NULL
+ *
+ * Creates a new frame buffer info structure. Also reserves @size bytes
+ * for driver private data (fb_info->par). fb_info->par (if any) will be
+ * aligned to sizeof(long).
+ * 
+ * Returns the new structure, or NULL if an error occurred.
+ *
+ */
+struct fb_info *framebuffer_alloc(size_t size, struct device *dev) {
+#define BYTES_PER_LONG (BITS_PER_LONG/8)
+#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
+	struct fb_info *fb_info;
+	char *p;
+	int fb_info_size = sizeof(struct fb_info);
+
+	if (size)
+		fb_info_size += PADDING;
+
+	p = kmalloc(fb_info_size + size, GFP_KERNEL);
+	if (!p)
+		return NULL;
+	memset(p, 0, fb_info_size + size);
+	fb_info = (struct fb_info *)p;
+	fb_info->class_dev.dev = dev;
+
+	if (size)
+		fb_info->par = p + fb_info_size;
+
+	return fb_info;
+#undef PADDING
+#undef BYTES_PER_LONG
+}
+
+/**
+ * framebuffer_release - marks the structure available for freeing
+ * 
+ * @fb_info: frame buffer info structure
+ *
+ * Drop the reference count of the class_device embedded in the
+ * framebuffer info structure.
+ * 
+ */
+void framebuffer_release(struct fb_info *fb_info) {
+	class_device_put(&fb_info->class_dev);
 }
 
 /**

@@ -1315,6 +1325,9 @@
 			break;
 	fb_info->node = i;
 	
+	if (fb_add_class_device(fb_info))
+		return -EINVAL;
+	
 	if (fb_info->pixmap.addr == NULL) {
 		fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL);
 		if (fb_info->pixmap.addr) {

@@ -1352,7 +1365,6 @@
 	devfs_mk_cdev(MKDEV(FB_MAJOR, i),
 			S_IFCHR | S_IRUGO | S_IWUGO, "fb/%d", i);
 
-	fb_add_class_device(i, fb_info->dev);
 	return 0;
 }
 

@@ -1381,7 +1393,7 @@
 		kfree(fb_info->pixmap.addr);
 	registered_fb[i]=NULL;
 	num_registered_fb--;
-	fb_remove_class_device(i);
+	class_device_del(&fb_info->class_dev);
 	return 0;
 }
 

@@ -1491,6 +1503,8 @@
 
 EXPORT_SYMBOL(register_framebuffer);
 EXPORT_SYMBOL(unregister_framebuffer);
+EXPORT_SYMBOL(framebuffer_alloc);
+EXPORT_SYMBOL(framebuffer_release);
 EXPORT_SYMBOL(num_registered_fb);
 EXPORT_SYMBOL(registered_fb);
 EXPORT_SYMBOL(fb_prepare_logo);

Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
Una donna sposa un uomo sperando che cambi, e lui non cambiera`. Un
uomo sposa una donna sperando che non cambi, e lei cambiera`.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

On Tue, Sep 16, 2003 at 09:27:53PM +0200, Kronos wrote:

Hi,
this is a new version of the new framebuffer_alloc API. It is based on the
feedback I had (thanks to Russel King).

framebuffer_alloc  is unchanged,  driver  specific  release callback  is
gone. struct fb_info won't go away until we drop the refcount with a new
function. I think that framebuffer_free can  be misleading as it doesn't
free anything, so I named it framebuffer_release.

Just a quick question about this. I didn't really follow this thread,
but it seem to me that you are implementing a sort of reference counting
memory allocation, or something such.

If so, it has been proven that exploratory garbage collection is more
effective than reference-sounting in normal programming, even in
imperative languages like C. I don't know if these results will apply as
well in this case which handles framebuffer memory (or perhaps also the
texture memory the DRI framework uses ?). I think the disadvantage of
not being able to handle circular structure is of no moment, as i doubt
circular structures will be implemtented in framebuffer memory.

Anyway have you at least looked a bit at the possibility of using an
exploratory garbage collection in this case, as well as designing a
framework which can be shared between not only the fbdevs but also the
other framebuffer using stuff (dri, X, etc.).

Disclaimer: i am by no means an expert of memory allocation or garbage
collection, altough my main programming language is ocaml, so what i say
hear may well be nonsense or something. Please excuse me if this is the
case.

Friendly,

Sven Luther



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Wed, Sep 17, 2003 at 10:52:57AM +0200, Sven Luther ha scritto: 

On Tue, Sep 16, 2003 at 09:27:53PM +0200, Kronos wrote:

quoted

Hi,
this is a new version of the new framebuffer_alloc API. It is based on the
feedback I had (thanks to Russel King).

framebuffer_alloc  is unchanged,  driver  specific  release callback  is
gone. struct fb_info won't go away until we drop the refcount with a new
function. I think that framebuffer_free can  be misleading as it doesn't
free anything, so I named it framebuffer_release.

Just a quick question about this. I didn't really follow this thread,
but it seem to me that you are implementing a sort of reference counting
memory allocation, or something such.

We are talking about controlling  lifetime of framebuffer info structure
(struct fb_info). This  structure contains a description  of framebuffer
(current  mode, timings,  color  map, etc.). We  want  to register  this
structure with the driver core so  that drivers can export whatever they
want using sysfs.

If so, it has been proven that exploratory garbage collection is more
effective than reference-sounting in normal programming, even in
imperative languages like C. I don't know if these results will apply as
well in this case which handles framebuffer memory (or perhaps also the
texture memory the DRI framework uses ?).

Framebuffer memory  (ie. the  place where  we draw)  is the  video board
memory,  we just  map it  upon  load and  unmap it  on exit. AFAIK  only
virtual framebuffer uses vmalloc() for its framebuffer memory.

Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
Se il  destino di un uomo  e` annegare, anneghera` anche  in un bicchier
d'acqua.
Proverbio yddish


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

The patch looks nice except for one thing. framebuffer_alloc is incorrect 
in assuming there is one struct xxx_par per strut fb_info. Struct xxx_par
represents the physically hardware in a way independent of the 
framebuffer. The reason for this was to handle cards with more than one 
physical framebuffer. For example take a dual headed radeon card. You 
would have a design like this.
		   |-----------------	
 ____________	   |	 | /dev/fb0 |
| radeon_par | ----|	 ------------
|____________|	   |
		   |
		   |-----------------
		   	 | /dev/fb1 |
		   	 ------------

the reason for this design is simple. If fb0 video resolution changes it 
affects par. Now par's state affect fb1. We need to see if changes to 
either fb0 or fb1 affects the other display as well. 

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf