| Message ID | 20120804165727.GA4980@thinkpad-t410 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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?
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
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.
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;