* [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+
@ 2018-07-03 2:56 Zhao Yakui
2018-07-03 2:56 ` [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg " Zhao Yakui
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Zhao Yakui @ 2018-07-03 2:56 UTC (permalink / raw)
To: intel-gfx
V1->V2: Fix one typo error.
Zhao Yakui (2):
drm/i915: Use 64-bit to Read/Write fence reg on SNB+
drm/i915: write fence reg only once on VGPU
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg on SNB+ 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui @ 2018-07-03 2:56 ` Zhao Yakui 2018-07-03 8:49 ` Daniel Vetter 2018-07-03 2:56 ` [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU Zhao Yakui ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Zhao Yakui @ 2018-07-03 2:56 UTC (permalink / raw) To: intel-gfx Based on HW spec the fence reg on SNB+ is defined as 64-bit. Just follow the b-spec to use 64-bit read/write mode. Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> --- drivers/gpu/drm/i915/i915_gem_fence_reg.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c index d548ac0..d92fe03 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c @@ -63,6 +63,7 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, i915_reg_t fence_reg_lo, fence_reg_hi; int fence_pitch_shift; u64 val; + struct drm_i915_private *dev_priv = fence->i915; if (INTEL_GEN(fence->i915) >= 6) { fence_reg_lo = FENCE_REG_GEN6_LO(fence->id); @@ -92,9 +93,14 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, val |= I965_FENCE_REG_VALID; } - if (!pipelined) { - struct drm_i915_private *dev_priv = fence->i915; + if (INTEL_GEN(fence->i915) >= 6) { + /* Use the 64-bit RW to read/write fence reg on SNB+ */ + I915_WRITE64_FW(fence_reg_lo, 0); + I915_READ64(fence_reg_lo); + I915_WRITE64_FW(fence_reg_lo, val); + I915_READ64(fence_reg_lo); + } else { /* To w/a incoherency with non-atomic 64-bit register updates, * we split the 64-bit update into two 32-bit writes. In order * for a partial fence not to be evaluated between writes, we -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg on SNB+ 2018-07-03 2:56 ` [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg " Zhao Yakui @ 2018-07-03 8:49 ` Daniel Vetter 2018-07-03 9:01 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2018-07-03 8:49 UTC (permalink / raw) To: Zhao Yakui; +Cc: intel-gfx On Tue, Jul 03, 2018 at 10:56:16AM +0800, Zhao Yakui wrote: > Based on HW spec the fence reg on SNB+ is defined as 64-bit. Just follow > the b-spec to use 64-bit read/write mode. > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> Please use git blame to understand why you've just re-introduced a bug that took months to debug. -Daniel > --- > drivers/gpu/drm/i915/i915_gem_fence_reg.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > index d548ac0..d92fe03 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > @@ -63,6 +63,7 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, > i915_reg_t fence_reg_lo, fence_reg_hi; > int fence_pitch_shift; > u64 val; > + struct drm_i915_private *dev_priv = fence->i915; > > if (INTEL_GEN(fence->i915) >= 6) { > fence_reg_lo = FENCE_REG_GEN6_LO(fence->id); > @@ -92,9 +93,14 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, > val |= I965_FENCE_REG_VALID; > } > > - if (!pipelined) { > - struct drm_i915_private *dev_priv = fence->i915; > + if (INTEL_GEN(fence->i915) >= 6) { > + /* Use the 64-bit RW to read/write fence reg on SNB+ */ > + I915_WRITE64_FW(fence_reg_lo, 0); > + I915_READ64(fence_reg_lo); > > + I915_WRITE64_FW(fence_reg_lo, val); > + I915_READ64(fence_reg_lo); > + } else { > /* To w/a incoherency with non-atomic 64-bit register updates, > * we split the 64-bit update into two 32-bit writes. In order > * for a partial fence not to be evaluated between writes, we > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg on SNB+ 2018-07-03 8:49 ` Daniel Vetter @ 2018-07-03 9:01 ` Chris Wilson 2018-07-03 10:11 ` Zhao, Yakui 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2018-07-03 9:01 UTC (permalink / raw) To: Daniel Vetter, Zhao Yakui; +Cc: intel-gfx Quoting Daniel Vetter (2018-07-03 09:49:29) > On Tue, Jul 03, 2018 at 10:56:16AM +0800, Zhao Yakui wrote: > > Based on HW spec the fence reg on SNB+ is defined as 64-bit. Just follow > > the b-spec to use 64-bit read/write mode. > > > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > > Please use git blame to understand why you've just re-introduced a bug > that took months to debug. And there's even a very nice comment explaining exactly what the HW does and why the double write is required. First rule of IT: turn if off and on again. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg on SNB+ 2018-07-03 9:01 ` Chris Wilson @ 2018-07-03 10:11 ` Zhao, Yakui 0 siblings, 0 replies; 19+ messages in thread From: Zhao, Yakui @ 2018-07-03 10:11 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Tuesday, July 3, 2018 5:01 PM >To: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui <yakui.zhao@intel.com> >Cc: intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write >fence reg on SNB+ > >Quoting Daniel Vetter (2018-07-03 09:49:29) >> On Tue, Jul 03, 2018 at 10:56:16AM +0800, Zhao Yakui wrote: >> > Based on HW spec the fence reg on SNB+ is defined as 64-bit. Just >> > follow the b-spec to use 64-bit read/write mode. >> > >> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> >> >> Please use git blame to understand why you've just re-introduced a bug >> that took months to debug. > >And there's even a very nice comment explaining exactly what the HW does >and why the double write is required. > >First rule of IT: turn if off and on again. Hi, Chris/Daniel Thanks for the detailed explanation. I check the history of this issue. It was one commit about five years ago. Maybe the op of fence reg on HW doesn't follow its description very strictly. Not sure whether it is changed on the latest HW. OK. Please ignore this patch as the double write is safer. >-Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui 2018-07-03 2:56 ` [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg " Zhao Yakui @ 2018-07-03 2:56 ` Zhao Yakui 2018-07-03 8:51 ` Daniel Vetter 2018-07-03 3:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Optimize the read/write fence_reg on SNB+ Patchwork ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Zhao Yakui @ 2018-07-03 2:56 UTC (permalink / raw) To: intel-gfx On VGPU scenario the read/write operation of fence_reg will be trapped by the GVT-g. And then gvt-g follows the HW spec to write the fence_reg. So it is unnecessary to read/write fence reg several times. This will help to reduce the unnecessary trap of fence_reg mmio operation. V1->V2: Fix one typo error of parameter when calling intel_vgpu_active Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> --- drivers/gpu/drm/i915/i915_gem_fence_reg.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c index d92fe03..9c97976 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c @@ -95,11 +95,15 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, if (INTEL_GEN(fence->i915) >= 6) { /* Use the 64-bit RW to read/write fence reg on SNB+ */ - I915_WRITE64_FW(fence_reg_lo, 0); - I915_READ64(fence_reg_lo); - - I915_WRITE64_FW(fence_reg_lo, val); - I915_READ64(fence_reg_lo); + if (intel_vgpu_active(dev_priv)) + I915_WRITE64_FW(fence_reg_lo, val); + else { + I915_WRITE64_FW(fence_reg_lo, 0); + I915_READ64(fence_reg_lo); + + I915_WRITE64_FW(fence_reg_lo, val); + I915_READ64(fence_reg_lo); + } } else { /* To w/a incoherency with non-atomic 64-bit register updates, * we split the 64-bit update into two 32-bit writes. In order -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 2:56 ` [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU Zhao Yakui @ 2018-07-03 8:51 ` Daniel Vetter 2018-07-03 9:05 ` Chris Wilson 2018-07-03 10:14 ` Zhao, Yakui 0 siblings, 2 replies; 19+ messages in thread From: Daniel Vetter @ 2018-07-03 8:51 UTC (permalink / raw) To: Zhao Yakui; +Cc: intel-gfx On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > On VGPU scenario the read/write operation of fence_reg will be trapped > by the GVT-g. And then gvt-g follows the HW spec to write the fence_reg. > So it is unnecessary to read/write fence reg several times. This will help > to reduce the unnecessary trap of fence_reg mmio operation. > > V1->V2: Fix one typo error of parameter when calling intel_vgpu_active > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> Ok this makes more sense. Except you need to put the 64bit entirely into the vpgu block, with a comment explaining why this is safe (since the vpgu will take care of updating fences correctly). -Daniel > --- > drivers/gpu/drm/i915/i915_gem_fence_reg.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > index d92fe03..9c97976 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > @@ -95,11 +95,15 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence, > > if (INTEL_GEN(fence->i915) >= 6) { > /* Use the 64-bit RW to read/write fence reg on SNB+ */ > - I915_WRITE64_FW(fence_reg_lo, 0); > - I915_READ64(fence_reg_lo); > - > - I915_WRITE64_FW(fence_reg_lo, val); > - I915_READ64(fence_reg_lo); > + if (intel_vgpu_active(dev_priv)) > + I915_WRITE64_FW(fence_reg_lo, val); > + else { > + I915_WRITE64_FW(fence_reg_lo, 0); > + I915_READ64(fence_reg_lo); > + > + I915_WRITE64_FW(fence_reg_lo, val); > + I915_READ64(fence_reg_lo); > + } > } else { > /* To w/a incoherency with non-atomic 64-bit register updates, > * we split the 64-bit update into two 32-bit writes. In order > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 8:51 ` Daniel Vetter @ 2018-07-03 9:05 ` Chris Wilson 2018-07-03 9:52 ` Daniel Vetter 2018-07-03 10:14 ` Zhao, Yakui 1 sibling, 1 reply; 19+ messages in thread From: Chris Wilson @ 2018-07-03 9:05 UTC (permalink / raw) To: Daniel Vetter, Zhao Yakui; +Cc: intel-gfx Quoting Daniel Vetter (2018-07-03 09:51:03) > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > > On VGPU scenario the read/write operation of fence_reg will be trapped > > by the GVT-g. And then gvt-g follows the HW spec to write the fence_reg. > > So it is unnecessary to read/write fence reg several times. This will help > > to reduce the unnecessary trap of fence_reg mmio operation. > > > > V1->V2: Fix one typo error of parameter when calling intel_vgpu_active > > > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > > Ok this makes more sense. Except you need to put the 64bit entirely into > the vpgu block, with a comment explaining why this is safe (since the vpgu > will take care of updating fences correctly). Except, who cares? Are fence registers being rewritten that frequently that special casing vgpu is worth the hassle. Part of that is that you need to leave a hint behind in the code that (a) explains why it is safe after having the "here be dragons" and (b) why we care. On a more pragmatic level if fencing doesn't plateau out to steady state, that is a worrying amount of contention -- the actual fence write itself would be the least of my worries. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 9:05 ` Chris Wilson @ 2018-07-03 9:52 ` Daniel Vetter 2018-07-03 12:47 ` Zhao, Yakui 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2018-07-03 9:52 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2018-07-03 09:51:03) > > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > > > On VGPU scenario the read/write operation of fence_reg will be trapped > > > by the GVT-g. And then gvt-g follows the HW spec to write the fence_reg. > > > So it is unnecessary to read/write fence reg several times. This will help > > > to reduce the unnecessary trap of fence_reg mmio operation. > > > > > > V1->V2: Fix one typo error of parameter when calling intel_vgpu_active > > > > > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > > > > Ok this makes more sense. Except you need to put the 64bit entirely into > > the vpgu block, with a comment explaining why this is safe (since the vpgu > > will take care of updating fences correctly). > > Except, who cares? Are fence registers being rewritten that frequently > that special casing vgpu is worth the hassle. Part of that is that you > need to leave a hint behind in the code that (a) explains why it is safe > after having the "here be dragons" and (b) why we care. > > On a more pragmatic level if fencing doesn't plateau out to steady > state, that is a worrying amount of contention -- the actual fence write > itself would be the least of my worries. I can easily imagine that with the few per-client fences vgpu hands out rewrites are much more common. But yeah some real data would be good. And more reasons to get mesa off of the gtt mmaps. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 9:52 ` Daniel Vetter @ 2018-07-03 12:47 ` Zhao, Yakui 2018-07-03 13:24 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Zhao, Yakui @ 2018-07-03 12:47 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Tuesday, July 3, 2018 5:52 PM >To: Chris Wilson <chris@chris-wilson.co.uk> >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui <yakui.zhao@intel.com>; >intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on >VGPU > >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: >> Quoting Daniel Vetter (2018-07-03 09:51:03) >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: >> > > On VGPU scenario the read/write operation of fence_reg will be >> > > trapped by the GVT-g. And then gvt-g follows the HW spec to write the >fence_reg. >> > > So it is unnecessary to read/write fence reg several times. This >> > > will help to reduce the unnecessary trap of fence_reg mmio operation. >> > > >> > > V1->V2: Fix one typo error of parameter when calling >> > > V1->intel_vgpu_active >> > > >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> >> > >> > Ok this makes more sense. Except you need to put the 64bit entirely >> > into the vpgu block, with a comment explaining why this is safe >> > (since the vpgu will take care of updating fences correctly). >> >> Except, who cares? Are fence registers being rewritten that frequently >> that special casing vgpu is worth the hassle. Part of that is that you >> need to leave a hint behind in the code that (a) explains why it is >> safe after having the "here be dragons" and (b) why we care. >> >> On a more pragmatic level if fencing doesn't plateau out to steady >> state, that is a worrying amount of contention -- the actual fence >> write itself would be the least of my worries. > >I can easily imagine that with the few per-client fences vgpu hands out >rewrites are much more common. But yeah some real data would be good. >And more reasons to get mesa off of the gtt mmaps. Hi, Daniel/Chris Thanks for your comments. The fence reg is used to assure the access of Tiled surface through aperature window. When fence is needed, the driver helps to find one available fence reg and then configure it. After it is not used, the fence will be turned off and then be allocated for next usage. It doesn't rely on the state of fence reg. In such case we don't need to worry about the unsteady state. For the VGPU operation: The op of fence reg is trapped. Then the gvt-g will follow the trapped value to program the fence_reg. (It will turn off and then write the expected value for any trapped write op of fence reg). The trapped op in GVT-g is safe. Based on the current logic, it needs the five traps when one fence reg is configured under VGPU mode.(Three writes, two reads). If it is programmed in one 64-bit op under VGPU mode, only one trap is needed. And the GVT-g still can configure the expected fence_value. As the trap is quite heavy for VGPU, the trap time can be saved. I will put some description in the code and commit log in next version. >-Daniel >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 12:47 ` Zhao, Yakui @ 2018-07-03 13:24 ` Chris Wilson 2018-07-03 13:58 ` Zhao, Yakui 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2018-07-03 13:24 UTC (permalink / raw) To: Zhao, Yakui, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org Quoting Zhao, Yakui (2018-07-03 13:47:46) > > >-----Original Message----- > >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > >Sent: Tuesday, July 3, 2018 5:52 PM > >To: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui <yakui.zhao@intel.com>; > >intel-gfx@lists.freedesktop.org > >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on > >VGPU > > > >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: > >> Quoting Daniel Vetter (2018-07-03 09:51:03) > >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > >> > > On VGPU scenario the read/write operation of fence_reg will be > >> > > trapped by the GVT-g. And then gvt-g follows the HW spec to write the > >fence_reg. > >> > > So it is unnecessary to read/write fence reg several times. This > >> > > will help to reduce the unnecessary trap of fence_reg mmio operation. > >> > > > >> > > V1->V2: Fix one typo error of parameter when calling > >> > > V1->intel_vgpu_active > >> > > > >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > >> > > >> > Ok this makes more sense. Except you need to put the 64bit entirely > >> > into the vpgu block, with a comment explaining why this is safe > >> > (since the vpgu will take care of updating fences correctly). > >> > >> Except, who cares? Are fence registers being rewritten that frequently > >> that special casing vgpu is worth the hassle. Part of that is that you > >> need to leave a hint behind in the code that (a) explains why it is > >> safe after having the "here be dragons" and (b) why we care. > >> > >> On a more pragmatic level if fencing doesn't plateau out to steady > >> state, that is a worrying amount of contention -- the actual fence > >> write itself would be the least of my worries. > > > >I can easily imagine that with the few per-client fences vgpu hands out > >rewrites are much more common. But yeah some real data would be good. > >And more reasons to get mesa off of the gtt mmaps. > > Hi, Daniel/Chris > > Thanks for your comments. > The fence reg is used to assure the access of Tiled surface through aperature window. When fence is needed, the driver > helps to find one available fence reg and then configure it. After it is not used, the fence will be turned off and then be allocated > for next usage. It doesn't rely on the state of fence reg. In such case we don't need to worry about the unsteady state. > > For the VGPU operation: The op of fence reg is trapped. Then the gvt-g will follow the trapped value to program the fence_reg. > (It will turn off and then write the expected value for any trapped write op of fence reg). The trapped op in GVT-g is safe. > > Based on the current logic, it needs the five traps when one fence reg is configured under VGPU mode.(Three writes, two reads). > If it is programmed in one 64-bit op under VGPU mode, only one trap is needed. And the GVT-g still can configure the expected fence_value. > As the trap is quite heavy for VGPU, the trap time can be saved. But the argument is can we avoid it entirely by never changing the fence. You say this is used for mapping through the aperture (GTT), we say userspace shouldn't be doing that for performance reasons :) A slow trap on top of a slow operation that is already causing contention seems more sensible to fix at source. (Albeit so long as the maintenance burden is considered and found to be reasonable, adding special cases with their rationale is acceptable.) So you have to sell why this mmio is worthy of special attention and curtail any future questions. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 13:24 ` Chris Wilson @ 2018-07-03 13:58 ` Zhao, Yakui 2018-07-03 14:07 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Zhao, Yakui @ 2018-07-03 13:58 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Tuesday, July 3, 2018 9:25 PM >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter <daniel@ffwll.ch> >Cc: intel-gfx@lists.freedesktop.org >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on >VGPU > >Quoting Zhao, Yakui (2018-07-03 13:47:46) >> >> >-----Original Message----- >> >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of >> >Daniel Vetter >> >Sent: Tuesday, July 3, 2018 5:52 PM >> >To: Chris Wilson <chris@chris-wilson.co.uk> >> >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui >> ><yakui.zhao@intel.com>; intel-gfx@lists.freedesktop.org >> >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg >> >only once on VGPU >> > >> >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: >> >> Quoting Daniel Vetter (2018-07-03 09:51:03) >> >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: >> >> > > On VGPU scenario the read/write operation of fence_reg will be >> >> > > trapped by the GVT-g. And then gvt-g follows the HW spec to >> >> > > write the >> >fence_reg. >> >> > > So it is unnecessary to read/write fence reg several times. >> >> > > This will help to reduce the unnecessary trap of fence_reg mmio >operation. >> >> > > >> >> > > V1->V2: Fix one typo error of parameter when calling >> >> > > V1->intel_vgpu_active >> >> > > >> >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> >> >> > >> >> > Ok this makes more sense. Except you need to put the 64bit >> >> > entirely into the vpgu block, with a comment explaining why this >> >> > is safe (since the vpgu will take care of updating fences correctly). >> >> >> >> Except, who cares? Are fence registers being rewritten that >> >> frequently that special casing vgpu is worth the hassle. Part of >> >> that is that you need to leave a hint behind in the code that (a) >> >> explains why it is safe after having the "here be dragons" and (b) why we >care. >> >> >> >> On a more pragmatic level if fencing doesn't plateau out to steady >> >> state, that is a worrying amount of contention -- the actual fence >> >> write itself would be the least of my worries. >> > >> >I can easily imagine that with the few per-client fences vgpu hands >> >out rewrites are much more common. But yeah some real data would be >good. >> >And more reasons to get mesa off of the gtt mmaps. >> >> Hi, Daniel/Chris >> >> Thanks for your comments. >> The fence reg is used to assure the access of Tiled surface >> through aperature window. When fence is needed, the driver helps to >> find one available fence reg and then configure it. After it is not used, the >fence will be turned off and then be allocated for next usage. It doesn't rely on >the state of fence reg. In such case we don't need to worry about the >unsteady state. >> >> For the VGPU operation: The op of fence reg is trapped. Then the gvt-g >will follow the trapped value to program the fence_reg. >> (It will turn off and then write the expected value for any trapped write op >of fence reg). The trapped op in GVT-g is safe. >> >> Based on the current logic, it needs the five traps when one fence reg is >configured under VGPU mode.(Three writes, two reads). >> If it is programmed in one 64-bit op under VGPU mode, only one trap is >needed. And the GVT-g still can configure the expected fence_value. >> As the trap is quite heavy for VGPU, the trap time can be saved. > >But the argument is can we avoid it entirely by never changing the fence. You >say this is used for mapping through the aperture (GTT), we say userspace >shouldn't be doing that for performance reasons :) A slow trap on top of a >slow operation that is already causing contention seems more sensible to fix >at source. (Albeit so long as the maintenance burden is considered and found >to be reasonable, adding special cases with their rationale is acceptable.) So >you have to sell why this mmio is worthy of special attention and curtail any >future questions. If the userspace driver/app can take care of the buffer allocation especially for the tiled surface, maybe it can reduce the ratio of changing the fence. But this can't be avoided if the tiled buffer is needed and allocated. This also depends on the userspace driver. And it is beyond the responsibility of the kernel driver. If it is configured in non-VGPU mode, the several writes of fence_reg doesn't matter. But under the VGPU mode, the trap of fence_reg will cause that it exits the mode of Virtual machine. After the trap emulation is finished, it can return to the guest OS and then resume the following sequence(For example: On KVMGT it will exit to the Linux host from the guest OS.). The exit of guest OS is quite costing. (For example: It needs to check the exit reason and check who/how to do the trap emulation). As it is mentioned in the previous email, the current sequence on VGPU needs five traps when one fence reg Is programmed.(Three writes, two reads). If only one trap is needed while it can configure the fence reg correctly, it can save the time of unnecessary traps for fence_reg programming. Of course it will help to improve the efficiency on the guest OS. >-Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 13:58 ` Zhao, Yakui @ 2018-07-03 14:07 ` Chris Wilson 2018-07-04 2:09 ` Zhao, Yakui 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2018-07-03 14:07 UTC (permalink / raw) To: Zhao, Yakui, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org Quoting Zhao, Yakui (2018-07-03 14:58:31) > >-----Original Message----- > >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > >Sent: Tuesday, July 3, 2018 9:25 PM > >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter <daniel@ffwll.ch> > >Cc: intel-gfx@lists.freedesktop.org > >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on > >VGPU > > > >Quoting Zhao, Yakui (2018-07-03 13:47:46) > >> > >> >-----Original Message----- > >> >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of > >> >Daniel Vetter > >> >Sent: Tuesday, July 3, 2018 5:52 PM > >> >To: Chris Wilson <chris@chris-wilson.co.uk> > >> >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui > >> ><yakui.zhao@intel.com>; intel-gfx@lists.freedesktop.org > >> >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg > >> >only once on VGPU > >> > > >> >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: > >> >> Quoting Daniel Vetter (2018-07-03 09:51:03) > >> >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > >> >> > > On VGPU scenario the read/write operation of fence_reg will be > >> >> > > trapped by the GVT-g. And then gvt-g follows the HW spec to > >> >> > > write the > >> >fence_reg. > >> >> > > So it is unnecessary to read/write fence reg several times. > >> >> > > This will help to reduce the unnecessary trap of fence_reg mmio > >operation. > >> >> > > > >> >> > > V1->V2: Fix one typo error of parameter when calling > >> >> > > V1->intel_vgpu_active > >> >> > > > >> >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > >> >> > > >> >> > Ok this makes more sense. Except you need to put the 64bit > >> >> > entirely into the vpgu block, with a comment explaining why this > >> >> > is safe (since the vpgu will take care of updating fences correctly). > >> >> > >> >> Except, who cares? Are fence registers being rewritten that > >> >> frequently that special casing vgpu is worth the hassle. Part of > >> >> that is that you need to leave a hint behind in the code that (a) > >> >> explains why it is safe after having the "here be dragons" and (b) why we > >care. > >> >> > >> >> On a more pragmatic level if fencing doesn't plateau out to steady > >> >> state, that is a worrying amount of contention -- the actual fence > >> >> write itself would be the least of my worries. > >> > > >> >I can easily imagine that with the few per-client fences vgpu hands > >> >out rewrites are much more common. But yeah some real data would be > >good. > >> >And more reasons to get mesa off of the gtt mmaps. > >> > >> Hi, Daniel/Chris > >> > >> Thanks for your comments. > >> The fence reg is used to assure the access of Tiled surface > >> through aperature window. When fence is needed, the driver helps to > >> find one available fence reg and then configure it. After it is not used, the > >fence will be turned off and then be allocated for next usage. It doesn't rely on > >the state of fence reg. In such case we don't need to worry about the > >unsteady state. > >> > >> For the VGPU operation: The op of fence reg is trapped. Then the gvt-g > >will follow the trapped value to program the fence_reg. > >> (It will turn off and then write the expected value for any trapped write op > >of fence reg). The trapped op in GVT-g is safe. > >> > >> Based on the current logic, it needs the five traps when one fence reg is > >configured under VGPU mode.(Three writes, two reads). > >> If it is programmed in one 64-bit op under VGPU mode, only one trap is > >needed. And the GVT-g still can configure the expected fence_value. > >> As the trap is quite heavy for VGPU, the trap time can be saved. > > > >But the argument is can we avoid it entirely by never changing the fence. You > >say this is used for mapping through the aperture (GTT), we say userspace > >shouldn't be doing that for performance reasons :) A slow trap on top of a > >slow operation that is already causing contention seems more sensible to fix > >at source. (Albeit so long as the maintenance burden is considered and found > >to be reasonable, adding special cases with their rationale is acceptable.) So > >you have to sell why this mmio is worthy of special attention and curtail any > >future questions. > > If the userspace driver/app can take care of the buffer allocation especially for the tiled > surface, maybe it can reduce the ratio of changing the fence. But this can't be avoided if the tiled > buffer is needed and allocated. This also depends on the userspace driver. And it is beyond the > responsibility of the kernel driver. We own the stack. If userspace is not behaving as well as it might, we need to let them know. Improving algorithms is several orders more beneficial than micro-optimising the implementation. (Speaking as one who sadly does do much of the latter.) But you are asking us to maintain this path with neither any CI coverage nor any indication of the userspace impact. > If it is configured in non-VGPU mode, the several writes of fence_reg doesn't matter. But under > the VGPU mode, the trap of fence_reg will cause that it exits the mode of Virtual machine. After the trap > emulation is finished, it can return to the guest OS and then resume the following sequence(For > example: On KVMGT it will exit to the Linux host from the guest OS.). The exit of guest OS is quite > costing. (For example: It needs to check the exit reason and check who/how to do the trap emulation). > As it is mentioned in the previous email, the current sequence on VGPU needs five traps when one fence reg > Is programmed.(Three writes, two reads). If only one trap is needed while it can configure the fence > reg correctly, it can save the time of unnecessary traps for fence_reg programming. Of course it will > help to improve the efficiency on the guest OS. Numbers help, we understand why it is slow to do several traps versus one, what we don't know is what impact that actually has to userspace, and whether it is worth optimising this versus every other register. What is the before after of something like gem_fence_upload? -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 14:07 ` Chris Wilson @ 2018-07-04 2:09 ` Zhao, Yakui 2018-07-04 9:40 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Zhao, Yakui @ 2018-07-04 2:09 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Tuesday, July 3, 2018 10:08 PM >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter <daniel@ffwll.ch> >Cc: intel-gfx@lists.freedesktop.org >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on >VGPU > >Quoting Zhao, Yakui (2018-07-03 14:58:31) >> >-----Original Message----- >> >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >> >Sent: Tuesday, July 3, 2018 9:25 PM >> >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter >> ><daniel@ffwll.ch> >> >Cc: intel-gfx@lists.freedesktop.org >> >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg >> >only once on VGPU >> > >> >Quoting Zhao, Yakui (2018-07-03 13:47:46) >> >> >> >> >-----Original Message----- >> >> >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of >> >> >Daniel Vetter >> >> >Sent: Tuesday, July 3, 2018 5:52 PM >> >> >To: Chris Wilson <chris@chris-wilson.co.uk> >> >> >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui >> >> ><yakui.zhao@intel.com>; intel-gfx@lists.freedesktop.org >> >> >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg >> >> >only once on VGPU >> >> > >> >> >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: >> >> >> Quoting Daniel Vetter (2018-07-03 09:51:03) >> >> >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: >> >> >> > > On VGPU scenario the read/write operation of fence_reg will >> >> >> > > be trapped by the GVT-g. And then gvt-g follows the HW spec >> >> >> > > to write the >> >> >fence_reg. >> >> >> > > So it is unnecessary to read/write fence reg several times. >> >> >> > > This will help to reduce the unnecessary trap of fence_reg >> >> >> > > mmio >> >operation. >> >> >> > > >> >> >> > > V1->V2: Fix one typo error of parameter when calling >> >> >> > > V1->intel_vgpu_active >> >> >> > > >> >> >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> >> >> >> > >> >> >> > Ok this makes more sense. Except you need to put the 64bit >> >> >> > entirely into the vpgu block, with a comment explaining why >> >> >> > this is safe (since the vpgu will take care of updating fences correctly). >> >> >> >> >> >> Except, who cares? Are fence registers being rewritten that >> >> >> frequently that special casing vgpu is worth the hassle. Part of >> >> >> that is that you need to leave a hint behind in the code that >> >> >> (a) explains why it is safe after having the "here be dragons" >> >> >> and (b) why we >> >care. >> >> >> >> >> >> On a more pragmatic level if fencing doesn't plateau out to >> >> >> steady state, that is a worrying amount of contention -- the >> >> >> actual fence write itself would be the least of my worries. >> >> > >> >> >I can easily imagine that with the few per-client fences vgpu >> >> >hands out rewrites are much more common. But yeah some real data >> >> >would be >> >good. >> >> >And more reasons to get mesa off of the gtt mmaps. >> >> >> >> Hi, Daniel/Chris >> >> >> >> Thanks for your comments. >> >> The fence reg is used to assure the access of Tiled surface >> >> through aperature window. When fence is needed, the driver helps to >> >> find one available fence reg and then configure it. After it is not >> >> used, the >> >fence will be turned off and then be allocated for next usage. It >> >doesn't rely on the state of fence reg. In such case we don't need >> >to worry about the unsteady state. >> >> >> >> For the VGPU operation: The op of fence reg is trapped. Then >> >> the gvt-g >> >will follow the trapped value to program the fence_reg. >> >> (It will turn off and then write the expected value for any trapped >> >> write op >> >of fence reg). The trapped op in GVT-g is safe. >> >> >> >> Based on the current logic, it needs the five traps when one >> >> fence reg is >> >configured under VGPU mode.(Three writes, two reads). >> >> If it is programmed in one 64-bit op under VGPU mode, only one trap >> >> is >> >needed. And the GVT-g still can configure the expected fence_value. >> >> As the trap is quite heavy for VGPU, the trap time can be saved. >> > >> >But the argument is can we avoid it entirely by never changing the >> >fence. You say this is used for mapping through the aperture (GTT), >> >we say userspace shouldn't be doing that for performance reasons :) A >> >slow trap on top of a slow operation that is already causing >> >contention seems more sensible to fix at source. (Albeit so long as >> >the maintenance burden is considered and found to be reasonable, >> >adding special cases with their rationale is acceptable.) So you have >> >to sell why this mmio is worthy of special attention and curtail any future >questions. >> >> If the userspace driver/app can take care of the buffer allocation >> especially for the tiled surface, maybe it can reduce the ratio of >> changing the fence. But this can't be avoided if the tiled buffer is >> needed and allocated. This also depends on the userspace driver. And it is >beyond the responsibility of the kernel driver. > >We own the stack. If userspace is not behaving as well as it might, we need to >let them know. Improving algorithms is several orders more beneficial than >micro-optimising the implementation. (Speaking as one who sadly does do >much of the latter.) But you are asking us to maintain this path with neither >any CI coverage nor any indication of the userspace impact. > >> If it is configured in non-VGPU mode, the several writes of fence_reg >> doesn't matter. But under the VGPU mode, the trap of fence_reg will >> cause that it exits the mode of Virtual machine. After the trap >> emulation is finished, it can return to the guest OS and then resume >> the following sequence(For >> example: On KVMGT it will exit to the Linux host from the guest OS.). >> The exit of guest OS is quite costing. (For example: It needs to check the exit >reason and check who/how to do the trap emulation). >> As it is mentioned in the previous email, the current sequence on VGPU >> needs five traps when one fence reg Is programmed.(Three writes, two >> reads). If only one trap is needed while it can configure the fence >> reg correctly, it can save the time of unnecessary traps for fence_reg >programming. Of course it will help to improve the efficiency on the guest OS. > >Numbers help, we understand why it is slow to do several traps versus one, >what we don't know is what impact that actually has to userspace, and >whether it is worth optimising this versus every other register. > Hi, Chris The userspace driver/library on guest OS has no sense that it is running on the virtual machine. In such case the trap of fence_reg on VGPU is transparent to the userspace driver/library. Only when trying to run the performance test, the performance will be affected by so many traps. If the trap of other register can also be optimized, it will help the performance of guest OS. Of course the Prerequisite is that we collect the corresponding trap info when running the corresponding workload. This is case by case. >What is the before after of something like gem_fence_upload? I check the case of gem_fence_upload. It only checks the uploading performance after fence is already programmed. So this patch has no impact on gem_fence_upload. >-Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-04 2:09 ` Zhao, Yakui @ 2018-07-04 9:40 ` Chris Wilson 0 siblings, 0 replies; 19+ messages in thread From: Chris Wilson @ 2018-07-04 9:40 UTC (permalink / raw) To: Zhao, Yakui, Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org Quoting Zhao, Yakui (2018-07-04 03:09:03) > >-----Original Message----- > >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > >Sent: Tuesday, July 3, 2018 10:08 PM > >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter <daniel@ffwll.ch> > >Cc: intel-gfx@lists.freedesktop.org > >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on > >VGPU > > > >Quoting Zhao, Yakui (2018-07-03 14:58:31) > >> >-----Original Message----- > >> >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > >> >Sent: Tuesday, July 3, 2018 9:25 PM > >> >To: Zhao, Yakui <yakui.zhao@intel.com>; Daniel Vetter > >> ><daniel@ffwll.ch> > >> >Cc: intel-gfx@lists.freedesktop.org > >> >Subject: RE: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg > >> >only once on VGPU > >> > > >> >Quoting Zhao, Yakui (2018-07-03 13:47:46) > >> >> > >> >> >-----Original Message----- > >> >> >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of > >> >> >Daniel Vetter > >> >> >Sent: Tuesday, July 3, 2018 5:52 PM > >> >> >To: Chris Wilson <chris@chris-wilson.co.uk> > >> >> >Cc: Daniel Vetter <daniel@ffwll.ch>; Zhao, Yakui > >> >> ><yakui.zhao@intel.com>; intel-gfx@lists.freedesktop.org > >> >> >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg > >> >> >only once on VGPU > >> >> > > >> >> >On Tue, Jul 03, 2018 at 10:05:28AM +0100, Chris Wilson wrote: > >> >> >> Quoting Daniel Vetter (2018-07-03 09:51:03) > >> >> >> > On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: > >> >> >> > > On VGPU scenario the read/write operation of fence_reg will > >> >> >> > > be trapped by the GVT-g. And then gvt-g follows the HW spec > >> >> >> > > to write the > >> >> >fence_reg. > >> >> >> > > So it is unnecessary to read/write fence reg several times. > >> >> >> > > This will help to reduce the unnecessary trap of fence_reg > >> >> >> > > mmio > >> >operation. > >> >> >> > > > >> >> >> > > V1->V2: Fix one typo error of parameter when calling > >> >> >> > > V1->intel_vgpu_active > >> >> >> > > > >> >> >> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > >> >> >> > > >> >> >> > Ok this makes more sense. Except you need to put the 64bit > >> >> >> > entirely into the vpgu block, with a comment explaining why > >> >> >> > this is safe (since the vpgu will take care of updating fences correctly). > >> >> >> > >> >> >> Except, who cares? Are fence registers being rewritten that > >> >> >> frequently that special casing vgpu is worth the hassle. Part of > >> >> >> that is that you need to leave a hint behind in the code that > >> >> >> (a) explains why it is safe after having the "here be dragons" > >> >> >> and (b) why we > >> >care. > >> >> >> > >> >> >> On a more pragmatic level if fencing doesn't plateau out to > >> >> >> steady state, that is a worrying amount of contention -- the > >> >> >> actual fence write itself would be the least of my worries. > >> >> > > >> >> >I can easily imagine that with the few per-client fences vgpu > >> >> >hands out rewrites are much more common. But yeah some real data > >> >> >would be > >> >good. > >> >> >And more reasons to get mesa off of the gtt mmaps. > >> >> > >> >> Hi, Daniel/Chris > >> >> > >> >> Thanks for your comments. > >> >> The fence reg is used to assure the access of Tiled surface > >> >> through aperature window. When fence is needed, the driver helps to > >> >> find one available fence reg and then configure it. After it is not > >> >> used, the > >> >fence will be turned off and then be allocated for next usage. It > >> >doesn't rely on the state of fence reg. In such case we don't need > >> >to worry about the unsteady state. > >> >> > >> >> For the VGPU operation: The op of fence reg is trapped. Then > >> >> the gvt-g > >> >will follow the trapped value to program the fence_reg. > >> >> (It will turn off and then write the expected value for any trapped > >> >> write op > >> >of fence reg). The trapped op in GVT-g is safe. > >> >> > >> >> Based on the current logic, it needs the five traps when one > >> >> fence reg is > >> >configured under VGPU mode.(Three writes, two reads). > >> >> If it is programmed in one 64-bit op under VGPU mode, only one trap > >> >> is > >> >needed. And the GVT-g still can configure the expected fence_value. > >> >> As the trap is quite heavy for VGPU, the trap time can be saved. > >> > > >> >But the argument is can we avoid it entirely by never changing the > >> >fence. You say this is used for mapping through the aperture (GTT), > >> >we say userspace shouldn't be doing that for performance reasons :) A > >> >slow trap on top of a slow operation that is already causing > >> >contention seems more sensible to fix at source. (Albeit so long as > >> >the maintenance burden is considered and found to be reasonable, > >> >adding special cases with their rationale is acceptable.) So you have > >> >to sell why this mmio is worthy of special attention and curtail any future > >questions. > >> > >> If the userspace driver/app can take care of the buffer allocation > >> especially for the tiled surface, maybe it can reduce the ratio of > >> changing the fence. But this can't be avoided if the tiled buffer is > >> needed and allocated. This also depends on the userspace driver. And it is > >beyond the responsibility of the kernel driver. > > > >We own the stack. If userspace is not behaving as well as it might, we need to > >let them know. Improving algorithms is several orders more beneficial than > >micro-optimising the implementation. (Speaking as one who sadly does do > >much of the latter.) But you are asking us to maintain this path with neither > >any CI coverage nor any indication of the userspace impact. > > > >> If it is configured in non-VGPU mode, the several writes of fence_reg > >> doesn't matter. But under the VGPU mode, the trap of fence_reg will > >> cause that it exits the mode of Virtual machine. After the trap > >> emulation is finished, it can return to the guest OS and then resume > >> the following sequence(For > >> example: On KVMGT it will exit to the Linux host from the guest OS.). > >> The exit of guest OS is quite costing. (For example: It needs to check the exit > >reason and check who/how to do the trap emulation). > >> As it is mentioned in the previous email, the current sequence on VGPU > >> needs five traps when one fence reg Is programmed.(Three writes, two > >> reads). If only one trap is needed while it can configure the fence > >> reg correctly, it can save the time of unnecessary traps for fence_reg > >programming. Of course it will help to improve the efficiency on the guest OS. > > > >Numbers help, we understand why it is slow to do several traps versus one, > >what we don't know is what impact that actually has to userspace, and > >whether it is worth optimising this versus every other register. > > > Hi, Chris > > The userspace driver/library on guest OS has no sense that it is running on the virtual machine. And? Fence thrashing is bad irrespective of virtualisation or not. Improving algorithms to avoid fencing should be effective whether or not userspace is running on a mediated vgpu, and avoiding everything in the path leading to rewriting fences (struct_mutex!) so much effective than improving the traps. It is not that one precludes the other, it's just the micro-optimisations still need to be justified, or they'll be forgotten and removed. > In such case the trap of fence_reg on VGPU is transparent to the userspace driver/library. > Only when trying to run the performance test, the performance will be affected by so many traps. But how much so? > If the trap of other register can also be optimized, it will help the performance of guest OS. Of course the > Prerequisite is that we collect the corresponding trap info when running the corresponding workload. > This is case by case. > > >What is the before after of something like gem_fence_upload? > > I check the case of gem_fence_upload. It only checks the uploading performance after fence is already programmed. > So this patch has no impact on gem_fence_upload. Incorrect. By the same token, you haven't measured yourself the impact of this change? -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU 2018-07-03 8:51 ` Daniel Vetter 2018-07-03 9:05 ` Chris Wilson @ 2018-07-03 10:14 ` Zhao, Yakui 1 sibling, 0 replies; 19+ messages in thread From: Zhao, Yakui @ 2018-07-03 10:14 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Tuesday, July 3, 2018 4:51 PM >To: Zhao, Yakui <yakui.zhao@intel.com> >Cc: intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: write fence reg only once on >VGPU > >On Tue, Jul 03, 2018 at 10:56:17AM +0800, Zhao Yakui wrote: >> On VGPU scenario the read/write operation of fence_reg will be trapped >> by the GVT-g. And then gvt-g follows the HW spec to write the fence_reg. >> So it is unnecessary to read/write fence reg several times. This will >> help to reduce the unnecessary trap of fence_reg mmio operation. >> >> V1->V2: Fix one typo error of parameter when calling intel_vgpu_active >> >> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > >Ok this makes more sense. Except you need to put the 64bit entirely into the >vpgu block, with a comment explaining why this is safe (since the vpgu will >take care of updating fences correctly). Thanks for your comment and reply. The gvt-g will take care how to update the fence reg. I will refine the comments and commit log. >-Daniel > >> --- >> drivers/gpu/drm/i915/i915_gem_fence_reg.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c >> b/drivers/gpu/drm/i915/i915_gem_fence_reg.c >> index d92fe03..9c97976 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c >> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c >> @@ -95,11 +95,15 @@ static void i965_write_fence_reg(struct >> drm_i915_fence_reg *fence, >> >> if (INTEL_GEN(fence->i915) >= 6) { >> /* Use the 64-bit RW to read/write fence reg on SNB+ */ >> - I915_WRITE64_FW(fence_reg_lo, 0); >> - I915_READ64(fence_reg_lo); >> - >> - I915_WRITE64_FW(fence_reg_lo, val); >> - I915_READ64(fence_reg_lo); >> + if (intel_vgpu_active(dev_priv)) >> + I915_WRITE64_FW(fence_reg_lo, val); >> + else { >> + I915_WRITE64_FW(fence_reg_lo, 0); >> + I915_READ64(fence_reg_lo); >> + >> + I915_WRITE64_FW(fence_reg_lo, val); >> + I915_READ64(fence_reg_lo); >> + } >> } else { >> /* To w/a incoherency with non-atomic 64-bit register updates, >> * we split the 64-bit update into two 32-bit writes. In order >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Optimize the read/write fence_reg on SNB+ 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui 2018-07-03 2:56 ` [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg " Zhao Yakui 2018-07-03 2:56 ` [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU Zhao Yakui @ 2018-07-03 3:13 ` Patchwork 2018-07-03 3:30 ` ✓ Fi.CI.BAT: success " Patchwork 2018-07-03 4:22 ` ✓ Fi.CI.IGT: " Patchwork 4 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2018-07-03 3:13 UTC (permalink / raw) To: Zhao Yakui; +Cc: intel-gfx == Series Details == Series: drm/i915: Optimize the read/write fence_reg on SNB+ URL : https://patchwork.freedesktop.org/series/45801/ State : warning == Summary == $ dim checkpatch origin/drm-tip 51157a4ce265 drm/i915: Use 64-bit to Read/Write fence reg on SNB+ e8b6a2596672 drm/i915: write fence reg only once on VGPU -:30: CHECK:BRACES: Unbalanced braces around else statement #30: FILE: drivers/gpu/drm/i915/i915_gem_fence_reg.c:100: + else { total: 0 errors, 0 warnings, 1 checks, 20 lines checked _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Optimize the read/write fence_reg on SNB+ 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui ` (2 preceding siblings ...) 2018-07-03 3:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Optimize the read/write fence_reg on SNB+ Patchwork @ 2018-07-03 3:30 ` Patchwork 2018-07-03 4:22 ` ✓ Fi.CI.IGT: " Patchwork 4 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2018-07-03 3:30 UTC (permalink / raw) To: Zhao Yakui; +Cc: intel-gfx == Series Details == Series: drm/i915: Optimize the read/write fence_reg on SNB+ URL : https://patchwork.freedesktop.org/series/45801/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4417 -> Patchwork_9503 = == Summary - WARNING == Minor unknown changes coming with Patchwork_9503 need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_9503, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://patchwork.freedesktop.org/api/1.0/series/45801/revisions/1/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in Patchwork_9503: === IGT changes === ==== Warnings ==== igt@gem_exec_gttfill@basic: fi-pnv-d510: PASS -> SKIP == Known issues == Here are the changes found in Patchwork_9503 that come from known issues: === IGT changes === ==== Possible fixes ==== igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: fi-bxt-dsi: INCOMPLETE (fdo#103927) -> PASS fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 == Participating hosts (45 -> 40) == Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u == Build changes == * Linux: CI_DRM_4417 -> Patchwork_9503 CI_DRM_4417: e781af726c3ad36c18cc21203189459dd2cbdd43 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4532: 840d12e2f050b784552197403d6575a57b6e896d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_9503: e8b6a25966721c297a1d15b6c0749775bcc086cb @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == e8b6a2596672 drm/i915: write fence reg only once on VGPU 51157a4ce265 drm/i915: Use 64-bit to Read/Write fence reg on SNB+ == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9503/issues.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Optimize the read/write fence_reg on SNB+ 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui ` (3 preceding siblings ...) 2018-07-03 3:30 ` ✓ Fi.CI.BAT: success " Patchwork @ 2018-07-03 4:22 ` Patchwork 4 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2018-07-03 4:22 UTC (permalink / raw) To: Zhao Yakui; +Cc: intel-gfx == Series Details == Series: drm/i915: Optimize the read/write fence_reg on SNB+ URL : https://patchwork.freedesktop.org/series/45801/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4417_full -> Patchwork_9503_full = == Summary - WARNING == Minor unknown changes coming with Patchwork_9503_full need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_9503_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. == Possible new issues == Here are the unknown changes that may have been introduced in Patchwork_9503_full: === IGT changes === ==== Warnings ==== igt@gem_exec_schedule@deep-bsd2: shard-kbl: PASS -> SKIP igt@kms_cursor_legacy@cursor-vs-flip-legacy: shard-snb: PASS -> SKIP == Known issues == Here are the changes found in Patchwork_9503_full that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_selftest@live_gtt: shard-glk: PASS -> INCOMPLETE (fdo#107094) igt@drv_selftest@mock_scatterlist: shard-kbl: NOTRUN -> DMESG-WARN (fdo#103667) igt@kms_flip@plain-flip-fb-recreate-interruptible: shard-glk: PASS -> FAIL (fdo#100368) +2 igt@kms_flip_tiling@flip-x-tiled: shard-glk: PASS -> FAIL (fdo#103822, fdo#104724) igt@kms_setmode@basic: shard-apl: PASS -> FAIL (fdo#99912) shard-hsw: PASS -> FAIL (fdo#99912) ==== Possible fixes ==== igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing: shard-glk: FAIL (fdo#105703) -> PASS igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic: shard-glk: FAIL (fdo#105454, fdo#106509) -> PASS igt@kms_flip@2x-flip-vs-expired-vblank: shard-glk: FAIL (fdo#102887) -> PASS igt@kms_flip@2x-modeset-vs-vblank-race-interruptible: shard-hsw: FAIL (fdo#103060) -> PASS igt@kms_flip_tiling@flip-to-x-tiled: shard-glk: FAIL (fdo#103822, fdo#104724) -> PASS fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887 fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060 fdo#103667 https://bugs.freedesktop.org/show_bug.cgi?id=103667 fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822 fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724 fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454 fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703 fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509 fdo#107094 https://bugs.freedesktop.org/show_bug.cgi?id=107094 fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 == Participating hosts (5 -> 5) == No changes in participating hosts == Build changes == * Linux: CI_DRM_4417 -> Patchwork_9503 CI_DRM_4417: e781af726c3ad36c18cc21203189459dd2cbdd43 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4532: 840d12e2f050b784552197403d6575a57b6e896d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_9503: e8b6a25966721c297a1d15b6c0749775bcc086cb @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9503/shards.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-07-04 9:40 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-03 2:56 [PATCH v2 0/2] drm/i915: Optimize the read/write fence_reg on SNB+ Zhao Yakui 2018-07-03 2:56 ` [PATCH v2 1/2] drm/i915: Use 64-bit to Read/Write fence reg " Zhao Yakui 2018-07-03 8:49 ` Daniel Vetter 2018-07-03 9:01 ` Chris Wilson 2018-07-03 10:11 ` Zhao, Yakui 2018-07-03 2:56 ` [PATCH v2 2/2] drm/i915: write fence reg only once on VGPU Zhao Yakui 2018-07-03 8:51 ` Daniel Vetter 2018-07-03 9:05 ` Chris Wilson 2018-07-03 9:52 ` Daniel Vetter 2018-07-03 12:47 ` Zhao, Yakui 2018-07-03 13:24 ` Chris Wilson 2018-07-03 13:58 ` Zhao, Yakui 2018-07-03 14:07 ` Chris Wilson 2018-07-04 2:09 ` Zhao, Yakui 2018-07-04 9:40 ` Chris Wilson 2018-07-03 10:14 ` Zhao, Yakui 2018-07-03 3:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Optimize the read/write fence_reg on SNB+ Patchwork 2018-07-03 3:30 ` ✓ Fi.CI.BAT: success " Patchwork 2018-07-03 4:22 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.