> 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. btw to match other description in the code, is it clear to rename pgt_device to gvt_device? 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
