Lukas Wunner
2015-Aug-12 23:37 UTC
[Nouveau] [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro
Hi Daniel, On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote:> > * Reprobing if the inactive GPU initializes before the apple-gmux module: > > v1 used Matthew Garrett's approach of adding a driver callback. > > v2 simply generates a hotplug event instead. nouveau polls its outputs > > every 10 seconds so we want it to poll immediately once apple-gmux > > registers. That is achieved by the hotplug event. The i915 driver is > > changed to behave identically to nouveau. (Right now it deletes LVDS > > and eDP connectors from the mode configuration if they can't be probed, > > deeming them to be ghosts.) > > I thought -EDEFERREDPROBE is what we should be using if drivers don't get > loaded in the right order? Hand-rolling depency avoidance stuff is imo a > horrible idea.[...]> I think just reading edid and the relevant dp aux data in apple-gmux or > somewhere like that and stalling driver load until that's ready is the > only clean option.I'm afraid we can't stall initialization of a driver like that because even though the GPU may not be switched to the panel, it may still have an external monitor attached. All MacBook Pros have external DP and/or HDMI ports and these are either soldered to the discrete GPU (model year 2011 and onwards) or switchable between the discrete and integrated GPU (until 2010; I think they are even switchable separately from the panel). So basically we'd have to initialize the driver normally, and if intel_lvds_init() or intel_edp_init_connector() fail we'd have to somehow pass that up the call chain so that i915_pci_probe() can return -EPROBE_DEFER. And whenever we're asked to reprobe we'd repeat initialization of the LVDS or eDP connector. I'm wondering what the benefit is compared to just keeping the connector in the mode configuration, but with status disconnected, and reprobing it when the ->output_poll_changed callback gets invoked? Because that's what nouveau already does, and what I've changed i915 to do with patch 13. vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler registers (patch 11), which will invoke ->output_poll_changed. So we're talking about the Official DRM Callback [tm] to probe outputs, not "hand-rolling depency avoidance". :-)> > * Framebuffer recreation if the inactive GPU initializes before the > > apple-gmux module (i.e. discarding the default 1024x768 fb and replacing > > with a new one with the actual panel resolution): v1 only supported this > > for i915, v2 has a generic solution which works with nouveau and radeon > > as well. (Necessary if the discrete GPU is forced to be the inactive one > > on boot via the EFI variable.) > > Would completely remove the need for this ;-)Unfortunately not: We'd still have to initialize the driver to be able to drive external displays. If there are initially no connectors with modes, we'll once again end up with the 1024x768 fb.> You can't share the dp aux like that. It's true that we need a bit more > data (there's a few eDP related feature blocsk we need), but sharing the > aux channel entirely is no-go. If you do you get drivers trying to link > train and at best this fails and at worst you'll upset the configuration > of the other driver and piss of the panel enough to need a hard reset > until it works again.Yes, so far proxying of the AUX channel is read-only. In those cases when writing is necessary for setting up the output, I'm adding code to check if the current content of the DPCD is identical to what's being written and if so, skip the write. We'll see if this stategy is sufficient for the drivers to set up their outputs.> I think the real tricky bit here with vgaswitcheroo is locking, I need to > take a separate lock at the patches for that.Locking when switching only the DDC lines is facilitated by the ddc_lock attribute of struct vgasr_priv. This is all local to vga_switcheroo.c and contained in patches 5 and 6. Locking when proxying the AUX channel is facilitated by the hw_mutex attribute of struct drm_dp_aux. nouveau has its own locking mechanism contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus, when proxying via nouveau, there are two locking mechanisms at work (drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is nothing introduced by this patch set, all existing code. Locking of access to the struct vgasr_priv is facilitated by the vgasr_mutex in vga_switcheroo.c. Also existing code. Best regards, Lukas
Daniel Vetter
2015-Aug-13 06:50 UTC
[Nouveau] [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro
On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner <lukas at wunner.de> wrote:> Hi Daniel, > > On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote: >> > * Reprobing if the inactive GPU initializes before the apple-gmux module: >> > v1 used Matthew Garrett's approach of adding a driver callback. >> > v2 simply generates a hotplug event instead. nouveau polls its outputs >> > every 10 seconds so we want it to poll immediately once apple-gmux >> > registers. That is achieved by the hotplug event. The i915 driver is >> > changed to behave identically to nouveau. (Right now it deletes LVDS >> > and eDP connectors from the mode configuration if they can't be probed, >> > deeming them to be ghosts.) >> >> I thought -EDEFERREDPROBE is what we should be using if drivers don't get >> loaded in the right order? Hand-rolling depency avoidance stuff is imo a >> horrible idea. > [...] >> I think just reading edid and the relevant dp aux data in apple-gmux or >> somewhere like that and stalling driver load until that's ready is the >> only clean option. > > I'm afraid we can't stall initialization of a driver like that because > even though the GPU may not be switched to the panel, it may still have > an external monitor attached. > > All MacBook Pros have external DP and/or HDMI ports and these are > either soldered to the discrete GPU (model year 2011 and onwards) > or switchable between the discrete and integrated GPU (until 2010; > I think they are even switchable separately from the panel). > > So basically we'd have to initialize the driver normally, and if > intel_lvds_init() or intel_edp_init_connector() fail we'd have to > somehow pass that up the call chain so that i915_pci_probe() can > return -EPROBE_DEFER. And whenever we're asked to reprobe we'd > repeat initialization of the LVDS or eDP connector. > > I'm wondering what the benefit is compared to just keeping the > connector in the mode configuration, but with status disconnected, > and reprobing it when the ->output_poll_changed callback gets invoked? > Because that's what nouveau already does, and what I've changed i915 > to do with patch 13. > > vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler > registers (patch 11), which will invoke ->output_poll_changed. > So we're talking about the Official DRM Callback [tm] to probe > outputs, not "hand-rolling depency avoidance". :-)Oh I didn't spot that one. This kind of layering inversions generally leads to deadlocks and fun stuff. Also reprobing lvds/edp is just a side-effect when you have fbdev emulation enabled. If we go with this re-probing approach then we definitely need a new hook in vga-switcheroo, and even then there's still the locking problem.>> > * Framebuffer recreation if the inactive GPU initializes before the >> > apple-gmux module (i.e. discarding the default 1024x768 fb and replacing >> > with a new one with the actual panel resolution): v1 only supported this >> > for i915, v2 has a generic solution which works with nouveau and radeon >> > as well. (Necessary if the discrete GPU is forced to be the inactive one >> > on boot via the EFI variable.) >> >> Would completely remove the need for this ;-) > > Unfortunately not: We'd still have to initialize the driver to be able > to drive external displays. If there are initially no connectors with > modes, we'll once again end up with the 1024x768 fb.EDEFERREDPROBE isn't something that gets returned to userspace, it's just internal handling so that the kernel knows there's a depency issue and it needs to retry probing once other drivers have finished loading. It is the generic means linux has to handle cross-driver depencies which aren't reflected in the bus hierarchy. I.e. it's just something to make sure that apple-gmux is fully loaded before i915/nouveau. The driver _will_ be initialized eventually.>> You can't share the dp aux like that. It's true that we need a bit more >> data (there's a few eDP related feature blocsk we need), but sharing the >> aux channel entirely is no-go. If you do you get drivers trying to link >> train and at best this fails and at worst you'll upset the configuration >> of the other driver and piss of the panel enough to need a hard reset >> until it works again. > > Yes, so far proxying of the AUX channel is read-only. In those cases > when writing is necessary for setting up the output, I'm adding code > to check if the current content of the DPCD is identical to what's > being written and if so, skip the write. We'll see if this stategy is > sufficient for the drivers to set up their outputs.You need to block anything that would write _much_ earlier. By the time we're doing link-training it's way too late.>> I think the real tricky bit here with vgaswitcheroo is locking, I need to >> take a separate lock at the patches for that. > > Locking when switching only the DDC lines is facilitated by the ddc_lock > attribute of struct vgasr_priv. This is all local to vga_switcheroo.c > and contained in patches 5 and 6. > > Locking when proxying the AUX channel is facilitated by the hw_mutex > attribute of struct drm_dp_aux. nouveau has its own locking mechanism > contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus, > when proxying via nouveau, there are two locking mechanisms at work > (drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is > nothing introduced by this patch set, all existing code. > > Locking of access to the struct vgasr_priv is facilitated by the > vgasr_mutex in vga_switcheroo.c. Also existing code.Making sure everything is protected is the easy part of locking review. Making sure you can't deadlock is the hard part, and the mutex_trylock game looks like that's a problem not handled correctly. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Lukas Wunner
2015-Aug-16 19:10 UTC
[Nouveau] [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro
Hi Daniel, On Wed, Aug 12, 2015 at 11:10:59PM +0200, Daniel Vetter wrote:> Yes just squash and mention that the patch is based on work from > $list_of_other_authors, plus cc them. There's not much point in > acknowledging when people write broken patches ;-)As you requested I've squashed the first 7 patches and I'll post the resulting 3 replacement patches to dri-devel immediately after sending this e-mail. I've also overhauled locking. These 3 patches lay the groundwork for DDC switching with gmux. The subsequent patches in the series mostly concern reprobing and though I've made locking-related changes to these as well, I don't want to spam the list with them again until we have reached consensus how reprobing should be implemented: On Thu, Aug 13, 2015 at 08:50:47AM +0200, Daniel Vetter wrote:> EDEFERREDPROBE isn't something that gets returned to userspace, it's > just internal handling so that the kernel knows there's a depency > issue and it needs to retry probing once other drivers have finished > loading. It is the generic means linux has to handle cross-driver > depencies which aren't reflected in the bus hierarchy. > > I.e. it's just something to make sure that apple-gmux is fully loaded > before i915/nouveau. The driver _will_ be initialized eventually.Aha, so you want to stall initialization of i915/nouveau/radeon *completely* until apple-gmux is loaded. So how do you determine if initialization should be stalled? You would have to hardcode DMIs for all MacBook Pro models. I just counted it, there are 5 DMIs which require apple-gmux, 2 DMIs which require nouveau and 1 DMI which requires radeon (they require nouveau/radeon for proxying, apple-gmux won't help these models). And every year you would have to add another DMI for the latest MacBook Pro model. People using the latest model with an older kernel won't be able to use switching and may open support tickets. And if the module required for initialization is not installed or has a different version than the kernel, i915 won't initialize at all, which will be another source for support cases that you'll have to deal with. I think this doesn't scale and it shows that it's the wrong approach. vga_switcheroo *knows* when the handler registers, or the driver to proxy AUX, and it can *notify* the inactive GPU's driver. No need to hardcode DMIs, keep it simple. And there *is* already a callback to notify drivers that they should reprobe their outputs: * struct drm_mode_config_funcs - basic driver provided mode setting functions * @output_poll_changed: function to handle output configuration changes> On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner <lukas at wunner.de> wrote: > > vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler > > registers (patch 11), which will invoke ->output_poll_changed. > > Oh I didn't spot that one. This kind of layering inversions generally > leads to deadlocks and fun stuff.Please elaborate why you think it's a layering inversion to call drm_kms_helper_hotplug_event() from vga_switcheroo.> Also reprobing lvds/edp is just a > side-effect when you have fbdev emulation enabled.No, even though ->output_poll_changed is most commonly used to update the fbcon output configuration, it gets called even if fbdev emulation is disabled, and I've changed i915's callback in patch 13 so that the LVDS/eDP connectors are reprobed even if CONFIG_DRM_I915_FBDEV is not set.> If we go with this re-probing approach then we definitely > need a new hook in vga-switcherooWhy? From my point of view, drm_kms_helper_hotplug_event() already does the job. The only problem is that i915 removes LVDS and eDP connectors from the mode configuration if they can't be probed. By contrast, nouveau (and I think radeon) will just keep them, but with status disconnected. I changed i915 with patch 13 to behave identically to nouveau/radeon. (Sorry, I've written this before but I feel like I need to overcommunicate in this medium.) The question is, do you consider it harmful to keep a modeless LVDS or eDP connector in the mode configuration (with status disconnected)? On the MacBooks it works fine but I have no idea if it causes issues on other platforms. If you absolutely positively believe that this causes issues and don't want to change i915's behaviour, then yes indeed we may need a separate vga_switcheroo callback.> and even then there's still the locking problem.The only lock held when calling drm_kms_helper_hotplug_event() from vga_switcheroo is vgasr_mutex. When can this deadlock? Whenever we call a vga_switcheroo function from the driver upon probing outputs, specifically: - vga_switcheroo_client_fb_set() gets called if the fb is recreated on reprobe - vga_switcheroo_lock_ddc() / _unlock_ddc() get called when probing DDC and retrieving the EDID - vga_switcheroo_set_ddc() / _get_ddc() / _set_aux() / get_aux() get called for proxying So we can't lock vgasr_mutex in those functions. In the case of _lock_ddc() / _unlock_ddc() what we're actually protecting against is the sudden deregistration of the handler while we've switched DDC lines. We can solve that by grabbing vgasr_priv.ddc_lock in vga_switcheroo_unregister_handler() to block deregistration until we've switched back. This is what I've done in v2.1 (the 3 patches accompanying this e-mail). In the case of _fb_set() and the proxying functions, we grab vgasr_mutex because we iterate over the client list and change or retrieve client attributes. What we're actually protecting against is the sudden deregistration of a client while we're working on the client list. We can solve that by introducing a new lock which is grabbed in vga_switcheroo_unregister_client(), in _fb_set() and in the proxying functions. Best regards, Lukas