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

[Zhi Wang]

On 02/24/16 16:22, Tian, Kevin wrote:
>> From: Wang, Zhi A
>> Sent: Thursday, February 18, 2016 7:42 PM
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
>> index 52cfa32..2099b7e 100644
>> --- 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;
>
> Do we need hard limiting fence number for host usage here? There is no
> continuity requirement as seen for graphics memory, since we do translate
> fence# between guest view and host view. So we could make it flexible
> as an on-demand allocation when creating a vGPU. Daniel even mentioned
> , iirc, that today i915 can dynamically grab a fence register away from
> an application, which could be useful even when host fence usage is high
> (not a typical case in server virtualization which runs few applications in host).
>
Yes. They steal fence in aperture mapping fault handler. We could steal 
the fence registers from i915 as well. Let me see the effort here. :)

>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 9127f8f..de09dd4 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2734,7 +2734,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>>   	i915_address_space_init(ggtt_vm, dev_priv);
>>   	ggtt_vm->total += PAGE_SIZE;
>>
>> -	if (intel_vgpu_active(dev)) {
>> +	if (intel_vgpu_active(dev) || intel_gvt_active(dev)) {
>
> above two conditions are bit confusing for others not familiar with
> this technology. vgpu_active is for driver running in a VM, while
> gvt_active is for driver running in host. Could we introduce a better
> name, or at least wrap them into a more meaningful macro like
> intel_ballooning_required?
>
>>   		ret = intel_vgt_balloon(dev);
>
> I saw several comments whether ballooning is a right term here,
> since we only do static reservation so far. How about renaming it
> to intel_reserve_gm_resource to be more clear? In the future even
> when we want to add true dynamic ballooning feature, it will be
> largely refactored anyway. :-)
>
Sure, will do that.
> Thanks
> Kevin
>