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

Fri May 20 12:18:10 UTC 2016

Thanks! See my replies below.

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Friday, May 20, 2016 2:49 PM
> To: Wang, Zhi A <zhi.a.wang at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; tvrtko.ursulin at linux.intel.com;
> joonas.lahtinen at linux.intel.com; Tian, Kevin <kevin.tian at intel.com>; Lv,
> Zhiyuan <zhiyuan.lv at intel.com>
> Subject: Re: [PATCH 3/9] drm/i915: gvt: Introduce the basic architecture of
> GVT-g
> 
> 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)?
> 
Basically that text is only technical introduction. Or maybe we could discuss what we want here:
a. Basic technical introduction.
b. Need Xen and KVM
c. Possible danger as it's only preliminary for now.

i915 Kconfig introduction consists 3 parts:
a. Introduction
b. Userspace requirement
c. Limitiation

Can I follow that? :)

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

Then how about [drm][gvt]? :)

> > +
> > +#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.)
> 
OK. Then I think I should remove most them in the first path and add them in the later patch. And do you want me to merge these HW descriptions into i915_device_info? Some of them might not be used by i915 driver itself. That's why I keep a dedicated data structure here.

> > +/**
> > + * 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.
> 
Let me think an approach to remove the "void *" here. How about we do like this:
struct a{
	Struct b;
};

Let i915 take only struct b, then in gvt we do container_of() so that we don't need to expose the declaration of struct a to i915? :)

> > +};
> 
> > +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).
OK. Will do.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre