The following patches are part of a larger series I've been working on to get vga_switcheroo working on hybrid graphics Macbooks. Some of these machines are not providing a VBT, and since the LVDS panel is connected to the discrete GPU at boot we can't get a mode for the panel during initialization. As a result the LVDS connector is not registered with DRM, and graphics switching is not possible.
These patches fix this issue by registering the connector even if we can't get a mode for the panel. If we don't have an EDID we check again from the vga_switcheroo reprobe callback.
This is working, except for the framebuffer console which isn't displaying when switched to the integrated GPU, which I still need to debug. I'm not entirely sure whether or not this is the correct approach though, so I wanted to go ahead and get some feedback on the patches now to make sure I'm on the right track.
Thanks, Seth
Andreas Heider (1): drm/i915: Add support for vga_switcheroo reprobe
Seth Forshee (4): drm/i915: separate out code to get EDID from LVDS panel drm/i915: register LVDS connector even if we can't get a panel mode drm/i915: make intel_lvds_get_edid() more robust drm/i915: check LVDS for EDID on GPU switches
drivers/gpu/drm/i915/i915_dma.c | 9 ++- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_lvds.c | 156 +++++++++++++++++++++---------------- 3 files changed, 97 insertions(+), 69 deletions(-)
From: Andreas Heider andreas@meetr.de
Signed-off-by: Andreas Heider andreas@meetr.de --- drivers/gpu/drm/i915/i915_dma.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 9cf7dfe..5b5176d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1263,6 +1263,12 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_ } }
+static void i915_switcheroo_reprobe(struct pci_dev *pdev) +{ + struct drm_device *dev = pci_get_drvdata(pdev); + intel_fb_output_poll_changed(dev); +} + static bool i915_switcheroo_can_switch(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); @@ -1276,7 +1282,7 @@ 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, + .reprobe = i915_switcheroo_reprobe, .can_switch = i915_switcheroo_can_switch, };
This code will be reused to support hybrid graphics on some Apple machines that can't get a mode for the LVDS panel at boot, so move it into a new function named intel_lvds_get_edid().
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/i915/intel_lvds.c | 95 +++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index e05c0d3..5069137 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -46,6 +46,7 @@ struct intel_lvds {
struct edid *edid;
+ u8 i2c_pin; int fitting_mode; u32 pfit_control; u32 pfit_pgm_ratios; @@ -897,6 +898,54 @@ static bool intel_lvds_supported(struct drm_device *dev) return IS_MOBILE(dev) && !IS_I830(dev); }
+static bool intel_lvds_get_edid(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_connector *connector = dev_priv->int_lvds_connector; + struct intel_lvds *intel_lvds = intel_attached_lvds(connector); + struct drm_display_mode *scan; /* *modes, *bios_mode; */ + + /* + * Attempt to get the fixed panel mode from DDC. Assume that the + * preferred mode is the right one. + */ + intel_lvds->edid = drm_get_edid(connector, + intel_gmbus_get_adapter(dev_priv, + intel_lvds->i2c_pin)); + if (intel_lvds->edid) { + if (drm_add_edid_modes(connector, + intel_lvds->edid)) { + drm_mode_connector_update_edid_property(connector, + intel_lvds->edid); + } else { + kfree(intel_lvds->edid); + intel_lvds->edid = NULL; + } + } + if (!intel_lvds->edid) { + /* Didn't get an EDID, so + * Set wide sync ranges so we get all modes + * handed to valid_mode for checking + */ + connector->display_info.min_vfreq = 0; + connector->display_info.max_vfreq = 200; + connector->display_info.min_hfreq = 0; + connector->display_info.max_hfreq = 200; + } + + list_for_each_entry(scan, &connector->probed_modes, head) { + if (scan->type & DRM_MODE_TYPE_PREFERRED) { + intel_lvds->fixed_mode = + drm_mode_duplicate(dev, scan); + intel_find_lvds_downclock(dev, + intel_lvds->fixed_mode, + connector); + return true; + } + } + return false; +} + /** * intel_lvds_init - setup LVDS connectors on this device * @dev: drm device @@ -912,7 +961,6 @@ bool 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_crtc *crtc; u32 lvds; int pipe; @@ -955,9 +1003,11 @@ bool intel_lvds_init(struct drm_device *dev) intel_lvds->pfit_control = I915_READ(PFIT_CONTROL); }
+ intel_lvds->i2c_pin = pin; intel_encoder = &intel_lvds->base; encoder = &intel_encoder->base; connector = &intel_connector->base; + dev_priv->int_lvds_connector = connector; drm_connector_init(dev, &intel_connector->base, &intel_lvds_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
@@ -991,6 +1041,7 @@ bool intel_lvds_init(struct drm_device *dev) dev->mode_config.scaling_mode_property, DRM_MODE_SCALE_ASPECT); intel_lvds->fitting_mode = DRM_MODE_SCALE_ASPECT; + /* * LVDS discovery: * 1) check for EDID on DDC @@ -1001,44 +1052,8 @@ bool intel_lvds_init(struct drm_device *dev) * if closed, act like it's not there for now */
- /* - * Attempt to get the fixed panel mode from DDC. Assume that the - * preferred mode is the right one. - */ - intel_lvds->edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - pin)); - if (intel_lvds->edid) { - if (drm_add_edid_modes(connector, - intel_lvds->edid)) { - drm_mode_connector_update_edid_property(connector, - intel_lvds->edid); - } else { - kfree(intel_lvds->edid); - intel_lvds->edid = NULL; - } - } - if (!intel_lvds->edid) { - /* Didn't get an EDID, so - * Set wide sync ranges so we get all modes - * handed to valid_mode for checking - */ - connector->display_info.min_vfreq = 0; - connector->display_info.max_vfreq = 200; - connector->display_info.min_hfreq = 0; - connector->display_info.max_hfreq = 200; - } - - list_for_each_entry(scan, &connector->probed_modes, head) { - if (scan->type & DRM_MODE_TYPE_PREFERRED) { - intel_lvds->fixed_mode = - drm_mode_duplicate(dev, scan); - intel_find_lvds_downclock(dev, - intel_lvds->fixed_mode, - connector); - goto out; - } - } + if (intel_lvds_get_edid(dev)) + goto out;
/* Failed to get EDID, what about VBT? */ if (dev_priv->lfp_lvds_vbt_mode) { @@ -1096,7 +1111,6 @@ out: dev_priv->lid_notifier.notifier_call = NULL; } /* keep the LVDS connector */ - dev_priv->int_lvds_connector = connector; drm_sysfs_connector_add(connector);
intel_panel_setup_backlight(dev); @@ -1105,6 +1119,7 @@ out:
failed: DRM_DEBUG_KMS("No LVDS modes found, disabling.\n"); + dev_priv->int_lvds_connector = NULL; drm_connector_cleanup(connector); drm_encoder_cleanup(encoder); kfree(intel_lvds);
Some Apple hybrid graphics machines do not have the LVDS panel connected to the integrated GPU at boot and also do not supply a VBT. The LVDS connector is not registered as a result, making it impossible to support graphics switching.
This patch changes intel_lvds_init() to register the connector even if we can't find any panel modes. This makes it necessary to always check intel_lvds->fixed_mode before use, as it could now be NULL.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/i915/intel_lvds.c | 48 +++++++++++++++---------------------- 1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 5069137..c1ab632 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -161,6 +161,8 @@ static int intel_lvds_mode_valid(struct drm_connector *connector, struct intel_lvds *intel_lvds = intel_attached_lvds(connector); struct drm_display_mode *fixed_mode = intel_lvds->fixed_mode;
+ if (!fixed_mode) + return MODE_PANEL; if (mode->hdisplay > fixed_mode->hdisplay) return MODE_PANEL; if (mode->vdisplay > fixed_mode->vdisplay) @@ -262,7 +264,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder, * with the panel scaling set up to source from the H/VDisplay * of the original mode. */ - intel_fixed_panel_mode(intel_lvds->fixed_mode, adjusted_mode); + if (intel_lvds->fixed_mode) + intel_fixed_panel_mode(intel_lvds->fixed_mode, adjusted_mode);
if (HAS_PCH_SPLIT(dev)) { intel_pch_panel_fitting(dev, intel_lvds->fitting_mode, @@ -461,12 +464,13 @@ static int intel_lvds_get_modes(struct drm_connector *connector) { struct intel_lvds *intel_lvds = intel_attached_lvds(connector); struct drm_device *dev = connector->dev; - struct drm_display_mode *mode; + struct drm_display_mode *mode = NULL;
if (intel_lvds->edid) return drm_add_edid_modes(connector, intel_lvds->edid);
- mode = drm_mode_duplicate(dev, intel_lvds->fixed_mode); + if (intel_lvds->fixed_mode) + mode = drm_mode_duplicate(dev, intel_lvds->fixed_mode); if (mode == NULL) return 0;
@@ -1073,26 +1077,21 @@ bool intel_lvds_init(struct drm_device *dev) */
/* Ironlake: FIXME if still fail, not try pipe mode now */ - if (HAS_PCH_SPLIT(dev)) - goto failed; - - lvds = I915_READ(LVDS); - pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0; - crtc = intel_get_crtc_for_pipe(dev, pipe); - - if (crtc && (lvds & LVDS_PORT_EN)) { - intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc); - if (intel_lvds->fixed_mode) { - intel_lvds->fixed_mode->type |= - DRM_MODE_TYPE_PREFERRED; - goto out; + if (!HAS_PCH_SPLIT(dev)) { + lvds = I915_READ(LVDS); + pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0; + crtc = intel_get_crtc_for_pipe(dev, pipe); + + if (crtc && (lvds & LVDS_PORT_EN)) { + intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc); + if (intel_lvds->fixed_mode) { + intel_lvds->fixed_mode->type |= + DRM_MODE_TYPE_PREFERRED; + goto out; + } } }
- /* If we still don't have a mode after all that, give up. */ - if (!intel_lvds->fixed_mode) - goto failed; - out: /* * Unlock registers and just @@ -1116,13 +1115,4 @@ out: intel_panel_setup_backlight(dev);
return true; - -failed: - DRM_DEBUG_KMS("No LVDS modes found, disabling.\n"); - dev_priv->int_lvds_connector = NULL; - drm_connector_cleanup(connector); - drm_encoder_cleanup(encoder); - kfree(intel_lvds); - kfree(intel_connector); - return false; }
On Fri, Aug 03, 2012 at 11:02:19AM -0500, Seth Forshee wrote:
Some Apple hybrid graphics machines do not have the LVDS panel connected to the integrated GPU at boot and also do not supply a VBT. The LVDS connector is not registered as a result, making it impossible to support graphics switching.
This patch changes intel_lvds_init() to register the connector even if we can't find any panel modes. This makes it necessary to always check intel_lvds->fixed_mode before use, as it could now be NULL.
This one kind of sucks. I think adding a quirk for this situation would be justifiable, rather than doing it for all devices.
On Fri, Aug 03, 2012 at 05:14:16PM +0100, Matthew Garrett wrote:
On Fri, Aug 03, 2012 at 11:02:19AM -0500, Seth Forshee wrote:
Some Apple hybrid graphics machines do not have the LVDS panel connected to the integrated GPU at boot and also do not supply a VBT. The LVDS connector is not registered as a result, making it impossible to support graphics switching.
This patch changes intel_lvds_init() to register the connector even if we can't find any panel modes. This makes it necessary to always check intel_lvds->fixed_mode before use, as it could now be NULL.
This one kind of sucks. I think adding a quirk for this situation would be justifiable, rather than doing it for all devices.
This is one of the things I wasn't so sure about. There are various checks in intel_lvds_init() that can cause it to bail out before we try to get the EDID, and I don't fully understand all of them. If non-laptop machines are expected to bail out before the EDID check then the approach I've taken seems reasonable. Otherwise adding a quirk probably is a good idea.
Seth
On Fri, Aug 03, 2012 at 11:24:51AM -0500, Seth Forshee wrote:
This is one of the things I wasn't so sure about. There are various checks in intel_lvds_init() that can cause it to bail out before we try to get the EDID, and I don't fully understand all of them. If non-laptop machines are expected to bail out before the EDID check then the approach I've taken seems reasonable. Otherwise adding a quirk probably is a good idea.
I know we've previously had problems with machines with phantom LVDS hardware, but I'm not sure what the current state of affairs is.
On Fri, Aug 03, 2012 at 05:27:02PM +0100, Matthew Garrett wrote:
On Fri, Aug 03, 2012 at 11:24:51AM -0500, Seth Forshee wrote:
This is one of the things I wasn't so sure about. There are various checks in intel_lvds_init() that can cause it to bail out before we try to get the EDID, and I don't fully understand all of them. If non-laptop machines are expected to bail out before the EDID check then the approach I've taken seems reasonable. Otherwise adding a quirk probably is a good idea.
I know we've previously had problems with machines with phantom LVDS hardware, but I'm not sure what the current state of affairs is.
It turns out that the framebuffer console issue is due to not having a mode when initializing LVDS. As a result 1024x768 is getting used for the framebuffer.
So quirking is going to fix this situation. In fact, with the changes below switcheroo seems to work perfectly, without any of these patches at all.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 627fe35..d83e5bc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -503,6 +503,7 @@ typedef struct drm_i915_private { enum intel_pch pch_type;
unsigned long quirks; + struct drm_display_mode *lvds_quirk_mode;
/* Register state */ bool modeset_on_lid; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f615976..c810177 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7069,7 +7069,7 @@ static void intel_init_display(struct drm_device *dev) * resume, or other times. This quirk makes sure that's the case for * affected systems. */ -static void quirk_pipea_force(struct drm_device *dev) +static void quirk_pipea_force(struct drm_device *dev, void *data) { struct drm_i915_private *dev_priv = dev->dev_private;
@@ -7080,7 +7080,7 @@ static void quirk_pipea_force(struct drm_device *dev) /* * Some machines (Lenovo U160) do not work with SSC on LVDS for some reason */ -static void quirk_ssc_force_disable(struct drm_device *dev) +static void quirk_ssc_force_disable(struct drm_device *dev, void *data) { struct drm_i915_private *dev_priv = dev->dev_private; dev_priv->quirks |= QUIRK_LVDS_SSC_DISABLE; @@ -7091,48 +7091,74 @@ static void quirk_ssc_force_disable(struct drm_device *dev) * A machine (e.g. Acer Aspire 5734Z) may need to invert the panel backlight * brightness value */ -static void quirk_invert_brightness(struct drm_device *dev) +static void quirk_invert_brightness(struct drm_device *dev, void *data) { struct drm_i915_private *dev_priv = dev->dev_private; dev_priv->quirks |= QUIRK_INVERT_BRIGHTNESS; DRM_INFO("applying inverted panel brightness quirk\n"); }
+/* + * Some machines (e.g. certain Macbooks) may not be able to determine the + * LVDS mode during driver initialization + */ +static void quirk_lvds_panel_mode(struct drm_device *dev, void *data) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + dev_priv->lvds_quirk_mode = data; + DRM_INFO("applying LVDS panel mode quirk: %p\n", data); +} + +/* LVDS panel mode for Macbook Pro 8,2 */ +struct drm_display_mode mbp82_panel_mode = { + DRM_MODE("1440x900", DRM_MODE_TYPE_DRIVER, 88750, 1440, 1488, 1520, + 1600, 0, 900, 903, 909, 926, 0, + DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC) +}; + struct intel_quirk { int device; int subsystem_vendor; int subsystem_device; - void (*hook)(struct drm_device *dev); + void (*hook)(struct drm_device *dev, void *data); + void *hook_data; };
static struct intel_quirk intel_quirks[] = { /* HP Mini needs pipe A force quirk (LP: #322104) */ - { 0x27ae, 0x103c, 0x361a, quirk_pipea_force }, + { 0x27ae, 0x103c, 0x361a, quirk_pipea_force, NULL },
/* Thinkpad R31 needs pipe A force quirk */ - { 0x3577, 0x1014, 0x0505, quirk_pipea_force }, + { 0x3577, 0x1014, 0x0505, quirk_pipea_force, NULL }, /* Toshiba Protege R-205, S-209 needs pipe A force quirk */ - { 0x2592, 0x1179, 0x0001, quirk_pipea_force }, + { 0x2592, 0x1179, 0x0001, quirk_pipea_force, NULL },
/* ThinkPad X30 needs pipe A force quirk (LP: #304614) */ - { 0x3577, 0x1014, 0x0513, quirk_pipea_force }, + { 0x3577, 0x1014, 0x0513, quirk_pipea_force, NULL }, /* ThinkPad X40 needs pipe A force quirk */
/* ThinkPad T60 needs pipe A force quirk (bug #16494) */ - { 0x2782, 0x17aa, 0x201a, quirk_pipea_force }, + { 0x2782, 0x17aa, 0x201a, quirk_pipea_force, NULL },
/* 855 & before need to leave pipe A & dpll A up */ - { 0x3582, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force }, - { 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force }, + { 0x3582, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force, NULL }, + { 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force, NULL },
/* Lenovo U160 cannot use SSC on LVDS */ - { 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable }, + { 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable, NULL },
/* Sony Vaio Y cannot use SSC on LVDS */ - { 0x0046, 0x104d, 0x9076, quirk_ssc_force_disable }, + { 0x0046, 0x104d, 0x9076, quirk_ssc_force_disable, NULL },
/* Acer Aspire 5734Z must invert backlight brightness */ - { 0x2a42, 0x1025, 0x0459, quirk_invert_brightness }, + { 0x2a42, 0x1025, 0x0459, quirk_invert_brightness, NULL }, + + /* + * Some versions of the Macbook Pro 8,2 cannot use SSC and + * cannot get the panel mode on LVDS + */ + { 0x0116, 0x106b, 0x00dc, quirk_ssc_force_disable, NULL }, + { 0x0116, 0x106b, 0x00dc, quirk_lvds_panel_mode, &mbp82_panel_mode }, };
static void intel_init_quirks(struct drm_device *dev) @@ -7148,7 +7174,7 @@ static void intel_init_quirks(struct drm_device *dev) q->subsystem_vendor == PCI_ANY_ID) && (d->subsystem_device == q->subsystem_device || q->subsystem_device == PCI_ANY_ID)) - q->hook(dev); + q->hook(dev, q->hook_data); } }
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index e05c0d3..303068d 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -996,6 +996,7 @@ bool intel_lvds_init(struct drm_device *dev) * 1) check for EDID on DDC * 2) check for VBT data * 3) check to see if LVDS is already on + * 4) check for LVDS panel mode quirk * if none of the above, no panel * 4) make sure lid is open * if closed, act like it's not there for now @@ -1058,15 +1059,25 @@ bool intel_lvds_init(struct drm_device *dev) */
/* Ironlake: FIXME if still fail, not try pipe mode now */ - if (HAS_PCH_SPLIT(dev)) - goto failed; - - lvds = I915_READ(LVDS); - pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0; - crtc = intel_get_crtc_for_pipe(dev, pipe); + if (!HAS_PCH_SPLIT(dev)) { + lvds = I915_READ(LVDS); + pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0; + crtc = intel_get_crtc_for_pipe(dev, pipe); + + if (crtc && (lvds & LVDS_PORT_EN)) { + intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc); + if (intel_lvds->fixed_mode) { + intel_lvds->fixed_mode->type |= + DRM_MODE_TYPE_PREFERRED; + goto out; + } + } + }
- if (crtc && (lvds & LVDS_PORT_EN)) { - intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc); + /* Check for panel mode quirk as a last resort */ + if (dev_priv->lvds_quirk_mode) { + intel_lvds->fixed_mode = + drm_mode_duplicate(dev, dev_priv->lvds_quirk_mode); if (intel_lvds->fixed_mode) { intel_lvds->fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
On Sat, Aug 04, 2012 at 11:57:27AM -0500, Seth Forshee wrote:
On Fri, Aug 03, 2012 at 05:27:02PM +0100, Matthew Garrett wrote:
On Fri, Aug 03, 2012 at 11:24:51AM -0500, Seth Forshee wrote:
This is one of the things I wasn't so sure about. There are various checks in intel_lvds_init() that can cause it to bail out before we try to get the EDID, and I don't fully understand all of them. If non-laptop machines are expected to bail out before the EDID check then the approach I've taken seems reasonable. Otherwise adding a quirk probably is a good idea.
I know we've previously had problems with machines with phantom LVDS hardware, but I'm not sure what the current state of affairs is.
It turns out that the framebuffer console issue is due to not having a mode when initializing LVDS. As a result 1024x768 is getting used for the framebuffer.
So quirking is going to fix this situation. In fact, with the changes below switcheroo seems to work perfectly, without any of these patches at all.
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ... -Daniel
On Sun, Aug 05, 2012 at 11:14:12PM +0200, Daniel Vetter wrote:
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ...
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
On Sun, Aug 05, 2012 at 10:18:38PM +0100, Matthew Garrett wrote:
On Sun, Aug 05, 2012 at 11:14:12PM +0200, Daniel Vetter wrote:
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ...
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
The "correct" approach of switching the mux before we fetch the edid is actualy the one I fear will result in fragile code: Only run on few machines, and as you say with tons of funky interactions with the init sequence ordering. And I guess people will bitch&moan about the flickering this will cause ;-)
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil. -Daniel
On Mon, Aug 6, 2012 at 7:40 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Aug 05, 2012 at 10:18:38PM +0100, Matthew Garrett wrote:
On Sun, Aug 05, 2012 at 11:14:12PM +0200, Daniel Vetter wrote:
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ...
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
The "correct" approach of switching the mux before we fetch the edid is actualy the one I fear will result in fragile code: Only run on few machines, and as you say with tons of funky interactions with the init sequence ordering. And I guess people will bitch&moan about the flickering this will cause ;-)
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Well in theory you can switch the ddc lines without switching the other lines, so we could do a mutex protected mux switch around edid retrival,
Of course someone would have to code it up first then we could see how ugly it would be.
Dave.
-Daniel
Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Sun, Aug 5, 2012 at 5:44 PM, Dave Airlie airlied@gmail.com wrote:
On Mon, Aug 6, 2012 at 7:40 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Aug 05, 2012 at 10:18:38PM +0100, Matthew Garrett wrote:
On Sun, Aug 05, 2012 at 11:14:12PM +0200, Daniel Vetter wrote:
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ...
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
The "correct" approach of switching the mux before we fetch the edid is actualy the one I fear will result in fragile code: Only run on few machines, and as you say with tons of funky interactions with the init sequence ordering. And I guess people will bitch&moan about the flickering this will cause ;-)
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Well in theory you can switch the ddc lines without switching the other lines, so we could do a mutex protected mux switch around edid retrival,
Depends on the system. On non-Macs, some systems have a single mux, others have a separate mux for i2c and display as specified in the ATPX ACPI methods. Not sure how the Macs do it. I've started cleaning up the PX radeon code along with a bunch of other radeon ralated ACPI fixes: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
Alex
On Sun, Aug 05, 2012 at 07:20:31PM -0400, Alex Deucher wrote:
On Sun, Aug 5, 2012 at 5:44 PM, Dave Airlie airlied@gmail.com wrote:
On Mon, Aug 6, 2012 at 7:40 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Aug 05, 2012 at 10:18:38PM +0100, Matthew Garrett wrote:
On Sun, Aug 05, 2012 at 11:14:12PM +0200, Daniel Vetter wrote:
I like this approach more - the only other solution I see is to ask the currently active driver (i.e. radeon) at bootime for the right mode. Which sounds much more hellish and fragile ...
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
The "correct" approach of switching the mux before we fetch the edid is actualy the one I fear will result in fragile code: Only run on few machines, and as you say with tons of funky interactions with the init sequence ordering. And I guess people will bitch&moan about the flickering this will cause ;-)
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Well in theory you can switch the ddc lines without switching the other lines, so we could do a mutex protected mux switch around edid retrival,
Depends on the system. On non-Macs, some systems have a single mux, others have a separate mux for i2c and display as specified in the ATPX ACPI methods. Not sure how the Macs do it. I've started cleaning up the PX radeon code along with a bunch of other radeon ralated ACPI fixes: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
The Macs mux the i2c and display separately. However they don't support the vendor ACPI interfaces for the mux. The driver that provides the vga_switcheroo handler is separate from the graphics drivers, and the same whether the discrete graphics are Radeon or nVidia.
Really to support this in any generic sort of way vga_switcheroo needs to support muxing the DDC separately from the display, but as Matthew pointed out the ordering of initialization could be a problem. Even if we protect the DDC with a mutex how can we guarantee that the switcheroo handler is registered to switch the DDC before i915 is ready to check for an EDID?
On Fri, Aug 10, 2012 at 05:19:48PM -0500, Seth Forshee wrote:
First, I don't have a solution for the ordering of initialization. It just happens to work out for me right now.
Okay, I've got a proof-of-concept implementation of delaying secondary GPU initialization until the i2c can be muxed over to that card. It's not exactly pretty, but it is working. I'd really like to get some feedback on the concept and implementation before spending more time on it. Patches follow.
One problem I'm aware of is if the switcheroo handler is in the driver for the secondary GPU. I think this was the case for a machine I used to have with Optimus graphics. In that scenario the secondary graphics device is never initialized because the switcheroo handler is registered during initialization of the secondary device. The driver load method would need to be split up to cope with this.
Thanks, Seth
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.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- 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 e25cf31..ea6bcc2 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -252,6 +252,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; @@ -342,9 +365,15 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) fb_notifier_call_chain(FB_EVENT_REMAP_ALL_CONSOLE, &event); }
+ 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); @@ -356,6 +385,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 ddb419c..b0d0839 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); @@ -53,6 +54,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);
@@ -66,6 +69,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) { 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,
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.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- 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 ea6bcc2..e53f67d 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -205,6 +205,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 b0d0839..e361858 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -61,6 +61,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);
#else @@ -76,6 +77,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; }
DRM needs to be notified of client and handler registration in order to defer initialization of the secondary GPU until the EDID can be read from the LVDS panel. To support this add a notifier call chain to vga_switcheroo for subscribing to switcheroo events. Events are generated for registration and unregistration of handlers and clients.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/vga/vga_switcheroo.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 14 ++++++++++++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index e53f67d..d5cd274 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -24,6 +24,7 @@ #include <linux/fs.h> #include <linux/debugfs.h> #include <linux/fb.h> +#include <linux/notifier.h>
#include <linux/pci.h> #include <linux/vga_switcheroo.h> @@ -70,6 +71,28 @@ static struct vgasr_priv vgasr_priv = { .clients = LIST_HEAD_INIT(vgasr_priv.clients), };
+static BLOCKING_NOTIFIER_HEAD(vga_switcheroo_notifier_list); + +int vga_switcheroo_register_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&vga_switcheroo_notifier_list, + nb); +} +EXPORT_SYMBOL(vga_switcheroo_register_notifier); + +int vga_switcheroo_unregister_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&vga_switcheroo_notifier_list, + nb); +} +EXPORT_SYMBOL(vga_switcheroo_unregister_notifier); + +static int vga_switcheroo_notifier_call_chain(enum vga_switcheroo_event event) +{ + return blocking_notifier_call_chain(&vga_switcheroo_notifier_list, + event, NULL); +} + static bool vga_switcheroo_ready(void) { /* we're ready if we get two clients + handler */ @@ -113,10 +136,18 @@ int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) vga_switcheroo_enable(); } mutex_unlock(&vgasr_mutex); + + vga_switcheroo_notifier_call_chain(VGA_SWITCHEROO_HANDLER_REGISTERED); return 0; } EXPORT_SYMBOL(vga_switcheroo_register_handler);
+bool vga_switcheroo_handler_registered(void) +{ + return !!vgasr_priv.handler; +} +EXPORT_SYMBOL(vga_switcheroo_handler_registered); + void vga_switcheroo_unregister_handler(void) { mutex_lock(&vgasr_mutex); @@ -127,6 +158,7 @@ void vga_switcheroo_unregister_handler(void) vgasr_priv.active = false; } mutex_unlock(&vgasr_mutex); + vga_switcheroo_notifier_call_chain(VGA_SWITCHEROO_HANDLER_UNREGISTERED); } EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
@@ -156,6 +188,7 @@ static int register_client(struct pci_dev *pdev, vga_switcheroo_enable(); } mutex_unlock(&vgasr_mutex); + vga_switcheroo_notifier_call_chain(VGA_SWITCHEROO_CLIENT_REGISTERED); return 0; }
@@ -250,6 +283,7 @@ void vga_switcheroo_unregister_client(struct pci_dev *pdev) vgasr_priv.active = false; } mutex_unlock(&vgasr_mutex); + vga_switcheroo_notifier_call_chain(VGA_SWITCHEROO_CLIENT_UNREGISTERED); } EXPORT_SYMBOL(vga_switcheroo_unregister_client);
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index e361858..c3d7c6f 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -11,6 +11,7 @@ #define _LINUX_VGA_SWITCHEROO_H_
#include <linux/fb.h> +#include <linux/notifier.h>
struct pci_dev;
@@ -28,6 +29,13 @@ enum vga_switcheroo_client_id { VGA_SWITCHEROO_MAX_CLIENTS, };
+enum vga_switcheroo_event { + VGA_SWITCHEROO_CLIENT_REGISTERED, + VGA_SWITCHEROO_CLIENT_UNREGISTERED, + VGA_SWITCHEROO_HANDLER_REGISTERED, + VGA_SWITCHEROO_HANDLER_UNREGISTERED, +}; + struct vga_switcheroo_handler { int (*switch_ddc)(enum vga_switcheroo_client_id id); int (*switchto)(enum vga_switcheroo_client_id id); @@ -44,6 +52,9 @@ struct vga_switcheroo_client_ops { };
#if defined(CONFIG_VGA_SWITCHEROO) +int vga_switcheroo_register_notifier(struct notifier_block *nb); +int vga_switcheroo_unregister_notifier(struct notifier_block *nb); +bool vga_switcheroo_handler_registered(void); void vga_switcheroo_unregister_client(struct pci_dev *dev); int vga_switcheroo_register_client(struct pci_dev *dev, const struct vga_switcheroo_client_ops *ops); @@ -66,6 +77,9 @@ int vga_switcheroo_get_client_state(struct pci_dev *dev);
#else
+static inline int vga_switcheroo_register_notifier(struct notifier_block *nb) { return 0; } +static inline int vga_switcheroo_unregister_notifier(struct notifier_block *nb) { return 0; } +static inline bool vga_switcheroo_handler_registered(void) { return false; } 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) { return 0; }
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.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- 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 c72e81e..f702e90 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -269,14 +269,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); } @@ -343,6 +350,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,
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.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..1a4b661 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -31,6 +31,7 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/vga_switcheroo.h> #include "drmP.h" #include "drm_edid.h" #include "drm_edid_modes.h" @@ -82,6 +83,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; @@ -395,12 +398,26 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { struct edid *edid = NULL; + 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)) edid = (struct edid *)drm_do_get_edid(connector, adapter);
+ if (active_pdev && active_pdev != pdev) + vga_switcheroo_switch_ddc(active_pdev); + connector->display_info.raw_edid = (char *)edid;
+ mutex_unlock(&drm_edid_mutex); return edid;
}
When deferred initialization support for pci devices is added some additional cleanup will be needed. Add a pci-specific put function to serve this purpose, and convert the pci drivers over to using it. For now it just calls drm_put_dev(), so this commit has no functional change.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/cirrus/cirrus_drv.c | 2 +- drivers/gpu/drm/drm_pci.c | 6 ++++++ drivers/gpu/drm/gma500/psb_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- drivers/gpu/drm/nouveau/nouveau_drv.c | 2 +- drivers/gpu/drm/radeon/radeon_drv.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- include/drm/drmP.h | 1 + 10 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index d0c4574..001298d 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -72,7 +72,7 @@ ast_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c index 7053140..c7ca02b 100644 --- a/drivers/gpu/drm/cirrus/cirrus_drv.c +++ b/drivers/gpu/drm/cirrus/cirrus_drv.c @@ -64,7 +64,7 @@ static void cirrus_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
static const struct file_operations cirrus_driver_fops = { diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 5320364..4896c96 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -388,6 +388,12 @@ err_g1: } EXPORT_SYMBOL(drm_get_pci_dev);
+void drm_put_pci_dev(struct drm_device *dev) +{ + drm_put_dev(dev); +} +EXPORT_SYMBOL(drm_put_pci_dev); + /** * PCI device initialization. Called direct from modules at load time. * diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index 0c47374..d7c3c9c 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -585,7 +585,7 @@ static void psb_driver_preclose(struct drm_device *dev, struct drm_file *priv) static void psb_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); - drm_put_dev(dev); + drm_put_pci_dev(dev); }
static const struct dev_pm_ops psb_pm_ops = { diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a24ffbe..86ae5a2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -856,7 +856,7 @@ i915_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
static int i915_pm_suspend(struct device *dev) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index ea1024d..a3b0a4a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -73,7 +73,7 @@ static void mga_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
static const struct file_operations mgag200_driver_fops = { diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.c b/drivers/gpu/drm/nouveau/nouveau_drv.c index 9a36f5f..b74b02a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.c +++ b/drivers/gpu/drm/nouveau/nouveau_drv.c @@ -168,7 +168,7 @@ nouveau_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
int diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index d7269f4..298697a 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -308,7 +308,7 @@ radeon_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
static int diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 4d9edea..cf901cc 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -982,7 +982,7 @@ static void vmw_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev); + drm_put_pci_dev(dev); }
static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d6b67bb..eb99e96 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1748,6 +1748,7 @@ extern void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver); extern int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, struct drm_driver *driver); +extern void drm_put_pci_dev(struct drm_device *dev);
#define DRM_PCIE_SPEED_25 1 #define DRM_PCIE_SPEED_50 2
Deferring initiailzation of the secondary GPU until switcheroo is ready will allow successfully reading the EDID in systems which support muxing the DDC seperately from the display.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/drm_drv.c | 3 + drivers/gpu/drm/drm_pci.c | 141 +++++++++++++++++++++++++++++++++++++++------ include/drm/drmP.h | 2 + 3 files changed, 129 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 9238de4..124fd8a 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -276,6 +276,8 @@ static int __init drm_core_init(void) goto err_p3; }
+ drm_pci_module_init(); + DRM_INFO("Initialized %s %d.%d.%d %s\n", CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); return 0; @@ -291,6 +293,7 @@ err_p1:
static void __exit drm_core_exit(void) { + drm_pci_module_exit(); remove_proc_entry("dri", NULL); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy(); diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 4896c96..9da0cd2 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -40,6 +40,9 @@ #include <linux/slab.h> #include <linux/dma-mapping.h> #include <linux/export.h> +#include <linux/notifier.h> +#include <linux/vgaarb.h> +#include <linux/vga_switcheroo.h> #include "drmP.h"
/**********************************************************************/ @@ -297,19 +300,8 @@ static struct drm_bus drm_pci_bus = { .agp_init = drm_pci_agp_init, };
-/** - * Register. - * - * \param pdev - PCI device structure - * \param ent entry from the PCI ID table with device type flags - * \return zero on success or a negative number on failure. - * - * Attempt to gets inter module "drm" information. If we are first - * then register the character device and inter module information. - * Try and register, if we fail to register, backout previous work. - */ -int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, - struct drm_driver *driver) +int __drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, + struct drm_driver *driver) { struct drm_device *dev; int ret; @@ -334,8 +326,6 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, dev->hose = pdev->sysdata; #endif
- mutex_lock(&drm_global_mutex); - if ((ret = drm_fill_in_dev(dev, ent, driver))) { printk(KERN_ERR "DRM: Fill_in_dev failed.\n"); goto err_g2; @@ -371,7 +361,6 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, pci_name(pdev), dev->primary->index);
- mutex_unlock(&drm_global_mutex); return 0;
err_g4: @@ -386,10 +375,116 @@ err_g1: mutex_unlock(&drm_global_mutex); return ret; } + +struct deferred_init_data { + struct list_head list; + struct pci_dev *pdev; + const struct pci_device_id *ent; + struct drm_driver *driver; +}; + +static LIST_HEAD(deferred_init_list); + +static void drm_deferred_init_work_fn(struct work_struct *work) +{ + struct deferred_init_data *di_data, *temp; + + mutex_lock(&drm_global_mutex); + + if (!vga_switcheroo_handler_registered() || + !vga_switcheroo_get_active_client()) { + mutex_unlock(&drm_global_mutex); + return; + } + + list_for_each_entry_safe(di_data, temp, &deferred_init_list, list) { + if (__drm_get_pci_dev(di_data->pdev, di_data->ent, + di_data->driver)) + DRM_ERROR("pci device initialization failed\n"); + list_del(&di_data->list); + kfree(di_data); + } + mutex_unlock(&drm_global_mutex); +} +static DECLARE_WORK(deferred_init_work, drm_deferred_init_work_fn); + +static int drm_switcheroo_notifier_fn(struct notifier_block *nb, + unsigned long val, void *unused) +{ + if (val == VGA_SWITCHEROO_CLIENT_REGISTERED || + val == VGA_SWITCHEROO_HANDLER_REGISTERED) + queue_work(system_nrt_wq, &deferred_init_work); + return NOTIFY_OK; +} +static struct notifier_block drm_switcheroo_notifier = { + .notifier_call = drm_switcheroo_notifier_fn, +}; + +/** + * Register. + * + * \param pdev - PCI device structure + * \param ent entry from the PCI ID table with device type flags + * \return zero on success or a negative number on failure. + * + * Attempt to gets inter module "drm" information. If we are first + * then register the character device and inter module information. + * Try and register, if we fail to register, backout previous work. + */ +int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, + struct drm_driver *driver) +{ + int ret = 0; + + mutex_lock(&drm_global_mutex); + + /* + * For secondary graphics devices shouldn't be initialized + * until the handler and primary graphics device have been + * registered with vga_switcheroo. + * + * FIXME: Is vga_default_device() reliable enough for this + * purpose? + * + * FIXME: If vga_switcheroo is disabled secondary devices + * never gets initialized. Is this okay? Maybe it is, since + * we can't switch to the secondary GPU anyway. + */ + if (vga_default_device() == pdev || + (vga_switcheroo_handler_registered() && + vga_switcheroo_get_active_client())) { + ret = __drm_get_pci_dev(pdev, ent, driver); + } else { + struct deferred_init_data *di_data = + kmalloc(sizeof(*di_data), GFP_KERNEL); + if (!di_data) { + ret = -ENOMEM; + } else { + di_data->pdev = pdev; + di_data->ent = ent; + di_data->driver = driver; + list_add_tail(&di_data->list, &deferred_init_list); + } + } + + return ret; +} EXPORT_SYMBOL(drm_get_pci_dev);
void drm_put_pci_dev(struct drm_device *dev) { + struct deferred_init_data *di_data; + + mutex_lock(&drm_global_mutex); + list_for_each_entry(di_data, &deferred_init_list, list) { + if (di_data->pdev == dev->pdev) { + list_del(&di_data->list); + kfree(di_data); + break; + } + } + mutex_unlock(&drm_global_mutex); + drm_put_dev(dev); } EXPORT_SYMBOL(drm_put_pci_dev); @@ -466,7 +561,7 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) pci_unregister_driver(pdriver); } else { list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) - drm_put_dev(dev); + drm_put_pci_dev(dev); } DRM_INFO("Module unloaded\n"); } @@ -520,3 +615,15 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask) return 0; } EXPORT_SYMBOL(drm_pcie_get_speed_cap_mask); + +int drm_pci_module_init(void) +{ + return vga_switcheroo_register_notifier(&drm_switcheroo_notifier); +} +EXPORT_SYMBOL(drm_pci_module_init); + +void drm_pci_module_exit(void) +{ + vga_switcheroo_unregister_notifier(&drm_switcheroo_notifier); +} +EXPORT_SYMBOL(drm_pci_module_exit); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index eb99e96..0e9401f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1749,6 +1749,8 @@ extern int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, struct drm_driver *driver); extern void drm_put_pci_dev(struct drm_device *dev); +extern int drm_pci_module_init(void); +extern void drm_pci_module_exit(void);
#define DRM_PCIE_SPEED_25 1 #define DRM_PCIE_SPEED_50 2
On Mon, Aug 20, 2012 at 10:31:04AM -0500, Seth Forshee wrote:
- /*
* For secondary graphics devices shouldn't be initialized* until the handler and primary graphics device have been* registered with vga_switcheroo.** FIXME: Is vga_default_device() reliable enough for this* purpose?** FIXME: If vga_switcheroo is disabled secondary devices* never gets initialized. Is this okay? Maybe it is, since* we can't switch to the secondary GPU anyway.*/
Won't this break the multiple cards with independent outputs case?
On Mon, Aug 20, 2012 at 04:36:40PM +0100, Matthew Garrett wrote:
On Mon, Aug 20, 2012 at 10:31:04AM -0500, Seth Forshee wrote:
- /*
* For secondary graphics devices shouldn't be initialized* until the handler and primary graphics device have been* registered with vga_switcheroo.** FIXME: Is vga_default_device() reliable enough for this* purpose?** FIXME: If vga_switcheroo is disabled secondary devices* never gets initialized. Is this okay? Maybe it is, since* we can't switch to the secondary GPU anyway.*/Won't this break the multiple cards with independent outputs case?
Yes, if they don't have a switcheroo handler. I only have experience with one such machine, which had optimus graphics. My recollection is that it did have a switcheroo handler, which was only capable of controlling power to the discrete card.
On Mon, Aug 20, 2012 at 10:56:33AM -0500, Seth Forshee wrote:
On Mon, Aug 20, 2012 at 04:36:40PM +0100, Matthew Garrett wrote:
Won't this break the multiple cards with independent outputs case?
Yes, if they don't have a switcheroo handler. I only have experience with one such machine, which had optimus graphics. My recollection is that it did have a switcheroo handler, which was only capable of controlling power to the discrete card.
So if I have a desktop machine and install two graphics cards?
On Mon, Aug 20, 2012 at 04:57:41PM +0100, Matthew Garrett wrote:
On Mon, Aug 20, 2012 at 10:56:33AM -0500, Seth Forshee wrote:
On Mon, Aug 20, 2012 at 04:36:40PM +0100, Matthew Garrett wrote:
Won't this break the multiple cards with independent outputs case?
Yes, if they don't have a switcheroo handler. I only have experience with one such machine, which had optimus graphics. My recollection is that it did have a switcheroo handler, which was only capable of controlling power to the discrete card.
So if I have a desktop machine and install two graphics cards?
Yeah, that would likely be broken.
I'm not sure how we support both of these cases without doing something more like what I originally proposed, i.e. registering the LVDS connector even if it doesn't look like a panel is attached. I still honestly favor that approach, although it does come with its own set of challenges.
The only other option I can come up with is to reprobe LVDS after switcheroo and add the connector at that time. I haven't investigated this option in detail, but at first glance it looks like there are at least some places where DRM isn't prepared to cope with adding connectors after initialization.
On Mon, Aug 20, 2012 at 11:24:44AM -0500, Seth Forshee wrote:
I'm not sure how we support both of these cases without doing something more like what I originally proposed, i.e. registering the LVDS connector even if it doesn't look like a panel is attached. I still honestly favor that approach, although it does come with its own set of challenges.
The only other option I can come up with is to reprobe LVDS after switcheroo and add the connector at that time. I haven't investigated this option in detail, but at first glance it looks like there are at least some places where DRM isn't prepared to cope with adding connectors after initialization.
Well, one option is to identify whether the hardware is switcheroo capable without the use of the switcheroo driver. It looks like we can do that in the majority of cases - Apple is the only special case I can see, and that one's a fairly easy workaround.
On Mon, Aug 06, 2012 at 07:44:16AM +1000, Dave Airlie wrote:
The "correct" approach is clearly to just have the drm core change the i2c mux before requesting edid, but that's made difficult because of the absence of ordering guarantees in initialisation. I don't like quirking this, since we're then back to the situation of potentially having to add every new piece of related hardware to the quirk list.
The "correct" approach of switching the mux before we fetch the edid is actualy the one I fear will result in fragile code: Only run on few machines, and as you say with tons of funky interactions with the init sequence ordering. And I guess people will bitch&moan about the flickering this will cause ;-)
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Well in theory you can switch the ddc lines without switching the other lines, so we could do a mutex protected mux switch around edid retrival,
Of course someone would have to code it up first then we could see how ugly it would be.
I coded it up, and it's not really too bad. I've put a dump of my local changes below. But there are a couple of problems.
First, I don't have a solution for the ordering of initialization. It just happens to work out for me right now.
Even so, the code only works if I delay loading i915. apple-gmux is definitely initializing first so the i2c mux should be getting switched, but the transactions are failing.
[ 19.445658] [drm:gmbus_xfer], GMBUS [i915 gmbus panel] timed out after NAK [ 19.445672] [drm:gmbus_xfer], GMBUS [i915 gmbus panel] NAK for addr: 0050 r(1)
But if I prevent i915 from being auto-loaded and load later on it works fine. I haven't been able to figure out what's going on. Any ideas?
Thanks, Seth
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..3f18e8a 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -31,6 +31,7 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/vga_switcheroo.h> #include "drmP.h" #include "drm_edid.h" #include "drm_edid_modes.h" @@ -82,6 +83,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; @@ -395,12 +398,25 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { struct edid *edid = NULL; + 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(); + vga_switcheroo_switch_ddc(pdev); + }
if (drm_probe_ddc(adapter)) edid = (struct edid *)drm_do_get_edid(connector, adapter);
+ if (active_pdev) + vga_switcheroo_switch_ddc(active_pdev); + connector->display_info.raw_edid = (char *)edid;
+ mutex_unlock(&drm_edid_mutex); return edid;
} diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index e25cf31..e53f67d 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -205,6 +205,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; @@ -252,6 +266,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; @@ -342,9 +379,15 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) fb_notifier_call_chain(FB_EVENT_REMAP_ALL_CONSOLE, &event); }
+ 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); @@ -356,6 +399,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/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 8ea2640..38f49fb 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -122,14 +122,21 @@ static const struct backlight_ops gmux_bl_ops = { .update_status = gmux_update_status, };
+static int gmux_switchddc(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); } @@ -196,6 +203,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data) }
static struct vga_switcheroo_handler gmux_handler = { + .switch_ddc = gmux_switchddc, .switchto = gmux_switchto, .power_state = gmux_set_power_state, .get_client_id = gmux_get_client_id, diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index ddb419c..e361858 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); @@ -53,11 +54,14 @@ 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);
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);
#else @@ -66,12 +70,14 @@ 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) { 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, 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; }
On Sun, Aug 05, 2012 at 11:40:16PM +0200, Daniel Vetter wrote:
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Doing this via quirks means that we'll always be broken on the hardware at the point where it ships. Implementing the functionality means we stand some chance of working out of the box.
On Mon, Aug 06, 2012 at 01:23:17PM +0100, Matthew Garrett wrote:
On Sun, Aug 05, 2012 at 11:40:16PM +0200, Daniel Vetter wrote:
As long as it's only apple shipping multi-gpu machines with broken/non-existing vbt, I'll happily stomach the quirk list entries. They're bad, but imo the lesser evil.
Doing this via quirks means that we'll always be broken on the hardware at the point where it ships. Implementing the functionality means we stand some chance of working out of the box.
I've been thinking some today about how this functionality might be implemented.
It looks like the simplest way to let the inactive GPU have the i2c bus temporarily is to have drm_get_edid() control the mux and serialize it with a mutex. It should be pretty easy to make vga_switcheroo support muxing the DDC separately from the display.
There is the problem of vga_switcheroo not really being operational until it has two clients and a handler, and the graphics drivers not registering clients until they've initialized LVDS. This probably won't be too dificult to solve.
The bigger problem is still making sure the switcheroo handler is registered, when it's needed, before trying to read the EDID. We could potentially delay initializing non-active graphics devices until after a switcheroo handler has been registered, but that's a problem if the handler is implemented by the driver for the secondary GPU (is this ever the case?). Otherwise we seem to be stuck with making i915 able to cope with getting modes for LVDS after initialization, which kind of puts us back where we started.
Any other ideas?
intel_lvds_get_edid() needs to be called when switching GPUs, but it's currently making assumptions that it will only be called once and that there's always an LVDS connector present when it's called. Fix these assumptions.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/i915/intel_lvds.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index c1ab632..9d05a90 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -906,9 +906,18 @@ static bool intel_lvds_get_edid(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_connector *connector = dev_priv->int_lvds_connector; - struct intel_lvds *intel_lvds = intel_attached_lvds(connector); + struct intel_lvds *intel_lvds; struct drm_display_mode *scan; /* *modes, *bios_mode; */
+ if (!connector) + return false; + + intel_lvds = intel_attached_lvds(connector); + + /* If we already have an EDID, no need to check again */ + if (intel_lvds->edid) + return true; + /* * Attempt to get the fixed panel mode from DDC. Assume that the * preferred mode is the right one. @@ -939,6 +948,12 @@ static bool intel_lvds_get_edid(struct drm_device *dev)
list_for_each_entry(scan, &connector->probed_modes, head) { if (scan->type & DRM_MODE_TYPE_PREFERRED) { + /* + * If we already have a preferred mode from another + * source, prefer the one from the EDID. + */ + if (intel_lvds->fixed_mode) + drm_mode_destroy(dev, intel_lvds->fixed_mode); intel_lvds->fixed_mode = drm_mode_duplicate(dev, scan); intel_find_lvds_downclock(dev,
If the LVDS panel wasn't connected at boot then we won't have an EDID for it. To fix this, call intel_lvds_get_edid() from the vga_switcheroo reprobe callback.
Signed-off-by: Seth Forshee seth.forshee@canonical.com --- drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_lvds.c | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 5b5176d..c9c942e 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1266,6 +1266,7 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_ static void i915_switcheroo_reprobe(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); + intel_lvds_get_edid(dev); intel_fb_output_poll_changed(dev); }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8435355..bcc14f9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -356,6 +356,7 @@ extern void intel_dvo_init(struct drm_device *dev); extern void intel_tv_init(struct drm_device *dev); extern void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj); +extern bool intel_lvds_get_edid(struct drm_device *dev); extern bool intel_lvds_init(struct drm_device *dev); extern void intel_dp_init(struct drm_device *dev, int dp_reg); void diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 9d05a90..39dbefc 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -902,7 +902,7 @@ static bool intel_lvds_supported(struct drm_device *dev) return IS_MOBILE(dev) && !IS_I830(dev); }
-static bool intel_lvds_get_edid(struct drm_device *dev) +bool intel_lvds_get_edid(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_connector *connector = dev_priv->int_lvds_connector;
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Dave Airlie -
Matthew Garrett -
Seth Forshee