[PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

This is a follow-up to the v1 posted in April:
http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html


Patches 1 - 17 enable GPU switching on the pre-retina MacBook Pro.
These were tested successfully by multiple people and solve two
tickets in Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=88861
https://bugs.freedesktop.org/show_bug.cgi?id=61115

Patches 18 - 22 are a preview of how we're tackling retina support.
Those are marked experimental and are NOT ready to be merged yet.
Feedback on them is welcome.

The patches are based on drm-next.

They were tested on the following hardware (thanks a lot everyone!):
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]


What's new:

* By default the MBP boots with the display switched to the discrete GPU
  but it can be forced to the integrated GPU with an EFI boot variable.
  Here's a handy tool for that: https://github.com/0xbb/gpu-switch
  v1 didn't work in this configuration, v2 does.

* Reprobing if the inactive GPU initializes before the apple-gmux module:
  v1 used Matthew Garrett's approach of adding a driver callback.
  v2 simply generates a hotplug event instead. nouveau polls its outputs
  every 10 seconds so we want it to poll immediately once apple-gmux
  registers. That is achieved by the hotplug event. The i915 driver is
  changed to behave identically to nouveau. (Right now it deletes LVDS
  and eDP connectors from the mode configuration if they can't be probed,
  deeming them to be ghosts.)

* Framebuffer recreation if the inactive GPU initializes before the
  apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
  with a new one with the actual panel resolution): v1 only supported this
  for i915, v2 has a generic solution which works with nouveau and radeon
  as well. (Necessary if the discrete GPU is forced to be the inactive one
  on boot via the EFI variable.)

* Generally lots of rough edges were smoothed to hopefully make the
  patches more suitable for merging. E.g. there's a bug in i915 where
  the SSC status set by BIOS is preserved too late and v1 only contained
  a workaround, whereas v2 contains a proper fix in a separate commit.


The long journey towards retina support:

The pixel clock required for retina resolution is not supported by LVDS
(which was used on pre-retinas), necessitating eDP instead. Problem is,
the gmux register which switches the DDC lines on pre-retinas doesn't
switch the AUX channel on retinas. Disassembling the OS X driver revealed
that the gmux in retina MBPs gained an additional register 0x7f which gets
written to when setting up the eDP configuration. There was some hope that
this might switch the AUX channel. Alas, we tried writing various values
to that register but were unable to get the inactive GPU to talk to the
panel. The purpose of register 0x7f remains a mystery.

Teardowns of the first generation retina MBP name the NXP CBTL06142 and
TI HD3SS212 as multiplexers and according to the data sheets I've found,
neither supports switching the AUX channel separately from the main link.

Matthew Garrett had the idea of having the active GPU stash the EDID and
the first 8 bytes of the DPCD (receiver capabilities) and letting the
inactive GPU retrieve that data. I rebased and rewrote his patches and
got everything working, only to discover that the drivers are unhappy
with just 8 bytes of DPCD. They need full access to the DPCD to set up
their outputs. We could stash the entire DPCD but some parts of it are
mutable so the stashed data may become stale when the active GPU performs
writes to the DPCD.

So I had the idea of using the active GPU as a proxy to talk to the panel,
thus emulating switching of the AUX channel in software. We can leverage
the struct drm_dp_aux and i2c_adapter (for DDC) to achieve this, swapping
the inactive GPU's structs with those of the active GPU on the fly.
That approach is implemented in patches 18 - 22 but there are still some
driver issues that I'm debugging. The current status as per the latest
logs Bruno sent me is that i915 rejects the mode retrieved via proxying
with CLOCK_HIGH and nouveau aborts link training halfway through.
Bottom line is that it's not yet working but we're getting closer.

As a side effect, the pre-retinas gain a second way to initialize their
outputs: They can either use gmux to switch the DDC lines, or use the
active GPU as a proxy for the DDC communication. Which method gets used
depends on the order in which the drivers initialize, the inactive GPU
will happily use whatever is available and it automatically receives
a hotplug event once either method becomes ready for use.

But, once again, the patches implementing proxying (patches 18 - 22)
are still in a state of flux and not ready for prime time, unlike the
prior ones which seem stable. Folks are hereby invited to poke holes
into them and I'm looking forward to your feedback.

Thanks,

Lukas


Dave Airlie (1):
  vga_switcheroo: Lock/unlock DDC lines

Lukas Wunner (15):
  vga_switcheroo: Lock/unlock DDC lines harder
  Revert "vga_switcheroo: Add helper function to get the active client"
  Revert "vga_switcheroo: add reprobe hook for fbcon to recheck
    connected outputs."
  drm/nouveau: Lock/unlock DDC lines on probe
  vga_switcheroo: Generate hotplug event on handler and proxy
    registration
  drm/i915: Preserve SSC earlier
  drm/i915: Reprobe eDP and LVDS connectors on hotplug event
  drm/i915: On fb alloc failure, unref gem object where it gets refed
  drm: Create new fb and replace default 1024x768 fb on hotplug event
  drm/nouveau/timer: Fall back to kernel timer if GPU timer read failed
  vga_switcheroo: Allow using active client as proxy when reading
    DDC/AUX
  drm: Amend struct drm_dp_aux with connector attribute
  drm: Use vga_switcheroo active client as proxy when reading DDC/AUX
  drm/nouveau/i2c: Use vga_switcheroo active client as proxy when
    reading DDC/AUX
  drm/nouveau: Use vga_switcheroo active client as proxy when probing
    DDC on LVDS

Matthew Garrett (1):
  apple-gmux: Assign apple_gmux_data before registering

Seth Forshee (4):
  vga_switcheroo: Add support for switching only the DDC
  vga_switcheroo: Add helper function to get the active client
  apple-gmux: Add switch_ddc support
  drm/edid: Switch DDC when reading the EDID

Tvrtko Ursulin (1):
  drm/i915: Fix failure paths around initial fbdev allocation

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c        |   1 -
 drivers/gpu/drm/drm_dp_helper.c                   |  14 ++
 drivers/gpu/drm/drm_edid.c                        |  23 ++-
 drivers/gpu/drm/drm_fb_helper.c                   |  41 ++++-
 drivers/gpu/drm/i915/i915_dma.c                   |   1 -
 drivers/gpu/drm/i915/intel_display.c              |  91 +++++++---
 drivers/gpu/drm/i915/intel_dp.c                   |  39 +++--
 drivers/gpu/drm/i915/intel_drv.h                  |   6 +
 drivers/gpu/drm/i915/intel_fbdev.c                |  46 ++---
 drivers/gpu/drm/i915/intel_lvds.c                 |  67 ++++---
 drivers/gpu/drm/i915/intel_panel.c                |   4 +-
 drivers/gpu/drm/msm/edp/edp_connector.c           |   2 +
 drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h |   1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c       |  18 +-
 drivers/gpu/drm/nouveau/nouveau_dp.c              |  20 +++
 drivers/gpu/drm/nouveau/nouveau_fbcon.c           |  10 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c             |   8 -
 drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c  |   6 +-
 drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c |   2 +-
 drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h |   1 +
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c     |  24 +++
 drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c |   4 +
 drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c  |   9 +
 drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h  |   1 +
 drivers/gpu/drm/radeon/atombios_dp.c              |   1 +
 drivers/gpu/drm/radeon/radeon_device.c            |   1 -
 drivers/gpu/drm/radeon/radeon_fb.c                |  11 +-
 drivers/gpu/drm/tegra/sor.c                       |   1 +
 drivers/gpu/vga/vga_switcheroo.c                  | 204 +++++++++++++++++++++-
 drivers/platform/x86/apple-gmux.c                 |  35 +++-
 include/drm/drm_dp_helper.h                       |   5 +
 include/linux/vga_switcheroo.h                    |  18 +-
 32 files changed, 590 insertions(+), 125 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v2 12/22] drm/i915: Preserve SSC earlier

[Lukas Wunner]

Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
added code to intel_modeset_gem_init to override the SSC status read
from VBT with the SSC status set by BIOS.

However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
which calls intel_setup_outputs, which *modifies* SSC status by way of
intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
doesn't preserve the SSC status set by BIOS but whatever
intel_init_pch_refclk decided on.

This is a problem on dual gpu laptops such as the MacBook Pro which
require either a handler to switch DDC lines, or the discrete gpu
to proxy DDC/AUX communication: Both the handler and the discrete
gpu may initialize after the i915 driver, and consequently, an LVDS
connector may initially seem disconnected and the SSC therefore
is disabled by intel_init_pch_refclk, but on reprobe the connector
may turn out to be connected and the SSC must then be enabled.

Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
it is assumed BIOS disabled it while in fact it was disabled by
intel_init_pch_refclk.

Also, because the SSC status is preserved so late, the preserved value
only ever gets used on resume but not on panel initialization:
intel_modeset_init calls intel_init_display which indirectly calls
intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
is the sole user of dev_priv->vbt.lvds_use_ssc).

Fix this by moving the code introduced by 92122789b2d6 from
intel_modeset_gem_init to intel_modeset_init before the invocation
of intel_setup_outputs and intel_init_display.

Add a DRM_DEBUG_KMS as suggested way back by Jani:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..6335883 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)
 	if (INTEL_INFO(dev)->num_pipes == 0)
 		return;
 
+	/*
+	 * There may be no VBT; and if the BIOS enabled SSC we can
+	 * just keep using it to avoid unnecessary flicker.  Whereas if the
+	 * BIOS isn't using it, don't assume it will work even if the VBT
+	 * indicates as much.
+	 */
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
+					    DREF_SSC1_ENABLE);
+
+		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
+			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
+				     bios_lvds_use_ssc ? "en" : "dis",
+				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
+			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
+		}
+	}
+
 	intel_init_display(dev);
 	intel_init_audio(dev);
 
@@ -15446,7 +15464,6 @@ err:
 
 void intel_modeset_gem_init(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *c;
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	intel_init_gt_powersave(dev);
 	mutex_unlock(&dev->struct_mutex);
 
-	/*
-	 * There may be no VBT; and if the BIOS enabled SSC we can
-	 * just keep using it to avoid unnecessary flicker.  Whereas if the
-	 * BIOS isn't using it, don't assume it will work even if the VBT
-	 * indicates as much.
-	 */
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
-		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
-						DREF_SSC1_ENABLE);
-
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 13/22] drm/i915: Reprobe eDP and LVDS connectors on hotplug event

[Lukas Wunner]

The i915 driver probes eDP and LVDS connectors once on startup by
invoking intel_setup_outputs(). If no DPCD or EDID can be obtained,
it will remove the connectors from the device's mode configuration,
presuming they're ghost connectors. As a result, subsequent calls to
drm_fb_helper_hotplug_event() won't be able to pick up changes on
these connectors.

This is a problem on dual gpu laptops such as the MacBook Pro which
require either a handler to switch DDC lines, or the discrete gpu
to proxy the DDC/AUX communication: Both the handler and the discrete
gpu may initialize after the i915 driver, and consequently, eDP and
LVDS connectors which may seem disconnected at startup may later turn
out to be connected.

By contrast, nouveau will keep eDP and LVDS connectors for which no
modes were found in the device's mode configuration and thus is able
to reprobe these connectors in drm_fb_helper_hotplug_event().

Assimilate to nouveau's behaviour: Keep modeless eDP and LVDS connectors
in the mode configuration and change the ->output_poll_changed callback
to reprobe them on hotplug events.

In the case of LVDS, split intel_lvds_init() in half: The first portion
is executed once on startup. This consists of detecting, setting up and
registering the connector. The second portion is executed both on
startup and on every reprobe. This consists of reading the panel's EDID,
determining if dual channel LVDS is used, and initializing the reference
clock.

In the case of eDP, reprobe involves calling intel_edp_init_connector()
and initializing the reference clock.

Based (loosely) on a patch by Matthew Garrett <mjg59@srcf.ucam.org> who
duplicated intel_setup_outputs() and reduced it to just the eDP probing
portion (which is not sufficient since pre-retina MBPs used LVDS):
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0024-i915-Add-support-for-reprobing-for-a-panel.patch

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dp.c      | 38 ++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
 drivers/gpu/drm/i915/intel_lvds.c    | 67 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_panel.c   |  4 +--
 5 files changed, 112 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6335883..907b73e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8223,11 +8223,11 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
 	for_each_intel_encoder(dev, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
-			has_panel = true;
+			has_panel = intel_lvds_has_panel(encoder);
 			has_lvds = true;
 			break;
 		case INTEL_OUTPUT_EDP:
-			has_panel = true;
+			has_panel = intel_edp_has_panel(encoder);
 			if (enc_to_dig_port(&encoder->base)->port == PORT_A)
 				has_cpu_edp = true;
 			break;
@@ -14478,15 +14478,44 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	return intel_framebuffer_create(dev, mode_cmd, obj);
 }
 
-#ifndef CONFIG_DRM_I915_FBDEV
-static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
+static void intel_output_poll_changed(struct drm_device *dev)
 {
-}
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector;
+
+	/* Reprobe LVDS and eDP as long as no EDID was retrieved from panel */
+	for_each_intel_connector(dev, intel_connector) {
+		struct drm_connector *connector = &intel_connector->base;
+
+		if ((connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
+		     connector->connector_type != DRM_MODE_CONNECTOR_eDP) ||
+		    !IS_ERR_OR_NULL(intel_connector->edid))
+			continue;
+
+		if ((connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
+		     !intel_lvds_probe_modes(connector)) ||
+		    (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
+		     !intel_edp_init_connector(
+			      enc_to_intel_dp(&intel_connector->encoder->base),
+			      intel_connector)))
+			continue;
+
+		intel_init_pch_refclk(dev);
+		drm_helper_move_panel_connectors_to_head(dev);
+		mutex_lock(&dev_priv->backlight_lock);
+		if (intel_connector->panel.backlight.device == NULL)
+			intel_backlight_device_register(intel_connector);
+		mutex_unlock(&dev_priv->backlight_lock);
+	}
+
+#ifdef CONFIG_DRM_I915_FBDEV
+	intel_fbdev_output_poll_changed(dev);
 #endif
+}
 
 static const struct drm_mode_config_funcs intel_mode_funcs = {
 	.fb_create = intel_user_framebuffer_create,
-	.output_poll_changed = intel_fbdev_output_poll_changed,
+	.output_poll_changed = intel_output_poll_changed,
 	.atomic_check = intel_atomic_check,
 	.atomic_commit = intel_atomic_commit,
 	.atomic_state_alloc = intel_atomic_state_alloc,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f93..23aa4ff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1998,6 +1998,15 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 	pps_unlock(intel_dp);
 }
 
+bool intel_edp_has_panel(struct intel_encoder *intel_encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+
+	return intel_dp->dpcd[DP_DPCD_REV] != 0 &&
+	       intel_connector->panel.fixed_mode != NULL;
+}
+
 /* Enable backlight in the panel power control. */
 static void _intel_edp_backlight_on(struct intel_dp *intel_dp)
 {
@@ -4338,6 +4347,9 @@ edp_detect(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	enum drm_connector_status status;
 
+	if (!intel_edp_has_panel(intel_dp->attached_connector->encoder))
+		return connector_status_disconnected;
+
 	status = intel_panel_detect(dev);
 	if (status == connector_status_unknown)
 		status = connector_status_connected;
@@ -5599,8 +5611,8 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 	return downclock_mode;
 }
 
-static bool intel_edp_init_connector(struct intel_dp *intel_dp,
-				     struct intel_connector *intel_connector)
+bool intel_edp_init_connector(struct intel_dp *intel_dp,
+			      struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -5614,9 +5626,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct edid *edid;
 	enum pipe pipe = INVALID_PIPE;
 
-	if (!is_edp(intel_dp))
-		return true;
-
 	pps_lock(intel_dp);
 	intel_edp_panel_vdd_sanitize(intel_dp);
 	pps_unlock(intel_dp);
@@ -5630,8 +5639,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 				intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
 				DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
 	} else {
-		/* if this fails, presume the device is a ghost */
-		DRM_INFO("failed to retrieve link info, disabling eDP\n");
+		DRM_INFO("failed to retrieve eDP link info\n");
 		return false;
 	}
 
@@ -5676,8 +5684,12 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	mutex_unlock(&dev->mode_config.mutex);
 
 	if (IS_VALLEYVIEW(dev)) {
-		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
-		register_reboot_notifier(&intel_dp->edp_notifier);
+		if (intel_dp->edp_notifier.notifier_call == NULL) {
+			intel_dp->edp_notifier.notifier_call =
+				edp_notify_handler;
+			if (register_reboot_notifier(&intel_dp->edp_notifier))
+				intel_dp->edp_notifier.notifier_call = NULL;
+		}
 
 		/*
 		 * Figure out the current pipe for the initial backlight setup.
@@ -5817,9 +5829,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_dp_mst_encoder_init(intel_dig_port,
 					  intel_connector->base.base.id);
 
-	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
-		drm_dp_aux_unregister(&intel_dp->aux);
-		if (is_edp(intel_dp)) {
+	if (is_edp(intel_dp)) {
+		if (!intel_edp_init_connector(intel_dp, intel_connector)) {
 			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 			/*
 			 * vdd might still be enabled do to the delayed vdd off.
@@ -5829,9 +5840,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			edp_panel_vdd_off_sync(intel_dp);
 			pps_unlock(intel_dp);
 		}
-		drm_connector_unregister(connector);
-		drm_connector_cleanup(connector);
-		return false;
 	}
 
 	intel_dp_add_properties(intel_dp, connector);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47cef0e..361320b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1171,11 +1171,14 @@ bool intel_dp_compute_config(struct intel_encoder *encoder,
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
 enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
 				  bool long_hpd);
+bool intel_edp_init_connector(struct intel_dp *intel_dp,
+			      struct intel_connector *intel_connector);
 void intel_edp_backlight_on(struct intel_dp *intel_dp);
 void intel_edp_backlight_off(struct intel_dp *intel_dp);
 void intel_edp_panel_vdd_on(struct intel_dp *intel_dp);
 void intel_edp_panel_on(struct intel_dp *intel_dp);
 void intel_edp_panel_off(struct intel_dp *intel_dp);
+bool intel_edp_has_panel(struct intel_encoder *intel_encoder);
 void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector);
 void intel_dp_mst_suspend(struct drm_device *dev);
 void intel_dp_mst_resume(struct drm_device *dev);
@@ -1258,6 +1261,8 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_device *dev);
+bool intel_lvds_probe_modes(struct drm_connector *connector);
+bool intel_lvds_has_panel(struct intel_encoder *intel_encoder);
 bool intel_is_dual_link_lvds(struct drm_device *dev);
 
 
@@ -1307,6 +1312,7 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_connector *connector);
 void intel_backlight_register(struct drm_device *dev);
 void intel_backlight_unregister(struct drm_device *dev);
+int intel_backlight_device_register(struct intel_connector *connector);
 
 
 /* intel_psr.c */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index cb634f4..4c7c8a2 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -359,7 +359,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 /**
  * Detect the LVDS connection.
  *
- * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
+ * Since LVDS doesn't have hotplug, 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.
  */
@@ -372,6 +372,9 @@ intel_lvds_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	if (!intel_lvds_has_panel(to_intel_connector(connector)->encoder))
+		return connector_status_disconnected;
+
 	status = intel_panel_detect(dev);
 	if (status != connector_status_unknown)
 		return status;
@@ -936,13 +939,6 @@ void intel_lvds_init(struct drm_device *dev)
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
-	struct drm_display_mode *scan; /* *modes, *bios_mode; */
-	struct drm_display_mode *fixed_mode = NULL;
-	struct drm_display_mode *downclock_mode = NULL;
-	struct edid *edid;
-	struct drm_crtc *crtc;
-	u32 lvds;
-	int pipe;
 	u8 pin;
 
 	/*
@@ -1052,6 +1048,29 @@ void intel_lvds_init(struct drm_device *dev)
 				      dev->mode_config.scaling_mode_property,
 				      DRM_MODE_SCALE_ASPECT);
 	intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
+
+	drm_connector_register(connector);
+	intel_lvds_probe_modes(connector);
+}
+
+bool intel_lvds_probe_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_lvds_connector *lvds_connector = to_lvds_connector(connector);
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	struct drm_encoder *encoder = &intel_encoder->base;
+	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
+	struct drm_display_mode *scan;
+	struct drm_display_mode *fixed_mode = NULL;
+	struct drm_display_mode *downclock_mode = NULL;
+	struct edid *edid;
+	struct drm_crtc *crtc;
+	u32 lvds;
+	int pipe;
+	u8 pin = GMBUS_PIN_PANEL;
+
 	/*
 	 * LVDS discovery:
 	 * 1) check for EDID on DDC
@@ -1079,7 +1098,7 @@ void intel_lvds_init(struct drm_device *dev)
 	} else {
 		edid = ERR_PTR(-ENOENT);
 	}
-	lvds_connector->base.edid = edid;
+	intel_connector->edid = edid;
 
 	if (IS_ERR_OR_NULL(edid)) {
 		/* Didn't get an EDID, so
@@ -1155,24 +1174,30 @@ out:
 	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
 				 LVDS_A3_POWER_MASK;
 
-	lvds_connector->lid_notifier.notifier_call = intel_lid_notify;
-	if (acpi_lid_notifier_register(&lvds_connector->lid_notifier)) {
-		DRM_DEBUG_KMS("lid notifier registration failed\n");
-		lvds_connector->lid_notifier.notifier_call = NULL;
+	if (lvds_connector->lid_notifier.notifier_call == NULL) {
+		lvds_connector->lid_notifier.notifier_call = intel_lid_notify;
+		if (acpi_lid_notifier_register(&lvds_connector->lid_notifier)) {
+			DRM_DEBUG_KMS("lid notifier registration failed\n");
+			lvds_connector->lid_notifier.notifier_call = NULL;
+		}
 	}
-	drm_connector_register(connector);
 
 	intel_panel_setup_backlight(connector, INVALID_PIPE);
 
-	return;
+	return true;
 
 failed:
 	mutex_unlock(&dev->mode_config.mutex);
 
-	DRM_DEBUG_KMS("No LVDS modes found, disabling.\n");
-	drm_connector_cleanup(connector);
-	drm_encoder_cleanup(encoder);
-	kfree(lvds_encoder);
-	kfree(lvds_connector);
-	return;
+	DRM_DEBUG_KMS("No LVDS modes found\n");
+	return false;
+}
+
+bool intel_lvds_has_panel(struct intel_encoder *intel_encoder)
+{
+	struct intel_lvds_connector *lvds_connector =
+		to_lvds_encoder(&intel_encoder->base)->attached_connector;
+	struct intel_connector *intel_connector = &lvds_connector->base;
+
+	return intel_connector->panel.fixed_mode != NULL;
 }
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index e2ab3f6..4dbfb3df 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1139,7 +1139,7 @@ static const struct backlight_ops intel_backlight_device_ops = {
 	.get_brightness = intel_backlight_device_get_brightness,
 };
 
-static int intel_backlight_device_register(struct intel_connector *connector)
+int intel_backlight_device_register(struct intel_connector *connector)
 {
 	struct intel_panel *panel = &connector->panel;
 	struct backlight_properties props;
@@ -1202,7 +1202,7 @@ static void intel_backlight_device_unregister(struct intel_connector *connector)
 	}
 }
 #else /* CONFIG_BACKLIGHT_CLASS_DEVICE */
-static int intel_backlight_device_register(struct intel_connector *connector)
+int intel_backlight_device_register(struct intel_connector *connector)
 {
 	return 0;
 }
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 14/22 RESEND] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We had two failure modes here:

1.
Deadlock in intelfb_alloc failure path where it calls
drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
(caller of intelfb_alloc) was already holding it.

2.
Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex and
intelfb_create was already holding it.

v2:
   * Reformat commit msg to 72 chars. (Lukas Wunner)
   * Add third failure mode. (Lukas Wunner)

v3:
   * On fb alloc failure, unref gem object where it gets refed,
     fix double unref in separate commit. (Ville Syrjälä)

v4:
   * Lock struct mutex on unref. (Chris Wilson)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
    framebuffer_references--")
Reported-by: Lukas Wunner <lukas@wunner.de>
[Lukas: Create v3 + v4 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 7eff33f..dd138aa 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -140,7 +140,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
-	struct drm_framebuffer *fb;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_device *dev = helper->dev;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct drm_i915_gem_object *obj;
@@ -158,6 +158,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
+	mutex_lock(&dev->struct_mutex);
+
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
 	obj = i915_gem_object_create_stolen(dev, size);
@@ -179,18 +181,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
-		goto out_fb;
+		goto out_unref;
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-out_fb:
-	drm_framebuffer_remove(fb);
 out_unref:
 	drm_gem_object_unreference(&obj->base);
 out:
+	mutex_unlock(&dev->struct_mutex);
+	if (fb)
+		drm_framebuffer_remove(fb);
 	return ret;
 }
 
@@ -208,8 +213,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	int size, ret;
 	bool prealloc = false;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
@@ -224,7 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
-			goto out_unlock;
+			return ret;
 		intel_fb = ifbdev->fb;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
@@ -236,6 +239,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	mutex_lock(&dev->struct_mutex);
+
 	info = framebuffer_alloc(0, &dev->pdev->dev);
 	if (!info) {
 		ret = -ENOMEM;
@@ -306,7 +311,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
-out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 15/22 RESEND] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Lukas Wunner]

Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call chain in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).

This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).

As suggested by Ville Syrjälä, improve code clarity by moving the unref
away from __intel_framebuffer_create to where the gem object gets refed.

[Changelog is in preceding patch by Tvrtko Ursulin.]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
    allocation")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 907b73e..5984d5f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10145,20 +10145,17 @@ __intel_framebuffer_create(struct drm_device *dev,
 	int ret;
 
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference(&obj->base);
+	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
 	if (ret)
 		goto err;
 
 	return &intel_fb->base;
+
 err:
-	drm_gem_object_unreference(&obj->base);
 	kfree(intel_fb);
-
 	return ERR_PTR(ret);
 }
 
@@ -10198,6 +10195,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 
@@ -10212,7 +10210,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 								bpp);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	return intel_framebuffer_create(dev, &mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static struct drm_framebuffer *
@@ -14468,6 +14470,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
 			      struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14475,7 +14478,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return intel_framebuffer_create(dev, mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static void intel_output_poll_changed(struct drm_device *dev)
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/22] drm/i915: Preserve SSC earlier

[Jesse Barnes]

On 07/15/2015 04:57 AM, Lukas Wunner wrote:
> Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
> added code to intel_modeset_gem_init to override the SSC status read
> from VBT with the SSC status set by BIOS.
> 
> However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
> which calls intel_setup_outputs, which *modifies* SSC status by way of
> intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
> doesn't preserve the SSC status set by BIOS but whatever
> intel_init_pch_refclk decided on.
> 
> This is a problem on dual gpu laptops such as the MacBook Pro which
> require either a handler to switch DDC lines, or the discrete gpu
> to proxy DDC/AUX communication: Both the handler and the discrete
> gpu may initialize after the i915 driver, and consequently, an LVDS
> connector may initially seem disconnected and the SSC therefore
> is disabled by intel_init_pch_refclk, but on reprobe the connector
> may turn out to be connected and the SSC must then be enabled.
> 
> Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
> it is assumed BIOS disabled it while in fact it was disabled by
> intel_init_pch_refclk.
> 
> Also, because the SSC status is preserved so late, the preserved value
> only ever gets used on resume but not on panel initialization:
> intel_modeset_init calls intel_init_display which indirectly calls
> intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
> overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
> is the sole user of dev_priv->vbt.lvds_use_ssc).
> 
> Fix this by moving the code introduced by 92122789b2d6 from
> intel_modeset_gem_init to intel_modeset_init before the invocation
> of intel_setup_outputs and intel_init_display.
> 
> Add a DRM_DEBUG_KMS as suggested way back by Jani:
> http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
> 
> Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..6335883 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->num_pipes == 0)
>  		return;
>  
> +	/*
> +	 * There may be no VBT; and if the BIOS enabled SSC we can
> +	 * just keep using it to avoid unnecessary flicker.  Whereas if the
> +	 * BIOS isn't using it, don't assume it will work even if the VBT
> +	 * indicates as much.
> +	 */
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> +		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
> +					    DREF_SSC1_ENABLE);
> +
> +		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
> +			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
> +				     bios_lvds_use_ssc ? "en" : "dis",
> +				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
> +			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
> +		}
> +	}
> +
>  	intel_init_display(dev);
>  	intel_init_audio(dev);
>  
> @@ -15446,7 +15464,6 @@ err:
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_init_gt_powersave(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	/*
> -	 * There may be no VBT; and if the BIOS enabled SSC we can
> -	 * just keep using it to avoid unnecessary flicker.  Whereas if the
> -	 * BIOS isn't using it, don't assume it will work even if the VBT
> -	 * indicates as much.
> -	 */
> -	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> -		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
> -						DREF_SSC1_ENABLE);
> -
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> 

Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/22] drm/i915: Preserve SSC earlier

[Jani Nikula]

On Mon, 31 Aug 2015, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On 07/15/2015 04:57 AM, Lukas Wunner wrote:
>> Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
>> added code to intel_modeset_gem_init to override the SSC status read
>> from VBT with the SSC status set by BIOS.
>> 
>> However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
>> which calls intel_setup_outputs, which *modifies* SSC status by way of
>> intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
>> doesn't preserve the SSC status set by BIOS but whatever
>> intel_init_pch_refclk decided on.
>> 
>> This is a problem on dual gpu laptops such as the MacBook Pro which
>> require either a handler to switch DDC lines, or the discrete gpu
>> to proxy DDC/AUX communication: Both the handler and the discrete
>> gpu may initialize after the i915 driver, and consequently, an LVDS
>> connector may initially seem disconnected and the SSC therefore
>> is disabled by intel_init_pch_refclk, but on reprobe the connector
>> may turn out to be connected and the SSC must then be enabled.
>> 
>> Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
>> it is assumed BIOS disabled it while in fact it was disabled by
>> intel_init_pch_refclk.
>> 
>> Also, because the SSC status is preserved so late, the preserved value
>> only ever gets used on resume but not on panel initialization:
>> intel_modeset_init calls intel_init_display which indirectly calls
>> intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
>> overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
>> is the sole user of dev_priv->vbt.lvds_use_ssc).
>> 
>> Fix this by moving the code introduced by 92122789b2d6 from
>> intel_modeset_gem_init to intel_modeset_init before the invocation
>> of intel_setup_outputs and intel_init_display.
>> 
>> Add a DRM_DEBUG_KMS as suggested way back by Jani:
>> http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html
>> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
>> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
>> Tested-by: William Brown <william@blackhats.net.au>
>>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
>> Tested-by: Lukas Wunner <lukas@wunner.de>
>>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
>> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
>> 
>> Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index af0bcfe..6335883 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)
>>  	if (INTEL_INFO(dev)->num_pipes == 0)
>>  		return;
>>  
>> +	/*
>> +	 * There may be no VBT; and if the BIOS enabled SSC we can
>> +	 * just keep using it to avoid unnecessary flicker.  Whereas if the
>> +	 * BIOS isn't using it, don't assume it will work even if the VBT
>> +	 * indicates as much.
>> +	 */
>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>> +		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
>> +					    DREF_SSC1_ENABLE);
>> +
>> +		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
>> +			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
>> +				     bios_lvds_use_ssc ? "en" : "dis",
>> +				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
>> +			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
>> +		}
>> +	}
>> +
>>  	intel_init_display(dev);
>>  	intel_init_audio(dev);
>>  
>> @@ -15446,7 +15464,6 @@ err:
>>  
>>  void intel_modeset_gem_init(struct drm_device *dev)
>>  {
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_crtc *c;
>>  	struct drm_i915_gem_object *obj;
>>  	int ret;
>> @@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  	intel_init_gt_powersave(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> -	/*
>> -	 * There may be no VBT; and if the BIOS enabled SSC we can
>> -	 * just keep using it to avoid unnecessary flicker.  Whereas if the
>> -	 * BIOS isn't using it, don't assume it will work even if the VBT
>> -	 * indicates as much.
>> -	 */
>> -	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
>> -		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
>> -						DREF_SSC1_ENABLE);
>> -
>>  	intel_modeset_init_hw(dev);
>>  
>>  	intel_setup_overlay(dev);
>> 
>
> Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Pushed this one patch to drm-intel-next-fixes, thanks for the patch and
review.

BR,
Jani.


>
> Thanks,
> Jesse
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Daniel Vetter]

On Tue, Aug 11, 2015 at 12:29:17PM +0200, Lukas Wunner wrote:
> This is a follow-up to the v1 posted in April:
> http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
> 
> 
> Patches 1 - 17 enable GPU switching on the pre-retina MacBook Pro.
> These were tested successfully by multiple people and solve two
> tickets in Bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=88861
> https://bugs.freedesktop.org/show_bug.cgi?id=61115
> 
> Patches 18 - 22 are a preview of how we're tackling retina support.
> Those are marked experimental and are NOT ready to be merged yet.
> Feedback on them is welcome.
> 
> The patches are based on drm-next.
> 
> They were tested on the following hardware (thanks a lot everyone!):
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
> 
> 
> What's new:
> 
> * By default the MBP boots with the display switched to the discrete GPU
>   but it can be forced to the integrated GPU with an EFI boot variable.
>   Here's a handy tool for that: https://github.com/0xbb/gpu-switch
>   v1 didn't work in this configuration, v2 does.
> 
> * Reprobing if the inactive GPU initializes before the apple-gmux module:
>   v1 used Matthew Garrett's approach of adding a driver callback.
>   v2 simply generates a hotplug event instead. nouveau polls its outputs
>   every 10 seconds so we want it to poll immediately once apple-gmux
>   registers. That is achieved by the hotplug event. The i915 driver is
>   changed to behave identically to nouveau. (Right now it deletes LVDS
>   and eDP connectors from the mode configuration if they can't be probed,
>   deeming them to be ghosts.)

I thought -EDEFERREDPROBE is what we should be using if drivers don't get
loaded in the right order? Hand-rolling depency avoidance stuff is imo a
horrible idea.

> * Framebuffer recreation if the inactive GPU initializes before the
>   apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
>   with a new one with the actual panel resolution): v1 only supported this
>   for i915, v2 has a generic solution which works with nouveau and radeon
>   as well. (Necessary if the discrete GPU is forced to be the inactive one
>   on boot via the EFI variable.)

Would completely remove the need for this ;-)

> * Generally lots of rough edges were smoothed to hopefully make the
>   patches more suitable for merging. E.g. there's a bug in i915 where
>   the SSC status set by BIOS is preserved too late and v1 only contained
>   a workaround, whereas v2 contains a proper fix in a separate commit.
> 
> 
> The long journey towards retina support:
> 
> The pixel clock required for retina resolution is not supported by LVDS
> (which was used on pre-retinas), necessitating eDP instead. Problem is,
> the gmux register which switches the DDC lines on pre-retinas doesn't
> switch the AUX channel on retinas. Disassembling the OS X driver revealed
> that the gmux in retina MBPs gained an additional register 0x7f which gets
> written to when setting up the eDP configuration. There was some hope that
> this might switch the AUX channel. Alas, we tried writing various values
> to that register but were unable to get the inactive GPU to talk to the
> panel. The purpose of register 0x7f remains a mystery.
> 
> Teardowns of the first generation retina MBP name the NXP CBTL06142 and
> TI HD3SS212 as multiplexers and according to the data sheets I've found,
> neither supports switching the AUX channel separately from the main link.
> 
> Matthew Garrett had the idea of having the active GPU stash the EDID and
> the first 8 bytes of the DPCD (receiver capabilities) and letting the
> inactive GPU retrieve that data. I rebased and rewrote his patches and
> got everything working, only to discover that the drivers are unhappy
> with just 8 bytes of DPCD. They need full access to the DPCD to set up
> their outputs. We could stash the entire DPCD but some parts of it are
> mutable so the stashed data may become stale when the active GPU performs
> writes to the DPCD.
> 
> So I had the idea of using the active GPU as a proxy to talk to the panel,
> thus emulating switching of the AUX channel in software. We can leverage
> the struct drm_dp_aux and i2c_adapter (for DDC) to achieve this, swapping
> the inactive GPU's structs with those of the active GPU on the fly.
> That approach is implemented in patches 18 - 22 but there are still some
> driver issues that I'm debugging. The current status as per the latest
> logs Bruno sent me is that i915 rejects the mode retrieved via proxying
> with CLOCK_HIGH and nouveau aborts link training halfway through.
> Bottom line is that it's not yet working but we're getting closer.
> 
> As a side effect, the pre-retinas gain a second way to initialize their
> outputs: They can either use gmux to switch the DDC lines, or use the
> active GPU as a proxy for the DDC communication. Which method gets used
> depends on the order in which the drivers initialize, the inactive GPU
> will happily use whatever is available and it automatically receives
> a hotplug event once either method becomes ready for use.
> 
> But, once again, the patches implementing proxying (patches 18 - 22)
> are still in a state of flux and not ready for prime time, unlike the
> prior ones which seem stable. Folks are hereby invited to poke holes
> into them and I'm looking forward to your feedback.

You can't share the dp aux like that. It's true that we need a bit more
data (there's a few eDP related feature blocsk we need), but sharing the
aux channel entirely is no-go. If you do you get drivers trying to link
train and at best this fails and at worst you'll upset the configuration
of the other driver and piss of the panel enough to need a hard reset
until it works again.

I think just reading edid and the relevant dp aux data in apple-gmux or
somewhere like that and stalling driver load until that's ready is the
only clean option. And of course we must ensure that inactive drivers
don't try to use the epd link at all since that will piss off the panel.

I think the real tricky bit here with vgaswitcheroo is locking, I need to
take a separate lock at the patches for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Daniel,

On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote:
> > * Reprobing if the inactive GPU initializes before the apple-gmux module:
> >   v1 used Matthew Garrett's approach of adding a driver callback.
> >   v2 simply generates a hotplug event instead. nouveau polls its outputs
> >   every 10 seconds so we want it to poll immediately once apple-gmux
> >   registers. That is achieved by the hotplug event. The i915 driver is
> >   changed to behave identically to nouveau. (Right now it deletes LVDS
> >   and eDP connectors from the mode configuration if they can't be probed,
> >   deeming them to be ghosts.)
> 
> I thought -EDEFERREDPROBE is what we should be using if drivers don't get
> loaded in the right order? Hand-rolling depency avoidance stuff is imo a
> horrible idea.
[...]
> I think just reading edid and the relevant dp aux data in apple-gmux or
> somewhere like that and stalling driver load until that's ready is the
> only clean option.

I'm afraid we can't stall initialization of a driver like that because
even though the GPU may not be switched to the panel, it may still have
an external monitor attached.

All MacBook Pros have external DP and/or HDMI ports and these are
either soldered to the discrete GPU (model year 2011 and onwards)
or switchable between the discrete and integrated GPU (until 2010;
I think they are even switchable separately from the panel).

So basically we'd have to initialize the driver normally, and if
intel_lvds_init() or intel_edp_init_connector() fail we'd have to
somehow pass that up the call chain so that i915_pci_probe() can
return -EPROBE_DEFER. And whenever we're asked to reprobe we'd
repeat initialization of the LVDS or eDP connector.

I'm wondering what the benefit is compared to just keeping the
connector in the mode configuration, but with status disconnected,
and reprobing it when the ->output_poll_changed callback gets invoked?
Because that's what nouveau already does, and what I've changed i915
to do with patch 13.

vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler
registers (patch 11), which will invoke ->output_poll_changed.
So we're talking about the Official DRM Callback [tm] to probe
outputs, not "hand-rolling depency avoidance". :-)


> > * Framebuffer recreation if the inactive GPU initializes before the
> >   apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
> >   with a new one with the actual panel resolution): v1 only supported this
> >   for i915, v2 has a generic solution which works with nouveau and radeon
> >   as well. (Necessary if the discrete GPU is forced to be the inactive one
> >   on boot via the EFI variable.)
> 
> Would completely remove the need for this ;-)

Unfortunately not: We'd still have to initialize the driver to be able
to drive external displays. If there are initially no connectors with
modes, we'll once again end up with the 1024x768 fb.


> You can't share the dp aux like that. It's true that we need a bit more
> data (there's a few eDP related feature blocsk we need), but sharing the
> aux channel entirely is no-go. If you do you get drivers trying to link
> train and at best this fails and at worst you'll upset the configuration
> of the other driver and piss of the panel enough to need a hard reset
> until it works again.

Yes, so far proxying of the AUX channel is read-only. In those cases
when writing is necessary for setting up the output, I'm adding code
to check if the current content of the DPCD is identical to what's
being written and if so, skip the write. We'll see if this stategy is
sufficient for the drivers to set up their outputs.


> I think the real tricky bit here with vgaswitcheroo is locking, I need to
> take a separate lock at the patches for that.

Locking when switching only the DDC lines is facilitated by the ddc_lock
attribute of struct vgasr_priv. This is all local to vga_switcheroo.c
and contained in patches 5 and 6.

Locking when proxying the AUX channel is facilitated by the hw_mutex
attribute of struct drm_dp_aux. nouveau has its own locking mechanism
contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus,
when proxying via nouveau, there are two locking mechanisms at work
(drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is
nothing introduced by this patch set, all existing code.

Locking of access to the struct vgasr_priv is facilitated by the
vgasr_mutex in vga_switcheroo.c. Also existing code.


Best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Daniel Vetter]

On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Daniel,
>
> On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote:
>> > * Reprobing if the inactive GPU initializes before the apple-gmux module:
>> >   v1 used Matthew Garrett's approach of adding a driver callback.
>> >   v2 simply generates a hotplug event instead. nouveau polls its outputs
>> >   every 10 seconds so we want it to poll immediately once apple-gmux
>> >   registers. That is achieved by the hotplug event. The i915 driver is
>> >   changed to behave identically to nouveau. (Right now it deletes LVDS
>> >   and eDP connectors from the mode configuration if they can't be probed,
>> >   deeming them to be ghosts.)
>>
>> I thought -EDEFERREDPROBE is what we should be using if drivers don't get
>> loaded in the right order? Hand-rolling depency avoidance stuff is imo a
>> horrible idea.
> [...]
>> I think just reading edid and the relevant dp aux data in apple-gmux or
>> somewhere like that and stalling driver load until that's ready is the
>> only clean option.
>
> I'm afraid we can't stall initialization of a driver like that because
> even though the GPU may not be switched to the panel, it may still have
> an external monitor attached.
>
> All MacBook Pros have external DP and/or HDMI ports and these are
> either soldered to the discrete GPU (model year 2011 and onwards)
> or switchable between the discrete and integrated GPU (until 2010;
> I think they are even switchable separately from the panel).
>
> So basically we'd have to initialize the driver normally, and if
> intel_lvds_init() or intel_edp_init_connector() fail we'd have to
> somehow pass that up the call chain so that i915_pci_probe() can
> return -EPROBE_DEFER. And whenever we're asked to reprobe we'd
> repeat initialization of the LVDS or eDP connector.
>
> I'm wondering what the benefit is compared to just keeping the
> connector in the mode configuration, but with status disconnected,
> and reprobing it when the ->output_poll_changed callback gets invoked?
> Because that's what nouveau already does, and what I've changed i915
> to do with patch 13.
>
> vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler
> registers (patch 11), which will invoke ->output_poll_changed.
> So we're talking about the Official DRM Callback [tm] to probe
> outputs, not "hand-rolling depency avoidance". :-)

Oh I didn't spot that one. This kind of layering inversions generally
leads to deadlocks and fun stuff. Also reprobing lvds/edp is just a
side-effect when you have fbdev emulation enabled. If we go with this
re-probing approach then we definitely need a new hook in
vga-switcheroo, and even then there's still the locking problem.

>> > * Framebuffer recreation if the inactive GPU initializes before the
>> >   apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
>> >   with a new one with the actual panel resolution): v1 only supported this
>> >   for i915, v2 has a generic solution which works with nouveau and radeon
>> >   as well. (Necessary if the discrete GPU is forced to be the inactive one
>> >   on boot via the EFI variable.)
>>
>> Would completely remove the need for this ;-)
>
> Unfortunately not: We'd still have to initialize the driver to be able
> to drive external displays. If there are initially no connectors with
> modes, we'll once again end up with the 1024x768 fb.

EDEFERREDPROBE isn't something that gets returned to userspace, it's
just internal handling so that the kernel knows there's a depency
issue and it needs to retry probing once other drivers have finished
loading. It is the generic means linux has to handle cross-driver
depencies which aren't reflected in the bus hierarchy.

I.e. it's just something to make sure that apple-gmux is fully loaded
before i915/nouveau. The driver _will_ be initialized eventually.

>> You can't share the dp aux like that. It's true that we need a bit more
>> data (there's a few eDP related feature blocsk we need), but sharing the
>> aux channel entirely is no-go. If you do you get drivers trying to link
>> train and at best this fails and at worst you'll upset the configuration
>> of the other driver and piss of the panel enough to need a hard reset
>> until it works again.
>
> Yes, so far proxying of the AUX channel is read-only. In those cases
> when writing is necessary for setting up the output, I'm adding code
> to check if the current content of the DPCD is identical to what's
> being written and if so, skip the write. We'll see if this stategy is
> sufficient for the drivers to set up their outputs.

You need to block anything that would write _much_ earlier. By the
time we're doing link-training it's way too late.

>> I think the real tricky bit here with vgaswitcheroo is locking, I need to
>> take a separate lock at the patches for that.
>
> Locking when switching only the DDC lines is facilitated by the ddc_lock
> attribute of struct vgasr_priv. This is all local to vga_switcheroo.c
> and contained in patches 5 and 6.
>
> Locking when proxying the AUX channel is facilitated by the hw_mutex
> attribute of struct drm_dp_aux. nouveau has its own locking mechanism
> contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus,
> when proxying via nouveau, there are two locking mechanisms at work
> (drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is
> nothing introduced by this patch set, all existing code.
>
> Locking of access to the struct vgasr_priv is facilitated by the
> vgasr_mutex in vga_switcheroo.c. Also existing code.

Making sure everything is protected is the easy part of locking
review. Making sure you can't deadlock is the hard part, and the
mutex_trylock game looks like that's a problem not handled correctly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Daniel,

On Wed, Aug 12, 2015 at 11:10:59PM +0200, Daniel Vetter wrote:
> Yes just squash and mention that the patch is based on work from
> $list_of_other_authors, plus cc them. There's not much point in
> acknowledging when people write broken patches ;-)

As you requested I've squashed the first 7 patches and I'll post
the resulting 3 replacement patches to dri-devel immediately after
sending this e-mail.

I've also overhauled locking.

These 3 patches lay the groundwork for DDC switching with gmux.
The subsequent patches in the series mostly concern reprobing
and though I've made locking-related changes to these as well,
I don't want to spam the list with them again until we have
reached consensus how reprobing should be implemented:


On Thu, Aug 13, 2015 at 08:50:47AM +0200, Daniel Vetter wrote:
> EDEFERREDPROBE isn't something that gets returned to userspace, it's
> just internal handling so that the kernel knows there's a depency
> issue and it needs to retry probing once other drivers have finished
> loading. It is the generic means linux has to handle cross-driver
> depencies which aren't reflected in the bus hierarchy.
> 
> I.e. it's just something to make sure that apple-gmux is fully loaded
> before i915/nouveau. The driver _will_ be initialized eventually.

Aha, so you want to stall initialization of i915/nouveau/radeon
*completely* until apple-gmux is loaded.

So how do you determine if initialization should be stalled?
You would have to hardcode DMIs for all MacBook Pro models.
I just counted it, there are 5 DMIs which require apple-gmux,
2 DMIs which require nouveau and 1 DMI which requires radeon
(they require nouveau/radeon for proxying, apple-gmux won't
help these models). And every year you would have to add another
DMI for the latest MacBook Pro model. People using the latest
model with an older kernel won't be able to use switching and
may open support tickets.

And if the module required for initialization is not installed
or has a different version than the kernel, i915 won't initialize
at all, which will be another source for support cases that you'll
have to deal with.

I think this doesn't scale and it shows that it's the wrong
approach.

vga_switcheroo *knows* when the handler registers, or the driver
to proxy AUX, and it can *notify* the inactive GPU's driver.
No need to hardcode DMIs, keep it simple.

And there *is* already a callback to notify drivers that they
should reprobe their outputs:

 * struct drm_mode_config_funcs - basic driver provided mode setting functions
 * @output_poll_changed: function to handle output configuration changes


> On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler
> > registers (patch 11), which will invoke ->output_poll_changed.
> 
> Oh I didn't spot that one. This kind of layering inversions generally
> leads to deadlocks and fun stuff.

Please elaborate why you think it's a layering inversion to call
drm_kms_helper_hotplug_event() from vga_switcheroo.


> Also reprobing lvds/edp is just a
> side-effect when you have fbdev emulation enabled.

No, even though ->output_poll_changed is most commonly used to
update the fbcon output configuration, it gets called even if
fbdev emulation is disabled, and I've changed i915's callback
in patch 13 so that the LVDS/eDP connectors are reprobed even
if CONFIG_DRM_I915_FBDEV is not set.


> If we go with this re-probing approach then we definitely
> need a new hook in vga-switcheroo

Why? From my point of view, drm_kms_helper_hotplug_event()
already does the job. The only problem is that i915 removes
LVDS and eDP connectors from the mode configuration if they
can't be probed. By contrast, nouveau (and I think radeon)
will just keep them, but with status disconnected. I changed
i915 with patch 13 to behave identically to nouveau/radeon.
(Sorry, I've written this before but I feel like I need to
overcommunicate in this medium.)

The question is, do you consider it harmful to keep a modeless
LVDS or eDP connector in the mode configuration (with status
disconnected)? On the MacBooks it works fine but I have no idea
if it causes issues on other platforms.

If you absolutely positively believe that this causes issues
and don't want to change i915's behaviour, then yes indeed we
may need a separate vga_switcheroo callback.


> and even then there's still the locking problem.

The only lock held when calling drm_kms_helper_hotplug_event()
from vga_switcheroo is vgasr_mutex. When can this deadlock?
Whenever we call a vga_switcheroo function from the driver
upon probing outputs, specifically:

- vga_switcheroo_client_fb_set()
  gets called if the fb is recreated on reprobe
- vga_switcheroo_lock_ddc() / _unlock_ddc()
  get called when probing DDC and retrieving the EDID
- vga_switcheroo_set_ddc() / _get_ddc() / _set_aux() / get_aux()
  get called for proxying

So we can't lock vgasr_mutex in those functions.

In the case of _lock_ddc() / _unlock_ddc() what we're actually
protecting against is the sudden deregistration of the handler
while we've switched DDC lines. We can solve that by grabbing
vgasr_priv.ddc_lock in vga_switcheroo_unregister_handler() to
block deregistration until we've switched back. This is what
I've done in v2.1 (the 3 patches accompanying this e-mail).

In the case of _fb_set() and the proxying functions, we grab
vgasr_mutex because we iterate over the client list and change
or retrieve client attributes. What we're actually protecting
against is the sudden deregistration of a client while we're
working on the client list. We can solve that by introducing
a new lock which is grabbed in vga_switcheroo_unregister_client(),
in _fb_set() and in the proxying functions.


Best regards,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Daniel, Hi Jani,

the patch set I've posted August 12 included 3 commits which fix bugs
in i915. These bugs should be fixed independently of MacBook Pro GPU
switching, please consider merging them:

drm/i915: Preserve SSC earlier
http://patchwork.freedesktop.org/patch/56921/

drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/

The latter two commits relate to a bug Jani was tracking before his
holidays which has unfortunately fallen by the wayside.

Thanks,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Jani Nikula]

On Sat, 29 Aug 2015, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Daniel, Hi Jani,
>
> the patch set I've posted August 12 included 3 commits which fix bugs
> in i915. These bugs should be fixed independently of MacBook Pro GPU
> switching, please consider merging them:
>
> drm/i915: Preserve SSC earlier
> http://patchwork.freedesktop.org/patch/56921/
>
> drm/i915: Fix failure paths around initial fbdev allocation
> http://patchwork.freedesktop.org/patch/53673/
> drm/i915: On fb alloc failure, unref gem object where it gets refed
> http://patchwork.freedesktop.org/patch/53674/
>
> The latter two commits relate to a bug Jani was tracking before his
> holidays which has unfortunately fallen by the wayside.

Sorry about that. Unfortunately the target is moving fast, and they no
longer apply. Please resend on top of current nightly.

BR,
Jani.


>
> Thanks,
>
> Lukas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Jani Nikula]

On Mon, 31 Aug 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Sat, 29 Aug 2015, Lukas Wunner <lukas@wunner.de> wrote:
>> Hi Daniel, Hi Jani,
>>
>> the patch set I've posted August 12 included 3 commits which fix bugs
>> in i915. These bugs should be fixed independently of MacBook Pro GPU
>> switching, please consider merging them:
>>
>> drm/i915: Preserve SSC earlier
>> http://patchwork.freedesktop.org/patch/56921/

Pushed this one.

Jani.

>>
>> drm/i915: Fix failure paths around initial fbdev allocation
>> http://patchwork.freedesktop.org/patch/53673/
>> drm/i915: On fb alloc failure, unref gem object where it gets refed
>> http://patchwork.freedesktop.org/patch/53674/
>>
>> The latter two commits relate to a bug Jani was tracking before his
>> holidays which has unfortunately fallen by the wayside.
>
> Sorry about that. Unfortunately the target is moving fast, and they no
> longer apply. Please resend on top of current nightly.
>
> BR,
> Jani.
>
>
>>
>> Thanks,
>>
>> Lukas
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Jani,

On Mon, Aug 31, 2015 at 10:15:07PM +0300, Jani Nikula wrote:
> On Sat, 29 Aug 2015, Lukas Wunner <lukas@wunner.de> wrote:
> > the patch set I've posted August 12 included 3 commits which fix bugs
> > in i915. These bugs should be fixed independently of MacBook Pro GPU
> > switching, please consider merging them:
> > [...]
> > drm/i915: Fix failure paths around initial fbdev allocation
> > http://patchwork.freedesktop.org/patch/53673/
> > drm/i915: On fb alloc failure, unref gem object where it gets refed
> > http://patchwork.freedesktop.org/patch/53674/
> 
> Sorry about that. Unfortunately the target is moving fast, and they no
> longer apply. Please resend on top of current nightly.

Alright, coming up in separate e-mails are the above 2 patches rebased on
drm-intel-nightly as of this morning. I didn't have to make any changes to
the code, if they didn't apply cleanly to your tree it was probably just
because of changed diff context.

To ease reviewing I've also pushed them to GitHub:
https://github.com/l1k/linux/commit/f0cd66427039ce1bdc61460a9d833e6d858cff3e
https://github.com/l1k/linux/commit/521e48fc5fc8d211ed2847070120ff4032b7a383

Briefly, the story of the 2 patches is this:
- I had originally reported the issue on June 3:
  http://lists.freedesktop.org/archives/intel-gfx/2015-June/067965.html
- Tvrtko came up with a patch which I've tested successfully:
  https://patchwork.freedesktop.org/patch/53207/
- However Ville responded to Tvrtko's patch: "I find it rather unexpected
  that the function drops the passed reference on error. My usual rule is:
  do nothing on error, if possible." (see comment section of patchwork link)
  Tvrtko answered that he didn't have time to look into this further and
  I wanted it fixed, so I submitted a set of 2 patches, consisting of an
  adjusted version of Tvrtko's patch, plus another one by me to address
  Ville's remarks.

So you can either merge Tvrtko's single patch or the 2 patches from me,
whichever you prefer. Or request something completely different.

Thanks,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx