Re: i915 | Bug in virtual PCH detection

["Ville Syrjälä" <ville.syrjala@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@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