> 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.
> +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?
> +}
> +
> +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.
[...]
> +
> +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?
[...]
> +
> +/**
> + * 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?
Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
