All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch 0/4] PS3 AV/FB updates
@ 2007-02-15 15:23 ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linuxppc-dev, linux-fbdev-devel

Here are a few updates for the PS3 AV Settings and Frame Buffer Device Drivers,
in response to recent review comments.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 0/4] PS3 AV/FB updates
@ 2007-02-15 15:23 ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linuxppc-dev, linux-fbdev-devel

Here are a few updates for the PS3 AV Settings and Frame Buffer Device Drivers,
in response to recent review comments.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 1/4] ps3fb: thread updates
  2007-02-15 15:23 ` Geert.Uytterhoeven
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

[-- Attachment #1: ps3-wip/ps3fb-kthread.diff --]
[-- Type: text/plain, Size: 2603 bytes --]

ps3fb: Replace the kernel_thread by a proper kthread.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -139,6 +141,7 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,12 +808,14 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
+	int error;
+
+	do {
+		try_to_freeze();
+		error = down_interruptible(&ps3fb.sem);
+		if (!error && !atomic_read(&ps3fb.ext_flip))
 			ps3fb_sync(0);	/* single buffer */
-	}
+	} while (!kthread_should_stop());
 	return 0;
 }
 
@@ -1050,9 +1055,17 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	ps3fb.task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(ps3fb.task)) {
+		retval = PTR_ERR(ps3fb.task);
+		ps3fb.task = NULL;
+		goto err_unregister_framebuffer;
+	}
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1096,10 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		kthread_stop(ps3fb.task);
+		ps3fb.task = NULL;
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 1/4] ps3fb: thread updates
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

ps3fb: Replace the kernel_thread by a proper kthread.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -139,6 +141,7 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,12 +808,14 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
+	int error;
+
+	do {
+		try_to_freeze();
+		error = down_interruptible(&ps3fb.sem);
+		if (!error && !atomic_read(&ps3fb.ext_flip))
 			ps3fb_sync(0);	/* single buffer */
-	}
+	} while (!kthread_should_stop());
 	return 0;
 }
 
@@ -1050,9 +1055,17 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	ps3fb.task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(ps3fb.task)) {
+		retval = PTR_ERR(ps3fb.task);
+		ps3fb.task = NULL;
+		goto err_unregister_framebuffer;
+	}
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1096,10 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		kthread_stop(ps3fb.task);
+		ps3fb.task = NULL;
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 2/4] ps3av: thread updates
  2007-02-15 15:23 ` Geert.Uytterhoeven
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

[-- Attachment #1: ps3-wip/ps3av-workqueue.diff --]
[-- Type: text/plain, Size: 3233 bytes --]

ps3av: Replace the kernel_thread and the ping pong semaphores by a singlethread
workqueue and a completion.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/ps3/ps3av.c         |   30 ++++++++++++++----------------
 include/asm-powerpc/ps3av.h |    5 +++--
 2 files changed, 17 insertions(+), 18 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av.c
@@ -438,7 +438,7 @@ static int ps3av_set_videomode(void)
 	ps3av_set_av_video_mute(PS3AV_CMD_MUTE_ON);
 
 	/* wake up ps3avd to do the actual video mode setting */
-	up(&ps3av.ping);
+	queue_work(ps3av.wq, &ps3av.work);
 
 	return 0;
 }
@@ -513,18 +513,10 @@ static void ps3av_set_videomode_cont(u32
 	ps3av_set_av_video_mute(PS3AV_CMD_MUTE_OFF);
 }
 
-static int ps3avd(void *p)
+static void ps3avd(struct work_struct *work)
 {
-	struct ps3av *info = p;
-
-	daemonize("ps3avd");
-	while (1) {
-		down(&info->ping);
-		ps3av_set_videomode_cont(info->ps3av_mode,
-					 info->ps3av_mode_old);
-		up(&info->pong);
-	}
-	return 0;
+	ps3av_set_videomode_cont(ps3av.ps3av_mode, ps3av.ps3av_mode_old);
+	complete(&ps3av.done);
 }
 
 static int ps3av_vid2table_id(int vid)
@@ -723,7 +715,7 @@ int ps3av_set_video_mode(u32 id, int boo
 	}
 
 	/* set videomode */
-	down(&ps3av.pong);
+	wait_for_completion(&ps3av.done);
 	ps3av.ps3av_mode_old = ps3av.ps3av_mode;
 	ps3av.ps3av_mode = id;
 	if (ps3av_set_videomode())
@@ -879,12 +871,16 @@ static int ps3av_probe(struct ps3_vuart_
 	memset(&ps3av, 0, sizeof(ps3av));
 
 	init_MUTEX(&ps3av.sem);
-	init_MUTEX_LOCKED(&ps3av.ping);
-	init_MUTEX(&ps3av.pong);
 	mutex_init(&ps3av.mutex);
 	ps3av.ps3av_mode = 0;
 	ps3av.dev = dev;
-	kernel_thread(ps3avd, &ps3av, CLONE_KERNEL);
+
+	INIT_WORK(&ps3av.work, ps3avd);
+	init_completion(&ps3av.done);
+	complete(&ps3av.done);
+	ps3av.wq = create_singlethread_workqueue("ps3avd");
+	if (!ps3av.wq)
+		return -ENOMEM;
 
 	ps3av.available = 1;
 	switch (ps3_os_area_get_av_multi_out()) {
@@ -924,6 +920,8 @@ static int ps3av_remove(struct ps3_vuart
 {
 	if (ps3av.available) {
 		ps3av_cmd_fin();
+		if (ps3av.wq)
+			destroy_workqueue(ps3av.wq);
 		ps3av.available = 0;
 	}
 
--- ps3-linux-2.6.20.orig/include/asm-powerpc/ps3av.h
+++ ps3-linux-2.6.20/include/asm-powerpc/ps3av.h
@@ -646,8 +646,9 @@ struct ps3av_pkt_avb_param {
 struct ps3av {
 	int available;
 	struct semaphore sem;
-	struct semaphore ping;
-	struct semaphore pong;
+	struct work_struct work;
+	struct completion done;
+	struct workqueue_struct *wq;
 	struct mutex mutex;
 	int open_count;
 	struct ps3_vuart_port_device *dev;

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 2/4] ps3av: thread updates
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

ps3av: Replace the kernel_thread and the ping pong semaphores by a singlethread
workqueue and a completion.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/ps3/ps3av.c         |   30 ++++++++++++++----------------
 include/asm-powerpc/ps3av.h |    5 +++--
 2 files changed, 17 insertions(+), 18 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av.c
@@ -438,7 +438,7 @@ static int ps3av_set_videomode(void)
 	ps3av_set_av_video_mute(PS3AV_CMD_MUTE_ON);
 
 	/* wake up ps3avd to do the actual video mode setting */
-	up(&ps3av.ping);
+	queue_work(ps3av.wq, &ps3av.work);
 
 	return 0;
 }
@@ -513,18 +513,10 @@ static void ps3av_set_videomode_cont(u32
 	ps3av_set_av_video_mute(PS3AV_CMD_MUTE_OFF);
 }
 
-static int ps3avd(void *p)
+static void ps3avd(struct work_struct *work)
 {
-	struct ps3av *info = p;
-
-	daemonize("ps3avd");
-	while (1) {
-		down(&info->ping);
-		ps3av_set_videomode_cont(info->ps3av_mode,
-					 info->ps3av_mode_old);
-		up(&info->pong);
-	}
-	return 0;
+	ps3av_set_videomode_cont(ps3av.ps3av_mode, ps3av.ps3av_mode_old);
+	complete(&ps3av.done);
 }
 
 static int ps3av_vid2table_id(int vid)
@@ -723,7 +715,7 @@ int ps3av_set_video_mode(u32 id, int boo
 	}
 
 	/* set videomode */
-	down(&ps3av.pong);
+	wait_for_completion(&ps3av.done);
 	ps3av.ps3av_mode_old = ps3av.ps3av_mode;
 	ps3av.ps3av_mode = id;
 	if (ps3av_set_videomode())
@@ -879,12 +871,16 @@ static int ps3av_probe(struct ps3_vuart_
 	memset(&ps3av, 0, sizeof(ps3av));
 
 	init_MUTEX(&ps3av.sem);
-	init_MUTEX_LOCKED(&ps3av.ping);
-	init_MUTEX(&ps3av.pong);
 	mutex_init(&ps3av.mutex);
 	ps3av.ps3av_mode = 0;
 	ps3av.dev = dev;
-	kernel_thread(ps3avd, &ps3av, CLONE_KERNEL);
+
+	INIT_WORK(&ps3av.work, ps3avd);
+	init_completion(&ps3av.done);
+	complete(&ps3av.done);
+	ps3av.wq = create_singlethread_workqueue("ps3avd");
+	if (!ps3av.wq)
+		return -ENOMEM;
 
 	ps3av.available = 1;
 	switch (ps3_os_area_get_av_multi_out()) {
@@ -924,6 +920,8 @@ static int ps3av_remove(struct ps3_vuart
 {
 	if (ps3av.available) {
 		ps3av_cmd_fin();
+		if (ps3av.wq)
+			destroy_workqueue(ps3av.wq);
 		ps3av.available = 0;
 	}
 
--- ps3-linux-2.6.20.orig/include/asm-powerpc/ps3av.h
+++ ps3-linux-2.6.20/include/asm-powerpc/ps3av.h
@@ -646,8 +646,9 @@ struct ps3av_pkt_avb_param {
 struct ps3av {
 	int available;
 	struct semaphore sem;
-	struct semaphore ping;
-	struct semaphore pong;
+	struct work_struct work;
+	struct completion done;
+	struct workqueue_struct *wq;
 	struct mutex mutex;
 	int open_count;
 	struct ps3_vuart_port_device *dev;

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 3/4] ps3fb: kill superfluous zero initializations
  2007-02-15 15:23 ` Geert.Uytterhoeven
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

[-- Attachment #1: ps3-wip/ps3fb-zero-init.diff --]
[-- Type: text/plain, Size: 1337 bytes --]

ps3fb: kill superfluous zero initializations

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -297,10 +297,10 @@ static const struct fb_videomode ps3fb_m
 #define VP_OFF(i)	(WIDTH(i) * Y_OFF(i) * BPP + X_OFF(i) * BPP)
 #define FB_OFF(i)	(GPU_OFFSET - VP_OFF(i) % GPU_OFFSET)
 
-static int ps3fb_mode = 0;
+static int ps3fb_mode;
 module_param(ps3fb_mode, bool, 0);
 
-static char *mode_option __initdata = NULL;
+static char *mode_option __initdata;
 
 
 static int ps3fb_get_res_table(u32 xres, u32 yres)

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 3/4] ps3fb: kill superfluous zero initializations
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

ps3fb: kill superfluous zero initializations

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -297,10 +297,10 @@ static const struct fb_videomode ps3fb_m
 #define VP_OFF(i)	(WIDTH(i) * Y_OFF(i) * BPP + X_OFF(i) * BPP)
 #define FB_OFF(i)	(GPU_OFFSET - VP_OFF(i) % GPU_OFFSET)
 
-static int ps3fb_mode = 0;
+static int ps3fb_mode;
 module_param(ps3fb_mode, bool, 0);
 
-static char *mode_option __initdata = NULL;
+static char *mode_option __initdata;
 
 
 static int ps3fb_get_res_table(u32 xres, u32 yres)

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 4/4] ps3av: misc updates
  2007-02-15 15:23 ` Geert.Uytterhoeven
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

[-- Attachment #1: ps3-wip/ps3av-misc.diff --]
[-- Type: text/plain, Size: 4204 bytes --]

ps3av:
  - Move the definition of struct ps3av to ps3av.c, as it's used locally only.
  - Kill ps3av.sem, use the existing ps3av.mutex instead.
  - Make the 512-byte buffer in ps3av_do_pkt() static to reduce stack usage.
    Its use is protected by a semaphore anyway.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/ps3/ps3av.c         |   29 ++++++++++++++++++++++-------
 include/asm-powerpc/ps3av.h |   22 +---------------------
 2 files changed, 23 insertions(+), 28 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av.c
@@ -36,7 +36,24 @@
 static int timeout = 5000;	/* in msec ( 5 sec ) */
 module_param(timeout, int, 0644);
 
-static struct ps3av ps3av;
+static struct ps3av {
+	int available;
+	struct mutex mutex;
+	struct work_struct work;
+	struct completion done;
+	struct workqueue_struct *wq;
+	int open_count;
+	struct ps3_vuart_port_device *dev;
+
+	int region;
+	struct ps3av_pkt_av_get_hw_conf av_hw_conf;
+	u32 av_port[PS3AV_AV_PORT_MAX + PS3AV_OPT_PORT_MAX];
+	u32 opt_port[PS3AV_OPT_PORT_MAX];
+	u32 head[PS3AV_HEAD_MAX];
+	u32 audio_port;
+	int ps3av_mode;
+	int ps3av_mode_old;
+} ps3av;
 
 static struct ps3_vuart_port_device ps3av_dev = {
 	.match_id = PS3_MATCH_ID_AV_SETTINGS
@@ -248,7 +265,7 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 		 struct ps3av_send_hdr *buf)
 {
 	int res = 0;
-	union {
+	static union {
 		struct ps3av_reply_hdr reply_hdr;
 		u8 raw[PS3AV_BUF_SIZE];
 	} recv_buf;
@@ -257,8 +274,7 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 
 	BUG_ON(!ps3av.available);
 
-	if (down_interruptible(&ps3av.sem))
-		return -ERESTARTSYS;
+	mutex_lock(&ps3av.mutex);
 
 	table = ps3av_search_cmd_table(cid, PS3AV_CID_MASK);
 	BUG_ON(!table);
@@ -288,11 +304,11 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 		goto err;
 	}
 
-	up(&ps3av.sem);
+	mutex_unlock(&ps3av.mutex);
 	return 0;
 
       err:
-	up(&ps3av.sem);
+	mutex_unlock(&ps3av.mutex);
 	printk(KERN_ERR "%s: failed cid:%x res:%d\n", __FUNCTION__, cid, res);
 	return res;
 }
@@ -870,7 +886,6 @@ static int ps3av_probe(struct ps3_vuart_
 
 	memset(&ps3av, 0, sizeof(ps3av));
 
-	init_MUTEX(&ps3av.sem);
 	mutex_init(&ps3av.mutex);
 	ps3av.ps3av_mode = 0;
 	ps3av.dev = dev;
--- ps3-linux-2.6.20.orig/include/asm-powerpc/ps3av.h
+++ ps3-linux-2.6.20/include/asm-powerpc/ps3av.h
@@ -18,8 +18,6 @@
 #ifndef _ASM_POWERPC_PS3AV_H_
 #define _ASM_POWERPC_PS3AV_H_
 
-#include <linux/mutex.h>
-
 /** command for ioctl() **/
 #define PS3AV_VERSION 0x205	/* version of ps3av command */
 
@@ -643,25 +641,6 @@ struct ps3av_pkt_avb_param {
 	u8 buf[PS3AV_PKT_AVB_PARAM_MAX_BUF_SIZE];
 };
 
-struct ps3av {
-	int available;
-	struct semaphore sem;
-	struct work_struct work;
-	struct completion done;
-	struct workqueue_struct *wq;
-	struct mutex mutex;
-	int open_count;
-	struct ps3_vuart_port_device *dev;
-
-	int region;
-	struct ps3av_pkt_av_get_hw_conf av_hw_conf;
-	u32 av_port[PS3AV_AV_PORT_MAX + PS3AV_OPT_PORT_MAX];
-	u32 opt_port[PS3AV_OPT_PORT_MAX];
-	u32 head[PS3AV_HEAD_MAX];
-	u32 audio_port;
-	int ps3av_mode;
-	int ps3av_mode_old;
-};
 
 /** command status **/
 #define PS3AV_STATUS_SUCCESS			0x0000	/* success */
@@ -719,6 +698,7 @@ static inline void ps3av_cmd_av_monitor_
 extern int ps3av_cmd_video_get_monitor_info(struct ps3av_pkt_av_get_monitor_info *,
 					    u32);
 
+struct ps3_vuart_port_device;
 extern int ps3av_vuart_write(struct ps3_vuart_port_device *dev,
 			     const void *buf, unsigned long size);
 extern int ps3av_vuart_read(struct ps3_vuart_port_device *dev, void *buf,

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [patch 4/4] ps3av: misc updates
@ 2007-02-15 15:23   ` Geert.Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert.Uytterhoeven @ 2007-02-15 15:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert Uytterhoeven, linuxppc-dev, linux-fbdev-devel

ps3av:
  - Move the definition of struct ps3av to ps3av.c, as it's used locally only.
  - Kill ps3av.sem, use the existing ps3av.mutex instead.
  - Make the 512-byte buffer in ps3av_do_pkt() static to reduce stack usage.
    Its use is protected by a semaphore anyway.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/ps3/ps3av.c         |   29 ++++++++++++++++++++++-------
 include/asm-powerpc/ps3av.h |   22 +---------------------
 2 files changed, 23 insertions(+), 28 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av.c
@@ -36,7 +36,24 @@
 static int timeout = 5000;	/* in msec ( 5 sec ) */
 module_param(timeout, int, 0644);
 
-static struct ps3av ps3av;
+static struct ps3av {
+	int available;
+	struct mutex mutex;
+	struct work_struct work;
+	struct completion done;
+	struct workqueue_struct *wq;
+	int open_count;
+	struct ps3_vuart_port_device *dev;
+
+	int region;
+	struct ps3av_pkt_av_get_hw_conf av_hw_conf;
+	u32 av_port[PS3AV_AV_PORT_MAX + PS3AV_OPT_PORT_MAX];
+	u32 opt_port[PS3AV_OPT_PORT_MAX];
+	u32 head[PS3AV_HEAD_MAX];
+	u32 audio_port;
+	int ps3av_mode;
+	int ps3av_mode_old;
+} ps3av;
 
 static struct ps3_vuart_port_device ps3av_dev = {
 	.match_id = PS3_MATCH_ID_AV_SETTINGS
@@ -248,7 +265,7 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 		 struct ps3av_send_hdr *buf)
 {
 	int res = 0;
-	union {
+	static union {
 		struct ps3av_reply_hdr reply_hdr;
 		u8 raw[PS3AV_BUF_SIZE];
 	} recv_buf;
@@ -257,8 +274,7 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 
 	BUG_ON(!ps3av.available);
 
-	if (down_interruptible(&ps3av.sem))
-		return -ERESTARTSYS;
+	mutex_lock(&ps3av.mutex);
 
 	table = ps3av_search_cmd_table(cid, PS3AV_CID_MASK);
 	BUG_ON(!table);
@@ -288,11 +304,11 @@ int ps3av_do_pkt(u32 cid, u16 send_len, 
 		goto err;
 	}
 
-	up(&ps3av.sem);
+	mutex_unlock(&ps3av.mutex);
 	return 0;
 
       err:
-	up(&ps3av.sem);
+	mutex_unlock(&ps3av.mutex);
 	printk(KERN_ERR "%s: failed cid:%x res:%d\n", __FUNCTION__, cid, res);
 	return res;
 }
@@ -870,7 +886,6 @@ static int ps3av_probe(struct ps3_vuart_
 
 	memset(&ps3av, 0, sizeof(ps3av));
 
-	init_MUTEX(&ps3av.sem);
 	mutex_init(&ps3av.mutex);
 	ps3av.ps3av_mode = 0;
 	ps3av.dev = dev;
--- ps3-linux-2.6.20.orig/include/asm-powerpc/ps3av.h
+++ ps3-linux-2.6.20/include/asm-powerpc/ps3av.h
@@ -18,8 +18,6 @@
 #ifndef _ASM_POWERPC_PS3AV_H_
 #define _ASM_POWERPC_PS3AV_H_
 
-#include <linux/mutex.h>
-
 /** command for ioctl() **/
 #define PS3AV_VERSION 0x205	/* version of ps3av command */
 
@@ -643,25 +641,6 @@ struct ps3av_pkt_avb_param {
 	u8 buf[PS3AV_PKT_AVB_PARAM_MAX_BUF_SIZE];
 };
 
-struct ps3av {
-	int available;
-	struct semaphore sem;
-	struct work_struct work;
-	struct completion done;
-	struct workqueue_struct *wq;
-	struct mutex mutex;
-	int open_count;
-	struct ps3_vuart_port_device *dev;
-
-	int region;
-	struct ps3av_pkt_av_get_hw_conf av_hw_conf;
-	u32 av_port[PS3AV_AV_PORT_MAX + PS3AV_OPT_PORT_MAX];
-	u32 opt_port[PS3AV_OPT_PORT_MAX];
-	u32 head[PS3AV_HEAD_MAX];
-	u32 audio_port;
-	int ps3av_mode;
-	int ps3av_mode_old;
-};
 
 /** command status **/
 #define PS3AV_STATUS_SUCCESS			0x0000	/* success */
@@ -719,6 +698,7 @@ static inline void ps3av_cmd_av_monitor_
 extern int ps3av_cmd_video_get_monitor_info(struct ps3av_pkt_av_get_monitor_info *,
 					    u32);
 
+struct ps3_vuart_port_device;
 extern int ps3av_vuart_write(struct ps3_vuart_port_device *dev,
 			     const void *buf, unsigned long size);
 extern int ps3av_vuart_read(struct ps3_vuart_port_device *dev, void *buf,

--
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 0/4] PS3 AV/FB updates
  2007-02-15 15:23 ` Geert.Uytterhoeven
@ 2007-02-15 15:48   ` James Simmons
  -1 siblings, 0 replies; 39+ messages in thread
From: James Simmons @ 2007-02-15 15:48 UTC (permalink / raw)
  To: linux-fbdev-devel; +Cc: linuxppc-dev, Andrew Morton



Andrew please apply.

Acked-By: James Simmons <jsimmons@infradead.org>


On Thu, 15 Feb 2007, Geert.Uytterhoeven@sonycom.com wrote:

> Here are a few updates for the PS3 AV Settings and Frame Buffer Device Drivers,
> in response to recent review comments.
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
> Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
> Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
> 
> 
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys-and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Linux-fbdev-devel mailing list
> Linux-fbdev-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel
> 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Linux-fbdev-devel] [patch 0/4] PS3 AV/FB updates
@ 2007-02-15 15:48   ` James Simmons
  0 siblings, 0 replies; 39+ messages in thread
From: James Simmons @ 2007-02-15 15:48 UTC (permalink / raw)
  To: linux-fbdev-devel; +Cc: linuxppc-dev, Andrew Morton



Andrew please apply.

Acked-By: James Simmons <jsimmons@infradead.org>


On Thu, 15 Feb 2007, Geert.Uytterhoeven@sonycom.com wrote:

> Here are a few updates for the PS3 AV Settings and Frame Buffer Device Drivers,
> in response to recent review comments.
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
> Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
> Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
> 
> 
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys-and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Linux-fbdev-devel mailing list
> Linux-fbdev-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
  2007-02-15 15:23   ` Geert.Uytterhoeven
@ 2007-02-15 17:50     ` Christoph Hellwig
  -1 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-15 17:50 UTC (permalink / raw)
  To: Geert.Uytterhoeven; +Cc: linuxppc-dev, Andrew Morton, linux-fbdev-devel

On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> +	do {
> +		try_to_freeze();
> +		error = down_interruptible(&ps3fb.sem);
> +		if (!error && !atomic_read(&ps3fb.ext_flip))
>  			ps3fb_sync(0);	/* single buffer */

this still can deadlock when calling kthread_stop.  You really want
to use wake_up_process to kick this thread or use a workqueue.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
@ 2007-02-15 17:50     ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-15 17:50 UTC (permalink / raw)
  To: Geert.Uytterhoeven; +Cc: linuxppc-dev, Andrew Morton, linux-fbdev-devel

On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> +	do {
> +		try_to_freeze();
> +		error = down_interruptible(&ps3fb.sem);
> +		if (!error && !atomic_read(&ps3fb.ext_flip))
>  			ps3fb_sync(0);	/* single buffer */

this still can deadlock when calling kthread_stop.  You really want
to use wake_up_process to kick this thread or use a workqueue.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
  2007-02-15 17:50     ` Christoph Hellwig
  (?)
@ 2007-02-15 21:43     ` Benjamin Herrenschmidt
  2007-02-16  0:59       ` Andrew Morton
  -1 siblings, 1 reply; 39+ messages in thread
From: Benjamin Herrenschmidt @ 2007-02-15 21:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Geert.Uytterhoeven, linuxppc-dev, Andrew Morton, linux-fbdev-devel

On Thu, 2007-02-15 at 18:50 +0100, Christoph Hellwig wrote:
> On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> > +	do {
> > +		try_to_freeze();
> > +		error = down_interruptible(&ps3fb.sem);
> > +		if (!error && !atomic_read(&ps3fb.ext_flip))
> >  			ps3fb_sync(0);	/* single buffer */
> 
> this still can deadlock when calling kthread_stop.  You really want
> to use wake_up_process to kick this thread or use a workqueue.

kthread_stop does wake_up_process no ? However, that might not get you
out of interruptible if you don't also have signal_pending...

Ben.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
  2007-02-15 21:43     ` Benjamin Herrenschmidt
@ 2007-02-16  0:59       ` Andrew Morton
  2007-02-16 15:58           ` Geert Uytterhoeven
                           ` (3 more replies)
  0 siblings, 4 replies; 39+ messages in thread
From: Andrew Morton @ 2007-02-16  0:59 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Geert.Uytterhoeven, linuxppc-dev, linux-fbdev-devel

On Fri, 16 Feb 2007 08:43:37 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2007-02-15 at 18:50 +0100, Christoph Hellwig wrote:
> > On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> > > +	do {
> > > +		try_to_freeze();
> > > +		error = down_interruptible(&ps3fb.sem);
> > > +		if (!error && !atomic_read(&ps3fb.ext_flip))
> > >  			ps3fb_sync(0);	/* single buffer */
> > 
> > this still can deadlock when calling kthread_stop.  You really want
> > to use wake_up_process to kick this thread or use a workqueue.
> 
> kthread_stop does wake_up_process no ? However, that might not get you
> out of interruptible if you don't also have signal_pending...
> 

No, it won't get you out of down_interruptible().  But the code would have
failed trivial testing so perhaps we're missing something.

It seems crufty to use semaphores in this manner.  afaict all we're doing
here is poking a kernel thread and asking it to do a bit of work.  The
standard way of doing this is to go to sleep on a waitqueue_head.

	DEFINE_WAIT(wait);

	while (!kthread_should_stop()) {
		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
		if (!atomic_read(&ps3fb.ext_flip))
			schedule();
		finish_wait(&wq, &wait);
		if (!atomic_read(&ps3fb.ext_flip))
			WARN_ON(1);
		else
			ps3fb_sync(0);
	}

and, elsewhere,

	atomic_inc(&ps3fb.ext_flip);
	wake_up_process(my_kernel_therad);

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
  2007-02-16  0:59       ` Andrew Morton
@ 2007-02-16 15:58           ` Geert Uytterhoeven
  2007-02-16 16:00           ` Geert Uytterhoeven
                             ` (2 subsequent siblings)
  3 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 15:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Benjamin Herrenschmidt, linux-fbdev-devel, Christoph Hellwig,
	linuxppc-dev

On Thu, 15 Feb 2007, Andrew Morton wrote:
> On Fri, 16 Feb 2007 08:43:37 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > On Thu, 2007-02-15 at 18:50 +0100, Christoph Hellwig wrote:
> > > On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> > > > +	do {
> > > > +		try_to_freeze();
> > > > +		error = down_interruptible(&ps3fb.sem);
> > > > +		if (!error && !atomic_read(&ps3fb.ext_flip))
> > > >  			ps3fb_sync(0);	/* single buffer */
> > > 
> > > this still can deadlock when calling kthread_stop.  You really want
> > > to use wake_up_process to kick this thread or use a workqueue.
> > 
> > kthread_stop does wake_up_process no ? However, that might not get you
> > out of interruptible if you don't also have signal_pending...
> 
> No, it won't get you out of down_interruptible().  But the code would have
> failed trivial testing so perhaps we're missing something.

As modular ps3fb is not yet supported, this trivial testing is not that
trivial...

> It seems crufty to use semaphores in this manner.  afaict all we're doing
> here is poking a kernel thread and asking it to do a bit of work.  The
> standard way of doing this is to go to sleep on a waitqueue_head.
> 
> 	DEFINE_WAIT(wait);
> 
> 	while (!kthread_should_stop()) {
> 		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> 		if (!atomic_read(&ps3fb.ext_flip))
> 			schedule();
> 		finish_wait(&wq, &wait);
> 		if (!atomic_read(&ps3fb.ext_flip))
> 			WARN_ON(1);
> 		else
> 			ps3fb_sync(0);
> 	}
> 
> and, elsewhere,
> 
> 	atomic_inc(&ps3fb.ext_flip);
> 	wake_up_process(my_kernel_therad);

Thanks! We don't wait on ext_flip, though, but I see your point. Updated patch
will follow.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
@ 2007-02-16 15:58           ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 15:58 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-fbdev-devel, linuxppc-dev

On Thu, 15 Feb 2007, Andrew Morton wrote:
> On Fri, 16 Feb 2007 08:43:37 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > On Thu, 2007-02-15 at 18:50 +0100, Christoph Hellwig wrote:
> > > On Thu, Feb 15, 2007 at 04:23:02PM +0100, Geert.Uytterhoeven@sonycom.com wrote:
> > > > +	do {
> > > > +		try_to_freeze();
> > > > +		error = down_interruptible(&ps3fb.sem);
> > > > +		if (!error && !atomic_read(&ps3fb.ext_flip))
> > > >  			ps3fb_sync(0);	/* single buffer */
> > > 
> > > this still can deadlock when calling kthread_stop.  You really want
> > > to use wake_up_process to kick this thread or use a workqueue.
> > 
> > kthread_stop does wake_up_process no ? However, that might not get you
> > out of interruptible if you don't also have signal_pending...
> 
> No, it won't get you out of down_interruptible().  But the code would have
> failed trivial testing so perhaps we're missing something.

As modular ps3fb is not yet supported, this trivial testing is not that
trivial...

> It seems crufty to use semaphores in this manner.  afaict all we're doing
> here is poking a kernel thread and asking it to do a bit of work.  The
> standard way of doing this is to go to sleep on a waitqueue_head.
> 
> 	DEFINE_WAIT(wait);
> 
> 	while (!kthread_should_stop()) {
> 		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> 		if (!atomic_read(&ps3fb.ext_flip))
> 			schedule();
> 		finish_wait(&wq, &wait);
> 		if (!atomic_read(&ps3fb.ext_flip))
> 			WARN_ON(1);
> 		else
> 			ps3fb_sync(0);
> 	}
> 
> and, elsewhere,
> 
> 	atomic_inc(&ps3fb.ext_flip);
> 	wake_up_process(my_kernel_therad);

Thanks! We don't wait on ext_flip, though, but I see your point. Updated patch
will follow.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4 (updated)] ps3fb: thread updates
  2007-02-16  0:59       ` Andrew Morton
@ 2007-02-16 16:00           ` Geert Uytterhoeven
  2007-02-16 16:00           ` Geert Uytterhoeven
                             ` (2 subsequent siblings)
  3 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 16:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Benjamin Herrenschmidt, linux-fbdev-devel, Christoph Hellwig,
	linuxppc-dev

ps3fb: Replace the kernel_thread by a proper kthread, which sleeps on a
waitqueue_head.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   45 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 35 insertions(+), 10 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -129,7 +131,6 @@ struct ps3fb_priv {
 	u64 context_handle, memory_handle;
 	void *xdr_ea;
 	struct gpu_driver_info *dinfo;
-	struct semaphore sem;
 	u32 res_index;
 
 	u64 vblank_count;	/* frame count */
@@ -139,6 +140,8 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	int is_kicked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,11 +808,16 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
-			ps3fb_sync(0);	/* single buffer */
+	DEFINE_WAIT(wait);
+	DECLARE_WAIT_QUEUE_HEAD(wq);
+
+	while (!kthread_should_stop()) {
+		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
+		if (!ps3fb.is_kicked)
+			schedule();
+		finish_wait(&wq, &wait);
+		ps3fb.is_kicked = 0;
+		ps3fb_sync(0);	/* single buffer */
 	}
 	return 0;
 }
@@ -830,8 +838,11 @@ static irqreturn_t ps3fb_vsync_interrupt
 	if (v1 & (1 << GPU_INTR_STATUS_VSYNC_1)) {
 		/* VSYNC */
 		ps3fb.vblank_count = head->vblank_count;
-		if (!ps3fb.is_blanked)
-			up(&ps3fb.sem);
+		if (ps3fb.task && !ps3fb.is_blanked &&
+		    !atomic_read(&ps3fb.ext_flip)) {
+			ps3fb.is_kicked = 1;
+			wake_up_process(ps3fb.task);
+		}
 		wake_up_interruptible(&ps3fb.wait_vsync);
 	}
 
@@ -968,6 +979,7 @@ static int __init ps3fb_probe(struct pla
 	u64 xdr_lpar;
 	int status;
 	unsigned long offset;
+	struct task_struct *task;
 
 	/* get gpu context handle */
 	status = lv1_gpu_memory_allocate(DDR_SIZE, 0, 0, 0, 0,
@@ -1050,9 +1062,18 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(task)) {
+		retval = PTR_ERR(task);
+		goto err_unregister_framebuffer;
+	}
+
+	ps3fb.task = task;
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1104,11 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		struct task_struct *task = ps3fb.task;
+		ps3fb.task = NULL;
+		kthread_stop(task);
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);
@@ -1195,7 +1221,6 @@ static int __init ps3fb_init(void)
 
 	atomic_set(&ps3fb.f_count, -1);	/* fbcon opens ps3fb */
 	atomic_set(&ps3fb.ext_flip, 0);	/* for flip with vsync */
-	init_MUTEX(&ps3fb.sem);
 	init_waitqueue_head(&ps3fb.wait_vsync);
 	ps3fb.num_frames = 1;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4 (updated)] ps3fb: thread updates
@ 2007-02-16 16:00           ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 16:00 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-fbdev-devel, linuxppc-dev

ps3fb: Replace the kernel_thread by a proper kthread, which sleeps on a
waitqueue_head.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   45 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 35 insertions(+), 10 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -129,7 +131,6 @@ struct ps3fb_priv {
 	u64 context_handle, memory_handle;
 	void *xdr_ea;
 	struct gpu_driver_info *dinfo;
-	struct semaphore sem;
 	u32 res_index;
 
 	u64 vblank_count;	/* frame count */
@@ -139,6 +140,8 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	int is_kicked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,11 +808,16 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
-			ps3fb_sync(0);	/* single buffer */
+	DEFINE_WAIT(wait);
+	DECLARE_WAIT_QUEUE_HEAD(wq);
+
+	while (!kthread_should_stop()) {
+		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
+		if (!ps3fb.is_kicked)
+			schedule();
+		finish_wait(&wq, &wait);
+		ps3fb.is_kicked = 0;
+		ps3fb_sync(0);	/* single buffer */
 	}
 	return 0;
 }
@@ -830,8 +838,11 @@ static irqreturn_t ps3fb_vsync_interrupt
 	if (v1 & (1 << GPU_INTR_STATUS_VSYNC_1)) {
 		/* VSYNC */
 		ps3fb.vblank_count = head->vblank_count;
-		if (!ps3fb.is_blanked)
-			up(&ps3fb.sem);
+		if (ps3fb.task && !ps3fb.is_blanked &&
+		    !atomic_read(&ps3fb.ext_flip)) {
+			ps3fb.is_kicked = 1;
+			wake_up_process(ps3fb.task);
+		}
 		wake_up_interruptible(&ps3fb.wait_vsync);
 	}
 
@@ -968,6 +979,7 @@ static int __init ps3fb_probe(struct pla
 	u64 xdr_lpar;
 	int status;
 	unsigned long offset;
+	struct task_struct *task;
 
 	/* get gpu context handle */
 	status = lv1_gpu_memory_allocate(DDR_SIZE, 0, 0, 0, 0,
@@ -1050,9 +1062,18 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(task)) {
+		retval = PTR_ERR(task);
+		goto err_unregister_framebuffer;
+	}
+
+	ps3fb.task = task;
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1104,11 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		struct task_struct *task = ps3fb.task;
+		ps3fb.task = NULL;
+		kthread_stop(task);
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);
@@ -1195,7 +1221,6 @@ static int __init ps3fb_init(void)
 
 	atomic_set(&ps3fb.f_count, -1);	/* fbcon opens ps3fb */
 	atomic_set(&ps3fb.ext_flip, 0);	/* for flip with vsync */
-	init_MUTEX(&ps3fb.sem);
 	init_waitqueue_head(&ps3fb.wait_vsync);
 	ps3fb.num_frames = 1;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH extra] ps3fb: atomic fixes
  2007-02-16  0:59       ` Andrew Morton
@ 2007-02-16 16:03           ` Geert Uytterhoeven
  2007-02-16 16:00           ` Geert Uytterhoeven
                             ` (2 subsequent siblings)
  3 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 16:03 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Benjamin Herrenschmidt, linux-fbdev-devel, Christoph Hellwig,
	linuxppc-dev

ps3fb: Use atomic_dec_if_positive() instead of bogus atomic_read()/atomic_dec()
combinations

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -680,13 +680,10 @@ EXPORT_SYMBOL_GPL(ps3fb_wait_for_vsync);
 
 void ps3fb_flip_ctl(int on)
 {
-	if (on) {
-		if (atomic_read(&ps3fb.ext_flip) > 0) {
-			atomic_dec(&ps3fb.ext_flip);
-		}
-	} else {
+	if (on)
+		atomic_dec_if_positive(&ps3fb.ext_flip);
+	else
 		atomic_inc(&ps3fb.ext_flip);
-	}
 }
 
 EXPORT_SYMBOL_GPL(ps3fb_flip_ctl);
@@ -786,8 +783,7 @@ static int ps3fb_ioctl(struct fb_info *i
 
 	case PS3FB_IOCTL_OFF:
 		DPRINTK("PS3FB_IOCTL_OFF:\n");
-		if (atomic_read(&ps3fb.ext_flip) > 0)
-			atomic_dec(&ps3fb.ext_flip);
+		atomic_dec_if_positive(&ps3fb.ext_flip);
 		retval = 0;
 		break;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH extra] ps3fb: atomic fixes
@ 2007-02-16 16:03           ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 16:03 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-fbdev-devel, linuxppc-dev

ps3fb: Use atomic_dec_if_positive() instead of bogus atomic_read()/atomic_dec()
combinations

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -680,13 +680,10 @@ EXPORT_SYMBOL_GPL(ps3fb_wait_for_vsync);
 
 void ps3fb_flip_ctl(int on)
 {
-	if (on) {
-		if (atomic_read(&ps3fb.ext_flip) > 0) {
-			atomic_dec(&ps3fb.ext_flip);
-		}
-	} else {
+	if (on)
+		atomic_dec_if_positive(&ps3fb.ext_flip);
+	else
 		atomic_inc(&ps3fb.ext_flip);
-	}
 }
 
 EXPORT_SYMBOL_GPL(ps3fb_flip_ctl);
@@ -786,8 +783,7 @@ static int ps3fb_ioctl(struct fb_info *i
 
 	case PS3FB_IOCTL_OFF:
 		DPRINTK("PS3FB_IOCTL_OFF:\n");
-		if (atomic_read(&ps3fb.ext_flip) > 0)
-			atomic_dec(&ps3fb.ext_flip);
+		atomic_dec_if_positive(&ps3fb.ext_flip);
 		retval = 0;
 		break;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
  2007-02-16  0:59       ` Andrew Morton
@ 2007-02-16 16:36           ` Christoph Hellwig
  2007-02-16 16:00           ` Geert Uytterhoeven
                             ` (2 subsequent siblings)
  3 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-16 16:36 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Geert.Uytterhoeven, Benjamin Herrenschmidt, linux-fbdev-devel,
	Christoph Hellwig, linuxppc-dev

On Thu, Feb 15, 2007 at 04:59:16PM -0800, Andrew Morton wrote:
> No, it won't get you out of down_interruptible().  But the code would have
> failed trivial testing so perhaps we're missing something.
> 
> It seems crufty to use semaphores in this manner.  afaict all we're doing
> here is poking a kernel thread and asking it to do a bit of work.  The
> standard way of doing this is to go to sleep on a waitqueue_head.

We don't even need the waitqueue.  Because it's just a single thread
waiting we can simply use wake_up_process.  (.. which you actually
used in the second half of the example, humm..)


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [patch 1/4] ps3fb: thread updates
@ 2007-02-16 16:36           ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-16 16:36 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Geert.Uytterhoeven, linux-fbdev-devel, linuxppc-dev

On Thu, Feb 15, 2007 at 04:59:16PM -0800, Andrew Morton wrote:
> No, it won't get you out of down_interruptible().  But the code would have
> failed trivial testing so perhaps we're missing something.
> 
> It seems crufty to use semaphores in this manner.  afaict all we're doing
> here is poking a kernel thread and asking it to do a bit of work.  The
> standard way of doing this is to go to sleep on a waitqueue_head.

We don't even need the waitqueue.  Because it's just a single thread
waiting we can simply use wake_up_process.  (.. which you actually
used in the second half of the example, humm..)

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (updated)] ps3fb: thread updates
  2007-02-16 16:00           ` Geert Uytterhoeven
@ 2007-02-16 16:38             ` Christoph Hellwig
  -1 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-16 16:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Benjamin Herrenschmidt, Andrew Morton, linux-fbdev-devel,
	Christoph Hellwig, linuxppc-dev

> +	DEFINE_WAIT(wait);
> +	DECLARE_WAIT_QUEUE_HEAD(wq);
> +
> +	while (!kthread_should_stop()) {
> +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> +		if (!ps3fb.is_kicked)
> +			schedule();
> +		finish_wait(&wq, &wait);
> +		ps3fb.is_kicked = 0;
> +		ps3fb_sync(0);	/* single buffer */

should probably be just:

	while (!kthread_should_stop()) {
		ps3fb_sync(0);
		schedule();
	}

given that you don't need a waitqueue and a spurious wakeup here
seems harmless.
		

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (updated)] ps3fb: thread updates
@ 2007-02-16 16:38             ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2007-02-16 16:38 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Andrew Morton, linux-fbdev-devel, linuxppc-dev

> +	DEFINE_WAIT(wait);
> +	DECLARE_WAIT_QUEUE_HEAD(wq);
> +
> +	while (!kthread_should_stop()) {
> +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> +		if (!ps3fb.is_kicked)
> +			schedule();
> +		finish_wait(&wq, &wait);
> +		ps3fb.is_kicked = 0;
> +		ps3fb_sync(0);	/* single buffer */

should probably be just:

	while (!kthread_should_stop()) {
		ps3fb_sync(0);
		schedule();
	}

given that you don't need a waitqueue and a spurious wakeup here
seems harmless.
		

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (updated)] ps3fb: thread updates
  2007-02-16 16:38             ` Christoph Hellwig
@ 2007-02-16 17:33               ` Geert Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 17:33 UTC (permalink / raw)
  To: Linux Frame Buffer Device Development
  Cc: Benjamin Herrenschmidt, Andrew Morton, Christoph Hellwig,
	Linux/PPC Development

On Fri, 16 Feb 2007, Christoph Hellwig wrote:
> > +	DEFINE_WAIT(wait);
> > +	DECLARE_WAIT_QUEUE_HEAD(wq);
> > +
> > +	while (!kthread_should_stop()) {
> > +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> > +		if (!ps3fb.is_kicked)
> > +			schedule();
> > +		finish_wait(&wq, &wait);
> > +		ps3fb.is_kicked = 0;
> > +		ps3fb_sync(0);	/* single buffer */
> 
> should probably be just:
> 
> 	while (!kthread_should_stop()) {
> 		ps3fb_sync(0);
> 		schedule();
> 	}
> 
> given that you don't need a waitqueue and a spurious wakeup here
> seems harmless.

Not always. If flipping is disabled, or external flip is enabled, you don't
want a spurious flip.

I'll send a new patch after the weekend.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Linux-fbdev-devel] [PATCH 1/4 (updated)] ps3fb: thread updates
@ 2007-02-16 17:33               ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-16 17:33 UTC (permalink / raw)
  To: Linux Frame Buffer Device Development
  Cc: Andrew Morton, Linux/PPC Development

On Fri, 16 Feb 2007, Christoph Hellwig wrote:
> > +	DEFINE_WAIT(wait);
> > +	DECLARE_WAIT_QUEUE_HEAD(wq);
> > +
> > +	while (!kthread_should_stop()) {
> > +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> > +		if (!ps3fb.is_kicked)
> > +			schedule();
> > +		finish_wait(&wq, &wait);
> > +		ps3fb.is_kicked = 0;
> > +		ps3fb_sync(0);	/* single buffer */
> 
> should probably be just:
> 
> 	while (!kthread_should_stop()) {
> 		ps3fb_sync(0);
> 		schedule();
> 	}
> 
> given that you don't need a waitqueue and a spurious wakeup here
> seems harmless.

Not always. If flipping is disabled, or external flip is enabled, you don't
want a spurious flip.

I'll send a new patch after the weekend.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Linux-fbdev-devel] [PATCH 1/4 (updated)] ps3fb: thread updates
  2007-02-16 17:33               ` [Linux-fbdev-devel] " Geert Uytterhoeven
  (?)
@ 2007-02-19 14:07               ` Geert Uytterhoeven
  2007-02-20 10:33                 ` Geert Uytterhoeven
  -1 siblings, 1 reply; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-19 14:07 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Andrew Morton, Linux Frame Buffer Device Development,
	Linux/PPC Development

On Fri, 16 Feb 2007, Geert Uytterhoeven wrote:
> On Fri, 16 Feb 2007, Christoph Hellwig wrote:
> > > +	DEFINE_WAIT(wait);
> > > +	DECLARE_WAIT_QUEUE_HEAD(wq);
> > > +
> > > +	while (!kthread_should_stop()) {
> > > +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> > > +		if (!ps3fb.is_kicked)
> > > +			schedule();
> > > +		finish_wait(&wq, &wait);
> > > +		ps3fb.is_kicked = 0;
> > > +		ps3fb_sync(0);	/* single buffer */
> > 
> > should probably be just:
> > 
> > 	while (!kthread_should_stop()) {
> > 		ps3fb_sync(0);
> > 		schedule();
> > 	}
> > 
> > given that you don't need a waitqueue and a spurious wakeup here
> > seems harmless.
> 
> Not always. If flipping is disabled, or external flip is enabled, you don't
> want a spurious flip.

Looks like the waitqueue is needed. Without it ps3fbd runs at 100% CPU and +15
million loops per second.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Linux-fbdev-devel] [PATCH 1/4 (updated)] ps3fb: thread updates
  2007-02-19 14:07               ` Geert Uytterhoeven
@ 2007-02-20 10:33                 ` Geert Uytterhoeven
  2007-02-20 10:42                   ` [PATCH 1/4 (final?)] " Geert Uytterhoeven
  0 siblings, 1 reply; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-20 10:33 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Andrew Morton, Linux Frame Buffer Device Development,
	Linux/PPC Development

On Mon, 19 Feb 2007, Geert Uytterhoeven wrote:
> On Fri, 16 Feb 2007, Geert Uytterhoeven wrote:
> > On Fri, 16 Feb 2007, Christoph Hellwig wrote:
> > > > +	DEFINE_WAIT(wait);
> > > > +	DECLARE_WAIT_QUEUE_HEAD(wq);
> > > > +
> > > > +	while (!kthread_should_stop()) {
> > > > +		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> > > > +		if (!ps3fb.is_kicked)
> > > > +			schedule();
> > > > +		finish_wait(&wq, &wait);
> > > > +		ps3fb.is_kicked = 0;
> > > > +		ps3fb_sync(0);	/* single buffer */
> > > 
> > > should probably be just:
> > > 
> > > 	while (!kthread_should_stop()) {
> > > 		ps3fb_sync(0);
> > > 		schedule();
> > > 	}
> > > 
> > > given that you don't need a waitqueue and a spurious wakeup here
> > > seems harmless.
> > 
> > Not always. If flipping is disabled, or external flip is enabled, you don't
> > want a spurious flip.
> 
> Looks like the waitqueue is needed. Without it ps3fbd runs at 100% CPU and +15
> million loops per second.

Nope, adding `set_current_state(TASK_INTERRUPTIBLE);' fixed it. Stay tuned for
a new patch.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4 (final?)] ps3fb: thread updates
  2007-02-20 10:33                 ` Geert Uytterhoeven
@ 2007-02-20 10:42                   ` Geert Uytterhoeven
  2007-02-21 23:20                       ` Andrew Morton
  0 siblings, 1 reply; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-20 10:42 UTC (permalink / raw)
  To: Andrew Morton, Christoph Hellwig
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development

ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
simply woken up when the screen must be updated

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)

--- ps3-linux-2.6.20.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.20/drivers/video/ps3fb.c
@@ -32,6 +32,7 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -129,7 +130,6 @@ struct ps3fb_priv {
 	u64 context_handle, memory_handle;
 	void *xdr_ea;
 	struct gpu_driver_info *dinfo;
-	struct semaphore sem;
 	u32 res_index;
 
 	u64 vblank_count;	/* frame count */
@@ -139,6 +139,8 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	int is_kicked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,11 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (ps3fb.is_kicked) {
+			ps3fb.is_kicked = 0;
 			ps3fb_sync(0);	/* single buffer */
+		}
+		schedule();
 	}
 	return 0;
 }
@@ -830,8 +834,11 @@ static irqreturn_t ps3fb_vsync_interrupt
 	if (v1 & (1 << GPU_INTR_STATUS_VSYNC_1)) {
 		/* VSYNC */
 		ps3fb.vblank_count = head->vblank_count;
-		if (!ps3fb.is_blanked)
-			up(&ps3fb.sem);
+		if (ps3fb.task && !ps3fb.is_blanked &&
+		    !atomic_read(&ps3fb.ext_flip)) {
+			ps3fb.is_kicked = 1;
+			wake_up_process(ps3fb.task);
+		}
 		wake_up_interruptible(&ps3fb.wait_vsync);
 	}
 
@@ -968,6 +975,7 @@ static int __init ps3fb_probe(struct pla
 	u64 xdr_lpar;
 	int status;
 	unsigned long offset;
+	struct task_struct *task;
 
 	/* get gpu context handle */
 	status = lv1_gpu_memory_allocate(DDR_SIZE, 0, 0, 0, 0,
@@ -1050,9 +1058,18 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(task)) {
+		retval = PTR_ERR(task);
+		goto err_unregister_framebuffer;
+	}
+
+	ps3fb.task = task;
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1100,11 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		struct task_struct *task = ps3fb.task;
+		ps3fb.task = NULL;
+		kthread_stop(task);
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);
@@ -1195,7 +1217,6 @@ static int __init ps3fb_init(void)
 
 	atomic_set(&ps3fb.f_count, -1);	/* fbcon opens ps3fb */
 	atomic_set(&ps3fb.ext_flip, 0);	/* for flip with vsync */
-	init_MUTEX(&ps3fb.sem);
 	init_waitqueue_head(&ps3fb.wait_vsync);
 	ps3fb.num_frames = 1;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
  2007-02-20 10:42                   ` [PATCH 1/4 (final?)] " Geert Uytterhoeven
@ 2007-02-21 23:20                       ` Andrew Morton
  0 siblings, 0 replies; 39+ messages in thread
From: Andrew Morton @ 2007-02-21 23:20 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development,
	Christoph Hellwig

On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
> simply woken up when the screen must be updated

<goes off and generates the incremental diff again so we can see what changed>


diff -puN drivers/video/ps3fb.c~ps3fb-thread-updates-2 drivers/video/ps3fb.c
--- a/drivers/video/ps3fb.c~ps3fb-thread-updates-2
+++ a/drivers/video/ps3fb.c
@@ -32,7 +32,6 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
-#include <linux/freezer.h>
 #include <linux/kthread.h>
 
 #include <asm/uaccess.h>
@@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	DEFINE_WAIT(wait);
-	DECLARE_WAIT_QUEUE_HEAD(wq);
-
 	while (!kthread_should_stop()) {
-		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
-		if (!ps3fb.is_kicked)
-			schedule();
-		finish_wait(&wq, &wait);
-		ps3fb.is_kicked = 0;
-		ps3fb_sync(0);	/* single buffer */
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (ps3fb.is_kicked) {
+			ps3fb.is_kicked = 0;
+			ps3fb_sync(0);	/* single buffer */
+		}
+		schedule();
 	}
 	return 0;
 }
_

There's still no try_to_freeze() in there.  Shouldn't we have one?



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
@ 2007-02-21 23:20                       ` Andrew Morton
  0 siblings, 0 replies; 39+ messages in thread
From: Andrew Morton @ 2007-02-21 23:20 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development

On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
> simply woken up when the screen must be updated

<goes off and generates the incremental diff again so we can see what changed>


diff -puN drivers/video/ps3fb.c~ps3fb-thread-updates-2 drivers/video/ps3fb.c
--- a/drivers/video/ps3fb.c~ps3fb-thread-updates-2
+++ a/drivers/video/ps3fb.c
@@ -32,7 +32,6 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
-#include <linux/freezer.h>
 #include <linux/kthread.h>
 
 #include <asm/uaccess.h>
@@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	DEFINE_WAIT(wait);
-	DECLARE_WAIT_QUEUE_HEAD(wq);
-
 	while (!kthread_should_stop()) {
-		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
-		if (!ps3fb.is_kicked)
-			schedule();
-		finish_wait(&wq, &wait);
-		ps3fb.is_kicked = 0;
-		ps3fb_sync(0);	/* single buffer */
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (ps3fb.is_kicked) {
+			ps3fb.is_kicked = 0;
+			ps3fb_sync(0);	/* single buffer */
+		}
+		schedule();
 	}
 	return 0;
 }
_

There's still no try_to_freeze() in there.  Shouldn't we have one?

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
  2007-02-21 23:20                       ` Andrew Morton
@ 2007-02-22  8:21                         ` Geert Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22  8:21 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development,
	Christoph Hellwig

On Wed, 21 Feb 2007, Andrew Morton wrote:
> On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
> > simply woken up when the screen must be updated
> 
> <goes off and generates the incremental diff again so we can see what changed>

Sorry, I thought you were just going to replace the patch you already had in
your queue/heap/stack.

> @@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
>  
>  static int ps3fbd(void *arg)
>  {
> -	DEFINE_WAIT(wait);
> -	DECLARE_WAIT_QUEUE_HEAD(wq);
> -
>  	while (!kthread_should_stop()) {
> -		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> -		if (!ps3fb.is_kicked)
> -			schedule();
> -		finish_wait(&wq, &wait);
> -		ps3fb.is_kicked = 0;
> -		ps3fb_sync(0);	/* single buffer */
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (ps3fb.is_kicked) {
> +			ps3fb.is_kicked = 0;
> +			ps3fb_sync(0);	/* single buffer */
> +		}
> +		schedule();
>  	}
>  	return 0;
>  }
> _
> 
> There's still no try_to_freeze() in there.  Shouldn't we have one?

Thanks, I'll add it.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
@ 2007-02-22  8:21                         ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22  8:21 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development

On Wed, 21 Feb 2007, Andrew Morton wrote:
> On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
> > simply woken up when the screen must be updated
> 
> <goes off and generates the incremental diff again so we can see what changed>

Sorry, I thought you were just going to replace the patch you already had in
your queue/heap/stack.

> @@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
>  
>  static int ps3fbd(void *arg)
>  {
> -	DEFINE_WAIT(wait);
> -	DECLARE_WAIT_QUEUE_HEAD(wq);
> -
>  	while (!kthread_should_stop()) {
> -		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> -		if (!ps3fb.is_kicked)
> -			schedule();
> -		finish_wait(&wq, &wait);
> -		ps3fb.is_kicked = 0;
> -		ps3fb_sync(0);	/* single buffer */
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (ps3fb.is_kicked) {
> +			ps3fb.is_kicked = 0;
> +			ps3fb_sync(0);	/* single buffer */
> +		}
> +		schedule();
>  	}
>  	return 0;
>  }
> _
> 
> There's still no try_to_freeze() in there.  Shouldn't we have one?

Thanks, I'll add it.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
  2007-02-21 23:20                       ` Andrew Morton
@ 2007-02-22 12:16                         ` Geert Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22 12:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development,
	Christoph Hellwig

On Wed, 21 Feb 2007, Andrew Morton wrote:
> On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> @@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
>  
>  static int ps3fbd(void *arg)
>  {
> -	DEFINE_WAIT(wait);
> -	DECLARE_WAIT_QUEUE_HEAD(wq);
> -
>  	while (!kthread_should_stop()) {
> -		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> -		if (!ps3fb.is_kicked)
> -			schedule();
> -		finish_wait(&wq, &wait);
> -		ps3fb.is_kicked = 0;
> -		ps3fb_sync(0);	/* single buffer */
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (ps3fb.is_kicked) {
> +			ps3fb.is_kicked = 0;
> +			ps3fb_sync(0);	/* single buffer */
> +		}
> +		schedule();
>  	}
>  	return 0;
>  }
> _
> 
> There's still no try_to_freeze() in there.  Shouldn't we have one?

Here it is. I'll also resend the whole patch for completeness.

Subject: ps3fb: add missing try_to_freeze()

ps3fb: add missing try_to_freeze()

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |    2 ++
 1 files changed, 2 insertions(+)

--- ps3-linux-2.6.21-rc1.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.21-rc1/drivers/video/ps3fb.c
@@ -33,6 +33,7 @@
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/kthread.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -808,6 +809,7 @@ static int ps3fb_ioctl(struct fb_info *i
 static int ps3fbd(void *arg)
 {
 	while (!kthread_should_stop()) {
+		try_to_freeze();
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (ps3fb.is_kicked) {
 			ps3fb.is_kicked = 0;

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/4 (final?)] ps3fb: thread updates
@ 2007-02-22 12:16                         ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22 12:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development

On Wed, 21 Feb 2007, Andrew Morton wrote:
> On Tue, 20 Feb 2007 11:42:04 +0100 (CET)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> @@ -808,16 +807,13 @@ static int ps3fb_ioctl(struct fb_info *i
>  
>  static int ps3fbd(void *arg)
>  {
> -	DEFINE_WAIT(wait);
> -	DECLARE_WAIT_QUEUE_HEAD(wq);
> -
>  	while (!kthread_should_stop()) {
> -		prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
> -		if (!ps3fb.is_kicked)
> -			schedule();
> -		finish_wait(&wq, &wait);
> -		ps3fb.is_kicked = 0;
> -		ps3fb_sync(0);	/* single buffer */
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (ps3fb.is_kicked) {
> +			ps3fb.is_kicked = 0;
> +			ps3fb_sync(0);	/* single buffer */
> +		}
> +		schedule();
>  	}
>  	return 0;
>  }
> _
> 
> There's still no try_to_freeze() in there.  Shouldn't we have one?

Here it is. I'll also resend the whole patch for completeness.

Subject: ps3fb: add missing try_to_freeze()

ps3fb: add missing try_to_freeze()

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |    2 ++
 1 files changed, 2 insertions(+)

--- ps3-linux-2.6.21-rc1.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.21-rc1/drivers/video/ps3fb.c
@@ -33,6 +33,7 @@
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/kthread.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -808,6 +809,7 @@ static int ps3fb_ioctl(struct fb_info *i
 static int ps3fbd(void *arg)
 {
 	while (!kthread_should_stop()) {
+		try_to_freeze();
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (ps3fb.is_kicked) {
 			ps3fb.is_kicked = 0;

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4 (final)] ps3fb: thread updates
  2007-02-21 23:20                       ` Andrew Morton
@ 2007-02-22 12:17                         ` Geert Uytterhoeven
  -1 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22 12:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development,
	Christoph Hellwig

ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
simply woken up when the screen must be updated

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

--- ps3-linux-2.6.21-rc1.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.21-rc1/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/kthread.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -129,7 +131,6 @@ struct ps3fb_priv {
 	u64 context_handle, memory_handle;
 	void *xdr_ea;
 	struct gpu_driver_info *dinfo;
-	struct semaphore sem;
 	u32 res_index;
 
 	u64 vblank_count;	/* frame count */
@@ -139,6 +140,8 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	int is_kicked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,11 +808,14 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
+	while (!kthread_should_stop()) {
+		try_to_freeze();
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (ps3fb.is_kicked) {
+			ps3fb.is_kicked = 0;
 			ps3fb_sync(0);	/* single buffer */
+		}
+		schedule();
 	}
 	return 0;
 }
@@ -830,8 +836,11 @@ static irqreturn_t ps3fb_vsync_interrupt
 	if (v1 & (1 << GPU_INTR_STATUS_VSYNC_1)) {
 		/* VSYNC */
 		ps3fb.vblank_count = head->vblank_count;
-		if (!ps3fb.is_blanked)
-			up(&ps3fb.sem);
+		if (ps3fb.task && !ps3fb.is_blanked &&
+		    !atomic_read(&ps3fb.ext_flip)) {
+			ps3fb.is_kicked = 1;
+			wake_up_process(ps3fb.task);
+		}
 		wake_up_interruptible(&ps3fb.wait_vsync);
 	}
 
@@ -968,6 +977,7 @@ static int __init ps3fb_probe(struct pla
 	u64 xdr_lpar;
 	int status;
 	unsigned long offset;
+	struct task_struct *task;
 
 	/* get gpu context handle */
 	status = lv1_gpu_memory_allocate(DDR_SIZE, 0, 0, 0, 0,
@@ -1050,9 +1060,18 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(task)) {
+		retval = PTR_ERR(task);
+		goto err_unregister_framebuffer;
+	}
+
+	ps3fb.task = task;
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1102,11 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		struct task_struct *task = ps3fb.task;
+		ps3fb.task = NULL;
+		kthread_stop(task);
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);
@@ -1195,7 +1219,6 @@ static int __init ps3fb_init(void)
 
 	atomic_set(&ps3fb.f_count, -1);	/* fbcon opens ps3fb */
 	atomic_set(&ps3fb.ext_flip, 0);	/* for flip with vsync */
-	init_MUTEX(&ps3fb.sem);
 	init_waitqueue_head(&ps3fb.wait_vsync);
 	ps3fb.num_frames = 1;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4 (final)] ps3fb: thread updates
@ 2007-02-22 12:17                         ` Geert Uytterhoeven
  0 siblings, 0 replies; 39+ messages in thread
From: Geert Uytterhoeven @ 2007-02-22 12:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linux/PPC Development, Linux Frame Buffer Device Development

ps3fb: Replace the kernel_thread and the semaphore by a proper kthread, which is
simply woken up when the screen must be updated

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/video/ps3fb.c |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

--- ps3-linux-2.6.21-rc1.orig/drivers/video/ps3fb.c
+++ ps3-linux-2.6.21-rc1/drivers/video/ps3fb.c
@@ -32,6 +32,8 @@
 #include <linux/ioctl.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/kthread.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <linux/fb.h>
@@ -129,7 +131,6 @@ struct ps3fb_priv {
 	u64 context_handle, memory_handle;
 	void *xdr_ea;
 	struct gpu_driver_info *dinfo;
-	struct semaphore sem;
 	u32 res_index;
 
 	u64 vblank_count;	/* frame count */
@@ -139,6 +140,8 @@ struct ps3fb_priv {
 	atomic_t ext_flip;	/* on/off flip with vsync */
 	atomic_t f_count;	/* fb_open count */
 	int is_blanked;
+	int is_kicked;
+	struct task_struct *task;
 };
 static struct ps3fb_priv ps3fb;
 
@@ -805,11 +808,14 @@ static int ps3fb_ioctl(struct fb_info *i
 
 static int ps3fbd(void *arg)
 {
-	daemonize("ps3fbd");
-	for (;;) {
-		down(&ps3fb.sem);
-		if (atomic_read(&ps3fb.ext_flip) == 0)
+	while (!kthread_should_stop()) {
+		try_to_freeze();
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (ps3fb.is_kicked) {
+			ps3fb.is_kicked = 0;
 			ps3fb_sync(0);	/* single buffer */
+		}
+		schedule();
 	}
 	return 0;
 }
@@ -830,8 +836,11 @@ static irqreturn_t ps3fb_vsync_interrupt
 	if (v1 & (1 << GPU_INTR_STATUS_VSYNC_1)) {
 		/* VSYNC */
 		ps3fb.vblank_count = head->vblank_count;
-		if (!ps3fb.is_blanked)
-			up(&ps3fb.sem);
+		if (ps3fb.task && !ps3fb.is_blanked &&
+		    !atomic_read(&ps3fb.ext_flip)) {
+			ps3fb.is_kicked = 1;
+			wake_up_process(ps3fb.task);
+		}
 		wake_up_interruptible(&ps3fb.wait_vsync);
 	}
 
@@ -968,6 +977,7 @@ static int __init ps3fb_probe(struct pla
 	u64 xdr_lpar;
 	int status;
 	unsigned long offset;
+	struct task_struct *task;
 
 	/* get gpu context handle */
 	status = lv1_gpu_memory_allocate(DDR_SIZE, 0, 0, 0, 0,
@@ -1050,9 +1060,18 @@ static int __init ps3fb_probe(struct pla
 	       "fb%d: PS3 frame buffer device, using %ld KiB of video memory\n",
 	       info->node, ps3fb_videomemory.size >> 10);
 
-	kernel_thread(ps3fbd, info, CLONE_KERNEL);
+	task = kthread_run(ps3fbd, info, "ps3fbd");
+	if (IS_ERR(task)) {
+		retval = PTR_ERR(task);
+		goto err_unregister_framebuffer;
+	}
+
+	ps3fb.task = task;
+
 	return 0;
 
+err_unregister_framebuffer:
+	unregister_framebuffer(info);
 err_fb_dealloc:
 	fb_dealloc_cmap(&info->cmap);
 err_framebuffer_release:
@@ -1083,6 +1102,11 @@ void ps3fb_cleanup(void)
 {
 	int status;
 
+	if (ps3fb.task) {
+		struct task_struct *task = ps3fb.task;
+		ps3fb.task = NULL;
+		kthread_stop(task);
+	}
 	if (ps3fb.irq_no) {
 		free_irq(ps3fb.irq_no, ps3fb.dev);
 		ps3_free_irq(ps3fb.irq_no);
@@ -1195,7 +1219,6 @@ static int __init ps3fb_init(void)
 
 	atomic_set(&ps3fb.f_count, -1);	/* fbcon opens ps3fb */
 	atomic_set(&ps3fb.ext_flip, 0);	/* for flip with vsync */
-	init_MUTEX(&ps3fb.sem);
 	init_waitqueue_head(&ps3fb.wait_vsync);
 	ps3fb.num_frames = 1;
 

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2007-02-22 12:18 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-15 15:23 [patch 0/4] PS3 AV/FB updates Geert.Uytterhoeven
2007-02-15 15:23 ` Geert.Uytterhoeven
2007-02-15 15:23 ` [patch 1/4] ps3fb: thread updates Geert.Uytterhoeven
2007-02-15 15:23   ` Geert.Uytterhoeven
2007-02-15 17:50   ` Christoph Hellwig
2007-02-15 17:50     ` Christoph Hellwig
2007-02-15 21:43     ` Benjamin Herrenschmidt
2007-02-16  0:59       ` Andrew Morton
2007-02-16 15:58         ` Geert Uytterhoeven
2007-02-16 15:58           ` Geert Uytterhoeven
2007-02-16 16:00         ` [PATCH 1/4 (updated)] " Geert Uytterhoeven
2007-02-16 16:00           ` Geert Uytterhoeven
2007-02-16 16:38           ` Christoph Hellwig
2007-02-16 16:38             ` Christoph Hellwig
2007-02-16 17:33             ` Geert Uytterhoeven
2007-02-16 17:33               ` [Linux-fbdev-devel] " Geert Uytterhoeven
2007-02-19 14:07               ` Geert Uytterhoeven
2007-02-20 10:33                 ` Geert Uytterhoeven
2007-02-20 10:42                   ` [PATCH 1/4 (final?)] " Geert Uytterhoeven
2007-02-21 23:20                     ` Andrew Morton
2007-02-21 23:20                       ` Andrew Morton
2007-02-22  8:21                       ` Geert Uytterhoeven
2007-02-22  8:21                         ` Geert Uytterhoeven
2007-02-22 12:16                       ` Geert Uytterhoeven
2007-02-22 12:16                         ` Geert Uytterhoeven
2007-02-22 12:17                       ` [PATCH 1/4 (final)] " Geert Uytterhoeven
2007-02-22 12:17                         ` Geert Uytterhoeven
2007-02-16 16:03         ` [PATCH extra] ps3fb: atomic fixes Geert Uytterhoeven
2007-02-16 16:03           ` Geert Uytterhoeven
2007-02-16 16:36         ` [patch 1/4] ps3fb: thread updates Christoph Hellwig
2007-02-16 16:36           ` Christoph Hellwig
2007-02-15 15:23 ` [patch 2/4] ps3av: " Geert.Uytterhoeven
2007-02-15 15:23   ` Geert.Uytterhoeven
2007-02-15 15:23 ` [patch 3/4] ps3fb: kill superfluous zero initializations Geert.Uytterhoeven
2007-02-15 15:23   ` Geert.Uytterhoeven
2007-02-15 15:23 ` [patch 4/4] ps3av: misc updates Geert.Uytterhoeven
2007-02-15 15:23   ` Geert.Uytterhoeven
2007-02-15 15:48 ` [patch 0/4] PS3 AV/FB updates James Simmons
2007-02-15 15:48   ` [Linux-fbdev-devel] " James Simmons

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.