Re: [Intel-gfx] [PATCH 2/2] configure.ac: Do not include `x11-xcb`, `xcb-dri2` and `xcb-aux` in `XVMCLIB`
The first patch looks fine (and I've applied it, thanks), but I'm not convinced this one is safe. On Sat, Feb 02, 2013 at 11:00:15PM +0100, Paul Menzel wrote: Date: Sat, 2 Feb 2013 20:33:36 +0100 Building the package under Debian Sid/unstable, `dh_shlibdeps` informs that `libI810XvMC.so.1.0.0` does not need to be linked against `libX11-xcb.so.1`, `libxcb-dri2.so.0`, `libxcb-util.so.0` or `libxcb.so.1` [1]. $ debuild -b -us -uc […] make[1]: Entering directory `/src/xserver-xorg-video-intel' dh_shlibdeps -- --warnings=6 dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libX11-xcb.so.1 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb-dri2.so.0 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb-util.so.0 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb.so.1 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) make[1]: Leaving directory `/src/xserver-xorg-video-intel' […] Not populating `XVMCLIB` with `x11-xcb`, `xcb-dri2` and `xcb-aux` makes the warnings go away and the libraries are still built without any issues. Notice that the warning is only generated for I810XvMC and not for IntelXvMC which does use the DRI2 interface to pass along video objects. Since XVMCLIB is shared between the two we would need to split out the I810XvMC dependencies from the IntelXvMC set first. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] PM: make VT switching to the suspend console optional v2
On Sat, 02 Feb 2013 21:50:35 +0100 Rafael J. Wysocki r...@sisk.pl wrote: + * Drivers can indicate support for switchless suspend/resume, which can + * save time and flicker, by using this routine and passing 'false' as + * the argument. If any loaded driver needs VT switching, or the + * no_console_suspend argument has been passed on the command line, VT + * switches will occur. + */ It seems to me that we'll need a separate counter for the number of registered drivers and do the switch if that number is equal to the number of drivers that have passed false to this thing. In which case we can simplify this slightly and introduce pm_vt_swtich_not_required(void) Sorry, that won't be sufficient. Rather something like pm_vt_switch_get() (indicating I'll do the switch, thanks) and pm_vt_switch_put() (indicating now you need to do the switch yourself). I thought of both of your approaches before posting this one, but each have other problems. And I found a bug in mine last night. So I think I need a separate count of drivers that need the switch, and ones that don't. Then if either no driver has registered or if the need_switch count is nonzero, we'll do the switch. Otherwise, if the dont_need_switch is nonzero, we can avoid the switch. I think that'll handle all the cases I outlined. My code as posted will fail if one driver needs a switch but then two switch free drivers register. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/10] [RFC v2] quick dump
On Sat, 2 Feb 2013 16:07:52 -0800 Ben Widawsky b...@bwidawsk.net wrote: This is my second attempt at winning approval for the series. First one was: https://patchwork.kernel.org/patch/1493131/ In spending the time to rework this tool, I've begun to lose my belief in some of the original motivations I had. Even if you don't want to review, but just like (or dislike) what you see, I'd appreciate such comments. I'd like to see it land in i-g-t. Having the regs defined in a text or xml file is an improvement over what we have today, and is easier to extend. At first the advantage of reg_dumper was that it parsed out the bitfields of the various regs. But we didn't keep up with that, and also haven't kept up with the regs on new platforms as well as we could. Text files would make that easier, and xml files might bring back the bit field parsing, which would be extra nice. Acked-by: Jesse Barnes jbar...@virtuousgeek.org Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/10] [RFC v2] quick dump
On Sun, Feb 03, 2013 at 10:29:15AM +0100, Jesse Barnes wrote: On Sat, 2 Feb 2013 16:07:52 -0800 Ben Widawsky b...@bwidawsk.net wrote: This is my second attempt at winning approval for the series. First one was: https://patchwork.kernel.org/patch/1493131/ In spending the time to rework this tool, I've begun to lose my belief in some of the original motivations I had. Even if you don't want to review, but just like (or dislike) what you see, I'd appreciate such comments. I'd like to see it land in i-g-t. Having the regs defined in a text or xml file is an improvement over what we have today, and is easier to extend. At first the advantage of reg_dumper was that it parsed out the bitfields of the various regs. But we didn't keep up with that, and also haven't kept up with the regs on new platforms as well as we could. Text files would make that easier, and xml files might bring back the bit field parsing, which would be extra nice. Completely agree. For me the big improvement would be being able to use the bspec register names or our internal approximation thereof rather than having to loop up the actual addresses every time. Having the name database available in python should make building integrated little snippets to parse traces which are also python accessible. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] configure.ac: Do not include `x11-xcb`, `xcb-dri2` and `xcb-aux` in `XVMCLIB`
Am Sonntag, den 03.02.2013, 09:32 + schrieb Chris Wilson: The first patch looks fine (and I've applied it, thanks), Thank you for the quick reaction. but I'm not convinced this one is safe. On Sat, Feb 02, 2013 at 11:00:15PM +0100, Paul Menzel wrote: Date: Sat, 2 Feb 2013 20:33:36 +0100 Building the package under Debian Sid/unstable, `dh_shlibdeps` informs that `libI810XvMC.so.1.0.0` does not need to be linked against `libX11-xcb.so.1`, `libxcb-dri2.so.0`, `libxcb-util.so.0` or `libxcb.so.1` [1]. $ debuild -b -us -uc […] make[1]: Entering directory `/src/xserver-xorg-video-intel' dh_shlibdeps -- --warnings=6 dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libX11-xcb.so.1 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb-dri2.so.0 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb-util.so.0 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) dpkg-shlibdeps: Warnung: debian/xserver-xorg-video-intel/usr/lib/libI810XvMC.so.1.0.0 sollte nicht gegen libxcb.so.1 gelinkt werden (es verwendet keines der Bibliotheks-Symbole) make[1]: Leaving directory `/src/xserver-xorg-video-intel' […] Not populating `XVMCLIB` with `x11-xcb`, `xcb-dri2` and `xcb-aux` makes the warnings go away and the libraries are still built without any issues. Notice that the warning is only generated for I810XvMC and not for IntelXvMC which does use the DRI2 interface to pass along video objects. I was surprised too that no error was generated. Do you have any idea why compilations succeeds? Since XVMCLIB is shared between the two we would need to split out the I810XvMC dependencies from the IntelXvMC set first. Thanks, Paul signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] PM: make VT switching to the suspend console optional v2
On Sunday, February 03, 2013 09:56:09 AM Jesse Barnes wrote: On Sat, 02 Feb 2013 21:50:35 +0100 Rafael J. Wysocki r...@sisk.pl wrote: + * Drivers can indicate support for switchless suspend/resume, which can + * save time and flicker, by using this routine and passing 'false' as + * the argument. If any loaded driver needs VT switching, or the + * no_console_suspend argument has been passed on the command line, VT + * switches will occur. + */ It seems to me that we'll need a separate counter for the number of registered drivers and do the switch if that number is equal to the number of drivers that have passed false to this thing. In which case we can simplify this slightly and introduce pm_vt_swtich_not_required(void) Sorry, that won't be sufficient. Rather something like pm_vt_switch_get() (indicating I'll do the switch, thanks) and pm_vt_switch_put() (indicating now you need to do the switch yourself). I thought of both of your approaches before posting this one, but each have other problems. And I found a bug in mine last night. So I think I need a separate count of drivers that need the switch, and ones that don't. Then if either no driver has registered or if the need_switch count is nonzero, we'll do the switch. Otherwise, if the dont_need_switch is nonzero, we can avoid the switch. I think that'll handle all the cases I outlined. Yes, it should cover them all I think. My code as posted will fail if one driver needs a switch but then two switch free drivers register. I see. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Advanced Deinterlacers
Hello, As far as I have seen you have successfully implementeted the API technology to select interlacers in the video pipeline as video postprocessor since middle of last year. The VDR community is still waiting for MotionAdaptive or MotionCompensated. Could you comment on your schedule for that? - Is it for some reason technologically impossible to get it implemented? - Are the plans for IVB or SBR to make it available? I am very interested in this also since I probably have to replace my old VDR in the near future and I need to know if I have to go the NVidia route because of working VDPAU support for advanced deinterlacers (although I would not like it since it's closed source) or if I can expect working advanced deinterlacers in the near future in the Intel stack. Because I would prefer to go the Intel route instead. Thanks and best regards, Jogi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/10] [RFC v2] quick dump
On Sun, Feb 03, 2013 at 12:22:25PM +, Chris Wilson wrote: On Sun, Feb 03, 2013 at 10:29:15AM +0100, Jesse Barnes wrote: On Sat, 2 Feb 2013 16:07:52 -0800 Ben Widawsky b...@bwidawsk.net wrote: This is my second attempt at winning approval for the series. First one was: https://patchwork.kernel.org/patch/1493131/ In spending the time to rework this tool, I've begun to lose my belief in some of the original motivations I had. Even if you don't want to review, but just like (or dislike) what you see, I'd appreciate such comments. I'd like to see it land in i-g-t. Having the regs defined in a text or xml file is an improvement over what we have today, and is easier to extend. At first the advantage of reg_dumper was that it parsed out the bitfields of the various regs. But we didn't keep up with that, and also haven't kept up with the regs on new platforms as well as we could. Text files would make that easier, and xml files might bring back the bit field parsing, which would be extra nice. Completely agree. For me the big improvement would be being able to use the bspec register names or our internal approximation thereof rather than having to loop up the actual addresses every time. Having the name database available in python should make building integrated little snippets to parse traces which are also python accessible. -Chris It's really nice to get support from you. A mix of fever and staring at the same code too long can really make someone go crazy. Still, a few concerns left from me, one of which I accidentally left out of the description. - Someone needs to give me a yes or no on the m4 extension macros. This will block any pushing. - The build kind of sucks on Arch because of Arch's choices regarding python libraries. To build this on Arch, you must run something like: ./autogen.sh PYTHON_LDFLAGS=-L/usr/lib/python3.3 -lpython3.3m I really don't like autogen not working out of the box. Perhaps I need to add an AC_ flag to default this tool to off? What do others think? Does it work properly on other distros? How to handle this? - Ideally, I'd like someone to send me some fixes for valleyview definitions if they're needed. I am not sure. Jesse, if you can send me a list of DPIO offsets to read, I'll add the appropriate patch. (It can wait until you get back). -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/10] quick_dump: Connect libpciaccess and other utils
On Sat, Feb 02, 2013 at 04:08:01PM -0800, Ben Widawsky wrote:
Make a register access library with sample to do register reads
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
tools/quick_dump/Makefile.am | 14 +-
tools/quick_dump/chipset.i | 16 ++--
tools/quick_dump/intel_chipset.c | 7 +++
tools/quick_dump/quick_dump.py | 5 ++---
tools/quick_dump/reg_access.py | 25 +
5 files changed, 57 insertions(+), 10 deletions(-)
create mode 100755 tools/quick_dump/reg_access.py
diff --git a/tools/quick_dump/Makefile.am b/tools/quick_dump/Makefile.am
index 6c04dd5..4711830 100644
--- a/tools/quick_dump/Makefile.am
+++ b/tools/quick_dump/Makefile.am
@@ -1,14 +1,18 @@
BUILT_SOURCES = chipset_wrap_python.c
-bin_SCRIPTS = quick_dump.py chipset.py
+bin_SCRIPTS = quick_dump.py chipset.py reg_access.py
lib_LTLIBRARIES = I915ChipsetPython.la
-I915ChipsetPython_la_CFLAGS = -I$(top_srcdir)/lib $(PYTHON_CPPFLAGS)
-I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS)
-I915ChipsetPython_la_SOURCES = chipset_wrap_python.c intel_chipset.c
+I915ChipsetPython_la_CFLAGS = -I$(top_srcdir)/lib $(PYTHON_CPPFLAGS)
$(CFLAGS) -I/usr/include/libdrm/
+I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS)
-lpciaccess
+I915ChipsetPython_la_SOURCES = chipset_wrap_python.c intel_chipset.c \
+../../lib/intel_drm.c \
+../../lib/intel_pci.c \
+../../lib/intel_reg_map.c \
+../../lib/intel_mmio.c
I should probably $(top_srcdir)/lib these sources. Fixed locally.
chipset_wrap_python.c: chipset.i
- $(SWIG) $(AX_SWIG_PYTHON_OPT) -I$(top_srcdir)/lib -o $@ $
+ $(SWIG) $(AX_SWIG_PYTHON_OPT) -I/usr/include -I$(top_srcdir)/lib -o $@
$
all-local: I915ChipsetPython.la
$(LN_S) -f .libs/I915ChipsetPython.so _chipset.so
diff --git a/tools/quick_dump/chipset.i b/tools/quick_dump/chipset.i
index 16c4932..2f4f5ef 100644
--- a/tools/quick_dump/chipset.i
+++ b/tools/quick_dump/chipset.i
@@ -1,12 +1,24 @@
-%module chipset
+%module chipset
+%include stdint.i
%{
+#include pciaccess.h
+#include stdint.h
#include intel_chipset.h
extern int is_sandybridge(unsigned short pciid);
extern int is_ivybridge(unsigned short pciid);
extern int is_valleyview(unsigned short pciid);
+extern struct pci_device *intel_get_pci_device();
+extern int intel_register_access_init(struct pci_device *pci_dev, int safe);
+extern uint32_t intel_register_read(uint32_t reg);
+extern void intel_register_access_fini();
+extern unsigned short pcidev_to_devid(struct pci_device *pci_dev);
%}
-%include intel_chipset.h
extern int is_sandybridge(unsigned short pciid);
extern int is_ivybridge(unsigned short pciid);
extern int is_valleyview(unsigned short pciid);
+extern struct pci_device *intel_get_pci_device();
+extern int intel_register_access_init(struct pci_device *pci_dev, int safe);
+extern uint32_t intel_register_read(uint32_t reg);
+extern void intel_register_access_fini();
+extern unsigned short pcidev_to_devid(struct pci_device *pci_dev);
diff --git a/tools/quick_dump/intel_chipset.c
b/tools/quick_dump/intel_chipset.c
index b242ffc..d6e7f91 100644
--- a/tools/quick_dump/intel_chipset.c
+++ b/tools/quick_dump/intel_chipset.c
@@ -1,3 +1,4 @@
+#include pciaccess.h
#include intel_chipset.h
int is_sandybridge(unsigned short pciid)
@@ -14,3 +15,9 @@ int is_valleyview(unsigned short pciid)
{
return IS_VALLEYVIEW(pciid);
}
+
+/* Simple helper because I couldn't make this work in the script */
+unsigned short pcidev_to_devid(struct pci_device *pdev)
+{
+ return pdev-device_id;
+}
diff --git a/tools/quick_dump/quick_dump.py b/tools/quick_dump/quick_dump.py
index 59cae1f..44aa2ba 100755
--- a/tools/quick_dump/quick_dump.py
+++ b/tools/quick_dump/quick_dump.py
@@ -32,9 +32,8 @@ if args.baseless == False:
parse_file(file)
if args.autodetect:
- sysfs_file = open('/sys/class/drm/card0/device/device', 'r')
- devid_str = sysfs_file.read()
- devid = int(devid_str, 16)
+ pci_dev = chipset.intel_get_pci_device()
+ devid = chipset.pcidev_to_devid(pci_dev)
if chipset.is_sandybridge(devid):
args.profile = open('sandybridge', 'r')
elif chipset.is_ivybridge(devid):
diff --git a/tools/quick_dump/reg_access.py b/tools/quick_dump/reg_access.py
new file mode 100755
index 000..0f63424
--- /dev/null
+++ b/tools/quick_dump/reg_access.py
@@ -0,0 +1,25 @@
+#!/usr/bin/env python3
+import chipset
+
+def read(reg):
+ reg = int(reg, 16)
+ val = chipset.intel_register_read(reg)
+ return val
+
+def init():
+ pci_dev = chipset.intel_get_pci_device()
+ ret = chipset.intel_register_access_init(pci_dev, 0)
+ if ret != 0:
+
Re: [Intel-gfx] Intel-gfx Digest, Vol 61, Issue 10
Can you remove me from this mailing list? Thanks, Florence -Original Message- From: intel-gfx-bounces+florence.geisler=intel@lists.freedesktop.org [mailto:intel-gfx-bounces+florence.geisler=intel@lists.freedesktop.org] On Behalf Of intel-gfx-requ...@lists.freedesktop.org Sent: Sunday, February 03, 2013 12:01 PM To: intel-gfx@lists.freedesktop.org Subject: Intel-gfx Digest, Vol 61, Issue 10 Send Intel-gfx mailing list submissions to intel-gfx@lists.freedesktop.org To subscribe or unsubscribe via the World Wide Web, visit http://lists.freedesktop.org/mailman/listinfo/intel-gfx or, via email, send a message with subject or body 'help' to intel-gfx-requ...@lists.freedesktop.org You can reach the person managing the list at intel-gfx-ow...@lists.freedesktop.org When replying, please edit your Subject line so it is more specific than Re: Contents of Intel-gfx digest... Today's Topics: 1. Re: [PATCH] PM: make VT switching to the suspend console optional v2 (Rafael J. Wysocki) 2. Re: Advanced Deinterlacers (Jochen Heuer) 3. Re: [PATCH 00/10] [RFC v2] quick dump (Ben Widawsky) 4. Re: [PATCH 09/10] quick_dump: Connect libpciaccess andother utils (Ben Widawsky) -- Message: 1 Date: Sun, 03 Feb 2013 13:59:49 +0100 From: Rafael J. Wysocki r...@sisk.pl To: Jesse Barnes jbar...@virtuousgeek.org Cc: intel-gfx@lists.freedesktop.org, linux-ker...@vger.kernel.org, Linux PM list linux...@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] PM: make VT switching to the suspend console optional v2 Message-ID: 2642200.vlpem05...@vostro.rjw.lan Content-Type: text/plain; charset=utf-8 On Sunday, February 03, 2013 09:56:09 AM Jesse Barnes wrote: On Sat, 02 Feb 2013 21:50:35 +0100 Rafael J. Wysocki r...@sisk.pl wrote: + * Drivers can indicate support for switchless suspend/resume, + which can + * save time and flicker, by using this routine and passing + 'false' as + * the argument. If any loaded driver needs VT switching, or + the + * no_console_suspend argument has been passed on the command + line, VT + * switches will occur. + */ It seems to me that we'll need a separate counter for the number of registered drivers and do the switch if that number is equal to the number of drivers that have passed false to this thing. In which case we can simplify this slightly and introduce pm_vt_swtich_not_required(void) Sorry, that won't be sufficient. Rather something like pm_vt_switch_get() (indicating I'll do the switch, thanks) and pm_vt_switch_put() (indicating now you need to do the switch yourself). I thought of both of your approaches before posting this one, but each have other problems. And I found a bug in mine last night. So I think I need a separate count of drivers that need the switch, and ones that don't. Then if either no driver has registered or if the need_switch count is nonzero, we'll do the switch. Otherwise, if the dont_need_switch is nonzero, we can avoid the switch. I think that'll handle all the cases I outlined. Yes, it should cover them all I think. My code as posted will fail if one driver needs a switch but then two switch free drivers register. I see. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- Message: 2 Date: Sun, 3 Feb 2013 15:37:45 +0100 From: Jochen Heuer jogi-intel-...@planetzork.ping.de To: Mario Schulz t...@arcor.de Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] Advanced Deinterlacers Message-ID: 20130203143745.ga8...@planetzork.ping.de Content-Type: text/plain; charset=us-ascii Hello, As far as I have seen you have successfully implementeted the API technology to select interlacers in the video pipeline as video postprocessor since middle of last year. The VDR community is still waiting for MotionAdaptive or MotionCompensated. Could you comment on your schedule for that? - Is it for some reason technologically impossible to get it implemented? - Are the plans for IVB or SBR to make it available? I am very interested in this also since I probably have to replace my old VDR in the near future and I need to know if I have to go the NVidia route because of working VDPAU support for advanced deinterlacers (although I would not like it since it's closed source) or if I can expect working advanced deinterlacers in the near future in the Intel stack. Because I would prefer to go the Intel route instead. Thanks and best regards, Jogi -- Message: 3 Date: Sun, 3 Feb 2013 10:13:10 -0800 From: Ben Widawsky b...@bwidawsk.net To: Chris Wilson ch...@chris-wilson.co.uk,Jesse Barnes jbar...@virtuousgeek.org, intel-gfx@lists.freedesktop.org,
Re: [Intel-gfx] [RFC PATCH 3/3] i915: ignore lid open event when resuming
On Tue, 2013-01-29 at 11:10 +0100, Daniel Vetter wrote: On Mon, Jan 28, 2013 at 06:06:38PM +0800, Zhang Rui wrote: On Mon, 2013-01-28 at 09:31 +0100, Daniel Vetter wrote: On Mon, Jan 28, 2013 at 3:36 AM, Zhang Rui rui.zh...@intel.com wrote: Given that this essentially requires users to manually set this module option to make stuff work I don't like this. sorry, I do not understand. this patch just disables modeset_on_lid during suspend. Pardon me, the misunderstanding is on my side - I've mixed up dev_priv-modeset_on_lid with the corresponding module option. Is my understanding correct that only with the new SCI pm state this can happen, since that still allows acpi events to be processed (and so our lid_notifier being called? yep. I see a few possible options: - plug the races between all the different parts - I've never really understood why acpi sends us events before the resume code has completed ... If that's indeed intentional, we could delay the handling a bit until at least the i915 resume code completed. IMO, the real question is that, during a suspend/resume cycle, if we need to take care of the lid open event or not. To me, the answer is no, because when resuming from STR, i915 is not aware of such an event, and the i915 resume code works well, right? so I do not think it is a problem to ignore the lid event for another lightweight suspend state, which is introduced in Patch 1/3 in this series. - Judging from the diff context you're not on the latest 3.8-rc codebase - we've applied a few fixes to this lid hack recently. Please retest on a kernel with the patch is based on 3.8.0-rc3, which already includes the commit below. And yes, the problem still exists. Ok, I think I see the issue now. But I suspect that we need some additional locking to make this solid, since currently dev_priv-modeset_on_lid updates can race with our lid notifier handler. Let me think about this a bit more. hmm, with this patch, intel_lid_notify() will return immediately if modeset_on_lid is set to -1. So the only problem in my mind is that we got a lid open event during i915_suspend(). Say, lid is opened - i915_lid_notify() is invoked (modeset_on_lid is 1 at this time) - i915_suspend() set modeset_on_lid to -1, just before intel_modeset_setup_hw_state() being invoked in i915_lid_notify() - intel_modeset_setup_hw_state() breaks the system. but my first question would be is this (lid open on suspend) possible in real world? If the answer is yes, then my second question is that, this problem exists for STR as well because SCI is still valid at this time when suspending to memory, have we seen such issues for STR before? Anyway, if the current code does not break STR, this patch should be enough for the new suspend state. what do you think? I think I have two wishlist changes for your patch ;-) - Convert dev_priv-modeset_on_lid to an enum, so that we have descriptive names instead of magic numbers. sure, will update in next version. - I think we can close all races by adding a new lid_notifier mutex (I prefer a new lock instead of overloading an existing one with). If we hold that in the suspend/resume paths just for changing modeset_on_lid and also for the entire lid notifier callback (i.e. grab the lock before first looking at modest_on_lid, only drop it once the modeset fixup has been completed at the end of the function) we should be race-free in all cases. Also, I think we should move the modeset_on_lid updates in the suspend/resume paths to the very beginning/end. Can you pls update your patch with these changes? Or do you see an additional race we need to plug somewhere? yep. will send out V2 soon. thanks for your comments. -rui ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE
PM_SUSPEND_FREEZE state is a general state that
does not need any platform specific support, it equals
frozen processes + suspended devices + idle processors.
Compared with PM_SUSPEND_MEMORY,
PM_SUSPEND_FREEZE saves less power
because the system is still in a running state.
PM_SUSPEND_FREEZE has less resume latency because it does not
touch BIOS, and the processors are in idle state.
Compared with RTPM/idle,
PM_SUSPEND_FREEZE saves more power as
1. the processor has longer sleep time because processes are frozen.
The deeper c-state the processor supports, more power saving we can get.
2. PM_SUSPEND_FREEZE uses system suspend code path, thus we can get
more power saving from the devices that does not have good RTPM support.
This state is useful for
1) platforms that do not have STR, or have a broken STR.
2) platforms that have an extremely low power idle state,
which can be used to replace STR.
The following describes how PM_SUSPEND_FREEZE state works.
1. echo freeze /sys/power/state
2. the processes are frozen.
3. all the devices are suspended.
4. all the processors are blocked by a wait queue
5. all the processors idles and enters (Deep) c-state.
6. an interrupt fires.
7. a processor is woken up and handles the irq.
8. if it is a general event,
a) the irq handler runs and quites.
b) goto step 4.
9. if it is a real wake event, say, power button pressing, keyboard touch,
mouse moving,
a) the irq handler runs and activate the wakeup source
b) wakeup_source_activate() notifies the wait queue.
c) system starts resuming from PM_SUSPEND_FREEZE
10. all the devices are resumed.
11. all the processes are unfrozen.
12. system is back to working.
Known Issue:
The wakeup of this new PM_SUSPEND_FREEZE state may behave differently
from the previous suspend state.
Take ACPI platform for example, there are some GPEs that only enabled
when the system is in sleep state, to wake the system backk from S3/S4.
But we are not touching these GPEs during transition to PM_SUSPEND_FREEZE.
This means we may lose some wake event.
But on the other hand, as we do not disable all the Interrupts during
PM_SUSPEND_FREEZE, we may get some extra wakeup Interrupts, that are
not available for S3/S4.
The patches has been tested on an old Sony laptop, and here are the results:
Average Power:
1. RPTM/idle for half an hour:
14.8W, 12.6W, 14.1W, 12.5W, 14.4W, 13.2W, 12.9W
2. Freeze for half an hour:
11W, 10.4W, 9.4W, 11.3W 10.5W
3. RTPM/idle for three hours:
11.6W
4. Freeze for three hours:
10W
5. Suspend to Memory:
0.5~0.9W
Average Resume Latency:
1. RTPM/idle with a black screen: (From pressing keyboard to screen back)
Less than 0.2s
2. Freeze: (From pressing power button to screen back)
2.50s
3. Suspend to Memory: (From pressing power button to screen back)
4.33s
From the results, we can see that all the platforms should benefit from
this patch, even if it does not have Low Power S0.
Signed-off-by: Zhang Rui rui.zh...@intel.com
---
drivers/base/power/wakeup.c |6
include/linux/suspend.h |5 +++-
kernel/power/main.c |2 +-
kernel/power/suspend.c | 69 +++
4 files changed, 67 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index e6ee5e8..79715e7 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -382,6 +382,12 @@ static void wakeup_source_activate(struct wakeup_source
*ws)
{
unsigned int cec;
+ /*
+* active wakeup source should bring the system
+* out of PM_SUSPEND_FREEZE state
+*/
+ freeze_wake();
+
ws-active = true;
ws-active_count++;
ws-last_time = ktime_get();
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0c808d7..7420ab5 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -34,8 +34,10 @@ static inline void pm_restore_console(void)
typedef int __bitwise suspend_state_t;
#define PM_SUSPEND_ON ((__force suspend_state_t) 0)
-#define PM_SUSPEND_STANDBY ((__force suspend_state_t) 1)
+#define PM_SUSPEND_FREEZE ((__force suspend_state_t) 1)
+#define PM_SUSPEND_STANDBY ((__force suspend_state_t) 2)
#define PM_SUSPEND_MEM ((__force suspend_state_t) 3)
+#define PM_SUSPEND_MIN PM_SUSPEND_FREEZE
#define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
enum suspend_stat_step {
@@ -192,6 +194,7 @@ struct platform_suspend_ops {
*/
extern void suspend_set_ops(const struct platform_suspend_ops *ops);
extern int suspend_valid_only_mem(suspend_state_t state);
+extern void freeze_wake(void);
/**
* arch_suspend_disable_irqs - disable IRQs for suspend
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 1c16f91..b1c26a9 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -313,7 +313,7 @@ static ssize_t state_show(struct kobject *kobj, struct
kobj_attribute
[Intel-gfx] [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend
Enable ACPI SCI during suspend so that SCI can be used
as wake events for PM_SUSPEND_FREEZE.
For S3/S4 transition,
We disable all GPEs in suspend_ops-prepare_late() to
fix a problem that GPEs may trigger SCI before
arch_suspend_disable_irqs() is run.
So it is safe to leave the SCI enabled until
arch_suspend_irq_disable() is run.
Signed-off-by: Zhang Rui rui.zh...@intel.com
---
drivers/acpi/osl.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3ff2678..3adeb10 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -787,7 +787,7 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler
handler,
acpi_irq_handler = handler;
acpi_irq_context = context;
- if (request_irq(irq, acpi_irq, IRQF_SHARED, acpi, acpi_irq)) {
+ if (request_irq(irq, acpi_irq, IRQF_SHARED | IRQF_NO_SUSPEND, acpi,
acpi_irq)) {
printk(KERN_ERR PREFIX SCI (IRQ%d) allocation failed\n, irq);
acpi_irq_handler = NULL;
return AE_NOT_ACQUIRED;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
i915 driver needs to do modeset when 1. system resumes from sleep 2. lid is opened In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes, thus it is the i915_resume code does the modeset rather than intel_lid_notify(). But in PM_SUSPEND_FREEZE state, this will be broken because system is still responsive to the lid events. 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid. 2. When we reopen the lid, intel_lid_notify() will do a modeset, before the system is resumed. here is the error log, [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]() [92146.548076] Hardware name: VGN-Z540N [92146.548078] pipe_off wait timed out [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: GW 3.8.0-rc3-s0i3-v3-test+ #9 [92146.548175] Call Trace: [92146.548189] [c10378e2] warn_slowpath_common+0x72/0xa0 [92146.548227] [f86398b4] ? intel_wait_for_pipe_off+0x184/0x190 [i915] [92146.548263] [f86398b4] ? intel_wait_for_pipe_off+0x184/0x190 [i915] [92146.548270] [c10379b3] warn_slowpath_fmt+0x33/0x40 [92146.548307] [f86398b4] intel_wait_for_pipe_off+0x184/0x190 [i915] [92146.548344] [f86399c2] intel_disable_pipe+0x102/0x190 [i915] [92146.548380] [f8639ea4] ? intel_disable_plane+0x64/0x80 [i915] [92146.548417] [f8639f7c] i9xx_crtc_disable+0xbc/0x150 [i915] [92146.548456] [f863ebee] intel_crtc_update_dpms+0x5e/0x90 [i915] [92146.548493] [f86437cf] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915] [92146.548535] [f8645b0b] intel_lid_notify+0x9b/0xc0 [i915] [92146.548543] [c15610d3] notifier_call_chain+0x43/0x60 [92146.548550] [c105d1e1] __blocking_notifier_call_chain+0x41/0x80 [92146.548556] [c105d23f] blocking_notifier_call_chain+0x1f/0x30 [92146.548563] [c131a684] acpi_lid_send_state+0x78/0xa4 [92146.548569] [c131aa9e] acpi_button_notify+0x3b/0xf1 [92146.548577] [c12df56a] ? acpi_os_execute+0x17/0x19 [92146.548582] [c12e591a] ? acpi_ec_sync_query+0xa5/0xbc [92146.548589] [c12e2b82] acpi_device_notify+0x16/0x18 [92146.548595] [c12f4904] acpi_ev_notify_dispatch+0x38/0x4f [92146.548600] [c12df0e8] acpi_os_execute_deferred+0x20/0x2b [92146.548607] [c1051208] process_one_work+0x128/0x3f0 [92146.548613] [c1564f73] ? common_interrupt+0x33/0x38 [92146.548618] [c104f8c0] ? wake_up_worker+0x30/0x30 [92146.548624] [c12df0c8] ? acpi_os_wait_events_complete+0x1e/0x1e [92146.548629] [c10524f9] worker_thread+0x119/0x3b0 [92146.548634] [c10523e0] ? manage_workers+0x240/0x240 [92146.548640] [c1056e84] kthread+0x94/0xa0 [92146.548647] [c106] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0 [92146.548652] [c15649b7] ret_from_kernel_thread+0x1b/0x28 [92146.548658] [c1056df0] ? kthread_create_on_node+0xc0/0xc0 three different modeset flags are introduced in this patch MODESET_ON_LID: do modeset on next lid open event MODESET_DONE: modeset already done MODESET_ON_RESUME: do modeset when system is resumed In this way, 1. when lid is closed, MODESET_ON_LID is set so that we'll do modeset on next lid open event. 2. when lid is opened, MODESET_DONE is set so that duplicate lid open events will be ignored. 3. when system suspends, MODESET_ON_RESUME is set. In this case, we will not do modeset on any lid events. Plus, locking mechanism is also introduced to avoid racing. Signed-off-by: Zhang Rui rui.zh...@intel.com --- drivers/gpu/drm/i915/i915_dma.c |1 + drivers/gpu/drm/i915/i915_drv.c | 14 +- drivers/gpu/drm/i915/i915_drv.h | 11 +-- drivers/gpu/drm/i915/intel_lvds.c | 33 - 4 files changed, 39 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 99daa89..c7cb546 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) spin_lock_init(dev_priv-dpio_lock); mutex_init(dev_priv-rps.hw_lock); + mutex_init(dev_priv-modeset_lock); if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) dev_priv-num_pipe = 3; diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index
