Re: [Nouveau] [PATCH] drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE

[Peter Wu <peter@lekensteyn.nl>]

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

-- 
Kind regards,
Peter Wu
https://lekensteyn.nl
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel