-----Original Message----- From: Tvrtko Ursulin [mailto:[email protected]] Sent: Monday, May 16, 2016 5:03 AM To: Wang, Zhi A <[email protected]>; [email protected]; Gordon, David S <[email protected]>; [email protected]; Tian, Kevin <[email protected]>; Lv, Zhiyuan <[email protected]> Subject: Re: [Intel-gfx] [PATCH 03/15] drm/i915: gvt: Introduce the basic architecture of GVT-g
Hi, Just a few comments from a non-assigned reviewer. On 15/05/16 18:32, Zhi Wang wrote: > This patch introduces the very basic framework of GVT-g device model, > includes basic prototypes, definitions, initialization. > > v3: > Take Joonas' comments: > - Change file name i915_gvt.* to intel_gvt.* > - Move GVT kernel parameter into intel_gvt.c > - Remove redundant debug macros > - Change error handling style > - Add introductions for some stub functions > - Introduce drm/i915_gvt.h. > > Take Kevin's comments: > - Move GVT-g host/guest check into intel_vgt_balloon in i915_gem_gtt.c > > v2: > - Introduce i915_gvt.c. > It's necessary to introduce the stubs between i915 driver and GVT-g > host, as GVT-g components is configurable in kernel config. When > disabled, the stubs here do nothing. > > Take Joonas' comments: > - Replace boolean return value with int. > - Replace customized info/warn/debug macros with DRM macros. > - Document all non-static functions like i915. > - Remove empty and unused functions. > - Replace magic number with marcos. > - Set GVT-g in kernel config to "n" by default. > > Signed-off-by: Zhi Wang <[email protected]> > --- > drivers/gpu/drm/i915/Kconfig | 15 +++ > drivers/gpu/drm/i915/Makefile | 5 + > drivers/gpu/drm/i915/gvt/Makefile | 5 + > drivers/gpu/drm/i915/gvt/debug.h | 36 ++++++ > drivers/gpu/drm/i915/gvt/gvt.c | 209 > +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/gvt/gvt.h | 85 ++++++++++++++ > drivers/gpu/drm/i915/gvt/hypercall.h | 38 +++++++ > drivers/gpu/drm/i915/gvt/mpt.h | 51 +++++++++ > drivers/gpu/drm/i915/i915_dma.c | 17 ++- > drivers/gpu/drm/i915/i915_drv.h | 12 ++ > drivers/gpu/drm/i915/intel_gvt.c | 106 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_gvt.h | 49 ++++++++ > include/drm/i915_gvt.h | 31 ++++++ > 13 files changed, 655 insertions(+), 4 deletions(-) > create mode 100644 drivers/gpu/drm/i915/gvt/Makefile > create mode 100644 drivers/gpu/drm/i915/gvt/debug.h > create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c > create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h > create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h > create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h > create mode 100644 drivers/gpu/drm/i915/intel_gvt.c > create mode 100644 drivers/gpu/drm/i915/intel_gvt.h > create mode 100644 include/drm/i915_gvt.h > > diff --git a/drivers/gpu/drm/i915/Kconfig > b/drivers/gpu/drm/i915/Kconfig index 29a32b1..782c97c 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -57,6 +57,21 @@ config DRM_I915_USERPTR > > If in doubt, say "Y". > > +config DRM_I915_GVT > + bool "Intel GVT-g host driver" > + depends on DRM_I915 > + default n > + help > + Enabling GVT-g mediated graphics passthrough technique for > +Intel i915 pass-through > + based integrated graphics card. With GVT-g, it's possible to have > one > + integrated i915 device shared by multiple VMs. Performance critical > + opterations such as apperture accesses and ring buffer > + operations operations, aperture > + are pass-throughed to VM, with a minimal set of conflicting > + resources passed-through to the host or hypervisor ? > + (e.g. display settings) mediated by vGT driver. The benefit of vGT > + is on both the performance, given that each VM could directly > operate > + its aperture space and submit commands like running on native, and > + the feature completeness, given that a true GEN hardware is > exposed. > + > menu "drm/i915 Debugging" > depends on DRM_I915 > depends on EXPERT > diff --git a/drivers/gpu/drm/i915/Makefile > b/drivers/gpu/drm/i915/Makefile index 63c4d2b..e48145b 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -103,6 +103,11 @@ i915-y += i915_vgpu.o > # legacy horrors > i915-y += i915_dma.o > > +ifeq ($(CONFIG_DRM_I915_GVT),y) > +i915-y += intel_gvt.o > +include $(src)/gvt/Makefile > +endif > + > obj-$(CONFIG_DRM_I915) += i915.o > > CFLAGS_i915_trace_points.o := -I$(src) diff --git > a/drivers/gpu/drm/i915/gvt/Makefile > b/drivers/gpu/drm/i915/gvt/Makefile > new file mode 100644 > index 0000000..d0f21a6 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/Makefile > @@ -0,0 +1,5 @@ > +GVT_DIR := gvt > +GVT_SOURCE := gvt.o > + > +ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) -Wall > +i915-y += $(addprefix $(GVT_DIR)/, > $(GVT_SOURCE)) > diff --git a/drivers/gpu/drm/i915/gvt/debug.h > b/drivers/gpu/drm/i915/gvt/debug.h > new file mode 100644 > index 0000000..5b067d2 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/debug.h > @@ -0,0 +1,36 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef __GVT_DEBUG_H__ > +#define __GVT_DEBUG_H__ > + > +#define gvt_info(fmt, args...) \ > + DRM_INFO("gvt: "fmt, ##args) > + > +#define gvt_err(fmt, args...) \ > + DRM_ERROR("gvt: "fmt, ##args) > + > +#define gvt_dbg_core(fmt, args...) \ > + DRM_DEBUG_DRIVER("gvt: core: "fmt, ##args) > + > +#endif > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c > b/drivers/gpu/drm/i915/gvt/gvt.c new file mode 100644 index > 0000000..72ca301 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/gvt.c > @@ -0,0 +1,209 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#include <linux/types.h> > +#include <xen/xen.h> > +#include <linux/kthread.h> > + > +#include "gvt.h" > + > +struct intel_gvt_host intel_gvt_host; > + > +static const char * const supported_hypervisors[] = { > + [INTEL_GVT_HYPERVISOR_XEN] = "XEN", > + [INTEL_GVT_HYPERVISOR_KVM] = "KVM", > +}; > + > +#define MB(x) (x * 1024ULL * 1024ULL) #define GB(x) (x * MB(1024)) > + > +/* > + * The layout of BAR0 in BDW: > + * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->| > + * > + * GTT offset in BAR0 starts from 8MB to 16MB, and > + * Whatever GTT size is configured in BIOS, > + * the size of BAR0 is always 16MB. The actual configured > + * GTT size can be found in GMCH_CTRL. > + */ > +static struct intel_gvt_device_info broadwell_device_info = { > + .max_gtt_gm_sz = GB(4), /* 4GB */ > + .gtt_start_offset = MB(8), /* 8MB */ > + .max_gtt_size = MB(8), /* 8MB */ > + .gtt_entry_size = 8, > + .gtt_entry_size_shift = 3, > + .gmadr_bytes_in_cmd = 8, > + .mmio_size = MB(2), /* 2MB */ > + .mmio_bar = 0, /* BAR 0 */ > + .max_support_vgpu = 8, > + .max_surface_size = MB(36), > +}; > + > +static int init_gvt_host(void) > +{ > + if (WARN(intel_gvt_host.initialized, > + "Intel GVT host has been initialized\n")) Maybe add "already" for extra clarity? > + return -EINVAL; > + > + /* Xen DOM U */ > + if (xen_domain() && !xen_initial_domain()) > + return -ENODEV; > + > + if (xen_initial_domain()) { > + /* 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 { > + /* not in Xen. Try KVMGT */ > + intel_gvt_host.mpt = try_then_request_module( > + symbol_get(kvmgt_mpt), "kvm"); > + intel_gvt_host.hypervisor_type = INTEL_GVT_HYPERVISOR_KVM; > + } > + > + if (!intel_gvt_host.mpt) { > + gvt_err("Fail to load any MPT modules.\n"); > + return -EINVAL; > + } > + > + if (!intel_gvt_hypervisor_detect_host()) > + return -ENODEV; > + > + gvt_info("Running with hypervisor %s in host mode\n", > + supported_hypervisors[intel_gvt_host.hypervisor_type]); > + > + idr_init(&intel_gvt_host.gvt_idr); > + mutex_init(&intel_gvt_host.gvt_idr_lock); > + > + intel_gvt_host.initialized = true; > + return 0; > +} > + > +static int init_device_info(struct intel_gvt *gvt) { > + if (IS_BROADWELL(gvt->dev_priv)) > + gvt->device_info = &broadwell_device_info; > + else > + return -ENODEV; > + > + return 0; > +} > + > +static void free_gvt_device(struct intel_gvt *gvt) { > + mutex_lock(&intel_gvt_host.gvt_idr_lock); > + idr_remove(&intel_gvt_host.gvt_idr, gvt->id); > + mutex_unlock(&intel_gvt_host.gvt_idr_lock); > + > + vfree(gvt); > +} > + > +static struct intel_gvt *alloc_gvt_device(struct drm_i915_private > +*dev_priv) { > + struct intel_gvt *gvt = NULL; Don't need to initialize since it is assigned to unconditionally below. > + int ret; > + > + gvt = vzalloc(sizeof(*gvt)); struct intel_gvt does not seem that large - why not cheaper kzalloc ? As this is just a stub patch, in the following patches, this structure will become huge. So use vzalloc here. > + if (!gvt) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&intel_gvt_host.gvt_idr_lock); > + ret = idr_alloc(&intel_gvt_host.gvt_idr, gvt, 0, 0, GFP_KERNEL); > + mutex_unlock(&intel_gvt_host.gvt_idr_lock); > + > + if (ret < 0) > + goto err; > + > + gvt->id = ret; > + mutex_init(&gvt->lock); > + gvt->dev_priv = dev_priv; > + idr_init(&gvt->vgpu_idr); > + > + return gvt; > +err: > + free_gvt_device(gvt); > + return ERR_PTR(ret); > +} > + > +/** > + * intel_gvt_destroy_device - destroy a GVT device > + * @gvt_device: gvt device > + * > + * This function is called at the driver unloading stage, to destroy > +a > + * GVT device and free the related resources. > + * > + * Returns: > + * None > + */ > +void intel_gvt_destroy_device(void *gvt_device) { > + struct intel_gvt *gvt = (struct intel_gvt *)gvt_device; Hm, why does this function need to take a void * instead of the correct type? I don't want i915 to include gvt/gvt.h... > + > + free_gvt_device(gvt); > +} > + > +/** > + * intel_gvt_create_device - create a GVT device > + * @dev: drm device > + * > + * This function is called at the initialization stage, to create a > + * GVT device and initialize necessary GVT components for it. > + * > + * Returns: > + * pointer to the intel gvt device structure, error pointer if failed. > + */ > +void *intel_gvt_create_device(void *dev) { > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct intel_gvt *gvt = NULL; No need to initialize. > + int ret; > + > + if (!intel_gvt_host.initialized) { > + ret = init_gvt_host(); > + if (ret) > + return ERR_PTR(ret); > + } > + > + gvt_dbg_core("create new gvt device, i915 dev_priv: %p\n", > +dev_priv); Probably not that interesting to log dev_priv address ? Can't remember every seeing any part of the driver doing it. Willl remove that > + > + gvt = alloc_gvt_device(dev_priv); > + if (IS_ERR(gvt)) { > + ret = PTR_ERR(gvt); > + goto out_err; > + } > + > + gvt_dbg_core("init gvt device, id %d\n", gvt->id); > + > + ret = init_device_info(gvt); > + if (ret) > + goto out_free_gvt_device; There is some redundacy in supported platform checking between init_device_info and is_supported_device. If you don't need both perhaps try to simplify the code a bit by eliminating one of them? Can we really remove platform check in init_device_info, anyway we have to attach different platform device info for different platform.. > + > + gvt_dbg_core("gvt device creation done, id %d\n", gvt->id); > + > + return gvt; > + > +out_free_gvt_device: > + free_gvt_device(gvt); > +out_err: > + return ERR_PTR(ret); > +} > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h > b/drivers/gpu/drm/i915/gvt/gvt.h new file mode 100644 index > 0000000..5ef9e1b > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/gvt.h > @@ -0,0 +1,85 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef _GVT_H_ > +#define _GVT_H_ > + > +#include "i915_drv.h" > +#include "i915_vgpu.h" > + > +#include "debug.h" > +#include "hypercall.h" > + > +#define GVT_MAX_VGPU 8 > +#define GVT_ALIGN(addr, size) ((addr) & (~((typeof(addr))(size) - > +1))) Same as existing kernel's ALIGN ? Nope, kernel ALIGN is a up-ALIGN, this is a down-ALIGN > + > +enum { > + INTEL_GVT_HYPERVISOR_XEN = 0, > + INTEL_GVT_HYPERVISOR_KVM, > +}; > + > +struct intel_gvt_host { > + bool initialized; > + int hypervisor_type; > + struct mutex gvt_idr_lock; > + struct idr gvt_idr; > + struct intel_gvt_mpt *mpt; > +}; > + > +extern struct intel_gvt_host intel_gvt_host; > + > +/* Describe the limitation of HW.*/ > +struct intel_gvt_device_info { > + u64 max_gtt_gm_sz; > + u32 gtt_start_offset; > + u32 gtt_end_offset; > + u32 max_gtt_size; > + u32 gtt_entry_size; > + u32 gtt_entry_size_shift; > + u32 gmadr_bytes_in_cmd; > + u32 mmio_size; > + u32 mmio_bar; > + u32 max_support_vgpu; > + u32 max_surface_size; What surface is this? Maybe add some comments for the fields? Sure, will do. > +}; > + > +struct intel_vgpu { > + struct intel_gvt *gvt; > + int id; > + int vm_id; > + bool warn_untrack; > +}; > + > +struct intel_gvt { > + struct mutex lock; > + int id; > + > + struct drm_i915_private *dev_priv; > + struct idr vgpu_idr; > + > + struct intel_gvt_device_info *device_info; }; > + > +#include "mpt.h" > + > +#endif > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h > b/drivers/gpu/drm/i915/gvt/hypercall.h > new file mode 100644 > index 0000000..254df8b > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h > @@ -0,0 +1,38 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef _GVT_HYPERCALL_H_ > +#define _GVT_HYPERCALL_H_ > + > +/* > + * Specific GVT-g MPT modules function collections. Currently GVT-g > +supports > + * both Xen and KVM by providing dedicated hypervisor-related MPT modules. > + */ > +struct intel_gvt_mpt { > + int (*detect_host)(void); > +}; > + > +extern struct intel_gvt_mpt xengt_mpt; extern struct intel_gvt_mpt > +kvmgt_mpt; > + > +#endif /* _GVT_HYPERCALL_H_ */ > diff --git a/drivers/gpu/drm/i915/gvt/mpt.h > b/drivers/gpu/drm/i915/gvt/mpt.h new file mode 100644 index > 0000000..d3f23cc > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/mpt.h > @@ -0,0 +1,51 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef _GVT_MPT_H_ > +#define _GVT_MPT_H_ > + > +/** > + * DOC: Hypervisor Service APIs for GVT-g Core Logic > + * > + * This is the glue layer between specific hypervisor MPT modules and > +GVT-g core > + * logic. Each kind of hypervisor MPT module provides a collection of > +function > + * callbacks via gvt_kernel_dm and will be attached to GVT host when > +driver > + * loading. GVT-g core logic will call these APIs to request specific > +services > + * from hypervisor. > + */ > + > +/** > + * intel_gvt_hypervisor_detect_host - check if GVT-g is running > +within > + * hypervisor host/privilged domain > + * > + * Returns: > + * Zero on success, -ENODEV if current kernel is running inside a VM > +*/ static inline int intel_gvt_hypervisor_detect_host(void) > +{ > + if (WARN_ON(!intel_gvt_host.mpt)) > + return -ENODEV; Is this condition impossible due the check in init_gvt_host ? Will remove that. > + return intel_gvt_host.mpt->detect_host(); > +} > + > +#endif /* _GVT_MPT_H_ */ > diff --git a/drivers/gpu/drm/i915/i915_dma.c > b/drivers/gpu/drm/i915/i915_dma.c index 547100f..795a5cf 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -35,6 +35,7 @@ > #include "intel_drv.h" > #include <drm/i915_drm.h> > #include "i915_drv.h" > +#include "intel_gvt.h" > #include "i915_vgpu.h" > #include "i915_trace.h" > #include <linux/pci.h> > @@ -1245,18 +1246,22 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > goto out_ggtt; > } > > + ret = intel_gvt_init(dev); > + if (ret) > + goto out_ggtt; > + > /* WARNING: Apparently we must kick fbdev drivers before vgacon, > * otherwise the vga fbdev driver falls over. */ > ret = i915_kick_out_firmware_fb(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); > - goto out_ggtt; > + goto out_gvt; > } > > ret = i915_kick_out_vgacon(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting VGA console\n"); > - goto out_ggtt; > + goto out_gvt; > } > > pci_set_master(dev->pdev); > @@ -1267,7 +1272,7 @@ static int i915_driver_init_hw(struct drm_i915_private > *dev_priv) > if (ret) { > DRM_ERROR("failed to set DMA mask\n"); > > - goto out_ggtt; > + goto out_gvt; > } > } > > @@ -1297,7 +1302,7 @@ static int i915_driver_init_hw(struct drm_i915_private > *dev_priv) > aperture_size); > if (!ggtt->mappable) { > ret = -EIO; > - goto out_ggtt; > + goto out_gvt; > } > > ggtt->mtrr = arch_phys_wc_add(ggtt->mappable_base, > @@ -1330,6 +1335,8 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > > return 0; > > +out_gvt: > + intel_gvt_cleanup(dev); > out_ggtt: > i915_ggtt_cleanup_hw(dev); > > @@ -1488,6 +1495,8 @@ int i915_driver_unload(struct drm_device *dev) > > intel_fbdev_fini(dev); > > + intel_gvt_cleanup(dev); > + > ret = i915_gem_suspend(dev); > if (ret) { > DRM_ERROR("failed to idle hardware: %d\n", ret); diff --git > a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 72f0b02..b256ba7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1703,6 +1703,10 @@ struct i915_workarounds { > u32 hw_whitelist_count[I915_NUM_ENGINES]; > }; > > +struct i915_gvt { > + void *gvt_device; Hm, again, why void * ? Will it be possible for this to hold some non i915 pointer in the future? > +}; > + > struct i915_virtual_gpu { > bool active; > }; > @@ -1742,6 +1746,8 @@ struct drm_i915_private { > > struct i915_virtual_gpu vgpu; > > + struct i915_gvt gvt; > + > struct intel_guc guc; > > struct intel_csr csr; > @@ -2868,6 +2874,12 @@ void intel_uncore_forcewake_put__locked(struct > drm_i915_private *dev_priv, > u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); > > void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); > + > +static inline bool intel_gvt_active(struct drm_i915_private > +*dev_priv) { > + return dev_priv->gvt.gvt_device ? true : false; } > + > static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv) > { > return dev_priv->vgpu.active; > diff --git a/drivers/gpu/drm/i915/intel_gvt.c > b/drivers/gpu/drm/i915/intel_gvt.c > new file mode 100644 > index 0000000..57b4910 > --- /dev/null > +++ b/drivers/gpu/drm/i915/intel_gvt.c > @@ -0,0 +1,106 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#include "i915_drv.h" > +#include "intel_gvt.h" > + > +/** > + * DOC: Intel GVT-g host support > + * > + * Intel GVT-g is a graphics virtualization technology which shares > +the > + * GPU among multiple virtual machines on a time-sharing basis. Each > + * virtual machine is presented a virtual GPU (vGPU), which has > +equivalent > + * features as the underlying physical GPU (pGPU), so i915 driver can > +run > + * seamlessly in a virtual machine. This file provides the > +englightments > + * of GVT and the necessary components used by GVT in i915 driver. > + */ > + > +struct gvt_kernel_params gvt_kparams = { > + .enable = false, > +}; > + > +/* i915.gvt_enable */ > +module_param_named(gvt_enable, gvt_kparams.enable, bool, 0600); > +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support"); > + > +static bool is_supported_device(struct drm_device *dev) { > + struct drm_i915_private *dev_priv = to_i915(dev); > + > + if (IS_BROADWELL(dev_priv)) > + return true; > + > + return false; > +} > + > +/** > + * intel_gvt_init - initialize GVT components > + * @dev: drm device * > + * > + * This function is called at the initialization stage to initialize > +the > + * GVT components. > + */ > +int intel_gvt_init(struct drm_device *dev) { > + struct drm_i915_private *dev_priv = to_i915(dev); > + void *device; > + > + if (!gvt_kparams.enable) { > + DRM_DEBUG_DRIVER("GVT-g is disabled by kernel params\n"); > + return 0; > + } > + > + if (!is_supported_device(dev)) { > + DRM_DEBUG_DRIVER("Unsupported device. GVT-g is disabled\n"); > + return 0; > + } > + > + device = intel_gvt_create_device(dev); > + if (IS_ERR(device)) { > + DRM_DEBUG_DRIVER("GVT-g is disabled\n"); > + return 0; > + } > + > + dev_priv->gvt.gvt_device = device; > + DRM_DEBUG_DRIVER("GVT-g is running in host mode\n"); Slightly redundant since init_gvt_host already would have logged a gvt_info message to the same effect. On the topic of which, perhaps code would be clearer if init_gvt_host was called explicitly here from intel_gvt_init, and not behind the scenes from intel_gvt_create_device ? Or is there a reason it has to be like it is which I missed? I thought init_gvt_host() is a part of GVT-g, it might be better to call them in create_device. > + > + return 0; > +} > + > +/** > + * intel_gvt_cleanup - cleanup GVT components when i915 driver is > +unloading > + * @dev: drm device * > + * > + * This function is called at the i915 driver unloading stage, to > +shutdown > + * GVT components and release the related resources. > + */ > +void intel_gvt_cleanup(struct drm_device *dev) { > + struct drm_i915_private *dev_priv = to_i915(dev); > + > + if (!intel_gvt_active(dev_priv)) > + return; > + > + intel_gvt_destroy_device(dev_priv->gvt.gvt_device); > + dev_priv->gvt.gvt_device = NULL; > +} > diff --git a/drivers/gpu/drm/i915/intel_gvt.h > b/drivers/gpu/drm/i915/intel_gvt.h > new file mode 100644 > index 0000000..d9b55ac50 > --- /dev/null > +++ b/drivers/gpu/drm/i915/intel_gvt.h > @@ -0,0 +1,49 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > +"Software"), > + * to deal in the Software without restriction, including without > +limitation > + * the rights to use, copy, modify, merge, publish, distribute, > +sublicense, > + * and/or sell copies of the Software, and to permit persons to whom > +the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the next > + * paragraph) shall be included in all copies or substantial portions > +of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef _INTEL_GVT_H_ > +#define _INTEL_GVT_H_ > + > +#ifdef CONFIG_DRM_I915_GVT > + > +#include <drm/i915_gvt.h> > + > +struct gvt_kernel_params { > + bool enable; > +}; > + > +extern struct gvt_kernel_params gvt_kparams; > + > +extern int intel_gvt_init(struct drm_device *dev); extern void > +intel_gvt_cleanup(struct drm_device *dev); #else static inline int > +intel_gvt_init(struct drm_device *dev) { > + return 0; > +} > +static inline void intel_gvt_cleanup(struct drm_device *dev) { } > +#endif > + > +#endif /* _INTEL_GVT_H_ */ > diff --git a/include/drm/i915_gvt.h b/include/drm/i915_gvt.h new file > mode 100644 index 0000000..e126536 > --- /dev/null > +++ b/include/drm/i915_gvt.h > @@ -0,0 +1,31 @@ > +/* > + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. > + * All Rights Reserved. > + * > + * Permission is hereby granted, free of charge, to any person > +obtaining a > + * copy of this software and associated documentation files (the > + * "Software"), to deal in the Software without restriction, > +including > + * without limitation the rights to use, copy, modify, merge, > +publish, > + * distribute, sub license, and/or sell copies of the Software, and > +to > + * permit persons to whom the Software is furnished to do so, subject > +to > + * the following conditions: > + * > + * The above copyright notice and this permission notice (including > +the > + * next paragraph) shall be included in all copies or substantial > +portions > + * of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > +EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > +MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > +SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > +OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > +ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > +OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > +#ifndef _I915_GVT_H > +#define _I915_GVT_H > + > +extern void *intel_gvt_create_device(void *dev); extern void > +intel_gvt_destroy_device(void *gvt_device); > + > +#endif /* _I915_GVT_H */ > Regards, Tvrtko _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
