Re: [PATCH v4] drm/i915/gvt: Change KVMGT as self load module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 to adapt
> to the new interfaces. 
> But I have some comments embedded. 
> 

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 != INTEL_GVT_HYPERVISOR_KVM &&
> > > +	    m->type != 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 = m;
> > > +	intel_gvt_host.hypervisor_type = m->type;
> > > +	gvt = (void *)kdev_to_i915(intel_gvt_host.dev)->gvt;
> > > +
> > > +	ret = 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-initialize,
> Thus we can return the intel_gvt_ops in intel_gvt_register_hypervisor() and
> moduels initialize its rest part. 
> Current kvmgt_init-> intel_gvt_register_hypervisor->host_init seems a little bit
> redundant.
> But it's up to you to make the call to remove it in this patch or other furture
> 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. 
>

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.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux