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

[Lukas Wunner]

Hi Daniel,

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

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

I've also overhauled locking.

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


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

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

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

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

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

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

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

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


> On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner <lukas 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-switcheroo

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

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

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


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

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

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

So we can't lock vgasr_mutex in those functions.

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

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


Best regards,

Lukas