Patchworkβ [2/2] eeepc-laptop: don't enable camera at startup if it's already on.

login
register
about
Submitter Corentin Chary
Date 2009-10-16 20:22:47
Message ID <1255724567-25597-3-git-send-email-corentincj@iksaif.net>
Download mbox | patch
Permalink /patch/54413/
State New
Delegated to: Len Brown
Headers show

Comments

Corentin Chary - 2009-10-16 20:22:47
From: Luca Niccoli <lultimouomo@gmail.com>

Switching the camera takes 500ms, checking if it's on is almost free...
The BIOS remembers the setting through reboots, so there's good chance the
camera is already enabled.

Signed-off-by: Luca Niccoli <lultimouomo@gmail.com>
Cc: Corentin Chary <corentincj@iksaif.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Cc: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 drivers/platform/x86/eeepc-laptop.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Andrey Rahmatullin - 2009-10-20 08:35:55
On Fri, Oct 16, 2009 at 10:22:47PM +0200, Corentin Chary wrote:
> Switching the camera takes 500ms, checking if it's on is almost free...
So if I don't want camera to be enabled, I should wait 500ms on boot to
enable it and then other 500ms to disable?
Alan Jenkins - 2009-10-20 09:04:04
On 10/20/09, Andrey Rahmatullin <wrar@altlinux.org> wrote:
> On Fri, Oct 16, 2009 at 10:22:47PM +0200, Corentin Chary wrote:
>> Switching the camera takes 500ms, checking if it's on is almost free...
> So if I don't want camera to be enabled, I should wait 500ms on boot to
> enable it and then other 500ms to disable?

Yup.  The original patch from Pekka was accepted alongside a patch
which enabled auto-suspend for the uvcvideo driver by default.  This
was shown to have the same effect on CPU wakeups as measured by
powertop.

So the question is why do you want to disable it?  Have you measured
the power consumption on the current kernel using a more comprehensive
method than powertop?

Or perhaps you have a different camera chip?  Have you tried enabling
auto-suspend manually?  If it works, perhaps that driver can enable
auto-suspend as well.

Regards
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrey Rahmatullin - 2009-10-20 09:24:02
On Tue, Oct 20, 2009 at 10:04:04AM +0100, Alan Jenkins wrote:
> Yup.  The original patch from Pekka was accepted alongside a patch
> which enabled auto-suspend for the uvcvideo driver by default.  This
> was shown to have the same effect on CPU wakeups as measured by
> powertop.
When the camera is enabled, powertop says "device is active 100% of time".
That was enough for me :)

> Have you measured the power consumption on the current kernel using a
> more comprehensive method than powertop?
No.

> Or perhaps you have a different camera chip?  Have you tried enabling
> auto-suspend manually?  If it works, perhaps that driver can enable
> auto-suspend as well.
/sys/module/usbcore/parameters/autosuspend and
/sys/devices/pci0000:00/0000:00:1d.7/usb5/5-8/power/autosuspend both
contain 1, but powertop still suggests adding usbcore.autosuspend=1.
Though nothing related to USB exists in wakeup causes list.
Alan Jenkins - 2009-10-20 09:38:17
On 10/20/09, Andrey Rahmatullin <wrar@altlinux.org> wrote:
> On Tue, Oct 20, 2009 at 10:04:04AM +0100, Alan Jenkins wrote:
>> Yup.  The original patch from Pekka was accepted alongside a patch
>> which enabled auto-suspend for the uvcvideo driver by default.  This
>> was shown to have the same effect on CPU wakeups as measured by
>> powertop.
> When the camera is enabled, powertop says "device is active 100% of time".
> That was enough for me :)

yes but if you are missing the original "enable camera by default"
patch  *which has already been merged* then you are almost certainly
also missing the patch to enable autosuspend on uvcvideo by default.

>> Have you measured the power consumption on the current kernel using a
>> more comprehensive method than powertop?
> No.
>
>> Or perhaps you have a different camera chip?  Have you tried enabling
>> auto-suspend manually?  If it works, perhaps that driver can enable
>> auto-suspend as well.
> /sys/module/usbcore/parameters/autosuspend and
> /sys/devices/pci0000:00/0000:00:1d.7/usb5/5-8/power/autosuspend both
> contain 1, but powertop still suggests adding usbcore.autosuspend=1.
> Though nothing related to USB exists in wakeup causes list.

Yeah, powertop's autosuspend recommendation is a bit outdated.

Here's a link to the original thread.
<http://lkml.indiana.edu/hypermail/linux/kernel/0906.0/02603.html>.
The command I used to manually enable autosuspend was

echo -n auto > /sys/bus/usb/drivers/uvcvideo/*:*/../power/level

obviously this will need adjusting if the driver is different.  (This
is still the same "power" directory at the end, you're just reaching
it through a different path).

I hope this addresses your concern.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrey Rahmatullin - 2009-10-20 09:41:43
On Tue, Oct 20, 2009 at 10:38:17AM +0100, Alan Jenkins wrote:
> yes but if you are missing the original "enable camera by default"
> patch  *which has already been merged* then you are almost certainly
> also missing the patch to enable autosuspend on uvcvideo by default.
No, I'm not missing it, I'm using .32-rc5

> Yeah, powertop's autosuspend recommendation is a bit outdated.
So, can I safely ignore it and assume all is right when I use latest
kernels?

> Here's a link to the original thread.
> <http://lkml.indiana.edu/hypermail/linux/kernel/0906.0/02603.html>.
OK, I'll read it, thanks.
Luca Niccoli - 2009-10-20 10:06:54
2009/10/20 Alan Jenkins <sourcejedi.lkml@googlemail.com>:

> So the question is why do you want to disable it?  Have you measured
> the power consumption on the current kernel using a more comprehensive
> method than powertop?

Actually, on my eee 901, linux 2.6.31, udev 146,
I have /sys/bus/usb/drivers/uvcvideo/*:*/../power/level = on
If I manually set level to auto, I save 0.2W
How's that supposed to work? Is it kernel's or udev responsibility to
set the autosuspend?
Cheers,

Luca
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Jenkins - 2009-10-20 14:19:54
On 10/20/09, Andrey Rahmatullin <wrar@altlinux.org> wrote:
> On Tue, Oct 20, 2009 at 10:38:17AM +0100, Alan Jenkins wrote:
>> yes but if you are missing the original "enable camera by default"
>> patch  *which has already been merged* then you are almost certainly
>> also missing the patch to enable autosuspend on uvcvideo by default.
> No, I'm not missing it, I'm using .32-rc5
>
>> Yeah, powertop's autosuspend recommendation is a bit outdated.
> So, can I safely ignore it and assume all is right when I use latest
> kernels?

Sorry, I assumed this was the case but I didn't check properly.  As
Luca observes it's not been merged yet.  <google>.

The patch exists in Fedora
<http://osdir.com/ml/fedora-extras-commits/2009-07/msg06079.html>.

with the aim of beta-testing it
<http://lkml.indiana.edu/hypermail/linux/kernel/0909.1/03142.html>.
It's fair to say there's a degree of risk to it.  UVC is a "standard"
which presumably has various implementations with their own potential
bugs.

In the mean time - the thread I linked to includes a helpful note from
Xandros.  They measured that enabling the camera on the 901 model cost
~3% of overall power.

I guess the argument is that it's better to waste a little power,
rather than make life hard for people who install Linux themselves
:-/.  After all these devices are mainly shipped with Windows, and the
original pre-installed Linux (which hacked around this in video
applications) is not seriously maintained.  The problem is that when
the pre-installed OS uses the hack, the camera will be disabled in the
BIOS.  When you install a generic version of Linux without hacked
applications, you either have eeepc-laptop enable the camera, or the
user has to learn how to do it themself.

I don't have a strong opinion myself, so long as it's fixed properly
in the long term.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luca Niccoli - 2009-10-20 14:31:34
2009/10/20 Alan Jenkins <sourcejedi.lkml@googlemail.com>:

> I guess the argument is that it's better to waste a little power,
> rather than make life hard for people who install Linux themselves
> :-/.  After all these devices are mainly shipped with Windows, and the
> original pre-installed Linux (which hacked around this in video
> applications) is not seriously maintained.  The problem is that when
> the pre-installed OS uses the hack, the camera will be disabled in the
> BIOS.  When you install a generic version of Linux without hacked
> applications, you either have eeepc-laptop enable the camera, or the
> user has to learn how to do it themself.
>
> I don't have a strong opinion myself, so long as it's fixed properly
> in the long term.

For the short term, I guess that distributions that ship some
eee-related package could make it include an udev rule - I'm going to
propose this in Debian.
For the long term:
if having blanket-enabled autosuspend is shown to rarely raise
problems, maybe a blacklist could be added in default udev rules?

Luca
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Len Brown - 2009-11-03 15:25:42
applied

thanks,
Len Brown, Intel Open Source Technology Center

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 789d6ae..4226e53 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -356,7 +356,8 @@  static void __devinit eeepc_enable_camera(void)
 	 * If the following call to set_acpi() fails, it's because there's no
 	 * camera so we can ignore the error.
 	 */
-	set_acpi(CM_ASL_CAMERA, 1);
+	if (get_acpi(CM_ASL_CAMERA) == 0)
+		set_acpi(CM_ASL_CAMERA, 1);
 }
 
 /*