From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhenyu Wang Subject: Re: [PATCH v4] drm/i915/gvt: Change KVMGT as self load module Date: Fri, 7 Dec 2018 15:50:28 +0800 Message-ID: <20181207075028.GL12743@zhen-hp.sh.intel.com> References: <20181203040550.5171-4-zhenyuw@linux.intel.com> <20181204024028.11311-1-zhenyuw@linux.intel.com> <20181206042759.GD12743@zhen-hp.sh.intel.com> Reply-To: Zhenyu Wang Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1554948960==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "He, Min" Cc: "intel-gfx@lists.freedesktop.org" , "Yuan, Hang" , "intel-gvt-dev@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org --===============1554948960== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="JZo5qZ4+1n7nJwOr" Content-Disposition: inline --JZo5qZ4+1n7nJwOr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 2018.12.07 07:47:59 +0000, He, Min wrote: > Zhenyu, > Overall I think the impact to AcrnGT is not big, we can modify our code t= o adapt > to the new interfaces.=20 > But I have some comments embedded.=20 >=20 Good! > > > @@ -467,6 +408,44 @@ int intel_gvt_init_device(struct drm_i915_private > > *dev_priv) > > > return ret; > > > } > > > > > > -#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT) > > > -MODULE_SOFTDEP("pre: kvmgt"); > > > -#endif > > > +int > > > +intel_gvt_register_hypervisor(struct intel_gvt_mpt *m) > > > +{ > > > + int ret; > > > + void *gvt; > > > + > > > + if (!intel_gvt_host.initialized) > > > + return -ENODEV; > > > + > > > + if (m->type !=3D INTEL_GVT_HYPERVISOR_KVM && > > > + m->type !=3D INTEL_GVT_HYPERVISOR_XEN) > > > + return -EINVAL; > > > + > > > + /* Get a reference for device model module */ > > > + if (!try_module_get(THIS_MODULE)) > > > + return -ENODEV; > > > + > > > + intel_gvt_host.mpt =3D m; > > > + intel_gvt_host.hypervisor_type =3D m->type; > > > + gvt =3D (void *)kdev_to_i915(intel_gvt_host.dev)->gvt; > > > + > > > + ret =3D intel_gvt_hypervisor_host_init(intel_gvt_host.dev, gvt, > > > + &intel_gvt_ops); > I think we can remove the host_init and host_exit interfaces, since now > it's mpt modules who proactively call the GVT-g to initialize and un-init= ialize, > Thus we can return the intel_gvt_ops in intel_gvt_register_hypervisor() a= nd > moduels initialize its rest part.=20 > Current kvmgt_init-> intel_gvt_register_hypervisor->host_init seems a lit= tle bit > redundant. > But it's up to you to make the call to remove it in this patch or other f= urture > optimization patches. > I'd like to keep as in current approach, as it can keep hypervisor init function in one place instead of passing gvt host info to hypervisor module. And calling submodule's init function is fine process I think. > > > @@ -67,6 +73,5 @@ struct intel_gvt_mpt { > > > }; > > > > > > extern struct intel_gvt_mpt xengt_mpt; > We can remove this definition, too.=20 > yeah, but maybe in another xengt cleanup patch. > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c > > b/drivers/gpu/drm/i915/gvt/kvmgt.c > > > index 1bbd04d30c42..ef248d577e49 100644 > > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > > > @@ -50,6 +50,7 @@ > > > #include "gvt.h" > > > > > > static const struct intel_gvt_ops *intel_gvt_ops; > > > +static struct intel_gvt *intel_gvt; > This variable intel_gvt seems useless. > Thanks for pointing this out, was trying to use for exit path, but looks I missed to remove it finally. --=20 Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 --JZo5qZ4+1n7nJwOr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQTXuabgHDW6LPt9CICxBBozTXgYJwUCXAomRAAKCRCxBBozTXgY JxDZAKCG2s7vy6qC5IcTthhqe7UYZT3YcgCeNASYdsaaglySYV4vncUPvk8XyVU= =8GcP -----END PGP SIGNATURE----- --JZo5qZ4+1n7nJwOr-- --===============1554948960== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1554948960==--