Re: [Intel-gfx] [PATCH 7/8] drm/i915/display: Eliminate IS_METEORLAKE checks

2023-07-19 Thread Gustavo Sousa
Quoting Gustavo Sousa (2023-07-19 16:39:25-03:00)
>Quoting Matt Roper (2023-07-18 19:28:01-03:00)
>>Most of the IS_METEORLAKE checks in the display code shouldn't actually
>>be tied to MTL as a platform, but rather to the Xe_LPD+ display IP
>>(which is used in MTL, but may show up again in future platforms).  In
>>cases where we're trying to match that specific IP, use a version check
>>against IP_VER(14, 0).  For cases where we're just handling new behavior
>>introduced by this IP (but which may also be inherited by future IP as
>>well), use a ver >= 14 check.
>>
>>The one exception here is the stolen memory workaround Wa_13010847436
>>(which is mislabelled as "Wa_22018444074" in the code).  That's truly a
>>MTL-specific issue rather than being tied to any of the IP blocks, so
>>leaving the condition as IS_METEORLAKE is correct there.
>
>I grepped and also saw usage for IS_METEORLAKE() for RC6-related code
>and around clock gating as well. I'm yet not familiar with those to be
>able to tell if they are platform-specific or rather specific to sub
>IP(s). Just thought it would be worth noting here, just in case.

Oops. I was too quick on sending this without realizing that this patch is
specific to display...

>
>>
>>Signed-off-by: Matt Roper 
>>---
>> drivers/gpu/drm/i915/display/intel_cdclk.c   | 4 ++--
>> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
>> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
>> drivers/gpu/drm/i915/display/intel_dmc.c | 2 +-
>> 4 files changed, 5 insertions(+), 5 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
>>b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>index dcc1f6941b60..4cb1dc397b62 100644
>>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>@@ -1840,7 +1840,7 @@ static bool 
>>cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
>> 
>> static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
>> {
>>-return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv)) &&
>>+return ((IS_DG2(dev_priv) || DISPLAY_VER_FULL(dev_priv) == 
>>IP_VER(14, 0)) &&
>> dev_priv->display.cdclk.hw.vco > 0 &&
>> HAS_CDCLK_SQUASH(dev_priv));
>> }
>>@@ -3559,7 +3559,7 @@ static const struct intel_cdclk_funcs i830_cdclk_funcs 
>>= {
>>  */
>> void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>> {
>>-if (IS_METEORLAKE(dev_priv)) {
>>+if (DISPLAY_VER(dev_priv) > 14) {
>
>I think you missed the equality part here, should be DISPLAY_VER(dev_priv) >=
>14, right?

...so, with this fix,

Reviewed-by: Gustavo Sousa 

>
>--
>Gustavo Sousa
>
>> dev_priv->display.funcs.cdclk = &mtl_cdclk_funcs;
>> dev_priv->display.cdclk.table = mtl_cdclk_table;
>> } else if (IS_DG2(dev_priv)) {
>>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c 
>>b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>index 1b00ef2c6185..025c80b9fece 100644
>>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>@@ -31,7 +31,7 @@
>> 
>> bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy)
>> {
>>-if (IS_METEORLAKE(i915) && (phy < PHY_C))
>>+if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && (phy < PHY_C))
>> return true;
>> 
>> return false;
>>diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>>b/drivers/gpu/drm/i915/display/intel_display.c
>>index 43cba98f7753..85efd77f491b 100644
>>--- a/drivers/gpu/drm/i915/display/intel_display.c
>>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>>@@ -1767,7 +1767,7 @@ bool intel_phy_is_tc(struct drm_i915_private *dev_priv, 
>>enum phy phy)
>> if (IS_DG2(dev_priv))
>> /* DG2's "TC1" output uses a SNPS PHY */
>> return false;
>>-else if (IS_ALDERLAKE_P(dev_priv) || IS_METEORLAKE(dev_priv))
>>+else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == 
>>IP_VER(14, 0))
>> return phy >= PHY_F && phy <= PHY_I;
>> else if (IS_TIGERLAKE(dev_priv))
>> return phy >= PHY_D && phy <= PHY_I;
>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c 
>>b/drivers/gpu/drm/i915/display/intel_dmc.c
>>index 5f479f3828bb..1623c0c5e8a1 100644
>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>>@@ -998,7 +998,7 @@ void intel_dmc_init(struct drm_i915_private *i915)
>> 
>> INIT_WORK(&dmc->work, dmc_load_work_fn);
>> 
>>-if (IS_METEORLAKE(i915)) {
>>+if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
>> dmc->fw_path = MTL_DMC_PATH;
>> dmc->max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
>> } else if (IS_DG2(i915)) {
>>-- 
>>2.41.0
>>


Re: [Intel-gfx] [PATCH 7/8] drm/i915/display: Eliminate IS_METEORLAKE checks

2023-07-19 Thread Gustavo Sousa
Quoting Matt Roper (2023-07-18 19:28:01-03:00)
>Most of the IS_METEORLAKE checks in the display code shouldn't actually
>be tied to MTL as a platform, but rather to the Xe_LPD+ display IP
>(which is used in MTL, but may show up again in future platforms).  In
>cases where we're trying to match that specific IP, use a version check
>against IP_VER(14, 0).  For cases where we're just handling new behavior
>introduced by this IP (but which may also be inherited by future IP as
>well), use a ver >= 14 check.
>
>The one exception here is the stolen memory workaround Wa_13010847436
>(which is mislabelled as "Wa_22018444074" in the code).  That's truly a
>MTL-specific issue rather than being tied to any of the IP blocks, so
>leaving the condition as IS_METEORLAKE is correct there.

I grepped and also saw usage for IS_METEORLAKE() for RC6-related code
and around clock gating as well. I'm yet not familiar with those to be
able to tell if they are platform-specific or rather specific to sub
IP(s). Just thought it would be worth noting here, just in case.

>
>Signed-off-by: Matt Roper 
>---
> drivers/gpu/drm/i915/display/intel_cdclk.c   | 4 ++--
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dmc.c | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
>b/drivers/gpu/drm/i915/display/intel_cdclk.c
>index dcc1f6941b60..4cb1dc397b62 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>@@ -1840,7 +1840,7 @@ static bool 
>cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
> 
> static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
> {
>-return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv)) &&
>+return ((IS_DG2(dev_priv) || DISPLAY_VER_FULL(dev_priv) == IP_VER(14, 
>0)) &&
> dev_priv->display.cdclk.hw.vco > 0 &&
> HAS_CDCLK_SQUASH(dev_priv));
> }
>@@ -3559,7 +3559,7 @@ static const struct intel_cdclk_funcs i830_cdclk_funcs = 
>{
>  */
> void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> {
>-if (IS_METEORLAKE(dev_priv)) {
>+if (DISPLAY_VER(dev_priv) > 14) {

I think you missed the equality part here, should be DISPLAY_VER(dev_priv) >=
14, right?

--
Gustavo Sousa

> dev_priv->display.funcs.cdclk = &mtl_cdclk_funcs;
> dev_priv->display.cdclk.table = mtl_cdclk_table;
> } else if (IS_DG2(dev_priv)) {
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c 
>b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index 1b00ef2c6185..025c80b9fece 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -31,7 +31,7 @@
> 
> bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy)
> {
>-if (IS_METEORLAKE(i915) && (phy < PHY_C))
>+if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && (phy < PHY_C))
> return true;
> 
> return false;
>diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>b/drivers/gpu/drm/i915/display/intel_display.c
>index 43cba98f7753..85efd77f491b 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.c
>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>@@ -1767,7 +1767,7 @@ bool intel_phy_is_tc(struct drm_i915_private *dev_priv, 
>enum phy phy)
> if (IS_DG2(dev_priv))
> /* DG2's "TC1" output uses a SNPS PHY */
> return false;
>-else if (IS_ALDERLAKE_P(dev_priv) || IS_METEORLAKE(dev_priv))
>+else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == 
>IP_VER(14, 0))
> return phy >= PHY_F && phy <= PHY_I;
> else if (IS_TIGERLAKE(dev_priv))
> return phy >= PHY_D && phy <= PHY_I;
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c 
>b/drivers/gpu/drm/i915/display/intel_dmc.c
>index 5f479f3828bb..1623c0c5e8a1 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>@@ -998,7 +998,7 @@ void intel_dmc_init(struct drm_i915_private *i915)
> 
> INIT_WORK(&dmc->work, dmc_load_work_fn);
> 
>-if (IS_METEORLAKE(i915)) {
>+if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0)) {
> dmc->fw_path = MTL_DMC_PATH;
> dmc->max_fw_size = XELPDP_DMC_MAX_FW_SIZE;
> } else if (IS_DG2(i915)) {
>-- 
>2.41.0
>