Re: [PATCH 02/14] drm/i915/gvt: simplify vgpu configuration management

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

 



On 2022.07.04 14:51:32 +0200, Christoph Hellwig wrote:
> Instead of copying the information from the vgpu_types arrays into each
> intel_vgpu_type structure, just reference this constant information
> with a pointer to the already existing data structure, and pass it into
> the low-level VGPU creation helpers intead of copying the data into yet
> anothe params data structure.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---

Looks fine to me. We still carry some legacy codes like vgpu create param
originally used for other hypervisor. Thanks for cleaning this up!

Reviewed-by: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>

>  drivers/gpu/drm/i915/gvt/aperture_gm.c |  20 +--
>  drivers/gpu/drm/i915/gvt/gvt.h         |  36 +++---
>  drivers/gpu/drm/i915/gvt/kvmgt.c       |  10 +-
>  drivers/gpu/drm/i915/gvt/vgpu.c        | 172 +++++++++----------------
>  4 files changed, 91 insertions(+), 147 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c b/drivers/gpu/drm/i915/gvt/aperture_gm.c
> index 557f3314291a8..7dd8163f8a569 100644
> --- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
> +++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
> @@ -240,13 +240,13 @@ static void free_resource(struct intel_vgpu *vgpu)
>  }
>  
>  static int alloc_resource(struct intel_vgpu *vgpu,
> -		struct intel_vgpu_creation_params *param)
> +		const struct intel_vgpu_config *conf)
>  {
>  	struct intel_gvt *gvt = vgpu->gvt;
>  	unsigned long request, avail, max, taken;
>  	const char *item;
>  
> -	if (!param->low_gm_sz || !param->high_gm_sz || !param->fence_sz) {
> +	if (!conf->low_mm || !conf->high_mm || !conf->fence) {
>  		gvt_vgpu_err("Invalid vGPU creation params\n");
>  		return -EINVAL;
>  	}
> @@ -255,7 +255,7 @@ static int alloc_resource(struct intel_vgpu *vgpu,
>  	max = gvt_aperture_sz(gvt) - HOST_LOW_GM_SIZE;
>  	taken = gvt->gm.vgpu_allocated_low_gm_size;
>  	avail = max - taken;
> -	request = MB_TO_BYTES(param->low_gm_sz);
> +	request = conf->low_mm;
>  
>  	if (request > avail)
>  		goto no_enough_resource;
> @@ -266,7 +266,7 @@ static int alloc_resource(struct intel_vgpu *vgpu,
>  	max = gvt_hidden_sz(gvt) - HOST_HIGH_GM_SIZE;
>  	taken = gvt->gm.vgpu_allocated_high_gm_size;
>  	avail = max - taken;
> -	request = MB_TO_BYTES(param->high_gm_sz);
> +	request = conf->high_mm;
>  
>  	if (request > avail)
>  		goto no_enough_resource;
> @@ -277,16 +277,16 @@ static int alloc_resource(struct intel_vgpu *vgpu,
>  	max = gvt_fence_sz(gvt) - HOST_FENCE;
>  	taken = gvt->fence.vgpu_allocated_fence_num;
>  	avail = max - taken;
> -	request = param->fence_sz;
> +	request = conf->fence;
>  
>  	if (request > avail)
>  		goto no_enough_resource;
>  
>  	vgpu_fence_sz(vgpu) = request;
>  
> -	gvt->gm.vgpu_allocated_low_gm_size += MB_TO_BYTES(param->low_gm_sz);
> -	gvt->gm.vgpu_allocated_high_gm_size += MB_TO_BYTES(param->high_gm_sz);
> -	gvt->fence.vgpu_allocated_fence_num += param->fence_sz;
> +	gvt->gm.vgpu_allocated_low_gm_size += conf->low_mm;
> +	gvt->gm.vgpu_allocated_high_gm_size += conf->high_mm;
> +	gvt->fence.vgpu_allocated_fence_num += conf->fence;
>  	return 0;
>  
>  no_enough_resource:
> @@ -340,11 +340,11 @@ void intel_vgpu_reset_resource(struct intel_vgpu *vgpu)
>   *
>   */
>  int intel_vgpu_alloc_resource(struct intel_vgpu *vgpu,
> -		struct intel_vgpu_creation_params *param)
> +		const struct intel_vgpu_config *conf)
>  {
>  	int ret;
>  
> -	ret = alloc_resource(vgpu, param);
> +	ret = alloc_resource(vgpu, conf);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index aee1a45da74bc..392c2ad49d376 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -295,15 +295,26 @@ struct intel_gvt_firmware {
>  	bool firmware_loaded;
>  };
>  
> +struct intel_vgpu_config {
> +	unsigned int low_mm;
> +	unsigned int high_mm;
> +	unsigned int fence;
> +
> +	/*
> +	 * A vGPU with a weight of 8 will get twice as much GPU as a vGPU with
> +	 * a weight of 4 on a contended host, different vGPU type has different
> +	 * weight set. Legal weights range from 1 to 16.
> +	 */
> +	unsigned int weight;
> +	enum intel_vgpu_edid edid;
> +	const char *name;
> +};
> +
>  #define NR_MAX_INTEL_VGPU_TYPES 20
>  struct intel_vgpu_type {
>  	char name[16];
> +	const struct intel_vgpu_config *conf;
>  	unsigned int avail_instance;
> -	unsigned int low_gm_size;
> -	unsigned int high_gm_size;
> -	unsigned int fence;
> -	unsigned int weight;
> -	enum intel_vgpu_edid resolution;
>  };
>  
>  struct intel_gvt {
> @@ -437,19 +448,8 @@ int intel_gvt_load_firmware(struct intel_gvt *gvt);
>  /* ring context size i.e. the first 0x50 dwords*/
>  #define RING_CTX_SIZE 320
>  
> -struct intel_vgpu_creation_params {
> -	__u64 low_gm_sz;  /* in MB */
> -	__u64 high_gm_sz; /* in MB */
> -	__u64 fence_sz;
> -	__u64 resolution;
> -	__s32 primary;
> -	__u64 vgpu_id;
> -
> -	__u32 weight;
> -};
> -
>  int intel_vgpu_alloc_resource(struct intel_vgpu *vgpu,
> -			      struct intel_vgpu_creation_params *param);
> +			      const struct intel_vgpu_config *conf);
>  void intel_vgpu_reset_resource(struct intel_vgpu *vgpu);
>  void intel_vgpu_free_resource(struct intel_vgpu *vgpu);
>  void intel_vgpu_write_fence(struct intel_vgpu *vgpu,
> @@ -496,7 +496,7 @@ void intel_gvt_clean_vgpu_types(struct intel_gvt *gvt);
>  struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt);
>  void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu);
>  struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> -					 struct intel_vgpu_type *type);
> +		const struct intel_vgpu_config *conf);
>  void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu);
>  void intel_gvt_release_vgpu(struct intel_vgpu *vgpu);
>  void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index e2f6c56ab3420..7b060c0e66ae7 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -151,10 +151,10 @@ static ssize_t description_show(struct mdev_type *mtype,
>  	return sprintf(buf, "low_gm_size: %dMB\nhigh_gm_size: %dMB\n"
>  		       "fence: %d\nresolution: %s\n"
>  		       "weight: %d\n",
> -		       BYTES_TO_MB(type->low_gm_size),
> -		       BYTES_TO_MB(type->high_gm_size),
> -		       type->fence, vgpu_edid_str(type->resolution),
> -		       type->weight);
> +		       BYTES_TO_MB(type->conf->low_mm),
> +		       BYTES_TO_MB(type->conf->high_mm),
> +		       type->conf->fence, vgpu_edid_str(type->conf->edid),
> +		       type->conf->weight);
>  }
>  
>  static ssize_t name_show(struct mdev_type *mtype,
> @@ -1624,7 +1624,7 @@ static int intel_vgpu_probe(struct mdev_device *mdev)
>  	if (!type)
>  		return -EINVAL;
>  
> -	vgpu = intel_gvt_create_vgpu(gvt, type);
> +	vgpu = intel_gvt_create_vgpu(gvt, type->conf);
>  	if (IS_ERR(vgpu)) {
>  		gvt_err("failed to create intel vgpu: %ld\n", PTR_ERR(vgpu));
>  		return PTR_ERR(vgpu);
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 5c828556cefd7..8e136dcc70112 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -73,24 +73,21 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
>  	drm_WARN_ON(&i915->drm, sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>  }
>  
> +/*
> + * vGPU type name is defined as GVTg_Vx_y which contains the physical GPU
> + * generation type (e.g V4 as BDW server, V5 as SKL server).
> + *
> + * Depening on the physical SKU resource, we might see vGPU types like
> + * GVTg_V4_8, GVTg_V4_4, GVTg_V4_2, etc. We can create different types of
> + * vGPU on same physical GPU depending on available resource. Each vGPU
> + * type will have a different number of avail_instance to indicate how
> + * many vGPU instance can be created for this type.
> + */
>  #define VGPU_MAX_WEIGHT 16
>  #define VGPU_WEIGHT(vgpu_num)	\
>  	(VGPU_MAX_WEIGHT / (vgpu_num))
>  
> -static const struct {
> -	unsigned int low_mm;
> -	unsigned int high_mm;
> -	unsigned int fence;
> -
> -	/* A vGPU with a weight of 8 will get twice as much GPU as a vGPU
> -	 * with a weight of 4 on a contended host, different vGPU type has
> -	 * different weight set. Legal weights range from 1 to 16.
> -	 */
> -	unsigned int weight;
> -	enum intel_vgpu_edid edid;
> -	const char *name;
> -} vgpu_types[] = {
> -/* Fixed vGPU type table */
> +static const struct intel_vgpu_config intel_vgpu_configs[] = {
>  	{ MB_TO_BYTES(64), MB_TO_BYTES(384), 4, VGPU_WEIGHT(8), GVT_EDID_1024_768, "8" },
>  	{ MB_TO_BYTES(128), MB_TO_BYTES(512), 4, VGPU_WEIGHT(4), GVT_EDID_1920_1200, "4" },
>  	{ MB_TO_BYTES(256), MB_TO_BYTES(1024), 4, VGPU_WEIGHT(2), GVT_EDID_1920_1200, "2" },
> @@ -106,63 +103,34 @@ static const struct {
>   */
>  int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
>  {
> -	unsigned int num_types;
> -	unsigned int i, low_avail, high_avail;
> -	unsigned int min_low;
> -
> -	/* vGPU type name is defined as GVTg_Vx_y which contains
> -	 * physical GPU generation type (e.g V4 as BDW server, V5 as
> -	 * SKL server).
> -	 *
> -	 * Depend on physical SKU resource, might see vGPU types like
> -	 * GVTg_V4_8, GVTg_V4_4, GVTg_V4_2, etc. We can create
> -	 * different types of vGPU on same physical GPU depending on
> -	 * available resource. Each vGPU type will have "avail_instance"
> -	 * to indicate how many vGPU instance can be created for this
> -	 * type.
> -	 *
> -	 */
> -	low_avail = gvt_aperture_sz(gvt) - HOST_LOW_GM_SIZE;
> -	high_avail = gvt_hidden_sz(gvt) - HOST_HIGH_GM_SIZE;
> -	num_types = ARRAY_SIZE(vgpu_types);
> +	unsigned int low_avail = gvt_aperture_sz(gvt) - HOST_LOW_GM_SIZE;
> +	unsigned int high_avail = gvt_hidden_sz(gvt) - HOST_HIGH_GM_SIZE;
> +	unsigned int num_types = ARRAY_SIZE(intel_vgpu_configs);
> +	unsigned int i;
>  
>  	gvt->types = kcalloc(num_types, sizeof(struct intel_vgpu_type),
>  			     GFP_KERNEL);
>  	if (!gvt->types)
>  		return -ENOMEM;
>  
> -	min_low = MB_TO_BYTES(32);
>  	for (i = 0; i < num_types; ++i) {
> -		if (low_avail / vgpu_types[i].low_mm == 0)
> -			break;
> -
> -		gvt->types[i].low_gm_size = vgpu_types[i].low_mm;
> -		gvt->types[i].high_gm_size = vgpu_types[i].high_mm;
> -		gvt->types[i].fence = vgpu_types[i].fence;
> +		const struct intel_vgpu_config *conf = &intel_vgpu_configs[i];
>  
> -		if (vgpu_types[i].weight < 1 ||
> -					vgpu_types[i].weight > VGPU_MAX_WEIGHT)
> +		if (low_avail / conf->low_mm == 0)
> +			break;
> +		if (conf->weight < 1 || conf->weight > VGPU_MAX_WEIGHT)
>  			goto out_free_types;
>  
> -		gvt->types[i].weight = vgpu_types[i].weight;
> -		gvt->types[i].resolution = vgpu_types[i].edid;
> -		gvt->types[i].avail_instance = min(low_avail / vgpu_types[i].low_mm,
> -						   high_avail / vgpu_types[i].high_mm);
> -
> -		if (GRAPHICS_VER(gvt->gt->i915) == 8)
> -			sprintf(gvt->types[i].name, "GVTg_V4_%s",
> -				vgpu_types[i].name);
> -		else if (GRAPHICS_VER(gvt->gt->i915) == 9)
> -			sprintf(gvt->types[i].name, "GVTg_V5_%s",
> -				vgpu_types[i].name);
> +		sprintf(gvt->types[i].name, "GVTg_V%u_%s",
> +			GRAPHICS_VER(gvt->gt->i915) == 8 ? 4 : 5, conf->name);
> +		gvt->types->conf = conf;
> +		gvt->types[i].avail_instance = min(low_avail / conf->low_mm,
> +						   high_avail / conf->high_mm);
>  
>  		gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
> -			     i, gvt->types[i].name,
> -			     gvt->types[i].avail_instance,
> -			     gvt->types[i].low_gm_size,
> -			     gvt->types[i].high_gm_size, gvt->types[i].fence,
> -			     gvt->types[i].weight,
> -			     vgpu_edid_str(gvt->types[i].resolution));
> +			     i, gvt->types[i].name, gvt->types[i].avail_instance,
> +			     conf->low_mm, conf->high_mm, conf->fence,
> +			     conf->weight, vgpu_edid_str(conf->edid));
>  	}
>  
>  	gvt->num_types = i;
> @@ -195,16 +163,16 @@ static void intel_gvt_update_vgpu_types(struct intel_gvt *gvt)
>  		gvt->fence.vgpu_allocated_fence_num;
>  
>  	for (i = 0; i < gvt->num_types; i++) {
> -		low_gm_min = low_gm_avail / gvt->types[i].low_gm_size;
> -		high_gm_min = high_gm_avail / gvt->types[i].high_gm_size;
> -		fence_min = fence_avail / gvt->types[i].fence;
> +		low_gm_min = low_gm_avail / gvt->types[i].conf->low_mm;
> +		high_gm_min = high_gm_avail / gvt->types[i].conf->high_mm;
> +		fence_min = fence_avail / gvt->types[i].conf->fence;
>  		gvt->types[i].avail_instance = min(min(low_gm_min, high_gm_min),
>  						   fence_min);
>  
>  		gvt_dbg_core("update type[%d]: %s avail %u low %u high %u fence %u\n",
>  		       i, gvt->types[i].name,
> -		       gvt->types[i].avail_instance, gvt->types[i].low_gm_size,
> -		       gvt->types[i].high_gm_size, gvt->types[i].fence);
> +		       gvt->types[i].avail_instance, gvt->types[i].conf->low_mm,
> +		       gvt->types[i].conf->high_mm, gvt->types[i].conf->fence);
>  	}
>  }
>  
> @@ -367,42 +335,53 @@ void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu)
>  	vfree(vgpu);
>  }
>  
> -static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
> -		struct intel_vgpu_creation_params *param)
> +/**
> + * intel_gvt_create_vgpu - create a virtual GPU
> + * @gvt: GVT device
> + * @conf: type of the vGPU to create
> + *
> + * This function is called when user wants to create a virtual GPU.
> + *
> + * Returns:
> + * pointer to intel_vgpu, error pointer if failed.
> + */
> +struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> +		const struct intel_vgpu_config *conf)
>  {
>  	struct drm_i915_private *dev_priv = gvt->gt->i915;
>  	struct intel_vgpu *vgpu;
>  	int ret;
>  
> -	gvt_dbg_core("low %llu MB high %llu MB fence %llu\n",
> -			param->low_gm_sz, param->high_gm_sz,
> -			param->fence_sz);
> +	gvt_dbg_core("low %u MB high %u MB fence %u\n",
> +			BYTES_TO_MB(conf->low_mm), BYTES_TO_MB(conf->high_mm),
> +			conf->fence);
>  
>  	vgpu = vzalloc(sizeof(*vgpu));
>  	if (!vgpu)
>  		return ERR_PTR(-ENOMEM);
>  
> +	mutex_lock(&gvt->lock);
>  	ret = idr_alloc(&gvt->vgpu_idr, vgpu, IDLE_VGPU_IDR + 1, GVT_MAX_VGPU,
>  		GFP_KERNEL);
>  	if (ret < 0)
> -		goto out_free_vgpu;
> +		goto out_unlock;;
>  
>  	vgpu->id = ret;
>  	vgpu->gvt = gvt;
> -	vgpu->sched_ctl.weight = param->weight;
> +	vgpu->sched_ctl.weight = conf->weight;
>  	mutex_init(&vgpu->vgpu_lock);
>  	mutex_init(&vgpu->dmabuf_lock);
>  	INIT_LIST_HEAD(&vgpu->dmabuf_obj_list_head);
>  	INIT_RADIX_TREE(&vgpu->page_track_tree, GFP_KERNEL);
>  	idr_init_base(&vgpu->object_idr, 1);
> -	intel_vgpu_init_cfg_space(vgpu, param->primary);
> +	intel_vgpu_init_cfg_space(vgpu, 1);
>  	vgpu->d3_entered = false;
>  
>  	ret = intel_vgpu_init_mmio(vgpu);
>  	if (ret)
>  		goto out_clean_idr;
>  
> -	ret = intel_vgpu_alloc_resource(vgpu, param);
> +	ret = intel_vgpu_alloc_resource(vgpu, conf);
>  	if (ret)
>  		goto out_clean_vgpu_mmio;
>  
> @@ -416,7 +395,7 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>  	if (ret)
>  		goto out_clean_gtt;
>  
> -	ret = intel_vgpu_init_display(vgpu, param->resolution);
> +	ret = intel_vgpu_init_display(vgpu, conf->edid);
>  	if (ret)
>  		goto out_clean_opregion;
>  
> @@ -441,6 +420,9 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>  	if (ret)
>  		goto out_clean_sched_policy;
>  
> +	intel_gvt_update_vgpu_types(gvt);
> +	intel_gvt_update_reg_whitelist(vgpu);
> +	mutex_unlock(&gvt->lock);
>  	return vgpu;
>  
>  out_clean_sched_policy:
> @@ -459,50 +441,12 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>  	intel_vgpu_clean_mmio(vgpu);
>  out_clean_idr:
>  	idr_remove(&gvt->vgpu_idr, vgpu->id);
> -out_free_vgpu:
> +out_unlock:
> +	mutex_unlock(&gvt->lock);
>  	vfree(vgpu);
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * intel_gvt_create_vgpu - create a virtual GPU
> - * @gvt: GVT device
> - * @type: type of the vGPU to create
> - *
> - * This function is called when user wants to create a virtual GPU.
> - *
> - * Returns:
> - * pointer to intel_vgpu, error pointer if failed.
> - */
> -struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> -				struct intel_vgpu_type *type)
> -{
> -	struct intel_vgpu_creation_params param;
> -	struct intel_vgpu *vgpu;
> -
> -	param.primary = 1;
> -	param.low_gm_sz = type->low_gm_size;
> -	param.high_gm_sz = type->high_gm_size;
> -	param.fence_sz = type->fence;
> -	param.weight = type->weight;
> -	param.resolution = type->resolution;
> -
> -	/* XXX current param based on MB */
> -	param.low_gm_sz = BYTES_TO_MB(param.low_gm_sz);
> -	param.high_gm_sz = BYTES_TO_MB(param.high_gm_sz);
> -
> -	mutex_lock(&gvt->lock);
> -	vgpu = __intel_gvt_create_vgpu(gvt, &param);
> -	if (!IS_ERR(vgpu)) {
> -		/* calculate left instance change for types */
> -		intel_gvt_update_vgpu_types(gvt);
> -		intel_gvt_update_reg_whitelist(vgpu);
> -	}
> -	mutex_unlock(&gvt->lock);
> -
> -	return vgpu;
> -}
> -
>  /**
>   * intel_gvt_reset_vgpu_locked - reset a virtual GPU by DMLR or GT reset
>   * @vgpu: virtual GPU
> -- 
> 2.30.2
> 

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux