Hi,
On 10-11-16 11:58, Peter Wu wrote:
On Wed, Nov 09, 2016 at 06:17:44PM +0100, Hans de Goede wrote:
Various notebooks with nvidia GPUs generate an ACPI_VIDEO_NOTIFY_PROBE
acpi-video event when an external device gets plugged in (and again on
modesets on that connector), the default behavior in the acpi-video
driver for this is to send a KEY_SWITCHVIDEOMODE evdev event, which
causes e.g. gnome-settings-daemon to ask us to rescan the connectors
(good), but also causes g-s-d to switch to mirror mode on a newly plugged
monitor rather then using the monitor to extend the desktop (bad)
as KEY_SWITCHVIDEOMODE is supposed to switch between extend the desktop
vs mirror mode.
Can confirm this behavior on my Clevo P651RA laptop with GTX 965M, it
shows a weird keystroke on console on inserting a monitor. On the Plasma
desktop, it shows a notification "no outputs detected". Only after
running "xrandr", the external monitor would be picked up.
Good, thank you for testing.
More troublesome are the repeated ACPI_VIDEO_NOTIFY_PROBE events on
changing the mode on the connector, which cause g-s-d to switch
between mirror/extend mode, which causes a new ACPI_VIDEO_NOTIFY_PROBE
event and we end up with an endless loop.
This commit fixes this by adding an acpi notifier block handler to
nouveau_display.c to intercept ACPI_VIDEO_NOTIFY_PROBE and:
1) Wake-up runtime suspended GPUs and call drm_helper_hpd_irq_event()
on them, this is necessary in some cases for the GPU to detect connector
hotplug events while runtime suspended
2) Return NOTIFY_BAD to stop acpi-video from emitting a bogus
KEY_SWITCHVIDEOMODE key-press event
There already is another acpi notifier block handler registered in
drivers/gpu/drm/nouveau/nvkm/engine/device/acpi.c, but that is not
suitable since that one gets unregistered on runtime suspend, and
we also want to intercept ACPI_VIDEO_NOTIFY_PROBE when runtime suspended.
From a quick look, it looks like a suitable mechanism except for the
removal on runtime suspend.
That is not the only problem, there also is no way to get to the
drm_device pointer that deep in the nvkm object tree.
Based on commit logs for core/core/event.c, the event system seems
initially used for communication between drm and nouveau, later this was
extended to userspace notifications. For some reason, nvkm_acpi_init is
called through this user.c path:
lspci-14658 [001] d..1 35887.344592: p_nvkm_acpi_init_0: (nvkm_acpi_init+0x0/0x20 [nouveau])
lspci-14658 [001] d..1 35887.344597: <stack trace>
=> nvkm_udevice_init
=> nvkm_object_init
=> nvkm_object_init
=> nvkm_client_init
=> nvkm_client_resume
=> nvif_client_resume
=> nouveau_do_resume
=> nouveau_pmops_runtime_resume
=> pci_pm_runtime_resume
runtime suspend follows similar code path with resume -> suspend and
init -> fini, but somehow I also see this weird path just before resume:
lspci-14658 [001] d..1 35887.176959: p_nvkm_acpi_fini_0: (nvkm_acpi_fini+0x0/0x20 [nouveau])
lspci-14658 [001] d..1 35887.176974: <stack trace>
=> nvkm_device_init
=> nvkm_udevice_init
This was observed on kernel 4.8.6-1-ARCH using kprobe tracing.
Yes I noticed that too, this is indeed weird.
> Hopefully
Ben can clarify this situation. One other comment below.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
Note that ACPI_VIDEO_NOTIFY_PROBE currently is a private define in
drivers/acpi/acpi_video.c, since it is passed to acpi_notifier_call_chain()
it really should be in a public header, so I've submitted a patch to
the acpi subsys to move it to include/acpi/video.h . In the mean time
this patch defines it with a #ifndef guard to allow merging without
introducing inter subsys dependencies. I will submit a follow up patch
removing the #ifndef block once both patches are in Linus' tree.
---
drivers/gpu/drm/nouveau/nouveau_display.c | 61 +++++++++++++++++++++++++++++++
drivers/gpu/drm/nouveau/nouveau_drv.h | 6 +++
2 files changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index afbf557..6cd6723 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -24,6 +24,7 @@
*
*/
+#include <acpi/video.h>
#include <drm/drmP.h>
#include <drm/drm_crtc_helper.h>
@@ -42,6 +43,8 @@
#include <nvif/cl0046.h>
#include <nvif/event.h>
+
+
static int
nouveau_display_vblank_handler(struct nvif_notify *notify)
{
@@ -358,6 +361,55 @@ static struct nouveau_drm_prop_enum_list dither_depth[] = {
} \
} while(0)
+#ifdef CONFIG_ACPI
+
+/*
+ * Hans de Goede: This define belongs in acpi/video.h, I've submitted a patch
+ * to the acpi subsys to move it there from drivers/acpi/acpi_video.c .
+ * This should 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
the _sync merely means that we are in a context where we can sleep,
so that if the runtime use count goes to 0 the runtime pm core can
call the idle handler directly rather then from a wq. The _sync does
not mean that we will suspend autmatically.
pm_runtime_put_autosuspend OTOH will actually cause an *immediate*
suspend if the runtime use count goes to 0, which is not what we
want, we want userspace to have some time to react to new connections
(if any) before doing a needless suspend/resume cycle.
Regards,
Hans
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(&drm->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(&drm->acpi_work, nouveau_display_acpi_work);
+ drm->acpi_nb.notifier_call = nouveau_display_acpi_ntfy;
+ register_acpi_notifier(&drm->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(&nouveau_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 <linux/notifier.h>
+
#include <nvif/client.h>
#include <nvif/device.h>
#include <nvif/ioctl.h>
@@ -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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
