<div dir="ltr"><div dir="ltr"><div dir="ltr">Thanks, that's more universal and works like a charm. </div><div dir="ltr">Rewrite patch again. In attachment.<div>Will be nice *if* it will be committed. </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">ср, 4 сент. 2024 г. в 19:28, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com" target="_blank">jani.nikula@linux.intel.com</a>>: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 04 Sep 2024, Andrey Toloknev <<a href="mailto:andreyhack@gmail.com" target="_blank">andreyhack@gmail.com</a>> 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 <<a href="mailto:andreyhack@gmail.com" target="_blank">andreyhack@gmail.com</a>>: > >> 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ä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a> >> >: >> >>> 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ä < >>> <a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>>: >>> > >>> > > 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 - >>> > > > <a href="https://patchwork.freedesktop.org/patch/412664/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/412664/</a> >>> > > > 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 </blockquote></div> </div><div> </div>-- <div dir="ltr" class="gmail_signature">Best regards, Andrey Toloknev</div> </div>