>>>> >-----Original Message----- >>>> >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On >>>> >Behalf Of Lucas De Marchi >>>> >Sent: Thursday, August 15, 2019 5:25 AM >>>> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>> >Subject: [PATCH] drm/i915/tgl: disable DDIC >>>> > >>>> >The current SKUs added for Tiger Lake don't have DDIC hooked up, >>>> >even though it is supported by the SoC. The current state for these >>>> >SKUs is problematic since while enabling the combo phy, >>>> >PORT_COMP_DW* return 0xFFFFFFFF, which is invalid per register definition. >>>> > >>>> >During initialization we check what phys are not yet enabled by >>>> >reading PHY_MISC_C and try to enable it by toggling the "DE to IO >>>> >Comp Pwr >>>Down" >>>> >bit. But after that any read to the PORT_COMP_DW* returns invalid >>>> >results. This removes the following warning >>>> > >>>> >[56997.634353] Missing case (val == 4294967295) [56997.639241] >>>> >WARNING: CPU: 5 >>>> >PID: 768 at drivers/gpu/drm/i915/display/intel_combo_phy.c:54 >>>> >cnl_get_procmon_ref_values+0xc9/0xf0 [i915] [56997.639808] Modules linked >in: >>>> >i915(+) prime_numbers x86_pkg_temp_thermal coretemp kvm_intel kvm >>>> >irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel e1000e >>>> >[last >>>unloaded: >>>> >prime_numbers] >>>> >[56997.639808] CPU: 5 PID: 768 Comm: insmod Tainted: G U W 5.2.0- >>>> >demarchi+ #65 >>>> >[56997.639808] Hardware name: Intel Corporation Tiger Lake Client >>>> >Platform/TigerLake U DDR4 SODIMM RVP, BIOS >>>> >TGLSFWI1.R00.2252.A03.1906270154 06/27/2019 [56997.639808] RIP: >>>> >0010:cnl_get_procmon_ref_values+0xc9/0xf0 [i915] [56997.639808] Code: >>>> >2c a0 85 >>>> >c9 74 e0 81 f9 00 00 00 01 75 09 48 c7 c0 0c a4 2c a0 eb cf 48 c7 >>>> >c6 3c 3a 31 a0 48 c7 >>>> >c7 40 3a 31 a0 e8 6b 4d ea e0 <0f> 0b 48 c7 c0 00 a4 2c a0 eb b1 48 >>>> >c7 c0 24 a4 2 c a0 eb a8 e8 be [56997.639808] RSP: >>>> >0018:ffffc9000068f8a8 EFLAGS: 00010286 [56997.639808] RAX: >>>0000000000000000 RBX: ffff88848fa90000 RCX: >>>> >0000000000000000 [56997.639808] RDX: ffff8884a08b5ef8 RSI: >>>> >ffff8884a08a6658 >>>> >RDI: 00000000ffffffff [56997.639808] RBP: 0000000000000002 R08: >>>> >0000000000000000 R09: 0000000000000000 [56997.639808] R10: >>>> >0000000000000000 R11: 0000000000000000 R12: ffff88848fa90000 >>>> >[56997.639808] >>>> >R13: 0000000000000000 R14: 0000000000000002 R15: 0006c00000162000 >>>> >[56997.639808] FS: 00007f61ca3d12c0(0000) >>>> >GS:ffff8884a0880000(0000) >>>> >knlGS:0000000000000000 [56997.639808] CS: 0010 DS: 0000 ES: 0000 CR0: >>>> >0000000080050033 [56997.639808] CR2: 00007f71be6a92c0 CR3: >>>> >0000000494750006 CR4: 0000000000760ee0 [56997.639808] PKRU: >>>> >55555554 [56997.639808] Call Trace: >>>> >[56997.639808] cnl_verify_procmon_ref_values+0x36/0xf0 [i915] >>>> >[56997.639808] >>>? >>>> >rcu_read_lock_sched_held+0x6f/0x80 >>>> >[56997.639808] ? gen11_fwtable_read32+0x257/0x290 [i915] >>>> >[56997.639808] >>>> >icl_combo_phy_verify_state.part.0+0x22/0xa0 [i915] [56997.639808] >>>> >intel_combo_phy_init+0x17e/0x3e0 [i915] [56997.639808] ? >>>> >icl_display_core_init+0x2c/0x1a0 [i915] [56997.639808] ? >>>> >_raw_spin_unlock_irqrestore+0x4c/0x60 >>>> >[56997.639808] icl_display_core_init+0x34/0x1a0 [i915] >>>> >[56997.639808] >>>> >intel_power_domains_init_hw+0x200/0x570 [i915] [56997.639808] >>>> >i915_driver_probe+0x103b/0x17e0 [i915] [56997.639808] ? >>>> >printk+0x53/0x6a [56997.639808] i915_pci_probe+0x3b/0x190 [i915] >>>> > >>>> >We may or may not need to change the implementation to account for >>>> >DDIC being available on other SKUs. For now I think the best thing >>>> >to do is to just >>>disable the port. >>>> >>>> This information ideally should be coming from VBT and based on that >>>> driver can take a call whether to enable the port or not. So is this >>>> an interim solution and later would be dropped, since there will/may >>>> be SKU's with >>>PORT C enabled. >>>> >>>> I feel revocation of this port in VBT should be the right approach, >>>> instead of an >>>interim solution. >>> >>>Ideally yes, but it's better something that works than the ideal >>>solution that doesn't. I wanted to go the VBT route, but it wouldn't >>>work with any machine that I have available. >>>Hence this patch. >>> >>>When/if there's one sku with ddic, hopefully VBT will already be fixed >>>or we may go the route of differentiating by PCI id. >> >>I feel this is a mistake in VBT which exposes an non-existent port and >>should be fixed. I don't think adding PCIID based checks for a platform >>is a good idea. We should call init for all ports spec claims to be >>supporting and based on VBT configuration just return if port is not >>available for use on that particular design. Now with current approach, >>if on a new SKU this is fixed (PORT C gets working) and > >I agree with all of that. But it would not fix the problem that we have *today*. >There's no forseable fix for VBT. > >>VBT exposes it also correctly, we will not even attempt to initialize >>it until we fix this patch and re-send a somewhat similar to a revert version of this >patch. > >the "fix" would be "make DDIC work and *start* exporting it". More like an additional >feature than a fix in itself. It was my mistake to send the initial patch with DDIC there >when it was actually not working. See that we still don't expose the TC ports and >initially we didn't expose the DSI port neither. > >In other words, expose the things that are *currently* working. DDIC is not and fixing >it by hooking up VBT if we should initialize it (my initial attempt) would not fix things. > >> >>So my take would be that we should push VBT guys to fix the problem >>instead of adding a temporary WA in our > >this has already been reported to people responsible for VBT. There's currently not >ETA for if/when this will be done. When/if it's done we can conditionally add DDIC. Ok fair point, if VBT fix is a long shot we can remove existence of PORT C and introduce it back later when/if it becomes alive again. With the above justifications, we can go ahead with this patch, but still keep pushing VBT guys to update their stuff. Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx> >Lucas De Marchi > >>driver. I will leave to maintainers to suggest what is the optimum approach. >> >>Regards, >>Uma Shankar >> >>>Lucas De Marchi >>> >>>> >>>> Regards, >>>> Uma Shankar >>>> >>>> >Cc: José Roberto de Souza <jose.souza@xxxxxxxxx> >>>> >Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >>>> >--- >>>> > drivers/gpu/drm/i915/display/intel_display.c | 3 +-- >>>> > 1 file changed, 1 insertion(+), 2 deletions(-) >>>> > >>>> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c >>>> >b/drivers/gpu/drm/i915/display/intel_display.c >>>> >index 5b733e38eae3..6c6a5a5f41bb 100644 >>>> >--- a/drivers/gpu/drm/i915/display/intel_display.c >>>> >+++ b/drivers/gpu/drm/i915/display/intel_display.c >>>> >@@ -6683,7 +6683,7 @@ bool intel_phy_is_combo(struct >>>> >drm_i915_private *dev_priv, enum phy phy) >>>> > if (phy == PHY_NONE) >>>> > return false; >>>> > >>>> >- if (IS_ELKHARTLAKE(dev_priv) || INTEL_GEN(dev_priv) >= 12) >>>> >+ if (IS_ELKHARTLAKE(dev_priv)) >>>> > return phy <= PHY_C; >>>> > >>>> > if (INTEL_GEN(dev_priv) >= 11) @@ -15317,7 +15317,6 @@ >>>> >static void intel_setup_outputs(struct drm_i915_private >>>> >*dev_priv) >>>> > /* TODO: initialize TC ports as well */ >>>> > intel_ddi_init(dev_priv, PORT_A); >>>> > intel_ddi_init(dev_priv, PORT_B); >>>> >- intel_ddi_init(dev_priv, PORT_C); >>>> > icl_dsi_init(dev_priv); >>>> > } else if (IS_ELKHARTLAKE(dev_priv)) { >>>> > intel_ddi_init(dev_priv, PORT_A); >>>> >-- >>>> >2.21.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
