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

[Chris Wilson]

On Fri, May 20, 2016 at 12:18:10PM +0000, Wang, Zhi A wrote:
> 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? :)

I'm never going to argue against giving good advice to the reader. Even
if it is "for further details please see 01.org/blah"

> > > +#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]? :)

Just a comment to say to hesitate before using anything above DRM_DEBUG
and ask yourself "is this the right way to provide this information to
the user, is it as clear/useful as possible?"
> > > +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.

It's going to be on a case by case basis, but I would rather have hw
descriptions in core.

> > > +/**
> > > + * 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? :)

That's one approach. For starters you can just

struct intel_gvt;
struct i915_gvt {
	struct intel_gvt *gvt;
};

But I wouldn't worry about encapsulation too much. You are only ever an
include away from us getting the innermost details of your structs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre