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@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> ---
>
> 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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
