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 <lukas@xxxxxxxxx> 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. (Daniel Vetter)
>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
> 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 "i915_trace.h"
> #include "intel_drv.h"
>
> -#include <linux/apple-gmux.h>
> #include <linux/console.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
> #include <linux/vga_switcheroo.h>
> #include <drm/drm_crtc_helper.h>
>
> @@ -970,13 +968,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, &driver);
> 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 <linux/apple-gmux.h>
> #include <linux/console.h>
> #include <linux/delay.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
> #include <linux/vga_switcheroo.h>
>
> #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 b/drivers/gpu/drm/radeon/radeon_drv.c
> index cad2555..7be0c38 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -34,11 +34,9 @@
> #include "radeon_drv.h"
>
> #include <drm/drm_pciids.h>
> -#include <linux/apple-gmux.h>
> #include <linux/console.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
> #include <linux/vga_switcheroo.h>
> #include <drm/drm_gem.h>
>
> @@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> {
> 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;
>
> /* Get rid of things like offb */
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 56287ae..6108ebe 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -30,6 +30,7 @@
>
> #define pr_fmt(fmt) "vga_switcheroo: " fmt
>
> +#include <linux/apple-gmux.h>
> #include <linux/console.h>
> #include <linux/debugfs.h>
> #include <linux/fb.h>
> @@ -375,6 +376,33 @@ find_active_client(struct list_head *head)
> }
>
> /**
> + * vga_switcheroo_client_probe_defer() - whether to defer probing a given client
> + * @pdev: client pci device
> + *
> + * Determine whether any prerequisites are not fulfilled to probe a given
> + * client. Drivers should invoke this early on in their ->probe callback
> + * and return %-EPROBE_DEFER if it evaluates to %true. The client need
> + * not be registered with vga_switcheroo_register_client() beforehand.
> + *
> + * Return: %true if probing should be deferred, otherwise %false.
> + */
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
> + /*
> + * apple-gmux is needed on pre-retina MacBook Pro
> + * to probe the panel if pdev is the inactive GPU.
> + */
> + if (apple_gmux_present() && pdev != vga_default_device() &&
> + !vgasr_priv.handler_flags)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
> +
> +/**
> * vga_switcheroo_get_client_state() - obtain power state of a given client
> * @pdev: client pci device
> *
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index b39a5f3..960bedb 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
>
> int vga_switcheroo_process_delayed_switch(void);
>
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev);
> enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev);
>
> void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic);
> @@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v
> static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
> static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
> static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
> +static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; }
> static inline enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
>
> static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}
> --
> 1.8.5.2 (Apple Git-48)
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
