Fix intel_detect_pch() to work in xen environment.

PATCH In review G.R. 5 participants 16 messages Dec 18, 2012 – Jun 20, 2013

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

G.R.

author

2012-12-18 16:49 UTC #0 lore raw

PATCH 1/1 Fix intel_detect_pch() to work in xen environment.

Hi guys,

In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.

Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.

Thanks,
Timothy

drivers/gpu/drm/i915/i915_drv.c +15 -3

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

Jesse Barnes

2012-12-18 16:53 UTC #1 lore raw

Quoted 45 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…

Hi guys,

Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.

Thanks,
Timothy

drivers/gpu/drm/i915/i915_drv.c +10 -2

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);

drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 461

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.c +5 -1

                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.

Ack on the general idea though; I'd like us to be able to run under Xen without modification.

Jesse

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

G.R.

author

2012-12-18 17:43 UTC #2 lore raw

Quoted 55 lines · Jesse Barnes

Quoted 52 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…

Hi guys,

Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.

Thanks,
Timothy

drivers/gpu/drm/i915/i915_drv.c +15 -3

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");

Quoted 9 lines

drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 461
Quoted 3 lines
drivers/gpu/drm/i915/i915_drv.c +0 -0
        }
 }
 
I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.

I can add a comment here if the overall idea is acceptable to you. But there is already a comment mentioning that the ISA bridge check is for virtualization:

404 /*

 405          * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 406          * make graphics device passthrough work easy for VMM, that only
 407          * need to expose ISA bridge to let driver know the real hardware
 408          * underneath. This is a requirement from virtualization team.
 409          */

Quoted 3 lines

Ack on the general idea though; I'd like us to be able to run under Xen without modification.

Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.

But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.

Quoted 4 lines

Jesse
--
Jesse Barnes, Intel Open Source Technology Center

Jesse Barnes

2012-12-18 18:19 UTC #3 lore raw

Quoted 81 lines · On Wed, 19 Dec 2012 01:43:22 +0800 "G.R." <[email protected]>…

Quoted 55 lines · Jesse Barnes

Quoted 52 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…

Hi guys,

Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.

Thanks,
Timothy

drivers/gpu/drm/i915/i915_drv.c +15 -3

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");

Quoted 9 lines

drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 461
Quoted 3 lines
drivers/gpu/drm/i915/i915_drv.c +0 -0
        }
 }
 
I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.

I can add a comment here if the overall idea is acceptable to you. But there is already a comment mentioning that the ISA bridge check is for virtualization:

404 /*

 405          * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 406          * make graphics device passthrough work easy for VMM, that only
 407          * need to expose ISA bridge to let driver know the real hardware
 408          * underneath. This is a requirement from virtualization team.
 409          */

Quoted 3 lines

Ack on the general idea though; I'd like us to be able to run under Xen without modification.

Quoted 8 lines

Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.
But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.

I meant that I'd like to see any other patches required for Xen get merged, that way people won't have to patch their kernels for i915 under Xen.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

G.R.

author

2012-12-19 03:40 UTC #4 lore raw

Quoted 89 lines · Jesse Barnes

Quoted 86 lines · On Wed, 19 Dec 2012 01:43:22 +0800 "G.R." <[email protected]>…
Quoted 55 lines · Jesse Barnes
Quoted 52 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…
Hi guys,
In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.
Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.
Thanks,
Timothy
drivers/gpu/drm/i915/i915_drv.c +15 -3
index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;
 
@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
Quoted 9 lines
drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 461
Quoted 3 lines
drivers/gpu/drm/i915/i915_drv.c +0 -0
        }
 }
 
I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.
I can add a comment here if the overall idea is acceptable to you. But there is already a comment mentioning that the ISA bridge check is for virtualization:
404 /*
 405          * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 406          * make graphics device passthrough work easy for VMM, that only
 407          * need to expose ISA bridge to let driver know the real hardware
 408          * underneath. This is a requirement from virtualization team.
 409          */
Quoted 3 lines
Ack on the general idea though; I'd like us to be able to run under Xen without modification.
Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.

Quoted 8 lines

Quoted 3 lines
But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.
I meant that I'd like to see any other patches required for Xen get merged, that way people won't have to patch their kernels for i915 under Xen.

Sorry for misreading your comment :-)

Quoted 2 lines

--
Jesse Barnes, Intel Open Source Technology Center

G.R.

author

2012-12-20 04:04 UTC #5 lore raw

In reply to Jesse Barnes

Quoted 27 lines · Jesse Barnes

Quoted 27 lines
Quoted 6 lines
I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.
I can add a comment here if the overall idea is acceptable to you. But there is already a comment mentioning that the ISA bridge check is for virtualization:
404 /*
 405          * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 406          * make graphics device passthrough work easy for VMM, that only
 407          * need to expose ISA bridge to let driver know the real hardware
 408          * underneath. This is a requirement from virtualization team.
 409          */
Quoted 3 lines
Ack on the general idea though; I'd like us to be able to run under Xen without modification.
Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.

Quoted 7 lines

Quoted 3 lines
But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.
I meant that I'd like to see any other patches required for Xen get merged, that way people won't have to patch their kernels for i915 under Xen.

Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?

Jesse Barnes

2012-12-20 16:13 UTC #6 lore raw

Quoted 32 lines · On Thu, 20 Dec 2012 12:04:11 +0800 "G.R." <[email protected]>…

Quoted 31 lines · Jesse Barnes
Quoted 30 lines
Quoted 6 lines
I'd like to see a comment about this being for Xen in here, and I wonder if there are other places where we might have to worry about the Xen implementation. In that case, setting a flag in dev_priv when we don't find the PCH where we expect would be a good idea too.
I can add a comment here if the overall idea is acceptable to you. But there is already a comment mentioning that the ISA bridge check is for virtualization:
404 /*
 405          * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 406          * make graphics device passthrough work easy for VMM, that only
 407          * need to expose ISA bridge to let driver know the real hardware
 408          * underneath. This is a requirement from virtualization team.
 409          */
Quoted 3 lines
Ack on the general idea though; I'd like us to be able to run under Xen without modification.
Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.
But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.

Quoted 8 lines

Quoted 3 lines
I meant that I'd like to see any other patches required for Xen get merged, that way people won't have to patch their kernels for i915 under Xen.
Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?

No, just cc Daniel Vetter.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

G.R.

author

2012-12-23 06:51 UTC #7 lore raw

Quoted 22 lines

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 3 lines
Ack on the general idea though; I'd like us to be able to run under Xen without modification.
Stefano may be able to comment if it's feasible to achieve zero modification in this case.
I believe this has something to do with getting rid of the PIIX3 device provided by qemu.
But generally I think it's very hard to achieve perfect emulation. You can't always foresee what assumption a guest driver would make. Maybe we need some compromise.
Quoted 8 lines
Quoted 3 lines
I meant that I'd like to see any other patches required for Xen get merged, that way people won't have to patch their kernels for i915 under Xen.
Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?

Quoted 4 lines

No, just cc Daniel Vetter.
--
Jesse Barnes, Intel Open Source Technology Center

Thanks,
Resend with updated patch && Daniel involved. Also include the background info for easy reading.

Against torvalds tree 3.8-rc1 (a49f0d1)

drivers/gpu/drm/i915/i915_drv.c +20 -3

index 530db83..8f949c1 100644
@@ -406,11 +406,18 @@ void intel_detect_pch(struct drm_device *dev)
         * make graphics device passthrough work easy for VMM, that only
         * need to expose ISA bridge to let driver know the real hardware
         * underneath. This is a requirement from virtualization team.
+        *
+        * In some virtualized environments (e.g. XEN), there is irrelevant
+        * ISA bridge in the system. To work reliably, we should scan trhough
+        * all the ISA bridge devices and check for the first match, instead
+        * of only checking the first one.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +447,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

G.R.

author

2013-06-18 12:54 UTC #8 lore raw

Quoted 14 lines · G.R.

Quoted 9 lines
Quoted 4 lines
Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?
No, just cc Daniel Vetter.
--
Jesse Barnes, Intel Open Source Technology Center
Thanks,
Resend with updated patch && Daniel involved. Also include the background info for easy reading.

Quoted 7 lines

In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.

This seems to have lost in the list. Retry it again with latest torvalds' kernel:

drivers/gpu/drm/i915/i915_drv.c +20 -3

index a2e4953..e38fa86 100644
@@ -454,11 +454,18 @@ void intel_detect_pch(struct drm_device *dev)
         * make graphics device passthrough work easy for VMM, that only
         * need to expose ISA bridge to let driver know the real hardware
         * underneath. This is a requirement from virtualization team.
+        *
+        * In some virtualized environments (e.g. XEN), there is irrelevant
+        * ISA bridge in the system. To work reliably, we should scan trhough
+        * all the ISA bridge devices and check for the first match, instead
+        * of only checking the first one.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -490,10 +497,20 @@ void intel_detect_pch(struct drm_device *dev)
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
                                WARN_ON(!IS_ULT(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

Daniel Vetter

2013-06-19 06:01 UTC #9 lore raw

Can you please resubmit this as a proper patch with a committ message, signed-off-by line and everything else (see Documentation/SubmittingPatches).
-Daniel

Quoted 72 lines · G.R.

Quoted 14 lines · G.R.

Quoted 9 lines
Quoted 4 lines
Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?
No, just cc Daniel Vetter.
--
Jesse Barnes, Intel Open Source Technology Center
Thanks,
Resend with updated patch && Daniel involved. Also include the background info for easy reading.

Quoted 7 lines

In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.

This seems to have lost in the list. Retry it again with latest torvalds' kernel:

drivers/gpu/drm/i915/i915_drv.c +20 -3

index a2e4953..e38fa86 100644
@@ -454,11 +454,18 @@ void intel_detect_pch(struct drm_device *dev)
         * make graphics device passthrough work easy for VMM, that only
         * need to expose ISA bridge to let driver know the real hardware
         * underneath. This is a requirement from virtualization team.
+        *
+        * In some virtualized environments (e.g. XEN), there is irrelevant
+        * ISA bridge in the system. To work reliably, we should scan trhough
+        * all the ISA bridge devices and check for the first match, instead
+        * of only checking the first one.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -490,10 +497,20 @@ void intel_detect_pch(struct drm_device *dev)
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
                                WARN_ON(!IS_ULT(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Rui Guo

author

2013-06-19 13:11 UTC #10 lore raw

PATCH 1/1 Fix PCH detect with multiple ISA bridges in VM

In some virtualized environments (e.g. XEN), there is irrelevant ISA bridge in the system. To work reliably, we should scan trhough all the ISA bridge devices and check for the first match, instead of only checking the first one.

Signed-off-by Rui Guo <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +17 -2

index a2e4953..10316dc 100644
@@ -454,9 +454,15 @@ void intel_detect_pch(struct drm_device *dev)
 	 * make graphics device passthrough work easy for VMM, that only
 	 * need to expose ISA bridge to let driver know the real hardware
 	 * underneath. This is a requirement from virtualization team.
+	 *
+	 * In some virtualized environments (e.g. XEN), there is irrelevant
+	 * ISA bridge in the system. To work reliably, we should scan trhough
+	 * all the ISA bridge devices and check for the first match, instead
+	 * of only checking the first one.
 	 */
 	pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-	if (pch) {
+	while (pch) {
+		struct pci_dev *curr = pch;
 		if (pch->vendor == PCI_VENDOR_ID_INTEL) {
 			unsigned short id;
 			id = pch->device & INTEL_PCH_DEVICE_ID_MASK;

@@ -490,11 +496,20 @@ void intel_detect_pch(struct drm_device *dev)
 				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
 				WARN_ON(!IS_HASWELL(dev));
 				WARN_ON(!IS_ULT(dev));
+			} else {
+				goto check_next;
 			}
+
 			BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+			pci_dev_put(pch);
+			break;
 		}
-		pci_dev_put(pch);
+check_next:
+		pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+		pci_dev_put(curr);
 	}
+	if (!pch)
+		DRM_DEBUG_KMS("No PCH found?\n");
 }
 
 bool i915_semaphore_is_enabled(struct drm_device *dev)

Signature · 1 line

1.7.10.4

G.R.

author

2013-06-19 15:29 UTC #11 lore raw

In reply to Daniel Vetter

Quoted 5 lines · Daniel Vetter

Can you please resubmit this as a proper patch with a committ message, signed-off-by line and everything else (see Documentation/SubmittingPatches).
-Daniel

Done, sent in a separate thread:
http://lists.freedesktop.org/archives/intel-gfx/2013-June/029161.html

Daniel Vetter

2013-06-20 12:01 UTC #12 lore raw

Quoted 8 lines · G.R.

Quoted 4 lines · Daniel Vetter
Can you please resubmit this as a proper patch with a committ message, signed-off-by line and everything else (see Documentation/SubmittingPatches).
-Daniel
Done, sent in a separate thread:
http://lists.freedesktop.org/archives/intel-gfx/2013-June/029161.html

Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Ville Syrjälä

2013-06-19 07:57 UTC #13 lore raw

In reply to G.R.

Quoted 35 lines · G.R.

Quoted 14 lines · G.R.
Quoted 9 lines
Quoted 4 lines
Hi Jesse, I think I need to resend the patch with proper comment to have it formally accepted.
Any guide line for formal patch submission? Do I need to start a separate thread?
No, just cc Daniel Vetter.
--
Jesse Barnes, Intel Open Source Technology Center
Thanks,
Resend with updated patch && Daniel involved. Also include the background info for easy reading.
Quoted 7 lines
In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.
This seems to have lost in the list. Retry it again with latest torvalds' kernel:
drivers/gpu/drm/i915/i915_drv.c +0 -0
index a2e4953..e38fa86 100644
@@ -454,11 +454,18 @@ void intel_detect_pch(struct drm_device *dev)
         * make graphics device passthrough work easy for VMM, that only
         * need to expose ISA bridge to let driver know the real hardware
         * underneath. This is a requirement from virtualization team.

drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 465

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.c +7 -1

+        *
+        * In some virtualized environments (e.g. XEN), there is irrelevant
+        * ISA bridge in the system. To work reliably, we should scan trhough
+        * all the ISA bridge devices and check for the first match, instead
+        * of only checking the first one.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;

No space there. I think checkpatch should catch that.

drivers/gpu/drm/i915/i915_drv.c void intel_detect_pch(struct drm_device *dev) · line 468

Quoted 3 lines

drivers/gpu/drm/i915/i915_drv.c +1 -0

                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;

bool found = true;

Although it would be easy to adjust the patch to not need a 'found' variable.

Quoted 29 lines

drivers/gpu/drm/i915/i915_drv.c +12 -2

                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -490,10 +497,20 @@ void intel_detect_pch(struct drm_device *dev)
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
                                WARN_ON(!IS_ULT(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Ben Widawsky

2012-12-18 17:21 UTC #14 lore raw

In reply to G.R.

Quoted 9 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…

Hi guys,
In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.

Quoted 5 lines

Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.
Thanks,
Timothy

Is this functionally equivalent to
http://lists.freedesktop.org/archives/intel-gfx/2012-July/019126.html?

Quoted 44 lines

drivers/gpu/drm/i915/i915_drv.c +15 -3

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

G.R.

author

2012-12-19 04:01 UTC #15 lore raw

Quoted 12 lines · Ben Widawsky

Quoted 9 lines · On Wed, 19 Dec 2012 00:49:28 +0800 "G.R." <[email protected]>…
Hi guys,
In XEN HVM guest, there is always an emulated PIIX3 ISA bridge on slot 01.0. This shadows the PCH ISA bridge on 1f.0 with the current intel_detect_pch() implementation.
The issue can be easily solved by looping through all the ISA bridges until the first match is found, instead of just check against the first one.

Quoted 9 lines

Quoted 5 lines
Here I attach the patch I used locally. It's created on Torvalds's git. Looking forward to your comments.
Thanks,
Timothy
Is this functionally equivalent to
http://lists.freedesktop.org/archives/intel-gfx/2012-July/019126.html?

I don't think so.
The patch you mentioned tries to detect the VM.
The info could be used enables any required workaround later. Jesse also expressed the same concern in his mail. The problem is that there is no standard way to detect a VM. The patch you mentioned should only work for XEN HVM config.

My patch tries to correctly detect the PCH type in a IGD passthrough VM. This enables PCH specific code path in the driver.
It really fixes some failure related to gaming / video play back with VAAPI etc in such a config.
(But it's not a cure-all solution of course, since this is specific to linux driver.
And there may still be other some xen specific issues to triage. For example, my linux box almost work great now, but win7 still keeping BSOD and I even have no idea how to debug)

I think it is feasible to combine the two ideas together if you are willing to provide VM specific support in your driver. But as I mentioned before, the problem is we may still need to do it case by case...

Quoted 49 lines

Quoted 40 lines

drivers/gpu/drm/i915/i915_drv.c +15 -3

index 530db83..3f7e5fb 100644
@@ -408,9 +408,11 @@ void intel_detect_pch(struct drm_device *dev)
         * underneath. This is a requirement from virtualization team.
         */
        pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-       if (pch) {
+       while (pch) {
+               struct pci_dev * curr = pch;
                if (pch->vendor == PCI_VENDOR_ID_INTEL) {
                        unsigned short id;
+                       unsigned found = 1;
                        id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
                        dev_priv->pch_id = id;

@@ -440,10 +442,20 @@ void intel_detect_pch(struct drm_device *dev)
                                dev_priv->num_pch_pll = 0;
                                DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
                                WARN_ON(!IS_HASWELL(dev));
+                       } else {
+                               found = 0;
+                       }
+                       if (found) {
+                               BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
+                               pci_dev_put(pch);
+                               break;
                        }
-                       BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
                }
-               pci_dev_put(pch);
+               pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
+               pci_dev_put(curr);
+       }
+       if (!pch) {
+               DRM_DEBUG_KMS("No PCH found?\n");
        }
 }

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ben Widawsky, Intel Open Source Technology Center