framebuffer_{alloc,release}

framebuffer_{alloc,release}

[Kronos]

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

Re: framebuffer_{alloc,release}

[Sven Luther]

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

Re: framebuffer_{alloc,release}

[Kronos]

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

Re: framebuffer_{alloc,release}

[James Simmons]

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. 

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



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

Re: framebuffer_{alloc,release}

[Kronos]

Hum, I don't see original email...

James Simmons wrote:
>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.

framebuffer_alloc doesn't assume anything. It allocates fb_info->par iff
it's  told to. For  dual  head you  can allocate  par  yourself (and  do
whatever you want with it) or do something like this (matrox fb is doing
something similar):

if (seconday) {
        fb_info = framebuffer_alloc(0, NULL);
        fb_info->par = primary->par;
} else
        fb_info = framebuffer_alloc(sizeof(struct foo_par), NULL);

release_fb_info  won't  touch  fb_info->par  unless it  was  created  by
framebuffer_alloc. If  secondary driver  can work  even when  primary is
unloaded  then you  can  use  class_device_get (or  a  wrapper) to  keep
primary fb data around:

if (secondary) {
        if (!primary_fb_info)
                secondary_fb_info = framebuffer_alloc(sizeof(struct foo_par), NULL);
        else {
                if (!class_device_get(&primary_fb_info->class_dev))
                        /* primary is going away */
                else {
                        secondary_fb_info = framebuffer_alloc(0, NULL);
                        secondary_fb_info->par = primary_fb_info->par;
                }
        }
}


I've created a bk tree here:

bk://mesa3d.bkbits.net/fbdev-2.5

It's cloned from your tree and you can do a bk pull. You'll get the
following changesets:

<kronos@kronoz.cjb.net> (03/09/17 1.1285)
   Update sparc framebuffer drivers to the new API.

<kronos@kronoz.cjb.net> (03/09/17 1.1281.1.1)
   Add new API framebuffer_alloc and framebuffer_release.

   Framebuffer info structure (ie. struct fb_info) must be obtained from
   framebuffer_alloc. When it is no longer needed (after unregister_framebuffer
   and clean up) it can be released using framebuffer_release.

   If the framebuffer is not registered yet (eg. on error path) then fb_info must
   be released via kfree.


David S. Miller has ack'ed sparc changes.

Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
Runtime error 6D at f000:a12f : user incompetente


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

Re: framebuffer_{alloc,release}

[James Simmons]

> framebuffer_alloc doesn't assume anything. It allocates fb_info->par iff
> it's  told to. For  dual  head you  can allocate  par  yourself (and  do
> whatever you want with it) or do something like this (matrox fb is doing
> something similar):
> 
> if (seconday) {
>         fb_info = framebuffer_alloc(0, NULL);
>         fb_info->par = primary->par;
> } else
>         fb_info = framebuffer_alloc(sizeof(struct foo_par), NULL);
> 
> release_fb_info  won't  touch  fb_info->par  unless it  was  created  by
> framebuffer_alloc. If  secondary driver  can work  even when  primary is
> unloaded  then you  can  use  class_device_get (or  a  wrapper) to  keep
> primary fb data around:
> 
> if (secondary) {
>         if (!primary_fb_info)
>                 secondary_fb_info = framebuffer_alloc(sizeof(struct foo_par), NULL);
>         else {
>                 if (!class_device_get(&primary_fb_info->class_dev))
>                         /* primary is going away */
>                 else {
>                         secondary_fb_info = framebuffer_alloc(0, NULL);
>                         secondary_fb_info->par = primary_fb_info->par;
>                 }
>         }
> }

This is acceptable. I applied your changes.




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

Re: framebuffer_{alloc,release}

[Kronos]

Il Mon, Sep 22, 2003 at 10:24:04PM +0100, James Simmons ha scritto: 
> This is acceptable. I applied your changes.

Good. I've just finished matroxfb, you can do a bk pull from:

bk://mesa3d.bkbits.net/fbdev-2.5

This will update the following files:

 drivers/video/Kconfig                   |   20 
 drivers/video/matrox/g450_pll.c         |  166 +++---
 drivers/video/matrox/g450_pll.h         |    6 
 drivers/video/matrox/i2c-matroxfb.c     |   18 
 drivers/video/matrox/matroxfb_DAC1064.c |  586 ++++++++++++------------
 drivers/video/matrox/matroxfb_DAC1064.h |    4 
 drivers/video/matrox/matroxfb_Ti3026.c  |  256 +++++-----
 drivers/video/matrox/matroxfb_accel.c   |  130 ++---
 drivers/video/matrox/matroxfb_accel.h   |    2 
 drivers/video/matrox/matroxfb_base.c    |  778 +++++++++++++++-----------------
 drivers/video/matrox/matroxfb_base.h    |   76 ---
 drivers/video/matrox/matroxfb_crtc2.c   |  259 +++++-----
 drivers/video/matrox/matroxfb_crtc2.h   |    2 
 drivers/video/matrox/matroxfb_g450.c    |  162 +++---
 drivers/video/matrox/matroxfb_g450.h    |    8 
 drivers/video/matrox/matroxfb_maven.c   |   38 -
 drivers/video/matrox/matroxfb_misc.c    |  264 +++++-----
 drivers/video/matrox/matroxfb_misc.h    |   10 
 18 files changed, 1355 insertions(+), 1430 deletions(-)

through these ChangeSets:

<kronos@kronoz.cjb.net> (03/09/23 1.1290)
   Port driver to the new framebuffer_{alloc,release} API.

<kronos@kronoz.cjb.net> (03/09/21 1.1289)
   Remove FB_MATROX_MULTIHEAD and helper macros.
   Now minfo is always dynamically allocated.


Luca
-- 
Reply-To: kronos@kronoz.cjb.net
Home: http://kronoz.cjb.net
"La mia teoria scientifica preferita e` quella secondo la quale gli 
 anelli di Saturno sarebbero interamente composti dai bagagli andati 
 persi nei viaggi aerei." -- Mark Russel


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