[Intel-gfx] [RFCv2 02/14] drm/i915/gvt: Introduce the basic architecture of GVT-g
Zhi Wang
zhi.a.wang at intel.com
Fri Feb 26 05:58:20 UTC 2016
Hi Joonas:
Thanks for you time and comments. :) See my replies below.
On 02/23/16 20:42, Joonas Lahtinen wrote:
> Hi,
>
> Code is looking a lot better.
>
> A general question still; why you seem to be preferring multi-stage
> alloc and destroy?
>
> Are there going to be scenarios when things will be allocated but not
> initialized? I don't see a such use scenario.
>
> I wouldn't split the init functions down as much as you currently do
> because that'll require a lot of boilerplate code to propagate the
> errors up, which is currently not done. The boilerplate for propagation
> becomes necessary when the teardown function is complex, but currently
> the teardown itself is less lines of code than the function
> boilerplate.
>
> So just squash those into gvt_device_create() and gvt_device_destroy()
> where _create() will propagate any lower level errors up and tear down
> a partially initialized struct. _destroy() can then expect to just tear
> the whole struct down with no ifs.
>
OK. Sure no problem.
> Regards, Joonas
>
> On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
>> This patch introduces the very basic framework of GVT-g device model,
>> includes basic prototypes, definitions, initialization.
>>
>> 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's 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 | 2 +
>> drivers/gpu/drm/i915/gvt/Makefile | 5 +
>> drivers/gpu/drm/i915/gvt/debug.h | 57 +++++
>> drivers/gpu/drm/i915/gvt/gvt.c | 393 +++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/gvt/gvt.h | 100 +++++++++
>> drivers/gpu/drm/i915/gvt/hypercall.h | 30 +++
>> drivers/gpu/drm/i915/gvt/mpt.h | 34 +++
>> drivers/gpu/drm/i915/gvt/params.c | 32 +++
>> drivers/gpu/drm/i915/gvt/params.h | 34 +++
>> drivers/gpu/drm/i915/gvt/reg.h | 34 +++
>> drivers/gpu/drm/i915/i915_dma.c | 14 ++
>> drivers/gpu/drm/i915/i915_drv.h | 12 ++
>> drivers/gpu/drm/i915/i915_gvt.c | 93 +++++++++
>> drivers/gpu/drm/i915/i915_gvt.h | 49 +++++
>> 15 files changed, 904 insertions(+)
>> 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/gvt/params.c
>> create mode 100644 drivers/gpu/drm/i915/gvt/params.h
>> create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
>> create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
>> create mode 100644 drivers/gpu/drm/i915/i915_gvt.h
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index 4c59793..082e77d 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -45,3 +45,18 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
>> option changes the default for that module option.
>>
>> If in doubt, say "N".
>> +
>> +config DRM_I915_GVT
>> + tristate "GVT-g host driver"
>> + depends on DRM_I915
>> + default n
>> + help
>> + Enabling GVT-g mediated graphics passthrough technique for Intel i915
>> + 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
>> + are pass-throughed to VM, with a minimal set of conflicting resources
>> + (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.
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 0851de07..c65026c 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -91,6 +91,8 @@ i915-y += dvo_ch7017.o \
>> intel_sdvo.o \
>> intel_tv.o
>>
>> +obj-$(CONFIG_DRM_I915_GVT) += i915_gvt.o gvt/
>> +
>> # virtual gpu code
>> i915-y += i915_vgpu.o
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
>> new file mode 100644
>> index 0000000..959305f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/Makefile
>> @@ -0,0 +1,5 @@
>> +GVT_SOURCE := gvt.o params.o
>> +
>> +ccflags-y += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
>> +i915_gvt-y := $(GVT_SOURCE)
>> +obj-$(CONFIG_DRM_I915_GVT) += i915_gvt.o
>> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
>> new file mode 100644
>> index 0000000..0747f28
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/debug.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * 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"\n", ##args)
>> +
>
> Just;
>
> DRM_INFO("gvt: " fmt, ##args)
>
> Do not automatically add newlines, it will confuse developers. Applies
> to all printing.
>
OK. Will improve that in the next version
>> +#define gvt_err(fmt, args...) \
>> + DRM_ERROR("gvt: "fmt"\n", ##args)
>> +
>
> Same here.
>
>> +#define gvt_warn(condition, fmt, args...) \
>> + WARN((condition), "gvt: "fmt"\n", ##args)
>> +
>> +#define gvt_warn_once(condition, fmt, args...) \
>> + WARN_ONCE((condition), "gvt: "fmt"\n", ##args)
>
> WARN and WARN_ONCE will include backtrace so prefixing is unnecessary.
> I would not prefix them at all. Just use what i915 kernel module
> already uses. If needed, split them to their own file first,
> i915_debug.h.
OK. Thanks.:)
>> +
>> +#define gvt_dbg(level, fmt, args...) \
>> + DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
>> +
>> +enum {
>> + GVT_DBG_CORE = (1 << 0),
>> + GVT_DBG_MM = (1 << 1),
>> + GVT_DBG_IRQ = (1 << 2),
>> +};
>
> This enum is not of use.
>
>> +
>> +#define gvt_dbg_core(fmt, args...) \
>> + gvt_dbg(GVT_DBG_CORE, fmt, ##args)
>> +
>
> Rahter like this (not to lose the topic)?
> gvt_dbg("core: " fmt, ##args)
>
Thanks for the idea. It's adorable. :)
>> +#define gvt_dbg_mm(fmt, args...) \
>> + gvt_dbg(GVT_DBG_MM, fmt, ##args)
>> +
>> +#define gvt_dbg_irq(fmt, args...) \
>> + gvt_dbg(GVT_DBG_IRQ, 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..52cfa32
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
>> @@ -0,0 +1,393 @@
>> +/*
>> + * 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
>> +#include
>> +#include
>> +
>> +#include "gvt.h"
>> +
>> +struct gvt_host gvt_host;
>> +
>> +static const char * const supported_hypervisors[] = {
>> + [GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
>> + [GVT_HYPERVISOR_TYPE_KVM] = "KVM",
>> +};
>> +
>> +static int gvt_init_host(void)
>> +{
>> + struct gvt_host *host = &gvt_host;
>> +
>
> Is it really that much more to write gvt_host.initialized? Counting the
> "->" vs "." it's three letters...
>
>> + if (!gvt.enable) {
>> + gvt_dbg_core("GVT-g has been disabled by kernel parameter");
>> + return -EINVAL;
>> + }
>> +
>> + if (host->initialized) {
>> + gvt_err("GVT-g has already been initialized!");
>> + return -EINVAL;
>> + }
>> +
>> + /* Xen DOM U */
>> + if (xen_domain() && !xen_initial_domain())
>> + return -ENODEV;
>> +
>> + if (xen_initial_domain()) {
>> + /* Xen Dom0 */
>> + host->kdm = try_then_request_module(
>> + symbol_get(xengt_kdm), "xengt");
>> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
>> + } else {
>> + /* not in Xen. Try KVMGT */
>> + host->kdm = try_then_request_module(
>> + symbol_get(kvmgt_kdm), "kvm");
>> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
>> + }
>> +
>> + if (!host->kdm)
>> + return -EINVAL;
>
> I think this error should be reported, to aid detecting problems in
> module loading.
>
Sure.
>> +
>> + if (!hypervisor_detect_host())
>> + return -ENODEV;
>> +
>
> This func should be prefixed gvt_, as it is not local to this file.
>
Will improve that.
>> + gvt_info("Running with hypervisor %s in host mode",
>> + supported_hypervisors[host->hypervisor_type]);
>> +
>> + idr_init(&host->device_idr);
>> + mutex_init(&host->device_idr_lock);
>> +
>> + host->initialized = true;
>> + return 0;
>> +}
>> +
>> +static int init_device_info(struct pgt_device *pdev)
>> +{
>> + struct gvt_device_info *info = &pdev->device_info;
>> +
>> + if (!IS_BROADWELL(pdev->dev_priv)) {
>> + DRM_DEBUG_DRIVER("Unsupported GEN device");
>> + return -ENODEV;
>> + }
>
> This could be "else" clause on the next if and will allow easier adding
> of future platforms.
>
Will refine it into dedicated functions. :)
>> +
>> + if (IS_BROADWELL(pdev->dev_priv)) {
>> + info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
>> + /*
>> + * 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.
>> + */
>> + info->gtt_start_offset = (1UL << 23); /* 8MB */
>> + info->max_gtt_size = (1UL << 23); /* 8MB */
>> + info->gtt_entry_size = 8;
>> + info->gtt_entry_size_shift = 3;
>> + info->gmadr_bytes_in_cmd = 8;
>> + info->mmio_size = 2 * 1024 * 1024; /* 2MB */
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
>> +{
>> + struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
>> + int i;
>> +
>> + gvt_dbg_core("init initial cfg space, id %d", pdev->id);
>> +
>> + for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
>> + pci_read_config_dword(pci_dev, i,
>> + (u32 *)&pdev->initial_cfg_space[i]);
>> +
>> + for (i = 0; i < GVT_BAR_NUM; i++) {
>> + pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
>> + gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
>> + }
>> +}
>> +
>> +static void clean_initial_mmio_state(struct pgt_device *pdev)
>> +{
>> + if (pdev->gttmmio_va) {
>> + iounmap(pdev->gttmmio_va);
>> + pdev->gttmmio_va = NULL;
>> + }
>> +
>> + if (pdev->gmadr_va) {
>> + iounmap(pdev->gmadr_va);
>> + pdev->gmadr_va = NULL;
>> + }
>> +}
>> +
>> +static int init_initial_mmio_state(struct pgt_device *pdev)
>> +{
>> + struct gvt_device_info *info = &pdev->device_info;
>> +
>> + u64 bar0, bar1;
>> + int rc;
>> +
>> + gvt_dbg_core("init initial mmio state, id %d", pdev->id);
>> +
>> + bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
>> + bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
>> +
>> + pdev->gttmmio_base = bar0 & ~0xf;
>> + pdev->reg_num = info->mmio_size / 4;
>> + pdev->gmadr_base = bar1 & ~0xf;
>
> Magic numbers still.
>
My bad. :)
>> +
>> + pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
>> + if (!pdev->gttmmio_va) {
>> + gvt_err("fail to map GTTMMIO BAR.");
>> + return -EFAULT;
>> + }
>> +
>> + pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
>> + if (!pdev->gmadr_va) {
>> + gvt_err("fail to map GMADR BAR.");
>> + rc = -EFAULT;
>> + goto err;
>> + }
>> +
>> + gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
>> + gvt_dbg_core("mmio size: %x", pdev->mmio_size);
>> + gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
>> + pdev->gmadr_base);
>> + gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
>> + gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
>> +
>> + return 0;
>> +err:
>> + clean_initial_mmio_state(pdev);
>> + return rc;
>> +}
>> +
>> +static int gvt_service_thread(void *data)
>> +{
>> + struct pgt_device *pdev = (struct pgt_device *)data;
>> + int r;
>> +
>> + gvt_dbg_core("service thread start, pgt %d", pdev->id);
>> +
>> + while (!kthread_should_stop()) {
>> + r = wait_event_interruptible(pdev->service_thread_wq,
>> + kthread_should_stop() || pdev->service_request);
>> +
>> + if (kthread_should_stop())
>> + break;
>> +
>> + if (gvt_warn_once(r,
>> + "service thread is waken up by unexpected signal."))
>> + continue;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void clean_service_thread(struct pgt_device *pdev)
>> +{
>> + if (pdev->service_thread) {
>> + kthread_stop(pdev->service_thread);
>> + pdev->service_thread = NULL;
>> + }
>> +}
>> +
>> +static int init_service_thread(struct pgt_device *pdev)
>> +{
>> + init_waitqueue_head(&pdev->service_thread_wq);
>> +
>> + pdev->service_thread = kthread_run(gvt_service_thread,
>> + pdev, "gvt_service_thread%d", pdev->id);
>> +
>> + if (!pdev->service_thread) {
>> + gvt_err("fail to start service thread.");
>> + return -ENOSPC;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void clean_pgt_device(struct pgt_device *pdev)
>> +{
>> + clean_service_thread(pdev);
>> + clean_initial_mmio_state(pdev);
>> +}
>> +
>> +static int init_pgt_device(struct pgt_device *pdev,
>> + struct drm_i915_private *dev_priv)
>> +{
>> + int rc;
>
> int ret;
>
>> +
>> + rc = init_device_info(pdev);
>> + if (rc)
>> + return rc;
>> +
>> + init_initial_cfg_space_state(pdev);
>> +
>> + rc = init_initial_mmio_state(pdev);
>> + if (rc)
>> + goto err;
>> +
>> + rc = init_service_thread(pdev);
>> + if (rc)
>> + goto err;
>> +
>> + return 0;
>> +err:
>> + clean_pgt_device(pdev);
>
> Add teardown path, see below.
>
>> + return rc;
>> +}
>> +
>> +static int post_init_pgt_device(struct pgt_device *pdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static void free_pgt_device(struct pgt_device *pdev)
>> +{
>> + struct gvt_host *host = &gvt_host;
>> +
>> + mutex_lock(&host->device_idr_lock);
>> + idr_remove(&host->device_idr, pdev->id);
>> + mutex_unlock(&host->device_idr_lock);
>> +
>> + vfree(pdev);
>> +}
>> +
>> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
>> +{
>> + struct gvt_host *host = &gvt_host;
>> + struct pgt_device *pdev = NULL;
>> +
>> + pdev = vzalloc(sizeof(*pdev));
>> + if (!pdev)
>> + return NULL;
>
> This is a memory error, would like to see this propagated up as
>
> return ERR_PTR(-ENOMEM);
>
>> +
>> + mutex_lock(&host->device_idr_lock);
>> + pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
>> + mutex_unlock(&host->device_idr_lock);
>> +
>> + if (pdev->id < 0) {
>> + gvt_err("fail to allocate pgt device id.");
>> + goto err;
>> + }
>> +
>
> Same here, propagate the error.
>
>> + mutex_init(&pdev->lock);
>> + pdev->dev_priv = dev_priv;
>> + idr_init(&pdev->instance_idr);
>> +
>> + return pdev;
>> +err:
>> + free_pgt_device(pdev);
>
> I'd like to see a teardown path here. Then free_pgt_device() (or rather
> destroy_pgt_device() can expect everything to be initialized and when
> it it is called, it doesn't need ifs. This makes the driver code more
> robust.
>
> Or are we expecting only partially initialized structs for some reason?
>
The idea here is to prevent teardown path maintain burden in future,
usually during the development, the teardown path is easy to be broken.
Sure, I will follow your idea. :)
>> + return NULL;
>> +}
>> +
>> +void gvt_destroy_pgt_device(void *private_data)
>> +{
>> + struct pgt_device *pdev = (struct pgt_device *)private_data;
>> +
>> + clean_pgt_device(pdev);
>> + free_pgt_device(pdev);
>
> Why multiple calls to destroy a device, there's only one alloc still?
>
clean_pgt_device() will call the clena_xxx() function of each
sub-components. Each sub-components will stop and release the resource
it's using. and free_pgt_device() will free the "pgt device" remove it
from the IDR pool. :)
>> +}
>> +
>> +/**
>> + * gvt_create_pgt_device - create a GVT physical device
>> + * @dev: drm device
>> + *
>> + * This function is called at the initialization stage, to create a physical
>> + * GVT device and initialize necessary GVT components for it.
>> + *
>> + * Returns:
>> + * pointer to the pgt_device structure, NULL if failed.
>> + */
>> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
>> +{
>> + struct pgt_device *pdev = NULL;
>> + struct gvt_host *host = &gvt_host;
>> + int rc;
>> +
>> + if (!host->initialized && !gvt_init_host()) {
>> + gvt_err("gvt_init_host fail");
>> + return NULL;
>> + }
>> +
>> + gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
>> +
>> + pdev = alloc_pgt_device(dev_priv);
>> + if (!pdev) {
>> + gvt_err("fail to allocate memory for pgt device.");
>> + goto err;
>> + }
>> +
>> + gvt_dbg_core("init pgt device, id %d", pdev->id);
>> +
>> + rc = init_pgt_device(pdev, dev_priv);
>> + if (rc) {
>
> Just call the return value variable ret like everywhere.
>
Sure. Good idea.
>> + gvt_err("fail to init physical device state.");
>> + goto err;
>> + }
>> +
>> + gvt_dbg_core("pgt device creation done, id %d", pdev->id);
>> +
>> + return pdev;
>> +err:
>> + if (pdev) {
>> + gvt_destroy_pgt_device(pdev);
>> + pdev = NULL;
>> + }
>
> Proper goto label based teardown path should be used.
>
Thanks. will change all error label like that.
> err_destroy_pgt:
> gvt_destroy_pgt_device(pdev);
> err:
>> + return NULL;
>> +}
>> +
>> +/**
>> + * gvt_post_init_pgt_device - post init a GVT physical device
>> + * @dev: drm device
>
> Double check the kerneldocs to be correct per arguments of function.
>
Will correct it. :)
>> + *
>> + * This function is called at the end of the initialization stage, to
>> + * post-initialize a physical GVT device and initialize necessary
>> + * GVT components rely on i915 components.
>> + *
>> + * Returns:
>> + * zero on success, non-zero if failed.
>> + */
>> +int gvt_post_init_pgt_device(void *private_data)
>> +{
>> + struct pgt_device *pdev = (struct pgt_device *)private_data;
>> + struct gvt_host *host = &gvt_host;
>> + int rc;
>> +
>> + if (!host->initialized) {
>> + gvt_err("gvt_host haven't been initialized.");
>> + return -ENODEV;
>> + }
>> +
>> + gvt_dbg_core("post init pgt device %d", pdev->id);
>> +
>> + rc = post_init_pgt_device(pdev);
>> + if (rc) {
>> + gvt_err("fail to post init physical device state.");
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> new file mode 100644
>> index 0000000..d450198
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -0,0 +1,100 @@
>> +/*
>> + * 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 "params.h"
>> +#include "reg.h"
>> +#include "hypercall.h"
>> +#include "mpt.h"
>> +
>> +#define GVT_MAX_VGPU 8
>> +
>> +enum {
>> + GVT_HYPERVISOR_TYPE_XEN = 0,
>> + GVT_HYPERVISOR_TYPE_KVM,
>> +};
>> +
>> +struct gvt_host {
>> + bool initialized;
>> + int hypervisor_type;
>> + struct mutex device_idr_lock;
>> + struct idr device_idr;
>> + struct gvt_kernel_dm *kdm;
>> +};
>> +
>> +extern struct gvt_host gvt_host;
>
> -->
>
>> +extern struct gvt_kernel_dm xengt_kdm;
>> +extern struct gvt_kernel_dm kvmgt_kdm;
>
> <-- These should likely be declared somewhere in include/ rather than
> here.
>
Will check how i915 does. :)
>> +
>> +/* Describe the limitation of HW.*/
>> +struct 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;
>> +};
>> +
>> +struct vgt_device {
>> + int id;
>> + int vm_id;
>> + struct pgt_device *pdev;
>> + bool warn_untrack;
>> +};
>> +
>> +struct pgt_device {
>
> Comments to this and other structs about what the memebers are.
>
>> + struct mutex lock;
>> + int id;
>> +
>> + struct drm_i915_private *dev_priv;
>> + struct idr instance_idr;
>> +
>> + struct gvt_device_info device_info;
>> +
>> + u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
>> + u64 bar_size[GVT_BAR_NUM];
>> +
>> + u64 gttmmio_base;
>> + void *gttmmio_va;
>> +
>> + u64 gmadr_base;
>> + void *gmadr_va;
>> +
>> + u32 mmio_size;
>> + u32 reg_num;
>> +
>> + wait_queue_head_t service_thread_wq;
>> + struct task_struct *service_thread;
>> + unsigned long service_request;
>> +};
>> +
>> +#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..0a41874
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * 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_
>> +
>> +struct gvt_kernel_dm {
>> +};
>
My bad. :)
> I would prefer more code introduced here in the initial patch, or this
> can be introduced later in the series as whole.
>
> It unnecessarily complicates the review if some files and calls are
> introduced with no documentation and implementation and only later
> their implementation is added.
>
> I can't really review if using a structure is a good idea if I can't
> see the context or implementation of their use.
>
>> +
>> +#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..e594bb8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * 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_
>> +
>> +struct vgt_device;
>> +
>> +static inline bool hypervisor_detect_host(void)
>> +{
>> + return false;
>> +}
>
> This is also not very reviewable and there's an unnecessary forward
> declaration.
>
>> +
>> +#endif /* _GVT_MPT_H_ */
>> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
>> new file mode 100644
>> index 0000000..d381d17
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/params.c
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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 "gvt.h"
>> +
>> +struct gvt_kernel_params gvt = {
>> + .enable = false,
>> + .debug = 0,
>> +};
>> +
>> +module_param_named(gvt_enable, gvt.enable, bool, 0600);
>
> This should just be
>
> module_param_named(enable, gvt.enable, bool, ...)
>
> otherwise parameter has to be passed at boot time like this:
>
> gvt.gvt_enable=1
>
> Where we want
>
> gvt.enable=1
>
> Right?
>
Yes. Will move these params into intel_gvt.c
>> +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
>> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
>> new file mode 100644
>> index 0000000..d2955b9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/params.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * 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_PARAMS_H_
>> +#define _GVT_PARAMS_H_
>> +
>> +struct gvt_kernel_params {
>> + bool enable;
>> + int debug;
>
> This member is unused currently, can be dropped.
>
Will remove that.
>> +};
>> +
>> +extern struct gvt_kernel_params gvt;
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
>> new file mode 100644
>> index 0000000..d363b74
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/reg.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * 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_REG_H
>> +#define _GVT_REG_H
>> +
>> +#define GVT_CFG_SPACE_SZ 256
>> +#define GVT_BAR_NUM 4
>> +
>> +#define GVT_REG_CFG_SPACE_BAR0 0x10
>> +#define GVT_REG_CFG_SPACE_BAR1 0x18
>> +#define GVT_REG_CFG_SPACE_BAR2 0x20
>
> Some documentation here would be great.
>
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 1c6d227..f3bed37 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
>> #include "i915_drv.h"
>> +#include "i915_gvt.h"
>> #include "i915_vgpu.h"
>> #include "i915_trace.h"
>> #include
>> @@ -1045,6 +1046,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>
>> intel_uncore_init(dev);
>>
>> + ret = intel_gvt_init(dev);
>> + if (ret)
>> + goto out_uncore_fini;
>> +
>> ret = i915_gem_gtt_init(dev);
>> if (ret)
>> goto out_uncore_fini;
>
> This needs to become "goto out_gvt_cleanup;"
>
Good idea. :)
>> @@ -1135,6 +1140,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>> goto out_power_well;
>> }
>>
>> + ret = intel_gvt_post_init(dev);
>> + if (ret) {
>> + DRM_ERROR("failed to post init pgt device\n");
>> + goto out_power_well;
>> + }
>> +
>> /*
>> * Notify a valid surface after modesetting,
>> * when running inside a VM.
>> @@ -1177,6 +1188,7 @@ out_gem_unload:
>> out_gtt:
>> i915_global_gtt_cleanup(dev);
>
> out_gvt_cleanup:
>
>> out_uncore_fini:
>>
>> + intel_gvt_cleanup(dev);
>
> This needs to be lifted up to its own label ensure proper teardown if
> i915_gem_gtt_init() fails.
>
>> intel_uncore_fini(dev);
>> i915_mmio_cleanup(dev);
>> put_bridge:
>> @@ -1223,6 +1235,8 @@ int i915_driver_unload(struct drm_device *dev)
>>
>> intel_modeset_cleanup(dev);
>>
>> + intel_gvt_cleanup(dev);
>> +
>> /*
>> * free the memory space allocated for the child device
>> * config parsed from VBT
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 20d9dbd..2f897c3 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1705,6 +1705,10 @@ struct i915_workarounds {
>> u32 hw_whitelist_count[I915_NUM_RINGS];
>> };
>>
>> +struct i915_gvt {
>> + void *pgt_device;
>> +};
>> +
>> struct i915_virtual_gpu {
>> bool active;
>> };
>> @@ -1744,6 +1748,8 @@ struct drm_i915_private {
>>
>> struct i915_virtual_gpu vgpu;
>>
>> + struct i915_gvt gvt;
>> +
>> struct intel_guc guc;
>>
>> struct intel_csr csr;
>> @@ -2780,6 +2786,12 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
>> void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
>> enum forcewake_domains domains);
>> void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
>> +
>> +static inline bool intel_gvt_active(struct drm_device *dev)
>> +{
>> + return to_i915(dev)->gvt.pgt_device ? true : false;
>> +}
>> +
>> static inline bool intel_vgpu_active(struct drm_device *dev)
>> {
>> return to_i915(dev)->vgpu.active;
>> diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
>> new file mode 100644
>> index 0000000..3ca7232
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_gvt.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * 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 "i915_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.
>> + */
>> +
>> +/**
>> + * intel_gvt_init - initialize GVT components at the beginning of i915
>> + * driver loading.
>> + * @dev: drm device *
>> + *
>> + * This function is called at the beginning of the initialization stage,
>> + * to initialize the GVT components that have to be initialized
>> + * before HW gets touched by other i915 components.
>> + */
>> +int intel_gvt_init(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> + dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
>> + if (intel_gvt_active(dev))
>> + DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * intel_gvt_post_init - initialize GVT components at the end of i915
>> + * driver loading.
>> + * @dev: drm device *
>> + *
>> + * This function is called at the end of the initialization stage,
>> + * to initialize the GVT components that have to be initialized after
>> + * other i915 components are ready.
>> + */
>> +int intel_gvt_post_init(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> + if (!intel_gvt_active(dev))
>> + return 0;
>> +
>> + return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
>> +}
>> +
>> +/**
>> + * 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))
>> + return;
>> +
>> + gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
>> + dev_priv->gvt.pgt_device = NULL;
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
>> new file mode 100644
>> index 0000000..30cc207
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_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 _I915_GVT_H_
>> +#define _I915_GVT_H_
>
> I think this file could be intel_gvt.h (all remaining functions are
> intel_gvt), and have respective intel_gvt.c file.
>
>> +
>> +#ifdef CONFIG_DRM_I915_GVT
>
> Starting here --->
>
>> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
>> +extern int gvt_post_init_pgt_device(void *data);
>> +extern void gvt_destroy_pgt_device(void *data);
>> +
>
> <-- to here, these should go to their own include file at
> include/drm/i915_gvt.h For an example, see include/drm/i915_drm.h
>
> The respective export symbols then need to be exported, For an example
> see;
>
> EXPORT_SYMBOL_GPL(i915_gpu_raise);
>
>> +extern int intel_gvt_init(struct drm_device *dev);
>> +extern int intel_gvt_post_init(struct drm_device *dev);
>> +extern void intel_gvt_cleanup(struct drm_device *dev);
>> +#else
>> +extern int intel_gvt_init(struct drm_device *dev)
>
> These should, by convention, rather be static inline;
>
> static inline int intel_gvt_init(...)
>
Sorry I miss to check them here. Surely should be static inline.....
>> +{
>> + return 0;
>> +}
>> +extern int intel_gvt_post_init(struct drm_device *dev)
>> +{
>> + return 0;
>> +}
>> +extern void intel_gvt_cleanup(struct drm_device *dev)
>> +{
>> +}
>> +#endif
>> +
>> +#endif /* _I915_GVT_H_ */
More information about the Intel-gfx
mailing list