[Intel-gfx] [PATCH 02/15] drm/i915/gvt: Fold vGPU active check into inner functions

[Wang, Zhi A]

Thanks for the comments. :)

-----Original Message-----
From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com] 
Sent: Monday, May 16, 2016 3:50 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>; intel-gfx at lists.freedesktop.org; Gordon, David S <david.s.gordon at intel.com>; joonas.lahtinen at linux.intel.com; Tian, Kevin <kevin.tian at intel.com>; Lv, Zhiyuan <zhiyuan.lv at intel.com>
Subject: Re: [Intel-gfx] [PATCH 02/15] drm/i915/gvt: Fold vGPU active check into inner functions


Hi,

On 15/05/16 18:32, Zhi Wang wrote:
> Fold vGPU related active check into the inner functions. (Kevin)
>
> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++-------
>   drivers/gpu/drm/i915/i915_vgpu.c    | 10 ++++++++--
>   drivers/gpu/drm/i915/i915_vgpu.h    |  2 +-
>   3 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7eab619..5c02c5a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2737,11 +2737,9 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>   	i915_address_space_init(&ggtt->base, dev_priv);
>   	ggtt->base.total += PAGE_SIZE;
>
> -	if (intel_vgpu_active(dev_priv)) {
> -		ret = intel_vgt_balloon(dev);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = intel_vgt_balloon(dev);
> +	if (ret)
> +		return ret;
>
>   	if (!HAS_LLC(dev))
>   		ggtt->base.mm.color_adjust = i915_gtt_color_adjust; @@ -2841,8 
> +2839,7 @@ void i915_ggtt_cleanup_hw(struct drm_device *dev)
>   	i915_gem_cleanup_stolen(dev);
>
>   	if (drm_mm_initialized(&ggtt->base.mm)) {
> -		if (intel_vgpu_active(dev_priv))
> -			intel_vgt_deballoon();
> +		intel_vgt_deballoon(dev);
>
>   		drm_mm_takedown(&ggtt->base.mm);
>   		list_del(&ggtt->base.global_link);
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c 
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index d5a7a5e..0b76c70 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -101,10 +101,13 @@ static struct _balloon_info_ bl_info;
>    * This function is called to deallocate the ballooned-out graphic memory, when
>    * driver is unloaded or when ballooning fails.
>    */
> -void intel_vgt_deballoon(void)
> +void intel_vgt_deballoon(struct drm_device *dev)

In the spirit of current driver cleanup you should make intel_vgt_balloon and deballoon take dev_priv. The former does not use dev for anything anyway, just to get to dev_priv.

>   {
>   	int i;
>
> +	if (!intel_vgpu_active(to_i915(dev)))
> +		return;
> +
>   	DRM_DEBUG("VGT deballoon.\n");
>
>   	for (i = 0; i < 4; i++) {
> @@ -187,6 +190,9 @@ int intel_vgt_balloon(struct drm_device *dev)
>   	unsigned long unmappable_base, unmappable_size, unmappable_end;
>   	int ret;
>
> +	if (!intel_vgpu_active(to_i915(dev)))
> +		return 0;

Even already has the dev_priv local variable.

> +
>   	mappable_base = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.base));
>   	mappable_size = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.size));
>   	unmappable_base = 
> I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.base));
> @@ -258,6 +264,6 @@ int intel_vgt_balloon(struct drm_device *dev)
>
>   err:
>   	DRM_ERROR("VGT balloon fail\n");
> -	intel_vgt_deballoon();
> +	intel_vgt_deballoon(dev);
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h 
> b/drivers/gpu/drm/i915/i915_vgpu.h
> index 07e67d5..c0e9569 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -28,6 +28,6 @@
>
>   extern void i915_check_vgpu(struct drm_i915_private *dev_priv);
>   extern int intel_vgt_balloon(struct drm_device *dev); -extern void 
> intel_vgt_deballoon(void);
> +extern void intel_vgt_deballoon(struct drm_device *dev);
>
>   #endif /* _I915_VGPU_H_ */
>

The rest looks good to me.

Regards,

Tvrtko