| Message ID | 1494497262-24855-1-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > initialised is fixup by the GVT shadow context as true to avoid the init > from the host because it cannot take the settings from the host. Add a > check to let host driver only overwrite it when the init callback is NULL During execlist_context_deferred_alloc() we presumed that the context is uninitialised (we only just allocated the state object for it!) and chose to optimise away the later call to engine->init_context() if engine->init_context were NULL. This breaks with GVT's contexts that are marked as pre-initialised to avoid us annoyingly calling engine->init_context(). The fix is to not override ce->initialised if it is already true. > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 319d9a8..d0e9b61 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1956,7 +1956,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx, > > ce->ring = ring; > ce->state = vma; > - ce->initialised = engine->init_context == NULL; > + if (!engine->init_context) > + ce->initialised = true; Does the compiler generate a cmov? -Chris
On Fri, May 12, 2017 at 10:49:24AM +0100, Chris Wilson wrote: > On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > > initialised is fixup by the GVT shadow context as true to avoid the init > > from the host because it cannot take the settings from the host. Add a > > check to let host driver only overwrite it when the init callback is NULL > > During execlist_context_deferred_alloc() we presumed that the context is > uninitialised (we only just allocated the state object for it!) and > chose to optimise away the later call to engine->init_context() if > engine->init_context were NULL. This breaks with GVT's contexts that are > marked as pre-initialised to avoid us annoyingly calling > engine->init_context(). The fix is to not override ce->initialised if it > is already true. > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 319d9a8..d0e9b61 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1956,7 +1956,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx, > > > > ce->ring = ring; > > ce->state = vma; > > - ce->initialised = engine->init_context == NULL; > > + if (!engine->init_context) > > + ce->initialised = true; > > Does the compiler generate a cmov? Test and jump, may as well just use ce->initialised |= init_context == NULL; -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Friday, May 12, 2017 6:11 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel- > gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when > init_context callback is NULL > > On Fri, May 12, 2017 at 10:49:24AM +0100, Chris Wilson wrote: > > On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > > > initialised is fixup by the GVT shadow context as true to avoid the > > > init from the host because it cannot take the settings from the > > > host. Add a check to let host driver only overwrite it when the init > > > callback is NULL > > > > During execlist_context_deferred_alloc() we presumed that the context > > is uninitialised (we only just allocated the state object for it!) and > > chose to optimise away the later call to engine->init_context() if > > engine->init_context were NULL. This breaks with GVT's contexts that > > engine->are > > marked as pre-initialised to avoid us annoyingly calling > > engine->init_context(). The fix is to not override ce->initialised if > > engine->it > > is already true. Thanks Chris, will use this as commit message > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > > > b/drivers/gpu/drm/i915/intel_lrc.c > > > index 319d9a8..d0e9b61 100644 > > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > @@ -1956,7 +1956,8 @@ static int > > > execlists_context_deferred_alloc(struct i915_gem_context *ctx, > > > > > > ce->ring = ring; > > > ce->state = vma; > > > - ce->initialised = engine->init_context == NULL; > > > + if (!engine->init_context) > > > + ce->initialised = true; > > > > Does the compiler generate a cmov? Just get the assembly code of this change, seems no cmov. if (!engine->init_context) ffffffff8156b6f3: 49 83 bf c0 01 00 00 cmpq $0x0,0x1c0(%r15) ffffffff8156b6fa: 00 ffffffff8156b6fb: 0f 84 e5 01 00 00 je ffffffff8156b8e6 <execlists_context_pin+0x486> ......... ce->initialised = true; ffffffff8156b8e6: 4d 6b ed 28 imul $0x28,%r13,%r13 ffffffff8156b8ea: 43 c6 44 2e 7c 01 movb $0x1,0x7c(%r14,%r13,1) ffffffff8156b8f0: e9 0c fe ff ff jmpq ffffffff8156b701 <execlists_context_pin+0x2a1> > > Test and jump, may as well just use ce->initialised |= init_context == NULL; - > Chris This is the new assembly code for this new change: ce->initialised |= engine->init_context == NULL; ffffffff8156b6f3: 49 83 bf c0 01 00 00 cmpq $0x0,0x1c0(%r15) ffffffff8156b6fa: 00 ffffffff8156b6fb: 0f 94 c2 sete %dl ffffffff8156b6fe: 08 50 7c or %dl,0x7c(%rax) looks your change is better because the compiler doesn't generate a jump. :) Will send out v2 with your idea. Thanks Chuanxiao
On Fri, May 12, 2017 at 11:34:21AM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: set initialised only when init_context callback is NULL (rev2) > URL : https://patchwork.freedesktop.org/series/24286/ > State : success > > == Summary == > > Series 24286v2 drm/i915: set initialised only when init_context callback is NULL > https://patchwork.freedesktop.org/api/1.0/series/24286/revisions/2/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s4-devices: > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 Thanks for the patch and pushed. -Chris
On 2017.05.12 12:44:48 +0100, Chris Wilson wrote: > On Fri, May 12, 2017 at 11:34:21AM -0000, Patchwork wrote: > > == Series Details == > > > > Series: drm/i915: set initialised only when init_context callback is NULL (rev2) > > URL : https://patchwork.freedesktop.org/series/24286/ > > State : success > > > > == Summary == > > > > Series 24286v2 drm/i915: set initialised only when init_context callback is NULL > > https://patchwork.freedesktop.org/api/1.0/series/24286/revisions/2/mbox/ > > > > Test gem_exec_suspend: > > Subgroup basic-s4-devices: > > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 > > Thanks for the patch and pushed. Could this be picked to -fixes? It should be in 4.12 and apply for -stable kernel too I think.
On Fri, 19 May 2017, Zhenyu Wang <zhenyuw@linux.intel.com> wrote: > On 2017.05.12 12:44:48 +0100, Chris Wilson wrote: >> On Fri, May 12, 2017 at 11:34:21AM -0000, Patchwork wrote: >> > == Series Details == >> > >> > Series: drm/i915: set initialised only when init_context callback is NULL (rev2) >> > URL : https://patchwork.freedesktop.org/series/24286/ >> > State : success >> > >> > == Summary == >> > >> > Series 24286v2 drm/i915: set initialised only when init_context callback is NULL >> > https://patchwork.freedesktop.org/api/1.0/series/24286/revisions/2/mbox/ >> > >> > Test gem_exec_suspend: >> > Subgroup basic-s4-devices: >> > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 >> >> Thanks for the patch and pushed. > > Could this be picked to -fixes? It should be in 4.12 and apply for > -stable kernel too I think. Argh, you asked this to be picked up on irc, so I did. But you only mentioned 4.12 and nothing about stable backports, so it doesn't have cc: stable now. Is this needed for stable?! BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 319d9a8..d0e9b61 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1956,7 +1956,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx, ce->ring = ring; ce->state = vma; - ce->initialised = engine->init_context == NULL; + if (!engine->init_context) + ce->initialised = true; return 0;
initialised is fixup by the GVT shadow context as true to avoid the init from the host because it cannot take the settings from the host. Add a check to let host driver only overwrite it when the init callback is NULL Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)