diff mbox series

drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to include tx_misc

Message ID 20241017205444.102979-2-gustavo.sousa@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to include tx_misc | expand

Commit Message

Gustavo Sousa Oct. 17, 2024, 8:53 p.m. UTC
There has been an update to the Bspec in which we need to set
tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
Xe2_LPD and newer. That field is mapped to the bits 7:0 of
SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
our state struct. Update the algorithm to reflect this change.

Bspec: 74489
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
 .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Saarinen, Jani Oct. 18, 2024, 6 a.m. UTC | #1
Hi, 

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Patchwork
> Sent: Friday, 18 October 2024 2.01
> To: Sousa, Gustavo <gustavo.sousa@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: ✗ Fi.CI.BAT: failure for drm/i915/xe2lpd: Update C20 HDMI TMDS
> algorithm to include tx_misc
> 
> Patch Details
> Series:	drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to include
> tx_misc
> URL:	https://patchwork.freedesktop.org/series/140136/
> State:	failure
> Details:	https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/index.html
> 
> CI Bug Log - changes from CI_DRM_15552 -> Patchwork_140136v1
> 
> 
> Summary
> 
> 
> FAILURE
> 
> Serious unknown changes coming with Patchwork_140136v1 absolutely need
> to be verified manually.
> 
> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_140136v1, please notify your bug team (I915-ci-
> infra@lists.freedesktop.org) to allow them to document this new failure
> mode, which will reduce false positives in CI.
> 
> External URL: https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/index.html
> 
> 
> Participating hosts (43 -> 42)
> 
> 
> Missing (1): fi-snb-2520m
> 
> 
> Possible new issues
> 
> 
> Here are the unknown changes that may have been introduced in Patchwork_140136v1:
> 
> 
> IGT changes
> 
> 
> Possible regressions
> 
> 
> *	igt@i915_selftest@live@active:
> 
> 	*	fi-glk-j4005: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/fi-glk-j4005/igt@i915_selftest@live@active.html>  ->
> DMESG-FAIL <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/fi-glk-j4005/igt@i915_selftest@live@active.html>
Seem known issue but got closed 1,5 months ago: https://gfx-ci.igk.intel.com/cibuglog-ng/issue/14086?query_key=d38d3a2b666aff93008569afe127695ab2dd9418 
I guess we need to re-open that? 

> 
> 
> Known issues
> 
> 
> Here are the changes found in Patchwork_140136v1 that come from known
> issues:
> 
> 
> IGT changes
> 
> 
> Issues hit
> 
> 
> *	igt@i915_selftest@live:
> 
> 	*	bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live.html>  -> DMESG-FAIL
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arls-
> 1/igt@i915_selftest@live.html>  (i915#10262
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10262>  /
> i915#10341 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/10341>  / i915#12133
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133> )
> 	*	fi-glk-j4005: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/fi-glk-j4005/igt@i915_selftest@live.html>  -> DMESG-
> FAIL <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/fi-glk-
> j4005/igt@i915_selftest@live.html>  (i915#12133
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133> )
> 
> *	igt@i915_selftest@live@gem_migrate:
> 
> 	*	bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live@gem_migrate.html>  -
> > DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/bat-arls-
> 1/igt@i915_selftest@live@gem_migrate.html>  (i915#10341
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341> )
> 
> *	igt@i915_selftest@live@guc_multi_lrc:
> 
> 	*	bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live@guc_multi_lrc.html>  -
> > DMESG-FAIL <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/bat-arls-
> 1/igt@i915_selftest@live@guc_multi_lrc.html>  (i915#10262
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10262> ) +10
> other tests dmesg-fail
> 
> 
> Possible fixes
> 
> 
> *	igt@i915_module_load@load:
> 
> 	*	bat-adlp-6: DMESG-WARN <https://intel-gfx-
> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-adlp-
> 6/igt@i915_module_load@load.html>  (i915#12253
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12253> ) -> PASS
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-adlp-
> 6/igt@i915_module_load@load.html>
> 
> *	igt@i915_selftest@live:
> 
> 	*	bat-arlh-2: DMESG-FAIL <https://intel-gfx-
> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-arlh-
> 2/igt@i915_selftest@live.html>  (i915#10341
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341> ) -> PASS
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arlh-
> 2/igt@i915_selftest@live.html>
> 
> *	igt@i915_selftest@live@workarounds:
> 
> 	*	bat-arlh-2: DMESG-FAIL <https://intel-gfx-
> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-arlh-
> 2/igt@i915_selftest@live@workarounds.html>  (i915#9500
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9500> ) -> PASS
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arlh-
> 2/igt@i915_selftest@live@workarounds.html>
> 	*	bat-mtlp-6: ABORT <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15552/bat-mtlp-6/igt@i915_selftest@live@workarounds.html>
> (i915#12216 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/12216> ) -> PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_140136v1/bat-mtlp-
> 6/igt@i915_selftest@live@workarounds.html>  +1 other test pass
> 
> *	igt@kms_chamelium_edid@hdmi-edid-read:
> 
> 	*	bat-dg2-13: DMESG-WARN <https://intel-gfx-
> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-dg2-
> 13/igt@kms_chamelium_edid@hdmi-edid-read.html>  -> PASS <https://intel-
> gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-dg2-
> 13/igt@kms_chamelium_edid@hdmi-edid-read.html>
> 
> 
> Build changes
> 
> 
> *	Linux: CI_DRM_15552 -> Patchwork_140136v1
> 
> CI-20190529: 20190529
> CI_DRM_15552: 4a1a4fc678a06046ca0443a2d0d61f2174c56b1d @
> git://anongit.freedesktop.org/gfx-ci/linux
> IGT_8078: 175bddf20141ccad40130719ca46bde5140f4872 @
> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> Patchwork_140136v1: 4a1a4fc678a06046ca0443a2d0d61f2174c56b1d @
> git://anongit.freedesktop.org/gfx-ci/linux
Pottumuttu, Sai Teja Oct. 18, 2024, 2:16 p.m. UTC | #2
On 18-10-2024 02:23, Gustavo Sousa wrote:
> There has been an update to the Bspec in which we need to set
> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
> our state struct. Update the algorithm to reflect this change.
>
> Bspec: 74489
Nit: Would 74491 be a better Bspec here?
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>   .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>   2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index f73d576fd99e..22184b2d5a11 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>   			    i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>   }
>   
> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
> +					   u64 pixel_clock,
> +					   struct intel_c20pll_state *pll_state)
>   {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +
>   	u64 datarate;
>   	u64 mpll_tx_clk_div;
>   	u64 vco_freq_shift;
> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>   	u64 mpll_fracn_rem;
>   	u8  mpllb_ana_freq_vco;
>   	u8  mpll_div_multiplier;
> +	u16  tx_misc;
>   
>   	if (pixel_clock < 25175 || pixel_clock > 600000)
>   		return -EINVAL;
> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>   	mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>   						  datarate), 255);
>   
> +	if (DISPLAY_VER(i915) >= 20)
> +		tx_misc = 0x5;
> +	else
> +		tx_misc = 0x0;
Looks like tx_misc changed from 0x1 to 0x5 and not from 0x0.

Thanks
Sai Teja
> +
>   	if (vco_freq <= DATARATE_3000000000)
>   		mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>   	else if (vco_freq <= DATARATE_3500000000)
> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>   
>   	pll_state->clock	= pixel_clock;
>   	pll_state->tx[0]	= 0xbe88;
> -	pll_state->tx[1]	= 0x9800;
> +	pll_state->tx[1]	= 0x9800 | C20_PHY_TX_MISC(tx_misc);
>   	pll_state->tx[2]	= 0x0000;
>   	pll_state->cmn[0]	= 0x0500;
>   	pll_state->cmn[1]	= 0x0005;
> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>   
>   	/* try computed C20 HDMI tables before using consolidated tables */
>   	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
> +		if (intel_c20_compute_hdmi_tmds_pll(encoder,
> +						    crtc_state->port_clock,
>   						    &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>   			return 0;
>   	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> index ab3ae110b68f..c1949aa1b909 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -280,6 +280,8 @@
>   #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>   		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>   #define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
> +#define   C20_PHY_TX_MISC_MASK		REG_GENMASK(7, 0)
> +#define   C20_PHY_TX_MISC(val)		REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>   
>   #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>   		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
Gustavo Sousa Oct. 18, 2024, 3:59 p.m. UTC | #3
Quoting Pottumuttu, Sai Teja (2024-10-18 11:16:43-03:00)
>
>On 18-10-2024 02:23, Gustavo Sousa wrote:
>> There has been an update to the Bspec in which we need to set
>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>> our state struct. Update the algorithm to reflect this change.
>>
>> Bspec: 74489
>Nit: Would 74491 be a better Bspec here?

Oops. You're right. I remember having that one open as well while
implementing this, so I ended up copying the wrong reference. Thanks!

>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>   .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>   2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index f73d576fd99e..22184b2d5a11 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>>                               i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>>   }
>>   
>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>> +                                           u64 pixel_clock,
>> +                                           struct intel_c20pll_state *pll_state)
>>   {
>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +
>>           u64 datarate;
>>           u64 mpll_tx_clk_div;
>>           u64 vco_freq_shift;
>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>           u64 mpll_fracn_rem;
>>           u8  mpllb_ana_freq_vco;
>>           u8  mpll_div_multiplier;
>> +        u16  tx_misc;
>>   
>>           if (pixel_clock < 25175 || pixel_clock > 600000)
>>                   return -EINVAL;
>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>           mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>>                                                     datarate), 255);
>>   
>> +        if (DISPLAY_VER(i915) >= 20)
>> +                tx_misc = 0x5;
>> +        else
>> +                tx_misc = 0x0;
>Looks like tx_misc changed from 0x1 to 0x5 and not from 0x0.

Yeah and it looks like we missed doing that update. However, I'm not
sure what you mean here. Do you mean to use 0x1 for the "else" case? If
so, then I think we should not do it. The change was from 0x1 to 0x5 for
Xe2_LPD and newer displays.

--
Gustavo Sousa

>
>Thanks
>Sai Teja
>> +
>>           if (vco_freq <= DATARATE_3000000000)
>>                   mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>           else if (vco_freq <= DATARATE_3500000000)
>> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>   
>>           pll_state->clock        = pixel_clock;
>>           pll_state->tx[0]        = 0xbe88;
>> -        pll_state->tx[1]        = 0x9800;
>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>           pll_state->tx[2]        = 0x0000;
>>           pll_state->cmn[0]        = 0x0500;
>>           pll_state->cmn[1]        = 0x0005;
>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>>   
>>           /* try computed C20 HDMI tables before using consolidated tables */
>>           if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>> +                                                    crtc_state->port_clock,
>>                                                       &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>>                           return 0;
>>           }
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> index ab3ae110b68f..c1949aa1b909 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> @@ -280,6 +280,8 @@
>>   #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>                   ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>   #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>> +#define   C20_PHY_TX_MISC(val)                REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>>   
>>   #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>                   ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
Pottumuttu, Sai Teja Oct. 18, 2024, 4:48 p.m. UTC | #4
On 18-10-2024 21:29, Gustavo Sousa wrote:
> Quoting Pottumuttu, Sai Teja (2024-10-18 11:16:43-03:00)
>> On 18-10-2024 02:23, Gustavo Sousa wrote:
>>> There has been an update to the Bspec in which we need to set
>>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>>> our state struct. Update the algorithm to reflect this change.
>>>
>>> Bspec: 74489
>> Nit: Would 74491 be a better Bspec here?
> Oops. You're right. I remember having that one open as well while
> implementing this, so I ended up copying the wrong reference. Thanks!
>
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>>    .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>>    2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> index f73d576fd99e..22184b2d5a11 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>>>                                i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>>>    }
>>>    
>>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
>>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>>> +                                           u64 pixel_clock,
>>> +                                           struct intel_c20pll_state *pll_state)
>>>    {
>>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>> +
>>>            u64 datarate;
>>>            u64 mpll_tx_clk_div;
>>>            u64 vco_freq_shift;
>>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>            u64 mpll_fracn_rem;
>>>            u8  mpllb_ana_freq_vco;
>>>            u8  mpll_div_multiplier;
>>> +        u16  tx_misc;
>>>    
>>>            if (pixel_clock < 25175 || pixel_clock > 600000)
>>>                    return -EINVAL;
>>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>            mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>>>                                                      datarate), 255);
>>>    
>>> +        if (DISPLAY_VER(i915) >= 20)
>>> +                tx_misc = 0x5;
>>> +        else
>>> +                tx_misc = 0x0;
>> Looks like tx_misc changed from 0x1 to 0x5 and not from 0x0.
> Yeah and it looks like we missed doing that update. However, I'm not
> sure what you mean here. Do you mean to use 0x1 for the "else" case? If
> so, then I think we should not do it. The change was from 0x1 to 0x5 for
> Xe2_LPD and newer displays.

Yeah you are right, I was just trying to get a clarification on the 
previous values. The tx values looks good now with this patch.

So, with the BSpec change addressed,

Reviewed-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>

Thanks
Sai Teja

>
> --
> Gustavo Sousa
>
>> Thanks
>> Sai Teja
>>> +
>>>            if (vco_freq <= DATARATE_3000000000)
>>>                    mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>>            else if (vco_freq <= DATARATE_3500000000)
>>> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>    
>>>            pll_state->clock        = pixel_clock;
>>>            pll_state->tx[0]        = 0xbe88;
>>> -        pll_state->tx[1]        = 0x9800;
>>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>>            pll_state->tx[2]        = 0x0000;
>>>            pll_state->cmn[0]        = 0x0500;
>>>            pll_state->cmn[1]        = 0x0005;
>>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>>>    
>>>            /* try computed C20 HDMI tables before using consolidated tables */
>>>            if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>>> +                                                    crtc_state->port_clock,
>>>                                                        &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>>>                            return 0;
>>>            }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> index ab3ae110b68f..c1949aa1b909 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> @@ -280,6 +280,8 @@
>>>    #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>>                    ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>>    #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>>> +#define   C20_PHY_TX_MISC(val)                REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>>>    
>>>    #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>>                    ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
Gustavo Sousa Oct. 21, 2024, 11:14 a.m. UTC | #5
Quoting Saarinen, Jani (2024-10-18 03:00:58-03:00)
>Hi, 
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Patchwork
>> Sent: Friday, 18 October 2024 2.01
>> To: Sousa, Gustavo <gustavo.sousa@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Subject: ✗ Fi.CI.BAT: failure for drm/i915/xe2lpd: Update C20 HDMI TMDS
>> algorithm to include tx_misc
>> 
>> Patch Details
>> Series:        drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to include
>> tx_misc
>> URL:        https://patchwork.freedesktop.org/series/140136/
>> State:        failure
>> Details:        https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/index.html
>> 
>> CI Bug Log - changes from CI_DRM_15552 -> Patchwork_140136v1
>> 
>> 
>> Summary
>> 
>> 
>> FAILURE
>> 
>> Serious unknown changes coming with Patchwork_140136v1 absolutely need
>> to be verified manually.
>> 
>> If you think the reported changes have nothing to do with the changes
>> introduced in Patchwork_140136v1, please notify your bug team (I915-ci-
>> infra@lists.freedesktop.org) to allow them to document this new failure
>> mode, which will reduce false positives in CI.
>> 
>> External URL: https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/index.html
>> 
>> 
>> Participating hosts (43 -> 42)
>> 
>> 
>> Missing (1): fi-snb-2520m
>> 
>> 
>> Possible new issues
>> 
>> 
>> Here are the unknown changes that may have been introduced in Patchwork_140136v1:
>> 
>> 
>> IGT changes
>> 
>> 
>> Possible regressions
>> 
>> 
>> *        igt@i915_selftest@live@active:
>> 
>>         *        fi-glk-j4005: PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/fi-glk-j4005/igt@i915_selftest@live@active.html>  ->
>> DMESG-FAIL <https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/fi-glk-j4005/igt@i915_selftest@live@active.html>
>Seem known issue but got closed 1,5 months ago: https://gfx-ci.igk.intel.com/cibuglog-ng/issue/14086?query_key=d38d3a2b666aff93008569afe127695ab2dd9418 

Thanks, Jani.

And this should not be related to the patch series, since we are only
touching tracepoint code (and I believe tracepoints are not exercised by
CI, are they?).

CI team, could we re-report, please?

--
Gustavo Sousa

>I guess we need to re-open that? 
>
>> 
>> 
>> Known issues
>> 
>> 
>> Here are the changes found in Patchwork_140136v1 that come from known
>> issues:
>> 
>> 
>> IGT changes
>> 
>> 
>> Issues hit
>> 
>> 
>> *        igt@i915_selftest@live:
>> 
>>         *        bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live.html>  -> DMESG-FAIL
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arls-
>> 1/igt@i915_selftest@live.html>  (i915#10262
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10262>  /
>> i915#10341 <https://gitlab.freedesktop.org/drm/i915/kernel/-
>> /issues/10341>  / i915#12133
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133> )
>>         *        fi-glk-j4005: PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/fi-glk-j4005/igt@i915_selftest@live.html>  -> DMESG-
>> FAIL <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/fi-glk-
>> j4005/igt@i915_selftest@live.html>  (i915#12133
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133> )
>> 
>> *        igt@i915_selftest@live@gem_migrate:
>> 
>>         *        bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live@gem_migrate.html>  -
>> > DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/bat-arls-
>> 1/igt@i915_selftest@live@gem_migrate.html>  (i915#10341
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341> )
>> 
>> *        igt@i915_selftest@live@guc_multi_lrc:
>> 
>>         *        bat-arls-1: PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/bat-arls-1/igt@i915_selftest@live@guc_multi_lrc.html>  -
>> > DMESG-FAIL <https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/bat-arls-
>> 1/igt@i915_selftest@live@guc_multi_lrc.html>  (i915#10262
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10262> ) +10
>> other tests dmesg-fail
>> 
>> 
>> Possible fixes
>> 
>> 
>> *        igt@i915_module_load@load:
>> 
>>         *        bat-adlp-6: DMESG-WARN <https://intel-gfx-
>> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-adlp-
>> 6/igt@i915_module_load@load.html>  (i915#12253
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12253> ) -> PASS
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-adlp-
>> 6/igt@i915_module_load@load.html>
>> 
>> *        igt@i915_selftest@live:
>> 
>>         *        bat-arlh-2: DMESG-FAIL <https://intel-gfx-
>> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-arlh-
>> 2/igt@i915_selftest@live.html>  (i915#10341
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341> ) -> PASS
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arlh-
>> 2/igt@i915_selftest@live.html>
>> 
>> *        igt@i915_selftest@live@workarounds:
>> 
>>         *        bat-arlh-2: DMESG-FAIL <https://intel-gfx-
>> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-arlh-
>> 2/igt@i915_selftest@live@workarounds.html>  (i915#9500
>> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9500> ) -> PASS
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-arlh-
>> 2/igt@i915_selftest@live@workarounds.html>
>>         *        bat-mtlp-6: ABORT <https://intel-gfx-ci.01.org/tree/drm-
>> tip/CI_DRM_15552/bat-mtlp-6/igt@i915_selftest@live@workarounds.html>
>> (i915#12216 <https://gitlab.freedesktop.org/drm/i915/kernel/-
>> /issues/12216> ) -> PASS <https://intel-gfx-ci.01.org/tree/drm-
>> tip/Patchwork_140136v1/bat-mtlp-
>> 6/igt@i915_selftest@live@workarounds.html>  +1 other test pass
>> 
>> *        igt@kms_chamelium_edid@hdmi-edid-read:
>> 
>>         *        bat-dg2-13: DMESG-WARN <https://intel-gfx-
>> ci.01.org/tree/drm-tip/CI_DRM_15552/bat-dg2-
>> 13/igt@kms_chamelium_edid@hdmi-edid-read.html>  -> PASS <https://intel-
>> gfx-ci.01.org/tree/drm-tip/Patchwork_140136v1/bat-dg2-
>> 13/igt@kms_chamelium_edid@hdmi-edid-read.html>
>> 
>> 
>> Build changes
>> 
>> 
>> *        Linux: CI_DRM_15552 -> Patchwork_140136v1
>> 
>> CI-20190529: 20190529
>> CI_DRM_15552: 4a1a4fc678a06046ca0443a2d0d61f2174c56b1d @
>> git://anongit.freedesktop.org/gfx-ci/linux
>> IGT_8078: 175bddf20141ccad40130719ca46bde5140f4872 @
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
>> Patchwork_140136v1: 4a1a4fc678a06046ca0443a2d0d61f2174c56b1d @
>> git://anongit.freedesktop.org/gfx-ci/linux
>
Jani Nikula Oct. 21, 2024, 12:29 p.m. UTC | #6
On Thu, 17 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> There has been an update to the Bspec in which we need to set
> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
> our state struct. Update the algorithm to reflect this change.
>
> Bspec: 74489
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index f73d576fd99e..22184b2d5a11 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>  			    i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>  }
>  
> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
> +					   u64 pixel_clock,
> +					   struct intel_c20pll_state *pll_state)
>  {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);

No new struct drm_i915_private local variables or parameters, please.

struct intel_display *display = to_intel_display(encoder);

> +
>  	u64 datarate;
>  	u64 mpll_tx_clk_div;
>  	u64 vco_freq_shift;
> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>  	u64 mpll_fracn_rem;
>  	u8  mpllb_ana_freq_vco;
>  	u8  mpll_div_multiplier;
> +	u16  tx_misc;
>  
>  	if (pixel_clock < 25175 || pixel_clock > 600000)
>  		return -EINVAL;
> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>  	mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>  						  datarate), 255);
>  
> +	if (DISPLAY_VER(i915) >= 20)
> +		tx_misc = 0x5;
> +	else
> +		tx_misc = 0x0;
> +
>  	if (vco_freq <= DATARATE_3000000000)
>  		mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>  	else if (vco_freq <= DATARATE_3500000000)
> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>  
>  	pll_state->clock	= pixel_clock;
>  	pll_state->tx[0]	= 0xbe88;
> -	pll_state->tx[1]	= 0x9800;
> +	pll_state->tx[1]	= 0x9800 | C20_PHY_TX_MISC(tx_misc);
>  	pll_state->tx[2]	= 0x0000;
>  	pll_state->cmn[0]	= 0x0500;
>  	pll_state->cmn[1]	= 0x0005;
> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>  
>  	/* try computed C20 HDMI tables before using consolidated tables */
>  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
> +		if (intel_c20_compute_hdmi_tmds_pll(encoder,
> +						    crtc_state->port_clock,

Alternatively you could just pass crtc_state. *shrug*.

>  						    &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>  			return 0;
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> index ab3ae110b68f..c1949aa1b909 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -280,6 +280,8 @@
>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>  #define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
> +#define   C20_PHY_TX_MISC_MASK		REG_GENMASK(7, 0)
> +#define   C20_PHY_TX_MISC(val)		REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))

REG_FIELD_PREP16 should have a mask of REG_GENMASK16, and all the masks
and fields and bits for a register should all use the same width.

BR,
Jani.


>  
>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
Gustavo Sousa Oct. 21, 2024, 12:59 p.m. UTC | #7
Quoting Jani Nikula (2024-10-21 09:29:30-03:00)
>On Thu, 17 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> There has been an update to the Bspec in which we need to set
>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>> our state struct. Update the algorithm to reflect this change.
>>
>> Bspec: 74489
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index f73d576fd99e..22184b2d5a11 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>>                              i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>>  }
>>  
>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>> +                                           u64 pixel_clock,
>> +                                           struct intel_c20pll_state *pll_state)
>>  {
>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>
>No new struct drm_i915_private local variables or parameters, please.
>
>struct intel_display *display = to_intel_display(encoder);

I actually had thought about this when writing this patch, but then I
concluded that a future major refactor in the entire file would make
more sense, since we are not using struct intel_display anywhere in this
file yet...

I can send a v2 to use struct intel_display here. Or maybe I could send
a series containing preparatory patch to convert everything related to
C10/C20 to use the new struct and then this patch as the second in the
series (although those two patches would not be not totally related to
each other).

Let me know what is preferred.

--
Gustavo Sousa

>
>> +
>>          u64 datarate;
>>          u64 mpll_tx_clk_div;
>>          u64 vco_freq_shift;
>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>          u64 mpll_fracn_rem;
>>          u8  mpllb_ana_freq_vco;
>>          u8  mpll_div_multiplier;
>> +        u16  tx_misc;
>>  
>>          if (pixel_clock < 25175 || pixel_clock > 600000)
>>                  return -EINVAL;
>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>          mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>>                                                    datarate), 255);
>>  
>> +        if (DISPLAY_VER(i915) >= 20)
>> +                tx_misc = 0x5;
>> +        else
>> +                tx_misc = 0x0;
>> +
>>          if (vco_freq <= DATARATE_3000000000)
>>                  mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>          else if (vco_freq <= DATARATE_3500000000)
>> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>  
>>          pll_state->clock        = pixel_clock;
>>          pll_state->tx[0]        = 0xbe88;
>> -        pll_state->tx[1]        = 0x9800;
>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>          pll_state->tx[2]        = 0x0000;
>>          pll_state->cmn[0]        = 0x0500;
>>          pll_state->cmn[1]        = 0x0005;
>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>>  
>>          /* try computed C20 HDMI tables before using consolidated tables */
>>          if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>> +                                                    crtc_state->port_clock,
>
>Alternatively you could just pass crtc_state. *shrug*.
>
>>                                                      &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>>                          return 0;
>>          }
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> index ab3ae110b68f..c1949aa1b909 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> @@ -280,6 +280,8 @@
>>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>  #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>> +#define   C20_PHY_TX_MISC(val)                REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>
>REG_FIELD_PREP16 should have a mask of REG_GENMASK16, and all the masks
>and fields and bits for a register should all use the same width.
>
>BR,
>Jani.
>
>
>>  
>>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
>
>-- 
>Jani Nikula, Intel
Jani Nikula Oct. 21, 2024, 1:15 p.m. UTC | #8
On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2024-10-21 09:29:30-03:00)
>>On Thu, 17 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> There has been an update to the Bspec in which we need to set
>>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>>> our state struct. Update the algorithm to reflect this change.
>>>
>>> Bspec: 74489
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> index f73d576fd99e..22184b2d5a11 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>>>                              i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>>>  }
>>>  
>>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
>>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>>> +                                           u64 pixel_clock,
>>> +                                           struct intel_c20pll_state *pll_state)
>>>  {
>>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>
>>No new struct drm_i915_private local variables or parameters, please.
>>
>>struct intel_display *display = to_intel_display(encoder);
>
> I actually had thought about this when writing this patch, but then I
> concluded that a future major refactor in the entire file would make
> more sense, since we are not using struct intel_display anywhere in this
> file yet...

I actually don't mind having files with mixed i915/display usage because
it's going to be a somewhat gradual conversion anyway, and there are
bound to be i915 stragglers even in the files that we mass convert.

> I can send a v2 to use struct intel_display here. Or maybe I could send
> a series containing preparatory patch to convert everything related to
> C10/C20 to use the new struct and then this patch as the second in the
> series (although those two patches would not be not totally related to
> each other).
>
> Let me know what is preferred.

Up to you, really.

BR,
Jani.


>
> --
> Gustavo Sousa
>
>>
>>> +
>>>          u64 datarate;
>>>          u64 mpll_tx_clk_div;
>>>          u64 vco_freq_shift;
>>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>          u64 mpll_fracn_rem;
>>>          u8  mpllb_ana_freq_vco;
>>>          u8  mpll_div_multiplier;
>>> +        u16  tx_misc;
>>>  
>>>          if (pixel_clock < 25175 || pixel_clock > 600000)
>>>                  return -EINVAL;
>>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>          mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>>>                                                    datarate), 255);
>>>  
>>> +        if (DISPLAY_VER(i915) >= 20)
>>> +                tx_misc = 0x5;
>>> +        else
>>> +                tx_misc = 0x0;
>>> +
>>>          if (vco_freq <= DATARATE_3000000000)
>>>                  mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>>          else if (vco_freq <= DATARATE_3500000000)
>>> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>>  
>>>          pll_state->clock        = pixel_clock;
>>>          pll_state->tx[0]        = 0xbe88;
>>> -        pll_state->tx[1]        = 0x9800;
>>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>>          pll_state->tx[2]        = 0x0000;
>>>          pll_state->cmn[0]        = 0x0500;
>>>          pll_state->cmn[1]        = 0x0005;
>>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>>>  
>>>          /* try computed C20 HDMI tables before using consolidated tables */
>>>          if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>>> +                                                    crtc_state->port_clock,
>>
>>Alternatively you could just pass crtc_state. *shrug*.
>>
>>>                                                      &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>>>                          return 0;
>>>          }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> index ab3ae110b68f..c1949aa1b909 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>>> @@ -280,6 +280,8 @@
>>>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>>  #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>>> +#define   C20_PHY_TX_MISC(val)                REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>>
>>REG_FIELD_PREP16 should have a mask of REG_GENMASK16, and all the masks
>>and fields and bits for a register should all use the same width.
>>
>>BR,
>>Jani.
>>
>>
>>>  
>>>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
>>
>>-- 
>>Jani Nikula, Intel
Bhadane, Dnyaneshwar Oct. 21, 2024, 8:09 p.m. UTC | #9
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Gustavo Sousa
> Sent: Friday, October 18, 2024 2:23 AM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Subject: [PATCH] drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to
> include tx_misc
> 
> There has been an update to the Bspec in which we need to set
> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
> our state struct. Update the algorithm to reflect this change.
> 
> Bspec: 74489
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index f73d576fd99e..22184b2d5a11 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct
> drm_i915_private *i915,
>  			    i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
> }
> 
> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct
> intel_c20pll_state *pll_state)
> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
> +					   u64 pixel_clock,
> +					   struct intel_c20pll_state *pll_state)
>  {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +
>  	u64 datarate;
>  	u64 mpll_tx_clk_div;
>  	u64 vco_freq_shift;
> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64
> pixel_clock, struct intel_c20pll_
>  	u64 mpll_fracn_rem;
>  	u8  mpllb_ana_freq_vco;
>  	u8  mpll_div_multiplier;
> +	u16  tx_misc;

In the case of tx_misc, it seems that using the u8 type have been a better fit.

Dnyaneshwar,

> 
>  	if (pixel_clock < 25175 || pixel_clock > 600000)
>  		return -EINVAL;
> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64
> pixel_clock, struct intel_c20pll_
>  	mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate
> >> 1)),
>  						  datarate), 255);
> 
> +	if (DISPLAY_VER(i915) >= 20)
> +		tx_misc = 0x5;
> +	else
> +		tx_misc = 0x0;
> +
>  	if (vco_freq <= DATARATE_3000000000)
>  		mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>  	else if (vco_freq <= DATARATE_3500000000) @@ -2182,7 +2192,7
> @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct
> intel_c20pll_
> 
>  	pll_state->clock	= pixel_clock;
>  	pll_state->tx[0]	= 0xbe88;
> -	pll_state->tx[1]	= 0x9800;
> +	pll_state->tx[1]	= 0x9800 | C20_PHY_TX_MISC(tx_misc);
>  	pll_state->tx[2]	= 0x0000;
>  	pll_state->cmn[0]	= 0x0500;
>  	pll_state->cmn[1]	= 0x0005;
> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct
> intel_crtc_state *crtc_state,
> 
>  	/* try computed C20 HDMI tables before using consolidated tables */
>  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
> +		if (intel_c20_compute_hdmi_tmds_pll(encoder,
> +						    crtc_state->port_clock,
>  						    &crtc_state-
> >dpll_hw_state.cx0pll.c20) == 0)
>  			return 0;
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> index ab3ae110b68f..c1949aa1b909 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -280,6 +280,8 @@
>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG :
> _MTL_C20_B_TX_CNTX_CFG) - (idx))
>  #define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
> +#define   C20_PHY_TX_MISC_MASK		REG_GENMASK(7, 0)
> +#define   C20_PHY_TX_MISC(val)
> 	REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
> 
>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ?
> _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
> --
> 2.46.1
Gustavo Sousa Oct. 22, 2024, 10:29 p.m. UTC | #10
Quoting Jani Nikula (2024-10-21 09:29:30-03:00)
>On Thu, 17 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> There has been an update to the Bspec in which we need to set
>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>> our state struct. Update the algorithm to reflect this change.
>>
>> Bspec: 74489
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index f73d576fd99e..22184b2d5a11 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>>                              i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>>  }
>>  
>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>> +                                           u64 pixel_clock,
>> +                                           struct intel_c20pll_state *pll_state)
>>  {
>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>
>No new struct drm_i915_private local variables or parameters, please.
>
>struct intel_display *display = to_intel_display(encoder);
>
>> +
>>          u64 datarate;
>>          u64 mpll_tx_clk_div;
>>          u64 vco_freq_shift;
>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>          u64 mpll_fracn_rem;
>>          u8  mpllb_ana_freq_vco;
>>          u8  mpll_div_multiplier;
>> +        u16  tx_misc;
>>  
>>          if (pixel_clock < 25175 || pixel_clock > 600000)
>>                  return -EINVAL;
>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>          mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>>                                                    datarate), 255);
>>  
>> +        if (DISPLAY_VER(i915) >= 20)
>> +                tx_misc = 0x5;
>> +        else
>> +                tx_misc = 0x0;
>> +
>>          if (vco_freq <= DATARATE_3000000000)
>>                  mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>          else if (vco_freq <= DATARATE_3500000000)
>> @@ -2182,7 +2192,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
>>  
>>          pll_state->clock        = pixel_clock;
>>          pll_state->tx[0]        = 0xbe88;
>> -        pll_state->tx[1]        = 0x9800;
>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>          pll_state->tx[2]        = 0x0000;
>>          pll_state->cmn[0]        = 0x0500;
>>          pll_state->cmn[1]        = 0x0005;
>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>>  
>>          /* try computed C20 HDMI tables before using consolidated tables */
>>          if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>> +                                                    crtc_state->port_clock,

Ah... Sorry, for some reason I did not scroll down for the rest of your
feedback.

>
>Alternatively you could just pass crtc_state. *shrug*.

Yeah, that makes a lot of sense. Will do that with v2.

>
>>                                                      &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
>>                          return 0;
>>          }
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> index ab3ae110b68f..c1949aa1b909 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> @@ -280,6 +280,8 @@
>>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>  #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>> +#define   C20_PHY_TX_MISC(val)                REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>
>REG_FIELD_PREP16 should have a mask of REG_GENMASK16, and all the masks
>and fields and bits for a register should all use the same width.

Right... And it seems like there are other macros here that should also
be converted to the 16-bit variants... I guess an overhaul of this
header is warranted.

I'll go ahead and fix the ones specifically for TX_MISC and then do
separate patch as follow-up fixing widths.

Thanks!

--
Gustavo Sousa

>
>BR,
>Jani.
>
>
>>  
>>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
>
>-- 
>Jani Nikula, Intel
Gustavo Sousa Oct. 22, 2024, 10:32 p.m. UTC | #11
Quoting Bhadane, Dnyaneshwar (2024-10-21 17:09:00-03:00)
>
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Gustavo Sousa
>> Sent: Friday, October 18, 2024 2:23 AM
>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Subject: [PATCH] drm/i915/xe2lpd: Update C20 HDMI TMDS algorithm to
>> include tx_misc
>> 
>> There has been an update to the Bspec in which we need to set
>> tx_misc=0x5 field for C20 TX Context programming for HDMI TMDS for
>> Xe2_LPD and newer. That field is mapped to the bits 7:0 of
>> SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1, which in turn translates to tx[1] of
>> our state struct. Update the algorithm to reflect this change.
>> 
>> Bspec: 74489
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c    | 17 ++++++++++++++---
>>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h   |  2 ++
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index f73d576fd99e..22184b2d5a11 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -2142,8 +2142,12 @@ static void intel_c10pll_dump_hw_state(struct
>> drm_i915_private *i915,
>>                              i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>> }
>> 
>> -static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct
>> intel_c20pll_state *pll_state)
>> +static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
>> +                                           u64 pixel_clock,
>> +                                           struct intel_c20pll_state *pll_state)
>>  {
>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +
>>          u64 datarate;
>>          u64 mpll_tx_clk_div;
>>          u64 vco_freq_shift;
>> @@ -2154,6 +2158,7 @@ static int intel_c20_compute_hdmi_tmds_pll(u64
>> pixel_clock, struct intel_c20pll_
>>          u64 mpll_fracn_rem;
>>          u8  mpllb_ana_freq_vco;
>>          u8  mpll_div_multiplier;
>> +        u16  tx_misc;
>
>In the case of tx_misc, it seems that using the u8 type have been a better fit.

As Jani mentioned in another reply to this series, fields should be of
same width as their target registers. So I think it would make sense to
keep this as u16.

--
Gustavo Sousa

>
>Dnyaneshwar,
>
>> 
>>          if (pixel_clock < 25175 || pixel_clock > 600000)
>>                  return -EINVAL;
>> @@ -2171,6 +2176,11 @@ static int intel_c20_compute_hdmi_tmds_pll(u64
>> pixel_clock, struct intel_c20pll_
>>          mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate
>> >> 1)),
>>                                                    datarate), 255);
>> 
>> +        if (DISPLAY_VER(i915) >= 20)
>> +                tx_misc = 0x5;
>> +        else
>> +                tx_misc = 0x0;
>> +
>>          if (vco_freq <= DATARATE_3000000000)
>>                  mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>>          else if (vco_freq <= DATARATE_3500000000) @@ -2182,7 +2192,7
>> @@ static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct
>> intel_c20pll_
>> 
>>          pll_state->clock        = pixel_clock;
>>          pll_state->tx[0]        = 0xbe88;
>> -        pll_state->tx[1]        = 0x9800;
>> +        pll_state->tx[1]        = 0x9800 | C20_PHY_TX_MISC(tx_misc);
>>          pll_state->tx[2]        = 0x0000;
>>          pll_state->cmn[0]        = 0x0500;
>>          pll_state->cmn[1]        = 0x0005;
>> @@ -2266,7 +2276,8 @@ static int intel_c20pll_calc_state(struct
>> intel_crtc_state *crtc_state,
>> 
>>          /* try computed C20 HDMI tables before using consolidated tables */
>>          if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> -                if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
>> +                if (intel_c20_compute_hdmi_tmds_pll(encoder,
>> +                                                    crtc_state->port_clock,
>>                                                      &crtc_state-
>> >dpll_hw_state.cx0pll.c20) == 0)
>>                          return 0;
>>          }
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> index ab3ae110b68f..c1949aa1b909 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> @@ -280,6 +280,8 @@
>>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG :
>> _MTL_C20_B_TX_CNTX_CFG) - (idx))
>>  #define   C20_PHY_TX_RATE                REG_GENMASK(2, 0)
>> +#define   C20_PHY_TX_MISC_MASK                REG_GENMASK(7, 0)
>> +#define   C20_PHY_TX_MISC(val)
>>         REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
>> 
>>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>>                  ((_IS_XE2HPD_C20(i915) ?
>> _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
>> --
>> 2.46.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index f73d576fd99e..22184b2d5a11 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2142,8 +2142,12 @@  static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
 			    i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
 }
 
-static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_state *pll_state)
+static int intel_c20_compute_hdmi_tmds_pll(struct intel_encoder *encoder,
+					   u64 pixel_clock,
+					   struct intel_c20pll_state *pll_state)
 {
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
 	u64 datarate;
 	u64 mpll_tx_clk_div;
 	u64 vco_freq_shift;
@@ -2154,6 +2158,7 @@  static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
 	u64 mpll_fracn_rem;
 	u8  mpllb_ana_freq_vco;
 	u8  mpll_div_multiplier;
+	u16  tx_misc;
 
 	if (pixel_clock < 25175 || pixel_clock > 600000)
 		return -EINVAL;
@@ -2171,6 +2176,11 @@  static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
 	mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
 						  datarate), 255);
 
+	if (DISPLAY_VER(i915) >= 20)
+		tx_misc = 0x5;
+	else
+		tx_misc = 0x0;
+
 	if (vco_freq <= DATARATE_3000000000)
 		mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
 	else if (vco_freq <= DATARATE_3500000000)
@@ -2182,7 +2192,7 @@  static int intel_c20_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_c20pll_
 
 	pll_state->clock	= pixel_clock;
 	pll_state->tx[0]	= 0xbe88;
-	pll_state->tx[1]	= 0x9800;
+	pll_state->tx[1]	= 0x9800 | C20_PHY_TX_MISC(tx_misc);
 	pll_state->tx[2]	= 0x0000;
 	pll_state->cmn[0]	= 0x0500;
 	pll_state->cmn[1]	= 0x0005;
@@ -2266,7 +2276,8 @@  static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
 
 	/* try computed C20 HDMI tables before using consolidated tables */
 	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
-		if (intel_c20_compute_hdmi_tmds_pll(crtc_state->port_clock,
+		if (intel_c20_compute_hdmi_tmds_pll(encoder,
+						    crtc_state->port_clock,
 						    &crtc_state->dpll_hw_state.cx0pll.c20) == 0)
 			return 0;
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
index ab3ae110b68f..c1949aa1b909 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
@@ -280,6 +280,8 @@ 
 #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
 		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
 #define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
+#define   C20_PHY_TX_MISC_MASK		REG_GENMASK(7, 0)
+#define   C20_PHY_TX_MISC(val)		REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
 
 #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
 		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))