[PATCH 0/2] vgaarb: Fixes for partial VGA opt-out

[PATCH 0/2] vgaarb: Fixes for partial VGA opt-out

[Alex Williamson]

I'm trying to add support for VGA arbitration on newer Intel graphics
devices.  The existing code attempts to do this, but appear to have
not been updated since GMCH devices roamed the Earth.  On newer
devices like Haswell, we can disable VGA memory through an MSR on the
device, but we rely on the VGA arbiter to manage VGA IO using the PCI
COMMAND register.  In trying to unregister legacy VGA memory, I found
that the VGA arbiter still wanted to disable both memory and IO on
the device and that it forgot to actually program the device to
disable IO when the decoding is updated.  This series attempts to fix
both of those.  Thanks,

Alex

---

Alex Williamson (2):
      vgaarb: Don't disable resources that are not owned
      vgaarb: Fix VGA decodes changes


 drivers/gpu/vga/vgaarb.c |   50 ++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

[PATCH 1/2] vgaarb: Don't disable resources that are not owned

[Alex Williamson]

If a device does not own a resource then we don't need to disable it.
This resolves the case where an Intel IGD device can be configured to
disable decode of VGA memory but we still need the arbiter to handle
VGA I/O port routing.  When the IGD device is in conflict, only
PCI_COMMAND_IO should be disabled since VGA memory does not require
arbitration on this device.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/vga/vgaarb.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index e893f6e..ea56471 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -257,9 +257,9 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 		if (!conflict->bridge_has_one_vga) {
 			vga_irq_set_state(conflict, false);
 			flags |= PCI_VGA_STATE_CHANGE_DECODES;
-			if (lwants & (VGA_RSRC_LEGACY_MEM|VGA_RSRC_NORMAL_MEM))
+			if (match & (VGA_RSRC_LEGACY_MEM|VGA_RSRC_NORMAL_MEM))
 				pci_bits |= PCI_COMMAND_MEMORY;
-			if (lwants & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO))
+			if (match & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO))
 				pci_bits |= PCI_COMMAND_IO;
 		}
 
@@ -267,11 +267,11 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 			flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
 
 		pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
-		conflict->owns &= ~lwants;
+		conflict->owns &= ~match;
 		/* If he also owned non-legacy, that is no longer the case */
-		if (lwants & VGA_RSRC_LEGACY_MEM)
+		if (match & VGA_RSRC_LEGACY_MEM)
 			conflict->owns &= ~VGA_RSRC_NORMAL_MEM;
-		if (lwants & VGA_RSRC_LEGACY_IO)
+		if (match & VGA_RSRC_LEGACY_IO)
 			conflict->owns &= ~VGA_RSRC_NORMAL_IO;
 	}
 

[PATCH 2/2] vgaarb: Fix VGA decodes changes

[Alex Williamson]

When VGA decodes change we need to do a bit more evaluation of exactly what
has changed.  We don't necessarily give up all the old owns resources and
we need to account for resources with locks.  The new algorithm is: If
something is added, update decodes.  If legacy resources were added and
none were there before, we have a new participant.  If something is
removed, update decodes.  If we previously owned it, we no longer own it.
If it was previously locked, invalidate all locks and release it.  If
legacy resources were removed and none are left, remove the participant
from VGA arbitration.

Previously we updated decodes, released ownership of everything that was
previously decoded, ignored all locks, and went off looking for another
device to transfer VGA to.  In a test case where Intel IGD removes only
legacy VGA memory decoding, this left the arbiter switching to discrete
graphics without actually disabling legacy VGA IO from the IGD.  As a
bonus, we bumped up the count of VGA arbitration participants for no
good reason.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/vga/vgaarb.c |   40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index ea56471..4ee444c 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -644,10 +644,13 @@ bail:
 static inline void vga_update_device_decodes(struct vga_device *vgadev,
 					     int new_decodes)
 {
-	int old_decodes;
+	int old_decodes, decodes_removed, decodes_unlocked;
 	struct vga_device *new_vgadev, *conflict;
 
 	old_decodes = vgadev->decodes;
+	decodes_removed = ~new_decodes & old_decodes;
+	decodes_unlocked = vgadev->locks & decodes_removed;
+	vgadev->owns &= ~decodes_removed;
 	vgadev->decodes = new_decodes;
 
 	pr_info("vgaarb: device changed decodes: PCI:%s,olddecodes=%s,decodes=%s:owns=%s\n",
@@ -656,31 +659,22 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
 		vga_iostate_to_str(vgadev->decodes),
 		vga_iostate_to_str(vgadev->owns));
 
-
-	/* if we own the decodes we should move them along to
-	   another card */
-	if ((vgadev->owns & old_decodes) && (vga_count > 1)) {
-		/* set us to own nothing */
-		vgadev->owns &= ~old_decodes;
-		list_for_each_entry(new_vgadev, &vga_list, list) {
-			if ((new_vgadev != vgadev) &&
-			    (new_vgadev->decodes & VGA_RSRC_LEGACY_MASK)) {
-				pr_info("vgaarb: transferring owner from PCI:%s to PCI:%s\n", pci_name(vgadev->pdev), pci_name(new_vgadev->pdev));
-				conflict = __vga_tryget(new_vgadev, VGA_RSRC_LEGACY_MASK);
-				if (!conflict)
-					__vga_put(new_vgadev, VGA_RSRC_LEGACY_MASK);
-				break;
-			}
-		}
+	/* if we removed locked decodes, lock count goes to zero, and release */
+	if (decodes_unlocked) {
+		if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
+			vgadev->io_lock_cnt = 0;
+		if (decodes_unlocked & VGA_RSRC_LEGACY_MEM)
+			vgadev->mem_lock_cnt = 0;
+		__vga_put(vgadev, decodes_unlocked);
 	}
 
 	/* change decodes counter */
-	if (old_decodes != new_decodes) {
-		if (new_decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
-			vga_decode_count++;
-		else
-			vga_decode_count--;
-	}
+	if (old_decodes & VGA_RSRC_LEGACY_MASK &&
+	    !(new_decodes & VGA_RSRC_LEGACY_MASK))
+		vga_decode_count--;
+	if (!(old_decodes & VGA_RSRC_LEGACY_MASK) &&
+	    new_decodes & VGA_RSRC_LEGACY_MASK)
+		vga_decode_count++;
 	pr_debug("vgaarb: decoding count now is: %d\n", vga_decode_count);
 }
 

Re: [PATCH 0/2] vgaarb: Fixes for partial VGA opt-out

[Ville Syrjälä]

On Thu, Aug 15, 2013 at 04:37:47PM -0600, Alex Williamson wrote:
> I'm trying to add support for VGA arbitration on newer Intel graphics
> devices.  The existing code attempts to do this, but appear to have
> not been updated since GMCH devices roamed the Earth.  On newer
> devices like Haswell, we can disable VGA memory through an MSR on the
> device, but we rely on the VGA arbiter to manage VGA IO using the PCI
> COMMAND register.  In trying to unregister legacy VGA memory, I found
> that the VGA arbiter still wanted to disable both memory and IO on
> the device and that it forgot to actually program the device to
> disable IO when the decoding is updated.  This series attempts to fix
> both of those.  Thanks,

The series looks good to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Alex
> 
> ---
> 
> Alex Williamson (2):
>       vgaarb: Don't disable resources that are not owned
>       vgaarb: Fix VGA decodes changes
> 
> 
>  drivers/gpu/vga/vgaarb.c |   50 ++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 28 deletions(-)

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 0/2] vgaarb: Fixes for partial VGA opt-out

[Daniel Vetter]

On Fri, Aug 30, 2013 at 03:41:19PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 15, 2013 at 04:37:47PM -0600, Alex Williamson wrote:
> > I'm trying to add support for VGA arbitration on newer Intel graphics
> > devices.  The existing code attempts to do this, but appear to have
> > not been updated since GMCH devices roamed the Earth.  On newer
> > devices like Haswell, we can disable VGA memory through an MSR on the
> > device, but we rely on the VGA arbiter to manage VGA IO using the PCI
> > COMMAND register.  In trying to unregister legacy VGA memory, I found
> > that the VGA arbiter still wanted to disable both memory and IO on
> > the device and that it forgot to actually program the device to
> > disable IO when the decoding is updated.  This series attempts to fix
> > both of those.  Thanks,
> 
> The series looks good to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged to dinq with Dave's irc-ack. I'll shuffle my tree a bit so that
this will be part of my 3.12 latecomers pull request to Dave.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch