| Message ID | 20161109171744.4064-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
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@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 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/nouveau >
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); +} + +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;
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 <hdegoede@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(+)