diff mbox

[1/5] drm/i915: make intel_gvt_init() later instead of too early

Message ID 20170110065252.1730-2-zhenyuw@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhenyu Wang Jan. 10, 2017, 6:52 a.m. UTC 
Previously intel_gvt_init() was called very early even before
MMIO initialization which had several drawbacks:
- Have to handle MMIO access for initial MMIO state dump if golden
  state firmware is not available
- Hypervisor detection should depend on pvinfo only instead of detecting
  hypervisor status.
- Don't know hw resource size e.g aperture, ggtt size to determine
  for vGPU type, etc.

This trys to move intel_gvt_init() call late after required info
has already been initialized for GVT host.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/gvt.c   | 20 ++++++++------------
 drivers/gpu/drm/i915/i915_drv.c  | 14 +++++++-------
 drivers/gpu/drm/i915/intel_gvt.c |  5 +++++
 3 files changed, 20 insertions(+), 19 deletions(-)

Comments

Jike Song Jan. 11, 2017, 3:13 a.m. UTC | #1 
On 01/10/2017 02:52 PM, Zhenyu Wang wrote:
> Previously intel_gvt_init() was called very early even before
> MMIO initialization which had several drawbacks:
> - Have to handle MMIO access for initial MMIO state dump if golden
>   state firmware is not available
> - Hypervisor detection should depend on pvinfo only instead of detecting
>   hypervisor status.
> - Don't know hw resource size e.g aperture, ggtt size to determine
>   for vGPU type, etc.
> 
> This trys to move intel_gvt_init() call late after required info
> has already been initialized for GVT host.
> 
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gvt.c   | 20 ++++++++------------
>  drivers/gpu/drm/i915/i915_drv.c  | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_gvt.c |  5 +++++
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index e6bf5c533fbe..35264a991776 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -73,23 +73,19 @@ int intel_gvt_init_host(void)
>  	if (intel_gvt_host.initialized)
>  		return 0;
>  
> -	/* Xen DOM U */
> -	if (xen_domain() && !xen_initial_domain())
> -		return -ENODEV;
> -
>  	/* Try to load MPT modules for hypervisors */
> -	if (xen_initial_domain()) {
> +#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> +	/* Try KVMGT */
> +	intel_gvt_host.mpt = try_then_request_module(
> +		symbol_get(kvmgt_mpt), "kvmgt");
> +	intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
> +#endif
> +
> +	if (!intel_gvt_host.mpt && xen_initial_domain()) {
>  		/* In Xen dom0 */
>  		intel_gvt_host.mpt = try_then_request_module(
>  				symbol_get(xengt_mpt), "xengt");
>  		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_XEN;
> -	} else {
> -#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> -		/* not in Xen. Try KVMGT */
> -		intel_gvt_host.mpt = try_then_request_module(
> -				symbol_get(kvmgt_mpt), "kvmgt");
> -		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
> -#endif
>  	}
>  

Hi Zhenyu,

It is always easy for xengt to detect dom0 case, but difficult for kvmgt
to detect host, so I guess xen_initial_domain() should be removed from here,
and placed in xengt.ko.

[xengt.ko]

	static int __init xengt_init(void)
	{
	       if (!xen_initial_domain())
		       return -EINVAL;
		...

[gvt]
	request xengt_mpt
	if (failed)
		request kvmgt_mpt

With logic above, we can avoid calling xen_initial_domain(), and remove
"#include <xen/xen.h>" from gvt.c.

What'd you say? :)

--
Thanks,
Jike

>  	/* Fail to load MPT modules - bail out */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4d22b4b479b8..9270dda354db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -815,10 +815,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = intel_gvt_init(dev_priv);
> -	if (ret < 0)
> -		goto err_workqueues;
> -
>  	/* This must be called before any calls to HAS_PCH_* */
>  	intel_detect_pch(dev_priv);
>  
> @@ -832,7 +828,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	intel_init_audio_hooks(dev_priv);
>  	ret = i915_gem_load_init(dev_priv);
>  	if (ret < 0)
> -		goto err_gvt;
> +		goto err_workqueues;
>  
>  	intel_display_crc_init(dev_priv);
>  
> @@ -844,8 +840,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  
>  	return 0;
>  
> -err_gvt:
> -	intel_gvt_cleanup(dev_priv);
>  err_workqueues:
>  	i915_workqueues_cleanup(dev_priv);
>  	return ret;
> @@ -1068,6 +1062,10 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  			DRM_DEBUG_DRIVER("can't enable MSI");
>  	}
>  
> +	ret = intel_gvt_init(dev_priv);
> +	if (ret)
> +		goto out_ggtt;
> +
>  	return 0;
>  
>  out_ggtt:
> @@ -1281,6 +1279,8 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  
> +	intel_gvt_cleanup(dev_priv);
> +
>  	i915_driver_unregister(dev_priv);
>  
>  	drm_vblank_cleanup(dev);
> diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
> index 290384e86c63..d23c0fcff751 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.c
> +++ b/drivers/gpu/drm/i915/intel_gvt.c
> @@ -67,6 +67,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
>  		return 0;
>  	}
>  
> +	if (intel_vgpu_active(dev_priv)) {
> +		DRM_DEBUG_DRIVER("GVT-g is disabled for guest\n");
> +		goto bail;
> +	}
> +
>  	if (!is_supported_device(dev_priv)) {
>  		DRM_DEBUG_DRIVER("Unsupported device. GVT-g is disabled\n");
>  		goto bail;
>
Zhenyu Wang Jan. 11, 2017, 3:29 a.m. UTC | #2 
On 2017.01.11 11:13:06 +0800, Jike Song wrote:
> On 01/10/2017 02:52 PM, Zhenyu Wang wrote:
> > Previously intel_gvt_init() was called very early even before
> > MMIO initialization which had several drawbacks:
> > - Have to handle MMIO access for initial MMIO state dump if golden
> >   state firmware is not available
> > - Hypervisor detection should depend on pvinfo only instead of detecting
> >   hypervisor status.
> > - Don't know hw resource size e.g aperture, ggtt size to determine
> >   for vGPU type, etc.
> > 
> > This trys to move intel_gvt_init() call late after required info
> > has already been initialized for GVT host.
> > 
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/gvt.c   | 20 ++++++++------------
> >  drivers/gpu/drm/i915/i915_drv.c  | 14 +++++++-------
> >  drivers/gpu/drm/i915/intel_gvt.c |  5 +++++
> >  3 files changed, 20 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> > index e6bf5c533fbe..35264a991776 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > @@ -73,23 +73,19 @@ int intel_gvt_init_host(void)
> >  	if (intel_gvt_host.initialized)
> >  		return 0;
> >  
> > -	/* Xen DOM U */
> > -	if (xen_domain() && !xen_initial_domain())
> > -		return -ENODEV;
> > -
> >  	/* Try to load MPT modules for hypervisors */
> > -	if (xen_initial_domain()) {
> > +#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> > +	/* Try KVMGT */
> > +	intel_gvt_host.mpt = try_then_request_module(
> > +		symbol_get(kvmgt_mpt), "kvmgt");
> > +	intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
> > +#endif
> > +
> > +	if (!intel_gvt_host.mpt && xen_initial_domain()) {
> >  		/* In Xen dom0 */
> >  		intel_gvt_host.mpt = try_then_request_module(
> >  				symbol_get(xengt_mpt), "xengt");
> >  		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_XEN;
> > -	} else {
> > -#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> > -		/* not in Xen. Try KVMGT */
> > -		intel_gvt_host.mpt = try_then_request_module(
> > -				symbol_get(kvmgt_mpt), "kvmgt");
> > -		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
> > -#endif
> >  	}
> >  
> 
> Hi Zhenyu,
> 
> It is always easy for xengt to detect dom0 case, but difficult for kvmgt
> to detect host, so I guess xen_initial_domain() should be removed from here,
> and placed in xengt.ko.
> 
> [xengt.ko]
> 
> 	static int __init xengt_init(void)
> 	{
> 	       if (!xen_initial_domain())
> 		       return -EINVAL;
> 		...
> 
> [gvt]
> 	request xengt_mpt
> 	if (failed)
> 		request kvmgt_mpt
> 
> With logic above, we can avoid calling xen_initial_domain(), and remove
> "#include <xen/xen.h>" from gvt.c.
> 
> What'd you say? :)
> 

yep, sounds good to me.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index e6bf5c533fbe..35264a991776 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -73,23 +73,19 @@  int intel_gvt_init_host(void)
 	if (intel_gvt_host.initialized)
 		return 0;
 
-	/* Xen DOM U */
-	if (xen_domain() && !xen_initial_domain())
-		return -ENODEV;
-
 	/* Try to load MPT modules for hypervisors */
-	if (xen_initial_domain()) {
+#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
+	/* Try KVMGT */
+	intel_gvt_host.mpt = try_then_request_module(
+		symbol_get(kvmgt_mpt), "kvmgt");
+	intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
+#endif
+
+	if (!intel_gvt_host.mpt && xen_initial_domain()) {
 		/* In Xen dom0 */
 		intel_gvt_host.mpt = try_then_request_module(
 				symbol_get(xengt_mpt), "xengt");
 		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_XEN;
-	} else {
-#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
-		/* not in Xen. Try KVMGT */
-		intel_gvt_host.mpt = try_then_request_module(
-				symbol_get(kvmgt_mpt), "kvmgt");
-		intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM;
-#endif
 	}
 
 	/* Fail to load MPT modules - bail out */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4d22b4b479b8..9270dda354db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -815,10 +815,6 @@  static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	if (ret < 0)
 		return ret;
 
-	ret = intel_gvt_init(dev_priv);
-	if (ret < 0)
-		goto err_workqueues;
-
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(dev_priv);
 
@@ -832,7 +828,7 @@  static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	intel_init_audio_hooks(dev_priv);
 	ret = i915_gem_load_init(dev_priv);
 	if (ret < 0)
-		goto err_gvt;
+		goto err_workqueues;
 
 	intel_display_crc_init(dev_priv);
 
@@ -844,8 +840,6 @@  static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 
 	return 0;
 
-err_gvt:
-	intel_gvt_cleanup(dev_priv);
 err_workqueues:
 	i915_workqueues_cleanup(dev_priv);
 	return ret;
@@ -1068,6 +1062,10 @@  static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 			DRM_DEBUG_DRIVER("can't enable MSI");
 	}
 
+	ret = intel_gvt_init(dev_priv);
+	if (ret)
+		goto out_ggtt;
+
 	return 0;
 
 out_ggtt:
@@ -1281,6 +1279,8 @@  void i915_driver_unload(struct drm_device *dev)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
+	intel_gvt_cleanup(dev_priv);
+
 	i915_driver_unregister(dev_priv);
 
 	drm_vblank_cleanup(dev);
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 290384e86c63..d23c0fcff751 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -67,6 +67,11 @@  int intel_gvt_init(struct drm_i915_private *dev_priv)
 		return 0;
 	}
 
+	if (intel_vgpu_active(dev_priv)) {
+		DRM_DEBUG_DRIVER("GVT-g is disabled for guest\n");
+		goto bail;
+	}
+
 	if (!is_supported_device(dev_priv)) {
 		DRM_DEBUG_DRIVER("Unsupported device. GVT-g is disabled\n");
 		goto bail;