Re: [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Zhi Wang <zhi.a.wang@intel.com>,
	"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 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g
Date: Fri, 26 Feb 2016 15:54:14 +0200	[thread overview]
Message-ID: <1456494854.7316.15.camel@linux.intel.com> (raw)
In-Reply-To: <56CFE0EF.8090108@intel.com>

Hi,

On pe, 2016-02-26 at 13:21 +0800, Zhi Wang wrote:
> 
> On 02/24/16 15:42, Tian, Kevin wrote:
> > 
> > > 
> > > From: Wang, Zhi A
> > > Sent: Tuesday, February 23, 2016 9:23 PM
> > > > 
> > > > > 
> > > > > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > > > > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > > > > @@ -348,6 +348,10 @@ void *gvt_create_pgt_device(struct drm_i915_private
> > > *dev_priv)
> > > > 
> > > > > 
> > > > >    		goto err;
> > > > >    	}
> > > > > 
> > > > > +	dev_priv->gvt.host_fence_sz = gvt.host_fence_sz;
> > > > > +	dev_priv->gvt.host_low_gm_sz_in_mb = gvt.host_low_gm_sz;
> > > > > +	dev_priv->gvt.host_high_gm_sz_in_mb = gvt.host_high_gm_sz;
> > > > I'm thinking, could we expose the pgt_device struct (at least
> > > > partially, and then have a PIMPL pattern), to avoid this kind of
> > > > duplication of declarations and unnecessary copies between i915 and
> > > > i915_gvt modules?
> > > > 
> > > > It's little rough that the gvt driver writes to i915_private struct.
> > > > I'd rather see that gvt.host_fence_sz and other variables get sanitized
> > > > and then written to pgt_device (maybe the public part would be
> > > > i915_pgt_device) and used by gvt and i915 code.
> > > > 
> > > > Was this ever considered?
> > > > 
> > > The previous version do something similar like that, both i915 and gvt
> > > reads GVT module kernel parameter but considered that GVT modules could
> > > be configured as "n" in kernel configuration, probably we should let
> > > i915 to store this information and GVT to configure it if GVT is active?
> > Agree with Joonas. We don't need another gvt wrap. Let's just expose
> > pgt_device directly. I believe all other information can be encapsulated
> > under pgt_device.
> > 
> How about this scheme:
> 
> 1. Move GVT kernel parameter into intel_gvt.{h, c}
> 2. Sanitize the partition configuration for host in intel_gvt.c
> 3. If CONFIG_DRM_I915_GVT = y, write the configuration into pgt_device 
> to inform GVT resource allocator ranges owned by host
> 

This sounds fine, if i915 driver wants to know about the gvt driver, it
will read its structure (if gvt was enabled), instead of gvt driver
pushing information to i915.

> > 
> > btw to match other description in the code, is it clear to rename pgt_device
> > to gvt_device?
> > 
> For the name of GVT physical device, if we use "gvt_device", it looks a 
> bit weird when both "gvt_device" and "vgt_device"(vGPU instance) 
> appeared in our code? :( And "pgpu" and "vgpu" also looks weird...
> 

The naming conventions are indeed very confusing, it would help if all
host related stuff was named gvt_* and client vgpu_*

Regards, Joonas

> > 
> > Another minor thing needs Joonas' feedback. Is it usual to capture
> > all module parameters belonging to one feature structurized together
> > (like 'gvt' in this patch), or just to leave them directly exposed?
> > 
> 
> > 
> > Thanks
> > Kevin
> > 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx