[Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init

[Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init

[Matt Roper]

Moving display-specific runtime info initialization into display/ makes
the display code more self-contained and also makes it easier to call
from the Xe driver.

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 .../drm/i915/display/intel_display_device.c   | 124 +++++++++++++++++
 .../drm/i915/display/intel_display_device.h   |   1 +
 drivers/gpu/drm/i915/intel_device_info.c      | 130 +-----------------
 3 files changed, 128 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
index 464df1764a86..8d379da877dc 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -7,6 +7,8 @@
 #include <drm/drm_color_mgmt.h>
 #include <linux/pci.h>
 
+#include "display/intel_de.h"
+#include "display/intel_display.h"
 #include "i915_drv.h"
 #include "i915_reg.h"
 #include "intel_display_device.h"
@@ -778,3 +780,125 @@ intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
 
 	return &no_display;
 }
+
+void intel_display_device_info_runtime_init(struct drm_i915_private *i915)
+{
+	struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(i915);
+	enum pipe pipe;
+
+	/* Wa_14011765242: adl-s A0,A1 */
+	if (IS_ADLS_DISPLAY_STEP(i915, STEP_A0, STEP_A2))
+		for_each_pipe(i915, pipe)
+			display_runtime->num_scalers[pipe] = 0;
+	else if (DISPLAY_VER(i915) >= 11) {
+		for_each_pipe(i915, pipe)
+			display_runtime->num_scalers[pipe] = 2;
+	} else if (DISPLAY_VER(i915) >= 9) {
+		display_runtime->num_scalers[PIPE_A] = 2;
+		display_runtime->num_scalers[PIPE_B] = 2;
+		display_runtime->num_scalers[PIPE_C] = 1;
+	}
+
+	if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
+		for_each_pipe(i915, pipe)
+			display_runtime->num_sprites[pipe] = 4;
+	else if (DISPLAY_VER(i915) >= 11)
+		for_each_pipe(i915, pipe)
+			display_runtime->num_sprites[pipe] = 6;
+	else if (DISPLAY_VER(i915) == 10)
+		for_each_pipe(i915, pipe)
+			display_runtime->num_sprites[pipe] = 3;
+	else if (IS_BROXTON(i915)) {
+		/*
+		 * Skylake and Broxton currently don't expose the topmost plane as its
+		 * use is exclusive with the legacy cursor and we only want to expose
+		 * one of those, not both. Until we can safely expose the topmost plane
+		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
+		 * we don't expose the topmost plane at all to prevent ABI breakage
+		 * down the line.
+		 */
+
+		display_runtime->num_sprites[PIPE_A] = 2;
+		display_runtime->num_sprites[PIPE_B] = 2;
+		display_runtime->num_sprites[PIPE_C] = 1;
+	} else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
+		for_each_pipe(i915, pipe)
+			display_runtime->num_sprites[pipe] = 2;
+	} else if (DISPLAY_VER(i915) >= 5 || IS_G4X(i915)) {
+		for_each_pipe(i915, pipe)
+			display_runtime->num_sprites[pipe] = 1;
+	}
+
+	if ((IS_DGFX(i915) || DISPLAY_VER(i915) >= 14) &&
+	    !(intel_de_read(i915, GU_CNTL_PROTECTED) & DEPRESENT)) {
+		drm_info(&i915->drm, "Display not present, disabling\n");
+		goto display_fused_off;
+	}
+
+	if (IS_GRAPHICS_VER(i915, 7, 8) && HAS_PCH_SPLIT(i915)) {
+		u32 fuse_strap = intel_de_read(i915, FUSE_STRAP);
+		u32 sfuse_strap = intel_de_read(i915, SFUSE_STRAP);
+
+		/*
+		 * SFUSE_STRAP is supposed to have a bit signalling the display
+		 * is fused off. Unfortunately it seems that, at least in
+		 * certain cases, fused off display means that PCH display
+		 * reads don't land anywhere. In that case, we read 0s.
+		 *
+		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
+		 * should be set when taking over after the firmware.
+		 */
+		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
+		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
+		    (HAS_PCH_CPT(i915) &&
+		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
+			drm_info(&i915->drm,
+				 "Display fused off, disabling\n");
+			goto display_fused_off;
+		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
+			drm_info(&i915->drm, "PipeC fused off\n");
+			display_runtime->pipe_mask &= ~BIT(PIPE_C);
+			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
+		}
+	} else if (DISPLAY_VER(i915) >= 9) {
+		u32 dfsm = intel_de_read(i915, SKL_DFSM);
+
+		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
+			display_runtime->pipe_mask &= ~BIT(PIPE_A);
+			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
+			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
+		}
+		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
+			display_runtime->pipe_mask &= ~BIT(PIPE_B);
+			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
+		}
+		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
+			display_runtime->pipe_mask &= ~BIT(PIPE_C);
+			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
+		}
+
+		if (DISPLAY_VER(i915) >= 12 &&
+		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
+			display_runtime->pipe_mask &= ~BIT(PIPE_D);
+			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
+		}
+
+		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
+			display_runtime->has_hdcp = 0;
+
+		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
+			display_runtime->fbc_mask = 0;
+
+		if (DISPLAY_VER(i915) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
+			display_runtime->has_dmc = 0;
+
+		if (IS_DISPLAY_VER(i915, 10, 12) &&
+		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
+			display_runtime->has_dsc = 0;
+	}
+
+	return;
+
+display_fused_off:
+	memset(display_runtime, 0, sizeof(*display_runtime));
+}
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 2aa82cbdf1c5..4f931258d81d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -124,5 +124,6 @@ struct intel_display_device_info {
 const struct intel_display_device_info *
 intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
 			   u16 *ver, u16 *rel, u16 *step);
+void intel_display_device_info_runtime_init(struct drm_i915_private *i915);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 2f79d232b04a..ed6183aaaa5c 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -27,9 +27,7 @@
 #include <drm/drm_print.h>
 #include <drm/i915_pciids.h>
 
-#include "display/intel_cdclk.h"
-#include "display/intel_de.h"
-#include "display/intel_display.h"
+#include "display/intel_display_device.h"
 #include "gt/intel_gt_regs.h"
 #include "i915_drv.h"
 #include "i915_reg.h"
@@ -411,126 +409,12 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_device_info *info = mkwrite_device_info(dev_priv);
 	struct intel_runtime_info *runtime = RUNTIME_INFO(dev_priv);
-	struct intel_display_runtime_info *display_runtime =
-		DISPLAY_RUNTIME_INFO(dev_priv);
-	enum pipe pipe;
 
-	/* Wa_14011765242: adl-s A0,A1 */
-	if (IS_ADLS_DISPLAY_STEP(dev_priv, STEP_A0, STEP_A2))
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_scalers[pipe] = 0;
-	else if (DISPLAY_VER(dev_priv) >= 11) {
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_scalers[pipe] = 2;
-	} else if (DISPLAY_VER(dev_priv) >= 9) {
-		display_runtime->num_scalers[PIPE_A] = 2;
-		display_runtime->num_scalers[PIPE_B] = 2;
-		display_runtime->num_scalers[PIPE_C] = 1;
-	}
+	if (HAS_DISPLAY(dev_priv))
+		intel_display_device_info_runtime_init(dev_priv);
 
 	BUILD_BUG_ON(BITS_PER_TYPE(intel_engine_mask_t) < I915_NUM_ENGINES);
 
-	if (DISPLAY_VER(dev_priv) >= 13 || HAS_D12_PLANE_MINIMIZATION(dev_priv))
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_sprites[pipe] = 4;
-	else if (DISPLAY_VER(dev_priv) >= 11)
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_sprites[pipe] = 6;
-	else if (DISPLAY_VER(dev_priv) == 10)
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_sprites[pipe] = 3;
-	else if (IS_BROXTON(dev_priv)) {
-		/*
-		 * Skylake and Broxton currently don't expose the topmost plane as its
-		 * use is exclusive with the legacy cursor and we only want to expose
-		 * one of those, not both. Until we can safely expose the topmost plane
-		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
-		 * we don't expose the topmost plane at all to prevent ABI breakage
-		 * down the line.
-		 */
-
-		display_runtime->num_sprites[PIPE_A] = 2;
-		display_runtime->num_sprites[PIPE_B] = 2;
-		display_runtime->num_sprites[PIPE_C] = 1;
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_sprites[pipe] = 2;
-	} else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv)) {
-		for_each_pipe(dev_priv, pipe)
-			display_runtime->num_sprites[pipe] = 1;
-	}
-
-	if (HAS_DISPLAY(dev_priv) &&
-	    (IS_DGFX(dev_priv) || DISPLAY_VER(dev_priv) >= 14) &&
-	    !(intel_de_read(dev_priv, GU_CNTL_PROTECTED) & DEPRESENT)) {
-		drm_info(&dev_priv->drm, "Display not present, disabling\n");
-
-		display_runtime->pipe_mask = 0;
-	}
-
-	if (HAS_DISPLAY(dev_priv) && IS_GRAPHICS_VER(dev_priv, 7, 8) &&
-	    HAS_PCH_SPLIT(dev_priv)) {
-		u32 fuse_strap = intel_de_read(dev_priv, FUSE_STRAP);
-		u32 sfuse_strap = intel_de_read(dev_priv, SFUSE_STRAP);
-
-		/*
-		 * SFUSE_STRAP is supposed to have a bit signalling the display
-		 * is fused off. Unfortunately it seems that, at least in
-		 * certain cases, fused off display means that PCH display
-		 * reads don't land anywhere. In that case, we read 0s.
-		 *
-		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
-		 * should be set when taking over after the firmware.
-		 */
-		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
-		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
-		    (HAS_PCH_CPT(dev_priv) &&
-		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
-			drm_info(&dev_priv->drm,
-				 "Display fused off, disabling\n");
-			display_runtime->pipe_mask = 0;
-		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
-			drm_info(&dev_priv->drm, "PipeC fused off\n");
-			display_runtime->pipe_mask &= ~BIT(PIPE_C);
-			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
-		}
-	} else if (HAS_DISPLAY(dev_priv) && DISPLAY_VER(dev_priv) >= 9) {
-		u32 dfsm = intel_de_read(dev_priv, SKL_DFSM);
-
-		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
-			display_runtime->pipe_mask &= ~BIT(PIPE_A);
-			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
-			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
-		}
-		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
-			display_runtime->pipe_mask &= ~BIT(PIPE_B);
-			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
-		}
-		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
-			display_runtime->pipe_mask &= ~BIT(PIPE_C);
-			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
-		}
-
-		if (DISPLAY_VER(dev_priv) >= 12 &&
-		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
-			display_runtime->pipe_mask &= ~BIT(PIPE_D);
-			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
-		}
-
-		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
-			display_runtime->has_hdcp = 0;
-
-		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
-			display_runtime->fbc_mask = 0;
-
-		if (DISPLAY_VER(dev_priv) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
-			display_runtime->has_dmc = 0;
-
-		if (IS_DISPLAY_VER(dev_priv, 10, 12) &&
-		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
-			display_runtime->has_dsc = 0;
-	}
-
 	if (GRAPHICS_VER(dev_priv) == 6 && i915_vtd_active(dev_priv)) {
 		drm_info(&dev_priv->drm,
 			 "Disabling ppGTT for VT-d support\n");
@@ -544,14 +428,6 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 		dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
 						   DRIVER_ATOMIC);
 		info->display = &no_display;
-
-		display_runtime->cpu_transcoder_mask = 0;
-		memset(display_runtime->num_sprites, 0, sizeof(display_runtime->num_sprites));
-		memset(display_runtime->num_scalers, 0, sizeof(display_runtime->num_scalers));
-		display_runtime->fbc_mask = 0;
-		display_runtime->has_hdcp = false;
-		display_runtime->has_dmc = false;
-		display_runtime->has_dsc = false;
 	}
 
 	/* Disable nuclear pageflip by default on pre-g4x */
-- 
2.40.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Extract display init from intel_device_info_runtime_init

[Patchwork]

== Series Details ==

Series: drm/i915/display: Extract display init from intel_device_info_runtime_init
URL   : https://patchwork.freedesktop.org/series/118730/
State : warning

== Summary ==

Error: dim checkpatch failed
7e88c49f41ae drm/i915/display: Extract display init from intel_device_info_runtime_init
-:304: CHECK:SPACING: No space is necessary after a cast
#304: FILE: drivers/gpu/drm/i915/intel_device_info.c:416:
+	BUILD_BUG_ON(BITS_PER_TYPE(intel_engine_mask_t) < I915_NUM_ENGINES);

total: 0 errors, 0 warnings, 1 checks, 291 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Extract display init from intel_device_info_runtime_init

[Patchwork]

== Series Details ==

Series: drm/i915/display: Extract display init from intel_device_info_runtime_init
URL   : https://patchwork.freedesktop.org/series/118730/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display: Extract display init from intel_device_info_runtime_init

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5782 bytes --]

== Series Details ==

Series: drm/i915/display: Extract display init from intel_device_info_runtime_init
URL   : https://patchwork.freedesktop.org/series/118730/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13217 -> Patchwork_118730v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/index.html

Participating hosts (38 -> 38)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in Patchwork_118730v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [PASS][1] -> [ABORT][2] ([i915#4983] / [i915#7461] / [i915#8347] / [i915#8384])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-rpls-1/igt@i915_selftest@live@reset.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-rpls-1/igt@i915_selftest@live@reset.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-dg2-11:         NOTRUN -> [SKIP][3] ([i915#7828])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-dg2-11/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][4] -> [FAIL][5] ([i915#7932])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-adlp-9:         NOTRUN -> [SKIP][6] ([i915#3546]) +1 similar issue
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html
    - bat-dg2-11:         NOTRUN -> [SKIP][7] ([i915#1845] / [i915#5354]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc.html

  
#### Possible fixes ####

  * igt@i915_module_load@load:
    - {bat-adlp-11}:      [ABORT][8] ([i915#4423]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-adlp-11/igt@i915_module_load@load.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-adlp-11/igt@i915_module_load@load.html

  * igt@i915_selftest@live@gt_mocs:
    - {bat-mtlp-8}:       [DMESG-FAIL][10] ([i915#7059]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html

  * igt@i915_selftest@live@gt_pm:
    - bat-rpls-2:         [DMESG-FAIL][12] ([i915#4258] / [i915#7913]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-rpls-2/igt@i915_selftest@live@gt_pm.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-rpls-2/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@reset:
    - bat-dg2-11:         [INCOMPLETE][14] ([i915#7913]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-11/igt@i915_selftest@live@reset.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-dg2-11/igt@i915_selftest@live@reset.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [FAIL][16] ([i915#7932]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4423]: https://gitlab.freedesktop.org/drm/intel/issues/4423
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#7059]: https://gitlab.freedesktop.org/drm/intel/issues/7059
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347
  [i915#8384]: https://gitlab.freedesktop.org/drm/intel/issues/8384
  [i915#8497]: https://gitlab.freedesktop.org/drm/intel/issues/8497


Build changes
-------------

  * Linux: CI_DRM_13217 -> Patchwork_118730v1

  CI-20190529: 20190529
  CI_DRM_13217: 37b9b6d05f2421323eb83d005d8863f59855b003 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7317: c902b72df45aa49faa38205bc5be3c748d33a3e0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_118730v1: 37b9b6d05f2421323eb83d005d8863f59855b003 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

2192511b2ba4 drm/i915/display: Extract display init from intel_device_info_runtime_init

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118730v1/index.html

[-- Attachment #2: Type: text/html, Size: 6555 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init

[Jani Nikula]

On Thu, 01 Jun 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> Moving display-specific runtime info initialization into display/ makes
> the display code more self-contained and also makes it easier to call
> from the Xe driver.

I like the direction here. A few minor issues, comments inline.

>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  .../drm/i915/display/intel_display_device.c   | 124 +++++++++++++++++
>  .../drm/i915/display/intel_display_device.h   |   1 +
>  drivers/gpu/drm/i915/intel_device_info.c      | 130 +-----------------
>  3 files changed, 128 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 464df1764a86..8d379da877dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -7,6 +7,8 @@
>  #include <drm/drm_color_mgmt.h>
>  #include <linux/pci.h>
>  
> +#include "display/intel_de.h"
> +#include "display/intel_display.h"

The display/ prefix should be dropped.

>  #include "i915_drv.h"
>  #include "i915_reg.h"
>  #include "intel_display_device.h"
> @@ -778,3 +780,125 @@ intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
>  
>  	return &no_display;
>  }
> +
> +void intel_display_device_info_runtime_init(struct drm_i915_private *i915)
> +{
> +	struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(i915);
> +	enum pipe pipe;
> +
> +	/* Wa_14011765242: adl-s A0,A1 */
> +	if (IS_ADLS_DISPLAY_STEP(i915, STEP_A0, STEP_A2))
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_scalers[pipe] = 0;
> +	else if (DISPLAY_VER(i915) >= 11) {
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_scalers[pipe] = 2;
> +	} else if (DISPLAY_VER(i915) >= 9) {
> +		display_runtime->num_scalers[PIPE_A] = 2;
> +		display_runtime->num_scalers[PIPE_B] = 2;
> +		display_runtime->num_scalers[PIPE_C] = 1;
> +	}
> +
> +	if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_sprites[pipe] = 4;
> +	else if (DISPLAY_VER(i915) >= 11)
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_sprites[pipe] = 6;
> +	else if (DISPLAY_VER(i915) == 10)
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_sprites[pipe] = 3;
> +	else if (IS_BROXTON(i915)) {
> +		/*
> +		 * Skylake and Broxton currently don't expose the topmost plane as its
> +		 * use is exclusive with the legacy cursor and we only want to expose
> +		 * one of those, not both. Until we can safely expose the topmost plane
> +		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> +		 * we don't expose the topmost plane at all to prevent ABI breakage
> +		 * down the line.
> +		 */
> +
> +		display_runtime->num_sprites[PIPE_A] = 2;
> +		display_runtime->num_sprites[PIPE_B] = 2;
> +		display_runtime->num_sprites[PIPE_C] = 1;
> +	} else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_sprites[pipe] = 2;
> +	} else if (DISPLAY_VER(i915) >= 5 || IS_G4X(i915)) {
> +		for_each_pipe(i915, pipe)
> +			display_runtime->num_sprites[pipe] = 1;
> +	}
> +
> +	if ((IS_DGFX(i915) || DISPLAY_VER(i915) >= 14) &&
> +	    !(intel_de_read(i915, GU_CNTL_PROTECTED) & DEPRESENT)) {
> +		drm_info(&i915->drm, "Display not present, disabling\n");
> +		goto display_fused_off;
> +	}
> +
> +	if (IS_GRAPHICS_VER(i915, 7, 8) && HAS_PCH_SPLIT(i915)) {
> +		u32 fuse_strap = intel_de_read(i915, FUSE_STRAP);
> +		u32 sfuse_strap = intel_de_read(i915, SFUSE_STRAP);
> +
> +		/*
> +		 * SFUSE_STRAP is supposed to have a bit signalling the display
> +		 * is fused off. Unfortunately it seems that, at least in
> +		 * certain cases, fused off display means that PCH display
> +		 * reads don't land anywhere. In that case, we read 0s.
> +		 *
> +		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> +		 * should be set when taking over after the firmware.
> +		 */
> +		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> +		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> +		    (HAS_PCH_CPT(i915) &&
> +		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> +			drm_info(&i915->drm,
> +				 "Display fused off, disabling\n");
> +			goto display_fused_off;
> +		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> +			drm_info(&i915->drm, "PipeC fused off\n");
> +			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> +		}
> +	} else if (DISPLAY_VER(i915) >= 9) {
> +		u32 dfsm = intel_de_read(i915, SKL_DFSM);
> +
> +		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> +			display_runtime->pipe_mask &= ~BIT(PIPE_A);
> +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> +			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> +		}
> +		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> +			display_runtime->pipe_mask &= ~BIT(PIPE_B);
> +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> +		}
> +		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> +			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> +		}
> +
> +		if (DISPLAY_VER(i915) >= 12 &&
> +		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> +			display_runtime->pipe_mask &= ~BIT(PIPE_D);
> +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> +		}

I'm not sure if it's likely at all, but what if ->pipe_mask == 0 after
the disables above? Then this no longer clears display runtime info like
it used to.

Although later on that still leads to setting info->display =
&no_display. But I guess I'd like to not make functional changes like
that here if possible.

> +
> +		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> +			display_runtime->has_hdcp = 0;
> +
> +		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> +			display_runtime->fbc_mask = 0;
> +
> +		if (DISPLAY_VER(i915) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> +			display_runtime->has_dmc = 0;
> +
> +		if (IS_DISPLAY_VER(i915, 10, 12) &&
> +		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> +			display_runtime->has_dsc = 0;
> +	}
> +
> +	return;
> +
> +display_fused_off:
> +	memset(display_runtime, 0, sizeof(*display_runtime));
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 2aa82cbdf1c5..4f931258d81d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -124,5 +124,6 @@ struct intel_display_device_info {
>  const struct intel_display_device_info *
>  intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
>  			   u16 *ver, u16 *rel, u16 *step);
> +void intel_display_device_info_runtime_init(struct drm_i915_private *i915);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 2f79d232b04a..ed6183aaaa5c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -27,9 +27,7 @@
>  #include <drm/drm_print.h>
>  #include <drm/i915_pciids.h>
>  
> -#include "display/intel_cdclk.h"
> -#include "display/intel_de.h"
> -#include "display/intel_display.h"
> +#include "display/intel_display_device.h"
>  #include "gt/intel_gt_regs.h"
>  #include "i915_drv.h"
>  #include "i915_reg.h"
> @@ -411,126 +409,12 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_device_info *info = mkwrite_device_info(dev_priv);
>  	struct intel_runtime_info *runtime = RUNTIME_INFO(dev_priv);
> -	struct intel_display_runtime_info *display_runtime =
> -		DISPLAY_RUNTIME_INFO(dev_priv);
> -	enum pipe pipe;
>  
> -	/* Wa_14011765242: adl-s A0,A1 */
> -	if (IS_ADLS_DISPLAY_STEP(dev_priv, STEP_A0, STEP_A2))
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_scalers[pipe] = 0;
> -	else if (DISPLAY_VER(dev_priv) >= 11) {
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_scalers[pipe] = 2;
> -	} else if (DISPLAY_VER(dev_priv) >= 9) {
> -		display_runtime->num_scalers[PIPE_A] = 2;
> -		display_runtime->num_scalers[PIPE_B] = 2;
> -		display_runtime->num_scalers[PIPE_C] = 1;
> -	}
> +	if (HAS_DISPLAY(dev_priv))
> +		intel_display_device_info_runtime_init(dev_priv);

I'm wondering if it would make sense to have that function return a
value that would get used instead of the later !HAS_DISPLAY() branch
later. Now it feels like there's a bit of a disconnect between the
two. Or at least move that block right here:

	if (HAS_DISPLAY(dev_priv)) {
		intel_display_device_info_runtime_init(dev_priv);

		if (!HAS_DISPLAY(dev_priv)) {
			...
		}
	}

This way there's more clarity I think. Can be a follow-up patch too.

I think the contents of that branch i.e.

		dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
						   DRIVER_ATOMIC);
		info->display = &no_display;

should stay here (like you've left them) to have all of that at about
the same layer.

BR,
Jani.


>  
>  	BUILD_BUG_ON(BITS_PER_TYPE(intel_engine_mask_t) < I915_NUM_ENGINES);
>  
> -	if (DISPLAY_VER(dev_priv) >= 13 || HAS_D12_PLANE_MINIMIZATION(dev_priv))
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_sprites[pipe] = 4;
> -	else if (DISPLAY_VER(dev_priv) >= 11)
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_sprites[pipe] = 6;
> -	else if (DISPLAY_VER(dev_priv) == 10)
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_sprites[pipe] = 3;
> -	else if (IS_BROXTON(dev_priv)) {
> -		/*
> -		 * Skylake and Broxton currently don't expose the topmost plane as its
> -		 * use is exclusive with the legacy cursor and we only want to expose
> -		 * one of those, not both. Until we can safely expose the topmost plane
> -		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> -		 * we don't expose the topmost plane at all to prevent ABI breakage
> -		 * down the line.
> -		 */
> -
> -		display_runtime->num_sprites[PIPE_A] = 2;
> -		display_runtime->num_sprites[PIPE_B] = 2;
> -		display_runtime->num_sprites[PIPE_C] = 1;
> -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_sprites[pipe] = 2;
> -	} else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv)) {
> -		for_each_pipe(dev_priv, pipe)
> -			display_runtime->num_sprites[pipe] = 1;
> -	}
> -
> -	if (HAS_DISPLAY(dev_priv) &&
> -	    (IS_DGFX(dev_priv) || DISPLAY_VER(dev_priv) >= 14) &&
> -	    !(intel_de_read(dev_priv, GU_CNTL_PROTECTED) & DEPRESENT)) {
> -		drm_info(&dev_priv->drm, "Display not present, disabling\n");
> -
> -		display_runtime->pipe_mask = 0;
> -	}
> -
> -	if (HAS_DISPLAY(dev_priv) && IS_GRAPHICS_VER(dev_priv, 7, 8) &&
> -	    HAS_PCH_SPLIT(dev_priv)) {
> -		u32 fuse_strap = intel_de_read(dev_priv, FUSE_STRAP);
> -		u32 sfuse_strap = intel_de_read(dev_priv, SFUSE_STRAP);
> -
> -		/*
> -		 * SFUSE_STRAP is supposed to have a bit signalling the display
> -		 * is fused off. Unfortunately it seems that, at least in
> -		 * certain cases, fused off display means that PCH display
> -		 * reads don't land anywhere. In that case, we read 0s.
> -		 *
> -		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> -		 * should be set when taking over after the firmware.
> -		 */
> -		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> -		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> -		    (HAS_PCH_CPT(dev_priv) &&
> -		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> -			drm_info(&dev_priv->drm,
> -				 "Display fused off, disabling\n");
> -			display_runtime->pipe_mask = 0;
> -		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> -			drm_info(&dev_priv->drm, "PipeC fused off\n");
> -			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> -		}
> -	} else if (HAS_DISPLAY(dev_priv) && DISPLAY_VER(dev_priv) >= 9) {
> -		u32 dfsm = intel_de_read(dev_priv, SKL_DFSM);
> -
> -		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> -			display_runtime->pipe_mask &= ~BIT(PIPE_A);
> -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> -			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> -		}
> -		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> -			display_runtime->pipe_mask &= ~BIT(PIPE_B);
> -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> -		}
> -		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> -			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> -		}
> -
> -		if (DISPLAY_VER(dev_priv) >= 12 &&
> -		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> -			display_runtime->pipe_mask &= ~BIT(PIPE_D);
> -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> -		}
> -
> -		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> -			display_runtime->has_hdcp = 0;
> -
> -		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> -			display_runtime->fbc_mask = 0;
> -
> -		if (DISPLAY_VER(dev_priv) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> -			display_runtime->has_dmc = 0;
> -
> -		if (IS_DISPLAY_VER(dev_priv, 10, 12) &&
> -		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> -			display_runtime->has_dsc = 0;
> -	}
> -
>  	if (GRAPHICS_VER(dev_priv) == 6 && i915_vtd_active(dev_priv)) {
>  		drm_info(&dev_priv->drm,
>  			 "Disabling ppGTT for VT-d support\n");
> @@ -544,14 +428,6 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>  		dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
>  						   DRIVER_ATOMIC);
>  		info->display = &no_display;
> -
> -		display_runtime->cpu_transcoder_mask = 0;
> -		memset(display_runtime->num_sprites, 0, sizeof(display_runtime->num_sprites));
> -		memset(display_runtime->num_scalers, 0, sizeof(display_runtime->num_scalers));
> -		display_runtime->fbc_mask = 0;
> -		display_runtime->has_hdcp = false;
> -		display_runtime->has_dmc = false;
> -		display_runtime->has_dsc = false;
>  	}
>  
>  	/* Disable nuclear pageflip by default on pre-g4x */

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init

[Matt Roper]

On Fri, Jun 02, 2023 at 01:04:06PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> > Moving display-specific runtime info initialization into display/ makes
> > the display code more self-contained and also makes it easier to call
> > from the Xe driver.
> 
> I like the direction here. A few minor issues, comments inline.
> 
> >
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_device.c   | 124 +++++++++++++++++
> >  .../drm/i915/display/intel_display_device.h   |   1 +
> >  drivers/gpu/drm/i915/intel_device_info.c      | 130 +-----------------
> >  3 files changed, 128 insertions(+), 127 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> > index 464df1764a86..8d379da877dc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > @@ -7,6 +7,8 @@
> >  #include <drm/drm_color_mgmt.h>
> >  #include <linux/pci.h>
> >  
> > +#include "display/intel_de.h"
> > +#include "display/intel_display.h"
> 
> The display/ prefix should be dropped.
> 
> >  #include "i915_drv.h"
> >  #include "i915_reg.h"
> >  #include "intel_display_device.h"
> > @@ -778,3 +780,125 @@ intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
> >  
> >  	return &no_display;
> >  }
> > +
> > +void intel_display_device_info_runtime_init(struct drm_i915_private *i915)
> > +{
> > +	struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(i915);
> > +	enum pipe pipe;
> > +
> > +	/* Wa_14011765242: adl-s A0,A1 */
> > +	if (IS_ADLS_DISPLAY_STEP(i915, STEP_A0, STEP_A2))
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_scalers[pipe] = 0;
> > +	else if (DISPLAY_VER(i915) >= 11) {
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_scalers[pipe] = 2;
> > +	} else if (DISPLAY_VER(i915) >= 9) {
> > +		display_runtime->num_scalers[PIPE_A] = 2;
> > +		display_runtime->num_scalers[PIPE_B] = 2;
> > +		display_runtime->num_scalers[PIPE_C] = 1;
> > +	}
> > +
> > +	if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_sprites[pipe] = 4;
> > +	else if (DISPLAY_VER(i915) >= 11)
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_sprites[pipe] = 6;
> > +	else if (DISPLAY_VER(i915) == 10)
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_sprites[pipe] = 3;
> > +	else if (IS_BROXTON(i915)) {
> > +		/*
> > +		 * Skylake and Broxton currently don't expose the topmost plane as its
> > +		 * use is exclusive with the legacy cursor and we only want to expose
> > +		 * one of those, not both. Until we can safely expose the topmost plane
> > +		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> > +		 * we don't expose the topmost plane at all to prevent ABI breakage
> > +		 * down the line.
> > +		 */
> > +
> > +		display_runtime->num_sprites[PIPE_A] = 2;
> > +		display_runtime->num_sprites[PIPE_B] = 2;
> > +		display_runtime->num_sprites[PIPE_C] = 1;
> > +	} else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_sprites[pipe] = 2;
> > +	} else if (DISPLAY_VER(i915) >= 5 || IS_G4X(i915)) {
> > +		for_each_pipe(i915, pipe)
> > +			display_runtime->num_sprites[pipe] = 1;
> > +	}
> > +
> > +	if ((IS_DGFX(i915) || DISPLAY_VER(i915) >= 14) &&
> > +	    !(intel_de_read(i915, GU_CNTL_PROTECTED) & DEPRESENT)) {
> > +		drm_info(&i915->drm, "Display not present, disabling\n");
> > +		goto display_fused_off;
> > +	}
> > +
> > +	if (IS_GRAPHICS_VER(i915, 7, 8) && HAS_PCH_SPLIT(i915)) {
> > +		u32 fuse_strap = intel_de_read(i915, FUSE_STRAP);
> > +		u32 sfuse_strap = intel_de_read(i915, SFUSE_STRAP);
> > +
> > +		/*
> > +		 * SFUSE_STRAP is supposed to have a bit signalling the display
> > +		 * is fused off. Unfortunately it seems that, at least in
> > +		 * certain cases, fused off display means that PCH display
> > +		 * reads don't land anywhere. In that case, we read 0s.
> > +		 *
> > +		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> > +		 * should be set when taking over after the firmware.
> > +		 */
> > +		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> > +		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> > +		    (HAS_PCH_CPT(i915) &&
> > +		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> > +			drm_info(&i915->drm,
> > +				 "Display fused off, disabling\n");
> > +			goto display_fused_off;
> > +		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> > +			drm_info(&i915->drm, "PipeC fused off\n");
> > +			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> > +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> > +		}
> > +	} else if (DISPLAY_VER(i915) >= 9) {
> > +		u32 dfsm = intel_de_read(i915, SKL_DFSM);
> > +
> > +		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> > +			display_runtime->pipe_mask &= ~BIT(PIPE_A);
> > +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> > +			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> > +		}
> > +		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> > +			display_runtime->pipe_mask &= ~BIT(PIPE_B);
> > +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> > +		}
> > +		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> > +			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> > +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> > +		}
> > +
> > +		if (DISPLAY_VER(i915) >= 12 &&
> > +		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> > +			display_runtime->pipe_mask &= ~BIT(PIPE_D);
> > +			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> > +		}
> 
> I'm not sure if it's likely at all, but what if ->pipe_mask == 0 after
> the disables above? Then this no longer clears display runtime info like
> it used to.
> 
> Although later on that still leads to setting info->display =
> &no_display. But I guess I'd like to not make functional changes like
> that here if possible.
> 
> > +
> > +		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> > +			display_runtime->has_hdcp = 0;
> > +
> > +		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> > +			display_runtime->fbc_mask = 0;
> > +
> > +		if (DISPLAY_VER(i915) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> > +			display_runtime->has_dmc = 0;
> > +
> > +		if (IS_DISPLAY_VER(i915, 10, 12) &&
> > +		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> > +			display_runtime->has_dsc = 0;
> > +	}
> > +
> > +	return;
> > +
> > +display_fused_off:
> > +	memset(display_runtime, 0, sizeof(*display_runtime));
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 2aa82cbdf1c5..4f931258d81d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -124,5 +124,6 @@ struct intel_display_device_info {
> >  const struct intel_display_device_info *
> >  intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
> >  			   u16 *ver, u16 *rel, u16 *step);
> > +void intel_display_device_info_runtime_init(struct drm_i915_private *i915);
> >  
> >  #endif
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index 2f79d232b04a..ed6183aaaa5c 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -27,9 +27,7 @@
> >  #include <drm/drm_print.h>
> >  #include <drm/i915_pciids.h>
> >  
> > -#include "display/intel_cdclk.h"
> > -#include "display/intel_de.h"
> > -#include "display/intel_display.h"
> > +#include "display/intel_display_device.h"
> >  #include "gt/intel_gt_regs.h"
> >  #include "i915_drv.h"
> >  #include "i915_reg.h"
> > @@ -411,126 +409,12 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_device_info *info = mkwrite_device_info(dev_priv);
> >  	struct intel_runtime_info *runtime = RUNTIME_INFO(dev_priv);
> > -	struct intel_display_runtime_info *display_runtime =
> > -		DISPLAY_RUNTIME_INFO(dev_priv);
> > -	enum pipe pipe;
> >  
> > -	/* Wa_14011765242: adl-s A0,A1 */
> > -	if (IS_ADLS_DISPLAY_STEP(dev_priv, STEP_A0, STEP_A2))
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_scalers[pipe] = 0;
> > -	else if (DISPLAY_VER(dev_priv) >= 11) {
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_scalers[pipe] = 2;
> > -	} else if (DISPLAY_VER(dev_priv) >= 9) {
> > -		display_runtime->num_scalers[PIPE_A] = 2;
> > -		display_runtime->num_scalers[PIPE_B] = 2;
> > -		display_runtime->num_scalers[PIPE_C] = 1;
> > -	}
> > +	if (HAS_DISPLAY(dev_priv))
> > +		intel_display_device_info_runtime_init(dev_priv);
> 
> I'm wondering if it would make sense to have that function return a
> value that would get used instead of the later !HAS_DISPLAY() branch
> later. Now it feels like there's a bit of a disconnect between the
> two. Or at least move that block right here:
> 
> 	if (HAS_DISPLAY(dev_priv)) {
> 		intel_display_device_info_runtime_init(dev_priv);
> 
> 		if (!HAS_DISPLAY(dev_priv)) {
> 			...

This probably shouldn't be nested since we still want to run this even
if the first HAS_DISPLAY was already false before we read these fuses
and such (e.g., on a PVC or ATS-M).  But moving these next to each other
sounds like a good idea.


Matt


> 		}
> 	}
> 
> This way there's more clarity I think. Can be a follow-up patch too.
> 
> I think the contents of that branch i.e.
> 
> 		dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
> 						   DRIVER_ATOMIC);
> 		info->display = &no_display;
> 
> should stay here (like you've left them) to have all of that at about
> the same layer.
> 
> BR,
> Jani.
> 
> 
> >  
> >  	BUILD_BUG_ON(BITS_PER_TYPE(intel_engine_mask_t) < I915_NUM_ENGINES);
> >  
> > -	if (DISPLAY_VER(dev_priv) >= 13 || HAS_D12_PLANE_MINIMIZATION(dev_priv))
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_sprites[pipe] = 4;
> > -	else if (DISPLAY_VER(dev_priv) >= 11)
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_sprites[pipe] = 6;
> > -	else if (DISPLAY_VER(dev_priv) == 10)
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_sprites[pipe] = 3;
> > -	else if (IS_BROXTON(dev_priv)) {
> > -		/*
> > -		 * Skylake and Broxton currently don't expose the topmost plane as its
> > -		 * use is exclusive with the legacy cursor and we only want to expose
> > -		 * one of those, not both. Until we can safely expose the topmost plane
> > -		 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> > -		 * we don't expose the topmost plane at all to prevent ABI breakage
> > -		 * down the line.
> > -		 */
> > -
> > -		display_runtime->num_sprites[PIPE_A] = 2;
> > -		display_runtime->num_sprites[PIPE_B] = 2;
> > -		display_runtime->num_sprites[PIPE_C] = 1;
> > -	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_sprites[pipe] = 2;
> > -	} else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv)) {
> > -		for_each_pipe(dev_priv, pipe)
> > -			display_runtime->num_sprites[pipe] = 1;
> > -	}
> > -
> > -	if (HAS_DISPLAY(dev_priv) &&
> > -	    (IS_DGFX(dev_priv) || DISPLAY_VER(dev_priv) >= 14) &&
> > -	    !(intel_de_read(dev_priv, GU_CNTL_PROTECTED) & DEPRESENT)) {
> > -		drm_info(&dev_priv->drm, "Display not present, disabling\n");
> > -
> > -		display_runtime->pipe_mask = 0;
> > -	}
> > -
> > -	if (HAS_DISPLAY(dev_priv) && IS_GRAPHICS_VER(dev_priv, 7, 8) &&
> > -	    HAS_PCH_SPLIT(dev_priv)) {
> > -		u32 fuse_strap = intel_de_read(dev_priv, FUSE_STRAP);
> > -		u32 sfuse_strap = intel_de_read(dev_priv, SFUSE_STRAP);
> > -
> > -		/*
> > -		 * SFUSE_STRAP is supposed to have a bit signalling the display
> > -		 * is fused off. Unfortunately it seems that, at least in
> > -		 * certain cases, fused off display means that PCH display
> > -		 * reads don't land anywhere. In that case, we read 0s.
> > -		 *
> > -		 * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> > -		 * should be set when taking over after the firmware.
> > -		 */
> > -		if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> > -		    sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> > -		    (HAS_PCH_CPT(dev_priv) &&
> > -		     !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> > -			drm_info(&dev_priv->drm,
> > -				 "Display fused off, disabling\n");
> > -			display_runtime->pipe_mask = 0;
> > -		} else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> > -			drm_info(&dev_priv->drm, "PipeC fused off\n");
> > -			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> > -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> > -		}
> > -	} else if (HAS_DISPLAY(dev_priv) && DISPLAY_VER(dev_priv) >= 9) {
> > -		u32 dfsm = intel_de_read(dev_priv, SKL_DFSM);
> > -
> > -		if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> > -			display_runtime->pipe_mask &= ~BIT(PIPE_A);
> > -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> > -			display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> > -		}
> > -		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> > -			display_runtime->pipe_mask &= ~BIT(PIPE_B);
> > -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> > -		}
> > -		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> > -			display_runtime->pipe_mask &= ~BIT(PIPE_C);
> > -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> > -		}
> > -
> > -		if (DISPLAY_VER(dev_priv) >= 12 &&
> > -		    (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> > -			display_runtime->pipe_mask &= ~BIT(PIPE_D);
> > -			display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> > -		}
> > -
> > -		if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> > -			display_runtime->has_hdcp = 0;
> > -
> > -		if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> > -			display_runtime->fbc_mask = 0;
> > -
> > -		if (DISPLAY_VER(dev_priv) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> > -			display_runtime->has_dmc = 0;
> > -
> > -		if (IS_DISPLAY_VER(dev_priv, 10, 12) &&
> > -		    (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> > -			display_runtime->has_dsc = 0;
> > -	}
> > -
> >  	if (GRAPHICS_VER(dev_priv) == 6 && i915_vtd_active(dev_priv)) {
> >  		drm_info(&dev_priv->drm,
> >  			 "Disabling ppGTT for VT-d support\n");
> > @@ -544,14 +428,6 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> >  		dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
> >  						   DRIVER_ATOMIC);
> >  		info->display = &no_display;
> > -
> > -		display_runtime->cpu_transcoder_mask = 0;
> > -		memset(display_runtime->num_sprites, 0, sizeof(display_runtime->num_sprites));
> > -		memset(display_runtime->num_scalers, 0, sizeof(display_runtime->num_scalers));
> > -		display_runtime->fbc_mask = 0;
> > -		display_runtime->has_hdcp = false;
> > -		display_runtime->has_dmc = false;
> > -		display_runtime->has_dsc = false;
> >  	}
> >  
> >  	/* Disable nuclear pageflip by default on pre-g4x */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation