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

[Zhi Wang]

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

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

> 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
>