Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup
On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote:
>
>
> Den 24.12.2018 00.10, skrev Peter Wu:
> > On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 23.12.2018 01.55, skrev Peter Wu:
> > > > After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
> > > > "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
> > > > have some effect). After that, drm_fb_helper_initial_config is called
> > > > which may call the "fb_probe" driver callback.
> > > >
> > > > This driver callback may call drm_fb_helper_defio_init (as is done by
> > > > drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
> > > > as documented. These are normally cleaned up on exit by
> > > > drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.
> > > >
> > > > If an error occurs after "fb_probe", but before setup is complete, then
> > > > calling just drm_fb_helper_fini will leak resources. This was triggered
> > > > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):
> > > >
> > > > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to
> > > > support this framebuffer
> > > > [ 50.009436] bochs-drm :00:02.0:
> > > > [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set
> > > > configuration (ret=-38)
> > > > [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for
> > > > :00:02.0 on minor 2
> > > > [ 50.013604] WARNING: CPU: 1 PID: 1 at
> > > > drivers/gpu/drm/drm_mode_config.c:477
> > > > drm_mode_config_cleanup+0x280/0x2a0
> > > > [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G
> > > > T 4.20.0-rc7 #1
> > > > [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
> > > > ...
> > > > [ 50.023155] Call Trace:
> > > > [ 50.023155] ? bochs_kms_fini+0x1e/0x30
> > > > [ 50.023155] ? bochs_unload+0x18/0x40
> > > >
> > > > This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.
> > > >
> > > > Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
> > > > Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
> > > > Fixes: 8741216396b2 ("drm/fb-helper: Add
> > > > drm_fb_helper_fbdev_setup/teardown()")
> > > > Reported-by: kernel test robot
> > > > Cc: Noralf Trønnes
> > > > Signed-off-by: Peter Wu
> > > > ---
> > > >drivers/gpu/drm/drm_fb_helper.c | 2 +-
> > > >1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > > > b/drivers/gpu/drm/drm_fb_helper.c
> > > > index 9d64f874f965..432e0f3b9267 100644
> > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device
> > > > *dev,
> > > > return 0;
> > > >err_drm_fb_helper_fini:
> > > > - drm_fb_helper_fini(fb_helper);
> > > > + drm_fb_helper_fbdev_teardown(dev);
> > >
> > > This change will break the error path for drm_fbdev_generic_setup()
> > > because drm_fb_helper_generic_probe() cleans up on error but doesn't
> > > clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove().
> >
> > This should probably considered a bug of drm_fb_helper_generic_probe.
> > Ownership of fb_helper should remain with the caller. The caller can
> > detect an error and act accordingly.
> >
> > > My assumption has been that the drm_fb_helper_funcs->fb_probe callback
> > > cleans up its resources on error. Clearly this is not the case for bochs,
> > > so
> > > my take on this is that bochsfb_create() needs to clean up on error.
> >
> > That assumption still holds for bochs. The problem is this sequence:
> > - drm_fb_helper_fbdev_setup is called.
> > - fb_probe succeeds (this is crucial).
> > - register_framebuffer fails.
> > - error path of setup is triggered.
> >
> > As fb_helper is fully setup by drivers, the drm_fb_helper core should
> > fully deallocate it again on the error path or else a leak occurs.
> >
Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup
On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote:
>
>
> Den 23.12.2018 01.55, skrev Peter Wu:
> > After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
> > "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
> > have some effect). After that, drm_fb_helper_initial_config is called
> > which may call the "fb_probe" driver callback.
> >
> > This driver callback may call drm_fb_helper_defio_init (as is done by
> > drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
> > as documented. These are normally cleaned up on exit by
> > drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.
> >
> > If an error occurs after "fb_probe", but before setup is complete, then
> > calling just drm_fb_helper_fini will leak resources. This was triggered
> > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):
> >
> > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support
> > this framebuffer
> > [ 50.009436] bochs-drm :00:02.0: [drm:drm_fb_helper_fbdev_setup]
> > *ERROR* fbdev: Failed to set configuration (ret=-38)
> > [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for
> > :00:02.0 on minor 2
> > [ 50.013604] WARNING: CPU: 1 PID: 1 at
> > drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0
> > [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G
> > T 4.20.0-rc7 #1
> > [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
> > ...
> > [ 50.023155] Call Trace:
> > [ 50.023155] ? bochs_kms_fini+0x1e/0x30
> > [ 50.023155] ? bochs_unload+0x18/0x40
> >
> > This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.
> >
> > Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
> > Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
> > Fixes: 8741216396b2 ("drm/fb-helper: Add
> > drm_fb_helper_fbdev_setup/teardown()")
> > Reported-by: kernel test robot
> > Cc: Noralf Trønnes
> > Signed-off-by: Peter Wu
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index 9d64f874f965..432e0f3b9267 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev,
> > return 0;
> > err_drm_fb_helper_fini:
> > - drm_fb_helper_fini(fb_helper);
> > + drm_fb_helper_fbdev_teardown(dev);
>
> This change will break the error path for drm_fbdev_generic_setup()
> because drm_fb_helper_generic_probe() cleans up on error but doesn't
> clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove().
This should probably considered a bug of drm_fb_helper_generic_probe.
Ownership of fb_helper should remain with the caller. The caller can
detect an error and act accordingly.
> My assumption has been that the drm_fb_helper_funcs->fb_probe callback
> cleans up its resources on error. Clearly this is not the case for bochs, so
> my take on this is that bochsfb_create() needs to clean up on error.
That assumption still holds for bochs. The problem is this sequence:
- drm_fb_helper_fbdev_setup is called.
- fb_probe succeeds (this is crucial).
- register_framebuffer fails.
- error path of setup is triggered.
As fb_helper is fully setup by drivers, the drm_fb_helper core should
fully deallocate it again on the error path or else a leak occurs.
> Gerd has a patchset that switches bochs over to the generic fbdev
> emulation, but ofc that doesn't help with 4.20:
> https://patchwork.freedesktop.org/series/54269/
And that does not help with other users of the drm_fb_helper who use
functions like drm_fb_helper_defio_init. They will likely run in the
same problem.
I don't have a way to test tinydrm or other drivers, but if you force
register_framebuffer to fail, you should be able to reproduce the
problem with drm_fb_helper_generic_probe.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup
info->par = >fb.helper;
fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL);
if (IS_ERR(fb)) {
DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb));
return PTR_ERR(fb);
}
/* setup helper */
bochs->fb.helper.fb = fb;
Note that "fb" is unhandled by "drm_fb_helper_fini", so it leaks.
What is the usual behavior? drm_fb_helper_fbdev_setup succeeds and on unload
drm_fb_helper_fbdev_teardown is called which properly releases "fb":
void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
{
struct drm_fb_helper *fb_helper = dev->fb_helper;
struct fb_ops *fbops = NULL;
if (!fb_helper)
return;
/* Unregister if it hasn't been done already */
if (fb_helper->fbdev && fb_helper->fbdev->dev)
drm_fb_helper_unregister_fbi(fb_helper);
if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) {
fb_deferred_io_cleanup(fb_helper->fbdev);
kfree(fb_helper->fbdev->fbdefio);
fbops = fb_helper->fbdev->fbops;
}
drm_fb_helper_fini(fb_helper);
kfree(fbops);
if (fb_helper->fb)
drm_framebuffer_remove(fb_helper->fb); // yay!
}
Due to calling "drm_fb_helper_fini" however, "dev->fb_helper" will be NULL and
thus this function does nothing on the error path.
So in summary, "drm_fb_helper_fbdev_setup" calls the driver callback
drm_fb_helper_funcs::fb_probe, detects an error but does not properly release
all resources from the callback even after calling "drm_fb_helper_fini". On
unload, "drm_fb_helper_fbdev_teardown" has no effect because the earlier call to
"drm_fb_helper_fini" and skips the required "drm_framebuffer_remove" call.
I'll send a proposed patch in a reply.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup
After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
"dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
have some effect). After that, drm_fb_helper_initial_config is called
which may call the "fb_probe" driver callback.
This driver callback may call drm_fb_helper_defio_init (as is done by
drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
as documented. These are normally cleaned up on exit by
drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.
If an error occurs after "fb_probe", but before setup is complete, then
calling just drm_fb_helper_fini will leak resources. This was triggered
by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):
[ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this
framebuffer
[ 50.009436] bochs-drm :00:02.0: [drm:drm_fb_helper_fbdev_setup]
*ERROR* fbdev: Failed to set configuration (ret=-38)
[ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for :00:02.0
on minor 2
[ 50.013604] WARNING: CPU: 1 PID: 1 at
drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0
[ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: GT
4.20.0-rc7 #1
[ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
...
[ 50.023155] Call Trace:
[ 50.023155] ? bochs_kms_fini+0x1e/0x30
[ 50.023155] ? bochs_unload+0x18/0x40
This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.
Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()")
Reported-by: kernel test robot
Cc: Noralf Trønnes
Signed-off-by: Peter Wu
---
drivers/gpu/drm/drm_fb_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9d64f874f965..432e0f3b9267 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev,
return 0;
err_drm_fb_helper_fini:
- drm_fb_helper_fini(fb_helper);
+ drm_fb_helper_fbdev_teardown(dev);
return ret;
}
--
2.20.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] qxl: fix null-pointer crash during suspend
On Mon, Oct 01, 2018 at 01:13:59PM -0700, Fubo Chen wrote: > On Tue, Sep 4, 2018 at 2:10 PM Peter Wu wrote: > > > > "crtc->helper_private" is not initialized by the QXL driver and thus the > > "crtc_funcs->disable" call would crash (resulting in suspend failure). > > Fix this by converting the suspend/resume functions to use the > > drm_mode_config_helper_* helpers. > > > > Tested system sleep with QEMU 3.0 using "echo mem > /sys/power/state". > > During suspend the following message is visible from QEMU: > > > > spice/server/display-channel.c:2425:display_channel_validate_surface: > > canvas address is 0x7fd05da68308 for 0 (and is NULL) > > spice/server/display-channel.c:2426:display_channel_validate_surface: > > failed on 0 > > > > This seems to be triggered by QXL_IO_NOTIFY_CMD after > > QXL_IO_DESTROY_PRIMARY_ASYNC, but aside from the warning things still > > seem to work (tested with both the GTK and -spice options). > > > > Signed-off-by: Peter Wu > > Is this a new issue or something that was introduced a long time ago? > In the latter case, please consider adding a "Cc: > " tag to this patch. I am not sure exactly when the issue was introduced, but the original code was added in v3.10-rc7-800-gd84300bf7934 while the new drm_mode_config_helper_suspend API was added in 4.16. The intended call chain to initialize the private object seems to be: drm_crtc_helper_add <- qdev_crtc_init <- qxl_modeset_init <- qxl_pci_probe If any error occurs along the callchain, then the helper_private pointer will remain NULL. Or if the crtc is obtained in a different way (not sure how). Not sure if it is worth backporting, suspend/resume does not seem an important use case for VMs using QXL and the fix was not validated for older kernels. -- Kind regards, Peter Wu https://lekensteyn.nl ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] qxl: refactor to use drm_fb_helper_fbdev_setup
Lots of code can be removed by relying on fb-helper:
- "struct drm_framebuffer" moves to fb_helper.fb.
- "struct drm_gem_object" moves to fb_helper.obj[0].
- "struct qxl_device" can be inferred as drm_fb_helper is embedded.
- qxl_user_framebuffer_create -> drm_gem_fb_create.
- qxl_user_framebuffer_destroy -> drm_gem_fb_destroy.
- qxl_fbdev_destroy -> drm_fb_helper_fbdev_teardown + vfree(shadow).
Remove unused code:
- qxl_fbdev_qobj_is_fb, qxl_fbdev_set_suspend.
- Unused fields of qxl_fbdev: delayed_ops, delayed_ops_lock, size.
Misc notes:
- The dirty callback is preserved as it is necessary to trigger update
commands in the hw (the screen stays black otherwise).
- No idea when .create_handle in drm_framebuffer_funcs is used, but use
the same drm_gem_fb_create_handle to match drm_gem_fb_funcs.
- I don't know why qxl_fb_find_or_create_single used to check for an
existing framebuffer and removed that check to match other drivers.
- Use of drm_fb_helper_fbdev_teardown also requires "info->fbdefio" to
be dynamically allocated. Replace the existing defio config by
drm_fb_helper_defio_init to accomodate this.
Testing results: startx with fbdev, modesetting and qxl all seems to
work. Tested also with CONFIG_DRM_FBDEV_EMULATION=n, fbdev obviously
fails but others are fine. QEMU -spice and QEMU -spice with vdagent and
multiple (resized) displays (via remote-viewer) also works.
unbind vtconsole and rmmod has *not* regressed (i.e. it still trips on a
use-after-free in qxl_check_idle via qxl_ttm_fini).
Ideally setup/teardown is replaced by drm_fbdev_generic_setup as that
would result in further code reduction, improve error handling (like not
leaking shadow memory), but unfortunately QXL has no implementation for
qxl_gem_prime_vmap.
Signed-off-by: Peter Wu
---
rmmod/unbind PCI driver is still broken for both the
CONFIG_DRM_FBDEV_EMULATION=y/n cases, but hopefully this cleanup makes it easier
to prepare further fixes (if it is worth it).
---
drivers/gpu/drm/qxl/qxl_display.c | 101 +++
drivers/gpu/drm/qxl/qxl_draw.c| 6 +-
drivers/gpu/drm/qxl/qxl_drv.h | 32 +
drivers/gpu/drm/qxl/qxl_fb.c | 197 +-
4 files changed, 60 insertions(+), 276 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 01704a7f07cb..87d16a0ce01e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include "qxl_drv.h"
#include "qxl_object.h"
@@ -388,17 +389,6 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
};
-void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
-{
- struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
- struct qxl_bo *bo = gem_to_qxl_bo(qxl_fb->obj);
-
- WARN_ON(bo->shadow);
- drm_gem_object_put_unlocked(qxl_fb->obj);
- drm_framebuffer_cleanup(fb);
- kfree(qxl_fb);
-}
-
static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
struct drm_file *file_priv,
unsigned flags, unsigned color,
@@ -406,15 +396,14 @@ static int qxl_framebuffer_surface_dirty(struct
drm_framebuffer *fb,
unsigned num_clips)
{
/* TODO: vmwgfx where this was cribbed from had locking. Why? */
- struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
- struct qxl_device *qdev = qxl_fb->base.dev->dev_private;
+ struct qxl_device *qdev = fb->dev->dev_private;
struct drm_clip_rect norect;
struct qxl_bo *qobj;
int inc = 1;
drm_modeset_lock_all(fb->dev);
- qobj = gem_to_qxl_bo(qxl_fb->obj);
+ qobj = gem_to_qxl_bo(fb->obj[0]);
/* if we aren't primary surface ignore this */
if (!qobj->is_primary) {
drm_modeset_unlock_all(fb->dev);
@@ -432,7 +421,7 @@ static int qxl_framebuffer_surface_dirty(struct
drm_framebuffer *fb,
inc = 2; /* skip source rects */
}
- qxl_draw_dirty_fb(qdev, qxl_fb, qobj, flags, color,
+ qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
clips, num_clips, inc);
drm_modeset_unlock_all(fb->dev);
@@ -441,31 +430,11 @@ static int qxl_framebuffer_surface_dirty(struct
drm_framebuffer *fb,
}
static const struct drm_framebuffer_funcs qxl_fb_funcs = {
- .destroy = qxl_user_framebuffer_destroy,
+ .destroy = drm_gem_fb_destroy,
.dirty = qxl_framebuffer_surface_dirty,
-/* TODO?
- * .create_handle = qxl_user_framebuffer_create_handle, */
+ .create_handle = drm_gem_fb_create_handle,
};
-int
-qxl_framebuffer_init(struct drm_device *dev,
[PATCH v2 1/4] bochs: use drm_fb_helper_set_suspend_unlocked in suspend/resume
The "initialized" member is going away. suspend/resume still works (even
if bochsfb_create is forced to fail).
Signed-off-by: Peter Wu
---
drivers/gpu/drm/bochs/bochs_drv.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
b/drivers/gpu/drm/bochs/bochs_drv.c
index c61b40c72b62..0e79d9acf89e 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -107,11 +107,7 @@ static int bochs_pm_suspend(struct device *dev)
drm_kms_helper_poll_disable(drm_dev);
- if (bochs->fb.initialized) {
- console_lock();
- drm_fb_helper_set_suspend(>fb.helper, 1);
- console_unlock();
- }
+ drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);
return 0;
}
@@ -124,11 +120,7 @@ static int bochs_pm_resume(struct device *dev)
drm_helper_resume_force_mode(drm_dev);
- if (bochs->fb.initialized) {
- console_lock();
- drm_fb_helper_set_suspend(>fb.helper, 0);
- console_unlock();
- }
+ drm_fb_helper_set_suspend_unlocked(>fb.helper, 0);
drm_kms_helper_poll_enable(drm_dev);
return 0;
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings
Clarify the relation between drm_fb_helper_fbdev_setup/teardown. Clarify
requirements for the new generic fbdev emulation API and log some more
details in case the driver does something wrong. Fix related typos.
Cc: Noralf Trønnes
Signed-off-by: Peter Wu
---
drivers/gpu/drm/drm_fb_helper.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b0dd20bccb8..7f92ff173986 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2821,7 +2821,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
* The caller must to provide a _fb_helper_funcs->fb_probe callback
* function.
*
- * See also: drm_fb_helper_initial_config()
+ * Use drm_fb_helper_fbdev_teardown() to destroy the fbdev.
+ *
+ * See also: drm_fb_helper_initial_config(), drm_fbdev_generic_setup().
*
* Returns:
* Zero on success or negative error code on failure.
@@ -3037,7 +3039,7 @@ static struct fb_deferred_io drm_fbdev_defio = {
* @fb_helper: fbdev helper structure
* @sizes: describes fbdev size and scanout surface size
*
- * This function uses the client API to crate a framebuffer backed by a dumb
buffer.
+ * This function uses the client API to create a framebuffer backed by a dumb
buffer.
*
* The _sys_ versions are used for _ops.fb_read, fb_write, fb_fillrect,
* fb_copyarea, fb_imageblit.
@@ -3165,8 +3167,10 @@ static int drm_fbdev_client_hotplug(struct
drm_client_dev *client)
if (dev->fb_helper)
return drm_fb_helper_hotplug_event(dev->fb_helper);
- if (!dev->mode_config.num_connector)
+ if (!dev->mode_config.num_connector) {
+ DRM_DEV_DEBUG(dev->dev, "No connectors found, will not create
framebuffer!\n");
return 0;
+ }
ret = drm_fb_helper_fbdev_setup(dev, fb_helper,
_fb_helper_generic_funcs,
fb_helper->preferred_bpp, 0);
@@ -3187,13 +3191,15 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
};
/**
- * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
+ * drm_fbdev_generic_setup() - Setup generic fbdev emulation
* @dev: DRM device
* @preferred_bpp: Preferred bits per pixel for the device.
* @dev->mode_config.preferred_depth is used if this is zero.
*
* This function sets up generic fbdev emulation for drivers that supports
- * dumb buffers with a virtual address and that can be mmap'ed.
+ * dumb buffers with a virtual address and that can be mmap'ed. If the driver
+ * does not support these functions, it could use drm_fb_helper_fbdev_setup().
+ * This function can only be used with devices created using
drm_dev_register().
*
* Restore, hotplug events and teardown are all taken care of. Drivers that do
* suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
@@ -3206,6 +3212,8 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
* This function is safe to call even when there are no connectors present.
* Setup will be retried on the next hotplug event.
*
+ * The fbdev is destroyed by drm_dev_unregister().
+ *
* Returns:
* Zero on success or negative error code on failure.
*/
@@ -3214,6 +3222,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int preferred_bpp)
struct drm_fb_helper *fb_helper;
int ret;
+ WARN(dev->fb_helper, "fb_helper is already set!\n");
+
if (!drm_fbdev_emulation)
return 0;
@@ -3224,12 +3234,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int preferred_bpp)
ret = drm_client_new(dev, _helper->client, "fbdev",
_fbdev_client_funcs);
if (ret) {
kfree(fb_helper);
+ DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
return ret;
}
fb_helper->preferred_bpp = preferred_bpp;
- drm_fbdev_client_hotplug(_helper->client);
+ ret = drm_fbdev_client_hotplug(_helper->client);
+ if (ret)
+ DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
return 0;
}
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/4] bochs: convert to drm_dev_register
The drm_get_pci_dev API is deprecated, replace it by drm_dev_register.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/bochs/bochs.h | 2 +-
drivers/gpu/drm/bochs/bochs_drv.c | 34 +--
drivers/gpu/drm/bochs/bochs_hw.c | 2 +-
3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 8514a84fbdbe..b4f6bb521900 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -117,7 +117,7 @@ static inline u64 bochs_bo_mmap_offset(struct bochs_bo *bo)
/* -- */
/* bochs_hw.c */
-int bochs_hw_init(struct drm_device *dev, uint32_t flags);
+int bochs_hw_init(struct drm_device *dev);
void bochs_hw_fini(struct drm_device *dev);
void bochs_hw_setmode(struct bochs_device *bochs,
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
b/drivers/gpu/drm/bochs/bochs_drv.c
index 0e79d9acf89e..f3dd66ae990a 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -35,7 +35,7 @@ static void bochs_unload(struct drm_device *dev)
dev->dev_private = NULL;
}
-static int bochs_load(struct drm_device *dev, unsigned long flags)
+static int bochs_load(struct drm_device *dev)
{
struct bochs_device *bochs;
int ret;
@@ -46,7 +46,7 @@ static int bochs_load(struct drm_device *dev, unsigned long
flags)
dev->dev_private = bochs;
bochs->dev = dev;
- ret = bochs_hw_init(dev, flags);
+ ret = bochs_hw_init(dev);
if (ret)
goto err;
@@ -82,8 +82,6 @@ static const struct file_operations bochs_fops = {
static struct drm_driver bochs_driver = {
.driver_features= DRIVER_GEM | DRIVER_MODESET,
- .load = bochs_load,
- .unload = bochs_unload,
.fops = _fops,
.name = "bochs-drm",
.desc = "bochs dispi vga interface (qemu stdvga)",
@@ -138,6 +136,7 @@ static const struct dev_pm_ops bochs_pm_ops = {
static int bochs_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
+ struct drm_device *dev;
unsigned long fbsize;
int ret;
@@ -151,14 +150,37 @@ static int bochs_pci_probe(struct pci_dev *pdev,
if (ret)
return ret;
- return drm_get_pci_dev(pdev, ent, _driver);
+ dev = drm_dev_alloc(_driver, >dev);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ dev->pdev = pdev;
+ pci_set_drvdata(pdev, dev);
+
+ ret = bochs_load(dev);
+ if (ret)
+ goto err_free_dev;
+
+ ret = drm_dev_register(dev, 0);
+ if (ret)
+ goto err_unload;
+
+ return ret;
+
+err_unload:
+ bochs_unload(dev);
+err_free_dev:
+ drm_dev_put(dev);
+ return ret;
}
static void bochs_pci_remove(struct pci_dev *pdev)
{
struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev);
+ drm_dev_unregister(dev);
+ bochs_unload(dev);
+ drm_dev_put(dev);
}
static const struct pci_device_id bochs_pci_tbl[] = {
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index a39b0343c197..16e4f1caccca 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -47,7 +47,7 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16
reg, u16 val)
}
}
-int bochs_hw_init(struct drm_device *dev, uint32_t flags)
+int bochs_hw_init(struct drm_device *dev)
{
struct bochs_device *bochs = dev->dev_private;
struct pci_dev *pdev = dev->pdev;
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/4] bochs: convert to drm_fb_helper_fbdev_setup/teardown
Currently unloading bochs_drm (after unbinding the vtconsole) results in
a warning about a leaked connector:
[drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked!
While investigating a potential fix I noticed that a lot of open-coded
functionality is already implemented elsewhere, so start converting it:
bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl).
bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding
"struct drm_framebuffer" from "struct bochs_framebuffer".
Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create
which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c.
For this to work, the GEM object is moved into "drm_framebuffer". After
that, "bochs_framebuffer" is no longer needed and therefore removed.
Remove the unused "size" and "initialized" fields from fb, the latter is
not necessary as drm_fb_helper_fbdev_teardown can be called even if
bochsfb_create fails. This theory was tested by returning early and
late (just before drm_gem_fbdev_fb_create). Both scenarios fail
gracefully although the latter seems to leak the object from
bochsfb_create_object (not a regression).
Guess on the reason for the encoder leak: drm_framebuffer_cleanup was
previously used, but did not destroy much. drm_fb_helper_fbdev_teardown
is now used and calls drm_framebuffer_remove which does a bit more work.
Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm'
and also with Xorg + fbdev (startx -> xterm). The latter triggered a
warning in ttm_bo_vm_open that existed before, see
https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de
Acked-by: Daniel Vetter
Signed-off-by: Peter Wu
---
drivers/gpu/drm/bochs/bochs.h | 19 ++-
drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++--
drivers/gpu/drm/bochs/bochs_kms.c | 7 +--
drivers/gpu/drm/bochs/bochs_mm.c| 74 ---
4 files changed, 22 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 375bf92cd04f..8514a84fbdbe 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -51,11 +51,6 @@ enum bochs_types {
BOCHS_UNKNOWN,
};
-struct bochs_framebuffer {
- struct drm_framebuffer base;
- struct drm_gem_object *obj;
-};
-
struct bochs_device {
/* hw */
void __iomem *mmio;
@@ -88,15 +83,11 @@ struct bochs_device {
/* fbdev */
struct {
- struct bochs_framebuffer gfb;
+ struct drm_framebuffer *fb;
struct drm_fb_helper helper;
- int size;
- bool initialized;
} fb;
};
-#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, base)
-
struct bochs_bo {
struct ttm_buffer_object bo;
struct ttm_placement placement;
@@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct
drm_device *dev,
int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
uint32_t handle, uint64_t *offset);
-int bochs_framebuffer_init(struct drm_device *dev,
- struct bochs_framebuffer *gfb,
- const struct drm_mode_fb_cmd2 *mode_cmd,
- struct drm_gem_object *obj);
int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr);
int bochs_bo_unpin(struct bochs_bo *bo);
-extern const struct drm_mode_config_funcs bochs_mode_funcs;
-
/* bochs_kms.c */
int bochs_kms_init(struct bochs_device *bochs);
void bochs_kms_fini(struct bochs_device *bochs);
@@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs);
/* bochs_fbdev.c */
int bochs_fbdev_init(struct bochs_device *bochs);
void bochs_fbdev_fini(struct bochs_device *bochs);
+
+extern const struct drm_mode_config_funcs bochs_mode_funcs;
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c
b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 14eb8d0d5a00..8f4d6c052f7b 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -6,6 +6,7 @@
*/
#include "bochs.h"
+#include
/* -- */
@@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info,
struct vm_area_struct *vma)
{
struct drm_fb_helper *fb_helper = info->par;
- struct bochs_device *bochs =
- container_of(fb_helper, struct bochs_device, fb.helper);
- struct bochs_bo *bo = gem_to_bochs_bo(bochs->fb.gfb.obj);
+ struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
return ttm_fbdev_mmap(vma, >bo);
}
@@ -101,19 +100,20 @@ static int bochsfb_create(struct drm_fb_helper *helper,
/* init fb device */
info = drm_fb_helper_alloc_fbi
[PATCH v2 0/4] bochs fixes and fb-helper documentation updates
Hi,
These series tries to improve the bochs driver and update documentation based on
my brief experience with the fb-helper API. Thank you Daniel and Gerd for your
previous feedback and helpful suggestions.
Patch 2 was previously posted and is unmodified except for acked-by + rebase on
drm-misc-next (9a09a42369a4a37a959c051d8e1a1f948c1529a4). Patch 1 is a trivial
(build) fix that was missing last time.
Patch 3 converts from the legacy API to the modern drm_dev_register approach.
This seems required for the "generic" fbdev API as suggested by Daniel, but as
bochs does not implement the required "gem_prime_vmap" function, the conversion
cannot be completed for now.
Patch 4 includes some documentation updates that would have helped me during
qxl/bochs development and a warning that made me realize that "a virtual address
and that can be mmap'ed" in the documentation referred to "gem_prime_vmap". It
is to my best of understanding, so please correct me if I am wrong.
Side note: I originally tried to fix the unbind/remove crash in QXL and then
turned to bochs as it seemed simpler to learn how to work on DRM drivers.
Hopefully I manage to eventually figure out how to fix QXL. QXL is a bit
strange, it advertises PRIME "support" but it only has stub functions (including
a stub gem_prime_vmap).
After my recent patch ("qxl: fix null-pointer crash during suspend") qxl can
suspend/resume in the normal situation, but doing anything (suspend or unload)
after unbinding just fails (suspend then fails with "failed to wait on release
23 after spincount 301", unload triggers a use-after-free according to KASAN).
On the other hand, bochs passes these tests:
# 1. s/r with unbound console
modprobe bochs_drm
echo 0 > /sys/class/vtconsole/vtcon1/bind
rtcwake -s 1 -m mem
# 2. s/r in normal sitation
echo 1 > /sys/class/vtconsole/vtcon1/bind
rtcwake -s 1 -m mem
# 3. unload module (and s/r for good measure)
echo 0 > /sys/class/vtconsole/vtcon1/bind
rmmod bochs_drm
rtcwake -s 1 -m mem
Kind regards,
Peter
Peter Wu (4):
bochs: use drm_fb_helper_set_suspend_unlocked in suspend/resume
bochs: convert to drm_fb_helper_fbdev_setup/teardown
bochs: convert to drm_dev_register
drm/fb-helper: improve documentation and print warnings
drivers/gpu/drm/bochs/bochs.h | 21 ++--
drivers/gpu/drm/bochs/bochs_drv.c | 46 +++--
drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++--
drivers/gpu/drm/bochs/bochs_hw.c| 2 +-
drivers/gpu/drm/bochs/bochs_kms.c | 7 +--
drivers/gpu/drm/bochs/bochs_mm.c| 74 ---
drivers/gpu/drm/drm_fb_helper.c | 25 ++---
7 files changed, 73 insertions(+), 181 deletions(-)
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
On Wed, Sep 05, 2018 at 04:46:29PM +0200, Daniel Vetter wrote:
> On Wed, Sep 05, 2018 at 04:41:27PM +0200, Peter Wu wrote:
> > Currently unloading bochs_drm (after unbinding the vtconsole) results in
> > a warning about a leaked connector:
> >
> > [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked!
> >
> > While investigating a potential fix I noticed that a lot of open-coded
> > functionality is already implemented elsewhere, so start converting it:
> > bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl).
> > bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding
> > "struct drm_framebuffer" from "struct bochs_framebuffer".
> >
> > Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create
> > which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c.
> > For this to work, the GEM object is moved into "drm_framebuffer". After
> > that, "bochs_framebuffer" is no longer needed and therefore removed.
> >
> > Remove the unused "size" and "initialized" fields from fb, the latter is
> > not necessary as drm_fb_helper_fbdev_teardown can be called even if
> > bochsfb_create fails. This theory was tested by returning early and
> > late (just before drm_gem_fbdev_fb_create). Both scenarios fail
> > gracefully although the latter seems to leak the object from
> > bochsfb_create_object (not a regression).
> >
> > Guess on the reason for the encoder leak: drm_framebuffer_cleanup was
> > previously used, but did not destroy much. drm_fb_helper_fbdev_teardown
> > is now used and calls drm_framebuffer_remove which does a bit more work.
> >
> > Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm'
> > and also with Xorg + fbdev (startx -> xterm). The latter triggered a
> > warning in ttm_bo_vm_open that existed before, see
> > https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de
>
> You can probably get rid of this one if you're refactoring even more. The
> generic fb_probe implementation (already merged) plus gem-shmem support
> for it (still in flight) from Noralf should be able to pull that off. That
> gives you the fb_mmap implementation, but with 100% generic code instead
> of a driver specific hack like Max did.
Aside from the warning, I have not observed actual issues. This patch
was prepared on top of v4.18.1 but the new drm_fb_helper_generic_probe
helper is in master (future 4.19). I suppose that it can be done as a
future cleanup. Nice work Noralf on reducing duplication!
.
> >
> > Signed-off-by: Peter Wu
>
> lgtm. Acked-by: Daniel Vetter
>
> I'll leave merging to Gerd.
> -Daniel
Thanks, I somehow missed a patch. This one does not compile due to
"fb.initialized" still being used in bochs_drv.c. Removal is trivial,
I'll wait for some more feedback and then send a v2 with another patch
prepended.
Kind regards,
Peter
> > ---
> > drivers/gpu/drm/bochs/bochs.h | 19 ++-
> > drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++--
> > drivers/gpu/drm/bochs/bochs_kms.c | 7 +--
> > drivers/gpu/drm/bochs/bochs_mm.c| 74 ---
> > 4 files changed, 22 insertions(+), 157 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> > index 375bf92cd04f..8514a84fbdbe 100644
> > --- a/drivers/gpu/drm/bochs/bochs.h
> > +++ b/drivers/gpu/drm/bochs/bochs.h
> > @@ -51,11 +51,6 @@ enum bochs_types {
> > BOCHS_UNKNOWN,
> > };
> >
> > -struct bochs_framebuffer {
> > - struct drm_framebuffer base;
> > - struct drm_gem_object *obj;
> > -};
> > -
> > struct bochs_device {
> > /* hw */
> > void __iomem *mmio;
> > @@ -88,15 +83,11 @@ struct bochs_device {
> >
> > /* fbdev */
> > struct {
> > - struct bochs_framebuffer gfb;
> > + struct drm_framebuffer *fb;
> > struct drm_fb_helper helper;
> > - int size;
> > - bool initialized;
> > } fb;
> > };
> >
> > -#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer,
> > base)
> > -
> > struct bochs_bo {
> > struct ttm_buffer_object bo;
> > struct ttm_placement placement;
> > @@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct
> > drm_device *dev,
> > int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
> >
[PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
Currently unloading bochs_drm (after unbinding the vtconsole) results in
a warning about a leaked connector:
[drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked!
While investigating a potential fix I noticed that a lot of open-coded
functionality is already implemented elsewhere, so start converting it:
bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl).
bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding
"struct drm_framebuffer" from "struct bochs_framebuffer".
Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create
which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c.
For this to work, the GEM object is moved into "drm_framebuffer". After
that, "bochs_framebuffer" is no longer needed and therefore removed.
Remove the unused "size" and "initialized" fields from fb, the latter is
not necessary as drm_fb_helper_fbdev_teardown can be called even if
bochsfb_create fails. This theory was tested by returning early and
late (just before drm_gem_fbdev_fb_create). Both scenarios fail
gracefully although the latter seems to leak the object from
bochsfb_create_object (not a regression).
Guess on the reason for the encoder leak: drm_framebuffer_cleanup was
previously used, but did not destroy much. drm_fb_helper_fbdev_teardown
is now used and calls drm_framebuffer_remove which does a bit more work.
Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm'
and also with Xorg + fbdev (startx -> xterm). The latter triggered a
warning in ttm_bo_vm_open that existed before, see
https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de
Signed-off-by: Peter Wu
---
drivers/gpu/drm/bochs/bochs.h | 19 ++-
drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++--
drivers/gpu/drm/bochs/bochs_kms.c | 7 +--
drivers/gpu/drm/bochs/bochs_mm.c| 74 ---
4 files changed, 22 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 375bf92cd04f..8514a84fbdbe 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -51,11 +51,6 @@ enum bochs_types {
BOCHS_UNKNOWN,
};
-struct bochs_framebuffer {
- struct drm_framebuffer base;
- struct drm_gem_object *obj;
-};
-
struct bochs_device {
/* hw */
void __iomem *mmio;
@@ -88,15 +83,11 @@ struct bochs_device {
/* fbdev */
struct {
- struct bochs_framebuffer gfb;
+ struct drm_framebuffer *fb;
struct drm_fb_helper helper;
- int size;
- bool initialized;
} fb;
};
-#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, base)
-
struct bochs_bo {
struct ttm_buffer_object bo;
struct ttm_placement placement;
@@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct
drm_device *dev,
int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
uint32_t handle, uint64_t *offset);
-int bochs_framebuffer_init(struct drm_device *dev,
- struct bochs_framebuffer *gfb,
- const struct drm_mode_fb_cmd2 *mode_cmd,
- struct drm_gem_object *obj);
int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr);
int bochs_bo_unpin(struct bochs_bo *bo);
-extern const struct drm_mode_config_funcs bochs_mode_funcs;
-
/* bochs_kms.c */
int bochs_kms_init(struct bochs_device *bochs);
void bochs_kms_fini(struct bochs_device *bochs);
@@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs);
/* bochs_fbdev.c */
int bochs_fbdev_init(struct bochs_device *bochs);
void bochs_fbdev_fini(struct bochs_device *bochs);
+
+extern const struct drm_mode_config_funcs bochs_mode_funcs;
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c
b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 14eb8d0d5a00..8f4d6c052f7b 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -6,6 +6,7 @@
*/
#include "bochs.h"
+#include
/* -- */
@@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info,
struct vm_area_struct *vma)
{
struct drm_fb_helper *fb_helper = info->par;
- struct bochs_device *bochs =
- container_of(fb_helper, struct bochs_device, fb.helper);
- struct bochs_bo *bo = gem_to_bochs_bo(bochs->fb.gfb.obj);
+ struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
return ttm_fbdev_mmap(vma, >bo);
}
@@ -101,19 +100,20 @@ static int bochsfb_create(struct drm_fb_helper *helper,
/* init fb device */
info = drm_fb_helper_alloc_fbi(helper);
- if (I
[PATCH] qxl: fix null-pointer crash during suspend
"crtc->helper_private" is not initialized by the QXL driver and thus the
"crtc_funcs->disable" call would crash (resulting in suspend failure).
Fix this by converting the suspend/resume functions to use the
drm_mode_config_helper_* helpers.
Tested system sleep with QEMU 3.0 using "echo mem > /sys/power/state".
During suspend the following message is visible from QEMU:
spice/server/display-channel.c:2425:display_channel_validate_surface:
canvas address is 0x7fd05da68308 for 0 (and is NULL)
spice/server/display-channel.c:2426:display_channel_validate_surface:
failed on 0
This seems to be triggered by QXL_IO_NOTIFY_CMD after
QXL_IO_DESTROY_PRIMARY_ASYNC, but aside from the warning things still
seem to work (tested with both the GTK and -spice options).
Signed-off-by: Peter Wu
---
Hi,
I found this issue while trying to suspend a VM that uses QXL. In order to see
the stack trace over serial, boot with no_console_suspend. Searching for
"qxl_drm_freeze" showed one recent report from Alan:
https://lkml.kernel.org/r/891e334c-cf19-032c-b996-59ac166fc...@gmail.com
Kind regards,
Peter
---
drivers/gpu/drm/qxl/qxl_drv.c | 26 +-
1 file changed, 5 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 2445e75cf7ea..d00f45eed03c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -136,20 +136,11 @@ static int qxl_drm_freeze(struct drm_device *dev)
{
struct pci_dev *pdev = dev->pdev;
struct qxl_device *qdev = dev->dev_private;
- struct drm_crtc *crtc;
-
- drm_kms_helper_poll_disable(dev);
-
- console_lock();
- qxl_fbdev_set_suspend(qdev, 1);
- console_unlock();
+ int ret;
- /* unpin the front buffers */
- list_for_each_entry(crtc, >mode_config.crtc_list, head) {
- const struct drm_crtc_helper_funcs *crtc_funcs =
crtc->helper_private;
- if (crtc->enabled)
- (*crtc_funcs->disable)(crtc);
- }
+ ret = drm_mode_config_helper_suspend(dev);
+ if (ret)
+ return ret;
qxl_destroy_monitors_object(qdev);
qxl_surf_evict(qdev);
@@ -175,14 +166,7 @@ static int qxl_drm_resume(struct drm_device *dev, bool
thaw)
}
qxl_create_monitors_object(qdev);
- drm_helper_resume_force_mode(dev);
-
- console_lock();
- qxl_fbdev_set_suspend(qdev, 0);
- console_unlock();
-
- drm_kms_helper_poll_enable(dev);
- return 0;
+ return drm_mode_config_helper_resume(dev);
}
static int qxl_pm_suspend(struct device *dev)
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/7] Modernize vga_switcheroo by using device link for HDA
1/0x80 [nouveau]
nouveau_drm_unload+0x6b/0xd0 [nouveau]
drm_dev_unregister+0x3c/0xe0
drm_put_dev+0x2e/0x60
nouveau_drm_device_remove+0x37/0x50 [nouveau]
pci_device_remove+0x36/0xb0
device_release_driver_internal+0x160/0x230
driver_detach+0x3a/0x70
bus_remove_driver+0x58/0xd0
pci_unregister_driver+0x3b/0x90
nouveau_drm_exit+0x15/0x432 [nouveau]
SyS_delete_module+0x16c/0x230
Issue 8 - acpi: sleeping function in atomic context.
(Issue is likely not related to this patch set.)
At some point I also got a BUG, nouveau was already unloaded and I ran:
"echo 1 | tee /sys/bus/pci/devices/:01:00.{0,1}/remove"
BUG: sleeping function called from invalid context at
/home/peter/linux/mm/slab.h:419
in_atomic(): 1, irqs_disabled(): 0, pid: 4844, name: kworker/3:4
INFO: lockdep is turned off.
CPU: 3 PID: 4844 Comm: kworker/3:4 Tainted: GW
4.15.0testing-00020-gb33d50c5c6ad #55
Hardware name: Notebook
P65_P67RGRERA/P65_P67RGRERA, BIOS 1.05.16 05/16/2016
Workqueue: events_power_efficient srcu_invoke_callbacks
Call Trace:
dump_stack+0x5f/0x86
___might_sleep+0x20c/0x240
kmem_cache_alloc_trace+0x4d/0x230
acpi_ut_evaluate_object+0x68/0x23c
? srcu_invoke_callbacks+0xa2/0x150
acpi_rs_get_prt_method_data+0x42/0xa2
acpi_get_irq_routing_table+0x70/0x9f
? __slab_free+0x11c/0x380
acpi_pci_irq_find_prt_entry+0x83/0x330
? srcu_invoke_callbacks+0xa2/0x150
acpi_pci_irq_lookup+0x27/0x2e0
acpi_pci_irq_disable+0x45/0xb0
pci_release_dev+0x29/0x60
device_release+0x2d/0x80
kobject_put+0xb7/0x190
__device_link_free_srcu+0x32/0x40
srcu_invoke_callbacks+0xba/0x150
process_one_work+0x273/0x670
worker_thread+0x4a/0x400
kthread+0x100/0x140
? process_one_work+0x670/0x670
? kthread_create_worker_on_cpu+0x50/0x50
? do_syscall_64+0x56/0x1a0
? SyS_exit_group+0x10/0x10
Issue 9 - potential memory corruption.
At some point (possibly after issue 7, but I am not fully sure), I saw
an artifact in the text console which would persist even when switching
between consoles. It was gone after system sleep/resume. If I remember
correctly, it looked like something from a Xorg session which I killed
before.
That was the bad news, the good news:
- Loading nouveau and snd-hda-intel (in any order) while RPM is enabled
and the port was in D3cold works.
- RPM interaction between audio and GPU seems good, when audio resumes,
the GPU RPM counter increments, when audio suspends it decrements.
- As the GPU enters D3cold, I can observe significant power savings
through /sys/class/power_supply/BAT0/ (no regressions here).
- In a default configuration I have no audio function (see also nouveau
bug https://bugs.freedesktop.org/show_bug.cgi?id=75985) so most of the
above issues should not occur.
Hope it helps, and if desired you can add:
Tested-by: Peter Wu <pe...@lekensteyn.nl>
For the following patches, you can also add my Reviewed-by:
vga_switcheroo: Update PCI current_state on power change
vga_switcheroo: Deduplicate power state tracking
vga_switcheroo: Use device link for HDA controller
vga_switcheroo: Let HDA autosuspend on mux change
drm/nouveau: Runtime suspend despite HDA being unbound
The two other PCI patches look fine as well.
Kind regards,
Peter
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
> Modernize vga_switcheroo by using a device link to enforce a runtime PM
> dependency from an HDA controller to the GPU it's integrated into, v2.
>
> Changes since v1:
>
> - Replace patch [1/7] to use pci_save_state() / pci_restore_state()
> for consistency between runtime PM code path of bound and unbound
> devices. (Rafael, Bjorn)
>
> - Patch [5/7]: Drop an unnecessary initialization. (Bjorn)
> Rephrase error message on failed link addition for clarity.
>
> Link to v1:
> https://www.spinics.net/lists/dri-devel/msg165889.html
>
> Testing on more machines would be greatly appreciated, particularly
> Nvidia Optimus or AMD PowerXpress.
>
> The series is based on 4.16-rc3. To test it on 4.15, you need to
> cherry-pick 7506dc798993 and 2fa6d6cdaf28. For your convenience
> I've pushed a 4.15-based branch to:
> https://github.com/l1k/linux/commits/switcheroo_devlink_v2
>
> Minimal test procedure:
>
> - Note well: Recent Optimus require that a Mini-DP or HDMI cable is
> plugged in on boot for the HDA device to be present.
>
> - Check that HDA, GPU and root port autosuspend when not in use:
> cat /sys/bus/pci/devices/:01:00.1/power/runtime_status # HDA
> cat /sys/bus/pci/devices/:01:00.0/power/runtime_status # GPU
> cat /sys/bus/pci/devices/:00:01.0/power/runtime_status # Root Port
>
> - Check that all three autoresume when accessing the HDA
Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo
On Wed, Mar 08, 2017 at 06:01:54AM +0100, Lukas Wunner wrote: > On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote: > > On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lu...@wunner.de> wrote: > > > An external Thunderbolt GPU can neither drive the laptop's panel nor be > > > powered off by the platform, so there's no point in registering it with > > > vga_switcheroo. In fact, when the external GPU is runtime suspended, > > > vga_switcheroo will cut power to the internal discrete GPU, resulting in > > > a lockup. > > > > I'm not necessarily opposed to this, but I'd prefer something more > > generic. E.g., what happens if someone uses another dGPU in a docking > > station or some other sort of PCIe bridge? > > Such a dGPU is only relevant to vga_switcheroo if it can either drive > the panel or be powered off by the platform. Does such a product exist? > > I've never heard of one, and think that's because such a product doesn't > make sense: A docking staton is not power-constrained, it's stationary > and connected to a wall outlet, so there's no need to power the dGPU off > when it's not in use. > > And at a docking station you're usually connected to a larger monitor, > so having the dGPU drive the laptop's smaller panel isn't necessary either. > In the rare cases where there's no larger monitor, you just use the dGPU > for render offloading, just as you would for contemporary ATPX laptops. > > OTOH, dGPUs in Thunderbolt enclosures *do* exist and connecting them > to an ATPX machine causes failure, as explained in the commit message. > > > > I think on AMD platforms > > at least we should be able to determine what devices are the > > switcheroo devices based on information in the ATIF and ATPX ACPI > > methods. In that case, we can be explicit in which devices we > > register with vga_switcheroo. > > Is there public documentation on these methods somewhere? The ACPI interface is documented in drivers/gpu/drm/amd/include/amd_acpi.h while drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c contains some glue for ACPI and the amdgpu driver (similar code exists for radeon). -- Kind regards, Peter Wu https://lekensteyn.nl ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE
uld be dropped once that is merged.
> + */
> +#ifndef ACPI_VIDEO_NOTIFY_PROBE
> +#define ACPI_VIDEO_NOTIFY_PROBE 0x81
> +#endif
> +
> +static void
> +nouveau_display_acpi_work(struct work_struct *work)
> +{
> + struct nouveau_drm *drm = container_of(work, typeof(*drm), acpi_work);
> +
> + pm_runtime_get_sync(drm->dev->dev);
> +
> + drm_helper_hpd_irq_event(drm->dev);
> +
> + pm_runtime_mark_last_busy(drm->dev->dev);
> + pm_runtime_put_sync(drm->dev->dev);
Nothing depends on the device being suspended immediately, I guess you
can drop _sync and also use:
pm_runtime_put_autosuspend
Kind regards,
Peter
> +}
> +
> +static int
> +nouveau_display_acpi_ntfy(struct notifier_block *nb, unsigned long val,
> + void *data)
> +{
> + struct nouveau_drm *drm = container_of(nb, typeof(*drm), acpi_nb);
> + struct acpi_bus_event *info = data;
> +
> + if (!strcmp(info->device_class, ACPI_VIDEO_CLASS)) {
> + if (info->type == ACPI_VIDEO_NOTIFY_PROBE) {
> + /*
> + * This may be the only indication we receive of a
> + * connector hotplug on a runtime suspended GPU,
> + * schedule acpi_work to check.
> + */
> + schedule_work(>acpi_work);
> +
> + /* acpi-video should not generate keypresses for this */
> + return NOTIFY_BAD;
> + }
> + }
> +
> + return NOTIFY_DONE;
> +}
> +#endif
> +
> int
> nouveau_display_init(struct drm_device *dev)
> {
> @@ -537,6 +589,12 @@ nouveau_display_create(struct drm_device *dev)
> }
>
> nouveau_backlight_init(dev);
> +#ifdef CONFIG_ACPI
> + INIT_WORK(>acpi_work, nouveau_display_acpi_work);
> + drm->acpi_nb.notifier_call = nouveau_display_acpi_ntfy;
> + register_acpi_notifier(>acpi_nb);
> +#endif
> +
> return 0;
>
> vblank_err:
> @@ -552,6 +610,9 @@ nouveau_display_destroy(struct drm_device *dev)
> {
> struct nouveau_display *disp = nouveau_display(dev);
>
> +#ifdef CONFIG_ACPI
> + unregister_acpi_notifier(_drm(dev)->acpi_nb);
> +#endif
> nouveau_backlight_exit(dev);
> nouveau_display_vblank_fini(dev);
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 822a021..71d4532 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -37,6 +37,8 @@
> * - implemented limited ABI16/NVIF interop
> */
>
> +#include
> +
> #include
> #include
> #include
> @@ -161,6 +163,10 @@ struct nouveau_drm {
> struct nvbios vbios;
> struct nouveau_display *display;
> struct backlight_device *backlight;
> +#ifdef CONFIG_ACPI
> + struct notifier_block acpi_nb;
> + struct work_struct acpi_work;
> +#endif
>
> /* power management */
> struct nouveau_hwmon *hwmon;
> --
> 2.9.3
>
> ___
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[Nouveau] [PATCH] acpi: video: Move ACPI_VIDEO_NOTIFY_* defines to acpi/video.h
On Wed, Nov 09, 2016 at 06:15:56PM +0100, Hans de Goede wrote:
> acpi_video.c passed the ACPI_VIDEO_NOTIFY_* defines as type code to
> acpi_notifier_call_chain(). Move these defines to acpi/video.h so
> that acpi_notifier listeners can check the type code using these
> defines.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Peter Wu
> ---
> drivers/acpi/acpi_video.c | 11 ---
> include/acpi/video.h | 11 +++
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index c5557d0..201292e 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -43,17 +43,6 @@
>
> #define ACPI_VIDEO_BUS_NAME "Video Bus"
> #define ACPI_VIDEO_DEVICE_NAME "Video Device"
> -#define ACPI_VIDEO_NOTIFY_SWITCH 0x80
> -#define ACPI_VIDEO_NOTIFY_PROBE 0x81
> -#define ACPI_VIDEO_NOTIFY_CYCLE 0x82
> -#define ACPI_VIDEO_NOTIFY_NEXT_OUTPUT0x83
> -#define ACPI_VIDEO_NOTIFY_PREV_OUTPUT0x84
> -
> -#define ACPI_VIDEO_NOTIFY_CYCLE_BRIGHTNESS 0x85
> -#define ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS0x86
> -#define ACPI_VIDEO_NOTIFY_DEC_BRIGHTNESS 0x87
> -#define ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS0x88
> -#define ACPI_VIDEO_NOTIFY_DISPLAY_OFF0x89
>
> #define MAX_NAME_LEN 20
>
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 4536bd3..bfe484d 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -30,6 +30,17 @@ struct acpi_device;
> #define ACPI_VIDEO_DISPLAY_LEGACY_PANEL 0x0110
> #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200
>
> +#define ACPI_VIDEO_NOTIFY_SWITCH 0x80
> +#define ACPI_VIDEO_NOTIFY_PROBE 0x81
> +#define ACPI_VIDEO_NOTIFY_CYCLE 0x82
> +#define ACPI_VIDEO_NOTIFY_NEXT_OUTPUT0x83
> +#define ACPI_VIDEO_NOTIFY_PREV_OUTPUT0x84
> +#define ACPI_VIDEO_NOTIFY_CYCLE_BRIGHTNESS 0x85
> +#define ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS 0x86
> +#define ACPI_VIDEO_NOTIFY_DEC_BRIGHTNESS 0x87
> +#define ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS0x88
> +#define ACPI_VIDEO_NOTIFY_DISPLAY_OFF0x89
> +
> enum acpi_backlight_type {
> acpi_backlight_undef = -1,
> acpi_backlight_none = 0,
> --
> 2.9.3
>
[PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info
On Wed, Nov 02, 2016 at 11:47:03AM +, Emil Velikov wrote:
> On 2 November 2016 at 11:14, Peter Wu wrote:
> > On Tue, Nov 01, 2016 at 06:13:34PM +, Emil Velikov wrote:
> >> sysfs file wakes up the device. The latter of which may
> >> be slow and isn't required to begin with.
> >>
> >> Reading through config is/was required since the revision is not
> >> available by other means, although with a kernel patch in the way we can
> >> 'cheat' temporarily.
> >>
> >> That should be fine, since no open-source project has ever used the
> >> value.
> >>
> >> Cc: Michel Dänzer
> >> Cc: Mauro Santos
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> >> Signed-off-by: Emil Velikov
> >
> > See below for one observation. Other than that, strace confirms that
> > the new sysfs files are being accessed.
> >
> > Reviewed-and-tested-by: Peter Wu
> >
> > This was tested with Linux 4.8.4 (unpatched) and libdrm 2.4.71 (+this
> > patch) with i915 + nouveau. Tested with running glxgears and glxinfo.
> > Note that glxinfo still accesses 'config' due to libpciaccess.
> >
> > + drm_intel_get_aperture_sizes
> > + drm_intel_probe_agp_aperture_size
> > + pci_system_init()
> > + pci_system_linux_sysfs_create
> > + populate_entries
> > + pci_device_linux_sysfs_read() <-- offending function
> > + pci_device_find_by_slot(0, 0, 2, 0)
> >
> > That populate_entries function can probably use a fix similar to this
> > one.
> >
> Indeed it would, although we ought to check if that won't cause any
> (extra) regressions.
>
> Skimming through my distro (Arch) the following depend on libpciaccess:
>
> intel-gpu-tools
Does not use the "revision" field.
> libdrm
Does not use the "revision" field of libpciaccess.
While amdgpu has the "pci_rev" line below, it is copied from the
response of an ioctl, not pciaccess, so it is safe:
drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev)
{
int r, i;
r = amdgpu_query_info(dev, AMDGPU_INFO_DEV_INFO, sizeof(dev->dev_info),
>dev_info);
// ...
dev->info.pci_rev_id = dev->dev_info.pci_rev;
> libvirt
> radeontool
> spice-vdagent
> vbetool
These four do not use the "revision" field and are safe.
> xorg-server
>
> Of which the first two are safe, while the last one isn't + it even
> exports the revision to DDX via xf86str.h's GDevRec::chipRev
xorg-server internally does not use the revision field, but it exports
them as you have observed:
GDevRec::chipRev
(copied from libpciaccess, struct pci_device::revision)
XF86ConfDevicePtr::dev_chiprev (copied from GDevRec::chipRev)
Not knowing what the users are, I tried to have a look at the git logs
for "chipRev", but the change introducing it is not that helpful:
commit ded6147bfb5d75ff1e67c858040a628b61bc17d1
Author: Kaleb Keithley
Date: Fri Nov 14 15:54:54 2003 +
R6.6 is the Xorg base-line
...
609 files changed, 262690 insertions(+)
To play safe, you could fallback to "config" in sysfs when "revision" is
not found, that would allow older kernels to work with no regressions in
the information while reducing the runtime wakeups for newer kernels.
> Which gives us even more places to check through. Can you lend a hand ?
>
> Thanks
> Emil
I am also on Arch, what other places are you thinking about? DDXes or
other users of libpciaccess?
Kind regards,
Peter
[PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info
On Tue, Nov 01, 2016 at 06:13:34PM +, Emil Velikov wrote:
> sysfs file wakes up the device. The latter of which may
> be slow and isn't required to begin with.
>
> Reading through config is/was required since the revision is not
> available by other means, although with a kernel patch in the way we can
> 'cheat' temporarily.
>
> That should be fine, since no open-source project has ever used the
> value.
>
> Cc: Michel Dänzer
> Cc: Mauro Santos
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov
See below for one observation. Other than that, strace confirms that
the new sysfs files are being accessed.
Reviewed-and-tested-by: Peter Wu
This was tested with Linux 4.8.4 (unpatched) and libdrm 2.4.71 (+this
patch) with i915 + nouveau. Tested with running glxgears and glxinfo.
Note that glxinfo still accesses 'config' due to libpciaccess.
+ drm_intel_get_aperture_sizes
+ drm_intel_probe_agp_aperture_size
+ pci_system_init()
+ pci_system_linux_sysfs_create
+ populate_entries
+ pci_device_linux_sysfs_read() <-- offending function
+ pci_device_find_by_slot(0, 0, 2, 0)
That populate_entries function can probably use a fix similar to this
one.
> ---
> Mauro can you apply this against libdrm and rebuild it. You do _not_
> need to rebuild mesa afterwords.
>
> Thanks
> ---
> xf86drm.c | 50 +++---
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 52add5e..5a5100c 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
> drmPciDeviceInfoPtr device)
> {
> #ifdef __linux__
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
> +static const char *attrs[] = {
> + "revision", /* XXX: make sure it's always first, see note below */
> + "vendor",
> + "device",
> + "subsystem_vendor",
> + "subsystem_device",
> +};
> char path[PATH_MAX + 1];
The size for snprintf already includes the nul-terminator, so strictly
speaking the +1 is not needed. It does not hurt either though.
> -unsigned char config[64];
> -int fd, ret;
> +unsigned int data[ARRAY_SIZE(attrs)];
> +FILE *fp;
> +int ret;
>
> -snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
> -fd = open(path, O_RDONLY);
> -if (fd < 0)
> -return -errno;
> +for (unsigned i = 0; i < ARRAY_SIZE(attrs); i++) {
> +snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/%s",
> + d_name, attrs[i]);
> +fp = fopen(path, "r");
> +if (!fp) {
> +/* Note: First we check the revision, since older kernels
> + * may not have it. Default to zero in such cases. */
> +if (i == 0) {
> +data[i] = 0;
> +continue;
> +}
> +return -errno;
> +}
>
> -ret = read(fd, config, sizeof(config));
> -close(fd);
> -if (ret < 0)
> -return -errno;
> +ret = fscanf(fp, "%x", [i]);
> +fclose(fp);
> +if (ret != 1)
> +return -errno;
> +
> +}
>
> -device->vendor_id = config[0] | (config[1] << 8);
> -device->device_id = config[2] | (config[3] << 8);
> -device->revision_id = config[8];
> -device->subvendor_id = config[44] | (config[45] << 8);
> -device->subdevice_id = config[46] | (config[47] << 8);
> +device->revision_id = data[0] & 0xff;
> +device->vendor_id = data[1] & 0x;
> +device->device_id = data[2] & 0x;
> +device->subvendor_id = data[3] & 0x;
> +device->subdevice_id = data[4] & 0x;
>
> return 0;
> #else
> --
> 2.10.0
[PATCH v2] drm/nouveau/acpi: fix check for power resources support
On Tue, Nov 01, 2016 at 09:24:23AM -0400, Alex Deucher wrote:
> On Tue, Nov 1, 2016 at 12:55 AM, Dave Airlie wrote:
> > On 1 November 2016 at 08:48, Peter Wu wrote:
> >> Check whether the kernel really supports power resources for a device,
> >> otherwise the power might not be removed when the device is runtime
> >> suspended (DSM should still work in these cases where PR does not).
> >>
> >> This is a workaround for a problem where ACPICA and Windows 10 differ in
> >> behavior. ACPICA does not correctly enumerate power resources within a
> >> conditional block (due to delayed execution of such blocks) and as a
> >> result power_resources is set to false even if _PR3 exists.
> >>
> >> Fixes: 692a17dcc292 ("drm/nouveau/acpi: fix lockup with PCIe runtime PM")
> >> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98398
> >> Reported-and-tested-by: Rick Kerkhof
> >> Reviewed-by: Mika Westerberg
> >> Signed-off-by: Peter Wu
> >
> > I've appled it this and cc'ed stable to drm-fixes.
> >
> > Are we going to get ACPICA fixed?
The ACPI folks are aware of the problem, see this thread and its
follow-ups:
http://www.spinics.net/lists/linux-acpi/msg70050.html
> Looks like we may have hit this on radeon/amdgpu as well:
> https://bugs.freedesktop.org/show_bug.cgi?id=98505
>
> Alex
That log seems to suggest that resume fails while this nouveau issue is
related to suspend not powering off a device. The root cause might be
different.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[PATCH v2] drm/nouveau/acpi: fix check for power resources support
Check whether the kernel really supports power resources for a device,
otherwise the power might not be removed when the device is runtime
suspended (DSM should still work in these cases where PR does not).
This is a workaround for a problem where ACPICA and Windows 10 differ in
behavior. ACPICA does not correctly enumerate power resources within a
conditional block (due to delayed execution of such blocks) and as a
result power_resources is set to false even if _PR3 exists.
Fixes: 692a17dcc292 ("drm/nouveau/acpi: fix lockup with PCIe runtime PM")
Link: https://bugs.freedesktop.org/show_bug.cgi?id=98398
Reported-and-tested-by: Rick Kerkhof
Reviewed-by: Mika Westerberg
Signed-off-by: Peter Wu
---
v2: collected tags from Rick and Mika; added ACPICA note as requested by Mika
I suggest Cc: stable (if the maintainer is OK with that?)
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index dc57b62..193573d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -240,7 +240,8 @@ static bool nouveau_pr3_present(struct pci_dev *pdev)
if (!parent_adev)
return false;
- return acpi_has_method(parent_adev->handle, "_PR3");
+ return parent_adev->power.flags.power_resources &&
+ acpi_has_method(parent_adev->handle, "_PR3");
}
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
--
2.10.1
[PATCH] drm/nouveau/acpi: fix check for power resources support
Check whether the kernel really supports power resources for a device,
otherwise the power might not be removed when the device is runtime
suspended (DSM should still work in these cases where PR does not).
Link: https://bugs.freedesktop.org/show_bug.cgi?id=98398
Fixes: 692a17dcc292 ("drm/nouveau/acpi: fix lockup with PCIe runtime PM")
Signed-off-by: Peter Wu
---
Hi,
Maybe Cc: stable (4.8+)?
Compile-tested only. Rick, can you test if this patch fixes the problem for you?
This check was actually in the original patch, but it was changed:
https://lists.freedesktop.org/archives/nouveau/2016-May/025125.html
Re-adding the check as suggested by Mika.
Kind regards,
Peter
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index dc57b62..193573d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -240,7 +240,8 @@ static bool nouveau_pr3_present(struct pci_dev *pdev)
if (!parent_adev)
return false;
- return acpi_has_method(parent_adev->handle, "_PR3");
+ return parent_adev->power.flags.power_resources &&
+ acpi_has_method(parent_adev->handle, "_PR3");
}
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
--
2.10.1
Kernel Freeze with American Megatrends BIOS
On Wed, Aug 31, 2016 at 02:21:31PM +0200, Roland Singer wrote:
> Am 31.08.2016 um 13:46 schrieb Peter Wu:
> > On Wed, Aug 31, 2016 at 01:27:36PM +0200, Roland Singer wrote:
> >> Am 30.08.2016 um 21:53 schrieb Peter Wu:
> >>> On Mon, Aug 29, 2016 at 11:02:10AM -0500, Bjorn Helgaas wrote:
> >>>> [+cc linux-acpi, linux-kernel, dri-devel]
> >>>>
> >>>> Hi Roland,
> >>>>
> >>>> I have no idea how to debug this problem. Are you seeing something
> >>>> that suggests it may be a PCI problem?
> >>>
> >>> Yes I suspect there is an ACPI and/ or PCI problem, possibly
> >>> device-specific. Steps to reproduce on the affected machines:
> >>>
> >>> 1. Load nouveau.
> >>> 2. Wait for it to runtime suspend.
> >>> 2. Invoke 'lspci', this resumes the Nvidia PCI device via nouveau.
> >>> 3. lspci never returns, few moments later an AML_INFINITE_LOOP is
> >>> reported.
> >>>
> >>
> >> I can confirm this. Same result on my machine.
> >>
> >> Here is a link to my ACPI tables:
> >> https://bugs.launchpad.net/lpbugreporter/+bug/752542/+attachment/4722651/+files/Razer-Blade.tar.gz
> >>
> >> The specific source for the NVIDIA card can be found in the ssdt5.dsl file.
> >>
> >>
> >> Method (PGON, 1, Serialized)
> >> {
> >> /* ... */
> >>
> >> GPPR (PION, One)
> >> If ((OSYS == 0x07D9)) /* Is Windows 2009 - In my case, setting to
> >> Windows 2009 only works! */
> >> {
> > [..]
> >> }
> >> Else
> >> {
> >> LKEN (PION)
> >> }
> >>
> >> /* ... */
> >>
> >> Return (Zero)
> >> }
> >>
> >>
> >>
> >> If not set to Windows 2009, then this is triggered:
> >>
> >>
> >> Method (LKEN, 1, NotSerialized)
> >> {
> > [..]
> >> }
> >
> > Yep, this is the same code. I stripped out irrelevant parts from the
> > previous mail for brevity.
> >
> >> Is it possible to override the specific ACPI table functions (SSDT) in the
> >> DSDT?
> >> This way I could try to debug to find some more information...
> >
> > See Documentation/acpi/initrd_table_override.txt and note that it is
> > important that the tables are really located at /kernel/firmware/acpi/
> > in your initrd (which must be the first, even before any possible
> > microcode updates).
> >
> > What are you trying to do? For ACPI method tracing, see
> > Documentation/acpi/method-tracing.txt
> >
>
> Oh, you're right.
>
> Thanks. Right now I am overriding the DSDT, but I am not able to override
> the SSDT, because I have to fix and compile all the SSDT files. There
> are too many compile errors... Wanted to find the exact line which
> is responsible for the hickup.
Have you disassembled with externs included? That is,
iasl -e *.dat -d ssdtX.dat
If you are sure that the remaining errors are harmless, you can use the
'-f' option to ignore errors. You can also use the `-ve` option to
suppress warnings and remarks so you can focus on the errors.
If you look at my notes.txt, you will see that _OFF always executes the
same code. PGON differs. When the problem occurs, "Q0L0" somehow always
reads back as non-zero and LNKS < 7.
> >>> Yes I suspect there is an ACPI and/ or PCI problem, possibly
> >>> device-specific. Steps to reproduce on the affected machines:
> >>>
> >>> 1. Load nouveau.
> >>> 2. Wait for it to runtime suspend.
> >>> 2. Invoke 'lspci', this resumes the Nvidia PCI device via nouveau.
> >>> 3. lspci never returns, few moments later an AML_INFINITE_LOOP is
> >>> reported.
>
> I noticed following:
>
> 1. Blacklist nouveau
> 2. Boot to GDM login manager (Wayland)
> 3. Switch to TTY with CTRL+ALT+FN2
> 4. Load bbswitch
> 5. Switch off GPU
> 6. run lspci -> no freeze
> 7. Switch to GDM
> 8. Login to a Wayland session (X11 won't work)
> 9. run lspci in a GUI terminal -> system freezes
Is nouveau somehow loaded anyway? All those extra components (X11,
Wayland, etc.) are unnecessary to reproduce the core problem. It occurs
whenever the device is being resumed (either via DSM/_PS0 or via power
resource PG00._ON).
--
Kind regards,
Peter Wu
https://lekensteyn.nl
Kernel Freeze with American Megatrends BIOS
On Wed, Aug 31, 2016 at 01:27:36PM +0200, Roland Singer wrote:
> Am 30.08.2016 um 21:53 schrieb Peter Wu:
> > On Mon, Aug 29, 2016 at 11:02:10AM -0500, Bjorn Helgaas wrote:
> >> [+cc linux-acpi, linux-kernel, dri-devel]
> >>
> >> Hi Roland,
> >>
> >> I have no idea how to debug this problem. Are you seeing something
> >> that suggests it may be a PCI problem?
> >
> > Yes I suspect there is an ACPI and/ or PCI problem, possibly
> > device-specific. Steps to reproduce on the affected machines:
> >
> > 1. Load nouveau.
> > 2. Wait for it to runtime suspend.
> > 2. Invoke 'lspci', this resumes the Nvidia PCI device via nouveau.
> > 3. lspci never returns, few moments later an AML_INFINITE_LOOP is
> > reported.
> >
>
> I can confirm this. Same result on my machine.
>
> Here is a link to my ACPI tables:
> https://bugs.launchpad.net/lpbugreporter/+bug/752542/+attachment/4722651/+files/Razer-Blade.tar.gz
>
> The specific source for the NVIDIA card can be found in the ssdt5.dsl file.
>
>
> Method (PGON, 1, Serialized)
> {
> /* ... */
>
> GPPR (PION, One)
> If ((OSYS == 0x07D9)) /* Is Windows 2009 - In my case, setting to
> Windows 2009 only works! */
> {
[..]
> }
> Else
> {
> LKEN (PION)
> }
>
> /* ... */
>
> Return (Zero)
> }
>
>
>
> If not set to Windows 2009, then this is triggered:
>
>
> Method (LKEN, 1, NotSerialized)
> {
[..]
> }
Yep, this is the same code. I stripped out irrelevant parts from the
previous mail for brevity.
> Is it possible to override the specific ACPI table functions (SSDT) in the
> DSDT?
> This way I could try to debug to find some more information...
See Documentation/acpi/initrd_table_override.txt and note that it is
important that the tables are really located at /kernel/firmware/acpi/
in your initrd (which must be the first, even before any possible
microcode updates).
What are you trying to do? For ACPI method tracing, see
Documentation/acpi/method-tracing.txt
--
Kind regards,
Peter Wu
https://lekensteyn.nl
Kernel Freeze with American Megatrends BIOS
On Mon, Aug 29, 2016 at 11:02:10AM -0500, Bjorn Helgaas wrote:
> [+cc linux-acpi, linux-kernel, dri-devel]
>
> Hi Roland,
>
> I have no idea how to debug this problem. Are you seeing something
> that suggests it may be a PCI problem?
Yes I suspect there is an ACPI and/ or PCI problem, possibly
device-specific. Steps to reproduce on the affected machines:
1. Load nouveau.
2. Wait for it to runtime suspend.
2. Invoke 'lspci', this resumes the Nvidia PCI device via nouveau.
3. lspci never returns, few moments later an AML_INFINITE_LOOP is
reported.
If you use the external bbswitch module, the effect is the same. I have
been trying to debug this for some time on nouveau with no luck. The
PCI/PM D3cold patches from Mika makes no difference.
Runtime resume via nouveau triggers some ACPI methods (I'll assume the
Windows 8-style PR method and take the Clevo P651 as example):
\_SB.PCI0.PEG0.PG00._ON () ->
\_SB.PCI0.PGON (0)
Then:
Method (PGON, 1, Serialized) {
PION = Arg0 // note: 0 for PG00
// ...
If ((OSYS != 0x07DF)) { /* Not Windows 2015 (Windows 10), see below */ }
Else {
LKEN (PION)
}
// this is the infinite loop: it tries to bring the PCIe link to
// full speed, but fails to do so.
While ((\_SB.PCI0.PEG0.LNKS < 0x07)) {
Local0 = 0x20
While (Local0) {
If ((\_SB.PCI0.PEG0.LNKS < 0x07)) {
Stall (0x64)
Local0--
} Else { Break }
}
If ((Local0 == Zero)) {
\_SB.PCI0.PEG0.RTLK = One
Stall (0x64)
}
}
// ...
}
Without any workaround, this piece of code is invoked:
Method (LKEN, 1, NotSerialized) {
Local3 = (CPEX & 0x0F) // CPEX at 0x5ff9be7f and has value 000506e3
If ((Local3 == Zero)) {
/* Similar to below, but with Q0L0 -> P0L0 (register 0xBC bit 6) */
} ElseIf ((Local3 != Zero)) {
If ((Arg0 == Zero)) {
/* Enter L0 Activate state.
* (LKDS tries to enter L2, deep-energy-saving state.) */
Q0L0 = One // register 0x249 bit 0; \_SB.PCI0.OPG0.Q0L0
00:01.0
Sleep (0x10)
Local0 = Zero
While (Q0L0) {
If ((Local0 > 0x04)) { Break }
Sleep (0x10)
Local0++
}
} else { /* other cases, but we are only interested in PGON(0) */ }
}
}
The acpi_osi="!Windows 2015" workaround will invoke this instead:
If ((OSYS != 0x07DF)) {
If ((PION == Zero)) {
P0AP = Zero /* PGOF writes 3 */
P0RM = Zero /* PGOF writes 1 */
}
If ((PBGE != Zero)) { /* Observed to be false (PBGE == 0) */
If (SBDL (PION)) {
PUAB (PION)
CBDL = GUBC (PION)
MBDL = GMXB (PION)
If ((CBDL > MBDL)) {
CBDL = MBDL /* \_SB_.PCI0.MBDL */
}
PDUB (PION, CBDL)
}
}
If ((PION == Zero)) {
P0LD = Zero /* Link Disable = 0, PGOF sets 1 instead. */
P0TR = One /* Train? (PGOF does not set this). */
TCNT = Zero
While ((TCNT < LDLY)) { /* LDLY = 300 */
If ((P0VC == Zero)) {
/* VC Negotiation Pending 0 means VC negotation is
complete. */
Break
}
Sleep (0x10)
TCNT += 0x10 /* At most 19 iterations, sleeping for 304ms. */
}
}
}
The comments above are my own interpretation based on the acpidumps I
extracted from the machine. These notes and ACPI tables can be found at
https://github.com/Lekensteyn/acpi-stuff/blob/master/Clevo-P651RA/notes.txt
https://github.com/Lekensteyn/acpi-stuff/tree/master/dsl/Clevo_P651RA
Other affected devices have similar code, differences are small:
- No check for LNKS (avoids the infinite loop, but device is still off)
- Instead of a check for != "Windows 2015", they check for == "Windows
2009" or even for == "Windows 2009" || "Windows 2013" (Dell Inspiron
7559).
The tested kernels (with bbswitch or nouveau) were Linux 4.4.0, 4.6,
4.7 (nouveau + PCI/PM + nouveau PR patches). The PCIe device is
something from the GTX 9xxM family in all cases.
I have a bunch of PCI config dumps from Windows and Linux, but there is
nothing extraordinary. Also did an ACPI trace via a Checked/Debug build
of Windows, but it just confirms that the ACPI method we use for the
Nvidia device is the correct one.
Let me know if you need more information, I would be glad to provide.
Kind regards,
Peter
> On Tue, Aug 23, 2016 at 11:23:45AM +0200, Roland Singer wrote:
> > Hi,
> >
> > hope somebody can help me fix this kernel problem which affects the
>
Kernel Freeze with American Megatrends BIOS
On Tue, Aug 30, 2016 at 02:13:46PM -0400, Ilia Mirkin wrote: > On Tue, Aug 30, 2016 at 2:02 PM, Roland Singer > wrote: > > I configured bbswitch to not set any states automatically... > > So it's possible to obtain and verify the GPU power state. > > > > However I removed the bbswitch module and booted with nouveau. > > > > Kernel 4.7.2: nouveau switches the discrete GPU off. > > I can't trigger the freeze, because bbswitch is missing. > > I'll work with the system and see if it will freeze. > > > > Kernel 4.8-rc4: nouveau does not care about the power state and > > the discrete GPU is never switched off. I will notice > > this, because the second cooling FAN will stop... > > Same log messages as send before. > > That's surprising. I believe there's an issue with the new logic when > there's an HDMI audio subdevice. However that only appears if there's > a cable plugged in, at least in the systems Peter tested. You should > be able to see whether it's there or not with 'lspci'. I doubt that the audio device is responsible here, that should only show up after following very specific steps (runtime suspend/resume (PCI or ACPI magic), remove PCI device, rescan bus). > You can check for sure by looking in the vgaswitcheroo state. It > should say DynOff when it's powered off. > > Either way, I think using bbswitch + nouveau isn't supported by > anyone, so if you want to use it that way, you're on your own. (You > may want to load nouveau with runpm=0 so that nouveau doesn't try to > manage the GPU suspend stuff.) I understood that Roland's intent is to check the power state, not use the suspend functionality of bbswitch, if you load bbswitch without module options amd do not write to /proc/bbswitch, then it allows you to read out the actual status (you could also just use lspci -H1 for that though). -- Kind regards, Peter Wu https://lekensteyn.nl
[PATCH] drm/nouveau/acpi: use DSM if bridge does not support D3cold
Even if PR3 support is available on the bridge, it will not be used if
the PCI layer considers it unavailable (i.e. on all laptops from 2013
and 2014). Ensure that this condition is checked to allow a fallback to
the Optimus DSM for device poweroff.
Initially I wanted to call pci_d3cold_enable before checking bridge_d3
(in case the user changed d3cold_allowed), but that is such an unlikely
case and likely fragile anyway. The current patch is suggested by Mika
in http://www.spinics.net/lists/linux-pci/msg52599.html
Cc: Mika Westerberg
Signed-off-by: Peter Wu
---
Hi,
This idea is floating since July, but was blocked by the PCI D3cold patches.
Since these got into -rc1, here is the promised follow-up patch for v4.8.
Without this patch, some laptops from 2013 and 2014 will regress in v4.8 and
suck more power.
Kind regards,
Peter
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index f2ad17a..dc57b62 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -225,6 +225,17 @@ static bool nouveau_pr3_present(struct pci_dev *pdev)
if (!parent_pdev)
return false;
+ if (!parent_pdev->bridge_d3) {
+ /*
+* Parent PCI bridge is currently not power managed.
+* Since userspace can change these afterwards to be on
+* the safe side we stick with _DSM and prevent usage of
+* _PR3 from the bridge.
+*/
+ pci_d3cold_disable(pdev);
+ return false;
+ }
+
parent_adev = ACPI_COMPANION(_pdev->dev);
if (!parent_adev)
return false;
--
2.9.3
ATPX changes in drm-next-4.8 and D3cold handling
On Fri, Jul 29, 2016 at 03:45:50PM +, Deucher, Alexander wrote: > > -Original Message- > > From: Peter Wu [mailto:peter at lekensteyn.nl] > > Sent: Thursday, July 28, 2016 8:00 PM > > To: Lukas Wunner > > Cc: Deucher, Alexander; dri-devel at lists.freedesktop.org; Christoph Haag; > > Koenig, Christian; amd-gfx at lists.freedesktop.org; Zhang, Hawking > > Subject: Re: ATPX changes in drm-next-4.8 and D3cold handling > > > > On Thu, Jul 28, 2016 at 05:40:31PM +0200, Lukas Wunner wrote: > > > On Thu, Jul 28, 2016 at 03:33:25PM +, Deucher, Alexander wrote: > > > > > From: Peter Wu [mailto:peter at lekensteyn.nl] > > > > > Sent: Thursday, July 21, 2016 6:43 AM > > > > > In case you missed it, Dave's D3cold patches were succeeded by > > changes > > > > > in PCI core. Relevant commits in the pci/pm branch: > > > > > > > > > > 006d44e PCI: Add runtime PM support for PCIe ports > > > > > 16468c7 ACPI / hotplug / PCI: Runtime resume bridge before rescan > > > > > d963f65 PCI: Power on bridges before scanning new devices > > > > > 9d26d3a PCI: Put PCIe ports into D3 during suspend > > > > > 43f7f88 PCI: Don't clear d3cold_allowed for PCIe ports > > > > > > > > Did those get merged yet? > > > > > > They will go into 4.8. Should have gone into 4.7 already but were > > > dropped at the last minute. > > > > > > > > > > I just need to revert this commit once the d3cold patches land: > > > > https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next- > > 4.8=bdfb76040068d960cb9e226876be8a508d741c4a > > > > > > So you probably need to revert this now. > > > > > > Best regards, > > > Lukas > > > > It is better to revert it before the PCI/PM patches get merged, > > otherwise you risk that the device is already put in D3 before the > > bridge tries to do it again. This is currently happening with nouveau on > > -next. > > > > Do these AMD hw exist on BIOSes pre-2015? Currently the D3cold work in > > the PCI/PM branch only enable the D3cold handling via the bridge when > > the BIOS is >= 2015. > > Systems designed for windows 10 use d3 cold rather than the legacy > interfaces. Setting the ACPI OSI to windows10 will enable d3cold, > setting it to a previous version of windows will use the old method. > At least on AMD PX systems there is a bit in the ATPX information > block that indicates the current setting hybrid graphics (aka d3cold) > or the ATPX power control for dGPU power control. > > Alex Windows 10 is Windows 2015, so BIOS dates for new Windows 10 devices should be newer or equal to 2015 and no problem should occur. No worries! My initial concern was from the blacklist for ore-2015 BIOSes as can be seen in function pci_bridge_d3_possible in https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pm=9d26d3a8f1b0c442339a235f9508bdad8af91043 Kind regards, Peter
ATPX changes in drm-next-4.8 and D3cold handling
On Thu, Jul 28, 2016 at 05:40:31PM +0200, Lukas Wunner wrote: > On Thu, Jul 28, 2016 at 03:33:25PM +, Deucher, Alexander wrote: > > > From: Peter Wu [mailto:peter at lekensteyn.nl] > > > Sent: Thursday, July 21, 2016 6:43 AM > > > In case you missed it, Dave's D3cold patches were succeeded by changes > > > in PCI core. Relevant commits in the pci/pm branch: > > > > > > 006d44e PCI: Add runtime PM support for PCIe ports > > > 16468c7 ACPI / hotplug / PCI: Runtime resume bridge before rescan > > > d963f65 PCI: Power on bridges before scanning new devices > > > 9d26d3a PCI: Put PCIe ports into D3 during suspend > > > 43f7f88 PCI: Don't clear d3cold_allowed for PCIe ports > > > > Did those get merged yet? > > They will go into 4.8. Should have gone into 4.7 already but were > dropped at the last minute. > > > > I just need to revert this commit once the d3cold patches land: > > https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-4.8=bdfb76040068d960cb9e226876be8a508d741c4a > > So you probably need to revert this now. > > Best regards, > Lukas It is better to revert it before the PCI/PM patches get merged, otherwise you risk that the device is already put in D3 before the bridge tries to do it again. This is currently happening with nouveau on -next. Do these AMD hw exist on BIOSes pre-2015? Currently the D3cold work in the PCI/PM branch only enable the D3cold handling via the bridge when the BIOS is >= 2015. -- Kind regards, Peter Wu https://lekensteyn.nl
[Nouveau] [PATCH v3 0/4] nouveau RPM fixes for Optimus (final)
Ping, would it be possible to get some acks and merge it for 4.8? Current -next is broken (on modern laptops as expected) and these series fix the issues according to an IRC report. The audio issue mentioned below should not give issues, modern laptops do not seem to expose the audio device by default (https://bugs.freedesktop.org/show_bug.cgi?id=75985). On Windows the audio device only appears after inserting the HDMI/miniDP cable (my laptop has no other connectors), enabling the card for rendering purposes has no effect on the availability of the audio device. Kind regards, Peter On Fri, Jul 15, 2016 at 03:12:14PM +0200, Peter Wu wrote: > Hi, > > Here are two patches to fix an issue reported on kernel bugzilla (infinite > loop > due to unchecked function) and a more important fix to fix hanging Optimus > machines when runtime PM is enabled (with pm/pci patches). > > These are the final patches targeting v4.8. Changes compared to v2[1]: > collected R-b from Hans and Mika and fixed a minor comment style issue. > > I recommend it to be merged before the pci/pm patches[2], otherwise there is a > window where newer Nvidia Optimus laptops might fail to runtime resume and/or > lock up. Once the pci/pm branch is merged I will propose another patch to > improve reliability[3]. > > Known issue with patch 4: when a Nvidia HDMI audio function is present, the > bridge will not suspend and hence the Nvidia card will still be powered. > Fixing > this properly will require more work[4], until then you can kill the audio > device and make runtime PM work properly: > > echo 1 > /sys/bus/pci/devices/:01:00.1/remove > > Kind regards, > Peter > > [1]: https://lists.freedesktop.org/archives/nouveau/2016-July/025519.html > [2]: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/?h=pci/pm > [3]: http://www.spinics.net/lists/linux-pci/msg52601.html > [4]: https://lists.freedesktop.org/archives/dri-devel/2016-July/112759.html > > Peter Wu (4): > drm/nouveau/acpi: ensure matching ACPI handle and supported functions > drm/nouveau/acpi: return supported DSM functions > drm/nouveau/acpi: check for function 0x1B before using it > drm/nouveau/acpi: fix lockup with PCIe runtime PM > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 105 > + > 1 file changed, 68 insertions(+), 37 deletions(-) > > -- > 2.9.0
ATPX changes in drm-next-4.8 and D3cold handling
Hi Alex, There are a couple of changes for 4.8 that try to detect whether the "power_cntl" flag is present. Originally attributed to a firmware bug, it seems that the detection is performed too late resulting in flags that are always zero (https://bugzilla.kernel.org/show_bug.cgi?id=115321). What PX platform are these patches tested with, did they have the same issue? In case you missed it, Dave's D3cold patches were succeeded by changes in PCI core. Relevant commits in the pci/pm branch: 006d44e PCI: Add runtime PM support for PCIe ports 16468c7 ACPI / hotplug / PCI: Runtime resume bridge before rescan d963f65 PCI: Power on bridges before scanning new devices 9d26d3a PCI: Put PCIe ports into D3 during suspend 43f7f88 PCI: Don't clear d3cold_allowed for PCIe ports With these changes, the nouveau driver had to disable use of the _DSM ACPI method (comparable to ATPX), otherwise both interfaces are used which could cause issues like being unable to resume the device. Also note that pcieport currently only handles D3cold for devices with a BIOS date in 2015 (or newer), you need to detect this with an approach like http://www.spinics.net/lists/linux-pci/msg52602.html We also found that the Nvidia HDMI audio device (function 1) would prevent the pcieport from sleeping. For modern Nvidia hardware this is apparently not an issue because these somehow hide the audio device, but it might be an issue for AMD hardware. See also https://lists.freedesktop.org/archives/dri-devel/2016-July/112759.html -- Kind regards, Peter Wu https://lekensteyn.nl
[Nouveau] [PATCH v3 0/4] nouveau RPM fixes for Optimus (final)
On Fri, Jul 15, 2016 at 06:54:27PM +0200, Peter Wu wrote: > On Fri, Jul 15, 2016 at 12:41:49PM -0400, Ilia Mirkin wrote: > > On Fri, Jul 15, 2016 at 12:36 PM, Peter Wu wrote: > > > On Fri, Jul 15, 2016 at 12:10:23PM -0400, Ilia Mirkin wrote: > > >> On Fri, Jul 15, 2016 at 9:12 AM, Peter Wu wrote: > > >> > Hi, > > >> > > > >> > Here are two patches to fix an issue reported on kernel bugzilla > > >> > (infinite loop > > >> > due to unchecked function) and a more important fix to fix hanging > > >> > Optimus > > >> > machines when runtime PM is enabled (with pm/pci patches). > > >> > > > >> > These are the final patches targeting v4.8. Changes compared to v2[1]: > > >> > collected R-b from Hans and Mika and fixed a minor comment style issue. > > >> > > > >> > I recommend it to be merged before the pci/pm patches[2], otherwise > > >> > there is a > > >> > window where newer Nvidia Optimus laptops might fail to runtime resume > > >> > and/or > > >> > lock up. Once the pci/pm branch is merged I will propose another > > >> > patch to > > >> > improve reliability[3]. > > >> > > > >> > Known issue with patch 4: when a Nvidia HDMI audio function is > > >> > present, the > > >> > bridge will not suspend and hence the Nvidia card will still be > > >> > powered. Fixing > > >> > > >> That's basically all optimus gpu's, right? Anything GT21x+ has a HDMI > > >> audio subfunction, and prior to that, the nvidia gpu tended to be the > > >> only gpu, or hard-muxed. > > >> > > >> If that's the case, that's pretty much a non-starter, IMO. > > > > > > For some reason the audio function tends to disappear/hide, so maybe it > > > is not as problematic as it appears (see > > > https://bugs.freedesktop.org/show_bug.cgi?id=75985). For my laptop I > > > > I'm aware of that bug. I believe this is an exceedingly rare scenario > > or it would have been reported a lot more. > > > > > also had to runtime suspend/resume before lspci -H1 shows the device, > > > loading with runpm=0 didn't return my HDMI audio device. > > > > Hm ok. Do you have the same laptop as the reporter of that bug? > > Nope, I have a Clevo P651RA (GTX965M). That reporter has a Dell XPS 15, > but it also seems present for the Lenovo ThinkPad T420s (see comment on > bug), Asus N56VZ, MSI GT60 2PE, Dell L502x (Launchpad 1377653), Asus > G46vw (Ask Ubuntu user). There is another AU report for a GT 525M > (laptop brand/model unknown). > > Maybe there are more affected users, but then they did not notice it > because they did not use HDMI audio. > > > > > > > The powered on issue will also only appear on devices produced in 2013 > > > and newer that happen to have this ACPI _PR3 ACPI method (which is quite > > > common for new machines supporting Windows 8 though). > > > > > > For these newer laptops, after the pci/pm merge and after a patch like > > > http://www.spinics.net/lists/linux-pci/msg52601.html, the user can > > > revert to the old DSM method by booting with pcie_port_pm=off which will > > > retain the current behavior. > > > > > > The advantage of this patch is that it fixes memory corruption on some > > > devices. The risk is that the card stays on because the audio subsystem > > > needs some more work. FWIW, I was working on some patches that properly > > > suspended in presence of the HDA controller, but somehow the audio > > > device was not properly resumed resulting in "no AFG or MFG node found" > > > and "snd_hda_intel :01:00.1: no codecs initialized". > > > > Does this restriction (runpm being broken in presence of the audio > > subfunction) only affect devices with _PR3? If so, that's a lot more > > palatable - I bet Windows 8+ is in an era when the display-less thing > > became more popular, and thus less likely to affect a ton of people. > > Yes it only affects those devices with _PR3. I downloaded all .tar.gz files from the big Launchpad bug that collects DSDTs (and more recently also dmidecode/lspci) and ran an analysis. The result (limited to files which actually had a lspci and dmidecode file): - 111 Nvidia video devices - 20 out of these have an audio device. - 18 use _PR3, 93 use DSM (or gmux). - Exactly zero use _PR3 and have an audio device! The post-proce
[Nouveau] [PATCH v3 0/4] nouveau RPM fixes for Optimus (final)
On Fri, Jul 15, 2016 at 12:41:49PM -0400, Ilia Mirkin wrote: > On Fri, Jul 15, 2016 at 12:36 PM, Peter Wu wrote: > > On Fri, Jul 15, 2016 at 12:10:23PM -0400, Ilia Mirkin wrote: > >> On Fri, Jul 15, 2016 at 9:12 AM, Peter Wu wrote: > >> > Hi, > >> > > >> > Here are two patches to fix an issue reported on kernel bugzilla > >> > (infinite loop > >> > due to unchecked function) and a more important fix to fix hanging > >> > Optimus > >> > machines when runtime PM is enabled (with pm/pci patches). > >> > > >> > These are the final patches targeting v4.8. Changes compared to v2[1]: > >> > collected R-b from Hans and Mika and fixed a minor comment style issue. > >> > > >> > I recommend it to be merged before the pci/pm patches[2], otherwise > >> > there is a > >> > window where newer Nvidia Optimus laptops might fail to runtime resume > >> > and/or > >> > lock up. Once the pci/pm branch is merged I will propose another patch > >> > to > >> > improve reliability[3]. > >> > > >> > Known issue with patch 4: when a Nvidia HDMI audio function is present, > >> > the > >> > bridge will not suspend and hence the Nvidia card will still be powered. > >> > Fixing > >> > >> That's basically all optimus gpu's, right? Anything GT21x+ has a HDMI > >> audio subfunction, and prior to that, the nvidia gpu tended to be the > >> only gpu, or hard-muxed. > >> > >> If that's the case, that's pretty much a non-starter, IMO. > > > > For some reason the audio function tends to disappear/hide, so maybe it > > is not as problematic as it appears (see > > https://bugs.freedesktop.org/show_bug.cgi?id=75985). For my laptop I > > I'm aware of that bug. I believe this is an exceedingly rare scenario > or it would have been reported a lot more. > > > also had to runtime suspend/resume before lspci -H1 shows the device, > > loading with runpm=0 didn't return my HDMI audio device. > > Hm ok. Do you have the same laptop as the reporter of that bug? Nope, I have a Clevo P651RA (GTX965M). That reporter has a Dell XPS 15, but it also seems present for the Lenovo ThinkPad T420s (see comment on bug), Asus N56VZ, MSI GT60 2PE, Dell L502x (Launchpad 1377653), Asus G46vw (Ask Ubuntu user). There is another AU report for a GT 525M (laptop brand/model unknown). Maybe there are more affected users, but then they did not notice it because they did not use HDMI audio. > > > > The powered on issue will also only appear on devices produced in 2013 > > and newer that happen to have this ACPI _PR3 ACPI method (which is quite > > common for new machines supporting Windows 8 though). > > > > For these newer laptops, after the pci/pm merge and after a patch like > > http://www.spinics.net/lists/linux-pci/msg52601.html, the user can > > revert to the old DSM method by booting with pcie_port_pm=off which will > > retain the current behavior. > > > > The advantage of this patch is that it fixes memory corruption on some > > devices. The risk is that the card stays on because the audio subsystem > > needs some more work. FWIW, I was working on some patches that properly > > suspended in presence of the HDA controller, but somehow the audio > > device was not properly resumed resulting in "no AFG or MFG node found" > > and "snd_hda_intel :01:00.1: no codecs initialized". > > Does this restriction (runpm being broken in presence of the audio > subfunction) only affect devices with _PR3? If so, that's a lot more > palatable - I bet Windows 8+ is in an era when the display-less thing > became more popular, and thus less likely to affect a ton of people. Yes it only affects those devices with _PR3. -- Kind regards, Peter Wu https://lekensteyn.nl
[Nouveau] [PATCH v3 0/4] nouveau RPM fixes for Optimus (final)
On Fri, Jul 15, 2016 at 12:10:23PM -0400, Ilia Mirkin wrote: > On Fri, Jul 15, 2016 at 9:12 AM, Peter Wu wrote: > > Hi, > > > > Here are two patches to fix an issue reported on kernel bugzilla (infinite > > loop > > due to unchecked function) and a more important fix to fix hanging Optimus > > machines when runtime PM is enabled (with pm/pci patches). > > > > These are the final patches targeting v4.8. Changes compared to v2[1]: > > collected R-b from Hans and Mika and fixed a minor comment style issue. > > > > I recommend it to be merged before the pci/pm patches[2], otherwise there > > is a > > window where newer Nvidia Optimus laptops might fail to runtime resume > > and/or > > lock up. Once the pci/pm branch is merged I will propose another patch to > > improve reliability[3]. > > > > Known issue with patch 4: when a Nvidia HDMI audio function is present, the > > bridge will not suspend and hence the Nvidia card will still be powered. > > Fixing > > That's basically all optimus gpu's, right? Anything GT21x+ has a HDMI > audio subfunction, and prior to that, the nvidia gpu tended to be the > only gpu, or hard-muxed. > > If that's the case, that's pretty much a non-starter, IMO. For some reason the audio function tends to disappear/hide, so maybe it is not as problematic as it appears (see https://bugs.freedesktop.org/show_bug.cgi?id=75985). For my laptop I also had to runtime suspend/resume before lspci -H1 shows the device, loading with runpm=0 didn't return my HDMI audio device. The powered on issue will also only appear on devices produced in 2013 and newer that happen to have this ACPI _PR3 ACPI method (which is quite common for new machines supporting Windows 8 though). For these newer laptops, after the pci/pm merge and after a patch like http://www.spinics.net/lists/linux-pci/msg52601.html, the user can revert to the old DSM method by booting with pcie_port_pm=off which will retain the current behavior. The advantage of this patch is that it fixes memory corruption on some devices. The risk is that the card stays on because the audio subsystem needs some more work. FWIW, I was working on some patches that properly suspended in presence of the HDA controller, but somehow the audio device was not properly resumed resulting in "no AFG or MFG node found" and "snd_hda_intel :01:00.1: no codecs initialized". -- Kind regards, Peter Wu https://lekensteyn.nl
[PATCH v3 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
can be runtime-suspended which disables power resources via ACPI. This
is incompatible with DSM, resulting in a GPU device which is still in D3
and locks up the kernel on resume (on a Clevo P651RA, GTX965M).
Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
debugger trace) and stop using the DSM functions for D3cold when power
resources are available on the parent PCIe port.
pci_d3cold_disable() is not used because on some machines, the old DSM
method is broken. On a Lenovo T440p (GT 730M) memory and disk corruption
would occur, but that is fixed with this patch[2].
[1]:
https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
[2]:
https://github.com/Bumblebee-Project/bbswitch/issues/78#issuecomment-223549072
v2: simply check directly for _PR3. Added affected machines.
v3: fixed block comment coding style.
Reviewed-by: Mika Westerberg
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 35 ++
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index ad273ad..f2ad17a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
bool optimus_flags_detected;
+ bool optimus_skip_dsm;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,9 +213,28 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
+/*
+ * Firmware supporting Windows 8 or later do not use _DSM to put the device
into
+ * D3cold, they instead rely on disabling power resources on the parent.
+ */
+static bool nouveau_pr3_present(struct pci_dev *pdev)
+{
+ struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
+ struct acpi_device *parent_adev;
+
+ if (!parent_pdev)
+ return false;
+
+ parent_adev = ACPI_COMPANION(_pdev->dev);
+ if (!parent_adev)
+ return false;
+
+ return acpi_has_method(parent_adev->handle, "_PR3");
+}
+
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
bool *has_mux, bool *has_opt,
- bool *has_opt_flags)
+ bool *has_opt_flags, bool *has_pr3)
{
acpi_handle dhandle;
bool supports_mux;
@@ -239,6 +259,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
*has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
+ *has_pr3 = false;
if (optimus_funcs) {
uint32_t result;
@@ -248,6 +269,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
(result & OPTIMUS_ENABLED) ? "enabled" : "disabled",
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
+
+ *has_pr3 = nouveau_pr3_present(pdev);
}
}
@@ -260,6 +283,7 @@ static bool nouveau_dsm_detect(void)
bool has_mux = false;
bool has_optimus = false;
bool has_optimus_flags = false;
+ bool has_power_resources = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -275,14 +299,14 @@ static bool nouveau_dsm_detect(void)
vga_count++;
nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
/* find the optimus DSM or the old v1 DSM */
@@ -292,8 +316,11 @@ static bool nouveau_dsm_detect(void)
);
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
+ if (has_power_resources)
+ pr_info("nouveau: detected PR support, will not use
DSM\n");
nouveau_dsm_priv.optimus_detected = true;
nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
+
[PATCH v3 3/4] drm/nouveau/acpi: check for function 0x1B before using it
Do not unconditionally invoke function 0x1B without checking for its
availability, it leads to an infinite loop on some firmware.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=104791
Fixes: 5addcf0a5f0fad ("nouveau: add runtime PM support (v0.9)")
Reviewed-by: Hans de Goede
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 572ac30..ad273ad 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -45,6 +45,7 @@
static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
+ bool optimus_flags_detected;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,7 +213,8 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
};
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
- bool *has_mux, bool *has_opt)
+ bool *has_mux, bool *has_opt,
+ bool *has_opt_flags)
{
acpi_handle dhandle;
bool supports_mux;
@@ -236,6 +238,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
*dhandle_out = dhandle;
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
+ *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
if (optimus_funcs) {
uint32_t result;
@@ -256,6 +259,7 @@ static bool nouveau_dsm_detect(void)
acpi_handle dhandle = NULL;
bool has_mux = false;
bool has_optimus = false;
+ bool has_optimus_flags = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -270,13 +274,15 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
+ _optimus_flags);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
+ _optimus_flags);
}
/* find the optimus DSM or the old v1 DSM */
@@ -287,6 +293,7 @@ static bool nouveau_dsm_detect(void)
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
nouveau_dsm_priv.optimus_detected = true;
+ nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
ret = true;
} else if (vga_count == 2 && has_mux && guid_valid) {
nouveau_dsm_priv.dhandle = dhandle;
@@ -320,8 +327,9 @@ void nouveau_switcheroo_optimus_dsm(void)
if (!nouveau_dsm_priv.optimus_detected)
return;
- nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,
- 0x3, );
+ if (nouveau_dsm_priv.optimus_flags_detected)
+ nouveau_optimus_dsm(nouveau_dsm_priv.dhandle,
NOUVEAU_DSM_OPTIMUS_FLAGS,
+ 0x3, );
nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_CAPS,
NOUVEAU_DSM_OPTIMUS_SET_POWERDOWN, );
--
2.9.0
[PATCH v3 2/4] drm/nouveau/acpi: return supported DSM functions
Return the set of supported functions to the caller. No functional
changes.
Reviewed-by: Hans de Goede
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 886a67c..572ac30 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -107,7 +107,7 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
* requirements on the fourth parameter, so a private implementation
* instead of using acpi_check_dsm().
*/
-static int nouveau_check_optimus_dsm(acpi_handle handle)
+static int nouveau_dsm_get_optimus_functions(acpi_handle handle)
{
int result;
@@ -122,7 +122,9 @@ static int nouveau_check_optimus_dsm(acpi_handle handle)
* ACPI Spec v4 9.14.1: if bit 0 is zero, no function is supported.
* If the n-th bit is enabled, function n is supported
*/
- return result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS);
+ if (result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS))
+ return result;
+ return 0;
}
static int nouveau_dsm(acpi_handle handle, int func, int arg)
@@ -214,7 +216,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
{
acpi_handle dhandle;
bool supports_mux;
- bool supports_opt;
+ int optimus_funcs;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
@@ -225,17 +227,17 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
1 << NOUVEAU_DSM_POWER);
- supports_opt = nouveau_check_optimus_dsm(dhandle);
+ optimus_funcs = nouveau_dsm_get_optimus_functions(dhandle);
/* Does not look like a Nvidia device. */
- if (!supports_mux && !supports_opt)
+ if (!supports_mux && !optimus_funcs)
return;
*dhandle_out = dhandle;
*has_mux = supports_mux;
- *has_opt = supports_opt;
+ *has_opt = !!optimus_funcs;
- if (supports_opt) {
+ if (optimus_funcs) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
--
2.9.0
[PATCH v3 1/4] drm/nouveau/acpi: ensure matching ACPI handle and supported functions
Ensure that the returned set of supported DSM functions (MUX, Optimus)
match the ACPI handle that is set in nouveau_dsm_pci_probe.
As there are no machines with a MUX function on just one PCI device and
an Optimus on another, there should not be a functional impact. This
change however makes this implicit assumption more obvious.
Convert int to bool and rename has_dsm to has_mux while at it. Let the
caller set nouveau_dsm_priv.dhandle as needed.
v2: pass dhandle to the caller.
Reviewed-by: Hans de Goede
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 58 +++---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index db76b94..886a67c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -57,9 +57,6 @@ bool nouveau_is_v1_dsm(void) {
return nouveau_dsm_priv.dsm_detected;
}
-#define NOUVEAU_DSM_HAS_MUX 0x1
-#define NOUVEAU_DSM_HAS_OPT 0x2
-
#ifdef CONFIG_VGA_SWITCHEROO
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
@@ -212,26 +209,33 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
-static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
+static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
+ bool *has_mux, bool *has_opt)
{
acpi_handle dhandle;
- int retval = 0;
+ bool supports_mux;
+ bool supports_opt;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
- return false;
+ return;
if (!acpi_has_method(dhandle, "_DSM"))
- return false;
+ return;
- if (acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
- 1 << NOUVEAU_DSM_POWER))
- retval |= NOUVEAU_DSM_HAS_MUX;
+ supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
+ 1 << NOUVEAU_DSM_POWER);
+ supports_opt = nouveau_check_optimus_dsm(dhandle);
- if (nouveau_check_optimus_dsm(dhandle))
- retval |= NOUVEAU_DSM_HAS_OPT;
+ /* Does not look like a Nvidia device. */
+ if (!supports_mux && !supports_opt)
+ return;
- if (retval & NOUVEAU_DSM_HAS_OPT) {
+ *dhandle_out = dhandle;
+ *has_mux = supports_mux;
+ *has_opt = supports_opt;
+
+ if (supports_opt) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
@@ -240,10 +244,6 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
}
- if (retval)
- nouveau_dsm_priv.dhandle = dhandle;
-
- return retval;
}
static bool nouveau_dsm_detect(void)
@@ -251,11 +251,11 @@ static bool nouveau_dsm_detect(void)
char acpi_method_name[255] = { 0 };
struct acpi_buffer buffer = {sizeof(acpi_method_name),
acpi_method_name};
struct pci_dev *pdev = NULL;
- int has_dsm = 0;
- int has_optimus = 0;
+ acpi_handle dhandle = NULL;
+ bool has_mux = false;
+ bool has_optimus = false;
int vga_count = 0;
bool guid_valid;
- int retval;
bool ret = false;
/* lookup the MXM GUID */
@@ -268,32 +268,26 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
}
/* find the optimus DSM or the old v1 DSM */
- if (has_optimus == 1) {
+ if (has_optimus) {
+ nouveau_dsm_priv.dhandle = dhandle;
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
);
prin
[PATCH v3 0/4] nouveau RPM fixes for Optimus (final)
Hi, Here are two patches to fix an issue reported on kernel bugzilla (infinite loop due to unchecked function) and a more important fix to fix hanging Optimus machines when runtime PM is enabled (with pm/pci patches). These are the final patches targeting v4.8. Changes compared to v2[1]: collected R-b from Hans and Mika and fixed a minor comment style issue. I recommend it to be merged before the pci/pm patches[2], otherwise there is a window where newer Nvidia Optimus laptops might fail to runtime resume and/or lock up. Once the pci/pm branch is merged I will propose another patch to improve reliability[3]. Known issue with patch 4: when a Nvidia HDMI audio function is present, the bridge will not suspend and hence the Nvidia card will still be powered. Fixing this properly will require more work[4], until then you can kill the audio device and make runtime PM work properly: echo 1 > /sys/bus/pci/devices/:01:00.1/remove Kind regards, Peter [1]: https://lists.freedesktop.org/archives/nouveau/2016-July/025519.html [2]: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/?h=pci/pm [3]: http://www.spinics.net/lists/linux-pci/msg52601.html [4]: https://lists.freedesktop.org/archives/dri-devel/2016-July/112759.html Peter Wu (4): drm/nouveau/acpi: ensure matching ACPI handle and supported functions drm/nouveau/acpi: return supported DSM functions drm/nouveau/acpi: check for function 0x1B before using it drm/nouveau/acpi: fix lockup with PCIe runtime PM drivers/gpu/drm/nouveau/nouveau_acpi.c | 105 + 1 file changed, 68 insertions(+), 37 deletions(-) -- 2.9.0
[PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
On Wed, Jul 13, 2016 at 06:17:47PM +0100, Chris Wilson wrote:
> On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> > The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> > while nouveau was runtime suspended, a deadlock would occur due to
> > nouveau_fbcon_set_suspend also trying to obtain console_lock().
> >
> > Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> > i915 code (which was done for performance reasons though).
> >
> > Cc: Chris Wilson
> > Cc: Daniel Vetter
> > Signed-off-by: Peter Wu
> > ---
> > Tested on top of v4.7-rc5, the deadlock is gone.
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 4 +--
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 54
> > -
> > drivers/gpu/drm/nouveau/nouveau_fbcon.h | 2 +-
> > 4 files changed, 50 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 11f8dd9..f9a2c10 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -552,7 +552,7 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
> >
> > if (dev->mode_config.num_crtc) {
> > NV_INFO(drm, "suspending console...\n");
> > - nouveau_fbcon_set_suspend(dev, 1);
> > + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> > NV_INFO(drm, "suspending display...\n");
> > ret = nouveau_display_suspend(dev, runtime);
> > if (ret)
> > @@ -635,7 +635,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
> > NV_INFO(drm, "resuming display...\n");
> > nouveau_display_resume(dev, runtime);
> > NV_INFO(drm, "resuming console...\n");
> > - nouveau_fbcon_set_suspend(dev, 0);
> > + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> > }
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 822a021..a743d19 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -147,6 +147,7 @@ struct nouveau_drm {
> > struct nouveau_channel *channel;
> > struct nvkm_gpuobj *notify;
> > struct nouveau_fbdev *fbcon;
> > + struct work_struct fbdev_suspend_work;
> > struct nvif_object nvsw;
> > struct nvif_object ntfy;
> > struct nvif_notify flip;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> > b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> > index d1f248f..089156a 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> > @@ -492,19 +492,53 @@ static const struct drm_fb_helper_funcs
> > nouveau_fbcon_helper_funcs = {
> > .fb_probe = nouveau_fbcon_create,
> > };
> >
> > +static void nouveau_fbcon_suspend_worker(struct work_struct *work)
> > +{
> > + nouveau_fbcon_set_suspend(container_of(work,
> > + struct nouveau_drm,
> > + fbdev_suspend_work)->dev,
> > + FBINFO_STATE_RUNNING,
> > + true);
> > +}
> > +
> > void
> > -nouveau_fbcon_set_suspend(struct drm_device *dev, int state)
> > +nouveau_fbcon_set_suspend(struct drm_device *dev, int state, bool
> > synchronous)
> > {
> > struct nouveau_drm *drm = nouveau_drm(dev);
> > - if (drm->fbcon) {
> > - console_lock();
> > - if (state == FBINFO_STATE_RUNNING)
> > - nouveau_fbcon_accel_restore(dev);
> > - drm_fb_helper_set_suspend(>fbcon->helper, state);
> > + if (!drm->fbcon)
> > + return;
> > +
> > + if (synchronous) {
> > + /* Flush any pending work to turn the console on, and then
> > +* wait to turn it off. It must be synchronous as we are
> > +* about to suspend or unload the driver.
> > +*
> > +* Note that from within the work-handler, we cannot flush
> > +* ourselves, so only flush outstanding work upon suspend!
> > +*/
> > if (state != FBINFO_STATE_RUNNING)
> > -
[PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
On Wed, Jul 13, 2016 at 04:57:19PM +0200, Daniel Vetter wrote:
> On Wed, Jul 13, 2016 at 02:40:50PM +0200, Peter Wu wrote:
> > On Wed, Jul 13, 2016 at 11:54:49AM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> > > > The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> > > > while nouveau was runtime suspended, a deadlock would occur due to
> > > > nouveau_fbcon_set_suspend also trying to obtain console_lock().
> > > >
> > > > Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> > > > i915 code (which was done for performance reasons though).
> > > >
> > > > Cc: Chris Wilson
> > > > Cc: Daniel Vetter
> > > > Signed-off-by: Peter Wu
> > > > ---
> > > > Tested on top of v4.7-rc5, the deadlock is gone.
> > >
> > > If we bother with this, it should imo be moved into the drm_fb_helper.c
> > > function drm_fb_helper_set_suspend(). But this also smells like some kind
> > > of bad duct-tape. I think Lukas is working on some other rpm vs. fbdev
> > > deadlocks, maybe we could fix them all with one proper fix? I've made some
> > > comments on Lukas' last patch series.
> >
> > This patch is only needed for drivers that use console_lock (for
> > drm_fb_helper_set_suspend) in their runtime resume functions.
> > Lukas posted fixes for runtime PM reference leaks, those are different
> > from this deadlock (see
> > https://lists.freedesktop.org/archives/dri-devel/2016-July/113005.html
> > for a backtrace for this issue).
> >
> > The deadlock could also be avoided if the device backing the fbcon is
> > somehow runtime-resumed outside the lock, but that feels like a larger
> > hack that does not seem easy.
> >
> > The i915 patch was done to reduce resume time (due to console_lock
> > contention), that feature seems useful to all other drivers too even if
> > the deadlock is fixed in a different way.
>
> I might have imagined something, but I thought Lukas is already working on
> some rpm vs. vga_switcheroo inversions. But looking again this was on the
> audio side.
>
> I think the proper solution for fbcon would be for the fbdev emulation to
> also hold a runtime pm references when the console is in use.
nouveau does this by adding a fb_open and fb_release function that calls
pm_runtime_get and pm_runtime_put respectively (and is the only driver
doing this). So that is why it causes the console_lock issue for
nouveau, but not for others.
> This should already happen correctly for vblank, the more tricky part
> is fbdev mmap and fbcon:
>
> - I have no idea, but there should be a way to intercept fbdev userspace
> mmaps and make sure that as long as an app has the fbdev mmapped we
> don't runtime suspend. No one really should be doing that (at least for
> normal setups), hence this shouldn't result in unsafe.
mmap normally needs a fd right? When the chardev /dev/fbX is opened, the
fb_open function will be called. So nouveau should not have this issue
with mmap/read/write to a fb while the device is suspended.
(This RPM thing was added in f231976c2e89 ("drm/nouveau/fbcon: take
runpm reference when userspace has an open fd"), maybe it is not a bad
idea for other drivers with RPM support either.)
> - For fbcon, we could suspend it in the dpms off callbacks (maybe with a
> timer), and resume it only when enabling fbcon again - fbcon needs to
> redraw anyway on dpms on.
Would this guarantee that the fb is suspended before/during suspend (of
the graphics device) and resumed somewhere during/after resume?
Suspending the fb also has the effect that reads/writes to /dev/fbN
fail, maybe that is a bit too restricted since the framebuffer is just
accessible until the device is suspended.
(Hmm, fb_read/fb_write does not seem to do any locking...)
> Another solution for fbcon might be to untangle the suspend/resume stuff
> and protect it by something else than console_lock. But that means
> fixing up fbcon locking horror shows.
console_lock seems needed for some code down the call stack, removing it
risks some blow ups.
Some archaeology: this locking problem was introduced with 054430e773c9
("fbcon: fix locking harder"). In the past fb_set_suspend also took the
fb_info lock but that was removed in 9e769ff3f585 ("fb: avoid possible
deadlock caused by fb_set_suspend").
Peter
> > My current plan is to move stuff out of the lock and allow (just)
> > resuming the console to be delayed. Some drivers (nouveau,
> > radeon/amdgpu, i915) do unnecessary stuff under the console lock:
> >
> > - nouveau:
vga_switcheroo audio runpm
On Sun, Jul 10, 2016 at 03:20:13PM +0200, Lukas Wunner wrote: > Hi Peter, > > > [12:42] Lekensteyn: Should the video card always be powered up when a > > register is read from the HDMI audio controller? Or would it be > > better to leave the video card suspended and just fail the HDA > > register reads? > > It should probably be powered up. Seems sensible, a video signal is apparently also required if you want to play audio. > > [12:43] Lekensteyn: I'm trying to figure out how vga_switcheroo should > handle this, maybe l1k has an idea? > > [12:48] Lekensteyn: The current implemented policy is that the video card > > dictates the power state and that the HDMI audio device has to > > adapt (even if it is seemingly in use) > > This current architecture seems to have come about historically (Dave > Airlie first implemented vga_switcheroo with manual power control, > then added runtime pm in a second step). > > It may also be motivated by the fact that the driver core is currently > not supporting dependencies between devices beyond the hierarchical > parent/child relationship. > > Rafael Wysocki (cc:) posted a series in January addressing the latter > problem with so-called "device links". The series was reposted recently > by Marek Szyprowski: > https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1170031.html > > The vga_switcheroo audio handling should probably be reworked to use such > "device links". The result would be that the GPU won't runtime suspend > as long as a ref is held for the audio card. The audio card needs to then > make sure that it releases any refs if it has nothing to do. > > The "device links" series seems to impose more restrictions than we > actually need here, it seems to require that the "supplier" is bound > to a driver before the "consumer" may probe. IOW nouveau needs to be > bound before snd_hda_audio can probe. I'm not sure if that additional > (unnecessary) restriction is a problem. The device links feature looks promising. My initial guess that it is OK to wait for nouveau to become available (as supplier), the audio port (as consumer) probably does not work anyway without a video signal. > I've recently tried to add runtime pm for muxed laptops to vga_switcheroo, > this is something that Pierre Moreau (cc:) has expressed an interest in > for his MacBook Pro. I came across a major roadblock in the form of > vga_switcheroo_set_dynamic_switch(). That function is called from the > DRM driver when the GPU runtime suspends and resumes. It takes the > vgasr_mutex. The problem is, if the user initiates a switch of the mux, > that mutex is already taken in vga_switcheroo_debugfs_write(). If the > GPU we're switching to is asleep, it'll wake up for the switch and > we'll get a deadlock when the DRM driver subsequently calls > vga_switcheroo_set_dynamic_switch(). > > I would like to get rid of vga_switcheroo_set_dynamic_switch() to solve > this. The function has two purposes: Number one, vga_switcheroo updates > its internal power state representation for the GPU. That is actually > pointless for driver power control because we can always query the > driver core for this information (e.g. pm_runtime_suspended()). > The second purpose is to switch the audio client on and off. If we would > use a "device link" to express the dependency between the audio device > and the GPU, we wouldn't need this. So moving to "device links" is > a prerequisite to make runtime pm work for muxed laptops. This internal state is likely a historical artifact due to the manual control (ON / OFF) that was needed in the past. I have recently tried to draw the dependencies on paper and the suspend/resume to dynamic switch flow is not the prettiest ;) Using runtime pm would probably make the dependencies clearer in a logical way. > If you want to take a stab at using "device links" for vga_switcheroo > then please go ahead as I'm swamped with other work. (I've reverse- > engineered retrieval of Apple device properties from EFI this week, > which is needed for Thunderbolt.) Let me know if you have any questions > or need stuff reviewed or whatever. Since the "device links" series > hasn't landed yet, it's still possible I think to ask for feature > requests should it not work for this particular use case. The > linux-pm at vger.kernel.org mailing list is the right place to inquire > about the series. > > Thanks for raising this important question. I'll give this a go after finishing the PR3 nouveau patches and fixing some locking issues. -- Kind regards, Peter Wu https://lekensteyn.nl
[PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
On Wed, Jul 13, 2016 at 11:54:49AM +0200, Daniel Vetter wrote:
> On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> > The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> > while nouveau was runtime suspended, a deadlock would occur due to
> > nouveau_fbcon_set_suspend also trying to obtain console_lock().
> >
> > Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> > i915 code (which was done for performance reasons though).
> >
> > Cc: Chris Wilson
> > Cc: Daniel Vetter
> > Signed-off-by: Peter Wu
> > ---
> > Tested on top of v4.7-rc5, the deadlock is gone.
>
> If we bother with this, it should imo be moved into the drm_fb_helper.c
> function drm_fb_helper_set_suspend(). But this also smells like some kind
> of bad duct-tape. I think Lukas is working on some other rpm vs. fbdev
> deadlocks, maybe we could fix them all with one proper fix? I've made some
> comments on Lukas' last patch series.
This patch is only needed for drivers that use console_lock (for
drm_fb_helper_set_suspend) in their runtime resume functions.
Lukas posted fixes for runtime PM reference leaks, those are different
from this deadlock (see
https://lists.freedesktop.org/archives/dri-devel/2016-July/113005.html
for a backtrace for this issue).
The deadlock could also be avoided if the device backing the fbcon is
somehow runtime-resumed outside the lock, but that feels like a larger
hack that does not seem easy.
The i915 patch was done to reduce resume time (due to console_lock
contention), that feature seems useful to all other drivers too even if
the deadlock is fixed in a different way.
My current plan is to move stuff out of the lock and allow (just)
resuming the console to be delayed. Some drivers (nouveau,
radeon/amdgpu, i915) do unnecessary stuff under the console lock:
- nouveau: I *think* that cleraing/setting FBINFO_HWACCEL_DISABLED
(nouveau_fbcon_accel_restore) is safe outside the lock as the fb is
already suspended before clearing/after setting the flag.
- radeon: since the console is suspended, I don't think that that all
of the code is radeon_resume_kms is really needed.
- amdgpu: same as radeon. Btw, console_lock is leaked on an error path.
- i915: I think that clearing the fb memory can be done outside the
lock too as the console is suspended.
Please correct me if my assumptions are flawed.
> Besides this, when fixing a deadlock pls provide more details about the
> precise callchain and the locks involved in the deadlock. If you
> discovered this using lockdep, then just add the entire lockdep splat to
> the commit message. Otherwise there's lots of guesswork involved here.
> -Daniel
There was no lockdep splat, it was triggered via the ioctl in the commit
message. I'll include the verbose trace from the previous mail in the
next proposed patch to reduce hunting though.
Peter
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 4 +--
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 54
> > -
> > drivers/gpu/drm/nouveau/nouveau_fbcon.h | 2 +-
> > 4 files changed, 50 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 11f8dd9..f9a2c10 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -552,7 +552,7 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
> >
> > if (dev->mode_config.num_crtc) {
> > NV_INFO(drm, "suspending console...\n");
> > - nouveau_fbcon_set_suspend(dev, 1);
> > + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> > NV_INFO(drm, "suspending display...\n");
> > ret = nouveau_display_suspend(dev, runtime);
> > if (ret)
> > @@ -635,7 +635,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
> > NV_INFO(drm, "resuming display...\n");
> > nouveau_display_resume(dev, runtime);
> > NV_INFO(drm, "resuming console...\n");
> > - nouveau_fbcon_set_suspend(dev, 0);
> > + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> > }
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 822a021..a743d19 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -147,6 +147,7 @@ struct nouveau_drm {
> > struct nouveau_channel *channel;
[Nouveau] [PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
On Tue, Jul 12, 2016 at 09:16:22PM +0200, Lukas Wunner wrote:
> On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> > The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> > while nouveau was runtime suspended, a deadlock would occur due to
> > nouveau_fbcon_set_suspend also trying to obtain console_lock().
> >
> > Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> > i915 code (which was done for performance reasons though).
> >
> > Cc: Chris Wilson
> > Cc: Daniel Vetter
> > Signed-off-by: Peter Wu
> > ---
> > Tested on top of v4.7-rc5, the deadlock is gone.
>
> Hm, how did you trigger this deadlock?
>
> Thanks,
> Lukas
Here is a small Python script with hardcoded values:
#!/usr/bin/env python3
# see drivers/video/fbdev/core/fbmem.c ->
# drivers/video/console/fbcon.c for FB_EVENT_SET_CONSOLE_MAP
import array, fcntl
FBIOPUT_CON2FBMAP = 0x4610
console, framebuffer = 6, 1
with open("/dev/fb0") as f:
info = array.array('I', [console, framebuffer])
fcntl.ioctl(f, FBIOPUT_CON2FBMAP, info)
Ensure that the nouveau card is sleeping, then invoke:
python3 con2fbmap.py
If you check /proc/`pidof python3`/stack or the dmesg spew 120 seconds
later, you will see a trace like this on a kernel without this patch:
[ 60.738089] snd_hda_intel :01:00.1: Disabling via vga_switcheroo
[ 60.739810] nouveau :01:00.0: DRM: suspending console...
[ 60.740090] nouveau :01:00.0: DRM: suspending display...
[ 60.740581] nouveau :01:00.0: DRM: evicting buffers...
[ 60.740718] nouveau :01:00.0: DRM: waiting for kernel channels to go
idle...
[ 60.741096] nouveau :01:00.0: DRM: suspending client object trees...
[ 60.748015] nouveau :01:00.0: DRM: suspending kernel object tree...
[ 62.598156] nouveau :01:00.0: power state changed by ACPI to D3cold
[ 66.883880] nouveau :01:00.0: power state changed by ACPI to D0
[ 66.883987] nouveau :01:00.0: restoring config space at offset 0x4 (was
0x100403, writing 0x100407)
[ 66.884017] nouveau :01:00.0: calling nv_msi_ht_cap_quirk_leaf+0x0/0x30
[ 66.884032] nouveau :01:00.0: DRM: resuming kernel object tree...
[ 66.995505] nouveau :01:00.0: priv: GPC0: 419df4 (1f40820e)
[ 66.995512] nouveau :01:00.0: priv: GPC1: 419df4 (1f40820e)
[ 67.014829] nouveau :01:00.0: DRM: resuming client object trees...
[ 67.014905] nouveau :01:00.0: DRM: resuming display...
[ 67.014962] nouveau :01:00.0: DRM: resuming console...
[ 240.619840] INFO: task con2fb:482 blocked for more than 120 seconds.
[ 240.619844] Not tainted 4.7.0-rc1kasan-00011-g5c72d90 #2
[ 240.619845] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[ 240.619858] con2fb D 880769467378 25464 482447 0x
[ 240.619864] 880769467378 845eb340 83ed1708
00ff880769467330
[ 240.619868] 8807762a06e0 8807762a0708 88077629fdd8
88077629fdc0
[ 240.619872] 880772bc6200 88076f221880 88076946
ed00ed28c001
[ 240.619874] Call Trace:
[ 240.619881] [] schedule+0x95/0x1b0
[ 240.619885] [] schedule_timeout+0x3d0/0x8b0
[ 240.619889] [] ? usleep_range+0xe0/0xe0
[ 240.619894] [] ? debug_lockdep_rcu_enabled+0x77/0x90
[ 240.619897] [] ? mark_held_locks+0xc8/0x120
[ 240.619901] [] ? _raw_spin_unlock_irq+0x2c/0x30
[ 240.619904] [] ? trace_hardirqs_on_caller+0x3f9/0x580
[ 240.619907] [] __down+0xff/0x1d0
[ 240.619911] [] ? ww_mutex_unlock+0x270/0x270
[ 240.619925] [] ? _dev_info+0xc2/0xf0
[ 240.619929] [] down+0x63/0x80
[ 240.619933] [] console_lock+0x1e/0x70
[ 240.620012] [] nouveau_fbcon_set_suspend+0x71/0x390
[nouveau]
[ 240.620085] [] nouveau_do_resume+0x2e2/0x380 [nouveau]
[ 240.620157] [] nouveau_pmops_runtime_resume+0xce/0x210
[nouveau]
[ 240.620163] [] ? pci_restore_standard_config+0x70/0x70
[ 240.620167] [] pci_pm_runtime_resume+0x130/0x220
[ 240.620171] [] ? pci_restore_standard_config+0x70/0x70
[ 240.620175] [] __rpm_callback+0x62/0xe0
[ 240.620179] [] ? pci_restore_standard_config+0x70/0x70
[ 240.620182] [] rpm_callback+0x168/0x210
[ 240.620186] [] ? pci_restore_standard_config+0x70/0x70
[ 240.620189] [] rpm_resume+0xbc3/0x1880
[ 240.620193] [] ?
pm_runtime_autosuspend_expiration+0x60/0x60
[ 240.620196] [] ? __pm_runtime_resume+0x6a/0xa0
[ 240.620200] [] __pm_runtime_resume+0x78/0xa0
[ 240.620270] [] nouveau_fbcon_open+0xd0/0x120 [nouveau]
[ 240.620274] [] con2fb_acquire_newinfo+0xc7/0x2c0
[ 240.620277] [] set_con2fb_map+0x728/0xcb0
[ 240.620281] [] fbcon_event_notify+0xaac/0x1f90
[ 240.620285] [] notifier_call_chain+0xc9/0x130
[ 240.620288] [] __blocking_notifier_call_chain+0x70/0xb0
[ 240.620292] [] blocking_notifier_call_chain+0x
[PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
while nouveau was runtime suspended, a deadlock would occur due to
nouveau_fbcon_set_suspend also trying to obtain console_lock().
Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
i915 code (which was done for performance reasons though).
Cc: Chris Wilson
Cc: Daniel Vetter
Signed-off-by: Peter Wu
---
Tested on top of v4.7-rc5, the deadlock is gone.
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 4 +--
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 54 -
drivers/gpu/drm/nouveau/nouveau_fbcon.h | 2 +-
4 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 11f8dd9..f9a2c10 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -552,7 +552,7 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
if (dev->mode_config.num_crtc) {
NV_INFO(drm, "suspending console...\n");
- nouveau_fbcon_set_suspend(dev, 1);
+ nouveau_fbcon_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
NV_INFO(drm, "suspending display...\n");
ret = nouveau_display_suspend(dev, runtime);
if (ret)
@@ -635,7 +635,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
NV_INFO(drm, "resuming display...\n");
nouveau_display_resume(dev, runtime);
NV_INFO(drm, "resuming console...\n");
- nouveau_fbcon_set_suspend(dev, 0);
+ nouveau_fbcon_set_suspend(dev, FBINFO_STATE_RUNNING, false);
}
return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 822a021..a743d19 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -147,6 +147,7 @@ struct nouveau_drm {
struct nouveau_channel *channel;
struct nvkm_gpuobj *notify;
struct nouveau_fbdev *fbcon;
+ struct work_struct fbdev_suspend_work;
struct nvif_object nvsw;
struct nvif_object ntfy;
struct nvif_notify flip;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index d1f248f..089156a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -492,19 +492,53 @@ static const struct drm_fb_helper_funcs
nouveau_fbcon_helper_funcs = {
.fb_probe = nouveau_fbcon_create,
};
+static void nouveau_fbcon_suspend_worker(struct work_struct *work)
+{
+ nouveau_fbcon_set_suspend(container_of(work,
+ struct nouveau_drm,
+ fbdev_suspend_work)->dev,
+ FBINFO_STATE_RUNNING,
+ true);
+}
+
void
-nouveau_fbcon_set_suspend(struct drm_device *dev, int state)
+nouveau_fbcon_set_suspend(struct drm_device *dev, int state, bool synchronous)
{
struct nouveau_drm *drm = nouveau_drm(dev);
- if (drm->fbcon) {
- console_lock();
- if (state == FBINFO_STATE_RUNNING)
- nouveau_fbcon_accel_restore(dev);
- drm_fb_helper_set_suspend(>fbcon->helper, state);
+ if (!drm->fbcon)
+ return;
+
+ if (synchronous) {
+ /* Flush any pending work to turn the console on, and then
+* wait to turn it off. It must be synchronous as we are
+* about to suspend or unload the driver.
+*
+* Note that from within the work-handler, we cannot flush
+* ourselves, so only flush outstanding work upon suspend!
+*/
if (state != FBINFO_STATE_RUNNING)
- nouveau_fbcon_accel_save_disable(dev);
- console_unlock();
+ flush_work(>fbdev_suspend_work);
+ console_lock();
+ } else {
+ /*
+* The console lock can be pretty contented on resume due
+* to all the printk activity. Try to keep it out of the hot
+* path of resume if possible. This also prevents a deadlock
+* with FBIOPUT_CON2FBMAP.
+*/
+ WARN_ON(state != FBINFO_STATE_RUNNING);
+ if (!console_trylock()) {
+ schedule_work(>fbdev_suspend_work);
+ return;
+ }
}
+
+ if (state == FBINFO_STATE_RUNNING)
+ nouveau_fbcon_accel_restore(dev);
+ drm_fb_helper_set_suspend(>fbcon->helper, state);
+ if (state != FBINFO_STATE_RUNNING)
+
[PATCH v2 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
can be runtime-suspended which disables power resources via ACPI. This
is incompatible with DSM, resulting in a GPU device which is still in D3
and locks up the kernel on resume (on a Clevo P651RA, GTX965M).
Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
debugger trace) and stop using the DSM functions for D3cold when power
resources are available on the parent PCIe port.
pci_d3cold_disable() is not used because on some machines, the old DSM
method is broken. On a Lenovo T440p (GT 730M) memory and disk corruption
would occur, but that is fixed with this patch[2].
[1]:
https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
[2]:
https://github.com/Bumblebee-Project/bbswitch/issues/78#issuecomment-223549072
v2: simply check directly for _PR3. Added affected machines.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 33 +
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index ad273ad..38a6445 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
bool optimus_flags_detected;
+ bool optimus_skip_dsm;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,9 +213,26 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
+/* Firmware supporting Windows 8 or later do not use _DSM to put the device
into
+ * D3cold, they instead rely on disabling power resources on the parent. */
+static bool nouveau_pr3_present(struct pci_dev *pdev)
+{
+ struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
+ struct acpi_device *parent_adev;
+
+ if (!parent_pdev)
+ return false;
+
+ parent_adev = ACPI_COMPANION(_pdev->dev);
+ if (!parent_adev)
+ return false;
+
+ return acpi_has_method(parent_adev->handle, "_PR3");
+}
+
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
bool *has_mux, bool *has_opt,
- bool *has_opt_flags)
+ bool *has_opt_flags, bool *has_pr3)
{
acpi_handle dhandle;
bool supports_mux;
@@ -239,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
*has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
+ *has_pr3 = false;
if (optimus_funcs) {
uint32_t result;
@@ -248,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
(result & OPTIMUS_ENABLED) ? "enabled" : "disabled",
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
+
+ *has_pr3 = nouveau_pr3_present(pdev);
}
}
@@ -260,6 +281,7 @@ static bool nouveau_dsm_detect(void)
bool has_mux = false;
bool has_optimus = false;
bool has_optimus_flags = false;
+ bool has_power_resources = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -275,14 +297,14 @@ static bool nouveau_dsm_detect(void)
vga_count++;
nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
/* find the optimus DSM or the old v1 DSM */
@@ -292,8 +314,11 @@ static bool nouveau_dsm_detect(void)
);
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
+ if (has_power_resources)
+ pr_info("nouveau: detected PR support, will not use
DSM\n");
nouveau_dsm_priv.optimus_detected = true;
nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
+ nouveau_dsm_priv.optimus_skip_dsm = has_power_resources;
[PATCH v2 3/4] drm/nouveau/acpi: check for function 0x1B before using it
Do not unconditionally invoke function 0x1B without checking for its
availability, it leads to an infinite loop on some firmware.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=104791
Fixes: 5addcf0a5f0fad ("nouveau: add runtime PM support (v0.9)")
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 572ac30..ad273ad 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -45,6 +45,7 @@
static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
+ bool optimus_flags_detected;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,7 +213,8 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
};
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
- bool *has_mux, bool *has_opt)
+ bool *has_mux, bool *has_opt,
+ bool *has_opt_flags)
{
acpi_handle dhandle;
bool supports_mux;
@@ -236,6 +238,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
*dhandle_out = dhandle;
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
+ *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
if (optimus_funcs) {
uint32_t result;
@@ -256,6 +259,7 @@ static bool nouveau_dsm_detect(void)
acpi_handle dhandle = NULL;
bool has_mux = false;
bool has_optimus = false;
+ bool has_optimus_flags = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -270,13 +274,15 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
+ _optimus_flags);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus,
+ _optimus_flags);
}
/* find the optimus DSM or the old v1 DSM */
@@ -287,6 +293,7 @@ static bool nouveau_dsm_detect(void)
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
nouveau_dsm_priv.optimus_detected = true;
+ nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
ret = true;
} else if (vga_count == 2 && has_mux && guid_valid) {
nouveau_dsm_priv.dhandle = dhandle;
@@ -320,8 +327,9 @@ void nouveau_switcheroo_optimus_dsm(void)
if (!nouveau_dsm_priv.optimus_detected)
return;
- nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,
- 0x3, );
+ if (nouveau_dsm_priv.optimus_flags_detected)
+ nouveau_optimus_dsm(nouveau_dsm_priv.dhandle,
NOUVEAU_DSM_OPTIMUS_FLAGS,
+ 0x3, );
nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_CAPS,
NOUVEAU_DSM_OPTIMUS_SET_POWERDOWN, );
--
2.9.0
[PATCH v2 2/4] drm/nouveau/acpi: return supported DSM functions
Return the set of supported functions to the caller. No functional
changes.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 886a67c..572ac30 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -107,7 +107,7 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
* requirements on the fourth parameter, so a private implementation
* instead of using acpi_check_dsm().
*/
-static int nouveau_check_optimus_dsm(acpi_handle handle)
+static int nouveau_dsm_get_optimus_functions(acpi_handle handle)
{
int result;
@@ -122,7 +122,9 @@ static int nouveau_check_optimus_dsm(acpi_handle handle)
* ACPI Spec v4 9.14.1: if bit 0 is zero, no function is supported.
* If the n-th bit is enabled, function n is supported
*/
- return result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS);
+ if (result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS))
+ return result;
+ return 0;
}
static int nouveau_dsm(acpi_handle handle, int func, int arg)
@@ -214,7 +216,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
{
acpi_handle dhandle;
bool supports_mux;
- bool supports_opt;
+ int optimus_funcs;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
@@ -225,17 +227,17 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
acpi_handle *dhandle_out
supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
1 << NOUVEAU_DSM_POWER);
- supports_opt = nouveau_check_optimus_dsm(dhandle);
+ optimus_funcs = nouveau_dsm_get_optimus_functions(dhandle);
/* Does not look like a Nvidia device. */
- if (!supports_mux && !supports_opt)
+ if (!supports_mux && !optimus_funcs)
return;
*dhandle_out = dhandle;
*has_mux = supports_mux;
- *has_opt = supports_opt;
+ *has_opt = !!optimus_funcs;
- if (supports_opt) {
+ if (optimus_funcs) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
--
2.9.0
[PATCH v2 1/4] drm/nouveau/acpi: ensure matching ACPI handle and supported functions
Ensure that the returned set of supported DSM functions (MUX, Optimus)
match the ACPI handle that is set in nouveau_dsm_pci_probe.
As there are no machines with a MUX function on just one PCI device and
an Optimus on another, there should not be a functional impact. This
change however makes this implicit assumption more obvious.
Convert int to bool and rename has_dsm to has_mux while at it. Let the
caller set nouveau_dsm_priv.dhandle as needed.
v2: pass dhandle to the caller.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 58 +++---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index db76b94..886a67c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -57,9 +57,6 @@ bool nouveau_is_v1_dsm(void) {
return nouveau_dsm_priv.dsm_detected;
}
-#define NOUVEAU_DSM_HAS_MUX 0x1
-#define NOUVEAU_DSM_HAS_OPT 0x2
-
#ifdef CONFIG_VGA_SWITCHEROO
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
@@ -212,26 +209,33 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
-static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
+static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle
*dhandle_out,
+ bool *has_mux, bool *has_opt)
{
acpi_handle dhandle;
- int retval = 0;
+ bool supports_mux;
+ bool supports_opt;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
- return false;
+ return;
if (!acpi_has_method(dhandle, "_DSM"))
- return false;
+ return;
- if (acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
- 1 << NOUVEAU_DSM_POWER))
- retval |= NOUVEAU_DSM_HAS_MUX;
+ supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
+ 1 << NOUVEAU_DSM_POWER);
+ supports_opt = nouveau_check_optimus_dsm(dhandle);
- if (nouveau_check_optimus_dsm(dhandle))
- retval |= NOUVEAU_DSM_HAS_OPT;
+ /* Does not look like a Nvidia device. */
+ if (!supports_mux && !supports_opt)
+ return;
- if (retval & NOUVEAU_DSM_HAS_OPT) {
+ *dhandle_out = dhandle;
+ *has_mux = supports_mux;
+ *has_opt = supports_opt;
+
+ if (supports_opt) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
@@ -240,10 +244,6 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
}
- if (retval)
- nouveau_dsm_priv.dhandle = dhandle;
-
- return retval;
}
static bool nouveau_dsm_detect(void)
@@ -251,11 +251,11 @@ static bool nouveau_dsm_detect(void)
char acpi_method_name[255] = { 0 };
struct acpi_buffer buffer = {sizeof(acpi_method_name),
acpi_method_name};
struct pci_dev *pdev = NULL;
- int has_dsm = 0;
- int has_optimus = 0;
+ acpi_handle dhandle = NULL;
+ bool has_mux = false;
+ bool has_optimus = false;
int vga_count = 0;
bool guid_valid;
- int retval;
bool ret = false;
/* lookup the MXM GUID */
@@ -268,32 +268,26 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, , _mux, _optimus);
}
/* find the optimus DSM or the old v1 DSM */
- if (has_optimus == 1) {
+ if (has_optimus) {
+ nouveau_dsm_priv.dhandle = dhandle;
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
);
prin
[PATCH v2 0/4] nouveau RPM fixes for Optimus
Hi, Here are two patches to fix an issue reported on kernel bugzilla (infinite loop due to unchecked function) and a more important fix to fix hanging Optimus machines when runtime PM is enabled (with pm/pci patches). See the first version[1] for a background on the fixed problems. This is the second revision of incorporating feedback from Emil Velikov (patch 1), Mika Westerberg (patch 4). Patches 2 and 3 are unchanged. The previous patchset had R-b from Hans de Goede, I think they are still valid. Noteworthy is that the fourth patch now checks directly for _PR3. The commit message is updated to emphasize that memory/disk corruption is fixed for some machines. This patchset can be merged before or after the pci/pm changes[2] (expected to be merged for 4.8), see the original posting[1] for consequences. I have tested it on top of v4.7-rc5. To make patch four work properly, Lukas' RPM refcounting patches should be included. A similar (open/new) RPM refcounting issue in snd-hda-intel should also be fixed. Otherwise the bridge will not really sleep. There is another minor patch for nouveau_pr3_present, but it is not included here because it depends on visibility of pci_bridge_d3_possible(). I'll send a separate mail for this to linux-pci. Kind regards, Peter [1]: https://lists.freedesktop.org/archives/nouveau/2016-May/025116.html [2]: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/?h=pci/pm Peter Wu (4): drm/nouveau/acpi: ensure matching ACPI handle and supported functions drm/nouveau/acpi: return supported DSM functions drm/nouveau/acpi: check for function 0x1B before using it drm/nouveau/acpi: fix lockup with PCIe runtime PM drivers/gpu/drm/nouveau/nouveau_acpi.c | 103 + 1 file changed, 66 insertions(+), 37 deletions(-) -- 2.9.0
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Wed, Jun 01, 2016 at 12:28:47PM +0300, Mika Westerberg wrote:
> On Tue, May 31, 2016 at 01:02:31PM +0200, Peter Wu wrote:
> > On Tue, May 31, 2016 at 11:43:56AM +0300, Mika Westerberg wrote:
> > > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote:
> > > > Do you have any suggestions for the case where the pcieport driver
> > > > refuses to put the bridge in D3 (because the BIOS is too old)? In that
> > > > case the nouveau driver needs to fallback to the DSM method (but not
> > > > when runtime PM is deliberately disabled by writing control=on).
> > >
> > > Do you know what Windows does then? I think we should do the same if
> > > possible.
> >
> > If the BIOS is too old, then it probably does not have _PR3 objects nor
> > calls to _OSI("Windows 2013"). See below.
> >
> > > If user has disabled runtime PM from the root port deliberately, there
> > > might be good reason to do so. Why we want to fallback to something that
> > > could cause problems? I mean _DSM on such systems is probably not that
> > > much tested because everybody runs Windows 8+ and using standard ACPI
> > > power resources.
> >
> > I agree that when runtime PM on the root port is disabled (control=on),
> > then there should be no fallback to DSM. For devices without _PR3 it is
> > clear that DSM will always be used (if available).
> >
> > In other cases (where _PR3 is available) we can distinguish:
> > - pre-Windows 8 machines. I have never seen this combination. Firmware
> >writers seems to prefer sticking to reference code which did not use
> >power resources before.
> > - Machines targeting Windows 8 or newer. (Note that there exist
> >machines with Windows 8 support that do not have _PR3, DSM is used in
> >that case.)
> >
> > If Windows 7 is running on a Windows 8 machine, PR3 will not be used
> > anyway. If the Linux kernel claims support for Windows 8, but does not
> > use PR3, then we are probably approaching an untested area. So far
> > firmware seems fine with using *only* DSM *or* PR3, but at least my
> > laptop gets confused when you use both at the same time.
> >
> > The latter happens on pci/pm (8b71f565) without other patches:
> >
> > 1. nouveau invokes _DSM and _PS3, device is put in D3cold.
> > 2. pcieport driver calls PG00._OFF (PG00 is returned by _PR3).
> > 3. Wake up Nvidia device (e.g. by power=on).
> > 4. This will trigger PG00._ON (via pcieport) and _PS0 (via nouveau).
> > 5. Nvidia card is not really ready (observed via "restoring config
> > space at offset ... (was 0x, writing ...)", a soft lockup
> > and RCU stall after that requiring a reboot to recover).
> >
> > nouveau could be patched not to invoke DSM when PR3 is detected
> > (proposal is ready) but will keep the device powered on in these cases:
> > - nouveau is patched, but pci/pm patches are not.
> > - PR3 is supported but due to the cutoff date (2015) it is not used.
> > - Boot option pcie_port_pm=off.
> > - runtime PM is disabled for pcieport (should be fine).
>
> Since using only _DSM has been the only method to power down the card
> currently inÅLinux (even if the root port has had _PR3), and it has been
> working fine, why not stick with that when _DSM is supported?
Maybe it is not really working, people have been reporting memory
corruption[1] for example on certain Lenovo models that was gone after
hacking the bbswitch module to disable the root port:
https://bugs.freedesktop.org/show_bug.cgi?id=78530
https://github.com/Bumblebee-Project/bbswitch/issues/78
https://github.com/Bumblebee-Project/bbswitch/issues/115
I'll try to solicit some feedback from the affected people on these
patch series, whether it solves their memory corruption issue.
Dave also said "This fixes GPU auto powerdown on the Lenovo W541," when
he added PR3 support in https://patchwork.freedesktop.org/patch/76313/
So apparently it did not work with just DSM.
> In other words, something like this:
>
> nouveau_dsm_pci_probe()
> {
> ...
> if (retval & (NOUVEAU_DSM_HAS_OPT | NOUVEAU_DSM_HAS_MUX)) {
> /*
>* We have custom _DSM method to power down the card so
>* prevent the PCI core from transitioning the
>* card into D3cold.
>*/
> pci_d3cold_disable(pdev);
> }
> }
>
> (Not sure about those flags above, though).
>
> Yes, it does no
[Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Tue, May 31, 2016 at 02:20:26PM +0200, Lukas Wunner wrote: > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > Do you have any suggestions for the case where the pcieport driver > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > case the nouveau driver needs to fallback to the DSM method (but not > > when runtime PM is deliberately disabled by writing control=on). > > The BIOS cut-off date is meant to avoid issues when suspending ports > on older chipsets. However if the port is used for an Optimus GPU > and we can clearly identify that, and there's a _PR3 method provided, > it's probably safe to say that the port is *intended* to be suspended. > > So you may want to consider amending pci_bridge_d3_possible() to > allow D3 for such ports regardless of the BIOS date, as I've done > for Thunderbolt in this commit: > https://github.com/l1k/linux/commit/3cb8549cd4e5 Then we have heuristics based on BIOS year, on whether it is TB or not, and next to it whether it is an Optimus laptop? Maybe the PCI core needs to export a function that allows drivers to override the detection if this becomes more common. > Not sure how to uniquely identify such ports though. Perhaps check > if there's a device in slot 0 below the port which has > (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY && > (pdev->vendor == PCI_VENDOR_ID_NVIDIA || >pdev->vendor == PCI_VENDOR_ID_ATI) Seems fragile, there are desktop setups satisfying this match. -- Kind regards, Peter Wu https://lekensteyn.nl
[Nouveau] [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
On Tue, May 31, 2016 at 01:34:43PM +0200, Lukas Wunner wrote: > On Mon, May 30, 2016 at 07:03:46PM +0200, Peter Wu wrote: > > On Sun, May 29, 2016 at 05:50:06PM +0200, Lukas Wunner wrote: > > > How exactly did you reach the situation where the root port didn't wake > > > up when you tried to load nouveau again? (IRC conversation this week.) > > > > Ensure that the pci/pm patches are applied, then: > > > > 0. Unload nouveau (I have blacklisted it for testing). > > 1. Enable rpm for the root port and children (control = auto). > > 2. Verify in the kernel logs that the devices are sleeping: > > pcieport :00:01.0: power state changed by ACPI to D3cold > > 3. (Optional, to rule out issues with delays:) Disable rpm for the > > Nvidia device (control = on). > > 4. modprobe nouveau. > > > > The above test with v4.6 + 4 pci/pm patches (8b71f565) gives: > > > > 50.245795 MXM: GUID detected in BIOS > > 50.245948nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.GFX0._DSM] at AML address c9013b11 length 492 > > 50.246016 ACPI Warning: \_SB.PCI0.GFX0._DSM: Argument #4 type mismatch > > - Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95) > > 50.246044nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.GFX0._DSM] at AML address c9013b11 length 492 > > 50.246110nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F > > 50.246256 ACPI Warning: \_SB.PCI0.PEG0.PEGP._DSM: Argument #4 type > > mismatch - Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95) > > 50.246289nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F > > 50.246443 ACPI Warning: \_SB.PCI0.PEG0.PEGP._DSM: Argument #4 type > > mismatch - Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95) > > 50.246457nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F > > 50.246932 pci :01:00.0: optimus capabilities: enabled, status > > dynamic power, hda bios codec supported > > 50.247005 VGA switcheroo: detected Optimus DSM method > > \_SB_.PCI0.PEG0.PEGP handle > > 50.247084nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.PEG0.PG00._ON] at AML address c901086e length 11D > > 50.390140 pcieport :00:01.0: power state changed by ACPI to D0 > > 50.491893nseval-0227 ns_evaluate : Execute method > > [\_SB.PCI0.PEG0._DSW] at AML address c9010a2d length 1D > > 50.492285 pcieport :00:01.0: PME# disabled > > 50.492583 nouveau :01:00.0: unknown chipset () > > 50.492687 nouveau: probe of :01:00.0 failed with error -12 > > I've tested this on a MacBook Pro, which does not have ACPI _PR3 > methods for the root port to which the discrete GPU is attached. > The port can thus only suspend to D3hot, not D3cold. > > Even without patch [2/9], when unloading nouveau and letting the > root port go to D3hot, the port is subsequently correctly resumed > to D0 when reloading nouveau. > > So the issue that you're seeing without patch [2/9] seems to be > specific to Optimus/_PR3 machines. If possible you should try to > get it working without patch [2/9] because that patch is really > optional (as I've written in the commit message). I'm totally > unfamiliar with Optimus but maybe lspci could help to debug this? Without 2/9 I can prevent the issue by writing "on" to /sys/bus/pci/devices/:00:01.0/power/control (the PCIe port), but that effectively gives the same result as applying 2/9. The problem occurs when the power is lost (by putting the PCIe port in D3cold). Maybe it is a bug in the PCI core that does not re-initialize devices under the port, but since a workaround is available (2/9), I will focus on other issues first. Maybe it is worth to mention this issue in the commit message for 2/9 though. -- Kind regards, Peter Wu https://lekensteyn.nl
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Tue, May 31, 2016 at 11:43:56AM +0300, Mika Westerberg wrote:
> On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote:
> > Do you have any suggestions for the case where the pcieport driver
> > refuses to put the bridge in D3 (because the BIOS is too old)? In that
> > case the nouveau driver needs to fallback to the DSM method (but not
> > when runtime PM is deliberately disabled by writing control=on).
>
> Do you know what Windows does then? I think we should do the same if
> possible.
If the BIOS is too old, then it probably does not have _PR3 objects nor
calls to _OSI("Windows 2013"). See below.
> If user has disabled runtime PM from the root port deliberately, there
> might be good reason to do so. Why we want to fallback to something that
> could cause problems? I mean _DSM on such systems is probably not that
> much tested because everybody runs Windows 8+ and using standard ACPI
> power resources.
I agree that when runtime PM on the root port is disabled (control=on),
then there should be no fallback to DSM. For devices without _PR3 it is
clear that DSM will always be used (if available).
In other cases (where _PR3 is available) we can distinguish:
- pre-Windows 8 machines. I have never seen this combination. Firmware
writers seems to prefer sticking to reference code which did not use
power resources before.
- Machines targeting Windows 8 or newer. (Note that there exist
machines with Windows 8 support that do not have _PR3, DSM is used in
that case.)
If Windows 7 is running on a Windows 8 machine, PR3 will not be used
anyway. If the Linux kernel claims support for Windows 8, but does not
use PR3, then we are probably approaching an untested area. So far
firmware seems fine with using *only* DSM *or* PR3, but at least my
laptop gets confused when you use both at the same time.
The latter happens on pci/pm (8b71f565) without other patches:
1. nouveau invokes _DSM and _PS3, device is put in D3cold.
2. pcieport driver calls PG00._OFF (PG00 is returned by _PR3).
3. Wake up Nvidia device (e.g. by power=on).
4. This will trigger PG00._ON (via pcieport) and _PS0 (via nouveau).
5. Nvidia card is not really ready (observed via "restoring config
space at offset ... (was 0x, writing ...)", a soft lockup
and RCU stall after that requiring a reboot to recover).
nouveau could be patched not to invoke DSM when PR3 is detected
(proposal is ready) but will keep the device powered on in these cases:
- nouveau is patched, but pci/pm patches are not.
- PR3 is supported but due to the cutoff date (2015) it is not used.
- Boot option pcie_port_pm=off.
- runtime PM is disabled for pcieport (should be fine).
There is a wealth of acpidumps on Launchpad bug 752542
(https://bugs.launchpad.net/bugs/752542). Search for example for
comments in early 2015 or before, those will likely be machine from 2014
or before.
Interesting to see is the _PR3 method of a HP Envy TS 15 (11/20/2014):
Method (_PR3, 0, NotSerialized) {
If (\_OSI ("Windows 2013")) {
Return (Package (0x01) {
\NVP3
})
} Else {
Return (Package (0x00) {})
}
}
(Note for self: just checking for the _PR3 handle in the nouveau patch
is apparently not sufficient, it must really be evaluated.)
Other machines with _PR3:
- Dell Inspiron 3543 (11/04/2014), comment 757.
- Dell XPS 15 9530 (03/28/2014), comment 711.
- Novatech 15.6 NSPIRE Laptop (01/20/2014), comment 695.
- Lenovo ThinkPad T440p (10/27/2013), comment 659.
There were many models from 2013 without _PR3 method but still checking
for _OSI("Windows 2013"). Maybe some heuristics based on _PR3 would be
more helpful than just a cutoff date?
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[Nouveau] [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
On Sun, May 29, 2016 at 05:50:06PM +0200, Lukas Wunner wrote:
> Hi Peter,
>
> On Fri, May 27, 2016 at 03:07:33AM +0200, Peter Wu wrote:
> > On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> > > nouveau_drm_load() calls pm_runtime_put() if nouveau_runtime_pm != 0,
> > > but nouveau_drm_unload() calls pm_runtime_get_sync() unconditionally.
> > > We therefore leak a runtime pm ref whenever nouveau is loaded with
> > > runpm=0 and then unloaded. The GPU will subsequently never runtime
> > > suspend even if nouveau is loaded again with runpm=1.
> > >
> > > Fix by taking the runtime pm ref under the same condition that it was
> > > released on driver load.
> > >
> > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> > > Cc: Dave Airlie
> > > Reported-by: Karol Herbst
> > > Tested-by: Karol Herbst
> > > Signed-off-by: Lukas Wunner
> >
> > Looks good, I tested this scenario:
> >
> > ru(){ cat /sys/bus/pci/devices/\:01:00.0/power/runtime_usage;}
> > ru # reports 1
> > modprobe nouveau runpm=0
> > ru # reports 2
> > rmmod nouveau
> > ru # reports 1
> >
> > Without runpm=0 the count drops to 0 in the second step and stays 0 in
> > the third step. After applying patch 2/9, this correctly reports 1 as
> > expected (this is the same as manually setting power/control to on).
>
> How exactly did you reach the situation where the root port didn't wake
> up when you tried to load nouveau again? (IRC conversation this week.)
Ensure that the pci/pm patches are applied, then:
0. Unload nouveau (I have blacklisted it for testing).
1. Enable rpm for the root port and children (control = auto).
2. Verify in the kernel logs that the devices are sleeping:
pcieport :00:01.0: power state changed by ACPI to D3cold
3. (Optional, to rule out issues with delays:) Disable rpm for the
Nvidia device (control = on).
4. modprobe nouveau.
The above test with v4.6 + 4 pci/pm patches (8b71f565) gives:
50.245795 MXM: GUID detected in BIOS
50.245948nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.GFX0._DSM] at AML address c9013b11 length 492
50.246016 ACPI Warning: \_SB.PCI0.GFX0._DSM: Argument #4 type mismatch -
Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95)
50.246044nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.GFX0._DSM] at AML address c9013b11 length 492
50.246110nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F
50.246256 ACPI Warning: \_SB.PCI0.PEG0.PEGP._DSM: Argument #4 type mismatch
- Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95)
50.246289nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F
50.246443 ACPI Warning: \_SB.PCI0.PEG0.PEGP._DSM: Argument #4 type mismatch
- Found [Buffer], ACPI requires [Package] (20160108/nsarguments-95)
50.246457nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0.PEGP._DSM] at AML address c9018297 length 1F
50.246932 pci :01:00.0: optimus capabilities: enabled, status dynamic
power, hda bios codec supported
50.247005 VGA switcheroo: detected Optimus DSM method \_SB_.PCI0.PEG0.PEGP
handle
50.247084nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0.PG00._ON] at AML address c901086e length 11D
50.390140 pcieport :00:01.0: power state changed by ACPI to D0
50.491893nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0._DSW] at AML address c9010a2d length 1D
50.492285 pcieport :00:01.0: PME# disabled
50.492583 nouveau :01:00.0: unknown chipset ()
50.492687 nouveau: probe of :01:00.0 failed with error -12
50.501990nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0._S0W] at AML address c9010a8e length 2
50.502403 pcieport :00:01.0: PME# enabled
50.502601nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0._DSW] at AML address c9010a2d length 1D
50.513005nseval-0227 ns_evaluate : Execute method
[\_SB.PCI0.PEG0.PG00._OFF] at AML address c9010994 length 6D
50.533258 pcieport :00:01.0: power state changed by ACPI to D3cold
(Note that this patch is not included.) When nouveau is operating
normally, I see that _PS0 is also called (which does not happen above).
If you think that mixing power resources with DSM causes this issue, I
also tried to apply my power resources work for nouveau but it gives the
same problem:
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Mon, May 30, 2016 at 04:09:09PM +0300, Mika Westerberg wrote: ... > > > > > > + > > > > > > + if (!parent_pdev) > > > > > > + return false; > > > > > > + > > > > > > + ad = ACPI_COMPANION(_pdev->dev); > > > > > > + if (!ad) > > > > > > + return false; > > > > > > + > > > > > > + return ad->power.flags.power_resources; > > > > > > > > > > Is this sufficient to tell if the parent device has _PR3? I thought it > > > > > returns true if it has power resources in general, not necessarily > > > > > _PR3. > > > > > > > > > > Otherwise this looks okay to me. > > > > > > > > It is indeed set whenever there is any _PRx method. I wonder if it is > > > > appropriate to access fields directly like this, perhaps this would be > > > > more accurate (based on device_pm.c): > > > > > > > > /* Check whether the _PR3 method is available. */ > > > > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > > > > > > > I am also considering adding a check in case the pcieport driver does > > > > not support D3cold via runtime PM, what do you think of this? > > > > > > > > if (!parent_pdev) > > > > return false; > > > > /* If the PCIe port does not support D3cold via runtime PM, allow a > > > > * fallback to the Optimus DSM method to put the device in D3cold. > > > > */ > > > > if (parent_pdev->no_d3cold) > > > > return false; > > > > > > > > This is needed to avoid the regression reported in the cover letter, but > > > > also allows pre-2015 systems to (still) have the D3cold possibility. > > > > > > The _DSM method with 0 as index parameter should return a bit field > > > telling which functions are supported. Sane BIOS disables that > > > particular function if it detects Windows 8 and newer. Have you checked > > > if that's the case? > > > > > > Then you can call _DSM only if it is supported and otherwise expect the > > > parent device's power resources to turn off power when runtime > > > suspended. > > > > The _DSM methods (for the Nvidia device) are often still included and > > functions are reported as supported. I guess that vendors just check > > whether it is working and do not bother removing legacy functions. The > > Acer case below seems exceptional. > > > > I suggested the no_d3cold check such that DSM can still be called even > > though the runtime PM on the PCIe port does nothing. > > Somehow it does not feel right to poke parent device's fields directly. > > What if you just check if it has the method like: > > bool no_dsm = acpi_has_method(parent_adev->handle, "_PR3"); > > That should follow what Windows is doing. Checking for _PR3 was the intention, but it seems that the ACPI core does not really store it somewhere. Your check should be simple enough, I'll use that in the next version. Do you have any suggestions for the case where the pcieport driver refuses to put the bridge in D3 (because the BIOS is too old)? In that case the nouveau driver needs to fallback to the DSM method (but not when runtime PM is deliberately disabled by writing control=on). -- Kind regards, Peter Wu https://lekensteyn.nl
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Mon, May 30, 2016 at 12:57:09PM +0300, Mika Westerberg wrote:
> +Rafael
>
> On Fri, May 27, 2016 at 01:10:37PM +0200, Peter Wu wrote:
> > On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote:
> > > On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote:
> > > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
> > > > can be runtime-suspended which disables power resources via ACPI. This
> > > > is incompatible with DSM, resulting in a GPU device which is still in D3
> > > > and locks up the kernel on resume.
> > > >
> > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
> > > > debugger trace) and stop using the DSM functions for D3cold when power
> > > > resources are available on the parent PCIe port.
> > > >
> > > > [1]:
> > > > https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
> > > >
> > > > Signed-off-by: Peter Wu
> > > > ---
> > > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34
> > > > ++
> > > > 1 file changed, 30 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > > b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > > index df9f73e..e469df7 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
> > > > bool dsm_detected;
> > > > bool optimus_detected;
> > > > bool optimus_flags_detected;
> > > > + bool optimus_skip_dsm;
> > > > acpi_handle dhandle;
> > > > acpi_handle rom_handle;
> > > > } nouveau_dsm_priv;
> > > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler
> > > > nouveau_dsm_handler = {
> > > > .get_client_id = nouveau_dsm_get_client_id,
> > > > };
> > > >
> > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the
> > > > device into
> > > > + * D3cold, they instead rely on disabling power resources on the
> > > > parent. */
> > > > +static bool nouveau_pr3_present(struct pci_dev *pdev)
> > > > +{
> > > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> > > > + struct acpi_device *ad;
> > >
> > > Nit: please call this adev instead of ad.
> >
> > Will do.
> >
> > > > +
> > > > + if (!parent_pdev)
> > > > + return false;
> > > > +
> > > > + ad = ACPI_COMPANION(_pdev->dev);
> > > > + if (!ad)
> > > > + return false;
> > > > +
> > > > + return ad->power.flags.power_resources;
> > >
> > > Is this sufficient to tell if the parent device has _PR3? I thought it
> > > returns true if it has power resources in general, not necessarily _PR3.
> > >
> > > Otherwise this looks okay to me.
> >
> > It is indeed set whenever there is any _PRx method. I wonder if it is
> > appropriate to access fields directly like this, perhaps this would be
> > more accurate (based on device_pm.c):
> >
> > /* Check whether the _PR3 method is available. */
> > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid;
> >
> > I am also considering adding a check in case the pcieport driver does
> > not support D3cold via runtime PM, what do you think of this?
> >
> > if (!parent_pdev)
> > return false;
> > /* If the PCIe port does not support D3cold via runtime PM, allow a
> > * fallback to the Optimus DSM method to put the device in D3cold. */
> > if (parent_pdev->no_d3cold)
> > return false;
> >
> > This is needed to avoid the regression reported in the cover letter, but
> > also allows pre-2015 systems to (still) have the D3cold possibility.
>
> The _DSM method with 0 as index parameter should return a bit field
> telling which functions are supported. Sane BIOS disables that
> particular function if it detects Windows 8 and newer. Have you checked
> if that's the case?
>
> Then you can call _DSM only if it is supported and otherwise expect the
> parent device's power resources to turn off power
[Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Mon, May 30, 2016 at 11:48:34AM +0100, Emil Velikov wrote:
> On 27 May 2016 at 22:31, Peter Wu wrote:
> > On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote:
> >> Hi Peter,
> >>
> >> On 24 May 2016 at 23:53, Peter Wu wrote:
> >> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
> >> > can be runtime-suspended which disables power resources via ACPI. This
> >> > is incompatible with DSM, resulting in a GPU device which is still in D3
> >> > and locks up the kernel on resume.
> >> >
> >> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
> >> > debugger trace) and stop using the DSM functions for D3cold when power
> >> > resources are available on the parent PCIe port.
> >> >
> >> > [1]:
> >> > https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
> >> >
> >> > Signed-off-by: Peter Wu
> >> > ---
> >> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34
> >> > ++
> >> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> > b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> > index df9f73e..e469df7 100644
> >> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
> >> > bool dsm_detected;
> >> > bool optimus_detected;
> >> > bool optimus_flags_detected;
> >> > + bool optimus_skip_dsm;
> >> > acpi_handle dhandle;
> >> > acpi_handle rom_handle;
> >> > } nouveau_dsm_priv;
> >> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler
> >> > nouveau_dsm_handler = {
> >> > .get_client_id = nouveau_dsm_get_client_id,
> >> > };
> >> >
> >> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the
> >> > device into
> >> > + * D3cold, they instead rely on disabling power resources on the
> >> > parent. */
> >> > +static bool nouveau_pr3_present(struct pci_dev *pdev)
> >> > +{
> >> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> >> > + struct acpi_device *ad;
> >> > +
> >> > + if (!parent_pdev)
> >> > + return false;
> >> > +
> >> > + ad = ACPI_COMPANION(_pdev->dev);
> >> > + if (!ad)
> >> > + return false;
> >> > +
> >> > + return ad->power.flags.power_resources;
> >> > +}
> >> > +
> >> > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux,
> >> > - bool *has_opt, bool *has_opt_flags)
> >> > + bool *has_opt, bool *has_opt_flags,
> >> > + bool *has_power_resources)
> >> > {
> >> > acpi_handle dhandle;
> >> > bool supports_mux;
> >> > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev
> >> > *pdev, bool *has_mux,
> >> > *has_mux = supports_mux;
> >> > *has_opt = !!optimus_funcs;
> >> > *has_opt_flags = optimus_funcs & (1 <<
> >> > NOUVEAU_DSM_OPTIMUS_FLAGS);
> >> > + *has_power_resources = false;
> >> >
> >> > if (optimus_funcs) {
> >> > uint32_t result;
> >> > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev
> >> > *pdev, bool *has_mux,
> >> > (result & OPTIMUS_ENABLED) ? "enabled" :
> >> > "disabled",
> >> > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic
> >> > power, " : "",
> >> > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios
> >> > codec supported" : "");
> >> > +
> >> > + *has_power_resources = nouveau_pr3_present(pdev);
> >> > }
> >> > }
> >> >
> >> > @@ -258,6 +280,7 @@ static bool nouveau_dsm
[Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote:
> Hi Peter,
>
> On 24 May 2016 at 23:53, Peter Wu wrote:
> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
> > can be runtime-suspended which disables power resources via ACPI. This
> > is incompatible with DSM, resulting in a GPU device which is still in D3
> > and locks up the kernel on resume.
> >
> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
> > debugger trace) and stop using the DSM functions for D3cold when power
> > resources are available on the parent PCIe port.
> >
> > [1]:
> > https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
> >
> > Signed-off-by: Peter Wu
> > ---
> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34
> > ++
> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > index df9f73e..e469df7 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
> > bool dsm_detected;
> > bool optimus_detected;
> > bool optimus_flags_detected;
> > + bool optimus_skip_dsm;
> > acpi_handle dhandle;
> > acpi_handle rom_handle;
> > } nouveau_dsm_priv;
> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler
> > nouveau_dsm_handler = {
> > .get_client_id = nouveau_dsm_get_client_id,
> > };
> >
> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the
> > device into
> > + * D3cold, they instead rely on disabling power resources on the parent. */
> > +static bool nouveau_pr3_present(struct pci_dev *pdev)
> > +{
> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> > + struct acpi_device *ad;
> > +
> > + if (!parent_pdev)
> > + return false;
> > +
> > + ad = ACPI_COMPANION(_pdev->dev);
> > + if (!ad)
> > + return false;
> > +
> > + return ad->power.flags.power_resources;
> > +}
> > +
> > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux,
> > - bool *has_opt, bool *has_opt_flags)
> > + bool *has_opt, bool *has_opt_flags,
> > + bool *has_power_resources)
> > {
> > acpi_handle dhandle;
> > bool supports_mux;
> > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
> > bool *has_mux,
> > *has_mux = supports_mux;
> > *has_opt = !!optimus_funcs;
> > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
> > + *has_power_resources = false;
> >
> > if (optimus_funcs) {
> > uint32_t result;
> > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
> > bool *has_mux,
> > (result & OPTIMUS_ENABLED) ? "enabled" :
> > "disabled",
> > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic
> > power, " : "",
> > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios
> > codec supported" : "");
> > +
> > + *has_power_resources = nouveau_pr3_present(pdev);
> > }
> > }
> >
> > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void)
> > bool has_mux = false;
> > bool has_optimus = false;
> > bool has_optimus_flags = false;
> > + bool has_power_resources = false;
> > int vga_count = 0;
> > bool guid_valid;
> > bool ret = false;
> > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void)
> > vga_count++;
> >
> > nouveau_dsm_pci_probe(pdev, _mux, _optimus,
> > - _optimus_flags);
> > + _optimus_flags,
> > _power_resources);
> > }
> >
> > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) !=
> > NULL) {
> > vga_count++;
> >
> > nouveau_dsm_pci_probe(pdev, _mux, _optimus,
> &g
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote:
> On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote:
> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
> > can be runtime-suspended which disables power resources via ACPI. This
> > is incompatible with DSM, resulting in a GPU device which is still in D3
> > and locks up the kernel on resume.
> >
> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
> > debugger trace) and stop using the DSM functions for D3cold when power
> > resources are available on the parent PCIe port.
> >
> > [1]:
> > https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
> >
> > Signed-off-by: Peter Wu
> > ---
> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34
> > ++
> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > index df9f73e..e469df7 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
> > bool dsm_detected;
> > bool optimus_detected;
> > bool optimus_flags_detected;
> > + bool optimus_skip_dsm;
> > acpi_handle dhandle;
> > acpi_handle rom_handle;
> > } nouveau_dsm_priv;
> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler
> > nouveau_dsm_handler = {
> > .get_client_id = nouveau_dsm_get_client_id,
> > };
> >
> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the
> > device into
> > + * D3cold, they instead rely on disabling power resources on the parent. */
> > +static bool nouveau_pr3_present(struct pci_dev *pdev)
> > +{
> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> > + struct acpi_device *ad;
>
> Nit: please call this adev instead of ad.
Will do.
> > +
> > + if (!parent_pdev)
> > + return false;
> > +
> > + ad = ACPI_COMPANION(_pdev->dev);
> > + if (!ad)
> > + return false;
> > +
> > + return ad->power.flags.power_resources;
>
> Is this sufficient to tell if the parent device has _PR3? I thought it
> returns true if it has power resources in general, not necessarily _PR3.
>
> Otherwise this looks okay to me.
It is indeed set whenever there is any _PRx method. I wonder if it is
appropriate to access fields directly like this, perhaps this would be
more accurate (based on device_pm.c):
/* Check whether the _PR3 method is available. */
return adev->power.states[ACPI_STATE_D3_COLD].flags.valid;
I am also considering adding a check in case the pcieport driver does
not support D3cold via runtime PM, what do you think of this?
if (!parent_pdev)
return false;
/* If the PCIe port does not support D3cold via runtime PM, allow a
* fallback to the Optimus DSM method to put the device in D3cold. */
if (parent_pdev->no_d3cold)
return false;
This is needed to avoid the regression reported in the cover letter, but
also allows pre-2015 systems to (still) have the D3cold possibility.
Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G,
apparently from November 2013, Windows 8.1) and extracted the ACPI
tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support
for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a
related NVP3 device) when _OSI("Windows 2013") is true. (This is added
as alternative for the old DSM interface.)
Maybe 2014 is also an appropriate cutoff date? I wonder if it is
feasible to detect firmware use of _OSI("Windows 2013") and use that
instead of the BIOS year.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[Nouveau] [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> nouveau_drm_load() calls pm_runtime_put() if nouveau_runtime_pm != 0,
> but nouveau_drm_unload() calls pm_runtime_get_sync() unconditionally.
> We therefore leak a runtime pm ref whenever nouveau is loaded with
> runpm=0 and then unloaded. The GPU will subsequently never runtime
> suspend even if nouveau is loaded again with runpm=1.
>
> Fix by taking the runtime pm ref under the same condition that it was
> released on driver load.
>
> Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> Cc: Dave Airlie
> Reported-by: Karol Herbst
> Tested-by: Karol Herbst
> Signed-off-by: Lukas Wunner
Looks good, I tested this scenario:
ru(){ cat /sys/bus/pci/devices/\:01:00.0/power/runtime_usage;}
ru # reports 1
modprobe nouveau runpm=0
ru # reports 2
rmmod nouveau
ru # reports 1
Without runpm=0 the count drops to 0 in the second step and stays 0 in
the third step. After applying patch 2/9, this correctly reports 1 as
expected (this is the same as manually setting power/control to on).
Peter
> ---
> drivers/gpu/drm/nouveau/nouveau_drm.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 11f8dd9..faf7438 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -498,7 +498,10 @@ nouveau_drm_unload(struct drm_device *dev)
> {
> struct nouveau_drm *drm = nouveau_drm(dev);
>
> - pm_runtime_get_sync(dev->dev);
> + if (nouveau_runtime_pm != 0) {
> + pm_runtime_get_sync(dev->dev);
> + }
> +
> nouveau_fbcon_fini(dev);
> nouveau_accel_fini(drm);
> nouveau_hwmon_fini(dev);
> --
> 2.8.1
>
> ___
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM
Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port
can be runtime-suspended which disables power resources via ACPI. This
is incompatible with DSM, resulting in a GPU device which is still in D3
and locks up the kernel on resume.
Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi
debugger trace) and stop using the DSM functions for D3cold when power
resources are available on the parent PCIe port.
[1]:
https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index df9f73e..e469df7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,7 @@ static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
bool optimus_flags_detected;
+ bool optimus_skip_dsm;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
+/* Firmware supporting Windows 8 or later do not use _DSM to put the device
into
+ * D3cold, they instead rely on disabling power resources on the parent. */
+static bool nouveau_pr3_present(struct pci_dev *pdev)
+{
+ struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
+ struct acpi_device *ad;
+
+ if (!parent_pdev)
+ return false;
+
+ ad = ACPI_COMPANION(_pdev->dev);
+ if (!ad)
+ return false;
+
+ return ad->power.flags.power_resources;
+}
+
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux,
- bool *has_opt, bool *has_opt_flags)
+ bool *has_opt, bool *has_opt_flags,
+ bool *has_power_resources)
{
acpi_handle dhandle;
bool supports_mux;
@@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
bool *has_mux,
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
*has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
+ *has_power_resources = false;
if (optimus_funcs) {
uint32_t result;
@@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
bool *has_mux,
(result & OPTIMUS_ENABLED) ? "enabled" : "disabled",
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
+
+ *has_power_resources = nouveau_pr3_present(pdev);
}
}
@@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void)
bool has_mux = false;
bool has_optimus = false;
bool has_optimus_flags = false;
+ bool has_power_resources = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void)
vga_count++;
nouveau_dsm_pci_probe(pdev, _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
nouveau_dsm_pci_probe(pdev, _mux, _optimus,
- _optimus_flags);
+ _optimus_flags, _power_resources);
}
/* find the optimus DSM or the old v1 DSM */
@@ -289,8 +312,11 @@ static bool nouveau_dsm_detect(void)
);
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
+ if (has_power_resources)
+ pr_info("nouveau: detected PR support, will not use
DSM\n");
nouveau_dsm_priv.optimus_detected = true;
nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
+ nouveau_dsm_priv.optimus_skip_dsm = has_power_resources;
ret = true;
} else if (vga_count == 2 && has_mux && guid_valid) {
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
@@ -320,7 +346,7 @@ void nouveau_register_dsm_handler(void)
void nouveau_switcheroo_optimus_dsm(void)
{
u32 result = 0;
- if (!nouveau_dsm_priv.optimus_detected)
+ if (!nouveau_dsm_priv.optimus_detected ||
nouveau_dsm_priv.optimus_skip_dsm)
return;
if (nouveau_dsm_priv.optimus_flags_detected)
--
2.8.2
[PATCH 3/4] drm/nouveau/acpi: check for function 0x1B before using it
Do not unconditionally invoke function 0x1B without checking for its
availability, it leads to an infinite loop on some firmware.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=104791
Fixes: 5addcf0a5f0fad ("nouveau: add runtime PM support (v0.9)")
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 71d5e6a..df9f73e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -45,6 +45,7 @@
static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
+ bool optimus_flags_detected;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -212,7 +213,7 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
};
static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux,
- bool *has_opt)
+ bool *has_opt, bool *has_opt_flags)
{
acpi_handle dhandle;
bool supports_mux;
@@ -236,6 +237,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
bool *has_mux,
nouveau_dsm_priv.dhandle = dhandle;
*has_mux = supports_mux;
*has_opt = !!optimus_funcs;
+ *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
if (optimus_funcs) {
uint32_t result;
@@ -255,6 +257,7 @@ static bool nouveau_dsm_detect(void)
struct pci_dev *pdev = NULL;
bool has_mux = false;
bool has_optimus = false;
+ bool has_optimus_flags = false;
int vga_count = 0;
bool guid_valid;
bool ret = false;
@@ -269,13 +272,15 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- nouveau_dsm_pci_probe(pdev, _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, _mux, _optimus,
+ _optimus_flags);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- nouveau_dsm_pci_probe(pdev, _mux, _optimus);
+ nouveau_dsm_pci_probe(pdev, _mux, _optimus,
+ _optimus_flags);
}
/* find the optimus DSM or the old v1 DSM */
@@ -285,6 +290,7 @@ static bool nouveau_dsm_detect(void)
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
nouveau_dsm_priv.optimus_detected = true;
+ nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags;
ret = true;
} else if (vga_count == 2 && has_mux && guid_valid) {
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
@@ -317,8 +323,9 @@ void nouveau_switcheroo_optimus_dsm(void)
if (!nouveau_dsm_priv.optimus_detected)
return;
- nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,
- 0x3, );
+ if (nouveau_dsm_priv.optimus_flags_detected)
+ nouveau_optimus_dsm(nouveau_dsm_priv.dhandle,
NOUVEAU_DSM_OPTIMUS_FLAGS,
+ 0x3, );
nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_CAPS,
NOUVEAU_DSM_OPTIMUS_SET_POWERDOWN, );
--
2.8.2
[PATCH 2/4] drm/nouveau/acpi: return supported DSM functions
Return the set of supported functions to the caller. No functional
changes.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 45fa9b2..71d5e6a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -107,7 +107,7 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
* requirements on the fourth parameter, so a private implementation
* instead of using acpi_check_dsm().
*/
-static int nouveau_check_optimus_dsm(acpi_handle handle)
+static int nouveau_dsm_get_optimus_functions(acpi_handle handle)
{
int result;
@@ -122,7 +122,9 @@ static int nouveau_check_optimus_dsm(acpi_handle handle)
* ACPI Spec v4 9.14.1: if bit 0 is zero, no function is supported.
* If the n-th bit is enabled, function n is supported
*/
- return result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS);
+ if (result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS))
+ return result;
+ return 0;
}
static int nouveau_dsm(acpi_handle handle, int func, int arg)
@@ -214,7 +216,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
bool *has_mux,
{
acpi_handle dhandle;
bool supports_mux;
- bool supports_opt;
+ int optimus_funcs;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
@@ -225,17 +227,17 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev,
bool *has_mux,
supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
1 << NOUVEAU_DSM_POWER);
- supports_opt = nouveau_check_optimus_dsm(dhandle);
+ optimus_funcs = nouveau_dsm_get_optimus_functions(dhandle);
/* Does not look like a Nvidia device. */
- if (!supports_mux && !supports_opt)
+ if (!supports_mux && !optimus_funcs)
return;
nouveau_dsm_priv.dhandle = dhandle;
*has_mux = supports_mux;
- *has_opt = supports_opt;
+ *has_opt = !!optimus_funcs;
- if (supports_opt) {
+ if (optimus_funcs) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
--
2.8.2
[PATCH 1/4] drm/nouveau/acpi: ensure matching ACPI handle and supported functions
Ensure that the returned set of supported DSM functions (MUX, Optimus)
match the ACPI handle that is set in nouveau_dsm_pci_probe.
As there are no machines with a MUX function on just one PCI device and
an Optimus on another, there should not be a functional impact. This
change however makes this implicit assumption more obvious.
Convert int to bool and rename has_dsm to has_mux while at it.
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 55 ++
1 file changed, 23 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index cdf5227..45fa9b2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -57,9 +57,6 @@ bool nouveau_is_v1_dsm(void) {
return nouveau_dsm_priv.dsm_detected;
}
-#define NOUVEAU_DSM_HAS_MUX 0x1
-#define NOUVEAU_DSM_HAS_OPT 0x2
-
#ifdef CONFIG_VGA_SWITCHEROO
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
@@ -212,26 +209,33 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
-static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
+static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux,
+ bool *has_opt)
{
acpi_handle dhandle;
- int retval = 0;
+ bool supports_mux;
+ bool supports_opt;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
- return false;
+ return;
if (!acpi_has_method(dhandle, "_DSM"))
- return false;
+ return;
+
+ supports_mux = acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
+ 1 << NOUVEAU_DSM_POWER);
+ supports_opt = nouveau_check_optimus_dsm(dhandle);
- if (acpi_check_dsm(dhandle, nouveau_dsm_muid, 0x0102,
- 1 << NOUVEAU_DSM_POWER))
- retval |= NOUVEAU_DSM_HAS_MUX;
+ /* Does not look like a Nvidia device. */
+ if (!supports_mux && !supports_opt)
+ return;
- if (nouveau_check_optimus_dsm(dhandle))
- retval |= NOUVEAU_DSM_HAS_OPT;
+ nouveau_dsm_priv.dhandle = dhandle;
+ *has_mux = supports_mux;
+ *has_opt = supports_opt;
- if (retval & NOUVEAU_DSM_HAS_OPT) {
+ if (supports_opt) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
@@ -240,10 +244,6 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
(result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, "
: "",
(result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec
supported" : "");
}
- if (retval)
- nouveau_dsm_priv.dhandle = dhandle;
-
- return retval;
}
static bool nouveau_dsm_detect(void)
@@ -251,11 +251,10 @@ static bool nouveau_dsm_detect(void)
char acpi_method_name[255] = { 0 };
struct acpi_buffer buffer = {sizeof(acpi_method_name),
acpi_method_name};
struct pci_dev *pdev = NULL;
- int has_dsm = 0;
- int has_optimus = 0;
+ bool has_mux = false;
+ bool has_optimus = false;
int vga_count = 0;
bool guid_valid;
- int retval;
bool ret = false;
/* lookup the MXM GUID */
@@ -268,32 +267,24 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, _mux, _optimus);
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
- if (retval & NOUVEAU_DSM_HAS_MUX)
- has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
+ nouveau_dsm_pci_probe(pdev, _mux, _optimus);
}
/* find the optimus DSM or the old v1 DSM */
- if (has_optimus == 1) {
+ if (has_optimus) {
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
);
printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method
%s handle\n",
acpi_method_name);
nouveau_dsm_priv.optimus_detected = true;
ret = true;
- } else if (vga_count =
[PATCH 0/4] nouveau fixes for RPM/Optimus-related hangs
Hi, Here are two patches to fix an issue reported on kernel bugzilla (infinite loop due to unchecked function) and a more important fix to fix hanging Optimus machines when runtime PM is enabled (with pm/pci patches). An older (obsolete) patch for the first issue was tested by the reporter: https://bugzilla.kernel.org/show_bug.cgi?id=104791#c11 (it is replaced by "check for function 0x1B before using it"). The second issue will occur when: - A modern Optimus laptop is in use (designed for Windows 8 or newer). - nouveau runtime PM is enabled (1 or the default -1). - The patch "PCI: Add runtime PM support for PCIe ports" from Mika is pulled into v4.7 (or v4.8[1]?) via the pci/pm branch, https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pm=8b71f5652eeac561acf883da01ab4810f763ee42 (see also the discussion for "[PATCH] PCI: Power on bridges before scanning new devices" at http://article.gmane.org/gmane.linux.power-management.general/76411) The first two patches are just refactoring to reduce code duplication (and scratch an itch) and make the following patches possible. The next two patches fix the problems reported above. I intend to get these patches in 4.7 (or the first version where pci/pm gets merged) to avoid a lockup when runpm is enabled. Note: - If the fourth patch is merged before/without Mika's PCIe port patch, then those modern Optimus machines above will not be put into D3cold. - If the fourth patch is not merged (or merged after Mika's patch), then under the above conditions the affected machine can lock up. - The three other patches are unrelated to this issue and can safely be merged. Tested with: - Linux v4.6 + pci/pm + these four patches - Hardware: Clevo P651RA with acpi_osi="!Windows 2015" (the latter is a workaround for another PCIe issue). - Card is asleep, woke up with lspci, waited a bit and retried/suspended: - # lspci -nn >/dev/null; sleep 5 - # lspci -nn >/dev/null; sleep 5; systemctl suspend - # lspci -nn >/dev/null; systemctl suspend Kind regards, Peter [1]: https://lkml.kernel.org/r/20160524211309.GH1789 at lahna.fi.intel.com Peter Wu (4): drm/nouveau/acpi: ensure matching ACPI handle and supported functions drm/nouveau/acpi: return supported DSM functions drm/nouveau/acpi: check for function 0x1B before using it drm/nouveau/acpi: fix lockup with PCIe runtime PM drivers/gpu/drm/nouveau/nouveau_acpi.c | 100 + 1 file changed, 63 insertions(+), 37 deletions(-) -- 2.8.2
[Nouveau] [PATCH] gpu/nouveau/nouveau_acpi.c: Fix Type Mismatch ACPI warning
On Fri, May 20, 2016 at 02:22:57AM +, Marcos Souza wrote:
[..]
> > > I don't know if this is the right thing to do, I just looked at
> > intel_acpi.c to check how to use/check for ACPI Package.
> > > The patch below silenced the "type mismatch" warnings, and some of the
> > "evaluated _DSM" ones.
> > >
> > > If this is not the right approach, please let me know how to fix it, I
> > don't have knowledge in ACPI, but I really want to help.
> > >
> > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 14 +-
> > > 1 file changed, 1 insertion(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > index cdf5227..f04aef3 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > > @@ -73,22 +73,10 @@ static const char nouveau_op_dsm_muid[] = {
> > >
> > > static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg,
> > uint32_t *result)
> > > {
> > > - int i;
> > > union acpi_object *obj;
> > > - char args_buff[4];
> > > - union acpi_object argv4 = {
> > > - .buffer.type = ACPI_TYPE_BUFFER,
> > > - .buffer.length = 4,
> > > - .buffer.pointer = args_buff
> > > - };
> > > -
> > > - /* ACPI is little endian, AABBCCDD becomes {DD,CC,BB,AA} */
> > > - for (i = 0; i < 4; i++)
> > > - args_buff[i] = (arg >> i * 8) & 0xFF;
> > > -
> > > *result = 0;
> > > obj = acpi_evaluate_dsm_typed(handle, nouveau_op_dsm_muid,
> > 0x0100,
> > > - func, , ACPI_TYPE_BUFFER);
> > > + func, NULL, ACPI_TYPE_PACKAGE);
This effectively removes the fourth parameter (actually, using a zero
as fourth argument), making the function useless.
> > The last parameter you give to `acpi_evaluate_dsm_typed()` is the return
> > type
> > you expect (see [3]), which will be a buffer if func is 0, and is
> > implementation dependent otherwise (see section 9.14.1 _DSM of [4]). So you
> > donât want to change it to ACPI_TYPE_PACKAGE. If you look at the
> > implementation
> > of `acpi_evaluate_dsm()` (which is called by `acpi_evaluate_dsm_typed()`),
> > it
> > will automatically create a package for the 4th argument, if you pass it a
> > NULL
> > pointer (see [5]).
> >
> > [3]:
> > https://github.com/torvalds/linux/blob/46c13450624e36302547a2ac3695f2350fe7ffc3/include/acpi/acpi_bus.h#L69
> > [4]: http://www.acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf
> > [5]:
> > https://github.com/torvalds/linux/blob/46c13450624e36302547a2ac3695f2350fe7ffc3/drivers/acpi/utils.c#L628
>
>
> Thanks for all the links. I'll read the docs and send a new version of the
> patch when it makes more sense instead of just replacing random things.
The warning is unavoidable, most firmware expect a Buffer for the fourth
argument (counting from 0, this is Arg3). Excerpt from the DSM method on
a recent Skylake laptop (Clevo P651, but this format is found on many
other models from various manufacturers too):
If ((Arg2 == 0x1A))
{
CreateField (Arg3, 0x18, 0x02, OMPR)
CreateField (Arg3, Zero, One, FLCH)
CreateField (Arg3, One, One, DVSR)
CreateField (Arg3, 0x02, One, DVSC)
(The sections below refer to the one in the ACPI 6.1 document that can
be found at http://uefi.org/specifications.)
The first parameter for CreateField is evaluated as buffer (sec 19.6.21).
According to 19.3.5.6 (Data Types and Type Conversions) an implicit
conversion to a Buffer is only possible from an Integer and String, a
Package does not belong to the possibilities.
Note that the return value may be an integer for unsupported revision
IDs or UUIDs (like 0x8002). These should be compatible with Buffers
though as stated above and acpi_check_dsm() can handle that case, but
unfortunately sets a Package as fourth argument and can therefore not be
used in nouveau.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
[PATCH v2] drm/nouveau: check function before using it
Do not unconditionally invoke function 0x1B without checking for its
availability, it leads to an infinite loop on some firmware.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=104791
Fixes: 5addcf0a5f0fad ("nouveau: add runtime PM support (v0.9)")
Signed-off-by: Peter Wu
---
v2: only write optimus_funcs when an Optimus DSM is found.
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 43 ++
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index cdf5227..009712a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -45,6 +45,7 @@
static struct nouveau_dsm_priv {
bool dsm_detected;
bool optimus_detected;
+ bool flags_func_detected;
acpi_handle dhandle;
acpi_handle rom_handle;
} nouveau_dsm_priv;
@@ -58,7 +59,6 @@ bool nouveau_is_v1_dsm(void) {
}
#define NOUVEAU_DSM_HAS_MUX 0x1
-#define NOUVEAU_DSM_HAS_OPT 0x2
#ifdef CONFIG_VGA_SWITCHEROO
static const char nouveau_dsm_muid[] = {
@@ -110,9 +110,9 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
* requirements on the fourth parameter, so a private implementation
* instead of using acpi_check_dsm().
*/
-static int nouveau_check_optimus_dsm(acpi_handle handle)
+static uint32_t nouveau_check_optimus_dsm(acpi_handle handle)
{
- int result;
+ uint32_t result;
/*
* Function 0 returns a Buffer containing available functions.
@@ -123,9 +123,13 @@ static int nouveau_check_optimus_dsm(acpi_handle handle)
/*
* ACPI Spec v4 9.14.1: if bit 0 is zero, no function is supported.
-* If the n-th bit is enabled, function n is supported
+* If the n-th bit is enabled, function n is supported.
+* Check for both bit zero and the NOUVEAU_DSM_OPTIMUS_CAPS since
+* some implementations return 0x8001 on invalid parameters.
*/
- return result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS);
+ if (result & 1 && result & (1 << NOUVEAU_DSM_OPTIMUS_CAPS))
+ return result;
+ return 0;
}
static int nouveau_dsm(acpi_handle handle, int func, int arg)
@@ -212,10 +216,11 @@ static const struct vga_switcheroo_handler
nouveau_dsm_handler = {
.get_client_id = nouveau_dsm_get_client_id,
};
-static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
+static int nouveau_dsm_pci_probe(struct pci_dev *pdev, uint32_t *optimus_funcs)
{
acpi_handle dhandle;
int retval = 0;
+ uint32_t supported_funcs;
dhandle = ACPI_HANDLE(>dev);
if (!dhandle)
@@ -228,11 +233,10 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
1 << NOUVEAU_DSM_POWER))
retval |= NOUVEAU_DSM_HAS_MUX;
- if (nouveau_check_optimus_dsm(dhandle))
- retval |= NOUVEAU_DSM_HAS_OPT;
-
- if (retval & NOUVEAU_DSM_HAS_OPT) {
+ supported_funcs = nouveau_check_optimus_dsm(dhandle);
+ if (supported_funcs) {
uint32_t result;
+ *optimus_funcs = supported_funcs;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
);
dev_info(>dev, "optimus capabilities: %s, status %s%s\n",
@@ -252,7 +256,7 @@ static bool nouveau_dsm_detect(void)
struct acpi_buffer buffer = {sizeof(acpi_method_name),
acpi_method_name};
struct pci_dev *pdev = NULL;
int has_dsm = 0;
- int has_optimus = 0;
+ uint32_t optimus_funcs = 0;
int vga_count = 0;
bool guid_valid;
int retval;
@@ -268,30 +272,28 @@ static bool nouveau_dsm_detect(void)
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
NULL) {
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
+ retval = nouveau_dsm_pci_probe(pdev, _funcs);
if (retval & NOUVEAU_DSM_HAS_MUX)
has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
}
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL)
{
vga_count++;
- retval = nouveau_dsm_pci_probe(pdev);
+ retval = nouveau_dsm_pci_probe(pdev, _funcs);
if (retval & NOUVEAU_DSM_HAS_MUX)
has_dsm |= 1;
- if (retval & NOUVEAU_DSM_HAS_OPT)
- has_optimus = 1;
}
/* find the optimus DSM or the old v1 DSM */
- if (has_optimus == 1) {
+ if (optimus_funcs) {
acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
);
printk(KE
Stupid NVIDIA 3D vgaarb.c patch
On Tuesday 23 September 2014 03:52:48 C Bergstr?m wrote: > Here's where I originally found it > https://github.com/Bumblebee-Project/Bumblebee/issues/159 > (Adding Peter to cc chain) > > I guess there's already a bug id and some (snarky?) comments > https://bugzilla.kernel.org/show_bug.cgi?id=63641 > --- > This is for Aorus X3 laptop with NVIDIA GTX 870m card > > On Tue, Sep 23, 2014 at 3:43 AM, Linus Torvalds < > torvalds at linux-foundation.org> wrote: > > > Adding proper people and mailing lists.. > > > > The PCI_CLASS_DISPLAY_VGA test goes back to the very beginning by > > BenH, and I have no idea if adding PCI_CLASS_DISPLAY_3D is > > appropriate, but hopefully somebody does. The fact that it makes > > things work certainly argues fairly convincingly for it, but I want > > some backup here. > > > > Dave, BenH? > > > > Christopher(?), can you please also specify which laptop etc. And the > > patch itself seems to have come from somebody else, unless you're > > Lekensteyn. So we'd want to get the provenance of that too. If I understood this correctly, VGA arbitration is used to deal with multiple users of a fixed IO memory address. I don't know whether modern software still use it, but if they do, then it probably makes sense to include the 3D controller class as some Nvidia graphics card are configured with this class. Unfortunately I cannot provide more details than this as I don't know the specifics. Feel free to take this patch and make it suitable for inclusion. I don't have this kind of hardware, but mr. C is certainly not the only one with this. Kind regards, Peter > > Linus > > > > On Mon, Sep 22, 2014 at 1:28 PM, C Bergstr?m > > wrote: > > > Hi Linus, > > > > > > I don't know who the original author is and I can have one of the distro > > > graphics friends review it, but I need this patch below to get NVIDIA > > > drivers to work (without using any Intel graphics) on my laptop > > > http://pastebin.com/wpmFi38k > > > > > > Sorry - I know this is not the proper way to submit a patch, but it's > > > trivial and I'm not a kernel dev. > > > > > > Thanks > > > > > > ./C > > -- Kind regards, Peter https://lekensteyn.nl
lockdep splat while exiting PRIME
Hi,
While trying PRIME, I got a lockdep warning after exiting glxgears. Is
it harmful? The command was:
DRI_PRIME=1 glxgears
Offload provider is a GT425M (NVC0), output sink is an Intel i5-460M.
Kind regards,
Peter
dmesg:
=
[ INFO: possible recursive locking detected ]
3.15.0-rc8-custom-00058-gd2cfd31 #1 Tainted: G O
-
X/25827 is trying to acquire lock:
(>struct_mutex){+.+.+.}, at: []
i915_gem_unmap_dma_buf+0x36/0xd0 [i915]
but task is already holding lock:
(>struct_mutex){+.+.+.}, at: []
drm_gem_object_handle_unreference_unlocked+0x105/0x130 [drm]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(>struct_mutex);
lock(>struct_mutex);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by X/25827:
#0: (>struct_mutex){+.+.+.}, at: []
drm_gem_object_handle_unreference_unlocked+0x105/0x130 [drm]
stack backtrace:
CPU: 1 PID: 25827 Comm: X Tainted: G O
3.15.0-rc8-custom-00058-gd2cfd31 #1
Hardware name: CLEVO CO.B7130
/B7130 , BIOS 6.00 08/27/2010
822588a0 880230767ae0 815f14da 880226594260
880230767bb0 810a1461 30767bc0 880226594288
880230767b00 880226594ae0 00464232 0001
Call Trace:
[] dump_stack+0x4e/0x7a
[] __lock_acquire+0x19d1/0x1ab0
[] lock_acquire+0x95/0x130
[] ? i915_gem_unmap_dma_buf+0x36/0xd0 [i915]
[] ? i915_gem_unmap_dma_buf+0x36/0xd0 [i915]
[] mutex_lock_nested+0x65/0x400
[] ? i915_gem_unmap_dma_buf+0x36/0xd0 [i915]
[] i915_gem_unmap_dma_buf+0x36/0xd0 [i915]
[] dma_buf_unmap_attachment+0x4c/0x70
[] drm_prime_gem_destroy+0x22/0x40 [drm]
[] nouveau_gem_object_del+0x3e/0x60 [nouveau]
[] drm_gem_object_free+0x2a/0x40 [drm]
[] drm_gem_object_handle_unreference_unlocked+0x128/0x130
[drm]
[] drm_gem_handle_delete+0xba/0x110 [drm]
[] drm_gem_close_ioctl+0x25/0x30 [drm]
[] drm_ioctl+0x1e0/0x5f0 [drm]
[] ? drm_gem_handle_create+0x40/0x40 [drm]
[] ? _raw_spin_unlock_irqrestore+0x5d/0x80
[] ? trace_hardirqs_on_caller+0x15d/0x200
[] ? trace_hardirqs_on+0xd/0x10
[] ? _raw_spin_unlock_irqrestore+0x42/0x80
[] nouveau_drm_ioctl+0x65/0xa0 [nouveau]
[] do_vfs_ioctl+0x2f0/0x4f0
[] ? __fget+0xac/0xf0
[] ? __fget+0x5/0xf0
[] SyS_ioctl+0x81/0xa0
[] system_call_fastpath+0x16/0x1b
[PATCH] drm/nouveau/acpi: de-dup use of DSM methods
On Friday 02 August 2013 15:58:38 Dave Airlie wrote:
> On Fri, Aug 2, 2013 at 12:41 AM, Peter Wu wrote:
> > Observe that nouveau_optimus_dsm and nouveau_dsm are equal except for
> > the parameters handling (UUID, revision ID and function arguments). The
> > function arguments are passed as Buffer in the "optimus dsm" and Integer
> > in "nvidia dsm". As buffers are implicitly converted to integers, merge
> > both functions.
> >
> > The ACPI spec defines the fourth parameter (Arg3 a.k.a. "function
> > arguments") as Package, but many BIOSes expect a Buffer instead. For
> > instance, for the "nvidia DSM", the Lenovo T410s uses CreateByteField on
> > Arg3 which does not work with a package. The Clevo B7130 does something
> > similar for the "Optimus DSM". Unfortunately, this means that the
> > following ACPI warning (introduced with 29a241c) cannot be fixed (when
>
> > toggling power or muxing):
> By cannot be fixed, why is it there then? maybe ask the ACPI folks to
> drop this error, since as far as I can see all optimus dsm want a
> buffer here.
It helps tracking violations of the ACPI spec which can be used as debugging
tool, as a result there is also a patch for i915 (which was based on this
nouveau code). Of course, if real world only uses Buffers, then this warning
should be dropped/changed, but there are more users of _DSM besides the
graphics subsystem.
Regards,
Peter
> > ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
> > [Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
> > Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
> > [Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
> > Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
> > [Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
> > Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
> > [Buffer], ACPI requires [Package] (20130517/nsarguments-95)>
> > Signed-off-by: Peter Wu
> > ---
> >
> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 67
> > ++ 1 file changed, 20 insertions(+), 47
> > deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > b/drivers/gpu/drm/nouveau/nouveau_acpi.c index d97f200..a75684f 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > @@ -46,6 +46,9 @@ bool nouveau_is_v1_dsm(void) {
> >
> > #define NOUVEAU_DSM_HAS_MUX 0x1
> > #define NOUVEAU_DSM_HAS_OPT 0x2
> >
> > +#define NOUVEAU_DSM_REVID_NVIDIA 0x102
> > +#define NOUVEAU_DSM_REVID_OPTIMUS 0x100
> > +
> >
> > static const char nouveau_dsm_muid[] = {
> >
> > 0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
> > 0xB3, 0x4D, 0x7E, 0x5F, 0xEA, 0x12, 0x9F, 0xD4,
> >
> > @@ -56,7 +59,8 @@ static const char nouveau_op_dsm_muid[] = {
> >
> > 0xA7, 0x2B, 0x60, 0x42, 0xA6, 0xB5, 0xBE, 0xE0,
> >
> > };
> >
> > -static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg,
> > uint32_t *result) +static int nouveau_call_dsm(acpi_handle handle, const
> > char *uuid, int revid, + int func, int arg, uint32_t *result)
> >
> > {
> >
> > struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> > struct acpi_object_list input;
> >
> > @@ -68,12 +72,15 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
> > func, int arg, uint32_t *>
> > input.count = 4;
> > input.pointer = params;
> > params[0].type = ACPI_TYPE_BUFFER;
> >
> > - params[0].buffer.length = sizeof(nouveau_op_dsm_muid);
> > - params[0].buffer.pointer = (char *)nouveau_op_dsm_muid;
> > + params[0].buffer.length = 16;
> > + params[0].buffer.pointer = (char *)uuid;
> >
> > params[1].type = ACPI_TYPE_INTEGER;
> >
> > - params[1].integer.value = 0x0100;
> > + params[1].integer.value = revid;
> >
> > params[2].type = ACPI_TYPE_INTEGER;
> > params[2].integer.value = func;
> >
> > + /* Although the ACPI spec defines Arg3 as a Package, in practise
> > +* implementations expect a Buffer (CreateWordField and Index
> > functions +* are applied to it). */
> >
> > params[3].type = ACPI_TYPE_BUFFER;
> > params[3].buffer.length = 4;
> > /* ACPI is little endian, A
Re: [PATCH] drm/nouveau/acpi: de-dup use of DSM methods
On Friday 02 August 2013 15:58:38 Dave Airlie wrote:
On Fri, Aug 2, 2013 at 12:41 AM, Peter Wu lekenst...@gmail.com wrote:
Observe that nouveau_optimus_dsm and nouveau_dsm are equal except for
the parameters handling (UUID, revision ID and function arguments). The
function arguments are passed as Buffer in the optimus dsm and Integer
in nvidia dsm. As buffers are implicitly converted to integers, merge
both functions.
The ACPI spec defines the fourth parameter (Arg3 a.k.a. function
arguments) as Package, but many BIOSes expect a Buffer instead. For
instance, for the nvidia DSM, the Lenovo T410s uses CreateByteField on
Arg3 which does not work with a package. The Clevo B7130 does something
similar for the Optimus DSM. Unfortunately, this means that the
following ACPI warning (introduced with 29a241c) cannot be fixed (when
toggling power or muxing):
By cannot be fixed, why is it there then? maybe ask the ACPI folks to
drop this error, since as far as I can see all optimus dsm want a
buffer here.
It helps tracking violations of the ACPI spec which can be used as debugging
tool, as a result there is also a patch for i915 (which was based on this
nouveau code). Of course, if real world only uses Buffers, then this warning
should be dropped/changed, but there are more users of _DSM besides the
graphics subsystem.
Regards,
Peter
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95) ACPI
Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
Signed-off-by: Peter Wu lekenst...@gmail.com
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 67
++ 1 file changed, 20 insertions(+), 47
deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c index d97f200..a75684f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,9 @@ bool nouveau_is_v1_dsm(void) {
#define NOUVEAU_DSM_HAS_MUX 0x1
#define NOUVEAU_DSM_HAS_OPT 0x2
+#define NOUVEAU_DSM_REVID_NVIDIA 0x102
+#define NOUVEAU_DSM_REVID_OPTIMUS 0x100
+
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
0xB3, 0x4D, 0x7E, 0x5F, 0xEA, 0x12, 0x9F, 0xD4,
@@ -56,7 +59,8 @@ static const char nouveau_op_dsm_muid[] = {
0xA7, 0x2B, 0x60, 0x42, 0xA6, 0xB5, 0xBE, 0xE0,
};
-static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg,
uint32_t *result) +static int nouveau_call_dsm(acpi_handle handle, const
char *uuid, int revid, + int func, int arg, uint32_t *result)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_object_list input;
@@ -68,12 +72,15 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
input.count = 4;
input.pointer = params;
params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_op_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_op_dsm_muid;
+ params[0].buffer.length = 16;
+ params[0].buffer.pointer = (char *)uuid;
params[1].type = ACPI_TYPE_INTEGER;
- params[1].integer.value = 0x0100;
+ params[1].integer.value = revid;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = func;
+ /* Although the ACPI spec defines Arg3 as a Package, in practise
+* implementations expect a Buffer (CreateWordField and Index
functions +* are applied to it). */
params[3].type = ACPI_TYPE_BUFFER;
params[3].buffer.length = 4;
/* ACPI is little endian, AABBCCDD becomes {DD,CC,BB,AA} */
@@ -108,50 +115,16 @@ static int nouveau_optimus_dsm(acpi_handle handle,
int func, int arg, uint32_t *
return 0;
}
-static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t
*result) -{
- struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
- struct acpi_object_list input;
- union acpi_object params[4];
- union acpi_object *obj;
- int err;
-
- input.count = 4;
- input.pointer = params;
- params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_dsm_muid;
- params[1].type = ACPI_TYPE_INTEGER
[PATCH 1/4] gpu/vga_switcheroo: add driver control power feature. (v3)
Hi Dave,
I don't know anything of PM domains, but there was one minor thing I wanted to
mention, see below.
On Monday 29 July 2013 16:06:56 Dave Airlie wrote:
> From: Dave Airlie
Something went wrong here I guess?
> [..]
>
> +static int vga_switcheroo_runtime_resume_hdmi_audio(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int ret;
> + struct vga_switcheroo_client *client, *found = NULL;
> +
> + /* we need to check if we have to switch back on the video
> +device so the audio device can come back */
> + list_for_each_entry(client, _priv.clients, list) {
> + if ((client->pdev->devfn & ~(0x7)) == (pdev->devfn & ~(0x7)) &&
> client_is_vga(client)) {
Less obfuscated:
PCI_SLOT(client->pdev->devfn) == PCI_SLOT(pdev->devfn)
> + found = client;
> + ret = pm_runtime_get_sync(>pdev->dev);
> + if (ret) {
> + if (ret != 1)
> + return ret;
> + }
> + break;
> + }
> + }
> + ret = dev->bus->pm->runtime_resume(dev);
> + if (ret)
> + return ret;
> +
> + /* put the reference for the gpu */
> + if (found)
> + pm_runtime_put_autosuspend(>pdev->dev);
> + return 0;
> +}
> [..]
Regards,
Peter
[PATCH] i915: fix ACPI _DSM warning
Since commit 29a241c (ACPICA: Add argument typechecking for all
predefined ACPI names), _DSM parameters are validated which trigger the
following warning:
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
As the Intel _DSM method seems to ignore this parameter, let's comply to
the ACPI spec and use a Package instead.
Signed-off-by: Peter Wu
---
What is this code useful for? It seems unfinished, all it does it
printing some information when a mux is available, but besides that
there is no interaction with the driver.
---
drivers/gpu/drm/i915/intel_acpi.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_acpi.c
b/drivers/gpu/drm/i915/intel_acpi.c
index bcbbaea..57fe1ae 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -28,7 +28,7 @@ static const u8 intel_dsm_guid[] = {
0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
};
-static int intel_dsm(acpi_handle handle, int func, int arg)
+static int intel_dsm(acpi_handle handle, int func)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_object_list input;
@@ -46,8 +46,9 @@ static int intel_dsm(acpi_handle handle, int func, int arg)
params[1].integer.value = INTEL_DSM_REVISION_ID;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = func;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = arg;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
ret = acpi_evaluate_object(handle, "_DSM", , );
if (ret) {
@@ -151,8 +152,9 @@ static void intel_dsm_platform_mux_info(void)
params[1].integer.value = INTEL_DSM_REVISION_ID;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = INTEL_DSM_FN_PLATFORM_MUX_INFO;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = 0;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
ret = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", ,
);
@@ -205,7 +207,7 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
return false;
}
- ret = intel_dsm(dhandle, INTEL_DSM_FN_SUPPORTED_FUNCTIONS, 0);
+ ret = intel_dsm(dhandle, INTEL_DSM_FN_SUPPORTED_FUNCTIONS);
if (ret < 0) {
DRM_DEBUG_KMS("failed to get supported _DSM functions\n");
return false;
--
1.8.3.4
[PATCH] drm/nouveau/acpi: de-dup use of DSM methods
Observe that nouveau_optimus_dsm and nouveau_dsm are equal except for
the parameters handling (UUID, revision ID and function arguments). The
function arguments are passed as Buffer in the "optimus dsm" and Integer
in "nvidia dsm". As buffers are implicitly converted to integers, merge
both functions.
The ACPI spec defines the fourth parameter (Arg3 a.k.a. "function
arguments") as Package, but many BIOSes expect a Buffer instead. For
instance, for the "nvidia DSM", the Lenovo T410s uses CreateByteField on
Arg3 which does not work with a package. The Clevo B7130 does something
similar for the "Optimus DSM". Unfortunately, this means that the
following ACPI warning (introduced with 29a241c) cannot be fixed (when
toggling power or muxing):
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
Signed-off-by: Peter Wu
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 67 ++
1 file changed, 20 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index d97f200..a75684f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,9 @@ bool nouveau_is_v1_dsm(void) {
#define NOUVEAU_DSM_HAS_MUX 0x1
#define NOUVEAU_DSM_HAS_OPT 0x2
+#define NOUVEAU_DSM_REVID_NVIDIA 0x102
+#define NOUVEAU_DSM_REVID_OPTIMUS 0x100
+
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
0xB3, 0x4D, 0x7E, 0x5F, 0xEA, 0x12, 0x9F, 0xD4,
@@ -56,7 +59,8 @@ static const char nouveau_op_dsm_muid[] = {
0xA7, 0x2B, 0x60, 0x42, 0xA6, 0xB5, 0xBE, 0xE0,
};
-static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t
*result)
+static int nouveau_call_dsm(acpi_handle handle, const char *uuid, int revid,
+ int func, int arg, uint32_t *result)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_object_list input;
@@ -68,12 +72,15 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
input.count = 4;
input.pointer = params;
params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_op_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_op_dsm_muid;
+ params[0].buffer.length = 16;
+ params[0].buffer.pointer = (char *)uuid;
params[1].type = ACPI_TYPE_INTEGER;
- params[1].integer.value = 0x0100;
+ params[1].integer.value = revid;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = func;
+ /* Although the ACPI spec defines Arg3 as a Package, in practise
+* implementations expect a Buffer (CreateWordField and Index functions
+* are applied to it). */
params[3].type = ACPI_TYPE_BUFFER;
params[3].buffer.length = 4;
/* ACPI is little endian, AABBCCDD becomes {DD,CC,BB,AA} */
@@ -108,50 +115,16 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
return 0;
}
-static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
-{
- struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
- struct acpi_object_list input;
- union acpi_object params[4];
- union acpi_object *obj;
- int err;
-
- input.count = 4;
- input.pointer = params;
- params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_dsm_muid;
- params[1].type = ACPI_TYPE_INTEGER;
- params[1].integer.value = 0x0102;
- params[2].type = ACPI_TYPE_INTEGER;
- params[2].integer.value = func;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = arg;
-
- err = acpi_evaluate_object(handle, "_DSM", , );
- if (err) {
- printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
- return err;
- }
-
- obj = (union acpi_object *)output.pointer;
-
- if (obj->type == ACPI_TYPE_INTEGER)
- if (obj->integer.value == 0x8002)
- return -ENODEV;
-
- if (obj->type == ACPI_TYPE_BUFFER) {
- if (obj->buffer.length == 4 && result) {
- *result = 0;
- *result
[PATCH] drm/nouveau/acpi: de-dup use of DSM methods
Observe that nouveau_optimus_dsm and nouveau_dsm are equal except for
the parameters handling (UUID, revision ID and function arguments). The
function arguments are passed as Buffer in the optimus dsm and Integer
in nvidia dsm. As buffers are implicitly converted to integers, merge
both functions.
The ACPI spec defines the fourth parameter (Arg3 a.k.a. function
arguments) as Package, but many BIOSes expect a Buffer instead. For
instance, for the nvidia DSM, the Lenovo T410s uses CreateByteField on
Arg3 which does not work with a package. The Clevo B7130 does something
similar for the Optimus DSM. Unfortunately, this means that the
following ACPI warning (introduced with 29a241c) cannot be fixed (when
toggling power or muxing):
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Buffer], ACPI requires [Package] (20130517/nsarguments-95)
Signed-off-by: Peter Wu lekenst...@gmail.com
---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 67 ++
1 file changed, 20 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index d97f200..a75684f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -46,6 +46,9 @@ bool nouveau_is_v1_dsm(void) {
#define NOUVEAU_DSM_HAS_MUX 0x1
#define NOUVEAU_DSM_HAS_OPT 0x2
+#define NOUVEAU_DSM_REVID_NVIDIA 0x102
+#define NOUVEAU_DSM_REVID_OPTIMUS 0x100
+
static const char nouveau_dsm_muid[] = {
0xA0, 0xA0, 0x95, 0x9D, 0x60, 0x00, 0x48, 0x4D,
0xB3, 0x4D, 0x7E, 0x5F, 0xEA, 0x12, 0x9F, 0xD4,
@@ -56,7 +59,8 @@ static const char nouveau_op_dsm_muid[] = {
0xA7, 0x2B, 0x60, 0x42, 0xA6, 0xB5, 0xBE, 0xE0,
};
-static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t
*result)
+static int nouveau_call_dsm(acpi_handle handle, const char *uuid, int revid,
+ int func, int arg, uint32_t *result)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_object_list input;
@@ -68,12 +72,15 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
input.count = 4;
input.pointer = params;
params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_op_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_op_dsm_muid;
+ params[0].buffer.length = 16;
+ params[0].buffer.pointer = (char *)uuid;
params[1].type = ACPI_TYPE_INTEGER;
- params[1].integer.value = 0x0100;
+ params[1].integer.value = revid;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = func;
+ /* Although the ACPI spec defines Arg3 as a Package, in practise
+* implementations expect a Buffer (CreateWordField and Index functions
+* are applied to it). */
params[3].type = ACPI_TYPE_BUFFER;
params[3].buffer.length = 4;
/* ACPI is little endian, AABBCCDD becomes {DD,CC,BB,AA} */
@@ -108,50 +115,16 @@ static int nouveau_optimus_dsm(acpi_handle handle, int
func, int arg, uint32_t *
return 0;
}
-static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
-{
- struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
- struct acpi_object_list input;
- union acpi_object params[4];
- union acpi_object *obj;
- int err;
-
- input.count = 4;
- input.pointer = params;
- params[0].type = ACPI_TYPE_BUFFER;
- params[0].buffer.length = sizeof(nouveau_dsm_muid);
- params[0].buffer.pointer = (char *)nouveau_dsm_muid;
- params[1].type = ACPI_TYPE_INTEGER;
- params[1].integer.value = 0x0102;
- params[2].type = ACPI_TYPE_INTEGER;
- params[2].integer.value = func;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = arg;
-
- err = acpi_evaluate_object(handle, _DSM, input, output);
- if (err) {
- printk(KERN_INFO failed to evaluate _DSM: %d\n, err);
- return err;
- }
-
- obj = (union acpi_object *)output.pointer;
-
- if (obj-type == ACPI_TYPE_INTEGER)
- if (obj-integer.value == 0x8002)
- return -ENODEV;
-
- if (obj-type == ACPI_TYPE_BUFFER) {
- if (obj-buffer.length == 4 result) {
- *result = 0;
- *result |= obj-buffer.pointer[0];
- *result |= (obj-buffer.pointer[1] 8
[PATCH] i915: fix ACPI _DSM warning
Since commit 29a241c (ACPICA: Add argument typechecking for all
predefined ACPI names), _DSM parameters are validated which trigger the
following warning:
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.GFX0._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
ACPI Warning: \_SB_.PCI0.P0P2.PEGP._DSM: Argument #4 type mismatch - Found
[Integer], ACPI requires [Package] (20130517/nsarguments-95)
As the Intel _DSM method seems to ignore this parameter, let's comply to
the ACPI spec and use a Package instead.
Signed-off-by: Peter Wu lekenst...@gmail.com
---
What is this code useful for? It seems unfinished, all it does it
printing some information when a mux is available, but besides that
there is no interaction with the driver.
---
drivers/gpu/drm/i915/intel_acpi.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_acpi.c
b/drivers/gpu/drm/i915/intel_acpi.c
index bcbbaea..57fe1ae 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -28,7 +28,7 @@ static const u8 intel_dsm_guid[] = {
0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
};
-static int intel_dsm(acpi_handle handle, int func, int arg)
+static int intel_dsm(acpi_handle handle, int func)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_object_list input;
@@ -46,8 +46,9 @@ static int intel_dsm(acpi_handle handle, int func, int arg)
params[1].integer.value = INTEL_DSM_REVISION_ID;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = func;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = arg;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
ret = acpi_evaluate_object(handle, _DSM, input, output);
if (ret) {
@@ -151,8 +152,9 @@ static void intel_dsm_platform_mux_info(void)
params[1].integer.value = INTEL_DSM_REVISION_ID;
params[2].type = ACPI_TYPE_INTEGER;
params[2].integer.value = INTEL_DSM_FN_PLATFORM_MUX_INFO;
- params[3].type = ACPI_TYPE_INTEGER;
- params[3].integer.value = 0;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
ret = acpi_evaluate_object(intel_dsm_priv.dhandle, _DSM, input,
output);
@@ -205,7 +207,7 @@ static bool intel_dsm_pci_probe(struct pci_dev *pdev)
return false;
}
- ret = intel_dsm(dhandle, INTEL_DSM_FN_SUPPORTED_FUNCTIONS, 0);
+ ret = intel_dsm(dhandle, INTEL_DSM_FN_SUPPORTED_FUNCTIONS);
if (ret 0) {
DRM_DEBUG_KMS(failed to get supported _DSM functions\n);
return false;
--
1.8.3.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] gpu/vga_switcheroo: add driver control power feature. (v3)
Hi Dave,
I don't know anything of PM domains, but there was one minor thing I wanted to
mention, see below.
On Monday 29 July 2013 16:06:56 Dave Airlie wrote:
From: Dave Airlie airl...@dhcp-40-90.bne.redhat.com
Something went wrong here I guess?
[..]
+static int vga_switcheroo_runtime_resume_hdmi_audio(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ int ret;
+ struct vga_switcheroo_client *client, *found = NULL;
+
+ /* we need to check if we have to switch back on the video
+device so the audio device can come back */
+ list_for_each_entry(client, vgasr_priv.clients, list) {
+ if ((client-pdev-devfn ~(0x7)) == (pdev-devfn ~(0x7))
client_is_vga(client)) {
Less obfuscated:
PCI_SLOT(client-pdev-devfn) == PCI_SLOT(pdev-devfn)
+ found = client;
+ ret = pm_runtime_get_sync(client-pdev-dev);
+ if (ret) {
+ if (ret != 1)
+ return ret;
+ }
+ break;
+ }
+ }
+ ret = dev-bus-pm-runtime_resume(dev);
+ if (ret)
+ return ret;
+
+ /* put the reference for the gpu */
+ if (found)
+ pm_runtime_put_autosuspend(found-pdev-dev);
+ return 0;
+}
[..]
Regards,
Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/5] nouveau: add dynamic gpu power off support.
Hi Dave,
+int nouveau_dynamic_power_set_state(struct drm_device *dev, int state)
+{
+ struct nouveau_drm *drm = nouveau_drm(dev);
+ pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
+
+ if (state == DRM_SWITCH_POWER_DYNAMIC_OFF) {
+ dev-switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
In existing set_state code, this switch_power_state is first set to
DRM_SWITCH_POWER_CHANGING. Is it sensible to do the same thing here?
+ drm_kms_helper_poll_disable(drm-dev);
+ vga_switcheroo_set_dynamic_switch(dev-pdev,
VGA_SWITCHEROO_OFF,
false);
+ nouveau_switcheroo_optimus_dsm();
+ nouveau_drm_suspend(drm-dev-pdev, pmm);
+ vga_switcheroo_set_dynamic_switch(dev-pdev,
VGA_SWITCHEROO_OFF,
true);
+ } else if (state == DRM_SWITCH_POWER_ON) {
+ vga_switcheroo_set_dynamic_switch(dev-pdev, VGA_SWITCHEROO_ON,
true);
+ nouveau_drm_resume(dev-pdev);
+ vga_switcheroo_set_dynamic_switch(dev-pdev, VGA_SWITCHEROO_ON,
false);
+ drm_kms_helper_poll_enable(dev);
+ dev-switch_power_state = DRM_SWITCH_POWER_ON;
Same here.
+ }
+
+ return 0;
+}
Regards,
Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/5] nouveau: add dynamic gpu power off support.
Hi Dave,
> +int nouveau_dynamic_power_set_state(struct drm_device *dev, int state)
> +{
> + struct nouveau_drm *drm = nouveau_drm(dev);
> + pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> +
> + if (state == DRM_SWITCH_POWER_DYNAMIC_OFF) {
> + dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
In existing set_state code, this switch_power_state is first set to
DRM_SWITCH_POWER_CHANGING. Is it sensible to do the same thing here?
> + drm_kms_helper_poll_disable(drm->dev);
> + vga_switcheroo_set_dynamic_switch(dev->pdev,
> VGA_SWITCHEROO_OFF,
false);
> + nouveau_switcheroo_optimus_dsm();
> + nouveau_drm_suspend(drm->dev->pdev, pmm);
> + vga_switcheroo_set_dynamic_switch(dev->pdev,
> VGA_SWITCHEROO_OFF,
true);
> + } else if (state == DRM_SWITCH_POWER_ON) {
> + vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON,
true);
> + nouveau_drm_resume(dev->pdev);
> + vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON,
false);
> + drm_kms_helper_poll_enable(dev);
> + dev->switch_power_state = DRM_SWITCH_POWER_ON;
Same here.
> + }
> +
> + return 0;
> +}
Regards,
Peter
