Re: [Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE
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:
=> 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:
=> 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
---
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
#include
#include
@@ -42,6 +43,8 @@
#include
#include
+
+
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_PROBE0x81
+#endif
+
+static void
+nouveau_display_acpi_work(struc
Re: [Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE
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.
> 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.
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:
=> 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:
=> nvkm_device_init
=> nvkm_udevice_init
This was observed on kernel 4.8.6-1-ARCH using kprobe tracing. Hopefully
Ben can clarify this situation. One other comment below.
> Signed-off-by: Hans de Goede
> ---
> 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
> #include
> #include
>
> @@ -42,6 +43,8 @@
> #include
> #include
>
> +
> +
> 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(*d
[Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE
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.
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.
Signed-off-by: Hans de Goede
---
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
#include
#include
@@ -42,6 +43,8 @@
#include
#include
+
+
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_PROBE0x81
+#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);
+}
+
+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_
