Re: i915 | Bug in virtual PCH detection

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 04 Sep 2024, Andrey Toloknev <andreyhack@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@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@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
>>>
>>
>>
>> --
>> Best regards, Andrey Toloknev
>>

-- 
Jani Nikula, Intel