[Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE
Peter Wu
peter at lekensteyn.nl
Thu Nov 10 10:58:19 UTC 2016
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
More information about the dri-devel
mailing list