| Message ID | 20140509201655.2849.97478.stgit@bling.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 09, 2014 at 02:20:41PM -0600, Alex Williamson wrote: > Commit 81b5c7bc found that the current VGA arbiter support in i915 > only works for ancient GMCH-based IGD devices and attempted to update > support for newer HD devices. Unfortunately newer devices cannot > completely opt-out of VGA arbitration like the old devices could. > The VGA I/O space cannot be disabled internally. The only way to > route VGA I/O elsewhere is by disabling I/O at the device PCI command > register. This means that with commit 81b5c7bc and multiple VGA > adapters, the VGA arbiter will report that multiple VGA devices are > participating in arbitration, Xorg will notice this and disable DRI. > Therefore, 81b5c7bc was reverted because DRI is more important than > being correct. > > There is however an actual need for i915 to correctly participate in > VGA arbitration; VGA device assignment. If we want to use VFIO to > assign a VGA device to a virtual machine, we need to be able to > access the VGA resources of that device. By adding an i915 module > option we can allow i915 to continue with its charade by default, but > also allow an easy path for users who require working VGA arbitration. > Hopefully Xorg can someday be taught to behave better with multiple > VGA devices. > > This also rolls in reverted commit 6e1b4fda, which corrected an > ordering issue with 81b5c7bc by delaying the disabling of VGA memory > until after vgacon->fbcon handoff. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Dave Airlie <airlied@redhat.com> > --- > > This should be a nop with the default module setting, so if there's > any opportunity to get this into v3.15, it would be appreciated. I really don't like module options to work around bugs elsewhere. It means the 5 users who know about a given bug are now happen, and the 3 bazillion without enough clue/savy/time still suffer from problems. My goal is to actually add a bit of support to the core kernel's module option parsing code so that most of the options we have can spit warnings into dmesg and taint the kernel. Can't we fix this in any other way? -Daniel > > drivers/gpu/drm/i915/i915_dma.c | 22 +++++++++++++++++++--- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > include/linux/vgaarb.h | 7 +++++++ > 6 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 96177ee..c0d0c03 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1306,10 +1306,20 @@ static int i915_load_modeset_init(struct drm_device *dev) > * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA), > * then we do not take part in VGA arbitration and the > * vga_client_register() fails with -ENODEV. > + * > + * NB. The set_decode callback here actually only works on GMCH > + * devices, on newer HD devices we can only disable VGA MMIO space. > + * Disabling VGA I/O space requires disabling I/O in the PCI command > + * register. Nonetheless, we like to pretend that we participate in > + * VGA arbitration and can dynamically disable VGA I/O space because > + * this makes X happy, even though it's a complete lie. > */ > - ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode); > - if (ret && ret != -ENODEV) > - goto out; > + if (!i915.enable_hd_vgaarb || !HAS_PCH_SPLIT(dev)) { > + ret = vga_client_register(dev->pdev, dev, NULL, > + i915_vga_set_decode); > + if (ret && ret != -ENODEV) > + goto out; > + } > > intel_register_dsm_handler(); > > @@ -1369,6 +1379,12 @@ 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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ec82f6b..f3908f6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2080,6 +2080,7 @@ struct i915_params { > bool prefault_disable; > bool reset; > bool disable_display; > + bool enable_hd_vgaarb; > }; > extern struct i915_params i915 __read_mostly; > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index d1d7980..64d96c6 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = { > .invert_brightness = 0, > .disable_display = 0, > .enable_cmd_parser = 0, > + .enable_hd_vgaarb = false, > }; > > module_param_named(modeset, i915.modeset, int, 0400); > @@ -152,3 +153,7 @@ MODULE_PARM_DESC(disable_display, "Disable display (default: false)"); > module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600); > MODULE_PARM_DESC(enable_cmd_parser, > "Enable command parsing (1=enabled, 0=disabled [default])"); > + > +module_param_named(enable_hd_vgaarb, i915.enable_hd_vgaarb, bool, 0444); > +MODULE_PARM_DESC(enable_hd_vgaarb, > + "Enable support for VGA arbitration on Intel HD IGD. (default: false)"); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 69bcc42..2cd35ba 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11284,6 +11284,33 @@ 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 (i915.enable_hd_vgaarb && 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 (i915.enable_hd_vgaarb && 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_prepare_ddi(dev); > @@ -11594,6 +11621,7 @@ void i915_redisable_vga_power_on(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); > } > } > > @@ -11848,6 +11876,8 @@ 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 328b1a7..fbe5d360 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -934,4 +934,6 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data, > /* intel_tv.c */ > void intel_tv_init(struct drm_device *dev); > > +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 2c02f3a..80cf817 100644 > --- a/include/linux/vgaarb.h > +++ b/include/linux/vgaarb.h > @@ -65,8 +65,15 @@ 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 >
On Mon, 2014-05-12 at 21:08 +0200, Daniel Vetter wrote: > On Fri, May 09, 2014 at 02:20:41PM -0600, Alex Williamson wrote: > > Commit 81b5c7bc found that the current VGA arbiter support in i915 > > only works for ancient GMCH-based IGD devices and attempted to update > > support for newer HD devices. Unfortunately newer devices cannot > > completely opt-out of VGA arbitration like the old devices could. > > The VGA I/O space cannot be disabled internally. The only way to > > route VGA I/O elsewhere is by disabling I/O at the device PCI command > > register. This means that with commit 81b5c7bc and multiple VGA > > adapters, the VGA arbiter will report that multiple VGA devices are > > participating in arbitration, Xorg will notice this and disable DRI. > > Therefore, 81b5c7bc was reverted because DRI is more important than > > being correct. > > > > There is however an actual need for i915 to correctly participate in > > VGA arbitration; VGA device assignment. If we want to use VFIO to > > assign a VGA device to a virtual machine, we need to be able to > > access the VGA resources of that device. By adding an i915 module > > option we can allow i915 to continue with its charade by default, but > > also allow an easy path for users who require working VGA arbitration. > > Hopefully Xorg can someday be taught to behave better with multiple > > VGA devices. > > > > This also rolls in reverted commit 6e1b4fda, which corrected an > > ordering issue with 81b5c7bc by delaying the disabling of VGA memory > > until after vgacon->fbcon handoff. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Dave Airlie <airlied@redhat.com> > > --- > > > > This should be a nop with the default module setting, so if there's > > any opportunity to get this into v3.15, it would be appreciated. > > I really don't like module options to work around bugs elsewhere. It means > the 5 users who know about a given bug are now happen, and the 3 > bazillion without enough clue/savy/time still suffer from problems. > > My goal is to actually add a bit of support to the core kernel's module > option parsing code so that most of the options we have can spit warnings > into dmesg and taint the kernel. > > Can't we fix this in any other way? Do you have any suggestions? I proposed creating a new VGA arbiter interface that would allow Xorg to mmap the legacy VGA MMIO space regardless of the number of VGA arbitration participants, but didn't get any nibbles on supporting that through the rest of the stack. Dave suggested maybe he could rip out the VGA arbiter support from Xorg and nobody would notice. In either case, at what point do we get to flip i915 to behave correctly with arbitration? It seems like anything we do has a compatibility period where we must leave i915 in it's current broken state, which means that anyone wanting VGA arbitration needs to patch their kernel. In the best case, that compatibility window could extend for years. Since we don't seem to be making progress on any of the other fronts, it seemed time to propose a module switch. It's not a good solution, but it's better than leaving it broken. Thanks, Alex
On Mon, May 12, 2014 at 01:30:39PM -0600, Alex Williamson wrote: > On Mon, 2014-05-12 at 21:08 +0200, Daniel Vetter wrote: > > On Fri, May 09, 2014 at 02:20:41PM -0600, Alex Williamson wrote: > > > Commit 81b5c7bc found that the current VGA arbiter support in i915 > > > only works for ancient GMCH-based IGD devices and attempted to update > > > support for newer HD devices. Unfortunately newer devices cannot > > > completely opt-out of VGA arbitration like the old devices could. > > > The VGA I/O space cannot be disabled internally. The only way to > > > route VGA I/O elsewhere is by disabling I/O at the device PCI command > > > register. This means that with commit 81b5c7bc and multiple VGA > > > adapters, the VGA arbiter will report that multiple VGA devices are > > > participating in arbitration, Xorg will notice this and disable DRI. > > > Therefore, 81b5c7bc was reverted because DRI is more important than > > > being correct. > > > > > > There is however an actual need for i915 to correctly participate in > > > VGA arbitration; VGA device assignment. If we want to use VFIO to > > > assign a VGA device to a virtual machine, we need to be able to > > > access the VGA resources of that device. By adding an i915 module > > > option we can allow i915 to continue with its charade by default, but > > > also allow an easy path for users who require working VGA arbitration. > > > Hopefully Xorg can someday be taught to behave better with multiple > > > VGA devices. > > > > > > This also rolls in reverted commit 6e1b4fda, which corrected an > > > ordering issue with 81b5c7bc by delaying the disabling of VGA memory > > > until after vgacon->fbcon handoff. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Dave Airlie <airlied@redhat.com> > > > --- > > > > > > This should be a nop with the default module setting, so if there's > > > any opportunity to get this into v3.15, it would be appreciated. > > > > I really don't like module options to work around bugs elsewhere. It means > > the 5 users who know about a given bug are now happen, and the 3 > > bazillion without enough clue/savy/time still suffer from problems. > > > > My goal is to actually add a bit of support to the core kernel's module > > option parsing code so that most of the options we have can spit warnings > > into dmesg and taint the kernel. > > > > Can't we fix this in any other way? > > Do you have any suggestions? I proposed creating a new VGA arbiter > interface that would allow Xorg to mmap the legacy VGA MMIO space > regardless of the number of VGA arbitration participants, but didn't get > any nibbles on supporting that through the rest of the stack. Dave > suggested maybe he could rip out the VGA arbiter support from Xorg and > nobody would notice. In either case, at what point do we get to flip > i915 to behave correctly with arbitration? It seems like anything we do > has a compatibility period where we must leave i915 in it's current > broken state, which means that anyone wanting VGA arbitration needs to > patch their kernel. In the best case, that compatibility window could > extend for years. Since we don't seem to be making progress on any of > the other fronts, it seemed time to propose a module switch. It's not a > good solution, but it's better than leaving it broken. Thanks, Ripping out the vga arbiter stuff from X (or at least disabling it if there's only kms drivers) seems like a good start - we want that in any case I think. Then we could also look into disabling the userspace interface on the kernel side if we have modeset drivers for everything, and X is the requesting process. I.e. lie to Xorg. Ugly, but might work. Then once we've fixed userspace to no longer be stupid and have some way to work around old stupid userspace, we can fix i915. -Daniel
On Mon, 2014-05-12 at 21:38 +0200, Daniel Vetter wrote: > On Mon, May 12, 2014 at 01:30:39PM -0600, Alex Williamson wrote: > > On Mon, 2014-05-12 at 21:08 +0200, Daniel Vetter wrote: > > > On Fri, May 09, 2014 at 02:20:41PM -0600, Alex Williamson wrote: > > > > Commit 81b5c7bc found that the current VGA arbiter support in i915 > > > > only works for ancient GMCH-based IGD devices and attempted to update > > > > support for newer HD devices. Unfortunately newer devices cannot > > > > completely opt-out of VGA arbitration like the old devices could. > > > > The VGA I/O space cannot be disabled internally. The only way to > > > > route VGA I/O elsewhere is by disabling I/O at the device PCI command > > > > register. This means that with commit 81b5c7bc and multiple VGA > > > > adapters, the VGA arbiter will report that multiple VGA devices are > > > > participating in arbitration, Xorg will notice this and disable DRI. > > > > Therefore, 81b5c7bc was reverted because DRI is more important than > > > > being correct. > > > > > > > > There is however an actual need for i915 to correctly participate in > > > > VGA arbitration; VGA device assignment. If we want to use VFIO to > > > > assign a VGA device to a virtual machine, we need to be able to > > > > access the VGA resources of that device. By adding an i915 module > > > > option we can allow i915 to continue with its charade by default, but > > > > also allow an easy path for users who require working VGA arbitration. > > > > Hopefully Xorg can someday be taught to behave better with multiple > > > > VGA devices. > > > > > > > > This also rolls in reverted commit 6e1b4fda, which corrected an > > > > ordering issue with 81b5c7bc by delaying the disabling of VGA memory > > > > until after vgacon->fbcon handoff. > > > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Dave Airlie <airlied@redhat.com> > > > > --- > > > > > > > > This should be a nop with the default module setting, so if there's > > > > any opportunity to get this into v3.15, it would be appreciated. > > > > > > I really don't like module options to work around bugs elsewhere. It means > > > the 5 users who know about a given bug are now happen, and the 3 > > > bazillion without enough clue/savy/time still suffer from problems. > > > > > > My goal is to actually add a bit of support to the core kernel's module > > > option parsing code so that most of the options we have can spit warnings > > > into dmesg and taint the kernel. > > > > > > Can't we fix this in any other way? > > > > Do you have any suggestions? I proposed creating a new VGA arbiter > > interface that would allow Xorg to mmap the legacy VGA MMIO space > > regardless of the number of VGA arbitration participants, but didn't get > > any nibbles on supporting that through the rest of the stack. Dave > > suggested maybe he could rip out the VGA arbiter support from Xorg and > > nobody would notice. In either case, at what point do we get to flip > > i915 to behave correctly with arbitration? It seems like anything we do > > has a compatibility period where we must leave i915 in it's current > > broken state, which means that anyone wanting VGA arbitration needs to > > patch their kernel. In the best case, that compatibility window could > > extend for years. Since we don't seem to be making progress on any of > > the other fronts, it seemed time to propose a module switch. It's not a > > good solution, but it's better than leaving it broken. Thanks, > > Ripping out the vga arbiter stuff from X (or at least disabling it if > there's only kms drivers) seems like a good start - we want that in any > case I think. > > Then we could also look into disabling the userspace interface on the > kernel side if we have modeset drivers for everything, and X is the > requesting process. I.e. lie to Xorg. Ugly, but might work. > > Then once we've fixed userspace to no longer be stupid and have some way > to work around old stupid userspace, we can fix i915. I don't know what to do with this. It seems like a lot of wishful thinking that in the best case would drag on for years. Even if we get VGA arbitration out of Xorg, the bit about making the userspace VGA arbiter interface lie depending on current->comm sounds tricky and horrible. If we can lie to Xorg there, why don't we do that now? If we can't lie to Xorg now, then what deprecation event or detection of the caller is going to allow us to do so in the future? Meanwhile anyone that wants i915 to participate in arbitration like it should have from the start needs to patch their kernel with forward ports of the reverted commits. I just don't see this moving forward, which is why I thought a module option at least gives us a workaround. Thanks, Alex
On Thu, May 15, 2014 at 11:43 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > I don't know what to do with this. It seems like a lot of wishful > thinking that in the best case would drag on for years. Even if we get > VGA arbitration out of Xorg, the bit about making the userspace VGA > arbiter interface lie depending on current->comm sounds tricky and > horrible. If we can lie to Xorg there, why don't we do that now? If we > can't lie to Xorg now, then what deprecation event or detection of the > caller is going to allow us to do so in the future? Well we wouldn't necessarily need to lie to X, but could instead look whether all the vga devices in a system are claimed by kms drivers. If that's the case the userspace doesn't have an awful lot of business touching the VGA registers and we could simply not obey a vga arb request from userspace. More advanced would be if we still obey it for those devices not claimed by kms drivers. So not really a need to key on current->comm. > Meanwhile anyone that wants i915 to participate in arbitration like it > should have from the start needs to patch their kernel with forward > ports of the reverted commits. > > I just don't see this moving forward, which is why I thought a module > option at least gives us a workaround. Thanks, I know that this is awful, but a module option means that the few people interested in moving this forward will be happy enough to no longer bother. Which is even worse from my pov as driver maintainer. I highly consider module options used in production evil. -Daniel
On Fri, 2014-05-16 at 00:50 +0200, Daniel Vetter wrote: > On Thu, May 15, 2014 at 11:43 PM, Alex Williamson > <alex.williamson@redhat.com> wrote: > > I don't know what to do with this. It seems like a lot of wishful > > thinking that in the best case would drag on for years. Even if we get > > VGA arbitration out of Xorg, the bit about making the userspace VGA > > arbiter interface lie depending on current->comm sounds tricky and > > horrible. If we can lie to Xorg there, why don't we do that now? If we > > can't lie to Xorg now, then what deprecation event or detection of the > > caller is going to allow us to do so in the future? > > Well we wouldn't necessarily need to lie to X, but could instead look > whether all the vga devices in a system are claimed by kms drivers. If > that's the case the userspace doesn't have an awful lot of business > touching the VGA registers and we could simply not obey a vga arb > request from userspace. > > More advanced would be if we still obey it for those devices not > claimed by kms drivers. So not really a need to key on current->comm. This is a requirement for me, I don't really care about KMS or Xorg, the use case I want to enable is binding a VGA device to vfio-pci so that it can be assigned to a guest virtual machine. This works on an unmodified kernel today so long as you don't have an Intel IGD in your system. If you do, we try to switch the VGA device, but it doesn't actually get switched because i915 opts-out of arbitration yet still claims VGA accesses. > > Meanwhile anyone that wants i915 to participate in arbitration like it > > should have from the start needs to patch their kernel with forward > > ports of the reverted commits. > > > > I just don't see this moving forward, which is why I thought a module > > option at least gives us a workaround. Thanks, > > I know that this is awful, but a module option means that the few > people interested in moving this forward will be happy enough to no > longer bother. Which is even worse from my pov as driver maintainer. I > highly consider module options used in production evil. If I had a solution, I wouldn't be sending this patch :-\ Thanks, Alex
On Thu, May 15, 2014 at 10:46:50PM -0600, Alex Williamson wrote: > On Fri, 2014-05-16 at 00:50 +0200, Daniel Vetter wrote: > > On Thu, May 15, 2014 at 11:43 PM, Alex Williamson > > <alex.williamson@redhat.com> wrote: > > > I don't know what to do with this. It seems like a lot of wishful > > > thinking that in the best case would drag on for years. Even if we get > > > VGA arbitration out of Xorg, the bit about making the userspace VGA > > > arbiter interface lie depending on current->comm sounds tricky and > > > horrible. If we can lie to Xorg there, why don't we do that now? If we > > > can't lie to Xorg now, then what deprecation event or detection of the > > > caller is going to allow us to do so in the future? > > > > Well we wouldn't necessarily need to lie to X, but could instead look > > whether all the vga devices in a system are claimed by kms drivers. If > > that's the case the userspace doesn't have an awful lot of business > > touching the VGA registers and we could simply not obey a vga arb > > request from userspace. > > > > More advanced would be if we still obey it for those devices not > > claimed by kms drivers. So not really a need to key on current->comm. > > This is a requirement for me, I don't really care about KMS or Xorg, the > use case I want to enable is binding a VGA device to vfio-pci so that it > can be assigned to a guest virtual machine. This works on an unmodified > kernel today so long as you don't have an Intel IGD in your system. If > you do, we try to switch the VGA device, but it doesn't actually get > switched because i915 opts-out of arbitration yet still claims VGA > accesses. I get that its a requirement for you. I've also just detailed the solution for you above, but I'm not going to write the patch itself (since I can't test it really). We have two users of the vga-arb crap relevant here: - pci/pci-sysfs.c, used by X through libpciaccess - vfio/pci/vfio_pci_rdwr.c, for vfio-pci vga forwarding Make the former lie if all vga devices have kms drivers and the latter still work correctly. That will prevent X from going nasty if there are kms drivers, while still keeping vfio going. Then we re-re-revert the i915 to have proper vga-arb. Afaics no need for hacks, module options, special casing or breaking old userspace. And really, if there is a kms driver userspace has zero business touching the vga registers, so imo we don't lose an real functionality. You can always opt to not load kms drivers if you want userspace access. What am I missing? -Daniel
On Fri, 2014-05-16 at 11:06 +0200, Daniel Vetter wrote: > On Thu, May 15, 2014 at 10:46:50PM -0600, Alex Williamson wrote: > > On Fri, 2014-05-16 at 00:50 +0200, Daniel Vetter wrote: > > > On Thu, May 15, 2014 at 11:43 PM, Alex Williamson > > > <alex.williamson@redhat.com> wrote: > > > > I don't know what to do with this. It seems like a lot of wishful > > > > thinking that in the best case would drag on for years. Even if we get > > > > VGA arbitration out of Xorg, the bit about making the userspace VGA > > > > arbiter interface lie depending on current->comm sounds tricky and > > > > horrible. If we can lie to Xorg there, why don't we do that now? If we > > > > can't lie to Xorg now, then what deprecation event or detection of the > > > > caller is going to allow us to do so in the future? > > > > > > Well we wouldn't necessarily need to lie to X, but could instead look > > > whether all the vga devices in a system are claimed by kms drivers. If > > > that's the case the userspace doesn't have an awful lot of business > > > touching the VGA registers and we could simply not obey a vga arb > > > request from userspace. > > > > > > More advanced would be if we still obey it for those devices not > > > claimed by kms drivers. So not really a need to key on current->comm. > > > > This is a requirement for me, I don't really care about KMS or Xorg, the > > use case I want to enable is binding a VGA device to vfio-pci so that it > > can be assigned to a guest virtual machine. This works on an unmodified > > kernel today so long as you don't have an Intel IGD in your system. If > > you do, we try to switch the VGA device, but it doesn't actually get > > switched because i915 opts-out of arbitration yet still claims VGA > > accesses. > > I get that its a requirement for you. > > I've also just detailed the solution for you above, but I'm not going to > write the patch itself (since I can't test it really). I repeat it because it seems incompatible with any plan that involves modes of operation where the VGA arbiter says one thing if all VGA devices are bound to KMS drivers and another if they're not. That means Xorg would have a different feature set depending on the driver state of devices we might be using for entirely different purposes. Users want to be able to run X on the host i915 at the same time that they have other VGA devices assigned to guests. > We have two users of the vga-arb crap relevant here: > - pci/pci-sysfs.c, used by X through libpciaccess > - vfio/pci/vfio_pci_rdwr.c, for vfio-pci vga forwarding Do we really know that we only have these two users? > Make the former lie if all vga devices have kms drivers and the latter There's that "if all vga devices" again. Is the user expected to start Xorg with all devices attached to KMS drivers, then unbind the extra VGA devices to be used for other purposes? What happens if the user wants to restart X, do they get a different feature set? Does the output of the VGA arbiter change while X is running when the other devices are unbound from their KMS driver? (unbinding a graphics card from it's driver isn't a foolproof operation btw, so the driver may be blacklisted or avoided through other means) > still work correctly. That will prevent X from going nasty if there are > kms drivers, while still keeping vfio going. But by lying to Xorg, I think we're saying that it's ok to go mmap VGA MMIO space through /dev/mem and poke VGA I/O through inb/outb, it's not going to change... then at the same time we provide this other interface that allows it to change. Are we just masking i915's bug by breaking the entire interface? > Then we re-re-revert the i915 to have proper vga-arb. > > Afaics no need for hacks, module options, special casing or breaking old > userspace. But there is special casing isn't there? Aren't we only able to make the assumption that it's ok to lie through the VGA arbiter userspace interface if only KMS drivers are in use? > And really, if there is a kms driver userspace has zero > business touching the vga registers, so imo we don't lose an real > functionality. You can always opt to not load kms drivers if you want > userspace access. > > What am I missing? I must be the one missing something because this still all sounds terribly convoluted and fragile. Thanks, Alex
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 96177ee..c0d0c03 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1306,10 +1306,20 @@ static int i915_load_modeset_init(struct drm_device *dev) * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA), * then we do not take part in VGA arbitration and the * vga_client_register() fails with -ENODEV. + * + * NB. The set_decode callback here actually only works on GMCH + * devices, on newer HD devices we can only disable VGA MMIO space. + * Disabling VGA I/O space requires disabling I/O in the PCI command + * register. Nonetheless, we like to pretend that we participate in + * VGA arbitration and can dynamically disable VGA I/O space because + * this makes X happy, even though it's a complete lie. */ - ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode); - if (ret && ret != -ENODEV) - goto out; + if (!i915.enable_hd_vgaarb || !HAS_PCH_SPLIT(dev)) { + ret = vga_client_register(dev->pdev, dev, NULL, + i915_vga_set_decode); + if (ret && ret != -ENODEV) + goto out; + } intel_register_dsm_handler(); @@ -1369,6 +1379,12 @@ 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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ec82f6b..f3908f6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2080,6 +2080,7 @@ struct i915_params { bool prefault_disable; bool reset; bool disable_display; + bool enable_hd_vgaarb; }; extern struct i915_params i915 __read_mostly; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index d1d7980..64d96c6 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = { .invert_brightness = 0, .disable_display = 0, .enable_cmd_parser = 0, + .enable_hd_vgaarb = false, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -152,3 +153,7 @@ MODULE_PARM_DESC(disable_display, "Disable display (default: false)"); module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600); MODULE_PARM_DESC(enable_cmd_parser, "Enable command parsing (1=enabled, 0=disabled [default])"); + +module_param_named(enable_hd_vgaarb, i915.enable_hd_vgaarb, bool, 0444); +MODULE_PARM_DESC(enable_hd_vgaarb, + "Enable support for VGA arbitration on Intel HD IGD. (default: false)"); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 69bcc42..2cd35ba 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11284,6 +11284,33 @@ 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 (i915.enable_hd_vgaarb && 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 (i915.enable_hd_vgaarb && 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_prepare_ddi(dev); @@ -11594,6 +11621,7 @@ void i915_redisable_vga_power_on(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); } } @@ -11848,6 +11876,8 @@ 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 328b1a7..fbe5d360 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -934,4 +934,6 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data, /* intel_tv.c */ void intel_tv_init(struct drm_device *dev); +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 2c02f3a..80cf817 100644 --- a/include/linux/vgaarb.h +++ b/include/linux/vgaarb.h @@ -65,8 +65,15 @@ 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
Commit 81b5c7bc found that the current VGA arbiter support in i915 only works for ancient GMCH-based IGD devices and attempted to update support for newer HD devices. Unfortunately newer devices cannot completely opt-out of VGA arbitration like the old devices could. The VGA I/O space cannot be disabled internally. The only way to route VGA I/O elsewhere is by disabling I/O at the device PCI command register. This means that with commit 81b5c7bc and multiple VGA adapters, the VGA arbiter will report that multiple VGA devices are participating in arbitration, Xorg will notice this and disable DRI. Therefore, 81b5c7bc was reverted because DRI is more important than being correct. There is however an actual need for i915 to correctly participate in VGA arbitration; VGA device assignment. If we want to use VFIO to assign a VGA device to a virtual machine, we need to be able to access the VGA resources of that device. By adding an i915 module option we can allow i915 to continue with its charade by default, but also allow an easy path for users who require working VGA arbitration. Hopefully Xorg can someday be taught to behave better with multiple VGA devices. This also rolls in reverted commit 6e1b4fda, which corrected an ordering issue with 81b5c7bc by delaying the disabling of VGA memory until after vgacon->fbcon handoff. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Dave Airlie <airlied@redhat.com> --- This should be a nop with the default module setting, so if there's any opportunity to get this into v3.15, it would be appreciated. drivers/gpu/drm/i915/i915_dma.c | 22 +++++++++++++++++++--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_params.c | 5 +++++ drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ include/linux/vgaarb.h | 7 +++++++ 6 files changed, 64 insertions(+), 3 deletions(-)