Re: [Intel-gfx] [PATCH v2] drm/i915/rkl: new rkl ddc map for different PCH

From: "Lee, Shawn C" <shawn.c.lee@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>
Cc: "Chiou, Cooper" <cooper.chiou@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/rkl: new rkl ddc map for different PCH
Date: Sat, 31 Oct 2020 02:55:35 +0000	[thread overview]
Message-ID: <BY5PR11MB43074CA0CF93E3C6608FC83CA3120@BY5PR11MB4307.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201030173530.GO627052@mdroper-desk1.amr.corp.intel.com>

On Fri, Oct. 30, 2020, 5:35 p.m., Matt Roper wrote:
>On Fri, Oct 30, 2020 at 09:41:37PM +0800, Lee Shawn C wrote:
>> After boot into kernel. Driver configured ddc pin mapping based on 
>> predefined table in parse_ddi_port(). Now driver configure rkl ddc pin 
>> mapping depends on icp_ddc_pin_map[]. Then this table will give 
>> incorrect gmbus port number to cause HDMI can't work.
>> 
>> Refer to commit d0a89527d06 ("drm/i915/rkl: Add DDC pin mapping").
>> Create two ddc pin table for rkl TGP and CMP pch. Then HDMI can works 
>> properly on rkl.
>> 
>> v2: update patch based on latest dinq branch.
>> 
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Aditya Swarup <aditya.swarup@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Cooper Chiou <cooper.chiou@intel.com>
>> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2577
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c     | 20 +++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_vbt_defs.h |  4 ++++
>>  2 files changed, 24 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
>> b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 0a309645fe06..ca9426e1768a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1623,6 +1623,18 @@ static const u8 icp_ddc_pin_map[] = {
>>  	[TGL_DDC_BUS_PORT_6] = GMBUS_PIN_14_TC6_TGP,  };
>>  
>> +static const u8 rkl_pch_tgp_ddc_pin_map[] = {
>> +	[RKL_DDC_BUS_DDI_B] = GMBUS_PIN_2_BXT,
>> +	[RKL_DDC_BUS_DDI_D] = GMBUS_PIN_9_TC1_ICP,
>> +	[RKL_DDC_BUS_DDI_E] = GMBUS_PIN_10_TC2_ICP, };
>> +
>> +static const u8 rkl_pch_cmp_ddc_pin_map[] = {
>> +	[RKL_DDC_BUS_DDI_B] = GMBUS_PIN_2_BXT,
>> +	[RKL_DDC_BUS_DDI_D] = GMBUS_PIN_3_BXT,
>> +	[RKL_DDC_BUS_DDI_E] = GMBUS_PIN_4_CNP, };
>> +
>>  static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)  
>> {
>>  	const u8 *ddc_pin_map;
>> @@ -1630,6 +1642,14 @@ static u8 map_ddc_pin(struct drm_i915_private 
>> *dev_priv, u8 vbt_pin)
>>  
>>  	if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1) {
>>  		return vbt_pin;
>> +	} else if (IS_ROCKETLAKE(dev_priv)) {
>> +		if (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP) {
>> +			ddc_pin_map = rkl_pch_tgp_ddc_pin_map;
>> +			n_entries = ARRAY_SIZE(rkl_pch_tgp_ddc_pin_map);
>> +		} else {
>> +			ddc_pin_map = rkl_pch_cmp_ddc_pin_map;
>> +			n_entries = ARRAY_SIZE(rkl_pch_cmp_ddc_pin_map);
>> +		}
>>  	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) {
>>  		ddc_pin_map = icp_ddc_pin_map;
>>  		n_entries = ARRAY_SIZE(icp_ddc_pin_map); diff --git 
>> a/drivers/gpu/drm/i915/display/intel_vbt_defs.h 
>> b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> index 49b4b5fca941..2df009996128 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> @@ -319,6 +319,10 @@ enum vbt_gmbus_ddi {
>>  	ICL_DDC_BUS_DDI_A = 0x1,
>>  	ICL_DDC_BUS_DDI_B,
>>  	TGL_DDC_BUS_DDI_C,
>> +	RKL_DDC_BUS_DDI_B = 0x1,
>> +	RKL_DDC_BUS_DDI_C,
>> +	RKL_DDC_BUS_DDI_D,
>> +	RKL_DDC_BUS_DDI_E,
>
>These definitions don't really make sense; according to the VBT definition in the bspec (20124), the symbolic names map to different VBT input values depending on which PCH is paired with RKL.  E.g., VBT value of "2" refers to PHY-C when using a CMP PCH, but refers to PHY-B when using a TGP PCH.
>
>From what I can see, RKL+TGP is already handled properly today in the code and doesn't need any special handling.  The patch here would actually break it, because it would associate the wrong pins to outputs (and fail to associate anything at all with PHY-B [vbt value 2]).
>
>For RKL+CMP, we do need a change to the code to pick valid output pins in the range 1-4 rather than 1,2,9,A, but it doesn't look like the mapping being added here is quite right for that either.  CMP is a derivative of CNP, so I believe we should be following the "CNL-PCH"
>column of the VBT definition.
>
>
>Matt
>

Hi Matt, 

Thanks for your comments! Below is EFP configuration from vbt. And we know there is no real port "C" on physical hardware with TGP-PCH.
EFP1 : DisplayPort-B
EFP2 : HDMI-C
EFP3 : HDMI-D
EFP4 : no device

Below messages came from customer board with latest drm-tip kernel (5.10.0-rc1+). Port D/E will be mapped to ddc pin 0x3/0x9 according to icp_ddc_pin_map[].
But port D/E should map to 0x9/0xa on TGP-PCH.
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_bios_port_aux_ch [i915]] using AUX D for port D (platform default)
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Adding HDMI connector on [ENCODER:201:DDI D]
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Using DDC pin 0x3 for port D (VBT)
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_bios_port_aux_ch [i915]] using AUX E for port E (platform default)
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Adding HDMI connector on [ENCODER:205:DDI E]
Oct 23 18:39:14 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Using DDC pin 0x9 for port E (VBT)

This is what we got after applied this change.
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_bios_port_aux_ch [i915]] using AUX D for port D (platform default)
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Adding HDMI connector on [ENCODER:201:DDI D]
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Using DDC pin 0x9 for port D (VBT)
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_bios_port_aux_ch [i915]] using AUX E for port E (platform default)
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Adding HDMI connector on [ENCODER:205:DDI E]
Oct 28 18:18:47 ubuntu kernel: i915 0000:00:02.0: [drm:intel_hdmi_init_connector [i915]] Using DDC pin 0xa for port E (VBT)

Best regards,
Shawn

>>  	ICL_DDC_BUS_PORT_1 = 0x4,
>>  	ICL_DDC_BUS_PORT_2,
>>  	ICL_DDC_BUS_PORT_3,
>> --
>> 2.28.0
>> 
>
>-- 
>Matt Roper
>Graphics Software Engineer
>VTT-OSGC Platform Enablement
>Intel Corporation
>(916) 356-2795
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx