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

[Du, Changbin]

On Wed, Apr 26, 2017 at 04:54:39PM +0800, Zhi Wang wrote:
> 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.
>
Partially agree. Refactor is improving the code readability/structure for sanity,
which will be better to maintain. Re-factor can change or even remove some
logical(code) but it just cannot change the essential behaviour as origin code
intent to. We can implement same behaviour by different code. 

For this one, improved code and origin code actually has same output for same 
input. So it is a true refactor.
I think you are meaning to split bug fixing/new feature from refactor(improvment).

> 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.
> 
I do not quite understand. is it patch backporting? If so, this patch is
not needed. If it is  merging new :) patch, the new patch should based on last
kernel base. :)

> 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.
ok, then the two desc can be dumped in error msg.

> 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.
> 
Does above one can give the debugging info?

> 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;
> >  }
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Thanks,
Changbin Du
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20170428/f4294874/attachment-0001.sig>