Re: [PATCH 2/4] drm/i915: split out i915_switcheroo.[ch] from i915_drv.c

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 02, 2019 at 04:17:58PM +0300, Jani Nikula wrote:
> Split out code related to vga switcheroo register/unregister and state
> handling from i915_drv.c into new i915_switcheroo.[ch] files.
> 
> 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 the 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>
> ---
>  drivers/gpu/drm/i915/Makefile          |  1 +
>  drivers/gpu/drm/i915/i915_drv.c        | 63 +++---------------------
>  drivers/gpu/drm/i915/i915_drv.h        |  3 ++
>  drivers/gpu/drm/i915/i915_switcheroo.c | 67 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_switcheroo.h | 14 ++++++
>  5 files changed, 92 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_switcheroo.c
>  create mode 100644 drivers/gpu/drm/i915/i915_switcheroo.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index d2b53b5add81..136a1a51b6e2 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -46,6 +46,7 @@ i915-y += i915_drv.o \
>  	  i915_pci.o \
>  	  i915_scatterlist.o \
>  	  i915_suspend.o \
> +	  i915_switcheroo.o \
>  	  i915_sysfs.o \
>  	  i915_utils.o \
>  	  intel_csr.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3306c6bb515a..7349fd8c9796 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -72,6 +72,7 @@
>  #include "i915_perf.h"
>  #include "i915_query.h"
>  #include "i915_suspend.h"
> +#include "i915_switcheroo.h"
>  #include "i915_sysfs.h"
>  #include "i915_trace.h"
>  #include "i915_vgpu.h"
> @@ -269,56 +270,8 @@ intel_teardown_mchbar(struct drm_i915_private *dev_priv)
>  		release_resource(&dev_priv->mch_res);
>  }
>  
> -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))
> @@ -339,7 +292,7 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  
>  	intel_register_dsm_handler();
>  
> -	ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
> +	ret = i915_switcheroo_register(i915);
>  	if (ret)
>  		goto cleanup_vga_client;
>  
> @@ -394,7 +347,7 @@ 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);
> +	i915_switcheroo_unregister(i915);
>  cleanup_vga_client:
>  	intel_vga_unregister(i915);
>  out:
> @@ -403,13 +356,12 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  
>  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);
> +	i915_switcheroo_unregister(i915);
> +
>  	intel_vga_unregister(i915);
>  
>  	intel_csr_ucode_fini(i915);
> @@ -1825,8 +1777,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;
>  
> @@ -1994,7 +1945,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..0ca4d90dfa46 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_resume_switcheroo(struct drm_i915_private *i915);
> +int i915_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state);
> +
>  void intel_engine_init_hangcheck(struct intel_engine_cs *engine);
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>  
> diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
> new file mode 100644
> index 000000000000..39c79e1c5b52
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_switcheroo.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/vga_switcheroo.h>
> +
> +#include "i915_drv.h"
> +#include "i915_switcheroo.h"
> +
> +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;
> +	}

Is that dead code?

Patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	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,
> +};
> +
> +int i915_switcheroo_register(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = i915->drm.pdev;
> +
> +	return vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false);
> +}
> +
> +void i915_switcheroo_unregister(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = i915->drm.pdev;
> +
> +	vga_switcheroo_unregister_client(pdev);
> +}
> diff --git a/drivers/gpu/drm/i915/i915_switcheroo.h b/drivers/gpu/drm/i915/i915_switcheroo.h
> new file mode 100644
> index 000000000000..59b6c1e07d75
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_switcheroo.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __I915_SWITCHEROO__
> +#define __I915_SWITCHEROO__
> +
> +struct drm_i915_private;
> +
> +int i915_switcheroo_register(struct drm_i915_private *i915);
> +void i915_switcheroo_unregister(struct drm_i915_private *i915);
> +
> +#endif /* __I915_SWITCHEROO__ */
> -- 
> 2.20.1

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