Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Deucher, Alexander]

> -Original Message-
> From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, February 18, 2016 6:11 PM
> To: Lukas Wunner
> Cc: dri-devel; platform-driver-...@vger.kernel.org; intel-gfx; Ben Skeggs;
> Deucher, Alexander
> Subject: Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but
> its driver isn't
> 
> On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner  wrote:
> > Hi,
> >
> > On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
> >> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner 
> wrote:
> >> >
> >> >> Ok, makes sense. I still think adding the check to the client_register
> >> >> function would be good, just as a safety measure.
> >> >
> >> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> >> > causes me pain in the stomach. It's surprising for drivers which
> >> > just don't need it at the moment (amdgpu and snd_hda_intel) and
> >> > it feels like overengineering and pampering driver developers
> >> > beyond reasonable measure. Also while the single existing check is
> >> > cheap, we might later on add checks that take more time and slow
> >> > things down.
> >>
> >> It's motivated by Rusty's API Manifesto:
> >>
> >> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> >
> > Interesting, thank you.
> >
> >
> >> With the mandatory check in _register we reach level 5 - it'll blow up
> >> at runtime when we try to register it.
> >
> > The manifesto says "5. Do it right or it will always break at runtime".
> >
> > However even if we add a
> WARN_ON(vga_switcheroo_client_probe_defer(pdev))
> > to register_client(), it will not *always* spew a stacktrace but only on
> > the machines this concerns (MacBook Pros). Since failure to defer breaks
> > GPU switching, level 5 is already reached. Chances are this won't go
> > unnoticed by the user.
> 
> If we fail the register hopefully the driver checks for that and might
> blow up somewhere in untested error handling code. But there's a good
> chance it'll fail (we can encourage that more by adding must_check to
> the function declaration). In that case you get a nice bug report with
> splat from users hitting this.
> 
> Without this it'll silently work, and all the reports you get is
> "linux is shit, gpu switching doesn't work".
> 
> In both cases it can sometimes succeed, which is not great indeed. But
> I'm trying to fix that by injection EDEFER points artificially
> somehow. Not yet figured out that one.
> 
> But irrespective of the precise failure mode making the defer check
> mandatory by just including it in _register() is better since it makes
> it impossible to forget to call it when its needed. So imo clearly the
> more robust API. And that's my metric for evaluating new API - how
> easy/hard is it to abuse/get wrong.
> 
> >> For more context: We have tons of fun with EPROBE_DEFER handling
> >> between i915 and snd-hda
> >
> > I don't understand, there is currently not a single occurrence of
> > EPROBE_DEFER in i915, apart from the one I added.
> >
> > In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
> > ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
> > resides.
> >
> > Is the fun with EPROBE_DEFER handling caused by the lack thereof?
> 
> Yes, there's one instance where i915 shoudl defer missing. The real
> trouble is that snd-hda has some really close ties with i915, and
> resolves those with probe-defer. And blows up all the time since we
> started using this, and with hdmi/dp you really always have to test
> both together in CI, snd-hda is pretty much a part of the intel gfx
> driver nowadays. Deferred probing is ime real trouble.

To further complicate things, AMD dGPUs have HDA audio on board as well.

Alex

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner  wrote:
> Hi,
>
> On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
>> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner  wrote:
>> >
>> >> Ok, makes sense. I still think adding the check to the client_register
>> >> function would be good, just as a safety measure.
>> >
>> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
>> > causes me pain in the stomach. It's surprising for drivers which
>> > just don't need it at the moment (amdgpu and snd_hda_intel) and
>> > it feels like overengineering and pampering driver developers
>> > beyond reasonable measure. Also while the single existing check is
>> > cheap, we might later on add checks that take more time and slow
>> > things down.
>>
>> It's motivated by Rusty's API Manifesto:
>>
>> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
>
> Interesting, thank you.
>
>
>> With the mandatory check in _register we reach level 5 - it'll blow up
>> at runtime when we try to register it.
>
> The manifesto says "5. Do it right or it will always break at runtime".
>
> However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev))
> to register_client(), it will not *always* spew a stacktrace but only on
> the machines this concerns (MacBook Pros). Since failure to defer breaks
> GPU switching, level 5 is already reached. Chances are this won't go
> unnoticed by the user.

If we fail the register hopefully the driver checks for that and might
blow up somewhere in untested error handling code. But there's a good
chance it'll fail (we can encourage that more by adding must_check to
the function declaration). In that case you get a nice bug report with
splat from users hitting this.

Without this it'll silently work, and all the reports you get is
"linux is shit, gpu switching doesn't work".

In both cases it can sometimes succeed, which is not great indeed. But
I'm trying to fix that by injection EDEFER points artificially
somehow. Not yet figured out that one.

But irrespective of the precise failure mode making the defer check
mandatory by just including it in _register() is better since it makes
it impossible to forget to call it when its needed. So imo clearly the
more robust API. And that's my metric for evaluating new API - how
easy/hard is it to abuse/get wrong.

>> For more context: We have tons of fun with EPROBE_DEFER handling
>> between i915 and snd-hda
>
> I don't understand, there is currently not a single occurrence of
> EPROBE_DEFER in i915, apart from the one I added.
>
> In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
> ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
> resides.
>
> Is the fun with EPROBE_DEFER handling caused by the lack thereof?

Yes, there's one instance where i915 shoudl defer missing. The real
trouble is that snd-hda has some really close ties with i915, and
resolves those with probe-defer. And blows up all the time since we
started using this, and with hdmi/dp you really always have to test
both together in CI, snd-hda is pretty much a part of the intel gfx
driver nowadays. Deferred probing is ime real trouble.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner  wrote:
> >
> >> Ok, makes sense. I still think adding the check to the client_register
> >> function would be good, just as a safety measure.
> >
> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> > causes me pain in the stomach. It's surprising for drivers which
> > just don't need it at the moment (amdgpu and snd_hda_intel) and
> > it feels like overengineering and pampering driver developers
> > beyond reasonable measure. Also while the single existing check is
> > cheap, we might later on add checks that take more time and slow
> > things down.
> 
> It's motivated by Rusty's API Manifesto:
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Interesting, thank you.


> With the mandatory check in _register we reach level 5 - it'll blow up
> at runtime when we try to register it.

The manifesto says "5. Do it right or it will always break at runtime".

However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev))
to register_client(), it will not *always* spew a stacktrace but only on
the machines this concerns (MacBook Pros). Since failure to defer breaks
GPU switching, level 5 is already reached. Chances are this won't go
unnoticed by the user.


> For more context: We have tons of fun with EPROBE_DEFER handling
> between i915 and snd-hda

I don't understand, there is currently not a single occurrence of
EPROBE_DEFER in i915, apart from the one I added.

In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
resides.

Is the fun with EPROBE_DEFER handling caused by the lack thereof?


Best regards,

Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner  wrote:
>
>> Ok, makes sense. I still think adding the check to the client_register
>> function would be good, just as a safety measure.
>
> Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> causes me pain in the stomach. It's surprising for drivers which
> just don't need it at the moment (amdgpu and snd_hda_intel) and
> it feels like overengineering and pampering driver developers
> beyond reasonable measure. Also while the single existing check is
> cheap, we might later on add checks that take more time and slow
> things down.

It's motivated by Rusty's API Manifesto:

http://sweng.the-davies.net/Home/rustys-api-design-manifesto

With the mandatory check in _register we reach level 5 - it'll blow up
at runtime when we try to register it. Without that the failure is
completely silent, and you need to read the right mailing list thread
(level 1), but at least the kerneldocs lift it up to level 3.

For more context: We have tons of fun with EPROBE_DEFER handling
between i915 and snd-hda, and I'm looking into all possible means to
make any api/subsystem using deferred probing as robust as possible by
default. One of the ideas is to inject deferred probe failures at
runtime, but that's kinda hard to do in a generic way. At least making
it as close to impossible to abuse as feasible is the next best
option.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 16, 2016 at 05:08:40PM +0100, Daniel Vetter wrote:
> On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> > On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> > > On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner  wrote:
> > > > + * 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.
> 
> I'd reject any driver that does this, registering, then checking, then
> unregistering seems extermely unsafe. I'd really stick with mandatory
> language here to make this clear.

Ok, I've made it mandatory in the kerneldoc, updated patch follows below.


> Ok, makes sense. I still think adding the check to the client_register
> function would be good, just as a safety measure.

Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
causes me pain in the stomach. It's surprising for drivers which
just don't need it at the moment (amdgpu and snd_hda_intel) and
it feels like overengineering and pampering driver developers
beyond reasonable measure. Also while the single existing check is
cheap, we might later on add checks that take more time and slow
things down.

Best regards,

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Add helper for deferred 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)

v3: Mandate in kerneldoc that registration of client shall only
happen after calling this helper. (Daniel Vetter)

Cc: Daniel Vetter 
Cc: Ben Skeggs 
Cc: Alex Deucher 
Signed-off-by: Lukas Wunner 
---
 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  | 34 --
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-  

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> 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  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.

I'd reject any driver that does this, registering, then checking, then
unregistering seems extermely unsafe. I'd really stick with mandatory
language here to make this clear.

> > 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.

Ok, makes sense. I still think adding the check to the client_register
function would be good, just as a safety measure. And I don't think you're
case of register(), check(), unregister() in case of failure is a valid
use-case. Let's not allow anyone to open-code that horror ;-)

Cheers, Daniel

> > > + *
> > > + * 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. 

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

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  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 
Cc: Ben Skeggs 
Cc: Alex Deucher 
Signed-off-by: Lukas Wunner 
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e8d0f17..01ef24a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include 

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner  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.

> + * 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?

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.

> + *
> + * 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.
-Daniel

> + */
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> +   if (apple_gmux_present() && pdev != vga_default_device() &&
> +   !vgasr_priv.handler_flags)
> +   return true;
> +
> +   return false;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
> +




-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 09, 2016 at 10:04:01AM +0100, Daniel Vetter wrote:
> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
[...]
> > @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> > if (PCI_FUNC(pdev->devfn))
> > return -ENODEV;
> >  
> > +   /*
> > +* apple-gmux is needed on dual GPU MacBook Pro
> > +* to probe the panel if we're the inactive GPU.
> > +*/
> > +   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> > +   apple_gmux_present() && pdev != vga_default_device() &&
> > +   !vga_switcheroo_handler_flags())
> > +   return -EPROBE_DEFER;
> 
> I pulled in all patches to drm-misc, but this here is imo ugly and needs
> to be polished a bit. What about adding a vga_switcheroo_ready() function
> which contains this check (and might in the future contain even more
> checks)? Then i915/radeon/nouveau would just have a simple
> 
>   if (!vga_switcheroo_ready())
>   return -EPROBE_DEFER;
> 
> and instead of duplicating the same comment 3 times we could have it once
> in one place. Plus some neat kerneldoc for this new helper to describe how
> it's supposed to be used. Plus better encapsulation of concepts.
> 
> Can you pls follow up with a patch/series to do that?

You're right, this is indeed much better. It also allows me to drop the
IS_ENABLED(CONFIG_VGA_ARB) and IS_ENABLED(CONFIG_VGA_SWITCHEROO) checks.

A patch follows below after the scissors.

The name vga_switcheroo_ready() was already taken by a static function
in vga_switcheroo.c, so I've named it vga_switcheroo_client_probe_defer().
If anyone has a suggestion for a better name I'll be happy to amend the
patch.

I've switched all three drivers to the new helper within the same patch
but will gladly spin this out into one patch per driver if preferred.

Thank you!

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.)

Cc: Daniel Vetter 
Cc: Ben Skeggs 
Cc: Alex Deucher 
Signed-off-by: Lukas Wunner 
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> gmux is a microcontroller built into dual GPU MacBook Pros.
> On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
> to temporarily switch DDC so that we can probe the panel's EDID.
> 
> The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
> because if either of them is disabled but gmux is present, the driver
> would never load, even if we're the active GPU. (vga_default_device()
> would evaluate to NULL and vga_switcheroo_handler_flags() would
> evaluate to 0.)
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Lukas Wunner 
> [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> Signed-off-by: Lukas Wunner 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3ac616d..4a5fc5d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -35,9 +35,12 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  
>  static struct drm_driver driver;
> @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>   if (PCI_FUNC(pdev->devfn))
>   return -ENODEV;
>  
> + /*
> +  * apple-gmux is needed on dual GPU MacBook Pro
> +  * to probe the panel if we're the inactive GPU.
> +  */
> + if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> + apple_gmux_present() && pdev != vga_default_device() &&
> + !vga_switcheroo_handler_flags())
> + return -EPROBE_DEFER;

I pulled in all patches to drm-misc, but this here is imo ugly and needs
to be polished a bit. What about adding a vga_switcheroo_ready() function
which contains this check (and might in the future contain even more
checks)? Then i915/radeon/nouveau would just have a simple

if (!vga_switcheroo_ready())
return -EPROBE_DEFER;

and instead of duplicating the same comment 3 times we could have it once
in one place. Plus some neat kerneldoc for this new helper to describe how
it's supposed to be used. Plus better encapsulation of concepts.

Can you pls follow up with a patch/series to do that?

Thanks, Daniel

> +
>   return drm_get_pci_dev(pdev, ent, );
>  }
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lukas Wunner]

gmux is a microcontroller built into dual GPU MacBook Pros.
On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
to temporarily switch DDC so that we can probe the panel's EDID.

The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
because if either of them is disabled but gmux is present, the driver
would never load, even if we're the active GPU. (vga_default_device()
would evaluate to NULL and vga_switcheroo_handler_flags() would
evaluate to 0.)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Lukas Wunner 
[MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
Signed-off-by: Lukas Wunner 
---
 drivers/gpu/drm/i915/i915_drv.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3ac616d..4a5fc5d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,9 +35,12 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+#include 
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 static struct drm_driver driver;
@@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
+   /*
+* apple-gmux is needed on dual GPU MacBook Pro
+* to probe the panel if we're the inactive GPU.
+*/
+   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
+   apple_gmux_present() && pdev != vga_default_device() &&
+   !vga_switcheroo_handler_flags())
+   return -EPROBE_DEFER;
+
return drm_get_pci_dev(pdev, ent, );
 }
 
-- 
1.8.5.2 (Apple Git-48)

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx