i915 | Bug in virtual PCH detection

i915 | Bug in virtual PCH detection

[Andrey Toloknev]

[-- Attachment #1.1: Type: text/plain, Size: 1651 bytes --]

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.

-- 
Best regards, Andrey Toloknev

[-- Attachment #1.2: Type: text/html, Size: 2868 bytes --]

[-- Attachment #2: tgl_vpch_fix.patch --]
[-- Type: application/octet-stream, Size: 566 bytes --]

--- a/drivers/gpu/drm/i915/soc/intel_pch.c	2024-09-01 14:20:44.470847098 +0500
+++ b/drivers/gpu/drm/i915/soc/intel_pch.c	2024-09-01 14:29:46.695998586 +0500
@@ -170,7 +170,7 @@
 
 	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))
+	else if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) || IS_GEN9_BC(dev_priv))
 		id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
 	else if (IS_JASPERLAKE(dev_priv) || IS_ELKHARTLAKE(dev_priv))
 		id = INTEL_PCH_MCC_DEVICE_ID_TYPE;

Re: i915 | Bug in virtual PCH detection

[Ville Syrjälä]

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

Re: i915 | Bug in virtual PCH detection

[Andrey Toloknev]

[-- Attachment #1.1: Type: text/plain, Size: 3715 bytes --]

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.
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

[-- Attachment #1.2: Type: text/html, Size: 4728 bytes --]

[-- Attachment #2: tgl_vpch_fix_v2.patch --]
[-- Type: application/octet-stream, Size: 2092 bytes --]

diff -ur a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
--- a/drivers/gpu/drm/i915/i915_params.c	2024-09-04 13:59:20.626309515 +0500
+++ b/drivers/gpu/drm/i915/i915_params.c	2024-09-04 14:51:02.539226342 +0500
@@ -136,6 +136,9 @@
 		 "Enable support for unstable debug only userspace API. (default:false)");
 #endif
 
+i915_param_named_unsafe(force_tgp_vpch, bool, 0400,
+        "Enable force Tiger Point PCH Detection with Comet Lake CPU on virtual machines (default: false)");
+
 static void _param_print_bool(struct drm_printer *p, const char *name,
 			      bool val)
 {
diff -ur a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
--- a/drivers/gpu/drm/i915/i915_params.h	2024-09-04 13:59:29.005345962 +0500
+++ b/drivers/gpu/drm/i915/i915_params.h	2024-09-04 14:47:15.005895085 +0500
@@ -64,7 +64,8 @@
 	param(bool, enable_hangcheck, true, 0600) \
 	param(bool, error_capture, true, IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) ? 0600 : 0) \
 	param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ? 0400 : 0) \
-	param(bool, enable_debug_only_api, false, IS_ENABLED(CONFIG_DRM_I915_REPLAY_GPU_HANGS_API) ? 0400 : 0)
+	param(bool, enable_debug_only_api, false, IS_ENABLED(CONFIG_DRM_I915_REPLAY_GPU_HANGS_API) ? 0400 : 0) \
+	param(bool, force_tgp_vpch, false, 0400)
 
 #define MEMBER(T, member, ...) T member;
 struct i915_params {
diff -ur a/drivers/gpu/drm/i915/soc/intel_pch.c b/drivers/gpu/drm/i915/soc/intel_pch.c
--- a/drivers/gpu/drm/i915/soc/intel_pch.c	2024-09-04 14:07:30.274908882 +0500
+++ b/drivers/gpu/drm/i915/soc/intel_pch.c	2024-09-04 14:07:48.548895055 +0500
@@ -170,7 +170,8 @@
 
 	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))
+	else if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
+		 (IS_COMETLAKE(dev_priv) && dev_priv->params.force_tgp_vpch))
 		id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
 	else if (IS_JASPERLAKE(dev_priv) || IS_ELKHARTLAKE(dev_priv))
 		id = INTEL_PCH_MCC_DEVICE_ID_TYPE;

Re: i915 | Bug in virtual PCH detection

[Ville Syrjälä]

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

Re: i915 | Bug in virtual PCH detection

[Andrey Toloknev]

[-- Attachment #1: Type: text/plain, Size: 5035 bytes --]

>
> 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
>


-- 
С Уважением, Толокнев Андрей

[-- Attachment #2: Type: text/html, Size: 6682 bytes --]

Re: i915 | Bug in virtual PCH detection

[Andrey Toloknev]

[-- Attachment #1: Type: text/plain, Size: 5566 bytes --]

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.

ср, 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
>


-- 
Best regards, Andrey Toloknev

[-- Attachment #2: Type: text/html, Size: 7590 bytes --]

Re: i915 | Bug in virtual PCH detection

[Jani Nikula]

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

✗ Fi.CI.BUILD: failure for i915 | Bug in virtual PCH detection

[Patchwork]

== Series Details ==

Series: i915 | Bug in virtual PCH detection
URL   : https://patchwork.freedesktop.org/series/138213/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/soc/intel_pch.o
drivers/gpu/drm/i915/soc/intel_pch.c: In function ‘intel_virt_detect_pch’:
drivers/gpu/drm/i915/soc/intel_pch.c:171:22: error: ‘const struct i915_params’ has no member named ‘virt_pch_id’
  171 |  if (dev_priv->params.virt_pch_id)
      |                      ^
drivers/gpu/drm/i915/soc/intel_pch.c:172:24: error: ‘const struct i915_params’ has no member named ‘virt_pch_id’
  172 |   id = dev_priv->params.virt_pch_id;
      |                        ^
make[6]: *** [scripts/Makefile.build:244: drivers/gpu/drm/i915/soc/intel_pch.o] Error 1
make[5]: *** [scripts/Makefile.build:485: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:485: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:485: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/home/kbuild2/kernel/Makefile:1925: .] Error 2
make: *** [Makefile:224: __sub-make] Error 2
Build failed, no error log produced

Re: i915 | Bug in virtual PCH detection

[Andrey Toloknev]

[-- Attachment #1.1: Type: text/plain, Size: 7726 bytes --]

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@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
>


-- 
Best regards, Andrey Toloknev

[-- Attachment #1.2: Type: text/html, Size: 11021 bytes --]

[-- Attachment #2: tgl_vpch_fix_v3.patch --]
[-- Type: application/x-patch, Size: 1954 bytes --]