Re: [PATCH RESEND] drm/i915: register vga switcheroo later, unregister earlier

From: Lukas Wunner <lukas@wunner.de>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH RESEND] drm/i915: register vga switcheroo later, unregister earlier
Date: Sun, 6 Oct 2019 12:48:53 +0200	[thread overview]
Message-ID: <20191006104853.24txkx224pk36x5l@wunner.de> (raw)
In-Reply-To: <20191006094643.28824-1-jani.nikula@intel.com>

On Sun, Oct 06, 2019 at 12:46:43PM +0300, Jani Nikula wrote:
> Move vga switcheroo and dsm handler register later in
> i915_driver_register(), and unregister in i915_driver_unregister(). The
> dsm handler unregister is a nop, and is only added for completeness.
> 
> My unsubstantiated suspicion is that the vga switcheroo state change
> would not work as early as we register the hooks currently. In any case
> exposing the interfaces to the world only after we've got everything set
> up seems prudent.
> 
> Also replace the error handling in vga switcheroo register with a simple
> error message. This is done at the same time due to lack of error
> propagation from i915_driver_register().
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Acked-by: Lukas Wunner <lukas@wunner.de>

Seems like a good idea to limp on instead of bailing out if vga_switcheroo
registration fails.

Taking a brief look at intel_acpi.c, I notice intel_dsm_detect() checks
for presence of exactly 2 devices with class PCI_CLASS_DISPLAY_VGA.
This breaks if a third GPU is attached via Thunderbolt or CardBus.
Not sure if that can happen in reality, I suspect the DSM is only
present on older laptops?

I'm also confused that intel_acpi.c doesn't register a vga_switcheroo
handler.  Is the mux controlled by a separate ACPI method and the one
checked for in intel_acpi.c is only used to gather system information?

Thanks,

Lukas

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9354924576c4..63d47d699305 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -289,12 +289,6 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  	if (ret)
>  		goto out;
>  
> -	intel_register_dsm_handler();
> -
> -	ret = i915_switcheroo_register(i915);
> -	if (ret)
> -		goto cleanup_vga_client;
> -
>  	/* must happen before intel_power_domains_init_hw() on VLV/CHV */
>  	intel_update_rawclk(i915);
>  
> @@ -343,8 +337,6 @@ static int i915_driver_modeset_probe(struct drm_i915_private *i915)
>  cleanup_csr:
>  	intel_csr_ucode_fini(i915);
>  	intel_power_domains_driver_remove(i915);
> -	i915_switcheroo_unregister(i915);
> -cleanup_vga_client:
>  	intel_vga_unregister(i915);
>  out:
>  	return ret;
> @@ -356,8 +348,6 @@ static void i915_driver_modeset_remove(struct drm_i915_private *i915)
>  
>  	intel_bios_driver_remove(i915);
>  
> -	i915_switcheroo_unregister(i915);
> -
>  	intel_vga_unregister(i915);
>  
>  	intel_csr_ucode_fini(i915);
> @@ -1344,6 +1334,11 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  
>  	intel_power_domains_enable(dev_priv);
>  	intel_runtime_pm_enable(&dev_priv->runtime_pm);
> +
> +	intel_register_dsm_handler();
> +
> +	if (i915_switcheroo_register(dev_priv))
> +		DRM_ERROR("Failed to register vga switcheroo!\n");
>  }
>  
>  /**
> @@ -1352,6 +1347,10 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   */
>  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  {
> +	i915_switcheroo_unregister(dev_priv);
> +
> +	intel_unregister_dsm_handler();
> +
>  	intel_runtime_pm_disable(&dev_priv->runtime_pm);
>  	intel_power_domains_disable(dev_priv);
>  
> -- 
> 2.20.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx