From: Zhi Wang <zhi.a.wang@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"igvt-g@lists.01.org" <igvt-g@lists.01.org>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
"Cowperthwaite, David J" <david.j.cowperthwaite@intel.com>,
"Lv, Zhiyuan" <zhiyuan.lv@intel.com>
Subject: Re: [RFCv2 02/14] drm/i915/gvt: Introduce the basic architecture of GVT-g
Date: Fri, 26 Feb 2016 13:38:37 +0800 [thread overview]
Message-ID: <56CFE4DD.5030409@intel.com> (raw)
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D15F7C8FB1@SHSMSX101.ccr.corp.intel.com>
On 02/24/16 16:08, Tian, Kevin wrote:
>> From: Wang, Zhi A
>> Sent: Thursday, February 18, 2016 7:42 PM
>> 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
> [...]
>> +
>> +#include <linux/types.h>
>> +#include <xen/xen.h>
>
> would this inclusion lead to a dependency on Xen?
>
>> +#include <linux/kthread.h>
>> +
>> +#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;
>> +
>> + 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;
>> + }
>
> It'd be clearer to add a comment here that above is only a short-term
> solution. It's supposed to have a general registration framework in
> the future so any hypervisor (Xen or KVM) can register their callbacks
> at run-time, then we'll not need such direct Xen/KVM references or
> hard assumption in driver code. That framework is now still under
> discussion with Xen/KVM community. It doesn't prevent reviewing of
> other bits, but better to document it clear here.
>
I'm keeping thinking if we should split this patch into much smaller
patches and just push some basic definitions and functions for GVT
context patch review here before MPT framework is finally figured out
with RH guys?
>> +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;
>> + }
>> +
>> + 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 */
>
> Above are pure device info. Joonas, do you think whether it makes
> sense to make them to drm_i915_private, though gvt is the only
> user today?
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
>
> 'init' -> 'setup'
>
>> +{
>> + 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;
>> + }
>
> Can we reuse existing mapping in i915?
>
Yes, but we have to flush the tlb stuffs like i915, as i915 maps GTT
MMIOs as WC...
>> +}
>> +
>> +static int init_initial_mmio_state(struct pgt_device *pdev)
>> +{
>
> 'init' -> 'setup'
>
>> + 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;
>> +
>> + 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);
>> +
>
> since you called 'initial_mmio_state', suppose we should do a MMIO snapshot
> here.
>
Or we move these code into basic mmio emulation patch? :)
> [...]
>> +
>> +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;
>> +
>> + mutex_lock(&host->device_idr_lock);
>> + pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
>
> curious what such ID help here? We already have either dev_priv or
> pgt_device pointer passed around. Isn't it enough to mark a device?
>
This code piece comes from our pgt device list. currently seems there is
no for_each_pgt_device() requirement, will remove it in the next version
> [...]
>> +
>> +/**
>> + * 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)
>
> should we remove 'pgt' completely? We can always use 'gvt_device'
> as reference to the object, and then above can be gvt_create_device
> or gvt_create_physical_device, or if 'create' is a bit misleading maybe
> gvt_initialize_device is cleaner?
>
OK. :)
> Thanks
> Kevin
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-02-26 5:41 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-18 11:42 [RFCv2 PATCH 00/14] gvt: Hacking i915 for GVT context requirement Zhi Wang
2016-02-18 11:42 ` [RFCv2 01/14] drm/i915: factor out i915_pvinfo.h Zhi Wang
2016-02-22 13:23 ` Joonas Lahtinen
2016-02-23 2:40 ` Zhi Wang
2016-02-18 11:42 ` [RFCv2 02/14] drm/i915/gvt: Introduce the basic architecture of GVT-g Zhi Wang
2016-02-23 12:42 ` Joonas Lahtinen
2016-02-24 7:45 ` Tian, Kevin
2016-02-25 11:24 ` Joonas Lahtinen
2016-02-26 5:58 ` Zhi Wang
2016-02-23 12:53 ` Joonas Lahtinen
2016-02-24 7:50 ` Tian, Kevin
2016-02-24 8:08 ` Tian, Kevin
2016-02-26 5:38 ` Zhi Wang [this message]
2016-02-18 11:42 ` [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g Zhi Wang
2016-02-23 13:16 ` Joonas Lahtinen
2016-02-23 13:23 ` Zhi Wang
2016-02-24 7:42 ` Tian, Kevin
2016-02-25 13:13 ` Joonas Lahtinen
2016-02-26 5:21 ` Zhi Wang
2016-02-26 13:54 ` Joonas Lahtinen
2016-02-23 13:26 ` Joonas Lahtinen
2016-02-24 8:22 ` Tian, Kevin
2016-02-26 5:29 ` Zhi Wang
2016-02-18 11:42 ` [RFCv2 04/14] drm/i915: factor out alloc_context_idr() and __i915_gem_create_context() Zhi Wang
2016-02-24 8:27 ` Tian, Kevin
2016-02-18 11:42 ` [RFCv2 05/14] drm/i915: factor out __create_legacy_hw_context() Zhi Wang
2016-02-18 11:42 ` [RFCv2 06/14] drm/i915: let __i915_gem_context_create() takes context creation params Zhi Wang
2016-02-24 8:35 ` Tian, Kevin
2016-02-18 11:42 ` [RFCv2 07/14] drm/i915: factor out __intel_lr_context_deferred_alloc() Zhi Wang
2016-02-24 8:37 ` Tian, Kevin
2016-02-18 11:42 ` [RFCv2 08/14] drm/i915: Support per-PPGTT address space mode Zhi Wang
2016-02-24 8:47 ` Tian, Kevin
2016-02-18 11:42 ` [RFCv2 09/14] drm/i915: generate address mode bit from PPGTT instance Zhi Wang
2016-02-18 11:42 ` [RFCv2 10/14] drm/i915: update PDPs by condition when submit the LRC context Zhi Wang
2016-02-24 8:49 ` Tian, Kevin
2016-02-25 15:02 ` Wang, Zhi A
2016-02-26 13:49 ` Joonas Lahtinen
2016-02-18 11:42 ` [RFCv2 11/14] drm/i915: Introduce execlist context status change notification Zhi Wang
2016-02-18 11:42 ` [RFCv2 12/14] drm/i915: factor out execlists_i915_pick_requests() Zhi Wang
2016-02-18 11:42 ` [RFCv2 13/14] drm/i915: Support context single submission when GVT is active Zhi Wang
2016-02-24 8:52 ` Tian, Kevin
2016-02-18 11:42 ` [RFCv2 14/14] drm/i915: Introduce GVT context creation API Zhi Wang
2016-02-18 12:02 ` ✗ Fi.CI.BAT: failure for gvt: Hacking i915 for GVT context requirement Patchwork
2016-02-24 8:55 ` [RFCv2 PATCH 00/14] " Tian, Kevin
2016-02-24 9:18 ` Wang, Zhi A
2016-02-24 9:38 ` Tian, Kevin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CFE4DD.5030409@intel.com \
--to=zhi.a.wang@intel.com \
--cc=daniel.vetter@ffwll.ch \
--cc=david.j.cowperthwaite@intel.com \
--cc=igvt-g@lists.01.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=kevin.tian@intel.com \
--cc=zhiyuan.lv@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).