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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
