Re: [PATCH v7 05/11] drm/i915: gvt: Introduce the basic architecture of GVT-g

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Zhi Wang <zhi.a.wang@intel.com>,
	chris@chris-wilson.co.uk, zhiyuan.lv@intel.com,
	kevin.tian@intel.com, tvrtko.ursulin@linux.intel.com,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v7 05/11] drm/i915: gvt: Introduce the basic architecture of GVT-g
Date: Wed, 08 Jun 2016 11:28:01 +0300	[thread overview]
Message-ID: <1465374481.5803.18.camel@linux.intel.com> (raw)
In-Reply-To: <1465312727-2211-6-git-send-email-zhi.a.wang@intel.com>

On ti, 2016-06-07 at 11:18 -0400, Zhi Wang wrote:
> This patch introduces the very basic framework of GVT-g device model,
> includes basic prototypes, definitions, initialization.
> 
> v7:
> - Refine the URL link in Kconfig. (Joonas)
> - Refine the introduction of GVT-g host support in Kconfig. (Joonas)
> - Remove the macro GVT_ALIGN(), use round_down() instead. (Joonas)
> - Make "struct intel_gvt" a data member in struct drm_i915_private.(Joonas)
> 	- Remove {alloc, free}_gvt_device()
> 	- Rename intel_gvt_{create, destroy}_gvt_device()
> 	- Expost intel_gvt_init_host()
> - Remove the dummy "struct intel_gvt" declaration in intel_gvt.h (Joonas)
> 
> v6:
> - Refine introduction in Kconfig. (Chris)
> - The exposed API functions will take struct intel_gvt * instead of
> void *. (Chris/Tvrtko)
> - Remove most memebers of strct intel_gvt_device_info. Will add them
> in the device model patches.(Chris)
> - Remove gvt_info() and gvt_err() in debug.h. (Chris)
> - Move GVT kernel parameter into i915_params. (Chris)
> - Remove include/drm/i915_gvt.h, as GVT-g will be built within i915.
> - Remove the redundant struct i915_gvt *, as the functions in i915
> will directly take struct intel_gvt *.
> - Add more comments for reviewer.
> 
> v5:
> Take Tvrtko's comments:
> - Fix the misspelled words in Kconfig
> - Let functions take drm_i915_private * instead of struct drm_device *
> - Remove redundant prints/local varible initialization
> 
> v3:
> Take Joonas' comments:
> - Change file name i915_gvt.* to intel_gvt.*
> - Move GVT kernel parameter into intel_gvt.c
> - Remove redundant debug macros
> - Change error handling style
> - Add introductions for some stub functions
> - Introduce drm/i915_gvt.h.
> 
> Take Kevin's comments:
> - Move GVT-g host/guest check into intel_vgt_balloon in i915_gem_gtt.c
> 
> v2:
> - Introduce i915_gvt.c.
> It's necessary to introduce the stubs between i915 driver and GVT-g host,
> as GVT-g components is configurable in kernel config. When disabled, the
> stubs here do nothing.
> 
> Take Joonas' comments:
> - Replace boolean return value with int.
> - Replace customized info/warn/debug macros with DRM macros.
> - Document all non-static functions like i915.
> - Remove empty and unused functions.
> - Replace magic number with marcos.
> - Set GVT-g in kernel config to "n" by default.
> 
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

Again, add Cc: everyone who has given comments.

> ---
>  drivers/gpu/drm/i915/Kconfig         |  22 +++++
>  drivers/gpu/drm/i915/Makefile        |   5 ++
>  drivers/gpu/drm/i915/gvt/Makefile    |   5 ++
>  drivers/gpu/drm/i915/gvt/debug.h     |  34 ++++++++
>  drivers/gpu/drm/i915/gvt/gvt.c       | 158 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h       |  72 ++++++++++++++++
>  drivers/gpu/drm/i915/gvt/hypercall.h |  38 +++++++++
>  drivers/gpu/drm/i915/gvt/mpt.h       |  49 +++++++++++
>  drivers/gpu/drm/i915/i915_dma.c      |  16 +++-
>  drivers/gpu/drm/i915/i915_drv.h      |  10 +++
>  drivers/gpu/drm/i915/i915_params.c   |   5 ++
>  drivers/gpu/drm/i915/i915_params.h   |   1 +
>  drivers/gpu/drm/i915/intel_gvt.c     | 100 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_gvt.h     |  45 ++++++++++
>  14 files changed, 556 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
>  create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
>  create mode 100644 drivers/gpu/drm/i915/intel_gvt.c
>  create mode 100644 drivers/gpu/drm/i915/intel_gvt.h
> 
> +/**
> + * intel_gvt_clean_device - clean a GVT device
> + * @gvt: intel gvt device
> + *
> + * This function is called at the driver unloading stage, to free the
> + * resources owned by a GVT device.
> + *
> + */
> +void intel_gvt_clean_device(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_gvt *gvt = &dev_priv->gvt;
> +
> +	if (WARN_ON(!gvt->initialized))
> +		return;
> +
> +	mutex_lock(&intel_gvt_host.gvt_idr_lock);

Why is this lock necessary? I assume intel_gvt_init will be called on
driver load (same for fini part), and there should be no races.

With that explained or removed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> +	idr_remove(&intel_gvt_host.gvt_idr, gvt->id);
> +	mutex_unlock(&intel_gvt_host.gvt_idr_lock);
> +	/* Other de-initialization of GVT components will be introduced. */
> +
> +	gvt->initialized = false;
> +}
> +
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx