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: <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. Hopefully Ben can clarify this situation. One other comment below. > Signed-off-by: Hans de Goede <hdegoede at redhat.com> > --- > 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 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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau -- Kind regards, Peter Wu https://lekensteyn.nl