| Message ID | 1381246048-4749-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 08, 2013 at 05:27:28PM +0200, Daniel Vetter wrote: > Adding drm/i915 into the vga arbiter chain means that X (in a piece of > well-meant paranoia) will do a get/put on the vga decoding around > _every_ accel call down into the ddx. Which results in some nice > performance disasters [1]. And disables DRI for the system, interferring with the use of OpenGL compositors, playing games, PRIME, etc. This effect is not limited to users of i915, but any system with a dGPU and an igfx enabled Core CPU. I still think we are downplaying the complete and utter disaster caused by X and the patch, and why the revert is required. -Chris
On Tue, 2013-10-08 at 17:08 +0100, Chris Wilson wrote: > On Tue, Oct 08, 2013 at 05:27:28PM +0200, Daniel Vetter wrote: > > Adding drm/i915 into the vga arbiter chain means that X (in a piece of > > well-meant paranoia) will do a get/put on the vga decoding around > > _every_ accel call down into the ddx. Which results in some nice > > performance disasters [1]. > > And disables DRI for the system, interferring with the use of OpenGL > compositors, playing games, PRIME, etc. This effect is not limited to > users of i915, but any system with a dGPU and an igfx enabled Core CPU. > > I still think we are downplaying the complete and utter disaster caused > by X and the patch, and why the revert is required. I'm failing to understand the disaster myself, can you explain? i915 is already attempting to make use of the VGA arbiter, but the code hadn't been updated so what it was doing was ineffective. The external difference I see is that before i915 incorrectly reported that it was not involved in VGA arbitration (even though it still claims VGA transactions). With the fixes, it opts-out of MMIO arbitration, but requires I/O port arbitration. I'm OK with reverting these and going back to the drawing board for v3.13, but what i915 is doing is still incorrect and precludes making use of a guest assigned VGA device (or separate display server requiring VGA), because we can't use the VGA arbiter for what it's designed to do, re-route VGA accesses. Thanks, Alex
On Tue, Oct 08, 2013 at 10:18:54AM -0600, Alex Williamson wrote: > On Tue, 2013-10-08 at 17:08 +0100, Chris Wilson wrote: > > On Tue, Oct 08, 2013 at 05:27:28PM +0200, Daniel Vetter wrote: > > > Adding drm/i915 into the vga arbiter chain means that X (in a piece of > > > well-meant paranoia) will do a get/put on the vga decoding around > > > _every_ accel call down into the ddx. Which results in some nice > > > performance disasters [1]. > > > > And disables DRI for the system, interferring with the use of OpenGL > > compositors, playing games, PRIME, etc. This effect is not limited to > > users of i915, but any system with a dGPU and an igfx enabled Core CPU. > > > > I still think we are downplaying the complete and utter disaster caused > > by X and the patch, and why the revert is required. > > I'm failing to understand the disaster myself, can you explain? i915 is > already attempting to make use of the VGA arbiter, but the code hadn't > been updated so what it was doing was ineffective. The external > difference I see is that before i915 incorrectly reported that it was > not involved in VGA arbitration (even though it still claims VGA > transactions). With the fixes, it opts-out of MMIO arbitration, but > requires I/O port arbitration. I'm OK with reverting these and going > back to the drawing board for v3.13, but what i915 is doing is still > incorrect and precludes making use of a guest assigned VGA device (or > separate display server requiring VGA), because we can't use the VGA > arbiter for what it's designed to do, re-route VGA accesses. Thanks, The problem stems from the system reporting to X that there are two legacy vga IO capable devices, and then X disabling DRI and wrapping every operation with vga_get/vga_put. X will do this even if it is only using one of the devices and the other device is not being used by the system. We are not arguing that your fix isn't correct, it is just the rammifications it has to the existing install base outweighs improving a currently broken feature. -Chris
On Tue, 2013-10-08 at 17:29 +0100, Chris Wilson wrote: > On Tue, Oct 08, 2013 at 10:18:54AM -0600, Alex Williamson wrote: > > On Tue, 2013-10-08 at 17:08 +0100, Chris Wilson wrote: > > > On Tue, Oct 08, 2013 at 05:27:28PM +0200, Daniel Vetter wrote: > > > > Adding drm/i915 into the vga arbiter chain means that X (in a piece of > > > > well-meant paranoia) will do a get/put on the vga decoding around > > > > _every_ accel call down into the ddx. Which results in some nice > > > > performance disasters [1]. > > > > > > And disables DRI for the system, interferring with the use of OpenGL > > > compositors, playing games, PRIME, etc. This effect is not limited to > > > users of i915, but any system with a dGPU and an igfx enabled Core CPU. > > > > > > I still think we are downplaying the complete and utter disaster caused > > > by X and the patch, and why the revert is required. > > > > I'm failing to understand the disaster myself, can you explain? i915 is > > already attempting to make use of the VGA arbiter, but the code hadn't > > been updated so what it was doing was ineffective. The external > > difference I see is that before i915 incorrectly reported that it was > > not involved in VGA arbitration (even though it still claims VGA > > transactions). With the fixes, it opts-out of MMIO arbitration, but > > requires I/O port arbitration. I'm OK with reverting these and going > > back to the drawing board for v3.13, but what i915 is doing is still > > incorrect and precludes making use of a guest assigned VGA device (or > > separate display server requiring VGA), because we can't use the VGA > > arbiter for what it's designed to do, re-route VGA accesses. Thanks, > > The problem stems from the system reporting to X that there are two > legacy vga IO capable devices, and then X disabling DRI and wrapping > every operation with vga_get/vga_put. X will do this even if it is only > using one of the devices and the other device is not being used by the > system. > > We are not arguing that your fix isn't correct, it is just the > rammifications it has to the existing install base outweighs improving > a currently broken feature. Sure, my use case is not a priority. Do you have any thoughts on how we can redesign the interface? For my use for VM access, I'm pretty much resolved and OK with the fact that VGA access is slow. Without para-virtualizing VGA, I can't predict or batch accesses, so I take the arbiter lock for the appropriate resource around each individual access. If the lock is contended, VM boot can be painful, but eventually accelerated drivers take over and VGA access becomes rare. I know Ville was concerned about stopped processes holding the lock when i915 needs it. In my case, that can't happen because the VGA arbiter lock is handled within the VFIO interface. A read/write to the device file descriptor will grab and release the lock internally. It's hard to get around the benefits of knowing your access pattern and batching accesses within a lock, but I wonder if VGA arbiter's lazy switching would be sufficient to provide a general purpose legacy VGA file descriptor that X could open and use for all VGA accesses. For example, pci-sysfs will provide legacy I/O interfaces for some architectures, and I think X already makes use of these. What if they were provided on x86 and allowed read/write to VGA ranges wrapped in VGA arbiter locks? Thanks, Alex
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index c27a210..d5c784d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1290,12 +1290,9 @@ static int i915_load_modeset_init(struct drm_device *dev) * then we do not take part in VGA arbitration and the * vga_client_register() fails with -ENODEV. */ - if (!HAS_PCH_SPLIT(dev)) { - ret = vga_client_register(dev->pdev, dev, NULL, - i915_vga_set_decode); - if (ret && ret != -ENODEV) - goto out; - } + ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode); + if (ret && ret != -ENODEV) + goto out; intel_register_dsm_handler(); @@ -1351,12 +1348,6 @@ static int i915_load_modeset_init(struct drm_device *dev) */ intel_fbdev_initial_config(dev); - /* - * Must do this after fbcon init so that - * vgacon_save_screen() works during the handover. - */ - i915_disable_vga_mem(dev); - /* Only enable hotplug handling once the fbdev is fully set up. */ dev_priv->enable_hotplug_processing = true; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c89abd3..581fb4b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10045,33 +10045,6 @@ static void i915_disable_vga(struct drm_device *dev) POSTING_READ(vga_reg); } -static void i915_enable_vga_mem(struct drm_device *dev) -{ - /* Enable VGA memory on Intel HD */ - if (HAS_PCH_SPLIT(dev)) { - vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO); - outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE); - vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO | - VGA_RSRC_LEGACY_MEM | - VGA_RSRC_NORMAL_IO | - VGA_RSRC_NORMAL_MEM); - vga_put(dev->pdev, VGA_RSRC_LEGACY_IO); - } -} - -void i915_disable_vga_mem(struct drm_device *dev) -{ - /* Disable VGA memory on Intel HD */ - if (HAS_PCH_SPLIT(dev)) { - vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO); - outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE); - vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO | - VGA_RSRC_NORMAL_IO | - VGA_RSRC_NORMAL_MEM); - vga_put(dev->pdev, VGA_RSRC_LEGACY_IO); - } -} - void intel_modeset_init_hw(struct drm_device *dev) { intel_init_power_well(dev); @@ -10350,7 +10323,6 @@ void i915_redisable_vga(struct drm_device *dev) if (I915_READ(vga_reg) != VGA_DISP_DISABLE) { DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n"); i915_disable_vga(dev); - i915_disable_vga_mem(dev); } } @@ -10564,8 +10536,6 @@ void intel_modeset_cleanup(struct drm_device *dev) intel_disable_fbc(dev); - i915_enable_vga_mem(dev); - intel_disable_gt_powersave(dev); ironlake_teardown_rc6(dev); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 28cae80..9b7b68f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -793,6 +793,5 @@ extern void hsw_pc8_disable_interrupts(struct drm_device *dev); extern void hsw_pc8_restore_interrupts(struct drm_device *dev); extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv); extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv); -extern void i915_disable_vga_mem(struct drm_device *dev); #endif /* __INTEL_DRV_H__ */ diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h index 80cf817..2c02f3a 100644 --- a/include/linux/vgaarb.h +++ b/include/linux/vgaarb.h @@ -65,15 +65,8 @@ struct pci_dev; * out of the arbitration process (and can be safe to take * interrupts at any time. */ -#if defined(CONFIG_VGA_ARB) extern void vga_set_legacy_decoding(struct pci_dev *pdev, unsigned int decodes); -#else -static inline void vga_set_legacy_decoding(struct pci_dev *pdev, - unsigned int decodes) -{ -} -#endif /** * vga_get - acquire & locks VGA resources
Adding drm/i915 into the vga arbiter chain means that X (in a piece of well-meant paranoia) will do a get/put on the vga decoding around _every_ accel call down into the ddx. Which results in some nice performance disasters [1]. Ville tried to come up with a Great Hack to fiddle the required VGA I/O ops behind everyone's back using stop_machine, but that didn't really work out [2]. Given that we're fairly late in the -rc stage for such games let's just revert this all. This reverts commit 6e1b4fdad5157bb9e88777d525704aba24389bee Author: Ville Syrjälä <ville.syrjala@linux.intel.com> Date: Thu Sep 5 20:40:52 2013 +0300 drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done and commit 81b5c7bc8de3e6f63419139c2fc91bf81dea8a7d Author: Alex Williamson <alex.williamson@redhat.com> Date: Wed Aug 28 09:39:08 2013 -0600 i915: Update VGA arbiter support for newer devices One thing we might want to keep is to delay the disabling of the vga decoding until the fbdev emulation and the fbcon screen is set up. If we kill vga mem decoding beforehand fbcon can end up with a white square in the top-left corner it tried to save from the vga memory for a seamless transition. And we have bug reports on older platforms which seem to match these symptoms. But again that's something to play around with in -next. References: [1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html References: [2] http://www.spinics.net/lists/intel-gfx/msg34062.html Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_dma.c | 15 +++------------ drivers/gpu/drm/i915/intel_display.c | 30 ------------------------------ drivers/gpu/drm/i915/intel_drv.h | 1 - include/linux/vgaarb.h | 7 ------- 4 files changed, 3 insertions(+), 50 deletions(-)