drm/i915/display: split out intel_vga_client.[ch]

Submitted by Jani Nikula on Oct. 1, 2019, 1:43 p.m.

Details

Message ID 20191001134353.12028-1-jani.nikula@intel.com
State New
Headers show
Series "drm/i915/display: split out intel_vga_client.[ch]" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Oct. 1, 2019, 1:43 p.m.
Split out code related to vga client and vga switcheroo
register/unregister and state handling from i915_drv.c and
intel_display.c.

It's a bit difficult to draw the line how much to move to the new file
from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
and i915_resume_switcheroo() in place was cleanest.

No functional changes.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

It's also a bit fuzzy if this is a sensible split anyway. Could also
name it intel_vga and move these from intel_display.c there?

i915_vgacntrl_reg
i915_disable_vga
i915_redisable_vga
i915_redisable_vga_power_on
---
 drivers/gpu/drm/i915/Makefile                 |   3 +-
 drivers/gpu/drm/i915/display/intel_display.c  |  29 ----
 drivers/gpu/drm/i915/display/intel_display.h  |   1 -
 .../gpu/drm/i915/display/intel_vga_client.c   | 140 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_vga_client.h   |  14 ++
 drivers/gpu/drm/i915/i915_drv.c               |  94 +-----------
 drivers/gpu/drm/i915/i915_drv.h               |   3 +
 7 files changed, 166 insertions(+), 118 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.h

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e04463d85401..ca770463e01f 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -184,7 +184,8 @@  i915-y += \
 	display/intel_psr.o \
 	display/intel_quirks.o \
 	display/intel_sprite.o \
-	display/intel_tc.o
+	display/intel_tc.o \
+	display/intel_vga_client.o
 i915-$(CONFIG_ACPI) += \
 	display/intel_acpi.o \
 	display/intel_opregion.o
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f1328c08f4ad..6278d62bc87d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -17188,35 +17188,6 @@  void intel_modeset_driver_remove(struct drm_i915_private *i915)
 	intel_fbc_cleanup_cfb(i915);
 }
 
-/*
- * set vga decode state - true == enable VGA decode
- */
-int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool state)
-{
-	unsigned reg = INTEL_GEN(dev_priv) >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
-	u16 gmch_ctrl;
-
-	if (pci_read_config_word(dev_priv->bridge_dev, reg, &gmch_ctrl)) {
-		DRM_ERROR("failed to read control word\n");
-		return -EIO;
-	}
-
-	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !state)
-		return 0;
-
-	if (state)
-		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
-	else
-		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
-
-	if (pci_write_config_word(dev_priv->bridge_dev, reg, gmch_ctrl)) {
-		DRM_ERROR("failed to write control word\n");
-		return -EIO;
-	}
-
-	return 0;
-}
-
 #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
 
 struct intel_display_error_state {
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 4b9e18e5a263..a7b0d11a3316 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -579,7 +579,6 @@  void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
 void intel_modeset_init_hw(struct drm_i915_private *i915);
 int intel_modeset_init(struct drm_i915_private *i915);
 void intel_modeset_driver_remove(struct drm_i915_private *i915);
-int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool state);
 void intel_display_resume(struct drm_device *dev);
 void i915_redisable_vga(struct drm_i915_private *dev_priv);
 void i915_redisable_vga_power_on(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/display/intel_vga_client.c b/drivers/gpu/drm/i915/display/intel_vga_client.c
new file mode 100644
index 000000000000..04ef1443f40e
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_vga_client.c
@@ -0,0 +1,140 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/pci.h>
+#include <linux/vga_switcheroo.h>
+#include <linux/vgaarb.h>
+
+#include <drm/i915_drm.h>
+
+#include "i915_drv.h"
+#include "intel_acpi.h"
+#include "intel_vga_client.h"
+
+static int
+intel_vga_client_set_state(struct drm_i915_private *i915, bool enable_decode)
+{
+	unsigned int reg = INTEL_GEN(i915) >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
+	u16 gmch_ctrl;
+
+	if (pci_read_config_word(i915->bridge_dev, reg, &gmch_ctrl)) {
+		DRM_ERROR("failed to read control word\n");
+		return -EIO;
+	}
+
+	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !enable_decode)
+		return 0;
+
+	if (enable_decode)
+		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
+	else
+		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
+
+	if (pci_write_config_word(i915->bridge_dev, reg, gmch_ctrl)) {
+		DRM_ERROR("failed to write control word\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static unsigned int
+intel_vga_client_set_decode(void *cookie, bool enable_decode)
+{
+	struct drm_i915_private *i915 = cookie;
+
+	intel_vga_client_set_state(i915, enable_decode);
+
+	if (enable_decode)
+		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
+		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
+	else
+		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
+}
+
+static bool intel_vga_switcheroo_can_switch(struct pci_dev *pdev)
+{
+	struct drm_i915_private *i915 = pdev_to_i915(pdev);
+
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would
+	 * lead to locking inversion with the driver load path. And the access
+	 * here is completely racy anyway. So don't bother with locking for now.
+	 */
+	return i915 && i915->drm.open_count == 0;
+}
+
+static void intel_vga_switcheroo_set_state(struct pci_dev *pdev,
+					   enum vga_switcheroo_state state)
+{
+	struct drm_i915_private *i915 = pdev_to_i915(pdev);
+	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
+
+	if (!i915) {
+		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
+		return;
+	}
+
+	if (state == VGA_SWITCHEROO_ON) {
+		pr_info("switched on\n");
+		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
+		/* i915 resume handler doesn't set to D0 */
+		pci_set_power_state(pdev, PCI_D0);
+		i915_resume_switcheroo(i915);
+		i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
+	} else {
+		pr_info("switched off\n");
+		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
+		i915_suspend_switcheroo(i915, pmm);
+		i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;
+	}
+}
+
+static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
+	.set_gpu_state = intel_vga_switcheroo_set_state,
+	.reprobe = NULL,
+	.can_switch = intel_vga_switcheroo_can_switch,
+};
+
+int intel_vga_client_register(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = i915->drm.pdev;
+	int ret;
+
+	/*
+	 * If we have > 1 VGA cards, then we need to arbitrate access to the
+	 * common VGA resources.
+	 *
+	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
+	 * then we do not take part in VGA arbitration and the
+	 * vga_client_register() fails with -ENODEV.
+	 */
+	ret = vga_client_register(pdev, i915, NULL, intel_vga_client_set_decode);
+	if (ret && ret != -ENODEV)
+		goto out;
+
+	intel_register_dsm_handler();
+
+	ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
+	if (ret)
+		goto out_unregister;
+
+	return 0;
+
+out_unregister:
+	intel_unregister_dsm_handler();
+	vga_client_register(pdev, NULL, NULL, NULL);
+out:
+	return ret;
+}
+
+void intel_vga_client_unregister(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = i915->drm.pdev;
+
+	vga_switcheroo_unregister_client(pdev);
+	intel_unregister_dsm_handler();
+	vga_client_register(pdev, NULL, NULL, NULL);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_vga_client.h b/drivers/gpu/drm/i915/display/intel_vga_client.h
new file mode 100644
index 000000000000..334eb90a2e4b
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_vga_client.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __INTEL_VGA_CLIENT_H__
+#define __INTEL_VGA_CLIENT_H__
+
+struct drm_i915_private;
+
+int intel_vga_client_register(struct drm_i915_private *i915);
+void intel_vga_client_unregister(struct drm_i915_private *i915);
+
+#endif /* __INTEL_VGA_CLIENT_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 91aae56b4280..a36131b15a6f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -36,7 +36,6 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/pnp.h>
 #include <linux/slab.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <linux/vt.h>
 #include <acpi/video.h>
@@ -59,6 +58,7 @@ 
 #include "display/intel_overlay.h"
 #include "display/intel_pipe_crc.h"
 #include "display/intel_sprite.h"
+#include "display/intel_vga_client.h"
 
 #include "gem/i915_gem_context.h"
 #include "gem/i915_gem_ioctls.h"
@@ -269,69 +269,8 @@  intel_teardown_mchbar(struct drm_i915_private *dev_priv)
 		release_resource(&dev_priv->mch_res);
 }
 
-/* true = enable decode, false = disable decoder */
-static unsigned int i915_vga_set_decode(void *cookie, bool state)
-{
-	struct drm_i915_private *dev_priv = cookie;
-
-	intel_modeset_vga_set_state(dev_priv, state);
-	if (state)
-		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
-		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-	else
-		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-}
-
-static int i915_resume_switcheroo(struct drm_i915_private *i915);
-static int i915_suspend_switcheroo(struct drm_i915_private *i915,
-				   pm_message_t state);
-
-static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
-{
-	struct drm_i915_private *i915 = pdev_to_i915(pdev);
-	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
-
-	if (!i915) {
-		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
-		return;
-	}
-
-	if (state == VGA_SWITCHEROO_ON) {
-		pr_info("switched on\n");
-		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
-		/* i915 resume handler doesn't set to D0 */
-		pci_set_power_state(pdev, PCI_D0);
-		i915_resume_switcheroo(i915);
-		i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
-	} else {
-		pr_info("switched off\n");
-		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
-		i915_suspend_switcheroo(i915, pmm);
-		i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;
-	}
-}
-
-static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
-{
-	struct drm_i915_private *i915 = pdev_to_i915(pdev);
-
-	/*
-	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
-	 * locking inversion with the driver load path. And the access here is
-	 * completely racy anyway. So don't bother with locking for now.
-	 */
-	return i915 && i915->drm.open_count == 0;
-}
-
-static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
-	.set_gpu_state = i915_switcheroo_set_state,
-	.reprobe = NULL,
-	.can_switch = i915_switcheroo_can_switch,
-};
-
 static int i915_driver_modeset_probe(struct drm_i915_private *i915)
 {
-	struct pci_dev *pdev = i915->drm.pdev;
 	int ret;
 
 	if (i915_inject_probe_failure(i915))
@@ -346,22 +285,9 @@  static int i915_driver_modeset_probe(struct drm_i915_private *i915)
 
 	intel_bios_init(i915);
 
-	/* If we have > 1 VGA cards, then we need to arbitrate access
-	 * to the common VGA resources.
-	 *
-	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
-	 * then we do not take part in VGA arbitration and the
-	 * vga_client_register() fails with -ENODEV.
-	 */
-	ret = vga_client_register(pdev, i915, NULL, i915_vga_set_decode);
-	if (ret && ret != -ENODEV)
-		goto out;
-
-	intel_register_dsm_handler();
-
-	ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
+	ret = intel_vga_client_register(i915);
 	if (ret)
-		goto cleanup_vga_client;
+		goto out;
 
 	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
 	intel_update_rawclk(i915);
@@ -414,23 +340,18 @@  static int i915_driver_modeset_probe(struct drm_i915_private *i915)
 cleanup_csr:
 	intel_csr_ucode_fini(i915);
 	intel_power_domains_driver_remove(i915);
-	vga_switcheroo_unregister_client(pdev);
-cleanup_vga_client:
-	vga_client_register(pdev, NULL, NULL, NULL);
+	intel_vga_client_unregister(i915);
 out:
 	return ret;
 }
 
 static void i915_driver_modeset_remove(struct drm_i915_private *i915)
 {
-	struct pci_dev *pdev = i915->drm.pdev;
-
 	intel_modeset_driver_remove(i915);
 
 	intel_bios_driver_remove(i915);
 
-	vga_switcheroo_unregister_client(pdev);
-	vga_client_register(pdev, NULL, NULL, NULL);
+	intel_vga_client_unregister(i915);
 
 	intel_csr_ucode_fini(i915);
 }
@@ -1845,8 +1766,7 @@  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	return ret;
 }
 
-static int
-i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
+int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
 {
 	int error;
 
@@ -2014,7 +1934,7 @@  static int i915_drm_resume_early(struct drm_device *dev)
 	return ret;
 }
 
-static int i915_resume_switcheroo(struct drm_i915_private *i915)
+int i915_resume_switcheroo(struct drm_i915_private *i915)
 {
 	int ret;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 337d8306416a..1b64a88a54b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2223,6 +2223,9 @@  extern const struct dev_pm_ops i915_pm_ops;
 int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
 void i915_driver_remove(struct drm_i915_private *i915);
 
+int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state);
+int i915_resume_switcheroo(struct drm_i915_private *i915);
+
 void intel_engine_init_hangcheck(struct intel_engine_cs *engine);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 

Comments

On Tue, Oct 01, 2019 at 04:43:53PM +0300, Jani Nikula wrote:
> Split out code related to vga client and vga switcheroo
> register/unregister and state handling from i915_drv.c and
> intel_display.c.

The two things don't really have anything in common except both have
"vga" in the name, so not sure it makes sense to put them in the same
place. OTOH they are pretty small so probably not worth it to have two
files.

Also the vgaarb stuff is still broken but I guess no one really cares.

> 
> It's a bit difficult to draw the line how much to move to the new file
> from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
> and i915_resume_switcheroo() in place was cleanest.
> 
> No functional changes.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> It's also a bit fuzzy if this is a sensible split anyway. Could also
> name it intel_vga and move these from intel_display.c there?
> 
> i915_vgacntrl_reg
> i915_disable_vga
> i915_redisable_vga
> i915_redisable_vga_power_on

Considering that's the only reason we register with vgaarb it probably
makes sense to move them as well.

> ---
>  drivers/gpu/drm/i915/Makefile                 |   3 +-
>  drivers/gpu/drm/i915/display/intel_display.c  |  29 ----
>  drivers/gpu/drm/i915/display/intel_display.h  |   1 -
>  .../gpu/drm/i915/display/intel_vga_client.c   | 140 ++++++++++++++++++
>  .../gpu/drm/i915/display/intel_vga_client.h   |  14 ++
>  drivers/gpu/drm/i915/i915_drv.c               |  94 +-----------
>  drivers/gpu/drm/i915/i915_drv.h               |   3 +
>  7 files changed, 166 insertions(+), 118 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e04463d85401..ca770463e01f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -184,7 +184,8 @@ i915-y += \
>  	display/intel_psr.o \
>  	display/intel_quirks.o \
>  	display/intel_sprite.o \
> -	display/intel_tc.o
> +	display/intel_tc.o \
> +	display/intel_vga_client.o
>  i915-$(CONFIG_ACPI) += \
>  	display/intel_acpi.o \
>  	display/intel_opregion.o
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f1328c08f4ad..6278d62bc87d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -17188,35 +17188,6 @@ void intel_modeset_driver_remove(struct drm_i915_private *i915)
>  	intel_fbc_cleanup_cfb(i915);
>  }
>  
> -/*
> - * set vga decode state - true == enable VGA decode
> - */
> -int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool state)
> -{
> -	unsigned reg = INTEL_GEN(dev_priv) >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
> -	u16 gmch_ctrl;
> -
> -	if (pci_read_config_word(dev_priv->bridge_dev, reg, &gmch_ctrl)) {
> -		DRM_ERROR("failed to read control word\n");
> -		return -EIO;
> -	}
> -
> -	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !state)
> -		return 0;
> -
> -	if (state)
> -		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
> -	else
> -		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
> -
> -	if (pci_write_config_word(dev_priv->bridge_dev, reg, gmch_ctrl)) {
> -		DRM_ERROR("failed to write control word\n");
> -		return -EIO;
> -	}
> -
> -	return 0;
> -}
> -
>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>  
>  struct intel_display_error_state {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 4b9e18e5a263..a7b0d11a3316 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -579,7 +579,6 @@ void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
>  void intel_modeset_init_hw(struct drm_i915_private *i915);
>  int intel_modeset_init(struct drm_i915_private *i915);
>  void intel_modeset_driver_remove(struct drm_i915_private *i915);
> -int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool state);
>  void intel_display_resume(struct drm_device *dev);
>  void i915_redisable_vga(struct drm_i915_private *dev_priv);
>  void i915_redisable_vga_power_on(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/display/intel_vga_client.c b/drivers/gpu/drm/i915/display/intel_vga_client.c
> new file mode 100644
> index 000000000000..04ef1443f40e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_vga_client.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/vga_switcheroo.h>
> +#include <linux/vgaarb.h>
> +
> +#include <drm/i915_drm.h>
> +
> +#include "i915_drv.h"
> +#include "intel_acpi.h"
> +#include "intel_vga_client.h"
> +
> +static int
> +intel_vga_client_set_state(struct drm_i915_private *i915, bool enable_decode)
> +{
> +	unsigned int reg = INTEL_GEN(i915) >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
> +	u16 gmch_ctrl;
> +
> +	if (pci_read_config_word(i915->bridge_dev, reg, &gmch_ctrl)) {
> +		DRM_ERROR("failed to read control word\n");
> +		return -EIO;
> +	}
> +
> +	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !enable_decode)
> +		return 0;
> +
> +	if (enable_decode)
> +		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
> +	else
> +		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
> +
> +	if (pci_write_config_word(i915->bridge_dev, reg, gmch_ctrl)) {
> +		DRM_ERROR("failed to write control word\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int
> +intel_vga_client_set_decode(void *cookie, bool enable_decode)
> +{
> +	struct drm_i915_private *i915 = cookie;
> +
> +	intel_vga_client_set_state(i915, enable_decode);
> +
> +	if (enable_decode)
> +		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
> +		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> +	else
> +		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> +}
> +
> +static bool intel_vga_switcheroo_can_switch(struct pci_dev *pdev)
> +{
> +	struct drm_i915_private *i915 = pdev_to_i915(pdev);
> +
> +	/*
> +	 * FIXME: open_count is protected by drm_global_mutex but that would
> +	 * lead to locking inversion with the driver load path. And the access
> +	 * here is completely racy anyway. So don't bother with locking for now.
> +	 */
> +	return i915 && i915->drm.open_count == 0;
> +}
> +
> +static void intel_vga_switcheroo_set_state(struct pci_dev *pdev,
> +					   enum vga_switcheroo_state state)
> +{
> +	struct drm_i915_private *i915 = pdev_to_i915(pdev);
> +	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> +
> +	if (!i915) {
> +		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
> +		return;
> +	}
> +
> +	if (state == VGA_SWITCHEROO_ON) {
> +		pr_info("switched on\n");
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> +		/* i915 resume handler doesn't set to D0 */
> +		pci_set_power_state(pdev, PCI_D0);
> +		i915_resume_switcheroo(i915);
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
> +	} else {
> +		pr_info("switched off\n");
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> +		i915_suspend_switcheroo(i915, pmm);
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;
> +	}
> +}
> +
> +static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> +	.set_gpu_state = intel_vga_switcheroo_set_state,
> +	.reprobe = NULL,
> +	.can_switch = intel_vga_switcheroo_can_switch,
> +};
> +
> +int intel_vga_client_register(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = i915->drm.pdev;
> +	int ret;
> +
> +	/*
> +	 * If we have > 1 VGA cards, then we need to arbitrate access to the
> +	 * common VGA resources.
> +	 *
> +	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
> +	 * then we do not take part in VGA arbitration and the
> +	 * vga_client_register() fails with -ENODEV.
> +	 */
> +	ret = vga_client_register(pdev, i915, NULL, intel_vga_client_set_decode);
> +	if (ret && ret != -ENODEV)
> +		goto out;
> +
> +	intel_register_dsm_handler();
> +
> +	ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
> +	if (ret)
> +		goto out_unregister;
> +
> +	return 0;
> +
> +out_unregister:
> +	intel_unregister_dsm_handler();
> +	vga_client_register(pdev, NULL, NULL, NULL);
> +out:
> +	return ret;
> +}
> +
> +void intel_vga_client_unregister(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = i915->drm.pdev;
> +
> +	vga_switcheroo_unregister_client(pdev);
> +	intel_unregister_dsm_handler();
> +	vga_client_register(pdev, NULL, NULL, NULL);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_vga_client.h b/drivers/gpu/drm/i915/display/intel_vga_client.h
> new file mode 100644
> index 000000000000..334eb90a2e4b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_vga_client.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __INTEL_VGA_CLIENT_H__
> +#define __INTEL_VGA_CLIENT_H__
> +
> +struct drm_i915_private;
> +
> +int intel_vga_client_register(struct drm_i915_private *i915);
> +void intel_vga_client_unregister(struct drm_i915_private *i915);
> +
> +#endif /* __INTEL_VGA_CLIENT_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 91aae56b4280..a36131b15a6f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -36,7 +36,6 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/pnp.h>
>  #include <linux/slab.h>
> -#include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <linux/vt.h>
>  #include <acpi/video.h>
> @@ -59,6 +58,7 @@
>  #include "display/intel_overlay.h"
>  #include "display/intel_pipe_crc.h"
>  #include "display/intel_sprite.h"
> +#include "display/intel_vga_client.h"
>  
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_ioctls.h"
> @@ -269,69 +269,8 @@ intel_teardown_mchbar(struct drm_i915_private *dev_priv)
>  		release_resource(&dev_priv->mch_res);
>  }
>  
> -/* true = enable decode, false = disable decoder */
> -static unsigned int i915_vga_set_decode(void *cookie, bool state)
> -{
> -	struct drm_i915_private *dev_priv = cookie;
> -
> -	intel_modeset_vga_set_state(dev_priv, state);
> -	if (state)
> -		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
> -		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> -	else
> -		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> -}
> -
> -static int i915_resume_switcheroo(struct drm_i915_private *i915);
> -static int i915_suspend_switcheroo(struct drm_i915_private *i915,
> -				   pm_message_t state);
> -
> -static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
> -{
> -	struct drm_i915_private *i915 = pdev_to_i915(pdev);
> -	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> -
> -	if (!i915) {
> -		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
> -		return;
> -	}
> -
> -	if (state == VGA_SWITCHEROO_ON) {
> -		pr_info("switched on\n");
> -		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> -		/* i915 resume handler doesn't set to D0 */
> -		pci_set_power_state(pdev, PCI_D0);
> -		i915_resume_switcheroo(i915);
> -		i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
> -	} else {
> -		pr_info("switched off\n");
> -		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> -		i915_suspend_switcheroo(i915, pmm);
> -		i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;
> -	}
> -}
> -
> -static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
> -{
> -	struct drm_i915_private *i915 = pdev_to_i915(pdev);
> -
> -	/*
> -	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
> -	 * locking inversion with the driver load path. And the access here is
> -	 * completely racy anyway. So don't bother with locking for now.
> -	 */
> -	return i915 && i915->drm.open_count == 0;
> -}
> -
> -static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> -	.set_gpu_state = i915_switcheroo_set_state,
> -	.reprobe = NULL,
> -	.can_switch = i915_switcheroo_can_switch,
> -};
> -
>  static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  {
> -	struct pci_dev *pdev = i915->drm.pdev;
>  	int ret;
>  
>  	if (i915_inject_probe_failure(i915))
> @@ -346,22 +285,9 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  
>  	intel_bios_init(i915);
>  
> -	/* If we have > 1 VGA cards, then we need to arbitrate access
> -	 * to the common VGA resources.
> -	 *
> -	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
> -	 * then we do not take part in VGA arbitration and the
> -	 * vga_client_register() fails with -ENODEV.
> -	 */
> -	ret = vga_client_register(pdev, i915, NULL, i915_vga_set_decode);
> -	if (ret && ret != -ENODEV)
> -		goto out;
> -
> -	intel_register_dsm_handler();
> -
> -	ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
> +	ret = intel_vga_client_register(i915);
>  	if (ret)
> -		goto cleanup_vga_client;
> +		goto out;
>  
>  	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
>  	intel_update_rawclk(i915);
> @@ -414,23 +340,18 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  cleanup_csr:
>  	intel_csr_ucode_fini(i915);
>  	intel_power_domains_driver_remove(i915);
> -	vga_switcheroo_unregister_client(pdev);
> -cleanup_vga_client:
> -	vga_client_register(pdev, NULL, NULL, NULL);
> +	intel_vga_client_unregister(i915);
>  out:
>  	return ret;
>  }
>  
>  static void i915_driver_modeset_remove(struct drm_i915_private *i915)
>  {
> -	struct pci_dev *pdev = i915->drm.pdev;
> -
>  	intel_modeset_driver_remove(i915);
>  
>  	intel_bios_driver_remove(i915);
>  
> -	vga_switcheroo_unregister_client(pdev);
> -	vga_client_register(pdev, NULL, NULL, NULL);
> +	intel_vga_client_unregister(i915);
>  
>  	intel_csr_ucode_fini(i915);
>  }
> @@ -1845,8 +1766,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	return ret;
>  }
>  
> -static int
> -i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
> +int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state)
>  {
>  	int error;
>  
> @@ -2014,7 +1934,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  	return ret;
>  }
>  
> -static int i915_resume_switcheroo(struct drm_i915_private *i915)
> +int i915_resume_switcheroo(struct drm_i915_private *i915)
>  {
>  	int ret;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 337d8306416a..1b64a88a54b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2223,6 +2223,9 @@ extern const struct dev_pm_ops i915_pm_ops;
>  int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>  void i915_driver_remove(struct drm_i915_private *i915);
>  
> +int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state);
> +int i915_resume_switcheroo(struct drm_i915_private *i915);
> +
>  void intel_engine_init_hangcheck(struct intel_engine_cs *engine);
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>  
> -- 
> 2.20.1
Quoting Jani Nikula (2019-10-01 14:43:53)
> Split out code related to vga client and vga switcheroo
> register/unregister and state handling from i915_drv.c and
> intel_display.c.
> 
> It's a bit difficult to draw the line how much to move to the new file
> from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
> and i915_resume_switcheroo() in place was cleanest.
> 
> No functional changes.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> It's also a bit fuzzy if this is a sensible split anyway. Could also
> name it intel_vga and move these from intel_display.c there?

My initial thought that the switcheroo interface would remain in core,
that it is more of a global power state that we currently just use for
the legacy vga switching.

The patch looks fine, on a pure mechanical pov,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

For the sake of argument, could you float the split in the other
direction?

And maybe Ville has a good opinion on how it is meant to work :)
-Chris
On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote:
> Quoting Jani Nikula (2019-10-01 14:43:53)
> > Split out code related to vga client and vga switcheroo
> > register/unregister and state handling from i915_drv.c and
> > intel_display.c.
> > 
> > It's a bit difficult to draw the line how much to move to the new file
> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
> > and i915_resume_switcheroo() in place was cleanest.
> > 
> > No functional changes.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > 
> > ---
> > 
> > It's also a bit fuzzy if this is a sensible split anyway. Could also
> > name it intel_vga and move these from intel_display.c there?
> 
> My initial thought that the switcheroo interface would remain in core,

Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks.

> that it is more of a global power state that we currently just use for
> the legacy vga switching.
> 
> The patch looks fine, on a pure mechanical pov,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> For the sake of argument, could you float the split in the other
> direction?
> 
> And maybe Ville has a good opinion on how it is meant to work :)
> -Chris
On Tue, 01 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote:
>> Quoting Jani Nikula (2019-10-01 14:43:53)
>> > Split out code related to vga client and vga switcheroo
>> > register/unregister and state handling from i915_drv.c and
>> > intel_display.c.
>> > 
>> > It's a bit difficult to draw the line how much to move to the new file
>> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
>> > and i915_resume_switcheroo() in place was cleanest.
>> > 
>> > No functional changes.
>> > 
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > 
>> > ---
>> > 
>> > It's also a bit fuzzy if this is a sensible split anyway. Could also
>> > name it intel_vga and move these from intel_display.c there?
>> 
>> My initial thought that the switcheroo interface would remain in core,
>
> Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks.

Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb
stuff (incl the ones I mentioned from intel_display.c) to
intel_vga.[ch]?

>
>> that it is more of a global power state that we currently just use for
>> the legacy vga switching.
>> 
>> The patch looks fine, on a pure mechanical pov,
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> For the sake of argument, could you float the split in the other
>> direction?

Please elaborate. Move switcheroo higher in the call chain?

BR,
Jani.


>> 
>> And maybe Ville has a good opinion on how it is meant to work :)
>> -Chris
Quoting Jani Nikula (2019-10-01 15:28:51)
> On Tue, 01 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote:
> >> For the sake of argument, could you float the split in the other
> >> direction?
> 
> Please elaborate. Move switcheroo higher in the call chain?

I was thinking along the lines of pulling switcheroo into
i915/i915_vga_client.c and seeing where that lead. I leave the details
to the poor soul pulling at the stands of spaghetti.
-Chris
On Tue, Oct 01, 2019 at 05:28:51PM +0300, Jani Nikula wrote:
> On Tue, 01 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote:
> >> Quoting Jani Nikula (2019-10-01 14:43:53)
> >> > Split out code related to vga client and vga switcheroo
> >> > register/unregister and state handling from i915_drv.c and
> >> > intel_display.c.
> >> > 
> >> > It's a bit difficult to draw the line how much to move to the new file
> >> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
> >> > and i915_resume_switcheroo() in place was cleanest.
> >> > 
> >> > No functional changes.
> >> > 
> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> > 
> >> > ---
> >> > 
> >> > It's also a bit fuzzy if this is a sensible split anyway. Could also
> >> > name it intel_vga and move these from intel_display.c there?
> >> 
> >> My initial thought that the switcheroo interface would remain in core,
> >
> > Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks.
> 
> Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb
> stuff (incl the ones I mentioned from intel_display.c) to
> intel_vga.[ch]?

I can get behind that plan.

> 
> >
> >> that it is more of a global power state that we currently just use for
> >> the legacy vga switching.
> >> 
> >> The patch looks fine, on a pure mechanical pov,
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> 
> >> For the sake of argument, could you float the split in the other
> >> direction?
> 
> Please elaborate. Move switcheroo higher in the call chain?
> 
> BR,
> Jani.
> 
> 
> >> 
> >> And maybe Ville has a good opinion on how it is meant to work :)
> >> -Chris
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
On Tue, 01 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Oct 01, 2019 at 05:28:51PM +0300, Jani Nikula wrote:
>> On Tue, 01 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote:
>> >> Quoting Jani Nikula (2019-10-01 14:43:53)
>> >> > Split out code related to vga client and vga switcheroo
>> >> > register/unregister and state handling from i915_drv.c and
>> >> > intel_display.c.
>> >> > 
>> >> > It's a bit difficult to draw the line how much to move to the new file
>> >> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
>> >> > and i915_resume_switcheroo() in place was cleanest.
>> >> > 
>> >> > No functional changes.
>> >> > 
>> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> > 
>> >> > ---
>> >> > 
>> >> > It's also a bit fuzzy if this is a sensible split anyway. Could also
>> >> > name it intel_vga and move these from intel_display.c there?
>> >> 
>> >> My initial thought that the switcheroo interface would remain in core,
>> >
>> > Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks.
>> 
>> Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb
>> stuff (incl the ones I mentioned from intel_display.c) to
>> intel_vga.[ch]?
>
> I can get behind that plan.

Patch in your inbox, I'll look into switcheroo later.

BR,
Jani.


>
>> 
>> >
>> >> that it is more of a global power state that we currently just use for
>> >> the legacy vga switching.
>> >> 
>> >> The patch looks fine, on a pure mechanical pov,
>> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >> 
>> >> For the sake of argument, could you float the split in the other
>> >> direction?
>> 
>> Please elaborate. Move switcheroo higher in the call chain?
>> 
>> BR,
>> Jani.
>> 
>> 
>> >> 
>> >> And maybe Ville has a good opinion on how it is meant to work :)
>> >> -Chris
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center