| Message ID | 35e4cc1e5c711fa30c3081dc628ce672a0fea276.1463642956.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lukas, On 19 May 2016 at 15:39, Lukas Wunner <lukas@wunner.de> wrote: > +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) > +{ > + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { Not sure if we want/need this, yet at least. This changes behaviour which is not what refactoring patches should be doing, right ? if needed it ought to be a separate patch, imho. Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we should also check for DISPLAY_3D ? Regards, Emil
On Thu, May 19, 2016 at 04:08:31PM -0000, Patchwork wrote: > == Series Details == > > Series: vga_switcheroo: Add helper for deferred probing > URL : https://patchwork.freedesktop.org/series/7409/ > State : failure > > == Summary == > > Series 7409v1 vga_switcheroo: Add helper for deferred probing > http://patchwork.freedesktop.org/api/1.0/series/7409/revisions/1/mbox > > Test drv_hangman: > Subgroup error-state-basic: > fail -> PASS (ro-ilk1-i5-650) > Test drv_module_reload_basic: > dmesg-warn -> PASS (fi-hsw-i7-4770r) > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > fail -> PASS (ro-byt-n2820) > Test kms_pipe_crc_basic: > Subgroup hang-read-crc-pipe-b: > pass -> FAIL (fi-hsw-i7-4770r) [ 487.307292] [drm:pipe_crc_set_source] collecting CRCs for pipe B, pf [ 487.390551] [drm:pipe_crc_set_source] stopping CRCs for pipe B [ 487.423865] [drm:hsw_audio_codec_disable] Disable audio codec on pipe B [ 487.423882] [drm:intel_disable_pipe] disabling pipe B ... [ 498.853439] [drm:intel_get_hpd_pins] hotplug event received, stat 0x00200000, dig 0x00101012, pins 0x00000020 [ 498.853447] [drm:intel_hpd_irq_storm_detect] Received HPD interrupt on PIN 5 - cnt: 0 [ 498.853535] [drm:i915_hotplug_work_func] running encoder hotplug functions [ 498.853549] [drm:i915_hotplug_work_func] Connector HDMI-A-1 (pin 5) received hotplug event. [ 498.853563] [drm:intel_hdmi_detect] [CONNECTOR:37:HDMI-A-1] [ 498.941487] [drm:intel_hdmi_detect] HDMI live status down [ 498.941505] [drm:intel_hpd_irq_event] [CONNECTOR:37:HDMI-A-1] status updated from connected to disconnected [ 498.941844] [drm:pipe_crc_set_source] collecting CRCs for pipe B, pf [ 499.051160] [drm:intel_get_hpd_pins] hotplug event received, stat 0x00200000, dig 0x00101012, pins 0x00000020 [ 499.051169] [drm:intel_hpd_irq_storm_detect] Received HPD interrupt on PIN 5 - cnt: 1 [ 499.051246] [drm:i915_hotplug_work_func] running encoder hotplug functions [ 499.051259] [drm:i915_hotplug_work_func] Connector HDMI-A-1 (pin 5) received hotplug event. [ 499.051271] [drm:intel_hdmi_detect] [CONNECTOR:37:HDMI-A-1] [ 499.078751] [drm:do_gmbus_xfer] GMBUS [i915 gmbus dpb] NAK for addr: 0040 w(1) [ 499.078756] [drm:do_gmbus_xfer] GMBUS [i915 gmbus dpb] NAK on first message, retry [ 499.078975] [drm:do_gmbus_xfer] GMBUS [i915 gmbus dpb] NAK for addr: 0040 w(1) [ 499.078983] [drm:drm_detect_monitor_audio] Monitor has basic audio support [ 499.078987] [drm:intel_hpd_irq_event] [CONNECTOR:37:HDMI-A-1] status updated from disconnected to connected [ 500.827624] [drm:intel_print_rc6_info] Enabling RC6 states: RC6 on [ 500.827649] [drm:gen6_enable_rps] Overclocking supported. Max: 1300MHz, Overclock max: 1300MHz [ 503.942669] kms_pipe_crc_basic: exiting, ret=99 So we shut down the pipe, then the monitor got suddenly disconnected, we didn't even try to enabled the pipe, tried to grab some CRCs, and then the monitor re-appeared. I think the attempt to grab CRCs from a disabled pipe is a race in igt_kms. The same one that sometimes causes us to sometimes attempt a modeset with a too small an FB. So basically for_each_connected_output() sees the old connected state, but then later igt_display_commit() will refresh the state from the kernel and decided that there's actually nothing to do, and then we just go on thinking everything is fine. So we really need to remove the "refresh from kernel" part from igt_display_commit(), or at least only sample that information in the test once, at the very start. > > fi-bdw-i7-5557u total:219 pass:206 dwarn:0 dfail:0 fail:0 skip:13 > fi-bsw-n3050 total:218 pass:174 dwarn:0 dfail:0 fail:2 skip:42 > fi-byt-n2820 total:218 pass:175 dwarn:0 dfail:0 fail:2 skip:41 > fi-hsw-i7-4770k total:219 pass:198 dwarn:0 dfail:0 fail:0 skip:21 > fi-hsw-i7-4770r total:219 pass:192 dwarn:0 dfail:0 fail:1 skip:26 > fi-skl-i7-6700k total:219 pass:191 dwarn:0 dfail:0 fail:0 skip:28 > ro-bdw-i5-5250u total:217 pass:179 dwarn:0 dfail:0 fail:0 skip:38 > ro-bdw-i7-5557U total:217 pass:204 dwarn:0 dfail:0 fail:0 skip:13 > ro-bdw-i7-5600u total:217 pass:184 dwarn:0 dfail:0 fail:1 skip:32 > ro-byt-n2820 total:216 pass:172 dwarn:0 dfail:0 fail:3 skip:41 > ro-hsw-i3-4010u total:216 pass:191 dwarn:0 dfail:0 fail:0 skip:25 > ro-hsw-i7-4770r total:217 pass:191 dwarn:0 dfail:0 fail:0 skip:26 > ro-ilk-i7-620lm total:217 pass:149 dwarn:0 dfail:0 fail:1 skip:67 > ro-ilk1-i5-650 total:212 pass:150 dwarn:0 dfail:0 fail:1 skip:61 > ro-ivb-i7-3770 total:217 pass:181 dwarn:0 dfail:0 fail:0 skip:36 > ro-ivb2-i7-3770 total:217 pass:185 dwarn:0 dfail:0 fail:0 skip:32 > ro-skl-i7-6700hq total:212 pass:187 dwarn:0 dfail:0 fail:0 skip:25 > ro-snb-i7-2620M total:217 pass:175 dwarn:0 dfail:0 fail:1 skip:41 > ro-bsw-n3050 failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_943/ > > 521c3f7 drm-intel-nightly: 2016y-05m-19d-13h-07m-04s UTC integration manifest > 2e50a25 vga_switcheroo: Add helper for deferred probing > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Emil, On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote: > On 19 May 2016 at 15:39, Lukas Wunner <lukas@wunner.de> wrote: > > +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) > > +{ > > + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { > Not sure if we want/need this, yet at least. This changes behaviour > which is not what refactoring patches should be doing, right ? if > needed it ought to be a separate patch, imho. Well, the commit message doesn't claim "no functional change", does it? Daniel Vetter explicitly wanted the ability to use the helper in vga_switcheroo audio clients, and those shouldn't run the apple-gmux test. I think it makes sense to enclose it in the above-quoted if-block *now* even though it's not needed. Once someone adds a check for an audio client, chances are they'll forget to add this if-block. > Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus > systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we > should also check for DISPLAY_3D ? Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY and sent it out as v5 a few minutes ago. Thanks, Lukas
On Sat, 21 May 2016, Lukas Wunner <lukas@wunner.de> wrote: > Hi Emil, > > On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote: >> On 19 May 2016 at 15:39, Lukas Wunner <lukas@wunner.de> wrote: >> > +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) >> > +{ >> > + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { >> Not sure if we want/need this, yet at least. This changes behaviour >> which is not what refactoring patches should be doing, right ? if >> needed it ought to be a separate patch, imho. > > Well, the commit message doesn't claim "no functional change", does it? > > Daniel Vetter explicitly wanted the ability to use the helper in > vga_switcheroo audio clients, and those shouldn't run the apple-gmux > test. I think it makes sense to enclose it in the above-quoted if-block > *now* even though it's not needed. Once someone adds a check for an > audio client, chances are they'll forget to add this if-block. Please always keep functional changes separate from non-functional refactoring. Combining them into one just makes everyone's life harder. This may seem like a simple case here, but you should get into the habit and stick to it. If you do a non-functional refactoring change, it's easy for people to review that change. I could've reviewed your patch in a few minutes; that's what I was in fact going to do until I stumbled on the functional change. Maybe I don't have the time to dig all the implications in that change right now. Bummer. If you do a non-functional refactoring change, and claim it's non-functional in the commit message, it'll help anyone landing on that commit as a result of bisecting a regression. Either the bisect was bogus, or your patch was unintentionally functional after all. Either should be easier to verify than a combined change. If you do the functional change separately, and that regresses, you don't have to throw out perfectly good refactoring changes when you revert the functional change. Or maybe the refactoring change doesn't revert anymore, and you'll have to do a more involved fix on top. It's easy to do reverts of clearly regressing commits, even by people who don't fully understand everything about the commit. Anything more is harder. Not rocket science, but it just adds up to the burden of the already swamped maintainers. Usually, like here, you should do the non-functional refactoring first, and then the functional change on top. The notable exception to this is when the functional change is a fix, especially of the kind that may need to get backported. Then you should do the fix first, even if it's harder and more verbose that way, because you don't want to add a stable backport dependency on a refactoring commit (which may exceed stable patch limits) and the stable maintainers frown upon manually backported patches. > Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY > and sent it out as v5 a few minutes ago. Please make that a v6 and split up the patch. Thanks, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index dba03c0..61bf5a9 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> @@ -1030,13 +1028,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 db5c7d0..a0bb1d0 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" @@ -315,13 +313,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 b55aa74..a455dc7 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> @@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev, if (ret == -EPROBE_DEFER) return 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 cbd7c98..101e14c 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> @@ -308,7 +309,8 @@ static int register_client(struct pci_dev *pdev, * * Register vga client (GPU). Enable vga_switcheroo if another GPU and a * handler have already registered. The power state of the client is assumed - * to be ON. + * to be ON. Beforehand, vga_switcheroo_client_probe_defer() shall be called + * to ensure that all prerequisites are met. * * Return: 0 on success, -ENOMEM on memory allocation error. */ @@ -329,7 +331,8 @@ EXPORT_SYMBOL(vga_switcheroo_register_client); * @id: client identifier * * Register audio client (audio device on a GPU). The power state of the - * client is assumed to be ON. + * client is assumed to be ON. Beforehand, vga_switcheroo_client_probe_defer() + * shall be called to ensure that all prerequisites are met. * * Return: 0 on success, -ENOMEM on memory allocation error. */ @@ -376,6 +379,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 shall invoke this early on in their ->probe callback + * and return %-EPROBE_DEFER if it evaluates to %true. Thou shalt not + * register the client ere thou hast called this. + * + * 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) {}
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) v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when amdkfd not loaded") 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 | 34 ++++++++++++++++++++++++++++++++-- include/linux/vga_switcheroo.h | 2 ++ 5 files changed, 37 insertions(+), 29 deletions(-)