[Intel-gfx] [PATCH 3/9] drm/i915: gvt: Introduce the basic architecture of GVT-g

[Chris Wilson]

On Tue, May 17, 2016 at 04:19:03AM -0400, Zhi Wang wrote:
> +config DRM_I915_GVT
> +        bool "Intel GVT-g host driver"
> +        depends on DRM_I915
> +        default n
> +        help
> +          Enabling GVT-g mediated graphics pass-through 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
> +          operations such as aperture accesses and ring buffer operations
> +          are passed-through to VM, with a minimal set of conflicting resources
> +          (e.g. display settings) mediated by GVT host driver. The benefit of GVT
> +          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.

Just like that? No userspace support required? i.e. can it work with
kvm or xen? Wants a comment on any possible danger (or why is it 'n' by
default)?

> +#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)

I think it is fair to say that neither of these will look nice in
user-facing messages.

> +
> +#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..aa40357
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -0,0 +1,205 @@
> +/*
> + * 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,

Too much superfluous detail upfront. It is hard to critique when you have
no idea what it is for. At the moment, it looks like you are duplicating
information we have elsewhere, and I'd rather not. (For starters, it
makes GVT a bigger maintainance burden.)

> +/**
> + * 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.
> + *
> + */
> +void intel_gvt_destroy_device(void *device)

This should not be void *. You can forward declare struct intel_gvt such
that the core has an opaque pointer.

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72f0b02..7d0b8d3 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;

As before. No void * here.

> +};

> +struct gvt_kernel_params {
> +	bool enable;
> +};
> +
> +extern struct gvt_kernel_params gvt_kparams;

No. Just make room for them inside i915_params. Module parameters
deserve extra scrutiny as they are (soft) ABI (even if we think users
shouldn't be using them).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre