[Intel-gfx] [PATCH] drm/i915/tgl: disable DDIC
Shankar, Uma
uma.shankar at intel.com
Tue Aug 20 03:43:25 UTC 2019
>>>> >-----Original Message-----
>>>> >From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On
>>>> >Behalf Of Lucas De Marchi
>>>> >Sent: Thursday, August 15, 2019 5:25 AM
>>>> >To: intel-gfx at lists.freedesktop.org
>>>> >Subject: [Intel-gfx] [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 at intel.com>
>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 at intel.com>
>>>> >Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>>> >---
>>>> > 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
More information about the Intel-gfx
mailing list