[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 01/22] vga_switcheroo: Add support for switching only the DDC

[Lukas Wunner]

From: Seth Forshee <seth.forshee@canonical.com>

During graphics driver initialization its useful to be able to mux only
the DDC to the inactive client in order to read the EDID. Add a
switch_ddc callback to allow capable handlers to provide this
functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux
only the DDC.

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: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   |  4 ++++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 37ac7b5..0d3ac20 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -256,6 +256,29 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
+int vga_switcheroo_switch_ddc(struct pci_dev *pdev)
+{
+	int ret = 0;
+	int id;
+
+	mutex_lock(&vgasr_mutex);
+
+	if (!vgasr_priv.handler) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (vgasr_priv.handler->switch_ddc) {
+		id = vgasr_priv.handler->get_client_id(pdev);
+		ret = vgasr_priv.handler->switch_ddc(id);
+	}
+
+out:
+	mutex_unlock(&vgasr_mutex);
+	return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_switch_ddc);
+
 static int vga_switcheroo_show(struct seq_file *m, void *v)
 {
 	struct vga_switcheroo_client *client;
@@ -353,9 +376,15 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		console_unlock();
 	}
 
+	if (vgasr_priv.handler->switch_ddc) {
+		ret = vgasr_priv.handler->switch_ddc(new_client->id);
+		if (ret)
+			return ret;
+	}
+
 	ret = vgasr_priv.handler->switchto(new_client->id);
 	if (ret)
-		return ret;
+		goto restore_ddc;
 
 	if (new_client->ops->reprobe)
 		new_client->ops->reprobe(new_client->pdev);
@@ -367,6 +396,14 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 
 	new_client->active = true;
 	return 0;
+
+restore_ddc:
+	if (vgasr_priv.handler->switch_ddc) {
+		int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
+				VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
+		ret = vgasr_priv.handler->switch_ddc(id);
+	}
+	return ret;
 }
 
 static bool check_can_switch(void)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b483abd..2fef78b 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -29,6 +29,7 @@ enum vga_switcheroo_client_id {
 };
 
 struct vga_switcheroo_handler {
+	int (*switch_ddc)(enum vga_switcheroo_client_id id);
 	int (*switchto)(enum vga_switcheroo_client_id id);
 	int (*power_state)(enum vga_switcheroo_client_id id,
 			   enum vga_switcheroo_state state);
@@ -54,6 +55,8 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 				  struct fb_info *info);
 
+int vga_switcheroo_switch_ddc(struct pci_dev *pdev);
+
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);
 
@@ -72,6 +75,7 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
 static inline int vga_switcheroo_register_client(struct pci_dev *dev,
 		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
+static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; }
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-- 
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 02/22] vga_switcheroo: Add helper function to get the active client

[Lukas Wunner]

From: Seth Forshee <seth.forshee@canonical.com>

Add vga_switcheroo_get_active_client() to allow drivers to get the
active video client. This will be used by drivers wishing to temporarily
mux only the DDC to the inactive client.

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: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 14 ++++++++++++++
 include/linux/vga_switcheroo.h   |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 0d3ac20..620c4ac 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -209,6 +209,20 @@ find_active_client(struct list_head *head)
 	return NULL;
 }
 
+struct pci_dev *vga_switcheroo_get_active_client(void)
+{
+	struct vga_switcheroo_client *client;
+	struct pci_dev *pdev = NULL;
+
+	mutex_lock(&vgasr_mutex);
+	client = find_active_client(&vgasr_priv.clients);
+	if (client)
+		pdev = client->pdev;
+	mutex_unlock(&vgasr_mutex);
+	return pdev;
+}
+EXPORT_SYMBOL(vga_switcheroo_get_active_client);
+
 int vga_switcheroo_get_client_state(struct pci_dev *pdev)
 {
 	struct vga_switcheroo_client *client;
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 2fef78b..c81a686 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -62,6 +62,7 @@ void vga_switcheroo_unregister_handler(void);
 
 int vga_switcheroo_process_delayed_switch(void);
 
+struct pci_dev *vga_switcheroo_get_active_client(void);
 int vga_switcheroo_get_client_state(struct pci_dev *dev);
 
 void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic);
@@ -82,6 +83,7 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	int id, bool active) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
+static inline struct pci_dev *vga_switcheroo_get_active_client(void) { return NULL; }
 static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
 
 static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}
-- 
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 03/22] apple-gmux: Add switch_ddc support

[Lukas Wunner]

From: Seth Forshee <seth.forshee@canonical.com>

The gmux allows muxing the DDC independently from the display, so
support this functionality. This will allow reading the EDID for the
inactive GPU, fixing issues with machines that either don't have a VBT
or have invalid mode data in the VBT.

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: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 0dec3f5..f0a55a4 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -273,14 +273,21 @@ static const struct backlight_ops gmux_bl_ops = {
 	.update_status = gmux_update_status,
 };
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+	if (id == VGA_SWITCHEROO_IGD)
+		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+	else
+		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+	return 0;
+}
+
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
 	if (id == VGA_SWITCHEROO_IGD) {
-		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
 	} else {
-		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
 	}
@@ -347,6 +354,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data)
 }
 
 static struct vga_switcheroo_handler gmux_handler = {
+	.switch_ddc = gmux_switch_ddc,
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
-- 
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 04/22] drm/edid: Switch DDC when reading the EDID

[Lukas Wunner]

From: Seth Forshee <seth.forshee@canonical.com>

Some dual graphics machines support muxing the DDC separately from the
display, so make use of this functionality when reading the EDID on the
inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races
on the DDC mux state.

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: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/drm_edid.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e6e05bb..2424aef 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -32,6 +32,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_displayid.h>
@@ -87,6 +88,8 @@ struct detailed_mode_closure {
 #define LEVEL_GTF2	2
 #define LEVEL_CVT	3
 
+static DEFINE_MUTEX(drm_edid_mutex);
+
 static struct edid_quirk {
 	char vendor[4];
 	int product_id;
@@ -1377,6 +1380,16 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid;
+	struct pci_dev *pdev = connector->dev->pdev;
+	struct pci_dev *active_pdev = NULL;
+
+	mutex_lock(&drm_edid_mutex);
+
+	if (pdev) {
+		active_pdev = vga_switcheroo_get_active_client();
+		if (active_pdev != pdev)
+			vga_switcheroo_switch_ddc(pdev);
+	}
 
 	if (!drm_probe_ddc(adapter))
 		return NULL;
@@ -1384,6 +1397,11 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 	if (edid)
 		drm_get_displayid(connector, edid);
+
+	if (active_pdev && active_pdev != pdev)
+		vga_switcheroo_switch_ddc(active_pdev);
+
+	mutex_unlock(&drm_edid_mutex);
 	return edid;
 }
 EXPORT_SYMBOL(drm_get_edid);
-- 
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 05/22] vga_switcheroo: Lock/unlock DDC lines

[Lukas Wunner]

From: Dave Airlie <airlied@gmail.com>

Replace vga_switcheroo_switch_ddc() with vga_switcheroo_lock_ddc()
and vga_switcheroo_unlock_ddc(), move mutex from drm_get_edid() to
vga_switcheroo.c

Motivation for these changes according to Dave Airlie:
"I can't figure out why I didn't like this, but I rewrote this way
back to lock/unlock the ddc lines, bits are contained in this WIP
mess. I think I'd prefer something like that otherwise the interface
got really ugly."
http://lists.freedesktop.org/archives/dri-devel/2014-June/061629.html

Original commit by Dave Airlie contained additional experimental
and unused code. Reduced to the bare minimum and amended with this
commit message by Lukas Wunner <lukas@wunner.de>

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/drm_edid.c       | 16 ++------------
 drivers/gpu/vga/vga_switcheroo.c | 46 ++++++++++++++++++++++++++++++++++++++--
 include/linux/vga_switcheroo.h   |  3 ++-
 3 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2424aef..505eed1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -88,8 +88,6 @@ struct detailed_mode_closure {
 #define LEVEL_GTF2	2
 #define LEVEL_CVT	3
 
-static DEFINE_MUTEX(drm_edid_mutex);
-
 static struct edid_quirk {
 	char vendor[4];
 	int product_id;
@@ -1380,16 +1378,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid;
-	struct pci_dev *pdev = connector->dev->pdev;
-	struct pci_dev *active_pdev = NULL;
-
-	mutex_lock(&drm_edid_mutex);
 
-	if (pdev) {
-		active_pdev = vga_switcheroo_get_active_client();
-		if (active_pdev != pdev)
-			vga_switcheroo_switch_ddc(pdev);
-	}
+	vga_switcheroo_lock_ddc(connector->dev->pdev);
 
 	if (!drm_probe_ddc(adapter))
 		return NULL;
@@ -1398,10 +1388,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 	if (edid)
 		drm_get_displayid(connector, edid);
 
-	if (active_pdev && active_pdev != pdev)
-		vga_switcheroo_switch_ddc(active_pdev);
+	vga_switcheroo_unlock_ddc(connector->dev->pdev);
 
-	mutex_unlock(&drm_edid_mutex);
 	return edid;
 }
 EXPORT_SYMBOL(drm_get_edid);
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 620c4ac..0223020 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -57,6 +57,9 @@ struct vgasr_priv {
 	struct list_head clients;
 
 	struct vga_switcheroo_handler *handler;
+
+	struct mutex ddc_lock;
+	struct pci_dev *old_ddc_owner;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -70,6 +73,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);
 /* only one switcheroo per system */
 static struct vgasr_priv vgasr_priv = {
 	.clients = LIST_HEAD_INIT(vgasr_priv.clients),
+	.ddc_lock = __MUTEX_INITIALIZER(vgasr_priv.ddc_lock),
 };
 
 static bool vga_switcheroo_ready(void)
@@ -270,8 +274,9 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
-int vga_switcheroo_switch_ddc(struct pci_dev *pdev)
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 {
+	struct vga_switcheroo_client *client;
 	int ret = 0;
 	int id;
 
@@ -283,6 +288,18 @@ int vga_switcheroo_switch_ddc(struct pci_dev *pdev)
 	}
 
 	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
+
+		client = find_active_client(&vgasr_priv.clients);
+		if (!client) {
+			mutex_unlock(&vgasr_priv.ddc_lock);
+			ret = -ENODEV;
+			goto out;
+		}
+		vgasr_priv.old_ddc_owner = client->pdev;
+		if (client->pdev == pdev)
+			goto out;
+
 		id = vgasr_priv.handler->get_client_id(pdev);
 		ret = vgasr_priv.handler->switch_ddc(id);
 	}
@@ -291,7 +308,32 @@ out:
 	mutex_unlock(&vgasr_mutex);
 	return ret;
 }
-EXPORT_SYMBOL(vga_switcheroo_switch_ddc);
+EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
+
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
+{
+	int ret = 0;
+	int id;
+	mutex_lock(&vgasr_mutex);
+
+	if (!vgasr_priv.handler) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (vgasr_priv.handler->switch_ddc) {
+		if (vgasr_priv.old_ddc_owner != pdev) {
+			id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner);
+			ret = vgasr_priv.handler->switch_ddc(id);
+		}
+		vgasr_priv.old_ddc_owner = NULL;
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	}
+out:
+	mutex_unlock(&vgasr_mutex);
+	return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
 
 static int vga_switcheroo_show(struct seq_file *m, void *v)
 {
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index c81a686..352bef3 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -55,7 +55,8 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 				  struct fb_info *info);
 
-int vga_switcheroo_switch_ddc(struct pci_dev *pdev);
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
 
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);
-- 
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 06/22] vga_switcheroo: Lock/unlock DDC lines harder

[Lukas Wunner]

Unlock DDC lines if drm_probe_ddc() fails.

If the inactive client registers before the active client then
old_ddc_owner cannot be determined with find_active_client()
(null pointer dereference). Therefore change semantics of the
->switch_ddc handler callback to return old_ddc_owner on success
or a negative int on failure.

Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to
avoid locking inversion when one client locks vgasr_mutex on entering
vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another
client is holding ddc_lock and tries to acquire vgasr_mutex on entering
vga_switcheroo_unlock_ddc().

Lock ddc_lock in stage2 to avoid race condition where reading the
EDID and switching happens simultaneously.

Switch DDC lines on MIGD / MDIS commands and on runtime suspend.

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/drm_edid.c        |   9 ++--
 drivers/gpu/vga/vga_switcheroo.c  | 110 ++++++++++++++++++++++----------------
 drivers/platform/x86/apple-gmux.c |  15 +++++-
 include/linux/vga_switcheroo.h    |   3 +-
 4 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 505eed1..cdb2fa1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid;
+	struct pci_dev *pdev = connector->dev->pdev;
 
-	vga_switcheroo_lock_ddc(connector->dev->pdev);
+	vga_switcheroo_lock_ddc(pdev);
 
-	if (!drm_probe_ddc(adapter))
+	if (!drm_probe_ddc(adapter)) {
+		vga_switcheroo_unlock_ddc(pdev);
 		return NULL;
+	}
 
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 	if (edid)
 		drm_get_displayid(connector, edid);
 
-	vga_switcheroo_unlock_ddc(connector->dev->pdev);
+	vga_switcheroo_unlock_ddc(pdev);
 
 	return edid;
 }
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 0223020..f02e7fc 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -9,12 +9,13 @@
 
  Switcher interface - methods require for ATPX and DCM
  - switchto - this throws the output MUX switch
- - discrete_set_power - sets the power state for the discrete card
+ - switch_ddc - switch only DDC lines, return old DDC owner (or < 0 on failure)
+ - power_state - sets the power state for either GPU
 
  GPU driver interface
  - set_gpu_state - this should do the equiv of s/r for the card
 		  - this should *not* set the discrete power state
- - switch_check  - check if the device is in a position to switch now
+ - can_switch - check if the device is in a position to switch now
  */
 
 #include <linux/module.h>
@@ -59,7 +60,7 @@ struct vgasr_priv {
 	struct vga_switcheroo_handler *handler;
 
 	struct mutex ddc_lock;
-	struct pci_dev *old_ddc_owner;
+	enum vga_switcheroo_client_id old_ddc_owner;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -276,33 +277,24 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
 int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 {
-	struct vga_switcheroo_client *client;
-	int ret = 0;
-	int id;
+	int ret, id;
 
 	mutex_lock(&vgasr_mutex);
 
-	if (!vgasr_priv.handler) {
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
 		ret = -ENODEV;
 		goto out;
 	}
 
-	if (vgasr_priv.handler->switch_ddc) {
-		mutex_lock(&vgasr_priv.ddc_lock);
+	mutex_lock(&vgasr_priv.ddc_lock);
+	id = vgasr_priv.handler->get_client_id(pdev);
+	ret = vgasr_priv.handler->switch_ddc(id);
 
-		client = find_active_client(&vgasr_priv.clients);
-		if (!client) {
-			mutex_unlock(&vgasr_priv.ddc_lock);
-			ret = -ENODEV;
-			goto out;
-		}
-		vgasr_priv.old_ddc_owner = client->pdev;
-		if (client->pdev == pdev)
-			goto out;
-
-		id = vgasr_priv.handler->get_client_id(pdev);
-		ret = vgasr_priv.handler->switch_ddc(id);
-	}
+	if (ret < 0) {
+		pr_err("vga_switcheroo: failed to switch DDC lines\n");
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	} else
+		vgasr_priv.old_ddc_owner = ret;
 
 out:
 	mutex_unlock(&vgasr_mutex);
@@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
 
 int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
 {
-	int ret = 0;
-	int id;
-	mutex_lock(&vgasr_mutex);
+	int ret, id;
+	bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex);
 
-	if (!vgasr_priv.handler) {
-		ret = -ENODEV;
+	if (!mutex_is_locked(&vgasr_priv.ddc_lock)) {
+		ret = -EINVAL;
 		goto out;
 	}
 
-	if (vgasr_priv.handler->switch_ddc) {
-		if (vgasr_priv.old_ddc_owner != pdev) {
-			id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner);
-			ret = vgasr_priv.handler->switch_ddc(id);
-		}
-		vgasr_priv.old_ddc_owner = NULL;
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
+		pr_err("vga_switcheroo: handler disappeared\n");
 		mutex_unlock(&vgasr_priv.ddc_lock);
+		ret = -ENODEV;
+		goto out;
 	}
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	if (vgasr_priv.old_ddc_owner != id) {
+		ret = vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner);
+		if (ret < 0)
+			pr_err("vga_switcheroo: failed to switch DDC lines\n");
+	} else
+		ret = vgasr_priv.old_ddc_owner;
+	mutex_unlock(&vgasr_priv.ddc_lock);
+
 out:
-	mutex_unlock(&vgasr_mutex);
+	if (vgasr_mutex_acquired)
+		mutex_unlock(&vgasr_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
@@ -433,14 +433,26 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 	}
 
 	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		ret = vgasr_priv.handler->switch_ddc(new_client->id);
-		if (ret)
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		if (ret < 0) {
+			pr_err("vga_switcheroo: failed to switch DDC lines\n");
 			return ret;
+		}
 	}
 
 	ret = vgasr_priv.handler->switchto(new_client->id);
-	if (ret)
-		goto restore_ddc;
+	if (ret) {
+		pr_err("vga_switcheroo: failed to switch to client %d\n",
+		       new_client->id);
+		active->active = true;
+		/* restore DDC lines */
+		mutex_lock(&vgasr_priv.ddc_lock);
+		vgasr_priv.handler->switch_ddc(active->id);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		return ret;
+	}
 
 	if (new_client->ops->reprobe)
 		new_client->ops->reprobe(new_client->pdev);
@@ -452,14 +464,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 
 	new_client->active = true;
 	return 0;
-
-restore_ddc:
-	if (vgasr_priv.handler->switch_ddc) {
-		int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
-				VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
-		ret = vgasr_priv.handler->switch_ddc(id);
-	}
-	return ret;
 }
 
 static bool check_can_switch(void)
@@ -561,6 +565,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 	vgasr_priv.delayed_switch_active = false;
 
 	if (just_mux) {
+		if (vgasr_priv.handler->switch_ddc) {
+			mutex_lock(&vgasr_priv.ddc_lock);
+			ret = vgasr_priv.handler->switch_ddc(client_id);
+			mutex_unlock(&vgasr_priv.ddc_lock);
+			if (ret < 0) {
+				pr_err("vga_switcheroo: failed to switch DDC lines\n");
+				goto out;
+			}
+		}
 		ret = vgasr_priv.handler->switchto(client_id);
 		goto out;
 	}
@@ -716,6 +729,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
 	ret = dev->bus->pm->runtime_suspend(dev);
 	if (ret)
 		return ret;
+	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
+		ret = vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		if (ret < 0)
+			pr_err("vga_switcheroo: failed to switch DDC lines\n");
+	}
 	if (vgasr_priv.handler->switchto)
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
 	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index f0a55a4..08bdf1e 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = {
 
 static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
 {
+	enum vga_switcheroo_client_id old_ddc_owner;
+
+	if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
+		old_ddc_owner = VGA_SWITCHEROO_IGD;
+	else
+		old_ddc_owner = VGA_SWITCHEROO_DIS;
+
+	pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
+
+	if (id == old_ddc_owner)
+		return old_ddc_owner;
+
 	if (id == VGA_SWITCHEROO_IGD)
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
 	else
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
-	return 0;
+
+	return old_ddc_owner;
 }
 
 static int gmux_switchto(enum vga_switcheroo_client_id id)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 352bef3..39b25b0 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
 static inline int vga_switcheroo_register_client(struct pci_dev *dev,
 		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
-static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; }
+static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
+static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-- 
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 07/22] Revert "vga_switcheroo: Add helper function to get the active client"

[Lukas Wunner]

This reverts commit 26814ce68904c9faf977c90edac798156311981f.

The helper function is no longer needed after Dave Airlie's rewrite
of vga_switcheroo_switch_ddc(), the commit introducing it was only
included because 31f23c3d488e ("drm/edid: Switch DDC when reading
the EDID") does not compile without it.

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/vga/vga_switcheroo.c | 14 --------------
 include/linux/vga_switcheroo.h   |  2 --
 2 files changed, 16 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index f02e7fc..f0d5475 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -214,20 +214,6 @@ find_active_client(struct list_head *head)
 	return NULL;
 }
 
-struct pci_dev *vga_switcheroo_get_active_client(void)
-{
-	struct vga_switcheroo_client *client;
-	struct pci_dev *pdev = NULL;
-
-	mutex_lock(&vgasr_mutex);
-	client = find_active_client(&vgasr_priv.clients);
-	if (client)
-		pdev = client->pdev;
-	mutex_unlock(&vgasr_mutex);
-	return pdev;
-}
-EXPORT_SYMBOL(vga_switcheroo_get_active_client);
-
 int vga_switcheroo_get_client_state(struct pci_dev *pdev)
 {
 	struct vga_switcheroo_client *client;
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 39b25b0..62f303f 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -63,7 +63,6 @@ void vga_switcheroo_unregister_handler(void);
 
 int vga_switcheroo_process_delayed_switch(void);
 
-struct pci_dev *vga_switcheroo_get_active_client(void);
 int vga_switcheroo_get_client_state(struct pci_dev *dev);
 
 void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic);
@@ -85,7 +84,6 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	int id, bool active) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
-static inline struct pci_dev *vga_switcheroo_get_active_client(void) { return NULL; }
 static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
 
 static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}
-- 
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 08/22] Revert "vga_switcheroo: add reprobe hook for fbcon to recheck connected outputs."

[Lukas Wunner]

This reverts commit 8d608aa6295242fe4c4b6105b8c59c6a5b232d89.

Reprobing must happen asynchronously as the driver may grab vgasr_mutex
when invoking vga_switcheroo_client_fb_set(), when locking the DDC lines
or when using the upcoming DDC/AUX proxy functionality. And that lock is
already held in stage2 by vga_switcheroo_debugfs_write().

Infrastructure for asynchronous reprobing is added to vga_switcheroo
with a subsequent commit and could easily be used in stage2, but:

- The ->reprobe callback was only ever used by a single driver, nouveau.

- Drivers need to reprobe their outputs anyway after waking up, this is
  nothing specific to dual GPU laptops and should be added to the resume
  code of the driver.

- It would seem there's no need anymore to delay reprobing until after
  switching the mux: We have two methods now to make reprobing work with
  the display not switched (switching only the DDC lines and proxying),
  at least one of them should always work.

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/amd/amdgpu/amdgpu_device.c | 1 -
 drivers/gpu/drm/i915/i915_dma.c            | 1 -
 drivers/gpu/drm/nouveau/nouveau_vga.c      | 8 --------
 drivers/gpu/drm/radeon/radeon_device.c     | 1 -
 drivers/gpu/vga/vga_switcheroo.c           | 3 ---
 include/linux/vga_switcheroo.h             | 1 -
 6 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d79009b..55c8d51 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1076,7 +1076,6 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
 
 static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
 	.set_gpu_state = amdgpu_switcheroo_set_state,
-	.reprobe = NULL,
 	.can_switch = amdgpu_switcheroo_can_switch,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b1f9e55..c240944 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -388,7 +388,6 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 
 static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
 	.set_gpu_state = i915_switcheroo_set_state,
-	.reprobe = NULL,
 	.can_switch = i915_switcheroo_can_switch,
 };
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index c7592ec..42779f4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -55,13 +55,6 @@ nouveau_switcheroo_set_state(struct pci_dev *pdev,
 	}
 }
 
-static void
-nouveau_switcheroo_reprobe(struct pci_dev *pdev)
-{
-	struct drm_device *dev = pci_get_drvdata(pdev);
-	nouveau_fbcon_output_poll_changed(dev);
-}
-
 static bool
 nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 {
@@ -78,7 +71,6 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 static const struct vga_switcheroo_client_ops
 nouveau_switcheroo_ops = {
 	.set_gpu_state = nouveau_switcheroo_set_state,
-	.reprobe = nouveau_switcheroo_reprobe,
 	.can_switch = nouveau_switcheroo_can_switch,
 };
 
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index d8319da..2cb513f 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1258,7 +1258,6 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
 
 static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
 	.set_gpu_state = radeon_switcheroo_set_state,
-	.reprobe = NULL,
 	.can_switch = radeon_switcheroo_can_switch,
 };
 
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index f0d5475..8027c9f 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -440,9 +440,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		return ret;
 	}
 
-	if (new_client->ops->reprobe)
-		new_client->ops->reprobe(new_client->pdev);
-
 	if (active->pwr_state == VGA_SWITCHEROO_ON)
 		vga_switchoff(active);
 
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 62f303f..b935d83 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -39,7 +39,6 @@ struct vga_switcheroo_handler {
 
 struct vga_switcheroo_client_ops {
 	void (*set_gpu_state)(struct pci_dev *dev, enum vga_switcheroo_state);
-	void (*reprobe)(struct pci_dev *dev);
 	bool (*can_switch)(struct pci_dev *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 09/22] drm/nouveau: Lock/unlock DDC lines on probe

[Lukas Wunner]

On dual gpu laptops such as the MacBook Pro, ask vga_switcheroo to
switch the DDC lines to the Nvidia gpu before probing them.

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/nouveau/nouveau_connector.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 3162040..ad59eab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -27,6 +27,7 @@
 #include <acpi/button.h>
 
 #include <linux/pm_runtime.h>
+#include <linux/vga_switcheroo.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
@@ -148,7 +149,11 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 				break;
 		} else
 		if (nv_encoder->i2c) {
-			if (nv_probe_i2c(nv_encoder->i2c, 0x50))
+			int ret;
+			vga_switcheroo_lock_ddc(dev->pdev);
+			ret = nv_probe_i2c(nv_encoder->i2c, 0x50);
+			vga_switcheroo_unlock_ddc(dev->pdev);
+			if (ret)
 				break;
 		}
 	}
-- 
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 10/22] apple-gmux: Assign apple_gmux_data before registering

[Lukas Wunner]

From: Matthew Garrett <matthew.garrett@nebula.com>

Registering the handler after both GPUs will trigger a DDC switch for
connector reprobing. This will oops if apple_gmux_data hasn't already been
assigned. Reorder the code to do that.

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: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 08bdf1e..b120a11 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -609,18 +609,20 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		gmux_data->gpe = -1;
 	}
 
+	apple_gmux_data = gmux_data;
+	init_completion(&gmux_data->powerchange_done);
+	gmux_enable_interrupts(gmux_data);
+
 	if (vga_switcheroo_register_handler(&gmux_handler)) {
 		ret = -ENODEV;
 		goto err_register_handler;
 	}
 
-	init_completion(&gmux_data->powerchange_done);
-	apple_gmux_data = gmux_data;
-	gmux_enable_interrupts(gmux_data);
-
 	return 0;
 
 err_register_handler:
+	gmux_disable_interrupts(gmux_data);
+	apple_gmux_data = NULL;
 	if (gmux_data->gpe >= 0)
 		acpi_disable_gpe(NULL, gmux_data->gpe);
 err_enable_gpe:
-- 
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 11/22] vga_switcheroo: Generate hotplug event on handler and proxy registration

[Lukas Wunner]

On laptops which require the handler to switch DDC lines, already
registered clients must reprobe their connectors if the handler
registers late. This is the case on pre-retina MacBook Pros,
which use a gmux controller as handler.

Based (loosely) on a patch by Matthew Garrett <mjg59@srcf.ucam.org>
who used an additional driver callback rather than generating a
hotplug event:
http://lists.freedesktop.org/archives/dri-devel/2014-June/060941.html

Matthew Garrett invoked the driver callback in vga_switcheroo_enable().
On pre-retina MacBook Pros, this delayed reprobing until a second gpu
registers, which may never happen if its driver is not installed or
blacklisted. The MacBook Pro 2011 in particular suffers from widespread
gpu soldering issues due to overheating, eventually rendering it
unusable with OS X. Folks are solving this by moving to Linux and
disabling the discrete gpu entirely. In those cases we can't wait
for a second gpu to register, we do need to reprobe as soon as the
handler registers:
https://ifixit.org/blog/6882/why-i-drilled-holes-in-my-macbook-pro-and-put-it-in-the-oven/#comment-26745

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/vga/vga_switcheroo.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 8027c9f..94b0b6f 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -18,6 +18,8 @@
  - can_switch - check if the device is in a position to switch now
  */
 
+#include <drm/drm_crtc_helper.h>
+
 #include <linux/module.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
@@ -35,6 +37,7 @@
 struct vga_switcheroo_client {
 	struct pci_dev *pdev;
 	struct fb_info *fb_info;
+	struct work_struct reprobe_work;
 	int pwr_state;
 	const struct vga_switcheroo_client_ops *ops;
 	int id;
@@ -106,6 +109,30 @@ static void vga_switcheroo_enable(void)
 	vgasr_priv.active = true;
 }
 
+static void vga_switcheroo_reprobe_client(struct work_struct *work)
+{
+	struct vga_switcheroo_client *client =
+		container_of(work, struct vga_switcheroo_client, reprobe_work);
+	void (*generate_hotplug_event)(struct drm_device *);
+
+	generate_hotplug_event = symbol_get(drm_kms_helper_hotplug_event);
+	if (!generate_hotplug_event)
+		return;
+
+	generate_hotplug_event(pci_get_drvdata(client->pdev));
+}
+
+static void vga_switcheroo_reprobe_inactive_clients(void)
+{
+	struct vga_switcheroo_client *client;
+
+	list_for_each_entry(client, &vgasr_priv.clients, list)
+		if (!client->active && client_is_vga(client)) {
+			cancel_work_sync(&client->reprobe_work);
+			schedule_work(&client->reprobe_work);
+		}
+}
+
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler)
 {
 	mutex_lock(&vgasr_mutex);
@@ -115,11 +142,17 @@ int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler)
 	}
 
 	vgasr_priv.handler = handler;
+
 	if (vga_switcheroo_ready()) {
 		printk(KERN_INFO "vga_switcheroo: enabled\n");
 		vga_switcheroo_enable();
 	}
 	mutex_unlock(&vgasr_mutex);
+
+	/* clients which registered before the handler must reprobe */
+	if (vgasr_priv.handler->switch_ddc)
+		vga_switcheroo_reprobe_inactive_clients();
+
 	return 0;
 }
 EXPORT_SYMBOL(vga_switcheroo_register_handler);
@@ -153,6 +186,7 @@ static int register_client(struct pci_dev *pdev,
 	client->id = id;
 	client->active = active;
 	client->driver_power_control = driver_power_control;
+	INIT_WORK(&client->reprobe_work, vga_switcheroo_reprobe_client);
 
 	mutex_lock(&vgasr_mutex);
 	list_add_tail(&client->list, &vgasr_priv.clients);
-- 
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 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

[PATCH v2 16/22] drm: Create new fb and replace default 1024x768 fb on hotplug event

[Lukas Wunner]

If no connectors with modes are found, drm_fb_helper_single_fb_probe()
will allocate a default 1024x768 fb. This becomes a problem if one of
the connectors subsequently changes to connected status: We're stuck
with the default fb even if the display allows a larger resolution
(or doesn't support 1024x768 at all) since modes larger than the
existing fb will be deemed ineligible by drm_fb_helper_hotplug_event().

One relevant use case are dual gpu laptops such as the MacBook Pro
which require either a handler to switch DDC lines, or the active gpu
to proxy the DDC/AUX communication: Both the handler and the active gpu
may register late, and consequently, the inactive gpu's eDP and LVDS
connectors may seem disconnected at startup but may later on turn out
to be connected.

Although primarily aimed at vga_switcheroo setups, the patch may come
in handy in other use cases: One example is a desktop machine that is
(inadvertantly or otherwise) booted headless, and the user later on
plugs in a display. Another example is unplugging of the display by
the user and replacement with a larger one.

Solve this by checking if there aren't any modes at all. If so, don't
constrain probed modes to the existing fb size but rather allow them
to occupy the maximum surface size. After probing, check if we found
any modes. If we did, discard the existing fb and recreate it from
scratch by invoking the driver's ->fb_probe callback by way of
drm_fb_helper_single_fb_probe().

That function is normally called only once on driver initialization.
The fb_helper is kept so ensure that it's not added once more to the
panic handler list.

Because multiple hotplug events may fire simultaneously, protect
connector probing and fb recreation with drm_modeset_lock_all().
In particular, drm_fb_helper_hotplug_event() may itself fire another
hotplug event by way of drm_fb_helper_probe_connector_modes(), which
invokes the ->fill_modes callback, which for most drivers is
drm_helper_probe_single_connector_modes(), which schedules
output_poll_execute().

Amend the ->fb_probe callback in the i915, nouveau and radeon drivers
to discard an existing fb. Because the _destroy() function is now used
further up in intel_fbdev.c, nouveau_fbcon.c and radeon_fb.c, add a
prototype for it. This is intended to ease reviewing, the prototype
shall be replaced by the body of the function before this gets merged.

Drivers other than these three will just create an additional fb under
the limited circumstances described above, which wastes memory but is
otherwise "mostly harmless".

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/drm_fb_helper.c         | 41 ++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_fbdev.c      | 26 ++++++++++-----------
 drivers/gpu/drm/nouveau/nouveau_fbcon.c | 10 +++++++-
 drivers/gpu/drm/radeon/radeon_fb.c      | 11 ++++++---
 4 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 73f90f7..6df0ee8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1097,7 +1097,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 
-	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+	if (list_empty(&fb_helper->kernel_fb_list))
+		list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
 
 	return 0;
 }
@@ -1770,23 +1771,47 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	u32 max_width, max_height;
+	bool modes_before_probe = false;
+	bool modes_after_probe = false;
+	int i;
 
-	mutex_lock(&fb_helper->dev->mode_config.mutex);
+	drm_modeset_lock_all(dev);
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
-		mutex_unlock(&fb_helper->dev->mode_config.mutex);
+		drm_modeset_unlock_all(dev);
 		return 0;
 	}
 	DRM_DEBUG_KMS("\n");
 
-	max_width = fb_helper->fb->width;
-	max_height = fb_helper->fb->height;
+	/* If so far we have no modes and thus only the default 1024x768 fb,
+	 * allow probed modes to occupy the maximum surface size */
+	for (i = 0; i < fb_helper->crtc_count; i++)
+		if (fb_helper->crtc_info[i].desired_mode) {
+			modes_before_probe = true;
+			break;
+		}
+	if (!modes_before_probe) {
+		max_width = dev->mode_config.max_width;
+		max_height = dev->mode_config.max_width;
+	} else {
+		max_width = fb_helper->fb->width;
+		max_height = fb_helper->fb->height;
+	}
 
 	drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
-	mutex_unlock(&fb_helper->dev->mode_config.mutex);
-
-	drm_modeset_lock_all(dev);
 	drm_setup_crtcs(fb_helper);
+
+	/* If previously there were no connectors with modes and now we
+	 * found some, create new fb and replace default 1024x768 fb */
+	for (i = 0; i < fb_helper->crtc_count; i++)
+		if (fb_helper->crtc_info[i].desired_mode) {
+			modes_after_probe = true;
+			break;
+		}
+	if (!modes_before_probe && modes_after_probe)
+		drm_fb_helper_single_fb_probe(fb_helper,
+					      fb_helper->fb->bits_per_pixel);
+
 	drm_modeset_unlock_all(dev);
 	drm_fb_helper_set_par(fb_helper->fbdev);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index dd138aa..637146f 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -199,6 +199,8 @@ out:
 	return ret;
 }
 
+static void intel_fbdev_destroy(struct fb_info *info);
+
 static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
@@ -220,6 +222,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 			      " releasing it\n",
 			      intel_fb->base.width, intel_fb->base.height,
 			      sizes->fb_width, sizes->fb_height);
+		intel_fbdev_destroy(ifbdev->helper.fbdev);
 		drm_framebuffer_unreference(&intel_fb->base);
 		intel_fb = ifbdev->fb = NULL;
 	}
@@ -545,12 +548,9 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.fb_probe = intelfb_create,
 };
 
-static void intel_fbdev_destroy(struct drm_device *dev,
-				struct intel_fbdev *ifbdev)
+static void intel_fbdev_destroy(struct fb_info *info)
 {
-	if (ifbdev->helper.fbdev) {
-		struct fb_info *info = ifbdev->helper.fbdev;
-
+	if (info) {
 		unregister_framebuffer(info);
 		iounmap(info->screen_base);
 		if (info->cmap.len)
@@ -558,11 +558,6 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 		framebuffer_release(info);
 	}
-
-	drm_fb_helper_fini(&ifbdev->helper);
-
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_remove(&ifbdev->fb->base);
 }
 
 /*
@@ -750,13 +745,18 @@ void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 void intel_fbdev_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (!dev_priv->fbdev)
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+
+	if (!ifbdev)
 		return;
 
 	flush_work(&dev_priv->fbdev_suspend_work);
-
 	async_synchronize_full();
-	intel_fbdev_destroy(dev, dev_priv->fbdev);
+
+	intel_fbdev_destroy(ifbdev->helper.fbdev);
+	drm_fb_helper_fini(&ifbdev->helper);
+	drm_framebuffer_unregister_private(&ifbdev->fb->base);
+	drm_framebuffer_remove(&ifbdev->fb->base);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 6751553..7b7b112 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -305,6 +305,9 @@ nouveau_fbcon_zfill(struct drm_device *dev, struct nouveau_fbdev *fbcon)
 }
 
 static int
+nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon);
+
+static int
 nouveau_fbcon_create(struct drm_fb_helper *helper,
 		     struct drm_fb_helper_surface_size *sizes)
 {
@@ -322,6 +325,11 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
 	struct pci_dev *pdev = dev->pdev;
 	int size, ret;
 
+	if (helper->fbdev) {
+		nouveau_fbcon_accel_fini(dev);
+		nouveau_fbcon_destroy(dev, fbcon);
+	}
+
 	mode_cmd.width = sizes->surface_width;
 	mode_cmd.height = sizes->surface_height;
 
@@ -467,7 +475,6 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon)
 		drm_gem_object_unreference_unlocked(&nouveau_fb->nvbo->gem);
 		nouveau_fb->nvbo = NULL;
 	}
-	drm_fb_helper_fini(&fbcon->helper);
 	drm_framebuffer_unregister_private(&nouveau_fb->base);
 	drm_framebuffer_cleanup(&nouveau_fb->base);
 	return 0;
@@ -567,6 +574,7 @@ nouveau_fbcon_fini(struct drm_device *dev)
 
 	nouveau_fbcon_accel_fini(dev);
 	nouveau_fbcon_destroy(dev, drm->fbcon);
+	drm_fb_helper_fini(&drm->fbcon->helper);
 	kfree(drm->fbcon);
 	drm->fbcon = NULL;
 }
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index aeb6767..88b9f32 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -216,6 +216,8 @@ out_unref:
 	return ret;
 }
 
+static int radeon_fbdev_destroy(struct radeon_fbdev *rfbdev);
+
 static int radeonfb_create(struct drm_fb_helper *helper,
 			   struct drm_fb_helper_surface_size *sizes)
 {
@@ -231,6 +233,9 @@ static int radeonfb_create(struct drm_fb_helper *helper,
 	int ret;
 	unsigned long tmp;
 
+	if (helper->fbdev)
+		radeon_fbdev_destroy(rfbdev);
+
 	mode_cmd.width = sizes->surface_width;
 	mode_cmd.height = sizes->surface_height;
 
@@ -337,7 +342,7 @@ void radeon_fb_output_poll_changed(struct radeon_device *rdev)
 	drm_fb_helper_hotplug_event(&rdev->mode_info.rfbdev->helper);
 }
 
-static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfbdev)
+static int radeon_fbdev_destroy(struct radeon_fbdev *rfbdev)
 {
 	struct fb_info *info;
 	struct radeon_framebuffer *rfb = &rfbdev->rfb;
@@ -355,7 +360,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
 		radeonfb_destroy_pinned_object(rfb->obj);
 		rfb->obj = NULL;
 	}
-	drm_fb_helper_fini(&rfbdev->helper);
 	drm_framebuffer_unregister_private(&rfb->base);
 	drm_framebuffer_cleanup(&rfb->base);
 
@@ -419,7 +423,8 @@ void radeon_fbdev_fini(struct radeon_device *rdev)
 	if (!rdev->mode_info.rfbdev)
 		return;
 
-	radeon_fbdev_destroy(rdev->ddev, rdev->mode_info.rfbdev);
+	radeon_fbdev_destroy(rdev->mode_info.rfbdev);
+	drm_fb_helper_fini(&rdev->mode_info.rfbdev->helper);
 	kfree(rdev->mode_info.rfbdev);
 	rdev->mode_info.rfbdev = NULL;
 }
-- 
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 17/22] drm/nouveau/timer: Fall back to kernel timer if GPU timer read failed

[Lukas Wunner]

Unloading the nouveau module while the GPU is asleep (e.g. on dual GPU
laptops) leads to an infinite loop in nvkm_timer_wait_eq() because the
timer read out is 0xffffffffffffffff so the condition of the while loop
becomes -1 - (-1) < nsec and stays like that unless the GPU is woken up.

Use the kernel timer as fallback in this unlikely event. Synchronize the
kernel timer and GPU timer in nv04_timer_init() / gk20a_timer_init()
which should get called once on driver initialization and on every
resume.

Even with this fix applied, unloading the module takes a whopping
167 seconds. This could be reduced by changing the NV_WAIT_DEFAULT
timeout from the current (maybe excessive?) 2 seconds to 200 ms.

A WARN_ON is spewed out at nouveau_bo.c:398 after 81 seconds and
a null pointer dereference occurs in nouveau_cli_destroy(),
so there's more to fix here.

This patch might also be needed to properly handle a GPU connected
via Thunderbolt which is suddenly unplugged.

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/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 +
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c
index 80e3806..28d27ff 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c
@@ -41,6 +41,10 @@ gk20a_timer_init(struct nvkm_object *object)
 	/* restore the time before suspend */
 	nv_wr32(priv, NV04_PTIMER_TIME_1, hi);
 	nv_wr32(priv, NV04_PTIMER_TIME_0, lo);
+
+	/* save kernel time as fallback */
+	priv->suspend_ktime = ktime_to_ns(ktime_get()) - priv->suspend_time;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c
index 6b7facb..228749d 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c
@@ -36,6 +36,11 @@ nv04_timer_read(struct nvkm_timer *ptimer)
 		lo = nv_rd32(priv, NV04_PTIMER_TIME_0);
 	} while (hi != nv_rd32(priv, NV04_PTIMER_TIME_1));
 
+	if (unlikely(hi == -1 && lo == -1)) {
+		nv_spam(priv, "read failed, falling back to kernel timer\n");
+		return ktime_to_ns(ktime_get()) - priv->suspend_ktime;
+	}
+
 	return ((u64)hi << 32 | lo);
 }
 
@@ -216,6 +221,10 @@ nv04_timer_init(struct nvkm_object *object)
 	nv_wr32(priv, NV04_PTIMER_INTR_EN_0, 0x00000000);
 	nv_wr32(priv, NV04_PTIMER_TIME_1, hi);
 	nv_wr32(priv, NV04_PTIMER_TIME_0, lo);
+
+	/* save kernel time as fallback */
+	priv->suspend_ktime = ktime_to_ns(ktime_get()) - priv->suspend_time;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h
index 89996a9..1b83a0f 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h
@@ -15,6 +15,7 @@ struct nv04_timer_priv {
 	struct list_head alarms;
 	spinlock_t lock;
 	u64 suspend_time;
+	u64 suspend_ktime;
 };
 
 int  nv04_timer_ctor(struct nvkm_object *, struct nvkm_object *,
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v2 18/22 EXPERIMENTAL] vga_switcheroo: Allow using active client as proxy when reading DDC/AUX

[Lukas Wunner]

The retina MacBook Pro uses an eDP panel and a gmux controller to switch
the panel between its two GPUs. Unfortunately it seems that it cannot
switch the AUX channel separately from the main link.

But we can emulate switching of DDC/AUX in software by using the active
client as a proxy to talk to the panel.

Allow storing pointers to each client's struct i2c_adapter (for DDC) and
struct drm_dp_aux. Allow retrieving the active client's structures but
constrain access to vga_switcheroo clients to prevent non-clients from
using proxying.

Drivers store AUX first, then DDC because they access the DPCD before
the EDID. Retrieving AUX is only allowed if DDC is also stored, thereby
avoiding race condition where AUX is already stored but not DDC and the
inactive client uses AUX then fails on retrieving the EDID via DDC.

Upon storing DDC, generate hotplug event so that already registered
inactive clients reprobe once the active client has registered its
DDC/AUX structures.

Based (loosely) on patches by Matthew Garrett <mjg59@srcf.ucam.org>
who let the active client stash the EDID and the first 8 bytes of the
DPCD (receiver capabilities) in vga_switcheroo where the inactive client
would subsequently pick it up. It turns out that the drivers are unhappy
with just 8 bytes of DPCD, they need access to the full DPCD to set up
their outputs. Switching in software gives us more options (even write
access to the DPCD if need be):
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0016-vga_switcheroo-Allow-stashing-of-panel-data.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/vga/vga_switcheroo.c | 62 ++++++++++++++++++++++++++++++++++++++++
 include/linux/vga_switcheroo.h   | 11 +++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 94b0b6f..0c52eb4 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -19,6 +19,7 @@
  */
 
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_dp_helper.h>
 
 #include <linux/module.h>
 #include <linux/seq_file.h>
@@ -27,6 +28,7 @@
 #include <linux/debugfs.h>
 #include <linux/fb.h>
 
+#include <linux/i2c.h>
 #include <linux/pci.h>
 #include <linux/console.h>
 #include <linux/vga_switcheroo.h>
@@ -37,6 +39,8 @@
 struct vga_switcheroo_client {
 	struct pci_dev *pdev;
 	struct fb_info *fb_info;
+	struct i2c_adapter *ddc;
+	struct drm_dp_aux *aux;
 	struct work_struct reprobe_work;
 	int pwr_state;
 	const struct vga_switcheroo_client_ops *ops;
@@ -355,6 +359,64 @@ out:
 }
 EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
 
+void vga_switcheroo_set_ddc(struct pci_dev *pdev, struct i2c_adapter *ddc)
+{
+	struct vga_switcheroo_client *client;
+
+	mutex_lock(&vgasr_mutex);
+	client = find_client_from_pci(&vgasr_priv.clients, pdev);
+	if (client)
+		client->ddc = ddc;
+	mutex_unlock(&vgasr_mutex);
+
+	/* DDC is stored after AUX on eDP, so we have both now */
+	if (client->active)
+		vga_switcheroo_reprobe_inactive_clients();
+}
+EXPORT_SYMBOL(vga_switcheroo_set_ddc);
+
+struct i2c_adapter *vga_switcheroo_get_ddc(struct pci_dev *pdev)
+{
+	struct vga_switcheroo_client *active = NULL;
+
+	mutex_lock(&vgasr_mutex);
+	if (find_client_from_pci(&vgasr_priv.clients, pdev))
+		active = find_active_client(&vgasr_priv.clients);
+	mutex_unlock(&vgasr_mutex);
+	if (!active)
+		return NULL;
+
+	return active->ddc;
+}
+EXPORT_SYMBOL(vga_switcheroo_get_ddc);
+
+void vga_switcheroo_set_aux(struct pci_dev *pdev, struct drm_dp_aux *aux)
+{
+	struct vga_switcheroo_client *client;
+
+	mutex_lock(&vgasr_mutex);
+	client = find_client_from_pci(&vgasr_priv.clients, pdev);
+	if (client)
+		client->aux = aux;
+	mutex_unlock(&vgasr_mutex);
+}
+EXPORT_SYMBOL(vga_switcheroo_set_aux);
+
+struct drm_dp_aux *vga_switcheroo_get_aux(struct pci_dev *pdev)
+{
+	struct vga_switcheroo_client *active = NULL;
+
+	mutex_lock(&vgasr_mutex);
+	if (find_client_from_pci(&vgasr_priv.clients, pdev))
+		active = find_active_client(&vgasr_priv.clients);
+	mutex_unlock(&vgasr_mutex);
+	if (!active || !active->ddc)
+		return NULL;
+
+	return active->aux;
+}
+EXPORT_SYMBOL(vga_switcheroo_get_aux);
+
 static int vga_switcheroo_show(struct seq_file *m, void *v)
 {
 	struct vga_switcheroo_client *client;
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b935d83..1d4c07e 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -10,7 +10,10 @@
 #ifndef _LINUX_VGA_SWITCHEROO_H_
 #define _LINUX_VGA_SWITCHEROO_H_
 
+#include <drm/drm_dp_helper.h>
+
 #include <linux/fb.h>
+#include <linux/i2c.h>
 
 struct pci_dev;
 
@@ -56,6 +59,10 @@ void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 
 int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
 int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
+void vga_switcheroo_set_ddc(struct pci_dev *pdev, struct i2c_adapter *ddc);
+struct i2c_adapter *vga_switcheroo_get_ddc(struct pci_dev *pdev);
+void vga_switcheroo_set_aux(struct pci_dev *pdev, struct drm_dp_aux *aux);
+struct drm_dp_aux *vga_switcheroo_get_aux(struct pci_dev *pdev);
 
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);
@@ -77,6 +84,10 @@ static inline int vga_switcheroo_register_client(struct pci_dev *dev,
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
 static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
 static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
+static inline void vga_switcheroo_set_ddc(struct pci_dev *pdev, struct i2c_adapter *ddc) {}
+static inline struct i2c_adapter *vga_switcheroo_get_ddc(struct pci_dev *pdev) { return NULL; }
+static inline void vga_switcheroo_set_aux(struct pci_dev *pdev, struct drm_dp_aux *aux) {}
+static inline struct drm_dp_aux *vga_switcheroo_get_aux(struct pci_dev *pdev) { return NULL; }
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-- 
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 19/22 EXPERIMENTAL] drm: Amend struct drm_dp_aux with connector attribute

[Lukas Wunner]

On dual GPU laptops which cannot switch the AUX channel separately
from the main link and therefore rely on proxying through the
vga_switcheroo active client, we need to restrict this to eDP so
that only communication to the internal panel is proxied and not
to external DP-connected displays which cannot be switched between
GPUs, at least on the retina MacBook Pro. Therefore, amend struct
drm_dp_aux with a pointer to the connector that the AUX channel
belongs to so that we can check the connector type before using
proxying.

It actually seems more intuitive that the parent of an AUX channel
is a specific connector rather than a device: The device an AUX
channel belongs to can be determined from the connector by following
connector->dev, but the connector an AUX channel belongs to cannot
be unambiguously determined from dev since a device may have multiple
DP connectors. Unfortunately we cannot remove the dev attribute since
the msm driver calls drm_dp_aux_register way before initializing the
connector, and the tegra driver uses a distinct of_device_id for the
AUX channel so that it initializes separately from the connector.
These two drivers set the newly added connector attribute in struct
drm_dp_aux in a delayed fashion, i.e. upon initializing the connector.
They are not known to be used for dual GPU laptops right now but maybe
they will be in the future, so it seems reasonable to set the attribute
in these drivers as well.

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_dp.c             | 1 +
 drivers/gpu/drm/msm/edp/edp_connector.c     | 2 ++
 drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
 drivers/gpu/drm/radeon/atombios_dp.c        | 1 +
 drivers/gpu/drm/tegra/sor.c                 | 1 +
 include/drm/drm_dp_helper.h                 | 5 +++++
 6 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 23aa4ff..2b5a8ae 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1066,6 +1066,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 
 	intel_dp->aux.name = name;
 	intel_dp->aux.dev = dev->dev;
+	intel_dp->aux.connector = &connector->base;
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
 
 	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
index b4d1b46..5ca3df9 100644
--- a/drivers/gpu/drm/msm/edp/edp_connector.c
+++ b/drivers/gpu/drm/msm/edp/edp_connector.c
@@ -140,6 +140,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 
 	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
 
+	edp->ctrl->drm_aux->connector = connector;
+
 	/* We don't support HPD, so only poll status until connected. */
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index ad59eab..1e5224f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1149,6 +1149,7 @@ nouveau_connector_create(struct drm_device *dev, int index)
 	case DRM_MODE_CONNECTOR_DisplayPort:
 	case DRM_MODE_CONNECTOR_eDP:
 		nv_connector->aux.dev = dev->dev;
+		nv_connector->aux.connector = connector;
 		nv_connector->aux.transfer = nouveau_connector_aux_xfer;
 		ret = drm_dp_aux_register(&nv_connector->aux);
 		if (ret) {
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index f81e0d7..495e035 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -232,6 +232,7 @@ void radeon_dp_aux_init(struct radeon_connector *radeon_connector)
 
 	radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
 	radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
+	radeon_connector->ddc_bus->aux.connector = &radeon_connector->base;
 	if (ASIC_IS_DCE5(rdev)) {
 		if (radeon_auxch)
 			radeon_connector->ddc_bus->aux.transfer = radeon_dp_aux_transfer_native;
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 7591d89..372a05f 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1480,6 +1480,7 @@ static int tegra_sor_init(struct host1x_client *client)
 	drm_connector_helper_add(&sor->output.connector,
 				 &tegra_sor_connector_helper_funcs);
 	sor->output.connector.dpms = DRM_MODE_DPMS_OFF;
+	sor->dpaux->aux->connector = &sor->output.connector;
 
 	drm_encoder_init(drm, &sor->output.encoder, &tegra_sor_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 2e86f64..c8e693e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -658,6 +658,7 @@ struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @connector: pointer to connector that is the parent for this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -667,6 +668,9 @@ struct drm_dp_aux_msg {
  * The .name field may be used to specify the name of the I2C adapter. If set to
  * NULL, dev_name() of .dev will be used.
  *
+ * The .connector field should be set to a pointer to the connector that
+ * the AUX channel belongs to.
+ *
  * Drivers provide a hardware-specific implementation of how transactions
  * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg
  * structure describing the transaction is passed into this function. Upon
@@ -694,6 +698,7 @@ struct drm_dp_aux {
 	const char *name;
 	struct i2c_adapter ddc;
 	struct device *dev;
+	struct drm_connector *connector;
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
-- 
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 20/22 EXPERIMENTAL] drm: Use vga_switcheroo active client as proxy when reading DDC/AUX

[Lukas Wunner]

The retina MacBook Pro uses an eDP panel and a gmux controller to switch
the panel between its two GPUs. Unfortunately it seems that it cannot
switch the AUX channel separately from the main link.

But we can emulate switching of DDC/AUX in software by using the active
client as a proxy to talk to the panel.

When reading the DPCD or EDID, store the struct drm_dp_aux and struct
i2c_adapter (for DDC) in vga_switcheroo. Retrieve the active client's
structures and use them in lieu of the client's own structures.

To avoid using proxying with external DP-connected displays, this is
constrained to eDP connectors and further constrained by vga_switcheroo
to its clients.

Based (loosely) on patches by Matthew Garrett <mjg59@srcf.ucam.org>
who used stashing instead of proxying and changed each driver rather
than the generic helper functions:
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0025-i915-Use-vga_switcheroo-to-obtain-and-stash-EDID-DPC.patch
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0026-nouveau-Use-vga_switcheroo-to-obtain-and-stash-EDID-.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/drm_dp_helper.c | 14 ++++++++++++++
 drivers/gpu/drm/drm_edid.c      | 12 ++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..f34622f 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -27,6 +27,7 @@
 #include <linux/errno.h>
 #include <linux/sched.h>
 #include <linux/i2c.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drm_dp_helper.h>
 #include <drm/drmP.h>
 
@@ -239,6 +240,19 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 			 void *buffer, size_t size)
 {
+	struct pci_dev *pdev = to_pci_dev(aux->dev);
+	struct drm_dp_aux *proxy_aux;
+
+	if (aux->connector &&
+	    aux->connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		vga_switcheroo_set_aux(pdev, aux);
+		proxy_aux = vga_switcheroo_get_aux(pdev);
+		if (proxy_aux && proxy_aux != aux) {
+			DRM_DEBUG_KMS("Using vga_switcheroo active client as proxy\n");
+			aux = proxy_aux;
+		}
+	}
+
 	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
 				  size);
 }
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index cdb2fa1..b4b0e01 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1379,6 +1379,18 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 {
 	struct edid *edid;
 	struct pci_dev *pdev = connector->dev->pdev;
+	struct i2c_adapter *proxy_ddc;
+
+	if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS ||
+	    connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		vga_switcheroo_set_ddc(pdev, adapter);
+		proxy_ddc = vga_switcheroo_get_ddc(pdev);
+		if (proxy_ddc && proxy_ddc != adapter) {
+			DRM_DEBUG_KMS("Using vga_switcheroo active client as proxy\n");
+			adapter = proxy_ddc;
+			pdev = to_pci_dev(proxy_ddc->dev.parent);
+		}
+	}
 
 	vga_switcheroo_lock_ddc(pdev);
 
-- 
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 21/22 EXPERIMENTAL] drm/nouveau/i2c: Use vga_switcheroo active client as proxy when reading DDC/AUX

[Lukas Wunner]

The retina MacBook Pro uses an eDP panel and a gmux controller to switch
the panel between its two GPUs. Unfortunately it seems that it cannot
switch the AUX channel separately from the main link.

But we can emulate switching of DDC/AUX in software by using the active
client as a proxy to talk to the panel.

Proxying of the AUX channel is facilitated by way of Thierry Reding's
awesome struct drm_dp_aux abstraction (cf. c197db75ff5c, "drm/dp: Add
AUX channel infrastructure"). However, as regards usage of struct
drm_dp_aux, nouveau is the odd man out: A struct drm_dp_aux is defined
as part of struct nouveau_connector but never used. Instead, the AUX
channel is accessed directly with nv_rdaux() and nv_wraux(), even in
the DRM part of the driver.

To enable proxying in nouveau, inject a pointer to the struct drm_dp_aux
from the DRM part of the driver into the struct nvkm_i2c_port. Modify
nv_rdaux() to try drm_dp_dpcd_read() first. If that fails, fall back to
accessing the AUX channel directly. Enclose in #if IS_ENABLED(CONFIG_DRM
_KMS_HELPER) to keep the NVKM part of the driver portable and free of
DRM symbols.

Obviously this is a bit of a kludge but it seems there's no elegant way
short of factoring all the AUX communication in dport.c / outpdp.c out
and into the DRM part of the driver (plus the AUX initialization in
VBIOS).

When the driver first initializes the output with nvkm_output_dp_init(),
the pointer to the struct drm_dp_aux is not yet injected into the struct
nvkm_i2c_port. Thus, if the panel is not switched to the Nvidia GPU,
the dpcd attribute of struct nvkm_output_dp can't be filled and the link
doesn't get trained. Make up for this by checking the link training
status in nouveau_dp_detect() and calling nvkm_output_dp_detect()
if the link hasn't been trained yet.

Modify link training itself so that it does not fail when writing to
DPCD if the value to be written is identical with what's already
configured in DPCD. (Proxying is currently read only for safety
reasons.)

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/nouveau/include/nvkm/subdev/i2c.h |  1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c       |  4 ++--
 drivers/gpu/drm/nouveau/nouveau_dp.c              | 20 +++++++++++++++++++
 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 +++++++++++++++++++++++
 7 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h
index a2e3373..9fa95fb 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h
@@ -37,6 +37,7 @@ struct nvkm_i2c_port {
 	struct list_head head;
 	u8  index;
 	int aux;
+	void *drm_dp_aux;
 
 	const struct nvkm_i2c_func *func;
 };
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 1e5224f..159df7f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -144,8 +144,8 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 		nv_encoder = nouveau_encoder(encoder);
 
 		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
-			int ret = nouveau_dp_detect(nv_encoder);
-			if (ret == 0)
+			nv_encoder->i2c->drm_dp_aux = &nv_connector->aux;
+			if (nouveau_dp_detect(nv_encoder) == 0)
 				break;
 		} else
 		if (nv_encoder->i2c) {
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index c3ef30b..317d6b1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -30,6 +30,25 @@
 #include "nouveau_encoder.h"
 #include "nouveau_crtc.h"
 
+#include <engine/disp.h>
+#include <engine/disp/outpdp.h>
+
+static void
+nouveau_dp_check_link_training(struct nouveau_encoder *nv_encoder)
+{
+	struct nvkm_disp *disp = nvkm_disp(nv_encoder->i2c);
+	struct nvkm_output *outp;
+	struct nvkm_output_dp *outpdp;
+
+	list_for_each_entry(outp, &disp->outp, head)
+		if (outp->info.index == nv_encoder->dcb->index)
+			break;
+
+	outpdp = (struct nvkm_output_dp *)outp;
+	if (!atomic_read(&outpdp->lt.done))
+		nvkm_output_dp_detect(outpdp);
+}
+
 static void
 nouveau_dp_probe_oui(struct drm_device *dev, struct nvkm_i2c_port *auxch,
 		     u8 *dpcd)
@@ -85,5 +104,6 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
 		     nv_encoder->dp.link_nr, nv_encoder->dp.link_bw);
 
 	nouveau_dp_probe_oui(dev, auxch, dpcd);
+	nouveau_dp_check_link_training(nv_encoder);
 	return 0;
 }
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c
index 6834766..5257e4c 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c
@@ -61,7 +61,7 @@ dp_set_link_config(struct dp_state *dp)
 		.execute = 1,
 	};
 	u32 lnkcmp;
-	u8 sink[2];
+	u8 sink[2], sink_rd[2];
 	int ret;
 
 	DBG("%d lanes at %d KB/s\n", dp->link_nr, dp->link_bw);
@@ -98,6 +98,10 @@ dp_set_link_config(struct dp_state *dp)
 	if (outp->dpcd[DPCD_RC02] & DPCD_RC02_ENHANCED_FRAME_CAP)
 		sink[1] |= DPCD_LC01_ENHANCED_FRAME_EN;
 
+	if (nv_rdaux(outp->base.edid, DPCD_LC00_LINK_BW_SET, sink_rd, 2) == 0 &&
+	    memcmp(sink, sink_rd, 2) == 0)
+		return 0;
+
 	return nv_wraux(outp->base.edid, DPCD_LC00_LINK_BW_SET, sink, 2);
 }
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c
index 0bde0fa..b95373b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c
@@ -122,7 +122,7 @@ nvkm_output_dp_enable(struct nvkm_output_dp *outp, bool present)
 	}
 }
 
-static void
+void
 nvkm_output_dp_detect(struct nvkm_output_dp *outp)
 {
 	struct nvkm_i2c_port *port = outp->base.edid;
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h
index 70c77ae..0bd4dcb 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h
@@ -58,4 +58,5 @@ struct nvkm_output_dp_impl {
 };
 
 int nvkm_output_dp_train(struct nvkm_output *, u32 rate, bool wait);
+void nvkm_output_dp_detect(struct nvkm_output_dp *);
 #endif
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
index 1c18860..16ec3cc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
@@ -23,10 +23,34 @@
  */
 #include "priv.h"
 
+#if IS_ENABLED(CONFIG_DRM_KMS_HELPER)
+#include <drm/drm_dp_helper.h>
+#endif
+
+static int
+drm_rdaux(struct nvkm_i2c_port *port, u32 addr, u8 *data, u8 size)
+{
+#if IS_ENABLED(CONFIG_DRM_KMS_HELPER)
+	if (port->drm_dp_aux) {
+		nv_debug(port, "Try reading DPCD with KMS helper: addr=0x%x size=%d\n",
+			 addr, size);
+		return !(drm_dp_dpcd_read(port->drm_dp_aux, addr, data, size)
+			 == size);
+	}
+#endif
+	return -ENODEV;
+}
+
 int
 nv_rdaux(struct nvkm_i2c_port *port, u32 addr, u8 *data, u8 size)
 {
 	struct nvkm_i2c *i2c = nvkm_i2c(port);
+
+	if (drm_rdaux(port, addr, data, size) == 0)
+		return 0;
+
+	nv_debug(port, "Try reading DPCD directly:        addr=0x%x size=%d\n",
+		 addr, size);
 	if (port->func->aux) {
 		int ret = i2c->acquire(port, 0);
 		if (ret == 0) {
-- 
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 22/22 EXPERIMENTAL] drm/nouveau: Use vga_switcheroo active client as proxy when probing DDC on LVDS

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. While the gmux is able to
switch the DDC lines, we can also emulate DDC switching in software
by using the active client as a proxy to talk to the panel. This
gives us two ways to switch, one requiring the apple-gmux driver
and the other requiring the active client's driver.

To that end, when probing DDC, if we're unable to talk to the panel
ourselves and the connector in question is of type LVDS, try proxying
via the vga_switcheroo active client.

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/nouveau/nouveau_connector.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 159df7f..d1a6982 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -149,12 +149,18 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 				break;
 		} else
 		if (nv_encoder->i2c) {
+			struct i2c_adapter *proxy_ddc;
 			int ret;
 			vga_switcheroo_lock_ddc(dev->pdev);
 			ret = nv_probe_i2c(nv_encoder->i2c, 0x50);
 			vga_switcheroo_unlock_ddc(dev->pdev);
 			if (ret)
 				break;
+			proxy_ddc = vga_switcheroo_get_ddc(dev->pdev);
+			if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
+			    nv_connector->type == DCB_CONNECTOR_LVDS &&
+			    proxy_ddc && drm_probe_ddc(proxy_ddc))
+				break;
 		}
 	}
 
-- 
1.8.5.2 (Apple Git-48)

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

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: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"

[Daniel Vetter]

On Tue, Apr 21, 2015 at 10:39:45AM +0200, Lukas Wunner wrote:
> This reverts commit 26814ce68904c9faf977c90edac798156311981f.
> 
> The helper function is no longer needed after Dave Airlie's rewrite
> of vga_switcheroo_switch_ddc(), the commit introducing it was only
> included because 31f23c3d488e ("drm/edid: Switch DDC when reading
> the EDID") does not compile without it.
> 
> 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/vga/vga_switcheroo.c | 14 --------------
>  include/linux/vga_switcheroo.h   |  2 --
>  2 files changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index f02e7fc..f0d5475 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -214,20 +214,6 @@ find_active_client(struct list_head *head)
>  	return NULL;
>  }
>  
> -struct pci_dev *vga_switcheroo_get_active_client(void)
> -{
> -	struct vga_switcheroo_client *client;
> -	struct pci_dev *pdev = NULL;
> -
> -	mutex_lock(&vgasr_mutex);
> -	client = find_active_client(&vgasr_priv.clients);
> -	if (client)
> -		pdev = client->pdev;
> -	mutex_unlock(&vgasr_mutex);
> -	return pdev;
> -}
> -EXPORT_SYMBOL(vga_switcheroo_get_active_client);

you just added this earlier in this very series. Please reorder/squash
patches so that this isn't required.
-Daniel

> -
>  int vga_switcheroo_get_client_state(struct pci_dev *pdev)
>  {
>  	struct vga_switcheroo_client *client;
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 39b25b0..62f303f 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -63,7 +63,6 @@ void vga_switcheroo_unregister_handler(void);
>  
>  int vga_switcheroo_process_delayed_switch(void);
>  
> -struct pci_dev *vga_switcheroo_get_active_client(void);
>  int vga_switcheroo_get_client_state(struct pci_dev *dev);
>  
>  void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic);
> @@ -85,7 +84,6 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	int id, bool active) { return 0; }
>  static inline void vga_switcheroo_unregister_handler(void) {}
>  static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
> -static inline struct pci_dev *vga_switcheroo_get_active_client(void) { return NULL; }
>  static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
>  
>  static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"

[Lukas Wunner]

Hi Daniel,

thanks for taking a look at the patch set.

On Wed, Aug 12, 2015 at 04:25:52PM +0200, Daniel Vetter wrote:
> On Tue, Apr 21, 2015 at 10:39:45AM +0200, Lukas Wunner wrote:
> > This reverts commit 26814ce68904c9faf977c90edac798156311981f.
> > 
> > The helper function is no longer needed after Dave Airlie's rewrite
> > of vga_switcheroo_switch_ddc(), the commit introducing it was only
> > included because 31f23c3d488e ("drm/edid: Switch DDC when reading
> > the EDID") does not compile without it.
[...]
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -214,20 +214,6 @@ find_active_client(struct list_head *head)
> >  	return NULL;
> >  }
> >  
> > -struct pci_dev *vga_switcheroo_get_active_client(void)
> > -{
> > -	struct vga_switcheroo_client *client;
> > -	struct pci_dev *pdev = NULL;
> > -
> > -	mutex_lock(&vgasr_mutex);
> > -	client = find_active_client(&vgasr_priv.clients);
> > -	if (client)
> > -		pdev = client->pdev;
> > -	mutex_unlock(&vgasr_mutex);
> > -	return pdev;
> > -}
> > -EXPORT_SYMBOL(vga_switcheroo_get_active_client);
> 
> you just added this earlier in this very series. Please reorder/squash
> patches so that this isn't required.

I would have to squash patches 2, 4 (by Seth Forshee), 5 (by Dave Airlie),
6 and 7 (mine). The work of two of these authors would only be acknowledged
in the commit message and the history how the code evolved over the course
of 3 years would not be reflected in the git repo.

Are you sure? (y/n)

I deliberately didn't squash to preserve authorship and history but if
you're forcing me at point blank I'll do it. ;-)

Context: Seth Forshee of Canonical came up with patches in 2012 which Dave
Airlie didn't like. He rewrote them and left them as a WIP in his git repo
where I picked them up. Matthew Garrett posted patches of his own last year
but since they were based on Seth Forshee's code, they didn't get merged
either.

The first MacBooks with dual GPUs were introduced 2008, it's 2015 now and
we're still missing support in the mainline kernel. The issue is not so
much that GPU switching doesn't work (the screen just turns black) but
energy consumption because the discrete GPU is used by default and the
integrated GPU isn't turned off.

So, machines with huge marketshare + shoddy dual GPU support for years
= problem.

We need to fix this, hence the patch set.

Thanks,

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

Re: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"

[Daniel Vetter]

On Wed, Aug 12, 2015 at 07:34:32PM +0200, Lukas Wunner wrote:
> Hi Daniel,
> 
> thanks for taking a look at the patch set.
> 
> On Wed, Aug 12, 2015 at 04:25:52PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 21, 2015 at 10:39:45AM +0200, Lukas Wunner wrote:
> > > This reverts commit 26814ce68904c9faf977c90edac798156311981f.
> > > 
> > > The helper function is no longer needed after Dave Airlie's rewrite
> > > of vga_switcheroo_switch_ddc(), the commit introducing it was only
> > > included because 31f23c3d488e ("drm/edid: Switch DDC when reading
> > > the EDID") does not compile without it.
> [...]
> > > --- a/drivers/gpu/vga/vga_switcheroo.c
> > > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > > @@ -214,20 +214,6 @@ find_active_client(struct list_head *head)
> > >  	return NULL;
> > >  }
> > >  
> > > -struct pci_dev *vga_switcheroo_get_active_client(void)
> > > -{
> > > -	struct vga_switcheroo_client *client;
> > > -	struct pci_dev *pdev = NULL;
> > > -
> > > -	mutex_lock(&vgasr_mutex);
> > > -	client = find_active_client(&vgasr_priv.clients);
> > > -	if (client)
> > > -		pdev = client->pdev;
> > > -	mutex_unlock(&vgasr_mutex);
> > > -	return pdev;
> > > -}
> > > -EXPORT_SYMBOL(vga_switcheroo_get_active_client);
> > 
> > you just added this earlier in this very series. Please reorder/squash
> > patches so that this isn't required.
> 
> I would have to squash patches 2, 4 (by Seth Forshee), 5 (by Dave Airlie),
> 6 and 7 (mine). The work of two of these authors would only be acknowledged
> in the commit message and the history how the code evolved over the course
> of 3 years would not be reflected in the git repo.
> 
> Are you sure? (y/n)

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 ;-)
 
> I deliberately didn't squash to preserve authorship and history but if
> you're forcing me at point blank I'll do it. ;-)
> 
> Context: Seth Forshee of Canonical came up with patches in 2012 which Dave
> Airlie didn't like. He rewrote them and left them as a WIP in his git repo
> where I picked them up. Matthew Garrett posted patches of his own last year
> but since they were based on Seth Forshee's code, they didn't get merged
> either.
> 
> The first MacBooks with dual GPUs were introduced 2008, it's 2015 now and
> we're still missing support in the mainline kernel. The issue is not so
> much that GPU switching doesn't work (the screen just turns black) but
> energy consumption because the discrete GPU is used by default and the
> integrated GPU isn't turned off.
> 
> So, machines with huge marketshare + shoddy dual GPU support for years
> = problem.
> 
> We need to fix this, hence the patch set.

Apparently not a lot of people bothered yet to polish this, so it can't be
that bad. I guess everyone just buys the basic model with intel only.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 06/22] vga_switcheroo: Lock/unlock DDC lines harder

[Daniel Vetter]

On Fri, Mar 27, 2015 at 12:29:40PM +0100, Lukas Wunner wrote:
> Unlock DDC lines if drm_probe_ddc() fails.
> 
> If the inactive client registers before the active client then
> old_ddc_owner cannot be determined with find_active_client()
> (null pointer dereference). Therefore change semantics of the
> ->switch_ddc handler callback to return old_ddc_owner on success
> or a negative int on failure.
> 
> Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to
> avoid locking inversion when one client locks vgasr_mutex on entering
> vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another
> client is holding ddc_lock and tries to acquire vgasr_mutex on entering
> vga_switcheroo_unlock_ddc().
> 
> Lock ddc_lock in stage2 to avoid race condition where reading the
> EDID and switching happens simultaneously.
> 
> Switch DDC lines on MIGD / MDIS commands and on runtime suspend.

So essentially you have

bool locked = mutex_trylock();

/* nothing that looks at locked at all */

if (locked)
	mutex_unlock;

This doesn't protect anything at all and makes it look _very_ fishy.

I haven't dug deeper yet.
-Daniel

> 
> 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/drm_edid.c        |   9 ++--
>  drivers/gpu/vga/vga_switcheroo.c  | 110 ++++++++++++++++++++++----------------
>  drivers/platform/x86/apple-gmux.c |  15 +++++-
>  include/linux/vga_switcheroo.h    |   3 +-
>  4 files changed, 87 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 505eed1..cdb2fa1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> +	struct pci_dev *pdev = connector->dev->pdev;
>  
> -	vga_switcheroo_lock_ddc(connector->dev->pdev);
> +	vga_switcheroo_lock_ddc(pdev);
>  
> -	if (!drm_probe_ddc(adapter))
> +	if (!drm_probe_ddc(adapter)) {
> +		vga_switcheroo_unlock_ddc(pdev);
>  		return NULL;
> +	}
>  
>  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  	if (edid)
>  		drm_get_displayid(connector, edid);
>  
> -	vga_switcheroo_unlock_ddc(connector->dev->pdev);
> +	vga_switcheroo_unlock_ddc(pdev);
>  
>  	return edid;
>  }
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 0223020..f02e7fc 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -9,12 +9,13 @@
>  
>   Switcher interface - methods require for ATPX and DCM
>   - switchto - this throws the output MUX switch
> - - discrete_set_power - sets the power state for the discrete card
> + - switch_ddc - switch only DDC lines, return old DDC owner (or < 0 on failure)
> + - power_state - sets the power state for either GPU
>  
>   GPU driver interface
>   - set_gpu_state - this should do the equiv of s/r for the card
>  		  - this should *not* set the discrete power state
> - - switch_check  - check if the device is in a position to switch now
> + - can_switch - check if the device is in a position to switch now
>   */
>  
>  #include <linux/module.h>
> @@ -59,7 +60,7 @@ struct vgasr_priv {
>  	struct vga_switcheroo_handler *handler;
>  
>  	struct mutex ddc_lock;
> -	struct pci_dev *old_ddc_owner;
> +	enum vga_switcheroo_client_id old_ddc_owner;
>  };
>  
>  #define ID_BIT_AUDIO		0x100
> @@ -276,33 +277,24 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
>  
>  int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
>  {
> -	struct vga_switcheroo_client *client;
> -	int ret = 0;
> -	int id;
> +	int ret, id;
>  
>  	mutex_lock(&vgasr_mutex);
>  
> -	if (!vgasr_priv.handler) {
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
>  		ret = -ENODEV;
>  		goto out;
>  	}
>  
> -	if (vgasr_priv.handler->switch_ddc) {
> -		mutex_lock(&vgasr_priv.ddc_lock);
> +	mutex_lock(&vgasr_priv.ddc_lock);
> +	id = vgasr_priv.handler->get_client_id(pdev);
> +	ret = vgasr_priv.handler->switch_ddc(id);
>  
> -		client = find_active_client(&vgasr_priv.clients);
> -		if (!client) {
> -			mutex_unlock(&vgasr_priv.ddc_lock);
> -			ret = -ENODEV;
> -			goto out;
> -		}
> -		vgasr_priv.old_ddc_owner = client->pdev;
> -		if (client->pdev == pdev)
> -			goto out;
> -
> -		id = vgasr_priv.handler->get_client_id(pdev);
> -		ret = vgasr_priv.handler->switch_ddc(id);
> -	}
> +	if (ret < 0) {
> +		pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +	} else
> +		vgasr_priv.old_ddc_owner = ret;
>  
>  out:
>  	mutex_unlock(&vgasr_mutex);
> @@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
>  
>  int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
>  {
> -	int ret = 0;
> -	int id;
> -	mutex_lock(&vgasr_mutex);
> +	int ret, id;
> +	bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex);
>  
> -	if (!vgasr_priv.handler) {
> -		ret = -ENODEV;
> +	if (!mutex_is_locked(&vgasr_priv.ddc_lock)) {
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (vgasr_priv.handler->switch_ddc) {
> -		if (vgasr_priv.old_ddc_owner != pdev) {
> -			id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner);
> -			ret = vgasr_priv.handler->switch_ddc(id);
> -		}
> -		vgasr_priv.old_ddc_owner = NULL;
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
> +		pr_err("vga_switcheroo: handler disappeared\n");
>  		mutex_unlock(&vgasr_priv.ddc_lock);
> +		ret = -ENODEV;
> +		goto out;
>  	}
> +
> +	id = vgasr_priv.handler->get_client_id(pdev);
> +	if (vgasr_priv.old_ddc_owner != id) {
> +		ret = vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner);
> +		if (ret < 0)
> +			pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +	} else
> +		ret = vgasr_priv.old_ddc_owner;
> +	mutex_unlock(&vgasr_priv.ddc_lock);
> +
>  out:
> -	mutex_unlock(&vgasr_mutex);
> +	if (vgasr_mutex_acquired)
> +		mutex_unlock(&vgasr_mutex);
>  	return ret;
>  }
>  EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
> @@ -433,14 +433,26 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
>  	}
>  
>  	if (vgasr_priv.handler->switch_ddc) {
> +		mutex_lock(&vgasr_priv.ddc_lock);
>  		ret = vgasr_priv.handler->switch_ddc(new_client->id);
> -		if (ret)
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +		if (ret < 0) {
> +			pr_err("vga_switcheroo: failed to switch DDC lines\n");
>  			return ret;
> +		}
>  	}
>  
>  	ret = vgasr_priv.handler->switchto(new_client->id);
> -	if (ret)
> -		goto restore_ddc;
> +	if (ret) {
> +		pr_err("vga_switcheroo: failed to switch to client %d\n",
> +		       new_client->id);
> +		active->active = true;
> +		/* restore DDC lines */
> +		mutex_lock(&vgasr_priv.ddc_lock);
> +		vgasr_priv.handler->switch_ddc(active->id);
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +		return ret;
> +	}
>  
>  	if (new_client->ops->reprobe)
>  		new_client->ops->reprobe(new_client->pdev);
> @@ -452,14 +464,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
>  
>  	new_client->active = true;
>  	return 0;
> -
> -restore_ddc:
> -	if (vgasr_priv.handler->switch_ddc) {
> -		int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
> -				VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
> -		ret = vgasr_priv.handler->switch_ddc(id);
> -	}
> -	return ret;
>  }
>  
>  static bool check_can_switch(void)
> @@ -561,6 +565,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
>  	vgasr_priv.delayed_switch_active = false;
>  
>  	if (just_mux) {
> +		if (vgasr_priv.handler->switch_ddc) {
> +			mutex_lock(&vgasr_priv.ddc_lock);
> +			ret = vgasr_priv.handler->switch_ddc(client_id);
> +			mutex_unlock(&vgasr_priv.ddc_lock);
> +			if (ret < 0) {
> +				pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +				goto out;
> +			}
> +		}
>  		ret = vgasr_priv.handler->switchto(client_id);
>  		goto out;
>  	}
> @@ -716,6 +729,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
>  	ret = dev->bus->pm->runtime_suspend(dev);
>  	if (ret)
>  		return ret;
> +	if (vgasr_priv.handler->switch_ddc) {
> +		mutex_lock(&vgasr_priv.ddc_lock);
> +		ret = vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD);
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +		if (ret < 0)
> +			pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +	}
>  	if (vgasr_priv.handler->switchto)
>  		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
>  	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index f0a55a4..08bdf1e 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = {
>  
>  static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
>  {
> +	enum vga_switcheroo_client_id old_ddc_owner;
> +
> +	if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
> +		old_ddc_owner = VGA_SWITCHEROO_IGD;
> +	else
> +		old_ddc_owner = VGA_SWITCHEROO_DIS;
> +
> +	pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
> +
> +	if (id == old_ddc_owner)
> +		return old_ddc_owner;
> +
>  	if (id == VGA_SWITCHEROO_IGD)
>  		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
>  	else
>  		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
> -	return 0;
> +
> +	return old_ddc_owner;
>  }
>  
>  static int gmux_switchto(enum vga_switcheroo_client_id id)
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 352bef3..39b25b0 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
>  static inline int vga_switcheroo_register_client(struct pci_dev *dev,
>  		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
>  static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
> -static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; }
> +static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
> +static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
>  static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
>  static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	const struct vga_switcheroo_client_ops *ops,
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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 Dave,

as requested I've pushed the MacBook Pro GPU switching stuff to GitHub
to ease browsing/reviewing, latest branch is gmux-v5:
https://github.com/l1k/linux/commits/gmux-v5

(I had applied for a freedesktop.org account in April with bug 89906
but it's still pending, hence GitHub.)

I've overhauled locking once more because previously all EDID reads
were serialized even on machines which don't use vga_switcheroo at all.
Seems it's impossible to fix this with mutexes and still be race-free
and deadlock-free, so I've changed vgasr_mutex to an rwlock:
https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8

There are a number of vga_switcheroo functions added by Takashi Iwai
and yourself which don't lock anything at all, I believe this was in
part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks
vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume
which locks vgasr_mutex once again). After changing vgasr_mutex to an
rwlock we can safely use locking in those functions as well:
https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62

With this change, on machines which don't use vga_switcheroo, the
overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock
and EDID reads can happen in parallel. Thierry Reding and Alex Deucher
are in favor of adding a separate drm_get_edid_switcheroo() and changing
the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu
of drm_get_edid() so that drivers which don't use vga_switcheroo avoid
the lock_ddc/unlock_ddc calls. Performance-wise these additional calls
should be negligible, so I think the motivation can only be better
readability/clarity. One should also keep in mind that drivers which
don't use vga_switcheroo currently might do so in the future, e.g.
if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.

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

[Daniel Vetter]

On Tue, Aug 25, 2015 at 09:36:47AM +0200, Lukas Wunner wrote:
> Hi Dave,
> 
> as requested I've pushed the MacBook Pro GPU switching stuff to GitHub
> to ease browsing/reviewing, latest branch is gmux-v5:
> https://github.com/l1k/linux/commits/gmux-v5
> 
> (I had applied for a freedesktop.org account in April with bug 89906
> but it's still pending, hence GitHub.)
> 
> I've overhauled locking once more because previously all EDID reads
> were serialized even on machines which don't use vga_switcheroo at all.
> Seems it's impossible to fix this with mutexes and still be race-free
> and deadlock-free, so I've changed vgasr_mutex to an rwlock:
> https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8
> 
> There are a number of vga_switcheroo functions added by Takashi Iwai
> and yourself which don't lock anything at all, I believe this was in
> part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks
> vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume
> which locks vgasr_mutex once again). After changing vgasr_mutex to an
> rwlock we can safely use locking in those functions as well:
> https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62
> 
> With this change, on machines which don't use vga_switcheroo, the
> overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock
> and EDID reads can happen in parallel. Thierry Reding and Alex Deucher
> are in favor of adding a separate drm_get_edid_switcheroo() and changing
> the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu
> of drm_get_edid() so that drivers which don't use vga_switcheroo avoid
> the lock_ddc/unlock_ddc calls. Performance-wise these additional calls
> should be negligible, so I think the motivation can only be better
> readability/clarity. One should also keep in mind that drivers which
> don't use vga_switcheroo currently might do so in the future, e.g.
> if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.

Just a quick aside: Switching to rwlocks to make lockdep happy is a
fallacy - lockdep unfortunately doesn't accurately track read lock
depencies. Which means very likely you didn't fix the locking inversions
but just made lockdep shut up about them. Example:

thread A grabs read-lock 1 and mutex 2.

thread B grabs mutex 2 and write-lock 1.

lockdep won't complain here, but if thread A has tries to get mutex 2
while thread B tries to write-lock 1 then there's an obvious deadlock.

I'd highly suggest you try fixing the vga-switcheroo locking without
resorting to rw lock primitives - that just means we need to manually
prove the locking for many cases which is tons of work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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,

On Tue, Aug 25, 2015 at 10:21:20AM +0200, Daniel Vetter wrote:
> On Tue, Aug 25, 2015 at 09:36:47AM +0200, Lukas Wunner wrote:
> > I've overhauled locking once more because previously all EDID reads
> > were serialized even on machines which don't use vga_switcheroo at all.
> > Seems it's impossible to fix this with mutexes and still be race-free
> > and deadlock-free, so I've changed vgasr_mutex to an rwlock:
> > https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8
> > 
> > There are a number of vga_switcheroo functions added by Takashi Iwai
> > and yourself which don't lock anything at all, I believe this was in
> > part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks
> > vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume
> > which locks vgasr_mutex once again). After changing vgasr_mutex to an
> > rwlock we can safely use locking in those functions as well:
> > https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62
> > 
> > With this change, on machines which don't use vga_switcheroo, the
> > overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock
> > and EDID reads can happen in parallel. Thierry Reding and Alex Deucher
> > are in favor of adding a separate drm_get_edid_switcheroo() and changing
> > the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu
> > of drm_get_edid() so that drivers which don't use vga_switcheroo avoid
> > the lock_ddc/unlock_ddc calls. Performance-wise these additional calls
> > should be negligible, so I think the motivation can only be better
> > readability/clarity. One should also keep in mind that drivers which
> > don't use vga_switcheroo currently might do so in the future, e.g.
> > if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.
> 
> Just a quick aside: Switching to rwlocks to make lockdep happy is a
> fallacy - lockdep unfortunately doesn't accurately track read lock
> depencies. Which means very likely you didn't fix the locking inversions
> but just made lockdep shut up about them. [...]
> I'd highly suggest you try fixing the vga-switcheroo locking without
> resorting to rw lock primitives - that just means we need to manually
> prove the locking for many cases which is tons of work.

mutex is not the right tool for the job:

To make DDC switching bullet proof you need to block the handler from
unregistering between lock_ddc and unlock_ddc. Solution: ddc_lock grabs
a mutex, ddc_unlock releases it, unregister_handler grabs that same lock.

Downside: All EDID reads are serialized, you can't probe EDID in parallel
if you have multiple displays. Which is ugly.

One could be tempted to "solve" this for non-muxed machines by immediately
releasing the lock in lock_ddc if there's no handler, and by checking in
unlock_ddc if mutex_is_locked before releasing it. But on muxed machines
this would be racy: GPU driver A calls lock_ddc, acquires the mutex, sees
there's no handler registered, releases the mutex. Between this and the
call to unlock_ddc, a handler registers and GPU driver B calls lock_ddc.
This time the mutex is actually locked and when driver A calls unlock_ddc,
it will release it, even though the other driver had acquired it.

Of course one could come up with all sorts of ugly hacks like remembering
for which client the mutex was acquired, but this wouldn't be atomic and
100% bullet proof, unlike rwlocks.

So it seems there's no way with mutexes to achieve parallel EDID reads
and still be race-free and deadlock-free.

rwlocks have the right semantics for this use case: Registering and
unregistering requires write access to the lock, thus happens exclusively.
In lock_ddc we acquire read access to the rwlock and in unlock_ddc we
release it. So the handler can't disappear while we've switched DDC.
We use a mutex ddc_lock to lock the DDC lines but we only acquire that
lock if there's actually a handler. So on non-muxed machines, EDID reads
*can* happen in parallel, there's just the (negligible) overhead of
1 read_lock + 1 read_unlock:
https://github.com/l1k/linux/commit/d0b6f659ae8f4b8b94f4b3ded9fc80e93950d0b3

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