[Intel-gfx] [PATCH 1/3] drm/i915: Nuke the LVDS lid notifier

Rodrigo Vivi rodrigo.vivi at intel.com
Tue Jul 17 19:57:51 UTC 2018


On Tue, Jul 17, 2018 at 08:42:14PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> We broke the LVDS notifier resume thing in (presumably) commit
> e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.") as
> we no longer duplicate the current state in the LVDS notifier and
> thus we never resume it properly either.
>
> Instead of trying to fix it again let's just kill off the lid
> notifier entirely. None of the machines tested thus far have
> apparently needed it. Originally the lid notifier was added to
> work around cases where the VBIOS was clobbering some of the
> hardware state behind the driver's back, mostly on Thinkpads.
> We now have a few report of Thinkpads working just fine without
> the notifier. So maybe it was misdiagnosed originally, or
> something else has changed (ACPI video stuff perhaps?).
>
> If we do end up finding a machine where the VBIOS is still causing
> problems I would suggest that we first try setting various bits in
> the VBIOS scratch registers. There are several to choose from that
> may instruct the VBIOS to steer clear.
>
> With the notifier gone we'll also stop looking at the panel status
> in ->detect().
>
> Cc: stable at vger.kernel.org
> Cc: Wolfgang Draxinger <wdraxinger.maillist at draxit.de>
> Cc: Vito Caputo <vcaputo at pengaru.com>
> Cc: kitsunyan <kitsunyan at airmail.cc>
> Cc: Joonas Saarinen <jza at saunalahti.fi>
> Tested-by: Vito Caputo <vcaputo at pengaru.com> # Thinkapd X61s
> Tested-by: kitsunyan <kitsunyan at airmail.cc> # ThinkPad X200
> Tested-by: Joonas Saarinen <jza at saunalahti.fi> # Fujitsu Siemens U9210
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105902
> References: https://lists.freedesktop.org/archives/intel-gfx/2018-June/169315.html
> References: https://bugs.freedesktop.org/show_bug.cgi?id=21230
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c   |  10 ---
>  drivers/gpu/drm/i915/i915_drv.h   |   2 -
>  drivers/gpu/drm/i915/intel_lvds.c | 136 +-------------------------------------
>  3 files changed, 2 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3834bd758a2e..f8cfd16be534 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -900,7 +900,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	spin_lock_init(&dev_priv->uncore.lock);
>
>  	mutex_init(&dev_priv->sb_lock);
> -	mutex_init(&dev_priv->modeset_restore_lock);
>  	mutex_init(&dev_priv->av_mutex);
>  	mutex_init(&dev_priv->wm.wm_mutex);
>  	mutex_init(&dev_priv->pps_mutex);
> @@ -1570,11 +1569,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	pci_power_t opregion_target_state;
>
> -	/* ignore lid events during suspend */
> -	mutex_lock(&dev_priv->modeset_restore_lock);
> -	dev_priv->modeset_restore = MODESET_SUSPENDED;
> -	mutex_unlock(&dev_priv->modeset_restore_lock);
> -
>  	disable_rpm_wakeref_asserts(dev_priv);
>
>  	/* We do a lot of poking in a lot of registers, make sure they work
> @@ -1770,10 +1764,6 @@ static int i915_drm_resume(struct drm_device *dev)
>
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
>
> -	mutex_lock(&dev_priv->modeset_restore_lock);
> -	dev_priv->modeset_restore = MODESET_DONE;
> -	mutex_unlock(&dev_priv->modeset_restore_lock);
> -
>  	intel_opregion_notify_adapter(dev_priv, PCI_D0);
>
>  	enable_rpm_wakeref_asserts(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4fb937399440..1b0af905b74c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h

- enum modeset_restore {
-        MODESET_ON_LID_OPEN,
-        MODESET_DONE,
-        MODESET_SUSPENDED,
- };

with that:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

> @@ -1730,8 +1730,6 @@ struct drm_i915_private {
>
>  	unsigned long quirks;
>
> -	enum modeset_restore modeset_restore;
> -	struct mutex modeset_restore_lock;
>  	struct drm_atomic_state *modeset_restore_state;
>  	struct drm_modeset_acquire_ctx reset_ctx;
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index ca55b0a82ba6..f9f3b0885ba5 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -44,8 +44,6 @@
>  /* Private structure for the integrated LVDS support */
>  struct intel_lvds_connector {
>  	struct intel_connector base;
> -
> -	struct notifier_block lid_notifier;
>  };
>
>  struct intel_lvds_pps {
> @@ -452,26 +450,9 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  	return true;
>  }
>
> -/*
> - * Detect the LVDS connection.
> - *
> - * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
> - * connected and closed means disconnected.  We also send hotplug events as
> - * needed, using lid status notification from the input layer.
> - */
>  static enum drm_connector_status
>  intel_lvds_detect(struct drm_connector *connector, bool force)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> -	enum drm_connector_status status;
> -
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> -
> -	status = intel_panel_detect(dev_priv);
> -	if (status != connector_status_unknown)
> -		return status;
> -
>  	return connector_status_connected;
>  }
>
> @@ -496,117 +477,6 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
>  	return 1;
>  }
>
> -static int intel_no_modeset_on_lid_dmi_callback(const struct dmi_system_id *id)
> -{
> -	DRM_INFO("Skipping forced modeset for %s\n", id->ident);
> -	return 1;
> -}
> -
> -/* The GPU hangs up on these systems if modeset is performed on LID open */
> -static const struct dmi_system_id intel_no_modeset_on_lid[] = {
> -	{
> -		.callback = intel_no_modeset_on_lid_dmi_callback,
> -		.ident = "Toshiba Tecra A11",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "TECRA A11"),
> -		},
> -	},
> -
> -	{ }	/* terminating entry */
> -};
> -
> -/*
> - * Lid events. Note the use of 'modeset':
> - *  - we set it to MODESET_ON_LID_OPEN on lid close,
> - *    and set it to MODESET_DONE on open
> - *  - we use it as a "only once" bit (ie we ignore
> - *    duplicate events where it was already properly set)
> - *  - the suspend/resume paths will set it to
> - *    MODESET_SUSPENDED and ignore the lid open event,
> - *    because they restore the mode ("lid open").
> - */
> -static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> -			    void *unused)
> -{
> -	struct intel_lvds_connector *lvds_connector =
> -		container_of(nb, struct intel_lvds_connector, lid_notifier);
> -	struct drm_connector *connector = &lvds_connector->base.base;
> -	struct drm_device *dev = connector->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -
> -	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
> -		return NOTIFY_OK;
> -
> -	mutex_lock(&dev_priv->modeset_restore_lock);
> -	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> -		goto exit;
> -	/*
> -	 * check and update the status of LVDS connector after receiving
> -	 * the LID nofication event.
> -	 */
> -	connector->status = connector->funcs->detect(connector, false);
> -
> -	/* Don't force modeset on machines where it causes a GPU lockup */
> -	if (dmi_check_system(intel_no_modeset_on_lid))
> -		goto exit;
> -	if (!acpi_lid_open()) {
> -		/* do modeset on next lid open event */
> -		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> -		goto exit;
> -	}
> -
> -	if (dev_priv->modeset_restore == MODESET_DONE)
> -		goto exit;
> -
> -	/*
> -	 * Some old platform's BIOS love to wreak havoc while the lid is closed.
> -	 * We try to detect this here and undo any damage. The split for PCH
> -	 * platforms is rather conservative and a bit arbitrary expect that on
> -	 * those platforms VGA disabling requires actual legacy VGA I/O access,
> -	 * and as part of the cleanup in the hw state restore we also redisable
> -	 * the vga plane.
> -	 */
> -	if (!HAS_PCH_SPLIT(dev_priv))
> -		intel_display_resume(dev);
> -
> -	dev_priv->modeset_restore = MODESET_DONE;
> -
> -exit:
> -	mutex_unlock(&dev_priv->modeset_restore_lock);
> -	return NOTIFY_OK;
> -}
> -
> -static int
> -intel_lvds_connector_register(struct drm_connector *connector)
> -{
> -	struct intel_lvds_connector *lvds = to_lvds_connector(connector);
> -	int ret;
> -
> -	ret = intel_connector_register(connector);
> -	if (ret)
> -		return ret;
> -
> -	lvds->lid_notifier.notifier_call = intel_lid_notify;
> -	if (acpi_lid_notifier_register(&lvds->lid_notifier)) {
> -		DRM_DEBUG_KMS("lid notifier registration failed\n");
> -		lvds->lid_notifier.notifier_call = NULL;
> -	}
> -
> -	return 0;
> -}
> -
> -static void
> -intel_lvds_connector_unregister(struct drm_connector *connector)
> -{
> -	struct intel_lvds_connector *lvds = to_lvds_connector(connector);
> -
> -	if (lvds->lid_notifier.notifier_call)
> -		acpi_lid_notifier_unregister(&lvds->lid_notifier);
> -
> -	intel_connector_unregister(connector);
> -}
> -
>  /**
>   * intel_lvds_destroy - unregister and free LVDS structures
>   * @connector: connector to free
> @@ -639,8 +509,8 @@ static const struct drm_connector_funcs intel_lvds_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_get_property = intel_digital_connector_atomic_get_property,
>  	.atomic_set_property = intel_digital_connector_atomic_set_property,
> -	.late_register = intel_lvds_connector_register,
> -	.early_unregister = intel_lvds_connector_unregister,
> +	.late_register = intel_connector_register,
> +	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_lvds_destroy,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
> @@ -1114,8 +984,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	 * 2) check for VBT data
>  	 * 3) check to see if LVDS is already on
>  	 *    if none of the above, no panel
> -	 * 4) make sure lid is open
> -	 *    if closed, act like it's not there for now
>  	 */
>
>  	/*
> --
> 2.16.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


More information about the Intel-gfx mailing list