[Intel-gfx] [PATCH 03/15] drm/i915: gvt: Introduce the basic architecture of GVT-g
Wang, Zhi A
zhi.a.wang at intel.com
Tue May 17 02:55:01 UTC 2016
-----Original Message-----
From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
Sent: Monday, May 16, 2016 5:03 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>; intel-gfx at lists.freedesktop.org; Gordon, David S <david.s.gordon at intel.com>; joonas.lahtinen at linux.intel.com; Tian, Kevin <kevin.tian at intel.com>; Lv, Zhiyuan <zhiyuan.lv at intel.com>
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 <zhi.a.wang at intel.com>
> ---
> 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
More information about the Intel-gfx
mailing list