powermac: defer work in backlight key press
Subsystems (2)
PATCH 1/1 powermac: defer work in backlight key press
powermac: defer work in backlight key press
pmac_backlight_key() is called under interrupt context, can't use mutexes or semaphores, so defer the backlight level for later, as it's not critical
arch/powerpc/platforms/powermac/backlight.c +15 -2
@@ -15,6 +15,10 @@#define OLD_BACKLIGHT_MAX 15 +static void pmac_backlight_key_worker (void *data); +static DECLARE_WORK(pmac_backlight_key_work, pmac_backlight_key_worker, NULL); +static int pmac_backlight_key_queued; +/* Protect the pmac_backlight variable */ DEFINE_MUTEX (pmac_backlight_mutex);
@@ -71,7 +75,7 @@return level; } -static void pmac_backlight_key (int direction) +static void pmac_backlight_key_worker (void *data) {mutex_lock (&pmac_backlight_mutex);if (pmac_backlight) {
@@ -82,7 +86,8 @@ props = pmac_backlight->props; brightness = props->brightness + - ((direction?-1 :1 ) * (props->max_brightness /15 )); + ((pmac_backlight_key_queued?-1 :1 ) * + (props->max_brightness /15 ));if (brightness <0 ) brightness =0 ;
@@ -97,6 +102,14 @@mutex_unlock (&pmac_backlight_mutex); } +static void pmac_backlight_key (int direction) +{ +/* we can receive multiple interrupts here, but the scheduled work +* will run only once, with the last value */ + pmac_backlight_key_queued = direction; +schedule_work (&pmac_backlight_key_work); +} +void pmac_backlight_key_up () {pmac_backlight_key (0 );
arch/powerpc/platforms/powermac/backlight.c line 109
Quoted 4 lines
arch/powerpc/platforms/powermac/backlight.c +3
-0+/* we can receive multiple interrupts here, but the scheduled work +* will run only once, with the last value */ + pmac_backlight_key_queued = direction;
Shouldn't you be doing something like using the whole pmac_backlight_key_queued in the workqueue, then setting it to 0 there and using += here? That way all keypresses will be adhered to.
johannes
Quoted 7 lines
arch/powerpc/platforms/powermac/backlight.c line 109
Quoted 3 lines
arch/powerpc/platforms/powermac/backlight.c +3
-0+/* we can receive multiple interrupts here, but the scheduled work +* will run only once, with the last value */ + pmac_backlight_key_queued = direction;Shouldn't you be doing something like using the whole
pmac_backlight_key_queuedin the workqueue, then setting it to 0 there and using += here? That way all keypresses will be adhered to.
today pmac_backlight_key() is called either with 0 (to reduce backlight level) or 1 (to increase). I found that actually happening two keystrokes between interrupt being serviced and actually running that work queue would be so damn small that the extra code wouldn't be worth. but if you think it's worth for, I can fix it, no problem.
Signature · 1 line
Aristeu
Quoted 2 lines · Aristeu Sergio Rozanski Filho
today
pmac_backlight_key()is called either with 0 (to reduce backlight level) or 1 (to increase).
Oh, it looked on first look like it was -1 or 1, I see it now.
Quoted 4 lines
I found that actually happening two
keystrokes between interrupt being serviced and actually running that work queue would be so damn small that the extra code wouldn't be worth. but if you think it's worth for, I can fix it, no problem.
Yeah, you're right. Unless that thing has some fast repeat if you hold keys... Not sure what I was thinking.
johannes
Quoted 4 lines · Aristeu Sergio Rozanski Filho
powermac: defer work in backlight key press
pmac_backlight_key()is called under interrupt context, can't use mutexes or semaphores, so defer the backlight level for later, as it's not critical
Nack, needs spinlock around this part:
arch/powerpc/platforms/powermac/backlight.c line 110
Quoted 2 lines
arch/powerpc/platforms/powermac/backlight.c +2
-0+ pmac_backlight_key_queued = direction; +schedule_work (&pmac_backlight_key_work);
I'll submit another patch with more fixes soon.
Greets,
Michael
Re: [PATCH] powermac: defer work in backlight key press (and export fixes)
In reply to Aristeu Sergio Rozanski Filhopmac_backlight_key() is called under interrupt context, and therefore can't use mutexes or semaphores, so defer the backlight level for later, as it's not critical (code by Aristeu S. Rozanski F. <[email protected]>). Also, it fixes exports and Kconfig depdencies.
---
arch/powerpc/platforms/powermac/backlight.c +22 -9
@@ -15,6 +15,15 @@#define OLD_BACKLIGHT_MAX 15 +static void pmac_backlight_key_worker (void *data); +static DECLARE_WORK(pmac_backlight_key_work, pmac_backlight_key_worker, NULL); + +/* Although this variable is used in interrupt context, it makes no sense to +* protect it. No user is able to produce enough key events per second and +* notice the errors that might happen. +*/ +static int pmac_backlight_key_queued; +/* Protect the pmac_backlight variable */ DEFINE_MUTEX (pmac_backlight_mutex);
@@ -71,7 +80,7 @@ int pmac_backlight_curve_lookup(struct freturn level; } -static void pmac_backlight_key (int direction) +static void pmac_backlight_key_worker (void *data) {mutex_lock (&pmac_backlight_mutex);if (pmac_backlight) {
@@ -82,7 +91,8 @@ static void pmac_backlight_key(int direc props = pmac_backlight->props; brightness = props->brightness + - ((direction?-1 :1 ) * (props->max_brightness /15 )); + ((pmac_backlight_key_queued?-1 :1 ) * + (props->max_brightness /15 ));if (brightness <0 ) brightness =0 ;
@@ -97,14 +107,13 @@ static void pmac_backlight_key(int direcmutex_unlock (&pmac_backlight_mutex); } -void pmac_backlight_key_up () +void pmac_backlight_key (int direction) { -pmac_backlight_key (0 ); -} - -void pmac_backlight_key_down () -{ -pmac_backlight_key (1 ); +/* we can receive multiple interrupts here, but the scheduled work +* will run only once, with the last value +*/ + pmac_backlight_key_queued = direction; +schedule_work (&pmac_backlight_key_work); }int pmac_backlight_set_legacy_brightness (int brightness)
@@ -157,3 +166,7 @@ int pmac_backlight_get_legacy_brightnessreturn result; } + +EXPORT_SYMBOL_GPL (pmac_backlight); +EXPORT_SYMBOL_GPL (pmac_backlight_mutex); +EXPORT_SYMBOL_GPL (pmac_has_backlight_type);
drivers/macintosh/Kconfig +4 -1
@@ -113,7 +113,10 @@ config PMAC_MEDIABAY config PMAC_BACKLIGHT bool "Backlight control for LCD screens" - depends on ADB_PMU && (BROKEN || !PPC64) + depends on ADB_PMU && FB = y && (BROKEN || !PPC64) + select FB_BACKLIGHT + select BACKLIGHT_CLASS_DEVICE + select BACKLIGHT_LCD_SUPPORT help Say Y here to enable Macintosh specific extensions of the generic backlight code. With this enabled, the brightness keys on older
include/asm-powerpc/backlight.h +6 -3
@@ -16,13 +16,16 @@extern struct backlight_device *pmac_backlight;extern struct mutex pmac_backlight_mutex; -extern void pmac_backlight_calc_curve (struct fb_info *);extern int pmac_backlight_curve_lookup (struct fb_info *info,int value);extern int pmac_has_backlight_type (const char *type); -extern void pmac_backlight_key_up (void ); -extern void pmac_backlight_key_down (void ); +extern void pmac_backlight_key (int direction); + +#define pmac_backlight_key_up () \ + do { pmac_backlight_key(0); } while(0); +#define pmac_backlight_key_down () \ + do { pmac_backlight_key(1); } while(0);extern int pmac_backlight_set_legacy_brightness (int brightness);extern int pmac_backlight_get_legacy_brightness (void );