[Intel-gfx] [PATCH] Fix intel_detect_pch() to work in xen environment.

[G.R.]

On Wed, Dec 19, 2012 at 2:20 AM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Wed, 19 Dec 2012 01:43:22 +0800
> "G.R." <firemeteor at users.sourceforge.net> wrote:
>
>> On Wed, Dec 19, 2012 at 12:53 AM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
>> > On Wed, 19 Dec 2012 00:49:28 +0800
>> > "G.R." <firemeteor at users.sourceforge.net> wrote:
>> >
>> >> 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
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> >> index 530db83..3f7e5fb 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.c
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> >> @@ -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");
>> >>         }
>> >>  }
>> >>
>> >
>> > 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          */
>>
>> > 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.
>
> 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 :-)

> --
> Jesse Barnes, Intel Open Source Technology Center