[PATCH 01/16] drm/i915: Properly set PCH as NOP when display is disabled

[PATCH 01/16] drm/i915: Properly set PCH as NOP when display is disabled

[José Roberto de Souza]

num_pipes is set to 0 if disable_display is set inside
intel_device_info_runtime_init() but when that happen PCH will
already be set in intel_detect_pch().

i915_driver_load()
	i915_driver_init_early()
		...
		intel_detect_pch()
		...
	...
	i915_driver_init_hw()
		intel_device_info_runtime_init()

So now setting num_pipes = 0 earlier to avoid this problem.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c          | 5 +++++
 drivers/gpu/drm/i915/intel_device_info.c | 8 ++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index baac35f698f9..e3efc3dd8a30 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1649,6 +1649,11 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
 	memcpy(device_info, match_info, sizeof(*device_info));
 	device_info->device_id = pdev->device;
 
+	if (i915_modparams.disable_display) {
+		DRM_INFO("Display disabled (module parameter)\n");
+		device_info->num_pipes = 0;
+	}
+
 	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
 		     BITS_PER_TYPE(device_info->platform_mask));
 	BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 03df4e33763d..69be3f211737 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -775,12 +775,8 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
 			info->num_sprites[pipe] = 1;
 	}
 
-	if (i915_modparams.disable_display) {
-		DRM_INFO("Display disabled (module parameter)\n");
-		info->num_pipes = 0;
-	} else if (info->num_pipes > 0 &&
-		   (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
-		   HAS_PCH_SPLIT(dev_priv)) {
+	if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
+	    HAS_PCH_SPLIT(dev_priv)) {
 		u32 fuse_strap = I915_READ(FUSE_STRAP);
 		u32 sfuse_strap = I915_READ(SFUSE_STRAP);
 
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/16] drm/i915: Move out non-display related calls from display/modeset init/cleanup

[José Roberto de Souza]

i915_load_modeset_init() and intel_modeset_cleanup() was initializing
and cleaning up things that is not related to display or modeset.
This changes will make easy initialize driver without display block.

Also moving VLV/CHV/BYT czclk as it is a core clock used as base by
several other GPU blocks including GT.
Spec: 14370

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 81 +++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 24 ++-------
 drivers/gpu/drm/i915/intel_pm.c      | 10 ++++
 4 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e3efc3dd8a30..55212d059cca 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -664,28 +664,15 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_vga_client;
 
-	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
-	intel_update_rawclk(dev_priv);
-
-	intel_power_domains_init_hw(dev_priv, false);
-
 	intel_csr_ucode_init(dev_priv);
 
-	ret = intel_irq_install(dev_priv);
-	if (ret)
-		goto cleanup_csr;
-
 	intel_setup_gmbus(dev_priv);
 
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
 	ret = intel_modeset_init(dev);
 	if (ret)
-		goto cleanup_irq;
-
-	ret = i915_gem_init(dev_priv);
-	if (ret)
-		goto cleanup_modeset;
+		goto cleanup_gmbus;
 
 	intel_setup_overlay(dev_priv);
 
@@ -694,25 +681,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
-		goto cleanup_gem;
+		goto cleanup_modeset;
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	intel_hpd_init(dev_priv);
 
 	return 0;
 
-cleanup_gem:
-	if (i915_gem_suspend(dev_priv))
-		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
-	i915_gem_fini(dev_priv);
 cleanup_modeset:
 	intel_modeset_cleanup(dev);
-cleanup_irq:
-	drm_irq_uninstall(dev);
+cleanup_gmbus:
 	intel_teardown_gmbus(dev_priv);
-cleanup_csr:
 	intel_csr_ucode_fini(dev_priv);
-	intel_power_domains_fini_hw(dev_priv);
 	vga_switcheroo_unregister_client(pdev);
 cleanup_vga_client:
 	vga_client_register(pdev, NULL, NULL, NULL);
@@ -1728,9 +1708,25 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 			goto out_cleanup_hw;
 	}
 
+	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
+	intel_update_rawclk(dev_priv);
+
+	/* i915_gem_init() call chain will call
+	 * intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ);
+	 */
+	intel_power_domains_init_hw(dev_priv, false);
+
+	ret = intel_irq_install(dev_priv);
+	if (ret)
+		goto out_cleanup_power;
+
+	ret = i915_gem_init(dev_priv);
+	if (ret)
+		goto cleanup_irq;
+
 	ret = i915_load_modeset_init(&dev_priv->drm);
 	if (ret < 0)
-		goto out_cleanup_hw;
+		goto cleanup_gem;
 
 	i915_driver_register(dev_priv);
 
@@ -1742,6 +1738,14 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	return 0;
 
+cleanup_gem:
+	if (i915_gem_suspend(dev_priv))
+		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
+	i915_gem_fini(dev_priv);
+cleanup_irq:
+	drm_irq_uninstall(&dev_priv->drm);
+out_cleanup_power:
+	intel_power_domains_fini_hw(dev_priv);
 out_cleanup_hw:
 	i915_driver_cleanup_hw(dev_priv);
 out_cleanup_mmio:
@@ -1757,11 +1761,24 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return ret;
 }
 
-void i915_driver_unload(struct drm_device *dev)
+/* unload/cleanup the leftover of i915_load_modeset_init() */
+static void i915_modeset_unload(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
+	intel_bios_cleanup(dev_priv);
+
+	vga_switcheroo_unregister_client(pdev);
+	vga_client_register(pdev, NULL, NULL, NULL);
+
+	intel_csr_ucode_fini(dev_priv);
+}
+
+void i915_driver_unload(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
 	disable_rpm_wakeref_asserts(dev_priv);
 
 	i915_driver_unregister(dev_priv);
@@ -1773,14 +1790,18 @@ void i915_driver_unload(struct drm_device *dev)
 
 	intel_gvt_cleanup(dev_priv);
 
-	intel_modeset_cleanup(dev);
+	intel_modeset_cleanup_prepare(dev);
 
-	intel_bios_cleanup(dev_priv);
+	/*
+	 * Interrupts and polling as the first thing to avoid creating havoc.
+	 * Too much stuff here (turning of connectors, ...) would
+	 * experience fancy races otherwise.
+	 */
+	intel_irq_uninstall(dev_priv);
 
-	vga_switcheroo_unregister_client(pdev);
-	vga_client_register(pdev, NULL, NULL, NULL);
+	intel_modeset_cleanup(dev);
 
-	intel_csr_ucode_fini(dev_priv);
+	i915_modeset_unload(dev);
 
 	/* Free error state after interrupts are fully disabled. */
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3017ef037fed..a3d33245767c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3491,6 +3491,7 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
 /* modesetting */
 extern void intel_modeset_init_hw(struct drm_device *dev);
 extern int intel_modeset_init(struct drm_device *dev);
+void intel_modeset_cleanup_prepare(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv,
 				       bool state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 980f4ea68e48..36f887a2bdf2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -170,17 +170,6 @@ int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
 				 dev_priv->hpll_freq);
 }
 
-static void intel_update_czclk(struct drm_i915_private *dev_priv)
-{
-	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
-		return;
-
-	dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
-						      CCK_CZ_CLOCK_CONTROL);
-
-	DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
-}
-
 static inline u32 /* units of 100MHz */
 intel_fdi_link_freq(struct drm_i915_private *dev_priv,
 		    const struct intel_crtc_state *pipe_config)
@@ -15099,7 +15088,6 @@ int intel_modeset_init(struct drm_device *dev)
 	intel_shared_dpll_init(dev);
 	intel_update_fdi_pll_freq(dev_priv);
 
-	intel_update_czclk(dev_priv);
 	intel_modeset_init_hw(dev);
 
 	if (dev_priv->max_cdclk_freq == 0)
@@ -15804,7 +15792,7 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
 	drm_connector_list_iter_end(&conn_iter);
 }
 
-void intel_modeset_cleanup(struct drm_device *dev)
+void intel_modeset_cleanup_prepare(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
@@ -15812,13 +15800,11 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	flush_work(&dev_priv->atomic_helper.free_work);
 	WARN_ON(!llist_empty(&dev_priv->atomic_helper.free_list));
+}
 
-	/*
-	 * Interrupts and polling as the first thing to avoid creating havoc.
-	 * Too much stuff here (turning of connectors, ...) would
-	 * experience fancy races otherwise.
-	 */
-	intel_irq_uninstall(dev_priv);
+void intel_modeset_cleanup(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	/*
 	 * Due to the hpd irq storm handling the hotplug work can re-arm the
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fa5c48778a80..f83372c6cafe 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7450,6 +7450,14 @@ static void vlv_init_gpll_ref_freq(struct drm_i915_private *dev_priv)
 			 dev_priv->gt_pm.rps.gpll_ref_freq);
 }
 
+static void valleyview_update_czclk(struct drm_i915_private *dev_priv)
+{
+	dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
+						      CCK_CZ_CLOCK_CONTROL);
+
+	DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
+}
+
 static void valleyview_init_gt_powersave(struct drm_i915_private *dev_priv)
 {
 	struct intel_rps *rps = &dev_priv->gt_pm.rps;
@@ -7457,6 +7465,7 @@ static void valleyview_init_gt_powersave(struct drm_i915_private *dev_priv)
 
 	valleyview_setup_pctx(dev_priv);
 
+	valleyview_update_czclk(dev_priv);
 	vlv_init_gpll_ref_freq(dev_priv);
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
@@ -7503,6 +7512,7 @@ static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
 
 	cherryview_setup_pctx(dev_priv);
 
+	valleyview_update_czclk(dev_priv);
 	vlv_init_gpll_ref_freq(dev_priv);
 
 	mutex_lock(&dev_priv->sb_lock);
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: Move out non-display related calls from display/modeset init/cleanup

[Chris Wilson]

Quoting José Roberto de Souza (2018-10-12 22:52:04)
> i915_load_modeset_init() and intel_modeset_cleanup() was initializing
> and cleaning up things that is not related to display or modeset.
> This changes will make easy initialize driver without display block.

Still broken, as the display must reserve it's portion of the GTT before
i915_gem_init.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: Move out non-display related calls from display/modeset init/cleanup

[Souza, Jose]

On Mon, 2018-10-15 at 12:14 +0100, Chris Wilson wrote:
> Quoting José Roberto de Souza (2018-10-12 22:52:04)
> > i915_load_modeset_init() and intel_modeset_cleanup() was
> > initializing
> > and cleaning up things that is not related to display or modeset.
> > This changes will make easy initialize driver without display
> > block.
> 
> Still broken, as the display must reserve it's portion of the GTT
> before
> i915_gem_init.


Hi Chris

I did not found any calls to this functions:

i915_gem_info_add_obj()
i915_gem_object_create()
i915_gem_gtt_reserve()
i915_gem_gtt_insert()
i915_vma_insert()

before the i915_gem_init() call, I even added dump_stack() in those
functions but still did got any call before i915_gem_init(), could
explain more?
All of this running with drm-tip with display enabled.

Thanks

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/16] drm/i915: Move drm_vblank_init() to i915_load_modeset_init()

[José Roberto de Souza]

i915_load_modeset_init() is a more suitable place than
i915_driver_load() as vblank is part of modeset.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 55212d059cca..e93b214a3dfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -645,6 +645,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		ret = drm_vblank_init(&dev_priv->drm,
+				      INTEL_INFO(dev_priv)->num_pipes);
+		if (ret)
+			goto out;
+	}
+
 	intel_bios_init(dev_priv);
 
 	/* If we have > 1 VGA cards, then we need to arbitrate access
@@ -1696,18 +1703,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret < 0)
 		goto out_cleanup_mmio;
 
-	/*
-	 * TODO: move the vblank init and parts of modeset init steps into one
-	 * of the i915_driver_init_/i915_driver_register functions according
-	 * to the role/effect of the given init step.
-	 */
-	if (INTEL_INFO(dev_priv)->num_pipes) {
-		ret = drm_vblank_init(&dev_priv->drm,
-				      INTEL_INFO(dev_priv)->num_pipes);
-		if (ret)
-			goto out_cleanup_hw;
-	}
-
 	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
 	intel_update_rawclk(dev_priv);
 
@@ -1746,7 +1741,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	drm_irq_uninstall(&dev_priv->drm);
 out_cleanup_power:
 	intel_power_domains_fini_hw(dev_priv);
-out_cleanup_hw:
 	i915_driver_cleanup_hw(dev_priv);
 out_cleanup_mmio:
 	i915_driver_cleanup_mmio(dev_priv);
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/16] drm/i915: Move FBC init and cleanup calls to modeset functions

[José Roberto de Souza]

Although FBC helps save power it do not belongs to power management
also the cleanup was placed in i915_driver_unload() also not a good
place. intel_modeset_init()/intel_modeset_cleanup() are better places
also this will help make easy disable features that depends in
display being enabled in driver.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 1 -
 drivers/gpu/drm/i915/intel_display.c | 4 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 2 --
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e93b214a3dfa..8e9ecabde710 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1802,7 +1802,6 @@ void i915_driver_unload(struct drm_device *dev)
 	i915_reset_error_state(dev_priv);
 
 	i915_gem_fini(dev_priv);
-	intel_fbc_cleanup_cfb(dev_priv);
 
 	intel_power_domains_fini_hw(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 36f887a2bdf2..1d07fc41d492 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15028,6 +15028,8 @@ int intel_modeset_init(struct drm_device *dev)
 
 	intel_init_quirks(dev);
 
+	intel_fbc_init(dev_priv);
+
 	intel_init_pm(dev_priv);
 
 	/*
@@ -15829,6 +15831,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	intel_teardown_gmbus(dev_priv);
 
 	destroy_workqueue(dev_priv->modeset_wq);
+
+	intel_fbc_cleanup_cfb(dev_priv);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f83372c6cafe..838aaf2c719c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9340,8 +9340,6 @@ void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_i915_private *dev_priv)
 {
-	intel_fbc_init(dev_priv);
-
 	/* For cxsr */
 	if (IS_PINEVIEW(dev_priv))
 		i915_pineview_get_mem_freq(dev_priv);
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/16] drm/i915: Move intel_init_ipc() call to i915_load_modeset_init()

[José Roberto de Souza]

IPC is a display feature, so i915_load_modeset_init() is the right
place to initialize it.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8e9ecabde710..3f7514e981d5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -693,6 +693,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	intel_hpd_init(dev_priv);
 
+	intel_init_ipc(dev_priv);
+
 	return 0;
 
 cleanup_modeset:
@@ -1725,8 +1727,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	i915_driver_register(dev_priv);
 
-	intel_init_ipc(dev_priv);
-
 	enable_rpm_wakeref_asserts(dev_priv);
 
 	i915_welcome_messages(dev_priv);
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[José Roberto de Souza]

Display features should not be initialized or deinitialized when
display is disabled.

With this changes no plane, CRTC, encoder or connector
is being registered in drm when display is disabled so it was also
necessary unset DRIVER_MODESET and DRIVER_ATOMIC features from driver
otherwise it will crash when registering driver in drm.

There is still more modeset/display calls that will be removed in
futher patches.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

squash do not call modeset
---
 drivers/gpu/drm/i915/i915_drv.c         | 155 +++++++++++++++---------
 drivers/gpu/drm/i915/i915_suspend.c     |  24 ++--
 drivers/gpu/drm/i915/intel_runtime_pm.c |  21 ++--
 3 files changed, 124 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3f7514e981d5..8334d1797df7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -899,7 +899,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 	intel_wopcm_init_early(&dev_priv->wopcm);
 	intel_uc_init_early(dev_priv);
 	intel_pm_setup(dev_priv);
-	intel_init_dpio(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_init_dpio(dev_priv);
 	ret = intel_power_domains_init(dev_priv);
 	if (ret < 0)
 		goto err_uc;
@@ -907,8 +908,10 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 	intel_hangcheck_init(dev_priv);
 	intel_init_display_hooks(dev_priv);
 	intel_init_clock_gating_hooks(dev_priv);
-	intel_init_audio_hooks(dev_priv);
-	intel_display_crc_init(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_init_audio_hooks(dev_priv);
+		intel_display_crc_init(dev_priv);
+	}
 
 	intel_detect_preproduction_hw(dev_priv);
 
@@ -1539,23 +1542,26 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	if (IS_GEN5(dev_priv))
 		intel_gpu_ips_init(dev_priv);
 
-	intel_audio_init(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_audio_init(dev_priv);
 
-	/*
-	 * Some ports require correctly set-up hpd registers for detection to
-	 * work properly (leading to ghost connected connector status), e.g. VGA
-	 * on gm45.  Hence we can only set up the initial fbdev config after hpd
-	 * irqs are fully enabled. We do it last so that the async config
-	 * cannot run before the connectors are registered.
-	 */
-	intel_fbdev_initial_config_async(dev);
+		/*
+		 * Some ports require correctly set-up hpd registers for
+		 * detection to work properly (leading to ghost connected
+		 * connector status), e.g. VGA on gm45.  Hence we can only set
+		 * up the initial fbdev config after hpd irqs are fully enabled.
+		 * We do it last so that the async config cannot run before the
+		 * connectors are registered.
+		 */
+		intel_fbdev_initial_config_async(dev);
 
-	/*
-	 * We need to coordinate the hotplugs with the asynchronous fbdev
-	 * configuration, for which we use the fbdev->async_cookie.
-	 */
-	if (INTEL_INFO(dev_priv)->num_pipes)
+		/*
+		 * We need to coordinate the hotplugs with the asynchronous
+		 * fbdev configuration, for which we use the
+		 * fbdev->async_cookie.
+		 */
 		drm_kms_helper_poll_init(dev);
+	}
 
 	intel_power_domains_enable(dev_priv);
 	intel_runtime_pm_enable(dev_priv);
@@ -1570,15 +1576,17 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	intel_runtime_pm_disable(dev_priv);
 	intel_power_domains_disable(dev_priv);
 
-	intel_fbdev_unregister(dev_priv);
-	intel_audio_deinit(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_fbdev_unregister(dev_priv);
+		intel_audio_deinit(dev_priv);
 
-	/*
-	 * After flushing the fbdev (incl. a late async config which will
-	 * have delayed queuing of a hotplug event), then flush the hotplug
-	 * events.
-	 */
-	drm_kms_helper_poll_fini(&dev_priv->drm);
+		/*
+		 * After flushing the fbdev (incl. a late async config which
+		 * will have delayed queuing of a hotplug event), then flush the
+		 * hotplug events.
+		 */
+		drm_kms_helper_poll_fini(&dev_priv->drm);
+	}
 
 	intel_gpu_ips_teardown();
 	acpi_video_unregister();
@@ -1641,6 +1649,7 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (i915_modparams.disable_display) {
 		DRM_INFO("Display disabled (module parameter)\n");
 		device_info->num_pipes = 0;
+		i915->drm.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
 	}
 
 	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
@@ -1721,9 +1730,11 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto cleanup_irq;
 
-	ret = i915_load_modeset_init(&dev_priv->drm);
-	if (ret < 0)
-		goto cleanup_gem;
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		ret = i915_load_modeset_init(&dev_priv->drm);
+		if (ret < 0)
+			goto cleanup_gem;
+	}
 
 	i915_driver_register(dev_priv);
 
@@ -1780,11 +1791,13 @@ void i915_driver_unload(struct drm_device *dev)
 	if (i915_gem_suspend(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 
-	drm_atomic_helper_shutdown(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		drm_atomic_helper_shutdown(dev);
 
 	intel_gvt_cleanup(dev_priv);
 
-	intel_modeset_cleanup_prepare(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_modeset_cleanup_prepare(dev);
 
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
@@ -1793,9 +1806,11 @@ void i915_driver_unload(struct drm_device *dev)
 	 */
 	intel_irq_uninstall(dev_priv);
 
-	intel_modeset_cleanup(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_modeset_cleanup(dev);
 
-	i915_modeset_unload(dev);
+		i915_modeset_unload(dev);
+	}
 
 	/* Free error state after interrupts are fully disabled. */
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
@@ -1847,8 +1862,12 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file)
  */
 static void i915_driver_lastclose(struct drm_device *dev)
 {
-	intel_fbdev_restore_mode(dev);
-	vga_switcheroo_process_delayed_switch();
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_fbdev_restore_mode(dev);
+		vga_switcheroo_process_delayed_switch();
+	}
 }
 
 static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
@@ -1918,19 +1937,24 @@ static int i915_drm_suspend(struct drm_device *dev)
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	intel_power_domains_disable(dev_priv);
-
-	drm_kms_helper_poll_disable(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(pdev);
 
-	intel_display_suspend(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_display_suspend(dev);
 
-	intel_dp_mst_suspend(dev_priv);
+		intel_dp_mst_suspend(dev_priv);
+	}
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
-	intel_hpd_cancel_work(dev_priv);
 
-	intel_suspend_encoders(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_hpd_cancel_work(dev_priv);
+
+		intel_suspend_encoders(dev_priv);
+	}
 
 	intel_suspend_hw(dev_priv);
 
@@ -1943,11 +1967,13 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_opregion_unregister(dev_priv);
 
-	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
 
 	dev_priv->suspend_count++;
 
-	intel_csr_ucode_suspend(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_csr_ucode_suspend(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 
@@ -2056,10 +2082,12 @@ static int i915_drm_resume(struct drm_device *dev)
 	if (ret)
 		DRM_ERROR("failed to re-enable GGTT\n");
 
-	intel_csr_ucode_resume(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_csr_ucode_resume(dev_priv);
 
 	i915_restore_state(dev_priv);
-	intel_pps_unlock_regs_wa(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_pps_unlock_regs_wa(dev_priv);
 	intel_opregion_setup(dev_priv);
 
 	intel_init_pch_refclk(dev_priv);
@@ -2076,7 +2104,8 @@ static int i915_drm_resume(struct drm_device *dev)
 	 */
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
-	drm_mode_config_reset(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		drm_mode_config_reset(dev);
 
 	i915_gem_resume(dev_priv);
 
@@ -2084,27 +2113,30 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	if (dev_priv->display.hpd_irq_setup)
+	if (dev_priv->display.hpd_irq_setup && INTEL_INFO(dev_priv)->num_pipes)
 		dev_priv->display.hpd_irq_setup(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	intel_dp_mst_resume(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		intel_dp_mst_resume(dev_priv);
 
-	intel_display_resume(dev);
+		intel_display_resume(dev);
 
-	drm_kms_helper_poll_enable(dev);
+		drm_kms_helper_poll_enable(dev);
 
-	/*
-	 * ... but also need to make sure that hotplug processing
-	 * doesn't cause havoc. Like in the driver load code we don't
-	 * bother with the tiny race here where we might lose hotplug
-	 * notifications.
-	 * */
-	intel_hpd_init(dev_priv);
+		/*
+		 * ... but also need to make sure that hotplug processing
+		 * doesn't cause havoc. Like in the driver load code we don't
+		 * bother with the tiny race here where we might lose hotplug
+		 * notifications.
+		 */
+		intel_hpd_init(dev_priv);
+	}
 
 	intel_opregion_register(dev_priv);
 
-	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
 
 	intel_opregion_notify_adapter(dev_priv, PCI_D0);
 
@@ -3000,7 +3032,8 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	assert_forcewakes_inactive(dev_priv);
 
-	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
+	if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) &&
+						!IS_CHERRYVIEW(dev_priv)))
 		intel_hpd_poll_init(dev_priv);
 
 	DRM_DEBUG_KMS("Device suspended\n");
@@ -3057,10 +3090,12 @@ static int intel_runtime_resume(struct device *kdev)
 	 * power well, so hpd is reinitialized from there. For
 	 * everyone else do it here.
 	 */
-	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
+	if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) &&
+						!IS_CHERRYVIEW(dev_priv)))
 		intel_hpd_init(dev_priv);
 
-	intel_enable_ipc(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_enable_ipc(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 8f3aa4dc0c98..f697865236a6 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -63,11 +63,13 @@ int i915_save_state(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 
-	i915_save_display(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		i915_save_display(dev_priv);
 
-	if (IS_GEN4(dev_priv))
-		pci_read_config_word(pdev, GCDGMBUS,
-				     &dev_priv->regfile.saveGCDGMBUS);
+		if (IS_GEN4(dev_priv))
+			pci_read_config_word(pdev, GCDGMBUS,
+					     &dev_priv->regfile.saveGCDGMBUS);
+	}
 
 	/* Cache mode state */
 	if (INTEL_GEN(dev_priv) < 7)
@@ -108,10 +110,13 @@ int i915_restore_state(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 
-	if (IS_GEN4(dev_priv))
-		pci_write_config_word(pdev, GCDGMBUS,
-				      dev_priv->regfile.saveGCDGMBUS);
-	i915_restore_display(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes) {
+		if (IS_GEN4(dev_priv))
+			pci_write_config_word(pdev, GCDGMBUS,
+					      dev_priv->regfile.saveGCDGMBUS);
+
+		i915_restore_display(dev_priv);
+	}
 
 	/* Cache mode state */
 	if (INTEL_GEN(dev_priv) < 7)
@@ -143,7 +148,8 @@ int i915_restore_state(struct drm_i915_private *dev_priv)
 
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	intel_i2c_reset(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_i2c_reset(dev_priv);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3cf8533e0834..15fd88bb35f7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -625,7 +625,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Enabling DC9\n");
 
-	intel_power_sequencer_reset(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_power_sequencer_reset(dev_priv);
 	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9);
 }
 
@@ -1039,10 +1040,12 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
 	/* make sure we're done processing display irqs */
 	synchronize_irq(dev_priv->drm.irq);
 
-	intel_power_sequencer_reset(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_power_sequencer_reset(dev_priv);
 
 	/* Prevent us from re-enabling polling on accident in late suspend */
-	if (!dev_priv->drm.dev->power.is_suspended)
+	if (INTEL_INFO(dev_priv)->num_pipes &&
+	    !dev_priv->drm.dev->power.is_suspended)
 		intel_hpd_poll_init(dev_priv);
 }
 
@@ -1284,11 +1287,14 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 
 	if (power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) {
 		phy = DPIO_PHY0;
-		assert_pll_disabled(dev_priv, PIPE_A);
-		assert_pll_disabled(dev_priv, PIPE_B);
+		if (INTEL_INFO(dev_priv)->num_pipes) {
+			assert_pll_disabled(dev_priv, PIPE_A);
+			assert_pll_disabled(dev_priv, PIPE_B);
+		}
 	} else {
 		phy = DPIO_PHY1;
-		assert_pll_disabled(dev_priv, PIPE_C);
+		if (INTEL_INFO(dev_priv)->num_pipes)
+			assert_pll_disabled(dev_priv, PIPE_C);
 	}
 
 	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
@@ -1302,7 +1308,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 	/* PHY is fully reset now, so we can enable the PHY state asserts */
 	dev_priv->chv_phy_assert[phy] = true;
 
-	assert_chv_phy_status(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		assert_chv_phy_status(dev_priv);
 }
 
 static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> Display features should not be initialized or deinitialized when
> display is disabled.
>
> With this changes no plane, CRTC, encoder or connector
> is being registered in drm when display is disabled so it was also
> necessary unset DRIVER_MODESET and DRIVER_ATOMIC features from driver
> otherwise it will crash when registering driver in drm.
>
> There is still more modeset/display calls that will be removed in
> futher patches.
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>
> squash do not call modeset
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 155 +++++++++++++++---------
>  drivers/gpu/drm/i915/i915_suspend.c     |  24 ++--
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  21 ++--
>  3 files changed, 124 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3f7514e981d5..8334d1797df7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -899,7 +899,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>  	intel_wopcm_init_early(&dev_priv->wopcm);
>  	intel_uc_init_early(dev_priv);
>  	intel_pm_setup(dev_priv);
> -	intel_init_dpio(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_init_dpio(dev_priv);
>  	ret = intel_power_domains_init(dev_priv);
>  	if (ret < 0)
>  		goto err_uc;
> @@ -907,8 +908,10 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>  	intel_hangcheck_init(dev_priv);
>  	intel_init_display_hooks(dev_priv);
>  	intel_init_clock_gating_hooks(dev_priv);
> -	intel_init_audio_hooks(dev_priv);
> -	intel_display_crc_init(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_init_audio_hooks(dev_priv);
> +		intel_display_crc_init(dev_priv);
> +	}
>  
>  	intel_detect_preproduction_hw(dev_priv);
>  
> @@ -1539,23 +1542,26 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  	if (IS_GEN5(dev_priv))
>  		intel_gpu_ips_init(dev_priv);
>  
> -	intel_audio_init(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_audio_init(dev_priv);
>  
> -	/*
> -	 * Some ports require correctly set-up hpd registers for detection to
> -	 * work properly (leading to ghost connected connector status), e.g. VGA
> -	 * on gm45.  Hence we can only set up the initial fbdev config after hpd
> -	 * irqs are fully enabled. We do it last so that the async config
> -	 * cannot run before the connectors are registered.
> -	 */
> -	intel_fbdev_initial_config_async(dev);
> +		/*
> +		 * Some ports require correctly set-up hpd registers for
> +		 * detection to work properly (leading to ghost connected
> +		 * connector status), e.g. VGA on gm45.  Hence we can only set
> +		 * up the initial fbdev config after hpd irqs are fully enabled.
> +		 * We do it last so that the async config cannot run before the
> +		 * connectors are registered.
> +		 */
> +		intel_fbdev_initial_config_async(dev);

So for all of the above that we're in control of, I'd put the display
check and early return at the beginning of each function, and call them
unconditionally from here. This particularly so for all the higher level
functions. They become cluttered and hard to grasp. It's slightly
different at the lower levels.

I think we need a macro

#define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes != 0)

and use that for anything that checks for the binary has display or
not. We may need to redefine that later on, and I don't want to touch
all of these places again.

>  
> -	/*
> -	 * We need to coordinate the hotplugs with the asynchronous fbdev
> -	 * configuration, for which we use the fbdev->async_cookie.
> -	 */
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> +		/*
> +		 * We need to coordinate the hotplugs with the asynchronous
> +		 * fbdev configuration, for which we use the
> +		 * fbdev->async_cookie.
> +		 */
>  		drm_kms_helper_poll_init(dev);
> +	}

Obviously calls to drm helpers need to be wrapped around HAS_DISPLAY.

>  
>  	intel_power_domains_enable(dev_priv);
>  	intel_runtime_pm_enable(dev_priv);
> @@ -1570,15 +1576,17 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  	intel_runtime_pm_disable(dev_priv);
>  	intel_power_domains_disable(dev_priv);
>  
> -	intel_fbdev_unregister(dev_priv);
> -	intel_audio_deinit(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_fbdev_unregister(dev_priv);
> +		intel_audio_deinit(dev_priv);

All of these I want to be able to call unconditionally too, and instead
of checking for HAS_DISPLAY(), they might be better off checking whether
the stuff has been initialized. Indeed, I think the above two already
work like this; they can be safely called even when audio or fbdev
hasn't been initialized.

BR,
Jani.

>  
> -	/*
> -	 * After flushing the fbdev (incl. a late async config which will
> -	 * have delayed queuing of a hotplug event), then flush the hotplug
> -	 * events.
> -	 */
> -	drm_kms_helper_poll_fini(&dev_priv->drm);
> +		/*
> +		 * After flushing the fbdev (incl. a late async config which
> +		 * will have delayed queuing of a hotplug event), then flush the
> +		 * hotplug events.
> +		 */
> +		drm_kms_helper_poll_fini(&dev_priv->drm);
> +	}
>  
>  	intel_gpu_ips_teardown();
>  	acpi_video_unregister();
> @@ -1641,6 +1649,7 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (i915_modparams.disable_display) {
>  		DRM_INFO("Display disabled (module parameter)\n");
>  		device_info->num_pipes = 0;
> +		i915->drm.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
>  	}
>  
>  	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> @@ -1721,9 +1730,11 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ret)
>  		goto cleanup_irq;
>  
> -	ret = i915_load_modeset_init(&dev_priv->drm);
> -	if (ret < 0)
> -		goto cleanup_gem;
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		ret = i915_load_modeset_init(&dev_priv->drm);
> +		if (ret < 0)
> +			goto cleanup_gem;
> +	}
>  
>  	i915_driver_register(dev_priv);
>  
> @@ -1780,11 +1791,13 @@ void i915_driver_unload(struct drm_device *dev)
>  	if (i915_gem_suspend(dev_priv))
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  
> -	drm_atomic_helper_shutdown(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		drm_atomic_helper_shutdown(dev);
>  
>  	intel_gvt_cleanup(dev_priv);
>  
> -	intel_modeset_cleanup_prepare(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_modeset_cleanup_prepare(dev);
>  
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
> @@ -1793,9 +1806,11 @@ void i915_driver_unload(struct drm_device *dev)
>  	 */
>  	intel_irq_uninstall(dev_priv);
>  
> -	intel_modeset_cleanup(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_modeset_cleanup(dev);
>  
> -	i915_modeset_unload(dev);
> +		i915_modeset_unload(dev);
> +	}
>  
>  	/* Free error state after interrupts are fully disabled. */
>  	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> @@ -1847,8 +1862,12 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>   */
>  static void i915_driver_lastclose(struct drm_device *dev)
>  {
> -	intel_fbdev_restore_mode(dev);
> -	vga_switcheroo_process_delayed_switch();
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_fbdev_restore_mode(dev);
> +		vga_switcheroo_process_delayed_switch();
> +	}
>  }
>  
>  static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
> @@ -1918,19 +1937,24 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	/* We do a lot of poking in a lot of registers, make sure they work
>  	 * properly. */
>  	intel_power_domains_disable(dev_priv);
> -
> -	drm_kms_helper_poll_disable(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		drm_kms_helper_poll_disable(dev);
>  
>  	pci_save_state(pdev);
>  
> -	intel_display_suspend(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_display_suspend(dev);
>  
> -	intel_dp_mst_suspend(dev_priv);
> +		intel_dp_mst_suspend(dev_priv);
> +	}
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> -	intel_hpd_cancel_work(dev_priv);
>  
> -	intel_suspend_encoders(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_hpd_cancel_work(dev_priv);
> +
> +		intel_suspend_encoders(dev_priv);
> +	}
>  
>  	intel_suspend_hw(dev_priv);
>  
> @@ -1943,11 +1967,13 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_opregion_unregister(dev_priv);
>  
> -	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
>  
>  	dev_priv->suspend_count++;
>  
> -	intel_csr_ucode_suspend(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_csr_ucode_suspend(dev_priv);
>  
>  	enable_rpm_wakeref_asserts(dev_priv);
>  
> @@ -2056,10 +2082,12 @@ static int i915_drm_resume(struct drm_device *dev)
>  	if (ret)
>  		DRM_ERROR("failed to re-enable GGTT\n");
>  
> -	intel_csr_ucode_resume(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_csr_ucode_resume(dev_priv);
>  
>  	i915_restore_state(dev_priv);
> -	intel_pps_unlock_regs_wa(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_pps_unlock_regs_wa(dev_priv);
>  	intel_opregion_setup(dev_priv);
>  
>  	intel_init_pch_refclk(dev_priv);
> @@ -2076,7 +2104,8 @@ static int i915_drm_resume(struct drm_device *dev)
>  	 */
>  	intel_runtime_pm_enable_interrupts(dev_priv);
>  
> -	drm_mode_config_reset(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		drm_mode_config_reset(dev);
>  
>  	i915_gem_resume(dev_priv);
>  
> @@ -2084,27 +2113,30 @@ static int i915_drm_resume(struct drm_device *dev)
>  	intel_init_clock_gating(dev_priv);
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> -	if (dev_priv->display.hpd_irq_setup)
> +	if (dev_priv->display.hpd_irq_setup && INTEL_INFO(dev_priv)->num_pipes)
>  		dev_priv->display.hpd_irq_setup(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
> -	intel_dp_mst_resume(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		intel_dp_mst_resume(dev_priv);
>  
> -	intel_display_resume(dev);
> +		intel_display_resume(dev);
>  
> -	drm_kms_helper_poll_enable(dev);
> +		drm_kms_helper_poll_enable(dev);
>  
> -	/*
> -	 * ... but also need to make sure that hotplug processing
> -	 * doesn't cause havoc. Like in the driver load code we don't
> -	 * bother with the tiny race here where we might lose hotplug
> -	 * notifications.
> -	 * */
> -	intel_hpd_init(dev_priv);
> +		/*
> +		 * ... but also need to make sure that hotplug processing
> +		 * doesn't cause havoc. Like in the driver load code we don't
> +		 * bother with the tiny race here where we might lose hotplug
> +		 * notifications.
> +		 */
> +		intel_hpd_init(dev_priv);
> +	}
>  
>  	intel_opregion_register(dev_priv);
>  
> -	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
>  
>  	intel_opregion_notify_adapter(dev_priv, PCI_D0);
>  
> @@ -3000,7 +3032,8 @@ static int intel_runtime_suspend(struct device *kdev)
>  
>  	assert_forcewakes_inactive(dev_priv);
>  
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> +	if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) &&
> +						!IS_CHERRYVIEW(dev_priv)))
>  		intel_hpd_poll_init(dev_priv);
>  
>  	DRM_DEBUG_KMS("Device suspended\n");
> @@ -3057,10 +3090,12 @@ static int intel_runtime_resume(struct device *kdev)
>  	 * power well, so hpd is reinitialized from there. For
>  	 * everyone else do it here.
>  	 */
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> +	if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) &&
> +						!IS_CHERRYVIEW(dev_priv)))
>  		intel_hpd_init(dev_priv);
>  
> -	intel_enable_ipc(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_enable_ipc(dev_priv);
>  
>  	enable_rpm_wakeref_asserts(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 8f3aa4dc0c98..f697865236a6 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -63,11 +63,13 @@ int i915_save_state(struct drm_i915_private *dev_priv)
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  
> -	i915_save_display(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		i915_save_display(dev_priv);
>  
> -	if (IS_GEN4(dev_priv))
> -		pci_read_config_word(pdev, GCDGMBUS,
> -				     &dev_priv->regfile.saveGCDGMBUS);
> +		if (IS_GEN4(dev_priv))
> +			pci_read_config_word(pdev, GCDGMBUS,
> +					     &dev_priv->regfile.saveGCDGMBUS);
> +	}
>  
>  	/* Cache mode state */
>  	if (INTEL_GEN(dev_priv) < 7)
> @@ -108,10 +110,13 @@ int i915_restore_state(struct drm_i915_private *dev_priv)
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  
> -	if (IS_GEN4(dev_priv))
> -		pci_write_config_word(pdev, GCDGMBUS,
> -				      dev_priv->regfile.saveGCDGMBUS);
> -	i915_restore_display(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes) {
> +		if (IS_GEN4(dev_priv))
> +			pci_write_config_word(pdev, GCDGMBUS,
> +					      dev_priv->regfile.saveGCDGMBUS);
> +
> +		i915_restore_display(dev_priv);
> +	}
>  
>  	/* Cache mode state */
>  	if (INTEL_GEN(dev_priv) < 7)
> @@ -143,7 +148,8 @@ int i915_restore_state(struct drm_i915_private *dev_priv)
>  
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> -	intel_i2c_reset(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_i2c_reset(dev_priv);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3cf8533e0834..15fd88bb35f7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -625,7 +625,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>  
>  	DRM_DEBUG_KMS("Enabling DC9\n");
>  
> -	intel_power_sequencer_reset(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_power_sequencer_reset(dev_priv);
>  	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9);
>  }
>  
> @@ -1039,10 +1040,12 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
>  	/* make sure we're done processing display irqs */
>  	synchronize_irq(dev_priv->drm.irq);
>  
> -	intel_power_sequencer_reset(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_power_sequencer_reset(dev_priv);
>  
>  	/* Prevent us from re-enabling polling on accident in late suspend */
> -	if (!dev_priv->drm.dev->power.is_suspended)
> +	if (INTEL_INFO(dev_priv)->num_pipes &&
> +	    !dev_priv->drm.dev->power.is_suspended)
>  		intel_hpd_poll_init(dev_priv);
>  }
>  
> @@ -1284,11 +1287,14 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>  
>  	if (power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) {
>  		phy = DPIO_PHY0;
> -		assert_pll_disabled(dev_priv, PIPE_A);
> -		assert_pll_disabled(dev_priv, PIPE_B);
> +		if (INTEL_INFO(dev_priv)->num_pipes) {
> +			assert_pll_disabled(dev_priv, PIPE_A);
> +			assert_pll_disabled(dev_priv, PIPE_B);
> +		}
>  	} else {
>  		phy = DPIO_PHY1;
> -		assert_pll_disabled(dev_priv, PIPE_C);
> +		if (INTEL_INFO(dev_priv)->num_pipes)
> +			assert_pll_disabled(dev_priv, PIPE_C);
>  	}
>  
>  	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
> @@ -1302,7 +1308,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>  	/* PHY is fully reset now, so we can enable the PHY state asserts */
>  	dev_priv->chv_phy_assert[phy] = true;
>  
> -	assert_chv_phy_status(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		assert_chv_phy_status(dev_priv);
>  }
>  
>  static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Chris Wilson]

Quoting Jani Nikula (2018-10-22 09:25:45)
> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> > Display features should not be initialized or deinitialized when
> > display is disabled.

I completely disagree with this assertion. If the display is disabled,
so must all the associated hw so that we can power down the entire
chipset when idle. That means we have to complete the probe (so we
continue to rely on fuses and in place of accurate fuses pci-id quirks
for the infamous chipsets) and switch it off.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Jani Nikula]

On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-10-22 09:25:45)
>> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
>> > Display features should not be initialized or deinitialized when
>> > display is disabled.
>
> I completely disagree with this assertion. If the display is disabled,
> so must all the associated hw so that we can power down the entire
> chipset when idle. That means we have to complete the probe (so we
> continue to rely on fuses and in place of accurate fuses pci-id quirks
> for the infamous chipsets) and switch it off.

That actually doesn't contradict with what I said about
HAS_DISPLAY(). In many cases I think the early return on no display is
the right thing to do. However, no display isn't the same as display
disabled by module parameter (or whatnot)... which does require probe
before disable to achieve the power down.

But is the power down on display disable by module parameter a
requirement for us?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Souza, Jose]

On Mon, 2018-10-22 at 12:00 +0300, Jani Nikula wrote:
> On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Jani Nikula (2018-10-22 09:25:45)
> > > On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com>
> > > wrote:
> > > > Display features should not be initialized or deinitialized
> > > > when
> > > > display is disabled.
> > 
> > I completely disagree with this assertion. If the display is
> > disabled,
> > so must all the associated hw so that we can power down the entire
> > chipset when idle. That means we have to complete the probe (so we
> > continue to rely on fuses and in place of accurate fuses pci-id
> > quirks
> > for the infamous chipsets) and switch it off.
> 
> That actually doesn't contradict with what I said about
> HAS_DISPLAY(). In many cases I think the early return on no display
> is
> the right thing to do. However, no display isn't the same as display
> disabled by module parameter (or whatnot)... which does require probe
> before disable to achieve the power down.
> 
> But is the power down on display disable by module parameter a
> requirement for us?

Okay so I will continue with the current approach to not initialize
display stuff when HAS_DISPLAY() == false.

Also Jani could you take a look into the first 5 patches? Those are
moving some display initialization/uninitialization to proper
functions.


> 
> BR,
> Jani.
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Chris Wilson]

Quoting Jani Nikula (2018-10-22 10:00:39)
> On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Jani Nikula (2018-10-22 09:25:45)
> >> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> >> > Display features should not be initialized or deinitialized when
> >> > display is disabled.
> >
> > I completely disagree with this assertion. If the display is disabled,
> > so must all the associated hw so that we can power down the entire
> > chipset when idle. That means we have to complete the probe (so we
> > continue to rely on fuses and in place of accurate fuses pci-id quirks
> > for the infamous chipsets) and switch it off.
> 
> That actually doesn't contradict with what I said about
> HAS_DISPLAY(). In many cases I think the early return on no display is
> the right thing to do. However, no display isn't the same as display
> disabled by module parameter (or whatnot)... which does require probe
> before disable to achieve the power down.
> 
> But is the power down on display disable by module parameter a
> requirement for us?

We still see this error in BAT roughly every day:

<7> [557.273023] [drm:intel_power_well_enable [i915]] enabling display
<7> [557.273553] [drm:i915_redisable_vga_power_on [i915]] Something enabled VGA plane, disabling it
<4> [557.274207] ------------[ cut here ]------------
<4> [557.274420] Unclaimed write to register 0x44200
<4> [557.274637] WARNING: CPU: 2 PID: 370 at drivers/gpu/drm/i915/intel_uncore.c:1034 __unclaimed_reg_debug+0x40/0x50 [i915]
<4> [557.274643] Modules linked in: i915(+) amdgpu gpu_sched ttm vgem snd_hda_codec_hdmi coretemp btusb crct10dif_pclmul btrtl crc32_pclmul btbcm btintel ghash_clmulni_intel bluetooth cdc_ether usbnet r8152 mii ecdh_generic snd_hda_codec snd_hwdep snd_hda_core snd_pcm prime_numbers pinctrl_cherryview lpc_ich [last unloaded: i915]
<4> [557.274686] CPU: 2 PID: 370 Comm: rngd Tainted: G     U            5.1.0-rc4-CI-CI_DRM_5891+ #1
<4> [557.274690] Hardware name: GOOGLE Cyan/Cyan, BIOS MrChromebox 02/15/2018
<4> [557.274829] RIP: 0010:__unclaimed_reg_debug+0x40/0x50 [i915]
<4> [557.274836] Code: 74 05 5b 5d 41 5c c3 45 84 e4 48 c7 c0 2b 6a 7b a0 48 c7 c6 21 6a 7b a0 48 0f 44 f0 89 ea 48 c7 c7 34 6a 7b a0 e8 b0 a3 a1 e0 <0f> 0b 83 2d 97 46 1b 00 01 5b 5d 41 5c c3 66 90 41 56 41 55 41 89
<4> [557.274841] RSP: 0018:ffff888079903e30 EFLAGS: 00010082
<4> [557.274847] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
<4> [557.274851] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000ffffffff
<4> [557.274856] RBP: 0000000000044200 R08: 0000000000000000 R09: 0000000000000001
<4> [557.274860] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
<4> [557.274865] R13: ffff88806d820eb0 R14: 0000000000000006 R15: 0000000000000000
<4> [557.274870] FS:  00007f7c805d2740(0000) GS:ffff888079900000(0000) knlGS:0000000000000000
<4> [557.274875] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [557.274879] CR2: 00005647db5bc7d8 CR3: 000000006476c000 CR4: 00000000001006e0
<4> [557.274884] Call Trace:
<4> [557.274891]  <IRQ>
<4> [557.275026]  fwtable_write32+0x25f/0x2c0 [i915]
<4> [557.275149]  cherryview_irq_handler+0x180/0x210 [i915]
<4> [557.275170]  __handle_irq_event_percpu+0x41/0x2d0
<4> [557.275177]  ? handle_irq_event+0x27/0x50
<4> [557.275188]  handle_irq_event_percpu+0x2b/0x70
<4> [557.275197]  handle_irq_event+0x2f/0x50
<4> [557.275207]  handle_edge_irq+0xee/0x1a0
<4> [557.275216]  handle_irq+0x67/0x160
<4> [557.275229]  do_IRQ+0x5e/0x130
<4> [557.275240]  common_interrupt+0xf/0xf

Precisely because the display is not powered down on request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: Don't call modeset related functions when display is disabled

[Chris Wilson]

Quoting Chris Wilson (2019-04-08 21:50:28)
> Quoting Jani Nikula (2018-10-22 10:00:39)
> > On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Quoting Jani Nikula (2018-10-22 09:25:45)
> > >> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> > >> > Display features should not be initialized or deinitialized when
> > >> > display is disabled.
> > >
> > > I completely disagree with this assertion. If the display is disabled,
> > > so must all the associated hw so that we can power down the entire
> > > chipset when idle. That means we have to complete the probe (so we
> > > continue to rely on fuses and in place of accurate fuses pci-id quirks
> > > for the infamous chipsets) and switch it off.
> > 
> > That actually doesn't contradict with what I said about
> > HAS_DISPLAY(). In many cases I think the early return on no display is
> > the right thing to do. However, no display isn't the same as display
> > disabled by module parameter (or whatnot)... which does require probe
> > before disable to achieve the power down.
> > 
> > But is the power down on display disable by module parameter a
> > requirement for us?
> 
> We still see this error in BAT roughly every day:
> 
> <7> [557.273023] [drm:intel_power_well_enable [i915]] enabling display
> <7> [557.273553] [drm:i915_redisable_vga_power_on [i915]] Something enabled VGA plane, disabling it
> <4> [557.274207] ------------[ cut here ]------------
> <4> [557.274420] Unclaimed write to register 0x44200
> <4> [557.274637] WARNING: CPU: 2 PID: 370 at drivers/gpu/drm/i915/intel_uncore.c:1034 __unclaimed_reg_debug+0x40/0x50 [i915]
> <4> [557.274643] Modules linked in: i915(+) amdgpu gpu_sched ttm vgem snd_hda_codec_hdmi coretemp btusb crct10dif_pclmul btrtl crc32_pclmul btbcm btintel ghash_clmulni_intel bluetooth cdc_ether usbnet r8152 mii ecdh_generic snd_hda_codec snd_hwdep snd_hda_core snd_pcm prime_numbers pinctrl_cherryview lpc_ich [last unloaded: i915]
> <4> [557.274686] CPU: 2 PID: 370 Comm: rngd Tainted: G     U            5.1.0-rc4-CI-CI_DRM_5891+ #1
> <4> [557.274690] Hardware name: GOOGLE Cyan/Cyan, BIOS MrChromebox 02/15/2018
> <4> [557.274829] RIP: 0010:__unclaimed_reg_debug+0x40/0x50 [i915]
> <4> [557.274836] Code: 74 05 5b 5d 41 5c c3 45 84 e4 48 c7 c0 2b 6a 7b a0 48 c7 c6 21 6a 7b a0 48 0f 44 f0 89 ea 48 c7 c7 34 6a 7b a0 e8 b0 a3 a1 e0 <0f> 0b 83 2d 97 46 1b 00 01 5b 5d 41 5c c3 66 90 41 56 41 55 41 89
> <4> [557.274841] RSP: 0018:ffff888079903e30 EFLAGS: 00010082
> <4> [557.274847] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> <4> [557.274851] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000ffffffff
> <4> [557.274856] RBP: 0000000000044200 R08: 0000000000000000 R09: 0000000000000001
> <4> [557.274860] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> <4> [557.274865] R13: ffff88806d820eb0 R14: 0000000000000006 R15: 0000000000000000
> <4> [557.274870] FS:  00007f7c805d2740(0000) GS:ffff888079900000(0000) knlGS:0000000000000000
> <4> [557.274875] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [557.274879] CR2: 00005647db5bc7d8 CR3: 000000006476c000 CR4: 00000000001006e0
> <4> [557.274884] Call Trace:
> <4> [557.274891]  <IRQ>
> <4> [557.275026]  fwtable_write32+0x25f/0x2c0 [i915]
> <4> [557.275149]  cherryview_irq_handler+0x180/0x210 [i915]
> <4> [557.275170]  __handle_irq_event_percpu+0x41/0x2d0
> <4> [557.275177]  ? handle_irq_event+0x27/0x50
> <4> [557.275188]  handle_irq_event_percpu+0x2b/0x70
> <4> [557.275197]  handle_irq_event+0x2f/0x50
> <4> [557.275207]  handle_edge_irq+0xee/0x1a0
> <4> [557.275216]  handle_irq+0x67/0x160
> <4> [557.275229]  do_IRQ+0x5e/0x130
> <4> [557.275240]  common_interrupt+0xf/0xf
> 
> Precisely because the display is not powered down on request.

We still see this. Every day.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/16] drm/i915: Remove redundant checks for num_pipes == 0

[José Roberto de Souza]

This 'if's will always be false because of previous changes so let's
drop then.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 12 +++---------
 drivers/gpu/drm/i915/intel_bios.c    |  5 -----
 drivers/gpu/drm/i915/intel_display.c |  3 ---
 drivers/gpu/drm/i915/intel_fbdev.c   |  3 ---
 drivers/gpu/drm/i915/intel_i2c.c     |  3 ---
 5 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8334d1797df7..d69faf70dcfd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -645,12 +645,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
-	if (INTEL_INFO(dev_priv)->num_pipes) {
-		ret = drm_vblank_init(&dev_priv->drm,
-				      INTEL_INFO(dev_priv)->num_pipes);
-		if (ret)
-			goto out;
-	}
+	ret = drm_vblank_init(&dev_priv->drm, INTEL_INFO(dev_priv)->num_pipes);
+	if (ret)
+		goto out;
 
 	intel_bios_init(dev_priv);
 
@@ -683,9 +680,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_setup_overlay(dev_priv);
 
-	if (INTEL_INFO(dev_priv)->num_pipes == 0)
-		return 0;
-
 	ret = intel_fbdev_init(dev);
 	if (ret)
 		goto cleanup_modeset;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 1faa494e2bc9..1e0205572ff9 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1721,11 +1721,6 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 
-	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
-		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
-		return;
-	}
-
 	init_vbt_defaults(dev_priv);
 
 	/* If the OpRegion does not have VBT, look in PCI ROM. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1d07fc41d492..d369c7336286 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13957,9 +13957,6 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 
 	intel_pps_init(dev_priv);
 
-	if (INTEL_INFO(dev_priv)->num_pipes == 0)
-		return;
-
 	/*
 	 * intel_edp_init_connector() depends on this completing first, to
 	 * prevent the registeration of both eDP and LVDS and the incorrect
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 2480c7d6edee..5eaf1c35332b 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -672,9 +672,6 @@ int intel_fbdev_init(struct drm_device *dev)
 	struct intel_fbdev *ifbdev;
 	int ret;
 
-	if (WARN_ON(INTEL_INFO(dev_priv)->num_pipes == 0))
-		return -ENODEV;
-
 	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
 	if (ifbdev == NULL)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 33d87ab93fdd..9c05f7af68a6 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -817,9 +817,6 @@ int intel_setup_gmbus(struct drm_i915_private *dev_priv)
 	unsigned int pin;
 	int ret;
 
-	if (INTEL_INFO(dev_priv)->num_pipes == 0)
-		return 0;
-
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		dev_priv->gpio_mmio_base = VLV_DISPLAY_BASE;
 	else if (!HAS_GMCH_DISPLAY(dev_priv))
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: Remove redundant checks for num_pipes == 0

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> This 'if's will always be false because of previous changes so let's
> drop then.

I also think this will gain clarity if i915_load_modeset_init() starts
with early return on no display.

>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 12 +++---------
>  drivers/gpu/drm/i915/intel_bios.c    |  5 -----
>  drivers/gpu/drm/i915/intel_display.c |  3 ---
>  drivers/gpu/drm/i915/intel_fbdev.c   |  3 ---
>  drivers/gpu/drm/i915/intel_i2c.c     |  3 ---
>  5 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8334d1797df7..d69faf70dcfd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -645,12 +645,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (i915_inject_load_failure())
>  		return -ENODEV;
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes) {
> -		ret = drm_vblank_init(&dev_priv->drm,
> -				      INTEL_INFO(dev_priv)->num_pipes);
> -		if (ret)
> -			goto out;
> -	}
> +	ret = drm_vblank_init(&dev_priv->drm, INTEL_INFO(dev_priv)->num_pipes);
> +	if (ret)
> +		goto out;
>  
>  	intel_bios_init(dev_priv);
>  
> @@ -683,9 +680,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	intel_setup_overlay(dev_priv);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> -		return 0;
> -
>  	ret = intel_fbdev_init(dev);
>  	if (ret)
>  		goto cleanup_modeset;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 1faa494e2bc9..1e0205572ff9 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1721,11 +1721,6 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	const struct bdb_header *bdb;
>  	u8 __iomem *bios = NULL;
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> -		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
> -		return;
> -	}
> -

I might leave in asserts such as

	if (WARN_ON(!HAS_DISPLAY(dev_priv)))
        	return;

in most places anyway.

BR,
Jani.


>  	init_vbt_defaults(dev_priv);
>  
>  	/* If the OpRegion does not have VBT, look in PCI ROM. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1d07fc41d492..d369c7336286 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13957,9 +13957,6 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  
>  	intel_pps_init(dev_priv);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> -		return;
> -
>  	/*
>  	 * intel_edp_init_connector() depends on this completing first, to
>  	 * prevent the registeration of both eDP and LVDS and the incorrect
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2480c7d6edee..5eaf1c35332b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -672,9 +672,6 @@ int intel_fbdev_init(struct drm_device *dev)
>  	struct intel_fbdev *ifbdev;
>  	int ret;
>  
> -	if (WARN_ON(INTEL_INFO(dev_priv)->num_pipes == 0))
> -		return -ENODEV;
> -
>  	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
>  	if (ifbdev == NULL)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 33d87ab93fdd..9c05f7af68a6 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -817,9 +817,6 @@ int intel_setup_gmbus(struct drm_i915_private *dev_priv)
>  	unsigned int pin;
>  	int ret;
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> -		return 0;
> -
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		dev_priv->gpio_mmio_base = VLV_DISPLAY_BASE;
>  	else if (!HAS_GMCH_DISPLAY(dev_priv))

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/16] drm/i915: Keep overlay functions naming consistent

[José Roberto de Souza]

All other overlay functions(almost all other functions in i915)
follow intel_overlay_verb, so renaming overlay ones that do not match
that.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 2 +-
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 4 ++--
 drivers/gpu/drm/i915/intel_overlay.c | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d69faf70dcfd..22cebe4871c9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -678,7 +678,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_gmbus;
 
-	intel_setup_overlay(dev_priv);
+	intel_overlay_setup(dev_priv);
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d369c7336286..6db3ef975ea8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15823,7 +15823,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	drm_mode_config_cleanup(dev);
 
-	intel_cleanup_overlay(dev_priv);
+	intel_overlay_cleanup(dev_priv);
 
 	intel_teardown_gmbus(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3dea7a1bda7f..5c33d68a48a4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1875,8 +1875,8 @@ struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
 bool intel_is_dual_link_lvds(struct drm_device *dev);
 
 /* intel_overlay.c */
-void intel_setup_overlay(struct drm_i915_private *dev_priv);
-void intel_cleanup_overlay(struct drm_i915_private *dev_priv);
+void intel_overlay_setup(struct drm_i915_private *dev_priv);
+void intel_overlay_cleanup(struct drm_i915_private *dev_priv);
 int intel_overlay_switch_off(struct intel_overlay *overlay);
 int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 72eb7e48e8bc..20ea7c99d13a 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1338,7 +1338,7 @@ static int get_registers(struct intel_overlay *overlay, bool use_phys)
 	return err;
 }
 
-void intel_setup_overlay(struct drm_i915_private *dev_priv)
+void intel_overlay_setup(struct drm_i915_private *dev_priv)
 {
 	struct intel_overlay *overlay;
 	int ret;
@@ -1387,7 +1387,7 @@ void intel_setup_overlay(struct drm_i915_private *dev_priv)
 	kfree(overlay);
 }
 
-void intel_cleanup_overlay(struct drm_i915_private *dev_priv)
+void intel_overlay_cleanup(struct drm_i915_private *dev_priv)
 {
 	struct intel_overlay *overlay;
 
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/16] drm/i915: Keep overlay functions naming consistent

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> All other overlay functions(almost all other functions in i915)
> follow intel_overlay_verb, so renaming overlay ones that do not match
> that.

Changes like this at the *start* of the series would have been merged
already. Now it depends on all the contentious stuff, and can't merge.

BR,
Jani.


>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 2 +-
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 4 ++--
>  drivers/gpu/drm/i915/intel_overlay.c | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d69faf70dcfd..22cebe4871c9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -678,7 +678,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_gmbus;
>  
> -	intel_setup_overlay(dev_priv);
> +	intel_overlay_setup(dev_priv);
>  
>  	ret = intel_fbdev_init(dev);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d369c7336286..6db3ef975ea8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15823,7 +15823,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	drm_mode_config_cleanup(dev);
>  
> -	intel_cleanup_overlay(dev_priv);
> +	intel_overlay_cleanup(dev_priv);
>  
>  	intel_teardown_gmbus(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3dea7a1bda7f..5c33d68a48a4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1875,8 +1875,8 @@ struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
>  bool intel_is_dual_link_lvds(struct drm_device *dev);
>  
>  /* intel_overlay.c */
> -void intel_setup_overlay(struct drm_i915_private *dev_priv);
> -void intel_cleanup_overlay(struct drm_i915_private *dev_priv);
> +void intel_overlay_setup(struct drm_i915_private *dev_priv);
> +void intel_overlay_cleanup(struct drm_i915_private *dev_priv);
>  int intel_overlay_switch_off(struct intel_overlay *overlay);
>  int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 72eb7e48e8bc..20ea7c99d13a 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1338,7 +1338,7 @@ static int get_registers(struct intel_overlay *overlay, bool use_phys)
>  	return err;
>  }
>  
> -void intel_setup_overlay(struct drm_i915_private *dev_priv)
> +void intel_overlay_setup(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_overlay *overlay;
>  	int ret;
> @@ -1387,7 +1387,7 @@ void intel_setup_overlay(struct drm_i915_private *dev_priv)
>  	kfree(overlay);
>  }
>  
> -void intel_cleanup_overlay(struct drm_i915_private *dev_priv)
> +void intel_overlay_cleanup(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_overlay *overlay;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/16] drm/i915: Do not reset display when display is disabled

[José Roberto de Souza]

Display is always disabled and enabled when reseting any engine,
but if display is disabled it should not do anything with display
and only reset the needed engines.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2e242270e270..e7f551909bfe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3173,7 +3173,8 @@ static void i915_reset_device(struct drm_i915_private *dev_priv,
 
 	/* Use a watchdog to ensure that our reset completes */
 	i915_wedge_on_timeout(&w, dev_priv, 5*HZ) {
-		intel_prepare_reset(dev_priv);
+		if (INTEL_INFO(dev_priv)->num_pipes)
+			intel_prepare_reset(dev_priv);
 
 		error->reason = reason;
 		error->stalled_mask = engine_mask;
@@ -3199,7 +3200,8 @@ static void i915_reset_device(struct drm_i915_private *dev_priv,
 		error->stalled_mask = 0;
 		error->reason = NULL;
 
-		intel_finish_reset(dev_priv);
+		if (INTEL_INFO(dev_priv)->num_pipes)
+			intel_finish_reset(dev_priv);
 	}
 
 	if (!test_bit(I915_WEDGED, &error->flags))
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/16] drm/i915: Do not reset display when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> Display is always disabled and enabled when reseting any engine,
> but if display is disabled it should not do anything with display
> and only reset the needed engines.

Again, push the display checks to the functions themselves.

BR,
Jani.

>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2e242270e270..e7f551909bfe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3173,7 +3173,8 @@ static void i915_reset_device(struct drm_i915_private *dev_priv,
>  
>  	/* Use a watchdog to ensure that our reset completes */
>  	i915_wedge_on_timeout(&w, dev_priv, 5*HZ) {
> -		intel_prepare_reset(dev_priv);
> +		if (INTEL_INFO(dev_priv)->num_pipes)
> +			intel_prepare_reset(dev_priv);
>  
>  		error->reason = reason;
>  		error->stalled_mask = engine_mask;
> @@ -3199,7 +3200,8 @@ static void i915_reset_device(struct drm_i915_private *dev_priv,
>  		error->stalled_mask = 0;
>  		error->reason = NULL;
>  
> -		intel_finish_reset(dev_priv);
> +		if (INTEL_INFO(dev_priv)->num_pipes)
> +			intel_finish_reset(dev_priv);
>  	}
>  
>  	if (!test_bit(I915_WEDGED, &error->flags))

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/16] drm/i915: Do not initialize display clocks when display is disabled

[José Roberto de Souza]

cdclk and rawclk are the 2 display clocks that can now be completed
not initialized when display is disabled.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  9 ++++++---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 27 +++++++++++++++++--------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 22cebe4871c9..4ec598b0a737 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -900,7 +900,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 		goto err_uc;
 	intel_irq_init(dev_priv);
 	intel_hangcheck_init(dev_priv);
-	intel_init_display_hooks(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_init_display_hooks(dev_priv);
 	intel_init_clock_gating_hooks(dev_priv);
 	if (INTEL_INFO(dev_priv)->num_pipes) {
 		intel_init_audio_hooks(dev_priv);
@@ -1709,7 +1710,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_cleanup_mmio;
 
 	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
-	intel_update_rawclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_update_rawclk(dev_priv);
 
 	/* i915_gem_init() call chain will call
 	 * intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ);
@@ -2103,7 +2105,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	i915_gem_resume(dev_priv);
 
-	intel_modeset_init_hw(dev);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		intel_modeset_init_hw(dev);
 	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 15fd88bb35f7..3b2588a377a9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -801,6 +801,9 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
 	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
 	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
@@ -3293,7 +3296,8 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
-	skl_init_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		skl_init_cdclk(dev_priv);
 
 	gen9_dbuf_enable(dev_priv);
 
@@ -3310,7 +3314,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_dbuf_disable(dev_priv);
 
-	skl_uninit_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		skl_uninit_cdclk(dev_priv);
 
 	/* The spec doesn't call for removing the reset handshake flag */
 	/* disable PG1 and Misc I/O */
@@ -3355,7 +3360,8 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
-	bxt_init_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		bxt_init_cdclk(dev_priv);
 
 	gen9_dbuf_enable(dev_priv);
 
@@ -3372,7 +3378,8 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_dbuf_disable(dev_priv);
 
-	bxt_uninit_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		bxt_uninit_cdclk(dev_priv);
 
 	/* The spec doesn't call for removing the reset handshake flag */
 
@@ -3495,7 +3502,8 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
 	mutex_unlock(&power_domains->lock);
 
 	/* 5. Enable CD clock */
-	cnl_init_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		cnl_init_cdclk(dev_priv);
 
 	/* 6. Enable DBUF */
 	gen9_dbuf_enable(dev_priv);
@@ -3518,7 +3526,8 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
 	gen9_dbuf_disable(dev_priv);
 
 	/* 3. Disable CD clock */
-	cnl_uninit_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		cnl_uninit_cdclk(dev_priv);
 
 	/*
 	 * 4. Disable Power Well 1 (PG1).
@@ -3579,7 +3588,8 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
 	mutex_unlock(&power_domains->lock);
 
 	/* 5. Enable CDCLK. */
-	icl_init_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		icl_init_cdclk(dev_priv);
 
 	/* 6. Enable DBUF. */
 	icl_dbuf_enable(dev_priv);
@@ -3606,7 +3616,8 @@ static void icl_display_core_uninit(struct drm_i915_private *dev_priv)
 	icl_dbuf_disable(dev_priv);
 
 	/* 3. Disable CD clock */
-	icl_uninit_cdclk(dev_priv);
+	if (INTEL_INFO(dev_priv)->num_pipes)
+		icl_uninit_cdclk(dev_priv);
 
 	/*
 	 * 4. Disable Power Well 1 (PG1).
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/16] drm/i915: Do not initialize display clocks when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> cdclk and rawclk are the 2 display clocks that can now be completed
> not initialized when display is disabled.

Again, at least at the i915_driver_init_early/i915_driver_load level,
don't add the display checks, add early returns in the functions
themselves.

At the platform specific core uninit/init level this is fine as is.

BR,
Jani.

>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  9 ++++++---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 27 +++++++++++++++++--------
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 22cebe4871c9..4ec598b0a737 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -900,7 +900,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>  		goto err_uc;
>  	intel_irq_init(dev_priv);
>  	intel_hangcheck_init(dev_priv);
> -	intel_init_display_hooks(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_init_display_hooks(dev_priv);
>  	intel_init_clock_gating_hooks(dev_priv);
>  	if (INTEL_INFO(dev_priv)->num_pipes) {
>  		intel_init_audio_hooks(dev_priv);
> @@ -1709,7 +1710,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto out_cleanup_mmio;
>  
>  	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
> -	intel_update_rawclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_update_rawclk(dev_priv);
>  
>  	/* i915_gem_init() call chain will call
>  	 * intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ);
> @@ -2103,7 +2105,8 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>  	i915_gem_resume(dev_priv);
>  
> -	intel_modeset_init_hw(dev);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		intel_modeset_init_hw(dev);
>  	intel_init_clock_gating(dev_priv);
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 15fd88bb35f7..3b2588a377a9 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -801,6 +801,9 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
>  	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
>  	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> @@ -3293,7 +3296,8 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
>  
>  	mutex_unlock(&power_domains->lock);
>  
> -	skl_init_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		skl_init_cdclk(dev_priv);
>  
>  	gen9_dbuf_enable(dev_priv);
>  
> @@ -3310,7 +3314,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_dbuf_disable(dev_priv);
>  
> -	skl_uninit_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		skl_uninit_cdclk(dev_priv);
>  
>  	/* The spec doesn't call for removing the reset handshake flag */
>  	/* disable PG1 and Misc I/O */
> @@ -3355,7 +3360,8 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
>  
>  	mutex_unlock(&power_domains->lock);
>  
> -	bxt_init_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		bxt_init_cdclk(dev_priv);
>  
>  	gen9_dbuf_enable(dev_priv);
>  
> @@ -3372,7 +3378,8 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_dbuf_disable(dev_priv);
>  
> -	bxt_uninit_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		bxt_uninit_cdclk(dev_priv);
>  
>  	/* The spec doesn't call for removing the reset handshake flag */
>  
> @@ -3495,7 +3502,8 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
>  	mutex_unlock(&power_domains->lock);
>  
>  	/* 5. Enable CD clock */
> -	cnl_init_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		cnl_init_cdclk(dev_priv);
>  
>  	/* 6. Enable DBUF */
>  	gen9_dbuf_enable(dev_priv);
> @@ -3518,7 +3526,8 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
>  	gen9_dbuf_disable(dev_priv);
>  
>  	/* 3. Disable CD clock */
> -	cnl_uninit_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		cnl_uninit_cdclk(dev_priv);
>  
>  	/*
>  	 * 4. Disable Power Well 1 (PG1).
> @@ -3579,7 +3588,8 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&power_domains->lock);
>  
>  	/* 5. Enable CDCLK. */
> -	icl_init_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		icl_init_cdclk(dev_priv);
>  
>  	/* 6. Enable DBUF. */
>  	icl_dbuf_enable(dev_priv);
> @@ -3606,7 +3616,8 @@ static void icl_display_core_uninit(struct drm_i915_private *dev_priv)
>  	icl_dbuf_disable(dev_priv);
>  
>  	/* 3. Disable CD clock */
> -	icl_uninit_cdclk(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		icl_uninit_cdclk(dev_priv);
>  
>  	/*
>  	 * 4. Disable Power Well 1 (PG1).

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/16] drm/i915: Do not initialize display core when display is disabled

[José Roberto de Souza]

With display disabled, driver don't need to enable any power well
or load the DMC firmware.

The only thing that *_display_core_init will do when display is
disabled is call intel_pch_reset_handshake(), so PCH handshake
will be unset and in counterpart *_display_core_uninit() will
only disable DC.

The power wells enabled by BIOS during boot will be disabled in
futher patch.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 56 ++++++++++++++++---------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3b2588a377a9..8b1c4d0db0af 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3285,6 +3285,9 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
 	/* enable PCH reset handshake */
 	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	/* enable PG1 and Misc I/O */
 	mutex_lock(&power_domains->lock);
 
@@ -3296,8 +3299,7 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		skl_init_cdclk(dev_priv);
+	skl_init_cdclk(dev_priv);
 
 	gen9_dbuf_enable(dev_priv);
 
@@ -3312,10 +3314,12 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	gen9_dbuf_disable(dev_priv);
 
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		skl_uninit_cdclk(dev_priv);
+	skl_uninit_cdclk(dev_priv);
 
 	/* The spec doesn't call for removing the reset handshake flag */
 	/* disable PG1 and Misc I/O */
@@ -3352,6 +3356,9 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
 	 */
 	intel_pch_reset_handshake(dev_priv, false);
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	/* Enable PG1 */
 	mutex_lock(&power_domains->lock);
 
@@ -3360,8 +3367,7 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		bxt_init_cdclk(dev_priv);
+	bxt_init_cdclk(dev_priv);
 
 	gen9_dbuf_enable(dev_priv);
 
@@ -3376,10 +3382,12 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	gen9_dbuf_disable(dev_priv);
 
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		bxt_uninit_cdclk(dev_priv);
+	bxt_uninit_cdclk(dev_priv);
 
 	/* The spec doesn't call for removing the reset handshake flag */
 
@@ -3475,6 +3483,9 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
 	/* 1. Enable PCH Reset Handshake */
 	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	/* 2. Enable Comp */
 	val = I915_READ(CHICKEN_MISC_2);
 	val &= ~CNL_COMP_PWR_DOWN;
@@ -3502,8 +3513,7 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
 	mutex_unlock(&power_domains->lock);
 
 	/* 5. Enable CD clock */
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		cnl_init_cdclk(dev_priv);
+	cnl_init_cdclk(dev_priv);
 
 	/* 6. Enable DBUF */
 	gen9_dbuf_enable(dev_priv);
@@ -3520,14 +3530,16 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
-	/* 1. Disable all display engine functions -> aready done */
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
+	/* 1. Disable all display engine functions -> already done */
 
 	/* 2. Disable DBUF */
 	gen9_dbuf_disable(dev_priv);
 
 	/* 3. Disable CD clock */
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		cnl_uninit_cdclk(dev_priv);
+	cnl_uninit_cdclk(dev_priv);
 
 	/*
 	 * 4. Disable Power Well 1 (PG1).
@@ -3560,6 +3572,9 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
 	/* 1. Enable PCH reset handshake. */
 	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	for (port = PORT_A; port <= PORT_B; port++) {
 		/* 2. Enable DDI combo PHY comp. */
 		val = I915_READ(ICL_PHY_MISC(port));
@@ -3588,8 +3603,7 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
 	mutex_unlock(&power_domains->lock);
 
 	/* 5. Enable CDCLK. */
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		icl_init_cdclk(dev_priv);
+	icl_init_cdclk(dev_priv);
 
 	/* 6. Enable DBUF. */
 	icl_dbuf_enable(dev_priv);
@@ -3610,14 +3624,16 @@ static void icl_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
-	/* 1. Disable all display engine functions -> aready done */
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
+	/* 1. Disable all display engine functions -> already done */
 
 	/* 2. Disable DBUF */
 	icl_dbuf_disable(dev_priv);
 
 	/* 3. Disable CD clock */
-	if (INTEL_INFO(dev_priv)->num_pipes)
-		icl_uninit_cdclk(dev_priv);
+	icl_uninit_cdclk(dev_priv);
 
 	/*
 	 * 4. Disable Power Well 1 (PG1).
@@ -3784,11 +3800,11 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		skl_display_core_init(dev_priv, resume);
 	} else if (IS_GEN9_LP(dev_priv)) {
 		bxt_display_core_init(dev_priv, resume);
-	} else if (IS_CHERRYVIEW(dev_priv)) {
+	} else if (IS_CHERRYVIEW(dev_priv) && INTEL_INFO(dev_priv)->num_pipes) {
 		mutex_lock(&power_domains->lock);
 		chv_phy_control_init(dev_priv);
 		mutex_unlock(&power_domains->lock);
-	} else if (IS_VALLEYVIEW(dev_priv)) {
+	} else if (IS_VALLEYVIEW(dev_priv) && INTEL_INFO(dev_priv)->num_pipes) {
 		mutex_lock(&power_domains->lock);
 		vlv_cmnlane_wa(dev_priv);
 		mutex_unlock(&power_domains->lock);
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/16] drm/i915: Do not initialize display core when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> With display disabled, driver don't need to enable any power well
> or load the DMC firmware.
>
> The only thing that *_display_core_init will do when display is
> disabled is call intel_pch_reset_handshake(), so PCH handshake
> will be unset and in counterpart *_display_core_uninit() will
> only disable DC.
>
> The power wells enabled by BIOS during boot will be disabled in
> futher patch.
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 56 ++++++++++++++++---------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3b2588a377a9..8b1c4d0db0af 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -3285,6 +3285,9 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
>  	/* enable PCH reset handshake */
>  	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	/* enable PG1 and Misc I/O */
>  	mutex_lock(&power_domains->lock);
>  
> @@ -3296,8 +3299,7 @@ static void skl_display_core_init(struct drm_i915_private *dev_priv,
>  
>  	mutex_unlock(&power_domains->lock);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		skl_init_cdclk(dev_priv);
> +	skl_init_cdclk(dev_priv);

Please do the series in a way that doesn't need to undo stuff you add
earlier in the series.

>  
>  	gen9_dbuf_enable(dev_priv);
>  
> @@ -3312,10 +3314,12 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	gen9_dbuf_disable(dev_priv);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		skl_uninit_cdclk(dev_priv);
> +	skl_uninit_cdclk(dev_priv);
>  
>  	/* The spec doesn't call for removing the reset handshake flag */
>  	/* disable PG1 and Misc I/O */
> @@ -3352,6 +3356,9 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
>  	 */
>  	intel_pch_reset_handshake(dev_priv, false);
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	/* Enable PG1 */
>  	mutex_lock(&power_domains->lock);
>  
> @@ -3360,8 +3367,7 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
>  
>  	mutex_unlock(&power_domains->lock);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		bxt_init_cdclk(dev_priv);
> +	bxt_init_cdclk(dev_priv);
>  
>  	gen9_dbuf_enable(dev_priv);
>  
> @@ -3376,10 +3382,12 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	gen9_dbuf_disable(dev_priv);
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		bxt_uninit_cdclk(dev_priv);
> +	bxt_uninit_cdclk(dev_priv);
>  
>  	/* The spec doesn't call for removing the reset handshake flag */
>  
> @@ -3475,6 +3483,9 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
>  	/* 1. Enable PCH Reset Handshake */
>  	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	/* 2. Enable Comp */
>  	val = I915_READ(CHICKEN_MISC_2);
>  	val &= ~CNL_COMP_PWR_DOWN;
> @@ -3502,8 +3513,7 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
>  	mutex_unlock(&power_domains->lock);
>  
>  	/* 5. Enable CD clock */
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		cnl_init_cdclk(dev_priv);
> +	cnl_init_cdclk(dev_priv);
>  
>  	/* 6. Enable DBUF */
>  	gen9_dbuf_enable(dev_priv);
> @@ -3520,14 +3530,16 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> -	/* 1. Disable all display engine functions -> aready done */
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
> +	/* 1. Disable all display engine functions -> already done */
>  
>  	/* 2. Disable DBUF */
>  	gen9_dbuf_disable(dev_priv);
>  
>  	/* 3. Disable CD clock */
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		cnl_uninit_cdclk(dev_priv);
> +	cnl_uninit_cdclk(dev_priv);
>  
>  	/*
>  	 * 4. Disable Power Well 1 (PG1).
> @@ -3560,6 +3572,9 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
>  	/* 1. Enable PCH reset handshake. */
>  	intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
>  	for (port = PORT_A; port <= PORT_B; port++) {
>  		/* 2. Enable DDI combo PHY comp. */
>  		val = I915_READ(ICL_PHY_MISC(port));
> @@ -3588,8 +3603,7 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&power_domains->lock);
>  
>  	/* 5. Enable CDCLK. */
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		icl_init_cdclk(dev_priv);
> +	icl_init_cdclk(dev_priv);
>  
>  	/* 6. Enable DBUF. */
>  	icl_dbuf_enable(dev_priv);
> @@ -3610,14 +3624,16 @@ static void icl_display_core_uninit(struct drm_i915_private *dev_priv)
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> -	/* 1. Disable all display engine functions -> aready done */
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return;
> +
> +	/* 1. Disable all display engine functions -> already done */
>  
>  	/* 2. Disable DBUF */
>  	icl_dbuf_disable(dev_priv);
>  
>  	/* 3. Disable CD clock */
> -	if (INTEL_INFO(dev_priv)->num_pipes)
> -		icl_uninit_cdclk(dev_priv);
> +	icl_uninit_cdclk(dev_priv);
>  
>  	/*
>  	 * 4. Disable Power Well 1 (PG1).
> @@ -3784,11 +3800,11 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
>  		skl_display_core_init(dev_priv, resume);
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		bxt_display_core_init(dev_priv, resume);
> -	} else if (IS_CHERRYVIEW(dev_priv)) {
> +	} else if (IS_CHERRYVIEW(dev_priv) && INTEL_INFO(dev_priv)->num_pipes) {
>  		mutex_lock(&power_domains->lock);
>  		chv_phy_control_init(dev_priv);
>  		mutex_unlock(&power_domains->lock);
> -	} else if (IS_VALLEYVIEW(dev_priv)) {
> +	} else if (IS_VALLEYVIEW(dev_priv) && INTEL_INFO(dev_priv)->num_pipes) {
>  		mutex_lock(&power_domains->lock);
>  		vlv_cmnlane_wa(dev_priv);
>  		mutex_unlock(&power_domains->lock);

Please don't mix platform specific if ladders with display or not.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/16] drm/i915: Warn when display irq functions is executed when display is disabled

[José Roberto de Souza]

With previous changes none of those warnings will be printed but lets
add then so CI can caught regressions.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_hotplug.c |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e7f551909bfe..92e2c7e081e5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2029,6 +2029,8 @@ static u32 i9xx_hpd_irq_ack(struct drm_i915_private *dev_priv)
 	u32 hotplug_status = 0, hotplug_status_mask;
 	int i;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		hotplug_status_mask = HOTPLUG_INT_STATUS_G4X |
@@ -2067,6 +2069,8 @@ static void i9xx_hpd_irq_handler(struct drm_i915_private *dev_priv,
 {
 	u32 pin_mask = 0, long_mask = 0;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
 	    IS_CHERRYVIEW(dev_priv)) {
 		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
@@ -2511,6 +2515,8 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
 	enum pipe pipe;
 	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (hotplug_trigger)
 		ilk_hpd_irq_handler(dev_priv, hotplug_trigger, hpd_ilk);
 
@@ -2557,6 +2563,8 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
 	enum pipe pipe;
 	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG_IVB;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (hotplug_trigger)
 		ilk_hpd_irq_handler(dev_priv, hotplug_trigger, hpd_ivb);
 
@@ -2725,6 +2733,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 	u32 iir;
 	enum pipe pipe;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (master_ctl & GEN8_DE_MISC_IRQ) {
 		iir = I915_READ(GEN8_DE_MISC_IIR);
 		if (iir) {
@@ -3875,6 +3885,8 @@ static void spt_hpd_irq_setup(struct drm_i915_private *dev_priv)
 {
 	u32 hotplug_irqs, enabled_irqs;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	hotplug_irqs = SDE_HOTPLUG_MASK_SPT;
 	enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_spt);
 
@@ -3903,6 +3915,8 @@ static void ilk_hpd_irq_setup(struct drm_i915_private *dev_priv)
 {
 	u32 hotplug_irqs, enabled_irqs;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	if (INTEL_GEN(dev_priv) >= 8) {
 		hotplug_irqs = GEN8_PORT_DP_A_HOTPLUG;
 		enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bdw);
@@ -3965,6 +3979,8 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv)
 {
 	u32 hotplug_irqs, enabled_irqs;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bxt);
 	hotplug_irqs = BXT_DE_PORT_HOTPLUG_MASK;
 
@@ -4682,6 +4698,8 @@ static void i915_hpd_irq_setup(struct drm_i915_private *dev_priv)
 {
 	u32 hotplug_en;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	lockdep_assert_held(&dev_priv->irq_lock);
 
 	/* Note HDMI and DP share hotplug bits */
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 648a13c6043c..908d8e589f9a 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -399,6 +399,8 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	if (!pin_mask)
 		return;
 
+	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
+
 	spin_lock(&dev_priv->irq_lock);
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
 		enum hpd_pin pin = encoder->hpd_pin;
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/16] drm/i915: Warn when display irq functions is executed when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> With previous changes none of those warnings will be printed but lets
> add then so CI can caught regressions.

I like the asserts, but here too they make more sense to the casual
reader as !HAS_DISPLAY.

BR,
Jani.

>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_hotplug.c |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e7f551909bfe..92e2c7e081e5 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2029,6 +2029,8 @@ static u32 i9xx_hpd_irq_ack(struct drm_i915_private *dev_priv)
>  	u32 hotplug_status = 0, hotplug_status_mask;
>  	int i;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		hotplug_status_mask = HOTPLUG_INT_STATUS_G4X |
> @@ -2067,6 +2069,8 @@ static void i9xx_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  {
>  	u32 pin_mask = 0, long_mask = 0;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
>  	    IS_CHERRYVIEW(dev_priv)) {
>  		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
> @@ -2511,6 +2515,8 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
>  	enum pipe pipe;
>  	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (hotplug_trigger)
>  		ilk_hpd_irq_handler(dev_priv, hotplug_trigger, hpd_ilk);
>  
> @@ -2557,6 +2563,8 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
>  	enum pipe pipe;
>  	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG_IVB;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (hotplug_trigger)
>  		ilk_hpd_irq_handler(dev_priv, hotplug_trigger, hpd_ivb);
>  
> @@ -2725,6 +2733,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  	u32 iir;
>  	enum pipe pipe;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (master_ctl & GEN8_DE_MISC_IRQ) {
>  		iir = I915_READ(GEN8_DE_MISC_IIR);
>  		if (iir) {
> @@ -3875,6 +3885,8 @@ static void spt_hpd_irq_setup(struct drm_i915_private *dev_priv)
>  {
>  	u32 hotplug_irqs, enabled_irqs;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	hotplug_irqs = SDE_HOTPLUG_MASK_SPT;
>  	enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_spt);
>  
> @@ -3903,6 +3915,8 @@ static void ilk_hpd_irq_setup(struct drm_i915_private *dev_priv)
>  {
>  	u32 hotplug_irqs, enabled_irqs;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	if (INTEL_GEN(dev_priv) >= 8) {
>  		hotplug_irqs = GEN8_PORT_DP_A_HOTPLUG;
>  		enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bdw);
> @@ -3965,6 +3979,8 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv)
>  {
>  	u32 hotplug_irqs, enabled_irqs;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bxt);
>  	hotplug_irqs = BXT_DE_PORT_HOTPLUG_MASK;
>  
> @@ -4682,6 +4698,8 @@ static void i915_hpd_irq_setup(struct drm_i915_private *dev_priv)
>  {
>  	u32 hotplug_en;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	lockdep_assert_held(&dev_priv->irq_lock);
>  
>  	/* Note HDMI and DP share hotplug bits */
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 648a13c6043c..908d8e589f9a 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -399,6 +399,8 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  	if (!pin_mask)
>  		return;
>  
> +	WARN_ON_ONCE(!INTEL_INFO(dev_priv)->num_pipes);
> +
>  	spin_lock(&dev_priv->irq_lock);
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
>  		enum hpd_pin pin = encoder->hpd_pin;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/16] drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded

[José Roberto de Souza]

When DMC firmware is not loaded, it return earlier in
gen9_dc_off_power_well_disable() as it will have no effect without
DMC firmware loaded. But it will cause a mismatch state error when
running intel_power_domains_verify_state(), so skipping this error
in this case.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8b1c4d0db0af..629091ad8337 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -4014,11 +4014,22 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 		enabled = power_well->desc->ops->is_enabled(dev_priv,
 							    power_well);
 		if ((power_well->count || power_well->desc->always_on) !=
-		    enabled)
+		    enabled) {
+			/* If DMC firmware is not loaded it could cause a
+			 * mismatch state as we can't disable DC off, so let's
+			 * do not print any errors in this scenario.
+			 */
+
+			if (!strcmp("DC off", power_well->desc->name) &&
+			    !dev_priv->csr.dmc_payload)
+				goto skip_state_mismatch_error;
+
 			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
 				  power_well->desc->name,
 				  power_well->count, enabled);
+		}
 
+skip_state_mismatch_error:
 		domains_count = 0;
 		for_each_power_domain(domain, power_well->desc->domains)
 			domains_count += power_domains->domain_use_count[domain];
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/16] drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded

[Imre Deak]

On Fri, Oct 12, 2018 at 02:52:15PM -0700, José Roberto de Souza wrote:
> When DMC firmware is not loaded, it return earlier in
> gen9_dc_off_power_well_disable() as it will have no effect without
> DMC firmware loaded. But it will cause a mismatch state error when
> running intel_power_domains_verify_state(), so skipping this error
> in this case.

DC states are disabled when DMC is not loaded and we won't ever enable
them, as runtime PM as a whole is disabled. So not sure why you get a
mismatch error, but ignoring that by special casing it doesn't seem
correct.

> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8b1c4d0db0af..629091ad8337 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -4014,11 +4014,22 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  		enabled = power_well->desc->ops->is_enabled(dev_priv,
>  							    power_well);
>  		if ((power_well->count || power_well->desc->always_on) !=
> -		    enabled)
> +		    enabled) {
> +			/* If DMC firmware is not loaded it could cause a
> +			 * mismatch state as we can't disable DC off, so let's
> +			 * do not print any errors in this scenario.
> +			 */
> +
> +			if (!strcmp("DC off", power_well->desc->name) &&
> +			    !dev_priv->csr.dmc_payload)
> +				goto skip_state_mismatch_error;
> +
>  			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
>  				  power_well->desc->name,
>  				  power_well->count, enabled);
> +		}
>  
> +skip_state_mismatch_error:
>  		domains_count = 0;
>  		for_each_power_domain(domain, power_well->desc->domains)
>  			domains_count += power_domains->domain_use_count[domain];
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/16] drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded

[Souza, Jose]

On Mon, 2018-10-15 at 13:58 +0300, Imre Deak wrote:
> On Fri, Oct 12, 2018 at 02:52:15PM -0700, José Roberto de Souza
> wrote:
> > When DMC firmware is not loaded, it return earlier in
> > gen9_dc_off_power_well_disable() as it will have no effect without
> > DMC firmware loaded. But it will cause a mismatch state error when
> > running intel_power_domains_verify_state(), so skipping this error
> > in this case.
> 
> DC states are disabled when DMC is not loaded and we won't ever
> enable
> them, as runtime PM as a whole is disabled. So not sure why you get a
> mismatch error, but ignoring that by special casing it doesn't seem
> correct.

I just found out a bug that was hidden this bug from drm-tip, I just
sent a separated PR(https://patchwork.freedesktop.org/series/51039/)
with this patch and the fix please take a look.

But it was hidden because at every call to
intel_power_domains_verify_state() something was holding a reference to
one of DC_OFF domains, keeping it in the on state that means DC is
disabaled.
If you run drm-tip with the fix without any output all references to
DC_OFF domain will be release but as DMC firmware was not loaded DC is
not actualy enabled causing the state mismatch.
A easy way to reproduce is just run drm-tip with disable_display=1 and
check the error message in dmesg.

Although I'm not 100% happy with the way that I'm identifying DC_OFF
power well, do you have suggestion?

> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 8b1c4d0db0af..629091ad8337 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -4014,11 +4014,22 @@ static void
> > intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  		enabled = power_well->desc->ops->is_enabled(dev_priv,
> >  							    power_well)
> > ;
> >  		if ((power_well->count || power_well->desc->always_on)
> > !=
> > -		    enabled)
> > +		    enabled) {
> > +			/* If DMC firmware is not loaded it could cause
> > a
> > +			 * mismatch state as we can't disable DC off,
> > so let's
> > +			 * do not print any errors in this scenario.
> > +			 */
> > +
> > +			if (!strcmp("DC off", power_well->desc->name)
> > &&
> > +			    !dev_priv->csr.dmc_payload)
> > +				goto skip_state_mismatch_error;
> > +
> >  			DRM_ERROR("power well %s state mismatch
> > (refcount %d/enabled %d)",
> >  				  power_well->desc->name,
> >  				  power_well->count, enabled);
> > +		}
> >  
> > +skip_state_mismatch_error:
> >  		domains_count = 0;
> >  		for_each_power_domain(domain, power_well->desc-
> > >domains)
> >  			domains_count += power_domains-
> > >domain_use_count[domain];
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/16] drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded

[Imre Deak]

On Tue, Oct 16, 2018 at 04:31:37AM +0300, Souza, Jose wrote:
> On Mon, 2018-10-15 at 13:58 +0300, Imre Deak wrote:
> > On Fri, Oct 12, 2018 at 02:52:15PM -0700, José Roberto de Souza
> > wrote:
> > > When DMC firmware is not loaded, it return earlier in
> > > gen9_dc_off_power_well_disable() as it will have no effect without
> > > DMC firmware loaded. But it will cause a mismatch state error when
> > > running intel_power_domains_verify_state(), so skipping this error
> > > in this case.
> > 
> > DC states are disabled when DMC is not loaded and we won't ever
> > enable
> > them, as runtime PM as a whole is disabled. So not sure why you get a
> > mismatch error, but ignoring that by special casing it doesn't seem
> > correct.
> 
> I just found out a bug that was hidden this bug from drm-tip, I just
> sent a separated PR(https://patchwork.freedesktop.org/series/51039/)
> with this patch and the fix please take a look.
> 
> But it was hidden because at every call to
> intel_power_domains_verify_state() something was holding a reference to
> one of DC_OFF domains, keeping it in the on state that means DC is
> disabaled.

Yes, runtime PM is kept disabled if the DMC firmware is not loaded. This
also means we keep all power wells in the INIT domain - including the
DC_OFF power well - enabled.

> If you run drm-tip with the fix without any output all references to
> DC_OFF domain will be release but as DMC firmware was not loaded DC is
> not actualy enabled causing the state mismatch.
> A easy way to reproduce is just run drm-tip with disable_display=1 and
> check the error message in dmesg.
> 
> Although I'm not 100% happy with the way that I'm identifying DC_OFF
> power well, do you have suggestion?
> 
> > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 8b1c4d0db0af..629091ad8337 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -4014,11 +4014,22 @@ static void
> > > intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> > >  		enabled = power_well->desc->ops->is_enabled(dev_priv,
> > >  							    power_well)
> > > ;
> > >  		if ((power_well->count || power_well->desc->always_on)
> > > !=
> > > -		    enabled)
> > > +		    enabled) {
> > > +			/* If DMC firmware is not loaded it could cause
> > > a
> > > +			 * mismatch state as we can't disable DC off,
> > > so let's
> > > +			 * do not print any errors in this scenario.
> > > +			 */
> > > +
> > > +			if (!strcmp("DC off", power_well->desc->name)
> > > &&
> > > +			    !dev_priv->csr.dmc_payload)
> > > +				goto skip_state_mismatch_error;
> > > +
> > >  			DRM_ERROR("power well %s state mismatch
> > > (refcount %d/enabled %d)",
> > >  				  power_well->desc->name,
> > >  				  power_well->count, enabled);
> > > +		}
> > >  
> > > +skip_state_mismatch_error:
> > >  		domains_count = 0;
> > >  		for_each_power_domain(domain, power_well->desc-
> > > >domains)
> > >  			domains_count += power_domains-
> > > >domain_use_count[domain];
> > > -- 
> > > 2.19.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/16] drm/i915: Do not turn power wells on or off when display is disabled

[José Roberto de Souza]

There is just two power wells calls left after the previous changes:
- POWER_DOMAIN_INIT: used in load, unload, resume and suspend driver
paths
- POWER_DOMAIN_GT_IRQ: used by GEM to reduce interrupt latencies when
DMC is loaded

Instead of adding several more 'if (INTEL_INFO(dev_priv)->num_pipes)'
it will be handled in intel_display_power_get/put() and if any
erroneous call is added later a error message will be printed making
easy get regressions.

Other important point is that it will not turn power wells on or off
but it will still grab and release runtime power management
references this way kernel can power down the whole GPU when it is
not in use.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 629091ad8337..56c65d921acd 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1524,6 +1524,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		DRM_ERROR("Enabling a power well with display disabled");
+
 	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
 		intel_power_well_get(dev_priv, power_well);
 
@@ -1549,6 +1552,13 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 
 	intel_runtime_pm_get(dev_priv);
 
+	/* With display disabled this should be the only 2 power domains
+	 * requested
+	 */
+	if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) &&
+	    !INTEL_INFO(dev_priv)->num_pipes)
+		return;
+
 	mutex_lock(&power_domains->lock);
 
 	__intel_display_power_get_domain(dev_priv, domain);
@@ -1609,6 +1619,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	struct i915_power_domains *power_domains;
 	struct i915_power_well *power_well;
 
+	if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) &&
+	    !INTEL_INFO(dev_priv)->num_pipes)
+		goto end;
+
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -1623,6 +1637,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
+end:
 	intel_runtime_pm_put(dev_priv);
 }
 
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/16] drm/i915: Do not turn power wells on or off when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> There is just two power wells calls left after the previous changes:
> - POWER_DOMAIN_INIT: used in load, unload, resume and suspend driver
> paths
> - POWER_DOMAIN_GT_IRQ: used by GEM to reduce interrupt latencies when
> DMC is loaded
>
> Instead of adding several more 'if (INTEL_INFO(dev_priv)->num_pipes)'
> it will be handled in intel_display_power_get/put() and if any
> erroneous call is added later a error message will be printed making
> easy get regressions.
>
> Other important point is that it will not turn power wells on or off
> but it will still grab and release runtime power management
> references this way kernel can power down the whole GPU when it is
> not in use.
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 629091ad8337..56c65d921acd 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1524,6 +1524,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  	struct i915_power_well *power_well;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		DRM_ERROR("Enabling a power well with display disabled");

What can the user do about this? It's an assert, so please make it a
WARN_ON() and we'll get a backtrace as a bonus helping us fix the issue.

> +
>  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
>  		intel_power_well_get(dev_priv, power_well);
>  
> @@ -1549,6 +1552,13 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> +	/* With display disabled this should be the only 2 power domains
> +	 * requested
> +	 */
> +	if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) &&
> +	    !INTEL_INFO(dev_priv)->num_pipes)
> +		return;

I do wonder if this is the right thing to do. How bad is it? Where do we
have the power get calls in areas that aren't already bypassed due to no
displays?

BR,
Jani.


> +
>  	mutex_lock(&power_domains->lock);
>  
>  	__intel_display_power_get_domain(dev_priv, domain);
> @@ -1609,6 +1619,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	struct i915_power_domains *power_domains;
>  	struct i915_power_well *power_well;
>  
> +	if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) &&
> +	    !INTEL_INFO(dev_priv)->num_pipes)
> +		goto end;
> +
>  	power_domains = &dev_priv->power_domains;
>  
>  	mutex_lock(&power_domains->lock);
> @@ -1623,6 +1637,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  	mutex_unlock(&power_domains->lock);
>  
> +end:
>  	intel_runtime_pm_put(dev_priv);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/16] drm/i915: Power down any power well left on by BIOS

[José Roberto de Souza]

Just not enable power wells is not enough as BIOS/firmware can turn
on some power wells during boot, so is needed disable those to save
power and to avoid mismatch state errors in
intel_power_domains_verify_state().
So here disabling every non-real power well first as it could have
some dependency in a real power well and then disabling all power
wells in reverse(power well 2 depends on power well 1 and so on)
other as required by spec.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 59 +++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 56c65d921acd..0f5016b74228 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3785,6 +3785,61 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
 
 static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
 
+static void
+intel_power_domains_disable_leftovers(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
+	int i;
+
+	mutex_lock(&power_domains->lock);
+
+	/* Disable everything that is enabled and is not a HW power_well */
+	for_each_power_well(dev_priv, power_well) {
+		WARN_ON(power_well->count);
+
+		/*
+		 * Power wells not belonging to any domain (like the MISC_IO
+		 * and PW1 power wells) are under FW control, so ignore them,
+		 * since their state can change asynchronously.
+		 */
+		if (!power_well->desc->domains || power_well->desc->always_on)
+			continue;
+
+		if (power_well->desc->id != DISP_PW_ID_NONE)
+			continue;
+
+		if (!power_well->hw_enabled)
+			continue;
+
+		intel_power_well_disable(dev_priv, power_well);
+	}
+
+	/* Disabled HW power wells in reverse order, so power well 2 is
+	 * disabled before power well 1 and so on as required by spec.
+	 */
+	for (i = power_domains->power_well_count - 1; i >= 0; i--) {
+		power_well = &power_domains->power_wells[i];
+
+		WARN_ON(power_well->count);
+
+		if (!power_well->desc->domains || power_well->desc->always_on)
+			continue;
+
+		if (power_well->desc->id == DISP_PW_ID_NONE)
+			continue;
+
+		if (!power_well->hw_enabled)
+			continue;
+
+		intel_power_well_disable(dev_priv, power_well);
+	}
+
+	mutex_unlock(&power_domains->lock);
+
+	intel_power_domains_verify_state(dev_priv);
+}
+
 /**
  * intel_power_domains_init_hw - initialize hardware power domain state
  * @dev_priv: i915 device instance
@@ -3838,6 +3893,10 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	intel_power_domains_sync_hw(dev_priv);
 
+	/* Disable everything left enabled by BIOS/firmware */
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		intel_power_domains_disable_leftovers(dev_priv);
+
 	power_domains->initializing = false;
 }
 
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/16] drm/i915: Power down any power well left on by BIOS

[Imre Deak]

On Fri, Oct 12, 2018 at 02:52:17PM -0700, José Roberto de Souza wrote:
> Just not enable power wells is not enough as BIOS/firmware can turn
> on some power wells during boot, so is needed disable those to save
> power and to avoid mismatch state errors in
> intel_power_domains_verify_state().
> So here disabling every non-real power well first as it could have
> some dependency in a real power well and then disabling all power
> wells in reverse(power well 2 depends on power well 1 and so on)
> other as required by spec.

You can't disable a power well while the function depending on it is
still on, that would lead to a hang. Also some power wells can only be
enabled/disabled at a specific place in the modeset sequence, so
disabling them here would lead to timeouts. The correct way to get power
wells disbled is to disable the corresponding functions by doing a
modeset disabling any active outputs.

> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 59 +++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 56c65d921acd..0f5016b74228 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -3785,6 +3785,61 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>  
>  static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
>  
> +static void
> +intel_power_domains_disable_leftovers(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
> +	int i;
> +
> +	mutex_lock(&power_domains->lock);
> +
> +	/* Disable everything that is enabled and is not a HW power_well */
> +	for_each_power_well(dev_priv, power_well) {
> +		WARN_ON(power_well->count);
> +
> +		/*
> +		 * Power wells not belonging to any domain (like the MISC_IO
> +		 * and PW1 power wells) are under FW control, so ignore them,
> +		 * since their state can change asynchronously.
> +		 */
> +		if (!power_well->desc->domains || power_well->desc->always_on)
> +			continue;
> +
> +		if (power_well->desc->id != DISP_PW_ID_NONE)
> +			continue;
> +
> +		if (!power_well->hw_enabled)
> +			continue;
> +
> +		intel_power_well_disable(dev_priv, power_well);
> +	}
> +
> +	/* Disabled HW power wells in reverse order, so power well 2 is
> +	 * disabled before power well 1 and so on as required by spec.
> +	 */
> +	for (i = power_domains->power_well_count - 1; i >= 0; i--) {
> +		power_well = &power_domains->power_wells[i];
> +
> +		WARN_ON(power_well->count);
> +
> +		if (!power_well->desc->domains || power_well->desc->always_on)
> +			continue;
> +
> +		if (power_well->desc->id == DISP_PW_ID_NONE)
> +			continue;
> +
> +		if (!power_well->hw_enabled)
> +			continue;
> +
> +		intel_power_well_disable(dev_priv, power_well);
> +	}
> +
> +	mutex_unlock(&power_domains->lock);
> +
> +	intel_power_domains_verify_state(dev_priv);
> +}
> +
>  /**
>   * intel_power_domains_init_hw - initialize hardware power domain state
>   * @dev_priv: i915 device instance
> @@ -3838,6 +3893,10 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  	intel_power_domains_sync_hw(dev_priv);
>  
> +	/* Disable everything left enabled by BIOS/firmware */
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		intel_power_domains_disable_leftovers(dev_priv);
> +
>  	power_domains->initializing = false;
>  }
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/16] drm/i915: Power down any power well left on by BIOS

[Souza, Jose]

On Mon, 2018-10-15 at 14:06 +0300, Imre Deak wrote:
> On Fri, Oct 12, 2018 at 02:52:17PM -0700, José Roberto de Souza
> wrote:
> > Just not enable power wells is not enough as BIOS/firmware can turn
> > on some power wells during boot, so is needed disable those to save
> > power and to avoid mismatch state errors in
> > intel_power_domains_verify_state().
> > So here disabling every non-real power well first as it could have
> > some dependency in a real power well and then disabling all power
> > wells in reverse(power well 2 depends on power well 1 and so on)
> > other as required by spec.
> 
> You can't disable a power well while the function depending on it is
> still on, that would lead to a hang. Also some power wells can only
> be
> enabled/disabled at a specific place in the modeset sequence, so
> disabling them here would lead to timeouts. The correct way to get
> power
> wells disbled is to disable the corresponding functions by doing a
> modeset disabling any active outputs.

Do a modeset disabling would require initialize some display stuff and
that is what this PR is avoiding when display is disabled by parameter.
Also I did not reproduced any hang and CI did not too as somes IGT
tests load i915 with display off.

> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 59
> > +++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 56c65d921acd..0f5016b74228 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -3785,6 +3785,61 @@ static void vlv_cmnlane_wa(struct
> > drm_i915_private *dev_priv)
> >  
> >  static void intel_power_domains_verify_state(struct
> > drm_i915_private *dev_priv);
> >  
> > +static void
> > +intel_power_domains_disable_leftovers(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	struct i915_power_domains *power_domains = &dev_priv-
> > >power_domains;
> > +	struct i915_power_well *power_well;
> > +	int i;
> > +
> > +	mutex_lock(&power_domains->lock);
> > +
> > +	/* Disable everything that is enabled and is not a HW
> > power_well */
> > +	for_each_power_well(dev_priv, power_well) {
> > +		WARN_ON(power_well->count);
> > +
> > +		/*
> > +		 * Power wells not belonging to any domain (like the
> > MISC_IO
> > +		 * and PW1 power wells) are under FW control, so ignore
> > them,
> > +		 * since their state can change asynchronously.
> > +		 */
> > +		if (!power_well->desc->domains || power_well->desc-
> > >always_on)
> > +			continue;
> > +
> > +		if (power_well->desc->id != DISP_PW_ID_NONE)
> > +			continue;
> > +
> > +		if (!power_well->hw_enabled)
> > +			continue;
> > +
> > +		intel_power_well_disable(dev_priv, power_well);
> > +	}
> > +
> > +	/* Disabled HW power wells in reverse order, so power well 2 is
> > +	 * disabled before power well 1 and so on as required by spec.
> > +	 */
> > +	for (i = power_domains->power_well_count - 1; i >= 0; i--) {
> > +		power_well = &power_domains->power_wells[i];
> > +
> > +		WARN_ON(power_well->count);
> > +
> > +		if (!power_well->desc->domains || power_well->desc-
> > >always_on)
> > +			continue;
> > +
> > +		if (power_well->desc->id == DISP_PW_ID_NONE)
> > +			continue;
> > +
> > +		if (!power_well->hw_enabled)
> > +			continue;
> > +
> > +		intel_power_well_disable(dev_priv, power_well);
> > +	}
> > +
> > +	mutex_unlock(&power_domains->lock);
> > +
> > +	intel_power_domains_verify_state(dev_priv);
> > +}
> > +
> >  /**
> >   * intel_power_domains_init_hw - initialize hardware power domain
> > state
> >   * @dev_priv: i915 device instance
> > @@ -3838,6 +3893,10 @@ void intel_power_domains_init_hw(struct
> > drm_i915_private *dev_priv, bool resume)
> >  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> >  	intel_power_domains_sync_hw(dev_priv);
> >  
> > +	/* Disable everything left enabled by BIOS/firmware */
> > +	if (!INTEL_INFO(dev_priv)->num_pipes)
> > +		intel_power_domains_disable_leftovers(dev_priv);
> > +
> >  	power_domains->initializing = false;
> >  }
> >  
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/16] drm/i915: Power down any power well left on by BIOS

[Imre Deak]

On Tue, Oct 16, 2018 at 03:05:35AM +0300, Souza, Jose wrote:
> On Mon, 2018-10-15 at 14:06 +0300, Imre Deak wrote:
> > On Fri, Oct 12, 2018 at 02:52:17PM -0700, José Roberto de Souza
> > wrote:
> > > Just not enable power wells is not enough as BIOS/firmware can turn
> > > on some power wells during boot, so is needed disable those to save
> > > power and to avoid mismatch state errors in
> > > intel_power_domains_verify_state().
> > > So here disabling every non-real power well first as it could have
> > > some dependency in a real power well and then disabling all power
> > > wells in reverse(power well 2 depends on power well 1 and so on)
> > > other as required by spec.
> > 
> > You can't disable a power well while the function depending on it is
> > still on, that would lead to a hang. Also some power wells can only
> > be
> > enabled/disabled at a specific place in the modeset sequence, so
> > disabling them here would lead to timeouts. The correct way to get
> > power
> > wells disbled is to disable the corresponding functions by doing a
> > modeset disabling any active outputs.
> 
> Do a modeset disabling would require initialize some display stuff and
> that is what this PR is avoiding when display is disabled by parameter.

Right, but it's not the good approach to add support for the
.disable_display parameter. A proper modeset disable sequence has to be
run, we can't simply disable power while dependent functionality is
still active as I wrote. Chris has posted a patch to do such a disable
sequence we would need something along that line.

> Also I did not reproduced any hang and CI did not too as somes IGT
> tests load i915 with display off.
> 
> > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 59
> > > +++++++++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 56c65d921acd..0f5016b74228 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -3785,6 +3785,61 @@ static void vlv_cmnlane_wa(struct
> > > drm_i915_private *dev_priv)
> > >  
> > >  static void intel_power_domains_verify_state(struct
> > > drm_i915_private *dev_priv);
> > >  
> > > +static void
> > > +intel_power_domains_disable_leftovers(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +	struct i915_power_domains *power_domains = &dev_priv-
> > > >power_domains;
> > > +	struct i915_power_well *power_well;
> > > +	int i;
> > > +
> > > +	mutex_lock(&power_domains->lock);
> > > +
> > > +	/* Disable everything that is enabled and is not a HW
> > > power_well */
> > > +	for_each_power_well(dev_priv, power_well) {
> > > +		WARN_ON(power_well->count);
> > > +
> > > +		/*
> > > +		 * Power wells not belonging to any domain (like the
> > > MISC_IO
> > > +		 * and PW1 power wells) are under FW control, so ignore
> > > them,
> > > +		 * since their state can change asynchronously.
> > > +		 */
> > > +		if (!power_well->desc->domains || power_well->desc-
> > > >always_on)
> > > +			continue;
> > > +
> > > +		if (power_well->desc->id != DISP_PW_ID_NONE)
> > > +			continue;
> > > +
> > > +		if (!power_well->hw_enabled)
> > > +			continue;
> > > +
> > > +		intel_power_well_disable(dev_priv, power_well);
> > > +	}
> > > +
> > > +	/* Disabled HW power wells in reverse order, so power well 2 is
> > > +	 * disabled before power well 1 and so on as required by spec.
> > > +	 */
> > > +	for (i = power_domains->power_well_count - 1; i >= 0; i--) {
> > > +		power_well = &power_domains->power_wells[i];
> > > +
> > > +		WARN_ON(power_well->count);
> > > +
> > > +		if (!power_well->desc->domains || power_well->desc-
> > > >always_on)
> > > +			continue;
> > > +
> > > +		if (power_well->desc->id == DISP_PW_ID_NONE)
> > > +			continue;
> > > +
> > > +		if (!power_well->hw_enabled)
> > > +			continue;
> > > +
> > > +		intel_power_well_disable(dev_priv, power_well);
> > > +	}
> > > +
> > > +	mutex_unlock(&power_domains->lock);
> > > +
> > > +	intel_power_domains_verify_state(dev_priv);
> > > +}
> > > +
> > >  /**
> > >   * intel_power_domains_init_hw - initialize hardware power domain
> > > state
> > >   * @dev_priv: i915 device instance
> > > @@ -3838,6 +3893,10 @@ void intel_power_domains_init_hw(struct
> > > drm_i915_private *dev_priv, bool resume)
> > >  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > >  	intel_power_domains_sync_hw(dev_priv);
> > >  
> > > +	/* Disable everything left enabled by BIOS/firmware */
> > > +	if (!INTEL_INFO(dev_priv)->num_pipes)
> > > +		intel_power_domains_disable_leftovers(dev_priv);
> > > +
> > >  	power_domains->initializing = false;
> > >  }
> > >  
> > > -- 
> > > 2.19.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 16/16] drm/i915: Guard debugfs against invalid access when display is disabled

[José Roberto de Souza]

Without this checks in this debugfs, it would try access memory and
resorces from display causing the driver to crash.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 00c551d3e409..7864bf233d99 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1636,6 +1636,9 @@ static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	seq_printf(m, "FB tracking busy bits: 0x%08x\n",
 		   dev_priv->fb_tracking.busy_bits);
 
@@ -1653,6 +1656,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	if (!HAS_FBC(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	intel_runtime_pm_get(dev_priv);
 	mutex_lock(&fbc->lock);
 
@@ -1729,6 +1735,9 @@ static int i915_ips_status(struct seq_file *m, void *unused)
 	if (!HAS_IPS(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	intel_runtime_pm_get(dev_priv);
 
 	seq_printf(m, "Enabled by kernel parameter: %s\n",
@@ -1753,6 +1762,9 @@ static int i915_sr_status(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	bool sr_enabled = false;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	intel_runtime_pm_get(dev_priv);
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
@@ -1891,6 +1903,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	struct drm_framebuffer *drm_fb;
 	int ret;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
@@ -2777,6 +2792,9 @@ i915_edp_psr_debug_set(void *data, u64 val)
 	if (!CAN_PSR(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
 
 	intel_runtime_pm_get(dev_priv);
@@ -3222,6 +3240,9 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	intel_runtime_pm_get(dev_priv);
 	seq_printf(m, "CRTC info\n");
 	seq_printf(m, "---------\n");
@@ -3326,6 +3347,9 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 	struct drm_device *dev = &dev_priv->drm;
 	int i;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_modeset_lock_all(dev);
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
@@ -3398,6 +3422,9 @@ static int i915_ipc_status_open(struct inode *inode, struct file *file)
 	if (!HAS_IPC(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, i915_ipc_status_show, dev_priv);
 }
 
@@ -3445,6 +3472,9 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	if (INTEL_GEN(dev_priv) < 9)
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_modeset_lock_all(dev);
 
 	ddb = &dev_priv->wm.skl_hw.ddb;
@@ -3553,6 +3583,9 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
 	struct intel_crtc *intel_crtc;
 	int active_crtc_cnt = 0;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_modeset_lock_all(dev);
 	for_each_intel_crtc(dev, intel_crtc) {
 		if (intel_crtc->base.state->active) {
@@ -3579,6 +3612,9 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
@@ -3697,6 +3733,11 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 static int i915_displayport_test_active_open(struct inode *inode,
 					     struct file *file)
 {
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, i915_displayport_test_active_show,
 			   inode->i_private);
 }
@@ -3718,6 +3759,9 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		struct intel_encoder *encoder;
@@ -3762,6 +3806,9 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		struct intel_encoder *encoder;
@@ -3878,6 +3925,9 @@ static int pri_wm_latency_open(struct inode *inode, struct file *file)
 	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, pri_wm_latency_show, dev_priv);
 }
 
@@ -3888,6 +3938,9 @@ static int spr_wm_latency_open(struct inode *inode, struct file *file)
 	if (HAS_GMCH_DISPLAY(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, spr_wm_latency_show, dev_priv);
 }
 
@@ -3898,6 +3951,9 @@ static int cur_wm_latency_open(struct inode *inode, struct file *file)
 	if (HAS_GMCH_DISPLAY(dev_priv))
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, cur_wm_latency_show, dev_priv);
 }
 
@@ -4645,6 +4701,11 @@ static ssize_t i915_hpd_storm_ctl_write(struct file *file,
 
 static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
 {
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	return single_open(file, i915_hpd_storm_ctl_show, inode->i_private);
 }
 
@@ -4668,6 +4729,9 @@ static int i915_drrs_ctl_set(void *data, u64 val)
 	if (INTEL_GEN(dev_priv) < 7)
 		return -ENODEV;
 
+	if (!INTEL_INFO(dev_priv)->num_pipes)
+		return -ENODEV;
+
 	drm_modeset_lock_all(dev);
 	for_each_intel_crtc(dev, intel_crtc) {
 		if (!intel_crtc->base.state->active ||
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 16/16] drm/i915: Guard debugfs against invalid access when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> Without this checks in this debugfs, it would try access memory and
> resorces from display causing the driver to crash.

Hum, need to consider this. It's an option to not register the display
debugfs files at all in this case. Might be much neater.

BR,
Jani.


>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 64 +++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 00c551d3e409..7864bf233d99 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1636,6 +1636,9 @@ static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	seq_printf(m, "FB tracking busy bits: 0x%08x\n",
>  		   dev_priv->fb_tracking.busy_bits);
>  
> @@ -1653,6 +1656,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  	if (!HAS_FBC(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	intel_runtime_pm_get(dev_priv);
>  	mutex_lock(&fbc->lock);
>  
> @@ -1729,6 +1735,9 @@ static int i915_ips_status(struct seq_file *m, void *unused)
>  	if (!HAS_IPS(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	intel_runtime_pm_get(dev_priv);
>  
>  	seq_printf(m, "Enabled by kernel parameter: %s\n",
> @@ -1753,6 +1762,9 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	bool sr_enabled = false;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	intel_runtime_pm_get(dev_priv);
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  
> @@ -1891,6 +1903,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
>  	struct drm_framebuffer *drm_fb;
>  	int ret;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);
>  	if (ret)
>  		return ret;
> @@ -2777,6 +2792,9 @@ i915_edp_psr_debug_set(void *data, u64 val)
>  	if (!CAN_PSR(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>  
>  	intel_runtime_pm_get(dev_priv);
> @@ -3222,6 +3240,9 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	intel_runtime_pm_get(dev_priv);
>  	seq_printf(m, "CRTC info\n");
>  	seq_printf(m, "---------\n");
> @@ -3326,6 +3347,9 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  	struct drm_device *dev = &dev_priv->drm;
>  	int i;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_modeset_lock_all(dev);
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> @@ -3398,6 +3422,9 @@ static int i915_ipc_status_open(struct inode *inode, struct file *file)
>  	if (!HAS_IPC(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, i915_ipc_status_show, dev_priv);
>  }
>  
> @@ -3445,6 +3472,9 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>  	if (INTEL_GEN(dev_priv) < 9)
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_modeset_lock_all(dev);
>  
>  	ddb = &dev_priv->wm.skl_hw.ddb;
> @@ -3553,6 +3583,9 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
>  	struct intel_crtc *intel_crtc;
>  	int active_crtc_cnt = 0;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_modeset_lock_all(dev);
>  	for_each_intel_crtc(dev, intel_crtc) {
>  		if (intel_crtc->base.state->active) {
> @@ -3579,6 +3612,9 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> @@ -3697,6 +3733,11 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
>  static int i915_displayport_test_active_open(struct inode *inode,
>  					     struct file *file)
>  {
> +	struct drm_i915_private *dev_priv = inode->i_private;
> +
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, i915_displayport_test_active_show,
>  			   inode->i_private);
>  }
> @@ -3718,6 +3759,9 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct intel_encoder *encoder;
> @@ -3762,6 +3806,9 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
>  	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct intel_encoder *encoder;
> @@ -3878,6 +3925,9 @@ static int pri_wm_latency_open(struct inode *inode, struct file *file)
>  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, pri_wm_latency_show, dev_priv);
>  }
>  
> @@ -3888,6 +3938,9 @@ static int spr_wm_latency_open(struct inode *inode, struct file *file)
>  	if (HAS_GMCH_DISPLAY(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, spr_wm_latency_show, dev_priv);
>  }
>  
> @@ -3898,6 +3951,9 @@ static int cur_wm_latency_open(struct inode *inode, struct file *file)
>  	if (HAS_GMCH_DISPLAY(dev_priv))
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, cur_wm_latency_show, dev_priv);
>  }
>  
> @@ -4645,6 +4701,11 @@ static ssize_t i915_hpd_storm_ctl_write(struct file *file,
>  
>  static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
>  {
> +	struct drm_i915_private *dev_priv = inode->i_private;
> +
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	return single_open(file, i915_hpd_storm_ctl_show, inode->i_private);
>  }
>  
> @@ -4668,6 +4729,9 @@ static int i915_drrs_ctl_set(void *data, u64 val)
>  	if (INTEL_GEN(dev_priv) < 7)
>  		return -ENODEV;
>  
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		return -ENODEV;
> +
>  	drm_modeset_lock_all(dev);
>  	for_each_intel_crtc(dev, intel_crtc) {
>  		if (!intel_crtc->base.state->active ||

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Patchwork]

== Series Details ==

Series: series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled
URL   : https://patchwork.freedesktop.org/series/50962/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
06503ffcb9ca drm/i915: Properly set PCH as NOP when display is disabled
7f581040faab drm/i915: Move out non-display related calls from display/modeset init/cleanup
f4769b4e9130 drm/i915: Move drm_vblank_init() to i915_load_modeset_init()
8595e42148c9 drm/i915: Move FBC init and cleanup calls to modeset functions
5ff98abcaa8c drm/i915: Move intel_init_ipc() call to i915_load_modeset_init()
f74ca2ff2e32 drm/i915: Don't call modeset related functions when display is disabled
-:344: CHECK:CAMELCASE: Avoid CamelCase: <saveGCDGMBUS>
#344: FILE: drivers/gpu/drm/i915/i915_suspend.c:71:
+					     &dev_priv->regfile.saveGCDGMBUS);

total: 0 errors, 0 warnings, 1 checks, 374 lines checked
b02ae2fa6025 drm/i915: Remove redundant checks for num_pipes == 0
613ebe28e648 drm/i915: Keep overlay functions naming consistent
053d9534ebf3 drm/i915: Do not reset display when display is disabled
-:9: WARNING:TYPO_SPELLING: 'reseting' may be misspelled - perhaps 'resetting'?
#9: 
Display is always disabled and enabled when reseting any engine,

total: 0 errors, 1 warnings, 0 checks, 18 lines checked
116b8b16f761 drm/i915: Do not initialize display clocks when display is disabled
3c2b78c04b1d drm/i915: Do not initialize display core when display is disabled
95269b9f7664 drm/i915: Warn when display irq functions is executed when display is disabled
5bef5210349f drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded
6a298a5e61a8 drm/i915: Do not turn power wells on or off when display is disabled
0f888054ac79 drm/i915: Power down any power well left on by BIOS
c27f20cef091 drm/i915: Guard debugfs against invalid access when display is disabled

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Patchwork]

== Series Details ==

Series: series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled
URL   : https://patchwork.freedesktop.org/series/50962/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Properly set PCH as NOP when display is disabled
Okay!

Commit: drm/i915: Move out non-display related calls from display/modeset init/cleanup
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3725:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)

Commit: drm/i915: Move drm_vblank_init() to i915_load_modeset_init()
Okay!

Commit: drm/i915: Move FBC init and cleanup calls to modeset functions
Okay!

Commit: drm/i915: Move intel_init_ipc() call to i915_load_modeset_init()
Okay!

Commit: drm/i915: Don't call modeset related functions when display is disabled
Okay!

Commit: drm/i915: Remove redundant checks for num_pipes == 0
Okay!

Commit: drm/i915: Keep overlay functions naming consistent
Okay!

Commit: drm/i915: Do not reset display when display is disabled
Okay!

Commit: drm/i915: Do not initialize display clocks when display is disabled
Okay!

Commit: drm/i915: Do not initialize display core when display is disabled
Okay!

Commit: drm/i915: Warn when display irq functions is executed when display is disabled
Okay!

Commit: drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded
Okay!

Commit: drm/i915: Do not turn power wells on or off when display is disabled
Okay!

Commit: drm/i915: Power down any power well left on by BIOS
Okay!

Commit: drm/i915: Guard debugfs against invalid access when display is disabled
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Patchwork]

== Series Details ==

Series: series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled
URL   : https://patchwork.freedesktop.org/series/50962/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4978 -> Patchwork_10450 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10450 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10450, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50962/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10450:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rpm@module-reload:
      {fi-apl-guc}:       PASS -> SKIP
      fi-glk-dsi:         PASS -> SKIP
      fi-skl-6600u:       PASS -> SKIP
      fi-kbl-guc:         PASS -> SKIP
      fi-cfl-8109u:       PASS -> SKIP
      fi-cfl-s3:          PASS -> SKIP
      fi-kbl-7500u:       PASS -> SKIP
      fi-bxt-dsi:         PASS -> SKIP
      fi-skl-6700hq:      PASS -> SKIP
      fi-cfl-guc:         PASS -> SKIP
      fi-cnl-u:           PASS -> SKIP
      fi-skl-guc:         PASS -> SKIP
      fi-icl-u2:          PASS -> SKIP
      fi-cfl-8700k:       PASS -> SKIP
      fi-kbl-x1275:       PASS -> SKIP
      fi-skl-6770hq:      PASS -> SKIP
      fi-kbl-8809g:       PASS -> SKIP
      fi-kbl-r:           PASS -> SKIP
      fi-skl-6260u:       PASS -> SKIP
      fi-bxt-j4205:       PASS -> SKIP
      fi-whl-u:           PASS -> SKIP
      fi-kbl-7567u:       PASS -> SKIP
      fi-skl-iommu:       PASS -> SKIP
      fi-glk-j4005:       PASS -> SKIP
      fi-skl-6700k2:      PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@basic:
      fi-icl-u2:          SKIP -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-icl-u:           NOTRUN -> INCOMPLETE (fdo#107713)

    igt@pm_rpm@module-reload:
      fi-bdw-5557u:       PASS -> DMESG-FAIL (fdo#107603)
      fi-hsw-4770:        PASS -> DMESG-FAIL (fdo#107603)
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#107603)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS +1

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

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107603 https://bugs.freedesktop.org/show_bug.cgi?id=107603
  fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (44 -> 41) ==

  Additional (1): fi-icl-u 
  Missing    (4): fi-hsw-4770r fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


== Build changes ==

    * Linux: CI_DRM_4978 -> Patchwork_10450

  CI_DRM_4978: ca98b2681a49a1417f8157af2d94a4f2d0bd0e47 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4674: 93871c6fb3c25e5d350c9faf36ded917174214de @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10450: c27f20cef091a7610bc97d61e69a396d46a86b2d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c27f20cef091 drm/i915: Guard debugfs against invalid access when display is disabled
0f888054ac79 drm/i915: Power down any power well left on by BIOS
6a298a5e61a8 drm/i915: Do not turn power wells on or off when display is disabled
5bef5210349f drm/i915: Do not print DC off mismatch state when DMC firmware in not loaded
95269b9f7664 drm/i915: Warn when display irq functions is executed when display is disabled
3c2b78c04b1d drm/i915: Do not initialize display core when display is disabled
116b8b16f761 drm/i915: Do not initialize display clocks when display is disabled
053d9534ebf3 drm/i915: Do not reset display when display is disabled
613ebe28e648 drm/i915: Keep overlay functions naming consistent
b02ae2fa6025 drm/i915: Remove redundant checks for num_pipes == 0
f74ca2ff2e32 drm/i915: Don't call modeset related functions when display is disabled
5ff98abcaa8c drm/i915: Move intel_init_ipc() call to i915_load_modeset_init()
8595e42148c9 drm/i915: Move FBC init and cleanup calls to modeset functions
f4769b4e9130 drm/i915: Move drm_vblank_init() to i915_load_modeset_init()
7f581040faab drm/i915: Move out non-display related calls from display/modeset init/cleanup
06503ffcb9ca drm/i915: Properly set PCH as NOP when display is disabled

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10450/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Patchwork]

== Series Details ==

Series: series starting with [01/16] drm/i915: Properly set PCH as NOP when display is disabled
URL   : https://patchwork.freedesktop.org/series/50962/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4978_full -> Patchwork_10450_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10450_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10450_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10450_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt:
      shard-kbl:          PASS -> SKIP +40

    igt@kms_frontbuffer_tracking@fbc-tilingchange:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@sysfs-reader:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105079, fdo#103558, fdo#105602)

    igt@gem_linear_blits@normal:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-skl:          NOTRUN -> TIMEOUT (fdo#108039)

    igt@kms_atomic@plane_primary_legacy:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +2

    igt@kms_color@pipe-b-legacy-gamma:
      shard-apl:          PASS -> FAIL (fdo#104782)

    igt@kms_cursor_crc@cursor-256x256-dpms:
      shard-glk:          PASS -> FAIL (fdo#103232) +3

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          PASS -> FAIL (fdo#103232, fdo#103191)

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-ytiled:
      shard-skl:          NOTRUN -> FAIL (fdo#103184)

    igt@kms_fbcon_fbt@fbc:
      shard-skl:          NOTRUN -> FAIL (fdo#105682, fdo#103833)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-skl:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-skl:          NOTRUN -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-glk:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-render:
      shard-skl:          NOTRUN -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#105959, fdo#107773, fdo#104108)

    igt@kms_frontbuffer_tracking@fbcpsr-1p-indfb-fliptrack:
      shard-skl:          NOTRUN -> FAIL (fdo#105682)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-skl:          PASS -> INCOMPLETE (fdo#107773, fdo#104108)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105079, fdo#103558)

    igt@kms_plane@plane-position-covered-pipe-b-planes:
      shard-glk:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

    igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@gem_softpin@noreloc-s3:
      shard-skl:          INCOMPLETE (fdo#107773, fdo#104108) -> PASS

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-skl:          DMESG-WARN (fdo#107956) -> PASS +1

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-kbl:          DMESG-WARN (fdo#107956) -> SKIP

    igt@kms_cursor_crc@cursor-128x42-sliding:
      shard-kbl:          FAIL (fdo#103232) -> SKIP

    igt@kms_cursor_crc@cursor-256x85-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +6

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#105454, fdo#106509) -> PASS

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
      shard-apl:          FAIL (fdo#103167) -> PASS +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-kbl:          FAIL (fdo#103167) -> SKIP

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          FAIL (fdo#103167) -> PASS +2

    igt@kms_frontbuffer_tracking@fbc-stridechange:
      shard-skl:          FAIL (fdo#105683) -> PASS

    igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-render:
      shard-skl:          FAIL (fdo#105682) -> PASS +1

    igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-gtt:
      shard-skl:          FAIL (fdo#103167) -> PASS

    igt@kms_frontbuffer_tracking@psr-rgb565-draw-render:
      shard-skl:          FAIL (fdo#103167) -> SKIP

    igt@kms_plane@pixel-format-pipe-c-planes:
      shard-kbl:          FAIL (fdo#103166) -> SKIP

    igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
      shard-kbl:          FAIL (fdo#108145) -> SKIP +1

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-apl:          FAIL (fdo#103166) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@pm_rpm@modeset-non-lpsp:
      shard-skl:          INCOMPLETE (fdo#107807) -> SKIP

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103833 https://bugs.freedesktop.org/show_bug.cgi?id=103833
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105079 https://bugs.freedesktop.org/show_bug.cgi?id=105079
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105683 https://bugs.freedesktop.org/show_bug.cgi?id=105683
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#105959 https://bugs.freedesktop.org/show_bug.cgi?id=105959
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4978 -> Patchwork_10450

  CI_DRM_4978: ca98b2681a49a1417f8157af2d94a4f2d0bd0e47 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4674: 93871c6fb3c25e5d350c9faf36ded917174214de @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10450: c27f20cef091a7610bc97d61e69a396d46a86b2d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10450/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Jani Nikula]

On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> num_pipes is set to 0 if disable_display is set inside
> intel_device_info_runtime_init() but when that happen PCH will
> already be set in intel_detect_pch().
>
> i915_driver_load()
> 	i915_driver_init_early()
> 		...
> 		intel_detect_pch()
> 		...
> 	...
> 	i915_driver_init_hw()
> 		intel_device_info_runtime_init()
>
> So now setting num_pipes = 0 earlier to avoid this problem.

I'm growing this nagging feeling that this may be the wrong direction.

It's just that disabling an existing display and not having display are
two very different things. Even if our goal here is use the former to
test the latter, I think it's more subtle than this. You already saw
some of this in patch 15 of this series. You can't just pretend there's
no display if you also want to gain benefits.

I'm also not sure if we need to go as far as Chris is suggesting in [1],
but assuming disable display means num_pipes = 0 from start prevents us
from doing that later on, because we'll start making assumptions. Like
you already do in this series.

Again, there's a bunch of useful stuff in the series that could be
merged with a different ordering. Including patches 3-5 that look
superficially good but do need an in-depth review. The init sequences
are hard, and we should add more asserts and comments on the ordering,
because we tend to forget why things are the way they are.


BR,
Jani.

[1] https://patchwork.freedesktop.org/series/51000/

>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c          | 5 +++++
>  drivers/gpu/drm/i915/intel_device_info.c | 8 ++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index baac35f698f9..e3efc3dd8a30 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1649,6 +1649,11 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	memcpy(device_info, match_info, sizeof(*device_info));
>  	device_info->device_id = pdev->device;
>  
> +	if (i915_modparams.disable_display) {
> +		DRM_INFO("Display disabled (module parameter)\n");
> +		device_info->num_pipes = 0;
> +	}
> +
>  	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>  		     BITS_PER_TYPE(device_info->platform_mask));
>  	BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 03df4e33763d..69be3f211737 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -775,12 +775,8 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
>  			info->num_sprites[pipe] = 1;
>  	}
>  
> -	if (i915_modparams.disable_display) {
> -		DRM_INFO("Display disabled (module parameter)\n");
> -		info->num_pipes = 0;
> -	} else if (info->num_pipes > 0 &&
> -		   (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> -		   HAS_PCH_SPLIT(dev_priv)) {
> +	if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> +	    HAS_PCH_SPLIT(dev_priv)) {
>  		u32 fuse_strap = I915_READ(FUSE_STRAP);
>  		u32 sfuse_strap = I915_READ(SFUSE_STRAP);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Lucas De Marchi]

On Sat, Nov 03, 2018 at 11:41:10PM +0200, Jani Nikula wrote:
> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> > num_pipes is set to 0 if disable_display is set inside
> > intel_device_info_runtime_init() but when that happen PCH will
> > already be set in intel_detect_pch().
> >
> > i915_driver_load()
> > 	i915_driver_init_early()
> > 		...
> > 		intel_detect_pch()
> > 		...
> > 	...
> > 	i915_driver_init_hw()
> > 		intel_device_info_runtime_init()
> >
> > So now setting num_pipes = 0 earlier to avoid this problem.
> 
> I'm growing this nagging feeling that this may be the wrong direction.
> 
> It's just that disabling an existing display and not having display are
> two very different things. Even if our goal here is use the former to
> test the latter, I think it's more subtle than this. You already saw
> some of this in patch 15 of this series. You can't just pretend there's
> no display if you also want to gain benefits.

I'm tending to agree with this. If you want to *disable* display you
will need to do something with the HW because it's there in an unknown
state. If you don't have the HW, you can't do that because you will not
have the registers you need.

> I'm also not sure if we need to go as far as Chris is suggesting in [1],
> but assuming disable display means num_pipes = 0 from start prevents us

Could we have both approaches? I think they differ mainly on the
initialization part, but it is useful to just shutdown the display when
we don't need it.

Lucas De Marchi

> from doing that later on, because we'll start making assumptions. Like
> you already do in this series.
> 
> Again, there's a bunch of useful stuff in the series that could be
> merged with a different ordering. Including patches 3-5 that look
> superficially good but do need an in-depth review. The init sequences
> are hard, and we should add more asserts and comments on the ordering,
> because we tend to forget why things are the way they are.
> 
> 
> BR,
> Jani.
> 
> [1] https://patchwork.freedesktop.org/series/51000/
> 
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c          | 5 +++++
> >  drivers/gpu/drm/i915/intel_device_info.c | 8 ++------
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index baac35f698f9..e3efc3dd8a30 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1649,6 +1649,11 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	memcpy(device_info, match_info, sizeof(*device_info));
> >  	device_info->device_id = pdev->device;
> >  
> > +	if (i915_modparams.disable_display) {
> > +		DRM_INFO("Display disabled (module parameter)\n");
> > +		device_info->num_pipes = 0;
> > +	}
> > +
> >  	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >  		     BITS_PER_TYPE(device_info->platform_mask));
> >  	BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index 03df4e33763d..69be3f211737 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -775,12 +775,8 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
> >  			info->num_sprites[pipe] = 1;
> >  	}
> >  
> > -	if (i915_modparams.disable_display) {
> > -		DRM_INFO("Display disabled (module parameter)\n");
> > -		info->num_pipes = 0;
> > -	} else if (info->num_pipes > 0 &&
> > -		   (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> > -		   HAS_PCH_SPLIT(dev_priv)) {
> > +	if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> > +	    HAS_PCH_SPLIT(dev_priv)) {
> >  		u32 fuse_strap = I915_READ(FUSE_STRAP);
> >  		u32 sfuse_strap = I915_READ(SFUSE_STRAP);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx