* [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init
@ 2023-06-01 21:25 Matt Roper
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Matt Roper @ 2023-06-01 21:25 UTC (permalink / raw)
To: intel-gfx; +Cc: matthew.d.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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Extract display init from intel_device_info_runtime_init
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
@ 2023-06-01 23:31 ` Patchwork
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2023-06-01 23:31 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Extract display init from intel_device_info_runtime_init
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2023-06-01 23:31 ` Patchwork
2023-06-01 23:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-06-02 10:04 ` [Intel-gfx] [PATCH] " Jani Nikula
3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2023-06-01 23:31 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
== 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display: Extract display init from intel_device_info_runtime_init
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2023-06-01 23:45 ` Patchwork
2023-06-02 10:04 ` [Intel-gfx] [PATCH] " Jani Nikula
3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2023-06-01 23:45 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
` (2 preceding siblings ...)
2023-06-01 23:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2023-06-02 10:04 ` Jani Nikula
2023-06-02 18:04 ` Matt Roper
3 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2023-06-02 10:04 UTC (permalink / raw)
To: Matt Roper, intel-gfx; +Cc: matthew.d.roper
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init
2023-06-02 10:04 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2023-06-02 18:04 ` Matt Roper
0 siblings, 0 replies; 6+ messages in thread
From: Matt Roper @ 2023-06-02 18:04 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-02 18:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-06-01 23:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-06-02 10:04 ` [Intel-gfx] [PATCH] " Jani Nikula
2023-06-02 18:04 ` Matt Roper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox