[v5,10/12] drm/i915: Defer probe if gmux is present but its driver isn't

Hi,

On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >  /**
> > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU
> > + * @pdev: pci device of GPU
> > + *
> > + * Determine whether any prerequisites are not fulfilled to probe a given GPU.
> 
> I'd phrase this as "Determine whether the vgaswitcheroor support is
> fully loaded" and drop the GPU part - it could be needed by snd
> drivers eventually too.

Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU"
and moved the single existing check in an if block for
PCI_CLASS_DISPLAY_VGA devices.

> > + * DRM drivers should invoke this early on in their ->probe callback and return
> > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
> > + * vga_switcheroo_register_client() beforehand.
> 
> s/need not/must not/ ... is your native language German by any chance?

In principle there's no harm in registering the client first,
then checking if probing should be deferred, as long as the
client is unregistered before deferring. Thus the language
above is intentionally "need not" (muss nicht) rather than
"must not" (darf nicht). I didn't want to mandate something
that isn't actually required. The above sentence is merely
an aid for driver developers who might be confused in which
order to call what.

> Aside from that ... should vga_switcheroo_register_client call this
> helper instead directly and return the appropriate -EDEFER_PROBE
> error? I realize for some drivers it might be way too late to unrol
> from that point on, but e.g. i915 already uses -EDEFER_PROBE. And
> calling it unconditionally will make sure that it's easier to notice
> when people forget this. So maybe call it both from the register
> functions, and export as a separate hook? And for i915 we should be
> able to remove that early check entirely.

I'm afraid that wouldn't be a good idea. The ->probe hook is
potentially called dozens of times during boot until it finally
succeeds and vga_switcheroo_register_client() is usually called
in a fairly late driver load stage. In i915, we already have a
working GEM at that point and an almost fully brought up GPU.
Repeating bring up and tear down dozens of times is a nice
stress test but nothing I'd inflict on production systems.
I imagine it would also severely impact boot time and
clutter the kernel log.

So a separate helper seems to be the only sensible option.

> > + *
> > + * Return: %false unless one of the following applies:
> > + *
> > + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily
> > + *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
> > + *   Therefore return %true if gmux is built into the machine, @pdev is the
> > + *   inactive GPU and the apple-gmux driver has not registered its handler
> > + *   flags, signifying it has not yet loaded or has not finished initializing.
> 
> I think the apple-specific comment here should be a separate comment
> right above the if condition below - it doesn't explain the interface,
> only one specific case. And we might grow more of those.

Ok, I've moved that to a comment.

Updated patch follows after the scissors & perforation.

Thanks for the feedback!

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
 probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
    so rephrase kerneldoc to refer to "client" instead of "GPU"
    and move the single existing check in an if block specific
    to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
    that check from kerneldoc to a comment. (Daniel Vetter)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c       | 10 +---------
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +---------
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +---------
 drivers/gpu/vga/vga_switcheroo.c      | 28 ++++++++++++++++++++++++++++
 include/linux/vga_switcheroo.h        |  2 ++
 5 files changed, 33 insertions(+), 27 deletions(-)

[v5,10/12] drm/i915: Defer probe if gmux is present but its driver isn't

Commit Message

Comments

Patch