i915 | Bug in virtual PCH detection

Andrey Toloknev andreyhack at gmail.com
Thu Sep 5 04:35:15 UTC 2024 

Thanks, that's more universal and works like a charm.
Rewrite patch again. In attachment.
Will be nice *if* it will be committed.

ср, 4 сент. 2024 г. в 19:28, Jani Nikula <jani.nikula at linux.intel.com>:

> On Wed, 04 Sep 2024, Andrey Toloknev <andreyhack at gmail.com> wrote:
> > Sorry for replying twice.
> >
> > I thought about looking for the real PCH bridge, but I'm sure it will be
> a
> > real headache in some situations, configurations and virtualizations.
> > So, in my opinion, a better solution, as you noted in your first reply,
> is
> > modparam.
>
> *If* we were to add a module parameter for this, it should be more
> generic rather than forcing a single case. For example:
>
> diff --git a/drivers/gpu/drm/i915/soc/intel_pch.c
> b/drivers/gpu/drm/i915/soc/intel_pch.c
> index 542eea50093c..d76b05545308 100644
> --- a/drivers/gpu/drm/i915/soc/intel_pch.c
> +++ b/drivers/gpu/drm/i915/soc/intel_pch.c
> @@ -168,7 +168,9 @@ intel_virt_detect_pch(const struct drm_i915_private
> *dev_priv,
>          * make an educated guess as to which PCH is really there.
>          */
>
> -       if (IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv))
> +       if (dev_priv->params.virt_pch_id)
> +               id = dev_priv->params.virt_pch_id;
> +       else if (IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv))
>                 id = INTEL_PCH_ADP_DEVICE_ID_TYPE;
>         else if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv))
>                 id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
>
> That lets you pass in any PCH device id via i915.virt_pch_id=<id>, but
> it still checks the value in intel_pch_type(), fails on unknown ones,
> and warns about unexpected combos.
>
> See drivers/gpu/drm/i915/soc/intel_pch.h for the
> INTEL_PCH_*_DEVICE_ID_TYPE macros for possible values.
>
> BR,
> Jani.
>
>
>
> >
> > ср, 4 сент. 2024 г. в 18:00, Andrey Toloknev <andreyhack at gmail.com>:
> >
> >> Hmm. I wonder how many systems we'd break if we make it look
> >>> through all the bridges for a real match first, and only fall
> >>> back to intel_virt_detect_pch() if nothing was found...
> >>
> >>
> >> Yes, I definitely understand this, that's why I didn't touch this code
> at
> >> all in the second patch.
> >> I just add bool modparam force_tgp_vpch in i915_params and a bit modify
> >> method intel_virt_detect_pch() in intel_pch.c
> >>
> >>
> >>
> >> ср, 4 сент. 2024 г. в 17:52, Ville Syrjälä <
> ville.syrjala at linux.intel.com
> >> >:
> >>
> >>> On Wed, Sep 04, 2024 at 05:25:06PM +0500, Andrey Toloknev wrote:
> >>> > Hello!
> >>> >
> >>> > Thanks for your reply, Ville.
> >>> >
> >>> > I looked at the code again and understood you are definitely right
> about
> >>> > breaking other combinations of CPU+PCH with using IS_GEN9_BC in my
> >>> patch.
> >>> >
> >>> > Using libvirt (kvm) I can passthrough ISA/LPC bridge to VM, but the
> >>> problem
> >>> > is connected with method intel_detect_pch(). It searches only for the
> >>> first
> >>> > ISA Bridge.
> >>>
> >>> Hmm. I wonder how many systems we'd break if we make it look
> >>> through all the bridges for a real match first, and only fall
> >>> back to intel_virt_detect_pch() if nothing was found...
> >>>
> >>> > And the virtual ISA/LPC Bridge PCI in libvirt is hardcoded to address
> >>> > 00:01.0 - it's always first.
> >>> > So, method intel_detect_pch() correctly detects that the first
> bridge is
> >>> > virtual and then calls method intel_virt_detect_pch(), which just
> checks
> >>> > the iGPU platform and doesn't take into account the possible
> >>> combination of
> >>> > Comet Lake CPU and Tiger Lake PCH.
> >>> >
> >>> > Of course, It would be nice if we can have a universal modparam to
> >>> specify
> >>> > PCH id by hand in future.
> >>> > But as a fast fix of that small bug I think one more bool modparam
> may
> >>> be
> >>> > enough.
> >>> > I wrote the second version of patch which adds that bool modparam -
> >>> > force_tgp_vpch. It's false by default.
> >>> > When this param is true, we also check that the CPU is Comet Lake and
> >>> then
> >>> > set PCH type as Tiger Lake in the method intel_virt_detect_pch().
> >>> >
> >>> > The second version of patch is in attachment.
> >>> >
> >>> >
> >>> > вт, 3 сент. 2024 г. в 18:38, Ville Syrjälä <
> >>> ville.syrjala at linux.intel.com>:
> >>> >
> >>> > > On Sun, Sep 01, 2024 at 02:56:07PM +0500, Andrey Toloknev wrote:
> >>> > > > Hello!
> >>> > > >
> >>> > > > I have 2 machines with Comet Lake CPUs on Tiger Lake PCH (500
> >>> series of
> >>> > > > Intel chipsets).
> >>> > > > For that configuration there was a patch for adding support for
> >>> Tiger
> >>> > > Lake
> >>> > > > PCH with CometLake CPU in 2021 -
> >>> > > > https://patchwork.freedesktop.org/patch/412664/
> >>> > > > This patch made possible correct detection of such chipset and
> cpu
> >>> > > > configuration for i915 kernel module. Without it there was no
> >>> output to
> >>> > > any
> >>> > > > display (HDMI/DP/DVI, even VGA).
> >>> > > >
> >>> > > > But this patch doesn't touch intel_virt_detect_pch method, when
> you
> >>> > > > passthrough iGPU to a virtual machine.
> >>> > > > So, virtual PCH incorrectly detects as Cannon Lake and you have
> no
> >>> output
> >>> > > > to a physical display with i915 driver:
> >>> > > >
> >>> > > > [    2.933139] i915 0000:00:02.0: [drm:intel_virt_detect_pch
> [i915]]
> >>> > > > Assuming PCH ID a300
> >>> > > > [    2.933308] i915 0000:00:02.0: [drm:intel_pch_type [i915]]
> Found
> >>> > > Cannon
> >>> > > > Lake PCH (CNP)
> >>> > > >
> >>> > > >
> >>> > > > The bug is on line 173 in drivers/gpu/drm/i915/soc/intel_pch.c in
> >>> method
> >>> > > > intel_virt_detect_pch:
> >>> > > >
> >>> > > > else if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv))
> >>> > > >
> >>> > > > id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
> >>> > > >
> >>> > > > It must be:
> >>> > > >
> >>> > > > else if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> >>> > > > IS_GEN9_BC(dev_priv))
> >>> > > >
> >>> > > > id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
> >>> > > >
> >>> > > >
> >>> > > > After that small change you get correct detection of PCH and have
> >>> output
> >>> > > to
> >>> > > > a physical display in VM with passthrough iGPU:
> >>> > > >
> >>> > > > [   16.139809] i915 0000:00:02.0: [drm:intel_virt_detect_pch
> [i915]]
> >>> > > > Assuming PCH ID a080
> >>> > > > [   16.261151] i915 0000:00:02.0: [drm:intel_pch_type [i915]]
> Found
> >>> Tiger
> >>> > > > Lake LP PCH
> >>> > > >
> >>> > > >
> >>> > > > All kernel versions in any distro since 2021 are affected by this
> >>> small
> >>> > > bug.
> >>> > > > The patch for i915 module of the actual kernel version is in
> >>> attachment.
> >>> > >
> >>> > > You fix one CPU+PCH combo, but break the other. I don't think
> there is
> >>> > > any way to handle this mess in intel_virt_detect_pch(). The best
> thing
> >>> > > would be if the virtual machine would advertise the correct ISA/LPC
> >>> > > bridge, then the heiristic is not even invoked. If that's not
> possible
> >>> > > for some reason then I suppose we'd need a modparam/etc. so the
> user
> >>> > > can specify the PCH ID by hand.
> >>> > >
> >>> > > --
> >>> > > Ville Syrjälä
> >>> > > Intel
> >>> > >
> >>> >
> >>> >
> >>> > --
> >>> > Best regards, Andrey Toloknev
> >>>
> >>>
> >>>
> >>> --
> >>> Ville Syrjälä
> >>> Intel
> >>>
> >>
> >>
> >> --
> >> Best regards, Andrey Toloknev
> >>
>
> --
> Jani Nikula, Intel
>


-- 
Best regards, Andrey Toloknev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20240905/1041e7db/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tgl_vpch_fix_v3.patch
Type: application/x-patch
Size: 1954 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20240905/1041e7db/attachment-0001.bin>

More information about the Intel-gfx mailing list