[PATCH] Framebuffer: client notification mecanism & PM

[PATCH] Framebuffer: client notification mecanism & PM

[Benjamin Herrenschmidt]

Hi !

This patch adds a mecanism for in-kernel "clients" of a framebuffer
device to get notified of events on this framebuffer device. It adds
some basic Power Management callbacks based on this, and implements
support in fbcon.

This allows fbdev low level drivers to instruct clients like "fbcon"
to stop touching the framebuffer as the hardware is going to be
suspended, and to restore the display after resume. I also defined
(but didn't use yet) a mode change hook that would allow to restore
2.4 functionality of fbset to the fbdev driver keeping fbcon in sync
with the new mode (lots of users are asking for this).

There is room for improvement, but this is a first step that makes
suspend/resume work more reliably on pmacs. I'll send aty128fb and
radeonfb bits using this framework once it's merged.

Ben.

diff -urN linux-2.5/drivers/video/console/fbcon.c linuxppc-2.5-benh/drivers/video/console/fbcon.c
--- linux-2.5/drivers/video/console/fbcon.c	2003-07-29 08:51:30.000000000 -0400
+++ linuxppc-2.5-benh/drivers/video/console/fbcon.c	2003-06-04 10:03:18.000000000 -0400
@@ -199,7 +199,8 @@
 	if (!info || (info->cursor.rop == ROP_COPY))
 		return;
 	info->cursor.enable ^= 1;
-	info->fbops->fb_cursor(info, &info->cursor);
+	if (!info->suspended)
+		info->fbops->fb_cursor(info, &info->cursor);
 }
 
 #if (defined(__arm__) && defined(IRQ_VSYNCPULSE)) || defined(CONFIG_ATARI) || defined(CONFIG_MAC)
@@ -948,6 +949,9 @@
 	if (!height || !width)
 		return;
 
+	if (info->suspended)
+		return;
+
 	/* Split blits that cross physical y_wrap boundary */
 
 	y_break = p->vrows - p->yscroll;
@@ -972,6 +976,9 @@
 	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
 		return;
 
+	if (info->suspended)
+		return;
+
 	accel_putc(vc, info, c, real_y(p, ypos), xpos);
 }
 
@@ -987,6 +994,9 @@
 	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
 		return;
 
+	if (info->suspended)
+		return;
+
 	accel_putcs(vc, info, s, count, real_y(p, ypos), xpos);
 }
 
@@ -1001,6 +1011,9 @@
 	int y = real_y(p, vc->vc_y);
 	struct fb_cursor cursor;
 	
+	if (info->suspended)
+		return;
+
 	if (mode & CM_SOFTBACK) {
 		mode &= ~CM_SOFTBACK;
 		if (softback_lines) {
@@ -1385,6 +1398,9 @@
 	if (!count || vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
 		return 0;
 
+	if (info->suspended)
+		return 0;
+
 	fbcon_cursor(vc, CM_ERASE);
 
 	/*
@@ -1545,6 +1561,9 @@
 	if (!width || !height)
 		return;
 
+	if (info->suspended)
+		return;
+
 	/*  Split blits that cross physical y_wrap case.
 	 *  Pathological case involves 4 blits, better to use recursive
 	 *  code rather than unrolled case
@@ -1633,6 +1652,9 @@
 	struct fb_info *info = registered_fb[(int) con2fb_map[vc->vc_num]];
 	struct display *p = &fb_display[vc->vc_num];
 
+	if (info->suspended)
+		return 0;
+
 	if (softback_top) {
 		int l = fbcon_softback_size / vc->vc_size_row;
 		if (softback_lines)
@@ -1704,6 +1726,9 @@
 	if (blank < 0)		/* Entering graphics mode */
 		return 0;
 
+	if (info->suspended)
+		return 0;
+
 	fbcon_cursor(vc, blank ? CM_ERASE : CM_DRAW);
 
 	if (!info->fbops->fb_blank) {
@@ -2067,6 +2092,9 @@
 	int i, j, k;
 	u8 val;
 
+	if (info->suspended)
+		return 0;
+
 	if (!vc->vc_can_do_color
 	    || (!info->fbops->fb_blank && console_blanked))
 		return -EINVAL;
@@ -2171,6 +2199,9 @@
 	struct display *p = &fb_display[fg_console];
 	int offset, limit, scrollback_old;
 
+	if (info->suspended)
+		return 0;
+
 	if (softback_top) {
 		if (vc->vc_num != fg_console)
 			return 0;
@@ -2259,6 +2290,19 @@
 	return 0;
 }
 
+static void fbcon_suspended(void *data, struct fb_info *info)
+{
+	/* Here, we should do something to properly
+	 * synchronize with the cursor interrupt on
+	 * SMP...
+	 */
+}
+
+static void fbcon_resumed(void *data, struct fb_info *info)
+{
+	update_screen(fg_console);
+}
+
 /*
  *  The console `switch' structure for the frame buffer based console
  */
@@ -2285,16 +2329,25 @@
 	.con_resize             = fbcon_resize,
 };
 
+static struct fb_client_ops fbcon_client = {
+	.owner			= THIS_MODULE,
+	.mode_changed		= NULL, /* TODO */
+	.suspended		= fbcon_suspended,
+	.resumed		= fbcon_resumed,
+};
+
 int __init fb_console_init(void)
 {
 	if (!num_registered_fb)
 		return -ENODEV;
 	take_over_console(&fb_con, first_fb_vc, last_fb_vc, fbcon_is_default);
+	register_fb_client(&fbcon_client, NULL);
 	return 0;
 }
 
 void __exit fb_console_exit(void)
 {
+	unregister_fb_client(&fbcon_client);
 	give_up_console(&fb_con);
 }	
 
diff -urN linux-2.5/drivers/video/fbmem.c linuxppc-2.5-benh/drivers/video/fbmem.c
--- linux-2.5/drivers/video/fbmem.c	2003-07-29 08:51:28.000000000 -0400
+++ linuxppc-2.5-benh/drivers/video/fbmem.c	2003-07-23 20:05:02.000000000 -0400
@@ -143,6 +143,8 @@
 extern int sstfb_setup(char*);
 extern int i810fb_init(void);
 extern int i810fb_setup(char*);
+extern int ibmlcdfb_init(void);
+extern int ibmlcdfb_setup(char*);
 extern int ffb_init(void);
 extern int ffb_setup(char*);
 extern int cg6_init(void);
@@ -250,6 +252,9 @@
 #ifdef CONFIG_FB_STI
 	{ "stifb", stifb_init, stifb_setup },
 #endif
+#ifdef CONFIG_FB_IBMLCDC
+	{ "ibmlcdfb", ibmlcdfb_init, ibmlcdfb_setup },
+#endif
 #ifdef CONFIG_FB_FFB
 	{ "ffb", ffb_init, ffb_setup },
 #endif
@@ -392,6 +397,9 @@
 struct fb_info *registered_fb[FB_MAX];
 int num_registered_fb;
 
+static LIST_HEAD(fb_clients);
+static DECLARE_MUTEX(fb_clients_lock);
+
 #ifdef CONFIG_FB_OF
 static int ofonly __initdata = 0;
 #endif
@@ -935,6 +943,8 @@
 			fb_pan_display(info, &info->var);
 
 			fb_set_cmap(&info->cmap, 1, info);
+
+			fb_clients_call_mode_changed(info);
 		}
 	}
 	return 0;
@@ -1371,6 +1381,82 @@
 
 __setup("video=", video_setup);
 
+int
+fb_set_suspend(struct fb_info *info, int suspended)
+{
+	if (suspended == info->suspended)
+		return 0;
+
+	info->suspended = suspended;
+	if (suspended)
+		fb_clients_call_suspended(info);
+	else
+		fb_clients_call_resumed(info);
+
+	return 0;
+}
+
+int register_fb_client(struct fb_client_ops *ops, void *data)
+{
+	struct fb_client *client;
+
+	client = kmalloc(sizeof(*client), GFP_KERNEL);
+	if (client == NULL)
+		return -ENOMEM;
+
+	memset(client, 0, sizeof(*client));
+	client->ops = ops;
+	client->data = data;
+	
+	down(&fb_clients_lock);
+	list_add(&client->link, &fb_clients);
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+int unregister_fb_client(struct fb_client_ops *ops)
+{
+	struct fb_client *client = NULL;
+	struct list_head *pos;
+	
+	down(&fb_clients_lock);
+	list_for_each(pos, &fb_clients) {
+		client = list_entry(pos, struct fb_client, link);
+		if (client->ops == ops) {
+			list_del(&client->link);
+			kfree(client);
+			break;
+		}
+	}
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+#define make_fb_client_call(name) \
+int fb_clients_call_##name(struct fb_info *info) \
+{ \
+	struct fb_client *client = NULL; \
+	struct list_head *pos; \
+\
+	down(&fb_clients_lock); \
+	list_for_each(pos, &fb_clients) { \
+		client = list_entry(pos, struct fb_client, link); \
+		if (try_module_get(client->ops->owner)) { \
+			if (client->ops->name) \
+				client->ops->name(client->data, info); \
+			module_put(client->ops->owner); \
+		} \
+	} \
+	up(&fb_clients_lock); \
+	return 0; \
+}
+
+make_fb_client_call(mode_changed)
+make_fb_client_call(suspended)
+make_fb_client_call(resumed)
+
     /*
      *  Visible symbols for modules
      */
@@ -1387,5 +1473,11 @@
 EXPORT_SYMBOL(fb_get_buffer_offset);
 EXPORT_SYMBOL(move_buf_unaligned);
 EXPORT_SYMBOL(move_buf_aligned);
+EXPORT_SYMBOL(fb_set_suspend);
+EXPORT_SYMBOL(register_fb_client);
+EXPORT_SYMBOL(unregister_fb_client);
+EXPORT_SYMBOL(fb_clients_call_mode_changed);
+EXPORT_SYMBOL(fb_clients_call_suspended);
+EXPORT_SYMBOL(fb_clients_call_resumed);
 
 MODULE_LICENSE("GPL");
diff -urN linux-2.5/include/linux/fb.h linuxppc-2.5-benh/include/linux/fb.h
--- linux-2.5/include/linux/fb.h	2003-07-29 08:52:16.000000000 -0400
+++ linuxppc-2.5-benh/include/linux/fb.h	2003-07-23 20:05:24.000000000 -0400
@@ -352,6 +352,44 @@
 struct fb_info;
 struct vm_area_struct;
 struct file;
+struct fb_client;
+
+	/*
+	 * Framebuffer clients. Currently, this is only used
+	 * by fbcon to get notified of events on the framebuffer,
+	 * though that should be extended to the userland interface
+	 * some way.
+	 * 
+	 * We should also add more callbacks to better deal with
+	 * hotplug displays (add/removal notification). This is
+	 * not to replaced by a device class, though it could be
+	 * wrapped in a device interface according to the driver
+	 * model, I have to think more about it.
+	 * 
+	 * Locking rules: The callback should not take the console
+	 * semaphore explicitely (call acquire_console_sem()) as it
+	 * will typically already be owned.
+	 * 
+	 */ 
+struct fb_client_ops {	
+	struct module *owner;
+
+	/* Userland initiated mode change */
+	void	(*mode_changed)(void *data, struct fb_info *info);
+	/* The device is beeing suspended, do not access from
+	 * that point
+	 */
+	void	(*suspended)(void *data, struct fb_info *info);
+	/* The device is back to life, refresh screen
+	 */
+	void	(*resumed)(void *data, struct fb_info *info);
+};
+
+struct fb_client {
+	struct list_head	link;
+	struct fb_client_ops	*ops;
+	void			*data;
+};
 
     /*
      *  Frame buffer operations
@@ -399,6 +437,7 @@
    int node;
    int flags;
    int open;                            /* Has this been open already ? */
+   int suspended;			/* Is this currently suspended ? */
 #define FBINFO_FLAG_MODULE	1	/* Low-level driver is a module */
    struct fb_var_screeninfo var;        /* Current var */
    struct fb_fix_screeninfo fix;        /* Current fix */
@@ -412,6 +451,7 @@
    struct vc_data *display_fg;		/* Console visible on this display */
    int currcon;				/* Current VC. */	
    void *pseudo_palette;                /* Fake palette of 16 colors */ 
+   
    /* From here on everything is device dependent */
    void *par;	
 };
@@ -575,6 +615,22 @@
 			       unsigned int default_bpp);
 #endif
 
+/* Power Management: called by low driver to notify other layers,
+ * driver should have acquired the console semaphore prior to
+ * calling this
+ */
+extern int fb_set_suspend(struct fb_info *info, int suspended);
+
+/*
+ * fb_client operations
+ */
+
+extern int register_fb_client(struct fb_client_ops *ops, void *data);
+extern int unregister_fb_client(struct fb_client_ops *ops);
+extern int fb_clients_call_mode_changed(struct fb_info *info);
+extern int fb_clients_call_suspended(struct fb_info *info);
+extern int fb_clients_call_resumed(struct fb_info *info);
+
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_FB_H */

Re: [PATCH] Framebuffer: client notification mecanism & PM

[Pavel Machek]

Hi!

> There is room for improvement, but this is a first step that makes
> suspend/resume work more reliably on pmacs. I'll send aty128fb and
> radeonfb bits using this framework once it's merged.

> @@ -199,7 +199,8 @@
>  	if (!info || (info->cursor.rop == ROP_COPY))
>  		return;
>  	info->cursor.enable ^= 1;
> -	info->fbops->fb_cursor(info, &info->cursor);
> +	if (!info->suspended)
> +		info->fbops->fb_cursor(info, &info->cursor);
>  }
>  
>  	y_break = p->vrows - p->yscroll;

Would not it be simpler to replace info->fbops with some dummy ones?
-- 
				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Re: [PATCH] Framebuffer: client notification mecanism & PM

[Benjamin Herrenschmidt]

> 
> Would not it be simpler to replace info->fbops with some dummy ones?

I did that for 2.4, that's indeed an option. I suppose we are completely
sure fbcon will never touch the framebuffer without going through those
fb_ops (fb_fillrect, fb_imageblit, etc...) ? That mean we would still
run a lot of useless code, but that's not too bad.

We still want the notification mecanism though, at least for proper
refresh on wakeup, and other "clients" (the DRI comes to mind) may
make good use of it as well.

Ben.

[PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

So, here's a new version that removes checking for suspend field
all over fbcon. I tried it here with a modified radeonfb that sets
the fbops to some dummy functions (provided by fbmem.c in this
patch though you may want to move them around).
In theory, I could implement power management properly without the client
notification mecanism by just setting the dummy fbops and directly calling
update_screen from the low level fbdev on resume, but that doesn't smell
good. The console subsystem is a bit of a nightmare to me, James, can we
currently have several consoles on several heads ? (that currcons in
vt.c is disturbing me). It may be worth doing better than just update_screen
(fg_console) when resuming in fbcon.c... (this callback will be called for
each fbdev who is waking up).

Let me know what you think, in all cases, asap so I can push the remaining
fbdev driver bits.

Ben.

===== drivers/video/fbmem.c 1.77 vs edited =====
--- 1.77/drivers/video/fbmem.c	Mon May 26 20:51:43 2003
+++ edited/drivers/video/fbmem.c	Wed Jul 30 08:59:06 2003
@@ -392,6 +392,9 @@
 struct fb_info *registered_fb[FB_MAX];
 int num_registered_fb;
 
+static LIST_HEAD(fb_clients);
+static DECLARE_MUTEX(fb_clients_lock);
+
 #ifdef CONFIG_FB_OF
 static int ofonly __initdata = 0;
 #endif
@@ -935,6 +938,8 @@
 			fb_pan_display(info, &info->var);
 
 			fb_set_cmap(&info->cmap, 1, info);
+
+			fb_clients_call_mode_changed(info);
 		}
 	}
 	return 0;
@@ -1371,6 +1376,107 @@
 
 __setup("video=", video_setup);
 
+/*
+ * Dummy operations used typically when power managing
+ */
+
+int fb_dummy_cursor(struct fb_info *info, struct fb_cursor *cursor)
+{
+	return 0;
+}
+
+void fb_dummy_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
+{
+}
+
+void fb_dummy_copyarea(struct fb_info *info, const struct fb_copyarea *area)
+{
+}
+
+void fb_dummy_imageblit(struct fb_info *info, const struct fb_image *image)
+{
+}
+
+/*
+ * Wrappers to client calls
+ */
+
+int
+fb_set_suspend(struct fb_info *info, int suspended)
+{
+	if (suspended == info->suspended)
+		return 0;
+
+	info->suspended = suspended;
+	if (suspended)
+		fb_clients_call_suspended(info);
+	else
+		fb_clients_call_resumed(info);
+
+	return 0;
+}
+
+int register_fb_client(struct fb_client_ops *ops, void *data)
+{
+	struct fb_client *client;
+
+	client = kmalloc(sizeof(*client), GFP_KERNEL);
+	if (client == NULL)
+		return -ENOMEM;
+
+	memset(client, 0, sizeof(*client));
+	client->ops = ops;
+	client->data = data;
+	
+	down(&fb_clients_lock);
+	list_add(&client->link, &fb_clients);
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+int unregister_fb_client(struct fb_client_ops *ops)
+{
+	struct fb_client *client = NULL;
+	struct list_head *pos;
+	
+	down(&fb_clients_lock);
+	list_for_each(pos, &fb_clients) {
+		client = list_entry(pos, struct fb_client, link);
+		if (client->ops == ops) {
+			list_del(&client->link);
+			kfree(client);
+			break;
+		}
+	}
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+#define make_fb_client_call(name) \
+int fb_clients_call_##name(struct fb_info *info) \
+{ \
+	struct fb_client *client = NULL; \
+	struct list_head *pos; \
+\
+	down(&fb_clients_lock); \
+	list_for_each(pos, &fb_clients) { \
+		client = list_entry(pos, struct fb_client, link); \
+		if (try_module_get(client->ops->owner)) { \
+			if (client->ops->name) \
+				client->ops->name(client->data, info); \
+			module_put(client->ops->owner); \
+		} \
+	} \
+	up(&fb_clients_lock); \
+	return 0; \
+}
+
+make_fb_client_call(mode_changed)
+make_fb_client_call(suspended)
+make_fb_client_call(resumed)
+
     /*
      *  Visible symbols for modules
      */
@@ -1387,5 +1493,15 @@
 EXPORT_SYMBOL(fb_get_buffer_offset);
 EXPORT_SYMBOL(move_buf_unaligned);
 EXPORT_SYMBOL(move_buf_aligned);
+EXPORT_SYMBOL(fb_set_suspend);
+EXPORT_SYMBOL(register_fb_client);
+EXPORT_SYMBOL(unregister_fb_client);
+EXPORT_SYMBOL(fb_clients_call_mode_changed);
+EXPORT_SYMBOL(fb_clients_call_suspended);
+EXPORT_SYMBOL(fb_clients_call_resumed);
+EXPORT_SYMBOL(fb_dummy_fillrect);
+EXPORT_SYMBOL(fb_dummy_copyarea);
+EXPORT_SYMBOL(fb_dummy_imageblit);
+EXPORT_SYMBOL(fb_dummy_cursor);
 
 MODULE_LICENSE("GPL");
===== drivers/video/console/fbcon.c 1.102 vs edited =====
--- 1.102/drivers/video/console/fbcon.c	Tue May  6 09:50:50 2003
+++ edited/drivers/video/console/fbcon.c	Wed Jul 30 08:59:44 2003
@@ -2259,6 +2259,19 @@
 	return 0;
 }
 
+static void fbcon_suspended(void *data, struct fb_info *info)
+{
+	/* Here, we should do something to properly erase the
+	 * cursor and synchronize with the cursor interrupt on
+	 * SMP... (may not be that critical though...)
+	 */
+}
+
+static void fbcon_resumed(void *data, struct fb_info *info)
+{
+	update_screen(fg_console);
+}
+
 /*
  *  The console `switch' structure for the frame buffer based console
  */
@@ -2285,16 +2298,25 @@
 	.con_resize             = fbcon_resize,
 };
 
+static struct fb_client_ops fbcon_client = {
+	.owner			= THIS_MODULE,
+	.mode_changed		= NULL, /* TODO */
+	.suspended		= fbcon_suspended,
+	.resumed		= fbcon_resumed,
+};
+
 int __init fb_console_init(void)
 {
 	if (!num_registered_fb)
 		return -ENODEV;
 	take_over_console(&fb_con, first_fb_vc, last_fb_vc, fbcon_is_default);
+	register_fb_client(&fbcon_client, NULL);
 	return 0;
 }
 
 void __exit fb_console_exit(void)
 {
+	unregister_fb_client(&fbcon_client);
 	give_up_console(&fb_con);
 }	
 
===== include/linux/fb.h 1.53 vs edited =====
--- 1.53/include/linux/fb.h	Thu Apr 24 06:30:41 2003
+++ edited/include/linux/fb.h	Wed Jul 30 09:01:06 2003
@@ -352,6 +352,44 @@
 struct fb_info;
 struct vm_area_struct;
 struct file;
+struct fb_client;
+
+	/*
+	 * Framebuffer clients. Currently, this is only used
+	 * by fbcon to get notified of events on the framebuffer,
+	 * though that should be extended to the userland interface
+	 * some way.
+	 * 
+	 * We should also add more callbacks to better deal with
+	 * hotplug displays (add/removal notification). This is
+	 * not to replaced by a device class, though it could be
+	 * wrapped in a device interface according to the driver
+	 * model, I have to think more about it.
+	 * 
+	 * Locking rules: The callback should not take the console
+	 * semaphore explicitely (call acquire_console_sem()) as it
+	 * will typically already be owned.
+	 * 
+	 */ 
+struct fb_client_ops {	
+	struct module *owner;
+
+	/* Userland initiated mode change */
+	void	(*mode_changed)(void *data, struct fb_info *info);
+	/* The device is beeing suspended, do not access from
+	 * that point
+	 */
+	void	(*suspended)(void *data, struct fb_info *info);
+	/* The device is back to life, refresh screen
+	 */
+	void	(*resumed)(void *data, struct fb_info *info);
+};
+
+struct fb_client {
+	struct list_head	link;
+	struct fb_client_ops	*ops;
+	void			*data;
+};
 
     /*
      *  Frame buffer operations
@@ -399,6 +437,7 @@
    int node;
    int flags;
    int open;                            /* Has this been open already ? */
+   int suspended;			/* Is this currently suspended ? */
 #define FBINFO_FLAG_MODULE	1	/* Low-level driver is a module */
    struct fb_var_screeninfo var;        /* Current var */
    struct fb_fix_screeninfo fix;        /* Current fix */
@@ -412,6 +451,7 @@
    struct vc_data *display_fg;		/* Console visible on this display */
    int currcon;				/* Current VC. */	
    void *pseudo_palette;                /* Fake palette of 16 colors */ 
+   
    /* From here on everything is device dependent */
    void *par;	
 };
@@ -475,6 +515,10 @@
 extern void cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect); 
 extern void cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area); 
 extern void cfb_imageblit(struct fb_info *info, const struct fb_image *image);
+extern int fb_dummy_cursor(struct fb_info *info, struct fb_cursor *cursor);
+extern void fb_dummy_fillrect(struct fb_info *info, const struct fb_fillrect *rect); 
+extern void fb_dummy_copyarea(struct fb_info *info, const struct fb_copyarea *area); 
+extern void fb_dummy_imageblit(struct fb_info *info, const struct fb_image *image);
 
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
@@ -574,6 +618,22 @@
 			       const struct fb_videomode *default_mode,
 			       unsigned int default_bpp);
 #endif
+
+/* Power Management: called by low driver to notify other layers,
+ * driver should have acquired the console semaphore prior to
+ * calling this
+ */
+extern int fb_set_suspend(struct fb_info *info, int suspended);
+
+/*
+ * fb_client operations
+ */
+
+extern int register_fb_client(struct fb_client_ops *ops, void *data);
+extern int unregister_fb_client(struct fb_client_ops *ops);
+extern int fb_clients_call_mode_changed(struct fb_info *info);
+extern int fb_clients_call_suspended(struct fb_info *info);
+extern int fb_clients_call_resumed(struct fb_info *info);
 
 #endif /* __KERNEL__ */
 

[PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

(resent with proper recipients list, sorry)

So, here's a new version that removes checking for suspend field
all over fbcon. I tried it here with a modified radeonfb that sets
the fbops to some dummy functions (provided by fbmem.c in this
patch though you may want to move them around).
In theory, I could implement power management properly without the client
notification mecanism by just setting the dummy fbops and directly calling
update_screen from the low level fbdev on resume, but that doesn't smell
good. The console subsystem is a bit of a nightmare to me, James, can we
currently have several consoles on several heads ? (that currcons in
vt.c is disturbing me). It may be worth doing better than just update_screen
(fg_console) when resuming in fbcon.c... (this callback will be called for
each fbdev who is waking up).

Let me know what you think, in all cases, asap so I can push the remaining
fbdev driver bits.

Ben.

===== drivers/video/fbmem.c 1.77 vs edited =====
--- 1.77/drivers/video/fbmem.c	Mon May 26 20:51:43 2003
+++ edited/drivers/video/fbmem.c	Wed Jul 30 08:59:06 2003
@@ -392,6 +392,9 @@
 struct fb_info *registered_fb[FB_MAX];
 int num_registered_fb;
 
+static LIST_HEAD(fb_clients);
+static DECLARE_MUTEX(fb_clients_lock);
+
 #ifdef CONFIG_FB_OF
 static int ofonly __initdata = 0;
 #endif
@@ -935,6 +938,8 @@
 			fb_pan_display(info, &info->var);
 
 			fb_set_cmap(&info->cmap, 1, info);
+
+			fb_clients_call_mode_changed(info);
 		}
 	}
 	return 0;
@@ -1371,6 +1376,107 @@
 
 __setup("video=", video_setup);
 
+/*
+ * Dummy operations used typically when power managing
+ */
+
+int fb_dummy_cursor(struct fb_info *info, struct fb_cursor *cursor)
+{
+	return 0;
+}
+
+void fb_dummy_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
+{
+}
+
+void fb_dummy_copyarea(struct fb_info *info, const struct fb_copyarea *area)
+{
+}
+
+void fb_dummy_imageblit(struct fb_info *info, const struct fb_image *image)
+{
+}
+
+/*
+ * Wrappers to client calls
+ */
+
+int
+fb_set_suspend(struct fb_info *info, int suspended)
+{
+	if (suspended == info->suspended)
+		return 0;
+
+	info->suspended = suspended;
+	if (suspended)
+		fb_clients_call_suspended(info);
+	else
+		fb_clients_call_resumed(info);
+
+	return 0;
+}
+
+int register_fb_client(struct fb_client_ops *ops, void *data)
+{
+	struct fb_client *client;
+
+	client = kmalloc(sizeof(*client), GFP_KERNEL);
+	if (client == NULL)
+		return -ENOMEM;
+
+	memset(client, 0, sizeof(*client));
+	client->ops = ops;
+	client->data = data;
+	
+	down(&fb_clients_lock);
+	list_add(&client->link, &fb_clients);
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+int unregister_fb_client(struct fb_client_ops *ops)
+{
+	struct fb_client *client = NULL;
+	struct list_head *pos;
+	
+	down(&fb_clients_lock);
+	list_for_each(pos, &fb_clients) {
+		client = list_entry(pos, struct fb_client, link);
+		if (client->ops == ops) {
+			list_del(&client->link);
+			kfree(client);
+			break;
+		}
+	}
+	up(&fb_clients_lock);
+
+	return 0;
+}
+
+#define make_fb_client_call(name) \
+int fb_clients_call_##name(struct fb_info *info) \
+{ \
+	struct fb_client *client = NULL; \
+	struct list_head *pos; \
+\
+	down(&fb_clients_lock); \
+	list_for_each(pos, &fb_clients) { \
+		client = list_entry(pos, struct fb_client, link); \
+		if (try_module_get(client->ops->owner)) { \
+			if (client->ops->name) \
+				client->ops->name(client->data, info); \
+			module_put(client->ops->owner); \
+		} \
+	} \
+	up(&fb_clients_lock); \
+	return 0; \
+}
+
+make_fb_client_call(mode_changed)
+make_fb_client_call(suspended)
+make_fb_client_call(resumed)
+
     /*
      *  Visible symbols for modules
      */
@@ -1387,5 +1493,15 @@
 EXPORT_SYMBOL(fb_get_buffer_offset);
 EXPORT_SYMBOL(move_buf_unaligned);
 EXPORT_SYMBOL(move_buf_aligned);
+EXPORT_SYMBOL(fb_set_suspend);
+EXPORT_SYMBOL(register_fb_client);
+EXPORT_SYMBOL(unregister_fb_client);
+EXPORT_SYMBOL(fb_clients_call_mode_changed);
+EXPORT_SYMBOL(fb_clients_call_suspended);
+EXPORT_SYMBOL(fb_clients_call_resumed);
+EXPORT_SYMBOL(fb_dummy_fillrect);
+EXPORT_SYMBOL(fb_dummy_copyarea);
+EXPORT_SYMBOL(fb_dummy_imageblit);
+EXPORT_SYMBOL(fb_dummy_cursor);
 
 MODULE_LICENSE("GPL");
===== drivers/video/console/fbcon.c 1.102 vs edited =====
--- 1.102/drivers/video/console/fbcon.c	Tue May  6 09:50:50 2003
+++ edited/drivers/video/console/fbcon.c	Wed Jul 30 08:59:44 2003
@@ -2259,6 +2259,19 @@
 	return 0;
 }
 
+static void fbcon_suspended(void *data, struct fb_info *info)
+{
+	/* Here, we should do something to properly erase the
+	 * cursor and synchronize with the cursor interrupt on
+	 * SMP... (may not be that critical though...)
+	 */
+}
+
+static void fbcon_resumed(void *data, struct fb_info *info)
+{
+	update_screen(fg_console);
+}
+
 /*
  *  The console `switch' structure for the frame buffer based console
  */
@@ -2285,16 +2298,25 @@
 	.con_resize             = fbcon_resize,
 };
 
+static struct fb_client_ops fbcon_client = {
+	.owner			= THIS_MODULE,
+	.mode_changed		= NULL, /* TODO */
+	.suspended		= fbcon_suspended,
+	.resumed		= fbcon_resumed,
+};
+
 int __init fb_console_init(void)
 {
 	if (!num_registered_fb)
 		return -ENODEV;
 	take_over_console(&fb_con, first_fb_vc, last_fb_vc, fbcon_is_default);
+	register_fb_client(&fbcon_client, NULL);
 	return 0;
 }
 
 void __exit fb_console_exit(void)
 {
+	unregister_fb_client(&fbcon_client);
 	give_up_console(&fb_con);
 }	
 
===== include/linux/fb.h 1.53 vs edited =====
--- 1.53/include/linux/fb.h	Thu Apr 24 06:30:41 2003
+++ edited/include/linux/fb.h	Wed Jul 30 09:01:06 2003
@@ -352,6 +352,44 @@
 struct fb_info;
 struct vm_area_struct;
 struct file;
+struct fb_client;
+
+	/*
+	 * Framebuffer clients. Currently, this is only used
+	 * by fbcon to get notified of events on the framebuffer,
+	 * though that should be extended to the userland interface
+	 * some way.
+	 * 
+	 * We should also add more callbacks to better deal with
+	 * hotplug displays (add/removal notification). This is
+	 * not to replaced by a device class, though it could be
+	 * wrapped in a device interface according to the driver
+	 * model, I have to think more about it.
+	 * 
+	 * Locking rules: The callback should not take the console
+	 * semaphore explicitely (call acquire_console_sem()) as it
+	 * will typically already be owned.
+	 * 
+	 */ 
+struct fb_client_ops {	
+	struct module *owner;
+
+	/* Userland initiated mode change */
+	void	(*mode_changed)(void *data, struct fb_info *info);
+	/* The device is beeing suspended, do not access from
+	 * that point
+	 */
+	void	(*suspended)(void *data, struct fb_info *info);
+	/* The device is back to life, refresh screen
+	 */
+	void	(*resumed)(void *data, struct fb_info *info);
+};
+
+struct fb_client {
+	struct list_head	link;
+	struct fb_client_ops	*ops;
+	void			*data;
+};
 
     /*
      *  Frame buffer operations
@@ -399,6 +437,7 @@
    int node;
    int flags;
    int open;                            /* Has this been open already ? */
+   int suspended;			/* Is this currently suspended ? */
 #define FBINFO_FLAG_MODULE	1	/* Low-level driver is a module */
    struct fb_var_screeninfo var;        /* Current var */
    struct fb_fix_screeninfo fix;        /* Current fix */
@@ -412,6 +451,7 @@
    struct vc_data *display_fg;		/* Console visible on this display */
    int currcon;				/* Current VC. */	
    void *pseudo_palette;                /* Fake palette of 16 colors */ 
+   
    /* From here on everything is device dependent */
    void *par;	
 };
@@ -475,6 +515,10 @@
 extern void cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect); 
 extern void cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area); 
 extern void cfb_imageblit(struct fb_info *info, const struct fb_image *image);
+extern int fb_dummy_cursor(struct fb_info *info, struct fb_cursor *cursor);
+extern void fb_dummy_fillrect(struct fb_info *info, const struct fb_fillrect *rect); 
+extern void fb_dummy_copyarea(struct fb_info *info, const struct fb_copyarea *area); 
+extern void fb_dummy_imageblit(struct fb_info *info, const struct fb_image *image);
 
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
@@ -574,6 +618,22 @@
 			       const struct fb_videomode *default_mode,
 			       unsigned int default_bpp);
 #endif
+
+/* Power Management: called by low driver to notify other layers,
+ * driver should have acquired the console semaphore prior to
+ * calling this
+ */
+extern int fb_set_suspend(struct fb_info *info, int suspended);
+
+/*
+ * fb_client operations
+ */
+
+extern int register_fb_client(struct fb_client_ops *ops, void *data);
+extern int unregister_fb_client(struct fb_client_ops *ops);
+extern int fb_clients_call_mode_changed(struct fb_info *info);
+extern int fb_clients_call_suspended(struct fb_info *info);
+extern int fb_clients_call_resumed(struct fb_info *info);
 
 #endif /* __KERNEL__ */
 

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

James, if you are ok, can you get that upstream to Linus asap so
I can start pushing the driver bits for radeon & aty128 ?

Ben.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[James Simmons]

> James, if you are ok, can you get that upstream to Linus asap so
> I can start pushing the driver bits for radeon & aty128 ?

Working on it. I'm thinking about also how it effects userland and how 
userland affects the console if present. Basically the logic will go 

pci suspend ->  framebuffer driver supend function -> call each client

Just give me a few days to piece it together.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

On Thu, 2003-08-07 at 01:52, James Simmons wrote:
> > James, if you are ok, can you get that upstream to Linus asap so
> > I can start pushing the driver bits for radeon & aty128 ?
> 
> Working on it. I'm thinking about also how it effects userland and how 
> userland affects the console if present. Basically the logic will go 
> 
> pci suspend ->  framebuffer driver supend function -> call each client
> 
> Just give me a few days to piece it together.

Right now, we don't have a proper userland notification. So far, the
main affected thing is XFree, but this is ok as it will have received
a suspend request via /dev/apm_bios (which we emulate on PowerMacs),
and so won't touch the framebuffer until resumed.

There isn't much we can do against a userland client tapping the
framebuffer that it mmap'ed previously. I don't know how feasible it
would be to sort of "hack" this process mapping on the fly (would
involve some nasty SMP synchronisation issues) so that the userland
process is just put to sleep on fb access while the fb is suspended
(or get a SEGV). We probably want to extend the notification mecanism
to userland in some way, but this isn't something i cover in this
patch.

Ben.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Pavel Machek]

Hi!

> > > James, if you are ok, can you get that upstream to Linus asap so
> > > I can start pushing the driver bits for radeon & aty128 ?
> > 
> > Working on it. I'm thinking about also how it effects userland and how 
> > userland affects the console if present. Basically the logic will go 
> > 
> > pci suspend ->  framebuffer driver supend function -> call each client
> > 
> > Just give me a few days to piece it together.
> 
> Right now, we don't have a proper userland notification. So far, the
> main affected thing is XFree, but this is ok as it will have received
> a suspend request via /dev/apm_bios (which we emulate on PowerMacs),
> and so won't touch the framebuffer until resumed.
> 
> There isn't much we can do against a userland client tapping the
> framebuffer that it mmap'ed previously. I don't know how feasible it
> would be to sort of "hack" this process mapping on the fly (would
> involve some nasty SMP synchronisation issues) so that the userland
> process is just put to sleep on fb access while the fb is suspended
> (or get a SEGV). We probably want to extend the notification mecanism
> to userland in some way, but this isn't something i cover in this
> patch.

I believe solution to this is simple: always switch to kernel-owned
console during suspend. (swsusp does it, there's patch for S3 to do
the same). That way, Xfree (or qtopia or whoever) should clean up
after themselves and leave the console to the kernel. (See
kernel/power/console.c)
							Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

On Thu, 2003-08-07 at 12:03, Pavel Machek wrote:

> I believe solution to this is simple: always switch to kernel-owned
> console during suspend. (swsusp does it, there's patch for S3 to do
> the same). That way, Xfree (or qtopia or whoever) should clean up
> after themselves and leave the console to the kernel. (See
> kernel/power/console.c)

I admit I quite like this solution. It would also help displaying
something sane (blank, pattern, whatever) on screen during driver
teardown instead of the junk left by X...

I'll look into including that switch into my pmac code as well
and see if it works properly in all cases (I think so). Also,
recent DRI CVS finally has working suspend/resume (works on
console switch too).

Ben.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Benjamin Herrenschmidt]

> I believe solution to this is simple: always switch to kernel-owned
> console during suspend. (swsusp does it, there's patch for S3 to do
> the same). That way, Xfree (or qtopia or whoever) should clean up
> after themselves and leave the console to the kernel. (See
> kernel/power/console.c)

I tried using it on pmac, but it causes hell with XFree. I'm not sure
what's up yet, I suspect it may be XFree still doing things after
calling the RELDISP ioctl but I'm not completely sure yet.

The setup XFree + DRI is working without switching to suspend console
(with only the apm_bios emulation for XFree to suspend/restore itself)
but not when switching to suspend console right before doing the apm
emulation callbacks (which should be ignored by X since it's no longer
the frontmost process at this point).

For some reason, it seems that after we have switched to the suspend
console, we race with the X server on accel engine, and on resume, the X
server just crashes.

Ben.
 

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[James Simmons]

> I believe solution to this is simple: always switch to kernel-owned
> console during suspend. (swsusp does it, there's patch for S3 to do
> the same). That way, Xfree (or qtopia or whoever) should clean up
> after themselves and leave the console to the kernel. (See
> kernel/power/console.c)

Not very helpful on embedded systems that use the framebuffer without the 
VT console. 

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[James Simmons]

> > I believe solution to this is simple: always switch to kernel-owned
> > console during suspend. (swsusp does it, there's patch for S3 to do
> > the same). That way, Xfree (or qtopia or whoever) should clean up
> > after themselves and leave the console to the kernel. (See
> > kernel/power/console.c)
> 
> I admit I quite like this solution. It would also help displaying
> something sane (blank, pattern, whatever) on screen during driver
> teardown instead of the junk left by X...

There is the case of framebuffer without VT console. Of course X can't 
work except with specific patches. X shouldn't be touching the console.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[James Simmons]

> For some reason, it seems that after we have switched to the suspend
> console, we race with the X server on accel engine, and on resume, the X
> server just crashes.

Are you shutting down the accel engine in the fbdev driver on suspend?

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Pavel Machek]

Hi!

> > I believe solution to this is simple: always switch to kernel-owned
> > console during suspend. (swsusp does it, there's patch for S3 to do
> > the same). That way, Xfree (or qtopia or whoever) should clean up
> > after themselves and leave the console to the kernel. (See
> > kernel/power/console.c)
> 
> I tried using it on pmac, but it causes hell with XFree. I'm not sure
> what's up yet, I suspect it may be XFree still doing things after
> calling the RELDISP ioctl but I'm not completely sure yet.

Sounds like XFree bug to me ;-).
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Pavel Machek]

Hi!

> > I believe solution to this is simple: always switch to kernel-owned
> > console during suspend. (swsusp does it, there's patch for S3 to do
> > the same). That way, Xfree (or qtopia or whoever) should clean up
> > after themselves and leave the console to the kernel. (See
> > kernel/power/console.c)
> 
> Not very helpful on embedded systems that use the framebuffer without the 
> VT console. 

Okay, that might be a problem. But such system will need some special
notification... Is VT so much overhead?
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Nigel Cunningham]

Hi.

Would this play well with the suspend process itself displaying output?
(Eg progress, errors...).

Regards,

Nigel

On Fri, 2003-08-08 at 01:32, Benjamin Herrenschmidt wrote:
> On Thu, 2003-08-07 at 12:03, Pavel Machek wrote:
> 
> > I believe solution to this is simple: always switch to kernel-owned
> > console during suspend. (swsusp does it, there's patch for S3 to do
> > the same). That way, Xfree (or qtopia or whoever) should clean up
> > after themselves and leave the console to the kernel. (See
> > kernel/power/console.c)
> 
> I admit I quite like this solution. It would also help displaying
> something sane (blank, pattern, whatever) on screen during driver
> teardown instead of the junk left by X...
> 
> I'll look into including that switch into my pmac code as well
> and see if it works properly in all cases (I think so). Also,
> recent DRI CVS finally has working suspend/resume (works on
> console switch too).
> 
> Ben.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

You see, at just the right time, when we were still powerless,
Christ died for the ungodly.
	-- Romans 5:6, NIV.

Re: [Linux-fbdev-devel] [PATCH] Framebuffer: 2nd try: client notification mecanism & PM

[Pavel Machek]

Hi!

> Would this play well with the suspend process itself displaying output?
> (Eg progress, errors...).

Yes.
								Pavel


...
> > > I believe solution to this is simple: always switch to kernel-owned
> > > console during suspend. (swsusp does it, there's patch for S3 to do
> > > the same). That way, Xfree (or qtopia or whoever) should clean up
> > > after themselves and leave the console to the kernel. (See
> > > kernel/power/console.c)
> > 
> > I admit I quite like this solution. It would also help displaying
> > something sane (blank, pattern, whatever) on screen during driver
> > teardown instead of the junk left by X...
> > 
> > I'll look into including that switch into my pmac code as well
> > and see if it works properly in all cases (I think so). Also,
> > recent DRI CVS finally has working suspend/resume (works on
> > console switch too).
> > 
> > Ben.
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]