[PATCH] drm/i915/gvt: refactor function intel_vgpu_submit_execlist

[Zhi Wang]

Actually code re-factor means no actual logical changes. If your code 
involves both re-factor and code logical changes, please split it into 
two patches. One for code re-factor, one for code changes.

It's very important for us, as the code refactor might cause conflict 
during merging code, if the guy who still wants the patch, he has to 
split the re-factor piece from the code manually. It's painful.

BTW: Is there any other reason that you removed the other two checks?

1) Guest submits a totally empty ELSP write. It won't cause problem in 
your patch, but we still want to know that happen. Please keep it.
2) Similar with the case above, we still want to know if guest doesn't 
write ELSP0. It indicates guest driver is not doing well and may help 
during debugging.

Thanks,
Zhi.

于 04/10/17 17:31, changbin.du at intel.com 写道:
> From: Changbin Du <changbin.du at intel.com>
>
> The function intel_vgpu_submit_execlist could be more simpler. It
> actually does:
>    1) validate the submission. The first context must be valid,
>       and all two must be privilege_access.
>    2) submit valid contexts. The first one need emulate schedule_in.
>
> We do not need a bitmap, valid desc copy valid_desc. Local variable
> emulate_schedule_in also can be optimized out.
>
> Signed-off-by: Changbin Du <changbin.du at intel.com>
> ---
>   drivers/gpu/drm/i915/gvt/execlist.c | 45 +++++++++++++------------------------
>   1 file changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
> index ce4276a..220b60e 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -710,52 +710,37 @@ static int submit_context(struct intel_vgpu *vgpu, int ring_id,
>   int intel_vgpu_submit_execlist(struct intel_vgpu *vgpu, int ring_id)
>   {
>   	struct intel_vgpu_execlist *execlist = &vgpu->execlist[ring_id];
> -	struct execlist_ctx_descriptor_format *desc[2], valid_desc[2];
> -	unsigned long valid_desc_bitmap = 0;
> -	bool emulate_schedule_in = true;
> -	int ret;
> -	int i;
> +	struct execlist_ctx_descriptor_format desc[2];
> +	int i, ret;
>   
> -	memset(valid_desc, 0, sizeof(valid_desc));
> +	desc[0] = *get_desc_from_elsp_dwords(&execlist->elsp_dwords, 1);
> +	desc[1] = *get_desc_from_elsp_dwords(&execlist->elsp_dwords, 0);
>   
> -	desc[0] = get_desc_from_elsp_dwords(&execlist->elsp_dwords, 1);
> -	desc[1] = get_desc_from_elsp_dwords(&execlist->elsp_dwords, 0);
> +	if (!desc[0].valid) {
> +		gvt_vgpu_err("invalid elsp submission, desc0 is not valid\n");
> +		return -EINVAL;
> +	}
>   
>   	for (i = 0; i < 2; i++) {
> -		if (!desc[i]->valid)
> +		if (!desc[i].valid)
>   			continue;
> -
> -		if (!desc[i]->privilege_access) {
> +		if (!desc[i].privilege_access) {
>   			gvt_vgpu_err("unexpected GGTT elsp submission\n");
>   			return -EINVAL;
>   		}
> -
> -		/* TODO: add another guest context checks here. */
> -		set_bit(i, &valid_desc_bitmap);
> -		valid_desc[i] = *desc[i];
> -	}
> -
> -	if (!valid_desc_bitmap) {
> -		gvt_vgpu_err("no valid desc in a elsp submission\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!test_bit(0, (void *)&valid_desc_bitmap) &&
> -			test_bit(1, (void *)&valid_desc_bitmap)) {
> -		gvt_vgpu_err("weird elsp submission, desc 0 is not valid\n");
> -		return -EINVAL;
>   	}
>   
>   	/* submit workload */
> -	for_each_set_bit(i, (void *)&valid_desc_bitmap, 2) {
> -		ret = submit_context(vgpu, ring_id, &valid_desc[i],
> -				emulate_schedule_in);
> +	for (i = 0; i < 2; i++) {
> +		if (!desc[i].valid)
> +			continue;
> +		ret = submit_context(vgpu, ring_id, &desc[i], i == 0);
>   		if (ret) {
>   			gvt_vgpu_err("fail to schedule workload\n");
>   			return ret;
>   		}
> -		emulate_schedule_in = false;
>   	}
> +
>   	return 0;
>   }
>